Bad Programming Practices

I’m noticing a disturbing trend here. While the code being shared is great, the actual programming isn’t.

The majority of code here is barely commented, if at all. This is a poor practice. Well commented code is easier to read, easier to understand, and easier to debug. The more complicated the code, the more comments it should have. The examples provided by FRC Java are especially bad. The CircleTracker and Dashboard examples are extremely difficult to interpret without cross referencing them across multiple documents scattered all over the web.

Think about it: one of the advantages to Java is that classes are reusable. But two years from now how reusable is it if the original programmer has left and the new one now has to spend hours, or sometimes days, to figure out how the old stuff works? In many cases the old, hard to read stuff will get scrapped, and the new programmer will invent the wheel all over again.

I’ve been in college courses where it was a requirement to comment every single line. I’ve always thought that was a bit excessive, but in the courses I teach if the purpose of a code segment isn’t completely obvious it better be commented so I can understand what was intended, if the student intends on getting a decent grade :stuck_out_tongue:

I strongly urge you to take a look at your programming habits and try to improve them. Lets work on making things easier for the people who come after us, so they can concentrate on enhancing and improving our work rather than having to rewrite it.

Usually the code in college courses requires several thousand lines of code. FRC robots only require a couple hundred… if that… So commenting every line may not be excessive.

However I will note commenting every line was never a requirement of mine. Every function (except getters and setters) should be commented. Plus helpful comments throughout the function.

I will add that I am one of the guilty parties regarding the commenting of code. I will make an effort to be better about this in the future…

I agree commenting is good practice, and also that those darn camera classes are so hard to understand! My problem though is that I just come up with a good idea really fast and need to code it in before I forget it, and then I think of a way to improve it, and before you know it I’ve written hundreds of lines without a single comment because I was only spewing ideas out of my head. I just never get around to adding comments because since it’s my code, it’s obvious to me what it says. I totally agree though that comments are a good idea to add in as often as possible.

I go back to mine during the day, clean it up and add additional comments where necessary. I also try to comment as I go, so that when I go back and look at the mess I made during a flurry of lunchtime coding I can remember what I was trying to accomplish. You’d be surprised how much that helps you figure out why it isn’t working when it actually gets uploaded to the bot.

Another good practice is good variable names. Mine get clear, descriptive names like spinGyro and tiltGyro, instead of something cryptic like m_gyroA1 (my gyro on analog port one? And does that one tell me if I’m turning or tipping over?) The FRC classes are really bad about that; I mean, who came up with the idea that the top line of the LCD should be named kMain6, instead of lcdLine1? Took me a lot of test messages to figure out which line was which (kMain6 is the top line, then they are kUser2 through kUser6 going down.)

I try to be aware that next year they might not have me to help them, but if the code is clear and well commented they won’t have to start from scratch. There’s a composition book of notes, diagrams, and explanations to go with it as well. Maybe I can get one of my students to type that up some day into something a little more legible.

The following is my entirely unstructured views on code commenting and self documentation based on several years of professional programming, freelance, robotics, and just mucking around with code.

I believe additional refactoring should always be done after the initial code is done. I often try to get people who have never read code in their life to see how much of my program they can understand.

Commenting at this stage is especially useful because the concepts are still fresh in your mind, but you get a little more perspective on what you might not remember/understand a month down the road.

Ask yourself this in two months.

I try to make a rule that every function and class should have a Javadoc style comment with a description and if in anyway non-obvious - describing parameters and return values.

While very helpful as an overview, I’ve found most of the example code that comes with standard installations are very messy. Agreed.

This is dumb, it’s not only a waste of time but breaks the golden rule of commenting. You end up seeing things like this:


++i; // Increment i by one.

Frankly, it makes the code way harder to read - especially with syntax highlighting. I know some programmer’s who set comments to the same colour as their background colour; while I find this a bit extreme I quite frequently see their point.

Folding is also a great alternative to this, but it’s hard to find a good folding plugin for most editors in my opinion.

