Using Arrays Improperly ?

Hey guys, just to begin - I want to thank everyone who's reading and helping others out. This will help me vastly to start my project.
I will have many pins as outputs and inputs, so I manipulated an example code using fors and arrays to help establish my variables to pin numbers.
I am questioning if I am doing this correctly. The variables consist of: variable[0], variable[1] and so on. The point here that I am trying to establish is trying to make the code shorter instead of doing: pinMode(doorclosed2, INPUT);

If I am doing the code wrong, please help me correct my logic. I am very new to programming and have little guidance other than you guys.
I have been told if this code won't work, I can create a .h file and insert it as a header on the top before void Setup (). Thanks again guys !

PS I have named the variables according to their pin distinction.

Akash

Code:

// Variabls to pin numbers: Pins are mapped/numbered in such a way to match pin location in Mega 
int doorclosed[] = { 2, 3, 4, 5, 6 };
int motionsen[] = { 7, 8, 9, 10 };
int keyturned[] = { 11, 12, 13, 14, 15 };
int security[] = { 16, 17, 18 };

int led = 13; // TEST led


void setup()
{
  
  //Set each pin connected to an LED to output mode (pulling high (on) or low (off)
  for(int i = 2; i < 6; i++){         //<--------- doorclosed[2] to doorclosed[6]
      pinMode(doorclosed[i],INPUT); //we use this to set each LED pin to output 
  }

    for(int x = 7; x < 10; x++){         //<--------- motionsen[7] to motionsen[10]
      pinMode(motionsen[x],INPUT); 
  }

    for(int y = 11; y < 15; y++){         // <--------- keyturned[11] to keyturned[15]
      pinMode(keyturned[y],INPUT); 
  }
  
      for(int q = 16; q < 18; q++){         // <--------- security[16] to security[18]
      pinMode(security[q],INPUT); 
  }


  pinMode(led, OUTPUT);  // Test LED Pin Map
  
}

Close but you are confusing the contents of the array with the indexes of the contents. Each for loop should start at 0 and count until < sizeof(array), for example

  for(int i = 0; i < sizeof(doorclosed); i++){       
      pinMode([i],INPUT); 
  }

Rob

i, x, y, and q should all start at 0 and end at the array length. The numbers in the array are the pin numbers; not the ones in the for loop iterator.

Also, the iterator is almost always called i. So change the names of x, y, and q.

