Filtering cards into appropriate list elements

Question

Background:

I have two lists: one with a list of workcentres and the other with a list of the stations in the centres.

This code matches the stations to the centres by comparing the stations' workcentre number property with the name property of the centres.

    private void WsSorter(WorkcentreList wcl, List<WorkstationCard> wscl)
    {
        // Find index of workcentres
        foreach (Workcentre wc in wcl.WcList)
        {
            foreach (WorkstationCard wsc in wscl)
            {
                if (!(wsc.WorkcentreNo == MAIN_WORKCENTRE_1 ||
                 wsc.WorkcentreNo == MAIN_WORKCENTRE_2 ||
                 wsc.WorkcentreNo == MAIN_WORKCENTRE_3 ))
                {
                    wc.CardList.Add(wsc); // Must be adding to "Other WC"
                }
                // Add to matching workcentre
                else if (wsc.WorkcentreNo == wc.WcName) 
                {
                    wc.CardList.Add(wsc);
                }
            }
        }
    }

Question

The lint tool I am using, SonarLint is telling me off for violating the DRY principle in the else if condition.

It works, albeit with poor error checking but I would like to know, is there a better way I could write the code / approach the situation, especially regarding not repeating the line wc.CardList.Add(wsc);?

Additional context: There are 3 main workcentres. All other workcentres (of which there is an arbitrary amount) should be classified under 'other'. I set the code up such that if the stations don't belong to a main workcentre, it must belong in the 'other' workcentre.

Answer 1

• By simply adding the else condition to the if, the DRY warning will go away. It's just telling you that both the if and else block currently execute the same code, and can therefore be consolidated into a single block.
• The comment // Find index of workcentres is misleading and should be removed.
• Don't use abbreviations for naming. It makes it very hard to read and understand the code.
• By extracting the condition to a separate method, you can use the Where() LINQ method and call AddRange() instead of repeatedly calling Add() .
• Like @Paparazzi mentioned in a comment, you are filtering, rather than sorting, so you should change the name of the method.

Implementing the changes (except for the naming), you code will look like the following:

private void WsSorter(WorkcentreList wcl, List<WorkstationCard> wscl)
{
    foreach (Workcentre wc in wcl.WcList)
    {
        wc.CardList.AddRange(wscl.Where(workstationCard => IsDesiredWorkstationCard(workstationCard, wc.WcName)));
    }
}
private bool IsDesiredWorkstationCard(WorkstationCard workstationCard, string workCentreName)
{
    return !(workstationCard.WorkcentreNo == MAIN_WORKCENTRE_1 ||
             workstationCard.WorkcentreNo == MAIN_WORKCENTRE_2 ||
             workstationCard.WorkcentreNo == MAIN_WORKCENTRE_3 ) ||
             workstationCard.WorkcentreNo == workCentreName
}

Answer 2

Naming is important.

• WsSorter is far too vague.
• Do not needlessly abbreviate: wcl, wscl. Ditto for the "No" in WorkcentreNo. Ditto for the "Wc" in WcList and WcName.
• The various parts of compound words like Workcentre and Workstation should be properly capitalized, thus WorkCentre and WorkStation.
• Maybe it's me, but "centre"? Admittedly, it is correct in some cases, but here it feels out of place.
• I don't like the name WorkcentreList; moreover, why can't it simple be a List<WorkCenter>? Same for WcList. Same for CardList. Call them WorkCenters and Cards.
• Constants should be PascalCase: MAIN_WORKCENTRE_1.
• I'm a bit confused by the term WorkcentreNo (or WorkCenterNumber as it perhaps should be). Is this a number, or actually a name?
• Why is WcName prefixed with "Wc", considering it is a property of the WorkCenter class?

---

Quite frankly I find this to be very convoluted and hard to maintain:

if (!(wsc.WorkcentreNo == MAIN_WORKCENTRE_1 ||
wsc.WorkcentreNo == MAIN_WORKCENTRE_2 ||
wsc.WorkcentreNo == MAIN_WORKCENTRE_3 ))

What if you need to add another, e.g. MAIN_WORKCENTRE_4?

As Heslacher's answer shows: move this to a separate method, and consider moving MAIN_WORKCENTRE_1 etc. to a collection of "valid" WorkCenterNumbers, thus reducing the code to:

private bool IsDesiredWorkstationCard(WorkStationCard workStationCard, string workCenterNumber)
{
    return !(validWorkCenterNumbers.Contains(workStationCard.WorkCenterNumber))
        || workStationCard.WorkCenterNumber == workCenterNumber;
}

This could/should perhaps even move to a static class.