6
\$\begingroup\$

I've been learning Java for a month now using programmingbydoing.com in addition with reading the Java reference. I've had very very basic programming experience and I'm trying to improve my skills. These assignments are my first large projects that I've attempted.

I attempted an address book twice. The first time I was unhappy with my code. It was extremely messy and confusing. The second time I used comments and managed to complete it, but it's definitely still awkward.

import java.util.Scanner;
class Book{
    Scanner s = new Scanner(System.in);
    //Nested class for each entry
    class Entry{
        private String first;
        private String last;
        private String address;
        private String email;
        Entry(String first, String last, String address, String email){
            this.first = first;
            this.last = last;
            this.address = address;
            this.email = email;
        }
        Entry(){
            first = "";
            last = "";
            address = "";
            email = "";
        }
        public void readEntry(){
            System.out.println("First Name:"+first );
            System.out.println("Last Name:"+last );
            System.out.println("Address:"+address );
            System.out.println("Email:"+email );
        }
    }


    //Keeps track of how many entries are in the book
    private int entries = 0;
    Entry[] contents;
    public void initEntries(int e){
        contents = new Entry[e];
        for (int i = 0;i<contents.length;i++){      //Initializes an array of entries, then loops through to initialize each individual entry
            contents[i] = new Entry();
        }
    }
    public int getEntries(){
        return contents.length;
    }
    //Adds an entry to the book
    public void add(String first, String last, String address, String email){
        if (entries<contents.length){
        contents[entries] = new Entry(first, last, address, email);
        entries++;
        }
        else System.out.println("Error: book is full");
    }

    //Removes an entry from the book
    public void remove(int entry){
        if (entries>0){
            contents[entry] = new Entry();
            for (int i = 0;i<entries-entry;i++){
                if (entry+1==entries) contents[entry] = new Entry();
                else{
                    Entry temp = contents[entry+i];
                    contents[entry+i] = contents[entry+i+1]; //Removes an entry end moves each entry after it one backwards.
                    contents[entry+i+1] = temp;
                }
            }
            entries--;
            }
            else System.out.println("Error: book is empty.");
    }

    //Changes the values of an entry
    public void edit(String first, String last, String address, String email, int selection){
        contents[selection].first = first;
        contents[selection].last = last;
        contents[selection].address = address;
        contents[selection].email = email;
    }

    //Sorts the book based on a part of the entry
    //int n is used to tell which part of the entries to base the sort on
    public void sort(int n){
        for(int i = 0;i<contents.length;i++){
            for (int j = 0;j<contents.length;j++){
                switch(n){
                    case 1:
                        if (contents[i].first.compareTo(contents[j].first)<0){
                            Entry temp = contents[i];
                            contents[i] = contents[j];
                            contents[j] = temp;
                        }
                        break;
                    case 2:
                        if (contents[i].last.compareTo(contents[j].last)<0){
                            Entry temp = contents[i];
                            contents[i] = contents[j];
                            contents[j] = temp;
                        }
                        break;
                    case 3:
                        if (contents[i].address.compareTo(contents[j].address)<0){
                            Entry temp = contents[i];
                            contents[i] = contents[j];
                            contents[j] = temp;
                        }
                        break;
                    case 4:
                        if (contents[i].email.compareTo(contents[j].email)<0){
                            Entry temp = contents[i];
                            contents[i] = contents[j];
                            contents[j] = temp;
                        }
                        break;
                    default: 
                        System.out.println("Error: invalid field");
                        break;
                }
            }
        }
    }
    public void addFromCopy(Entry e){
        if (entries<contents.length){
            contents[entries] = e;
            entries++;
            }
            else System.out.println("Error: book is full");
    }

}

AddressBook2

