0

I'm doing some simple validation inside my Controller I know this would better placed inside something like a service layer but for this I want to keep it inside the Controller

The idea is to check that a valid url is being entered into a url field to display an image, e.g. http://domain.com/myawesomeimage.png would be valid and http://domain.com/ would not be valid.

// ValidateInt has a default value of 0
        int ValidateInt = 0;
        // If the url entered (if one at all) does not have correct extension then increment the ValidateInt
        if (!ArticleToEdit.image.Contains(".jpg"))
            ValidateInt++;
        if (!ArticleToEdit.image.Contains(".jpeg"))
            ValidateInt++;
        if (!ArticleToEdit.image.Contains(".png"))
            ValidateInt++;
        if (!ArticleToEdit.image.Contains(".gif"))
            ValidateInt++;
        if (!ArticleToEdit.image.Contains(".bmp"))
            ValidateInt++;
        // if ValidateInt is bigger than 0 then the url is invalid
        if (ValidateInt > 0)
            ModelState.AddModelError("Image", "Please enter a valid URL.");

EDITED CODE

Improve this question

3 Answers 3

Reset to default
2

The problem with your code

Your code is invalid, because your model state will always have at least 4 errors. Even though the URL would be correct. Your code requires that your URL must have all extensions which is of course incorrect. It can only have one. At most.

The solution

Use DataAnnotations instead and use regular expression validator. You're obviously already using some application model class called ArticleToCreate. You'll have to put data annotations attribute on the image property (one more observation: keey property names with Pascal casing so it's the same as .net):

public class ArticleToCreate
{
    [RegularExpression(@"...")] // add regular expression that fulfils your requirements
    public string Image { get; set; }
    ...
}

Then it all depends how complicated your regular expression is. The easiest one for your needs could be just that it starts with an http:// and end with the correct extension:

^http:\/\/.+\.(?:png|jpe?g|gif|bmp)$

And if you're directly providing your class instance to controller action it will get automatically validated for you without any additional code. This way you won't be able to forget to validate your objects manually.

Consider this controller action that automatically validates your model class object instance by validators defined on it (as per validator definition I've written above):

public ActionResult Create(ArticleToCreate data)
{
    if (!this.ModelState.IsValid)
    {
        // handle invalid object
    }
    // handle valid object
}

This way your actions will focus on the processing part which is their main objective instead of focusing on too many aspects of your business process like validation for instance.

Shorter code = simpler code = easier to maintain = less bugs = less work = happy clients

Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

1

Why not create custom ValidationAttributes (from DataAnnotations) and allow the Validation Engine do the work for you rather than worrying about where to put your logic?

I'm guessing it would look something like:

[AttributeUsage(AttributeTargets.Property | 
    AttributeTargets.Field, AllowMultiple = false)]
public class ValidImageUrlAttribute : ValidationAttribute
{
    public string ErrorMessage { get; set; }

    public override bool IsValid(object value)
    {
        var url = value as string;
        if(!url.Contains(".jpg") || !url.Contains(".jpeg")
           || !url.Contains(".gif") || !url.Contains(".bmp")
           || !url.Contains(".png"))
        {
            return false;
        }

        return true;               
    }

    public override string FormatErrorMessage(string name)
    {
        return ErrorMessage ?? base.FormatErrorMessage(name);
    }
}

And then you could decorate your Model:

[Required(ErrorMessage = "Image URL is required.")]
[ValidImageUrl(ErrorMessage = "Valid Image URL is required.")]
public string ImageUrl { get; set; }
Improve this answer

6 Comments

I don't follow I'm afraid. Can you explain? Thanks
@Cameron - Update with an example.
Writing a custom validator is not necessary when there's functionality out of the box that can be used. RegularExpressionAttribute does the trick just fine. Check my answer.
@Robert - It may not be necessary yet, but I'm guessing the OP is going to need to add more validation (considering a file extension really isn't going to cut it). A custom validator will allow for that added flexibility.
@Justin Niessner: What added validation could he want? Checking the net that a particular image file actually exists? If string is all he has to validate, regular expression is something that can validate well almost anything. but if he wants to validate some other things as well (like checking something against the DB), then a custom validator is the answer of course. But as it seems here from the example URL string validation is all that's needed here. YAGNI principle. He'll change things when he'll need them. In other words: he'll think about the bridge when he gets to the river.
|
0

changed if (validate1 > 0) and it works fine :)

Improve this answer

4 Comments

But still. You're doing something similar to this: You buy yourself a car and then you push it bare-handedly around instead of driving it. Asp.net MVC provides automatic validation. Why not use it instead?
And your multiple if statements could as well be all replaced by a single one. And no need to compare a boolean value to true...
AND your code is still not valid! The first one increments validation variable when it's invalid. Others increment it when it's actually valid! You should at least use Justin's single if statement.
Could you show an example of what you mean here. How would I combine it all into one piece of code like you mention? I've just updated my original question with my current final version of the code. Thanks.

Your Answer

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.