6
\$\begingroup\$

I have tried to achieve the following task using Factory Pattern with PHP version 7.0.

Define the classes and their methods (including parameters and return types) for a system that consist of a bookshelf, books, magazines, and notebooks. The bookshelf should allow store and retrieval of the items as well as reporting on the state of the bookshelf (how many items it has, how many more items it can hold) and initializing the capacity (in number of items it can hold in total). The other items should allow reading of a single page given the page number that returns the text of the page. A book has an accessible title and author. A magazine has an accessible name. A notebook has an accessible owner.

<?php
abstract class BookShelfFactory{

    /** Variables to hold BookShelf item and capacity */
    private static $bookShelfItems = array();
    private $bookShelfCapacity;

    /** Initialize the capacity of BookShelf */
    public function __construct(){
        $this->bookShelfCapacity = 3;
    }

    /**
     * Factory method that return abstract item.
     * This lets subclasses return any concrete product without breaking the
     * superclass' contract.
     */
    abstract public function getBookShelfItem(): BookShelfItem;

    /**
     * When the factory method is used inside the Creator's business logic, the
     * subclasses may alter the logic indirectly by returning different types of
     * the item from the factory method.
     */
    public function addItem(): int{
        $item = $this->getBookShelfItem();
        if (count(self::$bookShelfItems) < $this->bookShelfCapacity){
            array_push(self::$bookShelfItems, $item);
            end(self::$bookShelfItems);
            return key(self::$bookShelfItems);
        } else {
            echo "The Bookshelf reach it's maximum capacity.\n";
            return -1;
        }
    }

    public function retrieveItem($itemIndex): void{
        $instance = self::$bookShelfItems[$itemIndex];
        $instance->getProductInfo();
    }

    public function reportStatus(): void{
        $itemsInBookShelf = count(self::$bookShelfItems);
        $remainingCapacity = $this->bookShelfCapacity - $itemsInBookShelf;
        echo "Bookshelf contain {$itemsInBookShelf} items, ";
        echo "remaining capacity is {$remainingCapacity}\n";
    }

    public function getItemContent($itemIndex, $pageNumber): string{
        $instance = self::$bookShelfItems[$itemIndex];
        $content = $instance->getPageContent($pageNumber);
        return $content;
    }

}

class BookFactory extends BookShelfFactory{
    private $title, $author;
    private $content = array();

    public function __construct(string $title, string $author, array $content){
        parent::__construct();
        $this->title = $title;
        $this->author = $author;
        $this->content = $content;
    }

    public function getBookShelfItem(): BookShelfItem{
        return new BookProduct($this->title, $this->author, $this->content);
    }
}

class MagazineFactory extends BookShelfFactory{
    private $name;
    private $content = array();

    public function __construct(string $name, array $content){
        parent::__construct();
        $this->name = $name;
        $this->content = $content;
    }

    public function getBookShelfItem(): BookShelfItem{
        return new MagazineProduct($this->name, $this->content);
    }
}

class NotebookFactory extends BookShelfFactory{
    private $owner;
    private $content = array();

    public function __construct(string $owner, array $content){
        parent::__construct();
        $this->owner = $owner;
        $this->content = $content;
    }

    public function getBookShelfItem(): BookShelfItem{
        return new NotebookProduct($this->owner, $this->content);
    }
}

/**
 * The Product interface declares behaviors of various types of products.
 */
interface BookShelfItem{
    public function getProductInfo(): void;
    public function getPageContent(int $pageNumber): string;
}

/**
 * This concrete product implements the book
 */
class BookProduct implements BookShelfItem{
    private $title, $author;
    private $content = array();

    public function __construct(string $title, string $author, array $content){
        $this->title = $title;
        $this->author = $author;
        $this->content = $content;
    }

    public function getProductInfo(): void{
        echo "Title of Book is {$this->title} authored by {$this->author}\n";
    }

    public function getPageContent(int $pageNumber): string{
        // decrement 1 from page number as array is zero indexed
        if (key_exists($pageNumber - 1, $this->content)){
            return $this->content[$pageNumber - 1];
        } else {
            return "Given page doesn't exist.";
        }
    }
}

/**
 * This concrete product implements the magazine
 */
class MagazineProduct implements BookShelfItem{
    private $name;
    private $content = array();

    public function __construct(string $name, array $content){
        $this->name = $name;
        $this->content = $content;
    }

    public function getProductInfo(): void{
        echo "Name of Magazine is {$this->name}\n";
    }

    public function getPageContent(int $pageNumber): string{
        // decrement 1 from page number as array is zero indexed
        if (key_exists($pageNumber - 1, $this->content)){
            return $this->content[$pageNumber - 1];
        } else {
            return "Given page doesn't exist.";
        }
    }
}

/**
 * This concrete product implements the notebook
 */
