2
\$\begingroup\$

I'm making a data parser for road tax data (costs) which parses a JSON file with all weird variable names, which the customer gave me.

The costs are different on each vehicle type. The costs at some vehicles are also splitted over the provinces.

class RoadTaxDataParser
{
    /**
     * @var array $data The data for every vehicle type
     */
    private $data = [];

    /**
     * @var array
     */
    private $dataMaps = [
        "MotorCycle"   => 'dataMotor',
        "CampingCar"   => [
            'commerciallyRented'    => [
                'zeeland' => 'data_0_ZL'
                //.. more data
            ],
            'notCommerciallyRented' => [
                'zeeland' => 'data_1_ZL'
                //.. more data
            ],
        ],
        "PassengerCar" => [
            'zeeland' => 'dataZL'
        ]
        //.. more data
    ];

    private $isRented;

    /**
     * @param              $file string Parses data from a JSON file into the $data variable
     */
    public function __construct ($file) {
        $this->data = json_decode(file_get_contents($file));
    }

    /**
     * @param VehicleType  $vehicleType
     *
     * @param VehicleOwner $vehicleOwner
     *
     * @return mixed
     */
    public function getDataByVehicle (VehicleType $vehicleType, VehicleOwner $vehicleOwner) {
        $vehicleType = ($this->resolveVehicleTypeWithData($vehicleType));
        $resolveVehicleTypeDataKey = ($this->resolveVehicleTypeDataKey($vehicleType, $vehicleOwner));
        if (is_a($vehicleType, "CampingCar")) {
            /**
             * @type $vehicleType CampingCar
             */
            $this->isRented = $vehicleType->isCommerciallyRented();
        }
        return $this->data->$resolveVehicleTypeDataKey;
    }

    /**
     * Returns the right vehicle type with its data the user has chosen
     *
     * @param \VehicleType $vehicleType The vehicle type
     *
     * @return array|null The array of the vehicle type with it's data.
     */
    public function resolveVehicleTypeWithData (VehicleType $vehicleType) {
        foreach ($this->dataMaps as $vehicleTypeClassName => $vehicleTypeArrayKey) {
            if ($vehicleTypeClassName === get_class($vehicleType)) {
                return [$vehicleTypeClassName => $vehicleTypeArrayKey];
            }
            return null;
        }
        return null;
    }

    /**
     * @param array         $vehicleTypeWithData
     *
     * @param \VehicleOwner $vehicleOwner
     *
     * @return string
     */
    public function resolveVehicleTypeDataKey (array $vehicleTypeWithData, VehicleOwner $vehicleOwner) {
        $vehicleClassName = key($vehicleTypeWithData);
        $vehicleTypeValue = end($vehicleTypeWithData);

        if (is_array($vehicleTypeValue)) {
            $vehicleOwnersProvince = strtolower(Province::getProvinceName($vehicleOwner->getProvince()));

            /*
             * Check if the vehicle is a camping car
             */
            if ($vehicleClassName === "CampingCar") {
                foreach ($vehicleTypeValue as $isRentedString => $provinceData) {
                    if ($isRentedString === 'commerciallyRented' && $this->isRented == true) {
                        foreach ($provinceData as $provinceName => $provinceKey) {
                            if ($provinceName === $vehicleOwnersProvince) {
                                return $provinceKey;
                            }
                            return null;
                        }
                    } elseif ($isRentedString === 'notCommerciallyRented' && $this->isRented == false) {
                        foreach ($provinceData as $provinceName => $provinceKey) {
                            if ($provinceName === $vehicleOwnersProvince) {
                                return $provinceKey;
                            }
                            return null;
                        }
                    }
                }
            }

            foreach ($vehicleTypeValue as $provinceName => $provinceKey) {
                if ($provinceName === $vehicleOwnersProvince) {
                    return $provinceKey;
                }
                return null;
            }
        }
        return $vehicleTypeValue;
    }
}

For setting the correct variable names i used the $dataMaps array to map all variable names to specific classes.

The camping car is also splitted by if the camping car is commercially rented or not.

A VehicleType class looks like this:

/**
 *
 */
class CampingCar extends Car
{
    /**
     * @var
     */
    private $weight;

    /**
     * @var
     */
    private $fuelType;

    /**
     * @var
     */
    private $isCommerciallyRented;

