What's Wrong With This Code? (#8)

Joe Developer is working with a simple struct:

struct Point
{
    public int x;
    public int y;
}

Joe's tech lead asked him to write a method that will return an array of 10,000 initialized points. Joe wrote following code.

Point[] CreatePoints()
{
    Point[] points = new Point[10000];

    for (int i = 0; i < points.Length; i++)
    {
        points[i] = new Point();
    }

    return points;
}

The code doesn't create any runtime errors, but Joe is worried because his tech lead looked at the code and frowned. What could provoke such a reaction?

Hint: Joe's lead is a performance nut.

posted on Monday, October 23, 2006 11:34 PM by scott

Comments

Monday, October 23, 2006 8:48 PM by Alan Dean

# re: What's Wrong With This Code? (#8)

The iteration is unnecessary - structs are value types.

The declaration:

Point[] points = new Point[10000];

will call the default ctor.
Monday, October 23, 2006 9:13 PM by Anthony Mills

# re: What's Wrong With This Code? (#8)

Given that he's working with structs, the memory is zeroed anyway and "return new Point[10000];" is sufficient?
Monday, October 23, 2006 9:39 PM by Geoff Appleby

# re: What's Wrong With This Code? (#8)

Ha! That's a simple one :)

Because Point is a struct, it's a valuetype, which means it ALWAYS exist, it doesn't need to be new'ed.

As a result, all he needed was:

Point[] points = new Point[10000];
return points;

Monday, October 23, 2006 9:50 PM by Jeff

# re: What's Wrong With This Code? (#8)

Unless I'm missing a shorter way, each element of the array must be created via "new" so the structure is valid. This means we can't escape touching each index of the array. It feels like a naive answer to me, but just unroll the loop. Perhaps mod the length of the array by 10, allocate ten points per loop iteration and handle any extra indices after the loop (incase the length isn't evenly divisible by 10).
Monday, October 23, 2006 9:52 PM by Eber Irigoyen

# re: What's Wrong With This Code? (#8)

Joe's lead was pissed because Joe didn't initialize the structures (instances were created but not initialized)
Monday, October 23, 2006 10:09 PM by Haacked

# re: What's Wrong With This Code? (#8)

Joe should have done this instead, since it accomplishes the same thing. Instead he creates 10,000 points twice.

Point[] CreatePoints()
{
return new Point[10000];
}
Monday, October 23, 2006 10:31 PM by stej

# re: What's Wrong With This Code? (#8)

Hi, is the problem in "return points;"? I think the whole array will be copied (twice?)since the Point[] is array of structs.
Monday, October 23, 2006 10:40 PM by Phil Weber

# re: What's Wrong With This Code? (#8)

The loop is superfluous. Structs are value types, and as such do not need to be initialized with 'new'.
Monday, October 23, 2006 11:26 PM by Caffenero

# re: What's Wrong With This Code? (#8)

Has it something to do with the fact that Structs and arrays are passed by value? :)

So, maybe:

void CreatePoints(out Point[] result)
{
result = new Point[10000];

for (int i = 0; i < points.Length; i++)
{
result[i] = new Point();
}
}
Monday, October 23, 2006 11:28 PM by Wilhelm Svenselius

# re: What's Wrong With This Code? (#8)

Perhaps calling 'points.Length' on each iteration is a bit of a waste, since we know the size from the start. We could just put '10000' as the loop limit or put it in a separate const int and use it for both the array creation and loop limiter.
Monday, October 23, 2006 11:42 PM by Adrian.

# re: What's Wrong With This Code? (#8)

I would have said that in this case initialising the structs in the for loop is redundant.

Tuesday, October 24, 2006 12:03 AM by Eric

# re: What's Wrong With This Code? (#8)

... Because those 10,000 comments will be physically copied in memory when returned, instead of referenced?
Tuesday, October 24, 2006 12:08 AM by Laurent

# re: What's Wrong With This Code? (#8)

Because Point is a structure, considered as a value type, an instance exists as soon it is declared, so the 1-10000 loop is unusefull ?
Tuesday, October 24, 2006 12:19 AM by JP

# re: What's Wrong With This Code? (#8)

When the Array is created, it is already initialized with a point in each slot, with x=0 and y=0 so the loop has no use.
Although, it takes 10 000 000 points in the array to waist 125 ms on my system.
Tuesday, October 24, 2006 12:22 AM by fromaustria

# re: What's Wrong With This Code? (#8)

just a wild guess:
array of points gets created on the heap.
new Point() creates a value type on the stack.
points[i]=... copies the Point from the stack to the heap.
better: return new Point[10000]; // done.


Tuesday, October 24, 2006 12:43 AM by Duckboy

# re: What's Wrong With This Code? (#8)

Dude, you're allocating and initializing all your points twice!

Point is a value type, so the new Point[10000] will actually allocate them all and set their x and y fields to 0. Then, the for loop allocates and initialializes them again, unnecessarily.

So, you're wasting valuable CPU time, causing the GC to have a minor anurism and ultimately using more electricity, therefore hurting the environment.

No sir, I don't like it!
-- Mr. Horse (from Ren & Stimpy)
Tuesday, October 24, 2006 12:45 AM by James Newton-King

# re: What's Wrong With This Code? (#8)

The Points are automatically initialized when the array is created. The loop is unnecessary.
Tuesday, October 24, 2006 12:56 AM by Alexey

# re: What's Wrong With This Code? (#8)

The only thing I can think of is calling points.Length 10000 times, but won't the compiler inline it? Oh, Point is a struct, not a class, so we don't need to call the constructor 10000 times.
Tuesday, October 24, 2006 1:03 AM by fireping

