Grade my code? (Am I doing it in a dumb way.. LOL)

I was wondering if somone might take a look at my code and give me a general overview of anything I am doing "wrong"?

More of a best practices/more experienced eye to see if I am doing easy things the "hard way". I don't need specific code solutions, more of a "Hey dummy, don't use an array that way! Look at the ixnay on the rraynay function."

Overall project: A custom lightsaber driver that interfaces with a "Toy" soundcard and controls, via PWM a 4 color high power LED driver. The idea is to select a color then maintain that color through a fading in and fading out process. When it fades in it also turns on a sound card. When it fades out it turns off the sound card. It also checks for a signal from an Impact sensor and "flickers" the Amber LED for a "clash effect" Lastly, it selects one of two soundbanks from the soundcard.

I tried to be pretty verbose in my comments...

I appreciate any input!

Part one: Setup and Loop

/* Lightsaber test sketch controlling a "Joe Jedi" SW616 sound card 
 Modified by Troy Ollom
 Feb. 10, 2010
Based in part on the "Four button sketch" by Jeff Saltzman.
 To keep a physical interface as simple as possible, this sketch demonstrates generating three output events from a single push-button.
 1) Click: rapid press and release
 2) Double-Click: two clicks in quick succession
 3) Press and Hold: holding the button down
 Normal mode:
 Click: SaberOn (Ramps up LED, turns on sound)
 Double-click: (Ramps off LED, turns off sound
 Click-Hold: Enters MenuMode 1
 
 Menu Mode 1:
 Click: Cycle through RGB colors
 Double Click: Skip to next hue of RGB colors
 Click-Hold: Enters Menu Mode 2
 
 Menu Mode 2:
 Click: Select "Jedi" sound bank
 Double Click: Select "Sith" sound bank
 Click-Hold: Cycle back to menu mode 0 AKA "Normal"
 
 */
#include <EEPROM.h> // Set up the EEPROM library so I can read/write from permenant memory
#define buttonPin 2 // analog input pin to use as a digital input
#define redPin 3 // PWM output pin for Red LED 1
#define sndOnPin 4// Digital out to turn on soundcard (Joe Jedi, one side of switch.)
#define grnPin 5 // PWM output pin for Green LED 2
#define bluPin 6// PWM output pin for Blue LED 3
#define impactPin 7// Input pin for impact sensor (Joe Jedi, second yellow wire on plug)
#define sndSelectPin 8// Digital out to select sound True is Sith, False is Jedi (Joe Jedi, left most pad on RGB select switch)
#define flashPin 9 // FoC LED Pin PWM Pin connected to Amber die of RGBA LED driver

