Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have to work with a 3rd-party API that allows me to define and execute "commands", using XML. Since I don't like seeing mixed abstraction levels, I managed to remove all the inline XML / string concatenations by creating a simple XmlCmdBuilder object - here is the C# implementation (I have one in VB6 as well, the C# one will eventually replace the VB6 equivalent code):

public class XmlCmdBuilder
{
    private readonly IList<XmlCmd> _xmlCommands;

    public XmlCmdBuilder()
    {
        _xmlCommands = new List<XmlCmd>();
    }

    public void AddCommand(XmlCmd cmd)
    {
        _xmlCommands.Add(cmd);
    }

    public void Clear()
    {
        _xmlCommands.Clear();
    }

    public override string ToString()
    {
        var builder = new StringBuilder();

        builder.Append("<cmd:Commands xmlns:cmd=\"http://www.contoso.com/XmlCommand\">");
        foreach (var xmlCommand in _xmlCommands)
        {
            builder.Append(xmlCommand);
        }
        builder.Append("</cmd:Commands>");

        return builder.ToString();
    }

}

The "builder" simply exposes a AddCommand method that takes in a XmlCmd object:

public class XmlCmd
{
    private readonly IList<XmlCmdParameter> _parameters;

    public XmlCmd(string name)
        : this(name, new List<XmlCmdParameter>())
    {

    }

    public XmlCmd(string name, params XmlCmdParameter[] parameters)
        : this(name, parameters.ToList())
    {

    }

    public XmlCmd(string name, IList<XmlCmdParameter> parameters)
    {
        _parameters = parameters;
        Name = name;
    }

    public string Name { get; private set; }

    public void AddParameter(XmlCmdParameter parameter)
    {
        _parameters.Add(parameter);
    }

    public override string ToString()
    {
        var builder = new StringBuilder();

        builder.Append(string.Format("<cmd:Command name=\"{0}\">", Name));
        foreach (var xmlCommandParameter in _parameters)
        {
            builder.Append(xmlCommandParameter);
        }
        builder.Append("</cmd:Command>");

        return builder.ToString();
    }
}

Similarly, the XmlCmd exposes a method to add XmlCmdParameter objects, which in turn expose methods to add XmlCmdParameterListItem objects.

This all works very well, but since most commands are predefined (there can be "custom" commands too), I created a factory class that creates XmlCmd objects off parameter values, like this:

public XmlCmd SetOptionValue(string optionName, string optionValue)
{
    var parameters = new[]
        {
            _parameterFactory.Create("name", optionName, XmlCmdParameterType.StringParam),
            _parameterFactory.Create("value", optionValue, XmlCmdParameterType.StringParam)
        };

    return new XmlCmd("SetOptionValue", parameters);
}

Then in the calling code, I can eliminate all the inline XML and string concatenation and replace it all with a single method call:

var builder = new XmlCmdBuilder();
builder.AddCommand(_cmdFactory.SetOptionValue("OptionName", "OptionValue"));

Then I can continue "building" the xml by adding subsequent AddCommand calls, and when I need to, I just call ToString() and pass the generated XML string to the 3rd-party API that knows what to do with it.


I have put these objects in their own class library, which needs a strong name key and ends up deployed in the GAC / Global Assembly Cache, so I want this library to be as "immune to change" as possible; one potential issue is with the XmlCmdFactory class: the commands are predefined, but I haven't implemented all of them and I might need to change it (add more methods) in the future.

I'd like some feedback on the XmlCmdBuilder and XmlCmd classes (XmlCmdParameter is built the same way, listing it here, along with the rest of the classes, would be almost redundant), and also some pointers in terms of extensibility, not so much about the code itself, but about adding more factory methods: something doesn't feel right about deploying a new version of this library to the GAC whenever a new XML command is needed.

share|improve this question
    
Diy you try to use enumerations for all of your commands? –  Artur Mustafin Jan 15 at 14:04
    
I'm open to all ideas :) –  Mat's Mug Jan 15 at 14:04
3  
Ok, i'll give it to you my impression of what it might be –  Artur Mustafin Jan 15 at 14:05
2  
Does each command have a fixed set of parameter names and types, and fixed number of parameters? Or are methods overloaded with different types (e.g. does SetOptionValue also support int parameters)? –  ChrisW Jan 15 at 15:21
1  
@ChrisW Most commands have 1 specific way of being configured, but some might have 2-3 overloads with varying parameters and parameter types. –  Mat's Mug Jan 15 at 15:39
show 2 more comments

4 Answers 4

up vote 3 down vote accepted

I think there is almost never any need to write inline XML. Instead, you should use XElements and related types to represent XML. It will make your code simpler and safer (have you considered injection attacks?).

Another point is that it would be nice if your objects could be easily created using the object initializer syntax. For that, you need to implement IEnumerable and have method called Add(), but both are natural for your types.

Together, it would look something like:

public class XmlCmdBuilder : IEnumerable<XmlCmd>
{
    public static readonly XNamespace Namespace = "http://www.contoso.com/XmlCommand";

    private readonly IList<XmlCmd> _xmlCommands = new List<XmlCmd>();

    public XmlCmdBuilder()
    {
        _xmlCommands = new List<XmlCmd>();
    }

    public void Add(XmlCmd cmd)
    {
        _xmlCommands.Add(cmd);
    }

    public void Clear()
    {
        _xmlCommands.Clear();
    }

    public XElement GetXml()
    {
        return new XElement(
            Namespace + "Commands",
            new XAttribute(XNamespace.Xmlns + "cmd", Namespace),
            _xmlCommands.Select(cmd => cmd.GetXml()));
    }

    public IEnumerator<XmlCmd> GetEnumerator()
    {
        return _xmlCommands.GetEnumerator();
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return GetEnumerator();
    }
}

public class XmlCmd : IEnumerable<XmlCmdParameter>
{
    public static readonly XNamespace Namespace = XmlCmdBuilder.Namespace;

    private readonly IList<XmlCmdParameter> _parameters;

    public XmlCmd(string name)
        : this(name, new List<XmlCmdParameter>())
    {}

    public XmlCmd(string name, params XmlCmdParameter[] parameters)
        : this(name, parameters.ToList())
    {}

    public XmlCmd(string name, IList<XmlCmdParameter> parameters)
    {
        _parameters = parameters;
        Name = name;
    }

    public string Name { get; private set; }

    public void Add(XmlCmdParameter parameter)
    {
        _parameters.Add(parameter);
    }

    public XElement GetXml()
    {
        return new XElement(
            Namespace + "Command",
            new XAttribute("name", Name),
            _parameters.Select(p => p.GetXml()));
    }

    public IEnumerator<XmlCmdParameter> GetEnumerator()
    {
        return _parameters.GetEnumerator();
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return GetEnumerator();
    }
}

Usage could then look like this:

var builder = new XmlCmdBuilder
{
    new XmlCmd("cmd1") { new XmlCmdParameter(), new XmlCmdParameter() },
    new XmlCmd("cmd2")
};

Console.WriteLine(builder.GetXml());

Which outputs exactly what you'd expect:

<cmd:Commands xmlns:cmd="http://www.contoso.com/XmlCommand">
  <cmd:Command name="cmd1">
    <cmd:Parameter />
    <cmd:Parameter />
  </cmd:Command>
  <cmd:Command name="cmd2" />
</cmd:Commands>
share|improve this answer
1  
I was waiting for Linq-to-XML to get mentioned! I like that :) –  Mat's Mug Jan 15 at 15:45
add comment

You have to provide interfaces API, which helps users of your framework to add their custom interface implementations, and you have to replace default .ToString() ovverrides for readability and compatibility. In common, all is just fine.

Generally, you can provide any of the containers for user project extensibility (System.AddIn, for example) by using attributes, as a bonus, you can then control the versioning of you assembiles in GAC, and gain the ability to mark interfaces, classes and methods as [Obsolete] or [Deprecated] attributes

Also, if you provide an inteface-only assemblies with no of minimum dependencies of other project assembiles, you will keep in mind loose-coupling in design, which is good. Additinally, you can create reference interface implementations as a reference to developers in the future which are always work as a whatchdogs in deprecated classes.

public interface IXmlCmdBuilder
{
    string Xml { get; }
}

public interface IXmlCmd
{
    string Xml { get; }
}

public interface IXmlCmdParameter
{
    string Xml { get; }
}

as it listed here:

using System;
using System.Collections.Generic;
using System.Drawing;
using System.Linq;
using System.Net;
using System.Net.Sockets;
using System.Text;
using System.Threading.Tasks;

namespace Server
{
    public interface IXmlCmdBuilder
    {
        string Xml { get; }
    }

    public interface IXmlCmd
    {
        string Xml { get; }
    }

    public interface IXmlCmdParameter
    {
        string Xml { get; }
    }

    public sealed class XmlCmdBuilder : IXmlCmdBuilder
    {
        private readonly IList<IXmlCmd> _xmlCommands;

        public XmlCmdBuilder()
        {
            _xmlCommands = new List<IXmlCmd>();
        }

        public void AddCommand(IXmlCmd cmd)
        {
            _xmlCommands.Add(cmd);
        }

        public void Clear()
        {
            _xmlCommands.Clear();
        }

        public override string ToString()
        {
            return Xml;
        }

        public string Xml
        {
            get
            {
                var builder = new StringBuilder();

                builder.Append("<cmd:Commands xmlns:cmd=\"http://www.contoso.com/Xml\">");
                foreach (var xmlCommand in _xmlCommands)
                {
                    builder.Append(xmlCommand.Xml);
                }
                builder.Append("</cmd:Commands>");

                return builder.ToString();
            }
        }
    }

    public sealed class XmlCmd : IXmlCmd
    {
        private readonly IList<IXmlCmdParameter> _parameters;

        public XmlCmd(string name)
            : this(name, new List<IXmlCmdParameter>())
        {

        }

        public XmlCmd(string name, params IXmlCmdParameter[] parameters)
            : this(name, parameters.ToList())
        {

        }

