10
\$\begingroup\$

I have a class that I use to display on a web page fieldset. However, all of the properties of this class are optionally filled in by the user, which means all of them could be null/default. In this case I would like not to render the fieldset at all, instead of displaying an empty one.

I wrote a simple isEmpty() method that checks all property values using reflection, but I'm afraid this may become way too expansive.

public class User
{
    [Display(Name = "First Name:")]
    public string FirstName { get; set; }

    [Display(Name = "Last Name:")]
    public string LastName { get; set; }

    public bool isEmpty()
    {
        Type t = this.GetType();

        var properties = t.GetProperties(BindingFlags.Public | BindingFlags.Instance);

        foreach (var prop in properties)
        {
            var value = prop.GetValue(this, null);

            if (value != null && (string)value != "")
            {
                return false;
            }
        }

        return true;
    }
}

The isEmpty(), of course, assumes all properties are strings.

What do you think? Will this become potentially expansive as the class grows?

\$\endgroup\$
  • 1
    \$\begingroup\$ Well, to start, I think I'd rather see the method named IsEmpty instead of isEmpty. This is per Microsoft's Capitalization Guideline. \$\endgroup\$ – RubberDuck Nov 19 '14 at 21:36
  • \$\begingroup\$ Is this ASP.NET MVC 5? \$\endgroup\$ – mjolka Nov 19 '14 at 23:01
12
\$\begingroup\$

Naming:

Don't use variable names such as t. They're not meaningful, not for you, not for others. In this case, use type.

Class names, public fields and method names use PascalCase and not camelCase. So, isEmpty will become IsEmpty. But then again, this is also not a meaningful name. Boolean methods or properties in C# will most likely start with Is or Has. An example for the name of your method would be HasAllEmptyProperties.

var keyword:

Use var to declare your variables instead of declaring them explicitly. The compiler will determine the type for you and your code looks cleaner.

The method:

Your code looks good. You could however make it cleaner by using LinQ. Loops won't be avoided but it looks cleaner. I'm not saying this is a perfect solution but here goes:

public bool HasAllEmptyProperties()
{
    var type = GetType();
    var properties = type.GetProperties(BindingFlags.Public | BindingFlags.Instance);
    var hasProperty = properties.Select(x => x.GetValue(this, null))
                                .Any(y => y != null && !String.IsNullOrWhiteSpace(y.ToString()));
    return !hasProperty;
}

I could also have used the All() method and positive comparison instead of Any(). But this is inefficient as all elements have to be checked against the condition whereas Any() will return as soon as an element satisfies the condition (like in your loop).

You can take the comparison code and throw it in an extension method for reusability. It also makes your method-code cleaner, for example:

public static class Extensions
{
    public static bool IsNullOrEmpty(this object obj)
    {
        return obj == null || String.IsNullOrWhiteSpace(obj.ToString());
    }
}

public bool HasAllEmptyProperties()
{
    var type = GetType();
    var properties = type.GetProperties(BindingFlags.Public | BindingFlags.Instance);
    var hasProperty = properties.Select(x => x.GetValue(this, null))
                                .Any(x => !x.IsNullOrEmpty());
    return !hasProperty;
}

Since you only have two properties, performace won't be a problem. Even if you were to add a couple more properties. Hope this helps!

\$\endgroup\$
  • \$\begingroup\$ Thanks for your reply. I agree with you on the variable name; however, I chose "t" for "Type". I don't think it is a problem in this particular case because the code is really small (it is not a habit). As for the styling, I have only recently started to use C#, and the style is a bit different from other languages I'm familiar with, and sometimes I mix them up. Thanks for pointing it out. Regarding the var, I did use it. I kept "Type" because it is one letter longer, it's not a real time saver to type "var" instead. Had I used var, I would definitely have named it "type" instead of "t". \$\endgroup\$ – victor Nov 20 '14 at 4:49
  • \$\begingroup\$ About using LINQ, in this case, I disagree that it makes the code easier to read. I think it makes the code looks a lot more like code instead of a human readable text (again, in this case). The extension method is interesting, never used any extension method, I will look into it; still, this method is used only for this class and wont every be used by any other, so I think I will skip that :) Thanks for your review. --Edit-- I just checked, LINQ would probably be slower than foreach. \$\endgroup\$ – victor Nov 20 '14 at 4:52
  • 1
    \$\begingroup\$ As I said, your code looks good as it is. The LinQ option was just another way of achieving the same result. I've worked quite a bit with it so for me it's very readable. The extension method might indeed be overkill but it's a nice feature and I always have expansion in mind. That's why I often use (extension) methods. \$\endgroup\$ – Abbas Nov 20 '14 at 7:08
4
\$\begingroup\$

It looks rather hacky because I have the feeling there must be someplace down the line where you can more easily determine whether or not the properties are filled in.

Nevertheless: right now your code will only work for strings while it is probably realistic to say that there might be other fields in the future as well (Id, DateOfBirth, etc) which would require a code change. Therefore I would suggest making your code a little less string-specific:

public bool isEmpty()
{
    var properties = this.GetType().GetProperties(BindingFlags.Public | BindingFlags.Instance);

    foreach (var prop in properties)
    {
        var value = prop.GetValue(this, null);

        if (value != default(prop.PropertyType))
        {
            return false;
        }

        var stringValue = value as string;
        if(stringValue != null)
        {
            if(stringValue != string.IsNullOrWhiteSpace(value))
            {
                return false;
            }
        }
    }

    return true;
}
  • Using default so it words with primitive types (they won't be able to be 0 with this)
  • If you do want to be able to be 0, make them nullable types instead
  • as combined with != null
  • IsNullOrWhiteSpace(string) instead of comparison to empty quotes
  • Removed t since it was only used once
\$\endgroup\$
  • \$\begingroup\$ Thanks for the stackoverflow link. It looks hacky for me too, that's why I posted it here; however, the fact is that there is no place down the line where it would be easier to check the props because I need to check all of them, and without reflection there is no way to know which properties the class has, except by hard coding them "x.name not null && x.lastname not null etc"... does isNullOrWhiteSpace fails for empty string as well? \$\endgroup\$ – victor Nov 20 '14 at 14:07
  • 1
    \$\begingroup\$ I figured maybe you could check the ModelState somewhere combined with [Required] attributes, but that depends on your situation of course. Yes, it does. \$\endgroup\$ – Jeroen Vannevel Nov 20 '14 at 14:09

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.