Have Geek Brain

Can we avoid multiple if?

I tried to improve my level to write an improved version, but failed.

inFiles.ToList().ForEach(i =>
{
    filePath = inFolder + "\\" + i.Value;

    if (i.Key.Equals(replacementFile))
    {
        replacementCollection = GetReplacementDataFromFile(filePath);
    }
    else if (i.Key.Equals(standardizationFile))    
    {
        standardizationCollection = GetStandardizationDataFromFile(filePath);
    }                   
});

The problem is that I cannot use the switch case here because the comparison variables are not constant.

Please help improve this code.

I am using C # (3.0).

thanks

+2


a source to share


7 replies


The code you have does not have any redundant ifs.

You have three possibilities: Belongs to replacement, belongs to standardization, does not belong to any of them. You have an efficient, readable way to do it.



I would keep it as it is.

+9


a source


You can use Replace Conditional with Polymorphism if you have huge logic.



+1


a source


Okay one more try (and still no explicit if-statements!):

inFiles.ToList().ForEach(i =>

{

  filePath = inFolder + "\\" + i.Value;
  replacementCollection = testForReplacement(i,filePath,replacementFile);
  standardizationCollection =  testForStandardization(i,filePath,standardizationFile);
  someOtherCollection_1 =  testForOtherCollection(i,filePath,otherFile);
  ....//more statements like this...
});
...
...
...
Collection testForReplacement(i,filePath,testFile)
{
    return i.Key.Equals(testFile) ? GetReplacementDataFromFile(filePath) :null;
}
Collection testForStandardization(i,filePath,testfile)
{
    return i.Key.Equals(testFile) ? GetStandardizationDataFromFile(filePath) :null;
}
Collection testForSomeOtherCollection(i,filePath,testfile)
{
    return i.Key.Equals(testFile) ? GetOtherDataFromFile(filePath) :null;
}
...///more functions like this...

This is more pseudo code than real code (not sure if it will compile as is), but I hope this is important.;)

This code looks redundant, but it's easy to write a script / macro that can take a list of all possible file types and generate (or re-generate when the template changes) all the necessary functions and instructions for using them. The generated code can save you a lot of time if you do it right!

0


a source


You seem to be looking for the last item in the list, which is a replacement or standardization file.

I'm not sure what should happen if there are multiple files of the same type in the collection.

This is not as efficient as you have to go through the list twice .. but perhaps it is more expressive, and if the file list is small, it might be a premature optimization problem!

inFiles.Where(i => i.Key.Equals(replacementFile)).ToList().ForEach( i =>
{
    replacementCollection = GetReplacementDataFromFile(Path.Combine(inFolder, i.Value));
}

inFiles.Where(i => i.Key.Equals(standardizationFile)).ToList().ForEach( i =>
{
    standardizationCollection = GetStandardizationDataFromFile(Path.Combine(inFolder, i.Value));
}
0


a source


Very readable, I don't know what you are hoping to get by removing the if. I would leave it as it is, but maybe add the final version instead of just ending the method. This may throw an exception or write some information.

0


a source


If inFiles is a dictionary, you can do this:

replacementCollection = 
    inFiles.ContainsKey(replacementFile) ? 
        GetReplacementDataFromFile(
            Path.Combine(inFolder, inFiles[replacementFile]) ) : null;

standardizationCollection = 
    inFiles.ContainsKey(standardizationFile) ? 
        GetStandardizationDataFromFile(
            Path.Combine(inFolder, inFiles[standardizationFile]) ) : null;

You can make it shorter if your GetReplacementDataFromFile, GetStandardizationDataFromFile can handle an empty string parameter correctly:

string value;


replacementCollection = 
    GetReplacementDataFromFile( 
        inFiles.TryGetValue(replacementFile, out value) ? Path.Combine(inFolder, value) : string.Empty );

standardizationCollection = 
    GetStandardizationDataFromFile( 
        inFiles.TryGetValue(standardizationFile, out value) ? Path.Combine(inFolder, value) : string.Empty );
0


a source


My version:

foreach(var file in inFiles)
{
  filePath = Path.Combine(inFolder, file.Value);
  if(file.Key.Equals(replacementFile))
  {
    replacementCollection = GetReplacementDataFromFile(filePath);
    // ATTENTION ATTENTION LOOP CONTINUATION STATEMENT FOLLOWS
    continue;
    // THIS CONCLUDES OUR USE OF THE LOOP CONTINUATION STATEMENT
  }
  if (file.Key.Equals(standardizationFile))
    standardizationCollection = GetStandardizationDataFromFile(filePath);
}

Not sure why you should ToList and ForEach, but it doesn't improve readability at all. Also, your path concatenation is bad. Always use the Path type to perform file operations. I also don't like the instructions for stacking if, so I usually use gate commands; I will exit / shorten the method or loop instead of else if

. Some people think this is bad practice (only entry, only exit from logic), but I think they are full of it.

-2


a source






More articles:


All Articles