Skip to main content
added example of usage
Source Link
slepic
  • 5.7k
  • 2
  • 10
  • 27

And here is a simple example of usage:

// in DI container
$pageProvider = new DbPageProvider($pdo, 'users');
$usersPaginatorFactory = new PaginatorFactory($pageProvider);

// in lets say /users/list constroller we have the paginator factory injected
// $pageNumber is coming from request
$paginator = $this->usersPaginatorFactory->createPaginator($pageNumber, 20);

// maybe you wanna pass this to laravel view?
return view('users.list', ['paginator' => $paginator]);
// or maybe if you defined the getPaginatorViewData() function
return view('users.list', getPaginatorViewData($paginator));

And here is a simple example of usage:

// in DI container
$pageProvider = new DbPageProvider($pdo, 'users');
$usersPaginatorFactory = new PaginatorFactory($pageProvider);

// in lets say /users/list constroller we have the paginator factory injected
// $pageNumber is coming from request
$paginator = $this->usersPaginatorFactory->createPaginator($pageNumber, 20);

// maybe you wanna pass this to laravel view?
return view('users.list', ['paginator' => $paginator]);
// or maybe if you defined the getPaginatorViewData() function
return view('users.list', getPaginatorViewData($paginator));
added 4424 characters in body
Source Link
slepic
  • 5.7k
  • 2
  • 10
  • 27

oh and PS2: although this should have been a big warning on the beginning. Dont use eval(), avoid references, definitely dont return references, avoid instantiation of classes by string containing class name. The Pagination::factory is completly wrong. Immediately get rid of that file and never write anything like it again!!!

oh and PS2: although this should have been a big warning on the beginning. Dont use eval(), avoid references, definitely dont return references, avoid instantiation of classes by string containing class name. The Pagination::factory is completly wrong. Immediately get rid of that file and never write anything like it again!!!

added 4424 characters in body
Source Link
slepic
  • 5.7k
  • 2
  • 10
  • 27

EDIT: ok so i have added some more to point at the core of your problems:

The paginator doesnt really need the setters to be part of its interface.

Also see I use Iterator to represent the page items, instead of array. The array will only be used in specific implementation. You should avoid the array type in the abstraction.

interface PaginatorInterface
{
    public function getItems();

    public function getCurrentPageNumber(): int;

    public function getNumberOfPages(): int;

    public function getTotal(): int;

    public function getTotalOnCurrentPage(): int;

    public function getTotalPerPage(): int;
}

In fact, the class implementing that interface doesnt really need those setters too. It's actualy not even desired.

Also the paginator can do simple computations. You dont have to precompute and assign static values which actualy opens space for corrupted state.

class Paginator implements PaginatorInterface
{
    private $pageNumber;
    private $itemsPerPage;
    private $items;
    private $total;

    public function __construct(int $pageNumber, int $itemsPerPage, \Iterator $pageItems, int $total)
    {

        if ($pageNumber < 1) {
            throw new \InvalidArgumentException();
        }
        if ($itemsPerPage < 1) {
            throw new \InvalidArgumentException();
        }
        $this->pageNumber = $pageNumber;
        $this->itemsPerPage = $itemsPerPage;
        $this->items = $pageItems;
        $this->total = $total;
    }

    public function getItems(): \Iterator
    {
        return $this->items;
    }

    public function getCurrentPageNumber(): int
    {
        return $this->pageNumber;
    }

    public function getNumberOfPages(): int
    {
        return \ceil($this->getTotal() / $this->itemsPerPage);
    }

    public function getTotal(): int
    {
        return $this->total;
    }

    public function getTotalOnCurrentPage(): int
    {
        if ($this->items instanceof \Countable) {
            return \count($this->items);
        }
        return \iterator_count($this->items);
    }

    public function getTotalPerPage(): int
    {
        return $this->itemsPerPage;
    }
}

Here I define the page provider (extended by getTotalCount method since my edit):

interface PageProviderInterface
{
    public function getTotalCount(): int;
    public function getPage(int $offset, int $limit): \Iterator;
}

You then use the page provider within a paginator factory (that is your pagination interface but it actualy is paginator factory so i named it that way).

interface PaginatorFactoryInterface
{
    public function createPaginator(int $page, int $pageSize): PaginatorInterface;
}

class PaginatorFactory implements PaginatorFactoryInterface
{
    /** @var PageProviderInterface */
    private $pageProvider;

