Thursday, December 1, 2011

That Is Not Your Decision to Make

A recent Weinberg blog (there is a part 2 too) reminded me that I meant to write about the antipattern of programmers making business decisions for some time - so here it goes!


The linked example of a programmer purposefully implementing something other than what (s)he was asked to do - I hope - is not common, but there are other, more subtle situations where we make the same mistake of making decisions instead of our clients (which can be a customer, business analyst, product owner, etc.).


Not Specific Specifications


Project failure (or rework at best) happens when we don't deliver what we the customer wanted. One reason for that can be traced back to misunderstandings of requirements. I often say that our job as developers is to put on our detective hats and help our customers discover & formalize their processes - which they know, do every day, but usually never had to articulate it precisely. Despite the best efforts, specifications/acceptance tests leave space for interpretation, which we, as programmers with a special attention to detail, will discover.


And here comes the problem - another trait of us programmers is that we love solving difficult problems/puzzles. Thus we will first think of a solution that we would like to implement and we often stick with the first idea that makes sense to us as the logical one. Now we are doing exactly what the programmer in the linked article did. There is a reason we are developers and our customers are lawyers/traders/salesmen/etc. - they know their problem domain better. And their perspective (ROI, time to market, cost vs. benefit, etc.) likely differs from ours (such a great puzzle to solve!).
While it's hard to accept it as a programmer, it is a valid business decision to say we don't care about that or we are willing to take the risk - which to us would look bad, for being an imperfect system, an unhandled scenario. However, we might not actually be able to show an occurrence of the problem in the past 5 years' data... And it probably saved a week's worth of programmer work that was spent on some other feature important for the business.


The Road to Hell is Paved with Good Intentions


Has it ever occurred to you how the usability of the site could be improved, or how much better would be if there was a shortcut, or... It certainly has happened to me (and I hope will happen in the future). But again, even if we are right, and it would be a great improvement, that might not be needed, or not now. For instance, if you have a wizard form, where the first step is so beautiful it should be shown in the Louvre, and is super easy to use - almost reads the user's mind! - ... but it crashes as soon as you press the next button, have we really made a good decision? As the saying goes: shipping (functionality) is a feature too.


I have yet to meet a client who didn't appreciate programmers taking initiatives and making suggestions for improvement in the application, but I have met ones that were furious when they learned that only 10% has been delivered due to having too many bells and whistles (though of course, there are counterexamples to everything - but in my experience they are the exception, not the rule).


Let them decide!


If you find yourself in any of the cases above, I suggest you stop, and consult with the person who should make that decision instead of coding away one that makes sense to you - even if eventually it turns out that the customer's decision is the same as the one you had come up with.


Caveat: I am not saying you should stall work, waiting on a decision from customers. You might not have instant access to your customer, they may be on holiday, etc. If so, talk with the most senior person you have available (tech lead, team lead, etc.) and have her/him make a decision. If you are the most senior person, make a decision, but keep it minimal (even if you see myriad of other issues this might lead to), and make it easily undoable if needed.


To give a concrete example: recently while implementing a certain form, and the related business logic behind it, I realized that this action has different implications depending on the type of user that requests it - but it was not specified yet. The combinations I came up with were pretty big. But for my given feature, I just needed one of those combinations. I ended up using a single request handler class, but implemented the logic for each role as a separate mixin class, so when it comes to dealing with this issue, we don't have to spend the time excavating the code relating to each role from one class. Based on my prior experiences, I wouldn't be surprised if our customer will decide to simplify this feature, and say we only handle the two most common combinations, and don't make the feature available for the other users. If it'll turn out that is the case, it's rather a good thing that I haven't forged ahead and spent days writing a complex, composed object hierarchy to handle all possibilities.

Thursday, November 17, 2011

CITCON London 2011

Given what a great conference CITCON 2010 was, when registration opened for CITCON 2011, I didn't hesitate - which turned out to be a good thing, since spots had filled up rather quick. So next year, watch the mailing list, and rush to the registration form!


Friday Evening - Open Spaces Introduction/Topic Proposals


It was held at Skillsmatter, so I assumed there will be no surprises for me there (great infrastructure), but since I didn't have to take the tube this time (picked my accommodation to be walking distance from Glasswell Road on purpose), I've quickly learned that Bing Maps has little to no knowledge of Glasswell Road and Skillsmatter. So the first day I took a nice, leisurely, albeit somewhat longer road towards the location.


The registration went flawlessly, ran by PJ's mom. Great job!


PJ and Jeffery made the introductions in their usual, entertaining manner. While it wasn't new for me, I was glad to see this time they emphasized that the schedule can change throughout the day - apparently it wasn't only me who had been surprised by it last year. And of course the schedule did change.


