8
\$\begingroup\$

I am hoping someone can help me tidy up my code (for a practical for university). The practical is to use the random number generator to produce 100 integers (all between 1 and 10) and store them in an array. We then need to scan the array and print out how often each number appears. Following this, we needed to create a horizontal bar chart using asterisks to show how often each number appears, before finally printing out which number appeared the most.

My code works and produces the correct results:

import java.util.Random;

public class Practical4_Assessed 
{

public static void main(String[] args) 
{

    Random numberGenerator = new Random ();
    int[] arrayOfGenerator = new int[100];
    int[] countOfArray     = new int[10];
    int count;

    for (int countOfGenerator = 0; countOfGenerator < 100; countOfGenerator++)
    {
        count = numberGenerator.nextInt(10);
        countOfArray[count]++;
        arrayOfGenerator[countOfGenerator] = count + 1;
    }

    int countOfNumbersOnLine = 0;
    for (int countOfOutput = 0; countOfOutput < 100; countOfOutput++)
    {
        if (countOfNumbersOnLine == 10)
        {
            System.out.println("");
            countOfNumbersOnLine = 0;
            countOfOutput--;
        }
        else
        {
            if (arrayOfGenerator[countOfOutput] == 10)
            {
                System.out.print(arrayOfGenerator[countOfOutput] + "  ");
                countOfNumbersOnLine++;
            }
            else
            {
                System.out.print(arrayOfGenerator[countOfOutput] + "   ");
                countOfNumbersOnLine++;
            }
        }
    }

    System.out.println("");
    System.out.println("");

// This section

    for (int countOfNumbers = 0; countOfNumbers < countOfArray.length; countOfNumbers++)
        System.out.println("The number " + (countOfNumbers + 1) + " occurs " + countOfArray[countOfNumbers] + " times.");

    System.out.println("");

    for (int countOfNumbers = 0; countOfNumbers < countOfArray.length; countOfNumbers++)
    {
        if (countOfNumbers != 9)
                System.out.print((countOfNumbers + 1) + "   ");
        else
                System.out.print((countOfNumbers + 1) + "  ");

        for (int a = 0; a < countOfArray[countOfNumbers]; a++)
        {
            System.out.print("*");
        }
        System.out.println("");
    }

 // To this section

    System.out.println("");

    int max = 0;
    int test = 0;
    for (int counter = 0; counter < countOfArray.length; counter++)
    {
        if (countOfArray[counter] > max)
        {
            max = countOfArray[counter];
            test = counter + 1;
        }
    }

    System.out.println("The number that appears the most is " + test);

}
}

However, I know that the section that tells how often a number occurs and the section that prints out the asterisks both start with the same for statement (I have marked them off in the code with comments). Can anyone advise me how I could do this using just one for statement? It seems quite straightforward, and yet I just can't get it to work!

Additionally, I am sure there are plenty of other areas that the code could be improved on. Feel free to suggest any improvements!

\$\endgroup\$
6
\$\begingroup\$

There is much to say about this piece of code. I'll just focus on some parts of it.

One long method

There is one long main method. It makes it hard to get an overview of what is happening. Consider breaking the main method into several methods that each does its piece. Something like.

public static void main(String...args){
    int[] randomNumbers = generateNumbers();
    String result = format(randomNumbers);
    result += formatNumberOfOccurences(randomNumbers);
    result += formatGraph(randomNumber);
    System.out.println(result);
}

// 4 methods omitted....

In this way you will get smaller chunks that is easier to understand. Each of these methods except the number generator is also easy to unit test since the outcome is the same for each argument. The code you have now is hard to unit test.

Printing with System.out

This is handy in many ways. But this is the main reason you need two loops for the code you have highlighted. Since you want to print the occurrence count first and then the graph you ned to do it in separate loops. If you instead store the intermediate result in a local variable then you can do it in one loop. Something like:

String occurrencesReport = "";
String graph = "";

for (int countOfNumbers = 0; countOfNumbers < countOfArray.length; countOfNumbers++)
{
    occurrencesReport += "The number " + (countOfNumbers + 1) + 
        " occurs " + countOfArray[countOfNumbers] + " times.";

    if (countOfNumbers != 9)
        graph += (countOfNumbers + 1) + "   ";
    else
        graph += (countOfNumbers + 1) + "  ";

    for (int a = 0; a < countOfArray[countOfNumbers]; a++)
    {
        graph += "*";
    }
    graph += "\n";
}

System.out.println(occurrencesReport);
System.out.println(graph);

Readability example

This little piece:

if (countOfNumbers != 9)
    graph += (countOfNumbers + 1) + "   ";
else
    graph += (countOfNumbers + 1) + "  ";

can be simplified like so:

graph += (countOfNumbers + 1) + "  ";
if (countOfNumbers != 9) graph += " ";

