mal was kleines selbst geschrieben (neuling)...was sagt ihr dazu?

chris' 1

huhu..bin derzeit am c++ lernen und versuche ein bisschen anwendungspraxis zu bekommen, daher hab ich einfach mal irgendwas (sinnfreies, kleines) geschrieben, und wollte euch mal ganz gerne fragen ob soweit alles richtig angewandt und der stil ok ist...
wäre cool wenn sich mal jemand den source anschauen könnte und mir dazu was sagen würde...also ich hoffe es stört daher nich wenn ich mal den kompletten source poste

autohaus.cpp:

// Projektname: Autohaus
// Filename: autohaus.cpp

#include <iostream>
#include "auto.hpp"

using namespace std;

// main-Funktion
int main ()
{
    // Menü
    // Variablen
    int   Auswahl = 0;    // Menüauswahl
    CAuto Auto1;          // ein Auto

    do
    {
        // Menü anzeigen
        cout << "\nAutohaus-Menue:\n";
        cout << "---------------\n";
        cout << "1 - Auto erstellen\n";
        cout << "2 - Autodaten anzeigen\n";
        cout << "3 - Auto umlackieren\n";
        cout << "4 - Programm beenden\n";
        // Auswahl abfragen
        cout << "\nAuswahl: ";
        cin >> Auswahl;

        // Auswahl verzweigen
        switch (Auswahl)
        {
            // Auto erstellen?
            case 1:
            {
                Auto1.erstelleAuto ();
            } break;

            // Autodaten anzeigen?
            case 2:
            {
                // checken, ob Auto exisiert
                if (Auto1.existiertAuto () == true)
                {
                    // falls ja, führe gewünschten Menüpunkt aus
                    Auto1.zeigeAuto ();
                }
                else
                {
                    // falls nein, fordere User auf zunächst ein Auto zu erstellen
                    cout << "\nErstelle zunaechst ein Auto!" << endl;
                }
            } break;

            // Auto ändern?
            case 3:
            {
                // checken, ob Auto exisiert
                if (Auto1.existiertAuto () == true)
                {
                    // falls ja, führe gewünschten Menüpunkt aus
                    Auto1.neueFarbe ();
                }
                else
                {
                    // falls nein, fordere User auf zunächst ein Auto zu erstellen
                    cout << "\nErstelle zunaechst ein Auto!" << endl;
                }
            } break;

            // Programm beenden?
            case 4:
            {
                cout << "\nProgramm beendet." << endl;
            } break;

            // default für falsche Eingabe
            default:
            {
                cout << "\nFalsche Eingabe." << endl;
            }
        }
    } while (Auswahl != 4);

    return 0;

} // main-Funktion

auto.hpp:

// Filename: auto.hpp

// Klassen

// Auto
class CAuto
{
    private:

        // Membervariablen
        char m_chHersteller[21];      // Hersteller
        char m_chModell[21];          // Modell
        char m_chFarbe[11];           // Farbe
        int  m_Hubraum;               // Hubraum (CCM)
        int  m_Leistung;              // Leistung (PS)

    public:

        // Memberfunktionen (deklaration)
             CAuto ();              // Konstruktor
        void erstelleAuto ();       // Auto erstellen
        void zeigeAuto ();          // Autodaten anzeigen
        void neueFarbe ();          // neue Farbe für das Auto
        bool existiertAuto ();      // existiert überhaupt schon ein Auto?
};

auto.cpp

// Filename: auto.cpp
#include <iostream>
#include "auto.hpp"

using namespace std;

// Memberfunktionen (defininieren)

// Konstruktor
     CAuto :: CAuto ()
{
    m_chHersteller[0] = '\0';
    m_chModell[0]     = '\0';
    m_chFarbe[0]      = '\0';
    m_Hubraum         = 0;
    m_Leistung        = 0;

} // Konstruktor

// erstelleAuto
// User nach Daten für das Auto fragen
void CAuto :: erstelleAuto ()
{
    // Daten für das Auto abfragen
    cout << "\nHersteller des Autos: ";
    cin.ignore ();
    cin.get (m_chHersteller, 20);
    cout << "Modell des Autos      : ";
    cin.ignore ();
    cin.get (m_chModell, 20);
    cout << "Farbe des Autos       : ";
    cin.ignore ();
    cin.get (m_chFarbe, 10);
    cout << "Hubraum in ccm        : ";
    cin >> m_Hubraum;
    cout << "Leistung in ps        : ";
    cin >> m_Leistung;

} // erstelleAuto

