Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am new to java programming and to help me learn I created a simple telephone address book. the code runs as expected but I would be interested to here from more advanced programmers if they think I should do it differently.

    import com.mongodb.MongoClient;
    import com.mongodb.MongoException;
    import com.mongodb.DB;
    import com.mongodb.DBCollection;
    import com.mongodb.BasicDBObject;
    import com.mongodb.DBCursor;

    import java.net.UnknownHostException;
    import java.util.Scanner;

    import org.bson.types.ObjectId;

    public class MongodbPhonebook {
        public static void main(String[] args) {
            boolean boolContinue = true;
            String stringCommand;
            Scanner input = new Scanner(System.in);

            try {
                MongoClient mongoClient = new MongoClient();
                DB db = mongoClient.getDB( "myphonelist" );
                DBCollection coll = db.getCollection("myphonelist");



                do {
                    System.out.println("Enter command (add, find, show, quit): ");
                    stringCommand = input.nextLine();
                    if(stringCommand.equals("quit")){
                        boolContinue = false;
                    }
                    else if(stringCommand.equals("show")){
                        displayAll(coll);
                    }
                    else if(stringCommand.equals("add")){
                        insertNumber(coll);
                    }
                    else if(stringCommand.equals("find")){
                        findNumber(coll);
                    }
                    else {
                        System.out.println("Unknown command!");
                    }
                } while(boolContinue == true);
            }
            catch (UnknownHostException e) {
                e.printStackTrace();
            }
            catch (MongoException e) {
                e.printStackTrace();
            } 
        }

        public static void displayAll(DBCollection collDisplay) {
            DBCursor cursor = collDisplay.find();
            try {
                while(cursor.hasNext()) {
                    System.out.println(cursor.next());
                }
            } finally {
                cursor.close();
            }   
            return;
        }

        public static void insertNumber (DBCollection collInsert) {
            Scanner input = new Scanner(System.in);
            ObjectId stringID;
            String stringName = "";
            String stringNumber = "";
            System.out.println("Enter name: ");
            stringName = input.nextLine();
            System.out.println("Enter number: ");
            stringNumber = input.nextLine();
            BasicDBObject doc = new BasicDBObject("name", stringName).
                    append("number", stringNumber);
            collInsert.insert(doc);
            stringID = (ObjectId)doc.get( "_id" );
            System.out.println("Result: " + stringID.toString());
            return;
        }

        public static void findNumber (DBCollection collFind) {
            Scanner input = new Scanner(System.in);
            String stringName = "";
            System.out.println("Enter name: ");
            stringName = input.nextLine();
            BasicDBObject query = new BasicDBObject("name", stringName);

            DBCursor cursor = collFind.find();
            cursor = collFind.find(query);

            try {
                while(cursor.hasNext()) {
                    System.out.println("found " + cursor.next());
                }
            } finally {
                cursor.close();
            }
        }
    }

Here is the code after changing it to use a switch statement:

import com.mongodb.MongoClient;
import com.mongodb.MongoException;
import com.mongodb.DB;
import com.mongodb.DBCollection;
import com.mongodb.BasicDBObject;
import com.mongodb.DBCursor;

import java.net.UnknownHostException;
import java.util.Scanner;

import org.bson.types.ObjectId;

public class MongodbPhonebook {
    private MongoClient mongoClient;
    private DB db;
    private DBCollection coll;

    public static void main(String[] args) {
        MongodbPhonebook mpb = new MongodbPhonebook();
        mpb.doLoop();
    }

    public void doLoop() {

        boolean boolContinue = true;
        Scanner input = new Scanner(System.in);

        try {
            this.mongoClient = new MongoClient();
            this.db = mongoClient.getDB("myphonelist");
            this.coll = db.getCollection("myphonelist");

            do {
                System.out.println("Enter command (add, find, show, quit): ");
                String stringCommand = input.nextLine();
                switch (stringCommand) {
                case "quit":
                    boolContinue = false;
                    break;
                case "show":
                    displayAll(this.coll);
                    break;
                case "add":
                    insertNumber(this.coll);
                    break;
                case "find":
                    findNumber(this.coll);
                    break;
                default:
                    System.out.println("Unknown command!");
                    break;
                }
            } while (boolContinue);
        } catch (UnknownHostException e) {
            e.printStackTrace();
        } catch (MongoException e) {
            e.printStackTrace();
        }

    }

