Quellcode verbessern

[Maxga]

Guten Tag,
hatte für die Schule die simple Aufgabe, einen Ticketautomaten ohne GUI zu modellieren, also nur in der Konsole.
Wollte nun fragen, ob mir jemand Verbesserungsmaßnahmen zeigt, sofern vorhanden, bitte?

Hier mal mein Quellcode:

Klasse Fahrschein:

public class Fahrschein
{
    private int preis;
    private String name;
    
    public Fahrschein(String name)
    {
        if(name.equals("AB"))
        {
            preis = 210;
            this.name = "AB";
        }
        if(name.equals("BC"))
        {
            preis = 250;
            this.name = "BC";
        }
        if(name.equals("ABC"))
        {
            preis = 280;
            this.name = "ABC";
        }
    }
    
    
    public void drucken()
    {
        System.out.println("\"Ich bin eine Fahrkarte vom Typ " + name + " und koste " + preis + " cent.\"");
    }
    
    public String getName()
    {
        return name;
    }
    
    public int getPreis()
    {
        return preis;
    }
}

Klasse Konto:

public class Konto
{
    int gesamtGuthaben = 1000000;
    
    
    /** Gibt den in den Parametern angegebenen Geldbetrag, 
     * sofern er gueltig ist, aus.
     */
    public int geldAbheben(int x)
    {
        int abgehoben = 0;
        switch(x)
        {
            case 1:
            abgehoben = 1;
            gesamtGuthaben -= 1;
            break;
        
            case 2:
        
            abgehoben = 2;
            gesamtGuthaben -= 2;
            break;
        
            case 5:
            abgehoben = 5;
            gesamtGuthaben -= 5;
            break;
        
            case 10:            
            abgehoben = 10;
            gesamtGuthaben -= 10;
            break;
            
            case 20:
            abgehoben = 20;
            gesamtGuthaben -= 20;
            break;
            
            case 50:
            abgehoben = 50;
            gesamtGuthaben -= 50;
            break;
            
            case 100:
            abgehoben = 100;
            gesamtGuthaben -= 100;
            break;
            
            case 200:
            abgehoben = 200;
            gesamtGuthaben -= 200;
            break;
            
            default:
            abgehoben = 1;
            gesamtGuthaben -= 1;
            break;
        }
        return abgehoben;
    }
    
    public int getKontostand()
    {
        return gesamtGuthaben;
    }   
}

Klasse Automat:

/** Fahrkartenautomat v.1.0
 * @author Marcel Koseler und Dominic Belza
 * @version 10.01.09
 */

import java.util.*; 

public class Automat
{
   private int eingeworfenesGeld;
   private int schonBezahlt = 0;
   private int restbetrag;
   private int wechselgeld;
   private static int anzahlTickets = 1;
   
   private Fahrschein f;
   static Konto konto = new Konto();
   Scanner eingabe = new Scanner(System.in);
   
   /** Setze alle Werte auf 0/null zurueck*/
   public void zuruecksetzen()
   {
       f = null;
       schonBezahlt = 0;
       restbetrag = 0;
       eingeworfenesGeld = 0;
   }
   
   
   
   /** Pruefe, ob eine weitere Fahrkarte erwuenscht ist*/
   public void weitereFahrkarte()
   {
        try
        {
            System.out.println("\nWollen sie eine weitere Fahrkarte kaufen?\nJa/Nein");
            
            while(eingabe.hasNext())
            {
                String auswahl = eingabe.next();  

                if(auswahl.equalsIgnoreCase("Nein"))
                {
                    anzahlTickets -= 1;
                    eingabe.close();
                    System.out.println("\nBVG wuenscht ihnen weiterhin einen schoenen Tag.");
                    System.exit(0);
                }
                else if(auswahl.equalsIgnoreCase("Ja"))
                {
                    break;
                }
                else
                {
                    System.out.println("Bitte Ja oder Nein eingeben.");
                }
            }
        }
        catch (InputMismatchException e)
        {
            System.out.println("Bitte mit Ja oder Nein antworten.");
            eingabe.nextLine();                                         // "BUG" Der Reader springt nicht in die naechste Zeile, darum der Befehl
        } 
    }
   
    
    
