7
\$\begingroup\$

My assignment was to make a "Lending Library" that keeps track of items. We were advised to create two classes and a main method.

What I am unsure of is if any of my code would be considered bad practice or unnecessary, especially when it comes to declaring objects and instantiating classes.

Also, our professor suggested we use getter/setter methods for all class fields. Would those be necessary for this program?

class MediaItem {                           //fields
    String title;   
    String format;
    boolean onLoan;
    String loanedTo;
    String dateLoaned;

    MediaItem(){                            //default constructor
        title = null;
        format = null;
        onLoan = false;
        loanedTo = null;
        dateLoaned = null;
    }
    MediaItem(String title, String format){         //constructor
        onLoan = false;
        this.title = title;
        this.format = format;
    }


    void markOnLoan(String name, String date){      //methods
            if(onLoan == true)
                System.out.println(this.title + " is already loaned out");
            else {
                    onLoan = true;
                    loanedTo = name;
                    dateLoaned = date;
            }
    }
    void markReturned(){
        if(onLoan == false)
            System.out.println(this.title + " is not currently loaned out");
        onLoan = false; 
    }
}
 class Library{
     static Scanner in = new Scanner(System.in);    //instantiation
     MediaItem t = new MediaItem();
     MediaItem[] items = new MediaItem[100];
     String[] str = new String[100];
     int numberOfItems = 0;                         //fields
     int check = 0;
     int called = 0;

     int displayMenu(){                             //methods
         System.out.println("1. Add new item \n2. Mark an item as on loan \n3. List all items \n4. Mark an item as returned \n5. Quit");     
         System.out.print("What would you like to do? ");
         int a = in.nextInt();
         return a;
     }
     void addNewItem(String title, String format){
         MediaItem item = new MediaItem(title, format);
         items[numberOfItems] = item;
         numberOfItems++; 
     }
     void markItemOnLoan(String title, String name, String date){
         for(int b = 0; b < numberOfItems; b++){
             if(title.equals(items[b].title)){
                 items[b].markOnLoan(name, date);
                 called = 1;
             }
         }
             if(called == 0)
                 System.out.println(title + " doesn't exist");  
             called = 0;
     }
     void listAllItems(){
        for(int c = 0; c < numberOfItems; c++){
            if (items[c].onLoan)
                str[c] = "\n" + items[c].title + " " + items[c].format + " loaned to " + items[c].loanedTo + " on " + items[c].dateLoaned;
            else
                str[c] = "\n" + items[c].title + " " + items[c].format;
            System.out.println(str[c] + "\n");
        }
     }
     void markItemReturned(String title){
         for(int b = 0; b < numberOfItems; b++){
             if(title.equals(items[b].title)){
                 items[b].markReturned();
                 check = 1;
             }      
         }
         if(check == 0)
             System.out.println("Sorry, I couldn't find " + title + " in the library.");
         check = 0;
     }
 }  
public class MidtermProject {                   //main

    public static void main(String[] args) {    
        Scanner in = new Scanner(System.in);
        Library track = new Library();
        MediaItem obj = new MediaItem();
        int choice = 0;
        while (choice != 5){
            choice = track.displayMenu();
            switch(choice){
                case 1: System.out.print("What is the title? ");
                        obj.title = in.nextLine();
                        System.out.print("What is the format? ");
                        obj.format = in.nextLine();
                        track.addNewItem(obj.title, obj.format);
                        break;
                case 2: System.out.print("Which item? ");
                        obj.title = in.nextLine();
                        System.out.print("Who are you loaning it to? ");
                        obj.loanedTo = in.nextLine();
                        System.out.print("When did you loan the item? ");
                        obj.dateLoaned = in.nextLine();
                        track.markItemOnLoan(obj.title, obj.loanedTo, obj.dateLoaned);
                        break;
                case 3: track.listAllItems();
                        break;
                case 4: System.out.println("Which item? ");
                        obj.title = in.nextLine();
                        track.markItemReturned(obj.title);
                        break;
                case 5: System.out.println("Goodbye");
            }
        }
        in.close();
    }
}
\$\endgroup\$
4
\$\begingroup\$

Constructors

You have a 'default' constructor and a 'real' (?) constructor that mandates initial title and format values. The question to ask here is whether any MediaItem object can carry null values. If not, then your default constructor actually becomes risky in the sense that you can carelessly create such empty MediaItem objects, but parts of your code simply assumed they shouldn't exist and... causing NullPointerExceptions when they access the null fields.

