team3309 / 6cimdrive Goto Github PK

There are a lot of well-established best practices for code in Java that you should be following.
Google has published a really good style guide that you should familiarize yourself with and follow. A lot of people, especially relatively new programmers don't understand the importance of good style but it is very important for other people reading your code and your future self.

A few that I've noticed you're not following:

• Controller should not be capitalized here
• modifiedGyro should be ModifiedGyro

It is generally best practice to declare all member variables at the top of the class. You have some declarations interspersed everywhere in Drive mixed in among the methods, that way it's impossible to look in one place and see all of the variables contained in the class

Sometimes your indentation and other spacing is messed up and it bothers me. I always run auto-format in my IDE before saving a file. It might not seem like a big deal but the code is much more aesthetically pleasing and easier to read if the formatting is consistent everywhere

You should have everything in a package. (org.team3309.frc2015 is a good name for the season's code). When you have everything in the default package, other classes cannot import them, it is always best practice to never use the default package and always specify a package name. See 2014 for a sample package organization

Class names should always start with an upper case character and then camel-case after that. This would then be ModifiedGyro

You should also add a Javadoc comment on the declaration of the class to say what it does. Specifically, what does it do that a regular Gyro doesn't? I know, but other people reading it might not

Might want to update with proper class/package

Instead of having 6 separate variables to hold the 3 Victors for each side, you can declare two arrays of 3 Victors each, and loop through the array when you set a value for them

You should have comments on parts of code that people are likely to have difficulty understanding. Comments are also useful for yourself in the future, not just other people.
Comments like the following are useless:

• all of the sensors and motor controllers
  • reader can tell in a second that this is where all of the variables are declared
• all the booleans for which drive is being used
  • again reader can tell instantly - this comment doesn't add any value
• the constructor
  • this is obviously the constructor
• initialize victors
• initialize Encoders
• initialize gyro
• change this to change threshold
  • I don't need the comment to know that if I change this I'm changing "the threshold", but what is it a threshold for?
• tells if gyro is a yay or nay
  • unnecessary, this is what booleans always do...
• the max angular velocity (duh)
  • I don't think I need to tell you why this comment is unnecessary (it says itself lol)
• max voltage for drive
  • what happens if it exceeds that? how is this used?
  • I had to read around 40 lines down to finally see that it's supposed to "stop drive if drive motors voltage exceeds a value"
• there is no comment at all in this section of the code, which is probably one of the more confusing bits
• negative because Vinnie did it
  • If I'm someone else reading this code, this doesn't help me figure out why it's inverted
  • It also indicates that you yourself don't know
  • The reason is because the motors on the left side have to spin in the opposite directions as the ones on the right in order for the wheels to go in the same direction. The polarity switch can be done in wiring as well, but some inspectors don't like that and it's generally bad practice to switch ground/power wires.
• This method takes when the equation returns above the max or below the min and fixes it so it does not. Uses some math and gets the job done
  • Again this indicates that you don't know what the code is doing. Maybe you do and you're just not explaining it well...
  • What this really does is "skim" a little of the power off the top of the left or right signal if they are above 1, so that if the driver is going really fast (with wheel speeds calculated above 1) they can still turn
• turns the solenoid on and off, dont call this method, use setgears
  • I'll revisit this comment in another issue, but asking people nicely not to call your method is a terrible idea 😛

I felt bad for giving so many bad examples so here are some examples of good comments (from your code):

• speed barrier for encoder, when encoder.getRate() exceeds this value, high gear automatically happens
• joystick is driven like a halo warthog, left joystick goes forward and backward, right joystick goes left and right
  • this should really be a javadoc comment, but the text it contains is good
• if ther joystick is not pressed enough, immeaditely stop, don't even do the math
  • other than the spelling mistakes, this is a good and descriptive comment
• this
  • again, should be a javadoc comment, but at least its descriptive

I've read a lot about teams like 33 experiment with auto-shifting like it seems as if you are doing here and the general consensus is that it's a bad thing to do. My opinion (and maybe Michael has a different one) is that the driver should have full control over when they shift gears. There are countless different scenarios that could happen during a match, and you can't possibly think of all of them beforehand. Auto-shifting could have undesired consequences if for example you can't try and ram someone in low gear because it shifted to high automatically and now you brown-out while trying to push another robot.

Speaking from experience, it is really easy to forget to update all necessary variables when some are linked like this. Instead of having haloDriveEnabled and tankDriveEnabled, you should use an int for driveMode and private static final ints to represent something like MODE_HALO_DRIVE = 0 MODE_TANK_DRIVE = 1, etc and check if driveMode == one of those (this way you could also have more than 2 states)

As a general rule of thumb, everything other than static variables should always be declared as private. If you need to access or set the value of a class's member variable, you should have getter and setter methods to access/set the value respectively.

Here are some spots where I noticed you either didn't declare an access modifier or used it incorrectly:

• the drive mode booleans
  • should be private and have getters/setters if necessary
• the constructor of Drive
  • The constructor of Drive should be declared as private, not public
  • This might not make immediate sense but the reason for making it private is to enforce that no one else can create a Drive instance except code in the Drive class. Since a robot can only have one drivetrain, you should employ the Singleton pattern.
  • For an example of a Singleton, see this part of the Drive code from 2014
• gyroEnabled
  • should be private and have getter/setter if necessary
• driveHalo and driveTank
  • Since these methods are public, anyone can call them to control the drivetrain, completely bypassing the current mode selection. This is allowing other code to potentially circumvent your selected driving mode, which could cause hard to debug issues.
  • They should both be private and only called from the public drive() method that will then call the appropriate choice based on the current driving mode.
• setDriveshifter
  • this should obviously be private for the same reason as directly above
  • you even say not to call it in the comment right above the method declaration
    • you shouldn't just "ask nicely" to not call it, you should not allow anyone to call it at all by declaring it private
• straightPidEnabled
  • should be private and have getter/setter if necessary

There's no reason to have anything with a Scheduler in there if you don't have any Commands. See my 2014 code or the WPILib docs for more info. If you are going to use the Command based architecture, there should be a Command subclass for driving that you just start in teleopInit and stop in disabledInit again see 2014 code for more info

This should not be its own class, but instead make all of the fields in here (public static) members of XboxController. You really only need to use it in XboxController so it would make the code cleaner, but you still want them to be accessible if you want to use the JoystickButton class provided with the command-based framework from FRC