Newbie, is there a better, more simple way to code this?

Hello all! I am a near complete noob working my way through a tutorial book for Arduino. I jut wrote this code for simply flashing “S.O.S.” using the onboard LED of the Uno. The code works fine, however as I have a tendency to over-complicate everything, I am curious what a simpler / more sleek way may have been to write the code? Thank you all in advance for any insight!

void setup() {
  // put your setup code here, to run once:
pinMode(LED_BUILTIN, OUTPUT);

}

int i = 0;
bool doneFlag = false;

void loop() {

  // put your main code here, to run repeatedly:
if (i <= 2){
digitalWrite(LED_BUILTIN, HIGH);
delay (250);
digitalWrite(LED_BUILTIN, LOW);
delay (250);
i = ++i;
}

else if (i >=3 && i <= 5){
  
  if (doneFlag == false){
    delay(500);
    doneFlag = !doneFlag;
  }
  
digitalWrite(LED_BUILTIN, HIGH);
delay (500);
digitalWrite(LED_BUILTIN, LOW);
delay (500);
i = ++i;
}

else if (i >=6 && i <= 8){

 if (doneFlag){
    delay(500);
    doneFlag = !doneFlag;
  }
  
digitalWrite(LED_BUILTIN, HIGH);
delay (250);
digitalWrite(LED_BUILTIN, LOW);
delay (250);
i = ++i;
}

else if (i = 9){
delay (3000);
i = 0;
}
[code]

Look at examples of a "for" loop.

Paul

if (i = 9)

Oops

i = ++i;

"i++;" is much shorter.

Give this a shot
Perhaps using arrays will help you in more ways than you ask…
I didn’t compile or run this code - but is guaranteed 99% valid - maybe some debugging for you!
Certainly room for improvement !

// just so we can use words down below...
#define dit 0
#define dah 1
#define gap 2
#define ON HIGH
#define OFF LOW

// Your char array can be any message - or multiple preefined messages in a 2D array
// ---------------------------------------------------------------------------------------
uint8_t morse_string[] = {dit, dit, dit, gap, dah, dah, dah, gap, dit, dit, dit }; // SOS
// ---------------------------------------------------------------------------------------
// better still you could create a ascii->morse conversion function

// we'll use delay here - to simplify the explanation,
// but should rewrite using millis() timing
int dit_duration = 50; // milliseconds duration of dit
int dah_duration = 100; // milliseconds duration of dah
int gap_interval = 100; // milliseconds between dits & dahs
int pause_after = 500; // milliseconds between morse words
int bit_duration; // used in loop()

void setup() {
  // setup your I/O and other runtime values
 pinMode(LED_BUILTIN, OUTPUT);
}

void loop() {
 // sizeof() works here because morse_string][ is 8-bit bytes
 for (int counter = 0; counter < sizeof(morse_string); counter++) {
 switch (morse_string[counter]) {
 case dit:
 bit_duration = dit_duration;
 break;
 case dah:
 bit_duration = dah_duration;
 break;
 }
 if (morse_string[counter] < gap) { // is a dit or dah
 digitalWrite(LED_BUILTIN, ON);
 delay(bit_duration); // hold dit/dah as long as required
 digitalWrite(LED_BUILTIN, OFF);
 delay(gap_interval); // gap between dit & dahs
 } else {
 delay(gap_interval); // extra gap between characters
 }
 }
 delay(pause_after); // rest between words
}

For a very specific case like this, you can also try:

#define UNIT            200
#define INTER           (1*UNIT)
#define DIT             (1*UNIT)
#define DAH             (3*UNIT)
#define LETTER          (3*UNIT)
#define WORD            (7*UNIT)
#define MESSAGE         3000

unsigned long
    Delays[] = 
    {
        DIT, DIT, DIT,
        DAH, DAH, DAH,
        DIT, DIT, DIT
    };

void setup() 
{
  pinMode( LED_BUILTIN, OUTPUT );
  digitalWrite( LED_BUILTIN, LOW );
  delay(500);
}

void loop() 
{
    for( int i=0; i<3; i++ )
    {
        for( int j=0; j<3; j++ )
        {
            digitalWrite( LED_BUILTIN, HIGH );
            //on time for dit or dah
            delay( Delays[(i*3)+j] );
            //off time between dits/dahs of letter
            digitalWrite( LED_BUILTIN, LOW );
            delay( INTER );
            
        }//for
        
        //time between letters
        delay( LETTER );
        
    }//for

    //time between messages
    delay( MESSAGE );
    
}//loop

Paul_KD7HB:
Look at examples of a “for” loop.

Paul

I will do that Paul, thank you!

AWOL:

if (i = 9)

Oops

i = ++i;

“i++;” is much shorter.

Ah! I didn’t know I could do that! This is precisely the type of stuff I was looking for! Thank you!

lastchancename:
Give this a shot
Perhaps using arrays will help you in more ways than you ask…
I didn’t compile or run this code - but is guaranteed 99% valid - maybe some debugging for you!
Certainly room for improvement !

// just so we can use words down below...

#define dit 0
#define dah 1
#define gap 2
#define ON HIGH
#define OFF LOW

// Your char array can be any message - or multiple preefined messages in a 2D array
// ---------------------------------------------------------------------------------------
uint8_t morse_string = {dit, dit, dit, gap, dah, dah, dah, gap, dit, dit, dit }; // SOS
// ---------------------------------------------------------------------------------------
// better still you could create a ascii->morse conversion function

// we’ll use delay here - to simplify the explanation,
// but should rewrite using millis() timing
int dit_duration = 50; // milliseconds duration of dit
int dah_duration = 100; // milliseconds duration of dah
int gap_interval = 100; // milliseconds between dits & dahs
int pause_after = 500; // milliseconds between morse words
int bit_duration; // used in loop()

void setup() {
 // setup your I/O and other runtime values
pinMode(LED_BUILTIN, OUTPUT);
}

void loop() {
// sizeof() works here because morse_string][ is 8-bit bytes
for (int counter = 0; counter < sizeof(morse_string); counter++) {
switch (morse_string[counter]) {
case dit:
bit_duration = dit_duration;
break;
case dah:
bit_duration = dah_duration;
break;
}
if (morse_string[counter] < gap) { // is a dit or dah
digitalWrite(LED_BUILTIN, ON);
delay(bit_duration); // hold dit/dah as long as required
digitalWrite(LED_BUILTIN, OFF);
delay(gap_interval); // gap between dit & dahs
} else {
delay(gap_interval); // extra gap between characters
}
}
delay(pause_after); // rest between words
}

Thank you for all that! It did indeed compile on the first try! I will have to research the array, switch / case / break & “for” functions, as someone suggested earlier. Thank you again, you have given me much to work on!

Blackfin:
For a very specific case like this, you can also try:

#define UNIT            200

#define INTER           (1UNIT)
#define DIT             (1
UNIT)
#define DAH             (3UNIT)
#define LETTER          (3
UNIT)
#define WORD            (7*UNIT)
#define MESSAGE         3000

unsigned long
   Delays =
   {
       DIT, DIT, DIT,
       DAH, DAH, DAH,
       DIT, DIT, DIT
   };

void setup()
{
 pinMode( LED_BUILTIN, OUTPUT );
 digitalWrite( LED_BUILTIN, LOW );
 delay(500);
}

void loop()
{
   for( int i=0; i<3; i++ )
   {
       for( int j=0; j<3; j++ )
       {
           digitalWrite( LED_BUILTIN, HIGH );
           //on time for dit or dah
           delay( Delays[(i*3)+j] );
           //off time between dits/dahs of letter
           digitalWrite( LED_BUILTIN, LOW );
           delay( INTER );
           
       }//for
       
       //time between letters
       delay( LETTER );
       
   }//for

//time between messages
   delay( MESSAGE );
   
}//loop

Thank you @Blackfin I will dig into this as well! Much to research!