Gracefully handling xml api that returns error or success responses

Question

I am working with a third party XML api, which can only respond to api calls with a error or success xml response.

For example, here's an error xml response:

<?xml version="1.0" encoding="UTF-8"?>
<error>
    <message>
        <![CDATA[Unable to load XML document from resource ]]>
    </message>
    <exception>org.impalaframework.exception.ExecutionException</exception>
    <detail>
        <![CDATA[stack trace here... snipped]]>
    </detail>
</error>

and a sample success response:

<?xml version="1.0" encoding="UTF-8"?>
<success>
    <message>
        <![CDATA[Order successfully imported ]]>
    </message>
</success>

I have he following api wrapper class written:

public class ApiConnection
{
    /// <summary>
    /// Private class objects.
    /// </summary>
    private string apiUrl, apiUser, apiPass;

    /// <summary>
    /// Class constructor.
    /// </summary>
    /// <param name="_apiUrl"></param>
    /// <param name="_apiUser"></param>
    /// <param name="_apiPass"></param>
    public ApiConnection(string _apiUrl, string _apiUser, string _apiPass)
    {
        // Remember settings
        apiUrl = _apiUrl;
        apiUser = _apiUser;
        apiPass = _apiPass;
    }

    // todo: implement SyncNewOrder method
}

I want to implement this SyncNewOrder method, however knowing the api, the response could be success type or an error type.

What would be the best way to gracefully handle the api response (success or error).

So, I attempted the implementation like this:

public class ApiConnection
{
    // ... snipped ...

    public NewOrderResponse SyncNewOrder(NewOrderRequest newOrderRequest)
    {
        // login at api

        // call api with newOrderRequest
        // xml response string stored in: apiResult

        // check if response string contains the word 'error'
        if (apiResult.Contains("error"))
        {
            // Handle error response
            XmlSerializer serializer = new XmlSerializer(typeof(Error));
            using (TextReader reader = new StringReader(apiResult))
            {
                Error error = (Error)serializer.Deserialize(reader);
                return new NewOrderResponse()
                {
                    IsError = true,
                    ErrorMsg = error.Message
                };
            }
        }
        else
        {
            // Handle success response
            XmlSerializer serializer = new XmlSerializer(typeof(Success));
            using (TextReader reader = new StringReader(apiResult))
            {
                Success success = (Success)serializer.Deserialize(reader);
                return new NewOrderResponse()
                {
                    IsError = false,
                    Result = success.Message
                };
            }
        }
    }
}

public class NewOrderResponse
{
    bool IsError { get; set; }
    string ErrorMsg { get; set; }
    string Result { get; set; }
}

public class NewOrderRequest
{
    // ...
}

[XmlRoot(ElementName="error")]
public class Error
{
    [XmlElement(ElementName="message")]
    public string Message { get; set; }

    [XmlElement(ElementName="exception")]
    public string Exception { get; set; }

    [XmlElement(ElementName="detail")]
    public string Detail { get; set; }
}

[XmlRoot(ElementName="success")]
public class Success 
{
    [XmlElement(ElementName="message")]
    public string Message { get; set; }
}

Would this be considered the correct implementation to handle variable response (success/error)? Or is there a better/correct way to implement this?

Answer 1

This coding style is unusual for C#:

private string apiUrl, apiUser, apiPass;

The standard is to have one per line:

private string apiUrl;
private string apiUser;
private string apiPass;

---

Why the api prefix in apiUrl, apiUser, apiPass? The class is already called ApiConnection.

---

Underscores in front of variable names are only used for fields, not parameters. So this is doing it wrong:

public ApiConnection(string _apiUrl, string _apiUser, string _apiPass)
{
    // Remember settings
    apiUrl = _apiUrl;
    apiUser = _apiUser;
    apiPass = _apiPass;
}

---

To me it seems like SyncNewOrder does too much. Moreover, there is code duplication. It seems to me you can move some of it to a method like this:

private T GetValue<T>(string apiResult)
{
    XmlSerializer serializer = new XmlSerializer(typeof(T));
    using (TextReader reader = new StringReader(apiResult))
    {
        return (T)serializer.Deserialize(reader);
    }
}

That way you can reduce your code to something like this:

    if (apiResult.Contains("error"))
    {
        var error = GetValue<Error>(apiResult);
        return new NewOrderResponse()
        {
            IsError = true,
            ErrorMsg = error.Message
        };
    }
    else
    {
        var success = GetValue<Success>(apiResult);
        return new NewOrderResponse()
        {
            IsError = false,
            Result = success.Message
        };
    }

I'm sure that code can be improved even more, considering there's still duplication.

---

Always explicitly state the access modifiers. I should not need to figure out myself what the scope of IsError etc will be.

public class NewOrderResponse
{
    bool IsError { get; set; }
    string ErrorMsg { get; set; }
    string Result { get; set; }
}

---

Comments like // Handle success response are superfluous. Your code should tell me what it does, comments are used to tell me why it was implemented like this (if necessary).