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

Update GamepadRunIntake #6

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

ArvindVivek
Copy link

@ArvindVivek ArvindVivek commented Jan 3, 2019

Made requested changes to setting intake to 0 if neither bumpers are pressed.

This GamepadRunIntake command operates the intake mechanism by checking the status of the right bumper and left bumper of the gamepad and intakes and outakes a cube respectively.
@Override
protected void execute() {
if(Robot.oi.gamePad.getRightBumper()) {
Robot.intake.run(0.4);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: For this number and the one below (0.4 and -0.3), please extract them into a constant (in the current class or in the Constants class), to avoid magic numbers. You can name them something like INTAKE_FORWARD_POWER or something more relevant.

// Called once after isFinished returns true
@Override
protected void end() {
Robot.intake.stop();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, the comment above this method says that this will be called only when isFinished returns true, which it never does (line 41). This line is never reached. Is setting the intake power to 0 sufficient for stopping the intake?

Copy link

@jyue86 jyue86 Jan 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that isFinished() only returns false because it's suppose to start in teleopinit() and runs for the entirety of the teleop period (basically the rest of the match). In this case, end() is not really needed unless end() is called when the match ends. For setting the intake power to 0, I would hope that it would work to stop the intake.

@KTOmega
Copy link
Member

KTOmega commented Jan 3, 2019

I've left a few comments on the file you're adding. Please, review.

A top-level nit I have is code formatting. Please consult your team lead for your team's rules on formatting, but I've noticed that you have inconsistent spacing (sometimes 4-space indents, sometimes 2-space indents) and no space after if (if(condition) instead of if (condition)). While this does not break functionality, often times small things like these can impact code readability. This is based on my personal preferences for readability, so, again, please talk to your lead about this :)

@KTOmega
Copy link
Member

KTOmega commented Jan 3, 2019

If you want to resolve these comments, you can make the changes requested on the files, commit them, and push them to your PR's branch (arvind/gamepad-run-intake). GitHub will then see the commits you pushed and update your PR accordingly, incorporating your new changes. No need to submit a new PR.

Arvind and reviewers, please let me know if you have any questions about Git or the pull request review process. This is what I do at work, after all :)

Copy link
Member

@KTOmega KTOmega left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments - please review.

Added changes regarding constants and formatting.
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

Successfully merging this pull request may close these issues.

3 participants