5
\$\begingroup\$

I am working on a small and self-contained poll application, not unlike Straw Poll whereby one user can submit a poll and other users can vote (but only once).

I am building this little application in the open, using Angular for the front-end and Web Api for the back-end. I am using Fluent Validation to validate incoming models.

Using an action filter and Fluent Validation I have managed to remove all basic model validation from my controller.

I need to carry out some more advanced validation after I load the poll from the database. My action method has become overrun by such validation logic as you can see:

public IHttpActionResult Put(int pollId, VoteInput voteInput)
{
    var poll = _session.Load<Poll>(pollId);
    var voterIp = Request.GetOwinContext().Request.RemoteIpAddress;

    if (poll == null)
    {
        return BadRequest("you cannot vote on a poll that does not exist.");
    }

    if (poll.VoterIps.Contains(voterIp))
    {
        return BadRequest("you cannot vote on this poll because you have already voted.");
    }

    if (voteInput.Options.Length > 1 && !poll.MultiChoice)
    {
        return BadRequest("you cannot vote for more than one option. noob.");
    }

    foreach (var option in poll.Options.Where(option => voteInput.Options.Contains(option.Id)))
    {
        option.Votes += 1;
    }

    poll.VoterIps.Add(voterIp);
    _session.SaveChanges();

    return Ok();
}

I would very much like to remove some, if not all of this validation logic from the controller.

I have a really sound infrastructure - I am using Fluent Validation and a DI Framework - but owing to my inexperience with said technologies, I cannot think of a clean way to extract the validation logic.

What are some ways I can reduce or eliminate the validation in my controller?

\$\endgroup\$
1
  • \$\begingroup\$ Titles should reflect on the code's purpose, not a review request. If the edited title was misleading about that, then it can still be fixed. \$\endgroup\$
    – Jamal
    Commented Jan 6, 2015 at 23:25

2 Answers 2

4
\$\begingroup\$

I cannot think of a clean way to extract the validation logic.

All validation logic should be in an appropriate "validation service" class as @meWantToLearn said.

I assume something like this is possible in FluentValidation:

public class PollVoteValidator : AbstractValidator<Poll,VoteInput> 

If not then make a simple class that contains both of these and then create a validator for that:

public class PollInput {
    public Poll {get; set;}
    public VoteInput {get; set;}
}

public class PollVoteValidator : AbstractValidator<PollInput>

Validation Separation of Concerns

Validations such as if (poll == null) belong in a PollValidator class and if (poll.VoterIps.Contains(voterIp)) in a VoterIpsCollection class.

Even if you want to check for null method arguments up front (and that will simplify downstream control logic), the above still applies.

Validation Coordination

The PollVoteValidator would coordinate everything perhaps, with logic, somewhere, like this:

public class PollVoteValidator {
    public ValidateResults Validate (poll, voteInput) {
        var pollValidateResults = myPollValidator.Validate(poll);
        var voteInputValidateResults = myVoteInputValidator.Validate(voteInput);
        // I hope poll is not null!
        var voterIpsCollectionResults = myVoterIpsCollectionValidator.Validate(poll.VoterIps);

        if(/* validations above pass */)
            pollVoteValidatorResults = mypollVoteValidator.Validate(poll, voteInput);

        // consolidate validation results as desired (error messages)
        // for return to the caller

        return pollVoteValidatorResults;
    }
} 

One More Thing

This: var voterIp = Request.GetOwinContext().Request.RemoteIpAddress; seems "orphaned" from an OO perspective. voterIp feels like a Voter class property, so maybe you should instantiate a Voter; this implies a "validation service" class for Voter.

The above Voter class stuff may seem like overkill, but there is no OO principle that says "if something seems simple, ignore OO principles".

\$\endgroup\$
2
  • \$\begingroup\$ What might the calling code look like? Should I be concerned about querying the database two or thee times like I would be required to in your case? Thank you very much for your answer so far. \$\endgroup\$ Commented Jan 6, 2015 at 21:11
  • 1
    \$\begingroup\$ First, focus on design to meet your question's goals and overall class design in general. Then when they are instantiated - DB IO concerns - are mitigated with Lazy loading, object caching, etc. I suspect Validator objects can be re-used. Then how the calling code looks takes care of itself because good design exposes properties & methods that make sense for the business/problem domain. Your architecture (MVC, restful, etc) influences "when" and "how" but class design is paramount. \$\endgroup\$
    – radarbob
    Commented Jan 8, 2015 at 18:05
0
\$\begingroup\$

You need to move your validation code into a validation service class. It should return true or the validation exception to your controller. Validation logic will be implemented in the validation service class. Keep your controller always short and simple.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.