'Representing' roles (noun classnames) a sign of bad design?

[Afro Boy]

Yet again another fantastic thread. A great deal to think about here.

A simple example you might be able to help me with ... In a common web application, the code will generally be focused on reading data from a store and presenting it in the browser. eg ...

// Example 1
$eb =& new EventBroker();
$event =& $eb->findEventByName( 'myevent' );  // $event is my concern!
$template->setAttribute( 'venue', $event->getVenue() );
$template->setAttribute( 'date', $event->getDate() );

// Example 2
$event =& new Event();  // this also concerns me
$event->setName( 'phpgig' );
$event->setDate( '20050215' );
$eb =& new EventBroker();
$eb->save( $event ); 

This is working well for me as most of my classes actually do things.

I have an uncomfortable feeling about the numerous noun and simple storage classes I have (Event, Article, NewsItem). They do nothing much apart from getting\setting attributes. Is the logical end to convert these back to simple data arrays? Is this use of a noun acceptable or is there a more appropriate name or direction I can be coming from?

Cheers,
Af

[Brenden Vickery]

I'm sorry to bump this thread up, but I tried the following:
I wanted to see how a very simple application (part of) would turn out using noun-classes with representative roles, and one using doers with actual doer roles. Well, these designs are how I would do it, so there's probably something terrible wrong in the following examples, but ok

The idea was to create a simple betting mechanism (let users bet on a particular match), with the following purposes/requirements:
- Beeing able to bet on a match
- Check for winners when a match is played
- Update finance if a bet is a winner

So I started coding away with the noun approuch, leading to this:

Following Link For entire quote: http://www.sitepoint.com/forums/show...8&postcount=26

Did you notice how your class names followed how you described the requirements? After laying out your requirements you made a Bet, Match and Finance class. See how your Bet class is responsible for checking if itself is a winner and updating its finance? In normal conversation a Bet does not check if it won. See my point?

To carry on with the same theme, you've added Bets to Matches which doesn't happen in real life, and you can get an owner from Finance. Your code should make sense from a model of the real life domain.

Next you've tried to do things from a "Doers" perspective creating classes called BetPlacer, WinChecker and FinanceHolder. I think this worked out much better because it fit closer to the domain. There are still some problems like, why does a FinanceHolder have an Owner?

Bringing this to the real world domain of placing bets lets use this site:http://www.ildado.com/horse_racing_rules.html as an example of our "domain expert". Ive pulled a few quotes that show how the expert discusses aspects of placing bets etc.

# Bettor (US) - Someone who places or has a bet. A 'Punter' in the UK.
# Bet - A transaction in which monies are deposited or guaranteed.
# Bookie - (U.K.) Short for bookmaker. The person or shop who accepts bets.
# Bookmaker - Person who is licensed to accept bets on the result of an event based on their provision of odds to the customer. (Sportsbook US).

Using these definitions and our new knowledge of the domain, we can re-word our requirements to this:
- A Bettor places Bets.
- A Bookmaker accepts Bets.
- A Bet is a money transaction.
- A Bookmaker pays winning Bettors
- A Bookmaker make Events avaiable to Bet on.

This might yield something like this:

$bettor = new Bettor();
$bettor->money = 100;

$bookmaker = new BookMaker();
$bookmaker->makeEventAvailable(new Event('Hockey'));
$bookmaker->acceptBet($bettor->placeBet(40, 'Hockey', 5));
$bookmaker->acceptBet($bettor->placeBet(20, 'Hockey', 7));
$bookmaker->updateScore('Hockey', 5);
$bookmaker->payWinners();

class Bettor {
    var $money;
    function placeBet($amount, $event, $score) {
        $this->money -= $amount;
        return new Bet($this, $amount, $event, $score);
    }
}

class Bet {
    var $money;
    var $eventName;
    var $bettor;
    var $score;
    function Bet(&$bettor, $money, $eventName, $score) {
        $this->bettor =& $bettor;
        $this->money = $money;
        $this->eventName = $eventName;
        $this->score = $score;
    }
}

class Event {
    var $name;
    var $score;
    function Event($name) {
        $this->name = $name;
    }
}

class BookMaker {
    var $bets = array();
    var $events = array();
    function acceptBet($bet) {
        $this->bets[] = $bet;
    }
    function makeEventAvailable($event) {
        $this->events[$event->name] = $event;
    }
    function updateScore($eventName, $score) {
        $this->events[$eventName]->score = $score;
    }
    function payWinners() {
        foreach($this->bets as $bet) {
            if ($this->events[$bet->eventName]->score == $bet->score) {
                $bet->bettor->money += $bet->money * 2;
            }
        }
    }
} 