    public function __construct(PageProviderInterface $pageProvider)
    {
        $this->pageProvider = $pageProvider;
    }

    public function createPaginator(int $page, int $pageSize): PaginatorInterface
    {
        $total = $this->pageProvider->getTotalCount();
        $offset = ($page - 1) * $pageSize;
        if ($offset >= $total) {
            $items = new \EmptyIterator();
        } else {
            $items = $this->pageProvider->getPage($offset, $pageSize);
        }
        return new Paginator($page, $pageSize, $items, $total);
    }
}

And lastly some implementation of the page provider.

The array one which provider slices of an array that resides in memory:


class ArrayPageProvider implements PageProviderInterface
{
    private $items;

    public function __construct(array $items) {

        $this->items = $item;$items;
    }

    public function getTotalCount(): int
    {
        return \count($this->items);
    }

    public function getPage(int $offset, int $limit): \ArrayIterator\Iterator
    {
        return new \ArrayIterator(\array_slice($this->items, $offset, $limit));
  }
  
 }
}

And the silly PDO implementation, which would need some improvement to be actualy usable, but you should get the point:

class DbPageProvider implements PageProviderInterface
{
    private $pdo;
    private $table;

    public function __construct(PDO\PDO $pdo, string $table) {
        $this->pdo = $pdo;
        $this->table = $table;
    }

    public function getTotalCount(): int
    {
        $statement = $this->pdo->prepare('select count(*) from ' . $this->table);
        return $statement->fetchColumn();
    }

    public function getPage(int $offset, int $limit): \Generator\Iterator
    {
        $statement = $this->pdo->prepare('select * from ' . $this->table . ' limit ' . $offset . ',' . $limit);
        $result = $statement->execute();
        $i = 0;
        while ($row = $result->fetch()) {
            yield $i++ => $row;
        }
    }

}

And that should actualy be all you need. No need for some crazy collection outsourcing or too much of new new new in one method. Keep it simple and remember the naming is really important for whoever wants to understand the code.

Oh and one more, you hade a getViewData() method there, but that should be done by someone else:

