Making cosmetic changes to code is not "refactoring."

I beg to differ :-)

Making cosmetic changes to code is almost the essence of refactoring. According to Fowler (my emphasis):

Refactoring (noun): a change made to the internal structure of software to make it easier to understand and cheaper to modify without changing it's external observable behaviour.

People sometimes only consider structural refactorings like Replace Type Code with State/Strategy and ignore the smaller refactorings like Inline Temp that can be just as important.

Obviously 'easier to understand' is a subjective term (people can argue forever about Inline Temp vs Introduce Explaining Variable) - so take all the 'increases clarity' that follow as 'increases clarity for me'.

First off, the ones that I definitely consider to be refactorings:

Add use strict and warnings

Okay. This is actually not a refactoring in itself, but it's the first part of the common perl big refactoring 'Make Everything Compile with use strict and warnings'. The end result of this refactoring shouldn't alter behaviour and makes things easier to understand and cheaper to modify in the future.

Split uses $_ by default, so don't need to state it

Doesn't alter behaviour. You could even consider this the latter half of Remove Parameter if you look at it a certain way. Increases clarity. Valid refactoring.

Rename $id to $id_to_fetch since it's more descriptive

Doesn't alter behaviour. Increases clarity. Valid refactoring.

Since we're only interested in the school and name we can just extract those from the split ....

Doesn't alter behaviour. Does increase clarity. Valid refactoring.

We can then simplify the name return to 'return $name if $id eq $fetch_id;'

Doesn't alter behaviour. For me, this does increase clarity. Valid refactoring.

Next a bad refactoring:

Since we're not looking at the end of the line we don't need the chomp at all ...

You're quite correct and this only applies if each line has more than two fields. I should have made that assumption explicit in the code or left the chomp in.

Finally bug-fixes gone wrong. I said:

We don't need the close - the filehandle will automatically be closed when it falls out of scope.

And you said.

that's terrible advice. The filehandle should absolutely be closed. It's a global (package) variable and probably won't go out of scope until the program ends. Leaving it open is an undocumented and probably undesired side-effect.

Quite correct. I'm stupid. It was late. Foolish Adrian.

It's instructive that I made this error because of a classic mistake. Let's look at the original code again:

sub get_schoolname( $ )
{
    my $id = $_[0];
    my $schoolname;
    #get groupname from current database  
    open(SCHOOLDATA,"school_data.pdt") or die "Can't open school data 
+file: $!";
    while ( <SCHOOLDATA> )
    {
        chomp($_);
        @split_record = split( /\|/ , $_ );
        print $split_record[0];
        if( $split_record[0] eq $id )
        {    
            $schoolname = $split_record[1];
        }
    }
    close( SCHOOLDATA );
    return( $schoolname );
};
[download]

Now, when I looked at this I actually did notice that it stomped all over the global SCHOOLDATA :-)

What I should have done at this point is write a failing test.

However, due to a little too much hubris some variant of "The code is so simple! Who needs a test to demonstrate the bug" went through my head. Bad Adrian.

Later on I coding an extra close into the loop so that the file handle would be closed after the early exit in my version. As soon as I'd finished I thought - "Well that's pointless. Once you've localised SCHOOLDATA it will be closed when it falls out of scope" and removed both close statements.

I then wrote up my nice little decision to remove the close - and promptly forgot to go add the local that would make it valid!

So, instead of removing one bug I added a new one. Very bad Adrian.

Now, if I'd been a good little developer and written some failing tests:

use Test::More tests => 4;

{
    local *SCHOOLDATA;
    open(SCHOOLDATA, "school_data.pdt") or die;
    is(get_schoolname(2), 'bunny werks', 'existing id found');
    is(tell(*SCHOOLDATA{IO}), 0, 'filehandle still open');
};

{
    local *SCHOOLDATA;
    open(SCHOOLDATA, "school_data.pdt") or die;
    is(get_schoolname(-1), undef, 'bad id rejected');
    is(tell(*SCHOOLDATA{IO}), 0, 'filehandle still open');
};
[download]

I wouldn't have humiliated myself in front of my peers like this :-)

So the moral of the story is:

• Always write a failing test when you find a bug
• Don't put your bug-fixing and refactoring hats on at the same time

By the way, IMHO, his variable names were just fine for the task at hand. I would have chosen them differently and probably closer to the names you picked but that's largely a matter of style. Style is something we all develop for ourselves. It takes time and we find ourselves making niggling little decisions like whether to use $id_to_fetch or $fetch_id as you seem to have struggled with. Personally, I would have stuck with $fetch_id because I think prepositions in variable names are ugly and cumbersome but it's still a style decision.