Because Julian Simpson couldn't make it, I introduced his topic (Do you use your tests in prod?), and then two of my own - Continuous Release and Delivery in Downloadable Product Companies and Why Most People Don't have a Rollback Plan for Releases and instead "plan" to Hack Forward.


The great thing about proposing topics is that even when it doesn't meet the threshold to have a whole session dedicated to it, a lot of people now know that you are interested in this topic, and find you to share their experiences about it during the breaks.


To my surprise, we actually followed Jeffery's recommendation and we didn't run side conversations during the proposals, so the process was smooth and efficient, and there was more than enough time to chat later - especially that I didn't worry much about the agenda, since proposing two topics made my priorities to attend those, and I knew that whatever agenda we'll have at Friday closing time is not final.


As we learned the next day, when you propose a topic, you should be careful which words to use. In my case, rollback was a terrible word, since many interpreted as going back in time, undoing, while what I meant was more like backout (planned and disciplined retreat).


Sessions I attended


I've added my notes on the conference wiki:



Sessions I wish I attended



  • BitbeamBot: The Angry Birds Playing, Mobile Testing Robot by Jason Huggins ran a session about automated UI testing of touch screens

  • there was a TDD session - given that often I need (want) to explain TDD to people new to the concept, it would have been a great learning opportunity seeing someone (was it run by Steve Freeman?) else's approach to introducing it. Plus likely I would have gained a new understanding of TDD...

  • Backyard beekeeping - I ended up in a random chat, but I was would have been curious to see these non-technical, non-work related sessions

  • I couldn't stay long enough in the Slaughtered Lamb


The hallway chats


