18
\$\begingroup\$

To find the days off for our custom accounting software there is this method used to find when is Easter in any given year.

private static DateTime Easter(int year) {
    int a = year%19;
    int b = year/100;
    int c = (b - (b/4) - ((8*b + 13)/25) + (19*a) + 15)%30;
    int d = c - (c/28)*(1 - (c/28)*(29/(c + 1))*((21 - a)/11));
    int e = d - ((year + (year/4) + d + 2 - b + (b/4))%7);
    int month = 3 + ((e + 40)/44);
    int day = e + 28 - (31*(month/4));
    return new DateTime(year, month , day);
}
\$\endgroup\$
  • 36
    \$\begingroup\$ Man, that's a lot of magic numbers. I don't even know where to start reviewing other than that. \$\endgroup\$ – 23fc9a62-56de-47fb-97b4-737890 May 7 '18 at 15:08
  • 2
    \$\begingroup\$ Some comments and more descriptive variable names would help, too. \$\endgroup\$ – Null May 7 '18 at 15:17
  • 6
    \$\begingroup\$ more information about what formula you are using would also be helpful in giving a good review of your code. \$\endgroup\$ – Malachi May 7 '18 at 15:25
  • 3
    \$\begingroup\$ I know "This is how other people do it" is not the done thing here, but it seems worth referencing this SO thread. stackoverflow.com/questions/2510383/… I especially like the comment by Gordon Broom: "One method that has been used historically is table lookup -- don't discount this method" \$\endgroup\$ – Josiah May 7 '18 at 17:47
  • 3
    \$\begingroup\$ @Dain Ironfoot III: Well, isn't the date of Easter DEFINED by a lot of magic numbers? And worse, different sects apparently have different ways of computing it, so there may be different days that are "Easter" depending on which one you respect: timeanddate.com/calendar/determining-easter-date.html \$\endgroup\$ – jamesqf May 8 '18 at 3:57
46
\$\begingroup\$

The usual advice is "make meaningful variables". However, we know this algorithm (essentially a modification of that published by a young Gauß) and those variables don't have a simple meaning. (See Decoding Gauss Easter algorithm). We know b represents centuries, but even if you did find a word to describe what the variables represent, would using epact or even ageOfMoonOnJanFirst help to understand the algorithm? The variable represent stages of the algorithm. As Gauss wrote "It is not possible to present the full analysis that led to the algorithm".

So the usual cure of incomprehensible code - clear variables names - won't help much here. What can we do for the poor person who has to maintain this code 6 months, or 6 years, later? We can let them know where this algorithm comes from. This is easy enough. A comment referencing the source would allow someone seeing this to check that it is still doing what it is supposed to do.

Beyond adding this comment, the algorithm for calculating Easter is unlikely to be a bottleneck in any piece of software, so assuming that this algorithm is correct (proven and tested) I would leave it as is.

\$\endgroup\$
  • 4
    \$\begingroup\$ "A comment referencing the source..." - do not forget add algorithm name (Gauss Easter algorithm), sources tend to vanish. \$\endgroup\$ – Arvo May 9 '18 at 8:15
  • \$\begingroup\$ I think that adding proper names helps but if variables match an available public paper/document (which you correctly suggest to link/include) then it does not really matter. HOWEVER, to split that long cryptic formula in multiple small functions (which may - or not - be reusable elsewhere) may surely help to at least vaguely understand the algorithm. It's our duty as programmers to make code as easy to understand as possible (especially when we spent maybe one hour to study an algorithm...we/they don't want to do it again and again each time we look at it) \$\endgroup\$ – Adriano Repetti May 9 '18 at 13:43
15
\$\begingroup\$

When I finish writing code, I usually ask myself : "Okay, do I think I'd be able to understand my code in six months?"

If the answer is no, it means I'm not done. My code needs to be clearer.

I 100% think I wouldn't understand your code in six months as I don't get it now.

Naming

Crudely put, the variable names are terrible. What does a,b,c,d,e mean? Why is 19, 11, 21, etc there?

As I said above, I don't understand your code so I can't propose better names, but you should name your variables accordingly to what they do and use variables for all those "magic numbers". "Six months in the future" yourself might thank you if there's a bug in the code.

I'd also rename the method to GetEasterDate.

\$\endgroup\$
  • 3
    \$\begingroup\$ What would be the value of naming the method “GetEasterDate” ? – It is private static, so it is well documented enough to the class implementor is just a computation (not a state change). \$\endgroup\$ – Michael Le Barbier Grünewald May 7 '18 at 19:32
  • 4
    \$\begingroup\$ @MichaelLeBarbierGrünewald I'm not sure I understand your point. Because it is private static, it is well documented enough? \$\endgroup\$ – IEatBagels May 7 '18 at 20:05
  • \$\begingroup\$ I wouldn't be able to understand this one day after. \$\endgroup\$ – Captain Man May 9 '18 at 12:49
10
\$\begingroup\$

The code itself is fine. Computing the Easter date is an astronomical computation and it is not possible to make it easier to understand without actually adding a full demonstration about why it yields the correct result.

Since this is an accounting software I would consider the given level of (no) details fine. I would however increase the capacity of the maintenance programmer to ensure that the code is correct by ensure that:

  1. The method used has a traceable origin. AFAICT the computation implements the algorithm described by Claus Tøndering in his classical Calendar FAQ. (See Isn’t there a simpler way to calculate Easter?) The source should be referenced in the code, e.g. “Compute Easter day à la https://www.tondering.dk/claus/cal/easter.php”. If you are only interested by the modern era (post 1920) it is not worth mentioning the algorithm works only for the Gregorian calendar.

  2. The variables used in your implementation match as closely as possible those used in the source, to ease verification. (Which is not the case here, but you might have a better source for this purpose than I had.)

  3. Besides the snippet above, there is unit tests for this computation.

\$\endgroup\$
  • \$\begingroup\$ The algorithm is a bit older than that. This page sources it to J.M. Oudin in 1940. \$\endgroup\$ – eirikdaude May 8 '18 at 8:26
  • \$\begingroup\$ Oudin is also quoted on the page from Claus Tøndering: I cited the page as a possible reference for documenting the computation implemented, but there are probably a lot of other plausible choices. \$\endgroup\$ – Michael Le Barbier Grünewald May 8 '18 at 18:29

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.