Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?

share|improve this question
11  
You know you could just write BasketItems.Any(c => c.GroupID == GroupID), with the same result, right? –  nikie Aug 24 '11 at 16:24
1  
Nope didn't know that, thanks though! –  Tom Aug 24 '11 at 16:27
    
nikie's right, I think switching to .Any() with no other changes actually gives you the cleanest final solution. –  Toby Aug 24 '11 at 21:59
add comment

6 Answers

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

share|improve this answer
1  
+1 to @Karl Bielefeldt -- I'd add to this that static methods like this are often "bad smells" that signal a need for refactoring. In this code, I immediately question whether the seeker has identified the appropriate classes/objects to take responsibility for the actions involved. Were I reviewing this code, I would suggest some domain analysis to clarify what belongs where and which object should do what. –  John Tobler Aug 24 '11 at 21:59
add comment

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

share|improve this answer
1  
I disagree, it is repeated, the code in each method is nearly identical. The first method, should call the second, passing null for the last parameter. (btw, I'm not the downvote) –  CaffGeek Aug 24 '11 at 17:10
2  
@Chad: Every alternate solution proposed here is more complicated than the original code. How do you make things better by adding complexity? –  Robert Harvey Aug 24 '11 at 17:17
3  
yes and no. It adds some complexity to one method. However that is the only method that requires changing. For only two methods this isn't a big deal, but if you have many, with different parameters, you're going to be duplicating a lot of code between each of them. When instead you should be trying to share the code between them. –  CaffGeek Aug 24 '11 at 17:38
    
@Robert - The question isn't about how complex the code is, whether this the best pattern, or how to improve the code. The question is 'is this repeated code?'. It very clearly is repeated code, for better or worse. –  Kirk Broadhurst Aug 25 '11 at 2:06
    
@Kirk: I interpreted the question as more than just a frivolous one. That some code is duplicated is obvious. –  Robert Harvey Aug 25 '11 at 2:07
add comment

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.

share|improve this answer
add comment

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());
}
share|improve this answer
add comment

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);
}
share|improve this answer
    
Could the "var artWorkDate = ..." line be deleted and the null in the parameter list be replaced with DateTime.Now? –  compman Aug 24 '11 at 21:04
    
I don't think so because default value has to be a constant. You cannot use a property of a class that is been computed when called. –  rpgmaker Aug 25 '11 at 0:20
    
+1 I don't see any reason to write two methods when one really simple method will do. But I'd change it to be a one-liner, ending with - && c.ArtworkDate == artWorkDate ?? c.ArtworkDate); –  Kirk Broadhurst Aug 25 '11 at 2:05
    
The reason it is not a one liner is because there is no point of evaluating ArtworkDate whether is null then replace it with the DateTime.Now when it is. It is more optimized to store the value that will be compared against c.ArtworkDate. But then again, I could wrong since it is possible that the compiler optimize it by default. –  rpgmaker Aug 25 '11 at 2:28
add comment

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.

share|improve this answer
add comment

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.