Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
@implementation User

- (void)refreshProperties
{
    assert(!self.isSyncing);

    NetworkOperation *op = [[NetworkOperation alloc] initWithEndPoint:GetUserDetails parameters:nil];
    [self setupCompletionForRefresh:op];

    [self setNetworkOperation:op];
    [self setSyncing:YES];

    [[NetworkManager sharedManager] enqueueOperation:op];
}

The above code is used by a UIViewController subclass to update the properties of a User instance (i.e. the view controller calls [aUser refreshProperties] in pull-to-refresh).

Concerted efforts have been made to make sure there are clear lines defining the separation of concerns:

  • Network Operation class only deals with getting data from the network
  • Network Manager class deals with enqueuing operations and watching reachability
  • User class houses the strings, numbers, bools, etc that make up a User instance

However, there are some problems with the above method. First and foremost is it's hard to test. There isn't any way to mock a network operation or network manager and get them inside the method call without swizzling. Also, it couples the User class with the NetworkOperation and NetworkManager classes.

While the above code works, I would like to refactor it.

Looking through GoF, it looks like there are some ways to remove the dependency between User and Network-Operation/Manager: adapter, dependency inversion (using a protocol), etc. From what I can tell, that code would look something like:

[aUser setNetworkOperation:<object conforming to protocol>];
[aUser setNetworkManager:<object conforming to protocol>];
[aUser refreshProperties];

But... isn't this just shifting the dependency? The above calls would happen in the view controller, and now it has to know about and use three classes instead of just the main one it was originally concerned with - User.

Thoughts, discussion, resources, or code examples are greatly appreciated!

share|improve this question
add comment

2 Answers

up vote 3 down vote accepted
git checkout -b fetch_remote_users

Let's start by reviewing the concerns we want the system to address. We will need:

  1. A place to store the set of attributes which define a "user".
  2. A request (to some URI with some set of parameters) which defined how we can obtain an updated view of a user.
  3. A queue of requests to perform with the ability to suspend execution of those requests when the network is unavailable and retry network failures. We probably also want this queue to be an ordered set of requests so we can avoid enqueuing duplicate requests.
  4. A way to react to successful requests so that we can merge and persist the newly received set of User attributes.
  5. An interface to allow user interaction in some view to enqueue a new request and possibly to see some indication of the request's state (in progress, failed, finished).

That's a reasonable number of concerns and it'll be easy to end up with a confused our tightly coupled design trying to express all of them but let's see what we can do.

Starting with #1. We can define a User model to store our "user" attributes. It is likely that we'll find some behaviors which should also belong to this model but let's start simple.

@interface User : NSObject

@end

So far so good. No coupling and while we haven't been motivated to write a test yet we could easily test this User in isolation.

On to #2. NetworkOperation is probably still a good name for this since we're probably going to be dealing with create, read, update, or destroy operations in order to synchronize the state of our User resource. We've only talked about reads so far but it might be useful to capture the type of an operation when we create it so that we know what it is trying to accomplish.

This might be a good place to start writing tests. Let's think about creating a NetworkOperation (using Kiwi's BDD styntax).

describe(@"NetworkOperation", ^{
  context(@"when created", ^{
      __block NetworkOperation *operation;
      beforeEach(^{
          operation = [NetworkOperation operationOfType:CRUDOperationTypeRead URL:[NSURL URLWithString:@"example.com/users/1"] withParameters:nil];
      });
      it(@"has an operation type", ^{
          [[theValue(operation.type) should] equal: theValue (CRUDOperationTypeRead)];
      });
      it(@"use the specified URI", ^{
          [[operation.URL should] equal:[NSURL URLWithString:@"example.com/users/1"]];
      });
  });
});

That's easy enough to build.

typedef enum {
  CRUDOperationTypeCreate,
  CRUDOperationTypeRead,
  CRUDOperationTypeUpdate,
  CRUDOperationTypeDelete
} CRUDOperationType;

@interface NetworkOperation : NSObject
@property(readonly) CRUDOperationType operationType;
@property(readonly) NSURL *URL;

