How is this looking?

Technical Java
1

I’d love to hear some feedback from people who actually know about this stuff (My team is first year and nobody is really above me in terms of java knowledge - or frc strategies in code)

/*
 
THIS CODE IS MADE BY THE BEST PEOPLE IN THE WORLD, DO NOT STEAL.........please >_<.

*/
package edu.wpi.first.wpilibj.templates;

import edu.wpi.first.wpilibj.*;
 
public class Robot4334 extends SimpleRobot {  //DECLARE ALL METHODS IN INIT AND SIMPLEROBOT
                                                
    public RobotDrive drivetrain;
    public Joystick XBoxController;
    public Gyro gyro;
    public Timer timer;
    public Encoder leftEncoder;
    public Encoder rightEncoder;
    public static final double DRIVE_STRAIGHT = 0;
    public static final double DRIVE_FULL_SPEED = 1;
    public static final int LeftX = 1;
    public static final int LeftY = 2;
    public static final int Triggers = 3; //(Each trigger = 0 to 1, axis value = right - left)
    public static final int RightX = 4;
    public static final int RightY = 5;
    public static final int DPad = 6;
    public boolean brakeOn ;
    public float leftSpeed;
    public float rightSpeed;
    
    public void log(String theMessage) { // Logger -System.out.println function
        System.out.println(theMessage);
    }
    
    public void BRAKE() { //Brake method - stops motors
        drivetrain.drive(0.0,0.0);
        drivetrain.stopMotor();
    }
    
    public void DRIVE() {
        while(getBrake() == false && getEmergency() == false)
        {
            returnTurn();
            drivetrain.tankDrive(leftSpeed , rightSpeed);
        }
  
        if (getActualSpeed('L') > (leftSpeed + 0.15) || getActualSpeed('L') < (leftSpeed - 0.15)) {
            if(getActualSpeed('R') > (rightSpeed + 0.15) || getActualSpeed('R') < (rightSpeed - 0.15)) {
                log("Speed and encoder do not match");
            }
        }
    }
    
    public void EMERGENCY() { //ADD ALL FUNCTIONS HERE TO STOP ON ROBOT DURING EMERGENCY
        
        drivetrain.drive(0.0,0.0);
        timerControl("Start");
        while(getTimer()<10)
        {
            int time = (int) getTimer();
            log("EMERGENCY   :::   "+time+" SECONDS UNTIL RESTART");
        }
    }
    
    public void RESET() {
        
        gyro.reset();
        leftEncoder.reset();
        rightEncoder.reset();
        timer.reset();
        drivetrain.stopMotor();
    }
    
    public void runMotorFor(double seconds, double power, double turn) {
        
        timerControl("Start");
        while(getTimer()<seconds)
        drivetrain.drive(power,turn);
    }
    
    public void timerControl(String args) {
        if(args.equals("stopAndReset")) {
            timer.stop();
            timer.reset();     
        }else if(args.equals("Start")) {
            timer.start();
        }else if(args.equals("stopAndReset")) {
            timer.stop();
            timer.reset();
        }
    }
    
    public boolean getEmergency() {
        
        if(XBoxController.getRawButton(5)) {
            EMERGENCY();
            boolean emergency = true;
            return emergency;
        }
        else {
            return false;
        }
    }    
    
    public double getActualSpeed(char LorR) {
        double speed = 0;
        if(LorR == 'L') {
            speed = leftEncoder.getRate();
        }
        else if(LorR == 'R') {
            speed = rightEncoder.getRate();
        }
        //INSERT CODE TO CONVERT RATE INTO -1 to +1
        return speed;
    }
    
    public boolean getBrake() {
        
        brakeOn = false;
        if(XBoxController.getRawButton(2))
        {
            BRAKE();
            brakeOn = true;
        }
        return brakeOn;
    }
    
    public double getTimer() {
        double time = timer.get();
        return time;
    }
    
