2
\$\begingroup\$

My project was to compare the frequency of occurrences of lottery numbers based on a given text file. I started by reading the file line by line, then extracting the numbers, and finally comparing the frequency of numbers. I'm looking for all types of criticism!

/**
 * Author: Brandon F.
 * Purpose: Compares lottery numbers to see which are the most frequent and least frequent
 * Date: 4/25/2017
 * Status: Works while comparing one most frequent and one least frequent, but will not work
 *         if trying to compare two most frequents and two least frequents at the same time.
 */

package imDone;

import java.io.IOException;

public class TheDriver {
    public static void main(String args[]) throws IOException {

        // starts by allocating memory to a string of where the text
        // file is (should be in same file as bin and src directories are)
        String fileName = "DownloadAllNumbers.txt";

        // creates two arrays to keep track of all possible numbers in the
        // lottery
        int[] gg = new int[47];
        int[] megaNums = new int[27];

        // reads and records all of the lottery numbers
        try {
            Reader file = new Reader(fileName);
            String[] aryLines = file.OpenFile();

            // starts at line 5 because previous lines are irrelevent
            // information
            for (int i = 5; i < aryLines.length; i++) {
                // adds one to each slow in the array by reading between the
                // substring and trimming any extra whitespace
                gg[Integer.parseInt(aryLines[i].substring(36, 38).trim()) - 1]++;
                gg[Integer.parseInt(aryLines[i].substring(46, 50).trim()) - 1]++;
                gg[Integer.parseInt(aryLines[i].substring(58, 62).trim()) - 1]++;
                gg[Integer.parseInt(aryLines[i].substring(69, 74).trim()) - 1]++;
                gg[Integer.parseInt(aryLines[i].substring(80, 86).trim()) - 1]++;
                // adds the last digit to mega number array
                megaNums[Integer.parseInt(aryLines[i].substring(90).trim()) - 1]++;
            }
        } catch (IOException e) {
            System.out.println(e.getMessage());
        }

        System.out.println("Top 5 most frequent NUMBERS");
        getHotNumbers(gg);

        // System.out.println("Top 5 most frequent MEGA-NUMBERS");
        // getHotNumbers(megaNums);

        System.out.println("Least fequent MEGA-NUMBERS");
        getColdNumbers(megaNums);

        // System.out.println("Least fequent NUMBERS");
        // getColdNumbers(gg);

    } // end of main

    // gets the most frequent numbers in an array
    public static void getHotNumbers(int[] noRe) {

        // hotNums used for actual number, hotTime used for frequency
        int[] hotNums = new int[5];
        int[] hotTime = new int[5];

        // easy to beat number to compare for largest number
        int compare = 0;

        // loops through 5 times setting the greatest number to 0 each time
        for (int j = 0; j < 5; j++) {
            int largest = 0;
            compare = 0;
            for (int i = 0; i < noRe.length; i++) {
                // if compare is less than current number replace compare
                if (compare < noRe[i]) {
                    compare = noRe[i];
                    largest = i;
                }
            }
            // sets largest number in array to 0 to begin looking for next
            // largest, and adds largest numbers to array list
            noRe[largest] = 0;
            hotNums[j] = largest;
            hotTime[j] = compare;
        }

        // prints out largest numbers
        for (int i = 0; i < 5; i++) {
            System.out.println((i + 1) + ") " + (hotNums[i] + 1) + " - " + hotTime[i] + " times");
            // checks to see if the next number is tied with current number
            if ((i < 4) && hotTime[i] == hotTime[i + 1]) {
                System.out.println(
                        "   " + (hotNums[i + 1] + 1) + " - " + hotTime[i] + " times. (tied with pervious occurance)");
                i++;
            }
        }
        System.out.println();
    } // end of getHotNumbers

    // refer to getHotNumbers (it's the exact same except comparing lowest
    // numbers).
    public static void getColdNumbers(int[] noRe) {
        int[] hotNums = new int[5];
        int[] hotTime = new int[5];

        int compare = 200;

        for (int j = 0; j < 5; j++) {
            int smallest = 200;
            compare = 200;
            for (int i = 0; i < noRe.length; i++) {
                if (compare > noRe[i]) {
                    compare = noRe[i];
                    smallest = i;
                }
            }
            noRe[smallest] = 200;
            hotNums[j] = smallest;
            hotTime[j] = compare;
        }

        for (int i = 0; i < 5; i++) {
            System.out.println((i + 1) + ") " + (hotNums[i] + 1) + " - " + hotTime[i] + " times");
            if ((i < 4) && hotTime[i] == hotTime[i + 1]) {
                System.out.println(
                        "   " + (hotNums[i + 1] + 1) + " - " + hotTime[i] + " times. (tied with pervious occurance)");
                i++;
            }
        }
        System.out.println();
    } // end of getHotNumbers

} // end of class

