Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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:

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

share|improve this question
    
The answer from Svick below is exactly the kind of thing I was looking for. Being new to programming I hadn't even considered the concept of duplicating arrays. Techniques like this are good to know. –  Family Jan 21 '13 at 10:58

2 Answers 2

up vote 1 down vote accepted

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.

share|improve this answer
1  
Good answers, but from what I understand, the distinct clause won't work in the case because it removes all distinct values. I understand that you only remove them if they are the the same as the previous value. So 1, 2, 2, 3, 2, 3, 1 should turn into 1, 2, 3, 2, 3, 1. –  Jeff Vanzella Jan 20 '13 at 18:43
    
Unfortunately I copied this question from StackOverflow where I had some images, I don't have enough rep here to do that. This has highlighted a few shortcomings with my description. I apologise, I didn't mean remove the identical values, I meant reduce them to 0. I.e. if the values are 10,10,10,12, then I reduce the 2nd and 3rd 10's to 0 (because it's a cumulative reading, no power has been used that period). Then I subtract the last positive value (in this case 10) from 12, and replace 12 with this difference (power used in this period). I agree with descriptive names, I will do that. –  Family Jan 20 '13 at 19:02
    
As for performance, no I haven't done any profiling, I just thought that having the 2 loops didn't seem very efficient, but it may be the only way to achieve what I need. –  Family Jan 20 '13 at 19:16
    
@JeffVanzella You're right, I didn't realize that. –  svick Jan 20 '13 at 19:33
    
@Family Could you explain why are you reducing the values to 0? It doesn't make much sense to me. It only complicates the following code and doesn't give you anything. –  svick Jan 20 '13 at 19:36

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.

share|improve this answer
    
Sorry, I didn't explain my problem well enough. I had posted this at stackoverflow.com/questions/14425230/… where it has some images. The thing is that if values are the same from period to period they need to display 0, i.e. no power used, then the next value needs to be the reading minus the last positive reading, i.e. 10,10,10,12 would become 10,0,0,2 (indicating 10kWh, none, none, 2kWh used). –  Family Jan 20 '13 at 19:06
    
@Family That's exactly what you get if you subtract each pair of values, no need for zeroing same values first. –  svick Jan 20 '13 at 19:38

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.