Code Review: Semi-Novice

[michaelyork]

Hello,

I will start this thread out by saying thank you to all of the people here on CD who have helped me out this past season. If it was not for you guys, we wouldn't have ever been able to have our most successful season yet.

I would like to take this year as an opportunity to further my team's recognition and learning of Java as a robot programming language. Though we have used Java for the past couple of years, I have been the only one to work on the code, which has not lead to much innovation or review. I am teaching the rest of my team (the ones who are interested, anyways) how to use WPILib and Java as a whole for the upcoming 2017 season.

I was wondering if anyone here on CD could take a look at our code and see if there is anything I may be able to improve on for next season, and if there are any bad practices I am participating in.

GitHub Repository: https://github.com/Team3680/Stronghold

This would be of great help for the future of my team as a whole, so I welcome as much criticism as can be due. Thank you in advance to anyone who may post some advice.

[mikets]

We don't do command based programming and also not knowing what each subsystem is supposed to do, I just looked through the code and found a couple of minor optimizations on boolean expressions.

    	...
    	if(button10left.get() && !button10LeftLast) { 
    		button10LeftLast = true;
    		compDisable = !compDisable;
    	}
    	...
    	Intake.isFinished()
    	{
   		return !RobotMap.intakeOutSwitch.get();
    	}

[Lireal]

The major issue I see with your code is that you are using the sybsystems and RobotMap completely wrong. Your RobotMap should only really include numbers. All of your motor controllers, sensors, etc. should be a in your subsystems. Read through these examples and you will see how to do it: https://wpilib.screenstepslive.com/s.../13809/c/88893

[michaelyork]

Originally Posted by mikets

We don't do command based programming and also not knowing what each subsystem is supposed to do, I just looked through the code and found a couple of minor optimizations on boolean expressions.

    	...
    	if(button10left.get() && !button10LeftLast) { 
    		button10LeftLast = true;
    		compDisable = !compDisable;
    	}
    	...
    	Intake.isFinished()
    	{
   		return !RobotMap.intakeOutSwitch.get();
    	}

Thank you! I'll get those fixed up ASAP.

Originally Posted by Lireal

The major issue I see with your code is that you are using the sybsystems and RobotMap completely wrong. Your RobotMap should only include really include numbers. All of your motor controllers, sensors, etc. should be a in your subsystems. Read through these examples and you will see how to do it: https://wpilib.screenstepslive.com/s.../13809/c/88893

Oh gosh, I didn't know I was doing it wrong this whole time. I'll definitely beg our team to get a new control system so I'll be able to practice with doing it the right way. Thanks!

If anyone else sees something wrong with my code, please let me know. I'm making a bit of a list of stuff I need to practice.

[jreneew2]

Originally Posted by Lireal

The major issue I see with your code is that you are using the sybsystems and RobotMap completely wrong. Your RobotMap should only include really include numbers. All of your motor controllers, sensors, etc. should be a in your subsystems. Read through these examples and you will see how to do it: https://wpilib.screenstepslive.com/s.../13809/c/88893

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.

[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.

[Ben Wolsieffer]

Originally Posted by Lireal

The major issue I see with your code is that you are using the sybsystems and RobotMap completely wrong. Your RobotMap should only really include numbers. All of your motor controllers, sensors, etc. should be a in your subsystems. Read through these examples and you will see how to do it: https://wpilib.screenstepslive.com/s.../13809/c/88893

RobotMap does not necessarily need to only have numbers. In fact, RobotBuilder puts all the motor controller definitions in RobotMap, and then includes references to them in each subsystem. I don't necessarily think this is the best way to do it, but I would definitely say that it is not "wrong" to put things other than numbers in RobotMap.

[euhlmann]

This isn't really a code thing, but it's best practice in git to exclude anything binary (your bin, build, and dist) folders. You can use a .gitignore file to do this.

General
- You've got some redundant/unnecessary classes. Instead of things like IntakeIn and IntakeOut you could have a generic IntakeRoll that accepts an enum parameter for in or out.
- When using booleans in if blocks, you don't need an "== true"
So instead of

if(pSwitch.get() == true) { ... }

You'd do

if(pSwitch.get()) { ... }

And instead of

if(RobotMap.intakeOutSwitch.get() == false) { ... }

You'd do

if(!RobotMap.intakeOutSwitch.get()) { ... }

OI.java
- Using an array of joystick buttons might be neater

Robot.java
- What is pSwitch? A better variable name would help. Also the channel is hardcoded
- You store joystick buttons here. Those should be in OI

RobotMap.java
- You name some channel numbers "driveVictorX" and then create a Talon object with them
- Then you name your lift motor channel "liftTalon" and create a Victor from it
- Intake switches are hardcoded

Subsystems
- I agree with the design choice of having subsystems store hardware objects (instead of RobotMap), but it's really up to you.

Note that these are best practices/recommendations. It's difficult to write software well, and even our own code doesn't really meet these standards (especially as we piled on more and more mid-competition hacks)