First to state the commonly accepted rule of commenting (I like to call it the golden rule) for those who have never heard about it before:

Comment why you did it, not what it does.

If you have to comment what it does, you should be making better use of two awesome concepts called encapsulation and abstraction. For instance, would you prefer to see:


// ...
float processedEncoderValue = GetEncoder() * scalingCoefficient - offset; // Process the raw encoder value.
// ...

or


float GetAndProcessEncoderValue() {
  return GetEncoder() * scalingCoefficient - offset;
}
// ...
float processedEncoderValue = GetAndProcessEncoderValue();
// ...

This benefits in two ways, self-documented code and makes the program more modular and organized. You can put functions in another source file, you can’t put those comments in a difference source file.

Properly naming variables also heavily helps self-documented code making things clean and prevents end-of-line comments that not only slow down development time but add a lot of fluff to the code.

Using constants everywhere is something I treasure along the same lines as properly naming variables. If you ever have a value that somebody will need to check a comment elsewhere to see what it means, it should be in a constant with a human-readable name. For instance,


MyRobot():
  pan(3), // Pan servo PWM port #3
  tilt(4),  // Tilt servo PWM port #4
  searchSwitch(1), // Digital I/O search toggle switch port #1
  cameraLED(2), // Digital I/O camera LED port #2 
  joystick(1) // Left joystick USB port #1
{ // ... 
}

I would have as


MyRobot():
  pan(PAN_SERVO_PWM_PORT),
  tilt(TILT_SERVO_PWM_PORT),
  searchSwitch(SEARCH_TOGGLE_SWITCH_PORT),
  cameraLED(CAMERA_LED_PORT),
  joystick(LEFT_JOYSTICK_PORT)
{ // ... 
}

I frequently find myself, and others, guilty of doing things like this:


// Feed watchdog.
FeedWatchdog();

// Declare variables to store text.
char text[255];
char text2[255];
char text3[255];
char text4[255];

// Format and fill text.
sprintf(text, "X: %d  Y: %d", currentParticle.center_mass_x, currentParticle.center_mass_y);
sprintf(text2, "Particle:Image %.4f", currentParticle.particleToImagePercent);
sprintf(text3, "Iteration: %d", currentIterationCycle);
sprintf(text4, "Num Iters: %d", numberOfIterations);

// Clear Driver Station.
driverStation->Clear();

// Print to the Driver Station.
driverStation->PrintfLine(DriverStationLCD::kUser_Line1, text);
driverStation->PrintfLine(DriverStationLCD::kUser_Line2, text2);
driverStation->PrintfLine(DriverStationLCD::kUser_Line3, text3);
driverStation->PrintfLine(DriverStationLCD::kUser_Line4, text4);

// Update the Driver Station
driverStation->UpdateLCD();

Ask yourself, are any of these really necessary?

I’m sure you could find a million examples on Google of bad code commenting, but the best way to adopt a good code commenting style (and general code style, i.e. formatting, etc) is reading other’s code.

I’d like to raise another point on the formatting of your comments, when you do write proper comments, remember other human beings are reading them, and most likely reading them in English. It’s my personal preference, but I find it annoying when I see this (it takes your clean looking, professional code and makes it look…well, immature):


//necessary for proper calculations due to...

Comments are English text. They should start with a capital and should not be against punctuation (//).


// Necessary for proper calculations due to...

I generally only use comments for the following situations in robot code:

  • Header comment (author, description)
  • Explaining mathematical operations
  • Javadoc-style documentation for classes and functions
  • Organizing constants

I can’t stress enough: Place a comment to explain why it does it rather than what it’s doing.

Now of course, we don’t need to explain why something needs to be done due to explicit idiosyncrasies of a language or paradigm, for instance the following should never occur:


delete image; // We need to free up memory

However the following might be acceptable under certain conditions:


delete image; // At this point, the system is running out of memory and must sacrifice this image for more essential data.

One last point on TODO, FIX LATER comments. I am not totally against this, grepping through your programs for TODO does not make me want to hurt kittens. However I really feel like these sorts of comments are what version control and other tools are for.

That’s my two cents!

Here’s some reading material if I wasn’t boring enough:
Jakob Kulzer - My Case Against Code Comments
Comments vs. Self Documenting Code

If you are looking for an entire paradigm shift, I’d have to recommend getting involved with the Ruby community - Rubyists heavily stress modular, neat, beautiful code with emphasis on refactoring and human-readable code.

Here is an example of a Ruby Domain-Specific-Language code snippet, bow before it.


describe Account do
    context "transfering money" do
      it "deposits transfer amount to the other account" do
        source = Account.new(50, :USD)
        target = mock('target account')
        target.should_receive(:deposit).with(Money.new(5, :USD))
        source.transfer(5, :USD).to(target)
      end

      it "reduces its balance by the transfer amount" do
        source = Account.new(50, :USD)
        target = stub('target account')
        source.transfer(5, :USD).to(target)
        source.balance.should == Money.new(45, :USD)
      end
    end
  end

I’m not really sure how any comments could improve that code.

EDIT: I forgot the paragraph about TODO style comments.

/*----------------------------------------------------------------------------*/
/* Copyright (c) FIRST 2008. All Rights Reserved.                             */
/* Open Source Software - may be modified and shared by FRC teams. The code   */
/* must be accompanied by the FIRST BSD license file in the root directory of */
/* the project.                                                               */
/*----------------------------------------------------------------------------*/

package edu.wpi.first.wpilibj.Robot2011;


import edu.wpi.first.wpilibj.IterativeRobot;

/**
 * The VM is configured to automatically run this class, and to call the
 * functions corresponding to each mode, as described in the IterativeRobot
 * documentation. If you change the name of this class or the package after
 * creating this project, you must also update the manifest file in the resource
 * directory.
 */
public class main extends IterativeRobot
{
    /**
     * This function is run when the robot is first started up and should be
     * used for any initialization code.
     */
    Robot Bot;
    public void disabledPeriodic()
    {
        
    }
    public void robotInit()
    {
        Bot = new Robot();
    }
    /**
     * This function is called only once during autonomous
     */
    public void autonomousInit()
    {

    }

    /**
     * This function is called periodically during autonomous
     */
    public void autonomousPeriodic()
    {

    }
    /**
     * This Function is called only once during teleop
     */
    public void teleopInit()
    {
        
    }

    /**
     * This function is called periodically during operator control
     */
    public void teleopPeriodic()
    {
        Bot.RunTeleop();
    }
    
}

Now honestly how in the world can I comment better than that? (Serious) That is really this year’s code.

I haven’t seen much of the Java code, just C++ however I’d like to point out - quality over quantity.

Those latter few comments seem rather un-descriptive to me but I don’t mind I don’t think the original poster thought doc comments were the issue.

EDIT: On the note of quite a few of WPILib’s function comments, I find them lacking in actually useful documentation. A lot of them generally take the name of the function, and expand it to a grammatically correct English sentence instead of saying something useful like for instance, the unit of the return value (off the top of my head).

package edu.wpi.first.wpilibj.Robot2011;

import edu.wpi.first.wpilibj.Robot2011.Actuate.*;
import edu.wpi.first.wpilibj.Robot2011.Calculate.*;
import edu.wpi.first.wpilibj.Robot2011.Perceive.*;

/**
 *
 * @author davidthefat
 */
public class Robot
{
    private ArcadeDriveCalculation ArcadeDrive;
    private DataStructure Data;
    private Drive Arcade;
    private CVJoystick JoyManager;

    //Start Test Variables
    private CVEncoder e1;
    //End Test Variables

    public Robot()
    {
        ArcadeDrive = new ArcadeDriveCalculation();
        Arcade = new Drive();
        Data = new DataStructure();
        JoyManager = new CVJoystick();
    }
    public void RunTeleop()
    {
        JoyManager.update(Data);
        ArcadeDrive.Calculate(Data);
        Arcade.setMotors(Data);
    }
    //Use these test functions to test whatever you need
    public void test()
    {
        e1.update(Data);

    }
    public void testInit()
    {
        e1 = new CVEncoder(1,6);
        e1.setDiameter(11.1);
    }

}

That is how I code. I never understood commenting obvious things, but everything seems obvious to me. Its like “thanks captain obvious”.

Edit: why the CVEncoder takes in 2 variables is because the new kid made it take in indices for a 2d array to handle the encoder channel. So it is very inefficient. Do not judge.

So later down the road (be that yourself not remembering, or somebody who has not read your code) somebody will ask what is e1? Does 1 refer to something like an encoder on the arm? On a drive train motor?

But yes, for such simple, short code - comments would be rather pointless.

Most of the folks on this thread are experienced programmers and could probably understand whatever was put in front of them. But remember your audience! If someone is coming to CD for coding help they are often not just novices, but novices without resources and mentors locally that they can turn to for help.
One example given above was why comment i++?
I use ++ and += without thinking about it, but imagine what that looks like to a freshman who has never coded in any language. To them its just hieroglyphics. With every new batch of programmers you need to go over the basics. And while many requests are from obviously experienced people with sophisticated questions, there are still rookies lurking trying to learn.
We are not here just to build robots or show off our l337 coding skillz. The more knowledgable and experienced should be making an effort to raise the level of all. To make programming more accessible to all. You don’t want to scare off the new people or make them think only Labview is suitable for them.
I think that was the intent of the original poster’s comments. Comment your personal code as you like (its your problem later on) but comment code that will be your teams legacy for the future programmers. Especially structure and comment the code you post here so that when some eager new kid looking to try java comes across your code in five years they will think:
a) I can do this!
and
b) Wow, thanks for taking the time to post it!

Amen to that. I’ve noticed a trend of people thinking that javadoc comments are the same as documentation. Unless you’re extremely verbose in your comments they aren’t. Javdoc is a neat concept, but in most of the cases I’ve seen it only serves as about half of what is really needed in the way of documentation.

Whatever your personal philosophy is on commenting, you should try to remember that you aren’t writing them for professional programmers, you’re writing them for high school students who may or may not have a knowledgeable mentor to guide them in the future. Your code and comments should be clear as day so that someone just starting out can tell what you were doing and why.

If you had to explain what the compiler is doing additionally to what your program logic is doing, there would be way too many comments - pages and pages and pages of comments. Ctrl+T http://google.com is your friend.

I subscribe to the philosophy that there is no single way to write good code - it all depends on how it will be used, re-used, modified, etc. Writing code for an F-22 is fundamentally different than writing code for a FIRST robot.

When writing code for FRC, I put tremendous emphasis on techniques that enable very quick code changes with a minimum of potential for new mistakes. In between two matches, if I need to disable a mechanism or alter autonomous mode, etc., I want to be able to get in there, find the part I care about, and have tools in place that will prevent me from doing something foolish.

To this end…

Giving good, informative names to all classes, methods, and variables goes a long way towards making code self-documenting.

Bad:
double Gyro.get();

Better:
double Gyro.getAngle();

Better yet:
double Gyro.getHeadingInDegrees;

When you read code written in this way, you don’t need nearly as many comments to explain what you are doing, and it becomes quite obvious when you make a mistake. (Obviously, there’s a fine line between being descriptive and re-writing the entire function in its name)

On top of that, I always name variables according to scope…

m_memberVariable (starts with m_ to indicate it is a member of a class)
k_someConstant (starts with k_ to indicate it is a constant)
l_someLocalVariable (starts with l_ to indicate it is local in scope to a given block)
g_someGlobal (starts with g_ to indicate it is a global variable)
a_someArgument (starts with a_ to indicate it is an argument to the current method)

There are likewise certain design patterns I like a lot, and others that I eschew in FIRST simply because they are less intuitive or compact.

I entirely agree, however we are all human beings and can generally see when something is useless or not.

Excellent point, agreed once again. I generally try to have very modular and self documenting code for this reason.