- (id)operationOfType:(CRUDOperationType)type URL:(NSURL *)url withParameters:(NSDictionary *)parameters;
@end

So we can create a NetworkOperation to encapsulate a request. That should also give us a good place to parse the response from whatever format was sent over the wire, we'll see about how to map it to our User domain object later. Who then should be responsible for actually creating NetworkOperations?

We could follow the example in the original question and create operations inline when needed but, as noted, that's going to become hard to test and contributes to the coupling between the User model and the network classes. In order to separate the responsibilities of these classes it might be useful to be able to consider just the creation of a NetworkOperation as its own role. A Factory for building NetworkOperations for fetching Users. It does however seem reasonable for User to be the authority on how to locate its remote representation. We can use a Strategy to allow our domain model objects to act as Factories for their appropriate NetworkOperations.

describe(@"User", ^{
  __block User *user;
  beforeEach(^{
    user = [[User alloc] init];
  });
  describe(@"as a RemoteResource", ^{
    it(@"builds a read operation", ^{
      NetworkOperation *read = [user buildReadOperation];
      [[theValue(read.operationType) should] equal:theValue(CRUDOperationTypeRead)];
    });
  });
});

@protocol RemoteResource
- (NetworkOperation *)buildReadOperation;
@end

@interface User : NSObject <RemoteResource>

@end

Now Users can define the NetworkOperations needed to update themselves but the consumer of those operations can work with generic RemoteResources (some of whom happen to be users).

On to #3!

We probably want to have an object which manages our network operations and it is going to need to exist for most, if not all, of the life of our application so that it can keep track of the state of our operation queue as we wait for operations to finish. Sounds like a good fit for a service (often presented as part of the controller layer of MVC but not a view controller and certainly not a UIViewController).

describe(@"RemoteResourceManager", ^{
  context(@"given an existing resource", ^{
    context(@"reading the resource", ^{
      pending(@"enqueues the read network operation");
      pending(@"sets a success callback block");
      pending(@"sets a failure callback block");
    });
  });
});

Hang on a second! That's sounds like a nice interface for updating a RemoteResource but we're not talking about NetworkOperations anymore. We could have this RemoteResourceManager maintain a queue of the operations it is using but that seems like an internal detail that would be hard to test. Looks like we skipped a step, let's create another service instead. Something the RemoteResourceManager can depend on to manage operations but which doesn't need to know anything about RemoteResources.

describe(@"NetworkOperationManager", ^{
  context("@enqueuing an operation", ^{
    pending(@"adds the operation to the queue");
    context(@"when an equivalent operation is already in the queue", ^{
      pending(@"does not add the operation");
    });
  });
  context(@"when an operation succeeds", ^{
    __block id mockOperation;
    beforeEach(^{
      mockOperation = [NetworkOperation mock];
    });
    pending(@"calls the operation's success callback");
    pending(@"sends a notification containing the resource's identifier");
  });
  context(@"when an operation fails", ^{
    pending(@"calls the operation's failure callback");
    pending(@"sends a failure notification containing the resource's identifier");
  });
});

Now we can revist our RemoteResourceManager.

describe(@"RemoteResourceManager", ^{
  __block RemoteResourceManager *manager;
  __block id mockNetworkOperationManager;
  beforeEach(^{
    mockNetworkOperationManager = [NetworkOperationManager mock];
    manager = [[RemoteResourceManager alloc] initWithNetworkOperationManager:mockNetworkOperationManager];
  });
  context(@"given an existing resource", ^{
    context(@"reading the resource", ^{
      __block id mockOperation;
      __block id mockResource;
      beforeEach(^{
        mockOperation = [NetworkOperation mock];
        mockResource = [KWMock mockForProtocol:RemoteResource];
        [mockResource stub:@selector(buildReadOperation) andReturn:mockOperation];
      });
      it(@"enqueues the read network operation", ^{
        [[[mockNetworkOperationManager should] receive] enqueueOperation:mockOperation];
        [manager read:mockResource];
      });
      pending(@"sets a success callback block");
      pending(@"sets a failure callback block");
    });
  });
});

