Esta é uma pré-visualização de arquivo. Entre para ver o arquivo original
Bad Smells Baldoino Fonseca baldoino@ic.ufal.br Introduc7on • To learn common design problems so you can recognize them in your own code. • The most common design problems result from code that is: – Duplicated – Unclear – Complicated Duplicated Code You have the same code structure in more than one place Duplicated Code Similarity between two expressions in two methods of the same class CapitalStrategy +advisedLine(loan : Loan) : double +termLoan(loan : Loan) : double return loan.getUnusedPercentage()* loan.getCommitment()* dura7on(loan)* riskFactFor(loan); return loan.getCommitment()* dura7on(loan)* riskFactFor(loan); +dura7on(loan : Loan) : double +riskFactFor(loan : Loan) : double Similarity = 75% Duplicated Code Similarity between two expressions in subclasses CapitalStrategy +capital(loan : Loan) : double +dura7on(loan : Loan) : double +riskFactFor(loan : Loan) : double CapitalStrategyAdvisedLine +capital(loan : Loan) : double CapitalStrategyTermLoan +capital(loan : Loan) : double +dura7on(loan : Loan) : double return loan.getUnusedPercentage()* loan.getCommitment()* dura7on(loan)* riskFactFor(loan); return loan.getCommitment()* dura7on(loan)* riskFactFor(loan); Similarity = 75% Duplicated Code Similarity between codes in two unrelated classes Person name areaCode number getTelephoneNumber Company cnpj officeAreaCode officeNumber getTelephoneNumber Similarity = 66% Duplicated Code public class Loan { … public Loan(float no7onal, float outstanding, int ra7ng, Date expiry){ this.strategy = new TermROC(); this.no7onal = no7onal; this.outstanding = outstanding; this.ra7ng = ra7ng; this.expiry = expiry; } public Loan(float no7onal, float outstanding, int ra7ng, Date expiry, Date maturity){ this.strategy = new RevolvingTermROC(); this.no7onal = no7onal; this.outstanding = outstanding; this.ra7ng = ra7ng; this.expiry = expiry; this.maturity = maturity; } public Loan(CapitalStrategy strategy, float outstanding, int ra7ng, Date expiry, Date maturity){ this.strategy = strategy; this.no7onal = no7onal; this.outstanding = outstanding; this.ra7ng = ra7ng; this.expiry = expiry; this.maturity = maturity; } 80% 67% 67% Duplicated Code Similarity among the condi7onal logic applied throughout the system to deal with a null object MouseEventHandler +mouseDown(…) : boolean +mouseMove(…) : boolean MainMenuApplet +mouseDown(…) : boolean +mouseMove(…) : boolean Naviga7onApplet +mouseDown(…) : boolean +mouseMove(…) : boolean If (mouseEventHandler != null) return mouseEventHandler.mouseMove(…); return true; If (mouseEventHandler != null) return mouseEventHandler.mouseMove(…); return true; 100% Long Parameter List To pass in as parameters everything needed by a rou7ne Long Method The size of the parameter list analyze(start : String, end : String, startTime : int, endTime, amount, data: Type) : void 5 args Long Method When an object invokes a method, then passes the result as a parameter for a method Int basePrice = _quan7ty * _itemPrice; discountLevel = getDiscountLevel(); Double finalPrice = discountPrice ( basePrice, discountLevel); 1 result Long Method You are geEng several values from an object and passing these values as parameters in a method call Int low = range.getLow(); Int high = range.getHigh(); withinPlan = plan.withinRange(low, high); 2 values Long Method You have a group of parameters that naturally go together -‐ Data Clumps Customer amountInvoiceIn(start:Date, end:Date) amountReceivedIn(start:Date, end:Date) amountOverdueIn(start:Date, end:Date) 2 parameters 3 places Long Method A method is trying to do too much Long Method You have many temporary variables … Vegeta7on vegeta7on = new Vegeta7on( ); Slope slope = new Slope( ); Rain = new Rain( ); Soil = new Soil( ); Occupa7on occupa7on = new Occupa7on( ); Region region = new Region( ); … Analyze apply( ) : Strategy 6 temps Long Method The amount of switch statement for dispatching and handling request CatalogApp If ( ac7onName.equals(NEW_WORKSHOP){ //lots of code to create a new workshop } else if (ac7onName.equals(ALL_WORKSHOPS) { //lots of code to display informa7on about all workshops }… many more “else if” statements 2 ifs 2 requests Long Method The amount of switch statement to gather data from numerous classes with different interfaces Node StringBuffer results = new StringBuffer(); NodeIterator nodes = parser.elements(); while(nodes.hasMoreNodes()){ Node node = nodes.nextNode(); if(node instanceof StringNode){ …add contents to results }else if(node instanceof LinkTag){ …add contents to results }else if(node instanceof Tag){ …add contents to results }else if… } } return retults.toString(); LinkTag Tag StringNode … TextExtractor +extractText( ) : String 4 ifs 4 gather data Long Method The amount of versions of an algorithm and condiIonal logical to choose which version to use at runIme capital(){ if(expiry == null && maturity != null) return commitment*dura7on()*riskFactor(); if(expiry == null && maturity == null){ if(getUnusedPercentage() != 1.0) return commitment*getUnusedPercentage()* dura7on()*riskFactor(); else return (outstandingRiskAmount()*dura7on()*riskFactor() + (unusedRiskAmount()*dura7on()*unusedRiskFactor()); } return 0.0; } Loan +capital( ) : double 4 ifs 3 strategy Long Method You have a single bulky method that accumulates informaIon to a local variable String result = new String(); result += “<“ + tagName + “ ” + auributes + “>”; Iterator it = children.iterator(); while(it.hasNext()){ TagNode node = (TagNode)it.next(); result += node.toString(); } If(! tagValue.equals(“ ”)) result+= tagValue; result+= “<” + tagName + “>”; return result; TagNode toString( ) : String 9 lines Large Class A class is trying to do too much Large Class Fields and Methods Home dvd: DVD cd: CD watchMovie(…) : void tv: TV tuner: Tuner light: Light temperature: Temperature projector: Projector endMovie(…) : void listenToCd(…) : void endCd(…) : void listenToRadio(…) : void endRadio(…) : void … 7 lines 6 lines Large Class The condiIonal expressions that control an object’s state transiIons are complex. If ( state != REQUESTED && state != UNIX_REQUESTED) return; willBeHandledBy(admin); If ( state == REQUESTED) state = CLAIMED; else if (state == UNIX_REQUESTED) state = UNIX_CLAIMED; SystemPermission state: String REQUESTED : String claimedBy(…) : void CLAMED: String GRANTED: String DENIED: String UNIX_REQUESTED: String UNIX_CLAIMED: String grantedBy(…) : void deniedBy(…) : void 4 Verifica7ons Large Class Numerous methods on a class combine elements of an implicit language List nonWhiteProductsBelowNineDollars = productFinder.belowPriceAvoidingAColor(9.00f, Color.white); SystemPermission byColor(…) : List byPrice(…) : List bySize(…) : List belowPriceAvoidingAColor(…) : List byColorAndBelowPrice(…) : List byColorSizeAndBelowPrice(…) : List Client 6 lines Divergent Change On class is commonly changed in different ways for different reasons. Home watchMovie(…) : void endMovie(…) : void listenToCd(…) : void endCd(…) : void listenToRadio(…) : void endRadio(…) : void … onProjector(…) : void offProjector(…) : void 8 methods 8 concerns Shotgun Surgery When every 7me you make a kind of change, you have to make a lot of Liule changes to a lot of different classes Movie Cd DVD Radio Projector Media Feature Envy A method that seems more interested in a class other than the one it actually is in. Media descrip7on: String gender : String mediaDescrip7on(…) : String Contact email: String prefix: String getEmail(…) : String areacode: String number: String webpage: String getPrefix(…) : String getAreaCode(…) : String getNumber(…) : String getWebpage(…) : String return descrip7on + gender + getEmail() + getPrefix() + getAreaCode() + getNumber() + getWebpage(); 6 interests 2 auributes Primi7ve Obsession It occurs when the designer uses primi7ve data types to represent domain ideas Primi7ve Obsession You have an array in which certain elements mean different things String[] row = new String[3]; row[0] = “Liverpool”; row[1] = “15”; 2 different auributes Primi7ve Obsession The primiIve value controls logic in a class and the primiIve value isn’t type-‐safe SystemPermission state: String REQUESTED : String claimedBy(…) : void CLAMED: String GRANTED: String DENIED: String UNIX_REQUESTED: String UNIX_CLAIMED: String grantedBy(…) : void deniedBy(…) : void public final sta7c String REQUESTED = “REQUESTED”; If(state.equals(REQUESTED)) state = CLAIMED; 7 Primi7ve Types Primi7ve Obsession You have a data item that needs addiIonal data or behavior. Order customer : String Primi7ve Obsession You have an immutable type code that affects the behavior of a class. Order ENGINEER : int SALESMAN: int type: int Lazy Class A class that is not doing enough to pay for itself. Employee Salesman Only one subclass Specula7ve Generality I think we need the ability to this kind of thing someday. Specula7ve Generality You have abstract classes that aren’t doing much Employee Salesman Only one subclass Specula7ve Generality Methods with unused parameters Customer getContact(date:Date) 1 unused parameter Specula7ve Generality The name of a method does not reveal its purpose. Customer ge7nvcdtlmt Message Chains The client is coupled to the structure of the navigaIon. object.getE().getD().getC().getB().getA().getValue(); 6 chains Middle Man A class is doing too much simple delegaIon. Middle Man If only a few methods aren’t doing much int getRa7ng(){ return (moreThanFiveLateDeliveries()) ? 2 : 1; } Boolean moreThanFiveLateDeliveries(){ return _numberOfLateDeliveries > 5; } 1 line Indecent Exposure It occurs when methods or classes that ought not be visible to clients are publicity visible to them. Indecent Exposure Methods or classes that ought not to be visible to clients are publicly visible to them ARributeDescriptor +AuributeDescriptor(…) BooleanDescriptor +BooleanDescriptor (…) DefaultDescriptor +DefaultDescriptor (…) ReferenceDescriptor +ReferenceDescriptor (…) Client 3 subclass Data Class These are classes that have fields, ge~ng and se~ng method for fields, and nothing else. In early stages these classes are dumb data holders and are almost certainly being manipulated in far too much detail by other classes. Data Class Fields Home + dvd: DVD + cd: CD + tv: TV + tuner: Tuner + light: Light + temperature: Temperature + projector: Projector 7 fields Refused Bequest Subclasses get to inherit the methods and data of their parents. But what if they don’t want or need what they are given? They are given all these great gis and pick just a few to play with. Comments TIP: When you feel the need to write a comment, first try to refactor the code so that any comment becomes superfluous.