Benchmark/timer library - follow-up

Question

I created a timer library called PHPBenchTime a few years ago, and decided to rewrite it last week.

This new version is meant to add in missing functionality (pause/unpause) as well as expand on the reporting system (so now everything is a lap and has finer details). Additionally, the rewrite was aimed at cleaning the code and making it easier to follow the flow of information/expand on the library in the future (by making $laps the primary data point).

namespace PHPBenchTime;

class Timer {

    /**
     * Time that $this->start() was called
     *
     * @var int
     */
    private $startTime = 0;

    /**
     * Time that $this->end() was called
     *
     * @var int
     */
    private $endTime = 0;

    /**
     * Total time spent in pause
     *
     * @var int
     */
    private $totalPauseTime = 0;

    /**
     * Time spent in pause
     *
     * @var int
     */
    private $pauseTime = 0;

    /**
     * Difference between $this->startTime and $this->endTime
     *
     * @var int
     */
    private $totalTime = 0;

    /**
     * Contains all laps
     *
     * @var array
     */
    private $laps = array();

    /**
     * Is the timer currently actively running?
     *
     * @var bool
     */
    private $isRunning = false;

    /**
     * Determine if we are paused
     *
     * @var bool
     */
    private $isPaused = false;

    /**
     * Keeps track of what lap we are currently on
     *
     * @var int
     */
    private $lapCount = 0;

    /**
     * Class constructor
     */
    public function __construct() {
        $this->reset();
    }

    /**
     * Resets the timers, laps and summary
     */
    public function reset() {
        $this->startTime = 0;
        $this->endTime   = 0;
        $this->pauseTime = 0;
        $this->totalTime = 0;
        $this->laps      = array();
        $this->isRunning = false;
        $this->isPaused  = false;
        $this->lapCount  = 0;
    }

    /**
     * Starts the timer
     */
    public function start() {
        $this->setRunningPaused( true, false );

        # Set the start time
        $this->startTime = $this->getCurrentTime();

        # Create a lap with this start time
        $this->lap( "start" );
    }

    /**
     * Ends the timer
     */
    public function end() {
        $this->setRunningPaused( false, true );

        # Set the end time
        $this->endTime = $this->getCurrentTime();

        # end the last lap
        $this->endLap();

        return $this->summary();
    }

    /**
     * Creates a new lap in lap array property
     */
    public function lap( $name = null ) {
        $lapTime = $this->getCurrentTime();

        # end the last lap
        $this->endLap();

        # Create new lap
        $this->laps[] = array(
            "name"  => ( $name ? $name : $this->lapCount ),
            "start" => $lapTime,
            "end"   => -1,
            "total" => -1,
        );

        $this->lapCount += 1;
    }

    /**
     * Returns a summary of all timer activity so far
     *
     * @return array
     */
    public function summary() {
        $this->totalTime = $this->endTime - $this->startTime;

        $summary = array(
            'running' => ( $this->isRunning === true ? "true" : "false" ),
            'start'   => $this->startTime,
            'end'     => $this->endTime,
            'total'   => $this->totalTime,
            'paused'  => $this->totalPauseTime,
            'laps'    => $this->laps
        );

        return $summary;
    }

    /**
     * Initiates a pause in the timer
     */
    public function pause() {
        $this->setRunningPaused( false, true );
        $this->pauseTime = $this->getCurrentTime();
    }

    /**
     * Cancels the pause previously set
     */
    public function unPause() {
        $this->setRunningPaused( true, false );
        $this->totalPauseTime = $this->getCurrentTime() - $this->pauseTime;
        $this->pauseTime      = 0;
    }

    /**
     * Assign end and total times to the previous lap
     */
    private function endLap() {
        $lapCount = count( $this->laps ) - 1;
        if ( count( $this->laps ) > 0 ) {
            $this->laps[$lapCount]['end']   = $this->getCurrentTime();
            $this->laps[$lapCount]['total'] = $this->laps[$lapCount]['end'] - $this->laps[$lapCount]['start'];
        }
    }