    public double getEncoder(char LorR) {
        double distance = 0;
        if(LorR == 'L') {
            distance = leftEncoder.getDistance();
        }else if(LorR =='R') {
            distance = rightEncoder.getDistance();
        }
        return distance;
    }
    
    public double getGyro() {
        return gyro.getAngle();
    }
    
    public double getAndLoopTimer() {
        double time = timer.get();
        timer.stop();
        timer.reset();
        timer.start();
        return time;
    }    
    
    public float returnTriggerSpeed() { // Get the speed value from the xbox controller triggers
            double speed = XBoxController.getRawAxis(Triggers);
            // speed = exp* growth on trigger getRawAxis value
            return (float)speed;
    }    
    
    public void returnTurn() {
        
        // Check if triggers are on use for control if not use joysticks
        boolean triggersOn = XBoxController.getRawAxis(Triggers)>0.1 || XBoxController.getRawAxis(Triggers)<0.1;
            if(triggersOn) {
                
                rightSpeed = returnTriggerSpeed();
                leftSpeed = rightSpeed ; 
            } else if(triggersOn == false) {
                
                if(XBoxController.getRawAxis(LeftY)<= 0.05 && XBoxController.getRawAxis(LeftY)>= -0.05) {
                    
                    leftSpeed = 0;
                } else {
                   
                    leftSpeed = (float) XBoxController.getRawAxis(LeftY);
                }
                
                if(XBoxController.getRawAxis(RightY)<= 0.05 && XBoxController.getRawAxis(RightY)>= -0.05) {
                    
                    rightSpeed = 0;
                } else {
                    
                    leftSpeed = (float) XBoxController.getRawAxis(RightY);
                }
                
            }
            
            //press button one for fine control 25% speed
            if(XBoxController.getRawButton(1)) {
                
                rightSpeed *= 0.25;
                leftSpeed *= 0.25;
            }
            
            // Imma goin fast again :D !!!!!!
            // press button 3 to use 100% speed
            if(XBoxController.getRawButton(3)) {
                
                rightSpeed *= 2;
                leftSpeed *= 2;              
            }
            
            //clayton wants to use this button for 75% control
            if(XBoxController.getRawButton(4)) {
                
                rightSpeed *= 0.75;
                leftSpeed *= 0.75;
            }  
            
            // take right and left speed value calculate for exp* growth  
            rightSpeed = (float) (0.94*(rightSpeed*rightSpeed)+0.05); 
            leftSpeed = (float) (0.94*(leftSpeed*leftSpeed)+0.05);
            
            // default to 50% speed 
            rightSpeed *= 0.5;
            leftSpeed *= 0.5;
    }
      
    public void robotInit() { //REMEMBER TO INITIALIZE EVERYTHING
        
        drivetrain = new RobotDrive(1, 3, 2, 4);
        XBoxController = new Joystick(1);
        gyro = new Gyro(1 , 1); 
        timer = new Timer();
        leftEncoder = new Encoder(2,1,2,2);
        rightEncoder = new Encoder(2,3,2,4); 
    }
    
    public void autonomous() { 
        
        while(isAutonomous()&&isEnabled()){ //Makes sure in autonomous.
            log("Autonomous control enabled");
        }
    }
    
    public void operatorControl() {
        
        while (isOperatorControl() && isEnabled()) { //Make sure in driver
            
            log("Operator control enabled");
            DRIVE();
        }
    }
    
    public void disabled() {
        RESET();
        log("Is disabled");
    }
}

/*
1: A
2: B
3: X
4: Y
5: Left Bumper
6: Right Bumper
7: Back
8: Start
9: Left Joystick
10: Right Joystick
 
The axis on the controller follow this mapping
(all output is between -1 and 1)
1: Left Stick X Axis
-Left:Negative ; Right: Positive
2: Left Stick Y Axis
-Up: Negative ; Down: Positive
3: Triggers
-Left: Positive ; Right: Negative
4: Right Stick X Axis
-Left: Negative ; Right: Positive
5: Right Stick Y Axis
-Up: Negative ; Down: Positive
6: Directional Pad (Not recommended, buggy)
*/