// RGB LED variables
byte rgbArray [54][3]={  //RGB Array 3 wide 54 tall, stores RGB values (There's gotta be a prettier way to do this!)
  {
    100,0,0                                  }
  ,
  {
    100,17,0                                  }
  ,
  {
    100,43,0                                  }
  ,
  {
    100,70,0                                  }
  ,
  {
    100,100,0                                  }
  ,
  {
    70,100,0                                  }
  ,
  {
    43,100,0                                  }
  ,
  {
    17,100,0                                  }
  ,
  {
    0,100,0                                  }
  ,
  {
    0,100,17                                  }
  ,
  {
    0,100,43                                  }
  ,
  {
    0,100,70                                  }
  ,
  {
    0,100,100                                  }
  ,
  {
    0,70,100                                  }
  ,
  {
    0,43,100                                  }
  ,
  {
    0,17,100                                  }
  ,
  {
    0,0,100                                  }
  ,
  {
    17,0,100                                  }
  ,
  {
    43,0,100                                  }
  ,
  {
    70,0,100                                  }
  ,
  {
    100,0,100                                  }
  ,
  {
    100,0,70                                  }
  ,
  {
    100,0,43                                  }
  ,
  {
    100,0,17                                  }
  ,
  //Hue one, a lighter set starts at Array pointer 24 
  {
    100,17,17                                  }
  ,
  {
    100,43,17                                  }
  ,
  {
    100,70,17                                  }
  ,
  {
    100,100,17                                  }
  ,
  {
    70,100,17                                  }
  ,
  {
    43,100,17                                  }
  ,
  {
    17,100,17                                  }
  ,
  {
    17,100,43                                  }
  ,
  {
    17,100,70                                  }
  ,
  {
    17,100,100                                  }
  ,
  {
    17,70,100                                  }
  ,
  {
    17,43,100                                  }
  ,
  {
    17,17,100                                  }
  ,
  {
    43,17,100                                  }
  ,
  {
    70,17,100                                  }
  ,
  {
    100,17,100                                  }
  ,
  {
    100,17,70                                  }
  ,
  {
    100,17,43                                  }
  ,
  //Hue two, Lightest starts at Array pointer 42
  {
    100,43,43                                  }
  ,
  {
    100,70,43                                  }
  ,
  {
    100,100,43                                  }
  ,
  {
    70,100,43                                  }
  ,
  {
    43,100,43                                  }
  ,
  {
    43,100,70                                  }
  ,
  {
    43,100,100                                  }
  ,
  {
    43,70,100                                  }
  ,
  {
    43,43,100                                  }
  ,
  {
    70,43,100                                  }
  ,
  {
    100,43,100                                  }
  ,
  {
    100,43,70                                  }
  ,
};
byte rgbFactor = 100;      // Value to factor by.. IE, 100 would provide 100 steps 255 is Max 
byte randFlicker = 41;     // Value to store a random value used to make the LEDs flicker
int fadeValue = 0;       //tracks the current state of the fade value
boolean fadeDirection = false; // in this case, true means fade up, false means fade down
byte colorMode = 0; // Stores the current color
byte sndMode = 0; // Store the current sound mode
byte menuMode = 0; // Stores the current menuMode
byte impactRead = 0; // Place to store the last read of the impactPin
byte flashOn = 0; // Place to store the count for flashOn
//=================================================
// RGB Timing Variables
long rgbMillis = 0;      // Store millis to keep track of the last time rgb was updated
int rgbInterval = 30;    // How often to update the rgb values
//==========================================
void setup() 
{
  // Set button input pin
  pinMode(buttonPin, INPUT);
  digitalWrite(buttonPin, HIGH );
  pinMode(impactPin, INPUT);
  digitalWrite(impactPin, LOW);
  // Set LED output pins
  pinMode(redPin, OUTPUT);
  digitalWrite(redPin, 0);
  pinMode(grnPin, OUTPUT);
  digitalWrite(grnPin, 0);
  pinMode(bluPin, OUTPUT); 
  digitalWrite(bluPin, 0);
  pinMode(flashPin, OUTPUT); 
  digitalWrite(flashPin, 0);
  colorMode = EEPROM.read(0);  // Read in the stored colorMode
  sndMode = EEPROM.read(1); // Read in the stored sndMode
  digitalWrite (sndSelectPin,sndMode);// Use the stored sndMode to set the sndSelectPin
}
void loop()
{
  // Get button event and act accordingly
  int b = checkButton();
  if (b == 1) clickEvent();
  if (b == 2) doubleClickEvent();
  if (b == 3) holdEvent();

  impactRead = digitalRead (impactPin); // Read the impact pin and dump it in to impactRead
  if (fadeDirection == true && impactRead == HIGH && flashOn == 0){ // Three way check, see if the LED's are on if we have an impact and that we aren't already in impact mode//
    flashOn = 10; // if all those conditions are true set flashOn to 10. flashOn * 30 MS = total flash time
  }
  // Main LED ramp/fade routine
  if (millis () - rgbMillis >= rgbInterval){             // if it's been 30 ms since last time, lets update the LED 
    rgbMillis = millis ();                               // reset so we know the last time 
    if (fadeDirection == true){                          // This is gonna happen if we are fading UP
      if (fadeValue <=249) {                             // If fadeValue is less than or equalls 249
        fadeValue = fadeValue +6;                        // Lets add 6 to it (This number effects how fast it ramps the LED UP)
      }
      else {
        randFlicker = random(30);            
        fadeValue = 255 - randFlicker;                  // or else if it's already there, just make it 255 and flicker it there
      }
    }
    if (fadeDirection == false){                         //This is gonna happen if we are fading down
      if (fadeValue >=7) {                               // if its over or = to 7
        fadeValue = fadeValue - 7;                       // subtract 7 from it (This number effects how fast it ramps the LED DOWN)
      } 
      else {
        fadeValue = 0;                                   // if it's less than 7 hold it at zero
      }
    }
    if (flashOn >= 1){                                  // If we have a flashOn value lets do some flashing
      analogWrite (flashPin, 255 -randFlicker * 8);     // Write the flashPin high and 9X the flicker for crazy flicker madness
      flashOn --;                                       // Decrement by one so we only do this 10 times in a row
    }
    else{
      analogWrite (flashPin, 0);
    }
    updateLED ();
  }
}

