[asveikau]

      strcpy( ( char * ) commsOrderBuffer, "GET /v1/printer/");
  
      strcat( ( char * ) commsOrderBuffer, ( char * ) settings.getIMEI());
      strcat( ( char * ) commsOrderBuffer, "/orders.txt  HTTP/1.1\r\n");
      strcat( ( char * ) commsOrderBuffer, "HOST: ");
      strcat( ( char * ) commsOrderBuffer, SERVER_NAME);
      strcat( ( char * ) commsOrderBuffer, "\r\n");
      strcat( ( char * ) commsOrderBuffer, "Authorization: Basic ");

What the.... O(n) string concatenations, unnecessary pointer casts, no bounds checking... I think extra whitespace in an HTTP request is not their only problem.

[ams6110]

Those would be "safe" (assuming that settings.getIMEI() is completely under your control, everything else is string literals) but yeah snprintf seems way better here (though it's been well over 20 years since I wrote any significant C code.

[asveikau]

Possibly safe but definitely inefficient, since it has to find the end of the string to know where the destination pointer starts. The right way is to keep a pointer to the end.

(Or since they are already using std::string in other places, maybe just do that everywhere, I'm sure it makes better choices than they did here.)

The pointer cast thing is glaring. Why not simply declare the buffer as a char array and be done with it, instead of casting at every use? IMO over-use of pointer casts is a clear sign someone is lost in the language, your goal should be to reduce them.

[ams6110]

Yeah agree, to me casts like that are a smell that someone is trying to squash compiler complaints rather than understanding them. It also has every appearance of "copy/paste" code writing.

[drblast]

Since these are all string literals you really don't need any concatenation function at all except to concatenate with the output of getIMEI().

  char *a = "Hello " "world!";

Works just fine.

Edit to add: You can really see the difference in code between someone coming to C/C++ from a high level language and someone who learned assembly first, where a list of literals is a common idiom. The original style is not functionally wrong, but it does look like Java :-)

Also: DON'T post your potentially insecure string handling code on the Internet; are you crazy?

[linuxlizard]

The Arduino embedded C library (which I'm assuming they're using) isn't as rich as a Glibc or uclibc. Sometimes I have to fall back on very old school methods to build complex strings.

[asveikau]

This is a very weak excuse to use strcat(). Writing something that actually tracks the length instead of recomputing it every time takes 5 minutes.

[syncsynchalt]

I'd hesitate at suggesting it only for efficiency in some embedded device that is fast enough, if it sacrificed readability at all.

Yes, I'm saying that I'm ok with the code above assuming that there are no user inputs that can exceed the bounds (though casting away the const is strange, I assume the stdlib is not correctly consted?)

[asveikau]

It's not casting away const, the buffer is declared as uint8_t and they are casting that to char... Otherwise known as it should have been char to begin with.

[Dylan16807]

O what now? O(7) is the same as O(1)

[asveikau]

Yeah I'll admit I was a little fast and loose with that. O(n + m), where n is the length of the (pre-concatenation) destination and m is the length of the source. Do that enough times and you get a quadratic looking curve. My point was it's easy to get to O(m).