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.

I have a list of months in which users are required to update some data. I want to determine at what date they should have updated their data. They must update in March, May and August. If today is March, I want to get back the 1st of March. If today is February, I want to get back last year's August. In reality the list wouldn't be hardcoded.

I do have some working code, but I wonder if there is a simpler / more elegant of writing this (the GetCheckDate method).

using System;
using System.Collections.Generic;
using System.Linq;

internal class Program
{
    private static void Main()
    {
        var listOfMonths = new List<int?> {3, 5, 8};
        Console.WriteLine(GetCheckDate(listOfMonths,
            new DateTime(2014, 2, 1))); // expect 2013-08-01
        Console.WriteLine(GetCheckDate(listOfMonths,
            new DateTime(2014, 3, 1))); // expect 2014-03-01
        Console.WriteLine(GetCheckDate(listOfMonths,
            new DateTime(2014, 7, 1))); // expect 2014-05-01
        Console.WriteLine(GetCheckDate(null,
            new DateTime(2014, 7, 1))); // expect 2013-07-1
        Console.WriteLine(GetCheckDate(new List<int?>(),
            new DateTime(2014, 7, 1))); // expect 2013-07-1
    }

    private static DateTime GetCheckDate(IEnumerable<int?> listOfMonths,
        DateTime curDate)
    {
        DateTime result = curDate.AddYears(-1);
        if (listOfMonths == null) return result;
        listOfMonths = listOfMonths.ToList();
        if (!listOfMonths.Any()) return result;

        listOfMonths = listOfMonths.OrderByDescending(m => m).ToList();
        var greatestSmallerThan =
            listOfMonths.FirstOrDefault(m => m <= curDate.Month)
            ?? listOfMonths.FirstOrDefault(m => m > curDate.Month);
        result = new DateTime(
            greatestSmallerThan.Value <= curDate.Month
                ? curDate.Year : curDate.Year - 1, greatestSmallerThan.Value, 1);

        return result;
    }
}
share|improve this question
    
What if list of months is empty? –  Sergey Berezovskiy Mar 14 at 11:56
    
This is not complete code. If the list is empty, I'll return curDate.AddYears(-1) (edited). What I'd like to know is how to efficiently determine the date. –  comecme Mar 14 at 12:19

2 Answers 2

I suggest you to improve naming:

  • months instead of listOfMonths because its not list, and its better not to include variable type description into name
  • date instead of curDate - its better to avoid abbreviations, also current method should not care whether moth is current or not - it just search previous month from given months
  • Possibly I would rename method to something like GetPreviousMonth

Also I don't see any need to have nullable type for months sequence. It should have integers. If month exist in list, then you can avoid sorting - so do this check first. Thus you are not going to add/remove month, then saving ordered sequence to array is little better:

public static DateTime GetCheckDate(IEnumerable<int> months, DateTime date)
{
    if (month == null || !months.Any() || months.Contains(date.Month))
        return date.AddDays(1 - date.Day);

    var orderedMonths = months.OrderByDescending(m => m).ToArray();
    if (date.Month > orderedMonths.First())
        return new DateTime(date.Year, orderedMonths.First(), 1);

    if (date.Month < orderedMonths.Last())            
        return new DateTime(date.Year - 1, orderedMonths.First(), 1);

    return new DateTime(date.Year, orderedMonths.First(m => m < date.Month), 1);
}

But I also prefer readability on first place (performance should be improved only if its a problem) so I would go with Max and Min month instead of sorting them and keeping in mind what is Last and what is First:

public static DateTime GetCheckDate(IEnumerable<int> months, DateTime date)
{
    if (month == null || !months.Any() || months.Contains(date.Month))          
        return date.AddDays(1 - date.Day);            

    if (months.Max() < date.Month)
        return new DateTime(date.Year, months.Max(), 1);

    if (date.Month < months.Min())            
        return new DateTime(date.Year - 1, months.Max(), 1);

    return new DateTime(date.Year, months.Where(m => m < date.Month).Max(), 1);
}

If I would continue making it more readable, then following extension methods would be useful:

public static DateTime FirstDayOfMonth(this DateTime date, int month = 0)
{
    return new DateTime(date.Year, month == 0 ? date.Month : month, 1);
}

public static DateTime PreviousYear(this DateTime date)
{
    return date.AddYears(-1);
}

Now whole method is very easy to understand:

public static DateTime GetCheckDate(IEnumerable<int> months, DateTime date)
{
    if (month == null || !months.Any() || months.Contains(date.Month))
        return date.FirstDayOfMonth();

    if (months.Max() < date.Month)
        return date.FirstDayOfMonth(months.Max());

    if (date.Month < months.Min())
        return date.PreviousYear().FirstDayOfMonth(months.Max());

    return date.FirstDayOfMonth(months.Where(m => m < date.Month).Max());
}
share|improve this answer
    
Because I originally posted my question on StackOverflow, I simplified the code. In reality, it's not a List<int>, it's a list of an anonymous type containing the month and a number of months of slack time. The list is created from an xml string. If March is the month determined, it's ok if they've updated their data in the indicated number of months before that date. Should I write sample code that reflects the real code more? –  comecme Mar 14 at 12:37
    
Also, if date is 2014-09-01, your (first) method returns 2014-03-01 where I'd expect 2014-08-01. The second method doesn't have this problem. –  comecme Mar 14 at 12:56
    
@comecme hold on, I'll check. Also do you have some problems to apply this code to anonymous types? –  Sergey Berezovskiy Mar 14 at 12:59
    
I've updated my question to be more like my real code. I'm using a class MonthSlack here. In my code it's an anonymous type, but I don't think that would make a difference. I can't use months.Max() now. Could use months.Max(m => m.Month), but I'd still need the actual MonthSlack where Month has that max value. If you want to rewrite my new code, I guess you'd better do that in a new answer. –  comecme Mar 14 at 19:50

You could use DateTime.AddMonths to simplify the code:

for (int i = 0; i < 12; i++) {
    var checkDate = curDate.AddMonths(-i))
    if (listOfMonths.Contains(checkDate.Month)) {
        return new DateTime(checkDate.Year, checkDate.Month, 1);
    }
}
share|improve this answer

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.