Thread: Code Review!!
View Single Post
  #2   Spotlight this post!  
Unread 03-03-2011, 23:26
Jeanne Boyarsky Jeanne Boyarsky is offline
Java Mentor
FRC #0694 (StuyPulse)
Team Role: Mentor
 
Join Date: Jan 2010
Rookie Year: 2010
Location: New York
Posts: 100
Jeanne Boyarsky has much to be proud ofJeanne Boyarsky has much to be proud ofJeanne Boyarsky has much to be proud ofJeanne Boyarsky has much to be proud ofJeanne Boyarsky has much to be proud ofJeanne Boyarsky has much to be proud ofJeanne Boyarsky has much to be proud ofJeanne Boyarsky has much to be proud ofJeanne Boyarsky has much to be proud of
Re: Code Review!!

Minor nit: The URL to checkout on http://code.google.com/p/2011deploya...ource/checkout has a space in it so you can't copy/paste it.

Things that jump out as possible problems:
  1. In the drive class, if a CAN timeout happens, you print the error and move on. What should happen in this case. If you can't drive, you are kind of doomed. But is there anything you can do to partially recover? Drive with one motor maybe? (I don't think the CAN network precludes this depending on which fail.) If that's the case, you'd need null checks around the CANJaguar calls so some can work.
  2. We didn't extend RobotDrive directly, so I'm not sure about this one. But isn't while(true) dangerous? It looks like it would force you to reboot the robot after each match. Whereas while (!isEnabled()) would end the loop when you disable the bot.
  3. fibers[current].run(); looks strange. The fibers/components implement the Runnable interface in Java. The idea of which is to use it for threads which run in the background. To use them, one typically calls something like "new Thread(fibers[current]).start()". This creates a new thread and starts it in the background. Whereas fibers[current].run() just call the run method once synchronously. So you aren't using any of the benefits of threading. Which clearly works fine. But implementing Runnable adds confusion. At least to someone who works with Java regularly .
  4. If you were using threads to run things in the background, you might have a "CPU starvation" problem. The run methods do their work constantly instead of sleeping/waiting/yielding to let other threads run. If you were to switch to threads this is something important to be aware of. Due to #3, it's a point of theoretical interest.

Also one Java coding conventions. I wouldn't change it at this point, but you asked for a code review so you are getting one from a real Java programmer .
  1. Class names should begin with uppercase

And some things I particularly liked:
  1. Good package structure/design
  2. I like how you handled the minibot and have a check for whether the game is almost over
  3. It was easy to read and figure out what was going on.

Good luck!
__________________
Team 694 mentor 2010-present, FIRST Volunteer and Co-organizer of FIRST World Maker Faire Tent
2012 NYC Woodie Flowers Finalist
2015 NYC Volunteer of the Year
Reply With Quote