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
-
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\$Lars-Erik– Lars-Erik2012年01月27日 11:10:55 +00:00Commented Jan 27, 2012 at 11:10
1 Answer 1
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?
Explore related questions
See similar questions with these tags.