patterncsharpMinor
Can I improve this code for readability and/or performance?
Viewed 0 times
thiscanreadabilityandimproveforperformancecode
Problem
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:
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:
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).
Which produces my final data.
As I say, this code works fine but if anyone can help with some optimizing/readab
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 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 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/readab
Solution
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
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
Also, naming a single value in a chart
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.
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.
Code Snippets
var result = Enumerable.Zip(
data, data.Skip(1),
(first, second) =>
second.Value < first.Value
? second
: new BarChart { second.Time, Value = second.Value - first.Value });Context
StackExchange Code Review Q#20740, answer score: 2
Revisions (0)
No revisions yet.