patternjavaMinor
Tidy up number counting code
Viewed 0 times
codenumbercountingtidy
Problem
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:
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!
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 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!
Solution
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.
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:
Readability example
This little piece:
can be simplified like so:
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!
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!
Code Snippets
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....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);if (countOfNumbers != 9)
graph += (countOfNumbers + 1) + " ";
else
graph += (countOfNumbers + 1) + " ";graph += (countOfNumbers + 1) + " ";
if (countOfNumbers != 9) graph += " ";Context
StackExchange Code Review Q#17992, answer score: 6
Revisions (0)
No revisions yet.