Graynomad:

  for(int i = 0; i < sizeof(doorclosed); i++){

A small point — sizeof() returns the number of bytes that the array occupies, not the number of elements in the array. It will work as long as the array elements are one byte in size, but it carries risk.

starforgelabs:
It will work as long as the array elements are one byte in size

... which is not the case here. So that won't work. I usually have a separate variable containing the size rather than using the sizeof operator

Oops you're right, I missed the "int", type in hast, repent at leisure :slight_smile:

I normally do this then.

#define DOOR_ARRAY_SIZE (sizeof(doorclosed) / sizeof(doorclosed[0]))

for(int i = 0; i < DOOR_ARRAY_SIZE; i++){       
      pinMode(doorclosed[i],INPUT); 
}

Rob

For those kinds of array, that usually works great.

Another common approach (which I won't argue is necessarily better) is to declare the size explicitly.

#define DOOR_CLOSED_SIZE 5
int doorclosed[DOOR_CLOSED_SIZE] = {2, 3, 4, 5, 6};
...
  for(int i=0; i<DOOR_CLOSED_SIZE; i++) 
    pinMode(doorclosed[i], INPUT);

Among younger coders I see the same idea but using static const int:

static const int DoorClosedSize = 5;

int doorclosed[DoorClosedSize] = {2, 3, 4, 5, 6};
...
  for(int i=0; i<DoorClosedSize; i++) 
    pinMode(doorclosed[i], INPUT);

For the OP:

Regardless of which of the three specific variations on the idea you choose, the idea is sound.

One is code clarity; it eliminates magic numbers in your for() loops, for example.

Another is code safety if you change the size of the array, the compiler ensures you really have that number of elements in the array, but more importantly, all of your for() loops still work correctly.

Mind you, since the array contents are small, positive values < 256, I'd implement all the arrays as

uint8_t myarray[] = {...}

rather than int, to conserve ram. Good habit to get into, particularly with arrays.

static const int:

That is becoming more common, mostly I think because it allows the compiler to flag errors. I admit I'm starting to do that as well these days.

I still think the "constants in UPPER_CASE" rule/guideline should apply though regardless of how they are defined.


Rob

Graynomad:
I still think the "constants in UPPER_CASE" rule/guideline should apply though regardless of how they are defined.

This is a good point of C style, IMHO. Thanks for pointing that out.

(However, I do contract software work so I have to match the code style of the client I'm working with, so I slip quite easily into camel case.)

starforgelabs:
For those kinds of array, that usually works great.

Another common approach (which I won't argue is necessarily better) is to declare the size explicitly.

#define DOOR_CLOSED_SIZE 5

int doorclosed[DOOR_CLOSED_SIZE] = {2, 3, 4, 5, 6};
...
 for(int i=0; i<DOOR_CLOSED_SIZE; i++)
   pinMode(doorclosed[i], INPUT);




Among younger coders I see the same idea but using **static const int**: 



static const int DoorClosedSize = 5;

int doorclosed[DoorClosedSize] = {2, 3, 4, 5, 6};
...
 for(int i=0; i<DoorClosedSize; i++)
   pinMode(doorclosed[i], INPUT);





For the OP: 

Regardless of which of the three specific variations on the idea you choose, the idea is sound. 

One is code clarity; it eliminates magic numbers in your for() loops, for example. 

Another is code safety if you change the size of the array, the compiler ensures you really have that number of elements in the array, but more importantly, all of your for() loops still work correctly.

The magic number is different than the array size, for mine it is for when the next set of variables will start taking up the next set of pins. Is that okay ?

akashroy:
The magic number is different than the array size, for mine it is for when the next set of variables will start taking up the next set of pins. Is that okay ?

Your original approach was flawed, so I'd say no.

If you store the pin numbers in the arrays, traverse the array indices, which go from zero to one less than the size of the array.

Then get the pin number that exists in each array element using the index (myArray[ i ]).

Notice that the code traversing the arrays is nearly identical below. (Not compiled; there may be typos.)

// Variabls to pin numbers: Pins are mapped/numbered in such a way to match pin location in Mega 

static const byte DOOR_CLOSED_SIZE = 5;
byte doorclosed[DOOR_CLOSED_SIZE] = { 2, 3, 4, 5, 6 };

static const byte MOTION_SEN_SIZE = 4;
byte motionsen[MOTION_SEN_SIZE] = { 7, 8, 9, 10 };

static const byte KEY_TURNED_SIZE = 5;
byte keyturned[KEY_TURNED_SIZE] = { 11, 12, 13, 14, 15 };

static const byte SECURITY_SIZE = 3;
byte security[SECURITY_SIZE] = { 16, 17, 18 };

int led = 13; // TEST led


void setup()
{
  
  //Set each pin connected to an LED to output mode (pulling high (on) or low (off)

  for(byte i=0; i<DOOR_CLOSED_SIZE; i++)
      pinMode(doorclosed[i],INPUT); //we use this to set each LED pin to output 

  for(byte i=0; i<MOTION_SEN_SIZE; i++)
      pinMode(motionsen[i],INPUT); //we use this to set each LED pin to output 

  for(byte i=0; i<KEY_TURNED_SIZE; i++)
      pinMode(keyturned[i],INPUT); //we use this to set each LED pin to output 

  for(byte i=0; i<SECURITY_SIZE; i++)
      pinMode(security[i],INPUT); //we use this to set each LED pin to output 

  pinMode(led, OUTPUT);  // Test LED Pin Map

Okay, thank you very much, now I can get a good nights sleep knowing this part of the project is understood !
Thank you !

Graynomad:
Oops you're right, I missed the "int", type in hast, repent at leisure :slight_smile:

I normally do this then.

#define DOOR_ARRAY_SIZE (sizeof(doorclosed) / sizeof(doorclosed[0]))

for(int i = 0; i < DOOR_ARRAY_SIZE; i++){       
      pinMode(doorclosed[i],INPUT);
}

You are best off having a generic "number of items in an array" define, rather than making them over and over.

// number of items in an array
#define NUMITEMS(arg) ((unsigned int) (sizeof (arg) / sizeof (arg [0])))

Now you can say:

for (int i = 0; i < NUMITEMS (doorclosed); i++)
 {       
      pinMode (doorclosed [i],INPUT); 
  }

It's immediately obvious now that we are getting the number of items in this array (and not some other one).

Yep, good macro and clearer than having a bunch of sizeof()s somewhere else in the code.


Rob

int keyturned[] = { 11, 12, 13, 14, 15 };
int security[] = { 16, 17, 18 };

int led = 13; // TEST led

I suspect that that is going to create problems down the line.

On Friday?