Thanks :slight_smile:

2

I notice several interesting redundancies and different ways of doing things.

But practically, code is supposed to be judged on usefulness.

  1. Does it work?
  2. Does it break?

If yes to 1 and no to 2, then it’s great. No one on your team will know the difference. :stuck_out_tongue:

3

Consistency is key in making maintainable code, which will save you from headaches in the future.

Not all of the following things can be done immediately, since programming is something that takes time to learn.

  1. Having a standardized naming convention will make code much more readable, and thusly maintainable by larger teams. For example, the code contains the following:
    1a) VariableName, variableName
    1b) METHODNAME (which to a some would look like some sort of macro), methodName
    1c) CONSTANT_NAME, ConstantName
  2. Breaking up your code into organized classes increases code maintainability (You have object oriented programming! Take advantage of that!)
  3. Use the features available in your language.
    One thing that really caught my attention is this:
public void timerControl(String args) {
        if(args.equals("stopAndReset")) {
            timer.stop();
            timer.reset();     
        }else if(args.equals("Start")) {
            timer.start();
        }else if(args.equals("stopAndReset")) {
            timer.stop();
            timer.reset();
        }
    }

This isn’t maintainable. Someone somewhere is going to accidentally capitalize something and then wonder “Gee, why isn’t timerControl doing anything?” Consider using something similar to an enumeration (look at how WPILibJ does it with things like Relay.Value.*)
4) Consider designing the “contract” which comes with a method. When I call something named “Get Emergency”, I expect it tells me something along the lines of “Is there a critical failure?” or “Is the emergency button pressed?”. I don’t expect it to actually mutate the state of the robot (GetEmergency can stop the drive train)
5) I should note that


        while(getBrake() == false && getEmergency() == false)

is equivalent to the more readable


        while(!getBrake() && !getEmergency())

Best wishes,
ItzWarty

4

Your team is lucky to have you! While there are definitely some differences in conventions… I agree with the previous posters.

If the code works, then who cares! Of course, in a professional world, optimizing code can definitely be a big factor, but for this application it’s not necessary.

5

Thanks for the feedback guys :slight_smile:

I just have one question about the whole object oriented stuff. Honestly, I just havnt had to time to even start understanding how java really works. I’m mostly basing my code on what I know already from other languages.

My question is, how would I use separate classes and still pull variables from all of them? Or is that question invalid (public modifier)?

I would also love to hava a constantly updating print function that shows speed, turning, gyro, encoders, etc. But to do that, I think that I would need to either Thread.sleep() every time to keep it from flashing, or find a way to clear the console (which might be a bad idea for debugging other things).

Newbie question, I know, but how do you actually call classes from the main class, and how do I actually run methods from those? In FRC, is it just public static void main(String] args) to make a main method in separate classes?

All help is appreciated,
Thanks :slight_smile:

6

Here’s a simple example of code that I’ve written for this year’s game.
The code obviously isn’t a complex part of our program, but it will at least serve as an example of how object oriented programming can make your life really nice.

Note that our method names are UpperCamelCase as opposed to the usual lowerCamelCase. I come from a C# background.


package edu.wpi.first.wpilibj.templates;

import edu.wpi.first.wpilibj.*;

/**
 * The LED Rings class manages the state of the LED Rings which we use to light
 * up the vision targets of the robot.  As has been seen through our real life
 * tests, the LED Rings do indeed cause a dramatic lighting change for our target.
 * 
 * The lighted up targets are then detected by image processing.
 * @author ItzWarty
 */
public class LEDRingsManager
{
    //--------------------------------------------------------------------------
    //
    // Fields/Properties
    //
    //--------------------------------------------------------------------------
    //Dio 1 - white, Dio 2 - green.  Directions are forwards or off
    private static Relay m_whiteRelay, m_greenRelay;
    private static boolean m_whiteRelayEnabled, m_greenRelayEnabled;
    
