Bird watching database in Java

Question

I'm learning Java in a MOOC right now and this is my solution for exercise Big year.

This is the exercise:

In this exercise you will design and implement a database for bird-watchers. The database contains birds, each of which has a name (string) and a name in Latin (string). The database also counts the observations of each bird.

The program must implement the following commands:

• Add - adds a bird.
• Observation - adds an observation.
• All - prints all birds.
• One - prints one bird.
• Quit - ends the program.

Incorrect input must also be handled.

This is my solution:

Main.java:

import java.util.Scanner;

public class mainProgram {

    public static void main(String[] args) {
        // NB! Do not create other scanner objects than the one below
        // if and when you create other classes, pass the scanner to them
        // as a parameter

        Scanner scan = new Scanner(System.in);        
        BirdDataBase birds = new BirdDataBase();        
        UserInterface program = new UserInterface(birds, scan);
        
        program.start();

    }
}

Bird.java:

public class Bird {

    private final String name;
    private final String latinName;
    private int observation;

    public Bird(String name, String latinName) {
        this.name = name;
        this.latinName = latinName;
        this.observation = 0;
    }

    public String getName() {
        return name;
    }

    public String getLatinName() {
        return latinName;
    }
    
    public void observation(){
        observation++;
    }
    
    public int getObservation() {
        return observation;
    }

}

BirdDataBase.java:

import java.util.ArrayList;


public class BirdDataBase {

    private final ArrayList<Bird> birds;

    public BirdDataBase() {
        this.birds = new ArrayList<>();
    }

    public void add(Bird bird) {
        if (!(this.birds.contains(bird))) {
            birds.add(bird);
        } else {
            System.out.println("Bird is already in Birds Database");
        }
    }

    public ArrayList birds() {
        return birds;
    }
}

UserInterface.java:

import java.util.ArrayList;
import java.util.Scanner;


public class UserInterface {

    private final BirdDataBase birdsDataBase;
    private final Scanner scanner;

    public UserInterface(BirdDataBase birds, Scanner scanner) {
        this.birdsDataBase = birds;
        this.scanner = scanner;

    }

    public void start() {
        
        ArrayList<Bird> birds = this.birdsDataBase.birds();
        
        while (true) {
            System.out.print("? ");
            String command = scanner.nextLine().toLowerCase();
            if (command.equals("add")) {
                System.out.print("Name: ");
                String name = scanner.nextLine().toUpperCase();
                System.out.print("Name in Latin: ");
                String latinName = scanner.nextLine().toUpperCase();
                birds.add(new Bird(name, latinName));
            } else if (command.equals("observation")) {
                System.out.print("Bird: ");
                String name = scanner.nextLine().toUpperCase();
                for (int i = 0; i < birds.size(); i++) {
                    if (birds.get(i).getName().equals(name)) {
                        birds.get(i).observation();
                    } else {
                        System.out.println("Not a Bird!");
                    }
                }
            } else if (command.equals("all")) {
                for (Bird bird : birds) {
                    System.out.println(bird.getName() + " ("
                            + bird.getLatinName() + "): " + bird.getObservation()
                            + " observations.");
                }
            } else if (command.equals("one")) {
                System.out.println("Bird?");
                String name = scanner.nextLine();
                for (Bird bird : birds) {
                    if (bird.getName().equals(name)) {
                        System.out.println(bird.getName() + " ("
                                + bird.getLatinName() + "): " + bird.getObservation()
                                + " observations.");
                    }
                }
            } else if (command.equals("quit")) {
                break;
            }
        }
    }
}

In your opinion, is this a good solution?

Answer 1

Your Bird class lacks hashCode and equals methods, therefore the contains-check in BirdDataBase.add(Bird) can never work correctly. Without those methods the contains-method has to rely on object identity, e.g. it compares the objects and not the bird names, as one would expect. Writing robust equals and hashCode methods can be tricky. If you are using an IDE, you can take advantage of other peoples' wisdom by letting the IDE generate these methods. But relying on the equals method is probably not the right approach here. Each bird has a single unique identifier: the latin name. So when adding a bird, you should check that the latin name does not exist in the database yet.

The BirdDataBase should have a getByName and getByLatinName methods that return exactly one Bird so that database clients do not have to go through a list every time they, for example, add an observation. I assume the latin name refers to the bird's name in Linnean taxonomy, so you might want to specify that in the messages given to the user. :) You could even use it in method and field names so that it is clear to the person who reads the code.

You have put user interface responsibilities in the BirdDataBase by making it output error messages with System.out.println. Those messages are clearly intended for the end user. Primarily, errors should be passed from the storage layer to the user interface layer by throwing exceptions. Secondly, errors should be displayed using System.err.println.

The variable where you store the UserInterface is named program. This is slightly misleading, as your program is all the classes and configuration files together. Put the UserInterface into variable named userInterface. Then, when you call userInterface.start() it's immediately clear that you're starting the user interface within the program and nothing else.