    public void displayAll(DBCollection collDisplay) {
        DBCursor cursor = collDisplay.find();
        try {
            while (cursor.hasNext()) {
                System.out.println(cursor.next());
            }
        } finally {
            cursor.close();
        }
    }

    public void insertNumber(DBCollection collInsert) {
        Scanner input = new Scanner(System.in);
        ObjectId stringID;
        System.out.println("Enter name: ");
        String stringName = input.nextLine();
        System.out.println("Enter number: ");
        String stringNumber = input.nextLine();
        BasicDBObject doc = new BasicDBObject("name", stringName).append(
                "number", stringNumber);
        collInsert.insert(doc);
        stringID = (ObjectId) doc.get("_id");
        System.out.println("Result: " + stringID.toString());
    }

    public void findNumber(DBCollection collFind) {
        Scanner input = new Scanner(System.in);
        System.out.println("Enter name: ");
        String stringName = input.nextLine();
        BasicDBObject query = new BasicDBObject("name", stringName);

        DBCursor cursor = collFind.find();
        cursor = collFind.find(query);

        try {
            while (cursor.hasNext()) {
                System.out.println("found " + cursor.next());
            }
        } finally {
            cursor.close();
        }
    }
}
share|improve this question
1  
You did good. I like that you have methods that do exactly as you say they should. ie displayAll displays all your entries. My only small suggestion is to make all your methods not static and make the main method just make a instance of your class. For further learning and fun you could look into inheritance and separating your Database code from your main program so you can switch to different types of DB's – Robert Snyder May 10 at 12:11
I converted your "answer" to an edit as answers are reserved for actual answers to the question. – codesparkle May 14 at 12:02

2 Answers

Overall, it looks pretty good. Here are some points to consider:

There is no need to explicitly return at the end of void methods, for example, at the end of displayAll().

Keep your variable declarations close to where they are used. For example, in insertNumber(), I would change this...

String stringName = "";
String stringNumber = "";
System.out.println("Enter name: ");
stringName = input.nextLine();
System.out.println("Enter number: ");
stringNumber = input.nextLine();

...to this...

System.out.println("Enter name: ");
String stringName = input.nextLine();
System.out.println("Enter number: ");
String stringNumber = input.nextLine();

Also, limit the scope of variables to the block where they are used. This follows from the point above. For example, in main(), stringCommand is declared near the top, but is only used inside the do/while block. I would remove the declaration above and change this line...

stringCommand = input.nextLine();

...to this...

String stringCommand = input.nextLine();

These will reduce the number of lines slightly, but will also make reading and debugging the code easier.

As a slighter bigger project, you could change the methods to be non-static, then create an instance of MongodbPhonebook in main() to interact with instead. It may also make sense to move some of the code from main() somewhere else. For example, you could make the mongo variables private members of MongodbPhonebook and initialize them in the constructor.

public class MongodbPhonebook {

    private MongoClient mongoClient;
    private DB db;
    private DBCollection coll;

    public MongodbPhonebook() {
        this.mongoClient = new MongoClient();
        this.db = mongoClient.getDB( "myphonelist" );
        this.coll = db.getCollection("myphonelist");
    }

    ...

}

If you did this, you would no longer need to pass the DBCollection around to the other functions.

Beyond that, consider separating the user interaction and database logic into separate functions. For example, split findNumber() into two functions: one to ask the user for input and display the results, the other to perform the search based on the supplied input. This would allow the database logic functions to be reused in different circumstances, for example, a web application.

share|improve this answer

I'm not sure what version of the JDK you are using, but in Java 7 they introduced switch statements on a String variable instead of just integers. Your code could instead read like this, to make it a little bit more readable.

do {
                System.out.println("Enter command (add, find, show, quit): ");
                stringCommand = input.nextLine();
                switch(stringCommand)
                {
                    case "quit":
                         boolContinue = false;
                         break;
                    case "show":
                         displayAll(coll);
                         break;
                    case "add":
                         insertNumber(coll);
                         break;
                    case "find":
                         findNumber(coll);
                         break;
                    default:
                         System.out.println("Unknown command!");
                         break;
                }
            } while(boolContinue);   // A boolean variable by itself can be used as an expression. The == true expression is redundant.
share|improve this answer
Thank you for your comments. Here is the resulting code. – user2369812 May 13 at 22:17
I'd get rid of boolContinue and return directly inside case "quit". – codesparkle May 14 at 11:58

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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