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

rename maybeStartOrStopPolling to pollIfAnyControllersOpen #4533

Merged
merged 1 commit into from
Nov 22, 2021

Conversation

Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Nov 20, 2021

Cherry-picking a small change from #3641 to clean up the code smell of naming symbols with "maybe".

@Swiftb0y
Copy link
Member

Naming a method "maybe..." is not an issue with the name per se but rather the design of the class (big fat IMO). The proposed renaming just leaks implementation details.
What was your motivation for cherry-picking this commit instead of continuing your effort on #3641?

@Be-ing
Copy link
Contributor Author

Be-ing commented Nov 20, 2021

There is a lot more work to do on #3641 which is really difficult. As we agreed months ago, we are not focusing on the controller system rewrite for now in order to focus on more pressing issues.

@JoergAtGithub
Copy link
Member

If I look on the name maybeStartOrStopPolling alone, I've no clue, what this function does. While the new name is understandable to me without reading the code.

@Holzhaus
Copy link
Member

If I look on the name maybeStartOrStopPolling alone, I've no clue, what this function does. While the new name is understandable to me without reading the code.

I agree. However, it's also true that the new name leaks implementation details. And it's inconsistent with other parts of the codebase where we use used th3 tryFoo() pattern (e.g. the beat manipulation methods). So I think we should Name the method tryPoll()or something like that.

@Be-ing
Copy link
Contributor Author

Be-ing commented Nov 21, 2021

tryPoll implies that the function checks if polling fails. It does not.

@uklotzde
Copy link
Contributor

tryPoll implies that the function checks if polling fails. It does not.

I agree that the try prefix is commonly used for fallible functions. It does not fit here.

@uklotzde
Copy link
Contributor

The following remarks are out of scope of this PR! Just some thoughts to give you an idea how to improve the design.

Ideally, the function should return an enum class value that indicates what happened: StartedPolling, StoppedPolling. Maybe wrapped into a std::optional to indicate that the state didn't change, i.e. when polling continues or has already been stopped. Or you could add a 3rd enum variant for this case.

The function name could simply reflect the purpose, e.g. updatePollingStateAfterControllersAddedOrRemoved().

The current polling state can be derived from the state of the internal timer, i.e. it is either Idle or Busy.

States and state changes should be modeled explicitly from my experience.

@Swiftb0y
Copy link
Member

This is just a trivial name change and it seems that the change overall is welcomed, so lets just merge. Objections?

@uklotzde
Copy link
Contributor

The new name is a slight improvement. A better name might evolve when refactoring this code. LGTM

@uklotzde uklotzde merged commit 3e2c90d into mixxxdj:main Nov 22, 2021
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.

None yet

5 participants