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.

• Friendly King: To address your second point, this is quite true; however, for my course, I must use this class because it comes from my professor's textbook...and he wants us to use it...Anyway, I appreciate the feedback and will make the necessary changes. You make some very good points. Thanks.
• Simon Forsberg: @JohnZ Send your professor and his textbook over here and we would gladly review him!
• Friendly King: You have no idea how much I would love that. To be honest, considering I've only ever learned Java in his particular class, I'm very interested to know what I'm being taught versus what I should be being taught. Makes me wonder.
• Simon Forsberg: @JohnZ It all depends on how much you're ready to learn at once I think. Whenever you create something you'd like a second opinion on, Code Review is your friend.
• Friendly King: Well for what it's worth, your review was far more helpful than any other feedback I've received in class. I will definitely be using CodeReview more often.