views:

155

answers:

4

I have a set of classes with the same functions but with different logic. However, each class function can return a number of objects. It is safe to set the return type as the interface?

Each class (all using the same interface) is doing this with different business logic.

protected IMessage validateReturnType; <-- This is in an abstract class

public bool IsValid() <-- This is in an abstract class
{
    return (validateReturnType.GetType() == typeof(Success));
}

public IMessage Validate()
{
    if (name.Length < 5)
    {
        validateReturnType = new Error("Name must be 5 characters or greater.");
    }
    else
    {
        validateReturnType = new Success("Name is valid.");
    }

    return validateReturnType;
}

Are there any pitfalls with unit testing the return type of an function? Also, is it considered bad design to have functions needing to be run in order for them to succeed? In this example, Validate() would have to be run before IsValid() or else IsValid() would always return false.

Thank you.

+10  A: 

However, each class function can return a number of objects. It is safe to set the return type as the interface?

This is good practice and common. For example look at the way COM was built, it heavily relies on this methodology.

Are there any pitfalls with unit testing the return type of an function

No.

Also, is it considered bad design to have functions needing to be run in order for them to succeed? In this example, Validate() would have to be run before IsValid() or else IsValid() would always return false.

That's fine for working with an object oriented programming paradigm, take for example working with sockets. It's common to have a connect method before you can send and receive data.

That being said it's good to keep less state than more state as a general rule because in that way it's easier to prove your program is correct. For example you have to test each function that relies on this function not in one way but in 2 ways now. Possible program states grows exponentially if you have a lot of state. Take a look at functional programming if you are interested in why state is a bad thing.

Brian R. Bondy
+1 for the mention and explanation of states, and the sockets example. I've done a lot of socket wrappers, and the states are a pain.
Daniel Rasmussen
It's also a fairly common pattern to have IsValid call Validate() if there have been changes to object state since Validate() was last called. There are various ways to mark the object dirty (requiring revalidation). The one I favor is to implement INotifyPropertyChanged on my domain objects.
Eric J.
+1 for the general rule of less state, i agree heavily
Bob Fincheimer
+1  A: 

Having an interface as return type is good, because your method will be compatible with more types of messages.

Sander Pham
+3  A: 

Also, is it considered bad design to have functions needing to be run in order for them to succeed? In this example, Validate() would have to be run before IsValid() or else IsValid() would always return false.

There are exceptions, but in general you should avoid building this kind of API. This is called a "temporal dependency," which is just a fancy term for "one thing is required to happen before the other." The problem with temporal dependencies is that they are seldom self-describing. In other words, the API does not communicate by itself how it is intended to be used. API's that rely on temporal dependencies are often harder to intuitively understand and use than similar code.

In your example, I'd consider refactoring the API with the following design goals:

  1. Get rid of IsValid altogther.
  2. Validate return a list of validation errors.
  3. Redefine "success" as having Validate() return an empty list devoid of errors.

If you simply must have IsValid, redefine it to check #3. If you want to cache the results so that you don't have to recalculate IsValid constantly, consider implementing INotifyPropertyChanged in and invalidating the cached result on PropertyChanged.

Chris McKenzie
Sometimes there are complex situations in which object initialization cannot be finished in a single step. In these situations, one solution is to separate initialization from object construction. These techniques are commonly found in COM. If more than one initialization step is needed, it is suggested to decompose the initialization process into multiple objects, or to develop a clearly documented protocol for such initialization.
rwong
+1  A: 

Returning interfaces is good in general because it hides the implementation. (Of course, you should not hide more than necessary...)

In the case of your example though, I'm a bit worried that you want to unit-test the actual return type of your function, while the point of using interfaces is precisely to hide it. Looks like your actual code will rely on the actual return type, and that smells bad.

I'd better add an abstract method IsValid() to IMessage, override it in Success and Error, and call validateReturnType.IsValid() directly. Sounds more OO to me. The very goal of OO and virtual methods is to avoid type-based case switching like what you do in your IsValid().

Thanks for your reply. I do like your idea however wouldn't I still be testing the return type of IsValid()?
Mike
@Mike of course yes. But the implementation details of IsValid (namely the Success/Error subclasses) will be hidden so you won't depend on them: your code will be more readable and maintainable. That actually depends on what you want to do with all this, and what additional features the Success/Error subclasses offer. If Success/Error offer nothing more than their IsValid() method, then probably a simple Message class with a simple readonly isValid field would do (and would then be my preferred solution).
I agree but still aren't exactly sure how to refactor my code. So say my model has 20+ fields. I mean, you can define IsValid() as returning true if there are no errors. But even if there aren't any errors there could still be a warning. The only thing these 3 states are used for is to determine the background color. Each state class has one read-only property, cssClass, which is used to determine that CSS class to apply to the cell. Also, is there a specific term for this type of issue?
Mike