Showing posts with label Refactoring. Show all posts
Showing posts with label Refactoring. Show all posts

Sunday, November 11, 2018

Refactoring Towards Language

When implementing MVC in iOS and macOS, one common pattern is to use NSNotification to implement the notification that the model has changed and the view should redraw itself. This is a good pattern, as that is how MVC is intended to work, but it does lead to quite a bit of boilerplate in this incarnation.

Specifically, you typically have some version of the following code in one of the initialisation methods of your ViewControllers, which play the MVC role of the View in Cocoa and Cocoa Touch.


- (void)viewDidLoad
{
	...
    [[NSNotificationCenter defaultCenter] addObserver:self
                                             selector:@selector(modelDidChange:)
                                                 name:MPWAppNotificationModelDidChange
                                               object:nil];
	...
}

Refactor I

That's all good, except that you tend to repeat that code in every single ViewController, and there are usually quite a few of them. So one way of avoiding all that duplication is to refactor by extracting the duplicated functionality into a method.
- (void)subscribeToModelDidChangeNotification
{
    [[NSNotificationCenter defaultCenter] addObserver:self
                                             selector:@selector(modelDidChange:)
                                                 name:MPWAppNotificationModelDidChange
                                               object:nil];
}


- (void)unsubscribeFromModelDidChangeNotification
{
    [[NSNotificationCenter defaultCenter] removeObserver:self
                                                    name:MPWAppNotificationModelDidChange
                                                  object:nil];
}

- (void)subscribeToModelWasDeletedNotification
{
    [[NSNotificationCenter defaultCenter] addObserver:self
                                             selector:@selector(modelDidChange:)
                                                 name:MPWAppNotificationModelWasDeleted
                                               object:nil];
}


- (void)unsubscribeFromModelWasDeletedNotification
{
    [[NSNotificationCenter defaultCenter] removeObserver:self
                                                    name:MPWAppNotificationModelWasDeleted
                                                  object:nil];
}
...

Refactor II

The interesting thing that happens when you remove duplication is that it will often reveal more duplication. In particular, if you look at more of these, you will notice that the method bodies are largely identical. You always send a message to the defaultCenter of the NSNotificationCenter class, you pretty much always add yourself as the Observer, and I've rarely seen the object: parameter used. The only things that vary are the selector to send and the name of the notification.
    [[NSNotificationCenter defaultCenter] addObserver:self
                                             selector: <some selector>
                                                 name: <some notification>
                                               object:nil];


So yet another classic case for refactoring by extracting a helper method that encapsulates the things that do not change and takes the things that do vary as arguments.
-(void)subscribeToNotification:(NSString *)notificationName usingSelector:(SEL)selectorName
{
    [[NSNotificationCenter defaultCenter] addObserver:self
                                             selector:selectorName
                                                 name:notificationName
                                               object:nil];
}

We can also apply the same refactoring to unsubscribing.
-(void)unsubscribeFromNotification:(NSString *)notificationName
{
    [[NSNotificationCenter defaultCenter] removeObserver:self
                                                    name:notificationName
                                                  object:nil];
}

With the parameters extracted, our helper methods simplify to the following.
- (void)subscribeToModelDidChangeNotification
{
    [self subscribeToNotification: MPWAppNotificationModelDidChange usingSelector:@selector(modelDidChange:)];
}

- (void)unsubscribeFromModelDidChangeNotification
{
    [self unsubscribeFromNotification: MPWAppNotificationModelDidChange];
}

- (void)subscribeToModelWasDeletedNotification
{
    [self subscribeToNotification: MPWAppNotificationModelWasDeleted usingSelector:@selector(modelDidChange:)];
}

- (void)unsubscribeFromModelWasDeletedNotification
{
    [self unsubscribeFromNotification: MPWAppNotificationModelWasDeleted];
}

Refactor III

As happens very often when you extract code and remove duplication we discover more duplication that was previously somewhat hidden, for example because code that was too far away to really notice the duplication is now close enough together to make it more obvious. Specifically: we always define the same methods: subscribeTo<notification> and unsubscribeFrom<notification>. We also repeat the notification name in each of the method names, and also in each of the method bodies (with different prefixes/suffixes).
- (void)subscribeTo<notification-name>Notification
{
    [self subscribeToNotification: MPWAppNotification<notification-name> usingSelector:@selector(<notification-selector>:)];
}

- (void)unsubscribeFrom<notification-name>Notification
{
    [self unsubscribeFromNotification: MPWAppNotification<notification-name>];
}
- (void)<notification-selector>:(NSNotification *)notification
{
}

