View Single Post
  #3   Spotlight this post!  
Unread 26-06-2012, 18:56
WizenedEE's Avatar
WizenedEE WizenedEE is offline
Registered User
AKA: Adam
FRC #3238 (Cyborg Ferrets)
Team Role: Leadership
 
Join Date: Jan 2011
Rookie Year: 2010
Location: Anacortes, WA
Posts: 395
WizenedEE is a name known to allWizenedEE is a name known to allWizenedEE is a name known to allWizenedEE is a name known to allWizenedEE is a name known to allWizenedEE is a name known to all
Re: Code Check Please!

I'm no java expert (I like c++) but...
  • Change the name please?
  • You may want to define the port numbers for all the buttons and relays and stuff somewhere else so they're easily changed. In our code we used an extra header file but I don't know how to do it in java (maybe make another class with a bunch of "public static final"s in it?)
  • Get more than one file in there! Have an extra class for each subsystem of the robot. If you want to be fancy, make an interface for methods like start, idle, and stopEverything and then use those
  • In autonomous, there's a comment saying you wait for 5 seconds while there's a 2 in the code.
  • I'm not sure about this, but you might have watchdog timeouts while waiting. Make sure it works, otherwise disable the watchdog or keep calling the move command for 2 seconds.
  • Why do you have a for loop that runs once? Just get rid of it
  • You might want a timeout while waiting for the limit switch hit -- what if a wire gets broken and the arm keeps going up while you sit behind the DS praying nothing breaks?
  • (in OperatorConrol)time and time2 aren't very descriptive names for timers.
  • Code:
                    time.start(); //starts a timer at 0 seconds
                    while(time.get() < 1500)
    I'm pretty sure it's at least a terrible idea to do this. While this loop is going, everything else on the robot will be completely unresponsive for 1.5 seconds (although hopefully the watchdogs will shut the motors off if you haven't disabled them in autonomous). You need to essentially change the while to an if and let it be called each time, like this:
    Code:
    void OperatorControl() {
        Timer ballReleaseTimer = new Timer();
        ballReleaseTimer.start();
        boolean releaseBall = false;
        while(isOperatorControl() && isEnabled()) {
            if (ballReleaseTimer.get() > 1500) {
                releaseBall = false;
            }
            if(controlStick.getRawButton(3)) {
                releaseBall = true;
                ballReleasetimer.reset();
            }
            if(releaseBall) {
                 // Move servo
            }
        }
    }
    (treat as pseudocode)
    There should be no "while"s or "delay"s in your loops (except maybe one for several milliseconds to let other processes have time to go)
  • A style thing, but if I'm using an if/else/while/etc keyword without braces (like if(x) y() I put the statement (like y()) on the same line so I know I can't add something without putting in extra braces. It might prevent some bugs down the line.
  • I'm not sure about java, but I think it's still awkward to say "if (function() == true)" since the ==true is implicit. Similarly, instead of using ==false, use !.

If you don't like any of my suggestions (especially style ones), feel free to ignore them. Hopefully I helped a little.
Reply With Quote