class NotebookProduct implements BookShelfItem{
    private $owner;
    private $content = array();

    public function __construct(string $owner, array $content){
        $this->owner = $owner;
        $this->content = $content;
    }

    public function getProductInfo(): void{
        echo "Owner of Notebook is {$this->owner}\n";
    }

    public function getPageContent(int $pageNumber): string{
        // decrement 1 from page number as array is zero indexed
        if (key_exists($pageNumber - 1, $this->content)){
            return $this->content[$pageNumber - 1];
        } else {
            return "Given page doesn't exist.";
        }
    }
}


/**
 * The client code can work with any subclass of BookShelfFactory since it
 * doesn't depend on concrete classes.
 */
function clientCode(BookShelfFactory $creator){
    $index = $creator->addItem();
    if ($index >= 0){ // check if item is added
        $creator->retrieveItem($index);
        $pageNumber = 2;
        echo "Content of page {$pageNumber}: ".$creator->getItemContent($index,$pageNumber)."\n";
        $creator->reportStatus();
    }
}

/**
 * During the initialization phase, the app can decide which BookShelf item it
 * wants to work with, create an object of the proper subclass, and pass it to
 * the client code.
 */
echo "Testing BookFactory:\n";
clientCode(new BookFactory("Introduction to Algorithms", 
                            "Ronald Rivest", 
                            array("Book page1","Book page2")
                        )
        );
echo "\n\n";

echo "Testing MagazineFactory:\n";
clientCode(new MagazineFactory("DataQuest magazine",
                            array("Magazine page1","Magazine page2")
                        )
        );
echo "\n\n";

echo "Testing NotebookFactory:\n";
clientCode(new NotebookFactory("Whimpy Kid",
                            array("Notebook entry1","Notebook entry2")
                        )
        );
echo "\n\n";

echo "Testing when capacity reached:\n";
clientCode(new BookFactory("The C Programming Language", 
                            "keringhan & Ritchie", 
                            array("Book page1","Book page2")
                        )
        );
echo "\n\n";

The code is working fine as per best of my knowledge. I am looking for areas for improvement in the code following best practices.

\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

Is that really how a bookshelf work?

"Hello Magic Bookshelf, report status!"

"Bookshelf contain 0 items, remaining capacity is 3"

"Ok, spawn a new item!"

"0"

"Ok give me item 0!"

"Here it is, it is The C Programming Language by keringhan & Ritchie"

"Got it. Please spawn another item"

"1"

"Give it to me"

"Here it is, it is The C Programming Language by keringhan & Ritchie"

"What? Spawn another one and give it to me!"

"Here it is, it is The C Programming Language by keringhan & Ritchie"

"You must be kidding me!"

Bookshelf really isn't a "certain book clone" factory.

interface BookshelfItem
{
  public function countPages(): int;

  /** allow reading of a single page given the page number that returns the text of the page */
  public function getPageContent(int $page): string;
}

interface Bookshelf
{
  /** allow retrieval @return BookshelfItem[] */
  public function getItems(): array;
  public function getItem(int $position): BookshelfItem;
  /** how many items it has */
  public function countItems(): int;
  /** how many more items it can hold */
  public function countFreeSlots(): int;
  /** the capacity */
  public function getCapacity(): int;
  /** allow store */
  public function addItem(BookshelfItem $item): void;
}

interface BookshelfFactory
{
  /** initializing the capacity */
  public function createBookshelf(int $capacity): Bookshelf;
}

interface Book extends BookshelfItem
{
  public function getAuthor(): string;
  public function getTitle(): string;
}

interface Magazine extends BookshelfItem
{
  public function getName(): string;
}

interface Notebook extends BookshelfItem
{
  public function getOwner(): string;
}

What you can do now, is to add books to shelf and retrieve them later. But the bookshelf does not create its content on its own. That's up to someone else, bookshelf just holds whatever you put onto it.

$factory = new BookshelfFactoryImpl();
$shelf = $factory->createBookshelf(3);
$shelf->addItem(new BookImpl('The C Programming Language', 'keringhan & Ritchie', [/*...*/]));
$shelf->addItem(new NotebookImpl('Whimpy Kid', [/*...*/]));


echo "Bookshelf contain {$shelf->countItems()} items, remaining capacity is {$shelf->countFreeSlots()}"
$firstPageFirstItem = $shelf->getItem(0)->getPageContent(1);
echo "The first page of first item is {$firstPageFirstItem}";

Notice that I am only shoowing interfaces, the implementation is up to you. But here are a few remarks about your implementation.

Dont mix static properties with instance properties. Surely all bookshelfs dont share the array storage of items while each instance maintains its own capacity.

They asked in the specification that you provide a way to initialize the capacity - that means from outside. You implementation does initialize a capacity property, but since it is private it effectively behaves as a constant.

In modern PHP, you should prefer short array syntax [] over the oldschool array().

