ParseInt values for printing onto ssd1306 OLED code looks duck-taped together

Hi all,

I have been learning arduino on my own through browsing this forum, arduino official tutorial, youtube etc. Finally pieced together the following simple code. My question is: How can i improve the code?

Because, it seems overly drawn out to me(For a sketch to display only one string and 3 ints to a screen).

Looking forward to your response, thanks in advance.

/*Purpose was to integrate my understanding of arrays, while loop, for loop, switch-case statement
 * serial buffering, and the SSD1306 OLED hardware.
 */


#include <SPI.h>
#include <Wire.h>
#include <Adafruit_GFX.h>
#include <Adafruit_SSD1306.h>

#define OLED_RESET 4
Adafruit_SSD1306 display(OLED_RESET);

int myArray[4] = {};

void setup() {
  Serial.begin(9600);
  display.begin(SSD1306_SWITCHCAPVCC, 0x3C);  
  display.display();                            //Display adafruit logo. love them btw!
  delay(2000);

//Reset screen to blank slate, in preparation of next task
  display.clearDisplay();
  display.display();

//Prompt user for input at serial monitor
  Serial.println(F("Type 3 numbers, separated by ','"));
  display.setTextSize(1);                       //I noticed this could be declared once if no change expected.
  display.setTextColor(WHITE);                  //declared once as well.
  display.setCursor(0,0);
  display.println("Type 3 numbers,");
  display.setCursor(0,10);
  display.println("separated by ','");
  display.display();
}

void loop() {
  while(Serial.available()>0){
  display.clearDisplay();
  display.display(); 
  for(int i=0; i<3; i++){
  myArray[i] = Serial.parseInt();
  }

//Print numbers to serial monitor
  Serial.println(F("Numbers typed are:"));
  for(int i=0; i<3; i++){
  Serial.println(myArray[i]);
  }

//Print numbers onto OLED screen
  display.setCursor(0,0);
  display.println("Numbers typed are:");
  display.display();
  for(int x=10; x<31; x++){
  display.setCursor(0,x);
  switch (x){
    case 10:
       display.println(myArray[0]);
       display.display(); 
        break;
    case 20:
       display.println(myArray[1]);
       display.display(); 
        break; 
    case 30:
       display.println(myArray[2]);
       display.display(); 
        break;
          }
      }
  }

Please always do a Tools > Auto Format on your code before posting it. This will make it easier for you to spot bugs and make it easier for us to read.

My recommendation is to use this type of structure:

if(Serial.available() > 0) {
  // do all the stuff that only needs to be done once when the numbers are received
  for (int i=0; i<3; i++) {
    myArray[i] = Serial.parseInt();
    // print the received number to the Serial Monitor and OLED
  }
}
int myArray[4] = {};

If you are mot going to provide values, you look silly putting the empty {} after the =.

  for(int x=10; x<31; x++){
  display.setCursor(0,x);
  switch (x){
    case 10:
       display.println(myArray[0]);
       display.display();
        break;
    case 20:
       display.println(myArray[1]);
       display.display();
        break;
    case 30:
       display.println(myArray[2]);
       display.display();
        break;
          }
      }
  }

Using a for loop, where no two iterations do the same thing is stupid. The switch statement is useless, too. Just put the code in the switch statements.

pert:
Please always do a Tools > Auto Format on your code before posting it. This will make it easier for you to spot bugs and make it easier for us to read.

My recommendation is to use this type of structure:

if(Serial.available() > 0) {

// do all the stuff that only needs to be done once when the numbers are received
 for (int i=0; i<3; i++) {
   myArray[i] = Serial.parseInt();
   // print the received number to the Serial Monitor and OLED
 }
}

Ah, auto fomatter :wink: . thank you for the feedback!

PaulS:

int myArray[4] = {};

If you are mot going to provide values, you look silly putting the empty {} after the =.

Thank you for the information!

PaulS:

 for(int x=10; x<31; x++){

display.setCursor(0,x);
 switch (x){
   case 10:
      display.println(myArray[0]);
      display.display();
       break;
   case 20:
      display.println(myArray[1]);
      display.display();
       break;
   case 30:
      display.println(myArray[2]);
      display.display();
       break;
         }
     }
 }


Using a for loop, where no two iterations do the same thing is stupid. The switch statement is useless, too. Just put the code in the switch statements.

Yes actually that makes total sense. I learnt something new today, thank you!