CODEWITHSUNDEEP
oopsmalltalkpharo
Christophe De Troyer
Christophe De Troyer

Reputation: 2922

OO-design issue

I have a system where I have to model a household, which has a TVSubscription. This can either be a digital one, or an analog one.

A user logs into a SetTopBox. He can then rent Movies.

So the current scheme is the following:

//Existing instantiated variables in scope
aMovie
aUser
aSetTopBox
//End

--> execute this command: 
aUser rent: aVideo on: aSTB

Code:
User>>rent: aVideo on: aSTB
aVideo rentBy: self on: aSTB

Video>>rentBy: aUser on: aSTB
aUser rentActionMovie: self on: aSTB

User>> rentActionMovie: aMovie on: aSTB
aSTB rentActionMovie: aMovie by: self

STB>>rentActionMovie: aMovie by: aUser
(loggedInUser isNil)
    ifTrue: [ loggedInUser := aUser.
              --Do stuff to charge the movie]
    ifFalse: [ -- Show error that user is not logged in]

Technically speaking this is correct. But I have (Sorry for being anal) issues with this:

I have to pass aSTB 2 methods calls down to eventually use it. The double dispatch here is needed because I have Child and Adult and they can rent AdultMovie and ChildrensMovie. Therefore I use a double dispatch instead of typechecking (requirement). Therefore I thought of the following solution:

I can store currentlyLoggedIn on aSTB, and store a loggedInOn on aSTB. This however, makes the objects point to eachother.

My gut feeling tells me this is a bad smell. I'm not really sure how to fix it though.

Ideally I would like to do something like this:

aUser rent: aMovie.

Upvotes: 5

Views: 196

Answers (3)

Ben Coman
Ben Coman

Reputation: 111

Second alternative...

STB>>initialize
    aUser := UserNotLoggedIn new.

STB>>login
    aUser := self getUserFromAuthorisationCheck

STB>>rentMovie: aMovie by: aUser
    (aUser canRent: aMovie)
        ifTrue:  [ --Do stuff to charge the movie]

UserNotLoggedIn>>canRent: aMovie
    self displayErrorUserNotLoggedOn
    ^false

User>>canRent: aMovie
    aMovie ratingAge <= self ratingAge.

AdultMovie>>ratingAge
    ^18

Adult>>ratingAge
    ^18

ChildrensMovie>>ratingAge
    ^10

Child>>ratingAge
    ^10

User>>rent: aMovie
    aSetTopBox rentMovie: aMovie by: self

Upvotes: 2

Ben Coman
Ben Coman

Reputation: 111

I am no expert, but just an alternative off the top of my head...

STB>>initialize
    aUser := UserNotLoggedIn new.

STB>>rentMovie: aMovie by: aUser
    (aMovie okayFor: aUser)
        ifTrue:  [ --Do stuff to charge the movie]

AdultMovie>>okayFor: aUser
    ^aUser canRentAdultMovie

ChildrensMovie>>okayFor: aUser
    ^aUser canRentChildMovie

User>>canRentChildMovie
    ^true

User>>canRentAdultMovie
    self displayErrorCannotRentAdultMovie
    ^false

Adult>>canRentAdultMovie
    ^true

UserNotLoggedIn>>canRentChildMovie
    self displayErrorUserNotLoggedOn
    ^false

UserNotLoggedIn>>canRentAdultMovie
    self displayErrorUserNotLoggedOn
    ^false

Child "just the same as User"

User>rent: aMovie.
    aSetTopBox rentMovie: aMovie by: self.

aUser rent: aMovie.

Upvotes: 1

User
User

Reputation: 14873

Thought

You look so much at the relation between two objects, then it could be that you want to model the relation as an object, too.

An object is anything that is visible or tangible and is relatively stable in form.

Implementation Idea

You create a temporary object which only lives as long as the relation between the two objects. The object can function as a mediator or method object, if you want.

My feeling is that it might be a case of over-engineering and not understood right away by the someone who reads the code.

Upvotes: 1

Related Questions