...and so on.

For #4 we might extend the RemoteResource protocol to define a method to which we can pass data from our NetworkOperations in our success callbacks to apply updates to the model.

#5 is a little tricky and really depends on what UX we aim to provide. We can probably start by providing our view controller with a shared RemoteResourceManager. We could do that via a singleton but I'm reluctant to do so. A singleton would introduce a strong and non-obvious coupling between the view controller and the resource manager. Besides its not wrong to have several resource managers in our app. We just want them to be able to outlive view controllers so that they have time to finish their operations even if the view controller that started them is no longer needed. The fact that the singleton would be hard to replace with a double in a test is a good hint that this might be a poor design decision. Instead I would provide an instance of an existing resource manager either via a dependency injection framework or explicitly pass one to the controller from its creator. Either way, we can easily substitute a test double for the resource manager to test our controller's interaction with it. The NetworkOperationManager spec hints at how we might update the UI to reflect the state of the operation queue. Our resource manager supplies callback blocks to react to the success and failure of operations. On the other hand our view controller may no longer exist (or at least no longer be visible) when the operation finishes. Instead of callbacks we can listed for notifications about the particular resource we are interested in. If the controller gets a notification we can update the view to show that an update has started or finished. When we no longer need to maintain that view the controller unsubscribes from the notifications and ignores them.

git add .
git commit -m"stream of consciousness networking refactor"
git push codereview 19307/dependency-inversion-injection-networking-code-in-model-classes

Hope that's useful.

share|improve this answer
add comment

In GitHub for Mac, we use a combination of AFNetworking and our own Mantle and ReactiveCocoa frameworks for API requests.

AFNetworking makes it trivial to create your own "network manager" type object as a subclass of AFHTTPClient. We enqueue requests on it, ask them to hand back a certain kind of MTLModel subclass, and then subscribe to the resulting signal.

The resulting code looks something like this in the caller:

GHGitHubClient *client = [GHGitHubClient clientForUser:loggedInUser];
[[[client enqueueRequestWithMethod:@"GET" path:@"user" parameters:nil resultClass:GHGitHubUser.class]
    deliverOn:RACScheduler.mainThreadScheduler]
    subscribeNext:^(GHGitHubUser *user) {
        // Do something with the fetched user here.
    } error:^(NSError *error) {
        // Handle errors here.
    }];

There are several advantages to this:

  • You're building on the strength of AFNetworking. There's really no reason to write networking code from scratch anymore – this framework is validated in many, many applications, and can do almost anything you'd need.
  • MTLModel provides a way to parse model objects knowing only the desired class. GHGitHubClient can drive the parsing without being coupled to the specific model types.
  • Signals in ReactiveCocoa are composable, so this code can be easily modified to manipulate and transform the GHGitHubUser, or even perform other fetches and combine the results, before delivering to the main thread.
  • Your domain model isn't doing network activity. I firmly believe that domain models should care only about data, not the mechanics of persisting or fetching it.
share|improve this answer
    
can you share GHGitHubClient source code? –  Yosi Taguri Jun 7 '13 at 22:37
1  
@YosiTaguri It's since been released as part of octokit.objc (though renamed to OCTClient). –  Justin Spahr-Summers Jun 7 '13 at 23:11
    
While ReactiveCocoa provides for an awesome way to handle the complexity of one or several chained async requests, I'm not too sure about type safety. How do you handle the case that the client actually doesn't know that the request signal returns objects of type GHGitHubUser? I wouldn't get any compiler warnings if I expected the wrong type, would I? –  fabb Feb 16 at 14:26
    
@fabb Indeed, you would not. This is a limitation of Objective-C's type system, and unfortunately there's no easy fix. However, it's comparable to using NSArray or NSDictionary—you either have to assume that you retrieved a value of the correct type, or else add assertions everywhere (which still only help at runtime). –  Justin Spahr-Summers Feb 17 at 15:24
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.