Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ChopShop Code Review #9

Open
bot190 opened this issue May 3, 2020 · 0 comments
Open

ChopShop Code Review #9

bot190 opened this issue May 3, 2020 · 0 comments

Comments

@bot190
Copy link

bot190 commented May 3, 2020

Github Usage

It looks like you’re storing your code under a github user instead of an organization. Though there does appear to be a Github Organization for your team: https://github.com/frc-6901. If possible I would suggest gaining access/ownership of this organization and storing your code there instead of in a github user. You can read about organizations here:
https://help.github.com/en/github/setting-up-and-managing-organizations-and-teams/about-organizations

I’m glad to see that your commit descriptions are unique and fairly descriptive.
I would suggest providing a bit more detail in the rest of the commit comment, but this tends to be more of a wishlist item than something that actually happens

Code Organization

Using the command based programming model lends itself to reasonably well organized code so that’s a good start! I would recommend using some of the new convenience functions to simplify your commands. You can read about them at: https://docs.wpilib.org/en/stable/docs/software/commandbased/convenience-features.html

The SuperStructure subsystem looks like it implements part of a state machine. You can have each “state” transition to a common state that just calls runFeeder, which is common to all states, to reduce duplication. Another way to look at the same structure, is that those states can be built with command groups.

Accessing values in network tables can result in long lines that are mostly redundant. In your VisionController class, you could create a variable to store a reference to the “limelight” table, and use that throughout the class.
NetworkTable table = NetworkTableInstance.getDefault().getTable(‘limelight”);
Readability
More comments! It’s worth taking some time to replace the placeholder comments with a more detailed description of what each command/function does.

Consistent indentation. There are several places where the indentation is inconsistent (2 vs 4 spaces). You can turn on auto formatting on save in Visual Studio Code as a project setting, to get consistent formatting.

If a function doesn’t need to be implemented (for example, if the function body is empty in the base and derived classes) then you can omit it entirely. This occurs in several of the command classes. isFinished also defaults to return false, so those are unnecessary.

For situations like a direction, having a boolean for “in” vs “out” isn’t descriptive. Try making an enum Direction for in/out, and passing that in.

Some names could be more descriptive - the “upstring” seems to be able to be run in reverse, which could make it an “up and down string”

The Pigeon class appears to be entirely unused, and doesn’t seem like it’s supposed to be a subsystem, but it’s in the subsystem folder.

It’s good practice when naming boolean variables for them to be a true or false question. For example, it’s unclear what “topMotor” and “bottomMotor” variables mean when first looking at it. “topMotorAtSpeed” would make the intention much more obvious.

There are lots of files, with lots of unused imports. Making sure to clean those up from time to time can help to declutter your code.

Functionality

Your “AutonLimelightShoot” command appears to use a timer to control the shooting state. Timing based controls tend to have poor performance compared to sensor based controls. If mechanically possible using some form of sensor to detect when a ball has entered the shooter can result in a more reliable mechanism.

I really like how you broke the code into different parts, making it easier to read and work on. One thing I would suggest would be to add some type of averaging of the drive inputs for the controller, so that the robot would be less jerky and a lot more smooth. After some testing earlier in our season we saw that this was very beneficial to our drivers. This can be easily implemented by taking a rolling average of the drive inputs.

When trying to calculate the RPM of the shooter, it may be easier and more accurate to calculate the equation outside of the code and then implement it directly. To clarify, we used excel to plot all of the points in a table similar to what you have in the array in 217 and 218 of the polynomial regression.java file. Then you can create a graph with all the points and find a line of best fit using power regression rather than polynomial regression. During testing we found the power regression to be more specific to the data and more accurate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant