Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement StopWatch lap #709

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Implement StopWatch lap #709

wants to merge 1 commit into from

Conversation

michaldo
Copy link

@michaldo michaldo commented Feb 5, 2021

Example:
StopWatch watch = StopWatch.createStarted();
sleep(30ms)
watch.lap() // returns 30
sleep(10ms)
watch.lap() // returns 10

Example:
StopWatch watch = StopWatch.createStarted();
sleep(30ms)
watch.lap() // returns 30
sleep(10ms)
watch.lap() // returns 10
@michaldo
Copy link
Author

michaldo commented Feb 5, 2021

BTW, split has a bug: if stop watch is suspended and resumed, startTimeNanos is moved forward but stopTimeNanos not. For example:

watch.start()
watch.split()
watch.suspend()
sleep(100ms)
watch.resume()
watch.getSplitTime() // -100

IMHO split is overengineering. Instead of split()->getSplitTime()->unsplit() I can just call watch.getTime() when I need. I suggest delete split functionality at all.

BTW2, I suggest also drop reset, because buying brand new StopWatch cost nothing.
BTW3, I suggest also drop a stop, because when StopWatch is done, caller just calls watch.getTime() and does not care StopWatch anymore. There is no battery nor thread inside the StopWatch, and stop is not needed.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 94.978% when pulling 2b82829 on michaldo:master into a149529 on apache:master.

@garydgregory
Copy link
Member

garydgregory commented Feb 7, 2021

I think you need to explain in the class Javadoc how lap works, especially in contrast to split. What does it mean to lap a stopped watch, and such edge cases, and error states.

The whole class makes me wonder if we need to redo it in terms of java.time.Duration.

@michaldo
Copy link
Author

michaldo commented Feb 8, 2021

@garydgregory, explanation how StopWatch works would be much more demanding task that implementing lap()!

The problem is that current StopWatch is good model how physical device works, but bad class to use in software development. It is over-complicated.

First problem is state diagram (life-cycle). As a software developer, I want stop watch to measure how long runs some piece of code. I want get time spent between start and stop point, without learning stopwatch states.
Second problem is API. The difference between split and lap must be clarified because both exists. Split is overhead, because every time I need split stopwatch time, I can just call getTime().

I'm afraid that current StopWatch passed point of no return: bringing back to operability needs rebuild from scratch. I don't know if it is acceptable in commons-lang.

What do you think about creating new class, SimpleStopWatching with three methods:

static SimpleStopWatching.started();
SimpleStopWatching.getTime()
SimpleStopWatching.lap()

?
That API is self-descriptive (I will add Javadoc of course), simple, without state diagram and functionally identical with StopWatch. For example suspend/resume can be implemented with two simple stop watched.

SimpleStopWatch will match commons-lang spirit: classes from the library are not a rocket science, but saves developer write few lines of code. SimpleStopWatch match commons-lang, StopWatch match joda-time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants