ChiefDelphi Code Review Party

[andrewazores]

Here is the main issue for suggestions and comments from Team 3161 for the ChiefDelphi code review party.

• RobotState could probably be split up into smaller classes, ex. individual subsystem states. ie RobotState, ClimberState, FlywheelSubsystem, etc. Or, if possible with the Oblog annotations, expose the various states directly from the subsystem implementations?

• Robot#teleopPeriodic, Robot#autonomousPeriodic simply decrements timeRemaining by 0.02 upon each invocation. This is theoretically correct because the main robot loop "should" run at 50Hz, but in practice there can be some variance depending on external factors like CPU load or IO, so the timeRemaning value has some degree of uncertainty compared to the "real" match time remaining. WPILib Timer can probably replace this (ie Timer.getMatchTime())

• Robot#robotPeriodic looks like it's logging every other frame, but it's not really clear why this strategy was chosen. Also the static member log is declared in the middle of the file, rather than more conventionally at the top

• Robot#autonomousInit has some magic constant values for various commands, like adjusting the hood to "300" or setting the flywheel to -0.9. These should be extracted to a class constant (private static final double) for easier maintainability and reduced odds of missing changes in various places when refactoring

• RobotMap has some cool static members like CLIMB_CONNECTED, which appear to be used to conditionally (at compile time) disable robot subsystems which may be physically disconnected and not present. This is a neat feature, but the naming could probably be clearer, or at least some comment explaining how/why subsystems can be "disconnected" and various components such as solenoids intentionally initialized to null. It also isn't entirely clear to us why RobotMap contains these static members, which seem to really be used as instance state for the RobotContainer. Overall RobotMap is quite long and contains a lot of global, static "state" things.

• Following on from above - there are a lot of nullable fields for various components. Why do these all need to be nullable? What happens if ex. a sensor is not physically plugged in and the DigitalInput is instantiated anyway?

• RobotState and RobotMap both feel like large, do-everything, global state holders, but the separation of concerns between them doesn't feel too clear

• IndexIntakeSelector is an interesting pattern. The fields that it assigns belong to its parent class, RobotMap, and its constructor simply sets a series of fields by calling a utility method owned by the Selector. Why is this preferable to simply setting the indexSelector field of RobotMap by directly calling a utility method, rather than wrapping that utility method within a class and a constructor and a heap allocation?

• OI has nice enums for things like left and right Joysticks, but it seems like it misses the opportunity to also have an enum for the axes of these Joysticks

• More of a question - why does DriveBaseSubsystem set the motors to follower mode as well as put them into a SpeedControllerGroup?

• DriveBaseSubsystem#getWheelSpeeds has a magic number of 10 used somehow for scaling the sensor velocity. This should be extracted to a meaningfully-named constant

• DriveBaseSubsystem#getMoveThreeMetersForwardFromStartCommand seems redundant when you also have getMoveCertainAmountCommand

• Similarly, HoodSubsystem#servoExtend/servoWithdraw could probably just call setServo instead of copying the implementation

• DriveBaseSubsystem#getMoveToPowerCellPathFromPathWeaverCommand gets the deploy directory, then resolves an absolute path from it. But absolute path resolution doesn't depend on what the base path was. Also, the absolute path resolved includes a drive letter and a specific username. This should probably be a path relative to the deploy directory. Also, if the file at the path does not exist or is otherwise unreadable, an IOException is caught but the trajectory object is left null and passed to the RamseteCommand. How does the RamseteCommand handle a null trajectory? Should the exception handler return a different command, or some "empty" Command implementation?

• Why are there both IntakeOnOff and IntakeUpDown subsystems if they both deal with the intake? Could there be one "larger" subsystem that handles all responsibilities of the intake, similar to your indexer subsystem superstructure?

• ControlPanelColorSubsystem#colorToString seems like it could just be a method on each of the color enum members, rather than a method to perform this mapping of enum member to String

• ControlPanelColorSubsystem#rotateControlPanel has a magic number of 7 for the rotationCount, which should probably be extracted to a field/constant. Also the 0.25 and 0.5 wheel speeds could be constants

• Unit tests: there is a common pattern that looks like:

FooCommand command = new FooCommand(someSubsystem);
MockButton button = new MockButton();
button.whenPressed(button);
button.push();
CommandScheduler.getInstance().run();
button.release();
verify(someSubsystem).somethingHappened();

What is the reason for creating the MockButton and running through the command scheduler here? This seems like a very heavyweight approach. It looks like this would be equivalent to the following:

FooCommand command = new FooCommand(someSubsystem);
command.initialize();
command.execute();
verify(someSubsystem).somethingHappened();

But this way requires far less test setup since there is no command scheduler involved.