Calculating the angle between the clock's hour and minute hands with unit testing

I am learning PHP OOP and unit testing so I made a code using OOP and TDD approach where it calculates the degree of and angle betwee the clock's hour and minute hands.

I would like to know:

• Is the structure of my object class good? If not, what is the better approach?

• I know my test has covered the codes that need to be tested but is there anything I need to improve when it comes to unit testing?

_

hour    = explode(':', $time)[0];
        $this->minute  = explode(':', $time)[1];

        return $this->calculateAngleBetweenHands();
    }

    private function angleBetweenTwelveAndHourHand() {
        return self::HOUR_HAND_DEGREE_ROTATION_PER_MINUTES * (self::ONE_HOUR_IN_MINUTES * $this->hour + $this->minute);
    }

    private function angleBetweenTwelveAndMinuteHand() {
        return self::MINUTE_HAND_DEGREE_ROTATION_PER_MINUTES * $this->minute;
    }

    private function calculateAngleBetweenHands() {
        $angle = abs($this->angleBetweenTwelveAndHourHand() - $this->angleBetweenTwelveAndMinuteHand());

        if ($this->isAngleMoreThanHalfRotation($angle)) {
            return self::FULL_ROTATION_DEGREES - $angle;
        }

        return $angle;
    }

    private function isAngleMoreThanHalfRotation($angle) {
        return $angle > self::HALF_ROTATION_DEGREES;
    }
}

Test Code

```

clockAngle = new ClockAngle();
$this->reflector = new \ReflectionClass($this->clockAngle);
}

/**
* @dataProvider timeInputs
*/
public function test_it_can_get_angle_degree_between_clock_hands($input, $expected, $message) {
$angle = $this->clockAngle->getAngleBetweenClockHands($input);
$this->assertEquals($angle, $expected, $message);
}

public function timeInputs() {
return array(
'1:00' => array('1:00', 30, 'The angle between 1 and 00 is 30'),
'1:15' => array('1:15', 52.5, 'The angle between 1 and 15 is 52.5'),
'2:15' => array('2:15', 22.5, 'T

You seem to be caught in between making a decision on whether this should be a concrete class or a class to be used statically. If concrete as currently designed, I don't understand why you wouldn't just pass the time to the constructor and set the angle as a property which can be read vs. recalculatated every time via a method call.

If you truly want to treat these methods in essence as static methods like you are currently doing, then consider making these methods static and don't store state.

I think you may have gone a bit overboard in making granular methods here for such a simple calculation. This is reflected in your unit tests where you are trying to do the right thing to test each individual method, but end up exercising the same lines of code over and over again.

None of these more granular methods are ever exercised outside the context of being called downstream from getAngleBetweenClockHands() and will always be called exactly in the same order and exactly the same number of times regardless of input value. This seems like breaking apart a logically monolithic operation just for the sake of trying to get "methods that do only one thing". There are only like 10 lines of logic in this whole class, do they really need to be broken apart?

public function getAngleBetweenClockHands($time) { 
    $this->hour    = explode(':', $time)[0];
    $this->minute  = explode(':', $time)[1];

    return $this->calculateAngleBetweenHands();
}

You may consider passing DateTime object to this method instead of just a string. That gives you the ability to enforce the parameter type via typehint. As it stands right now, you have nothing here to validate that a proper string format is passed.

There is no reason to explode() twice here. Just explode once and work with the resulting array.

If wanting to work with concrete objects this logic should be in the constructor so that you can enforce that the object gets set up in proper state.

How does this method handle AM/PM? Is it using 24 hour clock?

Your test data only reflect values < 12:00, it seems as if you are missing coverage here.

Be consistent in using either camelCase or snake_case in your method names. Don't mix them in the same code section like you are doing in your unit tests.

I don't understand in your unit tests why you have one method using a data provider but then a slew of additional test methods that just test different values which could probably be covered in the method using the data provider.

Your tests are all happy path. You are testing no edge cases at all (i.e. bad input).

StackExchange Code Review Q#152919, answer score: 3