4
\$\begingroup\$

I'm generating a table to use on jQuery visualize plugin and generate a chart. The code is pretty good, but maybe there is a way to improve it. Maybe do more work on the LINQ query and less stuff on the rest of the code.

The idea is get every content and the launch date of them for each region. Separate all dates and the the amount of content launched that day. But maybe there is a better way. Anyone?

Here is the method i use:

 public KeyValuePair<DateTime[], List<KeyValuePair<string, int[]>>> TimeGrowth(int days, params string[] locales)
 {
 DateTime startdate = DateTime.Today.AddDays(days * (-1));
 var qry = from r in GlobalVariables.Regions
 where locales.Contains(r.ID)
 select new
 {
 Region = r,
 Launches = (from d in r.RegionalContents
 where d.RegionId == r.ID && d.PublishDate != null
 group d by d.PublishDate into groupeddates
 select new
 {
 Date = groupeddates.Key,
 Count = groupeddates.Count()
 }).OrderBy(x => x.Date)
 };
 var regions = qry.ToArray();
 var dates = new List<DateTime>();
 var datesbeforestart = new List<DateTime>();
 var countries = new List<KeyValuePair<string, int[]>>();
 for (int i = 0; i < regions.Length; i++)
 {
 for (int j = 0; j < regions[i].Launches.Count(); j++)
 {
 var data = regions[i].Launches.ElementAt(j).Date.Value;
 if (data > startdate)
 dates.Add(data);
 else
 datesbeforestart.Add(data);
 }
 }
 dates = dates.Distinct().OrderBy(x => x.Date).ToList();
 for (int i = 0; i < regions.Length; i++)
 {
 var values = new int[dates.Count];
 values[0] = CountDateInterval(datesbeforestart.Min(), startdate, regions[i].Region.ID);
 int acum = values[0];
 for (int j = 1; j < dates.Count; j++)
 {
 int valor = 0;
 if (regions[i].Launches.Any(x => x.Date == dates[j]))
 valor = regions[i].Launches.Single(x => x.Date == dates[j]).Count;
 acum += valor;
 values[j] = acum;
 }
 var country = new KeyValuePair<string, int[]>(regions[i].Region.CountryEnglish, values);
 countries.Add(country);
 }
 countries = countries.OrderByDescending(x => x.Value.Max()).ToList();
 var finalresult = new KeyValuePair<DateTime[], List<KeyValuePair<string, int[]>>>(dates.ToArray(), countries);
 return finalresult;
 }

You can see the actual running code here on the last chart: http://programad.net/xbltoolsv2beta/Stats

asked Jan 25, 2012 at 18:34
\$\endgroup\$
1
  • 1
    \$\begingroup\$ I'd start by splitting the method into several smaller methods with descripting names. Makes the logic easier to follow, and hopefully also exposes possible refactorings, field candidates etc. Contents of for loops are a prime candidate for methods of their own. You can also extract lambda expressions to methods. (Tried JetBrains' Resharper?) \$\endgroup\$ Commented Jan 27, 2012 at 11:10

1 Answer 1

1
\$\begingroup\$

A few general points:

(1) GlobalVariables.Regions -- I don't know how big your application is, but if I'm looking at a code base and I see some static class called "GlobalVariables" that usually makes me leery. In a pure object-oriented language, all data can (and arguably ought to be) associated with some object. A GlobalVariables class with a bunch of statics on it is the kind of thing that is likely to grow and rot with as the application grows (and rots).

(2) As mentioned by Lars-Erik, I think that breaking some of that functionality out into other methods would be beneficial. Personally, I look at control statements, especially nested ones for opportunities to do that. Looking at your code, the logic inside for (int i = 0; i < regions.Length; i++) would be a good candidate. Just by looking at the code casually, it's hard to tell what all is going on inside that loop. But if you pulled it out into a method and gave it a descriptive name, someone looking at your code could easily understand what the loop was doing.

(3) You have a lot of code (as Linq tends to) with sequences of statements like Foo.Bar().Baz().Rebar(); Generally speaking, if anything in those sequence chains returns null, you're going to get null reference exceptions. Perhaps clients of this method handle exceptions that it generates, but I can't see that you're handling anything but the "happy path" in this method. What happens if things go unexpectedly?

answered Jan 27, 2012 at 18:09
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.