While open spaces are already like an unconference, nonetheless a lot of great conversations took place during the breaks (didn't get to use the law of two feet this time either), and I've met a lot of great people. I just wish I had more time to talk more with each of them. Guess I queued these conversations for future processing on twitter or via email.


In contrast to last year's conference, I spent almost the whole time offline, in the analog world, and it didn't feel the least bit wrong. I've pretty much only used twitter when I followed someone on twitter - instead of exchanging emails or business card. Though I have to admit, business cards are great for jotting down small reminder notes on them.


Some travel lessons I've learned


While this is tangential to CITCON, I have learnt a few lessons on this trip. I'll list them here, hoping it's beneficial for others.



  • have your travel plans printed out (a'la TripIt. It made the check-in process much smoother, and going from the airport to my hotel was perfectly relaxed, knowing exactly which tube to take. The only part I forgot to plan was from the Nueremberg Airport to the office (only remembered that would be useful when I landed). Thanks to Career Tools for the idea (and they great advice for attending conferences, approaching your boss about sending you to a conference, and more)!

  • Phones, data plans, roaming. While this isn't the reason that I always buy my phones from the stores and not from the carriers, being able to just buy a prepaid SIM in London made a huge difference. It's not necessarily the money, though I think I've saved there (for the four days I stayed, just the data plan would have cost me 14 €, and there are the calls I made - contrast that with the £15), but rather that making a call or using mobile net wasn't something that I had to consciously choose (is it worth the extra roaming charge?).

  • have enough slack in your trip. I arrived Friday afternoon, and returned home on Monday morning. This allowed me to dedicate Friday and Saturday to the conference (and the Slaughtered Lamb afterwards), be able to meet up with friends living in London on Sunday, and the fact that some of my friends were an hour late just simply didn't cause a problem. And financially it doesn't have to be more expensive - I believe I spent about the same amount on the three nights' stay as I did last year for two nights. Advance planning, more research for accommodation helps you with that.

  • If you need travel accessories (e.g.: AC adapter for the UK), try them before leaving home - I managed to buy adapters incompatible with my laptop's charger. Luckily the reception at my hotel managed to find one of theirs that was compatible, but it took a long five minutes for them to find one, so I wouldn't rely on this

  • In addition to that, I forgot my phone charger at home, and while Bing Maps isn't perfect, it's much better than walking in London without a GPS. Lesson learned: it's handy when you have a USB cable with you that you can use to charge from somebody's laptop - the 10 cm long USB cable fits in any pocket.

Friday, November 4, 2011

Data Migrations As Acceptance Tests

While I have previously said that on migration projects both verification and regression tests are important, does it mean that the two should be separate? Like first, let's migrate the data, and then we'll rewrite the functionality, right? Or let's do it the other way around - we'll talk with the customer, incrementally figure out their requirements, deliver the software (with a proper regression test suite) that satisfies them, and then we migrate. Both approaches have problems:



  • customers want to use the software with their current, real data - having only the data and no application to use it with is no value to them. Neither is having only an application with no data in it

  • real data has lots of surprising scenarios that the domain expert might have forgotten to specify (see caveats though)

  • requirements are not static, and new ones will be introduced during the development process, that inevitably will cause the new application's models to change, which means that the migration has a moving target it needs to migrate to.


Doing them in parallel


If the data source is organized chronologically (order by date in the migration script), and organized in a format that resembles what the system's end users will enter into the system, then we can use the new application's outmost automatable entry point (Selenium, HTTP POST, a module's public API) to enter this data during the migration from the old system to the new.


Why


While a clear disadvantage of this approach is speed of the migration - it will be likely slower than an INSERT INTO new_db.new_table select .... FROM old_db.old_table join ... where .... statement, but in the case of non-trivial migrations it will likely compensate for the slowness, because:



  • changes to the new system's code/data structure become a problem localized to the new application code - no headaches to update the migration scripts in addition to the code

  • when the client requests the demo deployment to be in sync with the old system, the code is ready (spare for the part to figure out which records have changed)

  • the legacy data edge cases provides focus - no need to invent corner cases, for there will be enough in the data

  • likely there will be releasable functionality sooner than with either of the above approaches


How


First, create the acceptance tests for the migration:



  • Pick the data to be migrated

  • find the view in the original system that displays this data to the users and find a way to extract the data from there

  • define the equivalent view in the new system (it's about end to end, user visible features!)

  • write the verification script that compares the above two (be sure to list the offending records in the failure message!)

  • define the data entry point in the new system

  • write the migration script - extract from the old system, transform if needed (only to match the entry points expectations of the format - no quality verification as in classic ETL!), then send it into the new system (using the above defined entry point)


At this point both the new view, and the data entry points are empty. From here on, the TDD cycle becomes a nested loop



  • run the migration script. See which record it failed for

  • analyze the failing acceptance test, and find the missing features for it

  • (A)TDD the missing features

  • run the migration script to restart the cycle


Caveats


While the existing data makes one focus on the real edge cases instead of the imagined one, beware - not everything has to (or can be) migrated - for instance, in a payment system, the system used to accept many currencies in the past, but now only . IN this case, possibly the currency exchange handling logic could be dropped in the new system (and just to store the currency in a char field for the old ones); or in some other domains, maybe only the last ten years' data is needed. However, this should be a business decision, not a decision for a developer!


Source Data Quality is often a problem, one that will likely cause issues. If data needs to be fixed (as above, ask the stakeholders!), it should stay out from your application's code, and be in the Transform part of the migration script.

Sunday, October 23, 2011

Book Review - Python Testing Cookbook by Greg L. Turnquist

I have been doing (developer) automated testing for years now, but I recently moved from .NET to Python. Recently, at one point I suggested to collegues that we try Concordion, only to learn that there is the doctest module that could be used to achieve similar result (more about that in a later post). Remembering my own advice: When In Rome, Do as the Romans Do, I set out looking for a Python specific book about testing - and the Python Testing Cookbook by Greg L. Turnquist book seemed to be a good fit based on reviews.




Overall, I liked the book, and it lived up to my expectations - it provided me with a list of tools and some sample code to get started with each of them.


Beware that it is an entry level book - if, like me, you are already familiar with the testing concepts, and are looking for a book to learn about advanced testing concepts, theories, this book might be too little for you (or just read through the "There is more" sections of the recipies). But it is great for someone new to testing - though discussions with (and code reviews by) someone experienced in testing should accompany the reading.


Despite the below criticisms, which are meant to be rather a companion to the book than an advice against it (i.e.: probably the only book I wouldn't find anything missing from and nothing to criticise about would be written for me, in real time, based on my immediate needs). The fact that the list is short shows how I found the rest of the book valuable, with great advices that go beyond the cookbook format (why you shouldn't use fail(), why there should be one assert per test, etc.). While I don't see eye to eye on each topic with the book, but just as the book is not written in a "my way or the highway" style, I will not get into minor differrences of opinion.


Format of the book


Each chapter explores a topic, with multiple specific recipes. Each recipe is relatively self contained, so if we are in a hurry and need to get to the solution of one specific problem without reading the whole book/chapter, it's possible. However, for those reading whole chapters, this results in a bit of repetition - I had to fight the urge to just gloss over the code I (thought) I had seen before.


Each recepie follows the format of



  • stating the problem

  • showing code that solves it

  • explaining how the code works

  • and finally, providing warnings about pitfalls and problems in the code, and some further advice


While this format is easy to follow, it has a few drawbacks.



  • until I got used to this style, I often found myself cursing out loud like the code reviewers in this comic while reading the code that will later be explained to be bad/antipattern.

  • each recipe has a lot of additional testing insight, potentially unrelated to the tool being demonstrated - but one can miss these, thinking "oh, I know all about doctest, I'll just skip that chapter"

  • for people in a hurry, just scanning the cookbook and copying (typing up) the code - there is nothing to indicate in the code that there is an antipattern there, only in the later paragrpahs - which you might not read when in a hurry. Just thinking about the examples where the unit tests have no asserts but only print statements gives me the shivers (and it's even used for illustration in the chapter about Continious Integration!).


What was missing from the book



  • About testing legacy code, I was missing two things:

    • a pointer to Michael Feather's classic book, Working Effectively with Legacy Code

    • a warning about a mistake I have seen almost everyone (myself included) make when getting started with testing legacy code: don't write tests just because you can - only add cases for the area you are working on and only add enough to cover your current needs. This is hinted at, but I've found it's important to state it explicitly.



  • Notes about test maintainability

    • I strongly disagree with the approach of having one test class per physical class, and test methods directly excercising the class's method. I've found these can lead to maintainability problems down the road, so I prefer to introduce helper abstractions (e.g.: assert_roman_numeral_conversion(expected_result, roman_numeral_string) method) for most of my tests, and organize test methods by logical scenarios instead of mirroring code organizational units (on successful login, validating user input, etc.). These abstraction (indirections) makes it easier to update tests after an API change or refactoring. It might sound like an advanced topic, but I think it's a key concept for effective tests, and one that people should be exposed to early (just after they've made the mental jump from throwaway main methods with visual/human assertions to automated tests with automated assertions).

    • Acceptance Testing - it is notoriously difficult for us programmers to write good acceptance tests that are both maintainable and readable by the customers. I'm rather sure that in the example given in the book, the customers would not be interested in knowing which html tag contains the price in the output.




Minor criticisms



  • there is an inconsistent level of detail and further pointers. E.g.: while optparse is explained in detail, virtualenv and setuptools are glossed over.

  • In addition to the assertless test methods, the other thing that shocked me was the example in the doctest module that - to illustrate that the test works - introduced a bug in the test code. While the fact that test is code and thus can be buggy should be emphasized, but that wasn't the case here. This could leave the reader wondering why exactly we introduced the bug in the test code - aren't we testing the application?

  • The book is careful not to fall into the trap of elitist speak that might alienate people, but saying that coupling and cohesiveness are subjective terms is just providing gunpowder to unwinnable arguments(*).


Interesting notes



  • This might be a cultural thing (I'm coming from .NET), but I've found it rather surprising that such an entry level book talks about test runners, and write custom test runners. It's useful knowledge, just something that I have not seen mentioned in so much detail in the Java/.NET world so early. Maybe it's got to do with IDEs being more widespread, where running a subset of the tests is easy.


As said, the book lives up to the expectations, so if you would like to get a quick and practical introduction to testing in pytohn - both tools and concepts, I can recommend this one for you.




(*) Reminds me of a story from long ago. The team in question has decided to use bind variables for all SQL invocations (I've said it's been some time ago) to prevent SQL Injection. The one programmer wrote a stored procedure that concatenated the SQL command in its body... and argued that this is just a matter of style. At least the procedure was invoked from the application using bind parameters...

Monday, September 19, 2011

Testing Strategy On Migration Projects

It's common for migration projects (a.k.a.: rewrite) to specify scope initially as "to behave the same way as the old system does". Thus the testing approach to automatically compare the new system's results to the old system's seems to be a perfect choice (of course, in addition to the data migration, calculations should be reconciled too - at least for the records that were calculated using the latest version of the old system).


However, once the application goes live and the old one is decommissioned, we cannot rely on these tests anymore (there is no old system anymore), and we have no regression suite to rely on for the future change requests/enhancements.


Just to avoid any misunderstanding: I don't advocate not having automated reconciliation checks (verifications), on the contrary, I think they are immensely valuable. We can write the best specifications/code, but still miss some details, which, luckily for us, do pop up in the real data. These automated checks give everyone on the project team the peace of mind they so need before go-live.


The point I'm trying to make here is that while these checks are essential, they are not enough for the long term health of the system. These are a good starting point, but just as we do with the specifications, as we work on the new system, when we find and uncovered logic case (e.g.: as part of the calculation reconciliation), we need to add a test case to the new application's test suite to ensure proper regression coverage that we can rely on in the years to come. And adding this test case is easy - we can just copypaste (after making it anonymous of course) the input that caused the problem with the verification into the unit/functional tests, implement the missing functionality, and move on. But saving on these few seconds costs a lot later down the road.


Unit/functional/integration/system tests are supposed to be self contained - we would like to create (a) clean database(s), which we put into a known state before the tests (some frameworks support this out of the box, e.g.: Django, but we can easily implement this ourselves). Migration reconciliations, by their very nature, need to work on the live (snapshot) data. Also, as described earlier, these reconciliation tests are temporary artifacts, while the other tests supposed to be permanent (at least until the client decides to change the requirements). Separation of Concerns also applies to the test suite - running tests in the same suite with different assumptions (live db we shouldn't touch vs. empty test db we can read/write as we wish) is more than risky - keep them physically separated, both at runtime and in source control.


Even if the delivered project could be summed up in that vague sentence (does what the old does), this summary is never true - few start rewrite projects just to get the exact same functionality. Usually these projects are sponsored because the old application became unmaintainable and the client is missing opportunities, because the software is not supporting, but hindering their goals. Without the self-contained test suite, our shiny new migrated application is going to become another one of these.

Tuesday, July 19, 2011

About the export-to-Excel Anti-Pattern

Excel is a wonderful ad hoc data analysis tool for non-programmers, and is also often the first milestone in the software evolution. No wonder export to excel is the most common feature requested for business applications (some people even go as far as not developing a GUI at all, but just write Excel plugins).


The recent CRUD vs Task Based UI touches this issue, but focuses heavily on CRUD - and this problem is applicable to other types of applications (all applications have reports). The problem with the Excel export in any application is that important business activity happens outside the software. So when your users almost "beg" for a spreadsheet export, it's a strong clue that some aspect of the business process is not modelled, and our users likely perform some mundane, manual work - and IT is supposed to automate that!


Some examples I've run into (non exhaustive list!):


  • users were reconciling data from two sources, but it took three-four Whys to figure that out. And even after the discovery and the agreed automation solution I had to promise there will be an excel export feature (more on that later)!

  • users were manually looking for errors - turned out that adding a single filter to an existing report provided the same functionality.



Excel centric business processes indicate communication and trust problems - the business doesn't ask for features, and the developers don't know what their users do with the delivered software. There could be many reasons for that (organizational, bad experience, high turnover, etc.), but beware - and if you spot it, try to act to make the situation better.


I used to joke with users (now I believe it seriously) that my role as a developer is to force them to figure out and document (software is a form of executable documentation of a business process after all) their work processes, and enforce that for the future. Much better than a paper/Word based new hire getting started guide!

However, exporting to Excel is a feature business applications should have, possibly for every list/graph we have in the system. Some of the tasks don't have the ROI that would justify the development of a given new feature/report/etc., and giving users direct access to the data is more enabling (think innovation) than if they had to ask IT every single time they wanted to check something (the more hurdles we put up, the less likely they'll be to check, potentially damaging the business). We should monitor these exports, and look for patterns (potentially across screens), and when a pattern emerges, talk to our users about how we could make life easier for them. A few occasions don't warrant a development project, and we don't want to develop something too early (just like with refactoring - wait for the pattern to emerge before you prematurely refactor to a structure that hinders later development).

Sunday, May 15, 2011

On Grassroots/Peer TDD Introduction

While I'm no trainer/coach, I've been involved in spreading TDD among my peers, with varying extent of success. Recently someone asked for advice on how they should go about it, and in the spirit of Seth Godin, instead of responding in a private email, I wrote this post, a mix of experience and hindsight ideas.


So, we are one (or two) developers in the team/company, already sold on TDD, with some unit testing experience. The peers are willing to listen to the concept, and management, while not willing to invest in formal training, is ok giving a chance, as long as it doesn't hurt the projects' performance. How do we go about it?


Don't introduce TDD on the project


It's common that teams learn/practice new skills on live projects after a basic overview/training. While this might work, I would not recommend this approach for any grassroots initiative, because the chances of failure are significant, and one bad experience with a practice/technology can doom any further attempts to introduce it later, even if not the practice was at fault, only was applied without enough practice. A project manager friend of mine recently complained to me that since the team that worked on a massive legacy project started doing TDD, tasks that used to take hours begun to take days, and clearly, he wasn't happy about that. It turned out that the problem wasn't due to TDD, but lack of discipline (developers went overboard with refactoring, touching parts of the codebase not even remotely connected to the task they've been working on). Despite this, he could have easily come to the conclusion that TDD equals to dropped productivity, without much benefit (bug reports kept coming in, even if for different features - we are talking about legacy code!). Any new skill has a learning curve, you will be slowed down, and you won't get it perfect the first time - don't give a chance for the others to associate failures with the new concept. If you are like me and would need a more concrete example about refactoring just enough, I suggest you read Ron Jeffries' Extreme Programming Adventures in C# book, and on advice for dealing with legacy code, the undisputed classic book is Michael Feathers' Working Effectively with Legacy Code.


Introducing/selling TDD to the other developers


A lot of technical presentations focus on the how instead of why it is beneficial to the audience. When it comes to TDD, I would also suggest presenting it simple - skip the emerging design concept (some of the early advocates noted that good design emerging from TDD might had to do with the fact that those early advocates actually had great design sense anyway), don't mention YAGNI - in short, don't overload them with new concepts. I rather liked the approach Paul Butcher took in his Debug It! book - we tend to program one baby step at a time, articulating in our mind what the next behavior we'll implement should be, then we code it, and move to the next behavior. TDD (or Test First Development for the nitpickers :)) is just a minor step from that, i.e.: we actually write down the articulated concept in code (which we tend to do anyways with the throwaway main methods used for debugging). And in addition, we get an automated regression test suite for free :). While a live demo could be cool, I would probably show one of the Kata casts instead of me typing on the projector. Also, be sure to point out that at first we won't be able to code at this speed (I still can't)!


Learning/practice


TDD is a rather simple concept, but so are all the great wisdoms - it takes a lot of time and experience to fully understand those concepts. After the initial TDD sales pitch, I would dedicate a session to going over the tools and the basic usage - how to install the test library, how to run the tests, how to debug a single test, and the basic assert statements - AreEqual vs. AreSame, Assert.Throws, and also an example to show that most likely there are more asserts in the library (e.g. StringAssert.EndsWith).


With those interested/committed, we can move on to the supervised practice step. We've worked on simple problems, in a structure inspired by this Peer learning presentation by Jason Gorman (his site even has a full case study of this applied at the BBC). The coding dojo is another possible format. Important thing is discuss the experiences, lessons learnt, and problems overcome with the whole group, because while practice makes one perfect, bad, practiced habits are hard to unlearn. It is OK not to have all the answers - just be ready to admit it, and ensure that you can find it out from somewhere. Meetups, user groups, and similar forums are invaluably helpful.


Be sure to eat elephant in a piecemeal fashion - while at first the simple problems seem contrived, don't attempt to practice testing existing, complex production code. Stick to the simple problems for practice, every now and then actually demonstrate that it is possibly to test legacy code (though it'll require quite some preparation from you), maybe have some of these examples as a group exercise, but be sure to keep the practice focused on the simple mechanics initially. Just like any sport practice - you repeat rather basic exercises over and over, because it does pay off in the long run, but to keep you motivated, you do have the free practice sessions too.


Don't force it


Not everyone has the same passion as you do (you must have if you read this far :)), and some take a longer time to learn, have different pre-existing concerns they should overcome. Lead by example, be available to talk about it, answer questions, clarify, but don't try to force it on them. Urban cycling didn't became popular because we, cyclists, were preaching to pedestrians and car drivers - we were just approachable to friends, coworkers, etc. and provided answers and listened to their concerns. Yes, it could take a long time, but patience pays off - self-motivated people are more likely to overcome the hurdles they inevitably will face in the learning process!


Bringing it to production


I wouldn't set strong rules or hard deadlines like "two months from now, you all must write all production code in a TDD fashion", but rather just tell people to feel free to experiment during their daily work, but ensure they don't confuse work and practice. If they get stuck on testing some problem, they should just make a note, and deliver the feature untested, as they used to do before. Later, together with the group they should discuss it and potentially find a way to test it.


Further resources


This post is just a high level overview, and I have simply glossed over many of the actual difficulties about writing good unit tests, keeping them maintainable, etc.. There are books written about the topic, in almost all programming languages. Coming from a .NET background, I can recommend The Art of Unit Testing with examples in .NET by Roy Osherov, and I've found James Shore's Let's Play TDD Java episodes great - the best thing about the latter is the thought process narrative that goes with it. Driving Technical Change might be another good book, though I haven't yet read that.


Good luck!

Sunday, April 3, 2011

What have I learned from working with legacy code?

Legacy code (code you've inherited from someone else, likely without tests) is evil and feared by those who are sentenced to work with it. It sucks the life out of one, doesn't it? Yet it is similar to the "what have the romans ever done for us" scene from Monty Python's Life of Brian - there are a lot of great things to learn from it. I would even consider it a required experience to become a well-rounded software developer. Below is my (by no means complete) list of things I've learned from legacy code.


Humility was a hard lesson - I've been rather confident in my skills and ability, yet I have (and seen others too) break working applications without noticing, looking for bug sources in places where they are logically expected to be yet hiding nowhere even close to that area, that misleading can the code be. I've had to face that by no means can I just rely on my gut feeling, and I've picked up a few nice tricks (e.g.: run the application with a coverage tool attached to ensure proper and quick impact analysis area when you are tasked with enhancing the behavior for a button click - nasty surprises can be avoided!).


Scope discipline is a related lesson - stay focused, and only change the parts of the code that you really need to. You might realize that there is a broken csv parser implementation there, which you can rewrite or replace with a standard library while working on some nearby area of the codebase... Don't change it then - it might have quirks you are not aware of, bugs, which are actually required for the daily use of the software. It is going to take much longer than expected to fix that (and fix/discover the unintended consequences). Just make a note, then finish what you are working on, and then raise it as an improvement. If it ain't broken, don't fix it.


Some of these applications require a long time to start, and have tons of dependencies - unit testing was a cute thing I've learnt during my school days, but at the beginning of my career I didn't practice it for the short, quick projects I've worked on. Working with legacy applications pushed me to learn more about automated testing. Of course, legacy code is rather hard to test, so one learns about disciplined refactoring and other techniques to make them testable and ends up learning about (refactoring to) design patterns. The ability to quickly isolate just the bit you are trying to test and stop there has served me well ever since - perfect is the enemy of the good, so just enough is enough most of the time.


Legacy code usually also means being part of the maintenance phase of the lifecycle (which is much longer than the initial development), so one really starts to feel the pain of deployment and quickly learns to automate as much as possible. I've been lucky enough to have had the on-call support experience, where I learned to value useful log messages with appropriate alerting strategies. Even when the app doesn't have an ops team of its own I find some (friends) to ask about support/monitoring requirements they would have for the new features being developed.


Dealing with the change requests also made me realize that most of the time we just can't plan for the future - changes are never to be made at the planned extension points (aka YAGNI). Just keep the code as small and simple as possible - this also results in faster learning curves for new developers, since they don't need to learn complex architectures. Ron Jeffries' Extreme Programming Adventures in C# shows a great example of just enough design for a real program.


After having waded through some obscure cornet of the application, I always feel that I need to share the knowledge with the rest of the team. On the one hand, if I can save them time, the better, on the other hand, I don't want to become a single point of failure. Thus I value code reviews, post mortems/retrospectives, good documentation/fixlogs - knowledge silos are an antipattern!


So, you'll get familiar with antipatterns, smells, which, when read about in the abstract, sometimes are hard to actually understand. Seeing (and suffering because of) them will make you remember, and it will become second nature. Like intention revealing variables, method, and class names that can be trusted.


In summary: Legacy Code is a great learning opportunity both for technical skills and attitude.

Thursday, March 17, 2011

How & Why I do code reviews

Code review is one of the programming practices that I advocate. However, as with any practice, I find understanding and communicating the values and driving forces behind it even more important than the actual practice itself (see: Cargo Cult). So, here is my take on code reviews.

Why?

I'm pretty much against introducing any practice just for the sake of introducing it. Different goals require different means, and without clear goals, it's rather difficult to evaluate whether a practice has helped the team or not. The reason I practice code reviews are:

Knowledge sharing (note: none of the teams I've been part of practiced pair programming, which serves this purpose quite well). It's a way to break the silos that exist within the team, regardless if they are horizontal (UI, server side, data access, etc.) or vertical (this is Jane Doe's component, noone else can touch that for there dragons live). I rather like that when I ask for a holiday my manager doesn't have to consider whether or not there is anyone who could work with my (recent) code.

Mentoring, though could be considered part of knowledge sharing, deserves to stand out on its own. I've found a lot of the concepts I came across first way too abstract, when read in a book, but when explained in context (what you've just written there is known as the Factory pattern...) I get a better and deeper understanding of it.

Culture creation. Again, abstract vs. concrete, talking vs. doing. One can have so many coding guidelines, best practices, etc. documents written. However, these documents usually highlight high level objectives, and almost any concrete implementation can be argued to fit with the description. I much prefer people arguing whether or not a new class should be extracted or not to a dry design guideline setting limits on the number of methods of a class and just checking against that formally (if at all).

Quality Assurance. Are there enough and appropriate tests? Do they makes sense to the next person (I routinely had to rename tests for my reviewers, because I was already too close to the code and didn't communicate intent well enough)? Are actually all tests passing? Haven't you accidentally reinvented the wheel (I've once failed to recognize that what I'm working with is an existing domain concept - the beauty of working with legacy code - so I created another model class for it)?

Last, but not least, catching obvious bugs is a benefit not to be overlooked. Face it: we all make mistakes, that are embarassing in hindsight. Catching them during the development cycle is keeps the embarassment in a smaller circle :)

What?

Computers have been invented to automate tiresome, repetitive, and boring tasks. Spending programmer time on checking tabs vs. spaces, curly brace placement, and other such, easily automated checks (StyleCop, Checkstyle, FindBugs, etc.) is more than waste. If such things are important for you, do a quick check that there are no warnings in the build for these, and move on to the areas where value is added.

When?

Code reviews should be concerned with small, easily digestible chunks, just like commits. That's why I prefer to review commits. Before they are commited. I can only offer nonscientific justification (experience + anecdotal), and no explanation, but my observation is that committing code makes it just as rigid as clay becomes in the oven - it becomes much more difficult to change it. Renaming a method/variable is much easier while it's still in the staging area (maybe the burden of writing another commit message?). This also gives enough time to ensure that tests are run before each commit (run the command line build while the review is ongoing).

Some suggest that developers should ask for code review when they feel it's a riskier change they've made. Since I'm really bad at predicting when I make mistakes (to be frank, I tend not to be able to predict), just to be on the safe side (and to ensure the flow of information), I advocate to review every commit.

How?

There are multiple ways to achieve the same thing, one that I've found working is the following:

  1. the developer who just completed a commitable chunk of work, signals the others (via a team chat window, though ACM ICPC style balloons could work too) that she can be reviewed.
    1. someone from the team picks up the request in the next few minutes - this delay ensures that the reviewer is not interrupted and dragged out from the flow.
    2. If noone picks it up in a few minutes, the developer stashes his work, and continues working until someone becomes available, when they return to the original change set.
  2. the command line build is started on the developer's machine while the nature of changes is described in a few sentences to the reviewer, so the context is set
  3. the reviewer drives the order the changes are reviewed. She asks questions, requests clarifications, which usually result in rename refactorings.
    I tend to start reviewing the tests - first reviewing the names make sense and no relevant corner cases are missed, then the actual test code. Only once I'm happy with that do I turn to the actual code.
    1. if any major flaws (bug, failing test, duplication - anything that cannot be fixed as fast as discussing it) are discovered, the code cannot be commited, and another review is needed (we tend to have that next reviewer be the same person as the one before in order not to slow down the process).
    2. otherwise, the normal checkin dance begins, and the code is committed.

Cons, worries, and pitfalls (real and unreal ones)

Just like any other practice, this takes time to learn and to do. However, I rather prefer the extra few minutes spent discovering duplication than the days it'd take later to remove. Exposing the potential misunderstandings that wouldn't be discovered while just talking again justifies it. If a review takes longer than a few minutes, that signals there is a deeper issue there, which should be discussed - like agile practices promote, we've exposed a problem early. In summary: I think it's worth it.

Co-location, while certainly makes the process easier, is not required. If you are working in a remote team, you probably have appropriate communication channels. A voice call with shared screen is all that's needed.

It takes effort and discipline. If the reviewer doesn't pay enough attention, isn't engaged, it's not going to work. Discussing regularly what kind of problems slipped through the past code reviews and whether they could (should) have been caught could be part of retrospectives.

Just as it's possible to create horrible designs with TDD, it's just as easy to deviate from the architecture, design of the program in small, correct steps. Even when reviewing only small chunks, keep in mind the big picture.

Some teams do post commit code reviews instead, arguing that if we do it in an asynchronous fashion, the work of others is not interrupted, the developer is not slowed down by waiting for others. While I've seen teams practice post commit reviews effectively (as they say, a team of good developers is likely to achieve good results regardless of the process/practices they follow), so I'm not saying that can't work, but I prefer to do it before. In addition to the clay analogy used above, synchronous communication is more effective than asynchronous (at least between humans, between machines its another story). At the end of the day, developers will still need to talk with each other for clarification, so not much time is saved anyway.

Tooling. Most code review software don't support pre commit reviews, however, they have value for other purposes. I like them to start discussions on things I see in the code that don't need immediate changes for the features we are working on now, but should be discussed at some point - either to become a nice to have task, or to actually realize it's a must have refactoring. I find these tools can be more effective than leaving TODO comments in the source code - threaded discussions are nice, and hard to have in code comments. Another area where they shine is architecture/prototype reviews. It's a great way to prepare for a design review meeting. And yes, I do prefer code based architecture evaluation to UML diagrams (though the later are useful when communicating about the architecture), because concrete coded scenarios can bring up more flaws than the white board - see the Non Coding Architect.

And do I need to say it? Just as any other practice, it's no silver bullet!


With regards to silver bullets - nothing is perfect, including this approach and the post itself. Please, do leave a comment pointing out any improvements/problems!




Update: Code Review During Retrospective lays out a great approach for purely educational focused code reviews, I recommend reading that too!