Skip to content

For Team 2 programmers #2

Description

@ProfessorAtomicManiac

Several methods such as TeleopDrive.switchMode() and Intake.onOff() deserve to be in subsystems, not in commands. Remember, subsystems provide the implementation which commands can use for higher level actions, commands should not be implementing stuff for the subsystem.
For example the onOff() method should be in your IntakeFeederTreamill.java subsystem class (intakefeeder is a good enough name for your subsystem, don't make it so long). While onOff() can be considered as a command, its better to have it in a subsystem class where everything is encapsulated and organized together into one neat bundle. For example, your regurgitate method is perfect, but then you have some random static method in a command class that does not seem to be connected to anything. Its disorganized!

Also why static? You can use (insert class instance)::onOff as a lambda which works for instance methods. Don't randomly make methods static to make it possible to call, that method does not need to be shared across different command instances (another reason why you should put into intakeFeeder)

I also really don't like how you have essentially what is a global variable to control what drive mode your robot is in. That variable only needs to be in the dt subsystem, no other parts of the code need to know what drivetrain type is being used. Also global variables VERY BAD PRACTICE, since you can edit the variable anywhere in the code and its hard to track down where its edited, while if its in a single subsystem class its much easier to track down. A much better implementation would be a drive method that checks what mode the dt is in and drives accordingly. That way when you are coding a command, you don't even need to worry about the logic involved with checking what mode the drivetrain is in. Remember, the idea of subsystems is to encapsulate code so that when you making commands you don't need to worry about the logic as much.
Constants.java should only contain variables that are needed across the project, such as port numbers. Values such as the speed of your intake motor should go in your intakefeeder subsystem class as constants, not in Constants.java. This is again because you are providing more information than necessary, and it makes it hard to find where the variable may be defined or associated with.

On line 74 in robotcontainer.java, don't just use values like "55". These are "magic numbers" in which when other people look at your code they have no idea what the "55" means. Make a constant variable for that. Since the variable only concerns the distance traveled, that only needs to be in drivetrain subsystem, no need for other areas of code to know about that value.

For your autonomous.java command, you should ultilize the isFinished and end methods, which do the same thing as your current if statement. With your current if statement, the command never ends, which means that the command will run infinitely. Since your if statement tells your robot to stop after going a certain distance, your robot will stop forever. The same can be said for PickUpPlant.java and regurgitate.java.
For PickUpPlant.java, currently your command does not end but say you fix it and it ends. After the driver presses the button, it will slow down for 0.02 seconds then go back to normal speed. Do you really want to use "whenPressed()"?
For regurgitate.java, instead of creating an entire command class do new InstantCommand(intake::regurgitate).
Also for several command classes you forgot addRequirements();
For your intake.java command, setting values to 1 or 0 is confusing and is another instance of "magic numbers", either put comments or use enums. You can declare an private enum which will swap the 1 for "ON" and 0 for "OFF" or smth like that. Again DON'T USE GLOBAL VARIABLES for toggling your intake.

tl;dr General bad practice you guys are employing is that you give access to certain data that does not need to be accessed. Some code is also kinda thrown all over the place instead of one central subsystem class. If you guys just encapsulated all your code into organized subsystems with all constants and variables related to that subsystem ONLY in that subsystem, you would fix like 70% of the problems I have just mentioned. Also be sure to ultilize isFinished() and end() methods in a command correctly, which should fix another 25% of ur code.

I know I sound really harsh by giving what seems to be a lot of criticism, but you guys did a pretty good job as your first attempt on coding a robot. Keep it up! It just takes a lot of experience to notice and code effectively.

If you guys need any help be sure to always ping either me or Ryan in mattermost. I kinda just typed all of this up in the whim, so some parts may be a bit inaccurate or disorganized, so if you have any specific questions definitely ping me.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions