Discussion :

[ouiouioui]

Bonjour,
je souhaiterais votre avis savoir si selon vous la procédure que j'ai créée pour lire un fichier JSON Vous paraît pas trop moche ? Par rapport entre autres au niveau de la création, libération des objets.
Merci par avance pour vos réponses.

Code JSON :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
{
    "DelphiVersions": [
        {
            "Name": "Delphi Rio",
            "RegistryPath": "SOFTWARE\\Embarcadero\\BDS\\20.0\\Library",
            "Path": [
                "Browsing Path",
                "Search Path",
                "Debug DCU Path"
            ]
        },
        {
            "Name": "Delphi Sydney",
            "RegistryPath": "SOFTWARE\\Embarcadero\\BDS\\21.0\\Library",
            "Path": [
                "Browsing Path",
                "Search Path",
                "Debug DCU Path"
            ]
        },
        {
            "Name": "Delphi Alexandria",
            "RegistryPath": "SOFTWARE\\Embarcadero\\BDS\\22.0\\Library",
            "Path": [
                "Browsing Path",
                "Search Path",
                "Debug DCU Path"
            ]
        },
        {
            "Name": "Delphi Athens",
            "RegistryPath": "SOFTWARE\\Embarcadero\\BDS\\23.0\\Library",
            "Path": [
                "Browsing Path",
                "Search Path",
                "Debug DCU Path"
            ]
        }
    ]
}

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
 
  TDelphiDetails = Class
    FName, FRegistryPath: String;
    FPath: TList<String>;
    Constructor Create(AName, ARegistryPath: String);
    Destructor Destroy; Override;
  End;
 
  TDelphiVersion = Class(TObjectList<TDelphiDetails>)

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
 
Function TDelphiVersion.LoadFromFileDelphiVersion: Boolean;
Var
  JSONArray, JSONArrayPath: TJSONArray;
  JSONValue, JSONValueDelphiDetails, JSONValuePath: TJSONValue;
  Name, RegistryPath, Path: String;
  DelphiDetails: TDelphiDetails;
  sl: TStringList;
Begin
  sl        := TStringList.Create;
  JSONValue := Nil;
  Try
    sl.LoadFromFile(FileDelphiVersion);
    JSONValue := TJSONObject.ParseJSONValue(sl.Text);
 
    If Not JSONValue.TryGetValue<TJSONArray>('DelphiVersions', JSONArray) Then
      Exit(False);
 
    For JSONValueDelphiDetails In JSONArray Do
    Begin
      If Not JSONValueDelphiDetails.TryGetValue<String>('Name', Name) Then
        Exit(False);
 
      If Not JSONValueDelphiDetails.TryGetValue<String>('RegistryPath', RegistryPath) Then
        Exit(False);
 
      If Not JSONValueDelphiDetails.TryGetValue<TJSONArray>('Path', JSONArrayPath) Then
        Exit(False);
 
      DelphiDetails := TDelphiDetails.Create(Name, RegistryPath);
 
      For JSONValuePath In JSONArrayPath Do
      Begin
        If JSONValuePath.TryGetValue<String>(Path) Then
        Begin
          DelphiDetails.FPath.Add(Path);
        End;
      End;
 
      Add(DelphiDetails);
    End;
 
  Finally
    sl.Free;
    If Assigned(JSONValue) Then
      FreeAndNil(JSONValue);
  End;
 
  Result := True;
End;

[ShaiLeTroll]

As-tu activé ReportMemoryLeaksOnShutdown ?
Je sais que le TJSONObject possède ses objets en interne, d'ailleurs on a déjà eu des surprises à ce sujet avec la fonction TryGetValue avec un Path (Violation d'accès avec JSON Array).

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
If Assigned(JSONValue) Then
      FreeAndNil(JSONValue);

C'est redondant

la méthode Free vérifie que l'objet n'est pas nil avant d'appeler Destroy

JSONValue est une variable locale, l'affectation à nil n'est pas donc pas nécessaire

au final, ceci ceci suffit

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

JSONValue.Free();

D'ailleurs, j'aurais plutôt fait TStringList.create() try LoadFromFile ParseJSONValue try finally JSONValue finally TStringList.
c'est plus verbeux mais évite le JSONValue à nil et de vérifier si l'on doit libérer l'objet encore non instancié en cas d'erreur sur LoadFromFile


En cas de paranoia absolu, genre un EOutOfMemory ou un JSON mal formaté à ce niveau comme "Path": [{}], tu pourrais avoir TDelphiDetails instancié et non ajouté à la TObjectList (qui je suppose gère la libération ultérieurement)

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
    DelphiDetails := TDelphiDetails.Create(Name, RegistryPath);
    try 
      For JSONValuePath In JSONArrayPath Do
      Begin
        If JSONValuePath.TryGetValue<String>(Path) Then
        Begin
          DelphiDetails.FPath.Add(Path); // le Add échoue avec EOutOfMemory, il faut libérer DelphiDetails
        End;
      End;
 
      Add(DelphiDetails); // le Add échoue avec EOutOfMemory, il faut libérer DelphiDetails
      DelphiDetails := nil;
    except
      DelphiDetails.Free();
    end;

