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

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


  • ??? hat man die dlls nicht normalerweise wenn man BCB hat ?
    rtl120
    vcl120
    cc3290mt.dll
    borlndmm.dll

    EDIT: ach ich vergesse immer wieder das nicht alle BCB 09 haben ^^

    EDIT2: Ich lad sie mal hoch

    EDIT3:

    HIER SIND SIE:
    http://ul.to/cadovq

    EDIT 4: Sry bin grad etwas mies drauf 😡 :p

    0

  • wie wärs wenn wir mit ein paar extrakten beginnen...
    (ich sehe es läd ja eh keiner runter)

    Also:
    der Play Button: (Achtung etwas lang und unübersichtlich)

    void __fastcall TForm1::Image1Click(TObject *Sender)
{
	Player->TimeFormat = tfMilliseconds;
	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;
	while(clearing < 16800)
	{
		clearing++;
		merken[clearing] == "";
	}
	ifstream existing;
	std::string check;
	if (plli == 1)
	{
	   if (open == 1 && paused == 0)
	   {

			Datei = selecteditem;
			if (Datei.substr(Datei.size()-4,4)!=".mp3"&&Datei.substr(Datei.size()-4,4)!=".wma"&&Datei.substr(Datei.size()-4,4)!=".wav")
			{
			  MessageBox(NULL,"Unbekanntes Datei Format","ERROR",MB_OK|MB_ICONQUESTION);
			  ListBox1->ItemIndex = ListBox1->ItemIndex+1;
			  selecteditem = playlistpath[(ListBox1->ItemIndex)+1].c_str();
			}
			Datei = selecteditem;
			songID = ListBox1->ItemIndex+1;
			DateiName = NAMES[songID];
			Label4->Caption = NAMES[idex].c_str();
			Player->FileName = selecteditem.c_str();
			releasepath = Player->FileName.c_str();
			checker.open(Player->FileName.c_str());
			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);
				}

			}
			panelanpassung = NAMES[idex].size();
			if (paused == 0)
			{
				ScrollBar1->Max = Player->Length;
				ScrollBar3->Max = Player->Length;
		   //	Player->Position = 0;
			}
			Player->Play();
			MPSetVolume(Player, ScrollBar2->Position);
			playing = 1;
			Panel1->Width = panelanpassung*10+(panelanpassung*2)+(panelanpassung*2)+20;
	   }
	   if (paused == 1)
	   {
		  Player->Play();
	   }
	   if (open == 0&&plli == 0)
	   {
		  MessageBox(NULL,"Sie haben keine Datei geöffnet","ERROR",MB_OK|MB_ICONQUESTION);
	   }
	}
	if (plli == 2)
	{

	  std::string LbL;
	  if (paused == 0)
	  {
		Datei = FLB->FileName.c_str();
		if (Datei.substr(Datei.size()-4,4)!=".mp3"&&Datei.substr(Datei.size()-4,4)!=".wma"&&Datei.substr(Datei.size()-4,4)!=".wav")
		{
		  // MessageBox(NULL,"Unbekanntes Datei Format","ERROR",MB_OK|MB_ICONQUESTION);
			  FLB->ItemIndex = FLB->ItemIndex + 1;

		}
		 Player->FileName = FLB->FileName.c_str();
		 releasepath = FLB->FileName.c_str();
		 Player->Open();
	  }
	  pliop = 1;
	  Player->Play();
	  MPSetVolume(Player, ScrollBar2->Position);
	  playing = 1;
	  Label4->Caption = FLB->FileName;
	  if (paused == 0)
	  {
         ScrollBar1->Max = Player->Length;
		 ScrollBar3->Max = Player->Length;
	  }
	  LbL = Label4->Caption.c_str();
	  panelanpassung = LbL.size();
	  Panel1->Width = panelanpassung*10+(panelanpassung*2)+(panelanpassung*2)+20;
	}
	paused = 0;
	if (open == 1||plli == 2)
	{
		   Label9->Caption = releasepath.c_str();
		   convert.clear();
		   medialength = Player->Length;
		   medialength = medialength / 1000;
		   hms = medialength;
		   iterahms = 0;
		   convert.clear();
		   iterahms++;
		   s = hms;
		   gest = "";
		   m = s/60;
		   m = int(m);
		   uberlauf = m*60;
		   differenz = s - uberlauf;
		   s = differenz;
		   if (m > 60)
		   {
			 h = m / 60;
			 h = int(h);
			 uberlauf = h*60;
			 differenz = m - uberlauf;
			 m = differenz;
		   }
		   if (h<10)
		   {
			 convert << "0";
			 convert << h;
		   }
		   if (h >= 10)
		   {
			 convert << h;
		   }
		   convert << ":";
		   if (m < 10)
		   {
			 convert << "0";
			 convert << m;
		   }
		   if (m >= 10)
		   {
			 convert << m;
		   }
		   convert << ":";
		   if (s<10)
		   {
			 convert << "0";
			 convert << s;
		   }
		   if (s >= 10)
		   {
			 convert << s;
		   }
		   convert >> gest;
		   convert.clear();
	}
	if (open == 1 || plli == 2)
	{
	   paused = 0;
	   playing = 1;
	}
}

    Ist das elegant gelöst ? Ich sag mal im voraus nein.
    Wenn Fragen beantwort ich gern

    0
  • B

    Was mir auf den ersten Blick nicht gefällt:
    - Nichtssagende Variablennamen (keine Ahnung was "m" oder "iterahms" bedeuten soll. Und die Datei "checker" prüft irgendwas? Hä?).
    - Variablen alle am Anfang deklariert. Das macht man bei C89 so, in C++ deklariert man sie idR da, wo sie das erste mal gebraucht werden (oder weiter außen falls es Scope-Probleme gibt).
    - Funktion zu lang und kein einziger Kommentar. Nicht mal, was in dieser riesigen Funktion eigentlich ungefähr passiert.

    if (h<10)
{
  convert << "0";
  convert << h;
}
if (h >= 10)
{
  convert << h;
}

