Thread: 1st post by 1st time C coder

It's not that hard to spot, in some cases it can improve readability, consider:

Say I wanted to fill each dimension of 'x' with '10'.

Code:

#define DIM 5
/* ... */

int x[DIM][DIM][DIM][DIM];
int a, b, c, d;

for(a = 0; a < DIM; a++)
    for(b = 0; b < DIM; b++)
        for(c = 0; c < DIM; c++)
            for(d = 0; d < DIM; d++)
                x[a][b][c][d] = 10;

/* OR */

for(a = 0; a < DIM; a++)
{
    for(b = 0; b < DIM; b++)
    {
        for(c = 0; c < DIM; c++)
        {
            for(d = 0; d < DIM; d++)
            {
                x[a][b][c][d] = 10;
            }
        }
    }
}

It's just an example, there are easier ways to fill that array

Anyway it proves to be more compact and I'd say is readable at a first glance, not to mention the potential bug of leaving off a closing brace (and accidentally closing it later).

My point is, the author has done nothing wrong - it really comes into coding style, after all that's what tab-indenting is really supposed to help solve. Plus you should be able to recognize that if there is no braces the first statement after will be run, (keep going until you find a ';').

Plus you should be able to recognize that if there is no braces the first statement after will be run, (keep going until you find a ';').

I can, I'm saying it's a bad thing to teach a beginner. Make it easy for beginners, use braces for all if and for loops, put parantheses around everything. When they get more advanced, they can play it fast and loose

QuantumPete

I agree if he doesn't explain it in depth then it's bad, making assumptions are generally bad

Originally Posted by iMalc

First bug is using strcat on an uninitialised variable. Use strcpy(Blank, "0"); instead.

Actually, Bytes is a global variable. Global arrays are automatically initialized to all zeros. This means that the first time this function is called, it will work as expected; but subsequent calls to it would cause a buffer overrun.

The simplest thing to do, of course, is to replace the first strcat() with strcpy().

There's no need to insult anyone. They were trying to help you, and they certainly weren't obligated to do so. There's a chance that the people in this thread won't feel like helping you in the future.

That stated, the two that suggested replacing strcat() with strcpy() weren't too sharp because the variable would have only contain the last copied content.

Besides the tone, didn't you do just that in your final code? I see a strcpy(), not a strcat().

Code:

BSTR GetIOReadings()
{
if(strlen(Bytes)>2)
  {
   SysFreeString((OLECHAR*)Bytes);
  }
strcpy(Bytes,"0");

So, if the length of Bytes is more than two, you free the data allocated by Bytes, and then proceed to assign a value to the memory? You're overwriting memory that has been freed. This is an exceedingly bad idea. You'll likely get segmentation faults and random buffer overruns and other annoying bugs.

That's assuming that Bytes was dynamically allocated in the first place. If it's declared like this, then you have even bigger problems.

Code:

char Bytes[22];

Arrays aren't meant to be freed by the program.

Your indentation's not the best, either. It could use some working on.

Plus you're missing a closing curly brace, }.

No, if you think it works, than that's all that matters.