Part two... the functions...

// Main LED update routine
void updateLED (){
  analogWrite(redPin, fadeValue * rgbArray[colorMode][0] / rgbFactor);   //Now that the math has been done, update the LED
  analogWrite(bluPin, fadeValue * rgbArray[colorMode][1] / rgbFactor);
  analogWrite(grnPin, fadeValue * rgbArray[colorMode][2] / rgbFactor);
}
//=================================================
// Events to trigger by click and press+hold
void clickEvent() {
  if (menuMode == 0){  //0 is normal on/off saber mode
    fadeDirection = true; // Set fadeDirection and let the Mian ramp routine take care of it. True fades UP
    digitalWrite (sndOnPin,true); //Turns on the soundcard
  } 
  else
    if (menuMode == 1){ //1 is Color setting mode
      colorMode++; // Add 1 to the colormode
      if (colorMode >=54){ // If colorMode is over 53 loop it back to 0
        colorMode = 0;
      }
      updateLED (); // Update the LED with the current colorMode selected
    }
    else
      if (menuMode == 2){ // MenuMode 2 is sound bank setting mode
        sndMode=0;
        digitalWrite (sndSelectPin,sndMode);  // Set the soundbank. 0 is the Jedi soundbank
        delay (10); // A little wait to let it get active
        digitalWrite (sndOnPin,true); // Turn the sound card on (briefly) as an audible indicator
        digitalWrite (redPin,0); // Turn off active colors so you can see the Blue flash
        digitalWrite (grnPin,0);
        digitalWrite (bluPin,0); // Flash the Blue LED a couple of times as a visual indicator
        delay (300);
        digitalWrite (bluPin,1);
        delay (300);
        digitalWrite (bluPin,0);
        delay (300);
        digitalWrite (bluPin,1);
        delay (300);
        digitalWrite (bluPin,0);
        digitalWrite (sndOnPin,false); // Turn off the sound card
      }
}
void doubleClickEvent() {
  if (menuMode == 0){ //0 is normal on/off saber mode
    fadeDirection = false; // Set fadeDirection and let the Mian ramp routine take care of it. False fades DOWN
    digitalWrite (sndOnPin,false); // Turns off the soundcard
  }
  if (menuMode == 1){ // This skips through the RGBArray to select lighter colors
    if (colorMode <= 23){
      colorMode=24; // Go to the start of the medium light colors
    }
    else if (colorMode <=41){
      colorMode=42; // Go to the start of the lightest colors
    }
    else {
      colorMode=0; // Or cycle back to the begining (Most saturated colors)
    }
  }
  if (menuMode == 2){ //2 is Sound setting mode.
    sndMode = 1;
    digitalWrite (sndSelectPin,sndMode); // Set the soundbank. 1 is the Sith soundbank
    delay (10);// A little wait to let it get active
    digitalWrite (sndOnPin,true);// Turn the sound card on (briefly) as an audible indicator
    digitalWrite (redPin,0); // Turn off active colors so you can see the Red flash
    digitalWrite (grnPin,0); // Flash the Red LED a couple of times as a visual indicator
    digitalWrite (bluPin,0);
    delay (300);
    digitalWrite (redPin,1);
    delay (300);
    digitalWrite (redPin,0);
    delay (300);
    digitalWrite (redPin,1);
    delay (300);
    digitalWrite (redPin,0);
    digitalWrite (sndOnPin,false);// Turn off the sound card
  }
}
void holdEvent() { //Triggered with 3 second press hold
  menuMode++; //increment menuMode
  if (menuMode >= 3){ //If it's over 2 loop it back to 0
    menuMode = 0;
  }
  if (menuMode == 0){
    fadeDirection = false;
    digitalWrite (sndOnPin,false);// Turn off the sound card
    digitalWrite (redPin,0);
    digitalWrite (grnPin,0);
    digitalWrite (bluPin,0);
    EEPROM.write(1,sndMode); // Save the selected sndMode... good place to do it sice we are just going to delay anyway and it takes a few MS 
    delay (100);
    digitalWrite (redPin,1); // fast-Flash red pin to show we are going back to normal.
    delay (100);
    digitalWrite (redPin,0);
    digitalWrite (grnPin,1);
    delay (100);
    digitalWrite (grnPin,0);
    digitalWrite (bluPin,1);
    delay (100);
    digitalWrite (bluPin,0);
  }

  if (menuMode == 1){ // Cycle through R G and B to show we are in color mode
    digitalWrite (sndOnPin,false);// Turn off the sound card
    digitalWrite (redPin,0);//All off but red pin
    digitalWrite (grnPin,0);
    digitalWrite (bluPin,0);
    delay (300);
    digitalWrite (grnPin,0);//All off but red pin
    digitalWrite (bluPin,0);
    digitalWrite (redPin,1);
    delay (300);
    digitalWrite (redPin,0);// A little Green pin love
    digitalWrite (grnPin,1);
    delay (300);
    digitalWrite (grnPin,0);// How 'bout some for the Blue already!
    digitalWrite (bluPin,1);
    delay (300);
    digitalWrite (redPin,0);//All off
    digitalWrite (grnPin,0);
    digitalWrite (bluPin,0);
    fadeDirection = true; //Ramp up that LED
  }

  if (menuMode == 2){
    digitalWrite (sndOnPin,false);// Turn off the sound card
    fadeDirection = false;
    fadeValue = 0;
    digitalWrite (redPin,0);
    digitalWrite (grnPin,0);
    digitalWrite (bluPin,0);
    EEPROM.write(0,colorMode); // Save the selected colorMode... good place to do it sice we are just going to delay anyway and it takes a few MS 
    delay (100);
    digitalWrite (grnPin,1); // Flash the green pin to show we are in sound mode.
    delay (300);
    digitalWrite (grnPin,0);
    delay (300);
    digitalWrite (grnPin,1);
    delay (300);
    digitalWrite (grnPin,0);
  }
}
/*
MULTI-CLICK: One Button, Multiple Events
 
 Run checkButton() to retrieve a button event:
 Click
 Double-Click
 Hold
 */
