current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 2 down vote favorite

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; }
}
share|improve this question
    
is there any specific reason why you defined the get and set properties in this specific way – Tolani Jaiye-Tikolo May 31 at 13:04
up vote 6 down vote

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.
share|improve this answer
up vote 1 down vote

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.

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.