Cell phone user interface

Question

I'm self teaching programming via text I've found online and a course on Udemy. We had a challenge today to make a simple cell phone interface to add, remove, modify, etc. At this point I have learned the basic OOP principles, arrays, arraylists, and recently input.

I was wondering if someone could check my basic code and tell me if I'm doing anything terribly wrong. It's difficult to know if my custom answers to challenge problems are going okay. I'm trying to get the principles of OOP down well and this took me a bit to figure out on some parts.

MobilePhone.java

package com.company;
import java.util.Scanner;
import java.util.ArrayList;
public class MobilePhone {
private static Scanner scannerStr = new Scanner(System.in);
private ArrayList<Contacts> myMobilePhone = new ArrayList<Contacts>();
public void addContacts(Contacts contact) {
 myMobilePhone.add(contact);
}
public void printContacts() {
 System.out.println("Your contact list has " + myMobilePhone.size() + " contacts");
 for (int i = 0; i < myMobilePhone.size(); i++) {
 System.out.println(i + 1 + ". " + myMobilePhone.get(i));
 }
}
private int removeContacts(String name, String num) {
 for (int i = 0; i < myMobilePhone.size(); i++) {
 Contacts contact = this.myMobilePhone.get(i);
 if (contact.getName().equals(name) && contact.getPhoneNumber().equals(num)) {
 return i;
 }
 }
 return -1;
}
public void removeTheContacts(String name, String num) {
 int position = removeContacts(name, num);
 if (position >= 0) {
 myMobilePhone.remove(position);
 System.out.println("The contact " + name + ", " + num + " was removed");
 } else
 System.out.println("The contact doesn't exist");
}
private int findContactName(String name) {
 for (int i = 0; i < myMobilePhone.size(); i++) {
 Contacts contact = this.myMobilePhone.get(i);
 if (contact.getName().equals(name)) {
 return i; 
 }
 }
 return -1;
}
public void findContacts(String name) {
 int position = findContactName(name);
 if (position >= 0)
 System.out.println("Contact " + name + " was found");
 else
 System.out.println("Contact " + name + " was not found");
}
public void modifyContacts(String name, String number) {
 int position = findContactName(name);
 if (position >= 0) {
 System.out.println("Enter a new name");
 String newName = scannerStr.nextLine();
 System.out.println("Enter a new phone number");
 String newNumber = scannerStr.nextLine();
 modifyContacts(name, newName, newNumber);
 } else
 System.out.println("The contact doesn't exit");
}
private Contacts modifyContacts(String oldName, String newName, String newNumber) {
 int position = findContactName(oldName);
 Contacts contact = new Contacts(newName, newNumber);
 myMobilePhone.set(position, contact);
 return contact;
}
}

Contacts.java

public class Contacts {
private String name;
private String phoneNumber;
public Contacts(String name, String phoneNumber) {
 this.name = name;
 this.phoneNumber = phoneNumber;
}
public String getName() {
 return name;
}
public String getPhoneNumber() {
 return phoneNumber;
}
@Override
public String toString() {
 return "Contact: " + getName() + ", " + getPhoneNumber();
}
}

Main.java

import com.company.Contacts;
import com.company.MobilePhone;
import java.util.Scanner;
public class Main {
private static Scanner scannerInt = new Scanner(System.in);
private static Scanner scannerStr = new Scanner(System.in);
private static MobilePhone mobilePhone = new MobilePhone();
public static void main(String[] args) {
 printInstructions();
 boolean quit = false;
 int choice;
 while(!quit){
 choice = scannerInt.nextInt();
 switch(choice) {
 case 0:
 printInstructions();
 break;
 case 1:
 printContacts();
 break;
 case 2:
 addContacts();
 break;
 case 3:
 removeContacts();
 break;
 case 4:
 modifyContacts();
 break;
 case 5:
 searchContacts();
 break;
 case 6:
 quit = true;
 break;
 }
 }
}
private static void printInstructions(){
 System.out.println("Please choose from the following options: ");
 System.out.println("\t Press 0 to print instructions again");
 System.out.println("\t Press 1 to show your contacts");
 System.out.println("\t Press 2 to add a contact");
 System.out.println("\t Press 3 to remove a contact");
 System.out.println("\t Press 4 to update a contact");
 System.out.println("\t Press 5 to search for a contact");
 System.out.println("\t Press 6 to quit");
}
private static void printContacts(){
 mobilePhone.printContacts();
}
private static void removeContacts(){
 System.out.print("Enter the name for the contact you want to remove: ");
 String name = scannerStr.nextLine();
 System.out.print("Enter the number for the same contact you wish to remove: ");
 String number = scannerStr.nextLine();
 mobilePhone.removeTheContacts(name, number);
}
private static Contacts addContacts(){
 System.out.print("Enter the name for the contact you want to add: ");
 String name = scannerStr.nextLine();
 System.out.print("Enter the number for the same contact you wish to add: ");
 String number = scannerStr.nextLine();
 Contacts contact = new Contacts(name, number);
 mobilePhone.addContacts(contact);
 System.out.println("Contact " + name + ", " + number + " was added!");
 return contact;
}
private static void searchContacts (){
 System.out.print("Enter the name for the contact you want to find: ");
 String name = scannerStr.nextLine();
 mobilePhone.findContacts(name);
}
private static void modifyContacts() {
 System.out.print("Enter the name for the contact you want to change: ");
 String name = scannerStr.nextLine();
 System.out.print("Enter the number for the same contact you wish to change: ");
 String number = scannerStr.nextLine();
 mobilePhone.modifyContacts(name, number);
}
}

