Histogram of letters in string

Question

I'm pretty new to Java and I've been trying to use top-down design and make my code very readable, so I would appreciate any comments as to the clarity and efficiency of this program.

Looking for feedback on: readability (spacing, comments, etc) and just overall quality of code!

The task is to produce a histogram that shows how many times a certain letter appears in a string.

import java.util.Scanner;

/** This class creates a histogram of the letters in a string. Ie outputs
 ** how many times the letter a appears in the string, the letter b... and so on
 ** until z. Extra challege: only traversing the string once
 **/

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

    Scanner kb = new Scanner(System.in);
    final int LETTERS_IN_ALPHABET = 26;
    int[] letterCounter = new int[LETTERS_IN_ALPHABET]; //holds info on how many
    //times a letter appears. eg letterCounter[0] -> how many times a appears

    System.out.print("Enter string: ");
    String string = kb.nextLine();

    for(int i = 0; i < string.length(); i++){//traversing string
      char letterThere = string.charAt(i);//reads what character is at index
      int placeInLetterCtr = whereInLetterCtr(letterThere);//determining where it should go in array

      letterCounter[placeInLetterCtr] ++;//increasing corresponsing index
    }

    printNumbers(letterCounter);
    printLetters();


  }//end main

/* given a char, determines at what index of the character storage array the
 * char belongs to. Eg, if given the char 'c', it should return int 2
 */
  public static int whereInLetterCtr(char letter){
    int i =0 ;
    for(char comparisonLetter = 'a'; comparisonLetter <= 'z'; comparisonLetter++){
      if(letter == comparisonLetter){
        return i;
      }
      i++;
    }
    return i;
  }//end whereInLetterCtr

/* prints row of numbers */
  public static void printNumbers(int[] array){
    for(int i=0; i<array.length; i++){
      System.out.printf("%4d", array[i]);
    }

    System.out.println();
  }//end printNumbers

/* prints row of letters */
  public static void printLetters(){

    for(char letter ='a'; letter <='z'; letter++){
      System.out.printf("%4c", letter);
    }
  }//end printLetters

}//end class

Answer 1

• You've got way too many comments - you should only use comments when you need to explain an oddity. You don't need a comment to tell you when a method ends. I know this for loop for string.length() is traversing a string - it is simply visual clutter.

• A lot of your variable names don't follow programming standard glossary. You're trying to explain too much in your variable names. Trust the reader to figure it out to reduce visual clutter.

  • letterThere There? where's there? how about -> currentLetter

  • placeInLetterCtr place? letter ctr? is ctr center? constructor? we know from a bit of reading surrounding lines that it's the index in a letter counter (which is a crucial part of your code, so you don't need to keep reiterating everything is for the letter counter) how about -> letterIndex

  • whereInLetterCtr again, 'where'? it's an index! -> getIndexForCharacter

• I know it's a one-off class, but if you're going to do object-oriented programming do it right - Define your constants in your Histogram class, method, in the Histogram class. Remove 'static' from your methods, and create a Histogram class in your main() instead, using it like: new Histogram().create("someString")

Answer 2

1. Throughout your code you use for loops. Java 8 has been around for 4 years now. It is time you start using the new (now quite old) collection streams. Here are some snipets to get you started:

   string.chars().forEach(letterThere -> {
     ...
   })
   
   IntStream.rangeClosed('a', 'z').forEach(i -> {
     ...
   })
   

2. regarding whereInLetterCtr() : you advance comparisonLetter from 'a' to 'z' with the ++ operator so you realize that the letters form a sequence of numbers and that you can apply mathematical operations on them.
   so...
   instead of the whole loop you can get the result by doing letter - 'a'

Answer 3

Hmm... characters in the alphabet held in an array of dimension 26. Too bad, for me as a German person your program will not work, as we have äöüß. Or in other terms: this is the age of unicode and internationalization, and characters as array indexes don't go well any more. (そして、アジア全土に...)

Thus: better use a dynamic structure which takes the characters (or interger code-points in unicode) as the key and the count as a value.

Then, once again (I feel I start to repeat myself quite a lot) what remains is a grouping operation, and this is already in the standard library. Thus: yes OK for an exercise, but have a look at streams and grouping. In this case, start out with this central element:

String s = ...
Map<Integer, Long> perCharacterCount = s.codePoints()
    .boxed()
    .collect(Collectors.groupingBy(
        Function.identity(),
        Collectors.counting()));