PHP Vehicle / Event test

Question

I was given a test to write a class that calculates a vehicle's average and expected mileage without using a database or framework.

The mileages were to be calculated given some different types of events, which I've decided to implement in a flat way, as this is how I would store them in the DB.

If you could let me know what you think about it, that would be nice.

Event.php

namespace App;

class Event {
    protected $id = null;
    protected $event_type = null;
    protected $date = null;
    protected $mileage = null;
    protected $result = null;
    protected $price = null;
    protected $from_vrm = null;
    protected $to_vrm = null;

    private $eventTypes = ['ADVERTISED_FOR_SALE', 'MOT_TEST', 'VRM_CHANGE'];

    public function setId($id) {
        $this->id = $id;
    }

    public function setEventType($event_type) {
        if(!in_array($event_type, $this->eventTypes)) {
            throw new \Exception('Unrecognised event type');
        }
        else {
            $this->event_type = $event_type;
        }
    }

    public function setDate($date) {
        $this->date = $date;
    }

    public function setMileage($mileage) {
        $this->mileage = $mileage;
    }

    public function setResult($result) {
        $this->result = $result;
    }

    public function setPrice($price) {
        $this->price = $price;
    }

    public function setFromVrm($from_vrm) {
        $this->from_vrm = $from_vrm;
    }

    public function setToVrm($to_vrm) {
        $this->to_vrm = $to_vrm;
    }




    public function getId() {
        return $this->id;
    }

    public function getDate() {
        return $this->date;
    }

    public function getMilage() {
        return $this->mileage;
    }

    public function getEventType() {
        return $this->event_type;
    }
}

EventTest.php

class EventTest extends TestCase {
    public function testNamespacing() {
        $event = new Cazana\Event();
        $this->assertInstanceOf(Cazana\Event::class, $event);
    }

    public function testSetEventType() {
        $event = new Cazana\Event();
        $event->setEventType('ADVERTISED_FOR_SALE');
        $this->assertEquals($event->getEventType(), 'ADVERTISED_FOR_SALE');
    }

    public function testSetInvalidEventType() {
        $event = new Cazana\Event();
        $this->setExpectedException(Exception::class);
        $event->setEventType('INVALID_EVENT_TYPE');
    }


}

Vehicle.php

namespace App;

class Vehicle {
    protected $id = null;
    protected $vrm = null;
    protected $make = null;
    protected $model = null;
    protected $first_registration_date = null;
    protected $events = [];

    public function setId($id) {
        $this->id = $id;
    }

    public function setVrm($vrm) {
        $this->vrm = $vrm;
    }

    public function setMake($make) {
        $this->make = $make;
    }

    public function setModel($model) {
        $this->model = $model;
    }

    public function setFirstRegistrationDate($first_registration_date) {
        $this->first_registration_date = $first_registration_date;
    }

    public function addEvent(Event $event) {
        array_push($this->events, $event);
    }


    public function getId() {
        return $this->id;
    }

    public function getVrm() {
        return $this->vrm;
    }

    public function getMake() {
        return $this->make;
    }

    public function getModel() {
        return $this->model;
    }

    public function getFirstRegistrationDate() {
        return $this->first_registration_date;
    }

    public function getEvents() {
        return $this->events;
    }

    public function calculateAverageMilage() {
        if($this->first_registration_date == null) {
            throw new \Exception('No first registation date set, cannot calculate');
        }
        //calculate the average annual milage based off the last event that we have
        $lastEvent = null;
        foreach($this->events as $event) {
            //we only need to look at these two event types, as the others do not store milage info
            if(in_array($event->getEventType(), ['ADVERTISED_FOR_SALE', 'MOT_TEST'])) {
                if($lastEvent == null || ($event->getDate() > $lastEvent->getDate())) {
                    $lastEvent = $event;
                }
            }
        }
        if($lastEvent == null) {
            $average = 7900;
        }
        else {
            //return the last milage divided by the age of the car (at that time)
            $average = $lastEvent->getMilage() / ($lastEvent->getDate()->format('Y') - $this->first_registration_date->format('Y'));
        }
        return $average;
    }