// zeigeAuto
// User die Daten des Autos zeigen
void CAuto :: zeigeAuto ()
{
    cout << "\nHersteller: " << m_chHersteller << endl;
    cout << "Modell    : " << m_chModell << endl;
    cout << "Farbe     : " << m_chFarbe << endl;
    cout << "Hubraum   : " << m_Hubraum << " ccm" << endl;
    cout << "Leistung  : " << m_Leistung << " ps" << endl;

} // zeigeAuto

// neueFarbe
// dem Auto eine neue Farbe verpassen
void CAuto :: neueFarbe ()
{
    char chAuswahl = '\0';      // Auswahl ja/nein
    char chAuswahlMotor = '\0'; // Auswahl Hubraum/leistung
    // änderbare Daten des Autos abfragen 
    // Farbe? 
    do
    {
        cout << "\nDas Auto ist momentan " << m_chFarbe << "." << endl;
        cout << "Soll das Auto umlackiert werden (j / n)? ";
        cin >> chAuswahl;

        if (chAuswahl != 'j' && chAuswahl != 'J' &&
            chAuswahl != 'n' && chAuswahl != 'N')
        {
            cout << "\nFalsche Eingabe." << endl;
        }
        else if (chAuswahl == 'j' || chAuswahl == 'J')
        {
            cout << "In welcher Farbe soll das Auto neulackiert werden? ";
            cin.ignore ();
            cin.get (m_chFarbe, 10);
            cout << "Das Auto strahlt nun in " << m_chFarbe << "." << endl;
        }
    } while (chAuswahl != 'j' && chAuswahl != 'J' &&
             chAuswahl != 'n' && chAuswahl != 'N');

} // neueFarbe

// existiertAuto
// überprüfen, ob überhaupt schon ein Auto exisiert
bool CAuto :: existiertAuto ()
{
    // überprüfen, ob Auto exisiert
    if (m_chHersteller[0] == '\0' && m_chModell[0] == '\0' &&
        m_chFarbe[0] == '\0' && m_Hubraum == 0 && m_Leistung == 0)
    {
        // falls nein, return false
        return false;
    }
    else
    {
        // falls ja, return true
        return true;
    }
} // existiertAuto

ist alles sehr simpel gestrikt, von daher sollte das wohl jeder lesen können hoffe ich

danke vielmals

Korbinian

ganz hübsch einiges was mir nicht gefällt ist das große C vor dem Klassennamen. Damit kommt man (wenn man keine namespaces verwendet) leicht mit den MFC von Microsoft in Konflikt (dort beginnen alle Klassen mit einem C). Ach ja: Include-guards fehlen. Diese verhindern, dass du später Konflikte bekommst, wenn du die Datei mehrfach einbindest. Dazu sollte deine Headerdatei so aussehen:

#ifndef CAUTO_H_
#define CAUTO_H_

// header

#endif

Der Name ist natürlich frei wählbar, sollte aber eindeutig und zuordbar sein "__IRGENDWAS__" ist afaik für den Compiler reserviert, aber das wissen die Freaks hier besser als ich

ethereal

naja, das sieht doch schon prima aus.
doppelter unterstrich sollte imho tatsächlich nicht am anfang stehen, aus o.g. grund.
ein anderes ding find ich persönlich nicht optimal, aber wahrscheinlich ist es für deine zwecke am sinnvollsten. aber vielleicht mal als anregung:
ich fänds übersichtlich und kompakt, wenn die auto-klasse tatsächlich nu die daten über das auto verwaltet oder diese daten modifiziert. die ein-/ausgabe könntest du in der main machen.
dann bist du mit der auto-klasse unabhängig vom ausgabe-stream und kannst sie z.b. auch verwenden, wenn du dein auto in directx oder was weiß ich brauchst
durch das cout bindest du sie an die iostream-klasse, das find ich nicht so schön. ansonsten vorbildlicher code.

Dafür das du ein Anfänger bist ist dein Stil schon recht übersichtlich, da gibts wirklich Leute die sind 9999999999999x schlimmer als du Weiter so
Nur die Kommentare sind teilweise überflüssig (Wenn eine Variable Hersteller heißt, wieso das noch dahiter schreiben)

MFK