    /**
     * @param $weight               float
     * @param $fuelType             int
     * @param $isCommerciallyRented bool
     */
    public function __construct ($weight, $fuelType, $isCommerciallyRented) {
        $this->weight               = $weight;
        $this->fuelType             = $fuelType;
        $this->isCommerciallyRented = $isCommerciallyRented;
    }

    /**
     * @return mixed
     */
    public function getWeight () {
        return $this->weight;
    }

    /**
     * @return mixed
     */
    public function getFuelType () {
        return $this->fuelType;
    }

    /**
     * @return mixed
     */
    public function isCommerciallyRented () {
        return $this->isCommerciallyRented;
    }
}

(Car extends VehicleType)

Problems

  • It's messy
  • The functions are to big and unreadable
  • The isRented property is'nt really something that belongs to a RoadTaxDataParser

Questions

  • How do I split functionality of this class over multiple functions?
  • How do I make my code more readable?
  • How do i make my code less messy (so much {....} blocks..)
\$\endgroup\$
  • \$\begingroup\$ Your $isRented property is set after the call to resolveVehicleTypeDataKey, thus, it is never used anyways since its always null (at least if you only call getDataByVehicle on the instance). \$\endgroup\$ – Fleshgrinder Jan 13 '15 at 15:20
  • \$\begingroup\$ @Fleshgrinder Well, it is being set when you call that method indeed, and whenever the VehicleType is a CampingCar. \$\endgroup\$ – Bas Jan 13 '15 at 15:28
1
\$\begingroup\$

There are multiple issues in your class.

Declaring Properties

$data

/**
 * The data for every vehicle type.
 * @var array
 */
protected $data;
  • Properties are documented like methods if you declare them, the syntax you used is for parameters of methods.
  • There is no need for private, always start with protected and only make something private if you have a reason, that you can explain to another developer, for it.
  • Do not waste memory for no reason, no need to initialize the property with an array.

$dataMaps

/**
 * Mapping of ???
 * @var array
 */
protected static $dataMaps = [
    "MotorCycle"   => 'dataMotor',
    "CampingCar"   => [
        'commerciallyRented'    => [
            'zeeland' => 'data_0_ZL'
            //.. more data
        ],
        'notCommerciallyRented' => [
            'zeeland' => 'data_1_ZL'
            //.. more data
        ],
    ],
    "PassengerCar" => [
        'zeeland' => 'dataZL'
    ]
    //.. more data
];
  • Documentation is missing.
  • This property is better of being static since it will be the same for each instance of this class.

$isRented

Now we agree that this property should not be part of this class in the first place, no argument there. The joke is, you are not using it and you could delete it right away. If you take a look at the body of the getDataByVehicle method, you'll notice that the call to resolveVehicleTypeDataKey is before $isRented ever gets a value assigned.

    $resolveVehicleTypeDataKey = ($this->resolveVehicleTypeDataKey($vehicleType, $vehicleOwner));
    if (is_a($vehicleType, "CampingCar")) {
        $this->isRented = $vehicleType->isCommerciallyRented();
    }

In effect this means that the two checks within the resolveVehicleTypeDataKey method never evaluate to true, since $isRented is still set to null from its declaration. This means that $isRented is only used you call getDataByVehicle once and call resolveVehicleTypeDataKey again after that call finished. Since it is set to the correct boolean value after the call to getDataByVehicle, but only if the second call is performed with the very same vehicle (otherwise $isRented even has a wrong value, the value from the previous vehicle you called getDataByVehicle with).

This is a clear bug of your complete implementation.

Calling Methods

You do not need the brackets around method calls, e.g.:

    $vehicleType = ($this->resolveVehicleTypeWithData($vehicleType));
    $resolveVehicleTypeDataKey = ($this->resolveVehicleTypeDataKey($vehicleType, $vehicleOwner));

The correct way to write this:

    $vehicleType = $this->resolveVehicleTypeWithData($vehicleType);
    $resolveVehicleTypeDataKey = $this->resolveVehicleTypeDataKey($vehicleType, $vehicleOwner);

Naming Things

The correct taxonomy of vehicles does not start with a vehicle type, it starts with vehicle. You should try to use the appropriate names for things, especially if they represent things of the real world.

Use short but precise names, this includes that you will not bloat the names with unnecessary information.

