Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have some duplicate code using LINQ & XML. I'm sure there is a way to refactor it, but I'm not sure how to go about doing it. Would someone help, please?

var fileInfo = dataXL.Worksheet("data")
                .Where(t => t["F2"].Equals(company))
                .Select(t => new
                {
                    Price = t["F5"], 
                    EPS10Q1 = t["F12"], EPS10Q2 = t["F13"], EPS10Q3 = t["F14"], EPS10Q4 = t["F15"],
                    EPS11Q1 = t["F19"], EPS11Q2 = t["F20"], EPS11Q3 = t["F21"], EPS11Q4 = t["F22"],
                    EPS12Q1 = t["F26"], EPS12Q2 = t["F27"], EPS12Q3 = t["F28"], EPS12Q4 = t["F29"],
                    EPS13Q1 = t["F33"], EPS13Q2 = t["F34"], EPS13Q3 = t["F35"], EPS13Q4 = t["F36"],
                    EPS14Q1 = t["F40"], EPS14Q2 = t["F41"], EPS14Q3 = t["F42"], EPS14Q4 = t["F43"]
                });

xdoc.Element("company")
                .Add(new XElement("eps",
                        new XElement("year2010",
                            new XElement("EPS1", resAnGreyInfo.Select(x => x.EPS10Q1)),
                            new XElement("EPS2", resAnGreyInfo.Select(x => x.EPS10Q2)),
                            new XElement("EPS3", resAnGreyInfo.Select(x => x.EPS10Q3)),
                            new XElement("EPS4", resAnGreyInfo.Select(x => x.EPS10Q4))),
                        new XElement("year2011",
                            new XElement("EPS1", resAnGreyInfo.Select(x => x.EPS11Q1)),
                            new XElement("EPS2", resAnGreyInfo.Select(x => x.EPS11Q2)),
                            new XElement("EPS3", resAnGreyInfo.Select(x => x.EPS11Q3)),
                            new XElement("EPS4", resAnGreyInfo.Select(x => x.EPS11Q4))),
                        new XElement("year2012",
                            new XElement("EPS1", resAnGreyInfo.Select(x => x.EPS12Q1)),
                            new XElement("EPS2", resAnGreyInfo.Select(x => x.EPS12Q2)),
                            new XElement("EPS3", resAnGreyInfo.Select(x => x.EPS12Q3)),
                            new XElement("EPS4", resAnGreyInfo.Select(x => x.EPS12Q4))));
share|improve this question
    
Both of those XML schemas seem to be badly designed. Is there any possibility of changing them? –  svick Jun 24 '13 at 18:37
    
Also, can there be more than one element where t["F2"] == company? If yes, you're going to get weird results. If not, fileInfo should be a single object, not a collection. –  svick Jun 24 '13 at 18:40
    
@svick how do you suggest that I do that? Do you have any examples to get a single object rather than a collection? Also, regarding the XML, i need ideas on how to remodel them. This was the line of business requires but I don't want to repeat code. –  inquisitive_one Jun 24 '13 at 19:00
1  
The simplest solution would be to just call .Single() after your Select(). –  svick Jun 24 '13 at 19:16

2 Answers 2

This is what you could do:

  • Try to bring order to the chaos of field numbers. For instance, associate 'F12' to 'Year 2010, Quarter 1'. This is essential to reduce code duplication.
  • Fortunately, the field names of each year are sequential (that is, F12 is 2010Q1, F13 is 2010Q2, etc.)
  • Once you have that figured out, you can rewrite the fileInfo retrieval code to reduce duplication.
  • With fileInfo corrected, you can move to xdoc.
  • Assuming resAnGreyInfo is the same as fileInfo, the main obstacle to reduce duplication were the many different fields. With the new fileInfo layout, you should be able to get the fields with fileInfo[year][quarter], which is much better than fileInfo.EPSYEARQUARTER.
  • If changing the layout of fileInfo is not possible for some reason, you could try messing with reflection or dynamic typing to work around this mess.
  • Once that's done, the xdoc could be much simpler, since each year element has essentially the same format.

So here's a rough sketch of what the final code would look like (it does not have the equivalent of the Select for resAnGreyInfo`, but even if you add that the result should be similar):

var years = new List<int>(2010, 2011, 2012, 2013, 2014);
var yearToFieldNumber = new Dictionary<int, int>();
yearToFieldNumber[2010] = 12; //2010 starts at F12
yearToFieldNumber[2011] = 19; //2011 starts at F19
yearToFieldNumber[2012] = 26;
yearToFieldNumber[2013] = 33;
yearToFieldNumber[2014] = 40;

var fileInfo = dataXL.Worksheet("data")
                .Where(t => t["F2"].Equals(company))
                .Select(t => new
                {
                    Price = t["F5"],
                    Years = years.Select(year => Enumerable.Range(1, 4)
                        .ToDictionary(quarter => quarter, quarter => t["F" + (yearToFieldNumber[year] + quarter - 1)]).ToList()
                });

//Now, fileInfo.Years[year][quarter] gives us the data for that quarter

var yearElements = Enumerable.Range(2010, 3) //Take 3 years starting from 2010
        .Select(year => new XElement("year" + year,
            Enumerable.Range(1, 4).Select(quarter => new XElement("EPS" + quarter, fileInfo[year][quarter])).ToArray()
        )).ToArray();

xdoc.Element("company").Add(new XElement("eps", yearElements));
share|improve this answer

Let's start by indenting it:

var fileInfo = dataXL.Worksheet("data")
    .Where(t => t["F2"].Equals(company))
    .Select(t =>
        new
        {
            Price = t["F5"], 
            EPS10Q1 = t["F12"], EPS10Q2 = t["F13"], EPS10Q3 = t["F14"], EPS10Q4 = t["F15"],
            EPS11Q1 = t["F19"], EPS11Q2 = t["F20"], EPS11Q3 = t["F21"], EPS11Q4 = t["F22"],
            EPS12Q1 = t["F26"], EPS12Q2 = t["F27"], EPS12Q3 = t["F28"], EPS12Q4 = t["F29"],
            EPS13Q1 = t["F33"], EPS13Q2 = t["F34"], EPS13Q3 = t["F35"], EPS13Q4 = t["F36"],
            EPS14Q1 = t["F40"], EPS14Q2 = t["F41"], EPS14Q3 = t["F42"], EPS14Q4 = t["F43"]
        });

xdoc.Element("company")
    .Add(new XElement("eps",
        new XElement("year2010",
            new XElement("EPS1", resAnGreyInfo.Select(x => x.EPS10Q1)),
            new XElement("EPS2", resAnGreyInfo.Select(x => x.EPS10Q2)),
            new XElement("EPS3", resAnGreyInfo.Select(x => x.EPS10Q3)),
            new XElement("EPS4", resAnGreyInfo.Select(x => x.EPS10Q4))),
        new XElement("year2011",
            new XElement("EPS1", resAnGreyInfo.Select(x => x.EPS11Q1)),
            new XElement("EPS2", resAnGreyInfo.Select(x => x.EPS11Q2)),
            new XElement("EPS3", resAnGreyInfo.Select(x => x.EPS11Q3)),
            new XElement("EPS4", resAnGreyInfo.Select(x => x.EPS11Q4))),
        new XElement("year2012",
            new XElement("EPS1", resAnGreyInfo.Select(x => x.EPS12Q1)),
            new XElement("EPS2", resAnGreyInfo.Select(x => x.EPS12Q2)),
            new XElement("EPS3", resAnGreyInfo.Select(x => x.EPS12Q3)),
            new XElement("EPS4", resAnGreyInfo.Select(x => x.EPS12Q4))));

Let's remove unused code and replace the repetitive lines by arrays and a few anonymous types. Note that your original code won't compile, so the modified version doesn't exactly correspond to the original one.

var fileInfo = dataXL.Worksheet("data")
    .Where(t => t["F2"].Equals(company))
    .Select(
        t => new []
        {
            new { Year = 2010, Data = new [] { t["F12"], t["F13"], t["F14"], t["F15"] } },
            new { Year = 2011, Data = new [] { t["F19"], t["F20"], t["F21"], t["F22"] } },
            new { Year = 2012, Data = new [] { t["F26"], t["F27"], t["F28"], t["F29"] } },
            new { Year = 2013, Data = new [] { t["F33"], t["F34"], t["F35"], t["F36"] } },
            new { Year = 2014, Data = new [] { t["F40"], t["F41"], t["F42"], t["F43"] } },
        });

xdoc.Element("company")
    .Add(new XElement("eps",
        fileInfo
            .Take(3)
            .Select(
                year => new XElement(
                    "year" + year.Year,
                    year.Data.Select((d, i) => new XElement("EPS" + (i + 1), d))))));

Let's refactor:

var fileInfo = dataXL.Worksheet("data")
    .Where(t => t["F2"].Equals(company))
    .Select(
        t => new []
        {
            new { Year = 2010, Data = new [] { t["F12"], t["F13"], t["F14"], t["F15"] } },
            new { Year = 2011, Data = new [] { t["F19"], t["F20"], t["F21"], t["F22"] } },
            new { Year = 2012, Data = new [] { t["F26"], t["F27"], t["F28"], t["F29"] } },
            new { Year = 2013, Data = new [] { t["F33"], t["F34"], t["F35"], t["F36"] } },
            new { Year = 2014, Data = new [] { t["F40"], t["F41"], t["F42"], t["F43"] } },
        });

into:

var fileInfo = from t in dataXL.Worksheet("data")
               where t["F2"].Equals(company)
               select this.LoadYears(t);

private Dictionary<int, IEnumerable<dynamic>> LoadYears(dynamic t)
{
    return new Dictionary<int, IEnumerable<dynamic>>
    {
        { 2010, new [] { t["F12"], t["F13"], t["F14"], t["F15"] } },
        { 2011, new [] { t["F19"], t["F20"], t["F21"], t["F22"] } },
        { 2012, new [] { t["F26"], t["F27"], t["F28"], t["F29"] } },
        { 2013, new [] { t["F33"], t["F34"], t["F35"], t["F36"] } },
        { 2014, new [] { t["F40"], t["F41"], t["F42"], t["F43"] } },
    }
}

This is slightly more readable, isn't it? But what about those repetitive lines? What if, with a simple extension method, we could slice an array?

private IEnumerable<dynamic> LoadYears(dynamic t)
{
    return Enumerable
      .Range(12, 43)
      .Select(i => t["F" + i])
      .Slice(4)
      .Select(
          (groupValues, groupIndex) =>
          new
          {
              Year = 2010 + groupIndex,
              Data = groupValues
          });
}

The whole code is:

var fileInfo = from t in dataXL.Worksheet("data")
               where t["F2"].Equals(company)
               select this.LoadYears(t);

private IEnumerable<dynamic> LoadYears(dynamic t)
{
    return Enumerable
        .Range(12, 43)
        .Select(i => t["F" + i])
        .Slice(4)
        .Select((groupValues, groupIndex) => new { Year = 2010 + groupIndex, Data = groupValues });
}

xdoc.Element("company")
    .Add(new XElement("eps",
        fileInfo
            .Take(3)
            .Select(
                year => new XElement(
                    "year" + year.Year,
                    year.Data.Select((d, i) => new XElement("EPS" + (i + 1), d))))));

We don't repeat code here, but the solution is slightly unreadable. Maintaining such code is difficult as hell. Variable names are poorly chosen, too short and too cryptic. Debugging would be difficult as well.

Let's refactor it into:

var fileInfo = from dataSet in dataXL.Worksheet("data")
               where dataSet["F2"].Equals(company)
               select this.LoadYears(dataSet);

private IEnumerable<dynamic> LoadYears(dynamic dataSet)
{
    return Enumerable
        .Range(12, 43)
        .Select(dataIndex => t["F" + dataIndex])
        .Slice(4)
        .Select(
            (groupValues, groupIndex) =>
            new
            {
                Year = 2010 + groupIndex,
                Data = groupValues
            });
}

private dynamic SerializeYear(dynamic year)
{
    var quarters = year.Data.Select((quarter, index) => new XElement("EPS" + (index + 1), quarter));
    return new XElement("year" + year.Year, quarters);
}

document.Element(
    "company"
        new XElement(
            "eps",
            fileInfo.Take(3).Select(year => this.SerializeYear(year)))));
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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