2
\$\begingroup\$

Basically, all I have to do is read some xml from a file. And I have a working solution, although I am not sure that it is the best way of going about doing things, as I have only recently started working with xml. Any advice on how I could improve the following?

[XmlRoot("iosLayout")]
public class iosLayoutXml
{
    [XmlElement("transactionLayout")]
    public List<TransactionLayoutXml> transactionLayouts { get; set; } = new List<TransactionLayoutXml>();
}

[XmlRoot("transactionLayout")]
public class TransactionLayoutXml
{
    [XmlElement("field")]
    public List<FieldXml> fields { get; set; } = new List<FieldXml>();

    [XmlAttribute("type")]
    public string type { get; set; }
}

[XmlRoot("field")]
public class FieldXml
{
    [XmlAttribute("element")]
    public string element { get; set; }

    [XmlAttribute("text")]
    public string text { get; set; }

    [XmlAttribute("value")]
    public string value { get; set; }
}
\$\endgroup\$
1
  • \$\begingroup\$ is there any specific reason why you defined the get and set properties in this specific way \$\endgroup\$
    – Tolani
    May 31, 2016 at 13:04

2 Answers 2

6
\$\begingroup\$

It's a small code snippet but IMO you may improve few things about style:

  • Usually (in .NET environment) properties are TitleCase as well as classes.
  • Whatever your favorite naming convention is you should be consistent, if you use title case for classes then iosLayoutXml should be renamed IosLayoutXml.
  • Prefixes and suffixes are generally avoided, you're using Xml suffix and it has also another disadvantage: you're exposing an implementation detail. You may, in future, change storage to Json but calling code should not be aware of that.

Some notes (without more context they're almost questions) about code:

  • If your classes will not be derived then you should also mark them as sealed. To open a class to inheritance requires much more work.
  • You do not initialize properties with default values: is it null, for example, acceptable for FieldXml.text if omitted in XML file?
  • From caller point of view is it null equivalent to String.Empty? Do you want to move this responsability on caller?
  • Will anyone use these classes for setting properties? In this case you should also add proper argument checking.
  • You may not need it in this moment but it may be useful (think about it) to define FieldCollection and use it instead of List<Field>. Once it has been deserialized will it change in size? If not you may also consider to use FieldXml[].
  • Is it valid to have a null entry for TransactionLayoutXml.fields?
  • I don't know which kind of values will go into FieldXml.text and FieldXml.value (and if format is fixed or you have some freedom) but if they contains characters that must be escaped you may want to use node text instead of an attribute, especially if XML may edited by hand it's much more handy to use CDATA sections.
\$\endgroup\$
1
\$\begingroup\$

Two minor nitpicks I see:

  1. Class names ending with ...Xml - any particular reason for this?
  2. Property names should be PascalCased

Other than that -- looks good to me.

\$\endgroup\$

Your Answer

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

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