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


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 von bennySB:

Ich bin offen für Zuckerbrot und Peitschen

Ok, dann pack' ich mal die Peitsche aus

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? ) 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 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. 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 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 (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 , 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

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