3
\$\begingroup\$

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="address@address.com">
      <recipients>
        <address value="address@address.com />
        <address value="address@address.com />
      </recipients>

      <recipientsBcc>
        <address value="address@address.com />
        <address value="address@address.com />
      </recipientsBcc>

      <recipientsCC>
        <address value="address@address.com />
        <address value="address@address.com />
      </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="address@address.com">
      <recipients>
        <address value="address@address.com />
        <address value="address@address.com />
      </recipients>

      <recipientsBcc>
        <address value="address@address.com />
        <address value="address@address.com />
      </recipientsBcc>

      <recipientsCC>
        <address value="address@address.com />
        <address value="address@address.com />
      </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; }
}
\$\endgroup\$
  • \$\begingroup\$ If you have a choice whether you use xml or json I would pick up json.net and deserialize it as json without any additional parsers into an object. It'd be a two-liner then ;-] \$\endgroup\$ – t3chb0t Sep 5 '15 at 10:55
2
\$\begingroup\$

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" >
      address@address.com
    <recipient />
    <recipient cc="true">
      john.doe@domain.com
    <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">
    John.doe@domain.com
<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.

\$\endgroup\$
2
\$\begingroup\$

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": [
                    "address@address.com",
                    "address@address.com"
                ]
            }
        }
    ]
}

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

\$\endgroup\$
  • 1
    \$\begingroup\$ Well I wasn't really looking for ways to scrap my solution entirely, I'd rather use XML for this but this is a good answer for anyone else who comes across this post. \$\endgroup\$ – user9993 Sep 5 '15 at 12:56
  • \$\begingroup\$ XML can be serialized this way too. It doesn't matter much what format the data comes in, what matters is a good structure that maps well to a set of classes. Point is, you don't have to parse the XML either. \$\endgroup\$ – RubberDuck Sep 6 '15 at 11:20

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.