2
\$\begingroup\$

Although my code doesn't support a lot of data types (only one), it's enough to see the general design, which concerns me the most.

Range interface:

<?php

namespace Range;
require_once 'vendor/autoload.php';
interface Range {

    function __construct(string $range);

IntRange - implementation of range interface for integer

namespace Range;
require_once 'vendor\autoload.php';

class IntRange implements Range{
    private $bounds,
            $rangeType,
            $range;
    public function __construct(string $range) {
        $this->range = $range;
        $this->getType();
        $this->getBounds();
    }
    private function boundsToInt(){
        for($i = 0; $i < 2; $i++){
            $this->bounds[$i] = intval($this->bounds[$i]);
        }
    }
    private function getType(){
        $rangeTypeExtractor = new RangeTypeExtractor($this->range);
        $this->rangeType = $rangeTypeExtractor->rangeType();
    }
    private function getBounds(){
        $boundExtractor = new BoundExtractor($this->range, $this->rangeType);
        $this->bounds = $boundExtractor->extractBound();
    }
    private function getBoundForLooping() : array{
        if($this->rangeType == 'inclusive'){
            $start_bound = $this->bounds[0];
            $end_bound = $this->bounds[1] +1;
        }
        else if($this->rangeType == 'exclusive'){
            $start_bound = $this->bounds[0] + 1;
            $end_bound = $this->bounds[1];
        }
        return [$start_bound, $end_bound];
    }

    public function toArray() : array {
        $arr = array();
        $bounds = $this->getBoundForLooping();
        for($i = $bounds[0]; $i != $bounds[1]; $i++){
            array_push($arr, $i);
        }
        return $arr;
    }
    public function toIterator() {
        return new \ArrayIterator($this->toArray());
    }

}
    function toIterator();
    function toArray();

}

IBoundExtractor - extracts bounds from string

<?php

namespace Range;
require_once 'vendor/autoload.php';
interface IBoundExtractor {
    function extractBound();
}

BoundExtractor - generic class for extracting bounds, it returns the bound of string type, in form of array

<?php

namespace Range;
require_once 'vendor/autoload.php';
class BoundExtractor implements IBoundExtractor {
    private $rangeType;
    private $range;
    public function __construct(string $range, string $rangeType) {
        $this->rangeType = $rangeType;
        $this->isBoundValid($range);
        $this->range = $range;
    }
    public function extractBound() : array {
        switch ( $this->rangeType){
            case 'inclusive':
                return $this->getInclusiveRange();
            case 'exclusive':
                return $this->getExclusiveRange();
        }
    }
    private function isBoundValid(string $range){
        if( substr_count($range, ".") > 3 || substr_count($range, ".") < 2){
            throw new InvalidBoundException();
        }
    }
    private function getInclusiveRange() : array{
        return [
                substr($this->range, 0, strpos($this->range, "...")), // until ...
                substr($this->range, strpos($this->range, "...") + 3, strlen($this->range)) // after ...
                ];
    }
    private function getExclusiveRange() : array {
        return [
                substr($this->range, 0, strpos($this->range, "..")), // until ..
                substr($this->range, strpos($this->range, "..") + 2, strlen($this->range)) // after ..
                ];
    }

}

RangeTypeExtractor - it's extracting the range type, inclusive or exclusive, not to be confused with the data type (e.g. int, string, date). I don't like the name; it's misleading. If somebody can give recommendations for a better name I will be very grateful. On the other hand, the name isn't that bad. If I need a class to determine the date type of the range, I can call it DateTypeExtractor or something similar.

<?php
namespace Range;
require_once 'vendor/autoload.php';

class RangeTypeExtractor {
    private $range;
    public function __construct(string $range) {
        $this->range = $range;
    }

    public function rangeType() : string {
        if (strpos($this->range, "...") != FALSE) {
            return "inclusive";
        } else if (strpos($this->range, "..") != FALSE) {
            return "exclusive";
        } else {
            throw new InvalidRangeException();
        }
    }
}

InvalidBound and InvalidRange exceptions

<?php

namespace Range;
require_once 'vendor/autoload.php';
class InvalidBoundException extends \Exception{
     public function errorMessage() {
        return "Invalid Bound exception. Check if the ../... is correct";
     }
}

