Showing posts with label refactoring. Show all posts
Showing posts with label refactoring. Show all posts

Saturday, 22 September 2012

In which the general fear of TDD is discovered

Since I last wrote about test-driven development—since we spent that time at work learning how to do it, I’ve been trying to make use of it in my off-time development. I’ve mentioned before that I’ve been writing an ORM from scratch, to satisfy an itch that I have. In its current incarnation, I haven’t really had many opportunities to write anything using it, other than an aborted attempt to create a tracker for the No-Cry Sleep Solution.

Earlier this year, the note-taking web app that I’ve been using for years made a major overhaul of their user interface…and left mobile web out in the cold. Seriously. If you aren’t accessing the site from something that can fully act like a desktopfull-scale browser, then you’d better be on either an Android or iOS device, because otherwise, you’ve been left out in the cold.

At the time, I was well and truly in the cold. My mobile phone was, and still is, a Palm Centro. My only tablet-like device was my Kobo Touch (my wife owns a Nook Color, but I wasn't about to both commandeer it during the day and install a note taking app), though we’ve since also purchased an iPad with LTE. At work, at the time, I used my Kobo to present myself with my notes during scrums. Since then, I’ve been writing to a static HTML file on those days that I don't bring the iPad to the office, but there’s still a nontrivial issue of synchronisation. While I could probably use Dropbox and a reasonably simple PHP application to read and write to a single note file, that still just doesn’t do it for me.

So, I opted to begin writing my own, using Alchemy and Zend Framework on the back end. The initial progress wasn't so bad, and it isn’t as though I didn’t have alternatives that have worked reasonably well in the meantime. I decided to basically cater to my own use cases, since I could. Mobile Web would be reasonably fully featured, if a degraded experience. My Kobo Touch would get a good interface where I could edit notes, or write new ones, easily. It would all be there.

The problem is that it hasn’t always been smooth sailing. Ignoring the fact that I don’t often have the opportunity to work on it at home, having a toddler, it seems like with every model I implement, I find another thing about Alchemy that needs to be added or fixed. I’ve been trying to adhere to test-driven development to do that, but by God, I made it difficult to do that in some places. Doing the whole “TDD as if you meant it” thing can be particularly tricky when you’re working with an existing codebase that isn't particularly (or even remotely) tested, and particularly when you're writing web application controllers. Controllers are notoriously hard to unit test, if for no other reason that because their very purpose is side effects, which runs somewhat contrary to many of the premises of test-driven development. I’m finding that it’s far more straightforward to perform acceptance testing on your controllers, and actually go through the motions of the task you’re seeking to test.

Where I’ve been running into difficulty with Project Seshat,¹ though, is in code that I not only wrote a long time ago (somewhere on the order of three years), but also works perfectly well in isolation. The Model class, and its database-driven subclass, provide a parent class to all model-like activity in my application. It acts as entity, DAO, and service layer, mainly because that’s what made the most sense to me at the time I started writing it (this was well before I started working with enterprise Java. I still disagree with the notion of the DTO, but have yet to fully articulate why, to my own satisfaction). And that’s fine; it can still work reasonably well within that context. The problem is that, at some point when working with each of the last two Models I’ve added, the logic that stores the information in the database has both succeeded and failed in that regard at the same time.

Huh?

One of the core features of the ORM in Project Alchemy is that every change that’s written to the database with an expectation of long-term persistence (so, basically, everything that isn’t session data) also gets logged elsewhere in the database, so that complete change history is available. This way, if you ever need to figure out who did something stupid, it’s already there. As a developer, you don’t have to create and call that auditing layer, because it was always there, and done for you.

This audit trail, in its current form, is written to the database first—I decided to implement write-ahead logging for some reason that made perfect sense at the time. Not that it doesn’t make sense now, but there are a lot of features that still have to be implemented…like reading from this log and providing a straightforward function for reverting to any previous version. But at least the data will be there, if only, for now, for low-level analysis.

At any rate, because I can see these audits being written, I know that the ORM is at least trying to record the changes to the entities that I've specified; they’re available at the time that I call the write() method in the storage area for uncommitted data. The pain is that when it tries to create a new instance of the Model in the database, the model-specific fields aren’t being written to the entity table, only to the log. The yet-more painful part is that this doesn’t happen in testing, when I try to reproduce it in a controlled environment. This probably just means that these bug-hunting tests are insufficient; that they don’t fully reproduce the environment in which the failure is occurring.

So yeah. TDD, while it’s great for writing new code, is very difficult to integrate into existing code. I’ve had to do what felt like some strange things to shoehorn a test fixture in place around all this code I’ve already written. I recognise that the audit trail makes the testing aspect a little bit more difficult, since it's technically a side-effect. However, I don’t really want to refactor too much, of any, of the Model API, simply because my IDE isn’t nearly clever enough to be able to do it automagically, and because I still really, really want the audit trail to be something that doesn’t have to be specifically called.

I am, however, beginning to understand why so many developers who have never really tried TDD dismiss it, claiming that you end up writing your code twice. At first, you think that the test and the code are completely distinct entities, and that the structure of your tests will necessarily reflect your code. Yeah, this would mean that you’re doing everything twice. But that’s not TDD done properly. But then when you get into it, you realise that it isn’t the new code you have to write twice, but all the existing code that has to be massively refactored (and in some cases, virtually rewritten, so dissimilar is the result from the what you started with), and that’s always a daunting thought. You may even find yourself feeling compelled to throw out things you’ve spent a great deal of time and effort on, purely in order to get it testable.

I get that. That’s where I am right now. But there are two things to remember. First of all, your code is not you. If you want to work effectively in any kind of collaborative environment, whether at work or on an open-source project, you need to be able to write code and leave your ego at the door. Hell, the same thing goes for personal projects. The second is that if you refuse to make something better (whether better means more efficient, more maintainable, or whatever), simply because you invested x hours in it is foolish. You probably made something good, but you can always make it better if you’re willing to put in the effort.

And speaking of persistence issue, I’m sure I’ll fix it eventually. I already did once, though I didn’t properly record how I did it. Gee, if only I had some kind of mechanism for taking notes!


¹ Seshat was the Egyptian goddess of wisdom, knowledge, and writing. Seems appropriate to use a name that means "she who scrivens" for the tool you're going to use for your own scrivening.

Friday, 31 August 2012

In which a discipline seems not so impossible to define

At work, I’ve been trying for a while to do to things:

  1. Discover what $EMPLOYER considers to be “Software Engineering” at the next pay grade above mine, and
  2. Satisfy those requirements.

Every time I think about it, though, I’m revisited by my internal conflict over the title Software Engineer. I am not a Professional Engineer. I am not a trained engineer; I took exactly one course in “software engineering” in university, and it was a crash course in Scrum. So really, calling myself a Software Engineer, and trying to advocate for being called as such seems like attempting something between deceit and fraud.

This isn’t to say that engineering principles and practices can’t be applied to the discipline of software design and development. It absolutely can, and in most commercial cases, should. Part of the difficulty is that software development, and thus, the attempt to derive a working definition of Software Engineering, is still quite young. Not much younger than aerospace engineering, which is quite young, and quite well defined. Professional Engineering organisations are still at odds about whether or not they’re willing to license Software Engineers, because it’s still staggeringly hard to pin down exact what constitutes Software Engineering. So, the title of Software Engineer goes on uncontrolled and unchecked, as long as software engineers don’t call themselves Professional Engineers (unless they already are, from another engineering discipline).

I suspect that a large part of the problem is that software engineering is typically taught in university as a single course; maybe two. Every discipline of engineering recognised by P.Eng. organisations is taught as a four-year degree. Clearly, there’s a disconnect here.

Then, I got to thinking, could software engineering be taught as a four-year degree? What courses would be taught in this degree? What skills and disciplines have I learned, formally or informally, that have made me a better software developer? Certainly this curriculum would require a good foundation in computer science; many first- and second-year courses could be shared with CS. But then time would need to be spent on all the practices that help software to be written according to engineering principles and practices.

The current, most obvious, example is test-driven development. My group has been challenged by our manager to take what we learned in that TDD course to heart, and really try to apply it to our work. And I’ve been trying to do just that. I spent a day and a half refactoring code I’d written earlier that week in order to make it testable. In just one day, I’d done enough to establish 58% line coverage in unit tests. Trying to get the rest seemed like “not worth it” because the specific task should only be roughly a one-time-use process, and anyway, is already integration-tested and spot-check verified to be working correctly. But the refactoring-for-better-testing exercise was a useful reminder of what TDD provides.

Since then, there have been a couple of bugs reported by our QA team that I’ve had to hunt down. In the spirit of TDD as if you meant it, I stopped myself from trying to diagnose the issue and model the solution in my head, and just wrote a simple test that would fail in the presence of the bug. Then I went hunting for the root cause.

Before our morning scrum, it was staring me in the face, and I had a decision to make, of how to implement the fix. With a decision in place, more tests, in different places, because of how I wanted to implement that fix. Then back into the code to make the tests pass.

It ended up being a bit more complex than I’d originally expected (and I’d judged it as being reasonably complex), and took a day to really, properly, nail everything down to satisfy the requirements that weren’t being fulfilled. But the solution is one with thorough unit tests, that handle the full range of possible inputs (as well as some CANTHAPPENs), so I can state with confidence that the solution is robust, and thorough, and that it won’t fail in adverse situations.

These are engineering principles. I think it’s reasonable to posit that test-driven development could be taught as a part of a university degree in Software Engineering. But could you really spend an entire course teaching Test Driven Development (or whatever name it would have on a university timetable)? You could.

The course that we had was two weeks. Three days “in class”, and our team split into two groups that worked, with the instructor, for three days each of, essentially, labs. These were still very teachable, because they were full of real-life examples—the very code that we work with every day. At any rate, each developer got six working days, about seven hours each, spent learning Test Driven Development. Six working days of seven hours is, if you do the math, forty-two hours. The typical university course is three hours per week, for thirteen weeks, for a total of thirty-nine hours.

Suddenly, the possibilities open up for what could comprise a four-year degree in Software Engineering. Consider that any intensive, multi-day, all-day course you take as a professional can map to university credit courses if its length can be measured in whole weeks, one course per week. Suddenly, the notion of establishing a licensing program for Software Engineers that is on par with every other discipline of Professional Engineering looks feasible.

So if you were planning a curriculum, what would you put on it?

Thursday, 16 August 2012

Better refactoring... through testing

When you sit down to design a new class for your application, how many things do you normally intend it to do? When you’re finished implementing it… how many things does it actually do?

Are these different numbers?

I thought they might be. It’s far too easy—and I’m as guilty of this sin as the next programmer—to fold subtasks into a single class, because they’re all part of the same subsystem, right?

Wrong. Each class should focus on doing one thing, well. Subtasks need to be foisted off into a separate class that focusses on doing that subtask. There are two reasons for this:

  1. You reduce coupling of the class. If you have a class A, that does subtasks a1, a2, and a3, and the subtasks don’t really affect one another, then class A doesn’t need to know everything about a1, a2, and a3.
  2. You improve the testability of the class. Presumably the subtasks are all hidden behind private methods, because they shouldn’t be part of the parent class’s API. That much, at least, is correct. But if the subtasks are tightly coupled with the parent class, then you can’t readily test the subtasks without executing the entire parent task… and that may not be something you can express as a unit test, particularly if the parent task is dependent on an external Web service.
  3. In addition to improving testability, you also make your tests meaningful. A failing test on a complex method can mean a lot of different things, depending on the nature of the failure—how was the result different from the expectation? If you have simple tests, that focus on short methods (you’re keeping your methods short, right), then when your tests fail (and you may get cascading failures), you’ll be able to identify fairly readily why they failed.

For the sake of a really facile, fairly stupid example, here’s a class that’s doing two entirely separate subtasks:

package com.prophecynewmedia.example.subtask;public class GodObject {  public String performsAComplexTask(String parameter1, String parameter2,                                     String parameter3, String parameter4) {    return combineTransformations(parameter1, parameter2, parameter3, parameter4);  }   private String combineTransformations(String parameter1, String parameter2,                                        String parameter3, String parameter4) {    return transformParametersAnotherWay(parameter1, parameter2)         + transformParametersOneWay(parameter3, parameter4);  }   private String transformParametersOneWay(String parameter1, String parameter2) {    return parameter1 + parameter2;  }   private String transformParametersAnotherWay(String parameter1, String parameter2) {    if (parameter1.contains(parameter2)) {      return "((" + parameter2 + ")" + parameter1 + ")";    }    return "(())";  }}
What do transformParametersOneWay() and transformParametersAnotherWay() have to do with each other, practically? They get called on different parameters, do different things, and are only combined in combineTransformations(). Here's the test class for GodObject:
package com.prophecynewmedia.example.subtask;import org.junit.Test;import static org.junit.Assert.assertEquals;public class GodObjectTest {  private GodObject godObject = new GodObject();  @Test public void confirmsLastTwoParametersCombineCorrectlyWithDifferentFirstParameters() {    assertEquals("(())cd", godObject.performsAComplexTask("a", "b", "c", "d"));  }   @Test public void confirmsFirstTwoParametersDoNotCombineCorrectly() {    assertEquals("(())", godObject.performsAComplexTask("a", "ab", "", ""));  }   @Test public void confirmsFirstTwoParametersCombineCorrectly() {    assertEquals("((a)ab)", godObject.performsAComplexTask("ab", "a", "", ""));  }}

Now, because of the simpleness of what we’re doing, this test doesn’t look so bad. Given. However, the first test also has to take into account what happens to the first two parameters in its expectations. This just isn’t clean. Surely there’s a way we can clean this up!

GodObject is doing a task that requires two completely distinct subtasks, and it&rsuqo;s doing those tasks too. In order to test one of those tasks, we have to take into account the results of the other. Let’s clean this up, one step a time…

Step 1: Make the first subclass.

We’ll modify GodObject.combineTransformations():

  private String combineTransformations(String parameter1, String parameter2, String parameter3, String parameter4) {    return transformParametersAnotherWay(parameter1, parameter2)         + SimpleTransformer.transform(parameter3, parameter4);  }   static public class SimpleTransformer {    static public String transform(String parameter1, String parameter2) {      return parameter1 + parameter2;    }  }

We’ll add a new test for this class:

package com.prophecynewmedia.example.subtask;import org.junit.Test;import static org.junit.Assert.assertEquals;public class SimpleTransformerTest { @Test public void combinesCorrectly() {  assertEquals("ab", GodObject.SimpleTransformer.transform("a", "b")); }}

So far, so good. We'll leave the first ugly test in the GodObjectTest, because it's the only test that confirms that combineTransformations() actually works. We'll rename it later. For now, let’s move that other transformation out into another new class.

Step 2: Make the second subclass
  ...  private String combineTransformations(String parameter1, String parameter2, String parameter3, String parameter4) {    return ComplexTransformer.transform(parameter1, parameter2)         + SimpleTransformer.transform(parameter3, parameter4);  }  ...   static public class ComplexTransformer {    public static String transform(String parameter1, String parameter2) {      if (parameter1.contains(parameter2)) {        return "((" + parameter2 + ")" + parameter1 + ")";      }      return "(())";    }  }
And the tests...
package com.prophecynewmedia.example.subtask;import org.junit.Test;import static org.junit.Assert.assertEquals;public class ComplexTransformerTest {  @Test public void confirmsParametersDoNotCombineCorrectly() {    assertEquals("(())", GodObject.ComplexTransformer.transform("a", "ab"));  }   @Test public void confirmsParametersCombineCorrectly() {    assertEquals("((a)ab)", GodObject.ComplexTransformer.transform("ab", "a"));  }}

This has made the last two tests in GodObjectTest redundant, because now we know that the transformations are being performed correctly (and will learn of failure if they change because the relevant tests will fail. I won’t bother showing the truncated test class here. It’s step 3, at any rate. Step 4 is moving the static inner classes into their own classes within the package.

There. We’ve now turned a facile, ugly, hard-to-test example into a facile, slightly less-ugly, easy-to-test, easy-to-refactor, easy-to-read-the-tests example. Huzzah! Being able to do this—to see where your classes are becoming too complex, and be able to refactor them into multiple simpler classes—is an important part of test-driven development, when you’re working with older code. If you inherit a project that has low test coverage, you may find yourself spending days doing this kind of stuff.

I’m tempted to set up Git and Subversion precommit hooks to prevent me from checking in code that isn’t tested. Granted, it would require parsing a coverage report and a diff, but it would probably pay off in the long run, because I’d know that everything I’m writing is tested, and would force me to leave the code better than I found it. It’s a story for another time, but we’re probably all familiar with the fear of refactoring another developer’s code in case you break it, and the inclination to just cobble on the changes you need to make. I know I am.