Running the user interface in start() method is bit unexpected. For example, starting a car takes a short time and then it runs until it is stopped. So I would expect a long running method be called run. That way it follows the existing conventions from the java.lang.Thread and java.lang.Runnable classes.

On a positive note, your class structure suggests that you understand the single responsibility and dependency inversion principles. In the long run those are way more important than minor naming issues.

Answer 2

Data Model

you have a class Bird as Data Model - but it has too much responibilities: apart from being a data entitiy it is also kind of an Observation - that violates the single responisbility principle... create a separate class Observation and add a relation to Bird!

Command Model

just having pure strings and hardcoded parsing them is a very crude approach - i strongly recommend to implement the Command Design Pattern. it is always to to come to undo a command - and when you have to do it with your crude implementation, then it will get ugly ^^

CRUD

provide the basic database methods on your database - these are de facto standard and should always be implemented.

data base interface

right now the database is a class implementation - but sooner or later you want to switch to in-memory-db or any other DBMS... therefore would it be wise to create a proper interface that any Database can implement (Liskov Substitution principle)...

user interface

right now your interface violates the single responisbility principle because it does parse the input, manages the command logic and prints the ui. Managing the programm logic is already addressed with the command Design pattern (see above) - but to split the other i strongly recommend to create an input parser that handles the input from the user - that would also take the responibility of mistyping into the proper place and remove the error handling logic into the proper place (seperation of concerns).

minor issues

• hardcoded strings - should be taken out and kept separate (again seperation of concerns)
• use a logger
• write some tests
• missing exception handling: scanner.nextLine() can throw an exception

Answer 3

My post is going to include a lot of questions, rather than spelling out answers. That's intended so you can spend some effort learning, but if what I say is not obvious, let me know and I'll edit in the answers.

Bird.java

• I actually disagree about this violating the SRP. There's often a lot of debate about what a single responsibility means, but in this case, I think what you have is perfectly fine.
• I agree about implementing equals and hashCode for this class though.
• The only change that I could recommend is that observation is not a verb, which method names should be.

BirdDataBase.java

• You don't actually use this class. Your IDE should be able to show that the add method is never called. If you swapped ArrayList<Bird> birds = this.birdsDataBase.birds(); for ArrayList<Bird> birds = new ArrayList<>(); everything would work as is. There are a couple ways we can change the class to ensure it is used.

public ArrayList birds() {
    return birds;
}

• Rather than giving access to the private list, this should return what's called a defensive copy. Once you make this change, your program will break. It's still a good change. What else needs to change to let you add birds again?

public List<Bird> list() {
    return new ArrayList<Bird>(birds);
}

• I changed the return type as well. For the most part, the only time you should see ArrayList is when you instantiate. Everywhere else should be changed to List. There's actually two, pretty similar ways to have a list in Java: ArrayList and Vector. You almost always want ArrayList. The code that uses the list shouldn't care which it is though, it just needs to know that you're using a list, which perfectly matches the List interface.
• I also changed the name of the method from birds() to list(). Traditional names for database methods include: add (or create), get (or read), update, delete, and sometimes list. Some of those combine to form a CRUD acronym that you'll see often.
• With that change, the list of birds is not defensively copied, but the bird itself is not. Databases usually store that data in tables (which your ArrayList represents) and each table has rows, each row stores one bird. Your UserInterface lets anybody who has a bird object manipulate the contents of that database row. What if, instead, the database returned a defensive copy of the Bird? Something like:

public Bird getByName(String name) {
    // some code to find the bird that matches the name
    
    return bird.copy();
}

• Now, everything that is stored in the database must be manipulated through the method calls on the BirdDataBase. This will also require changing other parts of your code. But see if you can work out all of the implications. If you made this change, how would observing a bird change?
• Don't worry about having a database interface for now.

UserInterface.java

• I really like the idea of this class, but it's all jumbled together. One answerer mentioned using the Command pattern, which fits here perfectly. To give you more details, here are some method signatures that I'd expect for this class.

public AddBirdCommand promptAddBird()
public ObserveBirdCommand promptObserveBird()
public ListBirdsCommand promptListBirds()  // This is sort of a misnomer, as you are not prompting.
public GetBirdCommand promptGetBird()

• Each of those methods should prompt the user for whatever details are needed to do those actions, but they don't actually do those actions. Those methods all handle the input side of the program. We also need some methods for the output.

public void displayBird(Bird bird)
public void displayAllBirds(List<Bird> bird)

• There might be other display methods that you'd need.
• If you made all of those changes, you could end up with the remnants of your start method. That could be moved to a new class Program.
• I agree with the recommendation to rename this method to run().

Main.java

• Your class name doesn't match the file name. This can be your new Program class.