Full codebase for review #53
Conversation
| Flippable.init(); | ||
| LEDConstants.init(); | ||
| AutonomousConstants.init(); |
There was a problem hiding this comment.
Why not initialize in static blocks?
There was a problem hiding this comment.
These classes aren't necessarily called before the values they configure are used. This could also be an empty init function just to make sure the class is called when the code initializes before most other things, with a static block. (Though that's useless, might as well use the init function)
There was a problem hiding this comment.
These classes aren't necessarily called before the values they configure are used.
Whenever any Flippable is accesed, it is guarenteed that any static block there ran.
Reagarding LED and Autonomous, it seems that there is a try to avoid a singleton and have everything static, when in reality a singleton is much more fitting. (Maybe even a proper instance passed around as a parameter).
Nontheless, static block still applies for this antipattern
|
|
||
| public class Robot extends LoggedRobot { | ||
| public static final boolean IS_REAL = Robot.isReal(); | ||
| private final CommandScheduler commandScheduler = CommandScheduler.getInstance(); |
There was a problem hiding this comment.
shorthand? then make it static ig
| import org.littletonrobotics.junction.wpilog.WPILOGWriter; | ||
|
|
||
| public class Robot extends LoggedRobot { | ||
| public static final boolean IS_REAL = Robot.isReal(); |
There was a problem hiding this comment.
Robot is redundant - if you want to specify the class, then do any of LoggedRobot, IterativeRobotBase, or RobotBase.
| import org.littletonrobotics.junction.wpilog.WPILOGWriter; | ||
|
|
||
| public class Robot extends LoggedRobot { | ||
| public static final boolean IS_REAL = Robot.isReal(); |
There was a problem hiding this comment.
Why does this exist, if it is set to the value from a public method?
|
|
||
| public RobotContainer() { | ||
| initializeGeneralSystems(); | ||
| buildAutoChooser(); |
There was a problem hiding this comment.
better to make autoChooser final, and have buildAutoChooser() return a chooser
| public static final boolean IS_REAL = Robot.isReal(); | ||
| private final CommandScheduler commandScheduler = CommandScheduler.getInstance(); | ||
| private Command autonomousCommand; | ||
| private final RobotContainer robotContainer; |
There was a problem hiding this comment.
Final should be above non-final field
levyishai
left a comment
levyishai
left a comment
There was a problem hiding this comment.
@Strflightmight09 please create a quick PR to address these swerve comments
src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.java
Show resolved
Hide resolved
| new PIDConstants(4, 0, 0) : | ||
| new PIDConstants(10, 0, 0.1); | ||
| private static final double | ||
| MAXIMUM_ROTATION_VELOCITY = RobotHardwareStats.isSimulation() ? 720 : Units.radiansToDegrees(MAXIMUM_ROTATIONAL_SPEED_RADIANS_PER_SECOND), |
There was a problem hiding this comment.
What about it? Please answer on the new PR
No description provided.