Refactoring: VFP form calls external PRG, uses Publics for shared data

I’ve inherited another legacy VFP application and am updating a routine to use a new format required by my client’s customer. There is a VFP form that eventually calls a program to gather, format, and export relevant data. One of the first things I need to do is change the form and program so their code can be independently tested. Once that’s accomplished, I’ll change the routines to generate the new format.

The question is, as always with this common situation is whether to pull the code into a “blob” method in the form and change all variables to properties, or call the program from the form with parameters. I realized my usual inclination is to use the former because eventually I would be binding the form properties to properties on future business classes that I’d create down the road. It’s odd how I’ll suddenly think of a usual problem in a different light. This is a routine that’s run four times a year, the new format is needed soon, and the client wants to begin planning for a new application to replace this one. Visual FoxPro will probably not be the tool of choice, so there is no reason to build the Queen Mary when a dinghy will get us across the narrow inlet just fine.

Can you tell I’m prone to make everything into A Work Of Art? Maybe I’m changing after all.

Refactoring: Rename Method Example

Have you tried my refactoring challenge[1]?  Since I missed two days in my first week of the 100 Pushups challenge, I guess I can’t complain if you haven’t tried refactoring. Here’s another example that might prompt you to try it. (Refactoring, not push-ups.)

I’m preparing a simple utility so the code is stranger-stable. In prepping the code, the following lines gave me pause.

WriteCommand( lcNewCommandPrg )

When I have to stop, even briefly, and ask what code is doing, or worse, if I have to divert into another method, I smell a refactoring. At first, I read the code as “SaveCommand, WriteCommand.” Wha..aa? The “Old” in SaveOldCommand() explains it, but it’s buried in the middle of the function name. Similarly the explicitly named variable lcNewCommandPrg passed to WriteCommand is good, but, it comes at the end of the line. I can refactor this code to make it clearer. To do that I used Rename Method. (Whether or not this is the most compelling example, isn’t the point, you kibbitzers out there.)

In a nutshell, renaming a method starts by creating a new method with the new name. Then move the old method’s code into the new method. Finally, change existing calls to the new method. Stay with me: it works on legacy code, too. More on that later. Here are the steps:

  1. Test the code (baseline, right?)
  2. Create a new method with the new name, with the same parameters, and test.
  3. Copy the code from the old method into the new one, and test.
  4. Find all references to the old method and change them, one at a time, to the new method.
  5. Test after every change in step 4.

Yes. Step 4 is a good example of “And then a miracle occurs” programming. But, no fears, this technique is adaptable to more difficult situations. In this case, however, I know it’s safe to remove the old method. After I refactor, the code is:

WriteNew( lcNewPrg )

It’s usually impractical to rename a method other than in early development, isolated utilities, or hidden methods. Legacy code is too hard to search thoroughly and is rarely testable with the degree of assurance that’s required. If code is a control or application that is used by customers I cannot change method names at all. However, I can safely create new methods and change the old ones to call the new.

Consider the following changes to the steps above:

  1. Test the code.
  2. Use Extract Method to move the code from the old method to a new, better named method, and replace the old code with a call to the new method. Test.
  3. Change existing references to the old method, testing after each change for as long as you can stand it.
  4. Test.

“What’s the point of just adding another line of code to execute,” you ask? Consider a class that you will continue to use, or a class that customers use. Even if old code still calls the new method, a more clearly named method will be easier to work with, and the code that calls it will be more readable. Also, you might use BindEvent (VFP) to bind the old method to the new.

For example, years ago I adapted a Visual FoxPro foundation label class to send email. The original class used a method named something unhelpful. I’d added hooks so instances and subclasses could manipulate the email before it was sent. As I used the class in different projects, I wanted a more clearly named method, and so created a SendMail method.

The trouble with removing the old method was that I used the class in several different projects, and while I could have used Agent Ransack to find code references (this was in VFP 7.0, which doesn’t have Code References), I didn’t have time to find, replace, and test all the changes needed. Plus, I really just wanted to make it easier to use in the future. Otherwise it worked just fine as it already existed. So, I chose to the second refactoring and used Extract Method.

Why do I mention this? I want to convince the skeptic that refactoring is flexible, doesn’t require a massive investment in time, and leaves code better but still stable. I’ll keep coming back to these facts time and again. I can’t imagine why anyone wouldn’t want to refactor whenever possible. But then, I can’t imagine why anyone would eat lima beans willingly, either.

[1]No, I’m not really delusional about the size of my audience, its interest in refactoring, or my powers of persuasion. I just like to pretend.

Refactor for readability: replace comments with assertions

Arnon Rotem-Gal-Oz graciously took time to comment on my post Code comments, b-aaaad. He pointed out his recent post Code readability – Documentation vs. Refactoring, which I’ve had marked to write about. I’d gotten lost in some of the 3.0 issues he write about, though.

