REFACTORING
Tests against spect
Technical
debt!!!
2 Weird Tricks of Agile:
You start small
Then you add something
Then you add something else:
DELOCALIZED CHANGE!!!!!
And then just one more small thing
And then build more!
And then maybe you add more discounts (like a seniors discount)... and more kinds of statements (simple statement, long form statement… etc)
The code changes...
And code issues emerge (duplication, rigidity, lack of reusability, mess)
If it’s larger scale it’s called Technical Debt
If it’s a cosmetic issue you can point to, it’s called a Code Smell
But really technical debt and code smells refer to the same thing: code that needs be restructured, before changes can be safely made or new code can be added.
Technical debt/Code smells are expected in agile methodologies!
A basic principle emerges: (emergent abstraction)
Any time you are trying to make a change, and it is difficult because either...
...you run the risk of introducing bugs!
To avoid this: REFACTOR BEFORE you make the change introduce abstraction to either
This describes most (all?) code smells
What is REFACTORING?
A predictably meaning preserving code transformation.
Typical refactoring workflows
In an industry setting (where you are pushing code to production for release):
In a personal project, which you’ll be sharing with others on, say, Github:
How to refactor?
“apply a refactoring”
Change is desired
Don’t refactor...
Common Code Smells
“apply a refactoring”
Means restructure your code through one of these refactoring types
Common Code Smells
Note: Divergent change is only within one class
MAGIC NUMBERS
How do we deal with Magic Numbers?
double potentialEnergy(double mass, double height) {
return mass * 9.81 * height;
}
Any use of an actual number right in the code
Maybe make this a constant
Push your meanings into the type system.
double potentialEnergy(double mass, double height) {
Float gravitationalConstants = 9.81;
return mass * gravitationalConstant * height;
}
Magic Numbers
What to do?
These are not constants
maybe make them variables??
Fee
Days limit
Extra fee multiplier
Extra fee adjustment
Transforming Magic Numbers
No magic numbers...
Common Code Smells
COMMENTS
Comments are NECESSARY as specs.
So basically, “comments needed to explain code” do not refer to specifications (requires/modifies/effects) or “javadoc”. Those are integral to writing good code. Don’t stop specifying your methods!
Code smell: Needing comments to explain code
These comments are actually okay!
They are “TODO”s, not explanatory
Why are explanatory comments a problem?
People don’t respect explanatory comments!
You can change code, and not change the explanatory comments -- this leads to documentational drift
Explanatory comments don’t necessarily move with code (you can copy and paste code but leave an explanatory comment behind. Or implement something between the explanatory comment and the implementation, meaning the explanatory comment is now a little farther away!
Refactoring to fix explanatory comments
Extract method and give that method a useful name!
People respect method names -- people generally will not willingly change the contract of a method
Method “envelopes” move with code -- you invoke the code by calling the method, not by pasting the code. Also, you will always know which method you are in.
EXTRACT METHOD REFACTORING:
Let’s do an example in code...
What we did: Extracted methods that are named to reflect the comments
And keep going until you’re done
EXTRACT METHOD REFACTORING:
METHOD AESTHETICS PRINCIPLE:
All lines in a method should be at the same level of abstraction.
Common Code Smells
LONG METHOD
Method design aesthetics
They should do just one thing … if you’re thinking at the right level of abstraction
Method design aesthetics
They should do just one thing … if you’re thinking at the right level of abstraction
Method design aesthetics
Don’t make them too long…
(this isn’t the whole method)
Method design aesthetics
Can we fix this???
Method design aesthetics
All the same level of abstraction
Try it yourself:
Method design aesthetics
It’s not crazily long, but it does have identifiable regions.
Grab the code here:
tps://drive.google.com/open?id=1TLUslGeyPbJmcQUXI-8DzrZKsNI4bUe1
Then submit your new init method here for activity credit:
When faced with a long method
sample rate
Frames per wave
Harmonics and sound
Loop count
EXTRACT METHOD REFACTORING:
Common Code Smells
FEATURE ENVY
Code smell: Feature Envy
Means: The method is in the wrong class
When you find yourself wishing you had access to lots of fields from another class, probably the method you’re implementing should live in that class.
Law of Demeter: Too many dereferences mean the method is probably in the wrong spot
Code smell: Feature Envy
Means: The method is in the wrong class
When you find yourself wishing you had access to lots of fields from another class, probably the method you’re implementing should live in that class.
Law of Demeter: Too many dereferences mean the method is probably in the wrong spot
Move all of this into fido.reactToBeingChased()
Fixing Feature Envy
Refactoring: Move the method (carefully)
Now you try
Open the video store project, and look at the refactor this code into Customer.getThisAmount method and then it’s still in the wrong spot….
A call to getB().c() means you should likely move the behaviour in this method to B.
determineAmount()
Now you try
Open the video store project, and look at the refactor this code into Customer.getThisAmount method and then it’s still in the wrong spot….
A call to getB().c() means you should likely move the behaviour in this method to B.
determineAmount()
determineAmount()
determineAmount()
determineAmount()
Common Code Smells
SWITCH ON TYPE
code smell: switch statement/typed conditional
a conditional that chooses different behaviour depending on the type of an object (or a weird string representation of that type)
Class Bird:
code smell: switch statement/typed conditional
a conditional that chooses different behaviour depending on the type of an object (or a weird string representation of that type)
getSpeed
getSpeed
getSpeed
Class Parrot:
code smell: switch statement/typed conditional
refactoring: replace conditional with polymorphism
b.getSpeed()
Bird class
Each case gets its own class
Each class gets its own version of the method
return getBaseSpeed();
Each method gets the contents of the if statement
Instantiate the subtype
Call the method on the instance
Rerun tests
Tip: Copy over all the contents from the original class, then delete the parts you don’t want. This includes the if-statements you don’t need.
You will likely, at least temporarily, have to put in a call to the super constructor
2) Call the constructor for the new subclasses
Rerun tests
3) Copy the guts of the methods in the super class, paste them into the new subclass. Cut the bits I don’t need.
3b) and then comment them out in the super class, (this should actually be two steps)
rerun tests: The overriding methods should take over!
4) Make the original class abstract, and make the original methods abstract
Rerun tests.
5) Get rid of disused type variable (and associated constructors)
Note that the type variable is now unused!
Erase it and the associated constructors
Common Code Smells
Code for this portion: https://drive.google.com/file/d/0Bz_6Fc8r1CaHRllncUNnNW0tN28/view?usp=sharing
SAME METHOD IN SUBCLASSES
Smell: Same method in two classes
Refactoring: Pull up method
If the names are different but the contents the same:
Salesperson
Salesperson
NamedEmployee
Salesperson
Engineer
Sanitation
getName
Employee
Same code in two classes (w/o shared supertype)
Class A
abc()
Class B
abc()
Class SuperAB
Does duplication between classes always mean you should pull a method, and maybe introduce a supertype.
If A and B already have a shared supertype then YES GO FOR IT.
MUST DO IT ONLY if supertype is a real LSP style supertype
Consider doing DELEGATION -- a uses relationship.
Class A
abc()
Class B
abc()
Inheritance
(what we just saw)
Composition or Delegation
Same code in two classes (w/o shared supertype)
Class A
Class B
Class SuperAB
abc
Does duplication between classes always mean you should pull a method, and maybe introduce a supertype.
If A and B already have a shared supertype then YES GO FOR IT.
MUST DO IT ONLY if supertype is a real LSP style supertype
Consider doing DELEGATION -- a uses relationship.
Class A
abc(){
o.abc()
}
Class B
abc(){
o.abc()
}
Class Other
abc()
Make a local variable of Other, and call other.abc()
Inheritance
(What we just saw)
Composition AKA Delegation
a.abc()
Restaurant
serveFood
Plane
plane.serveFood();
Jet
restaurant.serveFood();
FlyingThing
serveFood
Same code in two classes (w/o shared supertype)
Class A
abc(){
o.abc()
}
Class B
abc(){
o.abc()
}
Class Other
abc()
a.abc()
Class A
abc(){ o.abc() }
Common Code Smells
Almost
ALMOST
DUPLICATE
CODE
ALMOST duplicate code
getBillableAmt(){
doSpecialStuff();
doRed
}
Abstract doSpecialStuff;
doSpecialStuff(){
doPurple
}
doSpecialStuff(){
doGreen
}
ALMOST duplicate code
rs.getBA()
if instanceof RS:
Do fuchsia
If instanceof LS:
Do green
base = unit * rate * getBaseMultiplier
tax = base * taxrate * getTaxMultiplier
Don’t do this!
Because it doesn’t ENFORCE that the same pattern is applied
doRed(){
}
getBillableAmt(){
doPurple
//super.doRed
doYellow
}
getBillableAmt(){
doGreen
super.doRed
}
Apply refactoring: Move to template method
Walker
Field namestring
walk() {printMessage(); ...}
abstract printMessage() {print “namestring is walking”};
Dog
constructor: nameString = ‘dog’
// walk(): print(‘I’m a dog walking’); super;
// printMessage: print(“I’m a dog walking”);
Cat
// walk: print(“I’m a cat walking”); super;
printMessage: print(“I’m a cat walking”);
Animal
walk(): does walking stuff;
LoudDog
walk: makes lots of noise; super.walk;
LoudCat
walk: slinks instead - doesn’t even use the supertype’s walk
Don’t keep the pattern in the subtypes
Class A
method():
print a
print bla
Class B
method():
print a
print eek
Class SuperAB
abstract method()
Class A
method():
print bla
printy()
Class B
method():
printy()
print eek
Class SuperAB
abstract method()
printy():
print a
The problem here, is that there is nothing FORCING the pattern to remain the same
A a = new A()
a.method()
client
Keep the template in the supertype
Class A
method():
print a
print bla
Class B
method():
print a
print eek
Class SuperAB
Class A
details():
print bla
Class B
details():
print eek
Class SuperAB
method()
print a
details()
ENFORCE that behavioural pattern remains the same, but details change
A a = new A()
a.method()
client
Template
Method
<<<<<<<<<<<<<
Enforces same pattern in all subtypes
Let’s do an example in code...
1) Identify what is unique, and what is the same
Unique to this class
Unique to this class
2) Pull up duplicate behaviour
3) Make an abstract method to placehold the unique elements,
Then override to provide specific behaviour
Common Code Smells
INTER-CLASS SMELLS
Experiential symptoms of code smells … issues that you notice while trying to change the code
The issues that we have seen up to this point are all “static” -- they can be readily identified just by looking at the code.
Code smells are often not so neatly identifiable. In reality, they are often identified experientially -- as you try to change the code, or in the process of living with the code.
Two experiential symptoms that point to perhaps more elusive code smells are divergent changes and shotgun surgery.
Both of these often point to the need for a new class to abstract the tangled or scattered design elements.
symptom: divergent changes
If you look at a class and say, "Well, I will have to change these three methods every time I get a new database; I have to change these four methods every time there is a new financial instrument," you likely have a situation in which two objects are better than one. That way each object is changed only as a result of one kind of change. Of course, you often discover this only after you've added a few databases or financial instruments.
symptom: divergent changes
will need to change whenever the investments implementation is changed
will need to change whenever the loans implementation is changed
will need to change every time the printing implementation is changed
Code Smell: One class is actually two
Refactoring: Extract class
Delegate as appropriate -- as fields or just dependencies.
Knows about relationship of any strength (field? Local? injected?)
Making the decision between DELEGATION and INHERITANCE
If the new classes feel like KINDS OF the original class ---> INHERITANCE
If the new classes feel like PARTS OF the original class ---> DELEGATION (using fields instead of subtyping)
We would not be able to say:
InvestmentAccount p = new PrinterThingy()
p.manageInvestments() //Can’t be substituted -- so can not be a subtype
symptom: divergent changes
code smell: one class is actually two
There is more than one field related to the telephone number, which gives it more weight. There might be additional methods supporting the phone number.
Code Smell: One class is actually two
Refactoring: Extract class
And delegate!!!
Person p = new Person()
p.getTelephoneNumber()
Code Smell: One class is actually two
Refactoring: Extract class
Seems to be about billing, not about being a customer!
Code Smell: One class is actually two
Refactoring: Extract class
1
The approach here is typically to make the class, then copy the methods over, then get them to compile, then alter the invocation so the new methods are being called, and run tests. Only then do you delete the old methods.
symptom: shotgun surgery
Catchy synonym for Delocalized Change
AKA Scattered Change
Code smell: Shotgun Surgery
Refactoring: move method; move field
A
subA
Summary of Entire Topic:
& Underpinning Principle
Refactoring (The Ruby Edition)
by Fields, Harvie and Fowler
Makes an important statement on OO design:
That classes not just be considered in terms of their apparent responsibility, but must also be centered around their empirically observed relevance!
Suggests we decompose your system to align with changes
Resources
Examinable skills
You should be able to…
Refactoring