Is my code good?

[olidem123]

Here's a link for my team's 2016 java program: https://github.com/olidem123/PLS5618

I was just wondering if there was ways to improve/make it lighter (other than clarity :cool: ).

PS: We are a french canadian team so most of the program is in english. Ask me if you have questions on words.

thx

[MrRoboSteve]

There is lots to like about your code. It's well organized and generally easy to read. A couple specific comments:

1. We like to use constants in RobotMap.java, organized by kind of interface. We can then print this out and use it as instructions for wiring.

For example:


/**
* PWM */
public static final int CHASSIS_JAGUAR_FRONT_LEFT = 2;
public static final int CHASSIS_JAGUAR_BACK_LEFT = 3;
public static final int CHASSIS_JAGUAR_FRONT_RIGHT = 0;
public static final int CHASSIS_JAGUAR_BACK_RIGHT = 1;
public static final int BALL_SHOOTER_LEFT_SPEED_CONTROLLER = 4;
public static final int BALL_SHOOTER_RIGHT_SPEED_CONTROLLER = 5;
public static final int ARM_EXTEND_SPEED_CONTROLLER = 6;
public static final int CAMERA_TILT_MOTOR = 7;


I'd then recommend moving the declarations out of RobotMap and into the subsystem that owns the device/controller. We generally make each controller/encoder/gyro be owned by exactly one subsystem, and hide the reference inside the subsystem (make them private). This way we know that there's only one place to look for code that changes those entities.

You're almost there with your final references in the subsystems, but they are copies of reference and the actual controller/encoder/gyro is available to everyone.

2. In AvancerHerse, consider using more descriptive variable names:


int h1 = 950;
int h2 = 450;


and think about simplifying "!(Robot.bras.distPot() < h1)" a bit:


if (!Robot.pelle.herseAcotee()) {
// h1, h2 = hauteur pelle
if ((Robot.bras.distPot() > h1)) {
Robot.chassis.reculer(-0.4, 0);
} else if ((Robot.bras.distPot() > h2) && !(Robot.bras.distPot() < h1)) {
Robot.chassis.reculer(-0.6, 0);
} else {
Robot.chassis.reculer(-0.9, 0);
}
}


3. In BrasCommand, you say:


Timer.delay(0.005);


Wondering why this is necessary here and in the other places in the code. Delay like this is needed only in very specialized cases.

4. In BrasCommand, you didn't put this in braces:


} else
Robot.bras.controlBras(0);


For student code, I recommend that every block be enclose in braces as it's consistent and simple to think about.

5. Generally you want the same code in end() and interrupted() -- see Reculer.

[GreyingJay]

For student code, I recommend that every block be enclose in braces as it's consistent and simple to think about.


Not just student code - many company coding standards require this as well, including Java code written for/at Google (https://google.github.io/styleguide/javaguide.html#s4.1.1-braces-always-used).

My team intends to use Google's style guide for all our code in the future. Consistently applying rules and formatting will help with integrating and understanding code written by multiple people, and it helps students get used to the idea that they will eventually have to work in an environment where such rules are imposed.

olidem123, it might be a good resource for you too :)

[euhlmann]

Just some git/project related things:

I recommend you name your project something that includes the year and/or game. If you're going to be making another robot program next year, having separate names will allow you to store both projects on GitHub. For example, naming it PLS5618-Stronghold-2016 will allow you to make another project next year: PLS5618-WaterGame-2017 :)

Make sure you allow your .gitignore to take effect. On your local copy of the project, commit anything if necessary, then run

git rm -r --cached .
git add .
git commit -a -m "Untrack gitignored files"


---

Instead of having the command pair InBallon/OutBallon, which are very similar, I think it makes more sense to have a single command handle both, eg

public class MoveBall extends Command {
public enum MoveBallDirection {
IN,
OUT
}

MoveBallDirection whichDirection;
public MoveBall (MoveBallDirection whichDirection) {
requires(Robot.pelle);
this.whichDirection = whichDirection;
}

protected void initialize() {

}

protected void execute() {
if (whichDirection == MoveBallDirection.OUT) {
Robot.pelle.outBallon();
} else {
Robot.pelle.inBallon();
}
Timer.delay(0.08);
}

protected boolean isFinished() {
if (whichDirection == MoveBallDirection.OUT) {
return !Robot.oi.stick.getRawButton(4);
} else {
return Robot.pelle.limitFond();
}
}
}

Similarly, you could even refactor Pelle.in/outBallon into a single function.

[olidem123]

I just edited the program. https://github.com/olidem123/PLS5618-2016

1. We like to use constants in RobotMap.java, organized by kind of interface. We can then print this out and use it as instructions for wiring.
I changed those things a bit and it looks a lot better, still need to test it. Thx

2. In AvancerHerse, consider using more descriptive variable names:
I just removed this part of the program because we found out it was useless.

4. In BrasCommand, you didn't put this in braces:
} else
Robot.bras.controlBras(0);
Guess I forgot them ;)

Instead of having the command pair InBallon/OutBallon, which are very similar, I think it makes more sense to have a single command handle both, eg
I use 2 commands because I found it easier to set up 2 buttons to one command each.

Not just student code - many company coding standards require this as well, including Java code written for/at Google.
I'll take a good look at that ;)

PS: I sent this post to our mentors for tips for the future years. Thx for all your support!

[euhlmann]

I use 2 commands because I found it easier to set up 2 buttons to one command each.


Well you'd have a constructor parameter that specifies which direction to roll the ball.

public enum Direction {
IN,
OUT
}
// ...
public class BallCommand extends Command {
Direction whichDirection;
public MyCommand(Direction whichDirection){
this.whichDirection = whichDirection;
}
// ...
}
// ...
// then in OI
Button buttonA = new JoystickButton(stick, 1),
buttonY = new JoystickButton(stick, 4);
buttonA.whenPressed(new BallCommand(Direction.IN));
buttonY.whenPressed(new BallCommand(Direction.OUT));