10
\$\begingroup\$

I'm a beginner at coding, and I am looking to improve the structure of how I write code and will take any tips. Posted a simple program that takes your name as input, then runs the string through a switch statement to print out how many of each character your name has, as well as any spaces and symbols.

package practice;
import java.util.Scanner;

public class counter {

    public static void main(String[] args) {
        Scanner data = new Scanner(System.in);
        String name;
        System.out.println("Enter name");
        name = data.nextLine();
        int len = name.length();
        int ch = 0;
        int charCount = 0;
        int space = 0;
        int symbols = 0;
        int a = 0,b = 0,c = 0,d = 0,e = 0,f = 0,g = 0,h = 0,i = 0,j = 0,k = 0,l = 0,m = 0;
        int n = 0,o = 0,p = 0,q = 0,r = 0,s = 0,t = 0,u = 0,v = 0,w = 0,x = 0,y = 0,z = 0;

            for (int in = 0;in < len; in++)
                {
                switch(name.charAt(ch))
                    {
                        case 'a':
                            a++;
                            charCount++;
                            break; 
                        case 'b':
                            b++;
                            charCount++;
                            break;
                        case 'c':
                            c++;
                            charCount++;
                            break;
                        case 'd':
                            d++;
                            charCount++;
                            break;
                        case 'e':
                            e++;
                            charCount++;
                            break;
                            case 'f':
                                f++;
                            charCount++;
                            break;
                        case 'g':
                            g++;
                            charCount++;
                            break;
                        case 'h':
                            h++;
                            charCount++;
                            break;
                        case 'i':
                            i++;
                            charCount++;
                            break;
                        case 'j':
                            j++;
                            charCount++;
                            break;
                        case 'k':
                            k++;
                            charCount++;
                            break;
                        case 'l':
                            l++;
                            charCount++;
                            break;
                        case 'm':
                            m++;
                            charCount++;
                            break;
                        case 'n':
                            n++;
                            charCount++;
                            break;
                        case 'o':
                            o++;
                            charCount++;
                            break;
                        case 'p':
                            p++;
                            charCount++;
                            break;
                        case 'q':
                            q++;
                            charCount++;
                            break;
                        case 'r':
                            r++;
                            charCount++;
                            break;
                        case 's':
                            s++;
                            charCount++;
                            break;
                        case 't':
                            t++;
                            charCount++;
                            break;
                        case 'u':
                            u++;
                            charCount++;
                            break;
                        case 'v':
                            v++;
                            charCount++;
                            break;
                        case 'w':
                            w++;
                            charCount++;
                            break;
                        case 'x':
                            x++;
                            charCount++;
                            break;
                        case 'y':
                            y++;
                            charCount++;
                            break;
                        case 'z':
                            z++;
                            charCount++;
                            break;
                    default:
                        if(name.charAt(ch) == ' ')
                            space++;
                        if(name.charAt(ch) != ' ')
                            symbols++;
                        break;
                    }
                char temp = Character.toUpperCase(name.charAt(ch));

                if(name.charAt(ch) == temp && temp != ' ' )
                    {
                        System.out.println("ERROR: UPPERCASE NOT ALLOWED");
                        System.out.println("EXITING APPLICATION");
                        System.exit(1);
                    }
                if(name.charAt(ch) == '0' && name.charAt(ch) == '1' && name.charAt(ch) == '2' )
                    {   
                        System.out.println("ERROR: NUMBERS NOT ALLOWED");
                        System.out.println("EXITING APPLICATION");
                        System.exit(1);
                    }
                if(name.charAt(ch) == '3' && name.charAt(ch) == '4' && name.charAt(ch) == '5' )
                    {
                        System.out.println("ERROR: NUMBERS NOT ALLOWED");
                        System.out.println("EXITING APPLICATION");
                        System.exit(1);
                    }
                if(name.charAt(ch) == '6' && name.charAt(ch) == '7' && name.charAt(ch) == '8' )
                    {
                        System.out.println("ERROR: NUMBERS NOT ALLOWED");
                        System.out.println("EXITING APPLICATION");
                        System.exit(1);
                    }
                if(name.charAt(ch) == '9' )
                    {
                        System.out.println("ERROR: NUMBERS NOT ALLOWED");
                        System.out.println("EXITING APPLICATION");
                        System.exit(1);
                    }
                ch++;
                }
            if (a > 0){

                System.out.println("There are ("+a+"-As)  in your name");

            }
            if (b > 0){

                System.out.println("There are ("+b+"-Bs)  int your name");

            }
            if (c > 0){

                System.out.println("There are ("+c+"-Cs)  int your name");

            }
            if (d > 0){

                System.out.println("There are ("+d+"-Ds)  int your name");

            }
            if (e > 0){

                System.out.println("There are ("+e+"-Es)  int your name");

            }
            if (f > 0){

                System.out.println("There are ("+f+"-Fs)  int your name");

            }
            if (g > 0){

                System.out.println("There are ("+g+"-Gs)  int your name");

            }
            if (h > 0){

                System.out.println("There are ("+h+"-Hs)  int your name");

            }
            if (i > 0){

                System.out.println("There are ("+i+"-Is)  int your name");

            }
            if (j > 0){

                System.out.println("There are ("+j+"-Js)  int your name");

            }
            if (k > 0){

                System.out.println("There are ("+k+"-Ks)  int your name");

            }
            if (l > 0){

                System.out.println("There are ("+l+"-Ls)  int your name");

            }
            if (m > 0){

                System.out.println("There are ("+m+"-Ms)  int your name");

            }
            if (n > 0){

                System.out.println("There are ("+n+"-Ns)  int your name");

            }
            if (o > 0){

                System.out.println("There are ("+o+"-Os)  int your name");

            }
            if (p > 0){

                System.out.println("There are ("+p+"-Ps)  int your name");

            }
            if (q > 0){

                System.out.println("There are ("+q+"-Qs)  int your name");

            }
            if (r > 0){

                System.out.println("There are ("+r+"-Rs)  int your name");

            }
            if (s > 0){

                System.out.println("There are ("+s+"-Ss)  int your name");

            }
            if (t > 0){

                System.out.println("There are ("+t+"-Ts)  int your name");

            }
            if (u > 0){

                System.out.println("There are ("+u+"-Us)  int your name");

            }
            if (v > 0){

                System.out.println("There are ("+v+"-Vs)  int your name");

            }
            if (w > 0){

                System.out.println("There are ("+w+"-Ws)  int your name");

            }
            if (x > 0){

                System.out.println("There are ("+x+"-Xs)  int your name");

            }
            if (y > 0){

                System.out.println("There are ("+y+"-Ys)  int your name");

            }
            if (z > 0){

                System.out.println("There are ("+z+"-Zs)  int your name");

            }
        System.out.println("\nThere are a total of "+charCount+" characters");
        System.out.println("\nThere are ("+space+"-Spaces)  in the data");
        System.out.println("\nThere are "+symbols+"-Symbols in the data");
    }

}
\$\endgroup\$
  • \$\begingroup\$ For a beginner is ok this code. Just read it as you were another person reading it. It's not confusing and the identation is acceptable. This page can help you introcs.cs.princeton.edu/java/11style \$\endgroup\$ – pb77 Dec 14 '15 at 11:31
  • \$\begingroup\$ @pb77 nope, not ok. Writing so much code for such an easy problem is not ok \$\endgroup\$ – Caridorc Dec 14 '15 at 14:13
  • 3
    \$\begingroup\$ This is shocking code, but I like that you're posting on code review in the right way trying to improve. So it gets my upvote. \$\endgroup\$ – JᴀʏMᴇᴇ Dec 14 '15 at 14:43
