Reading data from database, saving to new list and adding list to grid

Question

I made some changes to how I was saving the data in order for it to show up on a kendo grid. Right now my giant method is getting data from the database (using getValue), then creating a new list to save both data onto the single list to populate the kendo grid with one list even though they are two different types of data (although similar).

Here is my current code that works:

//Grabs the value of angles and points already existing in db and returns the data to the view 
[OutputCache(NoStore = true, Duration = 0, VaryByParam = "*")]
public ActionResult ReadMeasurements([DataSourceRequest] DataSourceRequest request, string viewType)
{
 try
 {
 JsonResult json = new JsonResult();
 List<AngleData> angledata = UserSession.GetValue(StateNameEnum.Planning, ScreenName.Planning.ToString() + "Angles" + viewType, UserSessionMode.Database) as List<AngleData> ?? new List<AngleData>();
 List<PointData> pointData = UserSession.GetValue(StateNameEnum.Planning, ScreenName.Planning.ToString() + "Points" + viewType, UserSessionMode.Database) as List<PointData> ?? new List<PointData>();
 List<PlanningViewParam> measurements = new List<PlanningViewParam>();
 if (pointData.Count() != 0 && angledata.Count() != 0)
 {
 foreach (AngleData i in angledata)
 {
 string col = "#" + ColorTranslator.FromHtml(String.Format("#{0:X2}{1:X2}{2:X2}", (int)(i.color.r * 255), (int)(i.color.g * 255), (int)(i.color.b * 255))).Name.Remove(0, 2);
 int angleVal = (int)i.angleValue;
 measurements.Add(new PlanningViewParam()
 {
 Color = col,
 Label = "Angle",
 Value = angleVal,
 Number = i.angleNumber,
 AngleName = i.name
 });
 }
 var numbers = new HashSet<int>();
 var distinctPoints = pointData.Where(x => numbers.Add(x.pointNumber));
 foreach (PointData f in distinctPoints)
 {
 string col = "#" + ColorTranslator.FromHtml(String.Format("#{0:X2}{1:X2}{2:X2}", (int)(f.color.r * 255), (int)(f.color.g * 255), (int)(f.color.b * 255))).Name.Remove(0, 2);
 string angleValWithQ = f.pointAnglesValue;
 string pointAnglesVal = "";
 if (angleValWithQ != null)
 {
 pointAnglesVal = angleValWithQ.Replace("?", "°");
 }
 string pointNam = f.pointUniqueKey;
 measurements.Add(new PlanningViewParam()
 {
 PointColor = col,
 PointLabel = "Point",
 PointValue = pointAnglesVal,
 PointName = pointNam,
 Number = f.pointNumber
 });
 }
 return json = Json(measurements.ToDataSourceResult(request, i => new PlanningViewParam()
 {
 Color = i.Color,
 Label = i.Label,
 Value = i.Value,
 PointColor = i.PointColor,
 PointLabel = i.PointLabel,
 PointValue = i.PointValue,
 PointName = i.PointName,
 AngleName = i.AngleName,
 Number = i.Number
 }), JsonRequestBehavior.AllowGet);
 }
 else if (angledata.Count() != 0)
 {
 foreach (AngleData i in angledata)
 {
 string col = "#" + ColorTranslator.FromHtml(String.Format("#{0:X2}{1:X2}{2:X2}", (int)(i.color.r * 255), (int)(i.color.g * 255), (int)(i.color.b * 255))).Name.Remove(0, 2);
 int angleVal = (int)i.angleValue;
 measurements.Add(new PlanningViewParam()
 {
 Color = col,
 Label = "Angle",
 Value = angleVal,
 Number = i.angleNumber,
 AngleName = i.name
 });
 }
 return json = Json(measurements.ToDataSourceResult(request, i => new PlanningViewParam()
 {
 Color = i.Color,
 Label = i.Label,
 Value = i.Value,
 Number = i.Number,
 AngleName = i.AngleName
 }), JsonRequestBehavior.AllowGet);
 }
 else if (pointData.Count() != 0)
 {
 var numbers = new HashSet<int>();
 var distinctPoints = pointData.Where(x => numbers.Add(x.pointNumber));
 foreach (PointData f in distinctPoints)
 {
 string col = "#" + ColorTranslator.FromHtml(String.Format("#{0:X2}{1:X2}{2:X2}", (int)(f.color.r * 255), (int)(f.color.g * 255), (int)(f.color.b * 255))).Name.Remove(0, 2);
 string angleValWithQ = f.pointAnglesValue;
 string pointAnglesVal = "";
 if (angleValWithQ != null)
 {
 pointAnglesVal = angleValWithQ.Replace("?", "°");
 }
 int pointNumber = f.pointNumber;
 string pointNam = f.pointUniqueKey;
 measurements.Add(new PlanningViewParam()
 {
 PointColor = col,
 PointLabel = "Point",
 PointValue = pointAnglesVal,
 PointName = pointNam,
 Number = pointNumber
 });
 }
 return json = Json(measurements.ToDataSourceResult(request, f => new PlanningViewParam()
 {
 PointColor = f.PointColor,
 PointLabel = f.PointLabel,
 PointValue = f.PointValue,
 PointName = f.PointName,
 Number = f.Number
 }), JsonRequestBehavior.AllowGet);
 }
 }
 catch (Exception ex)
 {
 Log.Item(ex);
 }
 return null;
}

