View Single Post
  #2   Spotlight this post!  
Unread 22-05-2016, 15:15
Pault's Avatar
Pault Pault is offline
Registered User
FRC #0246 (Overclocked)
Team Role: College Student
 
Join Date: Jan 2013
Rookie Year: 2012
Location: Boston
Posts: 618
Pault has a reputation beyond reputePault has a reputation beyond reputePault has a reputation beyond reputePault has a reputation beyond reputePault has a reputation beyond reputePault has a reputation beyond reputePault has a reputation beyond reputePault has a reputation beyond reputePault has a reputation beyond reputePault has a reputation beyond reputePault has a reputation beyond repute
Re: Code Review: Semi-Novice

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.
Reply With Quote