Any Code Reviewer/s

Using Arduino Programming Questions
#1

Just out of curiosity, as I approach near the end of my project, UMT Coil Winder Rev. 1. Is there someone or people that would review my code and give constructive comments on how to improve it? Basically what I'm looking for is someone to point out code that will or may cause potential problems. I have spent hours and hours of reading and trying different code and I'm getting better a correcting my own mistakes but, I feel it's always best to have someone review the code. Looking at things through different eyes always brings new insight, any volunteers?

Thank you

Ray

P.S. I need to find a way to quit using the String function, LOL. A hand slapper maybe!

#2

Testing code as you write it is the best reviewer! Does your code do what you want? IF so, leave it alone. Mucking about with working code just adds errors that have to be discovered and corrected.

Paul

#3

post your code. if not too tangled i review it

#4

Paul_KD7HB:
Does your code do what you want? IF so, leave it alone. Mucking about with working code just adds errors that have to be discovered and corrected.

Just because code works doesn’t mean it’s good code. The adage “if it aint broke, don’t fix it” has serious consequences. Also, upgrading code is a great learning tool for programmers at all levels.

#5

Power_Broker:
Just because code works doesn't mean it's good code. The adage "if it aint broke, don't fix it" has serious consequences. Also, upgrading code is a great learning tool for programmers at all levels.

My experience is just the opposite!

For years I worked as a programmer for a computer service bureau in Portland, OR. I worked in the banking division. Banks have all their data processing done at night. I had to be on-call to go in and fix program problems when they occurred. Usually after midnight.

One night I got a call about a program problem. I went in to the office and discovered the compile date of the program did not match the latest listing. I compiled the program, again and all was good.

At a meeting next day to discuss the problem, a new programmer, Chuck, admitted he had changed the program because he didn't think the code looked right. He was new, lived across the street from the business, had a key and was bored, so he had begun to study the programs and discovered what he though was an error. So, he fixed it without ever telling anyone or testing the program. Chuck was thoroughly chastised!

The problem with people wanting to make programs look pretty and think they can make them more efficient, what ever that is, is that they never thoroughly test the program changes as they are made. Same people who write a huge program and then begin to test. Only then do they post on the forum asking for help.

The correct procedure to make a program the way you want to change it, is to never destroy the current working program source. Always make a copy of the source code so you have an ultimate backup that can be the gold standard to compare the new version to. But that takes planning, doesn't it?

Paul

#6

it's pretty common practice that code is not accepted without review.

when I worked at Bell Labs, a Bell System update to the network in New Your City suffered a problem affecting emergency services and air traffic control. So the communication network is just one example of system that can have a grave affect

there are two types of tools: one to manage changes in the code and the other to facilitate reviews. One of the original tools for managing software changes was called SCCS, Source Code Control System. Others examples are RCS, Clearcase and Git. They all allow software changes to be submitted to the system and once approved, code is then built with those changes. If something goes wrong, those changes can be backed out restoring the code.

Code reviews were less systematic, but just as serious. At least one current tool I used at Qualcomm and which i believe is common through the Linux community is a web based tool called Gerrit. You would submit your change and Gerrit would facilitate the code being reviewed by one or more reviewers, any one of which could disapprove and and block the change

it seems incredibly naive and irresponsible for anyone not to take code reviews seriously and some don't. There's just no excuse for not reviewing code.

IBM decades ago had a goal of find 1 flaw per 100 lines of code. If too many flaws were found, the review was held to early, if too few were found, the review was held too late. The point being that many flaws could be found by a code review taking a couple hours by a few people and that takes less man hour that the developer spending several extra days testing. In other words, code reviews accelerate development.

#7

If you post your code, I'll take a look.

#8

So, he fixed it without ever telling anyone or testing the program.

And that is the actual point of OM Paul's story: the issue there was not whether or not the working code was good or bad, or whether or not Chuck's changes were improvements or not, but that Chuck worked outside the process. Even if Chuck's code had worked and been an improvement, it would have been unauthorised and therefore wrong in the grand scheme of things.

#9

Paul_KD7HB:
But that takes planning, doesn't it?

Who said it doesn't take planning? It sure wasn't me. You're also arguing against a point I didn't make and against assumptions that aren't there.

I'm not assuming OP is actually in charge of developing code for banks or is part of a team that includes senior devs. Nor am I suggesting OP use un-tested code. But the idea of letting a codebase drown in technical debt because "it works today" is dangerous in my experience. I know what it's like to inherent a software project like that from devs who shared your views...

Sure, in a team working on a software app code needs to be reviewed and tested - in fact that was half of my argument in the first place (don't discourage code reviews!). Yes, on real projects there needs to be a CI/CD process that must be completed before merging new code and circumventing those processes are dangerous. But do you really think a hobby Arduinoist has a CI/CD process, senior devs, or a project that could threaten life, limb, or treasure?

No. The normal hobbyist has none of these, so there's no process to circumnavigate, no senior devs to code review (although if a forum member can do the review, it's a big plus), and if the project has a software malfunction during operation, it usually means some green LED failed to light when a button on an iPhone app was pressed.

I also never said thoroughly testing updated code wasn't necessary, because it is. I figured it went without saying.

Also, I can promise you that the average hobbyist codebase is complete dog **** and needs refactoring in some way. When said hobbyist does refactor and test their codebase, they will learn a ton and make their code easier to understand, easier to maintain, and more robust in the long run.