I made a few assumptions here like in these requirements:
- A Bookmaker pays winning Bettors
- A Bookmaker make Events available to Bet on.

I would expect as we learn more about the domain we may find things that change the wording of these requirements. For instance does the bookmaker need to keep track of the amount of money it has? Does the Bookmaker pay winning bettors and if so where is the "domain experts" use of those terms? Does the Bookmaker make Events available? ...

Choosing the name of your classes is very important like marcus says. The language that those classes speak through their methods and dependencies is just as important and should be chosen based on how you speak about them.

Hope this makes sense.

EDIT: Fixed 100 spelling/grammar mistakes.. I rely on spell check as you can tell.

[Serenarules]

Afro, in your situation I would ask the following:

Does the event, article or newsitem class expose any methods at all, other than get/set routines? Second, is there some reason you can't skip the classes and use arrays to start with. If your packaging does not add anything more than what a stock datatype can offer, there's no reason to use it. There's one minor caveat...argument type hinting.

function (Event $item);

[Afro Boy]

Good point.

I was thinking of going back to using arrays to store the data. I think it was a case of 'overdesign' that created those classes anyway. Any drawbacks that people can see to using an array to store that data?

Brenden, that post made heaps of sense. Thanks for ellaborating so much on that idea. Looking forward to your blog article series too.

Thanks again,
Af.

[Serenarules]

I would probably re-evaluate why you made it a class to begin with. Possibly there was an underlying factor. Maybe you actually intended to add reponsibilities to it at some point, or maybe those that are currently part of of another object would be better off within Event? If so, using the class is ok, though I'd rather see you write an abstract IEvent, base CEvent, and then custom event types for each type of event, CCustomEvent, COtherEvent...this way you can write functions like the one below and pass in any class based on IEvent. This may also eliminate the need for a 'type' property within 'Event'.

function ( IEvent $item );

[lastcraft]

Hi...

They do nothing much apart from getting\setting attributes. Is the logical end to convert these back to simple data arrays? Is this use of a noun acceptable or is there a more appropriate name or direction I can be coming from?

I think this is one where the object view of the world comes into contact with something else (here persistent data). If you use arraays and you want to add additional data (such as one-to-many relationships) you would then have to change them all back to objects again. I would stick with objects. they are more self documenting anyway.

That said there will be a lot of duplication of pretty boring code. You could definitely code generate the classes, or perhaps use some generic class and reflection (with __call() perhaps). Do you need named accessors? Perhaps get($key) and set($key, $value) would result in a simpler one off class. Called Row perhaps?

My favourite is code generation becuase you could generate the DB schema at the same time. That way you wouldn't have to edit both every time you change the schema. I think there are lot's of choices though.

yours, Marcus

[Serenarules]

My thoughts exactly Marcus. By using __set and __get you can limit which properties are writable by a consumer.

public function __construct($namedValueArray) {
 
$this->_accepted = array('id', 'name');
$this->_readonly = array('id');
 
// may want to unset any entries that aren't acceptable here...
 
$this->_properties = $namedValueArray;
 
}
 
public function __get($name) {
 
if (in_array($name, $this->_accepted) && array_key_exists($name, $this->_properties)) {
return $this->_properties[$name];
} else {
// exception or other handling...
}
 
}
 
public function __set($name, $value) {
 
if (in_array($name, $this->_accepted) && array_key_exists($name, $this->_properties) && !in_array($name, $this->_readonly)) {
$this->_properties[$name] = $value;
} else {
// exception or other handling...
}
 
}

This control over handling of values can be a valid reason to use a class here. What I do in this instance is add a switch to the __set handlers once it is determined you can assign the value, and based on the property, further validate it's content. Just remember to set up the initial values for $_accepted, $_readonly, and $_properties in the constructor.

Excuse me for going off on a tangent by posting this code. Back to the discussion...

[Brenden Vickery]

I have an uncomfortable feeling about the numerous noun and simple storage classes I have (Event, Article, NewsItem). They do nothing much apart from getting\setting attributes. Is the logical end to convert these back to simple data arrays? Is this use of a noun acceptable or is there a more appropriate name or direction I can be coming from?

Just want to point out a couple of Refactorings:
Replace Array with Object : http://www.refactoring.com/catalog/r...ithObject.html
Replace Record with Data Class :http://www.refactoring.com/catalog/r...DataClass.html

