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

[akari]

Hallo

Es gibt keinen Grund Quellcode in eine exe zu packen.
Und einen sinnvollen Threadtitel hättest du dir auch ausdenken können.

bis bald
akari

[5cript]

ich habs bisher nur mal geschafft ein prog hochzustellen in nem anderen forum, dass versteckt sich von selbst und dann verbinde ich mit dem Prog und danach haben sich 20 Typen beschwert das sich ihre Pc's heruntgefahren sind .
Nein für jeden Download krieg ich Punkte .
Und das war alles ernst gemeint

[5cript]

das ist ein installer der alles kopiert INNO Setup. 1. der Quelltext ist über 750 Zeilen lang und unkomprimiert wäre alles zu groß. so sinds nur 6.19 MB

[akari]

Hallo

- Wenn wir hier den Quellcode begutachten sollen, dann brauchen wir auch nur den Quellcode und keine Binaries.
- 750 Zeilen Quellcode sind nur ein paar Kilobyte
- Für Quellcode braucht man keinen Installer, da reicht ein normales ZIP-Archiv.

bis bald
akari

[5cript]

weil ihr euch so anp****

HIER:
http://ul.to/ci2pcu

[Rostfrei**]

wieso anp*?
Ich kann mich ja täuschen, aber ich glaub, dass Du hier Hilfe suchst; und mit einer solchen Formulierung ist dies nicht sonderlich hilfreich.

im übrigen fehlt die cc3290mt.dll und wird vmtl. nicht ohne starten.

[5cript]

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

[5cript]

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

[Badestrand]

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

[Nico D.]

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.

[_matze]

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.

[tipgeber]

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