function getPaginatorViewData(PaginatorInterface $paginator) {
  return [
}    'elements' => $paginator->getItems(),
```    // ...

  ];
}

This could be a global function, or it could be static method of its own class, or could even be elevated to an interface level. But actualy i dont think it belong to the lib, it may be part of some framework bundle, or something like that, but not the lib itself. The moment it becomes shapeless array, it becomes hard to work with...

PS: try to avoid constructor with $options array. It really is better to name all the parameters separately, even omit default value, let the user or a factory (if there is one) decide. Default values usualy just complicate things even if it seems the opposite at first glance...

interface PageProviderInterface
{
  public function getPage(int $offset, int $limit): \Iterator;
}

class ArrayPageProvider implements PageProviderInterface
{
  private $items;

  public function __construct(array $items) {

    $this->items = $item;
  }

  public function getPage(int $offset, int $limit): \ArrayIterator
  {
     return new \ArrayIterator(\array_slice($this->items, $offset, $limit));
  }
  
 }

class DbPageProvider implements PageProviderInterface
{
  private $pdo;
  private $table;

  public function __construct(PDO $pdo, string $table) {
    $this->pdo = $pdo;
    $this->table = $table;
  }

  public function getPage(int $offset, int $limit): \Generator
  {
     $statement = $this->pdo->prepare('select * from ' . $this->table);
     $result = $statement->execute();
     $i = 0;
     while ($row = $result->fetch()) {
       yield $i++ => $row;
     }
  }
  
}
```

EDIT: ok so i have added some more to point at the core of your problems:

The paginator doesnt really need the setters to be part of its interface.

Also see I use Iterator to represent the page items, instead of array. The array will only be used in specific implementation. You should avoid the array type in the abstraction.

interface PaginatorInterface
{
    public function getItems();

    public function getCurrentPageNumber(): int;

    public function getNumberOfPages(): int;

    public function getTotal(): int;

    public function getTotalOnCurrentPage(): int;

    public function getTotalPerPage(): int;
}

In fact, the class implementing that interface doesnt really need those setters too. It's actualy not even desired.

Also the paginator can do simple computations. You dont have to precompute and assign static values which actualy opens space for corrupted state.

class Paginator implements PaginatorInterface
{
    private $pageNumber;
    private $itemsPerPage;
    private $items;
    private $total;

    public function __construct(int $pageNumber, int $itemsPerPage, \Iterator $pageItems, int $total)
    {

        if ($pageNumber < 1) {
            throw new \InvalidArgumentException();
        }
        if ($itemsPerPage < 1) {
            throw new \InvalidArgumentException();
        }
        $this->pageNumber = $pageNumber;
        $this->itemsPerPage = $itemsPerPage;
        $this->items = $pageItems;
        $this->total = $total;
    }

    public function getItems(): \Iterator
    {
        return $this->items;
    }

    public function getCurrentPageNumber(): int
    {
        return $this->pageNumber;
    }

    public function getNumberOfPages(): int
    {
        return \ceil($this->getTotal() / $this->itemsPerPage);
    }

    public function getTotal(): int
    {
        return $this->total;
    }

    public function getTotalOnCurrentPage(): int
    {
        if ($this->items instanceof \Countable) {
            return \count($this->items);
        }
        return \iterator_count($this->items);
    }

    public function getTotalPerPage(): int
    {
        return $this->itemsPerPage;
    }
}

Here I define the page provider (extended by getTotalCount method since my edit):

interface PageProviderInterface
{
    public function getTotalCount(): int;
    public function getPage(int $offset, int $limit): \Iterator;
}

You then use the page provider within a paginator factory (that is your pagination interface but it actualy is paginator factory so i named it that way).

interface PaginatorFactoryInterface
{
    public function createPaginator(int $page, int $pageSize): PaginatorInterface;
}

class PaginatorFactory implements PaginatorFactoryInterface
{
    /** @var PageProviderInterface */
    private $pageProvider;

    public function __construct(PageProviderInterface $pageProvider)
    {
        $this->pageProvider = $pageProvider;
    }

    public function createPaginator(int $page, int $pageSize): PaginatorInterface
    {
        $total = $this->pageProvider->getTotalCount();
        $offset = ($page - 1) * $pageSize;
        if ($offset >= $total) {
            $items = new \EmptyIterator();
        } else {
            $items = $this->pageProvider->getPage($offset, $pageSize);
        }
        return new Paginator($page, $pageSize, $items, $total);
    }
}

And lastly some implementation of the page provider.

The array one which provider slices of an array that resides in memory:


class ArrayPageProvider implements PageProviderInterface
{
    private $items;

    public function __construct(array $items) {

        $this->items = $items;
    }

    public function getTotalCount(): int
    {
        return \count($this->items);
    }

    public function getPage(int $offset, int $limit): \Iterator
    {
        return new \ArrayIterator(\array_slice($this->items, $offset, $limit));
    }
}

And the silly PDO implementation, which would need some improvement to be actualy usable, but you should get the point:

class DbPageProvider implements PageProviderInterface
{
    private $pdo;
    private $table;

    public function __construct(\PDO $pdo, string $table) {
        $this->pdo = $pdo;
        $this->table = $table;
    }

    public function getTotalCount(): int
    {
        $statement = $this->pdo->prepare('select count(*) from ' . $this->table);
        return $statement->fetchColumn();
    }

    public function getPage(int $offset, int $limit): \Iterator
    {
        $statement = $this->pdo->prepare('select * from ' . $this->table . ' limit ' . $offset . ',' . $limit);
        $result = $statement->execute();
        $i = 0;
        while ($row = $result->fetch()) {
            yield $i++ => $row;
        }
    }

}

And that should actualy be all you need. No need for some crazy collection outsourcing or too much of new new new in one method. Keep it simple and remember the naming is really important for whoever wants to understand the code.

Oh and one more, you hade a getViewData() method there, but that should be done by someone else:

function getPaginatorViewData(PaginatorInterface $paginator) {
  return [
    'elements' => $paginator->getItems(),
    // ...

  ];
}

This could be a global function, or it could be static method of its own class, or could even be elevated to an interface level. But actualy i dont think it belong to the lib, it may be part of some framework bundle, or something like that, but not the lib itself. The moment it becomes shapeless array, it becomes hard to work with...

PS: try to avoid constructor with $options array. It really is better to name all the parameters separately, even omit default value, let the user or a factory (if there is one) decide. Default values usualy just complicate things even if it seems the opposite at first glance...

Source Link
slepic
  • 5.7k
  • 2
  • 10
  • 27
Loading