        public XmlCmd(string name, IList<IXmlCmdParameter> parameters)
        {
            _parameters = parameters;
            Name = name;
        }

        public string Name { get; private set; }

        public void AddParameter(IXmlCmdParameter parameter)
        {
            _parameters.Add(parameter);
        }

        public class XmlCmdParameter : IXmlCmdParameter
        {
            public string Xml { get; private set; }

            public override string ToString()
            {
                return Xml;
            }
        }

        public override string ToString()
        {
            return Xml;
        }

        public string Xml
        {
            get
            {
                var builder = new StringBuilder();

                builder.Append(string.Format("<cmd:Command name=\"{0}\">", Name));
                foreach (var xmlCommandParameter in _parameters)
                {
                    builder.Append(xmlCommandParameter.Xml);
                }
                builder.Append("</cmd:Command>");

                return builder.ToString();
            }
        }
    }

    internal class Program
    {
        private static void Main(string[] args)
        {
            XmlCmdBuilder builder = new XmlCmdBuilder();
            builder.AddCommand(new XmlCmd(""));
            Console.WriteLine(builder.Xml);
        }
    }
}
share|improve this answer
1  
Doesn't 3 identical interfaces feel wrong? –  Mat's Mug Jan 15 at 18:49
1  
@retailcoder Not really, it is possible to provide a commin interface, but it will be completely meaningless and about semantics only, i.e you can derive from IXmlString { string Xml { get; } } but then all of 2 interfaces will immediately be empty interfaces, which is not good, in reality it is better to rename Xml interface members to apropriate XmlCommand, XmlCommandParamter, XmlCommandBuilder which is not very readable in corresponding independent interfaces, but still while all of that 3 interfaces are not derived from each other, it is not essential, so Xml is OK for naming. It is short. –  Artur Mustafin Jan 16 at 5:25
add comment

something doesn't feel right about deploying a new version of this library to the GAC whenever a new XML command is needed

XmlCmd has public constructors, so you don't need factory methods: people can construct new XmlCmd without new factory methods in the GAC.

And/or perhaps you can add new factory methods to the GAC without changing the previous/existing factory class in the GAC, by coding the new factory methods as "extension methods" of your XmlCmdFactory.

Also you may be able to devise a XmlCmd constructor which makes it easier for users to create their own new factory methods; for example, something like this:

public XmlCmd(string[] names, params object[] parameterValues)
{
    this.Name = names[0]; // command name is first, parameter names are next
    assert(names.Length - 1 == parameterValues.Length);
    for (int i = 0; i < parameterValues.Length; ++i)
        _parameters.Add(CreateParameter(names[i + 1], parameterValues[i]));
}
private XmlCmdParameter CreateParameter(string name, object value)
{
    if (value is string)
    {
        return _parameterFactory.Create(pname, value, XmlCmdParameterType.StringParam)
    }
    ... etc. for other parameter types
}

Which you could call from a factory method like this:

public XmlCmd SetOptionValue(string optionName, string optionValue)
{
    string[] names = new string[] { "SetOptionValue", "name", "value" };
    return new XmlCmd(names, optionName, optionValue);
}

An advantage of this would be that it would hide (inside the XmlCmd constructor) the XmlCmdParameterType and whatever complexity there may be in converting non-string values (e.g. DateTime or whatever) to an XML string.

share|improve this answer
    
Additionally, in real situation, applying loose-coulping with interfaces is a good idea, if it is not intended to allow users create their own interface implementations, it is possible to use sealing for the classes and declare members internallly within the assembly –  Artur Mustafin Jan 16 at 7:31
add comment

not sure exactly how this will be used, so this might be a null point.

you have add methods but you don't have any remove methods. not sure it would make extension easier, but it would probably provide you with a way to remove deprecated commands (you may or may not want to do this because of backwards compatibility in the future. but hey let someone else worry about that).

Let's say that you implement this in a GUI (in the future - sans aliens), you would want a remove method so that your Interface could show you what you have created so far and you could remove something that is an error.

maybe even a replace method for replacing a command that is broken again I am thinking of an interactive interface.

I always think of user usability, so the interface would allow you to create the XML by giving parameters, strings, etc.

maybe I am going off on a bunny trail, but I think it would be nice to have those methods so that you don't have to add them later.

share|improve this answer
    
I won't downvote as requested, but the idea is basically "build some xml and pass it to the 3rd-party API" - I don't need to get fancy with removing commands :) –  Mat's Mug Jan 15 at 14:48
3  
I think adding a Remove and a Replace method because it would be nice to have those methods so that I don't have to add them later smells like YAGNI... –  Mat's Mug Jan 15 at 14:56
    
@retailcoder, good point. I am always thinking about what the next step is going to be, which is sometimes counter-productive. –  Malachi Jan 15 at 15:04
1  
there are some agreement with the statemens, really need some code examples to prove you point of view, in my opinion, your answer is 60% agains 40% correct, so mostly i agreed that depending of the usage, framework developer MUST keep in mind cases of usability and appliance of provided public framework assembly for the users, so it is a plus –  Artur Mustafin Jan 16 at 7:34
add comment

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.