    public function calculateCurrentMilage() {
        $current = new \DateTime();

        return $this->calculateAverageMilage() * ($current->format('Y') - $this->first_registration_date->format('Y'));
    }

}

VehicleTest.php

class VehicleTest extends TestCase {
    public function testNamespacing() {
        $vehicle = new App\Vehicle();
        $this->assertInstanceOf(App\Vehicle::class, $vehicle);
    }

    public function testAddEvent() {
        $vehicle = new \App\Vehicle();

        $event = new \App\Event();
        $event->setId(1);
        $event->setEventType('ADVERTISED_FOR_SALE');
        $event->setDate(new DateTime('10-10-2010'));
        $event->setMileage(10000);
        $event->setPrice(10000.00);

        $vehicle->addEvent($event);

        $this->assertContainsOnly(App\Event::class, $vehicle->getEvents());
    }

    public function testAverageMilage() {
        $vehicle = new \App\Vehicle();

        $event = new \App\Event();
        $event->setId(1);
        $event->setEventType('ADVERTISED_FOR_SALE');
        $event->setDate(new DateTime('10-01-2000'));
        $event->setMileage(5000);
        $event->setPrice(50000.00);

        $event2 = new \App\Event();
        $event2->setId(1);
        $event2->setEventType('MOT_TEST');
        $event2->setDate(new DateTime('10-05-2005'));
        $event2->setMileage(10000);
        $event->setResult(true);

        $event3 = new \App\Event();
        $event3->setId(3);
        $event3->setEventType('VRM_CHANGE');
        $event3->setDate(new DateTime('10-10-2010'));
        $event3->setFromVrm('ABC-123');
        $event3->setToVrm('DEF-456');

        $event4 = new \App\Event();
        $event4->setId(4);
        $event4->setEventType('ADVERTISED_FOR_SALE');
        $event4->setDate(new DateTime('10-01-2015'));
        $event4->setMileage(20000);
        $event4->setPrice(37000.00);

        $vehicle->addEvent($event);
        $vehicle->addEvent($event2);
        $vehicle->addEvent($event3);
        $vehicle->addEvent($event4);
        $vehicle->setFirstRegistrationDate(new DateTime('01-01-1998'));

        $this->assertGreaterThan(500, $vehicle->calculateAverageMilage());
    }

    public function testAverageMilageNoFirstRegistationDate() {
        //should throw an exception
        $vehicle = new \App\Vehicle();

        $this->setExpectedException(Exception::class);
        $vehicle->calculateAverageMilage();
    }

    public function testAverageMilageNoEvents() {
        //testing the average milage when we have no events, should be 7,900
        $vehicle = new \App\Vehicle();
        $vehicle->setFirstRegistrationDate(new DateTime('01-01-1998'));

        $this->assertEquals(7900, $vehicle->calculateAverageMilage());
    }

    public function testCurrentMilage() {
        $vehicle = new \App\Vehicle();

        $event = new \App\Event();
        $event->setId(1);
        $event->setEventType('ADVERTISED_FOR_SALE');
        $event->setDate(new DateTime('10-01-2000'));
        $event->setMileage(5000);
        $event->setPrice(50000.00);

        $event2 = new \App\Event();
        $event2->setId(1);
        $event2->setEventType('MOT_TEST');
        $event2->setDate(new DateTime('10-05-2005'));
        $event2->setMileage(10000);
        $event->setResult(true);

        $event3 = new \App\Event();
        $event3->setId(3);
        $event3->setEventType('VRM_CHANGE');
        $event3->setDate(new DateTime('10-10-2010'));
        $event3->setFromVrm('ABC-123');
        $event3->setToVrm('DEF-456');

        $event4 = new \App\Event();
        $event4->setId(4);
        $event4->setEventType('ADVERTISED_FOR_SALE');
        $event4->setDate(new DateTime('10-01-2015'));
        $event4->setMileage(20000);

        $event4->setPrice(37000.00);

        $vehicle->addEvent($event);
        $vehicle->addEvent($event2);
        $vehicle->addEvent($event3);
        $vehicle->addEvent($event4);
        $vehicle->setFirstRegistrationDate(new DateTime('01-01-1998'));

        $this->assertGreaterThan(20000, $vehicle->calculateCurrentMilage());
    }

