2
\$\begingroup\$

Basically, I'm trying to find all active people with a roleId of 3 (analyst id). But I want to exclude ids that were created as test analysts, i.e. "130", "252", etc. I see that I'm repeating the part to exclude the test ids. Also, I'm repeating the part about the roleId. Isn't there a way to optimize it? My query works fine, but I'm trying to optimize to make it more DRY.

Here's what my code looks like (I've labeled where I think the code can be optimized):

from people in xDoc.Descendants("People")
where (int?) Convert.ToInt16(people.Element("active").Value) == 1 &&
    (int?)Convert.ToInt16(people.Element("id").Value) != 130 &&  // <-- to optimize
    (int?)Convert.ToInt16(people.Element("id").Value) != 252 &&  // <-- to optimize
    (int?)Convert.ToInt16(people.Element("id").Value) != 264 &&  // <-- to optimize
    (int?)Convert.ToInt16(people.Element("id").Value) != 302 &&  // <-- to optimize
    people.Descendants("roleIds").Descendants("int")
        .Select(p => (int?)Convert.ToInt16(p.Value)).FirstOrDefault() == 3  // <-- to optimize
select new PeopleClass
{
    ResAnId = (int) Convert.ToInt16(people.Element("id").Value),
    DpInit = (string)people.Element("clientCode").Value,
    ResAnFName = (string)people.Element("firstName").Value,
    ResAnLName = (string)people.Element("lastName").Value,
    ResAnName = String.Format("{0} {1}", (string)people.Element("firstName").Value,
        (string)people.Element("lastName").Value),
    RoleId = people.Descendants("roleIds").Descendants("int")
        .Select(p => (int?) Convert.ToInt16(p.Value)).FirstOrDefault(),  // <-- to optimize
    DivisionId = (int?) Convert.ToInt16(people.Element("divisionId").Value)
}).ToList();

If you're interested, here's my XML feed:

<People>
  <id>269</id>
  <dummy>false</dummy>
  <firstName>Steve</firstName>
  <lastName>Jobs</lastName>
  <initial></initial>
  <title>CEO</title>
  <active>1</active>
  <analystPosition></analystPosition>
  <clientCode>SJ</clientCode>
  <clientCode2></clientCode2>
  <divisionId>2</divisionId>
  <updatedBy>245</updatedBy>
  <associates>
    <Associate>
      <analystId>269</analystId>
      <associateId>257</associateId>
      <rank>0</rank>
    </Associate>
  </associates>
  <roleIds>
    <int>3</int>
    <int>4</int>
    <int>13</int>
  </roleIds>
</People>
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Please explain what the query is doing. \$\endgroup\$ Jan 19, 2017 at 22:19
  • 1
    \$\begingroup\$ Why is there so much casting going on? \$\endgroup\$
    – Denis
    Jan 19, 2017 at 23:39
  • \$\begingroup\$ I'm fairly a novice programmer. I thought that's programming should be done. I'm guessing that I should only do casting once, right? \$\endgroup\$ Jan 20, 2017 at 1:28

2 Answers 2

7
\$\begingroup\$

You can introduce range variables to hold data which you need several times in your query:

var analystRoleId = 3;
var testAnalystIds = new[] { 130, 252, 264, 302 };

var activeAnalysts = 
    from p in xdoc.Descendants("People")
    let id = (int)p.Element("id")
    let isActive = (int)p.Element("active") == 1                        
    let isAnalyst = p.Descendants("roleIds").Elements().Any(e => (int)e == analystRoleId)
    let firstName = (string)p.Element("firstName")
    let lastName = (string)p.Element("lastName")
    where isActive && isAnalyst && !testAnalystIds.Contains(id)
    select new PeopleClass {
        ResAnId = id,
        DpInit = (string)p.Element("clientCode"),
        ResAnFName = firstName,
        ResAnLName = lastName,
        ResAnName = $"{firstName} {lastName}",
        RoleId = analystRoleId,
        DivisionId = (int?)p.Element("divisionId")
    };

Also

  • You can use interpolated strings instead of String.Format
  • I believe id always should present in your file and it will not be null. There is no need to cast this value to nullable
  • I believe you should check all role ids for verifying whether person is analyst. But you can use FirstOrDeafult as well
  • You don't need to query for roleId second time - if person matched where clause then it's analyst and you know his role id
  • You don't need to access Value property of XElement. It's better to use explicit cast operator. Otherwise you can get NullReferenceExceptions
  • If you can change implementation of PeopleClass then start with renaming it to Person and give more descriptive names to its fields. You can also remove backing storage from ResAndName filed and make it calculated on the fly.
\$\endgroup\$
3
  • 1
    \$\begingroup\$ I've never seen this bit of code before: ResAnName = $"{firstName} {lastName}". Actually, I was surprised that this worked. Thanks for opening my eyes to the new paradigm. Also, thank for Value pointer. I thought that I had to use Value when accessing XML fields. I'm not sure I understand your last point. Would you mind explaining, please? \$\endgroup\$ Jan 20, 2017 at 1:44
  • 1
    \$\begingroup\$ This is a nice example how useful let can be. \$\endgroup\$
    – t3chb0t
    Jan 20, 2017 at 7:26
  • \$\begingroup\$ @inquisitive_one by last point I mean that you can create class named Person without class suffix, and give it properties with names Id, FirstName, LastName, and property Name which is implemented as get { return $"{FirstName} {LastName}"; } \$\endgroup\$ Jan 20, 2017 at 15:07
4
\$\begingroup\$

The query as shown in @Sergey Berezovskiy's answer is ok for a one time query but if you are going to create the People class more then once from XElements then you should encapsulate it and let the People class parse the XElement

class People
{
    private People(XElement xPeople)
    {
        id = (int)xPeople.Element("id");
        ..other properties
    }

    public int id { get; set; }

    .. other properties

    public static People Parse(XElement xPeople) => new People(xPeople);
}

You could then simplify the query to

var activeAnalysts = 
    from p in xdoc.Descendants("People").Select(People.Parse)    
    where p.isActive && p.isAnalyst && !testAnalystIds.Contains(p.id)
    select new p;

Other then this I support the suggestions given in the other answer.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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