Very basic Context Free Grammar in Java

Question

I wrote this program for an assignment, thus I had some requirements as far as the format of the cfg.txt file and some basic other classes that we had to use. Other than that, I am curious if there is a way to drastically simplify this code. Although the program runs fine and produces the desired output, is there a better way to go about writing this? (What initially spurred my concern was the iterateData method. I did not like the code repetition and thought it may be convenient to "reuse" some of it. However, in doing so, I then introduced numerous class variables to prevent from having to pass in 7 instance variables to the method. I can't say that I'm all too thrilled about this...) Regardless, advice and suggestions appreciated.

Thank you.

import java.util.Scanner; 
import java.io.*;
import java.util.Random;

public class ContextFreeGrammar {

    private static String nt, useExp = "";
    private static int totalWeight = 0, randomNum;
    private static ArrayUnsortedList<Production> data = new ArrayUnsortedList<Production>();
    private static Production pObj;

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

        String result = "<S>";
        File myFile = new File("cfg.txt");
        Scanner fileScan = new Scanner(myFile);

        // Read and process each line of the file
        while (fileScan.hasNext()) {
            Production p = new Production(fileScan.nextLine());
            data.add(p);
        }

        // If the result string still contains non-terminals
        while (result.contains("<")) {

            // Get the non-terminal
            nt = result.substring(result.indexOf('<')+1, result.indexOf('>'));

            // Cannot condense because we have an ambiguous
            // totalWeight. Hence, we need to iterate twice:
            // 1. To generate appropriate random number from totalWeight
            // 2. To determine when the totalWeight exceeds that random number.

            iterateData(false);

            Random rand = new Random(); // Generate a random number object
            randomNum = rand.nextInt(totalWeight - 1) + 1; // Generate actual random number

            // Resets
            totalWeight = 0;
            data.reset();

            iterateData(true);

            // Replace the non-terminal with the expression to use
            result = result.replaceFirst("<" + nt + ">", useExp);
            totalWeight = 0; // Reset totalWeight;
        }

        System.out.println(result);
    }

    public static void iterateData(boolean GETEXP) {
        // Iterate through list again
        for (int i = 0; i < data.size(); i++) {

            // Get the current value
            pObj = data.getNext();

            // Keep track of weight
            if (nt.equalsIgnoreCase(pObj.getNonTerminal())) {
                totalWeight += pObj.getWeight();

                if (GETEXP) {
                    if (totalWeight > randomNum) {
                        useExp = pObj.getExpression(); // This is expression to use
                        break;
                    }
                }
            }
        }
    }
}

My cfg.txt file is as such:

80 <S> = My homework is late because <reason>.
20 <S> = I want to <action> instead of writing this program.
40 <reason> = <who> ate it
40 <reason> = I set it on fire because I was cold
20 <reason> = I didn't feel like doing it
30 <who> = Deep Shah
70 <who> = my pet <animal>
10 <animal> = dog
20 <animal> = narwhal
50 <animal> = python
20 <animal> = velociraptor
30 <action> = go on a vacation
30 <action> = indulge in a giant feast
40 <action> = frolic in the snow

Answer 1

• It is said that "a method should do one thing, and it should do that one thing well". Your iterateData method is being used to do two separate things, depending on the parameter you send it.

• If your ArrayUnsortedList is this class (which really could use a code review itself) then I don't really understand why you are using it. The way I see it is that you could use a regular ArrayList.

• What you are referring to as "class variables" are static variables. It would be better to use them as instance variables. Currently you can't have two ContextFreeGrammar objects at once, each with it's own state. Remove the static keyword and *create an instance of ContextFreeGrammar to properly use it as an instance (which is a lot better practice, although you might not have learned about such things yet. If you haven't learned about it yet, you should ask your teacher about it)

• Only create a new Random object once. Random objects are meant to be re-used. When they are initialized, they get a random seed set depending on the system clock time. Repeatedly creating a Random object gives a loss in randomness.

• Instead of your current iterateData method, consider using one method such as int calculateTotalWeight(List<Production> list, String nonTerminal) and one String determineExpression(List<Production> list, String nonTerminal, int weightRandom). Determining such an important behavior of your method by sending it a true/false value makes your code harder to understand. You don't need to pass seven variables to the method when it only uses two or three.