Don't discourage beginners from expanding their knowledge and improving their code by telling others they shouldn't seek reviews from more experienced software devs!

OP, you can post your code and we'll look at it.

#10

Power_Broker:
But do you really think a hobby Arduinoist has ... a project that could threaten life, limb?

There are too many of those actually, imo. And those stem from folk thinking ok, it's easy to get a servo arm to go up and down, my model train level-crossing works really nicely, so I'll attach a sharp blade to one and automate the cutting part of my factory process to speed things up, (inadvertently) crossing the divide between hobby-ing and engineer-ing.

Does your code do what you want? IF so, leave it alone

What about errors that haven't shown themselves yet? The writer may in ignorance have used too short a data type somewhere but his or her hobby-grade Q&D testing may not yet have caused a value to go under zero, and someone with more experience may see that one coming a mile off.

That's where the blade on a servo in a factory will chop someone's arms off....

(My tongue's in my cheek a bit here ofc, but not much....)

#11

The problem with code reviews is they come after the problem is usually too far gone to do anything about. I mean, you've worked on some huge project for a year and then it gets reviewed just before release? If its a mess, that's a year wasted. Or, if its a mess that works, it may be years of other people's time to maintain the monster.

Also I'm a big believer in.. "If in doubt, throw it out.". I've worked in a few corporate projects that resembled collage rental refrigerators. People would show up, write code for a few years and then leave. A co-worker and I got our hands on a tool that would show us "dead code", code that was never actually called when compiled. We figured we'd find some in there. We found TONS! We spent most of the night tossing out source code. Just a mess!

-jimlee

#12

jimLee:
The problem with code reviews is they come after the problem is usually too far gone to do anything about. I mean, you've worked on some huge project for a year and then it gets reviewed just before release? If its a mess, that's a year wasted. Or, if its a mess that works, it may be years of other people's time to maintain the monster.

Also I'm a big believer in.. "If in doubt, throw it out.". I've worked in a few corporate projects that resembled collage rental refrigerators. People would show up, write code for a few years and then leave. A co-worker and I got our hands on a tool that would show us "dead code", code that was never actually called when compiled. We figured we'd find some in there. We found TONS! We spent most of the night tossing out source code. Just a mess!

-jimlee

Jimlee, this is not a general philosophical discussion. With all due respect, IMHO, you're missing the point. He's just asking for help: philosophy is too late for him now in this instance. Don't be distracted by being negative, let's just be pragmatic : let those people who are ready to help to take a look at his code if they want, and see if what's he's asking is realistic.

#13

jslovi:
Jimlee, this is not a general philosophical discussion.

I think it became one when the OP never posted anything.

#14

I would review your code. I also have a code to be reviewed and would appreciate if you could do mines as well!

#15

jslovi:
Jimlee, this is not a general philosophical discussion. With all due respect, IMHO, you're missing the point. He's just asking for help: philosophy is too late for him now in this instance. Don't be distracted by being negative, let's just be pragmatic : let those people who are ready to help to take a look at his code if they want, and see if what's he's asking is realistic.

jslovi, you're arguing discussion semantics in a bar.

-jim lee

#16

aarg:
I think it became one when the OP never posted anything.

LOL. Good point ! :slight_smile:

#17

jimLee:
The problem with code reviews is they come after the problem is usually too far gone to do anything about.

they shouldn't be if you're serious about it.

maybe you're just looking for a pat on the back?

#18

gcjr:
they shouldn't be if you're serious about it.

maybe you're just looking for a pat on the back?

Sure! Pat away.

-jim lee

#19

Oh boy so it seems that I opened a bit of a can of worms here.

Ok, I've been a computer analyst/programmer for 20 years now but most of that time was all DB stuff, code maintenance, reports and sh*t. So I know what I'm supposed to do, like document everything and I do mean document everything so others coming from behind me can understand it and maintain it! But that doesn't mean it happens every time but, I try just like all the other programmers. I'm not going to use Doxygen for this project.

Now having said that I should mention that I'm also a electronics technologist. But and this is a big but, it's only been about 2 years that I started writing firmware and it's all been with MicroChip stuff. Very simple stuff I should mention. Now that I'm retired and sometimes retarded LOL I have been using dev boards to hone my skills.

The different places that I have worked at or have done work for always had a senior or group review the code before implementing it. Just because it works on your computer doesn't mean it's going to work in production. I have reviewed code myself and recommended changes that improved performance by 3x. I have also written code that worked great on my computer but, not on others because I failed to notice something small. My ego is not so big that it can't stand some criticism

I have several projects on the go, they range from this coil winder to a UL certified life saving device using a STM32 processor along with LoRa and a certified RTOS. UL Certification will cost @$50,000 so before I tackle that I want to get some best practices for embedded under my belt. I already have a device that is all analog out in the field, including at airports across North America so the planes don't blow up when refueling, it is sold by Lind Equipment under license.

I haven't I posted my code because it is WIP, when it's ready for review I'll post it. I plan to release everything under GPL so anyone can make changes and/or make their own so I want to make rev.1 a very good start. Do I need a review no but, I would like one because this is my first Arduino project and I have seen too many projects that work great until they go live, pushed out, sent out, or go into production. If this project were just for myself I wouldn't bother with a review. I am a very big fan of code review. I'll probably post it here or in another thread in hopefully a weeks time.

Ray

#20

I forgot to mention that I have posted code snippets when I needed help solving a problem and got excellent help.

Ray