    /**
     * Handles isRunning and isPaused
     *
     * @param $running
     * @param $paused
     */
    private function setRunningPaused( $running, $paused ) {
        $this->isRunning = is_bool( $running ) ? $running : false;
        $this->isPaused  = is_bool( $paused ) ? $paused : false;
    }

    /**
     * Returns the current time
     *
     * @return float
     */
    private function getCurrentTime() {
        return microtime( true );
    }
}

Same code, without comments:

namespace PHPBenchTime;

class Timer {
    private $startTime = 0;
    private $endTime = 0;
    private $totalPauseTime = 0;
    private $pauseTime = 0;
    private $totalTime = 0;
    private $laps = array();
    private $isRunning = false;
    private $isPaused = false;
    private $lapCount = 0;

    public function __construct() {
        $this->reset();
    }

    public function reset() {
        $this->startTime = 0;
        $this->endTime   = 0;
        $this->pauseTime = 0;
        $this->totalTime = 0;
        $this->laps      = array();
        $this->isRunning = false;
        $this->isPaused  = false;
        $this->lapCount  = 0;
    }

    public function start() {
        $this->setRunningPaused( true, false );
        $this->startTime = $this->getCurrentTime();
        $this->lap( "start" );
    }

    public function end() {
        $this->setRunningPaused( false, true );
        $this->endTime = $this->getCurrentTime();
        $this->endLap();
        return $this->summary();
    }

    public function lap( $name = null ) {
        $lapTime = $this->getCurrentTime();
        $this->endLap();

        $this->laps[] = array(
            "name"  => ( $name ? $name : $this->lapCount ),
            "start" => $lapTime,
            "end"   => -1,
            "total" => -1,
        );

        $this->lapCount += 1;
    }

    public function summary() {
        $this->totalTime = $this->endTime - $this->startTime;

        $summary = array(
            'running' => ( $this->isRunning === true ? "true" : "false" ),
            'start'   => $this->startTime,
            'end'     => $this->endTime,
            'total'   => $this->totalTime,
            'paused'  => $this->totalPauseTime,
            'laps'    => $this->laps
        );

        return $summary;
    }

    public function pause() {
        $this->setRunningPaused( false, true );
        $this->pauseTime = $this->getCurrentTime();
    }

    public function unPause() {
        $this->setRunningPaused( true, false );
        $this->totalPauseTime = $this->getCurrentTime() - $this->pauseTime;
        $this->pauseTime      = 0;
    }

    private function endLap() {
        $lapCount = count( $this->laps ) - 1;
        if ( count( $this->laps ) > 0 ) {
            $this->laps[$lapCount]['end']   = $this->getCurrentTime();
            $this->laps[$lapCount]['total'] = $this->laps[$lapCount]['end'] - $this->laps[$lapCount]['start'];
        }
    }

    private function setRunningPaused( $running, $paused ) {
        $this->isRunning = is_bool( $running ) ? $running : false;
        $this->isPaused  = is_bool( $paused ) ? $paused : false;
    }

    private function getCurrentTime() {
        return microtime( true );
    }
}

Usage and expected result:

use PHPBenchTime\Timer;
$T = new Timer;
$T->start();
sleep(1);
$T->lap();
sleep(2);
$T->lap();

print_r($T->end());

Array
(
    [running] => false
    [start] => 1406563433.8282
    [end] => 1406563436.8289
    [total] => 3.0007529258728
    [paused] => 0
    [laps] => Array
        (
            [0] => Array
                (
                    [name] => start
                    [start] => 1406563433.8282
                    [end] => 1406563434.8285
                    [total] => 1.0003159046173
                )

            [1] => Array
                (
                    [name] => 1
                    [start] => 1406563434.8285
                    [end] => 1406563436.8289
                    [total] => 2.0004169940948
                )

            [2] => Array
                (
                    [name] => 2
                    [start] => 1406563436.8289
                    [end] => 1406563436.8289
                    [total] => 7.7962875366211E-5
                )

        )

)

Pinoniq · Accepted Answer · 2014-08-05 08:37:19Z