Fowler on the mechanics and motivations behind this refactoring:

"You now have a dumb data object. It has no behavior yet further refactoring will explore that issue." - pg. 217
"You can also encapsulate the information and use Move Method(142) to add behavior to it." - pg. 186

EDIT: thought an example would be good:

$event = array();
$event['name'] = 'phpgig';
$event['date'] = '20050215';

// is refactored to:

$event = new Event();
$event->setName( 'phpgig' );
$event->setDate( '20050215' ); 

[Manor]

To carry on with the same theme, you've added Bets to Matches which doesn't happen in real life, and you can get an owner from Finance. Your code should make sense from a model of the real life domain.

It seems logical to me that you would be able to place bets on matches. But thats probably just lack of knowledge on my side.

Next you've tried to do things from a "Doers" perspective creating classes called BetPlacer, WinChecker and FinanceHolder. I think this worked out much better because it fit closer to the domain. There are still some problems like, why does a FinanceHolder have an Owner?

Why not? Do you mean it would be better to say that the FinanceHolder holds a specific financeId? If so, yes I would agree with that. If not, could you tell me why you think it's bad?

Using these definitions and our new knowledge of the domain, we can re-word our requirements to this:
- A Bettor places Bets.
- A Bookmaker accepts Bets.
- A Bet is a money transaction.
- A Bookmaker pays winning Bettors
- A Bookmaker make Events avaiable to Bet on.

Nice idea to look the terms up

$bookmaker->acceptBet($bettor->placeBet(40, 'Hockey', 5)); 

This seems a little strange to me. You ask the bettor to *place* a bet, and you need to call the bookmaker again to accept it? $bettor->createBet() would be better then, not? I think that more clearly indicates what the method is doing. (I know Im paying too much attention to the small things, but ow well )

I like the rest of your code, but Im not a fan of using an in-between class (bet) to gain access to the bettor. But thats just my personal taste, and probably this is the best way. Nice job

[DougBTX]

EDIT: thought an example would be good:

$event = array();
  $event['name'] = 'phpgig';
  $event['date'] = '20050215';
  
  // is refactored to:
  
  $event = new Event();
  $event->setName( 'phpgig' );
  $event->setDate( '20050215' ); 

You could do this too:

$event = new Event();
 $event['name'] = 'phpgig';
 $event['date'] = '20050215'; 

Using ArrayAccess SPL interface.

Douglas

[Manor]

After I had posted a comment in the "First steps in OOP" thread some time ago, I was hoping to get some comments on the way I tackled the problem (http://www.sitepoint.com/forums/show...6&postcount=17) because it actually just does exactly the same as I've been trying to evaluate and discuss in here, but now on an actual problem. And despite the good critics and discussion in here, critics and comments stayed away on that approach, whereas I was hoping to see some. Does this mean that the approach did well on that subject, or did many of you just fell asleep after my first line, and just didn't care to comment ?

[Galo]

Hi Manor,
You will always need containers though. for example, I'm working in some image processing software and the business rules require that while the display of a collection of images is in one area of the view, it must be seperated into sides within a page, and pages within a group, and groups within the collection. And its requirements include moving a page from one place to another (possibly from a place in one group to a place in another group). The logical way is of course to have an ImageGroupCollection containing ImagePage's, and a ImagePageCollection containing ImageSide's and an ImageCollection to manage the control of it. Without these 'noun' classes to break the problem down into its logical domain types the code would be far more difficult and unnatural to read.

I have read that a class without verbs is just an empty class, but i've found that sometimes you just need containers.

Isn't this what the model is supposed to do in an MVC combination, i mean if you apply the observer pattern to the Model and store all your collections there you're done no, just apply an update to the root element like
foreach($this->observers as $observer){
$observer->update($this)
}

this goes for those who would implement the iterator on to their collection class as well....

Cheers,
-Galo

[Galo]

or:

 $event = new EventCollection(); //Creates a new Collection storage
 $event->create('phpgig'); //Creates a new event object
 $event->setDate('20060215');
 $event->add(); //Stores date automaticly, 0 if date = set, add's new event object
 $event->invoke(); //triggers it 

- Galo

If you are going to decouple things anyway try to make things as dynamic as posible, here i automated the date storage, you can provide one but you dont have to, if none is provided and add is called the date get's automaticly created.

invoke is a method that is provided by the SPL through the reflection package.

-Galo