Code Check Please!

[jhellr13]

Over the past few days, I have been working on taking my labview code from this previous year and converting it to java. I have some knowledge of java from taking AP Computer Science this previous school year, and learned some stuff from looking around on here and the API. I believe I wrote the code code somewhat correctly, but I am not with the robot so I cannot run it to see. I was wondering if anyone could look it over. Give some tips, find any problems, help me out.

I have commented most of my code, so it will hopefully be easy to understand.
My main concerns are if I did the limit switches correctly, my drive system, relays, autonomous, my tasks that run over time(there is one servo where this is done, and the thrower motor), and how I initialized all my variables.

If you have any questions regarding the code or how our robot operated, please ask! I will help you out to the best of my ability.

Here is the code: http://code.google.com/p/jared-1751-...ource/checkout

Thank you!!!

[jhellr13]

Anyone take a look or find anything...?

[WizenedEE]

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.

•                 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:
  

  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.

[jhellr13]

Thanks for the help. I looked at all of your comments, and went through my code again.

• I don't really have a problem with the name as it doesn't bother me, but I could see what you mean.

• I have seen online a lot of programmers define the port numbers for buttons and other stuff. I could always do that, but the way it is does not bother me much, though I do agree it would look nicer.

• In regard to making multiple classes. I have thought about doing that over the past few days, but I haven't had the time to really go at it, and I wanted to get some responses to this post so I knew the stuff looked okay first.

• I switched that comment in autonomous. Thank you! I didn't even notice it before.

• I got rid of that for loop in autonomous. It really didn't have any purpose.

• I changed the name of the timers. I agree they weren't very descriptive names, but they worked.

• I fixed those spots that had the while loops with the delays. Thank you very much for helping me out with this. Your way is very nice.

• I do agree it does look awkward in code using "==true" or "==false," but it's just the way I have done booleans in learning java this past year in AP Computer Science. It's just how i do it.

• I also set the watchdog to false in autonomous, and to true in teleop.

Thank you for all your suggestions and help!

The new code is available here: http://code.google.com/p/jared-1751-...ource/checkout

[WizenedEE]

I think with the timers, you have to define them outside of the loop or else they'll get recreated each time and lose their value.

Like:

Timer timer1 = new Timer();
while (...) {
  // use timer1
}

not

while(...) {
  Timer timer1 = new Timer();
  // use timer1
}

In c++ you could make it static, but that doesn't exist in Java:

while(...) {
  static Timer timer1 = new Timer();
  // use timer1
}

EDIT:
same for things like spinThrower. You may want to read up a little on scope.

The loss of static is one of the main reasons I like c++ more than java. In java you have to put all of those booleans before the loop and then you have to keep going up and back to see what type they are, while in c++ you can make them static and they'll be right there. Oh, well.

[jhellr13]

Originally Posted by WizenedEE

I think with the timers, you have to define them outside of the loop or else they'll get recreated each time and lose their value.

Like:

Timer timer1 = new Timer();
while (...) {
  // use timer1
}

not

while(...) {
  Timer timer1 = new Timer();
  // use timer1
}

In c++ you could make it static, but that doesn't exist in Java:

while(...) {
  static Timer timer1 = new Timer();
  // use timer1
}

yeah, that might be a problem. Thanks