Proszę o ocenę czytelności kodu

0

Cześć,
Pracuję ostatnio nad poprawą czytelności mojego kodu, mam więc pytanie. Jak oceniacie czytelność kodu zamieszczonego poniżej

    public static String swapEveryTwoLetters(final String wordIn) {
        if (wordIn != null) {
            if (wordIn.length() == 0) {
                return "Error: The word is epmty!";
            } else if (wordIn.length() == 1) {
                return wordIn;
            } else if (wordIn.length() >= 2) {
                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);
            }
        } else {
            return "Error: The word is null!";
        }
        return "Why?"; //1
    }

Jeszcze jedno pytanie odnośnie kodu:
Dlaczego nie kompiluje się jeżeli usunę linię z komentarza 1?

0

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.

0

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

0

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

0

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

1

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.

0

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

1

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...

0
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.

0

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;
    }
}

1 użytkowników online, w tym zalogowanych: 0, gości: 1, botów: 0