current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

Just after a bit of feed back or peoples ideas on which practice is best.

I currently have a method but am unsure of the best way to approach it from a readability point of view or even from best practices point of view. Or perhaps it doesn't really matter at all and they are all fine?

My three options I explored were:

1)

function double GetObjectArea(CustomObject obj, CustomEnum source)
{
   return obj.ListOfApplications.Sum(p => p.Source == source ? object.Area : 0);
}

2)

function double GetObjectArea(CustomObject obj, CustomEnum source)
{
   var query = from record in obj.ListOfApplications
               where record.Source == source
               select object.Area

   return query.Sum();
}

3)

function double GetObjectArea(CustomObject obj, CustomEnum source)
{
   return obj.ListOfApplications.Where(p.Source == source).Sum(object.Area);
}
share|improve this question
1  
For compactness, I would use the third method, but for clarity, I would probably use the second one. Plus, I personally like queries over the function call chains for readability. –  Nick Bedford Feb 27 '12 at 0:42
    
I agree Nick, I prefer queries but think maybe in this case when it's a simple chain it, I'm leaning towards option 3 being the better option. –  dreza Feb 27 '12 at 7:57
    
I'm going to buck the trend and like the first one over the others. –  Jesse C. Slicer Feb 27 '12 at 14:55
    
@JesseC.Slicer ha really Jesse. That was my very fist way I did it but then started thinking that there may be better ways hence this question –  dreza Feb 27 '12 at 19:30

2 Answers 2

up vote 4 down vote accepted

I would go with the third, with the second as secondary choice.

The first mixes addition logic and comparison logic: it's not immediately clear that the zero is meant to exclude those objects, as opposed to giving a default for object.Area.

The second is better, but it hides the point behind a lot of unnecessary syntax. The fact that you're working on object.Area and not record.Area gets lost.

The third is even better. It can be conveniently read left-to-right, and all the information is grouped near what we're doing (condition is after the where, sum elements are after the sum).

Personally, I would use

double GetObjectArea(CustomObject obj, CustomEnum source)
{
   return obj.ListOfApplications.Count(p => p.Source == source) * object.Area;
}
share|improve this answer
    
Thanks Anton. I like your option you provided in seperating the Area out. In general I like option 2 myself where I can but agree option 3 probably makes better sense in this case. –  dreza Feb 27 '12 at 7:56
up vote 2 down vote

I like the third one since it's really clear what it's doing. Here's a question that I'd like to ask: Do you really need to encapsulate this logic in a method ? do you really need to use it in different places of your code ? if the answer is yes then you are doing DRY and if it's no it's a sign of YAGNI. You can filter your query whenever you need it without using (and adding) a function. If you still need to introduce a new function I suggest you use a more meaningful definition for it something like ( I use an extension method to make it more readable but it might be a function of CustomObject itself)

double GetSumOfAreaWhereSourceIs(this CustomObject obj,CustomEnum source);

then you will have :

var sum=myObj.GetSumOfAreaWhereSourceIs(mySource);

which is more readable than

var sum=GetObjectArea(myObj,mySource);

in my opinion.

share|improve this answer
    
I was making the same call in about 6 lines in a row to set some properties and the readability was getting a bit lost so having the method made it a bit easier to read. An extension method might be overkill in this particular situation but I agree that they do make for easier reading as well. –  dreza Feb 27 '12 at 19:28

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.