On a related note, even if you want to keep the default constructor around, you should chain/cascade your constructors properly so that it is more extensible to future changes... For example:

MediaItem() {
    this(null, null, false, null, null);
}

MediaItem(String title, String format) {
    this(title, format, false, null, null);
}

MediaItem(String title, String format, boolean onLoan, String loanedTo, String dateLoaned) {
    this.title = title;
    this.format = format;
    this.onLoan = onLoan;
    this.loanedTo = loanedTo;
    this.dateLoaned = dateLoaned;
}

This gives you the option to either:

  • fully specify all five fields when instantiating an object,
  • just title and format, or
  • simply specify nothing at all with the 'default' constructor.

If you choose to have another three-item constructor or change what the default values should be (e.g. an empty String - ""?), it's relatively easy to do that too. More importantly, if you decide to make MediaItem an immutable class, the chained constructor approach ensures that fields are properly initialized. For more information, see my previous answer to another question here.

Logging and prompting

You are doing the following in both your markOnLoan() and markReturned() methods:

if (onLoan) {
    System.out.println("some message here");
}

First, you don't need onLoan == true/false, you can simply put if (onLoan) { ... } or if (!onLoan) { ... }.

Second, such messages to System.out is not usually recommended within your model classes as it ties them to a specific output, making it harder to change in the future. Usually, you can use a logging framework such as SLF4J to handle the logging of these messages for you. The benefits are:

  • The flexibility of configuring the logging levels (e.g. you may want to show all DEBUG messages during development, but only show INFO messages for the assignment submission).
  • The output's destination (e.g. to System.out or to a file).
  • The output formatting (e.g. do you want prefixed timestamps? Show the class names?).

Alternatively, you may want to consider handling these cases at the callers' side. Instead of making these methods have void return types, they can return a boolean indicating if the change is successful. For example:

    public boolean markOnLoan(String name, String date) {
        // ...
    }

// caller side
boolean isMarked = item.markOnLoan(name, date);
if (!isMarked) {
    System.out.println(item.title + " is already loaned out");
}

Overriding methods

Within your main() method, you have the following two snippets:

for (int b = 0; b < numberOfItems; b++) {
    if (title.equals(items[b].title)) {
        // ...
    }
}

This particular comparison should be done in your MediaItem class, by overriding its equals() method. For example:

@Override
public boolean equals(Object o) {
    return o instanceof MediaItem && title.equals(((MediaItem)o).title);
}

Often, overriding equals() should require you to override hashCode(), and you can refer to Object.equals() for more information.

if (items[c].onLoan)
    str[c] = "\n" + items[c].title + " " + items[c].format + 
                " loaned to " + items[c].loanedTo + " on " + items[c].dateLoaned;
else
    str[c] = "\n" + items[c].title + " " + items[c].format;
System.out.println(str[c] + "\n");

Constructing a String representation can be done using the toString() method in your MediaItem class as well, for example:

@Override
public String toString() {
    // not sure why you prefer a trailing newline, including as such
    return title + " " + format + 
            (onLoan ? " loaned to " + loanedTo + " on " + dateLoaned : "") + "\n";
}

When you have overridden toString(), you can simply loop through your objects and call that instead:

// using the enhanced for-loop
for (MediaItem item : items) {
    System.out.println(item);
}

Arrays vs Collections classes

It seems slightly odd that you are hard-coding your items array to be exactly 100 items, is this an assignment requirement? Generally, you can use a List implementation such as ArrayList so that you do not have to be explicitly concerned about what is a suitable maximum number of items to store.

Variable names

Just a minor point, but I could not understand why you will call your Library object as track:

    Library track = new Library();
\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for the answer! Very, very helpful, I'll have to look more into constructors overrides and logging. I understand constructors at a very basic level but it is something I need to have a much better understanding of. That is why some things I did may be confusing. As for the assignment, it was noted that the items array held no more then 100 so I decided to hard-code it. I called Library "track" because it was the first thing that came to mind when I wanted to call something that keeps track of something. \$\endgroup\$ – Ryan Boyd Jun 29 '15 at 13:16
  • \$\begingroup\$ @RyanBoyd glad to help! \$\endgroup\$ – h.j.k. Jun 29 '15 at 13:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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