<?php

namespace Range;
require_once 'vendor/autoload.php';
class InvalidRangeException extends \Exception{
    public function errorMessage() {
        return "Invalid Range exception.";
     }
}
\$\endgroup\$
  • \$\begingroup\$ It seems that instead of RangeTypeExtractor, you pasted BoundExtractor twice, by mistake. \$\endgroup\$ – Stop ongoing harm to Monica Jul 9 '17 at 18:20
1
\$\begingroup\$

It seems a very odd choice to define Range as an interface instead of as a base class (perhaps abstract). Your interface strangely does not require any methods or constants outside of a constructor. This doesn't suggest to me a viable interface, as there are no actions or properties whatsoever inferred by being a class implementing Range (which coincidentally could be any concrete class).

Though I am a big believer in being biased towards composition over inheritance, I think you truly have an inheritance situation here with the way you have implemented this.


I am also struggling a bit to understand what you are gaining with all this code complexity. Why are you working with your data structure like a string? What value are you gaining in defining your own domain-specific language to handle what seems to be a task as simple as managing two pointers against an iterable data structure?


Why create your own exceptions for this small bit of code? PHP's built-in SPL exceptions should already have types that meet your needs in OutOfBounds, OutOfRange and similar exceptions.


Do you really need to define interfaces for specific-use classes?


I would like to also add to the "Avoid Undefined Behavior" thoughts from @janos♦ I think his example of "returning early" from within a method is a good demonstration trying to design away else conditions. Most of the time you can do this, unless both branches of code create specific, alternate side-effects. This helps minimize the number of code paths you have in your application, making is generally easier to read and easier to test/maintain.

\$\endgroup\$
2
\$\begingroup\$

API design

The Range interface is defined by a constructor that takes a string. What are the important key features of a range? I think they are:

  • start: the start index
  • limit: the end, exclusive index
  • step: optional, 1 by default

It doesn't matter that the range was originally written as a string, or in a text file, or in a song.

You could define a RangeParser whose important key feature will be a Range parse(string) method.

I guess the purpose of IBoundExtractor is what I mean by RangeParser. I have to guess, because this kind of interface declaration without any types doesn't describe itself very well:

interface IBoundExtractor {
    function extractBound();
}

I suggest:

interface RangeParser {
    function parse(string s): Range;
}

And the whole inclusive/exclusive concept just complicates things, because one can be converted into the other. I suggest to keep just one, exclusive, as I outlined at the top.

Iterator pattern

This implementation violates the iterator pattern:

public function toIterator() {
    return new \ArrayIterator($this->toArray());
}

The idea of an iterator is to encapsulate the state of an iteration. An iterator usually contains two things:

  • A reference to some underlying iterable
  • The current position

I would like to emphasize the reference there. It should not store a copy of the content it is iterating over. In theory, you should be able to have an iterator over an infinite data source, a copy of which would obviously not fit within memory.

The current implementation is not such a lightweight object, and therefore misleading.

Avoid undefined behavior

This method is deeply troubling:

private function getBoundForLooping() : array {
    if($this->rangeType == 'inclusive'){
        $start_bound = $this->bounds[0];
        $end_bound = $this->bounds[1] +1;
    }
    else if($this->rangeType == 'exclusive'){
        $start_bound = $this->bounds[0] + 1;
        $end_bound = $this->bounds[1];
    }
    return [$start_bound, $end_bound];
}

What if $this->rangeType is neither inclusive nor exclusive? Looking at other parts of the code, $this->rangeType is never validated. It should have been.

I suggest to avoid pointless local variables. The branches of the if statement could return values directly. And when we write that way, the problem of the undefined $this->rangeType becomes more obvious and harder to miss, for example:

private function getBoundForLooping() : array {
    if ($this->rangeType == 'inclusive') {
        return [$this->bounds[0], $this->bounds[1] + 1];
    }
    if ($this->rangeType == 'exclusive') {
        return [$this->bounds[0] + 1, $this->bounds[1]];
    }
    // ? what now?
}

Unused method

The private boundsToInt method is not used anywhere. As such it's dead code and should be deleted.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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