examples of good and bad oo designs - cs csu...

1

©Robert B. France 1

Examples of Good and Bad OO Designs

Robert B. France

Colorado State University

Fort Collins

Colorado


Section Outline

• Problem Description

• A novice design

• Improving the design

2


The Problem• [Variation of a program example used in the book

“Refactoring” by Martin Fowler]

• Program calculates and prints a statement of a customer’s charges at a video store. There are three types of movies: Regular, NewRelease, Children.

• Rates– Regular: 2.00 for 2 days; late 1.50/late day

– Children: 1.50 for 3 days; late 1.50/late day

– NewRelease: 3.00/day


Design 1: Class Diagram

Customer

Rental Movie

makes

refers_to

_rentals

_movie

1

*

*

1

name: String

addRental()getName()statement()

daysrented:int

getDaysRented()getMovie()

title:StringpriceCode:int

getPriceCode()setPriceCode()getTitle()

3


Design 1: Sequence Model (with activation boxes)


Design 1: Code for MoviePubl i c c l ass Movi e{

publ i c s t at i c f i nal i nt CHI LDREN = 2;

publ i c s t at i c f i nal i nt REGULAR = 0;

publ i c s t at i c f i nal i nt NEW = 1;

pr i vat e st r i ng _t i t l e;

pr i vat e i nt _pr i ceCode;

publ i c Movi e ( St r i ng t i t l e, i nt pr i ceCode) {

_t i t l e = t i t l e; _pr i ceCode=pr i ceCode; }

publ i c i nt get Pr i ceCode( ) {

r et ur n _pr i ceCode; }

publ i c i nt get Ti t l e( ) {

r et ur n _t i t l e; }

publ i c voi d set Pr i ceCode( i nt ar g) {

_pr i ceCode=ar g; }

}

4


Code for Rental

cl ass Rent al {

pr i vat e Movi e _movi e;

pr i vat e i nt _daysRent ed;

publ i c Rent al ( Movi e movi e, i nt daysRent ed) {

_movi e=movi e; _daysRent ed=daysRent ed; }

publ i c i nt get DaysRent ed( ) {

r et ur n _daysRent ed; }

publ i c Movi e get Movi e( ) {

r et ur n _movi e; }

}


Code for Customercl ass Cust omer {

pr i vat e st r i ng _name;pr i vat e vect or _r ent al s = new Vect or ( ) ;

publ i c Cust omer ( St r i ng name) {_name = name; }

publ i c voi d addRent al ( Rent al ar g) {_r ent al s. addEl ement ( ar g) ; }

publ i c St r i ng get Name( ) {r et ur n _name; }

publ i c St r i ng st at ement ( ) { …}

}

5


Code Skeleton for statement() Derived from Sequence Model

publ i c St r i ng St at ement ( ) {Enumer at i on r ent al s = _r ent al s. el ement s( ) ;\ …

whi l e( r ent al s. hasMor eEl ement s( ) ) {\ \ . . .

Rent al R =( Rent al ) r ent al s. next El ement ( ) ;\ \

Movi e M=aRent al . get Movi e( ) ;i nt PC=M. get Pr i ceCode( ) ;St r i ng T = M. get Ti t l e( ) ;i nt days=R. get DaysRent ed( ) ;

}\ \ …

}


Code for statement()publ i c St r i ng St at ement ( ) {

doubl e t ot al Amt = 0;Enumer at i on r ent al s = _r ent al s. el ement s( ) ;St r i ng r esul t = “ Recor d f or ” +get Name( ) +” \ n” ;whi l e( r ent al s. hasMor eEl ement s( ) ) {

Rent al aRent al =( Rent al ) r ent al s. next El ement ( ) ;t hi sAmt =get Amount ( aRent al ) ;St r i ng t =aRent al . get Movi e( ) . get Ti t l e( ) ;r esul t += ” \ t ” + t + St r i ng. val ueOf ( t hi sAmt ) +” \ n” ;t ot al Amt =t ot al Amt +t hi sAmt ;

}r esul t +=” Amount owed i s” +St r i ng. val ueOf ( t ot al Amt ) +” \ n” ;r et ur n r esul t ;

}

6


