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

GPS text: added grade, cadence, temperature + some small updates #934

Merged
merged 2 commits into from
Jul 28, 2023

Conversation

dany123
Copy link
Contributor

@dany123 dany123 commented Jul 25, 2023

I have a quick note about the re-formatting rules: in the long if..else if chains with multiple lines of code inside each if block it absolutely needs a newline somewhere between each if block otherwise the entire chain looks like a huge blob of text and finding something is harder than it needs to be (see the code below line 281 in the first diff below).
For short blocks it does look nice and cozy but those are readable no matter how you write them.

@ddennedy
Copy link
Member

looks like a huge blob of text

Maybe this is a sign that you need to refactor into more functions. You can also add comment lines as headings for sections of code perhaps followed by an empty line.

@dany123
Copy link
Contributor Author

dany123 commented Jul 25, 2023

Comments to group some of these does sound like a pretty good workaround; I'm not sure refactoring further would be a good idea, the code would be too scattered (in this particular case).

src/modules/qt/filter_gpstext.cpp Outdated Show resolved Hide resolved
src/modules/qt/filter_gpstext.cpp Outdated Show resolved Hide resolved
@ddennedy ddennedy added this to the v7.20.0 milestone Jul 28, 2023
@ddennedy ddennedy merged commit e3da1c6 into mltframework:master Jul 28, 2023
6 checks passed
@dany123 dany123 deleted the gpstext_grade_and_cadence branch October 6, 2023 19:18
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.

2 participants