6

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?

4
  • it depends, sometimes a default is good and sometime a missing field is a serious error – ratchet freak Jul 3 '14 at 15:45
  • 1
    It smells bad to me... there is a "fall early" guideline that states that your code should fail (hopefully in a controlled way) as soon as it gets an unexpected state. It should be the IDataReader client who knows if a missing field is something that can be expected/managed or not. – SJuan76 Jul 3 '14 at 15:48
  • Null Pointers vs Null Object pattern is closely related to what you're asking, but I'm a bit hesitant to call it an outright duplicate. – user53019 Jul 3 '14 at 15:50
4

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.

1
  • Thanks, you did a great job of expressing exactly what I was feeling but hadn't put into intelligible thoughts yet... that there are cases where a default makes sense but that's when the defaults are assigned by something that understands what it's working with which is not the case here. – BVernon Jul 3 '14 at 18:05
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.

7
  • There's an impedance mismatch between objects and relation tables so there's not really any way to get around using a DataReader (or equivalent) to move data from the query results into proper objects. – BVernon Jul 3 '14 at 17:31
  • @BVernon - sure, but you can isolate that into a basic translation layer that is dead simple to test. – Telastyn Jul 3 '14 at 17:32
  • It is in a translation layer. If a default value is a valid value for a property and it automatically stuffs properties with default values when their correlating field can't be found then how are you going to test that? The point of stuffing it with a default value is so that it won't throw an exception, but my argument is that it 'should' throw an exception... otherwise your tests are going to make it look like it works even when it's not right. – BVernon Jul 3 '14 at 17:58
  • Unless you're suggesting that our tests should write to the database and then read the values back in to make sure they're what we expect. And that would be great except that I'm dealing with code that I didn't write and I don't have time to go back and make unit tests for everything. – BVernon Jul 3 '14 at 18:00
  • @BVernon - It's pretty trivial to just step through code that points to a real database row with real (non-default) values and see if your data object is right. Unit tests can be harder since you're working at the lowest layer, but still. – Telastyn Jul 3 '14 at 18:50
1

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.)

7
  • In the context of data readers, it is especially dangerous to return default values for value-types (e.g., default(int)). Whereas default values for reference types will tend to eventually throw NullReferenceException, default values on value types will often not trigger visibly broken behavior quite so immediately. E.g., int NewId = getProductId(arg);setProductLabel(NewId,NewLabel); might very well silently corrupt (or discard) your data without any sort of warning or error. Database constraints can mitigate this issue, but cannot eliminate it entirely. – Brian Jul 3 '14 at 16:17
  • If you want to avoid nulls, returning a "default" value is really the only way to go. (And the way to fix your example is for the default value of "canOpenDoor" to be false, same as if the employee was terminated or telecommute-only.) – DougM Jul 3 '14 at 16:22
  • 1
    @DougM Returning a "default" value isn't the only option - see F#'s Option type. The key insight is recognizing that value of type T, or Nothing is a distinct type from T. For the more general version see How do you encode Algebraic Data Types in a C#- or Java-like language?. – Doval Jul 3 '14 at 17:42
  • I would suggest that it's often helpful and safe to have a "get something or return a caller-supplied default method if there is also a "get something or throw exception" fallback. If the caller has to supply the default, the called method should have no problem knowing a "safe" default for the context in which it is called. – supercat Jul 10 '14 at 23:56
  • @supercat: Observe that there is no real semantic difference between what you suggest and what I espouse. The only difference is how the caller supplies the default returner. In your method, you pass a callback method with your key when you call the get(). In mine, you wrap an exception catcher around the get(), and call the callback method yourself if the exception is thrown. Observe that it is usually easier to track down an unhandled exception than it is a branch to Never-Never Land. (Worst case is when a branch through a null pointer yields a valid code address: you die screaming.) – John R. Strohm Jul 11 '14 at 4:23

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.