6
\$\begingroup\$

try-with-resources

Since Java 7, you should use try-with-resources on your Scanner instance for safe and efficient handling of the underlying I/O resource (i.e. System.in).

Data structures

As daunting as it may sound, this is actually an important concept to grasp for beginners. As pointed out by others, there is code duplication, and that is bad because As pointed out by others, there is code duplication, and that is bad because As pointed out by others, there is code duplication, and that is bad becuase As pointed out by others, there is code duplication, and that is bad because As pointed out by others, there is code duplication, and that is bad because I wonder if you realized the third because had a typo.

Even though Java doesn't let you reference undeclared variable names by accident, typos such as using the wrong variable names (interchanging m and n perhaps?), numbers or operators may still allow the code to compile; that is when bugs sneak into your codebase and your 2:47 am coding nightmares.

The point here is, instead of using upwards of 28 standalone variables (a...z, spaces and symbols) to represent each counter, you should be using an array for starters, and eventually progress to Collections or even a Stream-based processing approach (which I'm simply offering as links here).

For example, if you are only interested in counting for a...z, spaces and symbols, you need an int[] array with 28 elements:

int[] counter = new int[28];

Next, think about 'converting' each desired condition (whether the character is a, b... a space or a symbol) into a 'place' in the array, i.e. the index. For example, you can count as in index 0, bs in index 1 and so on (ignoring case sensitivity for now):

// inside a loop, assume input has been sanitized to all lower case
char current = input.charAt(i);
if (current >= 'a' && current <= 'z') {
    counter[current - 'a']++;
} else if (current == ' ') {
    counter[26]++; // we start from 0, so index 26 is the 27th place, after 'z'
} else if (current < '0' || current > '9') {
    counter[27]++;
}

This is a simplified implementation, and there's definitely room for improvement for the condition checking, such as using the range of Character methods to test for whitespace or digits.

The other major benefit to using an array is that you can loop through it too:

String values = "abcdefghijklmnopqrstuvwxyz";
for (int i = 0; i < counter.length; i++) {
    if (counter[i] > 0) {
        String value = i < 26 ? values.charAt(i) : i == 26 ? "space" : "symbol";
        if (counter[i] > 1) {
            value += i < 26 ? "'s" : "s";
        }
        System.out.println("There are " + counter[i] + value + " in the input");
    }
}

Beats having to copy-paste an if-condition 28 times. :)

