How to apply if condition in multiple statements, as a best practice?

Question

I need to apply If condition in multiple statements like below.

Two questions:

Any best practice to get it done?

Returning control from foreach loop like below: is it safe doing so?

if (assettFileParser.AddFile(_cfgFile, MyFileType.CFG) == ParserReturnCode.FileNotFound) return false;
    if (_psFiles != null)
         foreach (var file in _psFiles)
                  if(assettFileParser.AddFile(file, MyFileType.PS)== ParserReturnCode.FileNotFound) return false;
    if (_picFiles != null)
         foreach (var file in _picFiles)
                  if (assettFileParser.AddFile(file, MyFileType.ParaIC) == ParserReturnCode.FileNotFound) return false;
    if (_icFiles != null)
        foreach (var file in _icFiles)
                 if (assettFileParser.AddFile(file, MyFileType.IC) == ParserReturnCode.FileNotFound) return false;
    if (_xMasterFiles != null)
        foreach (var file in _xMasterFiles)
                 if (assettFileParser.AddFile(file, MyFileType.XMaster) == ParserReturnCode.FileNotFound) return false;

1. Post a single question. 2. Question on best practices tend to be off-topic, as they are often opinion-based - as in your question. 3. What do you mean by "is it safe doing so"? — MakePeaceGreatAgain, Dec 07 '17 at 10:21
Best practice is to make it clear and readable. Define 'safe'? — Equalsk, Dec 07 '17 at 10:21
If you have the almost the same 3 lines of code repeated 4 times there's almost certainly an opportunity to refactor it. With the right helper functions the above could be expressed as something like ```return tryAdd(_cfgFile, MyFileType.CFG) && tryAdd(_psFiles, MyFileType.PS) && tryAdd(_picFiles, MyFileType.ParaIC) && tryAdd(_icFiles, MyFileType.IC) && tryAdd(_xMasterFiles, MyFileType.XMaster)``` — Dylan Nicholson, Dec 07 '17 at 10:31
Generally I prefer exceptions to return codes (e.g., see here https://stackoverflow.com/questions/99683/which-and-why-do-you-prefer-exceptions-or-return-codes), so IMHO AddFile should throw an exception if the file is not found. This would avoid you having to explicitly check the return code for each call to AddFile (but you would still want to handle the exception at some point, but probably higher up). — Polyfun, Dec 07 '17 at 10:48
@HimBromBeere, "is it safe doing so?" , should be read like - "should I do it?" — WpfBee, Dec 07 '17 at 10:56

Georg Patscheider · Accepted Answer · 2017-12-07T12:59:54.723

Here I combine the suggestions of Dylan Nicholson and Poylfun: use a helper method that throws an exception (you can introduce your own exception type by inheriting from System.Exception). I also moved the foreach into the helper method, it accepts a collection of files. The helper also makes sure that the collection of files is not null, so the caller does not have to check this.

public void AddAssetFiles(AssettFileParser assettFileParser,
                          IEnumerable<string> files, MyFileType fileType) {
   if (files == null) { 
       return; // do nothing
   }
   // note that looping over an empty collection will do nothing
   foreach (var file in files) {
        var returnCode = assettFileParser.AddFile(file, fileType); 
        if (returnCode == ParserReturnCode.FileNotFound) {
            throw new AssetFileNotFoundException($"File '{file}' was not found.");
        }
   }
}

Usage example. First file not found will result in termination of the whole try block. No further files are processed afterwards.

try {
    // put _cfgFile into a string array so it can be passed as IEnumerable<string>
    AddAssetFiles(assettFileParser, new string[] { _cfgFile }, MyFileType.CFG);
    AddAssetFiles(assettFileParser, _psFiles, MyFileType.PS);
    AddAssetFiles(assettFileParser, _picFiles, MyFileType.ParaIC);
    // ...
    return true;
}
catch (AssetFileNotFoundException ex) {
    Logger.Error(ex);
    return false;
}

Side note on code style: you should always put if and foreach blocks into brackets { }, even if the block contains only a single line. This can forestall problems with dangling else etc.

How to apply if condition in multiple statements, as a best practice?

1 Answers1