We had a mini-code review on Friday for one of the projects. This is a standard thing that we're doing, code reviews basically every week taking no more than an hour (and hopefully half an hour once we get into the swing of things). Code reviews are something you do to check the sanity of what someone did. Really. You wrote some code 6 months ago and now is the time to have some fresh eyes looking at it and pointing out stupid things you did.
Here's the method from the review:
1: protected virtual bool ValidateStateChangeToActive()
3: if ((Job.Key == INVALID_VALUE) ||
4: (Category.Key == INVALID_VALUE) ||
5: (StartDate == DateTime.MinValue) ||
6: (EndDate == DateTime.MinValue) ||
7: (Status.Key == INVALID_VALUE) ||
8: (Entity.Key == INVALID_VALUE) ||
9: (String.IsNullOrEmpty(ProjectManagerId.ToString()) || ProjectManagerId == INVALID_VALUE) ||
10: String.IsNullOrEmpty(ClientName) ||
15: return false;
17: return true;
It's horrible code. It's smelly code. Unfortunately, it's code that I can't think of a better way to refactor? The business rule is that in order for this object (a Project) to become active (a project can be active or inactive) it needs to meet a set of criteria:
- It has to have a valid job number
- It has to have a valid category
- It has to have a valid start and end date (valid being non-null and in DateTime terms this means not DateTime.MinValue)
- It has to have a valid status
- It has to have a valid entity
- It has to have a valid non-empty project manager
- It has to have a client name
- It has to have a title
That's a lot of "has to have" but hey, I don't make up business rules. So now the dilemma is how to refactor this to not be ugly. Or is it really ugly?
The only thing that was I was tossing around in my brain was to use a State pattern to implement active and inactive state, but even with that I still have these validations to deal with when the state transition happens and I'm back at a big if statement. Sure, it could be broken up into smaller if statements, but that's really not doing anything except formatting (and perhaps readability?).
Anyone out there got a better suggestion?