3

Consider the following check. I can find the same identical error check 10 times across my solution.

if (request == null)
 throw GetFaultException("Request object is null");

Now consider the next one:

if (model == null)
 throw GetFaultException("Request model not well formed");

The only stuff that is changing is the error message and the name of the checked variable.

One part of refactoring had already been done since the GetFaultException is already hiding complexity and separating responsibilities.

I see many possible further refactoring. Like using Monads, AOP, events (?).

I think the really really best solution would be to make the code thinking like in a MVC model.

Thus if (condition) request == null then (it means) A mandatory parameter has not been specified then (it means) An exception is needed with a specific message

The good stuff I see in this approach is that the procedural programming makes the code unspeaking and unaware. Instead if I raise a NullMandatoryParameterEvent it's really clear why I'm checking a variable for being null and what I do if it is null.

The question is: how would you refactor this kind of Check (according to the objective I've defined)? And do you share these objectives?

Community
  • 1
  • 1
Revious
  • 7,816
  • 31
  • 98
  • 147
  • 5
    This question might be better suited for [Code Review](http://codereview.stackexchange.com/). – rene Apr 25 '14 at 14:25
  • I'm voting to close this question as off-topic because working code looking for a revision or refactor should be posted about on Code Review, not Stack Overflow. – TylerH Jan 30 '17 at 20:04
  • @TylerH: thanks for the explanation, but in my humble opionion it has no sense to vote to close a question which is years old and has already an answer. It's probably showing a need to have everything perfect according to our internal rules – Revious Feb 01 '17 at 08:36
  • @Revious Yes, it's a matter of voting to close a question that has been off-topic (in a number of ways) since it was posted. Not only is it asking for a code review/refactor, it's also asking an opinion-based question ("How would *you* write this?" is not quite objective). Further, you've indicated with the [tag:software-engineering] tag that you are seeking input on a broad subject that is off-topic (there's a whole site for software-engineering questions, aptly named Software Engineering). – TylerH Feb 01 '17 at 14:43
  • @Revious incidentally, that tag is [undergoing burnination](http://meta.stackoverflow.com/questions/341843/should-we-burninate-software-engineering) right now. – TylerH Feb 01 '17 at 14:44
  • @TylerH: Try to view things from another perspective. This need for classifying old questions it's totally useless (near to excessive perfectionism in psychology, and your time can be used in a more productive way) UNLESS it will be used to instruct a deep learning system which will authomatize this kind of task. – Revious Feb 07 '17 at 10:06
  • @TylerH: try an experiment. Explain me which rational benefits were reached by getting this question closed or which benefits would be reached if it would be reopened? I see 0 in both cases. – Revious Feb 07 '17 at 10:11

1 Answers1

4

I assume that your program cannot continue if the validated property is null. Therefore, I'd stick with exceptions - this is exactly the situation that the ArgumentNullException was invented for. You can state the parameter that was null and by that communicate clearly what the caller should fix in order to get the code running. Events are not an alternative to exceptions as the caller might or might not handle them.

In my projects I've also tried to optimize the code layout by putting the validation logic in one place. In smaller projects by creating some helper methods like AssertIsNotNull that takes the parameter and the name as an input so that it can construct and throw an ArgumentNullException.

In my last project, one that is expected to grow heavily, I've created a specific ValidationService and used AOP to put the logic in place. I've also used the attributes located in the System.ComponentModel.DataAnnotations namespace to declare the rules that should be validated in addition to the input parameter being not null. MVC uses the same attributes to validate the models both on the client (through JavaScript) and on the server.

Another approach you might consider is using code contracts. Though you still have to define the preconditions in each method, you can use static code analysis to find spots that violate the contract. You can also configure when the checks are done. For instance, you can run all the checks while in testing and skip certain tests for a production environment for performance reasons. For an overview also see this link.

Markus
  • 20,838
  • 4
  • 31
  • 55