Maybe that shouldn’t be settable

I think this is pretty, and I wanted to share. I like classes where the compiler ensures they are always in a valid state—you can’t help but use them correctly.

I was writing a class that needed to call a third-party service and then return a result that either indicated there was an error (with the error reason) or reported success and parsed the JSON into a collection of objects. The Result class could have been implemented as a dumb property bag:

public class Result<T>
{
    public bool IsSuccess { get; set; }
    public string ErrorMessage { get; set; }
    public IEnumerable<T> Results { get; set; }
}

The code that calls the service and constructs the result would set properties accordingly:

public class MyCoolService
{
    public Result<CoolThing> Fetch(Uri uri)
    {
        HttpResponseMessage response = CallThatThingInTheCloud(uri);

        if (response.IsSuccessStatusCode)
        {
            return new Result<CoolThing>
                {
                    IsSuccess = true,
                    Results = Deserialize(response)
                };
        }

        return new Result<CoolThing>
            {
                IsSuccess = false,
                ErrorMessage = string.Format("Error {0}: {1}", 
                                response.StatusCode, response.ReasonPhrase)
            };
    }
    //...
}

The syntactic brevity of auto-properties lulls us into writing { get; set; } without thinking about it, providing public accessors without considering whether or not we’re flouting encapsulation. We lose the protection and power that properties had granted us, and might as well be using public fields. Talk about anemic classes.

The pitfall, then, is that consuming code has to check the IsSuccess property before accessing other members. If you try to use the Results collection when IsSuccess is false, you’ll get a null reference exception. And what if some code at runtime changed one of those properties, what would that even mean? If a Result had both an error message and a set of parsed results, that would mean… um…

Okay, I promised pretty. What did I do instead?

public class Result<T>
{
    public bool IsSuccess { get; private set; }
    public string ErrorMessage { get; private set; }
    public IEnumerable<T> Results { get; private set; }

    private Result()
    { }

    public static Result<T> Success(IEnumerable<T> results)
    {
        return new Result<T>
        {
            IsSuccess = true,
            Results = results
        };
    }

    public static Result<T> Success(T result)
    {
        return Success(new[] { result });
    }

    public static Result<T> Error(int statusCode, string reasonPhrase)
    {
        return new Result<T>
        {
            ErrorMessage = string.Format("Error {0}: {1}", statusCode, reasonPhrase),
            Results = Enumerable.Empty<T>()
        };
    }
}

The constructor of the class is private. So are the setters on its properties. You can only get at this thing through the entry points I’m giving you. And those entry points are:

  • Make a Success response with a collection of results.
  • Make a Success response with only one result, but wrap it up as a collection-with-one-item so that consumers can just iterate over Results without worrying about what the third-party service returned.
  • Make an Error response with a consistently formatted error message.