Dont be afraid to throw exceptions. For example I would design it in a way that trying to retrieve item 6 when capcaity is 4 would throw.

Dont put I/O (echo/printf) into your logic. That is something that belongs to the edge of application. For example if you report status by echoing it, I have no means in the code to write conditional statements like "if nothing in the shelf".

\$\endgroup\$
0
\$\begingroup\$

I agree with the advice presented by @slepic. Below are some other tips.

Use consistent spacing, Follow PHP standards for readability

There are recommended standards for PHP in the PHP Standards Recommendations - i.e. P.S.R.s. Between PSR-1 Basic Coding Standard and PSR-12: Extended Coding Style many common conventions for idiomatic PHP are outlined. The code adheres to many of the guidelines there except for a few - e.g.

4. Classes, Properties, and Methods

...

The opening brace for the class MUST go on its own line; the closing brace for the class MUST go on the next line after the body.

Opening braces MUST be on their own line and MUST NOT be preceded or followed by a blank line.

Closing braces MUST be on their own line and MUST NOT be preceded by a blank line.

So instead of

class NotebookProduct implements BookShelfItem{

Idiomatic PHP code is written as

class NotebookProduct implements BookShelfItem
{

Additionally,

4.4 Methods and Functions

...

Method and function names MUST NOT be declared with space after the method name. The opening brace MUST go on its own line, and the closing brace MUST go on the next line following the body. There MUST NOT be a space after the opening parenthesis, and there MUST NOT be a space before the closing parenthesis.

So instead of

public function __construct(string $owner, array $content){
    $this->owner = $owner;
    $this->content = $content;
}

public function getProductInfo(): void{
    echo "Owner of Notebook is {$this->owner}\n";
}

Idiomatic PHP methods are written as:

public function __construct(string $owner, array $content)
{
    $this->owner = $owner;
    $this->content = $content;
}

public function getProductInfo(): void
{
    echo "Owner of Notebook is {$this->owner}\n";
}

This allow anyone reading the code to spot the return type of the methods easily.

Accept capacity as constructor parameter or make it a constant

The capacity for the BookShelfFactory can be passed as a parameter when a bookshelf is created. The current code treats that capacity as a constant.

If you were to keep that capacity as a property that isn’t assigned via constructor method, that value could be set when the property is declared, eliminating the need for the constructor:

   private $bookShelfCapacity = 3;

And since it is treated like a constant, it could be declared as a constant:

const CAPACITY = 3;

and whenever it was referenced the class namespace would be part of it:

BookShelfFactory::CAPACITY; // 3

All implementations have same code for getPageContent()

In all three classes that implement the method getPageContent() is the same. The could be abstracted into a parent class of all three to avoid the redundancy.

Keyword else can be eliminated

In some places the else keyword is used when not necessary - e.g. following a conditional block with a return statement - for example the getPageContent of class BookProduct:

    if (key_exists($pageNumber - 1, $this->content)){
        return $this->content[$pageNumber - 1];
    } else {
        return "Given page doesn't exist.";
    }

This could be simplified to this:

    if (key_exists($pageNumber - 1, $this->content)){
        return $this->content[$pageNumber - 1];
    }
    return "Given page doesn't exist.";

Methods can be optimized and simplified

The method BookShelfFactory::addItem() appears to be a bit more complex than it needs to be:

public function addItem(): int{
    $item = $this->getBookShelfItem();
    if (count(self::$bookShelfItems) < $this->bookShelfCapacity){
        array_push(self::$bookShelfItems, $item);
        end(self::$bookShelfItems);
        return key(self::$bookShelfItems);
    } else {
        echo "The Bookshelf reach it's maximum capacity.\n";
        return -1;
    }
}
  1. If the shelf is full, there is no need to call the getBookShelfItem method and save the return value in a variable since that isn’t going to be used.
  2. When an item is added, square bracket syntax can be used instead of calling array_push().
  3. After adding an item to the array the functions to move the pointer on the array and return the key at that pointer seem unnecessary, since the pointer doesn’t seem to be used elsewhere, and the keys are numeric.

I haven’t tested this but would expect it could be simplified and optimized like this:

 public function addItem(): int{
    if (count(self::$bookShelfItems) > $this->bookShelfCapacity){
        echo "The Bookshelf reach it's maximum capacity.\n";
        return -1; 
    }
    self::$bookShelfItems[] = $this->getBookShelfItem();         
    return count(self::$bookShelfItems) - 1;
}

Also the method getItemContent() has a variable used only once: $content.

public function getItemContent($itemIndex, $pageNumber): string{
    $instance = self::$bookShelfItems[$itemIndex];
    $content = $instance->getPageContent($pageNumber);
    return $content;
}

Maybe that was used with debugging but it can be eliminated and the method can simply return $instance->getPageContent($pageNumber).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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