Jtdylktuy

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);

}

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.

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");

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:TempOriginal.xml");

        doc.RemoveXMLNode("Questions", "quPage", "PAGE60");

        doc.Save(@"C:TempNewXmlDoc.xml");

    }

}