Some remarks

why do you call the reset method in your constructor? The constructor is called on object creation, so nothing has changed yet. Everything still has it's default values.

I like your approach of laps and calling it laps. Makes it easy to understand since we all know how a timer of laps works. But, I don't think it is optimal. for instance:

$t = new Timer();
$t->start();
//I need to call lap now because I can't pass in a name in the start method
$t->lap('database');
//do some DB stuff
$t->lap();
//do some random stuff
$t->lap('database');
//do some more db stuff
$t->end();

We now ha ve lap called start that added some execution time, but nothing happened. It is only there because we wanted to pass in a different name then start.

Now a good thing would be to be able to retrieve all laps with a certain name. Or sort them by name. But this could be arguably also be handled by the view. So more of a improvement request ;)

$t->summary('database');

A timer should be lightweight

You really don't want you timer to be complex. It should do the least possible things. Your method setRunningPaused and how you handle state is smelly. First of, your method could be improved:

private function setRunningPaused( $running, $paused ) {
    $this->isRunning = !!$running ;
    $this->isPaused  = !!$paused ;
}

yes, it doesn't do the exact same thing. But it's a private method. If we pass in 1 we probably mean true.

A better approach would probably be the use of constants:

class Timer {
    const RUNNING = 1;
    const PAUSED  = 0;
    const STOPPED = -1;
}

And then instead of

public function start() {
    $this->setRunningPaused( true, false );
}

we use constants to set the state:

public function start() {
    $this->state = Timer::RUNNING;
}

Reads better, and you don't have the worry about the edge case where somehow the timer is running and paused at the same time setRunningPaused(true, true);

Only return when I want it to

when I call $t->end(); it returns the summary. why? this seems undesirable. I want to end the timer, not get the summary. I didn't know it was returning the summary after I looked into the code. Simply remove the return there.

Only use variables if you are actualy going to use them

for instance your lap method. You are creating a $lapTime variable, but you aren't really using it. Only to pass it to the array. This will do the job aswell:

$this->laps[] = array(
    "start" => $this->getCurrentTime()
);

endlap does to much

In the endlap, you calculate the current position of the array pointer. There is afunction for that: key So:

$lapCount = key($this->laps);

another option here would be to use a pointer reference instead of accessing the laps array multiple time. But this should only be done if it reads easier:

$lastLap          = &current($this->laps);
$lastLap['end']   = $this->getCurrentTime();
$lastLap['total'] = $lastLap['end'] - $lastLap['start'];

Unused variables.

You return a totaltime in your summary. It is even a property of the class. but it is only used inside summary() method. So simply remove itfrom the class and make it a variable of the method.

Multiple pauses

You don't allow multiple pauses (it will give a faulty output). In your unpause methoed:

$this->totalPauseTime = $this->getCurrentTime() - $this->pauseTime;

should probably be

$this->totalPauseTime += $this->getCurrentTime() - $this->pauseTime;

You decieded to not end a lap if you pause. this is correct. But, it gives some peculiar output:

$t->lap('test');
sleep(1);
$t->pause();
sleep(1);
$t->unpause();
sleep(1);
$t->endlap();

The lap has no knowledge of a pause.

Let's create a new timer class

Ofcourse, some are just opinions. But I rewrote your class to how I would write it looking at the comments I gave:

<?php namespace PHPBenchTime;

class Timer {

    /**
     * These constants define the state of our timer.
     *
     * @var int
     */
    const RUNNING = 1;
    const PAUSED  = 0;
    const STOPPED = -1;

    /**
     * Holds the current state of the timer
     * 
     * @var int
     */
    private $state;

    /**
     * Time that $this->start() was called
     *
     * @var int
     */
    private $startTime = 0;

    /**
     * Time that $this->end() was called
     *
     * @var int
     */
    private $endTime = 0;

    /**
     * Contains all laps
     *
     * @var array
     */
    private $laps = array();

    /**
     * The time the pause button was hit
     * 
     * @var float
     */
    private $pauseTime = -1;

