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

\$\endgroup\$
  • 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 Jan 27 '12 at 11:10
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?

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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