12
\$\begingroup\$

I wrote this program to check the number of times that each letter appears in a string input by the user. It works fine, but is there a more efficient or alternative solution of going about this task than reiterating through a twenty-six-element-long array for every single character?

import java.util.Scanner;
public class Letters {
    public static void main(String[] args) {
        @SuppressWarnings("resource")
        Scanner sc = new Scanner(System.in);
        char[] c = {'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z'};
        int[] f = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
        System.out.println("Enter a string.");
        String k = sc.nextLine();
        String s = k.toUpperCase();
        s = s.trim();
        int l = s.length();
        System.out.println("Checking string = " + s);
        char ch;
        for (int i = 0; i < l; i++) {
            ch = s.charAt(i);
            for (int j = 0; j < c.length; j++) {
                if (ch == c[j]) {
                    f[j]++;
                }
            }
        }
        System.out.println("Char\tFreq");
        for (int i = 0; i < c.length; i++) {
            if (f[i] != 0) {
               System.out.println(c[i] + "\t" + f[i]);
            }
        }
    }
}
\$\endgroup\$
12
\$\begingroup\$

Separate logical elements

It's good to separate different logical parts of a program, for example:

  • Parse input: a function that takes an InputStream and returns a String
  • Compute frequencies: a function that takes a String and returns frequencies in some form. In your current program you used an int[], it could have been a Map<Character, Integer>.
  • Print the frequencies: a function that takes the frequencies in some form, returns nothing, and prints to screen the frequencies nicely formatted.

Computing indexes of letters

If the input string contains only uppercase letters, then you can translate those letters to array indexes in the range of 0 to 25 (inclusive) like this:

int index = ch - 'A';

This eliminates the nested loop you had. It also eliminates the need for the c array.

Initializing arrays

Instead of this:

int[] f = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};

You could write simply int[] f = new int[26];

Instead of this:

char[] c = {'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z'};

I would take a lazy approach and write char[] c = "ABCDEFGHIJKLMNOPQRSTUVWXYZ".toCharArray();

Use better variable names

Single-letter variable names should only be used for trivial, highly transient things. The names f and c in the program are inappropriate, and make the program more difficult to read.

\$\endgroup\$
  • \$\begingroup\$ I've been sticking to arrays because I haven't yet started learning about Maps and HashMaps. I'll be sure to check them out now... Thanks for pointing out the toCharArray() function. I was unaware that it existed. \$\endgroup\$ – Artemis Hunter Mar 17 at 8:43
  • \$\begingroup\$ I didn't mean that you should use a Map. I mentioned as an option. Both ways have advantages and disadvantages, the best solution depends on the use case. Here it doesn't matter much. \$\endgroup\$ – janos Mar 17 at 8:47
  • \$\begingroup\$ Understood. BTW, thought I'd point out about the variable names, I actually find it pretty convenient to go with single-letter names: c for character, f for frequency, i for iterator and so on. \$\endgroup\$ – Artemis Hunter Mar 17 at 8:51
  • 1
    \$\begingroup\$ @TomG But your consumer usually wouldn't have access to your code, would they? The consumer is meant to receive the application which the code runs. In case of another coder, you might be right, but even a simple remark next to the variable's declaration might suffice, would it not? Actually, I only use single letters in my basic programs, like the one above. Usually my programs have clear names. \$\endgroup\$ – Artemis Hunter Mar 17 at 10:14
  • 7
    \$\begingroup\$ @ArtemisHunter There are aspects of style that are subjective. In this example, good objective arguments exist, against comments. 1. The consumers of your binaries indeed don't care about variable names. But the consumers of your source code do: the human reviewers and maintainers. Without descriptive names, it's hard to keep in mind what f meant, you may have to scan back in the program to understand, which is a mental burden. 2. Comments can go out of sync with the code they describe. If instead of comments you write in a way that's self-descriptive, that's a better long-term solution. \$\endgroup\$ – janos Mar 17 at 11:34
10
\$\begingroup\$

Warnings

Don't use the suppresswarnings annotation for things that can be fixed easily. I don't recognize the "resource" warning, I guess it's specific to the compiler you're using (Eclipse?). Probably comes from using the Scanner without closing it properly. By using the try-with-resources -statement for anything that supports java.lang.AutoCloseable, this will be handled for you automatically.

Variables and naming

There's no point in trying to save a few keystrokes by using short variable names. The compiler doesn't care what the names are, so you can just as well use human-readable names. The exception here being de-facto standardized loop indices like i and j.

Any decent code editor or IDE will autocomplete the names for you, so it's not that much more to type. A modified section of your original code, notice how I also chained the call to toUpperCase() directly after the nextLine() call. No need to create a new variable for the case-corrected string:

Scanner inputScanner = new Scanner(System.in);
String input = inputScanner.nextLine().toUpperCase();

Object methods

Familiarize yourself with the Java standard library and API. The String class has a method for returning its contents as a char array: toCharArray(). You could use that, combined with the enhanced for loop to simplify your loop:

String input = // fetch string somehow
for (char inputChar : input.toCharArray()) {
    // Loop processing here
}

Printing an array is similarly a one-line operation: System.out.println(Arrays.toString(your array here))

Tips and tricks

There's a neat(?) trick for calculations using chars in Java. As you are upper-casing all the chars, you can use 'A' as the base for the array index. So instead of having two arrays, one with the frequencies, one for the char-to-index mapping, use subtraction from 'A' to get the index:

for (char inputChar : input.toCharArray()) {
    frequencies[inputChar - 'A']++;
}

Alternative implementation

Here's my alternative implementation using only the same data structures as in your original post. I do agree with Vishal Dhanotiya about the use of a map for this.

import java.util.Arrays;
import java.util.Scanner;

public class Letters {
    public static void main(String[] args) {

        int[] frequencies = new int[26];

        try (Scanner scanner = new Scanner(System.in)) {
            System.out.print("Enter a string: ");
            String input = scanner.nextLine().toUpperCase().replaceAll("[^A-Z]", "");

            for (char inputChar : input.toCharArray()) {
                frequencies[inputChar - 'A']++;
            }

            for (int i = 0; i < frequencies.length; i++) {
                System.out.printf("%s: %d, ", (char)('A' + i), frequencies[i]);
            }
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Yes, the "resource" is from eclipse. I usually ignore it, but this time I just felt like adding it. \$\endgroup\$ – Artemis Hunter Mar 17 at 10:16
  • \$\begingroup\$ It would probably be a good idea to check that inputChar is an uppercase letter. The code fails with ArrayIndexOutOfBoundsException otherwise. \$\endgroup\$ – Eric Duminil Mar 17 at 14:32
  • \$\begingroup\$ @EricDuminil. I convert the whole string to uppercase, so that should be taken care of: String input = scanner.nextLine().toUpperCase();. Should it still be checked withCharacter.isUpperCase()? Of course, this code fails in many ways if there are non-ASCII characters in the input. But ASCII was an implicit requirement also for the original code. I could have mentioned it, though. \$\endgroup\$ – TomG Mar 17 at 15:37
  • \$\begingroup\$ @TomG: It fails with a space for example. \$\endgroup\$ – Eric Duminil Mar 17 at 16:34
  • 1
    \$\begingroup\$ If I was reviewing this, I think I'd want a comment for the inputChar - 'A' trick. Could also init the frequencies to 'Z' - 'A' just to make clear what the length means? \$\endgroup\$ – JollyJoker Mar 18 at 9:53

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.