Tipps zum Programmierstil

Hallo, ich bin mir nicht so sicher, ob mein Programmierstil so optimal ist. Ich lerne mit dem Buch "Der C++ Programmierer" und bin jetzt beim Kapitel 4 (OOP). Aufgabenstellung:

In der Kantine stehen Getränkeautomaten, unter anderem einer mit ObjektCola. Der Automat sei so eingestellt, dass eine Dose dieses Getränks 2€ kostet. Der Automat nimmt einen beliebigen Geldbetrag an. Wenn zu wenig eingeworfen wird, wird das eingeworfene Geld wieder herausgegeben. Wenn zu viel eingeworfen wird, wird eine Dose ObjektCola sowie der Restbetrag herausgegeben. Nach Geldeinwurf löst ein Knopfdruck die Prüfung des Geldbetrags und gegebenfalls die Ausgabe einer Dose aus. Wenn keine Dose mehr vorrätig ist, ist der Automat gesperrt, das heißt, dass jeder eingeworfene Geldbetrag vollständig zurückgegeben wird. Das folgende, bewusst einfach gehaltene Szenario soll mit einem Programm simuliert werden.

Ich habe mich jetzt nicht 100%-ig an die Aufgabenstellung gehalten und hab mal drauf los programmiert. Man werde sehen, dass es funktioniert. Meine Frage jedoch ist, wie es mit meinem Stil aussieht. Grauenhaft? Wenn ja, was verbessern? Ich poste dann mal eben den gesamten Quellcode der 5 Dateien. Ich hoffe und denke mal, dass der Code nicht allzu schwierig ist und ihr ohne Kommentare leben könnt. (Schade, dass ich hier keine "Spoilerfunktion" finden kann )

Person.h

#ifndef PERSON_H
#define PERSON_H

#include "Automat.h"

class Person
{
    public:
        Person(int num, int m); // Allg. Konstruktor
        ~Person(); // Destruktor

        // Aktivitäts-Fuktionen
        void muenzen_einwerfen(Automat& a, int m);
        void knopf_druecken(Automat& a);
        void entnehme_rest(Automat& a);
        void trinken();
        void dose_nehmen();
        // Abruf-Funktionen
        int get_muenzen();
        bool genug_muenzen(int m);
    private:
        int id;
        int muenzen;
};

#endif // PERSON_H

Person.cpp

#include "../include/Person.h"
#include <iostream>

// Konstruktor
Person::Person(int num, int m) : id(num), muenzen(m)
{
    std::cout << "Person " << id << " kommt mit " << muenzen << " Muenzen zum Automaten.\n";
}

Person::~Person()
{
    std::cout << "Person " << id << " geht mit " << muenzen << " Muenzen vom Automaten.\n\n";
}

// Elementfunktionen
bool Person::genug_muenzen(int m)
{
    return muenzen >= m; // Gibt "true" zurück, wenn die Person genug Muenzen hat
}

void Person::muenzen_einwerfen(Automat& a, int m)
{
    a.set_muenzenEingabe(m); // Muenzen werden eingeworfen
    muenzen -= m;
}

void Person::knopf_druecken(Automat& a)
{
    if(a.genug_muenzen())
    {
        dose_nehmen();
    }
}

void Person::entnehme_rest(Automat& a)
{
    muenzen += a.get_restMuenzen();
}

void Person::trinken()
{
    std::cout << "\t... trinkt die gekaufte Dose.\n";
}

void Person::dose_nehmen()
{
    std::cout << "\t... nimmt die gekaufte Dose.\n";
}

int Person::get_muenzen()
{
    return muenzen;
}

Automat.h

#ifndef AUTOMAT_H
#define AUTOMAT_H

class Automat
{
    public:
        Automat(int d, int m); // Allg. Konstruktor

        // Abruf-Funktionen
        int get_preis();
        int get_restMuenzen();
        bool ist_gesperrt();
        bool genug_muenzen();
        // Aktivitäts-Funktionen
        void set_muenzenEingabe(int m);
        void gebe_dose();
    private:
        int dosen;
        int preis;
        int muenzen_eingabe;
        int muenzen_rest;
};

#endif // AUTOMAT_H

Automat.cpp

#include "../include/Automat.h"
#include <iostream>

Automat::Automat(int d, int m) : dosen(d), preis(m) {  }

void Automat::set_muenzenEingabe(int m)
{
    muenzen_eingabe = m;
}

bool Automat::genug_muenzen()
{
    bool ergebnis = muenzen_eingabe >= preis;
    if(ergebnis)
    {
        muenzen_rest = muenzen_eingabe - preis;
        gebe_dose();
    }
    else
    {
        muenzen_rest = muenzen_eingabe;
    }
    return ergebnis;
}

