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

EngineBuffer: Add const & order methods #2997

Merged
merged 2 commits into from
Aug 7, 2020

Conversation

xeruf
Copy link
Contributor

@xeruf xeruf commented Aug 5, 2020

I was really struggling to understand the methods and their interactions, because the methods often seem to be sprinkled in at random order. I tried to at least somewhat improve logical grouping and match the order of declarations and definitions.
Also adds const to all getters.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such a reorder commit breaks the blame feature.
I don't see much benefit of such a reorder PR.
Do you actually navigate through a file by scrolling?

@xeruf
Copy link
Contributor Author

xeruf commented Aug 6, 2020

No I of course do not navigate by scrolling, but when I try to understand the relationships of methods, I go back and forth and it is much easier if the functions are close to each other because I can much more easily view them on one screen without using extra effort for splitting. There was so much chaos in that class that made my life unnecessarily hard.
And then there are changes like this, which allows you to view both methods side-by-side and actually understand what is going on.

I for one value developer productivity over the git blame feature, which, as I mentioned before, can be configured to skip individual commits if you so choose. Besides, how often do you really use that? I know it can be useful, but compared to the time I spent working in this class the slight inconvenience such a change introduces to using git blame pales in comparison, in my experience.

@xeruf xeruf requested a review from daschuer August 6, 2020 12:21
@ywwg
Copy link
Member

ywwg commented Aug 6, 2020

This seems like minimal reordering, and Blame is overrated (and still accessible if you go back in the past). I'm ok with cleanups like this

@xeruf
Copy link
Contributor Author

xeruf commented Aug 7, 2020

So, we can merge? :)

@Holzhaus Holzhaus merged commit 38e08cf into mixxxdj:master Aug 7, 2020
@xeruf xeruf deleted the cleanup-enginebuffer branch August 25, 2020 12:19
@xeruf xeruf mentioned this pull request Aug 29, 2020
13 tasks
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.

4 participants