AngleData has Color color property which is described as:

public partial class Color
{
 public float b;
 public float a;
 public float r;
 public float g;
}

So, basically, this method works perfectly but I want to simplify it and I'm not sure how. Currently the performance of this method is ~1.05s!

ToDataSourceResult Extension methods:

 public static DataSourceResult ToDataSourceResult(this DataTable dataTable, DataSourceRequest request);
 public static DataSourceResult ToDataSourceResult(this IEnumerable enumerable, DataSourceRequest request);
 public static DataSourceResult ToDataSourceResult(this IQueryable enumerable, DataSourceRequest request);
 public static DataSourceResult ToDataSourceResult<TModel, TResult>(this IEnumerable<TModel> enumerable, DataSourceRequest request, Func<TModel, TResult> selector);
 public static DataSourceResult ToDataSourceResult(this IEnumerable enumerable, DataSourceRequest request, ModelStateDictionary modelState);
 public static DataSourceResult ToDataSourceResult<TModel, TResult>(this IQueryable<TModel> enumerable, DataSourceRequest request, Func<TModel, TResult> selector);
 public static DataSourceResult ToDataSourceResult(this IQueryable queryable, DataSourceRequest request, ModelStateDictionary modelState);
 public static DataSourceResult ToDataSourceResult<TModel, TResult>(this IEnumerable<TModel> enumerable, DataSourceRequest request, ModelStateDictionary modelState, Func<TModel, TResult> selector);
 public static DataSourceResult ToDataSourceResult<TModel, TResult>(this IQueryable<TModel> enumerable, DataSourceRequest request, ModelStateDictionary modelState, Func<TModel, TResult> selector);

Answer 1

Distinct values of a list / IEnumerable<T>

Also it is a valid appoach to use

var numbers = new HashSet<int>();
var distinctPoints = pointData.Where(x => numbers.Add(x.pointNumber)); 

to get distinct PointData of a List<PointData> you should consider to do this right by adding a specialized IEqualityComparer<T> which only checks equality based on the pointNumber property/field.

This IEqualityComparer<PointData>, if you don't need it somewhere else too, could just be a private class like

private class SpecializedPointDataComparer : IEqualityComparer<PointData>
{
 public bool Equals(PointData x, PointData y)
 {
 // your implementation here
 }
 public int GetHashCode(PointData obj)
 {
 // your implementation here
 }
} 

For proper implementation of the GetHashCode() method you can take a look at https://stackoverflow.com/a/263416/2655508 .

and can then be used like

