Code Review: Semi-Novice

[Lireal]

Originally Posted by jreneew2

I create MotorControllers and Sensor in RobotMap all the time, in fact, I try to create a reference to each physical device (besides the roborio) in RobotMap. This way you have one common point where you can set up the controllers and change constants if need be. Then, in the subsystems is where I have a pointer to the RobotMap version. This makes it so that each member in a subsystem can be directly accessed while having all of the ugly stuff in robotmap.

This is the way RobotBuilder does it, so when I was learning I always just used that format. Your way is not wrong, but it would be inconvenient for me personally looking through multiple files and trying to find things.

I see. We just use the RobotMap to store all of the channel numbers of the various motor controllers and sensors, and point to them in the subsystems. Either way though, if you look at the OP's code, the subsystems should not be blank.

[Pault]

I have never seen a thread like this before. I think that it is a really cool idea that you are seeking out a code review from other people in the community.

You really need to work a lot on figuring out where things belong inside of your code. There is a lot of extraneous code in Robot.java, and I'm not always sure where to look to find a specific piece of code. In general, anything that has to do with joysticks should be managed in OI. ALL of your motors, sensors, actuators, compressor, etc., should be defined in RobotMap (or, if you want to use Lireal's method, the port numbers in RobotMap and the actual objects created in their respective subsystems). Also in your subsystems should be a bunch of methods that do specific things for a subsystem (i.e. drive, open clamp, raise lift, etc). Finally, Commands, should simply call those methods which are inside of the subsystems, and never interact with the motors/actuators directly. You should have very little code inside of Robot, besides the code that comes in there by default and instantiating the subsystems. Most of the other code that you added could have instead been run from commands. This tutorial describes exactly how to structure all of these things, even though the actual code is a little bit outdated.

When you do create a variable inside of Robot or RobotMap, it should always have the keyword "static" after "public", like you have when you instantiate the subsystems in Robot. The reasons for this are a bit complicated.

I noticed that in ClampOpen and ClampClose, you call Timer.delay(.5) in their isFininished() methods. You should NEVER use Timer.delay() unless you are multithreading (which you never do in this code). Use setTimeout() and isTimedOut() instead. Timer.delay will stop any other code on your robot from running for, in this case, .5 seconds. Which is obviously bad, because you lose control of everything.

Finally, noticed that you don't have any comments in your code. Especially when you are collaborating on code instead of doing it all yourself, good commenting is essential. I highly recommend using the standard format for documentation in Java: Javadocs. The standardized format basically forces you to include comprehensive comments, as long as you commit to putting it on every class and method that you write.