patternjavaMinor
Command line Contact Management
Viewed 0 times
contactcommandmanagementline
Problem
Can you please review the following code and give any suggestions for improvment?
Class ContactList.java
```
import java.io.*;
import java.util.*;
public class ContactList {
public static void main(String args[]) throws IOException {
Contact contact;
contact = new Contact();
int action = 0;
ArrayList contacts = new ArrayList();
while (action != 6) {
System.out.println("\nWelcome to Contact List DB. "
+ "What would you like to do? \n");
System.out.println("1. Enter a new person" + "\n"
+ "2. Print the contact list" + "\n"
+ "3. Retrieve a person's information by last name" + "\n"
+ "4. Retrieve a person's information by email address" + "\n"
+ "5. Retrieve all people who live in a given zip code" + "\n"
+ "6. Exit");
Scanner reader = new Scanner(System.in);
reader.useDelimiter("\n");
action = reader.nextInt();
if (action 6) {
System.out.println("Invalid selection. ");
}
switch (action) {
case 1: {
System.out.println("\nEnter Contact Last Name:");
String lastname = reader.next();
if (lastname == null) {
System.out.println("\nInvalid entry. ");
break;
}
else {
contact.setLastName(lastname.toLowerCase());
}
System.out.println("Enter Contact First Name: ");
String firstname = reader.next();
contact.setFirstName(firstname.toLowerCase());
System.out.println("Enter Contact Street Address: ");
String address = reader.next();
contact.setHouseAddress(address.toLowerCase());
System.out.println("Enter Contact City: ");
String city = reader.next();
contact.setCity(city.toLowerCase());
System.out.println("Enter Contact Zip Code: ");
Class ContactList.java
```
import java.io.*;
import java.util.*;
public class ContactList {
public static void main(String args[]) throws IOException {
Contact contact;
contact = new Contact();
int action = 0;
ArrayList contacts = new ArrayList();
while (action != 6) {
System.out.println("\nWelcome to Contact List DB. "
+ "What would you like to do? \n");
System.out.println("1. Enter a new person" + "\n"
+ "2. Print the contact list" + "\n"
+ "3. Retrieve a person's information by last name" + "\n"
+ "4. Retrieve a person's information by email address" + "\n"
+ "5. Retrieve all people who live in a given zip code" + "\n"
+ "6. Exit");
Scanner reader = new Scanner(System.in);
reader.useDelimiter("\n");
action = reader.nextInt();
if (action 6) {
System.out.println("Invalid selection. ");
}
switch (action) {
case 1: {
System.out.println("\nEnter Contact Last Name:");
String lastname = reader.next();
if (lastname == null) {
System.out.println("\nInvalid entry. ");
break;
}
else {
contact.setLastName(lastname.toLowerCase());
}
System.out.println("Enter Contact First Name: ");
String firstname = reader.next();
contact.setFirstName(firstname.toLowerCase());
System.out.println("Enter Contact Street Address: ");
String address = reader.next();
contact.setHouseAddress(address.toLowerCase());
System.out.println("Enter Contact City: ");
String city = reader.next();
contact.setCity(city.toLowerCase());
System.out.println("Enter Contact Zip Code: ");
Solution
The function you wrote is too long, and handles too many concerns. I would separate it out into either other functions, classes, or preferably, a combination of both.
Comments are overused. Comments should be used to enhance the code, i.e. add an explanation why something was done a certain way. They should not describe what is being done. If you have to use comments, the section of code is probably better off in a function with a descriptive name.
You don't need to instantiate a new Contact early in your main function. That should be done just before it is needed (like where you create a new contact);
The actions should be put into an enum:
I would move the file operations into their own class, using something like a repository pattern. In fact, the
Create a class to handle the main loop, something like
You have created a Constructor for Contact that takes all of the required values, why not use it. Read all the values into variables (or a Map), then at the end, create the contact with what's there. Data validation should be done in your Contact class, this will ensure invalid data will never appear in it.
The reading of the attributes could be encapsulated into a class:
In the addAContact function:
Add a repository that will deal with the reading/writing and searching of contacts
Add a ContactFileOperationsClass which contains all your file read/write logic
```
public class ContactFileOperation {
private string path;
public ContactFileOperation(string path) {
this.path = path;
createFileIfRequired();
}
private File createFileIfRequired() {
File file = new File(this.path);
if (!file.exists()) {
file.createNewFile();
}
}
private PrintWriter createPrintWriter() {
return new PrintWriter(new FileWriter(this.Path, true))
}
public save(Contact contact) {
try (PrintWriter o
Comments are overused. Comments should be used to enhance the code, i.e. add an explanation why something was done a certain way. They should not describe what is being done. If you have to use comments, the section of code is probably better off in a function with a descriptive name.
You don't need to instantiate a new Contact early in your main function. That should be done just before it is needed (like where you create a new contact);
The actions should be put into an enum:
public enum MainMenuAction {
AddContact,
PrintContactList,
FindPersonByLastName,
FindPersonByEmail,
SearchByZipCode,
Quit,
UnknownCommand
}I would move the file operations into their own class, using something like a repository pattern. In fact, the
ArrayList should be put in this class too.Create a class to handle the main loop, something like
public class ApplicationRunner {
private Scanner reader;
public ApplicationRunner(Scanner reader) {
this.reader = reader;
}
public void run() {
MainMenuAction action;
while (action = readAction() != MainMenuAction.Quit) {
switch(action) {
case MainMenuAction.AddContact:
addAContact();
break;
case MainMenuAction.PrintContactList:
printContactList();
break;
// Code the rest of the actions here
case MainMenuAction.UnknownCommand:
System.out.println("Invalid selection. ");
break;
}
}
}
private MainMenuAction readAction() {
System.out.println("1. Enter a new person" + "\n"
+ "2. Print the contact list" + "\n"
+ "3. Retrieve a person's information by last name" + "\n"
+ "4. Retrieve a person's information by email address" + "\n"
+ "5. Retrieve all people who live in a given zip code" + "\n"
+ "6. Exit");
int action = reader.nextInt();
// This might be able to be done more efficiently, I'm not that versed in
// java syntax.
switch (action) {
case 1:
return MainMenuAction.AddContact;
// Add rest here
default:
return MainMenuAction.UnknownCommand;
}
}You have created a Constructor for Contact that takes all of the required values, why not use it. Read all the values into variables (or a Map), then at the end, create the contact with what's there. Data validation should be done in your Contact class, this will ensure invalid data will never appear in it.
The reading of the attributes could be encapsulated into a class:
public class ConsoleContactReader {
private Scanner reader;
private Map enteredValues;
public ConsoleReader(Scanner reader) {
this.reader = reader;
enteredValues = new Map();
}
public void readConsole(string prompt, string fieldName) {
System.out.printl(prompt);
enteredValues.put(fieldName, reader.next());
}
public Contact createContactFromMap() {
return new Contact(
this.enteredValues.get("lastName"),
this.enteredValues.get("firstName"),
// etc...
);
}In the addAContact function:
public void addAContact()
{
ConsoleContactReader contactReader = new ConsoleContactReader(this.reader);
contactReader.readConsole("Enter Contact Last Name: ", "lastName");
contactReader.readConsole("Enter Contact First Name: ", "firstName");
// continue with logic
Contact newContact = contactReader.createContactFromMap();
this.repository.Add(newContact);
}Add a repository that will deal with the reading/writing and searching of contacts
public class ContactRepository {
private ArrayList contacts;
private ContactFileOperation fileOperations;
public ContactRepository(ContactFileOperation fileOperations) {
this.fileOperations = fileOperations;
this.contacts = new ArrayList();
}
public void addAContact(Contact contact)
{
contacts.add(contact);
fileOperations.save(contact);
}
// Implement other operations here
}Add a ContactFileOperationsClass which contains all your file read/write logic
```
public class ContactFileOperation {
private string path;
public ContactFileOperation(string path) {
this.path = path;
createFileIfRequired();
}
private File createFileIfRequired() {
File file = new File(this.path);
if (!file.exists()) {
file.createNewFile();
}
}
private PrintWriter createPrintWriter() {
return new PrintWriter(new FileWriter(this.Path, true))
}
public save(Contact contact) {
try (PrintWriter o
Code Snippets
public enum MainMenuAction {
AddContact,
PrintContactList,
FindPersonByLastName,
FindPersonByEmail,
SearchByZipCode,
Quit,
UnknownCommand
}public class ApplicationRunner {
private Scanner reader;
public ApplicationRunner(Scanner reader) {
this.reader = reader;
}
public void run() {
MainMenuAction action;
while (action = readAction() != MainMenuAction.Quit) {
switch(action) {
case MainMenuAction.AddContact:
addAContact();
break;
case MainMenuAction.PrintContactList:
printContactList();
break;
// Code the rest of the actions here
case MainMenuAction.UnknownCommand:
System.out.println("Invalid selection. ");
break;
}
}
}
private MainMenuAction readAction() {
System.out.println("1. Enter a new person" + "\n"
+ "2. Print the contact list" + "\n"
+ "3. Retrieve a person's information by last name" + "\n"
+ "4. Retrieve a person's information by email address" + "\n"
+ "5. Retrieve all people who live in a given zip code" + "\n"
+ "6. Exit");
int action = reader.nextInt();
// This might be able to be done more efficiently, I'm not that versed in
// java syntax.
switch (action) {
case 1:
return MainMenuAction.AddContact;
// Add rest here
default:
return MainMenuAction.UnknownCommand;
}
}public class ConsoleContactReader {
private Scanner reader;
private Map<string, string> enteredValues;
public ConsoleReader(Scanner reader) {
this.reader = reader;
enteredValues = new Map<string, string>();
}
public void readConsole(string prompt, string fieldName) {
System.out.printl(prompt);
enteredValues.put(fieldName, reader.next());
}
public Contact createContactFromMap() {
return new Contact(
this.enteredValues.get("lastName"),
this.enteredValues.get("firstName"),
// etc...
);
}public void addAContact()
{
ConsoleContactReader contactReader = new ConsoleContactReader(this.reader);
contactReader.readConsole("Enter Contact Last Name: ", "lastName");
contactReader.readConsole("Enter Contact First Name: ", "firstName");
// continue with logic
Contact newContact = contactReader.createContactFromMap();
this.repository.Add(newContact);
}public class ContactRepository {
private ArrayList<Contact> contacts;
private ContactFileOperation fileOperations;
public ContactRepository(ContactFileOperation fileOperations) {
this.fileOperations = fileOperations;
this.contacts = new ArrayList<Contact>();
}
public void addAContact(Contact contact)
{
contacts.add(contact);
fileOperations.save(contact);
}
// Implement other operations here
}Context
StackExchange Code Review Q#23793, answer score: 5
Revisions (0)
No revisions yet.