    //--------------------------------------------------------------------------
    //
    // Methods which are invokable by other classes
    //
    //--------------------------------------------------------------------------
    /**
     * Initializes the relays which we will use to enable/disable the LED rings
     * located at the goal-tracking camera of our robot.
     */
    public static void Initialize()
    {
        m_whiteRelay = RelayManager.GetInstance(2);
        m_whiteRelayEnabled = false;
        
        m_greenRelay = RelayManager.GetInstance(1);
        m_greenRelayEnabled = false;
    }
    
    /**
     * Toggles the state of the white LED relay.
     * 
     * If the LED is on, it will be set to off.
     * If the LED is off, it will be set to on.
     */
    public static void ToggleWhiteLed()
    {
        SetWhiteLedRingEnabled(!m_whiteRelayEnabled);
    }
    
    /**
     * Sets whether or not the white LED Ring is on or off
     */
    public static void SetWhiteLedRingEnabled(boolean enable)
    {
        SetLedRingEnabled(m_whiteRelay, m_whiteRelayEnabled = enable);
    }
    
    /**
     * Gets whether or not the white LED Ring is on or off
     */
    public static boolean GetWhiteLedRingEnabled()
    {
        return m_whiteRelayEnabled;
    }
    
    /**
     * Toggles the state of the green LED relay
     * 
     * If the LED is on, it will be set to off.
     * If the LED is off, it will be set to on.
     */
    public static void ToggleGreenLed()
    {
        SetGreenLedRingEnabled(!m_greenRelayEnabled);
    }
    
    /**
     * Sets whether or not the green LED Ring is on or off
     */
    public static void SetGreenLedRingEnabled(boolean enable)
    {
        SetLedRingEnabled(m_greenRelay, m_greenRelayEnabled = enable);
    }
    
    /**
     * Gets whether or not the green LED Ring is on or off
     */
    public static boolean GetGreenLedRingEnabled()
    {
        return m_greenRelayEnabled;
    }
    
    /**
     * Turns off all LEDs
     */
    public static void TurnOffAllLeds()
    {
        SetWhiteLedRingEnabled(false);
        SetGreenLedRingEnabled(false);
    }
    
    /**
     * Turns on all LEDs
     */
    public static void TurnOnAllLeds()
    {
        SetWhiteLedRingEnabled(true);
        SetGreenLedRingEnabled(true);
    }
    
    //--------------------------------------------------------------------------
    //
    // Helper Methods
    //
    //--------------------------------------------------------------------------
    /**
     * Sets the state of the given LED Ring relay
     * It should be note that this does not set the m_whiteRelayEnabled/m_greenRelayEnabled states
     */
    private static void SetLedRingEnabled(Relay relay, boolean enable)
    {
        if(enable)
            relay.set(Relay.Value.kForward);
        else
            relay.set(Relay.Value.kOff);
    }
}

Basically, you’ll notice that there are various “getters” and “setters”, which do nothing but get or set. When you call get, the state of the robot doesn’t change. This makes the code maintainable, as our methods are not “black boxes” - they have a contract, and they meet that contract.

Edit: I should note that RelayManager is one of our own classes; WPILibJ does not have a RelayManager class.

Printing stuff through System.out.println should be fine as long as you aren’t doing anything completely excessive. Clearing the console will do nothing except free memory on the controlling interface (ex: netbeans). Netconsole is a pretty simple protocol - you send text and the guy on the other side gets that raw text.

In our example:
RobotMain calls Initialize on the LED Rings manager: LEDRingsManager.Initialize();
RobotMain’s teleop and autonomous methods do so too: SetWhiteLedRingEnabled(true);

Best wishes,
ItzWarty

edit: I did end up renaming some methods (the *AllLeds methods) to enable/disable methods for consistency