5
\$\begingroup\$

I've been been working on a vertical histogram that prints an asterisk in place of a number in a certain range (say 1-10... and so on). My code is working as required.

Is there a better, simpler or more efficient way of doing it? If so, would you mind explaining or showing me better code if you have it handy?

import java.util.Scanner;

public class VerticalHistogram{
    public static void main(String args[]){
        int ranges[] = generateRandomNumbers(amountOfRandomNumbersToGenerate());
        printVerticalHistogram(largestValueInArray(ranges), ranges);
    }
    public static int amountOfRandomNumbersToGenerate(){
        Scanner input = new Scanner(System.in);
        System.out.print("Enter amount of random numbers to generate: ");
        return Integer.parseInt(input.nextLine());
    }
    public static int[] generateRandomNumbers(int amount){
        int ranges[] = new int[10];
        for(int i = 0; i < amount; i++){
            int num = (int)(Math.random() * 100) + 1;
            if(num < 10) ranges[0]++;
            else if(num < 20) ranges[1]++;
            else if(num < 30) ranges[2]++;
            else if(num < 40) ranges[3]++;
            else if(num < 50) ranges[4]++;
            else if(num < 60) ranges[5]++;
            else if(num < 70) ranges[6]++;
            else if(num < 80) ranges[7]++;
            else if(num < 90) ranges[8]++;
            else ranges[9]++;
        }
        return ranges;
    }
    public static int largestValueInArray(int ranges[]){
        int largest = ranges[0];
        for(int i = 1; i < ranges.length; i++)
            if(ranges[i] > largest) largest = ranges[i];
        return largest;
    }
    public static void printVerticalHistogram(int rows, int asterisks[]){

        System.out.printf(" %-7d%-7d%-7d%-7d%-7d%-7d%-7d%-7d%-7d%d",asterisks[0],asterisks[1],
            asterisks[2],asterisks[3],asterisks[4],asterisks[5],asterisks[6],asterisks[7],asterisks[8],asterisks[9]);
        System.out.println();
        while(rows > 0){
            for(int i = 0; i < asterisks.length; i++){
                if(i == 0){
                    if(asterisks[i] < rows) System.out.printf(" %-7s"," ");
                    else System.out.printf(" %-7s","*");
                }
                else{
                    if(asterisks[i] < rows) System.out.printf("%-7s"," ");
                    else System.out.printf("%-7s","*");
                }
            }
            System.out.println();
            rows--;
        }
        System.out.printf("%-6s%-7s%-7s%-7s%-7s%-7s%-7s%-7s%-7s%s","1-10","11-20","21-30",
                                "31-40","41-50","51-60","61-70","71-80","81-90","91-100");
    }
}

Output:

Enter amount of random numbers to generate: 45

 3      5      8      2      4      7      3      4      5      4
               *                                                       
               *                    *                                  
               *                    *                                  
        *      *                    *                    *             
        *      *             *      *             *      *      *      
 *      *      *             *      *      *      *      *      *      
 *      *      *      *      *      *      *      *      *      *      
 *      *      *      *      *      *      *      *      *      *      
1-10  11-20  21-30  31-40  41-50  51-60  61-70  71-80  81-90  91-100
\$\endgroup\$

3 Answers 3

Reset to default
5
\$\begingroup\$

janos has given you some nice hints about your random, and array indexing. I believe you should take it further, though.

Specifically, your system should be parameterized. I would expect there to be three parameters:

  1. lowest number in random range
  2. largest number in the range
  3. size of the 'buckets' to partition the data in to.
  4. the number of samples to compute.

In your example above, it would be histogram(1, 100, 10);

That method would be generalized as:

public int[] histogram(final int min, final int max, final int bucket, final int count) {
    // note, in this method, there's no need to know the actual values, just the range.
    final int range = max - min + 1;
    final int buckets = (range + bucket - 1) / bucket;
    final int[] results = new int[buckets];
    final Random rand = new Random();
    for (int i = 0; i < count; i++) {
        int value = rand.nextInt(range);
        results[value / bucket]++;
    }
    return results;
}

Then, with that method, we just need to display the results with the correct offsets.

public void displayHistogram(int first, int bucketsize, int[] buckets) {
    ....
}

With that method, the main method is greatly simplified as:

public static void main(String...args) {
    int min = 1;
    int max = 100;
    int bsize = 10;
    int[] buckets = histogram(min, max, bsize, 100);
    displayHistogram(min, msize, buckets);
}

By doing things this way we have separated the model the view, and the controller. This is a primitive form of abstraction that is used in many GUI applications too. In essence, though, it is just a system for placing single functional components in single functions. Having multiple functions in a single method leads to maintenance problems.

The question then becomes a problem of how to display things.

Your display system has four functional issues:

  1. you are limited in the height of the histogram, there is no way to configure a sense of scale
  2. you have hard-coded the number of buckets to display
  3. you have a hard limit on the number of values you support. A result with more than 10 asterisks in a single bucket is quite valid, but it only supports at most 10.
  4. you have used a vertical bar for the histogram. This is OK, but in text consoles, the horizontal axis is typically more accommodating. Additionally, the horizontal axes would be easier to implement...

