Bewertet meinen Code - Part 1

[software]

Hallo,
ich bin dabei mir selber das programmieren beizubringen.
Allerdings würde ich immer gerne meinen Code verbessern.
Was würdet ihr hier anders/besser machen?

Es geht im Grunde darum, eine Kontaktliste auf dem Handy zu verwalten.
Man kann Kontakte hinzufügen, updaten, löschen, anzeigen, suchen.

Hier der Code:

Spoiler: Klasse Contact

public class Contact {
    private String name;
    private String number;

    public Contact(String name, String number){
        this.name=name;
        this.number=number;
    }

    public String getName() {
        return name;
    }

    public String getNumber() {
        return number;
    }
}

Spoiler: Klasse MobilePhone

import java.util.ArrayList;
import java.util.Scanner;

public class MobilePhone {

    private ArrayList<Contact> contact = new ArrayList();
    private static Scanner scanner = new Scanner(System.in);

    public void printContactList(){
        for(int i = 0; i< contact.size(); i++){
            System.out.println("Name: " + contact.get(i).getName() + "\t" + "Number: " + contact.get(i).getNumber());
        }
    }

    public void storeContact(String name){
        if(findContact(name)){
            System.out.println("Contact already exists");
        }
        else{
            System.out.print("Enter number: ");
            String number = scanner.nextLine();
            contact.add(new Contact(name, number));
            System.out.println("Contact " + name + " stored");
        }
    }

    
public void updateContact(String name){
    if(!findContact(name)){
        System.out.println("No contact exists with that name!");
    }
    else{
        int pos = getPosOfContact(name);
        System.out.print("Enter new name: ");
        name = scanner.nextLine();
        if(findContact(name)){
            System.out.println("Contact already exist. Update wasn't successful.");
        }
        else{
            System.out.print("Enter new number: ");
            String number = scanner.nextLine();
            contact.set(pos, new Contact(name, number));
            System.out.println("Contact updated successfully!");
        }
    }
}

    public void removeContact(String name){
        if(!findContact(name)){
            System.out.println("No contact exists with that name!");
        }
        else{
            int pos = getPosOfContact(name);
            contact.remove(pos);
            System.out.println("Contact " + name + " removed!");
        }
    }

    public boolean findContact(String name){
        for(int i =0;i<contact.size();i++){
            if(contact.get(i).getName().equals(name)){
                return true;
            }
        }
        return false;
    }

    public String getContactInfo(String name){
        for(int i =0;i<contact.size();i++){
            if(contact.get(i).getName().equals(name)){
                return "Contact " + contact.get(i).getName() + " has number " + contact.get(i).getNumber();
            }
        }
        return "That contact doesn't exist!";
    }

    public int getPosOfContact(String name){
        for(int i =0;i<contact.size();i++){
            if(contact.get(i).getName().equals(name)){
                return i;
            }
        }
        return -1;
    }
}

Spoiler: Main

import java.util.Scanner;

public class Main {

    public static void main(String[] args) {
        Main.printMenu();
        Scanner scanner = new Scanner(System.in);
        String name="",number="";
        boolean quit=false;
        MobilePhone mp =new MobilePhone();
        while(!quit){
            switch(scanner.nextInt()){
                case 1:
                    quit=true;
                    break;
                case 2:
                    mp.printContactList();
                    break;
                case 3:
                    System.out.print("Enter name: ");
                    scanner.nextLine();
                    name = scanner.nextLine();
                    mp.storeContact(name);
                    break;
                case 4:
                    System.out.print("Enter name to update: ");
                    scanner.nextLine();
                    name = scanner.nextLine();
                    mp.updateContact(name);
                    break;
                case 5:
                    System.out.print("Enter name to remove: ");
                    scanner.nextLine();
                    name = scanner.nextLine();
                    mp.removeContact(name);
                    break;
                case 6:
                    System.out.print("Enter name to find: ");
                    scanner.nextLine();
                    name = scanner.nextLine();
                    System.out.println(mp.getContactInfo(name));
                    break;
                default:
                    quit=true;
                    break;
            }
        }
    }

    public static void printMenu(){
        System.out.println("1: Quit");
        System.out.println("2: Print list of contacts");
        System.out.println("3: Add new contact");
        System.out.println("4: Update existing contact");
        System.out.println("5: Remove Contact");
        System.out.println("6: Search/Find Contact");
        System.out.println("---------------------------------\n\n");
    }
}

 

[mihe7]

MobilePhone kümmert sich sowohl um die Kontaktverwaltung als auch um das UI. Das ist nicht gut.

 

[software]

MobilePhone kümmert sich sowohl um die Kontaktverwaltung als auch um das UI. Das ist nicht gut.

Kannst du das genauer beschreiben? Ich verstehe nicht, was du meinst.
Was ist nicht gut?

 

[LimDul]

Kannst du das genauer beschreiben? Ich verstehe nicht, was du meinst.
Was ist nicht gut?