It’s like a memo to my teammates with instructions for how to use this class. (It’s like a ///<comment> without being a stupid comment.)

Consumers can even safely call foreach over the Results collection without checking whether the service call was successful or not. “Display all the ones you got, and if you didn’t get any, whatevs.”

Here’s what the consuming class looks like:

public class MyCoolService
{
    public Result<CoolThing> Fetch(Uri uri)
    {
        HttpResponseMessage response = CallThatThingInTheCloud(uri);

        return response.IsSuccessStatusCode
                    ? Result<CoolThing>.Success(Deserialize(response))
                    : Result<CoolThing>.Error(response.StatusCode, response.ReasonPhrase);
    }
    //...
}

Briefer and clearer. #success

Code that uses this class needs less test coverage. There’s no wrong way to eat a Reese’s, and there’s no wrong way to construct this class. Therefore, I don’t need to test the code that is constructing it (“Make sure that IsSuccess gets set to false…”). The only thing I might get wrong is calling .Success() when I should have called .Error(), but really. And less test coverage, on code whose correctness is compiler-enforced, means the rest of the code is more amenable to refactoring.

The general concept here is encapsulation. Challenge yourself to make property setters private by default. Move property-setting logic into the class itself. Let the class protect itself from getting into an invalid state. You get to write a lot fewer if statements that way, and your code becomes easier to read.

Related Articles:

About Sharon Cichelli

I am a Headspring Senior Consultant, developing custom enterprise software for our clients and leading training classes on the latest Microsoft technologies. I blog about .NET development, best practices in agile software development, and my nerdy hobbies.
This entry was posted in refactoring. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • Rémi BOURGAREL

    You also can use polymorphism here, but it might be too much.

    I use to declare properties with private accessor when i’m feeling like i’m building an anemic domain model. With this practice, every domain state change has a domain explanation.

    But I don’t see how it can make your unit test easier, you still have your condition, the execution tree is the same.

    • scichelli

      “Every domain state change has a domain explanation” Oh, I like that. :)

      You’re right that the execution tree hasn’t changed. What I was thinking of was the MyCoolService.Fetch() test that mocks the third-party service and verifies MyCoolService sets the right properties on the result it’s returning. The logic is still there, but I think a test that checks whether the Fetch method called Result.Success or Result.Error isn’t worth keeping. Either the right one is called, or you notice the moment you run your app for real, and it isn’t domain behavior you need to document or nail down lest a teammate unwittingly breaks it.

    • MBR

      >You also can use polymorphism here, but it might be too much.

      That’s where F#/Scala/Haskel/etc shine – when polymorphic types are easy. In the F# case:

      type ‘t Result = Error of (int,string) | Result of ‘t seq

      or if you like, the more familiar:

      type Result = Error of (int,string) | Success of IEnumerable

      This creates a class hierarchy of three classes, constructors, etc.
      Then:

      let fetch uri =
      let resp = CallThatThingInTheCloud(uri)
      if resp.IsSuccessStatusCode
      then Deserialize(resp) |> Success
      else Error (response.StatusCode, response.ReasonPhrase)

      Then the client uses pattern matching to handle each possible return type – the compiler will complain if the user doesn’t consider everything:

      match fetch uri with
      | Success stuff -> doSomethingWith stuff
      | Error (code,msg) -> showError code msg

      • scichelli

        This is so cool! I haven’t read much F# syntax, so it’s neat to see how much sense it makes. How fun.

  • Frank Black

    I like this – giving something a little more thought results in an imminently more usable and understandable code.

    • scichelli

      Thanks. :) I’ve been on a refactoring tear at work lately, and I get kind of giddy when I rip out three huge foreach loops in favor of some linq and encapsulation. “Usable and understandable” make me smile.

  • Piotr Perak

    I like the refactoring but I don’t see how you removed the need for ifs. Your Service contains if as it did before (in a form of ? operator). And clients of this Service still need ifs to check whether Error occurred to either display error or results. Just to add what you achieved could be achieved with two constructors but wouldn’t look as nice.

    • scichelli

      I was thinking clients, in the case where a successful service call is nice but not required, e.g., showing most recent tweets. But yes, in most cases you’re probably still checking the state of Result.IsSuccess.

  • Daniel Marbach

    Public static methods on generic types is as ugly us it can get because you have to specify the generic type parameter all the time. I’d rather use a builder as an entry point which is non generic but return generic result.

    • uvwu

      A builder with two mutually exclusive methods (Success/Error) seems to be overkill.

      I think a static non-generic ‘companion’ class would be much better here:

      public static class Result

      {

      private const int UNKNOWN_STATUS_CODE = -1;

      private const string UNKNOWN_STATUS_PHRASE = @"WAT?";

      public static Result Success(T result = default(T))

      {

      return Result.Success(new[] { result });

      }

      public static Result Error(int statusCode = UNKNOWN_STATUS_CODE,

      string reasonPhrase = UNKNOWN_STATUS_PHRASE)

      {

      return new Result.Error(statusCode, reasonPhrase);

      }

      }

      And with a neat type inference for the Success() method :)

      public static Result ToResult(this HttpResponseMessage response)

      {

      return response.IsSuccessStatusCode

      ? Result.Success(Deserialize(response))

      : Result.Error(response.StatusCode, response.ReasonPhrase);

      }

      • uvwu

        My apologies for the formatting, used pre/code through bad habit :(

        • scichelli

          Oh, that’s unfortunate. I never know what blog-comment platforms are going to do with code. I turned these two snippets into a gist; feel free to comment with any corrections if I didn’t transcribe correctly.

          https://gist.github.com/scichelli/8419568

  • Duncan

    But the default .NET serializer does not serialize properties without a public get and set function.

  • Mike

    This works really well, until you want to use the entity in a web api / web service / entity framework. Then without a public setter all the serialization fails

    • http://nikolar.com/ Nikola Radosavljević

      So true. However, this is not a business entity but a helper wrapper for returning results together with success indicator. For me it’s either this or returning an anonymous object in some cases. This just shims in some constraints instead of a convention.

  • Carlos Ribas

    Hi Sharon! I do this a lot these days. One note would be to use a backing field and actual readonly fields rather than private settable properties. You just get a little protection within the class itself, then, such that you don’t accidentally write code that mutates a success or an error into a succerror by using that private setter.

    When the answer to “is this readonly?” is “yes,” I always use an actual read-only semantic — in this case, a readonly backing field. Yeah, it’s more lines of code in my class, but it is lines I only write once, and they are easy (R# can write them for you, if you type the auto-property first) to write.

    I think the value of the more accurate readonly semantics will pay back those few extra lines of code as the system is maintained. Also, to address some of other comments: you shouldn’t be serializing anything other than DTOs unless you want to hurt yourself later (API versioning, more than one surfacing service with different serialization concerns, etc) so that doesn’t really apply here :) And yes, your DTO would just be a dumb property-bag representing bytes (volts!) flowing over ethernet (copper!).

    • scichelli

      Oh, that is a nice point: If you mean read-only, make it read-only. (“Succerror” made me laugh–as did a property-bag of volts!)

      • Carlos Ribas

        I’ve also been doing some stuff where I have a method that takes a success Action and a failure Action. So, the caller would supply the code they want run in each condition and the method will call the appropriate Action within its internal workflow. This removes the need for the caller to receive an instance back and test a property such as IsSuccess, etc. The caller can just provide an Action for each path and that’s that.

        You can do this with an interface too, of course, but lately I’m not seeing the point in some of these cases. Extra ceremony in terms of declaring an interface, implementing it, and some problems with combinatorial explosion.

        Just thought I would point that out, because in my projects when I have these multi-mode objects (objects that contain mixed state and can only be created via a factory method per mode that populates partial state), they usually are abstracted away one layer out with something like this :)

        Generally-speaking I avoid multi-mode objects, as they are the path to the dark side. In fact, I always try it explicitly before settling on using a multi-mode object, just so I’m sure the trade-offs are worth it!

        • scichelli

          This deserves highlighting.

          “I have a method that takes a success Action and a failure Action. … This removes the need for the caller to receive an instance back and test a property such as IsSuccess, etc. … Generally-speaking I avoid multi-mode objects, as they are the path to the dark side.”

  • http://www.siimviikman.com Siim Viikman

    Interesting, this class looks exactly the same as in my current project :)
    My main point for refactoring was that I could create success/failure results with single line of code without setting IsSuccess everywhere explicitly.

    • scichelli

      :) It makes the creation sites prettier, doesn’t it?

  • Indian

    Can’t we make overloaded constructors, one in which set of objects is passed and other in which pair of string is passed and without a default constructor? The properties will still remain private set.

  • Pingback: Interesting Links #2 | James Snape