Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This is the example code for the next project for my intro class. The assignment is to add an "edit method" to this program. Some of my concerns are why the methods are returning Integers, that seems unnecessary. Also the use of wrapper classes instead of primitives also seems to be incorrect. Im really thinking that my professor writes confusing sloppy code, am I wrong?

   import java.util.Scanner;
   import java.util.InputMismatchException;
   import java.io.File;
   import java.io.FileNotFoundException;
   import java.io.PrintWriter;

/** 
 *  Implements a Grocery List
 *   Version 4: Has code for all methods (final version)
 *  @author xxxxxxx
 */
    public class GroceryListProgram4 {
    /**
     * The main() Method Starts The Program.
     * @param commandlineArguments 1st argument is INPUT File, 
     *                             2nd argument is OUTPUT File 
     */  
       public static void main(String[] commandlineArguments) { 
        //Error Checking For 2 Command Line Arguments..
         if(commandlineArguments.length != 2){
            System.out.println("Please enter the INPUT file name as " 
                               + "the 1st commandline argument.");
            System.out.println("Please enter the OUTPUT file name as " 
                               + "the 2nd commandline argument.");
            System.out.println("Please enter exactly " 
                               + "two (2) commandline arguments.");                      
         }//end of if
         //if no commandline argument errors, continue program
         else{
         //Declare and instantiate array of 100 Strings
            final Integer MAX_SIZE = new Integer(100);
            String groceryList[] = new String[MAX_SIZE];      
         //Set size of grocery list to zero (0) items
            Integer size = new Integer(0);          
         //read grocery items from file & store in array for grocery list
            try{
               size = GroceryListProgram4.readFromFile(commandlineArguments[0], groceryList, size);
            }
                catch(ArrayIndexOutOfBoundsException exception){
                  System.out.print("ERROR: Too many items in input file. ");
                  System.out.println("Please limit to " + MAX_SIZE + " items.");
                  //Immediately terminates program
                  System.exit(1);
               }

         //user's choice for Menu
            Integer choice = new Integer(0); 
         //choice for ending program
            final Integer QUIT = new Integer(4);
         //if the user does not want to QUIT, keep looping
            while(!choice.equals(QUIT)){
            //get the user's choice
               choice = GroceryListProgram4.displayMenu();
            //add grocery item   
               if(choice.equals(1)){
                  size = GroceryListProgram4.add(groceryList, size);
               }
               //delete grocery item
               else if(choice.equals(2)){
                  size = GroceryListProgram4.delete(groceryList, size);
               }
               //display grocery item
               else if(choice.equals(3)){
                  GroceryListProgram4.display(groceryList, size);
               }
               //error message
               else if(!choice.equals(QUIT)){
                  System.out.println("ERROR: Please enter an integer in the range from 1 to 4");
               }
            }//end of "while"
            //write to from grocery list array to OUTPUT file
            GroceryListProgram4.writeToFile(commandlineArguments[1], groceryList, size);
         }//end of "else" 
      }//end of main() method   

   /**
     * Displays the menu for the program and returns user's choice
     * @return the choice of the user (if error, returns 0)   
     */
       public static Integer displayMenu(){
       //display menu
         System.out.println();
         System.out.println("\tGROCERY LIST MENU");
         System.out.println("\t Enter 1 to Add");
         System.out.println("\t Enter 2 to Delete");
         System.out.println("\t Enter 3 to Display");
         System.out.println("\t Enter 4 to Quit");
         System.out.print("\tEnter your choice: ");
       //get input from user
         Scanner keyboardInput = new Scanner(System.in);
         Integer choiceOfUser = new Integer(0);
         try{
         //non-integer input will throw InputMismatchException
            choiceOfUser = keyboardInput.nextInt();
         }
             catch(InputMismatchException exception){
            //Already have error message in main() method,
            //as choiceOfUser = 0
            }
         System.out.println();
         return choiceOfUser;
      }

   /**
     * Reads grocery items from a file and stores items in an array
     * @param inputFile is the INPUT File 
     * @param array is the Grocery List array
     * @param size is the number of items in Grocery List
     * @return the new size of the grocery list    
     * @throws ArrayIndexOutOfBoundsException if array size is less than number of items in input file                         
     */
       public static Integer readFromFile(String inputFile, String [] array, Integer size) throws ArrayIndexOutOfBoundsException{
         //connect to file (does NOT create new file)
         File file = new File(inputFile);
         Scanner scanFile = null;
         try {
            scanFile = new Scanner(file);
         } 
             catch (FileNotFoundException exception) {
               //Print error message.
               //In order to print double quotes("), 
               //the escape sequence for double quotes (\") must be used.
               System.out.print("ERROR: File not found for \"");
               System.out.println(inputFile+"\"");
            }        
         //if made connection to file, read from file
         if(scanFile != null){
            //keeps looping if file has more lines..
            while (scanFile.hasNextLine()) {
               //get a line of text..
               String line = scanFile.nextLine();
               //display a line of text to screen..
               //System.out.println(line);
               //array[0] stores the first row (headings) to table
               array[size]=line;
                //increment size
               ++size;
            }
         //In order to print double quotes("), 
         //the escape sequence for double quotes (\") must be used.             
            System.out.println("Read from file \"" + inputFile + "\"");
         }//end of "if" for connecting to file
         return size;
      }

   /**
   * Adds a grocery item to an array
   * @param list is the grocery list 
   * @param listSize is the size of the grocery list 
   * @return new size of the grocery list                             
   */
       public static Integer add(String [] list, Integer listSize){
      //get item from user
         Scanner keyboard = new Scanner(System.in);
         System.out.print("Enter name of item: ");
         String name = keyboard.nextLine();
         System.out.print("Enter number of items: ");
         String number = keyboard.nextLine();         
      //add to the end of the array
         list[listSize] = name + ", " + number;
      //add one to the size (one item to end of list)
         return listSize + 1;
      }

   /**
   * Deletes a grocery item from an array
   * @param list is the grocery list 
   * @param listSize is the size of the grocery list 
   * @return new size of the grocery list                             
   */
       public static Integer delete(String [] list, Integer listSize){
       //get user input
         System.out.print("Enter the row number of the item you wish to delete: ");
         Scanner keyboard = new Scanner(System.in);
         try{
         //throws an exception if not an integer
            Integer row = keyboard.nextInt();
         //check for negative integers
            if(row <= 0){
               System.out.println("ERROR: The integer cannot be negative or zero.");
            }
            //check for integer too big
            else if(row > listSize-1){
               System.out.println("ERROR: The integer is too big for the list.");
            }
            else{
            //delete item by shifting items on the right of the item to the left
               for(int i=row;i<listSize;i++){
                  list[i] = list[i+1];
               }
            //subtract one from the size (one item deleted from list)
               --listSize;
            }
         }
             catch(InputMismatchException exception){
               System.out.println("ERROR: You must enter an integer to delete an item.");
            }
         return listSize ;
      }

   /**
   * Displays a grocery list
   * @param list is the grocery list 
   * @param listSize is the size of the grocery list                   
   */
       public static void display(String [] list, Integer listSize){
       //loop through the array
         for(int i=0;i<listSize;i++){
         //display headings
            if(i==0){
               System.out.println("Row  " + list[i]);
            }
            //display grocery list items as a numbered list
            else{
               System.out.println(i + ".   " + list[i]);    
            }
         }
      } 

   /**
   * Write grocery list array to file
   * @param list is the grocery list 
   * @param listSize is the size of the grocery list                   
   */
       public static void writeToFile(String outputFile, String [] list, Integer listSize){
      //"PrintWriter" is a Class Used To Write to A File.
         PrintWriter fileWriter = null;
         try {
            /*
            Must use try-catch block,
            because PrintWriter may throw FileNotFoundException,
            which is a checked exception.
            This will connect to a file in the current directory.
            If the file does not exists, a new file will be created.
            If the file does exists, the file will be overwritten.
            */
            fileWriter = new PrintWriter(outputFile);
         } 
             catch (FileNotFoundException exception) {
               //Print error message.
               //In order to print double quotes("), 
               //the escape sequence for double quotes (\") must be used.
               System.out.print("ERROR: File not found for \"");
               System.out.println(outputFile+"\"");
            }
         //if file opened successfuly, then below code executes..
         //continue program if writeToFile is not "null"
         if(fileWriter != null){
         //loop through list (grocery list) and write to file
            for(int i=0;i<listSize;i++){
               fileWriter.println(list[i]);
            }
            //REMEMBER: always give feedback to the user!
            System.out.println("Wrote to file \"" + outputFile + "\"");
            //WARNING: don't forget to close the file!
            //will not write to file if not closed!
            fileWriter.close();
         }//end of "if" statement for writeToFile

      }     

   }//end of class