// =>
if ( h < 10 )
  convert << "0";
convert << h;

    edit: Zweiter Blick:
    - Mal englische, mal Deutsche Bezeichner
    - Rechtschreibfehler in den Fehlermeldungen
    - Fragezeichen in der MessageBox bei Fehlermeldungen? Wieso nicht das Error- oder Warning-Zeichen?
    - Inkonsistente Einrückungen, mal zwei mal vier Zeichen. Ich vermute du verwendest Tabs (was ich gut finde), hast aber ab und zu Leerzeichen in der Einrückung drin.

    • h = int(h); <- Was zur Hölle...?
    • FLB->ItemIndex = FLB->ItemIndex + 1; ++-Operator plz
    0

  • Tim06TR schrieb:

    Ist das elegant gelöst ? Ich sag mal im voraus nein.

    Damit hast du doch deine Frage bereits selbst beantwortet. Und ich stimme dem zu. Mich schaudert es bereits, den gesamten Code erblicken zu muessen. 🙄

    0
  • _

    Ehrlich gesagt habe ich sowieso nicht verstanden, wieso jemand deinen ganzen Quellcode braucht, um deinen Stil zu bewerten...

    Im Übrigens schließe ich mich der bisherigen Kritik an.

    0
  • ?

    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.

    0

  • 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

    0

  • 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. 😉

    0
  • _

    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.

    0

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

    0
  • _

    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.

    0

  • _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 ? 😃

    0
  • _

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

    0
  • A

    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).

    0

  • Dann will ich auch mal meine zwei Pfennig beisteuern:

    1. 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.

    2. 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_).

    3. 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, "" );

    1. Vermeidung von C-style Arrays, stattdessen Container aus der STL benutzen (z.B. std::vector). Ausserdem zwischen signed und unsigned Datentypen unterscheiden.

    2. 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;

    1. 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.

    2. 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).

    1. 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.

    1. 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.
    0
  • A

    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.

    0

  • 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.

    0
  • _

    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...

    0
  • _

    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;
    0

  • 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 ?

    0
Anmelden zum Antworten