ways to shorten repeating code in void loop.

hello. I am new to programming and i am sorry if this is posted elsewhere. I have been looking for last few days. Been watching videos etc But there is something i am not understanding. I seem to just be repeating the same code over and over in programmes. I started using arrays and for loops to clean up the code. Its all good for the setup section. But when i comes to the loop section i seem to be having difficulty.

At the moment i am just trying to set up eight pushbuttons to each turn on an LED.

Below is the long code that i am trying to clean up. As you can see the loop just repeats the same statement over and over. But the programme works as expected.

int leds[] = {9, 8, 7, 6, 5, 4, 3, 2};
int buttons[] = {10, 11, 12, A0, A1, A2, A3, A4};


// variables will change:
int buttonState1 = 0;         // variable for reading the pushbutton status
int buttonState2 = 0;
int buttonState3 = 0;
int buttonState4 = 0;
int buttonState5 = 0;
int buttonState6 = 0;
int buttonState7 = 0;
int buttonState8 = 0;

void setup() {
  for(int i; i<10; i++)
{
pinMode(leds[i], OUTPUT);
pinMode(buttons[i], INPUT);
}

  
}

void loop() {
    buttonState1 = digitalRead(buttons[0]);     // read the state of the pushbutton value:
    if (buttonState1 == HIGH) {                 // check if the pushbutton is pressed. If it is, the buttonState is HIGH:
    digitalWrite(leds[0], HIGH);               // turn LED on:
  } else {
    digitalWrite(leds[0], LOW);                 // turn LED off:
  }
   buttonState2 = digitalRead(buttons[1]);     // read the state of the pushbutton value:
    if (buttonState2 == HIGH) {                 // check if the pushbutton is pressed. If it is, the buttonState is HIGH:
    digitalWrite(leds[1], HIGH);               // turn LED on:
  } else {
    digitalWrite(leds[1], LOW);                 // turn LED off:
  }
   buttonState3 = digitalRead(buttons[2]);     // read the state of the pushbutton value:
    if (buttonState3 == HIGH) {                 // check if the pushbutton is pressed. If it is, the buttonState is HIGH:
    digitalWrite(leds[2], HIGH);               // turn LED on:
  } else {
    digitalWrite(leds[2], LOW);                 // turn LED off:
  }
   buttonState4 = digitalRead(buttons[3]);     // read the state of the pushbutton value:
    if (buttonState4 == HIGH) {                 // check if the pushbutton is pressed. If it is, the buttonState is HIGH:
    digitalWrite(leds[3], HIGH);               // turn LED on:
  } else {
    digitalWrite(leds[3], LOW);                 // turn LED off:
  }
   buttonState5 = digitalRead(buttons[4]);     // read the state of the pushbutton value:
    if (buttonState5 == HIGH) {                 // check if the pushbutton is pressed. If it is, the buttonState is HIGH:
    digitalWrite(leds[4], HIGH);               // turn LED on:
  } else {
    digitalWrite(leds[4], LOW);                 // turn LED off:
  }
   buttonState6 = digitalRead(buttons[5]);     // read the state of the pushbutton value:
    if (buttonState6 == HIGH) {                 // check if the pushbutton is pressed. If it is, the buttonState is HIGH:
    digitalWrite(leds[5], HIGH);               // turn LED on:
  } else {
    digitalWrite(leds[5], LOW);                 // turn LED off:
  }
   buttonState7 = digitalRead(buttons[6]);     // read the state of the pushbutton value:
    if (buttonState7 == HIGH) {                 // check if the pushbutton is pressed. If it is, the buttonState is HIGH:
    digitalWrite(leds[6], HIGH);               // turn LED on:
  } else {
    digitalWrite(leds[6], LOW);                 // turn LED off:
  }
   buttonState8 = digitalRead(buttons[7]);     // read the state of the pushbutton value:
    if (buttonState8 == HIGH) {                 // check if the pushbutton is pressed. If it is, the buttonState is HIGH:
    digitalWrite(leds[7], HIGH);               // turn LED on:
  } else {
    digitalWrite(leds[7], LOW);                 // turn LED off:
  }
}

