Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite
1

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 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 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 at 19:00
1  
The simplest solution would be to just call .Single() after your Select(). –  svick Jun 24 at 19:16

1 Answer

up vote 1 down vote

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

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.