share|improve this question
1  
Code with comments like //end of if makes me cry. –  Corbin Jan 14 '13 at 23:32
    
If your beef is with your prof, settle it with your prof. I'm not interested in giving you fuel to heckle your prof about his style. –  Eva Jan 16 '13 at 10:46
    
This question is closed as off-topic because you didn't write the code. With that said, you're not crazy; as the answers have pointed out, there are several things to improve on. But unless it's hindering your ability to do the assignment, I wouldn't confront the professor about it. Maybe mention it in your end-of-class review, if you have those. –  seand Jan 16 '13 at 17:07
add comment

closed as off topic by Glenn Rogers, Brian Reichle, Jeff Vanzella, Eva, seand Jan 16 '13 at 16:57

Questions on Code Review Stack Exchange are expected to relate to code review request within the scope defined by the community. Consider editing the question or leaving comments for improvement if you believe the question can be reworded to fit within the scope. Read more about reopening questions here.If this question can be reworded to fit the rules in the help center, please edit the question.

3 Answers

For educational purpose, this code is ok. As any code which is for educational purpose. Because it always depends what you want to show. As already guessed, if the goal is to show the possibility of dealing with objects only, then it looks mostly ok (I would avoid arrays, too).
For educational purpose, the comments are ok, too. In a "real" application, such amount of comments would be an indicator that something is wrong there. And there are some more small flaws, which do not count here.
You should not take this code as best practice for coding style. You should take this code as an extensive help for the first steps in programming.

