Problem
This is a follow-up to this phonebook code.
This Phone Book allows the user to Enter a value between 1-3.
Entering 1 lets the user Add a number to the phone book, when the process is finished, the user is asked again to pick 1 of the 3 tasks.
Entering 2 lets the user speed dial a number. If the number exists, it will be dialled, else an error statement will be printed. Again the user will pick from 1 of 3 options.
Entering 3 exits the program.
Question: How could I make this code easier to follow/understand. Can I make it more objected oriented and how? I’ve also been told using too many static methods is bad practice. I’ve passed the scanner and arraylist to the class to prevent this, and just want to make sure it was done correctly as I haven’t done this before.
PhoneBookV2.java
package com.java.phonebook;
import java.util.ArrayList; //Import ArrayList
import java.util.InputMismatchException;
import java.util.List;
import java.util.Scanner; //Import Scanner
public class PhoneBookV2 {
public static void main(String[] args){
final List<String> numbers = new ArrayList<>(); //Initialize new ArrayList
Scanner scan = null;
try {
scan = new Scanner(System.in);
PhoneBook phoneBook = new PhoneBook(scan, numbers);
phoneBook.task();
}catch(InputMismatchException e){
System.out.println("Scanner Not Found");
}finally{
if(scan != null){
scan.close();
}
}
}
}
PhoneBook.java
package com.java.phonebook;
import java.util.ArrayList;
import java.util.List;
import java.util.Scanner;
public class PhoneBook {
Scanner scan;
List<String> numbers;
public PhoneBook(Scanner scan, List<String> numbers) {
this.scan = scan;
this.numbers = numbers;
}
public void task() {
boolean cont = true;
do {
//Ask user for input on which task they want to do
System.out.println("Please Pick A Task: n 1:Add A Number to Speed Dial n 2:Speed Dial A Number n 3:Exit");
String choice = scan.nextLine(); //read input
//check user input and continue accordingly
switch (choice) {
case "1":
AddNumber(); //run AddNumber Method
cont = false;
break;
case "2":
CallNumber(); //Run CallNumber method
cont = false;
break;
case "3":
System.out.println("Goodbye!");
System.exit(0); //Terminate Program
cont = false;
break;
default:
System.err.println("Invalid");
}
}while(cont);
}
public void CallNumber() {
boolean cont = true;
//create Keypad
do try {
String[][] keys = {
{" ", "1", " ", "2", " ", "3"},
{" ", "4", " ", "5", " ", "6"},
{" ", "7", " ", "8", " ", "9"},
{" ", " ", " ", "0", " ", " "}
};
//print keypad
printPhoneBook(keys);
//ask user for key number
System.out.println("Pick A Speed Dial Number");
String phoneNum = null; // should not be initialized as "", otherwise dialing message would be displayed even if choice is invalid.
if (scan.hasNextInt()) {
int choice = Integer.parseInt(scan.nextLine()); //convert string to int
if (choice >= 0 && choice <= 9) {
phoneNum = numbers.get((choice + 9) % 10);
} else {
System.err.println("Does Not Exist");
continue;
}
}
//if number exists, Dial number
if (phoneNum != null) {
System.out.println("Dialing " + phoneNum + "....");
cont = false;
}
//or say no number exists
else {
System.out.println("There is No Number At This Location");
break;
}
//ask user tasks again
task();
} catch (IndexOutOfBoundsException e) { //catch possible errors
System.err.println("There is No Number At This Location");
//ask for tasks again
task();
}while(cont);
}
public void AddNumber() {
boolean cont = true; //initialize boolean for loop
do try{
System.out.print("Please Enter The Number You Wish To Save Under Speed Dial: ");
if(scan.hasNextLong()) {
long input = Long.parseLong((scan.nextLine().trim().strip()));
if ((input >= 100) && (input <= 999_999_999)) {
// Add the next number to the ArrayList
numbers.add(String.valueOf(input));
}else if((input < 1000) || (input > 999_999_999)){
System.out.println("Invalid Entry");
continue;
}
}else if (scan.hasNextLine()) {
scan.nextLine();
System.out.println("Invalid Entry");
continue;
}
System.out.print("Would you like to add another? Yes or No: ");
String answer = scan.nextLine().toLowerCase(); //take input and convert to lowercase
if (answer.equals("yes")){
continue; //if they want to continue, allow it
}else if (answer.equals("no")) { //if not, print arraylist and exit loop
print((ArrayList<String>) numbers);
cont = false;
}else if(answer.equals("")){
System.out.println("Invalid Entry");
}else{
System.out.println("Invalid");
}
}catch(NumberFormatException e){
}while(cont);
//ask user tasks again
task();
}
public static void printPhoneBook(String[][] keys){
// Loop to print array
for(String[] row : keys){
for(String s : row){
System.out.print(s);
}
System.out.println();
}
}
public void print(){
//loop to print array numbers
for(int i = 0; i< numbers.size(); i++){
System.out.println((i+1) + ") " + numbers.get(i));
}
}
}
Solution
Conventions
Following language conventions can make your code more approachable to other people. Java methods follow camelCase, so CallNumber should be callNumber etc. More naming conventions here.
keys
I think you’ve done this so that it’s easier to write the display code:
String[][] keys = {
{" ", "1", " ", "2", " ", "3"},
However, it means that keys doesn’t just contain keys. It also contains white space that is to be printed between keys. This can be a little confusion and may cause issues if you wanted change the way print the phone in the future.
Since keys is only used by the private method printPhoneBook, it might make sense for it to be defined as a local variable in that method, rather than in the CallNumber method.
Public interface
The public methods on your class setup the expectation about how the class is to be used. All of the methods in your PhoneBook class are public, however it looks like task is the only one that you’d really expect a client to call. Some methods like CallNumber seem like they could be public methods, however they then call task which presents the user with a ‘what do you want to do next decision’. This could be a little confusing.
It does what is says on the tin…
Naming is an important skill, which is surprisingly difficult get right. A good start is trying to make sure that names reflect what a variable/method/class represents/does/is responsible for. printPhoneBook doesn’t look to me like it prints a phonebook. It looks like it prints they key pad (assuming the client passes in the correct structure).
Exit!
System.out.println("Goodbye!");
System.exit(0); //Terminate Program
cont = false;
break;
Nothing after the exit is run, because the process terminates. Having code after the exit is confusing. Generally you don’t actually want to call System.exit other than in extreme circumstances. A better approach is to either throw an Exception, or in this case simply return from the task method so that the caller can decide what to do next.
Looping / Recursion.
Your task method has a loop in it that keeps going until a valid choice is selected. If a valid choice is selected though, each choice calls back to task to loop again. It seems like just having task keep looping until exit is selected may be more straightforward. It would also mean that AddNumber and CallNumber didn’t have to call task to display the menu again. This in turn would mean that it was possible to call AddNumber from a different class, which goes some way towards making the class more reusable.
The main things you seem to have done is a) share the Scanner and b) move the processing into a class. You’ve also added some validation of your input. This is still not OOP.
- The first problem I find is that your PhoneBook class will not compile. At the very least, code you submit here should compile and pass some basic sanity tests.
- Your approach to indexing is odd, and unclear. Are you sure you understand how 0-based indexing works in Java?
- The fixed size list is enforced in some parts of your code – you only allow entries 0-9 to be chosen, but you allow an unlimited number of entries to be added. (Is this because you want to allow an unlimited number of entries, and speed-dial applies to the first 10? If so, you should document that design decision).
- Your input validation rejects invalid numbers (like “banana”) without giving any feedback, and the try…catch block is bigger than it needs to be, making it harder to understand.
- Your range check on phone numbers is inconsistent (100 or 1000?) and if you were consistent, then the second “if” (in the “else”) would be redundant.
- You still use recursion for your main “loop”
- You still have no distinction between the data and its external presentation. Nothing about your Phonebook could be reused sensibly if you, for example, moved to a GUI or a REST API approach. Read up on “Separation Of Concerns”…
- As already mentioned, you don’t follow Java naming conventions, and your code formatting is also non-standard – I’ve never seen “do” and “try” on the same line before, and hope never to do so again. (Formatting is easily fixed by using a decent GUI, such as Eclipse or IntelliJ and getting it to format your code)
- Your commenting is frequently pointless –
import java.util.Scanner; //Import Scannerhas no value at all. Comments should add value, usually by explaining why the code is as it is.
Here’s a suggestion to get you started on an Object-Oriented approach.
In this design I assume that your PhoneBook should store an unlimited number of phone numbers in the order in which they have been entered, and that the first 10 entries (indexes 0 to 9) should be available as speed-dial numbers.
First, write a PhoneBook class which does no I/O – it should implement Iterable<String> and provide the following:
- A constructor which takes no arguments.
- An addPhoneNumber() method which takes a phone number as a parameter and stores it in some internal store – an ArrayList seems a reasonable choice (create this in the constructor).
- A getPhoneNumber() method which takes an integer as a parameter and returns the appropriate phone number, returning null if no corresponding phone number exists.
- An iterator() method which returns an iterator over the internal list of phone numbers.
Now write some unit tests for this class. Ideally use Junit, as that’s the norm in Java and learning it will give you a valuable skill. Make sure your code compiles, and passes your unit tests. You should consider offering this class and the unit tests for review.
Now write your interactive code to use this class, taking into account the feedback you’ve already been given, and following Java standards for naming and layout. Compile and test your program. Then raise a fresh request for review.
Here’s skeleton code for the PhoneBook class
import java.util.Iterator;
public class PhoneBook implements Iterable<String> {
public PhoneBook() {
// Add code to create the internal storage for phone numbers
}
public void addPhoneNumber(String phoneNumber) {
// Add the phone number to the storage
}
public String getPhoneNumber(int index) {
return null; // Change this to return the appropriate phone number, or null if none exists
}
public Iterator<String> iterator() {
return null; // change this to return a suitable Iterator
}
}