CODEWITHSUNDEEP
delphiswitch-statementcyclomatic-complexity
AlexV
AlexV

Reputation: 23108

In Delphi, should I care about Cyclomatic Complexity for "case of" statements?

I'm using the built-in "Method Toxicity Metrics" in Delphi 10.3 in my code review flow and I have the following method:

function LoadTagsFromFile(const Filename: String; var Tags: TFileTags; var LastError: Integer): Boolean;
var
    AudioType: TAudioFormat;

begin
    AudioType := ExtToAudioType(ExtractFileExt(FileName));

    case AudioType of
        afApe: Result := LoadApeTags(Filename, Tags, LastError);

        afFlac: Result := LoadFlacTags(Filename, Tags, LastError);

        afMp3: Result := LoadMp3Tags(Filename, Tags, LastError);

        afMp4: Result := LoadMp4Tags(Filename, Tags, LastError);

        afOgg: Result := LoadOggTags(Filename, Tags, LastError);

        afWav: Result := LoadWavTags(Filename, Tags, LastError);

        afWma: Result := LoadWmaTags(Filename, Tags, LastError);

        else
            Result := LoadTags(Filename, Tags, LastError);
    end;
end;

Which is red flagged (CC = 8 making overall toxicity over 1) but I'm puzzled as how I could fix this specific case? Should I even care for this example?

Upvotes: 1

Views: 602

Answers (1)

Dave Novo
Dave Novo

Reputation: 703

You should not care at all for this example, IMHO. Firstly, the code in each case statement in clear and easy to read. Any other approach would make the code much more difficult to understand for no performance gain. i.e. you are reading a file from disk, you will never be able to measure any performance difference arising from any re-formulation of your case statement compared to the disk reading process.

Also, since you are using Delphi 10.3 you should take advantage of record helpers. I would make a 

TAudioFormatHelper = record helper for TAudioFormat
 private
   function LoadApeTags(Filename, Tags, LastError):boolean; 
   ... other Load functions here...
 public 
    procedure LoadTagsFromFile(const Filename: String; var Tags: TFileTags; var LastError: Integer): Boolean;
    procedure SetFromFileName(const FileName:string)     

end

That way you get away from these global type methods and tie them to your enum.

i.e.

var
  audioFormat:TAudioFormat;
begin
  audioFormat.SetFromFileName(.....)
  audioFormat.LoadTagsFromFIle(.....)

of course, LoadTagsFromFile could set the audioFormat enum as well, but that is a different story. I am just going with our design that you set the enum value based on extension first.

Upvotes: 1

Related Questions