CodeSOD: Taking the Temperature

Mr. TA inherited some C# code that communicates with a humidity and a temperature sensor. Each sensor logs a series of datapoints as they run, and can provide them as an array of data points.

This leads to this code:

DataPoint[] humidDataPointArray = null; DataPoint[] tempDataPointArray = null;// For now, determine if this is the primary or secondary sensorif (sensorType == (int)SensorTypeID.HUMIDITY){ // create the array size then get the data humidDataPointArray = new DataPoint[ttPlusData.SecondarySensorData.GetDataPoints().Count]; humidDataPointArray = ttPlusData.SecondarySensorData.GetDataPoints().ToArray();}else{ if (sensorNum == 1) { // create the array size then get the data tempDataPointArray = new DataPoint[((SingleSensorTemptale)ttData).PrimarySensorData.GetDataPoints().Count]; tempDataPointArray = ((SingleSensorTemptale)ttData).PrimarySensorData.GetDataPoints().ToArray(); } else { // create the array size then get the data tempDataPointArray = new DataPoint[ttPlusData.SecondarySensorData.GetDataPoints().Count]; tempDataPointArray = ttPlusData.SecondarySensorData.GetDataPoints().ToArray(); }}

It starts out okay. We create the arrays to hold the data. Then we check the sensorType against an enum. And that's where things start to go wrong.

We initialize an empty array that's the same size as the number of data points, then we set that array equal to the array of data points.

I'm stuck trying to figure out if this is someone with no real experience, or a C programmer trying to migrate from pointers to references. Since C# is uses references, we don't need that new- we can just set humidPointsArray equal to the result of the function call.

Speaking of the result of the function call- it looks like GetDataPoints() returns a C# enumerable type. Which implies there's really no good reason to convert it into an array. I can't be certain about that, maybe they really need the array, but I suspect that's not the case- and it's a best practice in C# to use more abstract interfaces for collections.

But it gets worse.

We have an else with an if inside of it, instead of an else if. This second condition eschews the lovely enum we used before, and just checks sensorNum == 1. Then we repeat the same unnecessary allocation, with the bonus misspelling of SingleSensorTemptale, which is certain never to give any future developer problems.

For a bonus, this code runs in a tight loop, ensuring the garbage collector gets lots of practice cleaning up memory we never needed in the first place.

[Advertisement] Keep the plebs out of prod. Restrict NuGet feed privileges with ProGet. Learn more.