Picture of what the text file looks like:

enter image description here

Output:

enter image description here

The only thing you may be noticing missing is my Reader class, but it's a very simple bit of code to read text files.

\$\endgroup\$
1
  • \$\begingroup\$ For your next question, please add input and output as text instead of as picture. This allows us to test your code easily. Everything you do to make it easier on the reviewers can lead to better quality answers. \$\endgroup\$ – Mast May 1 '17 at 17:04
3
\$\begingroup\$

First I would argue that this is actually broken code (and thus against the rules of cr.se), because your solution can only find either the most frequent, or the least frequent, not both. It even can't find the most frequent when you call the method a second time.

The reason is obvious. You modify your input array inside the method. The quickfix would be to copy the input array and then work with that copy instead:

 public static void getHotNumbers(int[] noRe) {
    int[] input = new int[noRe.length];
    System.arraycopy(noRe, 0, input, 0, noRe.length);

If we then use input instead of noRe the original doesn't change.


The algorithm for finding the 5 most/least frequent can also be improved a lot.

Instead of going through the entire list 5 times, each time finding the largest element. You should only go through it once, keeping track of the 5 largest elements you've encountered and keep those 5 sorted.

I also want to point out that a function called getSomething() should return that something. Since we can only return one object but want to return both the number and it's frequency, let's use a specialised class:

public class Frequency {
    private final int number;
    private final int frequency;

    public Frequency(int number, int frequency) {
        this.number = number;
        this.frequency = frequency;
    }

    public int getNumber() {
        return number;
    }

    public int getFrequency() {
        return frequency;
    }
}

Now let's look at the algorithm we're going to use:

loop(all elements) {
    if(element should be in top 5) {
        remove the unwanted element
        add element to result list
        sort result list
    }
}

The first 5 we look at are always "wanted". So we should start with by creating the list of frequencies, add those 5 and then sort the list:

private static List<Frequency> getHotNumbers(int[] noRe) {
    List<Frequency> result = new ArrayList<>();
    for(int i = 0; i < 5 && i < noRe.length; i++){
        result.add(new Frequency(i, noRe[i]));
    }
    result.sort(comparator);

The sort needs a comparator here. Since both cold and hot numbers will use the same algorithm, let's pass in a comparator as a parameter for our method so we can re-use it easily.

private static List<Frequency> getTopFrequencies(int[] noRe,
            Comparator<Frequency> comparator) {

Now that we got that sorted out, here's the full method implementation:

private static List<Frequency> getTopFrequencies(int[] noRe,
            Comparator<Frequency> comparator) {
    List<Frequency> result = new ArrayList<>();
    for(int i = 0; i < 5 && i < noRe.length; i++){
        result.add(new Frequency(i, noRe[i]));
    }
    result.sort(comparator);
    for(int i = 5; i < noRe.length; i++){
        final Frequency nextFreq = new Frequency(i, noRe[i]);
        if (comparator.compare(nextFreq,result.get(4)) < 1) {
            result.remove(4);
            result.add(nextFreq);
            result.sort(comparator);
        }
    }
    return result;
}

Now that we got our main algorithm implemented, all we need to do is pass in the correct comparator to get things to work.

private static List<Frequency> getHotNumbers(int[] noRe){
    return getTopFrequencies(noRe, (n1, n2) -> Integer.compare(n2.getFrequency(), n1.getFrequency()));
}

private static List<Frequency> getColdNumbers(int[] noRe){
    return getTopFrequencies(noRe, (n1, n2) -> Integer.compare(n1.getFrequency(), n2.getFrequency()));
}

Note that the only difference between the two comparators is the order of n1 and n2.

Now I only have 1 issue with the way I implemented this method. It's the magic numbers 5 and 4. Instead, let's just pass in the requested ammount of numbers as a parameter as well. While we're at it, let's also improve rename noRe to something that makes it clear it's the input frequencies.

private static List<Frequency> getTopNFrequencies(int n, int[] frequencies,
            Comparator<Frequency> comparator) {
    List<Frequency> result = new ArrayList<>();
    for(int i = 0; i < n && i < frequencies.length; i++){
        result.add(new Frequency(i, frequencies[i]));
    }
    result.sort(comparator);
    for(int i = n; i < frequencies.length; i++){
        final Frequency nextFreq = new Frequency(i, frequencies[i]);
        if (comparator.compare(nextFreq,result.get(n-1)) < 1) {
            result.remove(n-1);
            result.add(nextFreq);
            result.sort(comparator);
        }
    }
    return result;
}

To finish my answer, here's some comments on your code style that should be improved:

  • Use names that actually show what the variable is. gg and noRe make me think about the end of an online match instead of numbers and frequencies.

  • Don't just copy paste code. If you didn't know how to use comparators, reusing the algorithm would indeed be hard. But printing the result is just a literal copy-paste instead of a method call:

    private static void printResult(int[] hotNums, int[] hotTime){
        for (int i = 0; i < 5; i++) {
            System.out.println((i + 1) + ") " + (hotNums[i] + 1) + " - " + hotTime[i] + " times");
            if ((i < 4) && hotTime[i] == hotTime[i + 1]) {
                System.out.println(
                    "   " + (hotNums[i + 1] + 1) + " - " + hotTime[i] + " times. (tied with pervious occurance)");
                i++;
            }
        }
        System.out.println();
    }
    

// refer to getHotNumbers (it's the exact same except comparing lowest
// numbers).

you did notice it's the exact same thing you're doing ...
  • you created a specialised class for reading a file ... but don't use it to actually read the file? Why isn't all that code specific to parsing that file inside your specialised class?

  • // gets the most frequent numbers in an array
    public static void getHotNumbers(int[] noRe) {

    Then where are they? this method doesn't get me anything.

  • int compare = 200;

    What is this magic number 200? Why not 10? or 1000? or Integer.MAX_VALUE? Same goes for the numbers 47, 27 and to a lesser extent, the numbers 5 and 4. Most of these should be replaced with constants instead. For example:

    private static final int AMMOUNT_OF_LOTTERY_NUMBERS = 47;
    

    or us a different name, but make it say what the number represents.

  • The //end of methodName comments ... It's usually better to just have shorter methods. If they fit on 1 screen and use correct indentation, you dont' need to tell which method is closed by that bracket. If they don't fit on 1 screen then you should probably extract parts of it to new methods, like for example printing the results.

\$\endgroup\$
1
  • \$\begingroup\$ Sorry for my late response, but thank you very much for all of your advice! I haven't been able to get back to this code yet. However, as soon as I have free time I would love to implement your features and make necessary changes as you have instructed. \$\endgroup\$ – Brandon May 17 '17 at 16:23
2
\$\begingroup\$

my two cents... (no special order)

  • Please don't do stuff like // end of class. It's the same as you would to put // start of class on top. In general, do not write comments, in my opinion, 99% of the time, there is no comment needed. E.g. "creates two arrays to keep track of all possible numbers in the lottery". And on the next two lines you declare two arrays, that's like saying "I'm walking down the street", and then walk down the street. Make the code tell the story, not your comments.

  • int[] gg - good game? Just write, what it is... also: megaNums. There's nothing wrong with writing megaNumbers. Try to avoid abbreviations in general, they can be quite confusing. An abbreviation is 'acceptable' or a 'good thing to do', if it's a common business term and known in the team (= somewhere explained in a glossary), e.g. rcc and scc for receiving and sending cost center.

  • String fileName = "DownloadAllNumbers.txt"; since the variable is only needed once, it's imo perfectly fine, to use the string in the Reader constructor.

  • Reader file = new Reader(fileName); When I read file, I think of java.io.File. That should be made more clear.

  • String[] aryLines = file.OpenFile(); I think it should be openFile. Well, actually, it should be loadContentsOfFile or something like that. And aryLines is quite confusing, too.

  • System.out.println(e.getMessage()); Never ever get rid of a stack trace. It's the difference between We know where the bug is and Well, we have to make a patch release to know what the bug is, sorry for losing that money, customer.

  • About the commented code. Never ever leave commented (and unused code). You have a code repository, if you need it again some day, it will be there.

  • // gets the most frequent numbers in an array public static void getHotNumbers(int[] noRe) { well, again those comments... wouldn't getMostFrequentNumbers be a more suitable name? And again: noRe ... no ragrets? A method has to be self explanatory, in this case, if have to check the caller of that method, what will be passed, to understand this parameter.

  • hotTime used for frequency - so name it like that.

  • you print the numbers within the getHotNumbers method. So, the method name is actually lying, it should be getAndPrintHotNumbers. But that's bad, too, since a method should do one thing. Move the printing to a separate method.

  • In general: You moved the reading of the 'hot' and 'cold' numbers to separate methods, this is good, but you didn't do it for e.g. the loading of the file or the parsing of it. Move that to separate methods too, so the main method will be really short and tells the summary.

Okay, about the algorithm itself:

To be honest, by the given explanation by you, I know what it should do, but I have a hard time to understand the code. See my approach. The inner classes actually should be moved to separate types, the util methods (create random lotto draws), too, the main type would be much more tidy and you don't have to jump around in the class that much, but to copy paste a whole class into your ide is quite convenient. Of course, it's not perfect and it would need a code review, too, but my main goal is to show you, how to tell a story. Or how to make the intention clear. Or at least more clear. It's usually quite clear for the writer, at least for a day...

import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Random;

public class Example {

    private final static Random random = new Random();

    public static void main(String[] args) {
        List<LottoDraw> lottoDraws = createRandomLottoDraws();

        printLottoDraws(lottoDraws);

        List<IntegerCounter> mostFrequentNumbers = getMostFrequentNumbers(lottoDraws);

        printMostFrequentNumbers(mostFrequentNumbers);
    }

    private static List<LottoDraw> createRandomLottoDraws() {
        List<LottoDraw> lottoDraws = new ArrayList<>();
        for (int i = 0; i < 20; i++) {
            lottoDraws.add(createRandomLottoDraw());
        }

        return lottoDraws;
    }

    private static LottoDraw createRandomLottoDraw() {
        int amountOfRandomNumbers = 5;

        int randomNumbers[] = new int[amountOfRandomNumbers];
        for (int i = 0; i < amountOfRandomNumbers; i++) {
            randomNumbers[i] = createRandomLottoNumber();
        }
        return new LottoDraw(randomNumbers);
    }

    private static int createRandomLottoNumber() {
        // this is bad, 44 and +1 is not clear
        return random.nextInt(44) + 1;
    }

    private static void printLottoDraws(List<LottoDraw> lottoDraws) {
        for (LottoDraw lottoDraw : lottoDraws) {
            for (int drawnNumber : lottoDraw.getDrawnNumbers()) {
                System.out.print(drawnNumber + " ");
            }
            System.out.print("\n");
        }
    }

    private static List<IntegerCounter> getMostFrequentNumbers(List<LottoDraw> lottoDraws) {
        List<IntegerCounter> allCountedNumbers = countAllNumbers(lottoDraws);

        sortByMostFrequentNumbers(allCountedNumbers);

        // this is bad, size could be 4
        return allCountedNumbers.subList(0, 5);
    }

    private static void printMostFrequentNumbers(List<IntegerCounter> mostFrequentNumbers) {
        for (IntegerCounter mostFrequentNumber : mostFrequentNumbers) {
            System.out.println("Lotto number: "
                    + mostFrequentNumber.getIntegerToCount()
                    + " / Amount of occurences: "
                    + mostFrequentNumber.getCounter());
        }
    }

    private static List<IntegerCounter> countAllNumbers(List<LottoDraw> lottoDraws) {
        Map<Integer, IntegerCounter> counterMap = new HashMap<>();

        for (LottoDraw lottoDraw : lottoDraws) {
            for (int drawnNumber : lottoDraw.getDrawnNumbers()) {
                if (counterMap.get(drawnNumber) == null) {
                    counterMap.put(drawnNumber, new IntegerCounter(drawnNumber));
                }

                counterMap.get(drawnNumber).incrementCounter();
            }
        }

        return new ArrayList<>(counterMap.values());
    }

    private static void sortByMostFrequentNumbers(List<IntegerCounter> numbersToCount) {
        Collections.sort(numbersToCount, new Comparator<IntegerCounter>() {

            @Override
            public int compare(IntegerCounter o1, IntegerCounter o2) {
                return o2.getCounter() - o1.getCounter();
            }
        });
    }

    private static class LottoDraw {

        private final int[] drawnNumbers;

        public LottoDraw(int[] drawnNumbers) {
            this.drawnNumbers = drawnNumbers;
        }

        public int[] getDrawnNumbers() {
            return drawnNumbers;
        }
    }

    private static class IntegerCounter {

        private final int integerToCount;
        private int counter;

        public IntegerCounter(int integerToCount) {
            this.integerToCount = integerToCount;
        }

        public void incrementCounter() {
            counter++;
        }

        public int getIntegerToCount() {
            return integerToCount;
        }

        public int getCounter() {
            return counter;
        }
    }
}
\$\endgroup\$

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.