   /** Wähle die Art des Tickets(1, 2 oder 3) und erstelle ein Objekt der jeweiligen Ticketart.
    * Gebe in der Konsole die gewählte Art und den Preis aus.
    * Entspricht die Eingabe nicht 1,2 oder 3, so gebe aus, dass es sich um eine ungültige Angabe handelt.
    */      
   public void ticketAnfordern()
   {
        System.out.println("Guten Tag, wählen sie ihr Ticket:\n1.AB für 210 cent.\n2.BC für 250 cent.\n3.ABC für 280 cent.");
        
        while(f == null)
        {
            try
            {
                int ticketArt = eingabe.nextInt();
                if(ticketArt == 1)
                {
                    f = new Fahrschein("AB");
                }
                else if(ticketArt == 2)
                {
                    f = new Fahrschein("BC");
                }
                else if(ticketArt == 3)
                {
                    f = new Fahrschein("ABC");
                }
                else
                {
                    System.out.println("Keine gültige Angabe. Bitte geben sie 1,2 oder 3 an.");
                }
            }
            catch(InputMismatchException e)
            {
                System.out.println("Keine gültige Angabe. Bitte geben sie 1,2 oder 3 an.");
                eingabe.nextLine();                                                         // "BUG" Der Reader springt nicht in die naechste Zeile, darum der Befehl
            }
         }System.out.println("\nSie haben " + f.getName() + " gewählt. Bitte bezahlen sie " + f.getPreis() + " cent.");
    }
   
    
    
    /** 
     * Fordere ein Ticket an.
     * Gebe die zu bezahlende Menge in der Konsole aus. Warte dann auf Eingaben.
     * Entspricht die Eingabe dem zu bezahlenden Preis, "drucke die Fahrkarte" und beende das Programm. Achte dabei auf gueltige Betraege.
     * Gebe den zu bezahlenden Restbetrag aus, wenn zu wenig bezahlt wurde.
     * Ist die Eingabe höher als der zu bezahlende Preis, "drucke die Fahrkarte", "gebe das Wechselgeld in gueltigen Betragen aus", und beende das Programm.
     */
    public void systemAblauf()
    {
        ticketAnfordern();
        while(eingabe.hasNext())
        {
            int eingabeSpeicher = eingabe.nextInt();
            if(eingabeSpeicher == 1 || eingabeSpeicher == 2 || eingabeSpeicher == 5 || eingabeSpeicher == 10 || eingabeSpeicher == 20 || eingabeSpeicher == 50 || eingabeSpeicher == 100 || eingabeSpeicher == 200)
            {
                eingeworfenesGeld = eingabeSpeicher;
                schonBezahlt = schonBezahlt + eingeworfenesGeld;                                
                restbetrag = f.getPreis() - schonBezahlt;
                
                if (schonBezahlt < f.getPreis())
                {
                    System.out.println("Noch zu bezahlen: " + restbetrag + " cent");
                }
                if (schonBezahlt > f.getPreis())
                {
                    wechselgeld = schonBezahlt - f.getPreis();
                    System.out.println("Ihr Wechselgeld:");
                    int i = 0;
                    while(wechselgeld > 0)
                    {
                        if(wechselgeld >= 200)
                        {
                            konto.geldAbheben(200);
                            wechselgeld -= 200;
                            System.out.println("200 cent");
                        }
                        if(wechselgeld >= 100)
                        {
                            konto.geldAbheben(100);
                            wechselgeld -= 100;
                            System.out.println("100 cent");
                        }
                        if(wechselgeld >= 50)
                        {
                            konto.geldAbheben(50);
                            wechselgeld -= 50;
                            System.out.println("50 cent");
                        }
                        if(wechselgeld >= 20)
                        {
                            konto.geldAbheben(20);
                            wechselgeld -= 20;
                            System.out.println("20 cent");
                        }
                        if(wechselgeld >= 10)
                        {
                            konto.geldAbheben(10);
                            wechselgeld -= 10;
                            System.out.println("10 cent");
                        }
                        if(wechselgeld >= 5)
                        {
                            konto.geldAbheben(5);
                            wechselgeld -= 5;
                            System.out.println("5 cent");
                        }
                        if(wechselgeld >= 2)
                        {
                            konto.geldAbheben(2);
                            wechselgeld -= 2;
                            System.out.println("2 cent");
                        }                        
                        if(wechselgeld >= 1)
                        {
                            konto.geldAbheben(1);
                            wechselgeld -= 1;
                            System.out.println("1 cent");
                        }
                       
                    }
                    f.drucken();
                    break;
                }
                
                if (schonBezahlt == f.getPreis())
                {
                    f.drucken();
                    break;
                }
            }
            
            else
            {
                System.out.println("BIEP BIEP! FALSCHGELD! Bezahlen sie bitte mit echtem Geld!");
            }
        }
    }
    
    
    
