Request: Code Review

[dr-o]

Hi, I have 480 lines of code separated in different files. Since I am used to code-reviews and input from more experienced users are always welcome, I wanted to ask kindly, if you can check the code out and see any obvious code smells and antipatterns in C. If such a request is not permitted, I beg my pardon.

Technologies:

• Analog Read
• PROGMEM
• TFT
• Serial reading (in form of GPS)
• Several geometric calculations

Initial Problems:

• Fuel Gauge is not working properly on my motorcycle
• Oil Temperature Gauge is placed very inconveniently, also through vibrations impossible to read values while driving
• Due to an accident last year also added an early warning system for probable accident sections on road.

Problems which are solved by following classes:

• InternalVoltage.h: Analog read InternalVoltage and provide this value for further Readings of Fuel.h and OilTemperature.h
• Fuel.h, OilTemperature.h: AnalogRead the Sensor Data and provide this for UI.
• DisplayOnTFT: Frontend Functions packed in one class and provides an API for the main.
• EmbeddedImages: Small Icons which are shown on UI.
• FrontendVariables: A collection of defines. Using define instead of variables to save memory.
• GpsAPI: Wraps the GPS functionality in a single class (Read and save values) and provides shorter, more comprehensible variables for passing by.
• RadarPositions.h: A collection of probable accident sections on road.
• RadarPositionsAPI.h: This class has the boilerplate code to read from PROGMEM. It provides an API to:
  • cache the next Point of Interest.
  • check if the next Point of Interest is in front of me
  • find the distance to the next Point of interest
• DistanceCalculator: Calculates distance (and sadly also Bearings) to next Point of Interest.

GitHub Link to Project

Thank you.

[gcjr]

when we did design reviews at Qualcomm, notes were provided describing the problem being solved and how the changes corrected the problem, otherwise your guessing at what the code is intended to do.

[system]

What's an "antipattern"?

[pcbbc]

Anti-pattern

[dr-o]

Hi. Added some more detailled describtion, what is happening there @gcjr. I hope this helps.

[gcjr]

dr-o:
Added some more detailled describtion

where?

i'm told github supports web based peer reviews. any good tutorials? I found Pull Requests, Code Review and the GitHub Flow. We used Gerrit at Qualcomm and i'm interested in understanding how to do this myself

[system]

gcjr:
where?

In the opening post....

[dr-o]

gcjr:
where?

i'm told github supports web based peer reviews. any good tutorials? I found Pull Requests, Code Review and the GitHub Flow. We used Gerrit at Qualcomm and i'm interested in understanding how to do this myself

To Do that, I require an initial brach and a Pull request from another branch. I am on mobile right now and will explain it to you very detailed how that works on git when I am back at home.

[gcjr]

dr-o:
To Do that, I require an initial brach and a Pull request from another branch. I am on mobile right now and will explain it to you very detailed how that works on git when I am back at home.

certainly there must be a way to review an initial submission. The video suggests send a pull request to a reviewer

is this what you had in mind or too much? i'd like to understand how to use the github feature

[dr-o]

Allright gcjr, since I dont know how familiar you are with Git, I will give you a small crash course Don't get mad at me when I explain stuff you already know.

What is Git?
Git is a version control software. People used to use SVN earlier, now is Git one of the most convenient way to manage your software. Therefore Git can be used for solo projects. Allthough Git has some very nice stuff for working in Groups.

How to work with Git?
Git makes it possible to copy the code and manipulate on it without changing the source material. This is called a branch. When you create a branch, a certain Code state will be copied and you can do your changes without interrupting anyone elses work. Almost every well-structured Project has at these two branches: master & development.

When you want to add a new feature to your project, or fix a bug, you create a branch from development - let's call it feaute/newShinyStuff. You make your changes on this new branch feaute/newShinyStuff. When you think, your work is done, your tests are written and your stuff is documented, you create a merge request towards development.

Now the other contributers check your code, sometimes download and build locally, give feedback and open discussion. Very important thing here is, everybody can create a merge request but only contributers can code review. (It is possible for you to fork my project, change my code to fix a bug and then create a merge request into my code)

If the merge request is accepted by contributers, before you deploy / release, you run all the tests (unit, integration, e2e, manual)on development branch and if, only if, the status is stable, you create a merge request towards master. This usually gets accepted pretty fast, when builds are green.

When you are working alone though, you don't need branches, since you can always move your Git HEAD back to a certain commit. A commit is a saved (and usually uploaded) state of your code.

Merge Requests in Detail:
A Merge Request can only occur from a branch to another, ultimately the code reaching to master. When you create a merge request, Git creates a very nice diff between old files and new files. This is sometimes buggy with auto formatting code but still very handy. A diff looks like this:

If you have something to say about a certain piece of code, you can select a line and write a comment. Developer pushed the code gets notificated by an email and the code can be improved.

