Code to get user input and write and read from a file

Question

This program is supposed to write a food storage that fills with 2 different items: food name and the amount. It then writes the food items into a text file. Also, it is supposed to read the same text file and load all the contents into an array to be edited again. Do the comments explain enough and are there enough or are there too many? Is the code organized in an efficient and easy to read manner?

public class Storage {
    private Food[] food;
    static final String IN_FILE_NAME = "FoodStorage.txt";
    static final String OUT_FILE_NAME = "FoodStorage.txt";
    public static final int MAX_STORAGE = 100;
    public static final String DELIM = "\t";
    public static final int FIELD_AMT = 2;
    public Storage()
    {
        food = new Food[MAX_STORAGE];
    }
    public Food[] getFood()
    {
        return this.food;
    }
    //Adds more food to the first empty spot in the array food
    public void addFood(Food aF)
    {
        for(int i = 0; i < food.length; i++)
        {
            if(food[i] == null)
            {
                food[i] = aF;
                return;
            }
        }
        //If storage is full
        System.out.println("You storage is full");
    }
    //Removes food from the Storage
    public void removeFood(Food food2)
    {
        for(int i = 0; i < food.length; i++)
        {
            if(food[i] != null && food[i].equals(food2))
            {
                food[i] = null;
                return;
            }
        }
        //If the food searched isn't in the storage
        System.out.println("The food was not found in storage");
    }
    //Prints the different options for manipulating the food array
    public static void printOptions()
    {
        System.out.println("1: Add Food\n2: Remove Food\n3: See current Food\n4: Quit");
    }
    //Returns an instance of a food based on user input
    public static Food makeAFood()
    {
        Food retF;
        System.out.println("Enter the Food's Name");
        //foodName is a string
        String foodName = keyboard.nextLine();
        System.out.println("Enter the amount of food");
        double amount = keyboard.nextDouble();
        keyboard.nextLine();
        retF = new Food(foodName,amount);
        return retF;
    }
    //Prints the storage of food into the current list
    public static void printStorage(Storage aF)
    {
        for(Food f : aF.getFood())
        {
            if(f == null)
                continue;
            System.out.println(f);
        }
    }
    //Reads the txt file for the array
    public static void readFile(String food)
    {
        Scanner scan = null;
        try 
        {
            scan = new Scanner(new File(IN_FILE_NAME));
            BufferedReader in = new BufferedReader(new FileReader(IN_FILE_NAME));
            String str;
            List<String> list = new ArrayList<String>();
            while((str = in.readLine()) != null){
                list.add(str);
            } 
            in.close();
        }
        catch (FileNotFoundException e)
        {
            e.printStackTrace();
        } 
        catch (IOException e) 
        {
            e.printStackTrace();
        }
        StringBuffer sb = new StringBuffer("");
        while(scan.hasNextLine())
        {
            String fileLine = scan.nextLine();
            //Splits the lines at the DELIM Making them different 
            //pieces and adds them to the array split Lines
            String[] splitLines = fileLine.split(DELIM);
            //Takes the first array index and sets it equal
            //to the variable name of the food
            String foodName = splitLines[0];
            if(!foodName.equals("null"))
            {
                System.out.println(fileLine);
                sb.append(fileLine + "\n");
            }
        }
        scan.close();
    }
    //Writes the Storage into the txt file
    public static void writeFile(String OUT_FILE_NAME, Storage aF)
    {
        PrintWriter pw = null;
        try
        {
            //Makes a new PW, BW and FW for the txt file
            pw = new PrintWriter(new BufferedWriter
                (new FileWriter(IN_FILE_NAME)));

        }
        catch(FileNotFoundException e)
        {
            e.printStackTrace();
        }
        catch (IOException e) 
        {
            e.printStackTrace();
        }
        //Reads the current food in storage and prints it
        //into the txt file
        for(Food f : aF.getFood())
        {
            if(f == null)
                continue;
            pw.println(f);
        }
        pw.close();
    }
    static Scanner keyboard;
    //Entry point of the program
    public static void main(String[] args)
    {
        keyboard = new Scanner(System.in);
        Storage S = new Storage();
        boolean quit = false;
        while(!quit)
        {
            System.out.println("Welcome to your food Cabinet");
            //Prints out the 4 different options for the Storage
            printOptions();
            //writes the storage S to the file
            writeFile(OUT_FILE_NAME, S);
            //sets pick to the next user input
            int pick = keyboard.nextInt();
            keyboard.nextLine();
            //The 4 different Options there are
            switch(pick)
            {
            case 1: //Adds Food to storage
                S.addFood(makeAFood());
                break;
            case 2: //Removes Food from storage
                S.removeFood(makeAFood());
                break;
            case 3: //Reads File from the txt file
                readFile(IN_FILE_NAME);
                break;
            case 4: // Quits the Program
                quit = true;
                break;
                default:
                    //for invalid values that are entered
                    System.out.println("Invalid input");
            }
        }
        //If the program is exited this prints
        System.out.println("Goodbye");
    }
}