Bug

Wells...

if(name.charAt(ch) == '0' && name.charAt(ch) == '1' && name.charAt(ch) == '2' ) {
    // ...
}

There is a bug here as the whole condition will never be true due to the usage of &&. What you are looking for here is ||, i.e. if character == '0' OR character == '1' OR character == '2'.

Program flow

if(name.charAt(ch) == '9' ) {
    System.out.println("ERROR: NUMBERS NOT ALLOWED");
    System.out.println("EXITING APPLICATION");
    System.exit(1);
}

Causing your program to inexplicably exit(1) when encountering a number is a particularly harsh way of handling errors... (Very) often, you will want to make it easier to recover from errors and retry wherever possible. For example, if you want to consider any input with numbers in them as wrong and re-prompt for a fresh input, you can do something like this:

private static boolean hasDigits(String input) {
    // test for digits here, return true if there are
}

public static void main(String[] args) {
    try (Scanner scanner = new Scanner(System.in)) {
        String input = scanner.nextLine();
        while (hasDigits(input)) {
            System.out.println("No numbers please.");
            input = scanner.nextLine();
        }
        // do something with input
    }
}

Or alternatively, wrap the validation into its own method:

private static String getInput(Scanner scanner) {
    String input = scanner.nextLine();
    while (hasDigits(input)) {
        System.out.println("No numbers please.");
        input = scanner.nextLine();
    }
    return input;
}

// refactored main()
public static void main(String[] args) {
    try (Scanner scanner = new Scanner(System.in)) {
        String input = getInput(scanner);
        // do something with input
    }
}

Formatting

Another useful advice for beginners is to standardize on the code formatting style. Currently, your code is already lengthy enough without the extra empty lines, and your bracing ({...}) is quite inconsistent.

There are different schools of thoughts about 'paragraphing' code into nice-looking blocks with empty lines as separators, and the one I think is arguably more practical is if you have to resort to this, the method is just too long (like your main() method now). Favor smaller 'bite-size' methods that more expressively captures a processing 'step', rather than creating monolithic ones that do everything.

As for braces, it's recommended not to skim on them, especially for beginners. This is so as they eliminate any chance of bugs, while enhancing readability at the same time. For example:

for (int i = 0; i < 10; i++)
    System.out.println(i);
    System.out.println(i + 1);

The last line is non-compilable as the scope of i only extends to the middle one. Of course, it's important to keep their position consistent too:

if (/* condition 1 */) {
    // ...
if (/* condition 2 */) {
    // ...
} else if (/* condition 3 */) {
    // ...
}
} else if (/* condition 4 */) {
    // ...
}

It's not easy to read the nesting here, in this admittedly contrived example. What looks like 4 if conditions on the same level is actually just 2 (1 and 4), with the other two a pair of nested if conditions if condition 1 is true.

If you can improve on the code formatting to enhance its readability, it's a lot easier to understand your own code, and subsequently improve on it. :)

\$\endgroup\$
3
\$\begingroup\$

Looping

Doing a thing many times is possible by the use of a loop, I see that you already know a bit about them, as you used a for loop in your code.

For the basics, you will want to use 2 loops one inside each other, as to make the code much closer to a sane size.

Modularity

Still, I do not like putting loops one inside each other (that is nesting loops) so I suggest writing a method to count the occurrences of a character and then using it inside a loop.

Re-use

The next step is realizing that in Java you almost never write something new, but assemble already written blocks, and looking for the library that counts how many times a char fits into a string.

After doing these two steps, your code is now down to 6-7 lines, and no further simplification is possible.

\$\endgroup\$
2
\$\begingroup\$

No, it is not good, it has way too much code replicated

Consider having an int array (one for each char a-z) and increament to the value of the array. e.g. arr[0] == counter for 'a' arr[1] == counter for 'b' etc. if the the char is >= 'a' and <= 'z' then the logic is the same, you just need to work out the offset in the array

\$\endgroup\$
  • 3
    \$\begingroup\$ A key value pair may be a better way to go. Maybe a dictionary where the character is the key and the value is the count. \$\endgroup\$ – RubberDuck Dec 14 '15 at 11:21

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.