Disclaimer: No offense is intended toward the many talented EEs who may or may not be on this forum (fora?). I lived with an EE for three years and he seemed alright. Besides, the things in this post are not that bad, just small errors that I thought were interesting for highlighting the difference between the way the EEs and the rest of us think about programs.
So I'm the only person on my team whose education was in Computer Science rather than Mechanical Engineering or Electrical Engineering. Since I'm able to bring a different perspective to the software we produce, my manager considers me a valuable member of the team. One of the first things I did at this job was conduct a code review of a C++ program written by a few of the team members. This software runs on a Texas Instruments microcontroller, so space is extremely limited (including ROM space, where the executable code lives).
One of the first things I noticed was a misunderstanding of compilation units and the behavior of the keyword extern. One file, main_vars.h, contained the following global variable definitions:
typedef struct Control_Setpoints { int Message_Type; int Message_Length; unsigned long long Time_Stamp_Sec; int Time_MS; /* snip 40 more fields */ } Control_Setpoints; /* snip 8 other structs just as large */
And in another file, main_extern_vars.h...
extern struct Control_Setpoints { int Message_Type; int Message_Length; unsigned long long Time_Stamp_Sec; int Time_MS; /* snip 40 more fields */ } Control_Setpoints; /* snip the rest of the structs, also duplicated entirely with the extern keyword added */
main_vars.h was included in one place and main_extern_vars.h was included everywhere else. Which means that any changes had to be duplicated across these files - which took a while, given the size of the files, and any typos would cause a compilation error. Half the comments for the file were in main_vars.h and the other half were in main_extern_vars.h. I moved the typedefs into one file, included it in the other vars.h files, and removed the superfluous type re-declarations.
The next disconnect was sort of amusing: the true and false keywords were nowhere to be found in the program, despite the 100+ boolean variables. Assigning to one of them was done with 1s and 0s, as in voltageError = 1; thingComplete = 0; etc. I assume this was done due to the authors thinking of the booleans directly as bits - which is understandable, given that they're EEs - and ignored the abstraction provided for them. I asked about changing them all to true and false and got a nonenthusiastic response, so I left it.
I also came across the following construct:
if (GIODINA & 8 == 8) voltageAlarm = 1; else voltageAlarm = 0;WTF, I thought. Never mind that GIODINA is a TI-provided variable of a TI-provided type which allows you to simply say GIODINA.bit3 to get the fourth bit. Why not replace that statement with voltageAlarm = GIODINA & 8;, assigning the bit to the boolean value directly? There were dozens of these throughout the codebase. I replaced them all with the corresponding one-liner. In one case this caused a bug since they used this construct with an integer, not a boolean. But since they assign to booleans and numbers the same way I couldn't tell which type the variable was without digging through the aforementioned struct typedefs - I just did a quick replace. Though I may be TRWTF for not making sure of that before committing the change.
Bonus WTF: the compiler used for this program is the 2006 version of IAR's C/C++ compiler for ARMs. We're required to turn off all optimization flags during compilation because they "might change the behavior of the program in some way". Since this is the nuclear industry, any amount of uncertainty is completely unacceptable. So unless I want to spend a few weeks digging through assembly to prove that the optimized and non-optimized compiler output are functionally identical I cannot use a new version or enable any optimizations.