Excruciatingly Obvious Ruminations About Extract Method
Last week I had the opportunity to use the Extract Method refactoring. I’m generally a proponent of this refactoring – I think it’s a worthy goal to have more numerous, shorter methods, if that means that any given method is more quickly understood at a glance.
The code I had, after I had made some further complicating modifications, looked something like this.
I didn’t like the nearly-duplicate logic duplicated in the if-else block. I felt there was an extract method in there somewhere. However, it took me three times before I came up with a solution I liked, which is basically this:
I won’t bore you with the first two tries, but I’ll just say that for some reason the use of the third defaultTo arg didn’t occur to me right away, but was just what I needed to make the method calls nice and clean.
After I looked at the finished product, I was reminded that another benefit of Extract Method is that differing levels of abstraction are separated. In this case, the determination of whether WebLogic or Tomcat is in use, and what delimiter and default to use to determine the web app name, is a sort of mid-level policy. The nuts and bolts of parsing that app name out of a String instance is the lowest level of abstraction and, in my opinion, is better off not inlined into the policy code. In a similar vein, we would not want to see the implementation of String.substring() inlined into every single invocation of that method.
I don’t think my finished product is perfect. I think maybe there’s a better method name that could be used, which communicates the intent better, because it’s not perfectly obvious at a glance why the delimiter and default need to be passed to the method and what happens. But then again that’s part of the point, we don’t want to know the details. Meanwhile, on the plus side, we see that the same method is invoked in both cases, and only the arguments differ, so it’s obvious that there’s a pattern there which wasn’t necessarily obvious before.
M Mathew:
Here is my version.
public String getAppName(String name) {
String result;
if (name.startsWith(TOMCAT_PREFIX) || name.startsWith(JETTY_PREFIX)) {
String appName = StringUtils.substringAfterDelimiter(name, ‘/’);
result = appName != null ? appName : ROOT_APP_NAME;
} else if (name.startsWith(WEBLOGIC_PREFIX)) {
String appName = StringUtils.substringAfterDelimiter(name, ‘@’);
result = appName != null ? appName : name;
} else {
result = name;
}
return result;
}
I am not sure whether its a right thing to reassign a parameter. Above version adds some more lines but I think it conveys the responsibility of getApp() in a more meaningful way. I dont have to dig further at the parse function to know what the third param does.
4 May 2008, 5:50 pmM Mathew:
Sorry for the messed up indentation.
4 May 2008, 5:54 pmScott:
Hmmm. Overall I like your version – if I came across it I would leave it as is. I definitely like the method name “StringUtils.substringAfterDelimiter” better than what I came up with. I’m conflicted on whether I would rather pass a third parameter, the value to default to, as I do in my version, or whether I like what you did with the ternary operator. My version is slightly more compact, but I think you make a valid argument that yours communicates intent better, and a developer would be less likely to need to go look at what the parse method is doing to understand what’s happening. Thinking about it now I’d probably give yours the slight edge just for that reason – but if the method grew any more complex in the future I might be quick to revert back to the way I did it
I definitely like either one of our versions better than the un-refactored version.
Thanks for the comment.
4 May 2008, 11:05 pmM Mathew:
Scott, thanks for your feedback.
Somewhere I read good code is a balance between brevity and readability. I think its a tough job to keep that balance.
Personally I like your brief version and wont change it just for the sake of making it a tad more expressive.
5 May 2008, 4:51 am