Business Validation Logic Code Smell

Question

Consider the following code:

partial class OurBusinessObject {
    partial void OnOurPropertyChanged() {
        if(ValidateOurProperty(this.OurProperty) == false) {
            this.OurProperty = OurBusinessObject.Default.OurProperty;
        }
    }
}

That is, when the value of OurProperty in OurBusinessObject is changed, if the value is not valid, set it to be the default value. This pattern strikes me as code smell but others here do not agree. What are your thoughts?

Edited to add: I've been asked to add an explanation for why this is thought to be okay. The idea was that rather than having the producers of the business object validate the data, the business object could validate its own properties, and set clean default values in cases when the validation failed. Further, it was thought, if the validation rules change, the business object producers won't have to change their logic as the business object will take care of validating and cleaning the data.

Answer 1

I think I have to agree with you. This could definitely lead to issues where the logic unexpectedly returns to the defaults, which could be very difficult to debug.

At the very least, this behavior should be logged, but this seems more like a case for throwing an exception.

Answer 2

It absolutely horrible. Good luck trying to debug issues in Production. The only thing it can lead to is to cover bugs, which will just pop up somewhere else, where it will be not obvious at all where they are coming from.

Answer 3

Distaste for the term 'code smell' aside, you might be right - depending on where it's coming from, silently changing the value is probably not a good thing. It would be better to ensure your value is valid instead of just reverting to the default.

Answer 4

It may or may not "smell", but I'm leaning more towards, "Yes it smells".

Does setting OurProperty to the default have a logical reason for doing so or is it simply convenient to do so in code? It is possible, however unlikely in practice, to contrive a scenario where this would be expected behavior, but I'm guessing that in most cases you should be throwing an exception and handling it cleanly somewhere.

Does setting the value to default get you closer to or move you away from the functional specifications description of how the application is supposed to work?

Answer 5

To me this looks like the symptom, rather than the actual problem. What's really going on is that the setter for OurProperty fails to preserve the original value for use in the OnOurPropertyChanged event. If you do that, suddenly it becomes easier to make better choices about how to proceed.

For that matter, what you really want is an OnOurPropertyChanging event that is raised from the setter before the assignment actually takes place. This way you can allow or deny the assignment in the first place. Otherwise there is a small amount of time where your object is not valid, and that means the type is not thread safe and you can't count on consistency if you you consider concurrency is a concern.

Answer 6

You are validating a change after it has been done? Validation should be done before the busyness property is altered.

Answering your questing: the solution presented in that code snippet can generate big issues in production, you don't know whether the default value appeared there due to invalid input or just because something else set the value to the default

Answer 7

Definitely a questionable practice.

How would an invalid value ever get assigned to this property? Wouldn't that indicate there's a bug somewhere in the calling code, in which case you'd probably want to know right away? Or that a user input something incorrectly in which case they should be informed right away?

In general, "failing fast" makes tracking down bugs a lot easier. Silently assigning a default behind the scenes is akin to "magic" and is only going to cause confusion to whoever has to maintain the codebase.

Answer 8

I would highly recommend refactoring it to validate before setting the property.

You could always have a method that was more like:

T GetValidValueForProperty<T>(T suggestedValue, T currentValue);

or even:

T GetValidValueForProperty<T>(string propertyName, T suggestedValue, T currentValue);

If you do that, before you set the property, you could pass it to the business logic to validate, and the business logic could return the default property value (your current behavior) OR (more reasonable in most cases), return the currentValue, so setting had no effect.

This would be used more like:

T OurProperty
{
    get
    {
        return this.propertyBackingField;
    }
    set
    {
        this.propertyBackingField = this.GetValidValueForProperty(value, this.propertyBackingField);
    }
}

It doesn't really matter what you do, but it is important to validate before you change your current value. If you change your value before you determine whether the new value is good, you're asking for trouble in the long term.

Answer 9

It's hard to say without knowing the context or business rules. Generally speaking though, you should just validate at time of input, and maybe once more before persistence, but the way you're doing it won't really allow you to validate since you're not allowing a property to contain an invalid value.

Answer 10

I think your validation logic should raise an exception if asked to use an invalid value. If your consumer wants to use a default value, it should ask for it explicitly, either through a special, documented value or through another method.

The only kind of exceptions I can think would be forgivable would be, like, normalizing case, like in email fields to detect duplicates.

Answer 11

Furthermore, why in the world is this partial? Using partial classes for anything but a generated code framework is itself is a codesmell since you're likely using them to hide complexity which should be split up anyways!

Answer 12

I agree with Grzenio and would add that the best way to handle a validation error down in the domain layer (aka business objects) is to generate an exception. That exception could propagate all the way up into the UI layer where it could be handled and interactively rectified with the user. However, depending on the capabilities and technologies involved, this may not always be feasible, in which case, you probably should be validating up in the UI layer (possibly in addition to the domain layer). It's less than ideal, but might be your only viable option. In any case, setting it to a default value is a horrible thing to do and will lead to subtle bugs that will be near impossible to diagnose. If done on a broad scale, you'll have an unmaintainable system in no time (especially if you have no unit tests backing you up).

Answer 13

An argument that I have against this is the following. Suppose the user/producer of the business object accidentally inputs an invalid value. Then this pattern will gloss over that fact and default to clean data. But the right way to handle this is to throw an error and have the user/producer verify/clean their input data.

Answer 14

I'd say, implement PropertyChanging and allow the business logic to approve/deny a value, and then afterwards, throw an exception for invalid values.

This way, you don't ever have an invalid value. That, and you should never change a user's information. What if a user adds an entry to the database, and keeps track of it for his own records? Your code would re-assign the value to the default, and he's now tracking the wrong information. Its better to inform the user ASAP.