How to refactor this series of null or empty string tests to make it smaller and cleaner?

Question

At the moment, I manually validate this way for 10 properties. Here I show only 3 properties.

if (string.IsNullOrEmpty(account.Name))
{
    return BadRequest("Name is required");
}

if (string.IsNullOrEmpty(account.Nationality))
{
    return BadRequest("Nationality is required");
}


if (string.IsNullOrEmpty(account.Phone))
{
    return BadRequest("Phone is required");
}

I believe there is a better way to refactor the code to reduce lines of code for API validation.

Writing a separate function can be a solution. What is your thought on it? — Prasad Telkikar, May 18 '21 at 12:50
It is unclear what exactly is the duplication that you want to remove. Do you want to avoid writing these exact 3 if checks everywhere? If so, extract a method. Do you not like writing `string.IsNullOrEmpty` all the time? If so, create a list of key value pairs containing the value you want to check, and the error message, and loop through it until you find a null/empty value. — Sweeper, May 18 '21 at 12:55
@Steve I updated my answer which was rubbish at first, and voted to reopen. — , May 18 '21 at 13:31

score 1 · Answer 1 · answered May 18 '21 at 12:57

1

you can decorate your properties with the Required Attribute from the System.ComponentModel.DataAnnotations namespace

then validate your ModelState

if (!ModelState.IsValid)
{
   return BadRequest(ModelState);
}

answered May 18 '21 at 12:57

Charles

67
8

if use Required attribute, the error response will be different compare to the manual validation. – Steve May 18 '21 at 13:07
2

you can specify the message in the Required attribute overload – Charles May 18 '21 at 13:12

score 0 · Answer 2 · 2021-05-18T14:09:08.520

Assuming you have such a class and want to check all the rw public string properties:

using System.Linq;
using System.Reflection;

class Account
{
  public string Name { get; set; }
  public string Nationality { get; set; }
  public string Phone { get; set; }
  public int Value { get; set; }
}

We can refactor using reflexion and Linq like that:

var account = new Account();
account.Name = "Name";

var type = account.GetType();

var flags = BindingFlags.Public 
          | BindingFlags.Instance 
          | BindingFlags.GetProperty 
          | BindingFlags.SetProperty;

var property = type.GetProperties(flags)
                   .Where(p => p.PropertyType == typeof(string))
                   .Where(p => string.IsNullOrEmpty((string)p.GetValue(account)))
                   .Select(p => p.Name)
                   .FirstOrDefault();

if ( property != null )
  return BadRequest($"{property} is required");

We take all properties of the type being public instance and not static as well as having getter and setter, then we filter on the required string type and we check the value to select the name of the first found else the result is null.

To not check all string properties, we can use an custom Ignore attribute:

class IgnoreAttribute : Attribute
{
}

class Account
{
  [Ignore]
  public string MyString { get; set; }
  public string Name { get; set; }
  public string Nationality { get; set; }
  public string Phone { get; set; }
  public int Value { get; set; }
}

The Linq query is now:

var property = type.GetProperties(flags)
                   .Where(p => p.PropertyType == typeof(string))
                   .Where(p => p.GetCustomAttribute(typeof(IgnoreAttribute)) == null)
                   .Where(p => string.IsNullOrEmpty((string)p.GetValue(account)))
                   .Select(p => p.Name)
                   .FirstOrDefault();

Also as @Charles mentionned, RequiredAttribute can be used instead:

using System.ComponentModel.DataAnnotations;

class Account
{
  public string MyString { get; set; }
  [Required]
  public string Name { get; set; }
  [Required]
  public string Nationality { get; set; }
  [Required]
  public string Phone { get; set; }
  public int Value { get; set; }
}

var property = type.GetProperties(flags)
                   .Where(p => p.PropertyType == typeof(string))
                   .Where(p => p.GetCustomAttribute(typeof(RequiredAttribute)) != null)
                   .Where(p => string.IsNullOrEmpty((string)p.GetValue(account)))
                   .Select(p => p.Name)
                   .FirstOrDefault();

Or in fact here a NotNullOrEmpty attribute will be appropriate:

class NotNullOrEmptyAttribute : Attribute
{
}

class Account
{
  public string MyString { get; set; }
  [NotNullOrEmpty]
  public string Name { get; set; }
  [NotNullOrEmpty]
  public string Nationality { get; set; }
  [NotNullOrEmpty]
  public string Phone { get; set; }
  public int Value { get; set; }
}

var property = type.GetProperties(flags)
                   .Where(p => p.PropertyType == typeof(string))
                   .Where(p => p.GetCustomAttribute(typeof(NotNullOrEmptyAttribute)) != null)
                   .Where(p => string.IsNullOrEmpty((string)p.GetValue(account)))
                   .Select(p => p.Name)
                   .FirstOrDefault();

LINQ: One "where" clause versus multiple chained "where clauses"

Proper Linq where clauses

RequiredAttribute Class

How Can I Use Data Annotations Attribute Classes to Fail Empty Strings in Forms?

Creating a Domain Object Validation Framework