22 Code Smells What we don’t want to see in your code • Inappropriate naming • Long method • Comments • Long parameter list • Dead code • Switch statements • Duplicate code • Speculative generality • Primitive obsession • Oddball solution • Large class • Feature Envy • God class • Refuse bequest • Lazy class • Black sheep • Middle Man • Contrived complexity • Data clumps • Divergent change • Data class • Shotgun surgery 49
Bloaters Bloaters are code, methods and classes that have increased to such gargantuan proportions that they are hard to work with. Single responsibility principle violated Large class Long method (> 20 LOC is usually bad) https://github.com/dianaelmasri/FreeCadMod/blob/master/Gui/ViewProviderSketch.cpp Data Clumps Primitive Obsession Symptoms of Bad Design Long Parameter List https://sourcemaking.com/refactoring/smells 50
Primitive obsession public Class Car{ red, green, blue; private int paint(int red, int green, int blue){ public void this.red = red; this.green = green; this.blue = blue; } } public Class Car{ private Color color; public void paint(Color color){ this.color = color; } } 51 https://www.slideshare.net/mariosangiorgio/clean-code-and-code-smells
Data Clumps bool SubmitCreditCardOrder (string creditCardNumber, int expirationMonth, int expirationYear, double saleAmount) { } bool Isvalid (string creditCardNumber, int expirationMonth, int expirationYear) { } bool Refund(string creditCardNumber, int expirationMonth, int expirationYear, double Amount) { } 52
bool SubmitCreditCardOrder (string creditCardNumber, int expirationMonth, int expirationYear, double saleAmount) { } bool Isvalid (string creditCardNumber, int expirationMonth, int expirationYear) { } bool Refund(string creditCardNumber, int expirationMonth, int expirationYear, double Amount) { } class CreditCard { private: string creditCardNumber;, int expirationMonth; int expirationYear; }; bool SubmitCreditCardOrder ( CreditCard card, double saleAmount) { } bool Isvalid (CreditCard card) { } bool Refund(CreditCard card , double Amount) { } 53
Problem: Hard to understand , change, and reuse 54
Long Parameter List 55
Long Parameter List 56 https://sourcemaking.com/refactoring/smells/long-parameter-list
Payoff • more flexible thanks to use of objects instead of primitives. • Better understandability and organization of code. Operations on particular data are in the same place, instead of being scattered. • No more guessing about the reason for all these strange constants and why they are in an array. 57
Object-Orientation Abusers All these smells are incomplete or incorrect application of object-oriented programming principles. Should use Polymorphism Switch Statements Alternative Classes with Poor class hierarchy Different Interfaces Refused Bequest https://sourcemaking.com/refactoring/smells 58
Switch statements • Why is this implementation bad? How can you improve it? class Animal { int MAMMAL = 0, BIRD = 1, REPTILE = 2; int myKind; // set in constructor ... string getSkin() { switch (myKind) { case MAMMAL: return "hair"; case BIRD: return "feathers"; case REPTILE: return "scales"; default: return "integument"; } } } http://slideplayer.com/slide/7833453/ slides 59 to 62 59
Switch statements Bad Implementation because • A switch statement should not be used to distinguish between various kinds of object • What if we add a new animal type? • What if the animals differ in other ways like “Housing” or “Food:? 60
Switch statements • Improved code: The simplest is the creation of subclasses class Animal { string getSkin() { return "integument"; } } class Mammal extends Animal { string getSkin() { return "hair"; } } class Bird extends Animal { string getSkin() { return "feathers"; } } class Reptile extends Animal { string getSkin() { return "scales"; } } 61
Switch statements • How is this an improvement? • Adding a new animal type, such as Insect does not require revising and recompiling existing code • Mammals, birds, and reptiles are likely to differ in other ways : class ”housing” or class “food” But we’ve already separated them out so we won’t need more switch statements we’re now using Objects as they were meant to be used 62
Refused bequest Subclass doesn’t use superclass methods and attributes public abstract class Employee{ private int quota; public int getQuota() ; ... } public class Salesman extends Employee{ ... } public class Engineer extends Employee{ ... public int getQuota(){ throw new NotSupportedException(); } } Engineer does not use quota. It should be pushed down to Salesman 63
Refused Bequest Inheritance (is a …). Does it make sense?? Delegation (has a…) 64
Refused Bequest 65
Refused Bequest How this is an improvement? • Won’t violate Liskov substitution principle i.e., if inheritance was implemented only to combine common code but not because the subclass is an extension of the superclass. • The subclass uses only a portion of the methods of the superclass. No more calls to a superclass method that a subclass was not supposed to call. 66
Dispensable Something pointless and unneeded whose absence would make the code cleaner, more efficient and easier to understand. Comments That isn’t useful Duplicate Code Dead Code Predicting the future Speculative Generality Lazy class Class not providing logic https://sourcemaking.com/refactoring/smells 67
Comments Explain yourself in the code Which one is clearer? (A) //Check to see if the employee is eligible for full benefits if((employee.flags & HOURLY_FLAG)&&(employee.age > 65)) (B) if(employee.isEligibleForFullBenefits()) 68
Duplicate Code: In the same class Refactor: Extract method int a [ ]; int calcAverage (int* array, int size) { int b [ ] ; int sum= 0; int sumofa = 0; for (int i = 0; i<size; i++) for (int i=0; i<size1; i++){ sum + =array[i]; sumofa += a[i]; return sum/size; } } int averageOfa= sumofa/size1; …. int a[]; int sumofb = 0; int b[]; for (int i = 0; i<size2; i++){ int averageOfa = calcAverage(a[], size1) sumofb += b[i]; int averageOfb = calcAverage(b[], size2) } int averageOfb = sumofb/size2; https://www.slideshare.net/annuvinayak/code-smells-and-its-type-with-example 69
Duplicate Code : In different classes • Example: consider the ocean scenario: • Fish move about randomly Fish • A big fish can move to where << abstract >> move() a little fish is (and eat it) • A little fish will not move to where a big fish is BigFish LittleFish • General move method: move() move() public void move() { choose a random direction; // same for both find the location in that direction; // same for both check if it’s ok to move there; // different if it’s ok, make the move; // same for both } http://slideplayer.com/slide/7833453/ 70
Duplicate Code Refactoring solution: • Extract the check on whether it’s ok to move • In the Fish class, put the actual Fish move() method move() <<abstract>>okToMove(locn):boolean • Create an abstract okToMove() method in the Fish class BigFish BigFish • Implement okToMove() in each okToMove(locn):boolean okToMove(locn):boolean subclass http://slideplayer.com/slide/7833453/ 71
Couplers All the smells in this group contribute to excessive coupling between classes or show what happens if coupling is replaced by excessive delegation. Misplaced responsibility Feature Envy Classes should know as little as possible Inappropriate Intimacy about each other ( Cohesion) Middle Man Message Chains T oo complex data access https://sourcemaking.com/refactoring/smells 72
Feature Envy It’s obvious that the method wants to beelsewhere, so we can simply use MOVE METHOD to give the method its dream home. Refactored Before We are reducing the coupling and enhancing the cohesion https://www.slideshare.net/annuvinayak/code-smells-and-its-type-with-example 73
Feature Envy • A method in one class uses primarily data and methods from another class to perform its work Indicates the method was incorrectly placed in the wrong class • Problems: • High class coupling • Difficult to change , understand, and reuse • Refactoring Solution: Extract Method & Method Movement • Move the method with feature envy to the class containing the most frequently used methods and data items 74
Feature Envy class OrderItemPanel { private: itemPanel _itemPanel; void updateItemPanel( ) { Item item = getItem(); int quant = getQuantity( ); if (item == null) _itemPanel.clear( ); else{ _itemPanel.setItem(item); _itemPanel.setInstock(quant); } } } http://slideplayer.com/slide/7833453/ slides 75 to 77 75
Feature Envy • Method updateItemPanel is defined in class OrderItemPanel, but the method interests are in class ItemPanel class OrderItemPanel { private: itemPanel _itemPanel; void updateItemPanel( ) { Item item = getItem(); int quant = getQuantity( ); if (item == null) _itemPanel.clear( ); else{ _itemPanel.setItem(item); _itemPanel.setInstock(quant); } } } 76
• Refactor ing solution: • Extract method doUpdate in class OrderItemPanel • Move method doUpdate to class ItemPanel class OrderItemPanel { private: itemPanel _itemPanel; void updateItemPanel( ) { Item item = getItem(); int quant = getQuantity( ); _itemPanel.doUpdate(item, quant); } } class ItemPanel { public: void doUpdate(Item item, int quantity){ if (item == null) clear( ); else{ setItem(item); setInstock(quantity); } } 77 }
Message chains a.getB().getC().getD().getTheNeededData() a.getTheNeededData() Law of Demeter : Each unit should only talk with friends https://www.slideshare.net/mariosangiorgio/clean-code-and-code-smells 78
Message chains To refactor a message chain, use Hide Delegate. Refactor: Hide delegate Message chains https://sourcemaking.com/refactoring/smells 79
Change preventers if you need to change something in one place in your code, you have to make many changes in other places too. Program development becomes much more complicated and expensive as a result. A class has to be changed in several parts Divergent change A single change requires changes in severall classes Shotgun surgery Parallel Inheritance Hierarchies https://sourcemaking.com/refactoring/smells 80
Shotgun surgery When changes are all over the place, they are hard to find and it’s easy to miss an important change 81
public class Account { private String type; private String accountNumber; private int amount; public Account(String type,String accountNumber, int amount) { this .amount=amount; this .type=type; this .accountNumber=accountNumber; } public void debit( int debit) throws Exception { if (amount <= 500) { throw new Exception("Mininum balance shuold be over 500"); } amount = amount-debit; System. out .println("Now amount is" + amount); } public void transfer(Account from,Account to, int cerditAmount) throws Exception { if (from.amount <= 500) { throw new Exception("Mininum balance shuold be over 500"); } to.amount = amount+cerditAmount; } } The problem occurs when we add another criterion in validation logic that is if account type is personal and balance is over 500 then we can perform above operations http://javaonfly.blogspot.ca/2016/09/code-smell-and-shotgun-surgery.html 82
public class AcountRefactored { private String type; private String accountNumber; private int amount; public AcountRefactored(String type,String accountNumber, int amount) { this .amount=amount; this .type=type; this .accountNumber=accountNumber; } private boolean isAccountUnderflow() { if (amount <= 500) { return true ; } return false ; } public void debit( int debit) throws Exception { if (isAccountUnderflow()) { throw new Exception("Mininum balance shuold be over 500"); } amount = amount-debit; System. out .println("Now amount is" + amount); } public void transfer(AcountRefactored from,AcountRefactored to, int cerditAmount) throws Exception { if (isAccountUnderflow()) { throw new Exception("Mininum balance shuold be over 500"); } to.amount = amount+cerditAmount; http://javaonfly.blogspot.ca/2016/09/code-smell-and-shotgun-surgery.html 83 }
Negative Impact of Bad Smells Bad Smells hinder code comprehensibility [Abbes et al.CSMR 2011] 84 https://www.slideshare.net/fabiopalomba/icse15-smell-inducingchange
Negative Impact of Bad Smells Bad Smells increase change- and fault-proneness [Khomh et al.EMSE 2012] 85 https://www.slideshare.net/fabiopalomba/icse15-smell-inducingchange
Negative Impact of Bad Smells Bad Smells increase maintenance costs [Banker et al.Communications of theACM] 86 https://www.slideshare.net/fabiopalomba/icse15-smell-inducingchange
Refactoring 87
Bad Code Smells: Indications • Frequent failures • Overly complex structure • Very large components • Excessive resource requirements • Deficient documentation • High personnel turnover • Different technologies in one system http://slideplayer.com/slide/7833453/ slides 88 to 90 88
Bad Code Smells: Solution • Steps: • Program Comprehension (Understanding) • Refactoring • A set of transformations that are guaranteed to preserve the behavior while they can remove bad code smells • Refining 89
Bad Code Smells: Refactoring http://slideplayer.com/slide/7833453/ 90
Linguistic Anti-Patterns (LA) 91
Linguistic Anti-Patterns (LA) • Represent recurring, poor naming and commenting choices • Negatively affects software: • Understandability • Maintainability • Quality Developers will Spend more time and effort when understanding these software artifacts Make wrong assumptions when they use them !! This is clearly reflected by the results of the activity you did last week 92
Results of Activity on LA - Correctness 93
Results of Activity (LA) - Effort (cognitive) 94
Results of Activity (LA) - Effort (time) 95
LA’s catalogue Method Attribute’s • Do more than they say • Name says more than the entity contains • A.1 - “Get” - more than an accessor • D.1 - Says one but contains many • A.2 - “Is” returns more than a Boolean • A.3 - “Set” method returns • Name says less than the entity contains • A.4 - Expecting but not getting a single • D.2 - Name suggests Boolean but type does instance not • Do less than they say • Name says the opposite of what the entity • B.1 - Not implemented condition contains • B.2 - Validation method does not • E.1 - Says many but contains one confirm • F.1 - Attribute name and type are opposite • B.3 - “Get” method does not return • F.2 - Attribute signature and comment are • B.4 - Not answered question opposite • B.5 - Transform method does not return • B.6 - Expecting but not getting a collection • Do the opposite of what they say • C.1 - Method name and return type are opposite • C.2 - Method signature and comment are opposite 96
Practice https://b.socrative.com/login/student/ Login to the room “SOEN6461LA” 97
A2. “Is” returns more than a Boolean 98
A2. “Is” returns more than a Boolean Rational : • Having an “is” method does not return a Boolean, but returns more information is counterintuitive. Consequences : • Such problems will be detected at compile time (or even by the IDE • Misleading naming can still cause misunderstanding from the maintainers’ side. • Refactoring : • By renaming the method to “ Validity” • By documenting that “The method check the source validity by considering correct time and the expired time.” 99
A3. “Set” method returns (do more ) Doesn't set anything!!! Creates a new objects and returns it 100
Recommend
More recommend
