Can I improve this code for readability and/or performance?

Question

I have some data that logs kWh. I want to be able to collect the data and produce a bar chart (I'm using the Microsoft ASP.Net Chart control). I have written code that works, but it looks a little clunky to me and I wondered if somebody could point out a method that may be easier to read and/or more efficient.

The data should normally constantly increase in value though it can be reset. As it is an accumulative value if the values from one period are the same as the previous period then I need to display zero for that time period. Then, when the value does increase, I need to display it as the difference between this period and the last period with a value.

I have the following code to collect the data at hourly intervals only:

      myValues = (from values in myEntities.PointValues
                      where
                        values.PointID == dataValue &&
                        SqlFunctions.DatePart("n", values.DataTime) == 0
                      orderby values.DataTime
                      select new BarChart
                        {
                          Time = values.DataTime,
                          Value = values.DataValue
                        }).ToList();

I then use the following code to set the values that are identical in consecutive periods to 0, though, of course, maintaining the first occurrence at its original value:

for (var i = 0; i < myValues.Count - 1; i++)
      {
        var j = 0;
        while (Math.Abs(myValues[i].Value - myValues[i + 1].Value) < double.Epsilon)
        {
          i++;
          j++;
          if (i == myValues.Count - 1) break;
        }
        while (j > 0)
        {
          myValues[i + 1 - j].Value = 0;
          j--;
        }
      }

Finally, I apply the following code to get the difference in values (difference between value this period and the last period with a positive value).

var subValue = double.MaxValue;
foreach (var t in myValues)
{
  if (t.Value > 0 && t.Value < subValue) subValue = t.Value;
  if (Math.Abs(t.Value - subValue) > double.Epsilon && Math.Abs(t.Value - 0) > double.Epsilon)
  {
    var j = t.Value;
    t.Value = t.Value - subValue;
    subValue = j;
  }
}

Which produces my final data.

As I say, this code works fine but if anyone can help with some optimizing/readability, it would be greatly appreciated.

EDIT: For some images see:

https://stackoverflow.com/questions/14425230/can-anyone-assist-with-some-code-optimization

Answer 1

I think that your code to “remove the values that are identical” is quite confusing.

First, it doesn't remove anything, it just sets the values to zero. And unless you filter that out later, it will cause problems in your final calculation.

Second, modifying the indexing variable of a for is usually not a good idea. It's unexpected and creates code that is hard to understand.

Third, I think you don't actually need this. It would be much easier to remove zero values after you compute the differences.

---

Now, in your code to get the differences, I find the logic hard to follow. (One reason is probably non-descriptive variable names.) And I don't understand why do you have all those conditions there, computing a difference (even with the possibility of a reset) should be simpler. Also, you seem to ignore the fact that if you compute the difference between n values, you will get only n - 1 results.

I would rewrite this code using Zip() and Skip() like this:

var result = Enumerable.Zip(
    data, data.Skip(1),
    (first, second) =>
    second.Value < first.Value
        ? second
        : new BarChart { second.Time, Value = second.Value - first.Value });

Also, naming a single value in a chart BarChart is confusing, a better name would be something like BarChartValue.

---

You also asked for performance optimizations. Is your code actually causing performance problems? Have you profiled your application and has this code been identified as bottleneck? If not, don't worry (much) about performance, it won't make a difference anyway.

Answer 2

For removing similar values just work through the list.

EDIT: Changing to meet requirements

var lastPositiveValue = myValues.First();

foreach (var current in myValues.Skip(1))
{
    var lastValue = lastPositiveValue.Value;

    if (Math.Abs(current.Value - lastPositiveValue.Value) > double.Epsilon)
    {
        lastPositiveValue = current;
    }

    current.Value = current.Value - lastValue;        
}

svick's answer for the difference seems clean enough :)

Another couple of suggestions, I would rename Value to MeterReading or something of the sort. That will describe it a little better.