public function getData(VehicleInterface $vehicle, OwnerInterface $owner) {
    $class = get_class($vehicle);

    if (!isset($this->dataMaps[$class]) {
        throw new \Exception;
    }

    $dataKey = $this->dataMaps[$class];

    if ($dataKey === (array) $dataKey) {
        $dataKey = $this->flattenDataKey($dataKey);
    }

    return $this->data->{$dataKey};
}

Never mind the changed code, just concentrate on the short names of everything. Notice that they are shorter, faster to read, and still more descriptive than yours?

Program against interfaces

It will make your life much easier in the future.

Array key exists?

Do not loop over a complete array, just to see if it contains a key.

NOPE

foreach ($this->dataMaps as $vehicleTypeClassName => $vehicleTypeArrayKey) {
    if ($vehicleTypeClassName === get_class($vehicleType)) {
        return [$vehicleTypeClassName => $vehicleTypeArrayKey];
    }
    return null;
}

YEAH

if (isset($this->dataMaps[$class])) {
    // exists
}
// does not exist

Keep it simple and stupid and Don’t Repeat Yourself (DRY)

Well, here is a cleaned version of your first class, notice how I keep it simple and stupid while doing everything once?

<?php

class RoadTaxDataParser
{

    protected $data;

    protected static $dataMaps = [ /* code code code code */ ];

    public function __construct($uri) {
        $this->data = json_decode(file_get_contents($uri));
    }

    public function getData(VehicleInterface $vehicle, OwnerInterface $owner)
    {
        $class = get_class($vehicle);

        if (!isset(static::$dataMaps[$class])) {
            throw new Exception("Could not find mapping for class.");
        }

        $dataKey = static::$dataMaps[$class];

        if (is_array($dataKey)) {
            if ($vehicle instanceof RentableInterface) {
                if ($vehicle->isRented()) {
                    $dataKey = $dataKey["commerciallyRented"];
                } else {
                    $dataKey = $dataKey["notCommerciallyRented"];
                }
            }

            $province = strtolower((new Province($owner->getProvince()))->getName());

            if (!isset($dataKey[$province])) {
                throw new Exception("Could not find province.");
            }

            $dataKey = $dataKey[$province];
        }

        return $this->data->{$dataKey};
    }

}

This method is not too long, it easily fits a normal screen and all paths are very easy to follow and understand.

Scaffolds

Notice how I use object oriented programming to implement absolutely everything only once while still allowing an implementing concrete class to overwrite anything for customization. The interfaces make sure that the outside world can be sure, that every class of an interface can be accessed the same way, but what they do with it is up to them. The trait and abstract class provide default implementations. I now have a toolbox available for ultra fast creation of new vehicles, even new rentable vehicles.

Vehicle Interface

<?php

interface VehicleInterface {

    public function getWeight();

    public function getFuelType();

}

Rntable Interface

<?php

interface RentableInterface {

    public function isRented();

}

Rentable Trait

<?php

trait RentableTrait {

    protected $rented;

    public function isRented() {
        return $this->rented;
    }

}

Abstract Vehicle

<?php

abstract class Vehicle implements VehicleInterface {

    protected $weight;

    protected $fuelType;

    public function getWeight() {
        return $this->weight;
    }

    public function getFuelType() {
        return $this->fuelType;
    }

}

Car

<?php

class Car extends Vehicle {
    // Intentionally left blank.
}

Camping Car

<?php

class CampingCar extends Vehicle implements RentableInterface {
    use RentableTrait;
    // Intentionally left blank.
}
\$\endgroup\$
  • \$\begingroup\$ Thank you very much, but as i noticed one thing, the CampingCar is the only thing that could be rented, is that a reason to create a whole interface? \$\endgroup\$ – Bas Jan 13 '15 at 17:40
  • \$\begingroup\$ Well, it depends, if you want your program to be easily extensible: yes. If you just want it to cover the existing requirements and possibly refactor at a later point: no. \$\endgroup\$ – Fleshgrinder Jan 13 '15 at 17:47
  • \$\begingroup\$ Alright :) And i see you pushed it all into one method, why did you do that? \$\endgroup\$ – Bas Jan 13 '15 at 17:47
  • \$\begingroup\$ Because things are too simple and I see no use for the other methods in another context. Of course, if there is, it might make sense to start splitting it. \$\endgroup\$ – Fleshgrinder Jan 13 '15 at 17:49
  • \$\begingroup\$ But if you're going to split it, do you need to pass the same arguments all over again? In each function \$\endgroup\$ – Bas Jan 13 '15 at 17:50

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.