So amongst other things i tried this. (this one made the most sense to me)

int leds[] = {9, 8, 7, 6, 5, 4, 3, 2};
int buttons[] = {10, 11, 12, A0, A1, A2, A3, A4};
int buttonState[8];

void setup() {
  for(int i; i<8; i++)
{
pinMode(leds[i], OUTPUT);
pinMode(buttons[i], INPUT);
} 
}

void loop() {
   for(int i; i <8; i++)
{
  
    buttonState[i] = digitalRead(buttons[i]);     // read the state of the pushbutton value:
      for(int i; i<8; i++)
{
    if (buttonState[i] == HIGH) {                 // check if the pushbutton is pressed. If it is, the buttonState is HIGH:
    digitalWrite(leds[i], HIGH);               // turn LED on:
  } else {
    digitalWrite(leds[i], LOW);                 // turn LED off:
  }
  }
}
}

This however, did not work.

Can anyone offer some suggestions or tell me where i am going wrong please.

Thank you.

  for(int i; i<8; i++)

No initialization?

Your inputs are probably "floating" when the buttons are not pressed and might show "ghost" presses, connect one side of the button to GND and the other side to the input pin. In setup(), set the pinMode() to INPUT_PULLUP, now the digitalRead()will be LOW when the button is pressed and HIGH when not, and it's not floating because the internal PULLUP resistor is holding it HIGH when not pressed. Try this version:

int leds[] = {9, 8, 7, 6, 5, 4, 3, 2};
int buttons[] = {10, 11, 12, A0, A1, A2, A3, A4};
int buttonState[8];

void setup() {
  for (int i = 0; i < 8; i++)
  {
    pinMode(leds[i], OUTPUT);
    pinMode(buttons[i], INPUT_PULLUP);
  }
}

void loop() {
  for (int i = 0; i < 8; i++)
  {
  buttonState[i] = digitalRead(buttons[i]);     // read the state of the pushbutton value:
    for (int i = 0; i < 8; i++)
    {
      if (buttonState[i] == LOW) {                 // check if the pushbutton is pressed. If it is, the buttonState is LOW:
        digitalWrite(leds[i], HIGH);               // turn LED on:
      } else {
        digitalWrite(leds[i], LOW);                 // turn LED off:
      }
    }
  }
}

dionnaki:
This however, did not work.

This is the single most useless piece of information. Yet I see it all the time on this forum. If you want people to try to help you by figuring out what it is doing you need to give that information.

What did/didn't it do that you expected it to do? What happened when you pressed the buttons? What happened when you didn't press the buttons.

Metallor:
This is the single most useless piece of information.

I agree with you but I think it is kinder to say something like "did not work does not convey any useful information from which to help you"

...R

I don't think I would use the same counting variable within embedded for Loops. Even if it compiles (which it shouldn't) it is ambiguous at best. I also think that you only Need one for Loop to handle everything. If buttonState contains LOW/HIGH, it probably should be of type boolean instead of int.

It really doesn't make much sense to test if a variable is HIGH and set another variable LOW and vice versa. Use the NOT Operator which does the same Thing with much less code.

Here's my two Cents.

int leds[] = {9, 8, 7, 6, 5, 4, 3, 2};
int buttons[] = {10, 11, 12, A0, A1, A2, A3, A4};
boolean state[8];

void setup() {
  for(int i; i<8; i++)
  {  pinMode(leds[i], OUTPUT);
     pinMode(buttons[i], INPUT);
     state[i] = LOW;
  } 
}