I assume, you do not want this code fully investigated, so I will spare this part. The rest is already answered by Lim H.

share|improve this answer
add comment
  1. The use of wrapper classes instead of primitives are not incorrect. I think your professor is just trying to show you that everything in Java can be object *almost" like the brochure says. Besides, primitives are rumored to be discarded, but don't quote me on it.

  2. The methods return Integer because the program is designed that way. One method controls the program's flow and the others have no side effect. Better than keeping a bunch of global variables for user's choices.

Why do you think it's sloppy?

share|improve this answer
    
If OP didn't modify the whitespace of the original code, then it's very sloppy. Proper alignment makes things much neater, as does vertical spacing (which this code lacks). There's also a lack of consistency to the style; for example, the strings inside the first few System.out.println statements inside main are set across two lines as if to meet some kind of code width requirement, but other code below that goes far beyond the width the strings would have had all on one line. –  seand Jan 16 '13 at 17:23
add comment

If this is all code that your professor wrote, I think this is a classic example of "Those who can't do, teach."

/** 
 *  Implements a Grocery List
 *   Version 4: Has code for all methods (final version)
 *  @author xxxxxxx
 */
public class GroceryListProgram4 {

There's a @version Javadoc tag.

/**
 * The main() Method Starts The Program.
 * @param commandlineArguments 1st argument is INPUT File, 
 *                             2nd argument is OUTPUT File 
 */  
public static void main(String[] commandlineArguments) { 

First thing - this method is way too long. The main method should do little more than instantiate needed objects, probably something that implements Runnable, then exit. Often times, it's best to put this method inside it's own class, that exists for this sole purpose. The name of the argument (commandlineArguments - that l should be capitalized, as line is a new word) is a bit weird - I think I'd have been fine with the Eclipse-standard args.

//Error Checking For 2 Command Line Arguments..
if(commandlineArguments.length != 2){

He's attempting to make sure there's two arguments, which is nice. Unfortunately:
1) There's no verification to see if commandlineArguments is null (I'm not sure what happens if no arguments are given, so it might not actually be an issue).
2) There's no verification of if the arguments aren't null, or otherwise invalid. Although I have to admit, it'd be hard to pass in a null argument...

 System.out.println("Please enter the INPUT file name as the 1st commandline argument.");
 System.out.println("Please enter the OUTPUT file name as the 2nd commandline argument.");
 System.out.println("Please enter exactly two (2) commandline arguments.");   

Really, you should never use the console directly; instead, use input/output streams that were 'injected' into the running code. Note that it's perfectly fine to use the streams that come from the console (System.out and System.in) as your injected resource, just don't call them in the code that's doing the writing.

}//end of if

