Was ist an diesen try und except block falsch?

[Deltachaos]

Ich weis nicht das ich hier falsch gemacht habe.
Es werden nur die versionen die mit dem erste und dem letzte try und except block ausgelesen werden angezeigt.

währe net wen da wer rüber gugen würde.

Delphi-Quellcode:

uses Registry;

procedure delphi_versions;
var regist: TRegistry;
    pub: TStringList;
    pubdir, pubbrc32, pubbrcc32: string;
    pubi: integer;
begin
  regist := TRegistry.Create;
  try
    regist.RootKey := HKEY_LOCAL_MACHINE;
    try
      pub := TStringList.Create;
      pubdir := '';
      pubbrc32 := '';
      pubbrcc32 := '';
      pubi := 0;
      regist.OpenKey('SOFTWARE\Borland\Delphi', false);
      regist.GetKeyNames(pub);
      regist.CloseKey;
      for pubi := 0 to pub.count - 1 do
      begin
        regist.OpenKeyReadOnly('SOFTWARE\Borland\Delphi\' + pub[pubi]);
        if regist.ValueExists('RootDir') then
        begin
          pubdir := regist.ReadString('RootDir');
          if pubdir[length(pubdir)] = '\' then
            pubdir := pubdir + 'Bin\'
          else
            if pubdir[length(pubdir)] = '/' then
              pubdir := pubdir + 'Bin/'
            else
              pubdir := pubdir + '\Bin\';
          if FileExists(pubdir + 'brc32.exe') then
            pubbrc32 := pubdir + 'brc32.exe';
          if FileExists(pubdir + 'brcc32.exe') then
            pubbrcc32 := pubdir + 'brcc32.exe';
          additem(pub[pubi], pubbrc32, pubbrcc32);
        end;
      end;
    finally
    end;


    try
      pub := TStringList.Create;
      pubdir := '';
      pubbrc32 := '';
      pubbrcc32 := '';
      pubi := 0;
      regist.OpenKey('SOFTWARE\CodeGear\BDS', false);
      regist.GetKeyNames(pub);
      regist.CloseKey;
      for pubi := 0 to pub.count - 1 do
      begin
        regist.OpenKeyReadOnly('SOFTWARE\CodeGear\BDS\' + pub[pubi]);
        if regist.ValueExists('RootDir') then
        begin
          pubdir := regist.ReadString('RootDir');
          if pubdir[length(pubdir)] = '\' then
            pubdir := pubdir + 'Bin\'
          else
            if pubdir[length(pubdir)] = '/' then
              pubdir := pubdir + 'Bin/'
            else
              pubdir := pubdir + '\Bin\';
          if FileExists(pubdir + 'brc32.exe') then
            pubbrc32 := pubdir + 'brc32.exe';
          if FileExists(pubdir + 'brcc32.exe') then
            pubbrcc32 := pubdir + 'brcc32.exe';
          additem(pub[pubi], pubbrc32, pubbrcc32);
        end;
      end;
    finally
    end;


    try
      pub := TStringList.Create;
      pubdir := '';
      pubbrc32 := '';
      pubbrcc32 := '';
      pubi := 0;
      regist.OpenKey('SOFTWARE\Borland\BDS', false);
      regist.GetKeyNames(pub);
      regist.CloseKey;
      for pubi := 0 to pub.count - 1 do
      begin
        regist.OpenKeyReadOnly('SOFTWARE\Borland\BDS\' + pub[pubi]);
        if regist.ValueExists('RootDir') then
        begin
          pubdir := regist.ReadString('RootDir');
          if pubdir[length(pubdir)] = '\' then
            pubdir := pubdir + 'Bin\'
          else
            if pubdir[length(pubdir)] = '/' then
              pubdir := pubdir + 'Bin/'
            else
              pubdir := pubdir + '\Bin\';
          if FileExists(pubdir + 'brc32.exe') then
            pubbrc32 := pubdir + 'brc32.exe';
          if FileExists(pubdir + 'brcc32.exe') then
            pubbrcc32 := pubdir + 'brcc32.exe';
          additem(pub[pubi], pubbrc32, pubbrcc32);
        end;
      end;
    finally
    end;

  finally
    regist.free;
  end;
end;

[jfheins]

Du weist schon, dass ein

Delphi-Quellcode:

try
// ...
// ...
finally
end;

Ungefähr ... gar keinen Effekt hat?

[Deltachaos]

nein weis ich nicht.
also muss ich etwas nach dem finally angeben oder wie schat das aus?

[taaktaak]

In den finally-Block gehört Code, der unter allen Umständen, also auch bei einem Fehler, ausgeführt werden muss. Also z.B. das Freigeben von Ressourcen.

[Deltachaos]

nun gut.

nur jetzt habe ich mal alles aus den blocks herausgenommen also so:

Delphi-Quellcode:

procedure delphi_versions;
var regist: TRegistry;
    pub: TStringList;
    pubdir, pubbrc32, pubbrcc32: string;
    pubi: integer;
begin
  regist := TRegistry.Create;
  try
    regist.RootKey := HKEY_LOCAL_MACHINE;
//    try
      pub := TStringList.Create;
      pubdir := '';
      pubbrc32 := '';
      pubbrcc32 := '';
      pubi := 0;
      regist.OpenKey('SOFTWARE\Borland\Delphi', false);
      regist.GetKeyNames(pub);
      regist.CloseKey;
      for pubi := 0 to pub.count - 1 do
      begin
        regist.OpenKeyReadOnly('SOFTWARE\Borland\Delphi\' + pub[pubi]);
        if regist.ValueExists('RootDir') then
        begin
          pubdir := regist.ReadString('RootDir');
          if pubdir[length(pubdir)] = '\' then
            pubdir := pubdir + 'Bin\'
          else
            if pubdir[length(pubdir)] = '/' then
              pubdir := pubdir + 'Bin/'
            else
              pubdir := pubdir + '\Bin\';
          if FileExists(pubdir + 'brc32.exe') then
            pubbrc32 := pubdir + 'brc32.exe';
          if FileExists(pubdir + 'brcc32.exe') then
            pubbrcc32 := pubdir + 'brcc32.exe';
          additem(pub[pubi], pubbrc32, pubbrcc32);
        end;
      end;
//    finally
//      showmessage('Borland\Delphi');
//    end;


//    try
      pub := TStringList.Create;
      pubdir := '';
      pubbrc32 := '';
      pubbrcc32 := '';
      pubi := 0;
      regist.OpenKey('SOFTWARE\CodeGear\BDS', false);
      regist.GetKeyNames(pub);
      regist.CloseKey;
      for pubi := 0 to pub.count - 1 do
      begin
        regist.OpenKeyReadOnly('SOFTWARE\CodeGear\BDS\' + pub[pubi]);
        if regist.ValueExists('RootDir') then
        begin
          pubdir := regist.ReadString('RootDir');
          if pubdir[length(pubdir)] = '\' then
            pubdir := pubdir + 'Bin\'
          else
            if pubdir[length(pubdir)] = '/' then
              pubdir := pubdir + 'Bin/'
            else
              pubdir := pubdir + '\Bin\';
          if FileExists(pubdir + 'brc32.exe') then
            pubbrc32 := pubdir + 'brc32.exe';
          if FileExists(pubdir + 'brcc32.exe') then
            pubbrcc32 := pubdir + 'brcc32.exe';
          additem(pub[pubi], pubbrc32, pubbrcc32);
        end;
      end;
//    finally
//      showmessage('CodeGear\BDS');
//    end;


//    try
      pub := TStringList.Create;
      pubdir := '';
      pubbrc32 := '';
      pubbrcc32 := '';
      pubi := 0;
      regist.OpenKey('SOFTWARE\Borland\BDS', false);
      regist.GetKeyNames(pub);
      regist.CloseKey;
      for pubi := 0 to pub.count - 1 do
      begin
        regist.OpenKeyReadOnly('SOFTWARE\Borland\BDS\' + pub[pubi]);
        if regist.ValueExists('RootDir') then
        begin
          pubdir := regist.ReadString('RootDir');
          if pubdir[length(pubdir)] = '\' then
            pubdir := pubdir + 'Bin\'
          else
            if pubdir[length(pubdir)] = '/' then
              pubdir := pubdir + 'Bin/'
            else
              pubdir := pubdir + '\Bin\';
          if FileExists(pubdir + 'brc32.exe') then
            pubbrc32 := pubdir + 'brc32.exe';
          if FileExists(pubdir + 'brcc32.exe') then
            pubbrcc32 := pubdir + 'brcc32.exe';
          additem(pub[pubi], pubbrc32, pubbrcc32);
        end;
      end;
//    finally
//      showmessage('Borland\BDS');
//    end;

  finally
    regist.free;
  end;
end;

nur der mitlere teil wird immernoch nicht ausgeführt. bzw. bringt kein ergebniss.

[Deltachaos]

hatt sich erledigt. nach

Delphi-Quellcode:

regist.OpenKeyReadOnly('SOFTWARE\Borland\BDS\' + pub[pubi]);

wurde ein

Delphi-Quellcode:

regist.CloseKey;

vergessen

[taaktaak]

Auch wenn es sich erledigt hat:

Es ist mühsam, den Code durchzuschauen. Im Buch "Clean code" wird dringend empfohlen, dass Funktionen nur 1-4 Zeilen enthalten sollen. Auch wenn ich das für übertrieben halte: Du solltest deinen Code möglichst in kleinere "Häppchen" unterteilen, dann wird sich ein Fehler viel schneller finden lassen!

PS: Wo wird/werden deine Stringlisten "pub" wieder freigegeben?

[Deltachaos]

nirgends
muss ich noch machen.

so ist er doch kürtzer:

Delphi-Quellcode:

procedure delphi_versionadd(ort: string);
var regist: TRegistry;
    pub: TStringList;
    pubdir, pubbrc32, pubbrcc32: string;
    pubi: integer;
begin
  regist := TRegistry.Create;
  try
    regist.RootKey := HKEY_LOCAL_MACHINE;
    pub := TStringList.Create;
    regist.OpenKey(ort, false);
    regist.GetKeyNames(pub);
    regist.CloseKey;
    if pub.count > 0 then
    begin
      for pubi := 0 to pub.count - 1 do
      begin
        regist.OpenKeyReadOnly(ort + '\' + pub[pubi]);
        if regist.ValueExists('RootDir') then
        begin
          pubdir := regist.ReadString('RootDir');
          regist.CloseKey;
          if pubdir[length(pubdir)] = '\' then
            pubdir := pubdir + 'Bin\'
          else
            if pubdir[length(pubdir)] = '/' then
              pubdir := pubdir + 'Bin/'
            else
              pubdir := pubdir + '\Bin\';
          if FileExists(pubdir + 'brc32.exe') then
            pubbrc32 := pubdir + 'brc32.exe';
          if FileExists(pubdir + 'brcc32.exe') then
            pubbrcc32 := pubdir + 'brcc32.exe';
          additem(pub[pubi], pubbrc32, pubbrcc32);
        end;
      end;
    end;
  finally
    regist.free;
    pub.Free;
  end;
end;

procedure delphi_versions;
begin
  delphi_versionadd('SOFTWARE\Borland\Delphi');
  delphi_versionadd('SOFTWARE\Borland\BDS');
  delphi_versionadd('SOFTWARE\CodeGear\BDS');
end;

[taaktaak]

Ja, schaut schon viel besser aus!
Wenn das Ganze jetzt z.B. noch zwei lokale Prozeduren bekommt, ist der Code schon "cleaner".
Mir ist noch aufgefallen: Was passiert, wenn weder 'brc32.exe' noch 'brcc32.exe' gefunden werden? Immerhin verwendest du in beiden Fällen ein if... - aber auch wenn in beiden Fällen nichts gefunden würde, fügst du in die Liste (dann) leere Inhalte ein?! Was/wo wird mit "additem" eigentlich aufgerufen?

[Deltachaos]

ja dann wir ein leere inhalt hinzugefügt

ich bin gerade dabei das GUI für mein "RES Builder" zu machen.
ich hab den nochmal komplett neu angefangen.

Ich leg das mal im anhang an.
hat aber noch keine richtige Funktion. Es wird nur das Forumular aufgebaut und in einem Feld benutzerringaben geprüft.

PS: Ich habs noch kürzer gemacht. Mehr oder weniger^^

Delphi-Quellcode:

procedure delphi_versionadd(ort, key: string);
var regist: TRegistry;
    pub: TStringList;
    pubdir, pubbrc32, pubbrcc32: string;
    pubi: integer;
begin
  regist := TRegistry.Create;
  try
    regist.RootKey := HKEY_LOCAL_MACHINE;
    pub := TStringList.Create;
    regist.OpenKey(ort, false);
    regist.GetKeyNames(pub);
    regist.CloseKey;
    if pub.count > 0 then
    begin
      for pubi := 0 to pub.count - 1 do
      begin
        regist.OpenKeyReadOnly(ort + '\' + pub[pubi]);
        if regist.ValueExists(key) then
        begin
          pubdir := regist.ReadString(key);
          regist.CloseKey;
          pubdir := IncludeTrailingPathDelimiter(pubdir) + 'Bin\';
          if FileExists(pubdir + 'brc32.exe') then
            pubbrc32 := pubdir + 'brc32.exe';
          if FileExists(pubdir + 'brcc32.exe') then
            pubbrcc32 := pubdir + 'brcc32.exe';
          additem(pub[pubi], pubbrc32, pubbrcc32);
        end;
      end;
    end;
  finally
    regist.free;
    pub.Free;
  end;
end;

procedure delphi_versions;
var i: integer;
begin
//L_NAME := lang.ReadString('lang', 'L_NAME', 'Deutsch');
  if settings.SectionExists('DelphiVersions') then
  begin
    i := 1;
    while settings.ValueExists('DelphiVersions', inttostr(i) + 'KEY') do
    begin
      delphi_versionadd(settings.ReadString('DelphiVersions', inttostr(i) + 'KEY', ''), settings.ReadString('DelphiVersions', inttostr(i) + 'DIR', 'RootDir'));
      i := i +1;
    end;
  end
  else
  begin
    delphi_versionadd('SOFTWARE\Borland\Delphi', 'RootDir');
    delphi_versionadd('SOFTWARE\Borland\BDS', 'RootDir');
    delphi_versionadd('SOFTWARE\CodeGear\BDS', 'RootDir');
  end;
end;

[Satty67]

Weil man hier schnell mit (konstruktiver) Kritik ist, auch mal was positives hinterher.

Dafür das Du Anfänger bist (wie im anderen Thread geschrieben), sieht Dein Code durchaus schon sehr gut aus. Gute Einrückungen und aussagekräftige Namen. Das hat man hier von jungen Programmierern schon anders gesehen. Ich hab recht schnell die Funktion des Codes verstehen können.

Die Verbesserung der Übersicht durch aufteilen in kleinere Funktion, die taaktaak vorschlägt, sind auch sehr wichtig. Das ist aber nicht immer ganz leicht und ich bastel auch heute noch oft mega-Funktionen, die ich dann erst hinterher ausmisten muss.

[Hansa]

Zitat:

Zitat von Satty67

...Die Verbesserung der Übersicht durch aufteilen in kleinere Funktion, die taaktaak vorschlägt, sind auch sehr wichtig...

aber nicht so :

Zitat:

Zitat von taaktaak

..Im Buch "Clean code" wird dringend empfohlen, dass Funktionen nur 1-4 Zeilen enthalten sollen.

Ich dehne das mal auf Prozeduren aus und halte es da lieber mit Daniel Magins Faustregel : möglichst maximal 1 DIN A 4 Seite pro Prozedur. Zu stark aufgeteilt geht der Zusammenhang eventuell komplett verloren und man sucht sich dumm und dämlich nach Mini-Funktionen/Prozeduren. :zwinker:

[Deltachaos]

Zitat:

Zitat von Satty67

Dafür das Du Anfänger bist (wie im anderen Thread geschrieben), sieht Dein Code durchaus schon sehr gut aus. Gute Einrückungen und aussagekräftige Namen. Das hat man hier von jungen Programmierern schon anders gesehen. Ich hab recht schnell die Funktion des Codes verstehen können.

Gut Anfänger mit Delphi aber ich habe bis jetzt schon so einiges mit PHP gemacht und möcht sagen das ich da ganz guz bin.
Daher kommen warscheinlich auch die aussagekräftige Namen und die gute Einrückung.

Zu dem Zerteilen sage ich volgendes:

Ich Persönlich finde das Delphi Felnster für den Code sehr klein. und wenn man es Maximirt kann ich aber nicht damit arbeiten da ich andere Wichtige teile nicht sehe.
Aber das liegt warscheinlich auch an meinem 15" Monitor (das ist bald zum Glück nicht mehr - Hab mir heute einen neuen bestellt 21,5" ich komme)

Nur bei PHP hatte ich halt eine viel grösere Editor Fläche.
Da hat man auch bei viel Code nicht den Überblick verloren.

[blablab]

1-4 zeilen pro funktion ist ja mehr als übertrieben. allein deine funktion (function..., begin, end) hat ja schonmal mindestens 3 zeilen. Das würde dann ja bedeuten, dass der quelltext am schluss weniger als 50% "richtigen code" enthält...

Also ich find es kommt auch drauf an was in der prozedur/funktion steht. Eigentlich benutz ich nur Funktionen um redundanz zu verhindern. Ich find es schwachsinnig in einer Funktion 10 weitere Funktionen aufzurufen, die nur von dieser und sonst keiner Funktion benutzt werden. Ich les doch viel lieber den Code von oben nach unten wie ein Buch, statt dass ich die ganze Zeit hin und herspringen muss.
Funktionen, richtig angewandt, sind auf jeden Fall sinnvoll und auch wichtig, aber man kann es auch wirklich übertreiben. Wenn man nunmal 30 zeilen Code hat bei denen es sich nicht lohnt ihn in Funktionen aufzuteilen, dann soll man es halt lassen. Wenn man versucht krampfhaft was aufzuteilen wirds am schluss eher unübersichtlicher als übersichtlicher...

[Satty67]

Zuerst entschuldige ich mich, das ich eine OffTopic Diskussion im Thread ausgelöst habe. Leider nicht zum ersten mal, mir fehlt es da wohl etwas an Disziplin :oops:

Zitat:

Zitat von Deltachaos

Gut Anfänger mit Delphi aber ich habe bis jetzt schon so einiges mit PHP gemacht
[...]
Zu dem Zerteilen sage ich folgendes:

Gut, das relativiert das Lob dann etwas ;)

1-4 Zeilen ist auch etwas übertrieben. aber der Code Deiner letzten Prozedure enthält noch ein paar in sich abgeschlossenen Aufgaben, die sich einfach extrahieren lassen. Eine ganze Bildschirmseite finde ich nur sinnvoll, wenn der Code eine in sich geschlossene Aufgabe erfüllt (Sortierroutine, Case-Block etc.)

[taaktaak]

Moin, Moin.
Also, ich finde nicht, dass wir so deutlich "off-topic" geworden sind. Der Vorsatz, einen übersichtlichen Code zu schreiben, ist eine wesentliche Voraussetzung um die Frage zu beantworten, warum ein bestimmtes Konstrukt falsch ist.
Ich muss gestehen, mit dem Postulat "1-4 Zeilen" wollte ich auch ein wenig provozieren. Diese These aus dem zitierten Buch hat mich heftig irritert und unsicher gemacht - bin ich doch selbst der Letzte, der sich auch nur annähernd an diese Vorgabe hält. Eure Anmerkungen hierzu haben mich aber nun doch wieder beruhigt :-D

[Hansa]

Zitat:

Zitat von Satty67

...Eine ganze Bildschirmseite finde ich nur sinnvoll, wenn der Code eine in sich geschlossene Aufgabe erfüllt...

Sofern sich das auf mich bezieht : ich habe gesagt eine DIN A 4 Seite, also ein Blatt Papier mit 72 Zeilen !! Habe mal nachgeguckt. Eine Prozedur bedi mir hat tatsächlich sogar 140 Zeilen. Das ist eine OnDrawCell für Stringgrid. Kaum sinnvoll aufzuteilen. Ich könnte die zwar so aufteilen, dass sie auf nur eine DIN A 4 Seite passen würde, aber ich hätte dann 20 Prozeduren mehr mit 2-3 Zeilen. Macht mind. noch 100 Zeilen mehr. Dann wär ich aber schon bei 1-4 Zeilen pro Prozedur. :wall: Bis auf die eine natürlich. :mrgreen:

[himitsu]

jupp, es ist halt nicht immer besser alles auf Biegen und Brechen aufzuzeilen

Billiges Beispiel:

Delphi-Quellcode:

procedure ProcedureX;
begin
  WriteLn('1');
  WriteLn('2');
  WriteLn('3');
  WriteLn('4');
  WriteLn('5');
end;

und nochmal mit 2 Zeilen ... ich frag micht was da nun übersichtlicher ist :gruebel:
(wenn hier die Zahlen nicht in den Befehlen drinstehn würden, würden viele wohl nichtmal richtig erkennen können in welcher Reinfolge hier was ausgeführt würde)

Delphi-Quellcode:

procedure Procedure1;
begin
  WriteLn('2');
  WriteLn('3');
end;

procedure Procedure2;
begin
  Procedure1;
  WriteLn('4');
end;

procedure Procedure3;
begin
  WriteLn('1');
  Procedure2;
end;

procedure ProcedureX;
begin
  Procedure3;
  WriteLn('5');
end;

[Deltachaos]

Ich denke mal darüber kann man sich streiten.

Man sollte so aufteilen das Zusammenhänge erhalten bleiben und am ende aber weniger code herauskommt.

Es hatt keinen sin zwanghaft zu unterteilen wenn mann am ende das doppelte an code hat

[blablab]

Und der Code von himitsu steht ja noch schön sortiert untereinander, so dass man es wieder recht einfach von oben nach unten lesen kann und alles im Überblick hat.
Aber meistens ist es eben nicht so, da musst du dann in der unit hin und herspringen und man hat bei prozedur 5 angelangt schon wieder vergessen was prozedur 1 macht.
Und wenn es sich nicht gerade um WriteLn handelt, dann muss man wahrscheinlich noch Parameter übergeben usw...

Also ich find die Behauptung "Je mehr man den Code in Prozeduren/Funktionen aufteilt, desto besser" ist meistens falsch. Ich finds meistens übersichtlicher, wenn man nur das nötigste aufteilt. (Wobei das natürlich auch wieder nicht optimal ist...)

Man sollte einfach sinnvoll damit umgehen und weder vor einer Funktion mit 2 Zeilen noch vor einer mit 50 zeilen zurückschrecken. Hauptsache ist, es macht Sinn zu unterteilen bzw nicht zu unterteilen.