    /**
     * Resets the timers, laps and summary
     */
    public function reset()
    {
        $this->startTime = 0;
        $this->endTime   = 0;
        $this->laps      = array();
        $this->state     = Timer::STOPPED;
    }

    /**
     * Starts the timer
     */
    public function start($name)
    {
        # Set the start time
        $this->startTime = $this->getCurrentTime();

        # Create a lap with this start time
        $this->lap($name);
    }

    /**
     * Ends the timer
     */
    public function end()
    {
        $this->state = Timer::STOPPED;

        # First end the last lap
        $this->endLap();

        # Set the end time
        $this->endTime = $this->getCurrentTime();
    }

    /**
     * Creates a new lap in lap array property
     */
    public function lap( $name = false )
    {
        #set the state to running
        $this->state = Timer::RUNNING;

        # end the last lap
        $this->endLap();

        # Create new lap
        $this->laps[] = array(
            "name"  => ( $name ? $name : $this->lapCount ),
            "start" => $this->getCurrentTime(),
            "pause" => 0,
            "end"   => -1,
            "total" => -1,
        );
    }

    /**
     * Returns a summary of all timer activity so far
     *
     * @return array
     */
    public function summary()
    {
        $totalTime = $this->endTime - $this->startTime;

        $summary = array(
            'running' => ( $this->state === Timer::RUNNING),
            'start'   => $this->startTime,
            'end'     => $this->endTime,
            'total'   => $totalTime
        );

        return $summary;
    }

    /**
     * Initiates a pause in the timer.
     * If allready paused, do nothing
     */
    public function pause()
    {
        if ( $this->state !== Timer::PAUSED )
        {
            $this->state = Timer::PAUSED;
            $this->pauseTime = $this->getCurrentTime();
        }
    }

    /**
     * Cancels the pause previously set
     */
    public function unPause()
    {
        if ( $this->state === Timer::PAUSED )
        {
            $this->state     = Timer::RUNNING;
            $this->laps[key($this->laps)]['pause'] += $this->getCurrentTime() - $this->pauseTime
            $this->pauseTime = -1;
        }
    }

    /**
     * Assign end and total times to the previous lap
     */
    private function endLap()
    {
        if ( $this->state !== Timer::STOPPED )
        {
            $lastLap          = &current($this->laps);
            $lastLap['end']   = $this->getCurrentTime();
            $lastLap['total'] = $lastLap['end'] - $lastLap['start'] - $lastLap['pause'];
        }
    }

    /**
     * Returns the current time
     *
     * @return float
     */
    private function getCurrentTime()
    {
        return microtime(true);
    }
}

DISCLAIMER

I didn't test this code, so don't shoot me for syntax errors ;) I wrote this on a tablet without a dev enviroment :p

\$\begingroup\$ Very excellent critique and advice! Thank you @Pinoniq \$\endgroup\$

jsanc623
– jsanc623

2014-08-05 14:02:52 +00:00
Commented Aug 5, 2014 at 14:02 — jsanc623
– jsanc623, Commented Aug 5, 2014 at 14:02
\$\begingroup\$ you are welcome :) Glad to know it helped! \$\endgroup\$

Pinoniq
– Pinoniq

2014-08-05 14:17:53 +00:00
Commented Aug 5, 2014 at 14:17 — Pinoniq
– Pinoniq, Commented Aug 5, 2014 at 14:17

Stack Exchange Network

Benchmark/timer library - follow-up

1 Answer 1

Some remarks

A timer should be lightweight

Only return when I want it to

Only use variables if you are actualy going to use them

endlap does to much

Unused variables.

Multiple pauses

Let's create a new timer class

DISCLAIMER

You must log in to answer this question.

Hot Network Questions

Benchmark/timer library - follow-up

1 Answer 1

Some remarks

A timer should be lightweight

Only return when I want it to

Only use variables if you are actualy going to use them

endlap does to much

Unused variables.

Multiple pauses

Let's create a new timer class

DISCLAIMER

You must log in to answer this question.

Related

Hot Network Questions