Zu deiner Methode existiertAuto: Ein Konstrukt der Art

if(Bedingung)
{
    return true;
}
else
{
    return false;
}

ist unnötig umständlich und lang.

return Bedingung;

mach genau das gleiche.

finix

Korbinian schrieb:

Der Name ist natürlich frei wählbar, sollte aber eindeutig und zuordbar sein "__IRGENDWAS__" ist afaik für den Compiler reserviert, aber das wissen die Freaks hier besser als ich

Reserviert sind Namen die
2 aufeinander folgende Unterstriche enthalten
mit einem Unterstrich gefolgt von einem Großbuchstaben beginnen
im globalen Namensraum mit einem Unterstrich beginnen

Bezüglich Include-Guards wüsste ich nicht was gegen ein schlichtes
"(NAMESPACE_)*(CLASSNAME_|FILENAME_)(HPP|CPP)" sprechen würde..

chris' 1

finix schrieb:

Bezüglich Include-Guards wüsste ich nicht was gegen ein schlichtes
"(NAMESPACE_)*(CLASSNAME_|FILENAME_)(HPP|CPP)" sprechen würde..

das hier versteh ich leider noch nich so ganz.
aber ansonsten danke für euer feedback, werde den code in den von euch genannten punkten mal n bissl optimieren...

danke&bye,

chris

jerry

Für einen Anfänger ist das wirklich prima. Dennoch einen kleinen Hinweis:

do cin >> auswahl; while (auswahl < 1 || auswahl > 4);

verhindert unsinnige Eingaben und ist sicherer.

Mach weiter so!
Gruss Jerry

chris' 1

jerry schrieb:

Für einen Anfänger ist das wirklich prima. Dennoch einen kleinen Hinweis:

do cin >> auswahl; while (auswahl < 1 || auswahl > 4);

verhindert unsinnige Eingaben und ist sicherer.

Mach weiter so!
Gruss Jerry

huhu

also meinst wäre es so besser:

do 
        {
            cin >> Auswahl; 
            if (Auswahl < 1 || Auswahl > 4)
            {
                cout << "Falsche Eingabe." << endl;
            }

        } while (Auswahl < 1 || Auswahl > 4);

und den rest der rest des menüs kann so bleiben? also mit der do-while schleife und dem switch-case?
den default block kann ich dann ja weglassen, da er dort dann eh nich reinkommt oder?

danke

Cpt_Future

ethereal schrieb:

ich fänds übersichtlich und kompakt, wenn die auto-klasse tatsächlich nu die daten über das auto verwaltet oder diese daten modifiziert. die ein-/ausgabe könntest du in der main machen.