Tu peux aussi créer une TStringList temporaire pour récupérer les Path et si tout ce passe bien, tu peux faire l'instanciation de TDelphiDetails comme ceci :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
 
    LPaths := StringList.Create();
    try 
      For JSONValuePath In JSONArrayPath Do
      Begin
        If JSONValuePath.TryGetValue<String>(Path) Then
        Begin
          LPaths.Add(Path);
        End;
      End;
 
      DelphiDetails := TDelphiDetails.Create(Name, RegistryPath);
      Add(DelphiDetails); 
      DelphiDetails.AddPaths(LPaths); 
    finally
      LPaths .Free();
    end;

Ci-dessus une version qui ne fait pas confiance au JSON mais qui espère que la mémoire c'est bon, tu peux être extremement parano avec la forme

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
 
      try
        Add(DelphiDetails); 
      except
      begin
         DelphiDetails.Free(); 
         raise;
      end;

Sinon ajoute des sections private et public, FPath évite de l'utiliser en direct, passe par une property en lecture seule.

[ouiouioui]

Effectivement j'ajoute ReportMemoryLeaksOnShutdown Avec une directive conditionnelle en mode DEBUG Dès que je crée un projet ça devrait être inclus par défaut je trouve.

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
 
Function TDelphiVersion.LoadFromFileDelphiVersion: Boolean;
Var
  JSONArray, JSONArrayPath: TJSONArray;
  JSONValue, JSONValueDelphiDetails, JSONValuePath: TJSONValue;
  Name, RegistryPath, Path: String;
  DelphiDetails: TDelphiDetails;
  sl: TStringList;
Begin
  sl := TStringList.Create;
  Try
    sl.LoadFromFile(FileDelphiVersion);
    JSONValue   := Nil;
    Try
      JSONValue := TJSONObject.ParseJSONValue(sl.Text);
 
      If Not JSONValue.TryGetValue<TJSONArray>('DelphiVersions', JSONArray) Then
        Exit(False);
 
      For JSONValueDelphiDetails In JSONArray Do
      Begin
        If Not JSONValueDelphiDetails.TryGetValue<String>('Name', Name) Then
          Exit(False);
 
        If Not JSONValueDelphiDetails.TryGetValue<String>('RegistryPath', RegistryPath) Then
          Exit(False);
 
        If Not JSONValueDelphiDetails.TryGetValue<TJSONArray>('Path', JSONArrayPath) Then
          Exit(False);
 
        DelphiDetails := TDelphiDetails.Create(Name, RegistryPath);
        Try
          For JSONValuePath In JSONArrayPath Do
          Begin
            If JSONValuePath.TryGetValue<String>(Path) Then
            Begin
              DelphiDetails.FPath.Add(Path);
            End;
          End;
 
          Add(DelphiDetails);
        Except
          DelphiDetails.Free;
          Raise;
        End;
      End;
 
      Result := True;
    Finally
      If Assigned(JSONValue) Then
        JSONValue.Free;
    End;
  Finally
    sl.Free;
  End;
End;

J'ai corrigé selon tes commentaires merci. Par contre si j'initialise pas la variable J'ai un message du compilateur

[dcc32 Avertissement] uDelphiVersion.pas(151): W1036 La variable 'JSONValue' n'a pas été initialisée

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
 
  TDelphiDetails = Class
  Private
    FName, FRegistryPath: String;
    FPath: TList<String>;
  Public
    Constructor Create(AName, ARegistryPath: String);
    Destructor Destroy; Override;
    Property Name: String Read FName Write FName;
    Property RegistryPath: String Read FRegistryPath Write FRegistryPath;
    Property Path: TList<String> Read FPath;
  End;

Je ne comprends pas pourquoi il faut passer par une propriété avec lecture seule sur FPath, Éviter d'assigner un nouvel objet alors l'objet initial ne serait pas libéré lors de la destruction de la classe ?


Merci pour ces précieux conseils

[ShaiLeTroll]

Parce que c'est toujours Create try ... finally free et dans ces cas c'est Parse try ... finally free ... car Parse une Factory (proche d'un constructeur)

[ouiouioui]

C'est noté, pour tes explications

[ShaiLeTroll]

Je ne comprends pas pourquoi il faut passer par une propriété avec lecture seule sur FPath, Éviter d'assigner un nouvel objet alors l'objet initial ne serait pas libéré lors de la destruction de la classe ?

Il y a l'aspect de "protection", avec private voir même strict private, tu réduis le risque que l'on modifie la référence de FPath
Tu ne sais jamais qui maintiendra le code à l'avenir, toi, un autre développeur, c'est mon point de vue de professionnel.

Il y a aussi l'aspect "objet" / "encapsulation", tu peux vouloir modifier l'implémentation de ton objet sans devoir tout réécrire les appels
Cela serait surtout pour le cas d'une TStrings que tu instancierais par un TMaPropreStringList avec ton implémentation alternative, de l'extérieur de l'objet, on voit juste une TStrings.
La TStrings est surtout utile si tu veux afficher tes path dans une TListBox par exemple, ça fera moins de code qu'avec une TList<string>

En fait, je suis du genre à écrire

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

Property Paths[Index: Integer]: String Read GetPath write SetPath;

Avec ce cas là, tu masques totalement le stockage du tableau, il s'utilise pareil soit Paths[] qu'avec en TList ou TStrings grâce à la propriété Items[] ou Strings[] par défaut

et de fournir une méthode AddPath dans TDelphiDetails,
éventuellement une méthode PathsTo(TStrings) pour un besoin d'affichage