1
\$\begingroup\$

I have a 6 x 7 grid for a calendar = 42 DateTimes. I'm trying to display the displayed month/year at the top of the calendar.

If there are 3 months displayed, I only want to show the name for the middle month (October 2018, for instance). If 2 months are displayed, split the name (October - November 2018). If 2 months are displayed and have different years, append with year on both sides (December 2018 - January 2019).

This is what I have, and it works, but it's in a property that may be "getted" often and I want this to be as efficient as possible. Here's the code:

public string ViewedMonthYearStr
{
    get
    {
        if (_grid != null)
        {
            string first;
            string middle = string.Empty;
            string last = string.Empty;
            HashSet<int> months = new HashSet<int> { _grid[0].Month, _grid[21].Month, _grid[41].Month };
            if (months.Count == 3) first = _grid[21].ToString("MMMM yyyy");
            else
            {
                first = _grid[0].ToString("MMMM");
                middle = " - ";
                last = _grid[41].ToString("MMMM yyyy");
            }
            if (_grid[0].Year != _grid[41].Year && months.Count != 3)
            {
                middle = $" {_grid[0].ToString("yyyy")} - ";
            }
            _viewedMonthYearStr = $"{first}{middle}{last}";
        }
        return _viewedMonthYearStr;
    }
}

Is there a more efficient way to do this? My gut tells me that if/else's aren't the most efficient way.

\$\endgroup\$
  • 1
    \$\begingroup\$ I find the title is misleading because this question isn't about displaying anything in a calender but rather about a function that calculates something for the calender. You should update the title so that it properly describes the real porpose of the code. Besides I think it lacks context because it's reading some magic values from the grid like (0, 21, 41) etc. You should improve it by adjusting the title accordingly and adding more information. \$\endgroup\$ – t3chb0t Dec 10 '18 at 7:36
  • 1
    \$\begingroup\$ @t3chb0t: I think there is sufficient context here: the wpf tag tells me that this property is likely bound to by a calendar view, and those magic values are used to get the first, middle and last date that are displayed. \$\endgroup\$ – Pieter Witvoet Dec 10 '18 at 9:35
  • 1
    \$\begingroup\$ @t3chb0t -- someone edited the title, I originally named it "A more efficient way of parsing displayed month/year for calendar". The "grid" is simply a list of datetimes, as I said, with 42 values (6*7) so 0 is the first date, 21 is the middle, 41 is the last -- I grab these because if there are 3 months to be shown, each of those will have a different month always. \$\endgroup\$ – linus72982 Dec 10 '18 at 17:54
  • \$\begingroup\$ Oh, that was an unfortunate change. I restored it. This one is much better indeed. I didn't see the other edit. \$\endgroup\$ – t3chb0t Dec 10 '18 at 17:59
3
\$\begingroup\$

I wouldn't worry about this being inefficient - you're not doing anything crazy here, and a calendar header probably won't need to be refreshed millions of times per second anyway.

Still, I can think of some changes that will improve readability and maintainability:

  • I would use a guard clause to reduce nesting, and to make it more obvious what happens when grid is null: if (_grid == null) return null;. This also gets rid of edge-cases early on, while in the current code you have to scan down to the end to see if maybe there's still an else statement following.
  • Adding an empty line between the end of the else block and the next if statement will make it more obvious that they're not related.
  • However, the last if statement only applies when displaying two months, so why not put it inside the above else block? That also allows you to remove the months.Count != 3 check.
  • Putting first = _grid[21]... on the same line as that if statement, and without braces, makes the control flow more difficult to see at a glance. Omitting braces can also result in subtle bugs, so some people make a point of always using braces. Either way, consistency matters: if an else statement uses braces I'd expect the related if statement to also use them.
  • Why assign the result to _viewMonthYearStr instead of returning it directly? That field does not seem to serve any purpose.
  • 0, 21 and 41 are 'magic values': the meaning of those values isn't directly clear, and they easily break the code when the size of _grid needs to be changed. Replacing _grid[0] with _grid.First(), _grid[41] with _grid.Last() and _grid[21] with _grid[_grid.Length / 2] will make the code more robust and easier to understand. You could also assign their results to local variables, such as: var middleDate = _grid[_grid.Length / 2];. That simplifies the code somewhat, and it allows you to use _grid[0] and _grid[_grid.Length - 1] instead of First and Last without sacrificing readability, if you really need that little bit of performance gain.
  • Instead of using the local variables first, middle and last, you can return results immediately. That will make the possible output formats easier to see.
  • Instead of using a hash set, you can also check if the Month of the middle date is different from that of both the first and the last date. That will put a tiny bit less pressure on the GC.
  • Instead of calling date.ToString(format), and then using the result in an interpolated string, you can also specify the format in the interpolated string directly: $"{date:format}".
  • I'd rename ViewedMonthYearStr to Header or Title - that's what it's used for, after all.

With all that, this is what I would end up with:

public string Header
{
    get
    {
        if (_grid == null)
            return null;

        var firstDate = _grid.First();
        var middleDate = _grid[_grid.Length / 2];
        var lastDate = _grid.Last();

        // If more than 2 months are displayed, focus only on the middle month:
        if (middleDate.Month != firstDate.Month && middleDate.Month != lastDate.Month)
            return $"{middleDate:MMMM yyyy}";

        if (firstDate.Year != lastDate.Year)
            return $"{firstDate:MMMM yyyy} - {lastDate:MMMM yyyy}";

        return $"{firstDate:MMMM} - {lastDate:MMMM yyyy}";
    }
}
\$\endgroup\$
  • \$\begingroup\$ That is much more readable, thank you. I had the extra if at the end so that if two months were shown of the same year then the year would only display on the last, but that's a minor thing that I think gets trumped by code elegance. I was trying to copy how Outlook does it in their calendar :) Oh, and about the private backer, it was there because I was always taught a block should only ever have 1 return statement -- slavish devotion to something that isn't important, I know, it's just automatic by now. \$\endgroup\$ – linus72982 Dec 10 '18 at 18:04
  • 1
    \$\begingroup\$ The same-year case is also taken care of - see the final return statement. :) \$\endgroup\$ – Pieter Witvoet Dec 10 '18 at 18:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.