CODEWITHSUNDEEP
c#-3.0
Newbie
Newbie

Reputation: 1111

Can we avoid multiple if''s?

I tried my level best 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 a switch case over here because the comparison variables are not constant.

Kindly help to improve this code.

I am using C#(3.0).

Thanks

Upvotes: 2

Views: 495

Answers (6)

Michael Buen
Michael Buen

Reputation: 39393

if inFiles is a dictionary, you could 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 empty string parameter gracefully:

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 );

Upvotes: 0

marr75
marr75

Reputation: 5715

Very readable, I don't know what you hope to gain by removing an if. I would leave as is, but maybe add a final else instead of just ending the method. This might throw an exception or log some information.

Upvotes: 0

Mongus Pong
Mongus Pong

Reputation: 11477

You seem to be looking for the last element in the list that is a replacement file or a standardisation file.

Im 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 list of files is small, perhaps just a premature optimisation issue!

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));
}

Upvotes: 0

FrustratedWithFormsDesigner
FrustratedWithFormsDesigner

Reputation: 27486

Ok here's another 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 this will compile as-is) but I hope it makes the point. ;)

This code looks redundant, but it is easy to write a script/macro that can take a list of all the possible file types and generate (or re-generate when the pattern changes) all the necessary functions and statements to use them. Generated code can save a lot of time if you do it right!

Upvotes: 0

Santosh Gokak
Santosh Gokak

Reputation: 3411

You can use Replace Conditional with Polymorphism refactoring in case you have huge logic.

Upvotes: 1

corsiKa
corsiKa

Reputation: 82579

The code you have doesn't have an excessive amount of if's.

You appear to have three possibilities: Belongs to replacement, belongs to standardization, Belongs to neither of those. What you have is an efficient, readable way to do it.

I'd keep it the way it is.

Upvotes: 9

Related Questions