void loop() {
   for(int i; i <8; i++)
   { if(digitalRead(buttons[i]);       // read the pushbutton
     { // The button was pressed
       state[i] = !state[i];           // Flip the state ON/OFF
       digitalWrite(leds[i], state[i]; // Set the LED ON/OFF
     }
   }
}

Please note that debouncing the Buttons still hasn't been addressed.

As I said. I am just starting with Arduino. So please forgive any stupid statements or questions.

First of all:

The first code uploaded and worked as expected. I'd press a button and the corresponding led would light up. The second code uploaded. But when I pressed buttons. They did not light up the LEDs.

Coding badly can you explain no initialisation please. I was under there impression that the statement, (int i;..) did this.

Outsider: my inputs pins have a 10k pulldown resistor to ground. Should that prevent ghosting?

Jaba: thanks for your suggestions and code. Can you explain your statement about how Ian using the same counting variable? My understanding was that the counters were different because they were in different brackets. I also have not got as far as using the Boolean function so thanks for suggesting this and I will research this function more. I will also try your code when I get back. I was also going to hardware bounce the switches with some D flip flops. Do you think it would be better doing with software? I've gone through the debounce tutorial but I have not utilized it in a project yet.

Thanks again everyone.

dionnaki:
Coding badly can you explain no initialisation please. I was under there impression that the statement, (int i;..) did this.

No, an initialization is where you assign an initial value to your variable. You have just declared to the compiler, that there is a variable i. The content of i will be undefined. The correct way would be: int i = 0; i < ...

dionnaki:
Jaba: thanks for your suggestions and code. Can you explain your statement about how Ian using the same counting variable? My understanding was that the counters were different because they were in different brackets.

This is correct and it will compile just fine. However, as others have said, your inner for-loop makes no sense at all. You could either use two for loops, first read all the sensors and then test all read values in a new for loop. However, this approach is pretty inefficient, because you can also test the value directly after reading it.

dionnaki:
I was also going to hardware bounce the switches with some D flip flops. Do you think it would be better doing with software? I've gone through the debounce tutorial but I have not utilized it in a project yet.

That is up to you, but in most cases software debouncing is enough. You should try software debouncing and if your software is to busy for that go to hardware debouncing.

Light UC. What is my inner for loop?

With this code I thought I did have 2 loops.

for(int i; i <8; i++)
{
 
   buttonState[i] = digitalRead(buttons[i]);

Read button state from buttons.

for(int i; i<8; i++)
{
   if (buttonState[i] == HIGH) {                 
   digitalWrite(leds[i], HIGH);

If button state is high write from led array.

This is not correct?

I was refering to your original code (the second one). The scope of your for-loop is defined by the {} brackets. Inside of these you have another loop, that is your inner loop. Like JaBa already said this inner loop doesn't make sense in the way you use it.
What you have:

Read button 0
For all buttons (0-7) do:
    check state and toggle led

Read button 1
For all buttons (0-7) do:
    check state and toggle led

Read button 2
For all buttons (0-7) do:
    check state and toggle led

Read button 3
For all buttons (0-7) do:
    check state and toggle led

Read button 4
For all buttons (0-7) do:
    check state and toggle led

Read button 5
For all buttons (0-7) do:
    check state and toggle led

What I think you want:

Read button 0
For all buttons (0-7) do:
    check state and toggle led

Read button 1
For all buttons (0-7) do:
    check state and toggle led

Read button 2
For all buttons (0-7) do:
    check state and toggle led

Read button 3
For all buttons (0-7) do:
    check state and toggle led

Read button 4
For all buttons (0-7) do:
    check state and toggle led

Read button 5
For all buttons (0-7) do:
    check state and toggle led

What I think you want:

For all buttons (0-7) do:
    Read button state
    Check state and toggle led

Which is the same as:

For all buttons (0-7) do:
    Store button state

For all button states (0-7) do:
    Check state and toggle led

To clarify what I said about using the same variable in embedded for Loops. I was referring to the code in the second program of the original post which was also included in post #2 (from outsider).

  for (int i = 0; i < 8; i++)                                // Let's call this Loop 1
  {  buttonState[i] = digitalRead(buttons[i]);
      for (int i = 0; i < 8; i++)                            // Let's call this Loop 2 and it is embedded within Loop 1
      {    if (buttonState[i] == LOW) {                 // Does this i refer to i from Loop 1 or Loop 2?
       ...
      }
   }

As I am not sitting at an Arduino IDE, I won't argue about if it will compile or not. I do consider it to be ambiguous even if the IDE is compensating for it and making some sort of assumption which allows it to be technically correct. If one of my Trainees did this, I wouldn't let it go through even if it is correct because ambiguity makes for code that is hard to understand/maintain.

It's not ambiguous at all, it's standard C++. The inner declaration of i masks the outer one. If you don't need access to the outer loop counter, it's perfectly legitimate.

Jaba: I just tried your code. It did not compile. I had 2 brackets to the end of 2 different lines. It just lit up all of the 8 less. But pushing the buttons did not affect them. Did i do something wrong.

This is the code i loaded.

int leds[] = {9, 8, 7, 6, 5, 4, 3, 2};
int buttons[] = {10, 11, 12, A0, A1, A2, A3, A4};
boolean state[8];

void setup() {
  for(int i; i<8; i++)
  {  pinMode(leds[i], OUTPUT);
     pinMode(buttons[i], INPUT);
     state[i] = LOW;
  } 
}

void loop() {
   for(int i; i <8; i++)
   { if(digitalRead(buttons[i]));       // read the pushbutton
     { // The button was pressed
       state[i] = !state[i];           // Flip the state ON/OFF
       digitalWrite(leds[i], state[i]); // Set the LED ON/OFF
     }
   }
}

coding badly. I added the 0 to the i in the for loop, and the code now functions as expected. Thank you very much.

This is the new code:

int leds[] = {9, 8, 7, 6, 5, 4, 3, 2};
int buttons[] = {10, 11, 12, A0, A1, A2, A3, A4};
int buttonState[8];

void setup() {
 for(int i; i<8; i++)
{
pinMode(leds[i], OUTPUT);
pinMode(buttons[i], INPUT);
} 
}

void loop() {
  for(int i = 0; i <8; i++)
{
 
   buttonState[i] = digitalRead(buttons[i]);     // read the state of the pushbutton value:
     for(int i = 0; i<8; i++)
{
   if (buttonState[i] == HIGH) {                 // check if the pushbutton is pressed. If it is, the buttonState is HIGH:
   digitalWrite(leds[i], HIGH);               // turn LED on:
 } else {
   digitalWrite(leds[i], LOW);                 // turn LED off:
 }
 }
}
}
void setup() {
 for(int i; i<8; i++)

oops

  for(int i = 0; i <8; i++)
{
 
   buttonState[i] = digitalRead(buttons[i]);     // read the state of the pushbutton value:
     for(int i = 0; i<8; i++)
{

and again.

   if (buttonState[i] == HIGH) {                 // check if the pushbutton is pressed. If it is, the buttonState is HIGH:
   digitalWrite(leds[i], HIGH);               // turn LED on:
 } else {
   digitalWrite(leds[i], LOW);                 // turn LED off:
 }

Aka

digitalWrite(leds[i], buttonState[i]);

Oops. My bad. I didn't catch the for(int i; when I copied the code. Good Thing I admitted I was not sitting at an IDE...

Thanks for the catch and I'm glad it's now working as expected.

Yeah thanks to the forum and the people who replied. You were alot of help.

AWOL. Should my code have worked with those errors. It did. I have fixed the errors now. Still works. Also, thanks for the shorthand code. Makes things easier.

dionnaki:
AWOL. Should my code have worked with those errors. It did.

You got lucky.

Don't rely on mere fortune.

Yeah do not want to rely on luck. That's why I went in changed it. This is my first week of coding so I am still learning. I actually have no clue what I'm doing.

digitalWrite(leds[i], buttonState[i]);

I am reading it. turn LEDs and button state high. Does that work the same as if button state is high turn digital pin high. With no if statement is there anything to compare against. Like how does it know when push button is not pressed. How does it know what the buttonstate is?