checking an array of string objects for null values

Question

This is a fairly basic question I think but either my brain hasn't woken up yet or I'm just being thick!

I have a class that has a property of a collection of strings defined below (names are simplified)

public class IdentifierCollection : BaseSubCollection, IIdentifierCollection
{
    public string Id1{ get; set; }
    public string Id2{ get; set; }
    public string Id3{ get; set; }
    // ...      
}

I want to check if any of the properties actually have a value before saving so I am currently doing something like this...

if (string.IsNullOrEmpty(primaryObj.Identifiers?.Id2) &&
    string.IsNullOrEmpty(primaryObj.Identifiers?.Id2) &&
    string.IsNullOrEmpty(primaryObj.Identifiers?.Id3) &&
    string.IsNullOrEmpty(primaryObj.Identifiers?.Id4) &&
    string.IsNullOrEmpty(primaryObj.Identifiers?.Id5) &&
    string.IsNullOrEmpty(primaryObj.Identifiers?.Id6) &&
    string.IsNullOrEmpty(primaryObj.Identifiers?.Id7) &&
    string.IsNullOrEmpty(primaryObj.Identifiers?.Id8))
{

}

Just typing this feels wrong!! There must be a better way...

Why do you have numbered properties? Why not use a dictionary or list? You could use reflection to find and inspect the properties though, if you can't lose them. — CodeCaster, Feb 25 '16 at 10:23
@CodeCaster I'm wondering if that's the effect of _"(names simplfied)"_. If it _isn't_, then there may well be a very different answer to the Q, ie what you're suggesting... — James Thorpe, Feb 25 '16 at 10:25
Yeah it is, the actual names are what I'm working on which is vaguely confidentual — dreadeddev, Feb 25 '16 at 10:26
But using reflection would be more expensive than just ugly code... or not? — dreadeddev, Feb 25 '16 at 10:27
Do you actually access these properties by name somewhere, or is a dictionary still an option? And define "expensive". Yes, reflection is "relatively slow", but it is backed by the runtime caching various objects. — CodeCaster, Feb 25 '16 at 10:27
If `IIdentifierCollection` hints that is a collection why does it not implement IEnumerable? You could then just iterate over it with a foreach or something. — Neijwiert, Feb 25 '16 at 10:28
Yeah they are used and they make up the Poco and JSON objects that are then saved to DocumentDb — dreadeddev, Feb 25 '16 at 10:28
That might be a better solution @Neijwiert... will try it now, feel free to add it as an answer! — dreadeddev, Feb 25 '16 at 10:29
@Neijwiert keeping in mind that the OP wants to keep the properties there: how can you iterate over a singular class with properties? You can create an enumerator that iterates over the properties and returns their values, but then you're back to square one. — CodeCaster, Feb 25 '16 at 10:39
How many times do you call this method (checking any of the properties being null) and how many such objects do you have? Are there different types of configurations: e.g. checking only some of the properties being null based on some criteria, etc.? — Tamas Ionut, Feb 25 '16 at 10:43
@CodeCaster I don't see any other 'clean' solution. If they are separate member variables. Reflection don't seem clean to me. If you force the coder to implement the iterator then it is going to work for every newly made class that implements IIdentifierCollection. — Neijwiert, Feb 25 '16 at 10:43
@Neijwiert "doesn't seem clean" is an opinion. Reflection is more fool-proof than a hand-coded enumerator that contains of just `yield return Id1; yield return Id2;` and so on, because it is error-prone. — CodeCaster, Feb 25 '16 at 10:44

score 1 · Accepted Answer · answered Feb 25 '16 at 11:38

I don't think there is anything wrong with this kind of property checking. Using reflection or implementing some interface just to be able to iterate over the properties looks like overkill to me. Though I agree that such a long statement looks awkward as a conditional check. To make the code more readable I'd extract this check to a separate private method.

if (NoPropIsNullOrEmpty())
{
}

private bool NoPropIsNullOrEmpty()
{
    return !(string.IsNullOrEmpty(primaryObj.Identifiers?.Id2) ||
             string.IsNullOrEmpty(primaryObj.Identifiers?.Id2) ||
             string.IsNullOrEmpty(primaryObj.Identifiers?.Id3) ||
             string.IsNullOrEmpty(primaryObj.Identifiers?.Id4) ||
             string.IsNullOrEmpty(primaryObj.Identifiers?.Id5) ||
             string.IsNullOrEmpty(primaryObj.Identifiers?.Id6) ||
             string.IsNullOrEmpty(primaryObj.Identifiers?.Id7) ||
             string.IsNullOrEmpty(primaryObj.Identifiers?.Id8));
}

I'm tempted to agree, the BaseCollection does implement an indexer so that I can access the properties as if they were an array, so the IEnumerable made sense but it ends up being overkill to avoid a few ugly lines of code... I've taken it out to a helper method already so I guess I'm just being too picky!!! — dreadeddev, Feb 25 '16 at 12:14
_"I don't think there is anything wrong with this kind of property checking"_ - it's copy-paste code, so subject to all drawbacks thereof. — CodeCaster, Feb 25 '16 at 12:28

checking an array of string objects for null values

1 Answers1