Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

As the title states. I need a different type of icon for the different nodes types. at the moment I have this hacky way of achieving that.

for (var x = 0; x <= nodeList.Count - 1; x++)
{
     if (xNode.Attributes["TYPE"].Value == "server.point.AV")
     {
         var tNode2 = new TreeNode(xNode.Attributes["NAME"].Value, AnalogImagesIndex, AnalogImagesIndex);
         inTreeNode.Nodes.Add(tNode2);
         tNode2 = inTreeNode.Nodes[x];
         AddNode(xNode, tNode2);
     }
     else if (xNode.Attributes["TYPE"].Value == "schedule.NSPDigitalSchedule")
     {
         var tNode2 = new TreeNode(xNode.Attributes["NAME"].Value, ScheduleImageIndex, ScheduleImageIndex);
         inTreeNode.Nodes.Add(tNode2);
         tNode2 = inTreeNode.Nodes[x];
         AddNode(xNode, tNode2);
     }
     else if (xNode.Attributes["TYPE"].Value == "server.point.SV")
     {
         var tNode2 = new TreeNode(xNode.Attributes["NAME"].Value, StringImageIndex, StringImageIndex);
         inTreeNode.Nodes.Add(tNode2);
         tNode2 = inTreeNode.Nodes[x];
         AddNode(xNode, tNode2);
     }
     else if (xNode.Attributes["TYPE"].Value.StartsWith("trend"))
     {
         var tNode2 = new TreeNode(xNode.Attributes["NAME"].Value, TrendImageIndex, TrendImageIndex);
         inTreeNode.Nodes.Add(tNode2);
         tNode2 = inTreeNode.Nodes[x];
             AddNode(xNode, tNode2);
         }
    .
    .
    .and so on

I do not want to add the long list of else if's.

share|improve this question
    
Do you have 1 image type for each type or you have several groups and each group has 1 image? – Ziv Weissman yesterday
    
Just one image list – QANew 22 hours ago

It doesn't look like you would need the for loop. You can do the same much simpler with a foreach and a little bit of linq:

// assuming they are ints
var AnalogImagesIndex = 0;
var TrendImageIndex = 2;

// simplifies the ifs
var imageIndicies = new Func<string, int?>[]
{
    attr => attr == "server.point.AV" ? AnalogImagesIndex : (int?)null,
    attr => attr.StartsWith("trend") ? TrendImageIndex : (int?)null,
    // ...
};

var imageIndex = imageIndicies
    .Select(x => x(xNode.Attributes["NAME"].Value))
    .First(x => x.HasValue)
    .Value;

To do the rest you can use the helper from BCdotWEB's answer.

Another assumption is that there is no else at the end, otherwise you need to use FirstOrDefault or provide an else lambda.


or with even less repetition the extreme solution:

var getImageIndexOrDefault = new Func<bool, int, int?>(
    (condition, value) => condition ? value : (int?)null);

var imageIndicies = new Func<string, int?>[]
{
    attr => getImageIndexOrDefault(attr == "server.point.AV", AnalogImagesIndex),
    attr => getImageIndexOrDefault(attr.StartsWith("trend"), TrendImageIndex),
};
share|improve this answer
    
I have multiple else at the end – QANew 22 hours ago
    
Wondering if i could skip the node and continue when it comes across a node not satisfying any of the conditions – QANew 21 hours ago
    
I want to be like you when I grow up ^.^ – Tolani Jaiye-Tikolo 13 hours ago

The moment you start copy-pasting code and only changing one small thing, it's time to move that code to a method.

private void CreateAndAddNode(WhateverTypeImagesIndexIs imagesIndex, int x)
{
    var tNode2 = new TreeNode(xNode.Attributes["NAME"].Value, imagesIndex, imagesIndex);
    inTreeNode.Nodes.Add(tNode2);
    tNode2 = inTreeNode.Nodes[x];
    AddNode(xNode, tNode2);
}

I'm assuming AnalogImagesIndex etc. all inherit from some base type; that's the one to use as the method's parameter.


Moreover, all of those ifs can be changed to a switch.

share|improve this answer
    
all of those ifs can be changed to a switch. - nope, they cannot, see: .Value.StartsWith("trend") – t3chb0t yesterday
    
Not until C# 7 where you can have conditions in case – t3chb0t yesterday
    
@t3chb0t Oops, didn't notice the StartsWith. – BCdotWEB yesterday
    
I have changed few into swithch switch (xNode.Attributes["TYPE"].Value) { case "server.point.BV": { Then back to default: if where I need to check the starting string – QANew 22 hours ago
    
I want to be like you when I grow up ^.^ – Tolani Jaiye-Tikolo 13 hours ago

On top of @BCdotWEB & @t3chb0t answers you can use the .Any() condition.

For example:

List<string> group1 = new List<string> {"schedule.NSPDigitalSchedule, schedule.Whatever, WhenEver.WhatEver"};

List<string> group2 = ...

 //in foreach loop:
string nodeValue = xNode.Attributes["TYPE"].Value;
if (group1.Any(m=>m==nodeValue)) 
{
   //Call the method for adding image for group1
   continue; //This will go to the next iteration so you don't need nested if's
}

if (group2.Any(m=>nodeValue.StartWith(m)) 
{
    //Call the method for adding image for group2
   continue; 
}

//and so on.
share|improve this answer
    
There is one problem with .Any - the OP needs a different image index for each attribute value and not just a check if it has any of those values. – t3chb0t 13 hours ago
    
Hmm didn't realize that... return the index of the array maybe? – Ziv Weissman 12 hours ago
    
Perhaps, but the OP didn't provide enough information to be able to suggest anything about it. We don't know how the image index works or even what data type it is. – t3chb0t 12 hours ago
    
@QANew Please provide more information about the image index, if possible, you can arrange the List in the same index you need it, and use the IndexOf instead. – Ziv Weissman 12 hours ago

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.