Dividing pointer char array to other arrays

Question

When I printed it I got error like this 17:1733╠╠╠╠╠╠╠╠17:╠╠. I couldn't figure it out. I would appreciate if you solve and give me a better approach? Thanks for your help.

#include "stdafx.h"
#include <iostream>
#include <conio.h>
#include <cstdlib>
#include <iomanip>
#include <string>
using namespace std;

int main()
{


    char* time = "173324";

    char holdh[3];
    char holdM[3];
    char holds[3];
    holdh[2] = '\0';
    holdM[2] = '\0';
    holds[2] = '\0';

    int t;
    for (t = 0; t < 6;t++)
    {
        if (t < 2)
            holdh[t] = *(time + t);
        else if (2 <= t < 4) {
            t = t - 2;
            holdM[t] = *(time + t);
            t = t + 2;
        }
        else if (4 <= t < 6)
        {
            t = t - 4;
            holds[t] = *(time + t);
            t = t + 4;
        }
    }


    string h(holdh);
    string M(holdM);
    string s(holds);
    string datex = h + ":" + M + ":" + s;
    cout << datex;
    return 0;
}

It might be overflow of memory but I tried to prevent that by assigning null values. So if I have a problem in there too please inform. Thanks again.

Answer 1

The expression 2 <= t < 4 is equal to (2 <= t) < 4. That is, check if the result of 2 <= t (which is a bool true or false) is smaller than 4, which is will always be as boolean results are either 0 (for false) or 1 (for true).

If you want to compare a range, you need to to e.g. 2 <= t && t < 4.

---

More generally, I advice you to not use a loop for this. Do the assignments directly instead:

// Create three arrays and initialize all elements to zero
char holdh[3] = {};
char holdM[3] = {};
char holds[3] = {};

holdh[0] = time[0];
holdh[1] = time[1];
holdM[0] = time[2];
holdM[1] = time[3];
holds[0] = time[4];
holds[1] = time[5];

Much simpler and show your intent much more clearly.

---

And you don't even need the temporary holdX variables, as you can just get the sub-strings from time and initialize h, M and s directly:

const char* time = "173324";

std::string h(time + 0, time + 2);
std::string M(time + 2, time + 4);
std::string s(time + 4, time + 6);

And do you really need the also temporary h, M and s variables?

std::cout << std::string(time + 0, time + 2) << ':'
          << std::string(time + 2, time + 4) << ':'
          << std::string(time + 4, time + 6) << '\n';

And do you really need freshly allocated strings?

std::cout << std::string_view(time + 0, 2) << ':'
          << std::string_view(time + 2, 2) << ':'
          << std::string_view(time + 4, 2) << '\n';

Answer 2

Your code is an textbook example of bad use of if() statement inside of loop.

What happens that you repeat all same checks in every iterations, while you actually know where iterations must stop.

And you made assumption that if() checks every comparison inside its expression. It doesn't. It evaluates expression and checks if result is equivalent of non-zero value or boolean true. Thus if(4 <= t < 6) is a bug. It's an equivalent of if( (4 <= t) < 6 ). (a <= t) < b is always true, if b is grater than 1.

SImplest conversion of your code would be:

#include <iostream>
#include <cstdlib>
#include <iomanip>
#include <string>

using std::string;
using std::cout;

int main()
{
    const char* time = "173324"; 
    // note, that making it non-const is a not standard compliant

    char hold[3][3] = {};  // zero initialization

    for (int t = 0; t < 6;t++)
    {
        hold[t / 2][t % 2] = time[t];
    }


    string h(hold[0]);
    string M(hold[1]);
    string s(hold[2]);
    string datex = h + ":" + M + ":" + s;
    cout << datex;
    return 0;
}

Or maybe even so:

string hold[3];  
for (int t = 0; t < 6;t++)
{
    hold[t / 2] += time[t];
}
string datex = hold[0] + ":" + hold[1] + ":" + hold[2];

But better you should avoid making loops at all, provided string got constructor that receives iterators for beginning and end of source.

std::string h(time + 0, time + 2);