I agree with Arnon when he says that “while both of these efforts can help satisfy a customer specific requirement for ‘comprehansive [sic] documentation’ they have very little value in making anyone understand anything about your code.” (Emphasis is his.)

IOW, instead of explaining code, make code explain itself. In my own post I suggest a one week challenge for any readers who remain unconvinced of how much of an improvement even simple and minor refactoring can make. I wrote about looking for comments in longish sections of code, and refactoring with Extract Method. Arnon addresses long sections of code, and he gives a detailed example of how to do it. (Even if you don’t code in C#, it’s instructive.)

I find I’m most often refactoring long sections of code, and that Extract Method is the commonest technique I use. Comments are a useful hint in other situations, though, such as when they state a programming assumption. Instead of commenting assumptions, program an assertion.

It’s easiest for me to find examples in Visual FoxPro code because of its untyped variables, and because parameters are optional. VFP programmers are accommodating folk so we sometimes try to make sense of unexpected parameter types. For example,

LPARAMETERS toObject,tcName,tvClass,tvClassLibrary
IF TYPE("toObject")#"O" OR ISNULL(toObject)

It was only after programming in C# for a time that I realized this spirit of cooperation was flawed. Why? Frequently these functions change behavior depending on what is passed: sometimes subtly, sometimes not so subtly. It’s also confusing to the programmer (even if it’s just our future selves) who is trying to understand either the function or a mysterious error that occurs down stream because we’ve tried to protect the programmer from themselves. A non-VFP programmer expressed disgust with this co-dependent style by saying “sometimes an error should cause, uh, an error.”

I’m a convert, and I’ve long since stopped this practice except in rare and exceptional circumstances. Instead, I let bad parameters throw errors. I have an Intellisense script now that I use whenever writing a function or method. The IS script inserts the following code when I type “badparms”:

Assert Vartype( ) = "C" ;
Message "Did you mean to pass a bad parameter to " + ;
Program( Program(-1) -1 ) + "?"

(Obviously I specify the parameter in Vartype() and change the type (“C”) if need be.)

If I pass a bad parameter and it makes it into the client’s hands, the program will throw (and log) the error. When I run the code while debugging or testing, the Assert will help me debug the places I call the function incorrectly.

So, while you’re looking for comments (hello, hello, is this thing on?) look for ones that you can replace with an assertion. It will document the assumption and help you debug.

Code comments, b-aaaad

Jeff Atwood of Coding Horror is on my daily circuit of blogs (which I read right after the daily funnies, ‘natch). Coding without Comments had me singin’ out “amen, brother!”

Comments are a “bad smell” [1] code uses to whisper “refactor me, please, I beg you.” The Problem with Comments and Coders Who Love Them is one of the top five lessons I gleaned from Martin Fowler’s Refactoring [2]. Good comments explain why or how, not what. I love this smell. It’s got everything:

  • Glamor–comments attract the eye
  • Brains–comments encapsulate the solution by the very comment
  • Wit–the refactoring is often simple, quick, and it’s got great ROI

I’m taking the 100 pushup challenge (I’m up to two). Here’s is your challenge, and it’s only for a week. Watch for comments as you work, and use them to identify the places code can be broken down into smaller, more discoverable chunks.

There…right there…that code you’re mucking around in? Unsure how or where to add or change that feature? Is it a long section of code with comments interspersed explaining what different sections of code do? You probably have a case for … da ta daaaa … Extract Method. The general plan is simple enough. The comment will hint at what the new method should be called, and all the code that the comment applies to will go in that new method. In a nutshell:

  1. Test the code (so you know later if something’s gone wrong)
  2. Create a new, empty method, test (yes, it really is that easy to make a bug)
  3. Copy the code to the new method, test. Yes, if you get interrupted, no harm done and you’ll have a good idea of where you where the next time you visit.
  4. Look for variables that are initialized or used elsewhere. Are they used elsewhere? Then, pass as parameters. Otherwise, localize them to the new method.
  5. Hm. I feel like I’m forgetting something…Oh! Right! Test!
  6. Replace (or comment) the code that’s been moved to the new method with a method call. I comment until after the last test.
  7. And, last, class? Yes, very good. Test.

If this time is hard to justify either to yourself or your muckety-muck (hey, more muck), say it’s preparing the ground for adding features. At least you’ll see whether you are in the right section of code. If so, how can the effort be wasted? If you’re in the wrong place…even better. Yes, I mean that. It saves you screwing up some code that was just minding it’s own business (refactoring doesn’t change behavior), and you’ll waste less time the next time you visit.

[1] “Bad smells” are patterns in code that hint, but only hint, there is something violating good coding practice.

[2] Refactoring is the controlled process of changing code without changing behavior so that the overall design is improved. One doesn’t open up code at random and start refactoring willy-nilly. Only a nut would want to change code just to change code. And, yes, I’ve done it, and yes, I was…well…I called myself worse than “nut.” The only reason to touch code is because something needs adding or changing. Duh.