    public function testCurrentMilageNoEvents() {
        //should return the result of the vehicles age * 7900
        $vehicle = new \App\Vehicle();

        $vehicle->setFirstRegistrationDate(new DateTime('01-01-1998'));

        $this->assertEquals($vehicle->calculateCurrentMilage(), 150100);
    }

    public function testCurrentMilageNoFirstRegistationDate() {
        //should throw an exception
        $vehicle = new \App\Vehicle();

        $this->setExpectedException(Exception::class);
        $vehicle->calculateCurrentMilage();
    }


}

Mike Brant · Accepted Answer · 2017-03-06 22:57:50Z

Overall

You only have a few cases where you validate data passed to your class setters. Generally, it is a good idea to type hint and/or validate (when type hinting can not be performed) all values passed to public class methods before working with them in your classes.
I would strongly suggest you get in the habit of using strict comparisons (===, !==) as your default comparison methodology. Loose comparisons tend to lead to bugs in your code around unexpected truthy/falsey evaluations.
I don't understand your namespacing. Why is Cazana used in the event test class? Consider using a more meaningful namespace (App is not very meaningful).
In your unit tests, you should consider using setUp() and teardown() methods to manage your test fixtures (your objects) set up for each test path. There is no reason to be instantiating these repeatedly in every test method.
Be consistent in writing assertions in your test cases. There are times where you are inverting order of expected and actual values in your assertions. Typically, expected value goes first. Make sure you look at the documentation to properly understand parameter ordering for your various assertions.
Make sure and test your exceptions in your test cases, not just your happy path.
Use data providers in your test methods to simplify you test code. There is no reason to write the same code X times in a test to cover X different input values. Use a data provider to provide variable input to your methods to simplify the test code.
Consider using annotations around your test methods, particularly @dataProvider, @covers and @expectedException. These can greatly simplify your test code.
Use code coverage tools and you will find you have a lot of lines of code which are not exercised by your test cases.
Don't mix snake_case and camelCase in your user-defined code. Be consistent.

Event class

Should this class be this mutable? In other words, should caller be able to individually changes values for an event like currently allowed, or is the "event" something that should be set up on a one-time basis (i.e. by passing values to a constructor) and then be immutable? The reason I ask this is that you talk about potentially working with a database in the future, so you would need to think about what changes to the class object mean with regards to the backing data in the database.
Consider setting event_types as constant on this class as opposed to property.
On setEventType() consider throwing the more specific \InvalidArgumentException in case where bad value is passed.
Should dates use specific date format (like DateTime class for example)?

Vehicle Class

Again I wonder about mutability of these class objects and whether it is appropriate as shown.
You are allowing this class to be set up in a bad state. Your calculateAverageMileage() method will throw exception if their is no first registration date, so why not enforce that a first registration data must be set in class constructor?
Consider refactoring the foreach loop in calcualteAverageMileage() into a method to get most recent event. You might also consider enforcing event ordering upon insertion of events into the storage array such that you don't need to loop the whole event array every time you want to get the last event - you could just grab the last event (or first event if reverse ordered) from the array.
I don't understand the logic of supplying a value of 7900 when last event is not populated? Should you document this and/or or make this value a class constant?
Is calculateCurrentMilage() a meaningful method name? It seems like you are "estimating" mileage, not calculating it.

Thank you for your comment, I agree with you on many points - especially the mutability of the classes. A couple of questions: Can you recommend a library for use with php to generate code coverage? When you talk about the enforcement of event ordering, are you referring to something as simple as a stack? Thanks again for taking the time to go through my code. — Ben Robey
– Ben Robey, Commented Mar 7, 2017 at 11:07
@BenRobey PHPUnit can be configured to generate code coverage assuming that your dev/test environment has Xdebug or phpdbg. You could conceivably use a stack, queue, linked list, etc. for event ordering, depending on what your needs are and how you need to populate events into the collection. — Mike Brant
– Mike Brant, Commented Mar 7, 2017 at 14:14

Stack Exchange Network

PHP Vehicle / Event test

1 Answer 1

You must log in to answer this question.

Hot Network Questions

PHP Vehicle / Event test

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions