Go to Post Is it legal to build 2 robots? yes. Is it crazy to do so? maybe. - Andy Baker [more]
Home
Go Back   Chief Delphi > Technical > Programming > Java
CD-Media   CD-Spy  
portal register members calendar search Today's Posts Mark Forums Read FAQ rules

 
Reply
Thread Tools Rate Thread Display Modes
  #1   Spotlight this post!  
Unread 19-12-2011, 10:55
gixxy's Avatar
gixxy gixxy is offline
Programming and Arduino Mentor
AKA: Gustave Michel III
FRC #3946 (Tiger Robotics)
Team Role: Mentor
 
Join Date: Nov 2011
Rookie Year: 2012
Location: Ruston, LA
Posts: 207
gixxy is on a distinguished road
Would someone review my code?

We are a Rookie Team and since only one of us had any LabVIEW practice, and one had PHP Scripting experience (myself) we decided to give Java a shot.

We built an air cannon to shoot t-shirts at football/sports games, (using a vex controller). That was practice for our build team. Now the Programming team is working on reprogramming the Robot with FRC Java, as practice for build season, however we do not have a cRio yet so we can't test it.

If someone would look over the code, and give suggestions on correcting it, or alternative ways to do something. It would be appreciated.

https://github.com/gixxy/AirCannon-Robot/

Don't too harsh of this turns out to be really bad code, I wrote this all yesterday in about 2 1/2 hours.

Last edited by gixxy : 19-12-2011 at 10:56. Reason: spelling/wording corrections i didn't catch at first.
Reply With Quote
  #2   Spotlight this post!  
Unread 20-12-2011, 02:02
linuxboy linuxboy is offline
Registered User
AKA: Oliver Graff
FRC #3780
Team Role: Alumni
 
Join Date: Nov 2010
Rookie Year: 2009
Location: MI, USA
Posts: 217
linuxboy has much to be proud oflinuxboy has much to be proud oflinuxboy has much to be proud oflinuxboy has much to be proud oflinuxboy has much to be proud oflinuxboy has much to be proud oflinuxboy has much to be proud oflinuxboy has much to be proud oflinuxboy has much to be proud of
Re: Would someone review my code?

Hi,
Let me start off by saying, the structure looks well done to me in terms of thinking about it in an OO way, which makes coding a lot easier (as I'm sure you know) so most of my comments will probably be pretty nitpicky. Also, props on using git for source control and github to share it. Also, good commenting, be sure, however, when your code gets more intricate, to comment it (this is a case of do as I say, not as I do). I don't know if you attended a beta presentation or read about the beta, but singletons are a big part of it, and you are already using them.

On to criticisms (I can't think of a benign word, sorry):
I would reccomend using a seperate package from all the WPILib stuff, I use org.frc3780 for team stuff and com.ograff for personal stuff, so that would be my suggestion for a naming convention.

Since you singleton class constructors are protected, they can still be called by other classes in the same package, and since all of your other classes are in the same package, it kind of defeats the purpose imo. The default code for singletons in the beta use private constructors, and I would suggest using the same convention.

Also, sometimes its useful to preface instance variables with something (I've seen it done with _var or m_var)

Here on out I will try to go file by file:

AirCannon.java:
I would make the static variable that holds the air cannon instance private
I would make the instance variable that holds the PWM aircannon private, and initialize it in the constructor, not the variable declaration. Also, I don't know what MotorController you are using but if it is a Jaguar, or a Victor, I would suggest using the respective class.
For the fire method, when the beta stuff is generally released, I would create a openValve() method and a closeValve() method for the AirCannon class, and then use CommandGroups to make one happen after the other (there are wait commands that can be used). I suggest that, so as to not block the main thread.

ControlBoard.java:
I recommend having instance be private, and only accessible through a getInstance() method.
I really recommend against initializing instance variables at declaration.
I think that the two instance variable should also be private, if another class needs access, use get methods. I would even recommend using private getters if no other class needs access to them, because then you can use lazy instantiation (same idea as a getInstance() method, don't initialize instance until someone requests it) however that is a habit from iPhone development, but I feel is good practice on an embedded platform.
If getStopToggle() were to be called continuously then it would flip back and forth (the new Operator Interface class will help with this), since no one can hold down a joystick button for the exact right amount of time (20 ms is how often the iterative methods are called IIRC).
Also, getStopToggle does more than just get the stopped state, while polling the button is okay there (however, with the new robot template, that won't be necessary), I don't know why you are logging values, since that method only deals with the stop toggle, not the values that its logging. When code works, those values will be right, but if it doesn't those values could be wrong, and then they will just confuse you.

A lot of what you did there will be dealt with by the OperatorInterface class I think.

ControlMap.java:
I don't quite understand the reason for this.
Public instance variables tend to be bad practice, and this whole interface thing seems a little unnecessary to me (but to each their own). Either way, I think that instance variable should be implementation specific, not interface wide.

Controller.java:
Private instance variable, possibly with private (or public) getters and lazy instantiation, not instantiated at declaration.
masterControl looks fine for now, good job separating this from the main file, although your naming seems a little unusual to me (class name, and method), but that is just preference.
In the Command Based robot (the templates for next year) I believe you would have a CommandGroup for the main control method that drives and moves on to a fire command when the button is pressed, or maybe a drive command, that gets interrupted by a fire command when the button is pressed.
For the line you use to set the stopped state, direct instance variable access tends to be a bad idea, I would recommend getters and setters.

DriveTrain.java:
Make that static instance variable private.
Initialization done in constructor, or lazily.
Private initializer
Driving can be done with arcadeDrive, just pass it a joystick, might make things simpler.
stop(): I would call self.drive(0,0) that way if you do any logging in your drive method it will get called.

SoftSwitch.java:
I would re-implement toggleState() with getters and setters, instead of direct variable access, also, if you want a one liner for it, that would be setState(!getState());

UglyBetty.java:
Looks fine, it will change for the command based robot a bit.

I encourage you to look through some of the beta presentations, as I reference them a lot. I will also fork this, and make the changes I have suggested with and without beta stuff so you can see what I'm talking about.

Again, really good job, the stuff I mentioned was really pretty particular, and probably mostly personal preference.

- Oliver
Reply With Quote
  #3   Spotlight this post!  
Unread 20-12-2011, 03:17
linuxboy linuxboy is offline
Registered User
AKA: Oliver Graff
FRC #3780
Team Role: Alumni
 
Join Date: Nov 2010
Rookie Year: 2009
Location: MI, USA
Posts: 217
linuxboy has much to be proud oflinuxboy has much to be proud oflinuxboy has much to be proud oflinuxboy has much to be proud oflinuxboy has much to be proud oflinuxboy has much to be proud oflinuxboy has much to be proud oflinuxboy has much to be proud oflinuxboy has much to be proud of
Re: Would someone review my code?

Here is my fork:
https://github.com/monks700/AirCannon-Robot

I will be implementing it with beta stuff soon as well in a separate branch. Also, it is entirely possible I did not follow some of my own advice, but take a look at the changes I did make, I hope you will find them helpful.

Oliver
Reply With Quote
  #4   Spotlight this post!  
Unread 20-12-2011, 08:36
gixxy's Avatar
gixxy gixxy is offline
Programming and Arduino Mentor
AKA: Gustave Michel III
FRC #3946 (Tiger Robotics)
Team Role: Mentor
 
Join Date: Nov 2011
Rookie Year: 2012
Location: Ruston, LA
Posts: 207
gixxy is on a distinguished road
Re: Would someone review my code?

Explation my use of a ControlMap interface: Its for if you may have more than one controls setup. Since right now i only have one ControlBoard class it is fairly useless, but if say I wanted to make it so that I could switch between Arcade and Tank Drive, It would come in handy. I can just re-init the same ControlMap interface in Controller using the other ControlBoard's object.


did i really use protected on the singleton classes constructors...... (I wrote it, and the getInstance() method once then copy and pasted it, changing the class names). Yep, derp on my part.

As with packages, I normally would have used them, but this was simple enough that it didn't matter.

With masterControl() and Controller.... I changed it when I realized I had called the class TheUser and the Method masterControlProgram().... I felt I had let my geek get away from me too much.....

also, I did take a look at the command based template, and personally haven't taken to it yet, I prefer it being open ended. I'm sure once I actually use it I will feel differently, but for now I wrote it with the template I felt I liked best.

And in any case: Thanks, some very good information.
Reply With Quote
Reply


Thread Tools
Display Modes Rate This Thread
Rate This Thread:

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

vB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Forum Jump


All times are GMT -5. The time now is 12:45.

The Chief Delphi Forums are sponsored by Innovation First International, Inc.


Powered by vBulletin® Version 3.6.4
Copyright ©2000 - 2017, Jelsoft Enterprises Ltd.
Copyright © Chief Delphi