Initial Code Review:
As already mentioned, a merge request can only be created from one branch to another. Therefore an initial merge request requires an empty master (with a dummy file, since there can't be any empty git repositories) and a branch with your full version 1.0 code. This is not the way to go tbh. In Multi-developer projects, you should start code reviewing as soon as possible. Since this is a one man private project, I pushed into master with comprehensible commit messages and all was fine.

Allthough the above idea with new repository and a branch with full code is not bad, there is only one problem: I have to add everyone who are ready to review my code as contributor. Sadly, a contributor has full access to the repository and can also alter the code, commit history and fully delete it without history, leaving me without a backup... Yeah, there is that.

What can you do as non-contributor:
Github allows to create issues for every repository. These are official problems people found with your code, or problems they had with your library etc. You can check out the code and write an issue

I hope, I could answer your question gcjr.

[gcjr]

are you saying i just need to download the code or look at it on github and put comments as text in the thread?

i've used git code managing changes. I've used gerrit to review initial submissions (a new file) as well as changes to existing files and post comments in the gerrit.

[dr-o]

No no no. You can check out the code in your browser and select the line, then create a github issue. Check this official thread out

Creating an issue - GitHub Docs

or tl;dr

• go to the file
• select the line(s) with mouse click (with shift you can select more lines)
• click on the "..." left of the lines
• "Reference in new issue"

Sorry, never used gerrit. I have experience in Github, Gitlab and Bitbucket. But is the same on all. You can't create a merge request (and therefore a code review request) without two different branches.

[gcjr]

thanks for your patience but do i need to create a pull request?

going to the file on the github link you provided and simply clicking on a line doesn't do anything. double clicking the line highlights it right clicking it shows browser options

[dr-o]

Absolutly no problems mate.

Here, I made a video for you:

[gcjr]

dr-o

thanks for the video. very helpful. (At Qualcomm, and working with many experienced developers from Bell Labs who were learning yet another code reviewing process, we often just went to one another to figure out how to use the latest and greatest tool)

in the meantime, i just downloaded your code and started looking at it with vi and ctags which brings the file and line with a symbol definitions.

i have a general comment about your use of classes for voltage, oil pressure and fuel. each class contains functions for measuring the value and limits, but the test for it is in loop(). I think it would make more sense for the class to completely handle the measurement and tests. loop() becomes trivial.

i assume one issue was displaying information on the TFT. Since DisplayOnTft.h has specific functions for voltage, oil and fuel, i think it would make sense to have functions in that class for setting a display parameter (or string) and the class have a function called once in loop() to update the display.

regarding code inspection vs review. At Bell Labs we started having code "inspections" (late 80s). The goal was to find bugs, not improve the code. So any suggested changes along the lines: it would be better, smaller, faster, easier to read, ... weren't policy at that time ... If it works, it's ok.

this was mostly true at Qualcomm, except there were coding style guidelines that had to be followed regarding indentation, Capitalization of variables and constant, use of symbols vs hardcoded values and naming conventions (typedefs, enums, ...)

my comments above are not inspection comments. I realize I'm suggesting architectural changes which you may be reluctant to do at this time. I understand.

but I hope you understand that I believe making the classes more complete, they do everything pertaining to the object they represent, makes the code easier to read and review/inspect.

again, thanks for the video.

greg

[dr-o]

Hey greg,

I can also create a pull request video when do some changes. Then you see how things are handled in reality (Pull requests). Since you mentioned Quallcom, I can only guess, you will work with internal people who are part of the project (and therefore contributers). In a real life scenario, you will use pull requests with your collagues, not issues.

To your Feedback:

I don't have any problems with making architectural changes. I moreover didn't quietly understand what you want to be changed.

I guess, the "test in the loop" you mentioned is this line?

if (!alert && (time - oilTemperatureSensor.lastTimeSensorRead) > oilTemperatureSensor.sensorReadInterval) {

so you suggest, just call the function with the parameters and the class handles the query? That way, tftScreen.displayFuel() should be introduced in Class Fuel, not in main loop()?

Do I understand this correctly? If yes, this is a nice idea and will fix this after the initial field testing tomorrow.

[dr-o]

Hey greg,

I did some minor changes and created a merge request so you can see that. I can't post the video here though, since it is 6mb large and therefore cant upload here on forum you may pass me your Email address and I will send you the gif file.

[gcjr]

i walked into the office of a co-worker to ask about git years ago and he said "your not the only one with a dunce cap on". that's how i feel

I see on this page that you've made recent changes. I would expect to be able to view them but don't seem to be pressing the correct buttons. I re-downloaded the code from this page and it appears all the files are the same. I must not have downloaded from the the branch. I get to this page to compare changes and it says zero changed files.

i'm trying but this isn't obvious to me

moreover didn't quietly understand what you want to be changed.

i'm trying to suggest that loop() make a calls a class function, something like voltage.chk() that measures the voltage, compares it to the parameters defined in the class and calls a tft function to report whatever on the display. loop() knows nothing about voltage except to call the function.

same for oil pressure and fuel. not sure about gps/radar, by something similar seems possible

since loop() makes individual calls to each, they are independent of one another except for the tft display. If you want to keep them independent of tft, maybe the function should return a string that is passed to the tft class. this may or may not be important to you depending on if you ever want to re-use or enhance the code (i don't think it's worth sweating the philosophical conundrums).

tft.voltage (voltage.chk ());

maximizing cohesion and minimizing coupling are usually good things

cargill provides examples of misusing C++