Code for two bulb fading alternatively

Question

I have written this code

const int A = 3;
const int B = 5;
void setup() {
  pinMode(A, OUTPUT);
  pinMode(B, OUTPUT);

}

void loop() {
  for(int i=1;i<=255;i++)
  {
    int j =255-i;
    analogWrite(A,i);
    analogWrite(B,j);
    delay(100);
  }
  for(int i=255;i>0;i--)
  {
    int j =255-i;
    analogWrite(B,j);
    analogWrite(A,i);
    delay(100);
  }
}

Is this code correct? If yes then can it be modified in any way to make it better?

Answer 1

To keep the two loops more symmetrical, let the first loop go from 0 until 254: [0 .. 255) and the second loop from 255 to 1: [255 .. 0), so both iterations have 255 values each.

Also, it is quite common in programming to include the start element, and not include the end element (as in the values above: [start_element .. end_element).

Also, for the simple formula (255 - i), I would leave out the formula. And to make things more clear define constants for the min/max values (0,255) and the delay.

Than you get:

const int A = 3;
const int B = 5;

const int MIN_INTENSITY = 0;
const int MAX_INTENSITY = 255;
const int DELAY_TIME    = 100;

void setup() {
  pinMode(A, OUTPUT);
  pinMode(B, OUTPUT);
}

void loop() 
{
  for(int i = MIN_INTENSITY; i < MAX_INTENSITY ; i++)
  {
    analogWrite(A, i);
    analogWrite(B, MAX_INTENSITY - i);
    delay(DELAY_TIME);
  }

  for(int i = MAX_INTENSITY ; i > MIN_INTENSITY; i--)
  {
    analogWrite(B, MAX_INTENSITY - i);
    analogWrite(A, i);
    delay(DELAY_TIME);
  }
}

By making a function and use arguments for the variable values, and create some better names for constants and variables you get this:

const int LED_1_PIN     =   3;
const int LED_2_PIN     =   5;

const int MIN_INTENSITY =   0;
const int MAX_INTENSITY = 255;
const int DELAY_TIME    = 100;

void setup()
{
  pinMode(LED_1_PIN, OUTPUT);
  pinMode(LED_2_PIN, OUTPUT);
}

void loop() 
{
  fade(MIN_INTENSITY, MAX_INTENSITY,  1);
  fade(MAX_INTENSITY, MIN_INTENSITY, -1);
}

void fade(int startIntensity, int endIntensity, int stepValue)
{
  for(int intensity = startIntensity; intensity < endIntensity; intensity += stepValue)
  {
    analogWrite(LED_1_PIN, intensity);
    analogWrite(LED_2_PIN, MAX_INTENSITY - intensity);
    delay(DELAY_TIME);
  }
}