View Single Post
  #3   Spotlight this post!  
Unread 29-06-2016, 19:28
euhlmann's Avatar
euhlmann euhlmann is offline
CTO, Programmer
AKA: Erik Uhlmann
FRC #2877 (LigerBots)
Team Role: Leadership
 
Join Date: Dec 2015
Rookie Year: 2015
Location: United States
Posts: 312
euhlmann has much to be proud ofeuhlmann has much to be proud ofeuhlmann has much to be proud ofeuhlmann has much to be proud ofeuhlmann has much to be proud ofeuhlmann has much to be proud ofeuhlmann has much to be proud ofeuhlmann has much to be proud of
Re: Rewrote Robot Code: Opinions/Thoughts?

A lot of these recommendations are based on the assumption that people will want to read your code (that includes you, two weeks or so after you finish this project and stop looking at the code).

Code:
	private String[] autoArray = { autoChoice1, autoChoice2, autoChoice3, autoChoice4, autoChoice5, autoChoice6 };
	private static final String tankDrive = "Tank Drive";
	private static final String arcadeDrive = "Arcade Drive";
	private static final String autoChoice1 = "Rough Terrain";
	private static final String autoChoice2 = "Portcullis";
	private static final String autoChoice3 = "Chival de Frise";
	private static final String autoChoice4 = "Low Bar (shooting)";
	private static final String autoChoice5 = "Low Bar (without shooting)";
	private static final String autoChoice6 = "Moat";
Don't define choices twice. It just makes everything confusing. Try this instead:

Code:
private String[] autonomousChoices = {
	"Tank Drive",
	"Arcade Drive",
	"Rough Terrain",
	"Portcullis",
	"Chival de Frise",
	"Low Bar (shooting)",
	"Low Bar (without shooting)",
	"Moat"
};
The same thing applies for drive motors, and also cameras/doublesolenoids if you wanted to make those arrays (I think you should). Just make sure you define your arrays as "VictorSP[] driveMotors" for example, and not "Object[] driveMotors". That makes it a bit easier to read.

There's no need to publish arm speeds in the Robot constructor - you won't know them at that point, plus the robot is disabled. Instead, publish them in the teleop loop.

Instead of using magic numbers like "ds1 = new DoubleSolenoid(0, 1)", use a RobotMap file to store the numbers. That makes it easier to read. Right now it's not easy to figure out what 0 and 1 mean in the context. Instead, you can have nice code like "sampleTextSolenoid = new DoubleSolenoid(RobotMap.SAMPLE_TEXT_HARDWARE_DOWN, RobotMap.SAMPLE_TEXT_HARDWARE_UP)" where it's clear what the numbers mean.

Change some functions a bit. "closeOpenCam" might be better as "reinitializeCameraStream(Camera setActive, Camera setInactive)"
"rumbleABit" can be refactored as "rumbleFor(double seconds)"

Finally:
This isn't code golf!! As @ahartnet said, separate the logic for drive motors and auto modes. When writing code like this, always go for readability and not shortest length possible.
__________________
Creator of SmartDashboard.js, an extensible nodejs/webkit replacement for SmartDashboard


https://ligerbots.org
Reply With Quote