Parsing an XML file and creating an object with multiple properties from the XML

Question

This question contains some C# code I've written as well as a XML format I have designed. My C# code runs a series of "jobs" by loading the XML file and then calling different SQL stored procedures and E-Mailing the results to different addresses. My question specifically relates to the C# code to parse the XML document and also how well designed the XML format is.

The code allows for a variable number of email addresses and stored procedure parameters. To allow for multiple named stored procedure parameters, I'm using a dictionary. The key is the name of the parameter and the value is the value to be used with said parameter.

My questions:

• How well designed is the XML?
• How good or bad is the C# code to parse the file?
  • How can it be improved?
  • Is there anything majorly wrong with it?

This is the XML format I'm using (in this case there are only two jobs defined):

<?xml version="1.0" encoding="utf-8"?>
<jobService>

  <job name="Report" storedProcedure="Procedure">
    <procedureParameter name="anotherStoredProcedureParameter1" value="test" />
    <procedureParameter name="anotherStoredProcedureParameter2" value="123456789" />

    <emailSettings subjectName="Subject" attachmentName="SomeFileName1.csv" senderAddress="[email protected]">
      <recipients>
        <address value="[email protected] />
        <address value="[email protected] />
      </recipients>

      <recipientsBcc>
        <address value="[email protected] />
        <address value="[email protected] />
      </recipientsBcc>

      <recipientsCC>
        <address value="[email protected] />
        <address value="[email protected] />
      </recipientsCC>
    </emailSettings>
  </job>

  <job name="Report #2" storedProcedure="Procedure2">
    <procedureParameter name="anotherStoredProcedureParameter1" value="test" />
    <procedureParameter name="anotherStoredProcedureParameter2" value="test" />
    <procedureParameter name="anotherStoredProcedureParameter3" value="test" />
    <procedureParameter name="anotherStoredProcedureParameter4" value="test" />

    <emailSettings subjectName="Subject" attachmentName="SomeFileName1.csv" senderAddress="[email protected]">
      <recipients>
        <address value="[email protected] />
        <address value="[email protected] />
      </recipients>

      <recipientsBcc>
        <address value="[email protected] />
        <address value="[email protected] />
      </recipientsBcc>

      <recipientsCC>
        <address value="[email protected] />
        <address value="[email protected] />
      </recipientsCC>
    </emailSettings>
  </job>
</jobService>

And this is the C# code to parse the XML:

public IEnumerable<JobConfiguration> GetJobConfigurations()
{
    XDocument jobs = XDocument.Load(@"jobs.xml");

    var query = jobs.Descendants("jobService").Descendants("job")
        .Select(x => new JobConfiguration
        {
            Name = x.Attribute("name").Value,
            StoredProcedure = x.Attribute("storedProcedure").Value,
            ProcedureParameters = x.Elements("procedureParameter").ToDictionary(p => p.Attribute("name").Value, p => p.Attribute("value").Value),
            EmailProperties = x.Elements("emailSettings").Select(y => new EmailProperties
            {
                SubjectName = y.Attribute("subjectName").Value,
                AttachmentName = y.Attribute("attachmentName").Value,
                SenderAddress = y.Attribute("senderAddress").Value,
                RecipientAddresses = y.Elements("recipients").Elements("address").Select(z => z.Attribute("value").Value).ToList(),
                RecipientAddressesBcc = y.Elements("recipientsBcc").Elements("address").Select(z => z.Attribute("value").Value).ToList(),
                RecipientAddressesCC = y.Elements("recipientsCC").Elements("address").Select(z => z.Attribute("value").Value).ToList()
            }).ToList()
        }).ToList();

    return query;
}

And these are the JobConfiguration and EMailProperties classes:

public class JobConfiguration
{
    public string Name { get; set; }
    public string StoredProcedure { get; set; }
    public IDictionary<string, string> ProcedureParameters { get; set; }
    public EMailProperties EMailProperties { get; set; }
}

public class EmailProperties
{
    public string SenderAddress { get; set; }
    public IList<string> Recipients { get; set; }
    public IList<string> RecipientsBcc { get; set; }
    public IList<string> RecipientsCC { get; set; }
}

Answer 1

I'm going to put a disclaimer on this. My answer isn't necessarily a better way to handle this particular use case. You seem to have the simplest thing that works as far as your XML schema goes. So, take this with a grain of salt and discuss the pros/cons of each with your colleagues.

public class EmailProperties
{
    public string SenderAddress { get; set; }
    public IList<string> Recipients { get; set; }
    public IList<string> RecipientsBcc { get; set; }
    public IList<string> RecipientsCC { get; set; }
}

This isn't a very object oriented way to do this. What happens if someone should be both "To" and "Bcc"? You'd have to duplicate the email address in the data. I would introduce another class.

public class Recepient
{
    public string EmailAddress { get; set; }
    public bool To { get; set; }
    public bool Cc { get; set; }
    public bool Bcc { get; set; }
}

Which makes your XML look like this.

  <recipients>
    <recipient to="true" >
      [email protected]
    <recipient />
    <recipient cc="true">
      [email protected]
    <recipient  />
 <recipients />

Note that the inner and outer elements match, but the outer is plural, while the inner is singular. Also, the value is placed as the value of the element instead of as a property. These are all semantically the same type of thing, but they have different attributes for whether they're "To", "Cc", etc. If it's a business rule that no one can ever have more than one of these, I'd change it a bit.

<recipient type="to">
    [email protected]
<recipient />

And the corresponding change to the new class.

public enum RecepientType 
{
    To, 
    Cc,
    Bcc
}

public class Recepient
{
    public string EmailAddress { get; set; }
    public RecepientType Type { get; set; }
}

Now, rather than review your parsing code (props for using XDoc by the way, a lot of people try to parse XML the hard way), I'm going to suggest you look at using serialization to do this instead. I won't get into the nitty gritty here, but look into the System.Xml.Serialization namespace. It allows us to work directly with strongly typed objects and doing all that nasty parsing for us.

Answer 2

If you are not forced to use xml then json might be a better solution. You wouldn't have to worry about parsing. Here'a sample:

Everything you need to do to load it into an object is:

class Program
{
    static void Main(string[] args)
    {
        var jobs = File.ReadAllText("jobs.json");
        var jobsService = JsonConvert.DeserializeObject<JobService>(jobs);
    }
}  

For this to work you'd to define a couple of classes:

public class JobService
{
    public IList<Job> Jobs { get; set; }
}

public class Job
{
    public IList<ProcedureParameter> ProcedureParameters { get; set; }
    public EmailSettings EmailSettings { get; set; }
}

public class Recipient
{
    public IList<string> Addressess { get; set; }
}

public class ProcedureParameter
{
    public string Name { get; set; }
    public string Value { get; set; }
}

public class EmailSettings
{
    public string SubjectName { get; set; }
    public string AttachmentName { get; set; }
    public IList<string> Recipients { get; set; }        
}

Your json would then directly be converted into an object:

{
    "Jobs": [
        {
            "Name": "Report",
            "EmailSettings": {
                "SubjectName": "Subject",
                "AttachmentName": "SomeFileName1.csv",
                "Recipients": [
                    "[email protected]",
                    "[email protected]"
                ]
            }
        }
    ]
}

It'd also be a good idea to follow @RubberDuck's naming suggestion.