Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I've created custom validation attribute that will be used to validate model. It allows me to specify values that are valid for specific field.
Here is my code:

public sealed class InAttribute : ValidationAttribute
{
    private const string DefaultErrorMessage = "{1} is invalid value for {0}.";

    private readonly int[] testArray;
    private int _toTest;

    public InAttribute(int[] @in) : base(DefaultErrorMessage)
    {
        if ([email protected]())
        {
            throw new ArgumentNullException("in");
        }

        testArray = @in;
    }

    public override string FormatErrorMessage(string name)
    {
        return string.Format(ErrorMessageString, name, _toTest);
    }

    protected override ValidationResult IsValid(object value, ValidationContext validationContext)
    {
        if (value == null)
        {
            return new ValidationResult("" + validationContext.DisplayName + " cant be null");
        }
        _toTest = Convert.ToInt32(value);
        if (!testArray.Contains(_toTest))
        {
            return new ValidationResult(FormatErrorMessage(validationContext.DisplayName));
        }

        return ValidationResult.Success;
    }
}

Then I can use it like so:

public class RequestHelpBindingModel
{
    [Required]
    [In(new []{1,3,4,7},ErrorMessage = "{1} is invalid value.")]
    [Display(Name = "Typ")]
    public int Type { get; set; }

    [Required]
    [DataType(DataType.MultilineText)]
    public string Message { get; set; }
}

I'd like to get review on this.

My questions are:

  1. Is it build right?
  2. If there are places to optimize please let me know.
share|improve this question
    
@BCdotWEB as I wrote my code is written and it is working. If part 2 and 3 is invalid I can remove it, but as I wrote they are about optimization of my code, not about something not working. – Misiu Jul 29 '16 at 12:28
    
@BCdotWEB already fixed :) sorry for that. – Misiu Jul 29 '16 at 12:37
if ([email protected]())
{
    throw new ArgumentNullException("in");
}

This won't work the way you expect it.

It should be either C# 6

 if (@in?.Any())

or for previous versions

 if (@in == null || [email protected]())

return new ValidationResult("" + validationContext.DisplayName + " cant be null");

Why the empty string at the beginning?


One time you do

return new ValidationResult("" + validationContext.DisplayName + " cant be null");

and the other time

return new ValidationResult(FormatErrorMessage(validationContext.DisplayName));

I thought you forgot to use the FormatErrorMessage in the first case but it produces a completely different error message.

Create another method and name it accordignly like FormatNullValueErrorMessage and the other one FormatNotInErrorMessage.


ErrorMessage = "{1} is invalid value.")

I don't think this parameter has any value and is actually very error prone because the user of this attribute has to know that he can use the {1} and what it means. I'd stick to an automatic message about the value not beeing allowed.


private readonly int[] testArray;

I would rename this field to soemthing like _allowedValues. As I first time saw it I thought it's a test code. Reserve the test for tests or really rare cases when there is no better word.


_toTest = Convert.ToInt32(value);

It's not necessary to store the value locally. Pass it as a paramter to the FormatXErrorMessage.

share|improve this answer
    
Thank You for review. I'll try to fix all mistakes and I'll add my code as edit in question so that You could take a second look. – Misiu Aug 2 '16 at 8:54

While writing any attribute, please add the information about where the attribute will be used, like below.

[System.AttributeUsage(AttributeTargets.Property, Inherited = false, AllowMultiple = false)]
public sealed class InAttribute : ValidationAttribute
{
     //TODO: your code here
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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