foreach (PointData f in pointData.Distinct(new SpecializedPointDataComparer())
{
 .......
} 

---

Unfortunately you didn't provide neither the AngleData nor the PointData class. Therefor this is just guessing:

This line

string col = "#" + ColorTranslator.FromHtml(String.Format("#{0:X2}{1:X2}{2:X2}", (int)(f.color.r * 255), (int)(f.color.g * 255), (int)(f.color.b * 255))).Name.Remove(0, 2); 

could be simplified, if f.color is a System.Drawing.Color, by using

string color = String.Format("#{0}", f.color.Name.Remove(0,2).ToUpper()); 

and this should be extracted to a method like

private string GetColorAsText(Color color)
{
 return String.Format("#{0}", color.Name.Remove(0,2).ToUpper());
} 

to reduce code duplication which leads us to the next point....

Code duplication

You should always follow the DRY principle, which means Don't Repeat Yourself which boils down to reduce code duplication if possible by extracting duplicated code to methods.
This has the advantage of smaller methods which are easier to read and maintain. Bob the Maintainer will thank you for following this.

So, what parts are duplicated ?

• the loop over the List<AngleData>
• the loop over the List<PointData>

So let us add a method for each loop.

private IEnumerable<PlanningViewParam> GetPlanningViewParameters(IEnumerable<AngleData> angleData)
{
 foreach (AngleData aData in angleData)
 {
 yield return new PlanningViewParam()
 {
 Color = GetColorAsText(aData.color),
 Label = "Angle",
 Value = (int)aData.angleValue;,
 Number = aData.angleNumber,
 AngleName = aData.name
 };
 }
}

and

private IEnumerable<PlanningViewParam> GetPlanningViewParameters(IEnumerable<PointData> pointData)
{
 foreach (PointData pData in pointData)
 {
 yield return new PlanningViewParam()
 {
 PointColor = GetColorAsText(pData.color),
 PointLabel = "Point",
 PointValue = (pData.pointAnglesValue != null) ? pData.pointAnglesValue.Replace("?", "°") : string.Empty,
 PointName = pData.pointUniqueKey,
 Number = pData.pointNumber
 };
 }
}

this would reduce the if..else if statements of your former method like

if (pointData.Count() != 0 && angledata.Count() != 0)
{
 measurements.AddRange(GetPlanningViewParameters(angledata));
 var distinctPoints = pointData.Distinct(new SpecializedPointDataComparer());
 measurements.AddRange(GetPlanningViewParameters(distinctPoints));
 return json = Json(measurements.ToDataSourceResult(request, i => new PlanningViewParam()
 {
 Color = i.Color,
 Label = i.Label,
 Value = i.Value,
 PointColor = i.PointColor,
 PointLabel = i.PointLabel,
 PointValue = i.PointValue,
 PointName = i.PointName,
 AngleName = i.AngleName,
 Number = i.Number
 }), JsonRequestBehavior.AllowGet);
}
else if (angledata.Count() != 0)
{
 measurements.AddRange(GetPlanningViewParameters(angledata));
 return json = Json(measurements.ToDataSourceResult(request, i => new PlanningViewParam()
 {
 Color = i.Color,
 Label = i.Label,
 Value = i.Value,
 Number = i.Number,
 AngleName = i.AngleName
 }), JsonRequestBehavior.AllowGet);
}
else if (pointData.Count() != 0)
{
 var distinctPoints = pointData.Distinct(new SpecializedPointDataComparer());
 measurements.AddRange(GetPlanningViewParameters(distinctPoints));
 return json = Json(measurements.ToDataSourceResult(request, f => new PlanningViewParam()
 {
 PointColor = f.PointColor,
 PointLabel = f.PointLabel,
 PointValue = f.PointValue,
 PointName = f.PointName,
 Number = f.Number
 }), JsonRequestBehavior.AllowGet);
}

We now see, that the if..else if statements just decide what should be returned but are doing almost the same things.

• If we think about this, we can reduce the small code duplication by filling the List<PlanningViewParam> outside of the if.. else if construct.

• We can also ommit the last else if because it is redundant. This is because if one of the first conditions is met the method returns a value and therefor this won't be reached anymore.

• The line JsonResult json = new JsonResult(); is not needed at all. You can simply return Json(...).

• There is no need to use the Count() method on a List<T> because a List<T>has a property Count which should be used. If you plan to use this method with IEnumerable<PointData> and IEnumerable<AngleData> then you shouldn't use the Count() method either if you only want to know if there are any data in the IEnumerable<T>. By calling Count() on an IEnumerable<T> will iterate over all elements, if the underlaying type isn't a kind of collection.
  By using the Any() method the iteration will stop after the first occurance of an valid item.

Taking all this would then lead to

//Grabs the value of angles and points already existing in db and returns the data to the view 
[OutputCache(NoStore = true, Duration = 0, VaryByParam = "*")]
public ActionResult ReadMeasurements([DataSourceRequest] DataSourceRequest request, string viewType)
{
 try
 {
 List<AngleData> angledata = UserSession.GetValue(StateNameEnum.Planning, ScreenName.Planning.ToString() + "Angles" + viewType, UserSessionMode.Database) as List<AngleData> ?? new List<AngleData>();
 List<PointData> pointData = UserSession.GetValue(StateNameEnum.Planning, ScreenName.Planning.ToString() + "Points" + viewType, UserSessionMode.Database) as List<PointData> ?? new List<PointData>();
 List<PlanningViewParam> measurements = new List<PlanningViewParam>();
 measurements.AddRange(GetPlanningViewParameters(angledata));
 var distinctPoints = pointData.Distinct(new SpecializedPointDataComparer());
 measurements.AddRange(GetPlanningViewParameters(distinctPoints));
 if (pointData.Any() && angledata.Any())
 {
 return Json(measurements.ToDataSourceResult(request, m => new PlanningViewParam()
 {
 Color = i.Color,
 Label = i.Label,
 Value = m.Value,
 PointColor = m.PointColor,
 PointLabel = m.PointLabel,
 PointValue = m.PointValue,
 PointName = m.PointName,
 AngleName = m.AngleName,
 Number = m.Number
 }), JsonRequestBehavior.AllowGet);
 }
 else if (angledata.Any())
 {
 return Json(measurements.ToDataSourceResult(request, m => new PlanningViewParam()
 {
 Color = m.Color,
 Label = m.Label,
 Value = m.Value,
 Number = m.Number,
 AngleName = m.AngleName
 }), JsonRequestBehavior.AllowGet);
 }
 return Json(measurements.ToDataSourceResult(request, m => new PlanningViewParam()
 {
 PointColor = m.PointColor,
 PointLabel = m.PointLabel,
 PointValue = m.PointValue,
 PointName = m.PointName,
 Number = m.Number
 }), JsonRequestBehavior.AllowGet);
 }
 catch (Exception ex)
 {
 Log.Item(ex);
 }
 return null;
}

---

Because this method is public you should add proper xml documentation. By doing so the documentation will be seen in intellisense too.

Just hit 3 times / above the method and the IDE will provide the correct structure.

• Kala J: Thank you for your help! I'm still looking through your suggestions but what do you mean by, "PointValue = (pData.pointAnglesValue != null) ? pData.pointAnglesValue.Replace("?", "°"), string.Empty,"? I get an error for string.empty part.
• Heslacher: Sorry, the comma before string.Empty should have been a : . Updated answer
• Kala J: Thank you. Btw, what is f in this context, " return String.Format("#{0}", f.color.Name.Remove(0,2).ToUpper()); }"? That is f.color... what is f?
• Heslacher: A copy & paste error ;-) Updated
• Heslacher: By profiling the method in question you can identify the bottleneck.

Answer 2

By no means am I an expert.

Personally I would break it down into a few more distinct methods.

I find it easier to follow and maintain. rather than this morass of coding. 6 months later when you come back you might be like: wtf is this?

• RubberDuck: I'm not sure this quite qualifies as an answer. It's answer-ish, but reads more like a comment.