and this is class food

public class Food 
{
private String foodName;
private double amount;

public Food()
{
    this.foodName = "No Food name yet";
    this.amount = 0.0;
}
//Constructors
public Food(String aFoodName, double amount2) {
    this.foodName = aFoodName;
    this.amount = amount2;
}
//Accessors
public String getFoodName()
{
    return this.foodName;
}
public double getAmount()
{
    return this.amount;
}
//Mutators
public void setFoodName(String aFoodName)
{
    this.foodName = aFoodName;
}
public void setAmount(int anAmount)
{
    if(anAmount > 0)
    {
        this.amount = anAmount;
    }
    else
    {
        System.out.println("That is not a valid amount");
    }
}
public String toString()
{
    return this.foodName + "\t" + amount;
}
public boolean equals(Food aFood)
{
    return aFood != null && 
            this.foodName.equals(aFood.getFoodName()) &&
            this.amount == aFood.getAmount();
}   
}

Hi @Ralph DeFillo, welcome to Code Review stack exchange. I edited slightly your question to fix some typos. Most substantial change: I don't know what you meant with 'etiquette', so I changed it to 'formatting'. Feel free to change to something more appropriate, if you feel the need. — Attilio
– Attilio, Commented Aug 6, 2017 at 18:15

mdfst13 · Accepted Answer · 2017-08-06 19:54:33Z

        Scanner scan = null;
        try 
        {
            scan = new Scanner(new File(IN_FILE_NAME));
            BufferedReader in = new BufferedReader(new FileReader(IN_FILE_NAME));
            String str;
            List<String> list = new ArrayList<String>();
            while((str = in.readLine()) != null){
                list.add(str);
            } 
            in.close();
        }

This is the first of two times that you read the same file.

        List<String> list = new ArrayList<String>();
        try (BufferedReader in = new BufferedReader(new FileReader(IN_FILE_NAME))) 
        {
            for (String str = in.readLine(); str != null; str = in.readLine()) {
                list.add(str);
            } 
        }

Using the try-with-resources form will ensure that file is closed whether you are successful in reading the file or not. The original version only closed the file if successful.

For some reason, you read the entire file and throw away the result. If you would keep the result, then you don't have to read the file again. Moved the list out of the try scope so that it would persist.

Now we don't need scan.

        while(scan.hasNextLine())
        {
            String fileLine = scan.nextLine();
            //Splits the lines at the DELIM Making them different 
            //pieces and adds them to the array split Lines
            String[] splitLines = fileLine.split(DELIM);
            //Takes the first array index and sets it equal
            //to the variable name of the food
            String foodName = splitLines[0];
            if(!foodName.equals("null"))
            {
                System.out.println(fileLine);
                sb.append(fileLine + "\n");
            }
        }
        scan.close();

could be

        final String PREFIX = "null" + DELIM;
        for (String str : list) {
            if (str.startsWith(PREFIX)) {
                System.out.println(str);
                sb.append(str).append("\n");
            }
        }

We don't have to reread the file. We just process what we already read.

We don't need to split the lines either. The startsWith method will have the same result with less processing.

The whole point of using a StringBuilder is so that we don't have to do expensive String concatenation. We might as well take full advantage of that and use two append calls rather than use concatenation first.

But here's what I really don't understand. At the end, you throw away the StringBuilder without doing anything with it. So in the original, you read the file twice and throw away the result both times. Decisions like this are what should be commented. Your comments just restate what the code does. They don't tell me anything about why.

since you're reading the file line by line, you might consider Files.lines — Vogel612
– Vogel612, Commented Aug 7, 2017 at 13:20

Vogel612 · Accepted Answer · 2017-08-07 13:22:52Z

mdfst13 already gave some good pointers but there's more that can be improved in your design.

Bracket style

