Ayende @ Rahien

It's a girl

What is wrong with this API? Answer

Originally posted at 11/11/2010

The reason that I don’t like this API is that I think it provides a piss poor usability:

image

In short, it doesn’t provide me with any reasonable way to tell the user why I rejected a message.

Here is the proper way of doing this:

image

MessageVeto also contains a reason string, which provide a much better experience all around.

Comments

Rafal
11/13/2010 10:35 AM by
Rafal

I thought veto is about rejecting something without having to tell why. But maybe I didn't agonize long enough on this line of code...

Miguel Lira
11/13/2010 11:01 AM by
Miguel Lira

At first glance I had the same assumption Rafal did - I thought the bool could have simply been a void. Then I figured you'd prefer more context if it actually performed a returnable action on the message.

Coincidently, your answer implies the type returned is an enum. How are you planning to provide a reason string? via some implied annotation?

gecka
11/13/2010 11:37 AM by
gecka

It can also be a class with static properties like:

public class MessageVeto

{

public static Allowed

{

    get

    {

        return new MessageVeto(allowed: true, "Some message");

    }

}


private MessageVeto(bool allowed, string message)

{

    ...

}


...

}

Jason Young
11/13/2010 12:51 PM by
Jason Young

Returning something that can indicate the reason is indeed better. I thought most of the problems / solutions listed in the comments in the last post were good as well.

Scooletz
11/13/2010 03:37 PM by
Scooletz

Funny thing,

plenty of people tried to find a must of having everything strong-typed. Strong-typing is cool, but considering that there is sth wrong when using a primitive type for some result, I don't buy it:P

Kelly Summerlin
11/13/2010 03:53 PM by
Kelly Summerlin

I like the enum/class with a static property over the boolean value because it is more extensible. Today there may only be two values yes/no, true/false. With the enum you allow for the possibility of additional values without having to change the clients -- unless you really need to .

Jeff C
11/13/2010 06:49 PM by
Jeff C

So, to NOT veto a message, you return a MessageVeto? That's kind of confusing, as well.

NC
11/13/2010 11:04 PM by
NC

I don't get it now.

Brian Chavez
11/14/2010 12:11 AM by
Brian Chavez

I agree with Jeff,

MessageVeto.Allowed makes absolutely no sense.

Just the fact you have "Veto.Allowed" read on the same line to express some kind of action... is like saying "this.TempFeels = Hot.Cold".

"Oxymoron: A figure of speech in which incongruous or contradictory terms appear side by side." Indeed.

Brian Chavez
11/14/2010 12:22 AM by
Brian Chavez

I think it's better to go with actual your intention is:

EnqueueAction.Allow

EnqueueAction.Veto

Seems better to me.???

Brian Chavez
11/14/2010 12:38 AM by
Brian Chavez

I also like:

EnqueueVote.Allow

EnqueueVote.Veto

MessageVote.Allow

MessageVote.Veto

Or something more generic:

QueueVote.Allow

QueueVote.Veto

That's neat. Naming is cool.

Luke Schafer
11/14/2010 11:05 PM by
Luke Schafer

How about something like

//MessageAllowed with an internal constructor

MessageAllowed.Yes

MessageAllowed.No

MessageVeto : MessageAllowed {

public MessageAllowed Veto  { 

    get {

        return new MessageAllowed(MessageAllowedOptions.No, "Veto");

    }

 }

}

Rob Kent
11/15/2010 11:52 PM by
Rob Kent

I just think it would be better to use a positive method and a verb that is more common than veto, like:

public virtual MessageAllowed AllowMessage(IncomingMessage msg)

{

return MessageAllowed.Yes;

}

Jonas Stawski
11/16/2010 07:07 PM by
Jonas Stawski
This reasoning is always true with almost every API call that returns a boolean or some sort of notification to the user. Take into consideration an API call that updates some information for an entity. Usually we return true or false for specifying whether the action was successful or not, but we don't know why it wasn't successful. Was it because the record was not found, because of a DB error, because of a network error? 
  
  
So I always try to return some type of ReturnValue
<bool that returns whether it was successful plus any other type of information to let the caller take action on what to do next if it fails.
>
Redober
11/17/2010 07:30 AM by
Redober

Thanks to implicit constructors you can refactor the API without any breaking changes at callsites. I had a certain case recently with a constraints callback.

Max Schilling
11/23/2010 01:50 PM by
Max Schilling

Hey Ayende,

Instead of creating new "result" objects each time I need a method to return a reason, I use the following Result object that I keep in my common library.

It creates a generalized way of solving the problem you are addressing.

I have a blog post written about it here: http://maxoncode.com/ though it is pretty straightforward.

I also use it to encapsulate any exceptions. I know it is somewhat non-standard, but for general shared code I don't necessarily know how the calling code wants to do exception handling, so if something goes awry, I'd just set the Result.Value to false, and populate the Result.Exception property, return the result and let the calling code decide whether it needs to throw or not. If nothing else it makes unit testing the method much cleaner.

public class Result : Result <object,>

{

}

public class Result <t : Result <t,>

{

}

/// <summary
/// Wraps an object to allow a function to return additional data about its execution to the caller

///

/// <typeparamThe type of Item. This is the "return type" for the result object.

/// <typeparamThe type of ResultCode. This is an optional spot for an enumeration of possible ResultCodes.

public class Result <t1,>

{

public Result()

{

    Message = String.Empty;

}


public bool Value { get; set; }


public T1 Item { get; set; }


public T2 ResultCode { get; set; }


public string Message { get; set; }


public Exception Exception { get; set; }

}

Comments have been closed on this topic.