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

I have the following contract in a WCF-based web service:

public List<string> GetAllPossibleQueryEngineHostNames(Instance instance);

Obviously, I can call this and pass in an Instance object. However, I need to also support older versions of the client, which pass in a string like so:

string instance = "value";
svc.GetAllPossibleQueryEngineHostNames(instance);

To do this, I created a WCF behavior. This was new to me, which is why I'd appreciate some code review feedback. Note, right now only de-serialization is supported. Don't worry that WriteObjectContent is not implemented. I'll do that later, if I have the need.

public class InstanceSerializer : XmlObjectSerializer
{
    const string localName = "instance";

    public override bool IsStartObject(XmlDictionaryReader reader)
    {
        return String.Equals(reader.LocalName, localName, StringComparison.OrdinalIgnoreCase);
    }

    public override object ReadObject(XmlDictionaryReader reader, bool verifyObjectName)
    {
        string xml = reader.ReadOuterXml();
        XDocument doc = XDocument.Parse(xml);

        string shortCode = doc.Descendants()
            .Where(e => e.Name.LocalName == "ShortCode")
            .Select(e => e.Value)
            .FirstOrDefault();

        string connStr = doc.Descendants()
            .Where(e => e.Name.LocalName == "ConnectionString")
            .Select(e => e.Value)
            .FirstOrDefault();

        if (connStr != null || shortCode != null) // Instance passed as Instance object
        {
            return new Instance(shortCode, connStr);
        }

        // Instance passed as String
        Instance instance = ((XElement) doc.FirstNode).Value;
        return instance;
    }

    public override void WriteEndObject(XmlDictionaryWriter writer)
    {
        writer.WriteEndElement();
    }

    public override void WriteObjectContent(XmlDictionaryWriter writer, object graph)
    {
    }

    public override void WriteStartObject(XmlDictionaryWriter writer, object graph)
    {
        writer.WriteStartElement(localName);
    }
}

public class InstanceBehavior : DataContractSerializerOperationBehavior
{
    public InstanceBehavior(OperationDescription operation) : base(operation) { }

    public override XmlObjectSerializer CreateSerializer(Type type, string name, string ns, IList<Type> knownTypes)
    {
        return typeof(Instance) == type
            ? new InstanceSerializer()
            : base.CreateSerializer(type, name, ns, knownTypes);
    }

    public override XmlObjectSerializer CreateSerializer(Type type, XmlDictionaryString name, XmlDictionaryString ns, IList<Type> knownTypes)
    {
        return typeof(Instance) == type
            ? new InstanceSerializer()
            : base.CreateSerializer(type, name, ns, knownTypes);
    }
}

public class SupportStringInstanceAttribute : Attribute, IContractBehavior
{
    public void AddBindingParameters(ContractDescription contractDescription, ServiceEndpoint endpoint, System.ServiceModel.Channels.BindingParameterCollection bindingParameters)
    {
    }

    public void ApplyClientBehavior(ContractDescription contractDescription, ServiceEndpoint endpoint, System.ServiceModel.Dispatcher.ClientRuntime clientRuntime)
    {
        ReplaceSerializerOperationBehavior(contractDescription);
    }

    public void ApplyDispatchBehavior(ContractDescription contractDescription, ServiceEndpoint endpoint, System.ServiceModel.Dispatcher.DispatchRuntime dispatchRuntime)
    {
        ReplaceSerializerOperationBehavior(contractDescription);
    }

    public void Validate(ContractDescription contractDescription, ServiceEndpoint endpoint)
    {
    }

    private static void ReplaceSerializerOperationBehavior(ContractDescription contract)
    {
        foreach (OperationDescription od in contract.Operations)
        {
            for (int i = 0; i < od.Behaviors.Count; i++)
            {
                DataContractSerializerOperationBehavior dcsob = od.Behaviors[i] as DataContractSerializerOperationBehavior;
                if (dcsob != null)
                {
                    od.Behaviors[i] = new InstanceBehavior(od);
                }
            }
        }
    }
}
share|improve this question

I think your last method could look a tad cleaner with replacing long type names with var and not doing the null comparison (since dcsob is unused otherwise) and replacing it with a is comparison:

private static void ReplaceSerializerOperationBehavior(ContractDescription contract)
{
    foreach (var od in contract.Operations)
    {
        for (var i = 0; i < od.Behaviors.Count; i++)
        {
            if (od.Behaviors[i] is DataContractSerializerOperationBehavior)
            {
                od.Behaviors[i] = new InstanceBehavior(od);
            }
        }
    }
}
share|improve this answer
    
Cool. That method I got from a Blog anyway :) – Mike Christensen Aug 11 '14 at 22:05

When I see

    string shortCode = doc.Descendants()
        .Where(e => e.Name.LocalName == "ShortCode")
        .Select(e => e.Value)
        .FirstOrDefault();

    string connStr = doc.Descendants()
        .Where(e => e.Name.LocalName == "ConnectionString")
        .Select(e => e.Value)
        .FirstOrDefault();

I notice these are exactly the same except for the string e.Name.LocalName is being compared to. Follow the "Don't repeat yourself" pattern and extract the common code into a method. Also don't sprinkle string literals through your code; the literal strings should be replaced with const/readonly variables as you do with localname.

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.