Code for getAmount()Pr i vat e doubl e get Amount ( Rent al aRent al ) {

Mov i e aMov i e=aRent al . get Mov i e( ) ;

i nt pc=aMovi e. get Pr i ceCode( ) ;i nt days=aRent al . get DaysRent ed( ) ;

i nt t hi sAmt = 0;swi t ch ( pc) {

case Movi e. REGULAR:t hi sAmt = 2;

i f ( days > 2)t hi sAmt += ( days- 2) * 1. 5;

br eak ;

case Movi e. NEW:t hi sAmt = days* 3;

br eak ;case Movi e. CHI LDREN:

t hi sAmt = 1. 5;i f ( days > 3)

t hi sAmt += ( days- 3) * 1. 5;br eak ;

}r et ur n t hi sAmt ;

}


Modified Sequence Model (with activation boxes)

7


Tidying Up: statement()publ i c St r i ng St at ement ( ) {

doubl e t ot al Amt = 0;

Enumer at i on r ent al s = _r ent al s. el ement s( ) ;

St r i ng r esul t = “ Recor d f or ” +get Name( ) +” \ n” ;

whi l e( r ent al s. hasMor eEl ement s( ) ) {

Rent al aRent al =( Rent al ) r ent al s. next El ement ( ) ;

t hi sAmt =get Amount ( aRent al ) ;

r esul t += ” \ t ” +aRental.getMovie().getTitle()+

St r i ng. val ueOf ( t hi sAmt ) +” \ n” ;

t ot al Amt =t ot al Amt +t hi sAmt ;

}

r esul t +=” Amount owed

i s” +St r i ng. val ueOf ( t ot al Amt ) +” \ n” ;

r et ur n r esul t ;

}


Tidying Up: getAmount()Pr i vat e doubl e get Amount ( Rent al aRent al ) {

Mov i e aMov i e=aRent al . get Mov i e( ) ;

i nt days=aRent al . get DaysRent ed( ) ;i nt t hi sAmt = 0;

swi t ch ( aMovie.getPriceCode()) {case Movi e. REGULAR:

t hi sAmt = 2;i f ( days > 2)

t hi sAmt += ( days- 2) * 1. 5;br eak ;

case Movi e. NEW:

t hi sAmt = days* 3;br eak ;

case Movi e. CHI LDREN:t hi sAmt = 1. 5;

i f ( days > 3)t hi sAmt += ( days- 3) * 1. 5;

br eak ;}

r et ur n t hi sAmt ;}

8


Why is Design 1 Bad?

• A large portion of the functionality is within the Customer class. Some of the functionality can be migrated to other classes that contain the relevant information.

• If a new type of movie is added to the system (e.g., FAVORITES) then the switch statement in the Customer class would have to be extended and recompiled. This is an undesirable coupling between the Customer and Movie class.


Design 2 Overview• Functionality distributed

– Rental is responsible for calculating its charges– Customer interface extended to include a

getTotalCharge() method that returns the sum of the charges for the customer.

– Solution requires 2 loops through a customer’s rentals; one to record the charges for each rental, the other to calculate total charge.

• Here is a compromise in which reuse is given greater weight than efficiency: by restricting getTotalCharge to calculating only the sum of charges it can be used by other functions that require just that and nothing more; but this means that two loops are needed when printing out statements. In design 3 I show the other approach in which efficiency outweighs reuse.

• Case statement problem not solved.

9


Design 2 Class Diagram

Customer

Rental Movie

makes

refers_to

_rentals

_movie

1

*

*

1

name: String

addRental()getName()statement()getTotalCharge()

daysRented:int

getDaysRented()getMovie()getCharge()

title:StringpriceCode:int

getPriceCode()setPriceCode()getTitle()


Design 2 Sequence Model

10