This is just a minor point but the Java community has the convention to put the opening { on the same line. Not that your way is really wrong though, and it's a good thing you are consistent in your style:)

Single responsibility principle

The idea here is that each class should have 1 thing that it does. And it should only really know about how to do that thing.

If we look at your Storage class I would expect that it handles storing a bunch of Foods (which it does). It should have some methods for handling that storage: adding and removing a Food, a getter so you can see which Food is in the storage.

You could have the saving/loading from file in this same class, but this might already conflict with that main responsibility. It's also possible to create a specific class that handles the persistence of a Storage. This is especially useful if you ever decide to change your file based approach to a real database for example. (But you can always refactor this separation at the moment you make that decission, which is why these 2 methods can still stay in the Storage class).

What the Storage class should NOT do however is handle the main program loop. The main method itself would be fine since they can be put anywhere. But you also provide helper methods to go along with the main program. I suggest making a new class App, Main, FoodApp, or something similar that is responsible for starting the food application.

The makeAFood method also doesn't really belong in the Storage class. I would start with putting this in the Main class until you have more user interaction implemented that makes it worth it to create a specific class to do just that.

Wrong equals

An equals method should take a Object as a parameter. This is to override the equals method of the Object class. That way, it can be used automatically by collections for example to see if that collections contains a certain Food instance or not.

You should also ALWAYS implement both equals(Object o) and hashCode(). Otherwise you can get some weird errors when using a hashmap for example. The contract says that if 2 objects are equal, they must also have the same hashCode.

The easiest way to do this is to use an IDE and let it generate both methods for you. (Most popular free IDE's are IntelliJ and Eclipse, not saying you can't use a different one though).

Here's what IntelliJ generated for me:

@Override
public boolean equals(Object o) {
    if (this == o) return true;
    if (o == null || getClass() != o.getClass()) return false;

    Food food = (Food) o;

    if (Double.compare(food.amount, amount) != 0) return false;
    if (foodName != null ? !foodName.equals(food.foodName) : food.foodName != null) return false;

    return true;
}

@Override
public int hashCode() {
    int result;
    long temp;
    result = foodName != null ? foodName.hashCode() : 0;
    temp = Double.doubleToLongBits(amount);
    result = 31 * result + (int) (temp ^ (temp >>> 32));
    return result;
}

(Quick note that it will be slightly different if you also want subclasses of Food te be treated as "equals").

Immutability is your friend

Here's a stackoverflow question about what immutable is and why to use it.

Some purists like to go all the way with immutability. For your food class for example, this would mean that you wouldn't have any setters at all. If you want to "change" anything to a Food, like adding some amount, you return a new instance with the updated amount instead.

The Food class would then look like this:

public class Food {
    private final String foodName;
    private final double amount;

    //Constructors
    public Food(String aFoodName, double amount) {
        this.foodName = aFoodName;
        this.amount = amount;
    }

    //Accessors
    public String getFoodName() {
        return this.foodName;
    }

    public double getAmount() {
        return this.amount;
    }

    @Override
    public String toString() {
        return this.foodName + "\t" + amount;
    }

    public Food addAmount(double amount){
        return new Food(foodName, this.amount + amount);
    }

    @Override
    public boolean equals(Object o) {
        if (this == o) return true;
        if (o == null || getClass() != o.getClass()) return false;

        Food food = (Food) o;

        if (Double.compare(food.amount, amount) != 0) return false;
        if (foodName != null ? !foodName.equals(food.foodName) : food.foodName != null) return false;

        return true;
    }

    @Override
    public int hashCode() {
        int result;
        long temp;
        result = foodName != null ? foodName.hashCode() : 0;
        temp = Double.doubleToLongBits(amount);
        result = 31 * result + (int) (temp ^ (temp >>> 32));
        return result;
    }
}

The disadvantage of this approach is that you constantly need to update the Food variable with the new instance instead of just modifying it.

I'm not one of those purists so I don't mind having the addAmount method just updating the value of this Food instance.

You do still need to be careful about what you do with the instances though. If we would "take" a certain amount of apples from the Storage for example, what exactly do we return then? I would add a method to the food class like this:

public Food takeAmount(double amount){
    if(amount < this.amount){
        throw new IllegalArgumentException("Requested "+amount+" of " + foodName + " but only " + this.amount + "available.");
    }
    this.amount -= amount;
    return new Food(foodName, amount);
}

A good example of why Food should be immutable is this method in your Storage:

public Food[] getFood()
{
    return this.food;
}

What would happen if someone does this:

public Food stealAnApple(){
    Food apples = null;
    for(Food food : storage.getFood()){
        if ("apples".equals(food.getFoodName())) {
            apples = food;
        }
    }
    if(apples == null) {
        return new Food("apples", 0);
    }
    return apples.takeAmount(1L);
}

And this is just a friendly example that actually keeps your program consistent. It could be a lot worse if someone would accidentally change the amounts or names of your stored foods.

If your Food class is mutable, you shouldn't have methods like the getFood(). If you do need a list and don't want your storage's integrity violated, you have to clone each Food instance, put those clones in a list and return that safe list instead.

Stack Exchange Network

Code to get user input and write and read from a file

2 Answers 2

Bracket style

Single responsibility principle

Wrong equals

Immutability is your friend

You must log in to answer this question.

Hot Network Questions

Code to get user input and write and read from a file

2 Answers 2

Bracket style

Single responsibility principle

Wrong equals

Immutability is your friend

You must log in to answer this question.

Related

Hot Network Questions