1
\$\begingroup\$

Was just wondering if I was using nested if statements to much. I've been looking around and it seems that people try to not use them. Also does the code look messy in any way? Anyways here it is:

CheckScores.java

import java.util.Arrays;

public class CheckScores {

private int maxScore = 6;
private int sum;

public void CheckScores(String userScores) {
    String[] scoresArray;

    // Check if userScores is valid
    // ^ Match with beginning of line | [0-9] Allow 0-9 | , Allow comma | + Match one or more | $ Match End of line
    if (userScores.matches("^[0-9,]+$")) {

        // Check if string starts with an ,
        if (userScores.charAt(0) == ',') {
            // If it does parse and substring to remove them
            // otherwise the following regex leaves one , behind
            int i = 0;
            while (!Character.isDigit(userScores.charAt(i))) i++;
            int j = userScores.length();
            userScores = userScores.substring(i, j);
        }

        // (.) Match any character) | \1 If it is followed by itself | + Match one or more | $1 replace by the first captured char.
        userScores = userScores.replaceAll("(.)\\1+", "$1");

        System.out.println(userScores);

        // Split at the ',' and put each number in it's own cell in the array
        scoresArray = userScores.split(",");
        System.out.println(Arrays.toString(scoresArray));

        // Check if scoresArray is equal to maxScores
        if (scoresArray.length == maxScore) {
            int[] uiArray = new int[scoresArray.length];

            // Parse String[] into int[]
            for (int i = 0; i < scoresArray.length; i++) {
                try {
                    uiArray[i] = Integer.parseInt(scoresArray[i]);
                } catch (NumberFormatException nfe) { // If triple checking isn't enough...
                }
            }
            System.out.println("scoresArray array(String): " + Arrays.toString(scoresArray));
            System.out.println("uiArray(int): " + Arrays.toString(uiArray));

            // Add up all elements in uiArray
            for (int j = 0; j < uiArray.length; j++) {
                sum += uiArray[j];
            }
            System.out.println("Scores sum:" + sum + " Number of scores:" + uiArray.length + " Number of ends:" + uiArray.length / 6);
        } else {
            System.out.println("You have " + scoresArray.length + " scores. Acceptable amount is " + maxScore);
        }
    } else {
        System.out.println("Invalid Input. (Only #'s and ,'s allowed)");
    }
  }
}

Main.java

public class Main {

public static void main(String[] args) {
    CheckScores cs = new CheckScores();
    cs.CheckScores("1,M,7,10,,4,8,");
    cs.CheckScores("1,6,7,10,,4,8,,,,,,,1,2,6,10,2,10");
    cs.CheckScores(",,,,,,,1,2,6,10,2,10");
    cs.CheckScores("10,2,1,5,7,1");
    cs.CheckScores("6,2, ,,5,6,1");
    }
}
\$\endgroup\$
  • 3
    \$\begingroup\$ Please try to include a description of what the code is supposed to accomplish. The people reviewing your code will gives better reviews if they clearly understand the goal. \$\endgroup\$ – RubberDuck Nov 22 '14 at 4:34
  • 1
    \$\begingroup\$ @RubberDuck Ok, will do next time. \$\endgroup\$ – user2999980 Nov 22 '14 at 19:29
4
\$\begingroup\$

Was just wondering if I was using nested if statements to much.

Yes. The less nesting the better, when possible. In this example it's possible. Less nested code is often more readable, clear.

Also does the code look messy in any way?

It's messy in many ways:

  • CheckScore class with CheckScore method...
    • Only the constructor should have the same name as the class
    • Methods should be camelCase, not CamelCase
  • The validation logic is far more complicated than it needs to be
    • The step of catching NumberFormatException is paranoid, and it basically reveals your lack of trust in the logic
  • The variable name maxScore = 6 is misleading: the code requires exactly 6 values, not maximum 6.
  • The use of sum looks buggy: consecutive calls with valid parameters will add on top of the previous sum, and it's not at all clear if this side effect of the method is intended or not
  • Instead of printing when something is not valid, it would be better to throw IllegalArgumentException
  • Instead of testing the code with print statements, it would be better to write proper unit tests
  • Instead of old-fashioned for (;;) it's better to use modern and elegant for-each loop when possible
  • Many of the comments are trivial and should be removed

Suggested implementation:

class CheckScores {

    private static final int REQUIRED_SCORE_COUNT = 6;

    private static final Pattern ILLEGAL_SYMBOLS = Pattern.compile("[^0-9,]");
    private static final Pattern STARTING_COMMAS = Pattern.compile("^,+");  // to replace with ""
    private static final Pattern TOO_MANY_COMMAS = Pattern.compile(",,+");  // to replace with ","

    public String checkScores(String textInput) {
        if (ILLEGAL_SYMBOLS.matcher(textInput).find()) {
            throw new IllegalArgumentException("Invalid Input. (Only digits and commas are allowed)");
        }

        textInput = textInput.replaceFirst(STARTING_COMMAS.pattern(), "");
        textInput = textInput.replaceAll(TOO_MANY_COMMAS.pattern(), ",");

        String[] scoresArray = textInput.split(",");
        if (scoresArray.length != REQUIRED_SCORE_COUNT) {
            throw new IllegalArgumentException("You have " + scoresArray.length + " scores. Acceptable amount is " + REQUIRED_SCORE_COUNT);
        }

        int[] uiArray = new int[scoresArray.length];
        for (int i = 0; i < scoresArray.length; i++) {
            uiArray[i] = Integer.parseInt(scoresArray[i]);
        }

        int sum = 0;
        for (int item : uiArray) {
            sum += item;
        }
        return "Scores sum:" + sum + " Number of scores:" + uiArray.length + " Number of ends:" + uiArray.length / 6;
    }
}

Unit tests:

public class CheckScoresTest {
    CheckScores cs = new CheckScores();

    @Test(expected = IllegalArgumentException.class)
    public void testInvalidInput_IllegalSymbols_Letters() {
        cs.checkScores("1,M,7,10,,4,8,");
    }

    @Test(expected = IllegalArgumentException.class)
    public void testInvalidInput_IllegalSymbols_Space() {
        cs.checkScores("6,2, ,,5,6,1");
    }

    @Test(expected = IllegalArgumentException.class)
    public void testInvalidInput_TooManyScores() {
        cs.checkScores("1,6,7,10,,4,8,,,,,,,1,2,6,10,2,10");
    }

    @Test
    public void testValidInput_31() {
        assertEquals("Scores sum:31 Number of scores:6 Number of ends:1", cs.checkScores(",,,,,,,1,2,6,10,2,10"));
    }

    @Test
    public void testValidInput_26() {
        assertEquals("Scores sum:26 Number of scores:6 Number of ends:1", cs.checkScores("10,2,1,5,7,1"));
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Wow, would have never thought of it like this. Thanks alot! \$\endgroup\$ – user2999980 Nov 22 '14 at 19:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.