Code Analyse von (semi) Profis

[bennySB]

Bitte schlagt mich nicht das ich euch mit so nem riesen Batzen an Code auf einmal zuballer^^

Ich wollte nur einmal die Profis unter euch ansprechen wie gut oder wie grässlich mein Code denn zu lesen ist und ob es vielleicht Verbesserungsvorschläge gibt. Hauptsächlich geht es um die Form des Codes, es gibt von den Funktionen her bestimmt auch Verbesserungen aber
damit soll sich hier nun keiner auseinander schlagen müssen (sowas kann man niemanden zumuten xD)^^

Ich bin offen für Zuckerbrot und Peitschen :-P


Ps. Es sei dazu gesagt das es mein allererster Code ist und für ein Setup gedacht ist das ich mit dem Programm "Inno Setup" erstellt habe^^

Delphi-Quellcode:

    var                                                                             //Globale Variablenvergabe
    UserDirPageCAE : TInputDirWizardPage;       
    DOSBoxConf : TStringList;     
    GPSDirCheckBox : TRadioButton;    
    SplashImage: TBitmapImage;    
    SplashForm: TForm;       
    a, b, c, d : word;   
    SetupMajor, SetupMinor, SavedMajor, SavedMinor : dword;
    
    UserFolderCAE, tmp, SplashFileName, DOSBoxCAE2000Path, DOSBoxCAE2000UninstallString,
    DOSBoxCAE2000UninstallPath, DOSBoxCAE2000UninstallName, DOSBoxCAE2000UninstallPathRemovedQuotes,
    DOSBoxCAE2000UninstallNameRemovedQuotes, DOSBoxCAE2000UninstallStringRemovedQuotes, CAE2000Path,
    GPSPath, GetCAE2000Path : string;
    
    GPSDirCheckBoxChecked, ResultDOSBoxCAE2000PathCheck, ResultAktuelleAppFound, ResultDOSBoxCAE2000DontDeinstall : boolean;    
    I: Integer;

    
    function CreateDWord(const Hi, Lo: word): dword;                                //Registry-Schlüssel für die Versionsnummer
      
      begin
        Result := (Hi shl 16) or Lo;
      end;

    
    function DecodeVersion(const dwMajor, dwMinor: dword): string;                  //Entschlüsselung der Versionsnummer

      begin
        a := word(dwMajor shr 16);
        b := word(dwMajor and not $ffff0000);
        c := word(dwMinor shr 16);
        d := word(dwMinor and not $ffff0000);
        Result := Format('%d.%d.%d.%d',[a,b,c,d])
      end;

    
    function IsSetupNewer: boolean;                                                 //Versionsabgleich der vorhandenen Installation zur neuen Installation
    
    var
    ResultCode : integer; 
      
      begin      
        if  (not RegQueryDWordValue(HKLM,'{#UNINSTKEY}','MajorVer',
            SavedMajor)) or
            (not RegQueryDWordValue(HKLM,'{#UNINSTKEY}','MinorVer',
            SavedMinor)) then
            begin
            if  (RegQueryStringValue(HKLM,'{#UNINSTKEY}',
                'UninstallString',tmp)) and
                (tmp <> '') and
                (fileexists(tmp)) then
              begin
                Result := (MsgBox(ExpandConstant('{cm:NotVerifiedVersionFound}'),mbConfirmation,MB_YESNO or MB_DEFBUTTON2) = IDYES);
                           exit;
              end;
            end;
      
        SetupMajor := CreateDWord({#MAJOR},{#MINOR});
        SetupMinor := CreateDWord({#RELEASE},{#BUILD});
      
        //Neuere Version ist installiert
        Result := (SetupMajor > SavedMajor) or ((SetupMajor = SavedMajor) and (SetupMinor >= SavedMinor));
        //Gleiche Version ist bereits installiert
        ResultAktuelleAppFound := (SetupMajor = SavedMajor) or ((SetupMajor = SavedMajor) and (SetupMinor = SavedMinor));

        //Ermitteln und Umformen des Deinstallationspfades
        RegQueryStringValue(HKLM,'{#UNINSTKEY}','UninstallString',DOSBoxCAE2000UninstallString);
        DOSBoxCAE2000UninstallPath := (ExtractFileDir(DOSBoxCAE2000UninstallString) + '\' + '"');
        DOSBoxCAE2000UninstallName := ('"' + ExtractFileName(DOSBoxCAE2000UninstallString));
        DOSBoxCAE2000UninstallStringRemovedQuotes := RemoveQuotes(DOSBoxCAE2000UninstallString);
        DOSBoxCAE2000UninstallPathRemovedQuotes := RemoveQuotes(DOSBoxCAE2000UninstallPath);
        DOSBoxCAE2000UninstallNameRemovedQuotes := RemoveQuotes(DOSBoxCAE2000UninstallName);


        if not Result then
          begin              
            if RegQueryStringValue(HKLM,'{#UNINSTKEY}','InstallLocation',DOSBoxCAE2000Path) then
              begin
                result := DirExists(DOSBoxCAE2000Path);
              end;  
              if not Result then 
                begin
                  //Installationspfad der vorhandenen Installation kann nicht ermittelt werden
                  MsgBox(ExpandConstant('{cm:DOSBoxPathNotFound}'),mbError,MB_OK);
                  //Abbruch der Installation wenn der Pfad akteuelle Installationspfad nicht ermittelt werden konnte
                  ResultDOSBoxCAE2000PathCheck := false;
                end;
            //Messagebox das eine aktuellere Version installiert ist                             
            if MsgBox(ExpandConstant('{cm:NewerAppFound}'),mbError,MB_YESNO or MB_DEFBUTTON2) = IDYES then
              begin
                //Silent-Ausführung der Deinstallation wenn aktuellere Version installiert ist und Benutzer dem zustimmte
                Exec(ExpandConstant(DOSBoxCAE2000UninstallStringRemovedQuotes),'/SILENT','',SW_SHOW,ewWaitUntilTerminated,ResultCode);
              end else
                //Abbruch des Setups sobald der Benutzer sich gegen eine Deinstallation der aktuellen Version entscheidet
                ResultDOSBoxCAE2000DontDeinstall := true;             
          end;
        
        if ResultAktuelleAppFound = true then
          begin
            //Messagebox das die aktuellere Version gleich der zu installierenden ist
            MsgBox(Format(ExpandConstant('{cm:AktuelleAppFound}'),[DecodeVersion(SavedMajor,SavedMinor)]),mbConfirmation,MB_OK);                                  
          end;
         
      end;


    function SaveVersionInfo: boolean;                                              //Aktuelle Versionsnummer in die Registry schreiben
      
      begin
        Result := (RegWriteDWordValue(HKLM,'{#UNINSTKEY}','MajorVer',
                  CreateDWord({#MAJOR},{#MINOR}))) and
                  (RegWriteDWordValue(HKLM,'{#UNINSTKEY}','MinorVer',
                  CreateDWord({#RELEASE},{#BUILD})));
      end;


    function InitializeSetup: boolean;

      begin
        UserFolderCAE := '';
        Result := true;
        Result := IsSetupNewer; 
        
        if ResultDOSBoxCAE2000PathCheck = true then                                 //Abbruchfunktion wenn der aktuelle Installationspfad nicht \
                                                                                      ermittelt werden konnte
          begin                                                                     
            result := false;
          end;

        if ResultDOSBoxCAE2000DontDeinstall = true then                             //Abbruchfunktion wenn der Benutzer sich gegen eine \
                                                                                      Deinstallation entschieden hat
          begin                                                                     
            result := false;
          end;

        if ResultAktuelleAppFound = true then                                       //Abbruchfunktion wenn bereits die aktuelle Version installiert ist
          
          begin
            result := false;
          end;

      end;
             
              
    function InstallCae(Param:String):String;                                       //Schreiben des, vom Benutzer ermittelten, CAE2000-Pfades

      begin
        Result := UserDirPageCAE.Values[0];
        CAE2000Path := UserDirPageCAE.Values[0];
        RegWriteStringValue(HKLM, '{#UNINSTKEY}','CAE2000Location', CAE2000Path);   //Erstellen eines Registry-Eintrages für den CAE2000-Pfad
      end;

   
    function GPSDir(Param:String):String;
     
      begin
        GPSPath := UserDirPageCAE.Values[1];
      end;

    procedure GPSDirCheckBoxOnClick(Sender: TObject);                               //Einblenden und Einstellen eines separaten GPS-Pfades            
 
      begin
        if GPSDirCheckBox.Checked then
          
          begin
            UserDirPageCAE.Add('Zielverzeichnis des GPS Ordner´s');                 //GPS-Verzeichnisauswahlfenster
         
            UserDirPageCAE.Values[1] := ExpandConstant('C:\CAE2000\');              //GPS-Standardpfad in der Verzeichnisauswahl
          end

      end;
   

    procedure InitializeWizard;

      begin
        SplashFileName := ExpandConstant('{tmp}\WISAG Logo weiß.bmp');              //Einblenden vom WISAG-Logo für Zeit x
        ExtractTemporaryFile(ExtractFileName(SplashFileName));                      //WISAG-Logo aus dem tmp-Verzeichnis aufrufen
 
        SplashForm := TForm.create(nil);
        with SplashForm do
          begin
            BorderStyle := bsNone;
            Position := poScreenCenter;                                             // Bildposition angeben
            ClientWidth := 980;                                                     // Bildgröße angeben (Breite in px)
            ClientHeight := 467;                                                    // Bildgröße angeben (Höhe in px)
          end;
 
        SplashImage := TBitmapImage.Create(SplashForm);
        with SplashImage do
          begin
            Bitmap.LoadFromFile(SplashFileName);
            Stretch := true;
            Align := alClient;
            Parent := SplashForm;
          end;
 
        with SplashForm do
          begin
            Show;
            for I := 1 to 2 do                                                      // Anzeigedauer in Sekunden; 1 to 2 = 2 Sekunden)
            begin
              Repaint;
              Sleep(1000);
            end;
          Close;
          Free;
          end;

      UserDirPageCAE := CreateInputDirPage(wpSelectDir,                             //Zusätzliche Verzeichnisauswahlseite einbinden
      ExpandConstant('{cm:CAEVerzeichnis}'),
      ExpandConstant('{cm:SpeicherortPRTDEF}'),
      ExpandConstant('{cm:SpeicherzielPRTDEF}'),
      false,
      'Neuer Ordner');

      UserDirPageCAE.Add('Zielverzeichnis der CAE2000.EXE');                        //CAE-Verzeichnisauswahlfenster

      UserDirPageCAE.Values[0] := ExpandConstant('C:\CAE2000\');                    //CAE-Standardpfad in der Verzeichnisauswahl

      GPSDirCheckBox := TRadioButton.Create(UserDirPageCAE);                        //Radio-Button zum Einblenden bei separaten GPS-Pfad
      
      with GPSDirCheckBox do                                                        //Parameter für die GPS-CheckBox
        
        begin       
          GPSDirCheckBox.Parent := UserDirPageCAE.Surface;
          Caption := ExpandConstant('{cm:GPSVerzeichnis}');
          Left := ScaleX(0);
          Top := ScaleY(120);
          Width := ScaleX(400);
          Height := ScaleY(40);
          Checked := GPSDirCheckBoxChecked;
          OnClick := @GPSDirCheckBoxOnClick;       
        end;      
      
      end;
  
    
    procedure CurStepChanged(CurStep: TSetupStep);                                  //Eintragen der benutzerspezifischen Pfadangaben \
                                                                                      in die DOSBox-Konfigurationsdatei
      begin
        if CurStep=ssPostInstall then
          begin;
            DOSBoxConf := TStringList.Create;
            DOSBoxConf.LoadFromFile((ExpandConstant('{localappdata}\DOSBox\dosbox-SVN_MB6.conf')));
            DOSBoxConf[298] := copy(DOSBoxConf[298],1,22)+(ExpandConstant('{app}\PrintOut\SELECT.PCL'))+copy(DOSBoxConf[298],30,length(DOSBoxConf[298]));
            DOSBoxConf[355] := copy(DOSBoxConf[355],1,9)+(ExpandConstant('"{code:InstallCae}\"'))+copy(DOSBoxConf[355],24,length(DOSBoxConf[355])); 
            DOSBoxConf[356] := copy(DOSBoxConf[356],1,9)+(ExpandConstant('"{app}\"'))+copy(DOSBoxConf[356],24,length(DOSBoxConf[356]));        
            if GPSDirCheckBox.Checked then
              begin
                DOSBoxConf[357] := copy(DOSBoxConf[357],1,1)+' mount D '+(ExpandConstant('"{code:GPSDir}\"'))+copy(DOSBoxConf[357],24,length(DOSBoxConf[357]));
              end;
            DOSBoxConf.SaveToFile((ExpandConstant('{localappdata}\DOSBox\dosbox-SVN_MB6.conf')));
            DOSBoxConf.Free;
            SaveVersionInfo;
          end;
      
      end;


    procedure CurUninstallStepChanged(CurUninstallStep: TUninstallStep);            //Code für deinstallation
      
      begin

        RegQueryStringValue(HKLM,'{#UNINSTKEY}','CAE2000Location',GetCAE2000Path);

        if CurUninstallStep = usAppMutexCheck then
          begin
            //Kopieren der Backupdatei ins Originalverzeichnis
            FileCopy(ExpandConstant(GetCAE2000Path) + '\Backup\PRTDEF.GER', ExpandConstant(GetCAE2000Path) + '\PRTDEF.GER', false);
          end;

        if CurUninstallStep=usDone then         
          begin
            RegDeleteKeyIncludingSubkeys(HKLM, '{#UNINSTKEY}');                     //Löschen des Rigistry-Eintrages nach der Deinstallation 
          end;
        
      end;

[Aphton]

Mal abgesehen davon, dass du dich nicht an den Delphi-Style-Guide hältst, sehe ich nur eine kleine Inkonsistenz:
Gehört nun eine Leerzeile vor begin oder nicht?

[Christian Seehase]

Moin,

Zitat:

Zitat von bennySB (Beitrag 1217216)

Ich bin offen für Zuckerbrot und Peitschen :-P

Ok, dann pack' ich mal die Peitsche aus :mrgreen:

Auf globale Variablen sollte man nach Möglichkeit verzichten, da man zu leicht Gefahr läuft die Übersicht über den jeweiligen Inhalt zu verlieren.
Das kann zu unschönem Fehlverhalten des Programmes führen.

Wenn Du Variablen funktionsübergreifend benötigst, dann kannst Du sie, z.B., als Felder des Formulares deklarieren.

Ansonsten:
Lesbar ist der Code durchaus.
Das wird auch von anderen gelesen werden können und von Dir, nach einiger Zeit auch noch ;-)

Sobald ich else verwende nehme ich grundsätzlich begin/end für beide Zweige, egal, ob ich eine oder mehrere Zeilen darin unterbringen muss.

[Der schöne Günther]

Wie sagt die alte (ist die überhaupt alt? :gruebel:) Weisheit so schön? Das Wichtige sind nicht die Formatierungsvorgaben selbst, sondern die Tatsache dass man überhaupt welche hat und sich alle dran halten. Man sollte also immer auf einen gemeinsamen Nenner kommen können sonst hat man in Wirklichkeit ganz andere Probleme...

Hier im Forum bin ich noch nie mit meiner rabiaten (wahren) Meinung aufgetreten, dass jede Methode, die nicht mehr auf einen Bildschirm passt, zu lang ist. Möglicherweise bin ich einfach nicht sonderlich schlau, aber ich wenn ich (so ganz heimlich) mir selbst eine Methode bombastisch anschwillt und ich bislang keine Zeit hatte, sie aufzusplitten, mache ich als erstes immer eine Art "Inhaltsverzeichnis" damit man auch weiter unten noch sieht, wo man eigentlich gerade steht. Etwas wie

Delphi-Quellcode:

begin
   
   // (1) Ofen vorheizen
   // (2) Zutaten zurechtlegen
   // (3) Anfangen zu backen
   // [4] Optional: Küche wieder sauber machen
   
   
   // (1) Ofen vorheizen
   ofenFabrik.getOfen().heize();
   [...]
      
end;

Möglicherweise gehen hier die Meinungen auch auseinander, aber ich bin ein Freund von ausführlich dokumentierten Funktionen. Warum schreibst du nicht in XML-Form zu jeder Methode hinzu, was sie macht, was reingeht, ...? Beispiel

Delphi-Quellcode:

///   <summary>Berechnet anhand eines hochkomplizierten und anerkannten
///      Verfahrens die aktuellen Lotozahlen
///   </summary>
///   <param name="sinnloserParameter">
///      Erhöhen Sie diese Zahl für gigantische Erfolgschancen. Kann auch
///      <c>NULL</c> sein.
///   </param>
///   <remarks>
///      Siehe auch: <see cref="Lotozahlenpackage|TLottozahlengenerator" />
///   </remarks>
function berechneLottoZahl(sinnloserParameter: Int=0): Int;
begin
   Result := Rnd();
end;

Wenn einen das XMLDoc stört, kann man es ja auch mit einem Klick ausblenden lassen...


Beim nächsten Thema kann ich nicht still sein. Ich bin sogar zwei Jahre bei einem indischen Guru gewesen, nur um mich in dieser Angelegenheit zu überwinden - Ich schaffe es nicht. Ich spreche es an. Hier und jetzt. Tabs und Leerzeichen.

Hat es einen bestimmten Glaubensgrund, dass du gerne mit Leerzeichen einrückst? Ich bin ein Tab-Verfechter. Soviel nur: Hier ein Klassiker, viel Spaß beim kurzen Lesen: Coding Horror: Death to the Space Infidels!.


Ansonsten: Speziell die Methode

Delphi-Quellcode:

IsSetupNewer

: Ich finde den "if not result ..."-Block relativ anstrengend zu lesen. Warum nicht ein paar Absätze? Mir fällt es nicht sonderlich leicht, direkt zu sehen, wo ein Block anfängt und aufhört.


Vielleicht klinge ich viel zu negativ. Deshalb zum Abschluss nochmal eine Verdeutlichung, wieviel Geschmack hier im Spiel ist: Ich sehe es in Sachen Blöcke ganz anders. Wenn ich irgendwo ein begin/end weglassen kann, tue ich es auch. :-D Ich bin ein Schlingel.

[nahpets]

Hallo Benny,

der Quelltext könnte bei mir in etwa so aussehen:
(Allerdings wird hier von der Forumssoftware ein nicht unerheblicher Teil von Leerzeichen unterschlagen. So stehen bei mir bei den Variablendeklarationen die : vor dem Variablentyp untereinander. Ebenso stehen bei mehreren Zuweisung hintereinander die := untereinander.)

Delphi-Quellcode:

// Die Zeilenbreite geht bei mit bis ca. 80 Zeichen.
// Ich hasse es, wenn ich zum Lesen von Quelltext zwei Monitoren nebeneinander
// benötige bzw. ständig hin- und herscrollen muss.
// Damit ich Quelltexte anstrengungsfrei lesen kann, muss die Schriftgröße von Courier-New
// so sein, dass etwa 90 Zeichen in eine Zeile passen. Bei kleineren Schriften bekomme ich
// (Seh-)Probleme oder muss mit der Nase (fast) in den Bildschirm. (Bildschirmauflösung: 1680 * 1050)

// Abgesehen davon ist "überbreiter" Quelltext schlecht zu drucken und sieht
// auch in Dokumentationen nicht wirklich toll aus, da er dort dann irgendwie
// umgebrochen wird und seine logische Struktur verloren geht.

// Für die Namensvergabe nutze ich (weitgehend) die ungarische Notation.

// Entgegen "Vorgaben" von Borland... gehört das "begin" bei mir hinter then...
// In einer eigenen Zeile macht es für mich den Quelltext ausschließlich länger,
// aber nicht lesbarer. Und wenn "begin" eingerückt wird und anschließend in der
// nächsten Zeile der weitere Quelltext auch noch, so macht es den Quelltext nur "breiter".
// Und wenn dann durch diese Einrückungen der Quelltext erst in der rechten Bildschirmhälfte 
// beginnt, geht die Lesbarkeit dann doch irgendwie flöten.

// Leerzeilen sind bei mir für die logische Trennung von Quelltextzeilen.
// Hinter function und procedure stören sie mich, weil ich hier immer davon
// ausgehe, dass nicht zusammengehörende Quelltexte folgen.

// Kommentare nur dann hinter eine Quelltextzeile, wenn sie vollständig im
// sichtbaren Bereich des Bildschirmes bleiben, bei einer "normalen" Auflösung,
// also auch für jemand, der keine Adleraugen hat, anstrengungsfrei lesbar bleiben.
// Ansonsten in die Zeile dadrüber unter Beachtung der Einrückung.

// Hinter kommentarbeginnende bzw. vor kommentarbeendende Zeichen gehört ein
// Leerzeichen.
// Kommentare, die aus vollständigen Sätze bestehen, sollte auch die entsprechenden
// Satzzeichen enthalten. Und wenn die Rechtschreibung auch noch (weitgehend)
// in Ordnung ist, bin ich zufrieden.
// Bei Meldungen, die an den Benutzer gehen, muss die Rechtschreibung stimmen.
// Aus der Qualität der Meldungen schließe ich immer auf die Qualität der Software.
// Fehlerhafte Meldung bedeuten bei mir, dass die Software vermutlich auch
// nicht besser ist.

// Bei Funktionsaufrufen... werden die Parameter untereinandergeschrieben,
// sofern sie hintereinandergeschrieben die normale Bildschirmbreite deutlich
// übersteigen.
// Hinter Satzzeichen (auch in Funktionsaufrufen...) gehört ein Leerzeichen.
// Dadurch sind die einzelnen Parameter besser unterscheidbar.

// Wenn hinter einem else mehr als eine Zeile folgt (incl. Kommentarzeilen)
// wird mit begin und end "geklammert". Dadurch ist (für mich) die Logik
// besser optisch darstellbar.

// Vor und hinter + - := ... gehört ein Leerzeichen, damit man die Trennung
// innerhalb der Quelltextzeilen besser erkennen kann. "eingebettete" + und - ...
// kann man schonmal schnell übersehen.

// Bitte auf Apostroph verzichten, wenn nicht erforderlich.
// 'Zielverzeichnis des GPS Ordner´s' <- seit wann mit? Und dann nichtmal '?
// 'Zielverzeichnis des GPS-Ordners'  <- finde ich irgendwie "schöner" ;-)

var
  UserDirPageCAE                            : TInputDirWizardPage;
  DOSBoxConf                                : TStringList;
  GPSDirCheckBox                            : TRadioButton;
  SplashImage                               : TBitmapImage;
  SplashForm                                : TForm;
  SetupMajor                                : dword;
  SetupMinor                                : dword;
  SavedMajor                                : dword;
  SavedMinor                                : dword;

  CAE2000Path                               : string;
  DOSBoxCAE2000Path                         : string;
  DOSBoxCAE2000UninstallName                : string;
  DOSBoxCAE2000UninstallNameRemovedQuotes   : string;
  DOSBoxCAE2000UninstallPath                : string;
  DOSBoxCAE2000UninstallPathRemovedQuotes   : string;
  DOSBoxCAE2000UninstallString              : string;
  DOSBoxCAE2000UninstallStringRemovedQuotes : string;
  GPSPath                                   : string;
  GetCAE2000Path                            : string;
  SplashFileName                            : string;
  UserFolderCAE                             : string;
  tmp                                       : string;

  GPSDirCheckBoxChecked                     : boolean;
  ResultDOSBoxCAE2000PathCheck              : boolean;
  ResultAktuelleAppFound                    : boolean;
  ResultDOSBoxCAE2000DontDeinstall          : boolean;

  // Das ist nicht zwingend aussagefähig!
  a, b, c, d                                : word;
  // abcd scheinen mir nichtmal erforderlich zu sein.

  // I kann in der Prozedur deklariert werden.
  I                                         : Integer;
  // Variablen möglichst dort deklarieren, wo sie benötigt werdenund nicht pauschal
  // als globale Variabeln.
  // Auch dann nicht, wenn man zufällig in zwei Funktionen eine Stringvariabel tmp
  // benötigt. Dann bekommt jede Funktion ihre eigene Variabel.

// Registry-Schlüssel für die Versionsnummer
function CreateDWord(const Hi, Lo : word) : dword;
begin
  Result := (Hi shl 16) or Lo;
end;

// Entschlüsselung der Versionsnummer
function DecodeVersion(const dwMajor, dwMinor : dword) : string;
begin
  a      := word(dwMajor shr 16);
  b      := word(dwMajor and not $ffff0000);
  c      := word(dwMinor shr 16);
  d      := word(dwMinor and not $ffff0000);
  Result := Format('%d.%d.%d.%d',[a,b,c,d]);
   // Geht das nicht?
  Result := Format('%d.%d.%d.%d',[word(dwMajor shr 16),
                                  word(dwMajor and not $ffff0000),
                                  word(dwMinor shr 16),
                                  word(dwMinor and not $ffff0000)]);
end;

// Versionsabgleich der vorhandenen Installation zur neuen Installation
function IsSetupNewer : boolean;
var
  ResultCode : integer;
begin
  if (not RegQueryDWordValue(HKLM,'{#UNINSTKEY}', 'MajorVer', SavedMajor))
  or (not RegQueryDWordValue(HKLM,'{#UNINSTKEY}', 'MinorVer', SavedMinor)) then begin
    if (RegQueryStringValue(HKLM,'{#UNINSTKEY}', 'UninstallString', tmp))
    and (tmp <> '')
    and (fileexists(tmp)) then begin
      Result := (MsgBox(ExpandConstant('{cm:NotVerifiedVersionFound}'),
                        mbConfirmation,
                        MB_YESNO or MB_DEFBUTTON2) = IDYES);
      exit;
    end;
  end;

  SetupMajor := CreateDWord({#MAJOR},{#MINOR});
  SetupMinor := CreateDWord({#RELEASE},{#BUILD});

  // Neuere Version ist installiert
  Result := (SetupMajor > SavedMajor)
         or ((SetupMajor = SavedMajor) and (SetupMinor >= SavedMinor));
  // Gleiche Version ist bereits installiert
  ResultAktuelleAppFound := (SetupMajor = SavedMajor)
                         or ((SetupMajor = SavedMajor) and (SetupMinor = SavedMinor));

  // Ermitteln und Umformen des Deinstallationspfades
  RegQueryStringValue(HKLM,'
                      {#UNINSTKEY}',
                      'UninstallString',
                      DOSBoxCAE2000UninstallString);
  DOSBoxCAE2000UninstallPath := (ExtractFileDir(DOSBoxCAE2000UninstallString) + '\' + '"');
  DOSBoxCAE2000UninstallName := ('"' + ExtractFileName(DOSBoxCAE2000UninstallString));

  DOSBoxCAE2000UninstallStringRemovedQuotes := RemoveQuotes(DOSBoxCAE2000UninstallString);
  DOSBoxCAE2000UninstallPathRemovedQuotes   := RemoveQuotes(DOSBoxCAE2000UninstallPath);
  DOSBoxCAE2000UninstallNameRemovedQuotes   := RemoveQuotes(DOSBoxCAE2000UninstallName);

  if not Result then begin
    if RegQueryStringValue(HKLM,
                           '{#UNINSTKEY}',
                           'InstallLocation',
                           DOSBoxCAE2000Path) then begin
      result := DirExists(DOSBoxCAE2000Path);
    end;
    if not Result then begin
      // Installationspfad der vorhandenen Installation kann nicht ermittelt werden.
      MsgBox(ExpandConstant('{cm:DOSBoxPathNotFound}'), mbError, MB_OK);
      // Abbruch der Installation, wenn der Pfad aktuelle Installationspfad nicht
      // ermittelt werden konnte
      ResultDOSBoxCAE2000PathCheck := false;
    end;
    // Meldung, dass eine aktuellere Version installiert ist.
    if MsgBox(ExpandConstant('{cm:NewerAppFound}'),
              mbError,
              MB_YESNO or MB_DEFBUTTON2) = IDYES then begin
      // Silent-Ausführung der Deinstallation, wenn aktuellere Version installiert
      // ist und der Benutzer diesem zustimmte.
      Exec(ExpandConstant(DOSBoxCAE2000UninstallStringRemovedQuotes),
           '/SILENT',
           '',
           SW_SHOW,
           ewWaitUntilTerminated,
           ResultCode);
    end else begin
      // Abbruch des Setups, sobald der Benutzer sich gegen eine Deinstallation
      // der aktuellen Version entscheidet.
      ResultDOSBoxCAE2000DontDeinstall := true;
    end;
  end;

  if ResultAktuelleAppFound = true then begin
    // Meldung, dass die aktuellere Version gleich der zu installierenden ist.
    MsgBox(Format(ExpandConstant('{cm:AktuelleAppFound}'),
                  [DecodeVersion(SavedMajor, SavedMinor)]),
           mbConfirmation,
           MB_OK);
  end;
end;

// Aktuelle Versionsnummer in die Registry schreiben.
function SaveVersionInfo: boolean;
begin
  Result := (RegWriteDWordValue(HKLM,'{#UNINSTKEY}','MajorVer',
                                CreateDWord({#MAJOR},{#MINOR})))
        and (RegWriteDWordValue(HKLM,'{#UNINSTKEY}','MinorVer',
                                CreateDWord({#RELEASE},{#BUILD})));
end;

function InitializeSetup: boolean;
begin
  UserFolderCAE := '';
  // Die folgende Zeile dürfte wohl überflüssig sein.
  Result        := true;
  Result        := IsSetupNewer;

  // Abbruchfunktion, wenn der aktuelle Installationspfad nicht ermittelt werden
  // konnte.
  // Ist hier gemeint: Abbruch der Funktion.
  // Für mich ist eine Abbruchfunktion eine Funktion die ein Programm abbricht.
  // (Ich weiß, Haarspalterei ;-))
  if ResultDOSBoxCAE2000PathCheck = true then begin
    result := false;
  end;

  // Abbruchfunktion, wenn der Benutzer sich gegen eine Deinstallation entschieden hat.
  if ResultDOSBoxCAE2000DontDeinstall = true then begin
    Result := false;
  end;

  // Abbruchfunktion, wenn bereits die aktuelle Version installiert ist.
  if ResultAktuelleAppFound = true then begin
    result := false;
  end;
end;

// Schreiben des, vom Benutzer ermittelten, CAE2000-Pfades.
function InstallCae(Param : String) : String;
begin
  Result      := UserDirPageCAE.Values[0];
  CAE2000Path := UserDirPageCAE.Values[0];
  // Erstellen eines Registry-Eintrages für den CAE2000-Pfad
  RegWriteStringValue(HKLM, '{#UNINSTKEY}', 'CAE2000Location', CAE2000Path);
end;

function GPSDir(Param:String):String;
begin
  GPSPath := UserDirPageCAE.Values[1];
end;

// Einblenden und Einstellen eines separaten GPS-Pfades.
procedure GPSDirCheckBoxOnClick(Sender : TObject);
begin
  if GPSDirCheckBox.Checked then begin
    // GPS-Verzeichnisauswahlfenster
    UserDirPageCAE.Add('Zielverzeichnis des GPS Ordners');
    // GPS-Standardpfad in der Verzeichnisauswahl
    UserDirPageCAE.Values[1] := ExpandConstant('C:\CAE2000\');
  end
end;

procedure InitializeWizard;
begin
  // Einblenden vom WISAG-Logo für Zeit x.
  SplashFileName := ExpandConstant('{tmp}\WISAG Logo weiß.bmp');
  // WISAG-Logo aus dem tmp-Verzeichnis aufrufen.
  ExtractTemporaryFile(ExtractFileName(SplashFileName));

  SplashForm := TForm.create(nil);
  with SplashForm do begin
    BorderStyle  := bsNone;
    Position     := poScreenCenter; // Bildposition angeben
    ClientWidth  := 980;            // Bildgröße angeben (Breite in px)
    ClientHeight := 467;            // Bildgröße angeben (Höhe in px)
  end;

  SplashImage := TBitmapImage.Create(SplashForm);
  with SplashImage do begin
    Bitmap.LoadFromFile(SplashFileName);
    Stretch := true;
    Align   := alClient;
    Parent  := SplashForm;
  end;

  with SplashForm do begin
    Show;
    // So schnell kann ich nicht gucken, da kann man auf den Splashscreen
    // auch verzichten.
    // Anzeigedauer in Sekunden, 1 to 2 = 2 Sekunden.
    for I := 1 to 2 do begin
      Repaint;
      Sleep(1000);
    end;
    Close;
    Free;
  end;

  // Zusätzliche Verzeichnisauswahlseite einbinden.
  UserDirPageCAE := CreateInputDirPage(wpSelectDir,
                    ExpandConstant('{cm:CAEVerzeichnis}'),
                    ExpandConstant('{cm:SpeicherortPRTDEF}'),
                    ExpandConstant('{cm:SpeicherzielPRTDEF}'),
                    false,
                    'Neuer Ordner');

  // CAE-Verzeichnisauswahlfenster
  UserDirPageCAE.Add('Zielverzeichnis der CAE2000.EXE');

  // CAE-Standardpfad in der Verzeichnisauswahl.
  UserDirPageCAE.Values[0] := ExpandConstant('C:\CAE2000\');

  // Radio-Button zum Einblenden bei separatem GPS-Pfad.
  GPSDirCheckBox := TRadioButton.Create(UserDirPageCAE);

  // Parameter für die GPS-CheckBox.
  with GPSDirCheckBox do begin
    GPSDirCheckBox.Parent := UserDirPageCAE.Surface;
    Caption               := ExpandConstant('{cm:GPSVerzeichnis}');
    Left                  := ScaleX(0);
    Top                   := ScaleY(120);
    Width                 := ScaleX(400);
    Height                := ScaleY(40);
    Checked               := GPSDirCheckBoxChecked;
    OnClick               := @GPSDirCheckBoxOnClick;
  end;
end;

// Eintragen der benutzerspezifischen Pfadangaben in die DOSBox-Konfigurationsdatei.
procedure CurStepChanged(CurStep: TSetupStep);
begin
  if CurStep = ssPostInstall then begin;
    DOSBoxConf := TStringList.Create;
    DOSBoxConf.LoadFromFile((ExpandConstant('{localappdata}\DOSBox\dosbox-SVN_MB6.conf')));
    DOSBoxConf[298] := copy(DOSBoxConf[298],1,22)
                     + (ExpandConstant('{app}\PrintOut\SELECT.PCL'))
                     + copy(DOSBoxConf[298],30,length(DOSBoxConf[298]));
    DOSBoxConf[355] := copy(DOSBoxConf[355],1,9)
                     + (ExpandConstant('"{code:InstallCae}\"'))
                     + copy(DOSBoxConf[355],24,length(DOSBoxConf[355]));
    DOSBoxConf[356] := copy(DOSBoxConf[356],1,9)
                     + (ExpandConstant('"{app}\"'))
                     + copy(DOSBoxConf[356],24,length(DOSBoxConf[356]));
    if GPSDirCheckBox.Checked then begin
      DOSBoxConf[357] := copy(DOSBoxConf[357],1,1)
                       + ' mount D '
                       + (ExpandConstant('"{code:GPSDir}\"'))
                       + copy(DOSBoxConf[357],24,length(DOSBoxConf[357]));
    end;
    DOSBoxConf.SaveToFile((ExpandConstant('{localappdata}\DOSBox\dosbox-SVN_MB6.conf')));
    DOSBoxConf.Free;
    SaveVersionInfo;
  end;
end;

// Code für die Deinstallation.
procedure CurUninstallStepChanged(CurUninstallStep: TUninstallStep);
begin
  RegQueryStringValue(HKLM,'{#UNINSTKEY}','CAE2000Location',GetCAE2000Path);
  if CurUninstallStep = usAppMutexCheck then begin
    // Kopieren der Backupdatei ins Originalverzeichnis.
    FileCopy(ExpandConstant(GetCAE2000Path) + '\Backup\PRTDEF.GER',
             ExpandConstant(GetCAE2000Path) + '\PRTDEF.GER',
             false);
  end;

  if CurUninstallStep = usDone then begin
    // Löschen des Rigistryeintrages nach der Deinstallation.
    RegDeleteKeyIncludingSubkeys(HKLM, '{#UNINSTKEY}');
  end;
end;

Allerdings gibt es keine Verpflichtung meinen Ansichten auch nur ansatzweise zuzustimmen. Es ist immer auch etwas Geschmackssache. Man sollte sich in Firmen und/oder Teams aber auf einheitliche optische Gestaltung von Quelltexten einigen. Ebenso sollte die Vergabe von Varibalnnamen, Funktion- und Prozedurenamen... einheitlich sein.

In anbetracht dessen, was ich im Laufe meines Berufslebens schon so an Quelltexten zu sehen und zu pflegen bekommen habe, ist Dein "Erstlingswerk" doch durchaus akzeptabel. Spätestens nach dem dritten Lesen sollte man damit (trotz vollkommen fremder Materie) klarkommen können.
Oder anders formuliert: Auch wer kein Pascalscript kann, sollte Deinen Quelltext lesen und (weitgehend) verstehen können.

PS.:

Zitat:

Zitat von Christian Seehase

Wenn Du Variablen funktionsübergreifend benötigst, dann kannst Du sie, z.B., als Felder des Formulares deklarieren.

Das dürfte hier nicht gehen, da es sich nicht um Delphi sondern um Pascalscript innerhalb von INNO-Setup handelt.

[Dalai]

Auch wenn ich weder Profi noch Semiprofi bin, gebe ich meinen Senf dazu. Ich kann nahpets in allen Punkten zustimmen :thumb: (bis auf das "begin" hinter "then", aber das ist Geschmackssache). Ich möchte noch einen Punkt ergänzen: Einrückung/Ausrichtung von Variablentypdefinitionen. Damit lässt sich IMO schneller eine optische Erfassung von Variablenname und dessen Typ erreichen. Konkret würde ich also die Definitionen so formatieren:

Code:

var
  UserDirPageCAE        : TInputDirWizardPage;
  DOSBoxConf            : TStringList;
  GPSDirCheckBox        : TRadioButton;
  SplashImage           : TBitmapImage;
  SplashForm            : TForm;
  SetupMajor            : dword;
  SetupMinor            : dword;
  SavedMajor            : dword;
  SavedMinor            : dword;

  CAE2000Path                              : string;
  DOSBoxCAE2000Path                        : string;
  DOSBoxCAE2000UninstallName               : string;
  DOSBoxCAE2000UninstallNameRemovedQuotes  : string;
  DOSBoxCAE2000UninstallPath               : string;
  DOSBoxCAE2000UninstallPathRemovedQuotes  : string;
  DOSBoxCAE2000UninstallString             : string;
  DOSBoxCAE2000UninstallStringRemovedQuotes: string;
  GPSPath                                  : string;
  GetCAE2000Path                           : string;
  SplashFileName                           : string;
  UserFolderCAE                            : string;
  tmp                                      : string;

OK, das sieht jetzt bei den langen Variablennamen ein bisschen Scheiße aus, aber bei deutlich kürzeren Namen erhöht es (für mich) die Lesbarkeit und fördert die schnelle Erfassung sowohl des Namens als auch dessen Typ.

Komischerweise entfernt das Forum aus obigem Code die wiederholten Leerzeichen, wenn er in DELPHI-Tags steht :gruebel:, daher in CODE-Tags.

MfG Dalai

[nahpets]

Genau so mache ich das bei den Variabeldeklarationen auch.
So kann man auch mal schnell nach Typ sortieren, halt Sotierung ab der Position vom :,
wobei ich den Typ (meist) auch durch die ungarische Notation bereits am Beginn des Variablennamens erkennen kann. Dadurch brauche ich beim Lesen von Quelltext nicht mehr bei der Deklaration nachschauen, von welchem Typ eine Variabel ist. Das sähe dann in etwa so aus:

Code:

var
  UserDirPageCAE                              : TInputDirWizardPage;
  slDOSBoxConf                                : TStringList;
  rbGPSDirCheckBox                            : TRadioButton;
  bmpSplashImage                              : TBitmapImage;
  fmSplashForm                                : TForm;
  dwSetupMajor                                : dword;
  dwSetupMinor                                : dword;
  dwSavedMajor                                : dword;
  dwSavedMinor                                : dword; 
  sCAE2000Path                                : string;
  sDOSBoxCAE2000Path                          : string;
  sDOSBoxCAE2000UninstallName                 : string;
  sDOSBoxCAE2000UninstallNameRemovedQuotes    : string;
  sDOSBoxCAE2000UninstallPath                 : string;
  sDOSBoxCAE2000UninstallPathRemovedQuotes    : string;
  sDOSBoxCAE2000UninstallString               : string;
  sDOSBoxCAE2000UninstallStringRemovedQuotes  : string;
  sGPSPath                                    : string;
  sGetCAE2000Path                             : string;
  sSplashFileName                             : string;
  sUserFolderCAE                              : string;
  stmp                                        : string;

[bennySB]

Wow hätte nicht gedacht das dich welche so auf meinen Code stürzen xD
Freut mich ein bißchen was an Anregung bekommen zu haben und besonders das Thema Einrückungen werde ich mal angreifen und noch so das ein oder andere.
Auf jedenfall schonmal danke an euch.

[DeddyH]

Da es noch niemand erwähnt hat: man tut nicht mit true oder false vergleichen tun :mrgreen:

[bennySB]

Oh Ok warum denn das nicht?
Denn binär-vergleiche sind doch eigentlich das sicherste was es gibt. Bei 0 oder 1 kann nichts vertauscht werden.

[DeddyH]

Typische Anfängerfehler
Vergleichen Sie niemals mit Boolean-Konstanten

[dataspider]

Hi,

es ist wirklich lesbar und damit schon recht ordentlich.
Worauf ich noch achten würde:

Delphi-Quellcode:

function IsSetupNewer: boolean;

Vom Name her würde ich nur erwarten, dass geprüft und ein Boolean zurückgeliefert wird.
Du ermittelst in dieser Funktion aber Werte für übergreifende Variablen,
wertest Result aus und reagierst mit Meldungen bzw. startest UnInstall...
Das Alles ist beim Lesen des Codes nicht zu vermuten, man muss erst in die Funktion, um zu erkennen,
das z.B. ResultAktuelleAppFound ermittelt wird etc.
Eine Funktion sollte in aller Regel nur EINE Aufgabe erfüllen, und das muss am Name erkennbar sein.

Diese riesigen Header mit viel Kommentaren halte ich für überflüssig. Wenn du diese nicht mehr brauchst, in dein Code lesbar.
Das ist IMHO eher anzustreben als sich in ewigen Kommentaren zu verlieren, die man bei Änderung des Codes nicht mit korrigiert und uns irgendwann in die Irre leiten.

Allerdings musste ich auch erst "Clean Code" lesen, um umzudenken.

Frank


Hier würde ich mehrere kleinere Routinen draus machen.

[Furtbichler]

10 Programmierer, 20 Meinungen. Alle gleichwertig. Hier ist meine:

Wenn Du wirklich blutiger Anfänger bist, ziehe ich meinen Hut. Das ist -für dein Level- richtig gut lesbarer Code.

Aaaber ;-)

Ich würde aus DOSBOXCAE2000Uninstall erst einmal ein Record machen. Dann ist der Name zwar schön lang, aber mir sagt er nix. Wenn er Dir bzw. demjenigen, der das Programm kennt, etwas sagt: Gut. Wenn nicht: Umbenennen.

Zitat:

Einrückung/Ausrichtung von Variablentypdefinitionen...

Eigentlich ist es zweitrangig, von welchem Typ die Variablen sind. Wenn ich es wissen möchte, gehe ich mit dem Cursor rauf.

Zitat:

...und fördert die schnelle Erfassung sowohl des Namens als auch dessen Typ.

Wenn der Variablenname sehr weit von der Deklaration entfernt ist? Das Auge muss ja mühsam versuchen, beim nach-rechts-blicken in der gleichen Zeile zu bleiben.

Variablen- und Typbezeichnungen tabellarisch anzuordnen sieht vielleicht 'ordentlich' aus, aber gut bzw. lesbar bzw. 'schnell erfassbar' ist das nicht.

Zitat:

wobei ich den Typ (meist) auch durch die ungarische Notation bereits am Beginn des Variablennamens erkennen kann. Dadurch brauche ich beim Lesen von Quelltext nicht mehr bei der Deklaration nachschauen, von welchem Typ eine Variabel ist.

Siehe oben: Die ungarische Notation war bei C sinnvoll, da implizite Typkonvertierungen schnell zu Programmfehlern führte und als die IDE in etwa den Funktionsumfang von Notepad hatte, aber heutzutage braucht man das nicht mehr. Und da Code imho lesbar wie ein (englisches) Buch sein sollte, ist Code wie

Delphi-Quellcode:

if (hmHuman.dwMode=mdHungry) then

nicht wirklich besser lesbar, als z.B.

Delphi-Quellcode:

if (Human.Mode=Hungry)

, wobei ich hier eher schreiben würde

Delphi-Quellcode:

if Human.IsHungry

. Zum Verständnis der Funktionsweise einer Methode ist es unerheblich, von welchem Typ eine Variable ist. Nebenbei: Was passiert, wenn sich der Typ doch mal ändert? Dann muss ich über Refactoring den Namen überall ändern. Was für eine überflüssige Arbeit.

Jede zweite Zeile würde ich auch nicht kommentieren. Wenn man Programmzeilen kommentieren muss, dann packt man sie in eine eigene Prozedur, der man einen aussagekräftigen Namen gibt.

Delphi-Quellcode:

 // Neuere Version ist installiert
   Result := (SetupMajor > SavedMajor)
          or ((SetupMajor = SavedMajor) and (SetupMinor >= SavedMinor));
   // Gleiche Version ist bereits installiert
   ResultAktuelleAppFound := (SetupMajor = SavedMajor)
                          or ((SetupMajor = SavedMajor) and (SetupMinor = SavedMinor));

   // Ermitteln und Umformen des Deinstallationspfades
   RegQueryStringValue(HKLM,'
 {#UNINSTKEY}',

Ich würde das so schreiben

Delphi-Quellcode:

Result := NewerVersionIsInstalled();
ResultAktuelleAppFound = SameVersionIsInstalled();
DOSBoxCAE2000Uninstall.ExtractPath();

Das ist für mich lesbarer und kommt ganz ohne Kommentare aus. Wenn ich wissen will, was z.B. 'ExtractPath' macht, navigiere ich eben rein. Aber um die eigentliche Funktion 'IsSetupNew' zu verstehen, ist es unerheblich, ob der Pfad in der Registry steht oder nicht. Nebenbei: Heißt es nun 'Deinstallation' oder 'Uninstall'?

Dann (eigentlich als Erstes) würde ich -wie Frank schon gesagt hat- die Routinen in kleinere Routinen aufteilen. Jede Einzelroutine macht genau ein Ding und sie heißt dann auch so. Und -schwupps- sind Kommentare im Code überflüssig. Delphi unterstützt dich hier durch die 'Refactoring-Extract Method' Funktion.

Prozeduren/Funktionen würde ich auch nicht kommentieren, denn so ein Kommentar wird selten mitgepflegt. Und überflüssig ist er auch:

Delphi-Quellcode:

// Entschlüsselung der Versionsnummer
function DecodeVersion(const dwMajor, dwMinor : dword) : string;
...
// Versionsabgleich der vorhandenen Installation zur neuen Installation
function IsSetupNewer : boolean;

Wozu dient dieser Kommentar? Wo ist der Mehrwert ggü dem Funktionsnamen? Wieso kommentiere ich in deutsch, verwende jedoch englische Bezeichner? Dann kommentiere ich nicht, sondern übersetze für deutsche Leser. Und wenn ich eh nur deutsche Leser/Programmierer im Fokus habe, wieso verwende ich dann englische Bezeichner? Entscheide dich für eine Sprache. Und zwar für eine, die Du gut beherrscht. Wenn Du meinst, kommentieren zu müssen, formatiere den Code um, bis er lesbar ist.

Ein generelles Wort zu Kommentaren: Kommentare sind fast immer böse, sinnlos, müllen den Code zu und sind zudem auch noch falsch.

Böse sind sie, weil sie einem etwas erklären wollen, was i.a. sowieso nicht (mehr) stimmt.
Sinnlos sind sie, weil sie Dinge erklären, die eh da stehen (Code). Wenn der Code so kompliziert ist, das man ihn kommentieren muss, dann schreibt man ihn eben leserlich, sodaß man ihn nicht kommentieren muss.
Falsch sind Kommentare deshalb, weil sie spätestens bei der 3.Änderung nicht mehr nachgepflegt werden.

Man benötigt keine Kommentare, um Code zu erklären. Ja gut. Fast keine. Also: Man benötigt fast nie Kommentare.
Ausnahmen: Quellenangaben, Gesetzesbestimmungen (Copyright, aber auch Implementierungsdetails, MWST-Rechnung etc.), Anmerkungen zum verwendeten Verfahren u.ä.

Delphi-Quellcode:

// Sorts the list using Quicksort
Procedure TMyThing.Sort();

Ist also doch besser als (es gilt ja auch: Implementierungsdetails verbergen, d.h. nicht in den Bezeichner packen)

Delphi-Quellcode:

//
Procedure TMyThing.QuickSort();

Abschließend noch ein Wort zu 'Tabs', 'Leerzeichen', 'Einrückung' generell: Da jeder einen Code-Beautifier in seiner IDE oder seinem Portfolio (=externes Tool) haben sollte, ist jegliche Diskussion darüber imho überflüssig. Wenn mir der Code nicht gefällt, drücke ich auf Ctrl+D (bei mir formatiert das den Code) und schon bekomme ich keinen Augenkrebs mehr. Ich muss mich mit dem Autor nicht streiten, ob nun Tabs oder Leerzeichen besser sind, oder das 'begin' eingerückt wird oder nicht oder auf einer eigenen Zeile steht oder nicht und ob Typdeklarationen untereinanderstehen. Nein. CTRL+D löst alle dies Probleme.

Nur eines ist wichtig (zig mal gesagt): Wenn Du im Team arbeitest, sollte alle die gleichen Einstellungen im Beautifier vornehmen. Code wird aber nicht ständig formatiert, sondern nur in 'Beautifier-Sessions'. Wieso? Wenn ich Codeänderungen in meiner Versionsverwaltung prüfe (Welche Codezeilen wurden denn in der Klasse XY verändert), geht der Bugfix unter, wenn gleichzeitig der Code mal wieder aufgehübscht wurde. Denn dann zeigt mir mein CVS alle Zeilen als 'verändert' an.

Also: Code wird hübsch, indem er lesbar, d.h. verständlich wird. Lesbar und Verständlich wird er, wenn man die für das Verständnis unerheblichen Teile versteckt, z.B. durch Verlagerung in kleine private Methoden mit einem verständlichen Namen.
Kommentare sollten überflüssig sein, bzw. knete deinen Code so lange, bis sie überflüssig werden, denn die Dinger altern schneller als eine Banane.

Einrückung etc. ist wie Make-Up. Kann man mal eben raufklatschen, aber 'hübscher' wird der Code dadurch auch nicht.

[p80286]

Zitat:

Zitat von Furtbichler (Beitrag 1217246)

Zitat:

Einrückung/Ausrichtung von Variablentypdefinitionen...

Eigentlich ist es zweitrangig, von welchem Typ die Variablen sind. Wenn ich es wissen möchte, gehe ich mit dem Cursor rauf.

Wenn man Sourcen liest (Papier) ist das schon ein Unterscheid;

Zitat:

Zitat von Furtbichler (Beitrag 1217246)

Zitat:

...und fördert die schnelle Erfassung sowohl des Namens als auch dessen Typ.

Wenn der Variablenname sehr weit von der Deklaration entfernt ist? Das Auge muss ja mühsam versuchen, beim nach-rechts-blicken in der gleichen Zeile zu bleiben.

Wie üblich gibt es auch hier ein sowohl als auch. Wenn der Typ nicht direkt hinter dem Variablennamen steht, ist der Variablenname durchaus einfacher zu erfassen. wenn der Typ dann allerdings erst auf stelle 78 anfängt.....

Zitat:

Zitat von Furtbichler (Beitrag 1217246)

Zitat:

wobei ich den Typ (meist) auch durch die ungarische Notation bereits am Beginn des Variablennamens erkennen kann. Dadurch brauche ich beim Lesen von Quelltext nicht mehr bei der Deklaration nachschauen, von welchem Typ eine Variabel ist.

Siehe oben: Die ungarische Notation war bei C sinnvoll, da implizite Typkonvertierungen schnell zu Programmfehlern führte und als die IDE in etwa den Funktionsumfang von Notepad hatte, aber heutzutage braucht man das nicht mehr.

Insbesonders wenn ich mich beim Debugging nicht wild durch die Sourcen wühlen will ist zumindestens ein Typhinweis nicht kontraproduktiv.

Achja Kommentare, Darin sollte stehen warum man etwas mach, das wie sollte eigentlich aus dem Quelltext hervor gehen.

Gruß
K-H

[Furtbichler]

Zitat:

Zitat von p80286 (Beitrag 1217254)

Wenn man Sourcen liest (Papier) ist das schon ein Unterschied

Man schreibt Code nicht für diesen sehr unwahrscheinlichen Fall, sondern für den Normalfall: Die Programmierer sitzen vor dem Bildschirm um müssen fremden Code verändern. Es gilt aber auch: Ich muss mich der Situation anpassen: Entwickle ich in PHP und weiß, das alle Kollegen nur Notepad benutzen, sieht mein Code so aus, das man ihn auch ohne IDE schnell versteht.

Zitat:

Zitat von Furtbichler (Beitrag 1217246)

Wie üblich gibt es auch hier ein sowohl als auch. Wenn der Typ nicht direkt hinter dem Variablennamen steht, ist der Variablenname durchaus einfacher zu erfassen.

Zum Verständnis ist der Datentyp unerheblich. Ich lese Code und keine Deklarationen, um zu verstehen, was passiert. Aber wenn ich den Typen denn mal wissen will, scrolle ich ganz bestimmt nicht zur Deklaration (ob sie nun hübsch tabellarisch ist oder nicht): Ich halte den Cursor drauf, das ist irgendwie einfacher. Das geht zwar im Debugmodus nicht, aber i.A. debugge ich eh erst, wenn ich den Code verstanden habe. Und wenn, dann stringe ich eben per Strg+Click zur Deklaration und wieder zurück.

Nicht umsonst bietet C# hier ein 'var' anstatt der expliziten Typbezeichnug an: Weil es egal ist. Der Typ der Variablen ist zu 99% eh aus dem Code ersichtlich. Diese wird fast immer initialisiert, also ist klar, was das für ein Typ ist.

Delphi-Quellcode:

a := 1.0;  // Ganz klar
foo := TFoobar.Create; // Auch

Zitat:

Insbesonders wenn ich mich beim Debugging nicht wild durch die Sourcen wühlen will ist zumindestens ein Typhinweis nicht kontraproduktiv.

Nun ja. Schau einfach, was die Variable enthält, das ist in 99,9% der Fälle eh das, was dich interessiert. Die ungarische Notation greift ja nicht bei Klassen, sondern nur bei einfachen Typen. Und 'Strg-Click' ist eigentlich kein 'wild durch die Sourcen wühlen'.

Und wann ist es wichtig, ob das nun ein Byte, Word, DWord, Unsigned schießmichtot oder wasauchimmer ist? Eigentlich nie, außer z.B. bei hardwarenaher Programmierung. Und dann soll man das auch so machen. Die Regel lautet: Verwende klare Bezeichner. Und wenn es wichtig ist, das der Status ein Byte ist (und genau ein Byte!), dann schreibt man das eben: StatusByte. Aber als Dogma bzw. allgemeine Regel? 'ubStatus'... Nee.

Zitat:

Achja Kommentare, Darin sollte stehen warum man etwas mach, das wie sollte eigentlich aus dem Quelltext hervor gehen.

Bei mir würde dann stehen: "To make money" (Das ist der Grund, warum ich den Code schreibe).

Hast Du ein paar Beispiele parat, wann beim Debuggen der Typ einer Variablen wichtig ist? Wie würde ein Kommentar bei dir aussehen, bei dem Du beschreibst, warum Du die Lösung so und nicht anders implementiert hast? Enthält der Kommentar einen Mehrwert oder ist das eher Prosa, die durchaus interessant sein kann, aber eher ins Tagebuch gehört?

[bennySB]

Ich merke schon ich habe hier eine kleine Glaubensfrage gestartet xD

[BUG]

Zitat:

Zitat von Furtbichler (Beitrag 1217261)

Wie würde ein Kommentar bei dir aussehen, bei dem Du beschreibst, warum Du die Lösung so und nicht anders implementiert hast?

1. Hinweise auf Entwurfsmuster
2. Quellen und wichtige Eigenschaften der verwendeten Algorithmen
3. Dokumentation von Sachen, deren Gründe sonst schlecht aus dem Code ersichtlich wären.
   (zB. Maßnahmen gegen das Auftreten von False Sharing)
4. Gründe für Lösungen, wo man sich gegen das intuitive/kanonische Vorgehen entschieden hat.
   (zB. Dokumentation für Workarounds bei Fehlern in verwendeten Bibliotheken)

Natürlich kann einiges davon durch Verweise auf weitere Dokumentation ersetzt werden (z.B. im internen Wiki zum Projekt).

[Furtbichler]

Hi, bis auf #1 stimmte ich dem zu. Punkt 1 ist für Lehrveranstaltungen sinnvoll, aber nicht in produktivem Code.

Aber jedem das Seine.

[dataspider]

Wenn man bedenkt, das es der erste Code des TS ist, der dafür eine erstaunlich hohe Qualität hat...
... das er sich bereits jetzt in dieser Phase um sauberen Code Gedanken macht - Alle Achtung!

Wenn ich dann aber Hinweise zu ungarischer Notation lese... dann mach ich mir schon Sorgen.

Ich musste mal eine FIBU zerlegen (Namens Moses mit Oracle - DB und Unique als Sprache).
Dort hatten alle Tabellen- und Feldnamen ein U am Anfang.

Wenn du dann Feldnamen suchst und du hast eine List wie
UAnzBez
UStrasse
UOrt
U...

dann merkst du erst, wie bescheuert das ist und wie schwer das zu lesen ist.
Es dauert einfach viel länger!
Irgendwann war mal eine Sendung, da wurden Worte nur einen Bruchteil einer Sekunde eingeblendet.
Auch recht komplexe Worte hat man erkannt, obwohl man gar nicht so schnell lesen kann.

Und so kann man auch Code sehr schnell erfassen, wenn die Namen von Methoden, Variablen etc. eben lesbar sind.

Aus meiner Sicht ist das mit das Wichtigste und man kann nicht früh genug damit anfangen, lesbaren Code zu schreiben.
Man kann sich damit sooo viel Arbeit ersparen!

Frank

[Furtbichler]

Da hast Du die Diskussion sehr schnell wieder in die Spur gebracht. Worum ging es? Um einen Anfänger, der sich Gedanken um lesbaren Code macht und *abliefert*.

:thumb:

[kwhk]

False = 0
True = <> 0 (Alles was nicht NULL ist)
Wenn ich also prüfe if xx = True , dann kann xx alles <> 0 sein - wenn das passt OK, dann aber gleich auf if xx <> 0 testen.

[Luckie]

Eventuell für dich interessant: http://michael-puff.de/Programmierun...leanCode.shtml

[p80286]

Zitat:

Zitat von Furtbichler (Beitrag 1217261)

Es gilt aber auch: Ich muss mich der Situation anpassen: Entwickle ich in PHP und weiß, das alle Kollegen nur Notepad benutzen, sieht mein Code so aus, das man ihn auch ohne IDE schnell versteht.

So sollte es sein!!

Zitat:

Zitat von Furtbichler (Beitrag 1217246)

Hast Du ein paar Beispiele parat, wann beim Debuggen der Typ einer Variablen wichtig ist?

Hir war doch gestern noch ein Thread mit Dateigrößen? Da wäre es z.B. gut zu wissen ob signed oder unsigned. Und jetzt sag nicht Namen sind Schall und Rauch.

Zitat:

Zitat von Furtbichler (Beitrag 1217246)

Wie würde ein Kommentar bei dir aussehen, bei dem Du beschreibst, warum Du die Lösung so und nicht anders implementiert hast?

Kommt natürlich auf die Situation an

Delphi-Quellcode:

irgendwas:=irgendwas and $7FFF;  //15Bit wrap around

irgendwas:=irgendwas and $7FFF;  //set highest bit=0

Das hat mit "so und nicht anders" allerdings wenig zu tun, beantwortet aber die Frage nach dem Warum?

Gruß
K-H

[Furtbichler]

Zitat:

Zitat von p80286 (Beitrag 1217389)

Hir war doch gestern noch ein Thread mit Dateigrößen? Da wäre es z.B. gut zu wissen ob signed oder unsigned. Und jetzt sag nicht Namen sind Schall und Rauch.

Eine Dateigröße ist doch eine Zahl. Wieso muss man dann wissen, ob sie signed oder unsigned ist. Außer, man programmiert in C.

Zitat:

Delphi-Quellcode:

irgendwas:=irgendwas and $7FFF;  //15Bit wrap around

irgendwas:=irgendwas and $7FFF;  //set highest bit=0

Ich mache das so:

Delphi-Quellcode:

irgendwas:= WrapAround15Bit(irgendwas); // <-- Wobei ich mich frage, was das bedeuten soll.
irgendwas:= ClearHighestBit(irgendwas);

Drei Fliegen mit einer Klappe. Kommentiert, refaktorisiert und wiederverwendbar. Mach das mal mit Kommentaren :stupid:

[Elvis]

Zitat:

Zitat von Furtbichler (Beitrag 1217394)

Zitat:

Zitat von p80286 (Beitrag 1217389)

Hir war doch gestern noch ein Thread mit Dateigrößen? Da wäre es z.B. gut zu wissen ob signed oder unsigned. Und jetzt sag nicht Namen sind Schall und Rauch.

Eine Dateigröße ist doch eine Zahl. Wieso muss man dann wissen, ob sie signed oder unsigned ist. Außer, man programmiert in C.

Weil du sonst Blödsinn kriegst, wenn die Datei größer als 2^31 ist.
Es sind genau diese total saudummen Fehler, die dazu führten, dass man den 3GB-switch in Windows 2003 Produktiv fast nie einsetzen konnte:
Bei mindestens einer, leider kritischen, Software/Treiber hat mindestens ein Held (aka Pi**birne) einen Pointer als int anstatt als uint benutzt. -> BSOD oder Crash
Ganz toll! :thumb: Wer sowas macht frisst auch kleine Kinder...

Das heißt nicht, dass man jetzt die Ungarn rausholen soll!
Typenhinweise im Namen von Bezeichnern machen nur Sinn, wenn es absolut wichtig ist (siehe StatusByte).
Für Franzosen mag dieser ungarische Mist ja vllt. sogar lesbar sein (die schreiben eh alles verkehrtrum :mrgreen: ), aber nicht für Nicht-Franzosen.

Mit Kommentaren sehe ich es ähnlich wie Furtbichler. Wenn ich viele Kommentare in Code sehe, werde ich meistens sehr misstrauisch.
Entweder weil der Code tatsächlich viele Fallen umschiffen muss (was Domain-abhängig ja durchaus unvermeidbar sein kann), oder weil der Author keine Verhältnismäßigkeit gelernt hat. (Es werden in Unis teilweise die allerschlimmsten Comment-Nazis herangezüchtet... :? )

[BUG]

Zitat:

Zitat von Elvis (Beitrag 1217456)

Es werden in Unis teilweise die allerschlimmsten Comment-Nazis herangezüchtet... :?

Wobei ich unverständlichen kommentierten Code bevorzugen würde, wenn die Alternative unverständlicher Code ohne Kommentare ist.
Gut zu kommentieren üben kann man nur, wenn man überhaupt kommentiert und sich überflüssige Kommentare abzugewöhnen kosten meiner Erfahrung nach weniger Überwindung, als überhaupt mit kommentieren anzufangen.

Zitat:

Zitat von Elvis (Beitrag 1217456)

siehe StatusByte

Allerdings könnte man solche Sachen auch gut wegkapseln (Records in Delphi, Klassen in C++). Dank inline hat man dabei auch keine Geschwindigkeitsverluste.

[Furtbichler]

Zitat:

Zitat von Elvis (Beitrag 1217456)

Weil du sonst Blödsinn kriegst, wenn die Datei größer als 2^31 ist.

Ach, stimmt ja. Ich programmiere nicht mehr mit Delphi.

Zitat:

Zitat von BUG (Beitrag 1217459)

Wobei ich unverständlichen kommentierten Code bevorzugen würde, wenn die Alternative unverständlicher Code ohne Kommentare ist.

Das man 99% Schrott dem 100%igen vorzieht, ist ja wohl logisch und keine Erwähnung wert.

Zitat:

Gut zu kommentieren üben kann man nur, wenn man überhaupt kommentiert und sich überflüssige Kommentare abzugewöhnen kosten meiner Erfahrung nach weniger Überwindung, als überhaupt mit kommentieren anzufangen.

Ich habe nie kommentiert, sondern immer eine korrekte Nomenklatur angestrebt. Nach Lektüre der Clean-Code Philosophien wurde mir dann klar, das ich wohl 20 Jahre in die richtige Richtung marschiert bin.