2
\$\begingroup\$

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 (!@in.Any())
        {
            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.
\$\endgroup\$
  • \$\begingroup\$ @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. \$\endgroup\$ – Misiu Jul 29 '16 at 12:28
  • \$\begingroup\$ @BCdotWEB already fixed :) sorry for that. \$\endgroup\$ – Misiu Jul 29 '16 at 12:37
2
\$\begingroup\$
if (!@in.Any())
{
    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 || !@in.Any())

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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Misiu Aug 2 '16 at 8:54
1
\$\begingroup\$

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
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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