8
\$\begingroup\$

I read somewhere that you should try to have as few method arguments as possible to make code easier to read, understand and use. I agree with this to a point but I'm not sure how I can make the following method easier. I think all 5 of the arguments are required.

Should I leave it as is? I think, if I was to add some comments, I may remember what this does if I come back to it later but there's a chance I won't know exactly what each argument is for.

public static void RemoveXMLNode(string pathToDocument, string descendant, string element, string elementValue,string newDocumentPath)
{
    var xDoc = XDocument.Load(pathToDocument);
    xDoc.Descendants(descendant)
    .Where(n => (string)n.Element(element) == elementValue)
    .Remove();
    xDoc.Save(newDocumentPath);
}
\$\endgroup\$
12
\$\begingroup\$

The method has definitely too many arguments and the reason for that is because it does too much. When you look at its name you could think it removes a XML node but under the cover it does three things:

  • it loads a document
  • it then does its job of removing a node
  • it then saves the document under new name

If you properly separated these three concerns your APIs would have much simpler signatures.

\$\endgroup\$
  • \$\begingroup\$ Ok I have edited my post to try and refactor. I think it is an improvement but I still seem to need 4 arguments on my node removal part. \$\endgroup\$ – Syntax Error Feb 15 at 11:32
  • \$\begingroup\$ @SyntaxError you're on the right track ;-) but unfortunatelly it's not allowed to add the improved version to the question. You can post a self-answer or a follow-up if you want us to take another look at your new code ;-] I had to rollback your edit. \$\endgroup\$ – t3chb0t Feb 15 at 11:34
  • \$\begingroup\$ Oh ok i didn't know that! Posted an answer instead. Thanks :) \$\endgroup\$ – Syntax Error Feb 15 at 11:40
3
\$\begingroup\$

As a follow-up to t3chb0t's and your self-answer, I agree that whilst the original RemoveXMLNode method you have written in your question is technically a one-liner, it is doing a lot.

In response to your self-answer, I have a few pointer:

  • In your case, I don't think you need a separate LoadXMLDocument method. The line XDocument.load(path) is easy enough to understand.
  • Regarding you new RemoveXMLNode method, you have two options:

1. Pass by Reference

In your new method, you are requesting an XDocument argument. Whilst XDocument is a reference type and can be modified through its public methods, you may benefit from using the ref keyword so that you pass the whole object as a reference. This means that any changes to the object you are passing happen on the original object. So to apply this, you would simply change the method arguments to:

                               // VVV - Note the 'ref' keyword!
private static void RemoveXMLNode(ref XDocument doc, string descendant, string element, string elementValue)

You would then use the method like so:

           // VVV - Note the 'ref' keyword!
RemoveXMLNode(ref doc, "Questions", "quPage", "PAGE60");

You can find more information on the difference between Pass By Reference and Pass By Copy over here.

There is also some more information on passing Reference Type classes from Jon Skeet's article over here.

I also wrote some dummy code to try out and demonstrate this behaviour over here.

2. Extension Method

This simply creates an extension method for your XDocument object. You do so by telling the compiler which of your arguments' object you are extending using the this keyword. When applying, you will actually reduce the number of required parameters by 1, effectively making it shorter.

The only requirement for this option is that your extension method(s) must be in a non-generic and non-nested static class.

You would write your method like so:

                               // VVVV - Note the 'this' keyword!
private static void RemoveXMLNode(this XDocument doc, string descendant, string element, string elementValue)

Now, you would call this method like so:

// Only three arguments!
doc.RemoveXMLNode("Questions", "quPage", "PAGE60");
\$\endgroup\$
  • 4
    \$\begingroup\$ Regarding #1, you're wrong: because XDocument is a reference type, the only thing that gets copied is a reference to the XDocument instance, not the instance itself. \$\endgroup\$ – Pieter Witvoet Feb 15 at 12:36
  • \$\begingroup\$ @PieterWitvoet If you're referring to the fact that I mentioned that it's passing a copy, then (on further digging around) I agree and I will modify my answer. If you're referring to the use of ref, I don't agree, as it still works. \$\endgroup\$ – Tom Feb 15 at 12:57
  • 1
    \$\begingroup\$ Adding ref won't break the code, but you'll be passing a reference to a reference around for no good reason. It'll clutter the code, send a confusing signal to other programmers and has a (tiny) performance implication. All for no benefit. \$\endgroup\$ – Pieter Witvoet Feb 15 at 13:05
0
\$\begingroup\$

I went with the extension method in the end as it produced the least code

Extension method

public static class XDocumentExtensions
{
    public static void RemoveXMLNode(this XDocument doc, string descendant, string element, string elementValue)
    {
        doc.Descendants(descendant)
        .Where(n => (string)n.Element(element) == elementValue)
        .Remove();
    }
}

Main program

class Program
{
    static void Main(string[] args)
    {
        XDocument doc = XDocument.Load(@"C:\Temp\Original.xml");
        doc.RemoveXMLNode("Questions", "quPage", "PAGE60");
        doc.Save(@"C:\Temp\NewXmlDoc.xml");
    }
}
\$\endgroup\$

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.