Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a base class with 3 sub ones: Item <-- Book, MusicCD, Software

They have some common and different properties but in the view I want to get a mixed list of them and show in one table:

Title Price Volume Author Artist Edition Version

So I have done this:

Classes:

abstract class Item {
    protected $id;
    protected $title;
    protected $price;
    function getTitle() { return $this->title; }
    function getPrice() { return $this->price; }
    function getAuthor() { return ''; }
    function getEdition() { return ''; }
    function getVolume() { return ''; }
    function getArtist() { return ''; }
    function getVersion() { return ''; }
}
class Book extends Item {
    private $author;
    private $edition;
    private $volume;
    function getAuthor() { return $this->author; }
    function getEdition() { return $this->edition; }
    function getVolume() { return $this->volume; }
}
class MusicCd extends Item {
    private $artist;
    private $volume;
    function getArtist() { return $this->artist; }
    function getVolume() { return $this->volume; }
}
class Software extends Item {
    private $version;
    function getVersion() { return $this->version; }
}

View:

<?php
// The controller pass items to view
$items = $this->items;
?>
<table>
<tr>
<th>Title</th><th>Price</th><th>Volume</th><th>Author</th><th>Artist</th><th>Edition</th><th>Version</th>
</tr>
<?php
foreach ($items as $i) :
?>
<tr>
<td><?=$i->getTitle()?></td>
<td><?=$i->getPrice()?></td>
<td><?=$i->getVolume()?></td>
<td><?=$i->getAuthor()?></td>
<td><?=$i->getArtist()?></td>
<td><?=$i->getEdition()?></td>
<td><?=$i->getVersion()?></td>
</tr>
<?php
endforeach;
?>
</table>

Here the question: I feel that the classes seem not good enough because if there are more subtypes appears, chance that I have to modify the Item class. Please help me improve that.

share|improve this question
    
Hi. Welcome to Code Review! You seem to be asking about code not yet written, which is off-topic for Code Review. You could write it both ways and ask for a comparative-review. Alternately, you could try changing the question to fit Programmers.SE or Stack Overflow. P.S. Rather than concatenating the results of methods that may or may not exist, I would give Item a __toString method and just write echo $i . "\n"; or echo "$i\n";. –  Brythan Dec 17 '14 at 10:40
    
@Brythan Thank you. I've read the Help and still haven't got the on/off-topic much. I will try more. In the meantime I will edit the question to be more complete with the code. –  greatmorro Dec 17 '14 at 11:16

1 Answer 1

Since you don't want to manipulate the objects in the $items list, but simply want to output them to the screen, use the __toString() method. For Book this would be:

public function __toString()
{
    $result = $this->getTitle() . '  ' .
              $this->getPrice() . '  ' .
              $this->getVolume() . '  ' .
              $this->getAuthor() . '  ' .
              . '    ' .  //Here comes 'artist'
              $this->getEdition() . '  '
    return $result;
}

And then do this for the other class as well. This is not the most ideal situation because when you want to change the order of the properties for the output, you'll have to change the method in all the derived classes. There's not magic automated way to do this I believe.

share|improve this answer
    
Thank you. The change in the view is exactly what I worry about. Anyway, I will edit the question so that it is more clear. –  greatmorro Dec 17 '14 at 11:17
    
I hope you'll find a fix for this, like I said.. I don't think there's aan easy automated way to do this. –  Abbas Dec 18 '14 at 0:56

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.