# re: What's Wrong With This Code? (#8)

Property value points.Length is calculated every iteration while you know exact array length.
Tuesday, October 24, 2006 1:26 AM by jayson knight

# re: What's Wrong With This Code? (#8)

Point[] points should be initialized to 10,000 outside the scope of the CreatePoints method, i.e. Point[] CreatePoints(ref Points[])
Tuesday, October 24, 2006 1:58 AM by Martin

# re: What's Wrong With This Code? (#8)

Value types (structs) are 'auto allocated'.
Tuesday, October 24, 2006 2:01 AM by Andrew

# re: What's Wrong With This Code? (#8)

I'd guess it's to do with the use of a for loop instead of do until
Tuesday, October 24, 2006 2:48 AM by Saamorim

# re: What's Wrong With This Code? (#8)

Well, it simple.

Why use the for?
The simple fact that the Joe’s Developer as created the array of Point structs, doesn't required him to create each of the Points. This is because the compiler already allocates the necessary space to accommodate the structs. This, however, didn't work if the Point object was a class. In that case it was necessary to create each of the point objects.
This is basically the biggest difference between a class and a struct?

Am I correct?

Regards
Tuesday, October 24, 2006 3:18 AM by Mitch Wheat

# re: What's Wrong With This Code? (#8)

Because Point is a defined as a struct it is a value type. Therefore, the line

Point[] points = new Point[10000];

does not just allocate space for an array of pointers to points, it actually allocates the space for them. The loop and subsequent allocations are unnecessary.
Tuesday, October 24, 2006 3:36 AM by Wesley

# re: What's Wrong With This Code? (#8)

Theres no need to construct a new point in every loop.

Since the point is a struct (=value type) Joe could create one new Point() before the start of the loop and asign it to every point in the array. Since it's a value type the Point will be copied to the array.

Cheers,
Wes
Tuesday, October 24, 2006 3:57 AM by Wesley

# re: What's Wrong With This Code? (#8)

Second thought: There's no need for the loop at all. Structs(value types) are initialized at the moment they're created.


Cheers,
Wes
Tuesday, October 24, 2006 4:07 AM by Skup

# re: What's Wrong With This Code? (#8)

Arrays are automaticaly filled whith 0's when initialized. Calling new Point() on each element of the array just fill it again with 0's which is useless.

The developper did that because when initializing a struct on the stack, it's not automatically filled with 0's (contrary to reference types), so you have to call at least the parameterless constructor to initialize it.
Tuesday, October 24, 2006 4:20 AM by Christiaan

# re: What's Wrong With This Code? (#8)

Well my gueuss is that Joe doens't need to initialize 10000 points as Point is a value type and must not be initialized.
Tuesday, October 24, 2006 5:54 AM by Sahil Malik

# re: What's Wrong With This Code? (#8)

Joe should be worried because his lead cares more about performance than anything else.

:)
Tuesday, October 24, 2006 6:44 AM by Scott

# re: What's Wrong With This Code? (#8)

I apologize that I have to moderate comments. There were lots of good comments overnight but they all appear at once- in the morning when I catch up.

A lot of people pointed out that the for loop isn't needed. Since Point is a struct (a value type), creating the array is enough to give us 10,000 zero-initialized points.

The array is created on the heap, so it is ok to return a reference to the calling method.
Tuesday, October 24, 2006 7:19 AM by Geoff Appleby

# re: What's Wrong With This Code? (#8)

Yeah, what's with the moderation dude? Maybe you need Gaptcha! :)
Tuesday, October 24, 2006 7:24 AM by scott

# re: What's Wrong With This Code? (#8)

Yeah Geoff - I need something. If I don't moderate this place would be filled with spam. I did have a captcha control installed previously but got some negative comments about it.

Perhaps Gaptcha is better than what I was using. I'll have to give it a look.
Wednesday, October 25, 2006 4:54 PM by Geoff Appleby

# re: What's Wrong With This Code? (#8)

Well, the source is available for you now, although it looks like you're running CS 1.1 (i'm guessing) and I wrote it as a 2.1 addin for .net 2. Is an upgrade worth it? (Hell yeah! :)
Tuesday, November 14, 2006 5:07 AM by Tin Bui

# re: What's Wrong With This Code? (#8)

omg,
finally, I have finished reading the 33 comments above.
Guys, look at the hint from the author. Regardless of whether or not you want to do a loop for initialization, just say we want to do the loop, then 10,000 would make a performance nut really goes nut.
However, against the author's intention, I would not be thinking about making the looping any faster, but to asynchronously do the loop on another thread and go do something else.


Hi Geoff, how are you?
Monday, November 20, 2006 6:11 AM by AJ

# re: What's Wrong With This Code? (#8)

The (entry level example) author wants an array of 10,000 points, 2 integers each. The best way is to malloc the block, (initialized to 0) and use a pointer to a struct of type point to access the array.

It does however, make sense, especially on complex projects, to make the first write of the code as clear as possible (as this is), with little regard to performance, and then increase performance as needed.

Code "sevicablility" is a paramount issue to support people, however, lack of clarity as we all know, adds to R&D job security.

Performance, unfortunately, often goes hand-in-hand with lack of clarity.
Wednesday, December 13, 2006 2:37 AM by Gaurav

# re: What's Wrong With This Code? (#8)

I think calling points.Length 10000 times is not a good idea and it will have a performance hit. Rather than code should be something like this.

Point[] CreatePoints()
{
Point[] points = new Point[10000];
int length = points.Length;

for (int i = 0; i < length; i++)
{
points[i] = new Point();
}

return points;
}