From Deobald

General Comments

• I haven't got a copy of the original 'Refactoring' handy in the Pune office (someone has it out on loan). I apologize if any of my comments below critique text that's been in the wild for years.

• Some of the catalog sections don't contain examples. It seems like this is intentional (the missing examples are for trivial refactorings, or refactorings which invert the preceding refactoring -- Push Down Method, Push Down Field). Is it?

• Toward the end of the book there are sections which are missing whole chunks or simply not converted. If you'd like me to go over these again once they're finished, please let me know: http://deobald.ca/steve/

Chapter 1

No comments.

Chapter 2

Defining Refactoring

"When you refactor, you make a point of not adding function; you only restructure the code. You don't add any tests (unless you find a case you missed earlier); you only restructure the code. You don't add any tests (unless you find a case you missed earlier); you only change tests when you absolutely need to in order to cope with a change in an interface."

• This passage is confusing, is it not? Waiting for a copy of the original so I can check if it was always this confusing. :-)

Chapter 3

Shotgun Surgery

• The title on this section is mis-labeled as "Divergent Change".

Primitive Obsession

"They may mean tables in a database, or they may be awkward to create when you want them for only one or two things."

• This sentence makes sense, but only after I read it a few times with the sentence which proceeds it ("Records always carry a certain amount of overhead."); it feels like the two verb phrases need to be parallelized.

"If you have a group of fields that should go together, use Extract Class. If you see these primitives in parameter lists, try a civilizing dose of Introduce Parameter Object."

• These two options were just mentioned in Data Clumps (although not so succinctly). I don't have any suggestions for a solution, but it would be nice to see these unified as they'd probably appear on the same sheet of paper in print.

Switch Statements

• The first sentence does touch on Ruby by including a parenthetical mention of 'case', but wouldn't it be more Rubyish to change all occurrences of 'switch' to 'case' and rename the section "Case Statements"? Or is that going too far?

Temporary Field

"You may also be able to eliminate conditional code by using Introduce Null Object to create an alternative component for when the variables aren't valid."

• "...for when..." always makes me a bit uneasy. [Needs confirmation with original text.]

"The new object is a method object [Beck]."

• Should "method object" be capitalized?

Message Chains

"You can do this at various points in the chain. In principle you can do this to every object in the chain, but doing this often turns every intermediate object into a middle man."

• I'm not sure if the repetition of "do this" variants is intended as a rhetorical device, but I find it leaves the passage somewhat dry. If it is intentional, ignore me.

Inappropriate Intimacy

"If it's time to leave home, apply Replace Delegation with Inheritance."

• If it's time to leave home, wouldn't we apply Replace Inheritance with Delegation?

Alternative Classes with Different Interfaces

• This section feels a bit terse. I had to read the passage a few times to realize how much material there was backing each sentence; without context, said sentences don't make much sense on their own. There is a substantial synaptic leap between the introduction (regarding method signatures) and the third sentence, which discusses how this is paralleled in the world of classes. The last two sentences describe the solution, but don't offer the reader a sense of the details or how these actions relate:

"Keep using Move Method to move behavior to the classes until the protocols are the same."

• "the classes" is vague. This is the first time we've seen interfaces described as "protocols" in the book, so elaboration might be worthwhile. Could one replace "Keep using" with a description of how I should "keep using" instead? Also, it's unclear why I'm using Move Method.

"If you have to redundantly move code to accomplish this, you may be able to use Extract Superclass to atone."

• Again, "redundantly move code" is a bit vague; this sentence feels like it could be broken into composite pieces and/or have its pace slowed. It may just be the use of passive voice causing this, though.

Incomplete Library Class

• I'm being pedantic here, but this section doesn't give a direct description of what an "incomplete library class" actually is. It should be obvious what this means from the current content, though, so maybe a straight-forward definition isn't required?

Chapter 4

Building Tests

• This section uses a great deal of first person. I'm sure it's intentional, but it gets a bit repetitive toward the end.

"There's a tremendious productivity gain as a result."

• tremendous

