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 :-)