public class AddressBook2 {
    public static void main(String[] args){
        Scanner s = new Scanner(System.in);
        System.out.print("How many books do you want to create? ");
        int howManyBooks;
        int howManyEntries;

        Book[] library = new Book[0];


        while(true){
            howManyBooks = s.nextInt();
            if (howManyBooks>0){
                library = new Book[howManyBooks];                   //This code decides how many books are in the array of books/the library
                break;
            }
            else System.out.print("You must create at least 1 book.");
            }



        for (int i=0;i<library.length;i++){


            library[i] = new Book(); //Fixed reference to null because each book in the library had not been initialized yet.

            while(true){
                System.out.print("How many entries in book "+i+"? ");
                howManyEntries = s.nextInt();
                if (howManyEntries>0) {
                    library[i].initEntries(howManyEntries);                 //This code decides how many entries are in each book in the library
                    break;
                }
                else System.out.println("You must create at least 1 Entry.");
                }


        }
        boolean done = false;
        int selectedBook = 0;
        int selection;
        while (done==false){
            System.out.println("Book "+selectedBook+" is currently selected.");

            for (int i = 0;i<library[selectedBook].getEntries();i++){
                System.out.println("===========Entry "+i+" ===========");
                library[selectedBook].contents[i].readEntry(); //Accessing the array of entries INSIDE the array of books/the library
                System.out.println("================================");
            }


            System.out.println("Select an option!");
            System.out.println("1. Add an entry");
            System.out.println("2. Remove an entry");
            System.out.println("3. Edit an entry");
            System.out.println("4. Sort all entries in this book");
            System.out.println("5. Select another book");
            System.out.println("6. Move entry across books");
            System.out.println("7. Exit the menu");
            System.out.print("> ");
            selection = s.nextInt();
            String first, last, address, email;
            switch(selection){
            case 1: 
                System.out.print("First name? ");
                first = s.next();
                System.out.print("Last name? ");
                last = s.next();
                System.out.print("Address? ");
                address = s.next();
                System.out.print("Email? ");
                email = s.next();
                library[selectedBook].add(first, last, address, email);
                break;
            case 2: 
                System.out.print("Remove which entry? ");
                int entry = s.nextInt();
                library[selectedBook].remove(entry);
                break;
            case 3:
                System.out.print("Edit which entry?");
                int whichEntry = s.nextInt();
                System.out.print("First name? ");
                first = s.next();
                System.out.print("Last name? ");
                last = s.next();
                System.out.print("Address? ");
                address = s.next();
                System.out.print("Email? ");
                email = s.next();
                library[selectedBook].edit(first, last, address, email, whichEntry);
                break;
            case 4: 
                System.out.println("Sort alphabetically by which field?");
                System.out.println("1. Sort by first name");
                System.out.println("2. Sort by last name");
                System.out.println("3. Sort by address");
                System.out.println("4. Sort by email");
                library[selectedBook].sort(s.nextInt());
                break;
            case 5: 
                System.out.print("Select which book?");
                selectedBook = s.nextInt();
                break;
            case 6:
                System.out.print("Move which entry? ");
                int entryNo = s.nextInt();
                Book.Entry entryToMove = library[selectedBook].contents[entryNo];
                library[selectedBook].remove(entryNo);
                System.out.print("To which book? ");
                int whichBook = s.nextInt();
                library[whichBook].addFromCopy(entryToMove);
                break;
            case 7:
                done = true;
                break;
            default:
                System.out.print("Please choose a valid menu number");


            }

        }
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to Code Review! I'm certain our Java experts will have some good advice for you. I hope you get some great answers! \$\endgroup\$ – Phrancis Jul 26 '15 at 17:27
3
\$\begingroup\$

The code looks fine for the most part. A few things that I would have done differently are:

Since you have a constructor that takes in all arguments you could just make the no argument constructor use that constructor with "" as the argument for each position. That is -

Entry() {
   this("", "", "", "");
}

If you decided to have some partially defined constructors the pattern would be the consistent.

Rather than making a readEntry method to read an Entry you could override the object's toString() method in a similar fashion. This would give you more flexibility in future (e.g you could reformat or extract the data from an entry with a regex if you wanted to)

Since the value entries keeps a count of all the Entry objects made you could make it a static variable in the Entry class.

protected static int entries = 0;

But that's purely a matter of choice and style.

A getter method like getEntries usually returns the thing that you claim it is getting. Since you are just returning the length of the contents array size() would be a better name for that method.

The remove method was tricky to follow at first. You have a loop independent conditional in the loop. This suggests that the conditional shouldn't be in the loop in the first place otherwise you're doing unnecessary checks on a method that runs through the length of the contents array anyway. Furthermore, it seems to be doing the same assignment as the first statement in you method. I can't tell why. If you re doing that to make a blank entry at the end in the case that the last element is deleted then put it just above the line where you decrement entries. On that note, I think making contents an ArrayList would have saved you a lot of the work you're doing in the removal process.

Also it would be more practical to either split edit into different edit functions (editEmail , editName etc). It seems more likely that you'd only want to edit a few details at a time for a contact (people don't usually change their names).

The sort method is quadratic and seems to be doing a lot of work that you could do with java Collections. You could rewrite to look like this:

List<Entry> to_sort = new ArrayList<Entry>(Arrays.asList(contents));
Collections.sort(to_sort, new Comparator<Entry>() {
    public int compare(Entry one, Entry other) {
        return one.[whatever is at n].compareTo(other.[whatever is at n]);
    }
}); 

The AddressBook2 class is a lot of boilerplate code so it's fine for the most part. I would personally split long print functions into different void functions. Otherwise this is good if done only after a moth of Java.

\$\endgroup\$
  • 1
    \$\begingroup\$ Your point about array list is spot on, and the project that the op is working on comes out of the array list lessons. Therefor array lists should have been used \$\endgroup\$ – Robert Snyder Jul 27 '15 at 2:04
3
\$\begingroup\$

Side note The link you provided was to the wrong project, but they are right next to each other.

I scanned through the website that you are learning from, and the project Address book is found in the ArrayList section but there is no reference to the ArrayList type. https://docs.oracle.com/javase/8/docs/api/java/util/ArrayList.html "Java doc for ArrayList". Not to be harsh, but if I was the teacher I would be requesting a re-write.

Using inner classes can very rarely be useful. This is not one of those cases. Entry has a proper place in the project as you use it as a data model. Making an ArrayList<Entry> in your Book class would be clean and to the point.

The Book class itself will become very easy and clean when you switch to ArrayList. For instance if you decided to use lambdas and kept using the number to sort your entries your code would look something like this in your switch

ArrayList<Entry> list = new ArrayList<>();
//populate the entries
list.sort((entry1, entry2) -> entry1.last.compareTo(entry2.last));
//list is now sorted by last name

However passing in an integer to the sort method is not very clear. What I mean is that, without looking at your code can you tell me what number is needed to sort by for each property in Entry? Nor would I want to. Even if you were to put a good comment that described very clearly what each number represented, it's still not clean. No instead make sort take in a Comparator<T> Then your code could look like one of two options.

book.sort((entry1, entry2) -> entry1.last.compareTo(entry2.last));

//or if you create a new class per property it could look like this
class sortEntriesByLastName : Comparator<Entry>{...}
book.sort(new sortEntriesByLastName())

Enough on that. Next is why is Scanner s = new Scanner(System.in); in Book? it isn't used. Even if it was used, this would be a bad place for it. Keeping a clear separation of concerns (Single Repsonsibility Principle) will show that putting logic into classes and/or methods that makes it harder to debug and fix code.

Naming

public int getEntries() gets the count of entries, not the entries themselves. A better name for this method should be getEntriesCount or count or size. same goes with contents[entries]. I nit pick the word entries because you have a class called Entry. Most collections are either called the plural version of the class name, or by the logical name of a collection of classes. (For example a collection of Animals in a Person class could be called pets, instead of animals)

Be concerned when methods start to have more than 3 parameters. Your add method has 4, and the intent of it is to just add a new entry. So instead of passing in those 4 entries, just have Entry as a parameter.

Duplicate code: add and addFromCopy do the same thing. that being said create a new Entry with the 4 parameters and then call addFromCopy in add. Or better yet, just delete the method add, and use only addFromCopy (but rename it to add). Be mindful of when you have code that essentially does the same thing and when you spot it, pull it out into a shared method. (yes even if it is only for 2 or 3 lines of code)

Last but not least, is that I'm suprised that a new blog about learning how to program doesn't mention ANY THING about unit tests. I know it is geared for beginners learning the basics, but the basics can still be taught by using a testing framework instead of viewing the output on the console. Normally I'd give a brief example, and I still might, but I have to get going. If I have time tonight I will edit my post and show an example.

\$\endgroup\$
1
\$\begingroup\$

You have made a mistake here for your first address book if it was still able to work then it's a miracle if not then fix it:

public void addFromCopy(Entry e){
        if (entries**<**contents.length){

There is only 1 symbol fix this via:

public void addFromCopy(Entry e){
            if (entries<contents.length>){
\$\endgroup\$
  • \$\begingroup\$ if (entries**<**contents.length){ - is the **<** supposed to be a bold less than (i.e. <)? \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jan 23 '18 at 22:07

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.