Is it a good idea to return a default value if a field in a query cannot be found?

Question

At the place I work we are using a SafeReader class that wraps an IDataReader. One of the 'features' is that if the field you are trying to access wasn't in the query then it just returns a default value.

So I am working with some code that is already in place, but apparently never fully worked as it accesses fields that aren't returned from the stored proc it's calling but it didn't throw any exceptions since the SafeReader kindly returned default values.

I personally don't like this because it means that when there are mistakes in the code (someone forgets to include a field in the query, or perhaps a misspelling) then it's harder to catch.

My question is whether this violates any general 'best practices' of development or object oriented programming ideals? Or does this seem perfectly reasonable?

Answer 1

It depends if the default value is the right value or not. I see that pattern way too often when it just happens to be the fastest way to make a particular use case work. It just makes it harder to debug other use cases down the road.

What do I mean by the default value being the right value? Well, take an integer field for example. What should it default to? If you said you don't know, give yourself a cookie. If you said zero, that's the right answer for, say, "number of upvotes," but not "seconds until self destruct."

In other words, a correct default value is specific to the field and can be used in all use cases without checking for it. If I have to pepper my code with if IntegerField == -1, the default not only isn't buying me anything I couldn't get with null, or even better an option or exception, it's actually hurting me because the check is easier to forget and serious errors are easier to ignore. Why would you forego using the tools which are specifically designed to help programmers in this situation?

However, if I can just write Upvotes += 1 without worrying about if that field has been used before, and know that it will produce the correct result due to a zero default, then the default is buying me something worthwhile.

The problem with the SafeReader class you describe is it is making assumptions about fields it knows nothing about. I can think of use cases where it might be handy to include defaults for fields not in a query, but they all involve some other form of validating field names, such as against a schema.

I personally consider anything named SafeWhatever a code smell. The name strikes me as somewhat Orwellian, because the safety is almost always an illusion. It's like taking down guard rails so a cliff road won't look so dangerous.

Answer 2

My question is whether this violates any general 'best practices' of development or object oriented programming ideals?

Sometimes it does. Sometimes, accessing a column that doesn't exist is a clear disconnect between the implied contract that is implemented by the data reader between the calling code and the data source. If it is a clearly a problem, then returning default is violating the "fail early, fail obviously" best practice.

But sometimes it doesn't. In something like a web service, not including a column is done as an implied "not specified" or "use default" (see EmitDefaultValue on DataMember for example). This reduces the payload of web service calls. Having unincluded columns provide a default is much cleaner here since it allows you to obey DRY (don't repeat yourself) rather than checking every single data access call.

Personally, I would avoid having your stuff working with a DataReader or its kin directly where possible. This entire problem exists because you're using a glorified dictionary rather than a real object. That is the real violation of OO ideals. Everything else is a trade off based on your particular situation.

Answer 3

No, it is not a good idea in general. It is a very bad idea.

For SOME things, it will be OK to return a default value, but this has to be resolved on a case-by-case basis for that particular thing. In order be able to do that case-by-case resolution, you have to provide for the case where it is NOT OK to return a default value.

Consider for example an employee badge system. All employees are by default allowed entry into a particular facility, but only some had access to certain rooms in the facility. The database is engineered to return the allowed access value when searched by employee number. It would be tempting in this case to return a default value, and only store those badge numbers that had HIGHER levels of access. This immediately fails, when Ivan Ivanovitch of the GRU (or whoever the current Russian external spook bureau is called) presents a counterfeit badge with the name "John Johnson" and a bogus employee number, and the badge reader opens the front door for him. (Yes, I'm giving away my age here.)

My personal experience is that, if your language supports exceptions, you throw an exception in the "not found" case, and FORCE the next guy up to provide an exception handler. Extensive experience in the industry demonstrates beyond reasonable doubt that the vast majority of C and C++ programmers never check function return codes. (For that matter, too many of them fail to check pointers for non-null before dereferencing them, even when their employer has hard-and-fast written coding standards requiring such checks. Yes, that sentence IS written in blood, thank you for asking.)