• Timothy Truckle: Welcome to CodeReview and thanks for sharing your code. What are the basic OOP principles you talking about? (I think they are different from the ones I have in mind)
• Zephers: I have been over classes, polymorphism, and encapsulation in 2 recent videos and read some text on it. This challenge was mainly focused on ArrayLists, however I tried to utilize what I thought fit in here from what I've learned. I'm finding programming quite intimidating due to mainly having no feedback I suppose. Which I know might be difficult to get from anyone since everyone has different experiences.
• Timothy Truckle: "Which I know might be difficult to get from anyone since everyone has different experiences" Different experiences are actually a good thing: it gives you different point of views to build your own from in between. No single person owns the ultimate truth (except me of cause... ;o) )

Answer 1

Thanks for sharing your code!

OOP

OOP doesn't mean to "split up" code into random classes.

The ultimate goal of OOP is to reduce code duplication, improve readability and support reuse as well as extending the code.

Doing OOP means that you follow certain principles which are (among others):

• information hiding / encapsulation
• single responsibility
• separation of concerns
• KISS (Keep it simple (and) stupid.)
• DRY (Don't repeat yourself.)
• "Tell! Don't ask."
• Law of demeter ("Don't talk to strangers!")
• replace branching with polymorphism

Class design

In OOP we create classes if thereis a specific behaviornot yet covered by any existing class.

Your example does that quite well.

single responsibility / separation of concerns

This means that each class or method should have a clear narrow defined responsibility.

The most common violation of that principle is to mix business logic with user interaction. So do you...

The method modifyContacts() should not live inside the CellPhone class since the classes primary responsibility seams to be "manage the address book entries".

polymorphism

In OOP we avoid switch statements (or if/els cascades) inside the business logic (in contrast to where we initialize the application and create its object tree).

You use the switch to select the operation the user chose.
^{BTW: Never use switch without a default entry!}
The better way of doing this is to collect the possible operations in a List and get it by the index the user entered. To enable this we need a common interface that all the operations implement:

 interface MyOperation{
 void execute(); 
 }

No you can have a List that holds all of this operations:

 private static List<MyOperation> operations = Arrays.asList(
 new MyOperation() { // legacy anonymous inner class
 public boolean execute(){
 printInstructions();
 }
 },
 ()-> printContacts(), // Java 8 Lambda
 ()-> addContacts(),
 ()-> removeContacts(),
 ()-> modifyContacts(),
 ()-> searchContacts()
 );
public static void main(String[] args) {
 boolean quit = false;
 int choice;
 while(!quit){
 choice = scannerInt.nextInt();
 if(choice < operations.size())
 operations.get(choice).execute();
 else
 quit = true;
 }
}

This separates the initialization from the business logic.

But this has more potential:

You also need to know which operations are available in your printInstructions() method.

If you want to add another operation you need to update two places: your switch and the printInstructions() method. And you have to keep them synchronized.

But what if you would use the new list of operations at both places?

private static List<MyOperation> operations = Arrays.asList(
 new MyOperation() { // legacy anonymous inner class
 public boolean execute(){
 printInstructions();
 }
 public String toString(){
 return "print instructions again";
 }
 },
 new MyOperation() { // no lambda possible
 public boolean execute(){
 printContacts();
 }
 public String toString(){
 return "show your contacts";
 }
 },
 // all the others ...
 );
public static void main(String[] args) {
 boolean quit = false;
 int choice;
 while(!quit){
 choice = scannerInt.nextInt();
 if(choice < operations.size())
 operations.get(choice).execute();
 else
 quit = true;
 }
}
private static void printInstructions(){
 System.out.println("Please choose from the following options: ");
 for(MyOperation operation : operations)
 System.out.println("\t Press "+ operations.indexOf(operation) +" to "+ operation.toString());
 System.out.println("\t Press "+operations.size()+" to quit");
}

Now, when you add a new entry into the list it gets printed and selected automatically.

Naming

Finding good names is the hardest part in programming. So always take your time to think carefully of your identifier names.

Naming Conventions

It looks like you already know the
Java Naming Conventions.

Don't surprise your readers

Your class is called CellPhone but actually it is only handling address book entries Therefore it should be named AddressBook.

If you intended it to be the cell phone then it should not implement the address book behavior its own. It should have an address book...

---

You have methods named findContacts() and modifyContacts(). They imply that you may return multiple results. But actually each of that methods deals with a single contact. Therefore the names should not have the plural s.

resource management

How many key boards does your computer have?

You create (at least) three instances of Scanner passing in the same instance of System.in. Accessing a unique resource from different "wrapper" objects begs for all kinds of problems due to concurrent access.

I didn't think I was using scanner right but I'm not sure how to or what would be the best way to use one scanner for every class? Do i need to make a class for it and call it from there for each time I need it? Thanks. – SushiMouse

Create a single Scanner instance on start and pass that in to the classes needing them as constructor parameter. This is called dependency injection:

public class MobilePhone {
 private final Scanner scannerStr; // not static but final!
 private ArrayList<Contacts> myMobilePhone = new ArrayList<Contacts>();
 public MobilePhone(Scanner scannerStr){
 this.scannerStr= scannerStr;
 }
 // ...
public class Main {
 private static Scanner input = new Scanner(System.in);
 private static MobilePhone mobilePhone = new MobilePhone(input);
 public static void main (String[] args) {
 // ...

• Zephers: Thank you for the help, I redid a challenge today that is similar and it turned out better. I didn't think I was using scanner right but I'm not sure how to or what would be the best way to use one scanner for every class? Do i need to make a class for it and call it from there for each time I need it? Thanks.

Answer 2

Looks decent!

Some smaller things:

The myMobilePhone is declared as ArrayList. Always declare it as List, since ArrayList is an "implementation detail". If you'd switch from an ArrayList, you might have to change an awful lot of code. Also, it's name is myMobilePhone, I would have used contacts.

Also, you want to think about if List is the best type. Suggestions: Use a Map<String, Contact>, where the String is the phone number. So it's a bit easier to figure out, if a phone number already exists and easier to remove a contact.

With that said, another suggestions: Use a custom type for a phone number. So you can save stuff such as the country code, if it's mobile or office. (But then, the Map<String, Contact suggestion doesn't work very well anymore).

Hope this helps, slowy

• Raimund Krämer: I like the idea with the custom type for the phone number. You can still use a Map<String, Contact> if you use PhoneNumber.toString() as key.

Answer 3

Here are my comments:

1) Do Not Use Constants inside the code

You specify the list of input choices in two places: the print instructions and switch statement. This is bad practice for (at least) three reasons:

1. You make yourself vulnerable to typos. what if you mistyped an option in one of the places? you will only discover this at run time.
2. What if you want to "insert" or remove a choice in the middle of the list and as a result need to assign new values to the other choices?
3. The literal value gives no clue to the logical meaning so you are forced to keep going back to the printInstructions() to remind yourself of the meaning of a certain choice

The best solution for that is to create a Java enum type to host all the choices. However, if you are not yet familiar with this construct, you can replace the constant literals with constant variables. These are static final variables that effectively become literals

public class MobilePhone {
 public static final int PRINT_INSTRUCTIONS_CHOICE = 0;
 public static final int SHOW_CONTACT_CHOICE = 1;
 public static final int ADD_CONTACT_CHOICE = 2;
 ...
private static void printInstructions(){
 System.out.println("Please choose from the following options: ");
 System.out.println("\t Press " + PRINT_INSTRUCTIONS_CHOICE + " to print instructions again");
 System.out.println("\t Press " + SHOW_CONTACT_CHOICE + " to show your contacts");
 ...
switch(choice) {
 case PRINT_INSTRUCTIONS_CHOICE:
 printInstructions();
 break;
 case SHOW_CONTACT_CHOICE:
 ...

AS you can see, this solves ALL three problems mentioned above.

2) Use Class names that reflect the scope of one instance

an instance of Contacts holds the info of one contact. Same as an instance of MobilePhone so it should be named in singular form.

3) misleading method names / redundant logic

The logic of modify and remove is fine but the method names are misleading: there is removeTheContacts() and removeContacts() which give the false impression that they are doing the same. In fact, removeContacts() is actually doing a search for the contact to be removed. same with modifyTheContacts() and modifyContacts() .
and while we are on the subject, why do you require both old contant's name AND number to find it? especially since you have a choice to search for a contact based on name only. Now, if we agree on that, you can use findContactName() when modifying and removing contacts.

• Zephers: Im not to enums yet but I did see it is in future tutorials. I've went back and fixed my naming and my switch. Also my logic on having to search for the name and number was because I was thinking if two contacts with the same name were added you would want to be able to search and verify with phone number as well, I'll go back and look at that. Thank you!
• Raimund Krämer: Do Not Use Constants inside the code sounds different from what you probably mean. Constants (declared with final) are in many cases the solution to be done instead of magic numbers (or strings).