void Automat::gebe_dose()
{
    --dosen;
    std::cout << "\t... Dose verkauft. " << dosen << " verbleiben.\n";
}

int Automat::get_restMuenzen()
{
    return muenzen_rest;
}

int Automat::get_preis()
{
    return preis;
}

bool Automat::ist_gesperrt()
{
    return dosen < 1;
}

main.cpp

#include <iostream>
#include "include/Automat.h"
#include "include/Person.h"

int main()
{
    Automat a(30, 3);
    int id = 1;
    while(!a.ist_gesperrt())
    {
        Person p(id, (id * 3) % 4);
        if(p.genug_muenzen(a.get_preis()))
        {
            p.muenzen_einwerfen(a, p.get_muenzen());
            p.knopf_druecken(a);
            p.entnehme_rest(a);
            p.trinken();
        }
        ++id;
    }

    return 0;
}

SeppJ

Sieht ganz ok aus ^*

Ein paar Punke:
- Gewöhne dir const-correctness an. Deine Methoden sollten klar machen, ob sie ein Objekt (auch das eigene!) verändern oder nicht. Gucke dir an, was const in welchem Zusammenhang alles bedeuten kann. Leichte Warnung: Das ist am Anfang nicht einfach. Außerdem kann man es nicht halbrichtig machen. Setzt man ein const, so ist dieses ansteckend und man muss es überall richtig machen. Aber dafür kann der Compiler später viel mehr Logikfehler finden als ohne und dieser Vorteil ist gewaltig.
Das heißt übrigens nicht, jede popelige Variable const zu machen! Mache Memberfunktionen ggf. const und zeige bei Pointer- und Referenzparametern, ob das dahinterliegende Objekt verändert wird oder nicht.
- Ein Pfad wie "../include/" vor Includes ist ungewöhnlich. Standardincludepfade sollten über Compileroptionen gesetzt werden, nicht absolut angegeben werden. Hier gehören nur Pfade relativ zu den Standardsuchpfaden hin.

^*: Alteingesessene Forenmitleser wissen, dass dies wohl das höchste Lob ist, welches von mir jemals auf solch eine Frage als Antwort kam . So drückt man das eben in Westfalen aus. Ein Drei-Sterne-Koch bekommt da auch nur ein "Kann man essen" zu hören .

Also für mich sieht das im Großen und Ganzen in Ordnung aus. Das Buch von Herrn Breymann ist auch wirklich ein tolles Buch. Das ist immer meine absolut erste Wahl.

Vielleicht noch ein paar Dinge, die ich anders machen würde (sollte aber auch im Buch in einem späteren Kapitel noch erwähnt werden):

Vor diese kurzen Funktionen wie

int Automat::get_preis()
{
    return preis;
}

kannst du das Wort "inline" schreiben. Das ist eine Empfehlung für den Compiler, statt die Funktion aufzurufen einfach den Code in der Funktion zu "kopieren". Der Compiler nimmt das aber wie gesagt nur als Empfehlung. Du zwingst ihn damit nicht.

Längere Zeilen wie deine Ausgabe hier

std::cout << "Person " << id << " kommt mit " << muenzen << " Muenzen zum Automaten.\n";

Kann man zur besseren Lesbarkeit auch auf mehrere Zeilen aufteilen. In diesem Fall könnte man zwar einfach die "<<" in die neue Zeile setzen

int main()
{
    std::cout << "Das "
              << "ist "
              << "ein "
              << "Test"
              << std::endl;
    return 0;
}