I sort of messed with this, and put it together as a horizontal display. This is what I got:

public static void displayHistogram(final int first, final int bucketsize, final int maxbar, final int[] buckets) {
    int maxcount = Arrays.stream(buckets).max().getAsInt();
    int scale = ((maxcount + maxbar - 1) / maxbar) + 1;
    int width = (maxcount + scale - 1) / scale;
    String format = "%5d-%-5d | %-" + width + "s | %d\n";
    for (int i = 0; i < buckets.length; i++) {
        int start = (bucketsize * i) + first;
        int end = start + bucketsize - 1;
        System.out.printf(format, start, end, buildAsterisks(buckets[i], scale), buckets[i]);
    }


    System.out.printf("%11s | %s\n", "", lines(width));
    System.out.printf("%-11s | %s\n", "Scale", marks(maxcount, scale));

}    

private static Object marks(int maxcount, int scale) {
    int width = (int)Math.log10(maxcount - 1) + 2;
    int count = ((maxcount + scale - 1) / scale) / width;
    StringBuilder sb = new StringBuilder();
    int mark = 0;
    int step = scale * width;
    for (int i = 0; i < count; i++) {
        mark += step;
        sb.append(String.format("%" + width + "d", mark));
    }
    return sb.toString();
}

private static String lines(int width) {
    char[] array = new char[width];
    Arrays.fill(array, '-');
    return new String(array);
}

private static String buildAsterisks(int size, int scale) {
    char[] array = new char[(size + scale - 1) / scale];
    Arrays.fill(array, '*');
    return new String(array);
}

The nice thing is that you can then run it with much larger counts, like 1,000,000

I ave put it all in an ideone here... enjoy!.

This is what the results look like:

    1-10    | *********************         | 84
   11-20    | **************************    | 102
   21-30    | ***************************** | 114
   31-40    | *************************     | 100
   41-50    | *************************     | 100
   51-60    | ************************      | 95
   61-70    | ************************      | 94
   71-80    | ***************************** | 113
   81-90    | ***********************       | 92
   91-100   | ***************************   | 106
            | -----------------------------
Scale       |   16  32  48  64  80  96 112
\$\endgroup\$
2
  • \$\begingroup\$ What's the parametrization good for? Don't you want the values equiprobable? I can see that the OP speaks about "number in a certain range", but IMHO you should either generate the real values and bucketize them later OR forget it and generate the distribution directly. \$\endgroup\$ Commented Jan 17, 2015 at 0:08
  • 1
    \$\begingroup\$ @maaartinus - interesting observation. I simply assumed the random source was an arbitrary source. The real code is about the buckets and histogram. I agree that your answer simplifies the production of the results, but loses some of the important steps too. In a sense, there's too much 'toyness' about the basic question to resolve the differences we see. Frankly, my answer could equally be a question, it's something I just decided I wanted to do differently. \$\endgroup\$ Commented Jan 17, 2015 at 1:52
6
\$\begingroup\$

You can simplify the tedious else-ifs in generateRandomNumbers like this:

public static int[] generateRandomNumbers(int amount) {
    int ranges[] = new int[10];
    for (int i = 0; i < amount; i++) {
        int num = (int)(Math.random() * 100) + 1;
        ranges[Math.min(9, num / 10)]++;
    }
    return ranges;
}

Getting random numbers with Math.random is a bit tedious. A more ergonomic way is using Random.nextInt, for example:

    Random random = new Random();
    // ...
    int num = random.nextInt(100) + 1;

Also, it is recommended to use braces with even single line statements, for example:

public static int largestValueInArray(int ranges[]) {
    int largest = ranges[0];
    for (int i = 1; i < ranges.length; i++) {
        if (ranges[i] > largest) {
            largest = ranges[i];
        }
    }
    return largest;
}
\$\endgroup\$
0
5
\$\begingroup\$

Janos' answer improves the random generator to something like

public static int[] generateRandomNumbers(int amount) {
    int ranges[] = new int[10];
    for (int i = 0; i < amount; i++) {
        int num = random.nextInt(100) + 1;
        ranges[Math.min(9, num / 10)]++;
    }
    return ranges;
}

but this is still far from sane. Without looking at how num gets exactly generated, we can say that Math.min(9, num / 10) either skews the result in favor of ranges[9] or it does nothing at all. Just kick it out.

Another thing is using a needlessly big range and then dividing num by 10. Again, this may (or may not) skew the result for no gain. This trivial line

ranges[random.nextInt(ranges.length)]++;

is way simpler and obviously correct.

Getting random numbers with Math.random is a bit tedious.

This is an understatement. It's tedious and in 99.99% cases wrong. When an int is required, it means that two random integers get generated, from them a double is obtained, which finally gets converted to an int. Most people fail to get it uniform and many of them even fail to obtain the desired interval. Note that Random.nextInt(int) does it all and guarantees uniformity.

I see I overlooked the OP mentioning the buckets - what I wrote applies to uniform random number generation. But it still applies: Use the above for generating the values and add a method bucketize, if that's what's required.

\$\endgroup\$
0

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.