"Writing the tests is a lot of extra code to write."

• The writing of the tests is a lot of extra code to write? Or tests themselves are a lot of extra code to write?

"It also means you have a clear point at which you are done coding-when the test works."

• Should be em-dash.

The Test::Unit Testing Framework

"The automatic test is the assert_equals method."

• Should be 'assert_equal'.

 "
 def test_read_4th_character
   contents = File.read('data.txt')
   assert_equal 'd', contents[3,1]
 end
 "

• Should this example include 'require test/unit' for completeness?

"A difference between test code and production code is that it is okay to copy and edit test code. When dealing with combinations and alternatives, I often do that. First take "regular pay event," now take "seniority" and "disabled before the end of the year." Now do it without "seniority" and "disabled before the end of the year," and so on. With simple alternatives like that on top of a reasonable fixture, I can generate tests very quickly."

• This paragraph is a bit confusing. The "First take...", "take...", and "Now do it without..." verb phrases are ambiguous. What am I "taking" and "doing"? "simple alternatives ... on top of a reasonable fixture" won't make much sense to someone who's new to testing.

Chapter 5

• No comments.

Chapter 6

Extract Method

"(If your language allows output parameters, you can use those. I prefer to use single return values as much as possible.) "

• Should this be made Ruby-specific?

Replace Method with Method Object

"In this case you reach deep into the tool bag and get out your method object [Beck]."

• Should 'Method Object' be capitalized?

Substitute Algorithm

• I honestly can't remember if this section was so open-ended in the original text (or if it was even there), but it feels like a Ruby example or two might help. The code provided at the beginning could be elaborated on, perhaps? Or maybe this section just needs to remain open-ended due to its mass.

Chapter 7

Move Field

• "Figure 7.1" here is repeated from "Move Method"

"I may choose to move the methods; this decision based on interface."

• Is this sentence missing a verb?

"

 class Account...
   attr_accessor interest_rate
 
   def interest_for_amount_days amount, days
     interest_rate * amount * days / 365
   end

That way I only need to do the redirection for the accessors:

 def interest_for_amount_and_days amount, days
   interest_rate * amount * days / 365
 end
 
 def interest_rate= arg
   @account_type.interest_rate = arg
 end
 
 def interest_rate
   @account_type.interest_rate
 end

"

• Personally, this section might feel clearer if it looked something like this:

 class Account...
   attr_accessor interest_rate  # bold
 
   def interest_for_amount_days amount, days
     interest_rate * amount * days / 365
   end

That way I only need to do the redirection for the accessors:

 class Account...  # list the class again
   # could use a strikeout, as in "Replace Data Value with Object"
 
   def interest_for_amount_and_days amount, days
     interest_rate * amount * days / 365
   end
 
   def interest_rate= arg
     @account_type.interest_rate = arg
   end
 
   def interest_rate
     @account_type.interest_rate
   end

• Actually, now that I look at it again, the original bold seems right (with my Ruby hat on straight). Given that it didn't look right upon first read, though, I'll leave this comment in here. I still think listing the class in the altered example is more obvious to someone reading the text quickly. The rest of this section was a very pleasant read - kudos.

Hide Delegate

• My UML is complete crap, but shouldn't the arrows used for inheritance differ from those used for dependency?

"

 # private String _chargeCode; # What to do with this noise? I prefer 06_02

"

• Buh?

Chapter 8

• The intro to Chapter 8 is labeled with "Organizing Data > Self Encapsulate Field"

Replace Data Value with Object

"Now I look at the methods on order..."

• "order" would be clearer if capitalized. ...Ack. I see lower-case is the convention in the rest of the prose. You have my vote. (Actually, in prose referencing code, I prefer a fixed-width font to denote the quotation.)

Change Value to Reference

"You do need to tell whether two of the objects are equal, so you need to override the eql? method (and the hash method too)."

• Again, it would be nice if code references ("eql?" and "hash") were in another font.

Change Reference to Value

"In this case the object is immutable, so the next step is to define an equals method:"

• "equals"? or "eql?"? -- same for the text below the code sample.