Hallo,
ich bin selbst noch Anfänger in C++ (eher PHP- oder C#-Schiene), aber das sieht mir echt nach übersichtlichem Code aus.

Was ethereal geschrieben hat stimmt, eine Klasse sollte immer unabhängig vom Ausgabe-Medium sein.

Mein ganz persönlicher Geschmack ist aber englische Begriffe für Klassen, Variablen, Funktionen, etc. zu verwenden.

if (Auto1.existiertAuto()) .. // if (car1.Exists()) ..

find ich eben nicht optimal. Allerdings können dem natürlich auch fehlende/schwache Englischkenntnisse im Wege stehen. Aber wofür gibts LEO?

Zwei andere Punkte finde ich unschön:

// ..
case XY: { 
// code
}
break;
// ..

Wozu die Klammern? Zuviele Klammern machen das ganze meiner Meinung nach unübersichtlicher.

Punkt 2 ist die Schreibweise von Methoden/Funktionen. Du schreibst z.B.

if (Auto1.existiertAuto () == true) // besser if (car1.Exists()) ..

Ein Leerzeichen zwischen Name und () wird üblicherweise nur bei Ausdrücken (if (), switch (), ..) verwendet um schon im Quellcode zu verdeutlichen, dass es sich nicht um eine Methode/Funktion (Auto1.ExistiertAuto(), int main()..) handelt (zumindest kenne ich das so von den PHP Coding Standards) sondern eben um einen Ausdruck.

Ein Vergleich per

var == true
var == false

ist ebenfalls unnötig, da der Vergleichswert ja offenbar sowieso bool ist. Daher also lieber

if (Auto1.existiertAuto())
// oder
if (!Auto1.existiertAuto())

schreiben.

Hört sich nach viel Kritik an, aber du wolltest ja Verbesserungsvorschläge & es sind ja schließlich nur Vorschläge & wie gesagt: Dein Code ist schon sehr gut Weiter so.

Gruß c

RHBaum

Da hier alle am "verbessern" sind :

using namespace std;

gewoehn dir das gar nich erst an !
Namespaces sind dazu da, benutzt zu werden, und nicht um global ausgehebelt zu werden ...

das schreiben von std::cout anstatt cout usw. macht nur kleine muehe im gegensatz zu dem aerger, den nen schlecht angewoehntes Aushebeln der namespaces bringt ...

Und im gegenteil, willst deine Autohaus dinger mal spaeter weiter verwenden, fuer eine menge anderer Programe, beispiel ne Verwaltung, Berechnungsprog oder so ... und die klassen demnach in diverse libs auslagern, solltest die gleich in nen eigenen namespace werfen ...

deine strings:

char m_chHersteller[21]; zum beispiel

Sowas bringt dir imense vorteile, wenn deine Daten binaer serialisieren musst ... also ueber nen stream (datenstrom) schicken musst.
Anwendungsfaelle dafuer sind: Lesen / schreiben in binaere (nich menschlich lesbare) Dateien, Datenaustausch zwischen prozessen (" getrennte programme auf einem rechner, oder unterschiedliche rechner, Netzwerk aller art)

Wenn das nich brauchst, solltest dynamische allokierte strings nehmen, mit ner passenden Stringklasse ... da ja die STL schon nimmst, schau dir vielleicht basic_string<> an.

Nen festnageln von Strings auf feste maximale laengen, bringt frueher oder spaeter, wenn dein code denn weiterverwendet wird, immer irgendwann Probleme bzw einschraenkungen.
Am Anfang bei kleineren Projekten sieht es immer lapidar aus wenn man sagt, ok, dann muss mer halt mal neu kompilieren ...
wenn dein zeugs von zig anwendungen gebraucht wird, und alles bisserl komplizierter wird, iss neukompilieren teilweisse das overkill kriterium ... wenn ne komplizierte versionierung und ne menge clients dranhaengen ....

Also sollt man von vornherein solche "Probleme" zumindest mit nem kritischen auge erst mal betrachten ...

Ciao ...

chris' 1

also statt char-arrays lieber strings nehmen? werd ich machen

btw: danke an alle noch mal für die kritik

Apollon

Was spricht eigentlich gegen ein

using std::cout;

z.B.?

terraner

RHBaum schrieb:

Da hier alle am "verbessern" sind :
using namespace std;
gewoehn dir das gar nich erst an !
Namespaces sind dazu da, benutzt zu werden, und nicht um global ausgehebelt zu werden ...

Dem würde ich so nicht zustimmen. In Headerdateien gehört soetwas meiner Meinung nach gründsätzlich verboten, weil man nicht kontrollieren kann, wo diese Datei im Endeffekt überall included wird.
Aber in Source-Files spart es zeit, besonders bei langen namespace-bezeichnern (z.b. boost::filesystem), auch wenn namespace-Aliase benutzt werden können. Spätestens aber wenn sich der namespaces ändert hat man sich damit allerdings auf die Füße getreten(ohne Alias). Außerdem passiert es nicht oft, dass man aus mehreren namespaces gleichnamige Funktionen/Konstanten innerhalb einer einzigen anderen Funktion benötigt.

mfg

RHBaum schrieb:

Da hier alle am "verbessern" sind :
using namespace std;
gewoehn dir das gar nich erst an ! ...

Seh ich nicht so, nur in Header-Datein, hat das using generell nichts verloren.
Auf einen Nameclash wird dich der Compiler normalerweise hinweisen. Und falls,
es Probleme gibt ist da immer noch das "eingeschränkte" using gemäss Apollon
möglich.
Bei Konstrukten die sich aus mehreren Bestandteilen der STL zusammen setzen
kann es ohne using ziemlich schnell lang und unübersichtlich werden.

terraner

finix schrieb:

"(NAMESPACE_)*(CLASSNAME_|FILENAME_)(HPP|CPP)" sprechen würde..

öhm... cpp?

Vielleicht sollte man noch das Datum mit reinbringen, denn die Wahrscheinlichkeit, dass man mehrere Klassen mit dem selben Namen am gleichen Tag implementiert, ist viel geringer, als die sich einmal im namespace zu irren/etc.

mfg

RHBaum

Dem würde ich so nicht zustimmen. In Headerdateien gehört soetwas meiner Meinung nach gründsätzlich verboten, weil man nicht kontrollieren kann, wo diese Datei im Endeffekt überall included wird.

Genau ... ok, ich schreib viel als templates / header implementierte Klassen, von daher vielleicht ...

Fuer XTausend mal in namespaces geschachtelte Klassen, wenn ich mir wirklich die finger bei brechen wuerd, gaebs immer noch typdefs. Ausserdem helfen einem die editoren da, sofern man nich mitm notepad schreibt :p

Wobei mehrfach geschachtelte namespaces hab ich eh extrem selten ...
Mit was fuer Libs arbeitet ihr denn ? :p

Wenn Ich Code durchschaue, und ich seh nen z.b.
ComboBox
muss ich erst an den anfang scrollen um das
using ATLControls
z.B zu finden.
steht dagegen gleich nen ATLControls::ComboBox (ich hab mir die finger echt nich bei verletzt grad ) weiss ich gleich mit was genau ich es zu tun hab ...

Ok, bei den meisten sachen isses allein schon durch den Context definiert, weil das jemand ATL Controls und Controls ner anderen GUI-Lib verwendet in einem Project, iss eher selten

Ciao ...

groovemaster

Juhuu, endlich mal jemad der weiss, dass es auf der Tastatur auch eine Leertaste gibt. Wobei man nicht unbedingt überall Leerzeichen machen muss. ZB beim Scope Operator oder den Funktionsklammern ist es imo unnötog.

//also statt
CAuto :: CAuto ()
// durchaus so
CAuto::CAuto()

Ansonsten, bis auf die angesprochenen Kleinigkeiten, recht gut für den Anfang.

RHBaum schrieb:

Da hier alle am "verbessern" sind :
using namespace std;
gewoehn dir das gar nich erst an !
Namespaces sind dazu da, benutzt zu werden, und nicht um global ausgehebelt zu werden ...

Grundsätzlich stimme ich dir da zu, gerade wenn man mit mehreren Bibliotheken arbeitet. Bei solchen Miniprogrammen ist das aber durchaus ok.

HumeSikkins

RHBaum schrieb:

Wenn Ich Code durchschaue, und ich seh nen z.b.
ComboBox
muss ich erst an den anfang scrollen um das
using ATLControls
z.B zu finden.
steht dagegen gleich nen ATLControls::ComboBox (ich hab mir die finger echt nich bei verletzt grad ) weiss ich gleich mit was genau ich es zu tun hab ...

Oder du besorgst dir eine moderne Entwicklungsumgebung. So mit kontext-sensitiver Hilfe, Source-Browser usw.

Btw:
Vorausgesetzt, dass wir von using-Direktiven in cpp-Dateien als erstes nach allen includes sprechen, halte ich Kommentare wie diesen

das schreiben von std::cout anstatt cout usw. macht nur kleine muehe im gegensatz zu dem aerger, den nen schlecht angewoehntes Aushebeln der namespaces bringt ...

für sehr fragwürdig, denn sie tun so, als gäbe es wirklich handfeste Gründe gegen using-Direktiven in cpp-Dateien. Aber gibt es die wirklich?
Welchen Ärger kann eine using-Direktive denn in einer cpp-Datei bringen? Das schlimmste was passieren kann ist eine freundliche Beschwerde des Compilers und in diesem Fall kann ich immer noch auf die using-Direktive verzichten. Problemlos, da das Problem vollständig lokal ist.

using-Direktiven in cpp-Dateien sind eine reine Frage des Stils.
Die einen (so wie ich) empfinden das ganze explizite Namespacegescope (besonders bei Standard-Typen) als unnötiges syntaktisches Rauschen. Die anderen finden das toll und hilfreich. Es gibt letztlich aber keinen technischen Grund für oder gegen sie (mal davon abgesehen, dass sie Compilern mit ADL-Problemen manchmal helfen).
Ganz im Gegensatz zu using-Direktiven in Headern. Die sind objektiv schlecht.

Es gibt ja noch eine Alternative für namespace-Inkludierung, nämlich innerhalb eines {}-Blocks:

void fn()
{
    using namespace std;
    using std::cout;

    //...
}

So sehe ich aber eher selten.