Refactor away the bool argument of Timer
#2926
Labels
C-Feature
A new feature, making something new possible
Comments
Masterhigure
added
C-Feature
|
Another option would be to replace the boolean with an enum |
|
It would also be possible to use two constructors instead of two structs. |
That is... maybe a better option. I'll include it in the main post. |
Masterhigure
changed the title
bool argument of Timer
|
Maybe if we resolve this in a way that keeps a single struct, |
|
There's a PR for DiscreteTimer and DiscreteStopWatch to : #2683 (very usefull for FixedTimeStep). If there's a change, I guess this need to be refined. |
james7132
pushed a commit
to james7132/bevy
that referenced
this issue
Oct 19, 2022
) As mentioned in bevyengine#2926, it's better to have an explicit type that clearly communicates the intent of the timer mode rather than an opaque boolean, which can be only understood when knowing the signature or having to look up the documentation. This also opens up a way to merge different timers, such as `Stopwatch`, and possibly future ones, such as `DiscreteStopwatch` and `DiscreteTimer` from bevyengine#2683, into one struct. Signed-off-by: Lena Milizé <me@lvmn.org> # Objective Fixes bevyengine#2926. ## Solution Introduce `TimerMode` which replaces the `bool` argument of `Timer` constructors. A `Default` value for `TimerMode` is `Once`. --- ## Changelog ### Added - `TimerMode` enum, along with variants `TimerMode::Once` and `TimerMode::Repeating` ### Changed - Replace `bool` argument of `Timer::new` and `Timer::from_seconds` with `TimerMode` - Change `repeating: bool` field of `Timer` with `mode: TimerMode` ## Migration Guide - Replace `Timer::new(duration, false)` with `Timer::new(duration, TimerMode::Once)`. - Replace `Timer::new(duration, true)` with `Timer::new(duration, TimerMode::Repeating)`. - Replace `Timer::from_seconds(seconds, false)` with `Timer::from_seconds(seconds, TimerMode::Once)`. - Replace `Timer::from_seconds(seconds, true)` with `Timer::from_seconds(seconds, TimerMode::Repeating)`. - Change `timer.repeating()` to `timer.mode() == TimerMode::Repeating`.
james7132
pushed a commit
to james7132/bevy
that referenced
this issue
Oct 28, 2022
) As mentioned in bevyengine#2926, it's better to have an explicit type that clearly communicates the intent of the timer mode rather than an opaque boolean, which can be only understood when knowing the signature or having to look up the documentation. This also opens up a way to merge different timers, such as `Stopwatch`, and possibly future ones, such as `DiscreteStopwatch` and `DiscreteTimer` from bevyengine#2683, into one struct. Signed-off-by: Lena Milizé <me@lvmn.org> # Objective Fixes bevyengine#2926. ## Solution Introduce `TimerMode` which replaces the `bool` argument of `Timer` constructors. A `Default` value for `TimerMode` is `Once`. --- ## Changelog ### Added - `TimerMode` enum, along with variants `TimerMode::Once` and `TimerMode::Repeating` ### Changed - Replace `bool` argument of `Timer::new` and `Timer::from_seconds` with `TimerMode` - Change `repeating: bool` field of `Timer` with `mode: TimerMode` ## Migration Guide - Replace `Timer::new(duration, false)` with `Timer::new(duration, TimerMode::Once)`. - Replace `Timer::new(duration, true)` with `Timer::new(duration, TimerMode::Repeating)`. - Replace `Timer::from_seconds(seconds, false)` with `Timer::from_seconds(seconds, TimerMode::Once)`. - Replace `Timer::from_seconds(seconds, true)` with `Timer::from_seconds(seconds, TimerMode::Repeating)`. - Change `timer.repeating()` to `timer.mode() == TimerMode::Repeating`.
Pietrek14
pushed a commit
to Pietrek14/bevy
that referenced
this issue
Dec 17, 2022
) As mentioned in bevyengine#2926, it's better to have an explicit type that clearly communicates the intent of the timer mode rather than an opaque boolean, which can be only understood when knowing the signature or having to look up the documentation. This also opens up a way to merge different timers, such as `Stopwatch`, and possibly future ones, such as `DiscreteStopwatch` and `DiscreteTimer` from bevyengine#2683, into one struct. Signed-off-by: Lena Milizé <me@lvmn.org> # Objective Fixes bevyengine#2926. ## Solution Introduce `TimerMode` which replaces the `bool` argument of `Timer` constructors. A `Default` value for `TimerMode` is `Once`. --- ## Changelog ### Added - `TimerMode` enum, along with variants `TimerMode::Once` and `TimerMode::Repeating` ### Changed - Replace `bool` argument of `Timer::new` and `Timer::from_seconds` with `TimerMode` - Change `repeating: bool` field of `Timer` with `mode: TimerMode` ## Migration Guide - Replace `Timer::new(duration, false)` with `Timer::new(duration, TimerMode::Once)`. - Replace `Timer::new(duration, true)` with `Timer::new(duration, TimerMode::Repeating)`. - Replace `Timer::from_seconds(seconds, false)` with `Timer::from_seconds(seconds, TimerMode::Once)`. - Replace `Timer::from_seconds(seconds, true)` with `Timer::from_seconds(seconds, TimerMode::Repeating)`. - Change `timer.repeating()` to `timer.mode() == TimerMode::Repeating`.
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this issue
Feb 1, 2023
) As mentioned in bevyengine#2926, it's better to have an explicit type that clearly communicates the intent of the timer mode rather than an opaque boolean, which can be only understood when knowing the signature or having to look up the documentation. This also opens up a way to merge different timers, such as `Stopwatch`, and possibly future ones, such as `DiscreteStopwatch` and `DiscreteTimer` from bevyengine#2683, into one struct. Signed-off-by: Lena Milizé <me@lvmn.org> # Objective Fixes bevyengine#2926. ## Solution Introduce `TimerMode` which replaces the `bool` argument of `Timer` constructors. A `Default` value for `TimerMode` is `Once`. --- ## Changelog ### Added - `TimerMode` enum, along with variants `TimerMode::Once` and `TimerMode::Repeating` ### Changed - Replace `bool` argument of `Timer::new` and `Timer::from_seconds` with `TimerMode` - Change `repeating: bool` field of `Timer` with `mode: TimerMode` ## Migration Guide - Replace `Timer::new(duration, false)` with `Timer::new(duration, TimerMode::Once)`. - Replace `Timer::new(duration, true)` with `Timer::new(duration, TimerMode::Repeating)`. - Replace `Timer::from_seconds(seconds, false)` with `Timer::from_seconds(seconds, TimerMode::Once)`. - Replace `Timer::from_seconds(seconds, true)` with `Timer::from_seconds(seconds, TimerMode::Repeating)`. - Change `timer.repeating()` to `timer.mode() == TimerMode::Repeating`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The
boolargument to theTimerconstructors should be refactored away.A function that takes in a
boolargument almost necessarily has an internalif-test that can be refactored into two separate functions. And the significance of theboolargument is almost never self-explanatory from reading the function call. (Consider, for instance, the first time you readTimer::from_seconds(2.0, true)in chapter 2.5 of the current Bevy book: The meaning of2.0is obvious, but what does atrueTimermean? How can aTimerbe false?) These are code smells. In addition, the nameTimer, at least to me, does not imply an automatic looping capability, because that's not how the timers you buy in hardware stores work (although I'm willing to concede this particular point).Also, if someone would want to extend
Timerin the future to include more modes, thisboolapproach is unfeasible.I propose that
Timeris split into two separate timer structs. For instanceTimerandLoopingTimer, orTimerOnceandTimerRepeating. (I am not terribly attached to these particular names, and other suggestions are welcome.)Other solutions (from comments below) include splitting the constructors, or swapping the
boolfor a more verboseenum TimerMode, both of which are self-explanatory at point of call, and also extensible should other kinds ofTimers be desired.This is related to issue #1127 that wants a
Cooldown-type timer.The text was updated successfully, but these errors were encountered: