Simple range class

Question

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.";
     }
}

Answer 1

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.

Answer 2

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.