From Code Review Party 2020, by 6171

[joshuaPurushothaman]

---IN PROGRESS--- I will update this with anything I find over time.

Heyo! Just a heads-up: I'm not all too familiar with the Command-Based framework, so if anything I point out is actually how you do something in Command-Based, pretend I never said it. - Josh

• Robot2020/src/main/java/frc/robot/RobotContainer.java

  Lines 67 to 91 in f2afadc

  	JoystickButton btn1 = new JoystickButton(driverJoyStick, 1);
  	JoystickButton btn2 = new JoystickButton(driverJoyStick, 2);
  	JoystickButton btn3 = new JoystickButton(driverJoyStick, 3);
  	JoystickButton btn4 = new JoystickButton(driverJoyStick, 4);
  	JoystickButton btn5 = new JoystickButton(driverJoyStick, 5);
  	JoystickButton btn6 = new JoystickButton(driverJoyStick, 6);
  	JoystickButton btn7 = new JoystickButton(driverJoyStick, 7);
  	JoystickButton btn8 = new JoystickButton(driverJoyStick, 8);
  	JoystickButton btn9 = new JoystickButton(driverJoyStick, 9);
  	JoystickButton btn10 = new JoystickButton(driverJoyStick, 10);
  	JoystickButton btn11 = new JoystickButton(driverJoyStick, 11);
  	JoystickButton btn12 = new JoystickButton(driverJoyStick, 12);
  	JoystickButton opbtn1 = new JoystickButton(operatorJoystick, 1);
  	JoystickButton opbtn2 = new JoystickButton(operatorJoystick, 2);
  	JoystickButton opbtn3 = new JoystickButton(operatorJoystick, 3);
  	JoystickButton opbtn4 = new JoystickButton(operatorJoystick, 4);
  	JoystickButton opbtn5 = new JoystickButton(operatorJoystick, 5);
  	JoystickButton opbtn6 = new JoystickButton(operatorJoystick, 6);
  	JoystickButton opbtn7 = new JoystickButton(operatorJoystick, 7);
  	JoystickButton opbtn8 = new JoystickButton(operatorJoystick, 8);
  	JoystickButton opbtn9 = new JoystickButton(operatorJoystick, 9);
  	JoystickButton opbtn10 = new JoystickButton(operatorJoystick, 10);
  	JoystickButton opbtn11 = new JoystickButton(operatorJoystick, 11);
  	JoystickButton opbtn12 = new JoystickButton(operatorJoystick, 12);

  So here I think names for your buttons could be a little more intuitive, such as using something like "buttonA" or "LTrigger" as opposed to "btn1" and using the raw IDs. It would make sense that programmers writing a new command shouldn't need to know what the ID of a button is. This might be fixed by changing the names of the variables. On my team (and many others) we use an OI class, maybe you could consider that as well. We use the same OI every year and this year, we've moved it to our own external library.

• Perhaps your motor factory could be turned into a library. We have https://github.com/sus-tenance/COMBUSTION that we use for any functions common to every year such as joysticks and our IMotor interface (similar to your motor factory). I can't say it's all that better than copy-pasting the common files into a new project, but perhaps you could consider it.

• In your Drivetrain.java, why don't you guys use the DifferentialDrive library? I don't see a reason why not to, I even attempted to rewrite it using the library and it seems just fine.

• Robot2020/src/main/java/frc/robot/subsystems/Drivetrain.java

  Lines 50 to 53 in f2afadc

  	/**
  	* Creates a new drivetrain.
  	*/
  	private static Drivetrain INSTANCE = new Drivetrain();

  I didn't know about Singleton patterns! Will use in our code now 👍 .

• JAVADOCS!!! The javadocs seem to be in the wrong spots in some places. By "wrong spot", I mean that VSCode won't show you the javadoc's content when hovering over it. Found this a few times in Drivetrain.java, and I imagine it's also in other places. Also, sometimes a normal //comment is used in place of a javadoc. However, they are properly used in other spots as well, so I'm sure you know what they do, how they work, and why one should use them.

[bryanyli]

For your first point, we used the raw button IDs because they're labeled as such on the Logitech Extreme 3D Pro joystick that we use.

We didn't use WPILib's DifferentialDrive as it didn't offer the flexibility that we wanted. Originally I was planning to test out WPILib's RAMSETE controller for autonomous which required a voltage tank drive method. This wasn't possible with WPILib's DifferentialDrive, except by jumping through hoops.