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.

Can anyone please tell which one is the more effective way to write C# code, among the two options below?

Option A:

internal static string GetLast3Years()
{
    return (DateTime.Now.Year - 2).ToString() + "," + (DateTime.Now.Year - 1).ToString() + "," + DateTime.Now.Year.ToString();
}

Or option B:

internal static string GetLast3Years()
{
    int currentYear = DateTime.Now.Year;
    return (currentYear - 2).ToString() + "," + (currentYear - 1).ToString() + "," + currentYear.ToString();
}
share|improve this question
    
Neither is particularly good... –  Servy Feb 7 '13 at 17:07
    
Option B, because it fetches the value only once. Ugly code though. –  X.L.Ant Feb 7 '13 at 17:08
2  
You should not usually worry about such trivial optimizations. A lot of times a release build and the CLR itself will take care of such things. Alternatively you could test which one is better in your context yourself with some diagnoses. –  gideon Feb 7 '13 at 17:08
1  
I'd use: return string.Join(",", Enumerable.Range(DateTime.Now.Year-2, 3)); –  Servy Feb 7 '13 at 17:13
2  
@Servy Nice, though that's a lot slower than the other two methods provided. –  DGibbs Feb 7 '13 at 17:25
show 7 more comments

migrated from stackoverflow.com Feb 8 '13 at 14:05

This question came from our site for professional and enthusiast programmers.

2 Answers

Note that semantically these options are not equal.

There is a chance (very-very small) that first option will output smth like 2011,2012,2014, so second option is better at least because it will always generate consistent results (3 consecutive years).

But second option is also quite cluttered. There is a string.Join method that is able to combine different values with separator, so your code may look like:

internal static string GetLast3Years()
{
    int currentYear = DateTime.Now.Year;
    return string.Join(",", currentYear - 2, currentYear - 1, currentYear);
}

But the question is still why would you need a string representation of last 3 years, comma-separated. I would consider having a method that will return enumerable of last N years instead...

share|improve this answer
add comment

I will do it the following way

  internal static string GetLast3Years()
        {
            int currentYear = DateTime.Now.Year;       

            return String.Format("{0},{1},{2}", currentYear - 2, currentYear - 1, currentYear);
        }
share|improve this answer
1  
technically not an answer to the question. –  Servy Feb 7 '13 at 17:12
    
@Servy He wants the more effective way of writing between the two options he wrote. and I modify it and give him a better solution. What is the problem –  Dan Hunex Feb 7 '13 at 17:14
1  
Can you explain why this is better than the code in the OP rather than just say you would do it a certain way, and not provide any reasoning? –  DGibbs Feb 7 '13 at 17:14
    
@DanHunex No, that's not what he asked at all. He asked which of the two options provided is better, he didn't ask for a third option that's better than both. –  Servy Feb 7 '13 at 17:15
    
@Servy, you give him third option in the comment above . Btw, Yes I choose the second method he wrote but improved. U still didnt get –  Dan Hunex Feb 7 '13 at 17:17
show 1 more comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.