Didn't have to struggle with the name change. Took longer to type than it did to think about.

While I agree that it is a style decision I think the fact that both of us would have chosen something other than '$id' is a pointer to the fact that the original choice wasn't as clear as it could be. Not wrong - just not as descriptive as it could be.

Rewriting code just because you can is rarely a good idea. You should always have a good reason for rewriting something.

For me increasing the clarity of the code (for some definition of clarity) is a good enough reason.

Even with good reason, you should be careful to keep the functionality equivalent unless you're sure it doesn't work.

Quite correct. Fouled up with the close & chomp changes.

In other words, if it ain't broke...

I tend to disagree. This approach can turn the code base of a large project into a Big Ball Of Mud in a surprisingly small period of time. Over the years I'm spending more and more time Refactoring Mercilessly and finding it works very well.

<Adrian wanders off to write out 'always write a failing test for a bug' 1000 times>

Let's look at that definition with a different emphasis:

Refactoring (noun): a change made to the internal structure of software to make it easier to understand and cheaper to modify without changing it's external observable behaviour.

Cosmetic changes such as renaming variables or eliminating the explicit use of the default variable where it will be used implicitly are not refactorings. They don't affect the structure of the code.

Add use strict and warnings

Okay. This is actually not a refactoring in itself, but it's the first part of the common perl big refactoring 'Make Everything Compile with use strict and warnings'.

The Addition of warnings or strict checking is definitely not a refactoring of any sort. Doing so changes the "external observable behaviour" of the code. I'm not saying it shouldn't be done. I agree that it should.

Split uses $_ by default, so don't need to state it

Doesn't alter behaviour. You could even consider this the latter half of Remove Parameter if you look at it a certain way. Increases clarity. Valid refactoring.

Split uses $_ whether you state it or not. It isn't at all like Remove Parameter because the parameter is used regardless of whether it do explicitly. It is not an "unused parameter." For an inexperienced Perl programmer, reducing it is likely to reduce clarity. For an experienced programmer it should be just as clear either way. In any case, it doesn't change the structure of the code and is not a refactoring.

Rename $id to $id_to_fetch since it's more descriptive

Doesn't alter behaviour. Increases clarity. Valid refactoring.

This does not alter the structure of the code. It is not a refactoring.

Since we're only interested in the school and name we can just extract those from the split ....

Doesn't alter behaviour. Does increase clarity. Valid refactoring.

Whether or not this is a refactoring at all is debatable. The one significant change resulting from this is that some unnecessary values may not be stored. The original did keep unnecessary values in the array when the split resulted in more than 2 elements. If the split always resulted in exactly 2 elements, I would argue that this would not be a refactoring but I don't know that to be the case and suspect it isn't.

We can then simplify the name return to 'return $name if $id eq $fetch_id;'

Doesn't alter behaviour. For me, this does increase clarity. Valid refactoring.

Besides the fact that this isn't exactly "simplified" from the original return $schoolname;, it doesn't really alter the structure of the code.

Although you didn't point it out yourself, you did drop the unnecessary temporary variable, $schoolname, and I suppose that is a minor refactoring.

On the filehandle issue, localizing a typeglob and relying on perl to close your filehandle for you is not, IMHO, good practice. Keep in mind that a localized typeglob localizes all the variables with that name, not just the filehandle. Consider:

perl -le '$A = "a"; {local *A; $A = "b" } print $A'
[download]

To get around that, you would need to use IO::Handle or allow perl to autovivify the handle for you (but only in 5.6.1 and above.) Doing so is often worth it but not for the sole purpose of obtaining a filehandle you don't have to explicitly close. In fact, closing the filehandle explicitly is generally considered a good practice.

Here's another quote from Refactoring: Improving the Design of Existing Code, this from the foreword:

So, what's the problem? Simply this: Refactoring is risky. It requires changes to working code that can introduce subtle bugs. Refactoring, if not done properly, can set you back days, even weeks. And refactoring becomes riskier when practiced informally or ad hoc. You start digging in the code. Soon you discover new opportunities for change, and you dig deeper. The more you dig, the more stuff you turn up. . .and the more changes you make. Eventually you dig yourself into a hole you can't get out of. To avoid digging your own grave, refactoring must be done systematically.

Refactoring is risky. It shouldn't be done informally. It requires a disciplined systematic approach. Here at the monastery I've noticed that the term "refactoring" is both overused and often incorrectly used altogether. Refactoring is not the same as editing or rewriting and we shouldn't further the impression that is.