this makes it easier to understand what is going on. (On a side note - most people dislike the absence of curly braces in this code snippet. If you add a statement yo your if-block it will actually be outside if curly braces are omitted. I tend to do ifs on one line when they are short. But people don't like that either....) This example can be applied to at least one more piece of the code where there is unnecessary repetition.

Better stop now - just some bits and pieces I hope you can use to improve!

\$\endgroup\$
  • \$\begingroup\$ I'll read through your comments and apply them to my code to try and tidy it up. Thanks very much for taking the time to go through it so thoroughly! \$\endgroup\$ – Andrew Martin Oct 27 '12 at 21:37
  • 1
    \$\begingroup\$ @froderik, excellent answer, but consider that printNumberOfOccurences and printGraph don't actually print anything. What do you think of less misleading names (such as formatNumberOfOccurrences and formatGraph, or something along those lines)? \$\endgroup\$ – Adam Oct 28 '12 at 17:53
  • \$\begingroup\$ @codesparkle: Can I ask how you would set up methods with arrays in them? For example, the formatNumberOfOccurrences method I could create - what is the syntax to do so? \$\endgroup\$ – Andrew Martin Oct 28 '12 at 18:24
  • \$\begingroup\$ @codesparkle: excellent naming suggestion. I am not sure about format but it sure is better than print! \$\endgroup\$ – froderik Oct 28 '12 at 19:35
  • \$\begingroup\$ @AndrewMartin: are you asking how a method that accepts an array looks like? public String formatGraph(int[] array){ /* code here */ } \$\endgroup\$ – froderik Oct 28 '12 at 19:39
6
\$\begingroup\$

Andrew, welcome to Code Review. Froderik already suggested some excellent improvements.

Warning
You're about to read a rather radical review. Proceed with caution (and, optionally, a cup of coffee).

An Architectural Perspective

Red flags

The code is:

  • within a single class: Practical4_Assessed
  • within a single method: main
  • deeply nested
  • full of magic numbers

These code smells indicate that refactoring is needed to achieve a more expressive result.

Single Responsibility Principle1 and Separation of Concerns2

A good way to start off is to count the number of responsibilities of your class Practical4_Assessed:

  • Entry point of the program
  • Generate random data
  • Count occurrences of data
  • Print a visualization of the data

These responsibilities should be separated into one class per concern:

  • Main: entry point, instantiates objects and gets the system going
  • RandomDataSource: generates random data
  • Counter: counts the number of occurrences of the data
  • CountPrinter: prints the visualization

But that's not enough: these classes have to be divided into sensibly-sized methods. Each one of those methods should do one thing: just a level of abstraction further down.

Code re-use

Counting some data is a rather common task. It's safe to say that you'll eventually need to count other things than ints. But you won't be able to, because your program only supports a single data type.

In other words, your code is not reusable. This can be solved by using generics: defining a data structure that can deal with any type of data.


Refactoring

Let's start by thinking about how to best represent the concept of a source of random data.

Data sources

There will be different data sources for different types, but all will adhere to the same contract and all will require a source of randomness. To accommodate these requirements, an abstract, generic base class is needed:

public abstract class RandomDataSource<T> {

    protected static final Random random = new Random();

    public abstract T nextElement();

    public Iterable<T> nextElements(int numberOfElements) {
        Collection<T> elements = new ArrayList<>(numberOfElements);
        for (int i = 0; i < numberOfElements; i++) {
            elements.add(nextElement());
        }
        return elements;
    }
}

Note that:

  • the contract is narrow: there are only two methods,

    • nextElement, which is abstract and needs to be implemented by deriving classes;
    • nextElements(int numberOfElements), which is a convenience method;
  • the most general interface that is appropriate (Iterable<T>) is returned in order to preserve flexibility and decouple the implementation from the interface.

To generate the random numbers, create a subclass where nextElement is overridden to return a random Integer between the specified bounds:

public class RandomNumberSource extends RandomDataSource<Integer> {

    private final int elements;
    private final int offset;

    public RandomNumberSource(int lowerBound, int upperBound) {
        elements = upperBound - lowerBound + 1;
        offset = lowerBound;
    }

    @Override
    public Integer nextElement() {
        return random.nextInt(elements) + offset;
    }
}

And to generate random names, a randomly chosen name chosen from an array of names is returned:

public class RandomNameSource extends RandomDataSource<String> {

    private final String[] names;

    public RandomNameSource(String... someNames) {
        names = someNames;
    }

    @Override
    public String nextElement() {
        return names[random.nextInt(names.length)];
    }
}

If you need to count a different type of elements (maybe complex objects), you can now easily create another subclass to generate them.

Entry point

Your main method (in the class Main) is now rather short. It creates the data sources:

public static void main(String[] args) {
    String[] names = { "Adam", "David", "Eve", "Frank", "John" };
    visualize(new RandomNumberSource(1, 10), 100);
    visualize(new RandomNameSource(names), 50);
}

Then, it creates a Counter to count them and fills that Counter with elements from the data source:

private static <T> void visualize(RandomDataSource<T> dataSource, int elementCount) {
    Counter<T> counter = new Counter<>();
    for (T element : dataSource.nextElements(elementCount)) {
        counter.increment(element);
    }
    print(counter);
}

Finally, it prints the occurrences using a CountPrinter:

private static <T> void print(Counter<T> counter) {
    CountPrinter<T> printer = new CountPrinter<>(counter, System.out);
    printer.printOccurences();
    printer.printChart();
    printer.printLargest();
}

Note that every class clearly has a single responsibility. Let's take a look at Counter<T> first.

Counting occurrences

We are going to want to iterate over the results, so we'll make our counting class implement Iterable. It uses a Map to store the elements and their counts as key-value-pairs. In this implementation, a HashMap is used; replace it with a TreeMap if you want to iterate in the natural ordering of T, or a LinkedHashMap if you want to preserve insertion order.

increment adds the specified element to the map if it doesn't yet exist; otherwise, it increments the count.

One of the problems with your assignment is that multiple elements can have the same count, so "which number appeared the most" is undefined. Instead, you should return all the elements with the maximum count, as shown in getLargestElements.

public class Counter<T> implements Iterable<T> {

    private Map<T, Integer> map = new HashMap<>();

    public void increment(T element) {
        int count = map.containsKey(element) ? map.get(element) : 0;
        map.put(element, count + 1);
    }

    public int getCount(T element) {
        return map.get(element);
    }

    public Iterable<T> getLargestElements() {
        int largest = Collections.max(map.values());
        Collection<T> keys = new ArrayList<>();
        for (T key : map.keySet()) {
            if (map.get(key) == largest) {
                keys.add(key);
            }
        }
        return keys;
    }

    @Override
    public Iterator<T> iterator() {
        return map.keySet().iterator();
    }
}

Printing the results

The output code is isolated in a single class that takes a Counter<T> and a PrintStream, for instance System.out, but it could just as well be a file or a socket stream. Instead of manually adding spaces to smooth out and align the numbers, you can specify a minimum width in Java format strings. For instance, %5s will pad the text to maintain a width of 5 spaces.

To visualize the bars of the bar graph, simply use Arrays.fill to create a bar of *.

public class CountPrinter<T> {

    private final Counter<T> counter;
    private final PrintStream out;

    public CountPrinter(Counter<T> source, PrintStream target) {
        counter = source;
        out = target;
    }

    public void printOccurences() {
        for (T entry : counter) {
            out.printf("%5s occurs %2s times.\n", entry, counter.getCount(entry));
        }
    }

    public void printChart() {
        for (T entry : counter) {
            char[] bar = new char[counter.getCount(entry)];
            Arrays.fill(bar, '*');
            out.printf("%5s: %s\n", entry, new String(bar));
        }
    }

    public void printLargest() {
        out.println("most occurrences:");
        for (T element : counter.getLargestElements()) {
            out.println(element);
        }
    }
}

Output

Numbers:

    1 occurs  7 times.
    2 occurs 10 times.
    3 occurs 16 times.
    4 occurs 13 times.
    5 occurs 10 times.
    6 occurs 12 times.
    7 occurs  9 times.
    8 occurs 10 times.
    9 occurs  6 times.
   10 occurs  7 times.
    1: *******
    2: **********
    3: ****************
    4: *************
    5: **********
    6: ************
    7: *********
    8: **********
    9: ******
   10: *******
most occurrences:
3

Names:

David occurs  5 times.
Frank occurs 13 times.
 Adam occurs 13 times.
 John occurs  8 times.
  Eve occurs 11 times.
David: *****
Frank: *************
 Adam: *************
 John: ********
  Eve: ***********
most occurrences:
Frank
Adam
\$\endgroup\$
  • 1
    \$\begingroup\$ I think 'wow' is appropriate. I'll work my way through all your suggestions and amend my work appropriately - but it'll take me quite a while! I'm not at all familiar with things like HashMap, TreeMap or Array Lists. Thank you so much for taking the time to provide all of this. \$\endgroup\$ – Andrew Martin Oct 29 '12 at 16:14
  • 1
    \$\begingroup\$ Thank you for the kind words. You may find this introduction to Java Collections helpful. There's also a Wikipedia article and an extensive tutorial (this page is the most helpful as an overview). \$\endgroup\$ – Adam Oct 29 '12 at 16:26
  • \$\begingroup\$ I promised to write a comment, so here we go. I will keep it short for limited space. SoC is a good thing, but I would use it carefully, not force it. Sometime, it is harder to understand code if it is divided across all the places. Using generics if you need only one case is not advised. Do not code for undefined future usage. I would keep the Random private & provide getter. Or do not provide it at all, because implementation is done in the subclass. The countPrinter approach looks rather fine. Overall, from my view it is a nice and clean approach. \$\endgroup\$ – tb- Nov 23 '12 at 17:38

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.