Is this considered repeating code?

Question

C#/ASP.net:

/// <summary>
/// Is a group in the basket already?
/// </summary>
public static bool isItemInBasket(List<BasketItem> BasketItems, int GroupID)
{
    return BasketItems.Where(c => c.GroupID == GroupID).SingleOrDefault() != null;
}
public static bool isItemInBasket(List<BasketItem> BasketItems, int GroupID, DateTime ArtworkDate)
{
    return BasketItems.Where(c => c.GroupID == GroupID && c.ArtworkDate == ArtworkDate).SingleOrDefault() != null;
}

Some people I spoke to consider this to be repeating code, whereas I don't consider it to be repeating essentially because the repeating code are all things you would have to pass to a method anyway if you wanted to adopt another technique. I don't think it's possible this can be simplified, or broken up into a more simplified version any further, therefore it's not repeated code.

Is this a reasonable thing to say?

Answer 1

Technically, it is repeated code. However, where your colleagues are mistaken is in thinking that repeated code is always bad. In this case, the clarity the repetition adds far outweighs the harmful aspects of repetition, such as creating multiple points of failure. Taking any design heuristic to extremes is unlikely to produce the best design. You have to understand the reasons behind the heuristic.

Now, if you had pages of these functions, each with slightly different combinations of criteria, then the DRY rule becomes the dominant factor. The trick is to recognize the balance point and refactor your code when it grows like that.

On a side note, I wanted to point out that when you repeatedly pass the same object as the first argument to a static function, that's a good sign it would probably fit better as an instance method, i.e. basketItems.isInBasket(groupID).

Answer 2

No, it's not repeated code; it's using polymorphism to allow callers to use two different overloads.

If the code were longer in each method, I would refactor the common parts into their own method, but the linq statements are already sufficiently terse (assuming you use Any() as suggested by others).

Answer 3

If you don't intend to add any additional logic to those methods then yes, you're duplicating code. You're better off with BasketItems.Any(c => /* insert condition here */).

However, if you have lots of places in code where you use it you could do something like this:

public bool IsInBasket(List<BasketItem> basketItems, Expression<Func<BasketItem, bool>> filter)
{
    return basketItems.Any(filter.Compile());
}

Answer 4

I didn't put this in a compiler, but, ideally, when you do overloading, you don't duplicate code from one method to another.

You code the complete method, and each overload calls it, passing nulls where it doesn't have parameters, and you end up with something like:

/// <summary>
/// Is a group in the basket already?
/// </summary>
public static bool isItemInBasket(List<BasketItem> BasketItems, int GroupID)
{
    //return BasketItems.Where(c => c.GroupID == GroupID).SingleOrDefault() != null;
    return isItemInBasket(BasketItems, GroupID, null)
}
public static bool isItemInBasket(List<BasketItem> BasketItems, int GroupID, DateTime? ArtworkDate)
{
    return BasketItems.Where(c => c.GroupID == GroupID && (ArtworkDate.HasValue ? c.ArtworkDate == ArtworkDate.Value : true)).SingleOrDefault() != null;
}

When you end up with more and more overloads, you only need to change the one complete method (albeit more complicated method). And each of your other methods are just wrappers, removing duplication.

Answer 5

Assuming you are using .net 4.0. You end up with just one method with an optional parameter

public static bool IsItemInBasket(List<BasketItem> BasketItems, int GroupID, DateTime? ArtworkDate = null)
{
    var artWorkDate = ArtworkDate ?? DateTime.Now;
    return BasketItems.Any(c => c.GroupID == GroupID && c.ArtworkDate == artWorkDate);
}

Answer 6

It is very borderline. I'm not fond of the unit tests you'd have to write.

I would rather see a method on the BasketItem which reads

public bool isInBasket(int GroupID, DateTime? ArtworkDate)
{
    return this.GroupID == GroupID &&
           this.ArtworkDate == ArtworkDate ?? this.ArtworkDate;
}

And then

/// <summary>
/// Is a group in the basket already?
/// </summary>
public static bool isItemInBasket(List<BasketItem> BasketItems, int GroupID)
{
    return isItemInBasket(BasketItems, GroupID, null);
}

public static bool isItemInBasket(List<BasketItem> BasketItems, int GroupID, DateTime? ArtworkDate)
{
    return BasketItems.Any(c => c.isInBasket(GroupID, ArtworkDate)) != null;
}

I might go further and put an interface on BasketItem so that I can put fake responses on these tests.

All that said, I wouldn't fail you on a code review for your code, I'd just warn you that if it got more complicated then it would be a problem.

Edit: Actually, reading that back, I've just noticed that it demonstrates the point. Because now it doesn't make sense. This forces me to reconsider my naming.

public bool isInGroup(int GroupID, DateTime? ArtworkDate)
{
    return this.GroupID == GroupID &&
           this.ArtworkDate == ArtworkDate ?? this.ArtworkDate;
}

And then

/// <summary>
/// Is a group in the basket already? (do I even need this comment any more?)
/// </summary>
public static bool isGroupInBasket(List<BasketItem> BasketItems, int GroupID)
{
    return isItemInBasket(BasketItems, GroupID, null);
}

public static bool isGroupInBasket(List<BasketItem> BasketItems, int GroupID, DateTime? ArtworkDate)
{
    return BasketItems.Any(c => c.isInGroup(GroupID, ArtworkDate)) != null;
}

Further Edit: Actually, the further we go down this road, the more I think I see the problem. Everyone, myself included, who feels it is duplicated is reacting to the method overload, which instinctively should be one method calling another.

But in reality, the second method is doing something very, very different. And the fact that none of us can understand what that ArtworkDate field is for suggests that maybe the name of the second method should be changed. isGroupInBasketAnd...What?

I think that you see the code as non-duplicated because you know how different it is, but everyone else looks, sees the same name and thinks it's just a standard overload, a bit of syntactic sugar.