Refactoring is the practice of restructuring a program by changing its basic components. It is worthwhile to remember the mathematical roots of the word. One might accurately construct the number 24 with the "program" 2**2 * 3 + 6 + 2 * 3 * 1 but refactoring that to 2**3 * 3 results in a program which is shorter, clearer, and more efficient.

-sauoq
"My two cents aren't worth a dime.";

Cosmetic changes such as renaming variables or eliminating the explicit use of the default variable where it will be used implicitly are not refactorings. They don't affect the structure of the code.

Unless I'm misunderstanding, this means that you would not accept Rename Method as a refactoring either. Correct?

I think renaming methods (and variables) can make code "easier to understand and cheaper to modify". I don't understand what advantage making a distinction between lexical changes (renaming, removing implicitly passed parameters, etc.) and other more structural changes (extracting classes, etc.) gives you?

Since Fowler is happy to call Rename Method a refactoring, I'm happy to call renaming a variable a refactoring :-)

We can, of course, disagree whether a particular renaming helps or hinders - but that is a separate issue.

---

The Addition of warnings or strict checking is definitely not a refactoring of any sort. Doing so changes the "external observable behaviour" of the code.

Not necessarily.

For my sins I have, in the past, had to take masses of non-documented, poorly written perl4 scripts and turn them into something sane.

When given a task like this one of the first things I do (after creating a test suite) is make everything compile okay with strict & warnings. The process will go something like this:

1. Run the test suite.
2. Do perl -Mstrict -Mwarnings whatever.pl
3. Fix one of the error messages.
4. Run the test suite again.
5. Tests failing? Undo last change and do it properly.
6. Repeat steps 1-5 until no errors
7. Add use strict; use warnings; to whatever.pl

Lots of nice small steps. Test suite runs properly after each small step. I interpret this as no change in the "external observable behaviour" of the code.

---

Split uses $_ whether you state it or not. It isn't at all like Remove Parameter because the parameter is used regardless of whether it do explicitly. It is not an "unused parameter." For an inexperienced Perl programmer, reducing it is likely to reduce clarity. For an experienced programmer it should be just as clear either way.

Personally I have found that if the $_ is mentioned explicitly then the clarity of the code is improved by either:

• removing it (if it's obvious it doesn't need to be mentioned - it's just visual clutter)
• having an explicitly named variable (if it's not obvious it needs to be named)

However, these sort of issues are influenced a great deal by personal taste and team experience so I'm happy to agree to disagree over the clarity issue.

---

I said:

Since we're only interested in the school and name we can just extract those from the split ....

You said:

Whether or not this is a refactoring at all is debatable. The one significant change resulting from this is that some unnecessary values may not be stored. The original did keep unnecessary values in the array when the split resulted in more than 2 elements. If the split always resulted in exactly 2 elements, I would argue that this would not be a refactoring but I don't know that to be the case and suspect it isn't.

I'm not sure I follow this. How does the number of items pulled off the split affect the behaviour of the function (returning the ID)?

To me the significant change is that we explicitly name what we were interested in (the $id and $name). The reader of the code no longer has to translate $split_record[0] to id and $split_record[1] to school name. This increases clarity and maintainability

---

On the filehandle issue, localizing a typeglob and relying on perl to close your filehandle for you is not, IMHO, good practice. Keep in mind that a localized typeglob localizes all the variables with that name, not just the filehandle.

You are quite correct, but that wasn't an issue in this case (and is very rarely an issue at all in my personal experience - but can lead to some evil bugs when it is).

I know many like having an explicit close. I tend to code so that the scope will close the filehandle for me since it mixes better with exception based code. If you don't have the scope close the filehandle you have to put a catch block around everything that opens a filehandle - which can be a pain.

This is an arguable point I freely admit (and the fact that I forgot the local in the original does make a strong case for keeping the close in from the defensive coding point of view :-).

---

Refactoring is risky. It shouldn't be done informally. It requires a disciplined systematic approach. Here at the monastery I've noticed that the term "refactoring" is both overused and often incorrectly used altogether. Refactoring is not the same as editing or rewriting and we shouldn't further the impression that is.

Amen.

I think my failure to keep my refactoring and debugging hats separate when removing that close demonstrates quite clearly what happens when you let that discipline slip.

However, this doesn't mean that all editing and rewriting is not refactoring. I class renaming methods and variables, along with similar changes that increase clarity without affecting the code behaviour, as refactoring :-)