Design 2: Code for Rentalcl ass Rent al {

\ …

doubl e get Char ge( ) {

doubl e t hi sAmt = 0;

swi t ch ( _movi e. get Pr i ceCode( ) ) {

case Movi e. REGULAR:

t hi sAmt = 2;

i f ( days > 2)

t hi sAmt += ( days- 2) * 1. 5;

br eak;

case Movi e. NEW:

t hi sAmt = days * 3;

br eak;

case Movi e. CHI LDREN:

t hi sAmt = 1. 5;

i f ( days > 3)

t hi sAmt += ( days- 3) * 1. 5;

br eak;

}

r et ur n t hi sAmt ;

}


Design 2: statement()publ i c St r i ng St at ement ( ) {


St r i ng r esul t = “ Recor d f or ” +get Name( ) +” \ n” ;



doubl e t hi sAmt = aRent al . get Char ge( ) ;

r esul t += ” \ t ” +aRent al . get Movi e( ) . get Ti t l e( ) +


}

r esul t +=” Amount owed i s” +St r i ng. val ueOf ( get Tot al Char ge( ) ) +” \ n” ;


}

11


Design 2: getTotalCharge()Pr i vat e doubl e get Tot al Char ge ( ) {

doubl e r esul t = 0;



Rent al aRent al =

( Rent al ) r ent al s. next El ement ( ) ;

r esul t += each. get Char ge( ) ;

}


}


Design 3: Notes• One may be tempted to subclass the Movie class but note

that a Movie object can be a New Release, then a Regular movie over a period of time (thus the specialization is not static). Better to have the price determinant part of the Movie as a separate abstract class (Price) that is subclassed; a Movie object can then have a reference to the specific Price subclass object (New, Regular, Children).– This is an application of the Gang of 4 State pattern.

• In this design getTotalCharge calculates more than just the total charge, it also documents the charges for each rental in the String object result. – Good aspect: program accesses rentals in one loop, unlike design 2

in which rentals are traversed twice.– Bad aspect: If only a total is needed the program does unnecessary

work w.r.t. documenting individual rentals.

12


Design 3: Class Diagram

Rental Movie

makes

refers_to

_rentals

_movie

1

*

*

1

Customer

name: String

addRental()getName()statement()getTotalCharge()

daysRented:int

getDaysRented()getMovie()getCharge()

title:String

getCharge()getTitle()

Price

getCharge(days)

NewRelPrice

getCharge(days)

ChildrenPrice

getCharge(days)

RegularPrice

getCharge(days)

1

*

has_state


Design 3: Sequence Model

13


Design 3: Communication Model

aCustomer:Customer

aRental:Rental aMovie:Movie

statement()

1.1:* [for each rental]: ch:=getCharge()

1:getTotalCharge()

1.1.1: ch:=getCharge(days)

aPrice:Price

1.1.1.1: ch:=getCharge(days)

1.1.2:t:=getTitle(){ transient}


Price and Subclasses Codecl ass Pr i ce{

\ …

abst r act doubl e get Char ge( i nt day sRent ed) ;

}

c l ass Regul ar Pr i ce ext ends Pr i ce {

doubl e get Char ge( i nt day sRent ed) {

doubl e r esul t = 2;

i f ( daysr ent ed > 2)

r esul t += ( day sRent ed- 2) * 1. 5;


}

c l ass NewRel Pr i ce ext ends Pr i ce {


r et ur n daysRent ed* 3;

}

c l ass Chi l dr enPr i ce ext ends Pr i ce {


r esul t = 1. 5;

i f ( daysRent ed > 3)

r esul t += ( day sRent ed- 3) * 1. 5;


}

14


Movie and Rental Codecl ass Movi e{

\ …

doubl e get Char ge( i nt daysRent ed) {

r et ur n _pr i ce. get Char ge( daysRent ed) ;

}

c l ass Rent al {

\ …

doubl e get Char ge( ) {

r et ur n _movi e. get Char ge( _daysRent ed) ;

}


statement() Codepubl i c St r i ng St at ement ( ) {

St r i ng r esul t = “ Recor d

f or ” +get Name( ) +” \ n” ;

doubl e t ot al Amt = get Tot al Char ge( r esul t ) ;

r esul t +=” Amount owed i s”

+St r i ng. val ueOf ( t ot al Amt ) +” \ n” ;


}

15


getTotalCharge() Codepubl i c doubl e get Tot al Char ge ( St r i ng r esul t ) {


doubl e t ot al Amt = 0;



doubl e t hi sAmt = aRent al . get Char ge( ) ;

r esul t += ” \ t ” +aRent al . get Movi e( ) . get Ti t l e( ) +


t ot al Amt += t hi sAmt

}

r et ur n t ot al Amt ;

}

examples of good and bad oo designs - cs csu...

Documents