Meinung zum Quellcode-Stil (war :Experten Meinung gefragt)

Und du solltest dir darüber klar werden ob du nun mit VCL oder lieber komplett ohne VCL.
Es gibt da ExtractFileExt dür sowas

if (Datei.substr(Datei.size()-4,4)!=".mp3"&&Datei.substr(Datei.size()-4,4)!=".wma"&&Datei.substr(Datei.size()-4,4)!=".wav")
            {

oder FileExists für sowas

if (checker)
            {
               Player->Open();
               checker.close();
            }
            else
            {
                checker.close();
                FLB->Font->Color = clRed;
                if (ListBox1->Count != 1)
                {
                  CLONE4->Click();
                }
                else
                {
                    MessageBox(NULL,"Die Datei Existiert nicht","ERROR",MB_OK|MB_ICONQUESTION);
                }

und z.B. die DateUtils hilft dir bei irgendwelchen Zeitumwandlungen.
Auch hat die VCL eine eigene AnsiString Klasse.
Ansonsten vernünftige eindeutige Namen für deine Objekte
[cppLabel4->...ListBox1-> etc.
[/cpp]
Und viel mehr versuchen soweit wie möglich in extra Funktionen bzw. Methoden auszulagern.(Spätestens wenn du noch ein Image2Click Event hast weisst du das zu schätzen)
Und sehr wichtig halt Quellcode kommentieren. Sonst hat niemand Lust sich da "durchzuwurschteln" und du nach ein paar Monaten auch nicht mehr.

5cript

1. VariablenNamen: Nach der Zeit sind sie mir ausgegangen zum Beispiel plli (Playlist PlayList und Playlist hatte isch schon. FLB steht für
FileListBox.

2. Englische Fehlermeldung: Mach ich unbewusst muss ich mir abgewöhnen

3. Kommentieren: benutzte ich nur zum "Wegkommentieren" es sei denn ich brauche Hilfe aus einem Forum, aber hier gings ja darum mein "Privat-Stil"/Mist zu bewerten. Werde ich in Zukunft auch so machen.

4. Einrückung: Hatte ich schon immer Problemchen mit, dass ist schon noch das Ordentlichste. Aber C++ Builder schlägt mir folgendes vor:

if () {

}

und das finde ich persönlich grausig und finde hinterher gar nicht mehr durch.

5.Mir nicht bekannte Funktionen: Ähm ja sowas wie "ExtractFileExt" kannte ich nicht und hab für mich dann persönlich das Rad neu erfunden.

6. Zeitumwandlung und Variablen 2: Dazu hab ich nix gefunden und einfach meinen eigenen Algorithmus zur umrechnung von sekunden in Sekunden, Minuten und Stunden geschrieben.

Wem es nich aufgefallen ist:
s wie Sekunde
m wie Minute
h wie hour (da fällts aus der reihe --> Eng)
iterahms itera wie iterator (Zählt irgendwas durch) und h m s:
für solche Variablen, die ich nur hochzähle fehlt mir nie ein guter name ein weil sie nur eine beiläufige funktion haben

7. Falsche MessageBox: MB_ICONERROR passt eher sagt ihr? Nun ja das war vorher aber mir nem Kommentar ausgeschnitten und wurde nur zum test gebraucht.
Diese Warnung stört nur wenn man eine ganze Playlist durchhört.

8. Was hat dieser ganze unsinn im PLAY Button zu suchen ?:
Das war meine Lösung um verschiedenes zu verhindern, so wird die Datei immer bei starten der Wiedergabe geöffnet und nicht beim öffnen der Datei selbst.
Das war nötig weil ich 3 verschiedene Möglichkeiten habe eine Audiodatei in mein Programm / in die Playlist zu kriegen. so bin ich "Kein mci gerät geöffnet" Fehler entgangen. Die gesamt Laufzeit wird dann auch auf den Track berechnet.

9. i = int(i) WTF ?: Was ist daran so schlimm ? ich brauchte nur den Ganzahligenteil meiner Variable aber als integer wäre die rechnung glaub ich nicht immer richtig abgelaufen, weil er immer abrundet.

So das war ein kurzes statement zu meinem grausigen Code

EDIT: Rechtschreibung

Nico D.

Du musst dich keineswegs rechtfertigen. Du wolltest unsere Meinung wissen, nun kennst du sie. Es liegt einzig und allein an dir, die gegebenen Ratschlaege oder Kritiken zu berucksichtigen, umzusetzen oder es zu lassen.

_matze

Tim06TR schrieb:

1. VariablenNamen: Nach der Zeit sind sie mir ausgegangen zum Beispiel plli (Playlist PlayList und Playlist hatte isch schon.

Also ich sehe in deinem Code kein Playlist oder PlayList. Innerhalb eines Scopes darfst du Lokale doch ruhig PlayList nennen, auch wenn du in einem anderen Scope schon mal diesen Namen verwendet hast... Oder habe ich dich irgendwie falsch verstanden? Jedenfalls wirst du später wohl kaum auf Anhieb wissen, was denn plli noch gleich war...

Tim06TR schrieb:

3. Kommentieren: benutzte ich nur zum "Wegkommentieren" es sei denn ich brauche Hilfe aus einem Forum, aber hier gings ja darum mein "Privat-Stil"/Mist zu bewerten. Werde ich in Zukunft auch so machen.

Derselbe Grund: damit du in einem Jahr noch weißt, wie dein Code funktioniert, machst du Kommentare. Es muss nicht jede Zeile kommentiert werden, aber zumindest die Funktionsweise einer Funktion und bestimmter (vielleicht nicht auf Anhieb verständlicher) Abschnitte darf mit einem aufklärenden Kommentar versehen werden.

Tim06TR schrieb:

4. Einrückung: Hatte ich schon immer Problemchen mit, dass ist schon noch das Ordentlichste. Aber C++ Builder schlägt mir folgendes vor:
if () {

}

Na, der Builder weiß was gut ist!

Na ja, jeder darf einrücken, wie er will. Wenn dir der gezeigte Stil nicht gefällt, dann pack halt die Klammer in die nächste Zeile. Nur konsistent muss es sein. Wenn du also 2 Zeichen einrückst, dann bleib dabei und wechsel nicht innerhalb des Codes.

5cript

Playlist wurde über das ganze Programm definiert
PlayList ist ein Objekt (eine ListBox)

_matze

Tim06TR schrieb:

Playlist wurde über das ganze Programm definiert
PlayList ist ein Objekt (eine ListBox)

Dennoch wird dir sicher ein sprechenderer Name als plli einfallen, oder?

Btw, Globale sollte man wo möglich vermeiden.

5cript

_matze schrieb:

Btw, Globale sollte man wo möglich vermeiden.

Warum ? Ich hab haufenweise Global definiert, weil ich Variablen in verschiedenen Funktionsaufrufen brauche ? zum Beispiel paused: um zu prüfen ob pausiert ist. mit lokal definieren wird da nichts draus. höchstens mit labelübergaben (was mir da einfiele) aber dann hätte ich ne millonen labels

EDIT: sprechen ich deutsch ?

_matze

Wie wär's mit Member-Variablen deiner Klasse?

asc

Auf jedenfall ist deine Variablenbenennung ein klassisches Beispiel warum ich persönlich die Ungarische Notation gänzlich ablehne (Man kann Variablen auch so benennen das sie alle verstehen - noch kein Entwickler ist an etwas längeren Variablennamen gestorben).

Zudem schließe ich mich mit der Empfehlung der "Member-Variablen" nur an. Kann es sei das du mit dem Codegear C++ Builder angefangen hast, ohne die C++ Grundlagen verinnerlicht zu haben? Ich würde dir jedenfalls dazu raten ein entsprechendes Buch nochmal anzuschauen (Gerade der Teil in den Klassen und Objekte beschrieben werden).

DocShoe

Dann will ich auch mal meine zwei Pfennig beisteuern:

One function - one purpose
Ein Funktion/Methode sollte genau einen einzigen Zweck erfüllen. Ausserdem sollten Funktionen/Methoden nur in Ausnahmefällen länger als eine oder zwei Bildschirmseiten sein. Wenn sie zu lang werden, unterteil das Problem in Teilprobleme und lager die Teilprobleme in eigene Funktionen/Methoden aus.
intuitiv verständliche Variablennamen
Für temporäre Variablennamen in Schleifen kann man ruhig Namen wie a,i oder x benutzen. Man sollte sinnvolle Namen benutzen, wenn der Schleifenrumpf länger ist oder verschachtelt ist. Über den Sinn oder Unsinn der Kenntlichmachung von Member variablen kann man streiten, grundsätzlich bin ich aber für ein Prä- oder Postfix für Member Variablen (zB. m_ voranstellen wie m_Open oder _ anhängen wie bei Open_).
Vermeidung von Schleifen so weit wie möglich, stattdessen Benutzung von Funktionen der Standardbibliothek. Ich weiss nicht genau, von welchem Typ die Variable merken ist, aber wenn es ein Array aus std::string oder AnsiString ist kann man std::fill benutzen.

clearing = 0;
while(clearing < 16800)
{
   clearing++;
   merken[clearing] == "";
}

// oder als Einzeiler
std::fill( merken, merken + 16800, "" );

Vermeidung von C-style Arrays, stattdessen Container aus der STL benutzen (z.B. std::vector). Ausserdem zwischen signed und unsigned Datentypen unterscheiden.
RAII (Resource acquisition is initialisation) Idiom benutzen
Variablen werden bei ihrer Deklaration initialisiert:

// statt
int m;
....
m = s / 60;

// besser
unsigned int Minute = Sekunde / 60;

Vermeiden von C-style casts, stattdessen C++ casts benutzen. Die C-Style casts wandeln ohne zu Murren Äpfel in Birnen um, die C++ Pendants (static_cast, dynamic_cast und reinterpret_cast) sind da schlauer und erlauben keine ungültigen Umwandlungen bzw. warnen den Programmierer bei möglicherweise ungültigen Umwandlungen.
Vermeidung von überflüssigem bzw. verwirrendem Code:

int m;

m = s / 60;
m = (int) m; // überflüssig

Ist irreführend und bewirkt nichts. m ist vom Datentyp integer, ein expliziter C-style cast macht ihn nicht noch "mehr" integer. Wenn du Fließkommazahlen nach integer wandeln willst und dabei auf- oder abrunden willst benutz die entsprechenden Funktionen dazu (ceil/floor).

Auswahl der richtigen Datentypen
In deinem Schnipsel verwendest du die Variablen paused und open als int. Wenn sie nur einen von zwei definierten Zuständen annehmen können sollten sie bool sein. Der Status des Players kann sicher mehrere Zustände haben (Idle, Playing, Paused), da bietet sich ein Aufzählungstyp an:

namespace PlayerState
{
   enum PlayerState_t
   {
      psIdle,
      psPlaying,
      psPaused
   };
}

PS:
Beim erneuten Durchlesen ist mir nicht klargeworden, was der Codeausschnitt aus Punkt 3) eigentlich macht. Das sind 16800 Tests auf einen leeren String, wobei kein Ergbnis berücksichtigt wird. Damit kann ich einen weiteren Punkt anführen, der nur indirekt etwas mit deinem Code zu tun hat.

const Correctness
Deklariere Variablen, deren Inhalt in einer Funktion nicht geändert werden, const. Deklariere Methoden, die den Status eines Objekts nicht ändern, const.
Zu diesem Thema kann man seitenweise schreiben (Stichworte observable state, mutable members), vielleicht findet google ja einige Artikel.

audacia

Tim06TR schrieb:

Aber C++ Builder schlägt mir folgendes vor:
if () {

}
und das finde ich persönlich grausig

Das zugrundeliegende Live-Template kann unter Ansicht|Templates nach Gusto bearbeitet werden.

Tim06TR schrieb:

s wie Sekunde
m wie Minute
h wie hour (da fällts aus der reihe --> Eng)

Stürmer der Netzkultur.

Zum Stil an sich wurde eigentlich alles gesagt; du hast es ja auch selbst passend zusammengefaßt. Die Ausdauer meiner Vorredner beeindruckt mich, mein Ratschlag wäre jedoch: bitte stell den C++Builder zur Seite und lerne BASIC oder Python.

5cript

Doch ich hab zuvor C++ Programmiert, einfache Konsolen Programm nix großes.
Ich bin nie ganz Strukturiert mit dem Lernen Vorgegangen aber, das ist alles "nur"
als reinschauen gedacht. Ich werde bald mit MFC anfangen und dabei solls dann auch bleiben.
Ich kenne da ein ganz nettes Buch, dass ich mir kaufen werde.
Danke für die ganze Tipps und Meinungen. Ich wusste zwar dass mein Code grotte ist
aber was alles (alles) Schlecht ist wollte ich mir nochmal sagen lassen.
Ich habe auch vor später was in der Richtung zu machen, studieren etc.

In der Schule werden wir ja Java machen . Hab ich eigentlich kein interesse dran. Aber ich werde bei C++ bleiben, hoffentlich bringt mich Java dann nicht durcheinander.

PS: Kommt auf den Informatiker Lehrer an, evtl (kann ich || die Anderen die Informatik gewählt haben) dem Lehrer mehr beibringen als er uns. Ja das Angebot bei uns ist eher traurig. Die Leben noch in der Steinzeit.

_matze

Tim06TR schrieb:

Ich werde bald mit MFC anfangen und dabei solls dann auch bleiben.

Gerade dann solltest du dich mit der OOP mal ein wenig mehr auseinandersetzen. Das 'Classes' in MFC steht ja für Klassen, wenn mir nix Falsches erzählt wurde. Da kann es also nicht schaden, wenn man zumindest weiß, wie man Klassenmember anlegt...

_matze

asc schrieb:

Auf jedenfall ist deine Variablenbenennung ein klassisches Beispiel warum ich persönlich die Ungarische Notation gänzlich ablehne (Man kann Variablen auch so benennen das sie alle verstehen - noch kein Entwickler ist an etwas längeren Variablennamen gestorben).

War ja klar, jetzt wird wieder auf der UN rumgehackt. Dabei ist sein Code doch nun wirklich kein Beispiel dafür.

ifstream checker; 
    int panelanpassung; 
    int iterahms; 
    int s; 
    int m; 
    int h = 0; 
    int idex = ListBox1->ItemIndex; 
    idex = idex+1; 
    length[1] = 0; 
    length[2] = 0; 
    zuruk = 0; 
    clearing = 0;

Das sind größtenteils einfach schlechte Bezeichner, mit UN hat das aber nix zu tun.

Btw, wie kommst du eigentlich darauf, dass die UN was mit kurzen Variablenbezeichnern zu tun hat? Es geht doch da nur um ergänzende Angaben. Das heißt noch lange nicht, dass man alles hinter dem Präfix bis zum geht-nicht-mehr abkürzen muss (mach ich auch nicht).

std::string sDiesIstMeinNamensStringUndDerIstTrotzPraefixSuperLangUndGarNichtAbgekuerzt;

5cript

audacia schrieb:

lerne BASIC.

Visual Basic ist Verdammt einfach. Ich habe schon mal Visual Basic Programmiert.
Doch mir fehlt irgendwie die Kenntniss über das Nutzen von Basic. Wo braucht man Basic ?

_matze

Tim06TR schrieb:

audacia schrieb:

lerne BASIC.

Visual Basic ist Verdammt einfach. Ich habe schon mal Visual Basic Programmiert.
Doch mir fehlt irgendwie die Kenntniss über das Nutzen von Basic. Wo braucht man Basic ?

Was für eine Frage! Du kannst mit VB schneller ans Ziel kommen, als mit C++, kannst weniger Fehler machen, die Syntax ist für Anfänger vielleicht leichter zu verstehen usw. Würdest du auch fragen, wozu man C# braucht? VB war übrigens zuerst da.

5cript

Zudem Code:

clearing = 0;
while(clearing < 16800)
{
   clearing++;
   merken[clearing] == "";
}

Muss ich zugeben ich weiß auch nicht mehr wozu das war.
Tja da sieht mans, ich glaube das war eine ursprüngliche Fehlerbehung im falle des wechsels der Playlist, kann glaub ich raus.
// Buh
// oh ein Kommentar

5cript

_matze schrieb:

Tim06TR schrieb:

audacia schrieb:

lerne BASIC.

Visual Basic ist Verdammt einfach. Ich habe schon mal Visual Basic Programmiert.
Doch mir fehlt irgendwie die Kenntniss über das Nutzen von Basic. Wo braucht man Basic ?

Was für eine Frage! Du kannst mit VB schneller ans Ziel kommen, als mit C++, kannst weniger Fehler machen, die Syntax ist für Anfänger vielleicht leichter zu verstehen usw. Würdest du auch fragen, wozu man C# braucht? VB war übrigens zuerst da.

Auch wenn das keine Antwort auf meine Frage war.
Außerdem errinert mich das an "C ein Aprillscherz ?"

DocShoe

Tim06TR schrieb:

Doch ich hab zuvor C++ Programmiert, einfache Konsolen Programm nix großes.
Ich bin nie ganz Strukturiert mit dem Lernen Vorgegangen aber, das ist alles "nur"
als reinschauen gedacht. Ich werde bald mit MFC anfangen und dabei solls dann auch bleiben.
Ich kenne da ein ganz nettes Buch, dass ich mir kaufen werde.
Danke für die ganze Tipps und Meinungen. Ich wusste zwar dass mein Code grotte ist
aber was alles (alles) Schlecht ist wollte ich mir nochmal sagen lassen.
Ich habe auch vor später was in der Richtung zu machen, studieren etc.

In der Schule werden wir ja Java machen . Hab ich eigentlich kein interesse dran. Aber ich werde bei C++ bleiben, hoffentlich bringt mich Java dann nicht durcheinander.

PS: Kommt auf den Informatiker Lehrer an, evtl (kann ich || die Anderen die Informatik gewählt haben) dem Lehrer mehr beibringen als er uns. Ja das Angebot bei uns ist eher traurig. Die Leben noch in der Steinzeit.

Fass die Vorschläge als konstruktive Kritik auf und nimm sie dir zu Herzen. Niemand hat das alles von heute auf morgen gelernt, selbst nach 15 Jahren lernt man noch Sachen dazu, man muss nur wollen.
Im übrigen ist es fast egal, ob du Java oder C++ machst, die Algorithmen bleiben die Gleichen. Du musst dir nur darüber im Klaren sein, dass es zwei verschiedene Programmiersprachen sind, die zwar eine ähnlich Syntax haben, unter der Haube allerdings grundverschieden sind. Wenn du eine neue Programmiersprache lernst, fang bei den Grundlagen an statt sie auszulassen in den Gedanken, dass es bei einer anderen Programmiersprache genauso aussieht und das Verhalten wohl das Gleiche sein wird (Besonders Java Programmierer, die C++ programmieren sollen fallen da in die eine oder andere Fallgrube).

5cript

Teilweise wollte ich damit eigentlich sagen, dass ich die Grundlagen nur zu Hälfte drin hab, das liegt daran, dass ich mir keine Lernvorlage genommen habe

Ich kenne jemand der hat keine Grundlagen gelernt, und der hat gar keine Ahnung was er für Algorithmen schreibt, wenn ihr versteht was ich meine.

Der hat sofort mit VCL angefangen ohne irgendeine Vorkenntnis und er überhäuft mich mit Fragen ,dass ist also noch ein Stück härter, somit versteh ich schon
was ihr mir Mitteilen wollt.