aber ganz allgemein kannst du Code in mehrere Zeilen trennen, indem du am Ende der Zeile ein einzelnes Backslash ("\") machst.

Aber wie gesagt, das grundliegende stimmt und die Feinheiten kommen, wenn du weiterliest

Gruß Jörg

Eisflamme

JoergK:
Ich würde beiden Vorschlägen nicht zustimmen. Der Compiler erkennt das schon ganz alleine, eigentlich wird inline sowieso einfach ignoriert.

Und aus einer Zeile fünf zu machen, na ja, Geschmackssache...

Eisflamme schrieb:

JoergK:
Ich würde beiden Vorschlägen nicht zustimmen. Der Compiler erkennt das schon ganz alleine, eigentlich wird inline sowieso einfach ignoriert.

Und aus einer Zeile fünf zu machen, na ja, Geschmackssache...

Also inline benutz ich da wo ich es für richtig halte, aber was ich so mitbekommen habe macht das ja ohnehin jeder anders.

Die 5 Zeilen waren oben nur ein Beispiel für die Verwendung - sinnvoll ist das in meinem Beispiel natürlich nicht. Nur aus dieser einen längeren Zeile, die ich vom Threadsteller zitiert hätte, da hätt' ich vielleicht 2 draus gemacht, ums der Länge der anderen etwas anzupasen.

SeppJ schrieb:

Ein paar Punke:
- Gewöhne dir const-correctness an. Deine Methoden sollten klar machen, ob sie ein Objekt (auch das eigene!) verändern oder nicht. Gucke dir an, was const in welchem Zusammenhang alles bedeuten kann. Leichte Warnung: Das ist am Anfang nicht einfach. Außerdem kann man es nicht halbrichtig machen. Setzt man ein const, so ist dieses ansteckend und man muss es überall richtig machen. Aber dafür kann der Compiler später viel mehr Logikfehler finden als ohne und dieser Vorteil ist gewaltig.
Das heißt übrigens nicht, jede popelige Variable const zu machen! Mache Memberfunktionen ggf. const und zeige bei Pointer- und Referenzparametern, ob das dahinterliegende Objekt verändert wird oder nicht.
- Ein Pfad wie "../include/" vor Includes ist ungewöhnlich. Standardincludepfade sollten über Compileroptionen gesetzt werden, nicht absolut angegeben werden. Hier gehören nur Pfade relativ zu den Standardsuchpfaden hin.

Okay, ist angekommen und werde ich ändern :). (Ich glaube das Thema "sicheres Programmieren" kommt sowieso noch im Buch vor.

SeppJ schrieb:

Alteingesessene Forenmitleser wissen, dass dies wohl das höchste Lob ist, welches von mir jemals auf solch eine Frage als Antwort kam . So drückt man das eben in Westfalen aus. Ein Drei-Sterne-Koch bekommt da auch nur ein "Kann man essen" zu hören

Dann werde ich mich wohl mal glücklich schätzen

JoergK schrieb:

Vor diese kurzen Funktionen wie
int Automat::get_preis()
{
    return preis;
}
kannst du das Wort "inline" schreiben. Das ist eine Empfehlung für den Compiler, statt die Funktion aufzurufen einfach den Code in der Funktion zu "kopieren". Der Compiler nimmt das aber wie gesagt nur als Empfehlung. Du zwingst ihn damit nicht.

Ja, ich hab auch darüber nachgedacht, aber dann hätte ich alle Methoden als inline markiert und habs mir deshalb mal gespart. Ist das eigentlich egal, oder bringt das wirklich etwas? Ich hab gelesen, die meisten Compiler arbeiten sowieso selbstständig in der Sache.

Danke für Eure Tipps

SeppJ

JoergK schrieb:

Eisflamme schrieb:

JoergK:
Ich würde beiden Vorschlägen nicht zustimmen. Der Compiler erkennt das schon ganz alleine, eigentlich wird inline sowieso einfach ignoriert.

Und aus einer Zeile fünf zu machen, na ja, Geschmackssache...

Also inline benutz ich da wo ich es für richtig halte, aber was ich so mitbekommen habe macht das ja ohnehin jeder anders.

Das inline an der Stelle ist aber sinnlos. inline wird heutzutage eigentlich nur wegen seine "Nebeneffekte" benutzt, da es Funktionsdefinitionen in Headern ermöglicht. Und es gibt einen Grund, warum man diesen Nebeneffekt hat: Bis vor wenigen Jahren (siehe unten) war es technisch notwendig, dass dem Compiler die Funktionsdefinition auch tatsächlich vorliegt, damit er die Definition direkt im Code einbauen kann. Das heißt bei Funktionsdefinitionen in separaten Übersetzungseinheiten besteht sowieso keine Möglichkeit, dass der Compiler den Code überhaupt inlined. Außer bei Benutzung der Funktion in der gleichen Übersetzungseinheit. Aber dann sagt Eisflamme schon richtig, dass ein optimierender Compiler sich dann sowieso sein eigenes Bild macht und inline oder nicht komplett ignoriert.

Diese Erklärung ist semi-veraltet. Es sind ein paar Techniken aufgekommen, einen weiteren Optimierungsschritt (oder die gesamte Optimierung) zur Linkzeit durchzuführen. Dann kann der Optimierer auch durch Übersetzungseinheiten hindurchgucken. Das ist aber nicht der übliche Weg der Übersetzung und muss normalerweise explizit aktiviert werden und ist meiner Meinung nach teilweise noch etwas frickelig (wird aber schnell besser).

Wenn es also wirklich auf Inlinecalls ankommt (hier ist dies wohl kaum der Fall und sollte nicht gemacht werden), dann sollte man die Funktion auch so definieren, dass die Definition an den entscheidenden Stellen für den Optimierungsschritt sichtbar ist. (D.h. in der Regel inline im Header. Man beachte dass Funktionsdefinitionen in einer Klassendefinition implizit inline sind).

Also die ganzen Ausgaben mit std::cout gehören nicht in die Klasse Person!

styleq schrieb:

Also die ganzen Ausgaben mit std::cout gehören nicht in die Klasse Person!

Warum sollten sie nicht in meine Klasse dürfen? Verstehe dein Problem dabei nicht. Die Klassen sind ausschließlich zum Üben mit Programmierung auf Kommandozeilenbasis, also werden sie später auch nicht für GUI-Programmierung benötigt (sowieso nicht :p).

Cyres

Ich denke, worauf styleq hinauswollte, ist eine strikte Trennung von Anzeige, Speicherung und Agierungsebene, kurz: MVC-Pattern - Model-View-Controller (ein Punkt, worauf das Buch afair nicht explizit eingeht, aber drauf hinarbeitet, dass ich das Buch gelesen habe ist auch schon einige Donnerstage her ), aber mach dir darüber fürs erste erstmal keine Gedanken. Gerade um zu wissen, was der Compiler eigentlich gerade mit deinem Code anstellt sind anfangs Ausgaben auf der Konsole sehr hilfreich, bzw. auch, um zu verstehe, wie C++ eigentlich arbeitet, wann z.B. ein Copy-Constructor aufgerufen wird und wann der Assign-Operator (=) seinen Nutzen findet. Wenn man sowas nicht einmal über die Konsole ausgibt, wird es schwer, es rein theoretisch zu verstehen.

Allgemein kann ich mich allerdings den Vorpostern anschließen, große Mankos im Stil sehe ich jetzt nicht.

Oberon_0

Das Einzige was mir noch auffällt ist eine inkonsistente Benennung der Methoden:
muenzen_einwerfen, knopf_druecken: objekt_verb
entnehme_rest: verb_objekt (vs. rest_entnehmen)
ist_gesperrt: ist-Präfix; genug_muenzen: kein Präfix (hat_genug_muenzen?)

Eisflamme

Vielleicht gibt es da auch Gegenargumente, aber ich finde, dass man _ in einer case-sensitiven Sprache nicht benötigt, sondern _x zu X konvertiert werden sollte (außer in Wörtern, die rein in Majuskeln geschrieben sind). Finde das schöner lesbar.

Cyres

Ein Gegenargument wäre, dass sowohl die STL als auch Boost beide die _-Schreibweise benutzen und man im Endeffekt die Schreibweisen mischt. Allerdings kann man so besser eigene Dinge erkennen, ich benutze eigentlich auch immer lower-CamelCase für Funktionen und Variablen und Upper-CamelCase für Klassen u.Ä.

daddy_felix

Noch eine Kleinigkeit: in der "Person.h" brauchst du nicht das "#include Automat.h". Es genügen in diesem forward-declarations, weil du "Automat" nur als Referenz verwendest. Das "#include Automat" muss nur in "Person.cpp".

Dsa ist in diesem Fall relativ unwichtig, aber wenn du ein größeres Projekt hast, dann kann die Änderung in einer Headerdatei dazu führen, dass sehr viele andere Dateien kompiliert werden. Da ist es aus zeitlichen Aspekten schon sehr hilfreich, wenn in Header-Dateien möglichst wenige includes stehen.

pumuckl

Drei Dinge die mir aufgefallen sind:

Zum Teil überflüssige Kommentare:

dass das Ding mit dem "~" davor ein Destruktor ist weiß man. Da noch extra einen Kommentar dranzuschreiben "//Destruktor" ist etwas zu viel des Guten. Du schreibst ja auch nicht überall dran "das ist eine Variable", "das ist eine Funktion", ... Ein "Kategorie"-Kommentar wie bei den Aktivitäts- und Abruf-Funktionen würde reichen (Konstruktoren, Destruktor, op=)
Kommentare, die etwas ganz offensichtliches beschreiben, sind imo auch überflüssig: "// Gibt "true" zurück, wenn die Person genug Muenzen hat" - hey, die Funktion heißt schon "genug_muenzen", oder? Genauso "// Muenzen werden eingeworfen". Du hast netterweise die Funktionsnamen schon ausführlich genug gewählt, dass die meisten Kommentare überflüssig werden.

Zum Teil zu kurze Namen: die Parameter deiner Funktionen heißen a, m etc. Das ist deutlich zu kurz, benenne sie so, dass man weiß, was dahinter steckt und nicht erst suchen muss. Automat(int d, int m); Hier kann ich aus dem Header nicht erkennen, was die beiden ints bedeuten, die ich dem Automaten übergebe.
Zum Teil fehlende Kommentare: bei beiden Klassen sind der op= und der Copy-Ctor nicht vorhanden. Wenn man sich die Klassen genauer anschaut, erkennt man, dass die Compilergenerierten da auch völlig ausreichend sind, dennoch wäre es noch besser, wenn du bei den anderen Konstruktoren einen kurzen Kommentar hinterlässt, dass du sie bewusst weggelassen hast. Damit man auf den ersten Blick sieht, dass sie nicht vergessen wurden.