These types of comments should just die. These are some of the fastest comments to become out of date. With a modern editor (like Eclipse), the pairs are automatically and dynamically highlighted, meaning they're pointless.

//if no commandline argument errors, continue program
else{

I'm unsure whether to dock him for 'worthless' comments, or to assume they were placed for program explanation for you and your fellow students. In the real world, comments that look like this are worthless.

//Declare and instantiate array of 100 Strings
final Integer MAX_SIZE = new Integer(100);

The comment obviously belongs to the next line, but just looks out of place. For anything that's supposed to be a value object, especially out of java.lang., never use the constructor - use getXxxx(...) or valueOf(...) which will often return a cached object (eg the references will be equal, not just the value of the object). And why isn't MAX_SIZE a class constant, instead of being created as a local variable?

String groceryList[] = new String[MAX_SIZE];   

So.... MAX_SIZE is a constant, but the groceryList[] reference is not? Even though he never modifies the reference? Also, never use type prefixes or suffixes - groceries would be better. I'll forgive him for using Strings for this, but not for using an array (an implementation of List would be better).

//Set size of grocery list to zero (0) items
Integer size = new Integer(0);   

.... Right....

//read grocery items from file & store in array for grocery list
try{
     size = GroceryListProgram4.readFromFile(commandlineArguments[0], groceryList, size);

readFromFile is part of this program, he doesn't need the static reference. Normally I'd assume from passing in groceryList that he's attempting to buffer the file, but there's not a good reason for that here. He passes in size.... which is completely pointless (especially because the method doesn't use it). Personally, I'd prefer a method where I pass in the file name, and get back a collection of all items.

} catch(ArrayIndexOutOfBoundsException exception){

Catching a specific error is good. The standard is to name the variable e, though.

    System.out.print("ERROR: Too many items in input file. ");
    System.out.println("Please limit to " + MAX_SIZE + " items.");

At least he's using the same constant.

    //Immediately terminates program
    System.exit(1);
}

Never call System.exit(...) directly, just keep hopping back up the stack until the program itself exits. System.exit(...) terminates the entire JVM instance, which may include other programs.

//user's choice for Menu
Integer choice = new Integer(0); 

This should have been null - the user hasn't made a choice yet. Requires some modifications to the rest of the program, though.

//choice for ending program
final Integer QUIT = new Integer(4);

Same critique as for MAX_SIZE.

while(!choice.equals(QUIT)){

Works, although would need to be changed if choice was null.

//get the user's choice
choice = GroceryListProgram4.displayMenu();

Violates get concept known as command-query separation, as well as good naming. Why does simply displaying a menu return the choice?

if(choice.equals(1)){
    size = GroceryListProgram4.add(groceryList, size);
}

He's got a constant for quitting, why not constants for anything else (hint, there should be).

if(choice.equals(1)){
    ...
} else if(choice.equals(2)){
    ...
} else if(choice.equals(3)){
    ...
} else if(!choice.equals(QUIT)){
    System.out.println("ERROR: Please enter an integer in the range from 1 to 4");
}

Good news, he's checking for the 'default' or 'error' condition. Bad news: the continuing positive checks may make it hard to parse the opposite conditional at the end. While an error message of some sort is needed, doing it this way just causes potential headaches. The error message is terrible (how many people know what an 'integer' is, as opposed to a number?). It's also the wrong place to put the limits - if you add an option to the menu, you have two places to modify; the menu, and this code.

GroceryListProgram4.writeToFile(commandlineArguments[1], groceryList, size);

Inputting size like that is going to be interesting. Personally, I'd rather just take the complete O(n) hit, and either stop when the next argument was null (or blank), or just silently ignore empty/null lines.

/**
 * Displays the menu for the program and returns user's choice
 * @return the choice of the user (if error, returns 0)   
 */
public static Integer displayMenu(){

The only reason to have anything public is that something else depends on it being exposed. That isn't necessary here.

//display menu
System.out.println();
System.out.println("\tGROCERY LIST MENU");
System.out.println("\t Enter 1 to Add");
System.out.println("\t Enter 2 to Delete");
System.out.println("\t Enter 3 to Display");
System.out.println("\t Enter 4 to Quit");
System.out.print("\tEnter your choice: ");

I... mislike doing menus displays like this. There's not necessarily a good, easy solution, although I've been debating about a simple framework for a while. Works for this, though (although, again, should be using a standard writer instead of to the console directly).

//get input from user
Scanner keyboardInput = new Scanner(System.in);

He keeps creating a new Scanner every time this (and any of the other methods) are entered. Expensive, and unnecessary. Instead of just injecting the input stream, the Scanner could have been injected instead.

Integer choiceOfUser = new Integer(0);
try{
    //non-integer input will throw InputMismatchException
    choiceOfUser = keyboardInput.nextInt();
}

This is somewhat disingenuous, and is probably related to 'single-entry, single-exit' thinking. While there is such a thing as too many exit points, small methods can't have more than a couple by virtue of being short. This just smacks of a forced paradigm. Plus, that's going to be a different Integer each time.

catch(InputMismatchException exception){
    //Already have error message in main() method,
    //as choiceOfUser = 0
}

So... he's admitting that the error message is in the wrong place?

/**
 * Reads grocery items from a file and stores items in an array
 * @param inputFile is the INPUT File 
 * @param array is the Grocery List array
 * @param size is the number of items in Grocery List
 * @return the new size of the grocery list    
 * @throws ArrayIndexOutOfBoundsException if array size is less than number of items in input file                         
 */
  public static Integer readFromFile(String inputFile, String [] array, Integer size) throws ArrayIndexOutOfBoundsException{

The only well-named variable is size, which is neither necessary, nor used. inputFile is really the file name. array is really the grocery list buffer. The return comment is misleading - it's supposed to be returning the (new) number of elements in array. Oh, and parameters should always be marked final, because from a caller's perspective, the parameter reference is always constant (will break stuff here, though).

//connect to file (does NOT create new file)
File file = new File(inputFile);

... heh heh heh ... Wonder how many students that line actually confused, instead of clarifying like it was meant to. The comment shouldn't be necessary, but it's attempting to refer to the fact that a new file will not be created on the OS filesystem (eg in Windows).

Scanner scanFile = null;
try {
    scanFile = new Scanner(file);
} 

This type of setup is begging for trouble, given how it's later used. Your prof would be better off just declaring it in the try block, and using it there.

catch (FileNotFoundException exception) {
    //Print error message.
    //In order to print double quotes("), 
    //the escape sequence for double quotes (\") must be used.
    System.out.print("ERROR: File not found for \"");
    System.out.println(inputFile+"\"");
}     

... I'm unsure why one call to System.out.println(...) wasn't used, like he does later.

//display a line of text to screen..
//System.out.println(line);

Not any more it doesn't! Out of date comment!

//array[0] stores the first row (headings) to table
array[size]=line;

Who cares? This is unimportant here.

//increment size
++size;

Two problems here:
1) This should really be a local variable, not something passed in (it's assumed to be 0, which could end up breaking in all sorts of fun ways).
2) It's using the object wrapper, at the moment. If you've heard anything bad about string concatenation in a loop.... it applies here as well.

/**
 * Adds a grocery item to an array
 * @param list is the grocery list 
 * @param listSize is the size of the grocery list 
 * @return new size of the grocery list                             
 */
public static Integer add(String [] list, Integer listSize){
    //get item from user
    Scanner keyboard = new Scanner(System.in);
    System.out.print("Enter name of item: ");
    String name = keyboard.nextLine();
    System.out.print("Enter number of items: ");
    String number = keyboard.nextLine();         
    //add to the end of the array
    list[listSize] = name + ", " + number;
    //add one to the size (one item to end of list)
    return listSize + 1;
}

Biggest (new) problem here is that he isn't checking for the size of the list, so you get to have fun with ArrayIndexOutOfBoundsExceptions that he never traps for.

//delete item by shifting items on the right of the item to the left
for(int i=row;i<listSize;i++){
   list[i] = list[i+1];
}

If listSize is equal to MAX_SIZE, this bombs when you attempt to remove an item.

Nothing new for the display method.

/**
 * Write grocery list array to file
 * @param list is the grocery list 
 * @param listSize is the size of the grocery list                   
 */
public static void writeToFile(String outputFile, String [] list, Integer listSize){

He's missing a parameter in the Javadoc.

fileWriter.close();

This has the potential to throw an IOException, but he never traps it.

share|improve this answer
add comment

Not the answer you're looking for? Browse other questions tagged or ask your own question.