    public static void main(String[] args)
    {
        Automat a = new Automat();
        while(anzahlTickets == 1)
        {
            if (konto.getKontostand() >= 210)
            {
                a.systemAblauf();
                a.zuruecksetzen();
                a.weitereFahrkarte();    
            }
            else
            {
                System.out.println("Fahrkartenautomat ist außer Betrieb");
                anzahlTickets -= 1;
            }
        } 
    }
}

Soweit so gut, hoffe auf Hilfe =)

MfG

 

[SlaterB]

niemals Dinge unnötig wiederholen

if(name.equals("AB"))
{
preis = 210;
this.name = "AB";
}
if(name.equals("BC"))
{
preis = 250;
this.name = "BC";
}
if(name.equals("ABC"))
{
preis = 280;
this.name = "ABC";
}


kann man einfacher schreiben als


if(name.equals("AB"))
{
preis = 210;
}
if(name.equals("BC"))
{
preis = 250;
}
if(name.equals("ABC"))
{
preis = 280;
}
this.name = name;
(wieso nicht this.preis statt preis?)

--------

switch(x)
{
case 1:
abgehoben = 1;
gesamtGuthaben -= 1;
break;
usw.

hier wirds noch schlimmer, kann nicht der ganze switch-Block wegfallen und stattdessen EINMAL

abgehoben = x;
gesamtGuthaben -= x;
stehen?
(evtl. bis auf den default-Fall)

---------

if(wechselgeld >= 2)
{
konto.geldAbheben(2);
wechselgeld -= 2;
System.out.println("2 cent");
}

wieder das gleiche, wobei es durch das >= kompliziert wird,
hier könnte man die Zahlen in ein Array schreiben und das per Schleife durchlaufen, das ist aber recht kompliziert,

schau dir als Zwischenform erstmal

test = 100;
if(wechselgeld >= test)
{
konto.geldAbheben(test);
wechselgeld -= test;
System.out.println(test+" cent");
}
test = 50;
if(wechselgeld >= test)
{
konto.geldAbheben(test);
wechselgeld -= test;
System.out.println(test+ " cent");
}

an, siehst du wie jede entscheidene Zahl nur maximal einmal auftaucht?
Variablen zu wiederholen ist weit weniger schlimm

der Weg zur Schleife ist nun nicht mehr weit

 

[Maxga]

Erstmal zu dem Switch-Fall:
Dass kann so nicht dort stehen, da nur mit gueltigen Betraegen bezahlt werden soll, darum muss die Switch-Anweisung, oder etwas vergleichbares, bleiben.
Das mit dem name, ok stimmt, hab ich geändert.
Das mit der test-Variable hab ich net verstanden, der Algorithmus ist ja dazu da, damit er nur gueltige Betraege auch auszahlt, je nachdem wieviel noch da ist,
also vergleiche ich ob wechselgeld > bzw. = 200, 100, 50 etc. ist. Verstehe net, wieso da noch eine Variable nötig ist, bzw. was an meinem Quellcode unnötig ist.
this.preis hab ich da nicht geschrieben, weil es in der Klasse nur eine einzige Variable mit dem Namen Preis gibt, da dachte ich mir war das this unnötig, weils klar ist.

MfG

 

[SlaterB]

> Dass kann so nicht dort stehen, da nur mit gueltigen Betraegen bezahlt werden soll, darum muss die Switch-Anweisung

wie wärs mit
if (x == 1 || x == 2 ||x == 5 || ..) {
abgehoben = x;
gesamtGuthaben -= x;

}

oder halt, ein switch ist da doch schöner, aber läßt sich auch wie folgt schreiben:

switch(x)
{
case 1:
case 2:
case 5:
case 10:
case 20:
case 50:
case 100:
case 200:
abgehoben = x;
gesamtGuthaben -= x;
break;

default:
abgehoben = 1;
gesamtGuthaben -= 1;
break;
}


------------

> Verstehe net, wieso da noch eine Variable nötig ist, bzw. was an meinem Quellcode unnötig ist.

das ist eine allgemeine Regel,
wenn du irgendwann mal eine 20 in eine 25 ändern musst, dann nur an einer Stelle,
kannst nicht den Fehler begehen

if(wechselgeld >= 25)
{
konto.geldAbheben(25);
wechselgeld -= 25;
System.out.println("20 cent");
}

zu schreiben, also eine Stelle zu vergessen,
mag an dieser Stelle nicht sehr wahrscheinlich sein,

 

[Maxga]

Ok hab das mit dem switch geändert.

Bei der Sache mit dem Wechselgeld, sollte ich besser eine Variable festlegen, und der vor jeder if-Abfrage einen neuen Wert zum vergleichen geben? Ist das nicht umständlicher, als einfach gleich den Wert zum vergleichen zu schreiben?

MfG

 

[SlaterB]

wie gesagt wäre die Endform eigentlich

for (wert aus sortierter Liste) {
  if(wechselgeld >= wert) 
  { 
       konto.geldAbheben(wert); 
       wechselgeld -= wert; 
       System.out.println(wert+ " cent"); 
   } 
}

was aber relativ aufwendig ist und ich deshalb nicht unbedingt fordere,
insofern macht die Einzelvariante in der Wiederholung wenig Sinn, weil das normalerweise niemand schreiben würde,
ich finde es aber zumindest besser, als jede Zahl 4x zu wiederholen, ja

-------

übrigens:
in die Schleife gehört noch ein break; / bei vielen ifs solltest du die mit else verknüpfen,
sonst wird für ein Rückgeld von 4 Euro
erst
2 Euro abgezogen, dann 1 Euro, dann 50 Cent usw.

statt 2x 2 Euro in zwei while-Schleife-Durchläufen

 

[Ark]

@Maxga: Algorithmen sind festgelegte Reihenfolgen von Anweisungen. Allerdings reicht das nicht. Vier wesentliche Eigenschaften gibt es, die eine Folge von Anweisungen erfüllen muss, damit sie sich Algorithmus nennen darf:

1. allgemein
2. ausführbar
3. eindeutig
4. endlich

Code wie dein hier erster strebt prinzipiell gegen unendlich viele Anweisungen: jede Scheingröße wird gesondert behandelt, und da es prinzipiell abzählbar unendlich viele verschiedene Scheingrößen gibt, ist die Anzahl der Anweisungen ebenfalls abzählbar unendlich. Aber selbst, wenn ich jetzt sagen würde, dein Code wäre endlich (was er in einer gewissen Weise auch ist), so ist er nicht allgemein genug: Er verarbeitet nur endlich viele Scheingrößen, obwohl es prinzipiell unendlich abzählbar viele gibt.

Ark

EDIT: SlaterBs Pseudocode erfüllt dagegen z.B. alle geforderten Eigenschaften.