Proszę o ocenę czytelności kodu

Nie kompiluje się bo każda funkcja, która nie zwraca void musi zawsze coś zwrócić, a kompilator nie wie, że else musi się wykonać, jeśli warunek będzie niespełniony. Jeśli nie chcesz mieć tej dodatkowej linijki to napisz:

public static String myFunc() {
  if (/* jakiś warunek/*) {
    // jakiś tam kod w którym jest return
  }
  return "Error: The word is null!";
}

Choć w tym przypadku powinieneś rypnąć wyjątkiem lub zwrócić null.

Ok, zrozumiałem, dzięki :) A jak jest z czytelnością?

jak na mnie brzydko sformatowany kod i za długie nazwy zmiennych :P

Ja bym to napisał bez else-ów(jeden poziom zagnieżdżenia mniej).

Ja bym zapisał to tak, wg mnie jest czytelniej:

 
        public static String swapEveryTwoLetters(final String wordIn) {
                
		if (wordIn == null)
			return "Error: The word is null!";
		if (wordIn.length()==0)
			return "Error: The word is epmty!";
                if (wordIn.length() == 1) 
                        return wordIn;

                char[] wordOut = wordIn.toCharArray();
                int firstLetterIndex = 0;
                int secondLetterIndex = firstLetterIndex+1;
                int lettersDistance=2;

                do {
                        swapLetters(wordOut,firstLetterIndex,secondLetterIndex);
                        firstLetterIndex += lettersDistance;
                        secondLetterIndex = firstLetterIndex + 1;
                } while (secondLetterIndex < wordIn.length());

                if (wordLengthIsOdd(wordIn)) {
                        wordOut[firstLetterIndex] = wordIn.charAt(firstLetterIndex);
                }

                return new String(wordOut);
        }

Nie wiem czy błędy powinny być zwracane przez specjalne stringi, ja bym albo rzucił wyjątek, albo zwrócił nulla i ustawił zmienną globalną informującą o rodzaju błędu.

edit/ dodatkowo zmienna secondLetterIndex wydaje się zbędna.

Ujdzie, choć osobiście powywalał bym większość ifów i zwracał null jeśli argument jest nullem a "" jeśli string jest pusty.

Szczerze... kod jest do d**y. Za dużo ifów w jednej metodzie. Za dużo poziomów zagłębienia w jednej metodzie. Kod jest namieszany dlatego masz problem z kompilacją.

Po kawałku

else if (wordIn.length() >= 2)

Brakuje jeszcze jednego else, które obstawiało by warunek wyjścia z metody jeżeli nie spełniony jest żaden z warunków.

Moja rada rozbij ten kod na mniejsze metody, które będą czytelniejsze będą miały jasną odpowiedzialność. Tu opchnąłeś wszystko w jednym miejscu przez co sam nie za bardzo ogarniasz przepływ sterowania w kodzie.

I żeby nie być gołosłownym:

public static String swapEveryTwoLetters(final String wordIn) {
		if (wordIn != null && wordIn.length() > 0) {
			char[] wordOut = wordIn.toCharArray();
			swapWord(wordOut);
			return new String(wordOut);
		}
		return "Error: The word is null!";
	}

	private static void swapWord(final char[] wordOut) {
		int firstLetterIndex = 0;
		int secondLetterIndex = firstLetterIndex + 1;
		int lettersDistance = 2;
		while (secondLetterIndex < wordOut.length) {
			swapLetters(wordOut, firstLetterIndex, secondLetterIndex);
			firstLetterIndex += lettersDistance;
			secondLetterIndex = firstLetterIndex + 1;
		}
	}

	private static void swapLetters(char[] wordOut, int firstLetterIndex,
			int secondLetterIndex) {
		char a = wordOut[firstLetterIndex];
		char b = wordOut[secondLetterIndex];
		wordOut[firstLetterIndex] = b;
		wordOut[secondLetterIndex] = a;

	}

Chyba znacznie czytelniejszy kod...

Zjarek napisał(a)

Ja bym zapisał to tak, wg mnie jest czytelniej:
Nie wiem czy błędy powinny być zwracane przez specjalne stringi, ja bym albo rzucił wyjątek, albo zwrócił nulla i ustawił zmienną globalną informującą o rodzaju błędu.
edit/ dodatkowo zmienna secondLetterIndex wydaje się zbędna.

Sposób zwracania błędów nie jest tutaj najważniejszy, dzęki jednak za radę. O ile nad rzuceniem wyjątku albo zwróceniem nulla bym sie jeszcze zastanowił to ustawianie zmiennej globalnej informującej o rodzaju błędu nie wydaje mi się za dobrym rozwiązaniem. W sumie chyba masz rację, że można usunąć. Początkowa wersja tego prostego algorytmu jej nie miała, jednak stwierdziłem, że zwiększy ona jego czytelność.

//edit:
@Koziołek: dzięki, biorę pod uwagę Twój komentarz, zastanowię się jeszcze nad tym.

Jak teraz?

class SentenceTwo {
	public String swapEveryTwoLetters(final String wordIn) {
		if (wordIn != null && wordIn.length() > 0) {
			return swapWord(wordIn);
		}
		else {
			return "The word is null!";
		}
	}

	private String swapWord(final String wordIn) {
		char[] wordOut = wordIn.toCharArray();
		int firstLetterIndex = 0;
		int lettersDistance=2;
		while (firstLetterIndex + 1 < wordIn.length()) {
			swapLetters(wordOut,firstLetterIndex,firstLetterIndex+1);
			firstLetterIndex += lettersDistance;
		} 
		return new String(wordOut);		
	}
		
	private void swapLetters(char[] wordIn, int firstLetterIndex, int secondLetterIndex) {
		char tmpLetter = wordIn[firstLetterIndex];
		wordIn[firstLetterIndex] = wordIn[secondLetterIndex];
		wordIn[secondLetterIndex] = tmpLetter;
	}
}

ja bym
return "The word is null!";
zamienił na
return null;
albo
throw new NullPointerException("word is null")

... sam String "The word is null!" jest kompletnie do niczego nikomu nieprzydatny...
takie coś to się wyświetla użytkownikowi programu, a nie przekazuje jako wartość zwrotna.

Przeklinam wszystkich takich "programistów", co zamiast sensownie obsłużyć błąd robią taki cyrk typu "The world is null!".. i domyśl się człowieku, jak tu coś takiego obsłużyć...
Wyobraź sobie później gdzieś w kodzie coś takiego:

String swapped = spawEveryTwoLetters(costam2);
if(swapped.equals("The word is null!"));
{
    //przerywamy task i wyswietlamy komunikat
}
else
{
    // ustawiamy stan na sukces i przekazujemy wynik dalej
}

true story... kiedyś podobnie musiałem, bo jakiś pacan postanowił, że wyjątki są "za wolne do sterowania przepływem programu" .. tak, na pewno porównywanie stringów jest szybsze, zjeb głupi.

Mam nadzieję, że to już wersja ostateczna:

class SentenceTwo {
        public String swapEveryTwoLetters(final String wordIn) {
                if (wordIn != null && wordIn.length() > 0) {
                        return swapWord(wordIn);
                }
                else {
                        return null;
                }
        }
 
        private String swapWord(final String wordIn) {
                char[] wordOut = wordIn.toCharArray();
                int firstLetterIndex = 0;
                int lettersDistance=2;
                while (firstLetterIndex + 1 < wordIn.length()) {
                        swapLetters(wordOut,firstLetterIndex,firstLetterIndex+1);
                        firstLetterIndex += lettersDistance;
                } 
                return new String(wordOut);                
        }
 
        private void swapLetters(char[] wordIn, int firstLetterIndex, int secondLetterIndex) {
                char tmpLetter = wordIn[firstLetterIndex];
                wordIn[firstLetterIndex] = wordIn[secondLetterIndex];
                wordIn[secondLetterIndex] = tmpLetter;
        }
}

Dzięki temu tematowi i krytyce dowiedziałem się kilku rzeczy :) Zastanawiam się jeszcze nad najlepszym możliwym sposobem zwracania błędów i przeglądając wypowiedzi w tym wątku wydaje mi się, że jest to null lub ewentualnie zdefiniowanie własnego typu wyliczeniowego. Wyjątki powinno się stosować jedynie w sytuacjach kiedy np. nie można odczytać danych z pliku (plik niedostępny, zablokowany przez inny proces itp.) ale nie w przypadku błędu dzielenia przez 0?

W sumie jeżeli zwracamy null to można napisać tak:

public static String swapEveryTwoLetters(final String wordIn) {
                String out = null;
                if (wordIn != null) {
                        char[] wordOut = wordIn.toCharArray();
                        swapWord(wordOut);
                        out = new String(wordOut);
                }
                return out;
        }

w ten sposób masz jeden punkt wyjścia z metody niezależnie co robisz.

Ok, niech już tak zostanie :) Jestem po prostu po przeczytaniu Kod doskonały / Code Complete - http://www.amazon.com/Code-Complete-Practical-Handbook-Construction/dp/0735619670 i próbuję stopniowo wcielać rzeczy tam opisane w życie.

Przeczytaj jeszcze "Czysty kod", "Piękny kod" i "Java. Efektywne programowanie." ;)

public static String swapPairs(String s) {
	if (s.length() <= 1) {
		return s;
	}
	else {
		return  "" + s.charAt(1) + s.charAt(0) + swapPairs(s.substring(2));
	}
}

Jeśli funkcja nie ma na celu przyjmowania gigantycznych danych (a tak podejrzewam w tym przypadku) to wersja rekurencyjna pod względem czytelności przebija imperatywne rozwiązania.

Nie pomyślałem w sumie o takim rozwiązaniu, ciekawe jak to by wyglądało pod względem wydajności... Trzeba będzie sprawdzić :)

Tutaj ten string będzie kopiowany więc złożoność kwadratowa zamiast liniowej.

Dzięki temu, że Stringi w Javie są niemodyfikowalne, metoda substring do tworzenia kolejnej instancji stosuje współdzielenie tej samej tablicy znaków. W tym celu używany jest niepubliczny konstruktor. Zatem kopiowanie nie jest tak straszne, jak piszesz.

// Package private constructor which shares value array for speed.
String(int offset, int count, char value[]) {
	this.value = value;
	this.offset = offset;
	this.count = count;
}

@code_killer, nie do końca. O ile substring rzeczywiście nie będzie kosztowny, to już dodawanie stringów będzie bardzo kosztowne. Zauważ, że tworzysz od cholery obiektów, które mają bardzo krótki czas życia. To spowoduje, że JVM będzie poświęcał dużo czasu na tworzenie i zwalnianie obiektów, które jest pozbawione sensu.

Java efektywne programowanie[0] :-), tak znam to. W tym wątku postawiłem na czytelność, a nie wydajność. W zasadzie można robić dodatkowa podfunkcja składająca dane na podstawie StringBuildera, albo po prostu odejść od wersji rekurencyjnej. Tylko po co? Jeśli funkcja będzie stosowana rzadko albo dla niewielkiej długości danych to po co przejmować się kilkoma cyklami?

Gdy profiler wskaże, że funkcja jest za wolna wtedy można zmienić ją inną, a jeśli nie to cieszmy się prostotą w końcu czas programisty jest ważniejszy niż czas procesora. Z resztą rozmowy o wydajności na tle Javy są dla mnie bardzo satyryczne :-).

[0] - http://helion.pl/ksiazki/java-efektywne-programowanie-wydanie-ii-joshua-bloch,javep2.htm

Co do wydajności Javy, to różnie z tym bywa: http://en.wikipedia.org/wiki/Java_performance

Co zaś do Sposobu działanie GC w starszych wersjach javy, to jest on niezbyt wydajny. W przypadku Stringów sprawa dodatkowo się będzie komplikować, ponieważ masz w użyciu FlyWeight co oznacza, że tak naprawdę obiekty nie będą zawsze usuwane. Będzie pozostawała w pamięci niewielka ilość "śmiecia", który będzie czekał na nie wiadomo co.

I dla zainteresowanych "debilotest" zużycia pamięci w przypadku tworzenia stringów inline:

import java.io.FileNotFoundException;
import java.io.PrintStream;

public class StringMemTest {

	public static void main(String[] args) throws FileNotFoundException {
		System.out.println("Max memory: " + Runtime.getRuntime().maxMemory());
		System.out.println("Free memory at start: " + Runtime.getRuntime().freeMemory());
		System.setErr(new PrintStream("NIL"));// pod UXami dajemy: System.setErr(new PrintStream("/dev/null"));
		for (int i = 0; i < 1000000; i++) {
			System.err.println("" + (i - 1) + "" + (i + 1) + "" + i);
			logFreeMemory(i);
		}
	}

	private static void logFreeMemory(int i) {
		if (i % 10000 == 0) {
         		final StringBuilder stringBuilder = new StringBuilder("Free memory after ");
			System.out.println(stringBuilder.append(i).append(" : ")
					.append(Runtime.getRuntime().freeMemory()).toString());
		}
	}
}

Widać, ze GC śmiga co pewien czas, ale widać też, że nie powraca do pierwotnej ilość wolnej pamięci nawet w najlepszym okresie.