How can I change my code to display information about failure of creation of a value object, when I want to process an array of such objects?

softwareengineering.stackexchange https://softwareengineering.stackexchange.com/questions/395408

Question

Consider class Crate that is a value object. Crate represents a valid 3-dimensional box.

In constructor I validate the given parameters, and I throw an exception, if supplied dimension parameters are invalid:

class Crate
{
    protected $length, $width, $height;

    function __construct(float $length, float $width, float $height)
    {
        if ($length <= 0 || $width <=0 || $height <=0)
            throw new RuntimeException("Invalid Dimension");

        $this->length = $length;
        $this->width = $width;
        $this->height = $height;
    }

    function getDimensions()
    {
        echo $this->length . 'x' . $this->width . 'x' . $this->height;
    }
}

In my project I have a need to display dimensions of several crate box configurations. Namely, there is a showCrates method that accepts an array of Crate and then uses that array to display each Crate's dimensions, and other info.

function showCrates(array $crates)
{
    foreach($crates as $key => $crate)
        echo 'Crate #' . $key . ':' . $crate->getDimensions() . PHP_EOL;
}

showCrates works great when all parameters given to all Crate objects are valid. However, when a Crate object throws an Exception on an invalid parameter, code execution stops.

I want to be able to keep the execution going and still show the valid crates, but for invalid crates to have a message saying "Crate at index i was invalid".

Sample output that I expect is:

 Crate #1: 2x5x9
 Crate #2: 1x3x4
 Crate #3 is invalid, because supplied dimensions were: 0x0x0
 Crate #4: 5x6x3

Question

I am looking for a way that will let me display the above output without modifying Crate object itself.

Potential Solution that I am rejecting:

One way to solve my question is to allow invalid Crate objects and have an error boolean flag inside them stating whether a Crate is valid or not. Then I can keep showCrates method signature - to accept an array of Crate object - but modify it to check first, whether a Crate is valid, and to display desired output accordingly.

However, ideally I would like to NOT modify my Crate object unless there is a very strong argument to do so. I have constructed Crate object to be an immutable value object that can only exist if parameters supplied to it are valid, and to otherwise throws an Exception. As to why, I believe that this way the Crate is more easily testable and there are no extra checks to see whether or not Crate is valid.

Sample Calling Code

class CrateRequestHandler
{
    function handle(ServerRequestInterface $request)
    {
        // mocked up data from Request
        // this is sample data for showcase purposes only
        // it usually supplied by user or from database
        // and it is impossible to tell ahead of time 
        // if it will be correct for Crate purposes
        $crates = array();
        $crates[0] = new Crate(2, 5, 9);
        $crates[1] = new Crate(1, 3, 4);
        $crates[2] = new Crate(0, 0, 0);
        $crates[3] = new Crate(5, 6, 3);

        // send to View
        $this->showCrates($crates);
    }

    function showCrates(array $crates)
    {
        foreach($crates as $key => $crate)
            echo 'Crate #' . $key . ':' . $crate->getDimensions() . PHP_EOL;
    }
}
Was it helpful?

Solution

To me, I'll create another value object called CrateCreation to model the creation of your crates and let showCrates accept the list of CrateCreation objects instead of the list of the actual Crate objects. The CrateCreation object holds these pieces of information:

  • The original input values.
  • The actual Crate object if the creation is success.

Sample class:

<?php
class CrateCreation
{
    private $length, $width, $height;
    private $crate;

    function __construct(float $length, float $width, float $height)
    {
        $this->length = $length;
        $this->width = $width;
        $this->height = $height;

        try {
            $this->crate = new Crate($length, $width, $height);
        }
        catch(RuntimeException $e)
        {
            // do some loging here
        }
    }

    // getters here

    // the same `getDimensions` method here
}

Then the showCrates function should be modified to:

<?php
function showCrates(array $crateCreations)
{
    foreach($crateCreations as $key => $crateCreation)
    {
        $crate = $crateCreation->getCrate();
        if (!$crate)
        {
            echo 'Crate #' . $key . ' is invalid, because supplied dimensions were: ' . $crateCreation->getDimensions() . PHP_EOL;
            continue;
        }

        echo 'Crate #' . $key . ':' . $crate->getDimensions() . PHP_EOL;
    }
}

OTHER TIPS

You are correct in asserting the Crate object should always be valid. This is a business class that should be enforcing its invariants — which it currently does. The problem lies with taking user input. You must allow user input to violate business rules and constraints. You will need at least 2 other classes:

  1. A "view model" or "parameter" object that allows you to reconstruct the Crate objects without enforcing business rules.

  2. A "validator" object that inspects the request data and collects error messages for each object in the collection, for each property being validated.

You must run these data validations against this collection of objects that might be violating business rules. When validations fail, return a collection of error messages back to the client.

Only upon successful validation should new Crate objects be initialized.

This just boils down to basic user input validation.

