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 have created a method to check for null/empty values of class properties and if any null property is found I'm stopping the checking process and returning the result as true. I'm used a solution from [http://stackoverflow.com/questions/22683040/how-to-check-all-properties-of-an-object-whether-null-or-empty] and following is the code i have implemented:

/// To check the properties of a class for Null/Empty values
/// </summary>
/// <param name="obj">The instance of the class</param>
/// <returns>Result of the evaluation</returns>
public static bool IsAnyNullOrEmpty(object obj)
{
    //Step 1: Set the result variable to false;
    bool result = false;

    try
    {
        //Step 2: Check if the incoming object has values or not.
        if (obj != null)
        {
            //Step 3: Iterate over the properties and check for null values based on the type.
            foreach (PropertyInfo pi in obj.GetType().GetProperties())
            {
                //Step 4: The null check condition only works if the value of the result is false, whenever the result gets true, the value is returned from the method.
                if (result == false)
                {
                    //Step 5: Different conditions to satisfy different types
                    dynamic value;
                    if (pi.PropertyType == typeof(string))
                    {
                        value = (string)pi.GetValue(obj);
                        result = (string.IsNullOrEmpty(value) ? true : false || string.IsNullOrWhiteSpace(value) ? true : false);
                    }
                    else if (pi.PropertyType == typeof(int))
                    {
                        value = (int)pi.GetValue(obj);
                        result = (value <= 0 ? true : false || value == null ? true : false);
                    }
                    else if (pi.PropertyType == typeof(bool))
                    {
                        value = pi.GetValue(obj);
                        result = (value == null ? true : false);
                    }
                    else if (pi.PropertyType == typeof(Guid))
                    {
                        value = pi.GetValue(obj);
                        result = (value == Guid.Empty ? true : false || value == null ? true : false);
                    }
                }
                //Step 6 - If the result becomes true, the value is returned from the method.
                else
                    return result;
            }
        }
    }
    catch (Exception ex)
    {
        throw ex;
    }

    //Step 7: If the value doesn't become true at the end of foreach loop, the value is returned.
    return result;
}

I want to extend this code to another end that instead of sending the bool result as true or false, I'll clone the incoming object and based on the values inside the properties can send the object.
For example: Let say this is my model: Order.cs

public class Order
{

    [DataMember]
    public Guid OrderId { get; set; }

    [DataMember]
    public string Owner { get; set; }

    [DataMember]
    public string Info { get; set; }

    [DataMember]
    public string Recipient { get; set; }

    [DataMember]
    public int Test { get; set; }

    [DataMember]
    public DateTime CreatedOn { get; set; }
}

Now if there is any null or empty value in any of the properties, I'll be returning a new class which contains only those values which are either null and empty and the status of it as:

//This class is the output class from the IsAnyNullOrEmpty() method and has only those properties which are either null or empty
public class ProcessedModel
{       
    public string Property1 { get; set; }   //has null value in type String (set property vaule to "Null value")

    public string Property2 { get; set; } //has empty value in type String (set property vaule to "Empty value")

    public string Property3 { get; set; } //has 0 value in type int (set property vaule to "0 value")
}

Am I following the right approach? Any suggestions, feedback is welcome.

share|improve this question

First of all I'd reduce indentation. It makes your code really too hard to read. Doing that you will see it may be simplified.

First of all try/catch: a catch-all block where you rethrow the exception is useless (and you may even throw away line number information.)

This code:

if (obj != null)
{
    // Do something...
}

May be replaced simply with:

if (obj == null)
    return false;

You do not need to declare a local variable for result, simply return its value where appropriate.

You do not have to check condition result == false inside your foreach loop (you will still enumerate through all properties), simply break the loop and exit immediately.

You do not need to use to dynamic, when casting you have the right type at compile-time. Also move all this code inside a separate function:

private static bool IsNullOrEmpty(object obj) {
}

Let's see how to fill this function. Let's read the value of the property:

object value = pi.GetValue(obj);

First of all a common case, valid for all reference types:

if (Object.ReferenceEquals(value, null))
    return true;

Then explicitly check for strings:

if (value is string && String.IsNullOrEmpty((string)value))
    return true;

For Int32 (note that in your original code value <= 0 ? true : false is redundant it may be simplified to value <= 0). BTW are you sure you want to consider negative numbers as empty values?!

if (value is int)
    return ((int)value) <= 0;

Boolean does not need to be checked in this case (null case has been already handled). We just need to work out Guid:

if (value is Guid)
    return ((Guid)value) == Guid.Empty;

That's all. Note that now or in future you may want to check for Nullable<T> and or other types. With the exception of strings and negative integer numbers you can also do something like this:

if (Object.ReferenceEquals(value, null))
    return false;

var type = value.GetType();
return type.IsValueType
    && Object.Equals(value, Activator.CreateInstance(type));

What left in your original function? In this example let me use simplified check (negative numbers are not empty and I do not check for String.Empty against strings however it handles nullable types), feel free to pick the one works for you:

public static bool IsAnyNullOrEmpty(object obj)
{
    if (Object.ReferenceEquals(obj, null))
        return false;

    return obj.GetType().GetProperties()
        .Any(x => IsNullOrEmpty(x.GetValue(obj)));
}

private static bool IsNullOrEmpty(object value)
{
    if (Object.ReferenceEquals(value, null))
        return false;

    var type = value.GetType();
    return type.IsValueType
        && Object.Equals(value, Activator.CreateInstance(type));
}

Last note: if working with databases you may also want to check equality with DBNull.Value.

share|improve this answer
    
-Thanks for the prompt reply and detailed feedback, you have reduced a significant amount of code from the existing approach. I have one doubt regarding try-catch. You have suggested to drop the try-catch but i'm using it to track any errors or may be for logging in future. As per the best practices are considered, the value of the function should be returned at the end that's the only reason for declaring result variable. Also I have one more requirement as asked in the question to return a new class with only null values Can you share your thoughts on it? Many thanks for the answer – iSahilSharma 5 hours ago
    
try/catch: if you have an exception handling policy (you should!) you probably don't want to handle any exception here. Let me explain: reading a property should never ever throw an exception. If it does then you forgot something you should handle/do/redesign. In that case you want to fix it during development, no need to hide this error. – Adriano Repetti 5 hours ago
1  
2nd, at function level, you should catch only exceptions you know may happen. Is an OutOfMemoryException reasonable here? Has it anything to do with reading properties? What about AccessViolationException? Catching Exception you're hiding possibly edge cases you MUST handle in your code (which if just goes ignored and swallowed are possible latent bugs and/or performance hits you do not want) – Adriano Repetti 5 hours ago
1  
Single exit point is an abandoned best practice. It had some sense (and it may have) in other languages but it's not something you want to follow in C#. There are still cases where it may be desiderable (for example extreme mission critical software) but not in 99.9999% of software (and probably not in something we will ever write in our life). Easier to read code and shorter+simpler functions are more often than not more advantageous than its drawbacks (slightly harder to follow execution path, which BTW should not be an issue if you keep your functions very short) – Adriano Repetti 4 hours ago

@Adriano Repetti was faster with his list of issues ;-) so I'll just post an alternative soltuion where you can have everyting in only one linq query using the All extension. It will stop as soon as some property doesn't meet the conditions.

Consider changing the name to AllPropertiesValid because with your custom conditions like value <= 0 there is no default solution for all properties and it better shows that it checks the properties of the parameter.

public static bool AllPropertiesValid(object obj)
{
    return !obj.GetType().GetProperties().All(p =>
    {
        var value = p.GetValue(obj);

        if (value == null) { return false; }

        if (p.PropertyType == typeof(string))
        {
            if (string.IsNullOrEmpty((string)value)) { return false; }
        }

        if (p.PropertyType == typeof(int))
        {
            if ((int)value <= 0) { return false; }
        }

        if (p.PropertyType == typeof(bool))
        {
            if (!(bool)value) { return false; }
        }

        if (p.PropertyType == typeof(Guid))
        {
            if (((Guid)value) == Guid.Empty) { return false; }
        }

        return true;
    });
}

(I hope I got the conditions right)

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.