Encapsulate Collection

"Add an add and remove method for the collection."

• Could be "Add [add] and [remove] methods for the collection." where square brackets denote a font for quoted code.

Replace Type Code with Subclasses

"This situation usually is indicated by the presence of case-like conditional statements. These may be switches or if-then-else constructs."

• Should this be Ruby-specific?

Chapter 9

Remove Control Flag

"

 set done to false
 while not done
   if (condition)
       do something
       set done to true
   next step of loop 

"

• Perhaps mention that this is pseudocode.

"Another approach, also usable in languages without break and next, is as follows:

Test after each replacement.

Even in languages with a break or next, I usually prefer use of an extraction and of a return."

• I assume "Test after each replacement." is missing another step preceding it (or an explanation of what it is we're replacing). The sentence following that point is also a bit confusing: "Even in languages with a break or next, I usually prefer use of an extraction and of a return." We were just talking about languages _without_ a break or next feature. Are we talking about languages with or without here? "I usually prefer use of an extraction and of a return" feels a bit stilted. Is it possible to describe what is meant by "extraction"? If not, reducing the end to "...use of an extraction and return." might read better.

 return found;

• Is there a reason for the semi-colon? Or the 'return', for that matter?

Chapter 10

Separate Query from Modifier

"To separate the query from the modifier, I first need to create a suitable query that returns the same value as the modifier does but without doing the side effects."

• Is the "doing" in "...without doing the side effects." superfluous?

Parameterize Method

• In the less obvious case, is 'last_usage' an undeclared local variable, method, or attribute? The latter 2 cases are valid given that not all source code is shown, but still somewhat confusing.

Replace Parameter with Method

"I can now get rid of the temp:

 def price
   base_price = @quantity * @item_price
   discounted_price(base_price)
 end

"

• I've noticed some examples like this simply mention the removal of code, while others actually cross out removed code to display its removal. I find the cross-outs easier to read, but is there a reason for doing one or the other?

Replace Exception with Test

"Exceptions should be used for exceptional behavior-behavior that is an unexpected error."

• Should be em-dash.

"Now I can test. If all goes well, I can remove the try block completely:"

• Should this still be referred to as a 'try' block?

Chapter 11

Pull Up Method

"If the feature is a method, you can either generalize the other method or create an abstract method in the superclass."

• What is meant by "generalize the other method"? "abstract method" ...in Ruby? Wups. I see a comment in the example at the bottom referring to abstract methods in Ruby. I'll leave this comment in here for the time being, since this needs to be revisited.

"I can't move the method up into the superclass, because chargeFor is different on each subclass."

• 'charge_for', not 'chargeFor'

"Then I can copy create_bill from one of the subclasses. I then remove the create_bill method from one of the subclasses, and test. I then remove it from the other and test:"

• I assume this section is not finished? Or was it just forgotten?

Extract Subclass

"At this stage I have not changed the type of the variable; I have changed only the type of the constructor. This is because I want to use the new type only where I have to."

• With a Ruby example, this sentence doesn't make much sense.

"Once I've compiled and tested..."

• The entire Extract Subclass section has a real Java feel to the prose. Whoever Rubified it might want to go over it again.

 class JobItem...
   #attr_reader :employee # method removed and pushed down

• This example feels inconsistent with the rest of the book. Usually a prose description or a strikeout is used.

"That I can't use it because a method uses the data is a signal for more work on the methods, either with Push Down Method or Replace Conditional with Polymorphism."

• "use it" - what is "it"? What "is a signal"? This sentence is a bit of a run-on and needs slicing. It could stand restructuring in the active voice.

Extract Superclass

"This change would involve making a corresponding change in the name addStaff and altering the parameter to be a party. The final change would be to make the headCount method recursive. I could do this by creating a headcount method for employee that just returns 1 and using Substitute Algorithm on the department's headcount to sum the headcounts of the components."

• 'addStaff' should be 'add_staff', 'headCount' should be 'head_count'.
• The entire paragraph feels like a bit of a tangent, but given the method names I assume it was there in the original book
• The last sentence runs on for a bit; could you cut it in two instead?

Extract Interface

• Given that this section doesn't make sense in Ruby (and the examples are still in Java), I'll assume the intention is to cut it from the Ruby Edition.

Collapse Hierarchy

"list("Choose which class is going to be removed: the superclass or the subclasses.", "Use Pull Up Field and Pull Up Method or Push Down Method and Push Down Field to move all the behavior and data of the removed class to the class with which it is being merged.", "Compile and test with each move.", "Adjust references to the class that will be removed to use the merged class. This will affect variable declarations, parameter types, and constructors.", "Remove the empty class.", "Compile and test.")"

• What the hell?

Form Template Method

"while the htmlStatement does them in HTML:"

• 'html_statement'

"For those trying this from the example, I also had to add a getRentals method to customer and relax the visibility of total_charge and total_frequent_renter_points."

• 'get_rentals'

"I compile and test and then continue with the other elements."

• "compile"?

"Figure 11.2. Classes after forming the template method"

• Figure is missing.

Replace Inheritance with Delegation

 private Vector _vector = this;

• Left in by accident.

"Once I've completed these subclass methods, I need to break the link to the superclass:"

• The colon implies a code example should follow here.

"If I forgot to add a delegating method, the compilation will tell me."

• Will it? (Most of Chapter 11 could probably use an author review to ensure all direct Java references in prose, such as this, are removed.)

Replace Delegation with Inheritance

"You can use Extract Interface in a similar way."

• Assuming the Extract Interface section will get the ax (axe? damn deodorant branding has confused my spelling), I assume this should go as well.

"Compile."

• There are a lot of references to compilation in Chapter 11. Is there a tool or testing strategy that can take the place of these steps? Or should the book simply drop them entirely?

"Compiling at this point alerts me to any method clashes."

• Whoever wrote this chapter: You're killing me here.

"I must remove all simple delegation methods such as getName and setName. ... In this case this means removing getName and setName from Employee."

• 'name' and 'name=', respectively.

"Once I've got the class working, ..." and "Once I've got rid of all methods that use delegate methods..."

• It seems like other chapters/sections would have included example code for these steps. Were they left out intentionally? (They seem obvious to me, but one assumes the reader hasn't performed Replace Delegation with Inheritance before.)

Chapter 12

Tease Apart Inheritance

• Figures 12.5, 12.6, 12.7: Are the brief descriptions around these figures sufficient in describing what is happening internally? (I'm actually inclined to say yes, but if anyone reviewing the book feels the jumps between figures are a bit stifling, more text or a code example may help.)

Separate Domain from Presentation

"The Rails framework naturally leads programmers to separate code into the model-view-controller hierarchy - it comes with folders to put each of these three different kinds of code, and an in-built Object Relational Mapper - ActiveRecord."

• In a book I'd expect to see full names for references, such as "Ruby on Rails" -- perhaps this is too verbose/redundant in this case.
• To me, "...a built-in Object Relational Mapper..." reads more smoothly than "...an in-built Object Relational Mapper..."

"

• Identify code in the controllers...

...

• Use Move Method to move it to the domain object.

...

• Identify code in the views...

...

• Use Move Method to move it to the domain object.

"

• It may be worth mentioning here that this refactoring likely requires Extract Method as well. Usually the shortcuts which lead to domain logic landing in views/presenters/controllers also mix up presentation and domain logic within methods.

• The example doesn't explicitly state that it's Rails-specific. Someone familiar with Ruby but unfamiliar with Rails will be quite confused by this. (Unlikely in 2007, but show knows how we'll use Ruby in 2010?)

Chapter 13

Resources and References for Refactoring

• This section is poorly formatted in HTML; could just be an issue with the formatter?

Chapter 14

• ...Chapter 14?

Chapter 15

"You see a big goal-a host of subclasses can be eliminated."

• Again, this should be an em-dash. Perhaps the cause of all these converted em-dashes is the HTML formatter again? I'm guessing this sentence wasn't re-written. :)