I don't often add a second answer, but I have another approach that might make more sense and require less code, and I still think my other answer has value as well.

In PHP you can return an array of values and split them out into multiple variables in the caller.

You could add a static method to the Crate class that accepts the arguments to the Crate constructor, and returns an array with a Boolean, a Crate object and an Exception object thrown during initialization.

class Crate
{
    public static function create(float $length, float $width, float $height) {
        $isValid = false;
        $crate = null;
        $error = null;

        try {
            $crate = new Crate($length, $width, $height);
            $isValid = true;
        } catch (Exception $err) {
             $error = $err;
             $crate = null;
        }

        return array($isValid, $crate, $error);
    }
}

And an example of using it:

list($isValid, $crate, $error) = Crate::create(10, 4, 8);

if ($isValid) {
    // Do something with $crate
}
else {
    // Log the $error
}

The advantage here is this creation logic is kept in the class that specializes in Crates: the Crate class, and doesn't bloat your code base with more classes.

So what you are asking is for it to be impossible to compile code which produces an invalid crate at run time.

This is not as impossible as it sounds, but you are going to have to use a strongly typed language. I'm not sure you can do it in PHP

for example, lets say for a second we allow a 0 size crate. you could change the construction parameters to unsigned integer. Now you can't pass in negative values.

You can imagine further than you can create more types that express your requirements and then use them rather than the underlying value types.

Obviously at some stage you will have to parse the input type to your special type and at that point you can either throw an exception, or define the parsing so that exceptions are impossible. 0 is read as 1 for example.

An alternative is to use null to mean an invalid crate. Now I can have a CrateFactory.CreateCrate(x,y,z) which returns null if the parameters are invalid and a show crates function which tests for null and outputs the required string.

I'm not sure thats much different from a bool IsValid flag though

OP here. I saw my problem as a need to postpone exception handling messages. Namely, exception handling interrupts the flow of the program to deliver an exception message right at the point of failure. I needed to postpone this reporting until the time of the view layer takes effect.

In other words I needed a way to store the exception messages and information about a lack of a Crate in case a Crate failed to initialize.

I thus created a wrapper object of [Crate|NULL + Exception message|NULL] and called it CrateRecord

Not sure if it is a way to go but it seems to be working.

class CrateRecord
{
    /** @var Crate */
    protected $crate;

    /** @var string */
    protected $error;

    function __construct(Crate $crate = null, string $error)
    {
        $this->crate = $crate;
        $this->error = $error;
    }

    public function getCrate()
    {
        return $this->crate;
    }

    public function getError()
    {
        return $this->error;
    }
}

class CrateRequestHandler
{

    function handle()
    {
        // mocked up data from Request
        // this is sample data for showcase purposes only
        // it usually supplied by user or from database
        // and it is impossible to tell ahead of time
        // if it will be correct for Crate purposes
        $data = array(
            array(2, 5, 9),
            array(1, 3, 4),
            array(0, 0, 0), //invalid
            array(5, 6, 3)
        );

        $crateRecords = array();
        foreach($data as $key => $d)
        {
            try
            {
                $crate = new Crate($d[0], $d[1], $d[2]);
                $error = "";
            }
            catch (Exception $e)
            {
                $crate = null;
                $error = $e->getMessage();
            }
            $crateRecords[$key] = new CrateRecord($crate, $error);
        }

        // send to View
        $this->showCrates($crateRecords);
    }

    /**
     *
     * @param CrateRecord[] $crateRecords            
     */
    function showCrates(array $crateRecords)
    {
        foreach($crateRecords as $key => $crateRecord)
            if ($crateRecord->getCrate())
                echo 'Crate #' . $key . ': ' . $crateRecord->getCrate()->getDimensions() . PHP_EOL;
            else
                echo 'Crate #' . $key . ': ' . $crateRecord->getError() . PHP_EOL;
    }
}

// To call
$handler = new CrateRequestHandler();
$handler->handle();

The problem here is that, despite your description of the problem and the name you have given your method, you aren't actually showing a Crate. You are displaying something else altogether. You are displaying a message indicating the result of attempting to create a Crate.

class CrateRequestHandler
{
    function handle(ServerRequestInterface $request)
    {
        $messages = [];
        // data is undefined
        foreach ($data as $key => $dimensions) {
            try {
                $crate = new Crate(...$dimensions);
                $messages[$key] = 'Crate #' . $key . ':' . $crate->getDimensions();
            } catch (Exception $ex) {
                $messages[$key] = 'Crate #' . $key . ':' . $ex->getMessage();
            }
        }

        $this->showMessages($messages);
    }

    function showMessages(array $messages) : void
    {
        foreach ($messages as $message) {
            echo $message . PHP_EOL;
        }
    }
}

I'm frankly astonished at the answers in this thread and sheer amount of over-engineering I am seeing when a simple try/catch along with a subtle shift in our understanding of the problem is all that is necessary.

Licensed under: CC-BY-SA with attribution
scroll top