During the development of many projects I tried to standardize the outlook of methods to help anybody to distinguish between some parts of them.
Check the code below:
public int SaveNewPartner(Partner partner, Operator modifier) { if (partner != null) { if (modifier != null) { if (partner.ID == null) { if (!SanityCheck(partner)) { throw new ApplicationException("partner failed sanity check"); } partner.ModificationTime = DateTime.Now; partner.Modifier = modifier; return Save(partner); } else { throw new InvalidOperationException("Already saved!"); //LOCSTR } } else { throw new ArgumentNullException("partner"); } } else { throw new ArgumentNullException("partner"); } }
I have problems with this code, and if I have them maybe others who meet it later in our project will have too. If You should determine which part is its business functionality it will be probably a shot in the dark. I really need to understand the whole method before I can point out how it works because parameter checking, control flow, exit point mixed with business part.
That’s why I wrote all my methods by following a pattern:
- check parameters
- define return value
- do business functionality
- return retval
Here is the rewritten method:
public int SaveNewPartner(Partner partner, Operator modifier) { if (partner == null) { throw new ArgumentNullException("partner"); } if (modifier == null) { throw new ArgumentNullException("modifier"); } int ret = 0; if (partner.ID == null) { throw new InvalidOperationException("Already saved!"); //LOCSTR } if (!SanityCheck(partner)) { throw new ApplicationException("partner failed sanity check"); } partner.ModificationTime = DateTime.Now; ret = Save(partner); return ret; }
In lines 3-10 the parameter check occures. There should be parameter checking in each public entrypoint of our class (methods, properties). I check all the parameters I use in the given method directly or in private members I call from here. It is not necessary to check params which are simly handled to other public methods (we may not know all the constraints about these, it’s not our business). I neither check the business validity here (line 3 vs. line 14).
In line 12 I define the return value, which I gave always the name ‘ret’. So if You check any line of the method You can clearly identify where the retval is set and You dont need to scroll anywhere to determine what is the retval variable.
In lines 14-24 placed the business logic. All extremalities are closed asap, so no long lasting ifs and unnecessarily deep indentations happen.
In line 26 we return from here. No other inline returns in the method body so the control flow is clear: we enter at the beginning and exit at the end.
Just a little comment:
I would move
int ret = 0;
after the checks, right before the
partner.ModificationTime = DateTime.Now;
ret = Save(partner);
This way it’s closer to the usage, what do you think about this? Thx
I wouldnt 🙂
The definition of return variable differs from the definition of other variables.
It is standing in front of all business functionality as the return statement is standing at the end of method and not in any intermediate place.
It should not suit the requirement of be as close to its usage as it can.
Its place distinct between checks above and below it. The above ones are not business checks but parameter (call validity) checks. The ones below are business checks which affects the logic of method. Thats why You may think the line is at bad place?
Because the scope of retval is the whole method, the definition and its usage (the return statement) should “wrap” business code but leave out contract related code.
I like this because it is handy and results in more readable code at least for me 🙂