6
\$\begingroup\$

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();
        }
    }
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ 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 \$\endgroup\$ May 10, 2013 at 12:11
  • \$\begingroup\$ You might want to use a ODM like Morphia for modeling your data as a next step. So you would create a Model for an address book entry and use the methods to manipulate that model. This makes the code more readable and maintainable on the long run. \$\endgroup\$ Oct 3, 2014 at 8:43

2 Answers 2

3
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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.
\$\endgroup\$
1
  • \$\begingroup\$ I'd get rid of boolContinue and return directly inside case "quit". \$\endgroup\$
    – Adam
    May 14, 2013 at 11:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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