Die Klasse macht zwei Dinge:
- Sie kümmert sich darum Eingaben vom Benutzer entgegen zu nehmen
- Sie kümmert sich darum Kontakte zu ändern bzw. generell mit der Kontaktliste zu interagieren.
Stell dir mal vor, du willst das Kommandozeilen Interface beibehalten, aber zusätzlich eine graphische Gui drauf setzen. Dann hast du ein Problem, weil die gesamte Verwaltung der Liste in deiner Klasse, die das Kommandozeilen Interface enthält drin ist. Die musst du noch mal schreiben. Daher sollte man das trennen.

PS: Es ist auch ein Bug drin. Versuch mal von einem Kontakt nur die Telefonnummer zu ändern, das geht nicht, weil er meckert das der Kontakt schon da ist.

Ansonsten eine weitere Anmerkung:
- Name soll ja der Primärschlüssel für Kontakte sein. Dann sollte deine Klasse Contact sowohl equals als auch hashCode entsprechend überschreiben.

 

[kneitzel]

Was ich noch etwas sehe:
a) Die Kontakte werden in einer ArrayList gespeichert, die private ist (korrekt!), aber du hast eine public methode, um eine Position von einem Kontakt zu bekommen? Die Position hat was für einen Wert außerhalb? Denn sonst greifst Du nur über den Namen zu, so ich nichts übersehen habe.
b) doppelter Code: Du hat in vielen Methoden eine Iteration über alle Kontakte um einen Kontakt zu finden? Dafür hast Du doch eine Methode. Du kannst einfach die Position des Elements abfragen und wenn die Position nicht -1 ist, hast du das Element gefunden ... Und warum suchst Du erst, ob der Kontakt da ist um dann die Position zu holen? Die Position ist -1 wenn der Kontakt nicht da ist. Daher ist das ein doppeltes Durchlaufen durch das Array das unnötig ist.

Und hier bieten sich Streams teilweise an. Also das Durchgehen der Kontakte um einen Kontakt zu finden wäre doch super parallelisierbar 🙂

 

[mihe7]

Kannst du das genauer beschreiben? Ich verstehe nicht, was du meinst.
Was ist nicht gut?

Sorry für die späte Anwort - @LimDul hat es bereits erklärt.

Wenn ich mir Deinen Code ansehe, weiß ich nicht, was MobilePhone darstellen soll. Soll es die Oberfläche eines Mobiltelefons sein? Das wäre denkbar. Oder soll das Code sein, der Kontakte verwaltet? Auch das wäre denkbar.

Man sieht also schon daran, dass etwas unklar ist.

Kommt nun der Anwender daher und sagt: "Mensch, mach das UI doch etwas schöner" oder änder den Tippfehler oder... dann musst Du die Klasse MobilePhone anpassen. Kommt dagegen Dein Auftraggeber um die Ecke und sagt: "Mensch, die Kontakte im RAM zu halten ist blöd, speicher das mal ab", dann musst Du ebenfalls die Klasse MobilePhone anpassen.

Die Klasse ist also für zwei Personenkreise verantwortlich, die ihre eigenen Anforderungen haben. Das führt dann dazu, was @LimDul geschrieben hat, nämlich dass sich nicht einfach mal ein weiteres UI hinzufügen oder das bestehende austauschen lässt. Erschwerend kommt hinzu, dass von einer UI-Änderung ggf. auch noch Main betroffen wäre.

 

[Xyz1]

Ich würd das weiter aufteilen

Spoiler

class MNumber {
	String mnumber;
	MNumber(String mnumber) {
		if (mnumber == null || mnumber.isBlank())
			throw new RuntimeException("");
		this.mnumber = mnumber;
	}
}

class Contact {
	String firstname;
	String surname;
	ArrayList<MNumber> numbers = new ArrayList<MNumber>();
	Contact(String firstname, String surname) {
		if (firstname == null || firstname.isBlank() || surname == null)
			throw new RuntimeException("");
		this.firstname = firstname;
		this.surname = surname;
	}
	void addNumber(MNumber number) {
		numbers.add(number);
	}
}

class ContactsBook {
	ArrayList<Contact> contacts = new ArrayList<Contact>();
	void addContact(Contact c) {
		contacts.add(c);
	}
}

class UI {
	public static final Scanner SCANNER = new Scanner(System.in);
	ContactsBook book = new ContactsBook();
	void a_newContact() {
		try {
			System.out.println("first name:");
			String first = SCANNER.nextLine();
			System.out.println("second name:");
			String second = SCANNER.nextLine();
			System.out.println("number name:");
			String number = SCANNER.nextLine();
			MNumber n = new MNumber(number);
			Contact c = new Contact(first, second);
			c.addNumber(n);
			book.addContact(c);
		} catch (RuntimeException re) {
			re.printStackTrace();
		}
	}
	void b_listAllContacts() {
		// TODO
	}
}

class Main1 {
	public static void main(String[] args) {
		UI ui = new UI();
		ui.a_newContact();
		ui.b_listAllContacts();
	}
}

 

[software]

Ich bin noch am Anfang meiner Programmierkarriere. Ich erarbeite mir alles Schritt für Schritt.
Deswegen sieht mein verbesserter Code jetzt so aus.

Spoiler: Contact

public class Contact {
    private String name;
    private String number;