// Button timing variables
int debounce = 20; // ms debounce period to prevent flickering when pressing or releasing the button
int DCgap = 250; // max ms between clicks for a double click event
int holdTime = 2000; // ms hold period: how long to wait for press+hold event
int longHoldTime = 5000; // ms long hold period: how long to wait for press+hold event
// Other button variables
boolean buttonVal = HIGH; // value read from button
boolean buttonLast = HIGH; // buffered value of the button's previous state
boolean DCwaiting = false; // whether we're waiting for a double click (down)
boolean DConUp = false; // whether to register a double click on next release, or whether to wait and click
boolean singleOK = true; // whether it's OK to do a single click
long downTime = -1; // time the button was pressed down
long upTime = -1; // time the button was released
boolean ignoreUp = false; // whether to ignore the button release because the click+hold was triggered
boolean waitForUp = false; // when held, whether to wait for the up event
boolean holdEventPast = false; // whether or not the hold event happened already
int checkButton()
{ 
  int event = 0;
  // Read the state of the button
  buttonVal = digitalRead(buttonPin);
  // Button pressed down
  if (buttonVal == LOW && buttonLast == HIGH && (millis() - upTime) > debounce) {
    downTime = millis();
    ignoreUp = false;
    waitForUp = false;
    singleOK = true;
    holdEventPast = false;
    if ((millis()-upTime) < DCgap && DConUp == false && DCwaiting == true) DConUp = true;
    else DConUp = false;
    DCwaiting = false;
  }
  // Button released
  else if (buttonVal == HIGH && buttonLast == LOW && (millis() - downTime) > debounce) { 
    if (not ignoreUp) {
      upTime = millis();
      if (DConUp == false) DCwaiting = true;
      else {
        event = 2;
        DConUp = false;
        DCwaiting = false;
        singleOK = false;
      }
    }
  }
  // Test for normal click event: DCgap expired
  if ( buttonVal == HIGH && (millis()-upTime) >= DCgap && DCwaiting == true && DConUp == false && singleOK == true) {
    event = 1;
    DCwaiting = false;
  }
  // Test for hold
  if (buttonVal == LOW && (millis() - downTime) >= holdTime) {
    // Trigger "normal" hold
    if (not holdEventPast) {
      event = 3;
      waitForUp = true;
      ignoreUp = true;
      DConUp = false;
      DCwaiting = false;
      //downTime = millis();
      holdEventPast = true;
    }
  }
  buttonLast = buttonVal;
  return event;
}
if (fadeDirection == true){
    }
    if (fadeDirection == false){                         //This is gonna happen if we are fading down

I see a couple of places in your code where you do something like this. The fadeDirection variable is either true or false. It can't be anything else. So, that code should look like this:

if (fadeDirection == true){
    }
    else {                         //This is gonna happen if we are fading down

The code is well commented, and nicely laid out. The use of white space is a bit inconsistent. Up top, you have way more than is needed, to the point where the code is a little hard to read on my notebook computer's small screen.

Then, farther down, the comments butt right up against the code. Adding a little space before the comment helps to see what is code, and what isn't.

You have chosen good names for the variables, so that without much study, the purpose of the code is clear.

The loop function is a bit long. The whole block of code for fading the LED would be a good candidate for another function, and would really shorten up loop.

For the most part, my comments are trivial. I'd give you about a 95 if I was grading this as an assignment.

Generally your code looks very good for a beginner. Layout and readability come with practice and experience.

One constructive critique is your array initialization which, as you put in your comment, could be prettier. I'd do it something like this...

byte rgbArray [54][3]={  //RGB Array 3 wide 54 tall, stores RGB values (There's gotta be a prettier way to do this!)
  {100,0,0},      {100,17,0},      {100,43,0},      {100,70,0},      {100,100,0},
  {70,100,0},      {43,100,0},      {17,100,0},      {0,100,0},      {0,100,17},
  etc....
};

Thanks, I appreciate the help!

PaulS, I see what you mean about the True/False.. I guess it's kind of like being pregnant. You either is, or you isn't.. LOL.

What about other if/else sections? I'm not real sure how if/else differs from just a series of if's? I guess I see the use if you are say checking for 3 out of 5 options but if all your possible options are covered what's the difference?

It's easy enough to move the ramp up/down function to a funtion instead. I can see where that would help to keep the Loop easier to understand.

RoyK, I'll go through and re-do the whitespace a bit. LOL.... I guess it's a newbs over-reliance on the "Auto format" tool in the IDE. I kind assumed it knew better than I did.

Overall no glaring dumb mistakes? It functions correctly so I guess it's "right enough" but I figured I'd see if I could improve anything before I actually built it in to the project.

Thanks again!

In an if/else if/else construct, only one of the clauses will be true. When a true clause is found, the block of code is executed, and the rest of the clauses are skipped.

With a series of if statements, every one needs to be evaluated. For something like you had:

if(b == 1)
  DoThis();
if(b == 2)
  DoThat();
if(b == 3)
  DoSomethingQuick();

the time it takes to evaluate the clauses is insignificant.

For an if statement like this:

if(Factorial(12) > Factorial(4) * Factorial(6) * sqrt(Factorial(14)/3.14159))

the time required to evaluate that statement is not trivial.

Sometimes, it matters. Sometimes it doesn't. It's easier to learn and practice good habits, though, than it is to learn and practice a bad habit, and then have to unlearn that habit.