# Nested for loop used to evaluate a two dimensional array of integers

I am trying to use a nested for loop to evaluate and change numbers within the two dimensional array. Basically these numbers are minutes, or hours. This function is meant to reset the button controlled numbers when they go above 23 (hours) or above 59 (minutes) and also from going below 0. The two dimensional array that I made also has three arrays that need to be skipped (P1, P1 and P1 because they do not contain any time variables. When I click the buttons and increment the numbers nothing unusual happens they just don't bounce back like they are supposed to.

Here is the two dimensional array:

``````int P1[] = { //0-10
{0}, //MODE
{0, 0, 0, 0,}, //TOD variables
// OnHR, OnMin, OffHr, OffMin
{0}, //DOW
{0, 0, 0, 0,},  //Sunday
{0, 0, 0, 0,},    // OnHR, OnMin, OffHr, OffMin
{0, 0, 0, 0,},
{0, 0, 0, 0,},
{0, 0, 0, 0,},
{0, 0, 0, 0,},
{0, 0, 0, 0,},  //Saturday
{0, 0, 0, 0, 0,} // Temperature variables
//Sensor, GreaterThan/LessThan, Temp, Buffer, State
};
``````

Here is the for loop that doesn't seem to work:

``````void HrMinBounce() {
int i = 0;
int q = 0;

for (int i; i < 10; i++) { //iterating down Plug arrays
for (int q; q < 4; q++) {
if (i != 0 || i != 2) { //non time arrays
if (q == 0 || q == 2) { //hours
if (P1[i][q] > 23) {
P1[i][q] = 0;
}
if (P1[i][q] < 0) {
P1[i][q] = 23;
}
}//End of hours
if (q == 1 || q == 3) { //minutes
if (P1[i][q] > 59) {
P1[i][q] = 0;
}
if (P1[i][q] < 0) {
P1[i][q] = 59;
}
}//End of minutes
}
}
}
}
``````

I have 5 of these arrays, so I was hoping to make one loop that could take care of all 5 at once. Here is my previous loop that works but I would have to copy and paste a whole lot to get the result without the extra for loop.

``````void MinHrLimit() {
int i = 0;

for (int i; i < 3; i++) {
if (i == 0 || i == 2) {
if (P1[i] > 23) {
P1[i] = 0;
}
if (P1[i] < 0) {
P1[i] = 23;
}
}
}
}
``````

``````            if(i != 0 || i != 2){ //non time arrays
``````

Let's see.

• If i is 0 then that is true (it isn't 2)
• If i is 1 then that is true (it isn't 0 or 2)
• If i is 2 then that is true (it isn't 0)
• If i is 3 then that is true (it isn't 0 or 2)

In fact that expression is always true. You probably want:

``````            if(i != 0 && i != 2){ //non time arrays
``````

``````   void MinHrLimit(){
int i = 0;

for(int i; i < 3; i++){
``````

You don't need the declaration of `i` there, because it is redefined in the `for` loop. In the `for` loop the variable `i` has an undefined value. You really want:

``````   void MinHrLimit(){

for(int i = 0; i < 3; i++){
``````

Your problem here is not in your code. It's in the data structures: you are doing it the wrong way. Of course, it can work, but not only is your approach wasteful of RAM, it will also inevitably lead to code that is difficult to write, to understand, to debug and to maintain.

You should really try hard to make your data structures as clean as possible. I would argue that clean data is even more important than clean code. If your data mode is clear and makes sense, then clean code flows naturally. If your data model is contrived, your code will always look awkward.

Here is how I would go about structuring your data. First thing: you are using (hour, minute) pairs to represent times. Thus, here is the obvious way to put them:

``````struct Time {
int8_t hour, minute;
};
``````

Notice the variable type: `uint8_t` takes only one byte, and it can represent numbers from −128 to +127. Now, this is the right place to write a function that clamps the numbers to the allowed values. In C, this would be a plain function. But since we are writing C++, it makes more sense to make it a method of the `struct`. Thus:

``````struct Time {
int8_t hour, minute;
void limit() {
if (hour > 23)   hour = 0;
if (hour < 0)    hour = 23;
if (minute > 59) minute = 0;
if (minute < 0)  minute = 59;
}
};
``````

It seems all your times come in pairs, with an “on time” and an “off time”. So here is a data structure for such a pair:

``````struct OnOffTime {
Time on_time, off_time;
void limit() {
on_time.limit();
off_time.limit();
}
};
``````

I added the convenience method `limit()`, so you can limit an `OnOffTime` rather than having to always explicitly limit moth times.

Then, you have a “mode” variable. A mode is normally represented as an `enum`, where each possible mode is given a name. Using names instead of numbers makes everything clearer. For the temperatures and the day of week, I would use the type `uint8_t`, which takes only one byte and can represent a number from 0 to 255. Then, your 2D array becomes an instance of this:

``````struct Settings {
enum { MODE_FOO, MODE_BAR, MODE_BAZ, MODE_QUX } mode;
uint8_t doy;
OnOffTime tod, days;
uint8_t temperatures;
};
``````

You can probably find better names for the fields of the struct. And you should.

So, back to the original problem: how to apply the `limit()` method to all the times in all your 5 settings. Here it goes:

``````const int settings_count = 5;

Settings P[settings_count];

void HrMinBounce()
{
for (int i = 0; i < settings_count; i++) {
P[i].tod.limit();
for (int j = 0; j < 7; j++) {
P[i].days[j].limit();
}
}
}
``````

As you see here, the code is short and easy to understand. This is the benefit of well though data structures: the code that follows is more natural.

a few pointers:

1) your array has unevent number of items. so an array of struct is likely more useful here;

2) use brackets liberally, if you want to be sure of sequence of computation, like:

``````if ((i == 0) || (i == 2)) {
``````

3) if you are going to use a lot of date / time types, think about unix time_t data type. it simplifies coding considerably, and makes your code much more portable.

• I will definitely look into Unix time_t. Thank you! – Tay Dec 1 '17 at 16:09