    public Contact(String name, String number){
        this.name=name;
        this.number=number;
    }

    public String getName() {
        return name;
    }

    public String getNumber() {
        return number;
    }

    public void setName(String name) {
        this.name = name;
    }

    public void setNumber(String number) {
        this.number = number;
    }
}

Spoiler: MobilePhone

import java.util.ArrayList;
import java.util.Scanner;

public class MobilePhone {

    private String number;
    private ArrayList<Contact> contacts;

    public MobilePhone(String number){
        this.number=number;
        this.contacts = new ArrayList<Contact>();
    }


    public int findContact(String name){
        for(int i=0;i<contacts.size();i++){
            if(contacts.get(i).getName().equals(name)){
                return i;
            }
        }
        return -1;
    }

    public Contact getContact(String name){
        for(int i=0;i<contacts.size();i++){
            if(contacts.get(i).getName().equals(name)){
                return contacts.get(i);
            }
        }
        return null;
    }

    public ArrayList<Contact> getContacts() {
        return contacts;
    }
}

Spoiler: Main

import java.util.Scanner;

public class Main {

    public static void main(String[] args) {

        Scanner scanner = new Scanner(System.in);
        String name="",number="";
        Contact currentContact;
        int contactPosition=-1;
        boolean quit=false;
        MobilePhone mobilePhone = new MobilePhone("123456789");

        Main.printMenu();
        while(!quit){
            switch(scanner.nextInt()){
                case 1:
                    Main.printMenu();
                    break;
                case 2:
                    for(Contact contact: mobilePhone.getContacts()){
                        System.out.println("Name: " + contact.getName() + "\tNumber: " + contact.getNumber());
                    }
                    break;
                case 3:
                    System.out.print("Enter name: ");
                    scanner.nextLine();
                    name = scanner.nextLine();
                    if(mobilePhone.findContact(name)>=0){
                        System.out.println("Contact already exist. Contact hasn't been added!");
                    }
                    else{
                        System.out.print("Enter number: ");
                        number = scanner.nextLine();
                        mobilePhone.getContacts().add(new Contact(name,number));
                        System.out.println("Contact has been added.");
                    }
                    break;
                case 4:
                    System.out.print("Enter name to update: ");
                    scanner.nextLine();
                    name = scanner.nextLine();
                    contactPosition=mobilePhone.findContact(name);
                    if(contactPosition==-1){
                        System.out.println("Contact doesn't exist.");
                    }
                    else{
                        System.out.print("Enter new name: ");

                        String newName = scanner.nextLine();
                        if(mobilePhone.findContact(newName) >=0){
                            System.out.println("Contact already exists. Contact hasn't been added.");
                        }
                        else{
                            currentContact = mobilePhone.getContact(name);
                            currentContact.setName(newName);
                            System.out.println("Contact name has been updated.");
                        }
                    }
                    break;
                case 5:
                    System.out.print("Enter contact name to edit number: ");
                    scanner.nextLine();
                    name = scanner.nextLine();
                    contactPosition=mobilePhone.findContact(name);
                    if(contactPosition==-1){
                        System.out.println("Contact doesn't exist.");
                    }
                    else{
                        System.out.print("Enter new number: ");
                        number = scanner.nextLine();
                        currentContact = mobilePhone.getContact(name);
                        currentContact.setNumber(number);
                        System.out.println("Contact number has been updated.");
                    }
                    break;
                case 6:
                    System.out.print("Select the name you want to delete: ");
                    scanner.nextLine();
                    name = scanner.nextLine();
                    contactPosition=mobilePhone.findContact(name);
                    if(contactPosition==-1){
                        System.out.println("Contact doesn't exist.");
                    }
                    else{
                        mobilePhone.getContacts().remove(contactPosition);
                        System.out.println("Contact " + name + " has been removed.");
                    }
                    break;
                case 7:
                    System.out.print("Enter the name you are looking for: ");
                    scanner.nextLine();
                    name = scanner.nextLine();
                    contactPosition=mobilePhone.findContact(name);
                    if(contactPosition==-1){
                        System.out.println("Contact doesn't exist.");
                    }
                    else{
                        currentContact = mobilePhone.getContact(name);
                        System.out.println("Contact " + currentContact.getName() + " has the number " + currentContact.getNumber() +".");
                    }
                    break;
                case 8:
                    quit=true;
                    break;
                default:
                    Main.printMenu();
                    break;
            }
        }
    }

    private static void printMenu(){
        System.out.println( "1: Print Menu\n" +
                            "2: Print list of contacts\n" +
                            "3: Add new contact\n" +
                            "4: Update contact name\n" +
                            "5: Update contact number\n" +
                            "6: Remove contact\n" +
                            "7: Find contact\n" +
                            "8: Quit\n" +
                            "---------------------------------");
    }
}

 

[mihe7]

Das sieht von der Aufteilung her schon wesentlich besser aus. Du darfst aus Deinem Menü ruhig ein Objekt machen, gerne auch mit eigene(r|n) Klass(e|en). Die main-Methode dient in erster Linie als "Programmeinstiegspunkt" - damit die Java Virtual Machine weiß, wo es los geht.