5
\$\begingroup\$

I have a HttpGet method which returns XML file:

[HttpGet]
public async Task<IActionResult> Get()
{
    XmlDocument xmlDoc = new XmlDocument();
    await Task.Run(() => xmldoc.Load(_appEnvironment.WebRootPath + @"\\user.config"));
    return Ok(xmlDoc);
}

Colleague said to me that this piece of code has smell and that I interrupted SOLID principles, but he did not explained to me what exact I do wrong.

Guys, please review my code and please explain me what I've done wrong.

\$\endgroup\$
  • 1
    \$\begingroup\$ Maybe ask your colleague what they mean exactly because just namedropping arbitrary comments like "you're violating SOLID" or "your code is not a GoF pattern" or something stupid like that is not constructive criticism. \$\endgroup\$ – Matti Virkkunen Jun 21 '17 at 14:22
  • \$\begingroup\$ @MattiVirkkunen he goes to vacation and promises in 2 weeks. \$\endgroup\$ – StepUp Jun 21 '17 at 14:26
8
\$\begingroup\$

Dependency Inversion Priciple

The smell is that your code violates the Dependency Inversion Priciple which reads

Entities must depend on abstractions not on concretions. It states that the high level module must not depend on the low level module, but they should depend on abstractions.

This meas that your API depends on the Load method of the XmlDocument and not an abstraction such as a service like this one.

interface IXmlLoader
{
    XmlDocument Load(string path);
}

In consequence you cannot replace it for testing or use dependency injection and easily exchange it later if necessary. A correct implementation could look like this where you pass the dependency via the constructor. In some cases it might be also a property:

class XmlController
{
    private readonly IXmlLoader _xmlLoader;
    public XmlControler(IXmlLoader xmlLoader)
    {
        _xmlLoader = xmlLoader;
    }

    // Sometimes a property.
    // public IXmlLoader XmlLoader { get; set; }

    [HttpGet]
    public async Task<IActionResult> Get()
    {
        await Task.Run(() => _xmlLoader.Load(_appEnvironment.WebRootPath + @"\\user.config"));
        return Ok(xmlDoc);
    }    

}

This is just an example. In your real code you should add null checks.


Another smaller smell is the hardcoded string user.config that you should at least configure via a property.

\$\endgroup\$
  • 1
    \$\begingroup\$ @StepUp there are various options... it could be a property of the controler or a value in some kind of a settings provider/dictionary/thing... \$\endgroup\$ – t3chb0t Jun 21 '17 at 9:52
  • 1
    \$\begingroup\$ @StepUp it might be, if you pass it via the constructor or otherwise it is as hardcoded as the current value. At least a public property would be fine if you don't want to use anything fancy. \$\endgroup\$ – t3chb0t Jun 21 '17 at 9:57
  • 1
    \$\begingroup\$ @StepUp sure, why not? It has a bunch of its own properties already (msdn.microsoft.com/en-us/library/…) See the example above. I included a property for the service. The one for the config name would just be of type string. \$\endgroup\$ – t3chb0t Jun 21 '17 at 10:40
  • 1
    \$\begingroup\$ I really cannot believe that I should create Interface and class to wrap up this just two lines of code. Is it really necessary just for two lines? \$\endgroup\$ – StepUp Jun 21 '17 at 14:14
  • 1
    \$\begingroup\$ @StepUp of course it's not necessary but you asked why it isn't SOLID so I showed you what is wrong. However if you want your application to be testable and for example use an xml file from an embeded resource inside a unit test project then you need an abstraction so that you can read it from somewhere else with a different service. This is how it works. It's up to you if you do it or not but I wouldn't allow your code into a production because it's practically untestable :-P \$\endgroup\$ – t3chb0t Jun 21 '17 at 17:29
4
\$\begingroup\$

It's hard to say exactly what you're doing "wrong" without a more concrete example, but for starters, I would argue that XML is merely a representation of an object, so passing an XMLDocument into Ok() does seem like a smell. At the very least, if you need to return an XML payload, use MVC formatters to serialize into XML from your response object (a Plain Old C# Object that you define).

(configured in Startup.cs):

mvcBuilder.AddMvcOptions(options => options.OutputFormatters.Add(
new Microsoft.AspNetCore.Mvc.Formatters.XmlDataContractSerializerOutputFormatter(
    new System.Xml.XmlWriterSettings() { NamespaceHandling = System.Xml.NamespaceHandling.OmitDuplicates, Async = true, OmitXmlDeclaration = false })));

See: Introduction to formatting response data in ASP.NET Core MVC

Also, DI is the name of the game here, so you do want to inject some sort of meaningful service, which would own the work of whatever logic you need and returns the appropriate object. I would never return that object directly to a consumer (encourages coupling), but that's purely a design decision based on the complexity involved and what you're trying to achieve.

Additionally, try your best to stay async/await all the way down. awaiting a Task.Run() will not break anything, but you may find extreme corner cases in the future if you continue down that path, and it's always best to let the compiler work it's magic where you can delegate more to it.

\$\endgroup\$
2
\$\begingroup\$

Parsing an XML document from disk just to later send it unchanged over the wire seems like a huge waste of CPU cycles. Why not simply send the file's content?

\$\endgroup\$
  • 1
    \$\begingroup\$ This is an interesing question but the answer is not so obvious I think and it depends... I could think of a few reasons why not to return a file directly (if it's not an image or other binary data): you might want to return it not only as xml but also a json or text later. You might want to modify the data on the way out and use a middleware that removes some sensitive information or transforms the document. \$\endgroup\$ – t3chb0t Aug 11 '17 at 18:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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