Problem
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.
Solution
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 nextif
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 aboveelse
block? That also allows you to remove themonths.Count != 3
check. - Putting
first = _grid[21]...
on the same line as thatif
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 anelse
statement uses braces I’d expect the relatedif
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
and41
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 ofFirst
andLast
without sacrificing readability, if you really need that little bit of performance gain.- Instead of using the local variables
first
,middle
andlast
, 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
toHeader
orTitle
– 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}";
}
}