How to go from simple working code to professional code

[whcirobotics]

Originally Posted by SuperS_5

If you post an example of your code that you don't think is not good, then maybe we can give some pointers directly related your code.

Here is the link to the code, i could not upload through megaupload for some reason so this is through Transfer Big files. https://www.transferbigfiles.com/69e...vGqTl9VG1ePaw2

Any pointers:?

[SuperS_5]

Browsing around, I have to say that I have definitely seen a lot worse code. This is actually not to bad. I don't have a lot of time right now but here are a few quick pointers:

1. Use SubVIs!
   • This is the only suggestion for most of the code, including the Begin.VI. Things would be a lot easier to clean up, and understand at a glance if there were SubVIs with descriptive icons. (I don't necessarily mean pictures, text works well too.)
   • The Teleop.VI is in dire need of subVIs.
   • The Autonomous Independent.VI also could use quite a few subVIs.
     For starters, this is not to bad to read.
     • In this case, I would make a shifter for this VI. A shifter is a typedef'd cluster. It holds all of the important data for that VI. It is replaced in OOP (Object Oriented Programming) by the the Class wire.
     • You would put everything like the various digital inputs though their own VIs. You declare that no one else writes to that specific variable in that shifter now. (This the verbal communication part of teamwork.)
     • Then for the bunch of subVIs that you create to do various things, you pull out the Boolean that it needs.
     • The same would be for the numeric controlling the main case structure. However, I would make this an typedef'd enum, and put that into the shifter, and used the same way.
     • All of this would allow you to reuse a lot of the code that you have duplicated many times.
     • I also see some potential race conditions here. It is also fairly difficult to follow the program flow.
2. Clean up the BD
   • After making your subVIs, clean up the BD. For each new subVI, I recommend using the standard 8x4x4x8 connector pane, with error clusters on the 2 bottom corners.
   • Wire the error clusters, even if your VI doesn't care about it. Typically, I won't use the case structure to disable the whole VI on an error like on many NI examples. Instead I will merge only important errors into the main error stream. Usually, most VIs don't really care about an error upstream. (VIs that command hardware, or write to something are my exemptions.)
     This allows for forcing program flow, and tracing errors.
   • Your use of comments is actually really good. The only problem is that without subVIs or any decorations to help point to what the comment refers to, they become kinda pointless. The is especially true for the teleop.VI.
3. Case structure use is okay
   -You mentioned that you have a lot of case structures, but I don't see anything that I would change in the way you used them. If you used subVIs like above, then you wouldn't even notice that you used quite a few.
   
4. ALL VIs should fit onto a single display without scrolling. The resolution of the display is the lowest common denominator of the programming group. If you have to scroll in any direction, pick only one direction, and only one. In fairly rare situations, some coding requires scrolling left and right. (Code with a lot of property nodes sometimes falls into this category. If you yourself scrolling the DB, make subVIs.

Overall I think this is pretty good code. I would probably expect this from a first year co-op student at work. I don't know if it actually works the way you want it to, but it looks like it just might. This is the most important thing.

The above pointers, would take this code into something I would expect from a more experienced programmer.