Question

What is wrong with the following code: http://ideone.com/xTbwI

Answer 1

Sorry I don't know C++ =(

Answer 2

can you be more specific?

Answer 3

@No-data
It's in C

@ktklown
I was just wondering if the repr_array function could be done in a better way.

Answer 4

so you're saying it works correctly but you're looking for style tips?

Answer 5

right. I'm also wondering if it could be written to run faster/consume less space.

Answer 6

well your memory allocation computation is a little weird. why are you computing n = ((N-1)*3 + 1? what does n represent?

Answer 7

n is the number of bytes to allocate for each number, space, and comma character.

Answer 8

that's not what the code says

Answer 9

so for an array of 10 digits, that's (10-1)*3 + 1 bytes

Answer 10

what does the code say?

Answer 11

n would be the correct number of bytes to allocate if each number were only a single digit

Answer 12

but then you multiply that entire thing by PLACES_MAX which is the maximum number of digits per number

Answer 13

right... the function used to only work for single digit positive integers, but then I forgot to update stuff when I made it work for any integer

Answer 14

but you're multiplying n times places_max, which means you're assuming each number, each space, and each comma can all be places_max bytes.

Answer 15

right.

Answer 16

so do you see how that is wrong?

Answer 17

http://ideone.com/ERUTG

Alright, what else is now wrong :-D

Answer 18

you're also multiplying that all by sizeof(result), which is wrong; why are you multiplying by the size of a pointer?

Answer 19

wait, your 'n' computation in the new code still doesn't make sense. why are you multiplying by 3?

Answer 20

I should multiply by 3, since I write each number together with a trailing comma and space character.

Good point about the sizeof(result)... forgot to add a star

Answer 21

so doesn't that mean "add two" instead of multiply by three?

Answer 22

why would multiplying places_max*3 be what you want?

Answer 23

right http://ideone.com/GDmhx

Answer 24

ok, that looks better. for style, first, why did you write "1 <= N" instead of what you really meant, which is "n >= 1"?

Answer 25

I might forget and write n = 1

Answer 26

with <= and >= you can't make that error anyway; it just makes the code harder to read, and that's 10x more likely to cause a bug. Say what you mean so that the code's meaning is immediately obvious to anyone reading it. Also, "if (n = 0)" is flagged by every modern compiler as a warning anyway.

Answer 27

right

Answer 28

I honestly didn't understand this code without thinking carefully about it for 15 seconds, whereas if you'd written:

if (N < 1) {
return NULL;
}

it would have been immediately obvious.

Answer 29

so that brings me to my next suggestion which is to write the code above and pull the rest of the code out of the conditional.

Answer 30

http://ideone.com/SSMZH

Answer 31

so I should write the 'return null' part before the part where n is computed? alright

Answer 32

I'd write
if (N <= 0)
{
return NULL;
}

That makes it immediately obvious what your intent is. To return an error if N is 0 or less.

There's no logic sense in putting the computation inside the conditional. If the input is valid, you're going to run both the n computation *and* everything else in the rest of the function. Why just put a few lines inside the conditional and the rest below outside the conditional?

Answer 33

By putting those 2 lines of code inside the conditional it makes them seem somehow special, but they're not: they run along with everything else in the function below. But by setting them apart, again the code is much less readable, it seems like there might be some case where you don't compute N but still run the rest of the function.

Answer 34

I'd say the same goes for your next conditional. Change it to

if (result == NULL) {
fprintf(stderr, "out of memory");
return NULL;
}

and take the common case out of the conditional.

Answer 35

that way the common-case flow of your program is what's in the main line and the exceptions are all in the conditionals.

Answer 36

and finally for the string generation part, nothing there is wrong, but to make it more readable I might do this:

result[0] = '[';

char *s = result+1;

for (j = 0; j < N-1; j++) {
sprintf(s, "%d, ", A[j]);
s += strlen(s);
}

for() loops here are easier to read than while() loops because you're putting all the initialization, per-loop increment, and termination condition on the same line, rather than spreading them out over three lines.

Answer 37

http://ideone.com/qs23s

Answer 38

so as i said earlier take out the elses

Answer 39

everything in the entire function is going to run if N<=0 fails, why put some inside an else?

Answer 40

I put another incase malloc fails

Answer 41

also your array optimizing code you added is not correct; it doesn't account for the initial '[' or the final number, or nulls. just keep the j loop as it was before and add at the end

result = realloc(result, strlen(result*sizeof(*result)+1);

However in reality that's not actually going to save you much memory.

Answer 42

both the N<=0 check and the reslut==NULL check should just be simple comparisons with a return in them and no else{}.

Answer 43

realloc rarely saves memory in practice because of heap fragmentation, so if you actually wanted to optimize the size of the array (at the expense of cpu time) the solution would be to test-sprintf all the numbers.

By the way, little known fact, sprintf() returns the number of characters converted. So your inner loop can actually look like this:

for (j = 0; j < (N-1); ++j)
{
s += sprintf(s, "%d, ", A[j]);
}

Answer 44

I forgot all about that stuff: the return value of the printf family! :-P as well as the reason why realloc() here is actually unecessary

thanks

Answer 45

yw

Answer 46

Alright my code is like this now: http://ideone.com/cURFL

Answer 47

what happened to your N <= 0 check?

Answer 48

why did you change from malloc to realloc there?

Answer 49

realloc is more elegant.

Answer 50

why?