Up to this point, we managed to achieve our goals using plain refactoring techniques. Often, that is enough, and it’s generally a good idea to stop there, because beyond lies metaprogramming, which tends to make the code significantly more obscure. In Objective-C, we generally have runtime tricks and macro-programming using the C pre-processor (well: and code generation). In this case, I decided to give the C preprocessor a try, because I wanted things done at compile-time and the problem was parametrising parts of identifiers, i.e. string processing.

Doing some pattern matching, we have two actual “parameters", the “base” of the notification name ( for example ‘ModelDidChange’ or ‘ModelWasDeleted’ ) and the name of the method to call. So we create a Macro that takes these parameters and applies them to a template of the methods we need:


#define SUBSCRIBE_UNSUBSCRIBE_WITHMETHODNAME( name , notification, theMessage ) \
- (void)subscribeTo##name \
{\
    [self subscribeToNotification:notification usingSelector:@selector(theMessage:)];\
}\
\
- (void)unsubscribeFrom##name \
{\
    [self unsubscribeFromNotification:notification];\
}\
-(void)theMessage:(NSNotification *)notification\
{\
}\

#define SUBSCRIBE_UNSUBSCRIBE( commonName, theMessage ) SUBSCRIBE_UNSUBSCRIBE_WITHMETHODNAME( commonName##Notification, MPWAppNotification##commonName , theMessage )

The first macro has three arguments, a version of the notification name suitable for adding to the method names, the full notification name and the message name. The ‘##’ is the token pasting operator, which allows us to create new lexical tokens out of existing ones. We “need” it in this case because we have commonality we want to express that is on a sub-token, sub-identifier level.

There are a bunch of issues with this approach. First, it is pretty unreadable. You need to terminate every line with backslashes because Macro definitions can only be on a single logical line, the backslashes continue the logical line over physical lines. Dealing with compiler errors and warnings is rather tricky, because the error will be with the result of the macro expansion, which the error message will usually not contain. (You can tell the compiler to run just the pre-processor and give you the result, in case you need to debug and can’t do the macro expansion in your head…). Finally, you cannot search for the tokens that get generated by the pre-processor. So for example no command-clicking, and you have to be careful if you ever change one of the notification names.

The advantage is that you get a representation that succinctly states what you want to accomplish, without any duplicated boilerplate. No duplication also means no chance to get those duplicates wrong: the first few code samples in this post actually contain an error. (MPWAppNotificationModelWasDeleted also sends the modelDidChange: message instead of its own modelWasDeleted:) This error, almost certainly a copy-and-paste bug, was actually in the original code and had gone unnoticed for months until I tried to remove that duplication.

I don't know about you, but my eyes just glaze over when scanning large swathes of mostly duplicated code.

Anyway, with the Macros, our help methods are now defined as follows:


SUBSCRIBE_UNSUBSCRIBE( ModelDidChange, modelDidChange )
SUBSCRIBE_UNSUBSCRIBE( ModelWasDeleted, modelWasDeleted )

SUBSCRIBE_UNSUBSCRIBE( ActivityCountDidChange, activityCountDidChange )
SUBSCRIBE_UNSUBSCRIBE( ObjectIdDidChange, objectIdDidChange )
SUBSCRIBE_UNSUBSCRIBE( UserDidLogin, userDidLogin )

Note that previously we showed just two notifications, now we are showing all five, which would have been quite unwieldy before.

Refactor IV

As is typical when you remove duplication, you notice more duplication, because stuff is closer together: the two parameters are almost identical, except for capitalisation. This doesn't have to be the case, but it is a good convention to have. Alas, the pre-processor can’t change the capitalisation of strings so we are stuck.

To recap, this was a process of detecting duplication, removing that duplication using available mechanisms and then detecting more duplication, over several iterations. What then usually happens is that you either manage to remove all the duplication, or you notice that you cannot reduce duplication any further. Not being able to remove duplication typically means that you have reached limitations of your language, with metaprogramming facilities allowing you to push those limitations at least a little, and sometimes quite a bit.

This particular exploration relied on a somewhat formulaic use of NSNotificationCenter, one that always matches a notification with the same message. Since messages are already late-bound, this doesn't really present much of a restriction and seems a useful simplification, and it is a simplification that I've seen used widely in practice. The other pattern is that specific classes typically observe notifications for essentially their entire lifetime, meaning that the ability to dynamically turn notifications on and off is often not needed.

If we imagine language support for notifications (the implicit invocation architectural style), we would probably like to be able to declare notifications, declare that a class listens to a specific notification and and check conformance. This would make usage even more convenient than the macros we refactored to, while at the same time addressing the problems of the macro solution.

And of course that is what Notification Protocols became: a slight misappropriation of Objective-C and Swift Protocols to get something that is extremely close to actual language support. I can now actually declare my notifications as almost a programming-language thing (at least better than a string), and also specify the relationship between a message and that notification, which otherwise is purely lost in convention:


