Removing code duplication and redundancy based on different criteria and priority level-CodePudding

I was wondering if I could get your suggestions on how to remove the code duplication/repetition here.

Any preferred and correct way of doing so?

Summary: This function returns a car's name and accepts the car code as a parameter

public string GetMatchingCar(string carCode)
{
    //CarResults is the list of cars that consists the details of the car as a collection.
    var carResults = new List<Car>
    {
        new Car{Name="AB car", Code="AB", Colors = new List<string>{"blue","green","black"}, Features=new List<string>{"compact","fast","light"} },
        new Car{Name="AC car", Code="AC", Colors = new List<string>{"gray","white","yellow"}, Features=new List<string>{"extended","fast","heavy"} },
        new Car{Name="DE car", Code="DE", Colors = new List<string>{"red","green","purple"}, Features=new List<string>{"sports","light"} },
        //and so on
    };

    //Specifications is a list of specification to choose from that includes color and feature.
    var specifications = new List<Specification>
    {
        new Specification{Color="blue", Feature="heavy"},
        new Specification{Color="red", Feature="light"},
        new Specification{Color="maroon", Feature="compact"},
        new Specification{Color="black", Feature="manual"},
        new Specification{Color="neon", Feature="heavy"},
        //and so on
    }

    //Now, we need to return the car based on the priority of criteria. Say P1, P2 and P3. P1's priority being highest.
    //P1: Code   Color   Feature
    //P2: Code   Feature
    //P3: Feature
    //just for example, priorities can be from P1 - P7

    //Now we have to combine the specification with carCode to check the priorities and return the matching car.

    //TODO: Here, is where I think I am doing wrong by having each for loop for each priority (as priorities can be upto Seven). All the foreach loop won't be executed if a priority is matched and returns a car.
    //TODO: Wondering how could I improve this?

    //priority P1: Code   Color   Feature 
    foreach(var specification in specifications){
        var matchingCarP1 = carResults.FirstOrDefault(x=> x.Code.Equals(carCode) && x.Colors.Contains(specification.Color) && x.Features.Contains(specification.Feature)); 
        if(matchingCarP1 != null) return matchingCarP1.Name;
    }

    //priority P2: Code   Feature
    foreach(var specification in specifications){
        var matchingCarP2 = carResults.FirstOrDefault(x=> x.Code.Equals(carCode) && x.Features.Contains(specification.Feature)); 
        if(matchingCarP2 != null) return matchingCarP2.Name;
    }

    //priority P3: Feature 
    foreach(var specification in specifications){
        var matchingCarP3 = carResults.FirstOrDefault(x.Features.Contains(specification.Feature)); 
        if(matchingCarP3 != null) return matchingCarP3.Name;
    }
    
    //Other priorities

    return string.Empty;
}

Any suggestions or feedback on this would be really helpful and highly appreciated! Thank you!

CodePudding user response：

In my view, this method has too many responsibilies and it is too long. Let's try to apply Single Responsibility Principle of SOLID.

So let's create a method with one goal. The goal or responsibility will be just to get cars:

private List<Car> GetCars() => 
    new List<Car>
    {
        new Car{Name="AB car", Code="AB", Colors = 
            new List<string>{"blue","green","black"}, 
            Features=new List<string>{"compact","fast","light"} },
        new Car{Name="AC car", Code="AC", Colors = 
            new List<string>{"gray","white","yellow"}, 
            Features=new List<string>{"extended","fast","heavy"} },
        new Car{Name="DE car", Code="DE", Colors = 
            new List<string>{"red","green","purple"}, 
            Features=new List<string>{"sports","light"} },
        //and so on
    };

and

private List<Car> GetSpecifications() =>
    new List<Specification>
    {
        new Specification{Color="blue", Feature="heavy"},
        new Specification{Color="red", Feature="light"},
        new Specification{Color="maroon", Feature="compact"},
        new Specification{Color="black", Feature="manual"},
        new Specification{Color="neon", Feature="heavy"},
        //and so on
    }

And it looks like that we can avoid unnecessary iteration of the whole specifications by adding nested if statements:

private string FindMatchingCar(List<Car> cars, 
    List<Specification> specifications) 
{
    foreach(var specification in specifications){
        var matchingCarP1 = carResults.FirstOrDefault(x=> x.Code.Equals(carCode) 
            && x.Colors.Contains(specification.Color) 
            && x.Features.Contains(specification.Feature)); 
        if(matchingCarP1 != null) return matchingCarP1.Name;
        
        var matchingCarP2 = carResults.FirstOrDefault(x=> x.Code.Equals(carCode) 
            && x.Features.Contains(specification.Feature)); 
        if(matchingCarP2 != null) return matchingCarP2.Name;
        
        var matchingCarP3 = carResults.FirstOrDefault(x =>  
            x.Features.Contains(specification.Feature)); 
        if(matchingCarP3 != null) return matchingCarP3.Name;
    }
        
    return string.Empty;
}

And then your method will look like this:

public string GetMatchingCar(string carCode) 
{
    var cars = GetCars();    
    var specifications = GetSpecifications();    
    return FindMatchingCar(cars, specifications);
}

CodePudding user response：

Based on the criteria, and each of them being different plus the specifications being a list, in my opinion what you have is okay.

p.s. pls treat as a comment