Violating CQS. Looking For Suggestions And Alternatives.

When doing simple input validation on forms, I often end up writing a lot of code like this:

public void SaveRequested()

{

 bool descriptionIsValid = ValidateDescription();

 bool abbreviationIsValid = ValidateAbbreviation();

 bool isValid = (descriptionIsValid && abbreviationIsValid);

 if (isValid)

 {

   VisitType visitType = new VisitType(Description, Abbreviation);

   VisitTypeRepository.Save(visitType);

 }

}

 

private bool ValidateAbbreviation()

{

 bool isValid = !string.IsNullOrEmpty(Abbreviation);

 if (isValid)

   View.HideAbbreviationRequiredMessage();

 else

   View.ShowAbbreviationRequiredMessage();

 return isValid;

}

 

private bool ValidateDescription()

{

 bool isValid = !string.IsNullOrEmpty(Description);

 if (isValid)

   View.HideDescriptionRequiredMessage();

 else

   View.ShowDescriptionRequiredMessage();

 return isValid;

}

I recognize the violations of Command-Query-Separation in this code, and potentially other issues in design and implementation. The ValidateDescription and ValidateAbbreviation methods are fairly horrendous, from a CQS perspective. They are both querying the data to see if it’s valid, and also executing commands if it is or is not valid. Then on top of that, the SaveRequested method is executing another set of commands if both Description and Abbreviation are valid.

The requirements of this functionality are:

  • If Description is null or empty, show the “Description Required” message
  • If Abbreviation is null or empty, show the “Abbreviation Required” message
  • If both Description and Abbreviation are ok, allow the save to happen.

I’m not considering the difference between active and passive validation (on typing, vs on clicking save) in this example. That’s a different discussion for a different time.

So, how do you handle this type of input validation, accounting for CQS, code readability, testability, etc? I’m looking for suggestions and alternatives to this type of ugly code. I have a lot of my own ideas, but before I travel too far down the road, I want input from the rest of the world.

Note On Responses: As a suggestion for responding – if your response will take more than a few lines of very simple code and text, I’d really like to see your full response on your blog. Post a comment here (or pingback/trackback from your post) when you have your post up. If you don’t have a blog to respond with – shame on you! Blogspot.com is free. Go get a blog and respond. :)


Post Footer automatically generated by Add Post Footer Plugin for wordpress.

About Derick Bailey

Derick Bailey is an entrepreneur, problem solver (and creator? :P ), software developer, screecaster, writer, blogger, speaker and technology leader in central Texas (north of Austin). He runs SignalLeaf.com - the amazingly awesome podcast audio hosting service that everyone should be using, and WatchMeCode.net where he throws down the JavaScript gauntlets to get you up to speed. He has been a professional software developer since the late 90's, and has been writing code since the late 80's. Find me on twitter: @derickbailey, @mutedsolutions, @backbonejsclass Find me on the web: SignalLeaf, WatchMeCode, Kendo UI blog, MarionetteJS, My Github profile, On Google+.
This entry was posted in Analysis and Design, C#, Principles and Patterns. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • vitalya

    you may be interested in caslte validator component http://castleproject.org/components/validator/index.html
    It is enviroment independent so you can use it either in web or non web applications.
    for asp.net mvc you can check this blog post for server validation
    http://blog.stormid.com/archive/2009/04/07/automatic-model-validation-with-asp.net-mvc-xval-castle-and-a.aspx and this one for client-side http://blog.codeville.net/2008/04/30/model-based-client-side-validation-for-aspnet-mvc/

  • DaRage

    First I would take our the validation logic from the SaveRequested() method and put it in it’s own method let’s call it bool Validate() which can be public to allow the client to do proactive validation. The Validate() will return true if it’s valid so the SaveRequested() will end up with only:

    if (Validate())
    {
    VisitType visitType = new VisitType(Description, Abbreviation);
    VisitTypeRepository.Save(visitType);
    }

    I know.. this is still the same code and it’s not seperating Command from Query but validation is a legitimate query within the command. I think the patterns means by Query the queries that are initiated by the client.

    Also, you can optomize the validation by returning false when only when error has occurred. Hide and Show Message methods show be in the UI code and not near the save. and by the way shame on you.

  • http://www.lostechies.com/members/derick.bailey/default.aspx derick.bailey

    @vitalya

    thanks for the links. I’ve seen Castle’s validators before, but had forgotten about them.

    @DaRage,

    the shame is felt… and admission is the first step to recovery, right? that’s why i post stuff like this. :)

  • ReSc

    Renaming might be a easy fix:

    rename ValidateAbbreviation to ShowAbbreviationValidationMessage and interprete the return bool as succes or failure of the command. voila, you now have a command that does what it says, instead of a command that does way more then what is says,
    and to make the query visible you can also extract the query

    bool isValid = !string.IsNullOrEmpty(Abbreviation);

    to its own method named AbbreviationIsValid().

    This can also be applied to the other methods.

  • http://www.bluespire.com/blogs Rob

    What about something like this:

    var result = Validate();

    if(result.HasErrors)
    result.Notify(View)
    else Save();

  • BjartN

    The Notification pattern gave me some hints on solving a similar problem.

    http://weblogs.asp.net/pgreborio/archive/2005/03/06/386260.aspx

    Maybe It can help you as well :)

  • http://siderite.blogspot.com Siderite

    First of all, I think that code is not that bad. I don’t believe the CQS should be applied religiously. In this case, it is irrelevant.

    But if I was to think of a “good” way to do it, I would move the validation logic in a Validate method and implement an IsValid property that Validate would change. Then the Save operation would check if IsValid is true (or even set) before saving.

    Kind of like the ASP.Net validation framework.

  • Rei

    IValidator

    RequiredValidator : IValidator for Both properties

    violations = ValidatorEngine.Validate(ListOfIValidators)

    violations.count > 0 ShowErrors(violations)

    Something like that?