@protocol ModelDidChange <MPWNotificationProtocol>

-(void)modelDidChange:(NSNotifiction*)notification;

@end

I can also easily and declaratively specify that a particular class listens to a notification:
@interface NotifiedView:NSView <ModelDidChange>

@end

Not only does this remove the problems with the Macro approach, unreadability and untraceability, it is actually quite a bit better than the approach without Macros, all while being at least as compact. Last not least, notification sending is not just compact, but also obvious and at least somewhat compiler-checked:
[@protocol(ModelDidChange) notify];

This is very, very close to native language support, due to some lucky coincidences and the wise decision to make Protocols first class objects in Objective-C. It also does not match the flexibility of the NSNotificationCenter APIs, but I doubt whether that additional flexibility is actually used/useful.

Had I not experimented with removing duplication, and iterated on removing duplication, I never would have come to the point where notification protocols became an obvious solution. And now that I have notification protocols, I also have a good idea what actual language support should look like, because I've been using something pretty close.

So for me, plain old refactoring, different kinds of metaprogramming and language support all live on a continuum of improving expressiveness, and all are, or at least should be part of our feedback loops. How can we improve our language so we don't need metaprogramming for common use cases? How can we improve our metaprogramming facilities so they are sufficient and we don't feel a need for replacing them with actual language support? How can we do both, turn things that currently require metaprogramming look more like plain base-level programming while making it integrate better than metaprogramming solutions?

That, Detective, is the right question. Program terminated

Sunday, April 8, 2012

Why Testing Matters for Performance

While performance certainly matters for testing, due to fast tests not breaking the feedback cycle that's so important in being agile (rather than "Doing Agile", which just isn't the same thing), the reverse is also true: with a trusted set of unit tests, you are much more likely to show the courage needed to make bold changes to working code in order to make it go faster.

So performance and testing are mutually dependent.

Tuesday, January 18, 2011

On switching away from CoreData

Like Brent Simmons, I have a project where I am currently in the process of switching away from CoreData. Unlike Brent, and somewhat surprisingly given my proclivities, the reason is not performance.

Rather, the issues we have had with CoreData were additional complexity and more importantly gratuitous dependencies that, at least for our application, were not offset by noticeable benefits.

One of the most significant structural dependencies is that CoreData requires all your model classes to be subclasses of NSManagedObject, a class provided by CoreData. This may not seem like a big problem at first, but it gets in the way of defining a proper DomainModel, which should always be independent. The Java community actually figured this out a while ago, which is why there was a recent move to persistence frameworks supporting POJOs. (Of course, POOO doesn't have quite the same ring to it, and also the Java frameworks were a lot more heavy-handed than CoreData). The model is where your value is, it should be unencumbered. For example, when we started looking at the iPhone, there was no CoreData there, so we faced the prospect of duplicating all our model code.

In addition to initially not having CoreData, the iPhone app also used (and still uses) a completely different persistence mechanism (more feed oriented), and there were other applications where yet a third persistence mechanism was used (more document centric than DB-centric, with an externally defined file format). A proper class hierarchy would have had an abstract superclass without any reference to a specific persistence mechanism, but capturing the domain knowledge of our model. With CoreData, this hierarchy was impossible.

Since we had externally defined file formats in every case, we had to write an Atomic Store adapter and thus also couldn't really benefit from CoreData's change management. When we did the move, it turned out that the Atomic Store adapter we had written was significantly more code than just serializing and de-serializing the XML ourselves.

Another benefit of CoreData is its integration with Bindings, but that also turned out to be of little use to us. The code we managed to save with Bindings was small and trivial, whereas the time and effort to debug bindings when they went wrong or to customize them for slightly specialized needs was very, very large. So we actually ditched Bindings a long time before we got rid of CoreData.

So why was CoreData chosen in the first place? Since I wasn't around for that decision, I don't know 100%, but as far as I can tell it was mostly "Shiny Object Syndrome". CoreData and Bindings were new Apple technologies at the time, therefore they had to be used.

So are there any lessons here? The first would be to avoid Shiny Object Syndrome. By all means have fun and play around, but not in production code. Second and related is to really examine your needs. CoreData is probably highly appropriate in many contexts, it just wasn't in ours. Finally, it would be a huge improvement if CoreData were to support Plain Old Objective-C Objects. In fact, if that were the case we probably would not have to ditch it.

Sunday, August 10, 2008

Code is not an asset

Michael Feathers wonders how to go beyond technical debt. I have been wondering about this for some time, and I think the answer is to account for code as a liability, not an asset.

The functionality that the code has, the value it delivers is an asset, but the code itself is a liability. This easily explains how refactoring and removing code add value, as long as functionality is preserved.