Code Review March 5th

[Shalmezad]

Did a quick once over. Everything's in branch code_review_2020_03_05. You'll want to install the TODO Tree extension by gruntfuggly if you haven't already as it'll help you find the FIXME's faster.

Here are the major items I found. I would strongly encourage you to share these with the rest of the programming team to help avoid them cropping up again.

• There are a lot of random SmartDashboard keys like "Counter" or "Position". You will want to put the name of the class (ie: ControlPanel or Turret) in the SmartDashboard key. This makes it significantly easier to find (both in the code and in the shuffleboard. If you haven't used it yet, shuffle board has a search feature for the network table to find keys) which will save you time in the future when you need to debug.
• Additionally, you should be putting as many of the SmartDashboard putNumbers in the subsystems periodic() method. Otherwise, the values won't show up on the smart dashboard unless the specific method/command is called. Again, will save you time in the future when you need to debug.
• System.out.println. Yes, there's a RIOLog that you can write to. Don't. It takes bandwidth, space on the driverstation, and makes it harder to find important messages that the CSA uses to diagnose your robot in the case of issues. Use the SmartDashboard.
• Empty methods. Since you're not using robot builder in this, you can leave out the empty methods you're not using. Helps keep the code clean/concise.
• InstantCommands. If a method has isFinished() as just return true(), it should be an InstantCommand
• Clean up. If you're no longer using something (snippet of code, method, etc), clean it up and get rid of it. Git keeps track of history, so if it was previously committed, we can get it back. Keeping clean code makes it easier to find what you need when things go wrong (note: when, not if)
• Booleans. Last I checked, the programming classes teach the boolean not operator (!), and hopefully they're also teaching that you can just do return x > 10 rather than if(x>10){return true;}else{return false;}
• Magic Numbers. Will be checking this thoroughly next. Anywhere you're using a random "magic" number (26, 0.936, etc), it should probably be a constant at the top with a good name/comment describing what the number does. This makes it easier to a) reuse, and b) change in the future (all the constants that control the subsystem are at the top).

There's still things I need to check, so leaving this issue as IN PROGRESS for now. Once I'm done, will remove it from the title and assign accordingly.

[Shalmezad]

Alright, that's probably as far as I'm going to get in the code review.

Keep in mind, that in the process of handling these that you may break the code, so would limit what you do at/during competition. The only ones I'd advise doing as soon as possible are:

• Removing the STDOUT calls (necessary for CSAs to help you).
  • Move the initializeMotors() calls. These should be handled by the subsystems in the constructor, not in a command's initialize(). The commands should assume the subsystem has gone through it's initialization (setting motors to invert/follow), and should only have to do command specific initialization (ie: change to velocity mode). This will cause you problems if you forget to call it for a command.
  • Use your sendable chooser for auton.

[Shalmezad]

Also, @AshwinS27: DO NOT ATTEMPT TO DO ALL THESE AT ONCE BY YOURSELF.

There's a lot there (136 based on a quick count). You will burn out if you do. Would strongly recommend:

1. Read all my comments above
2. Merge the branch in
3. Take care of the immediate items in my second comment.
4. Get through competition
5. After competition, review the items in my first comment with the programming team
6. Divide up the work. A lot of it's simple cleanup and can be handled by anybody. Some of the items will require a bit more thought.

[Shalmezad]

One last thing: Once done with the fixes, put an issue in my name to do another code review so I can go over the fixes. There's a good chance there are things I missed in this review that will be easier to catch once the codebase is cleaned up.