FxCop Complaint: Exposed concrete xml types and a bad improvement

Question

I want to save certain classes and since xml-serialization won't do it in my case i'm saving the values manually into an xml-document. Works fine, but FxCop doesn't like it and since FxCop normally gives good advice and reasons why i shouldn't do things in a certain way i try to keep it happy.

This time, i dont understand how this is an improvement.

This is what i had:

public void Save()
{
      XmlDocument doc = new XmlDocument();
      XmlNode XmlNodeJob = doc.CreateElement("Job");
      doc.AppendChild(XmlNodeJob);
      OtherclassSave2(XmlNodeJob);//Node as Parameter
 }

 public void OtherclassSave2(XmlNode node)
 {

 }

And this is what FxCop complained: "Modify member 'OtherclassSave2(XmlNode)' so that it no longer exposes the concrete type 'XmlNode'. Use IXPathNavigable to represent XML data sources."

And now my awesome solution:

    public void Save()
    {
        XmlDocument doc = new XmlDocument();
        XmlNode XmlNodeJob = doc.CreateElement("Job");
        doc.AppendChild(XmlNodeJob);
        OtherclassSave2(XmlNodeJob.CreateNavigator());//Interface from a node's navigator
    }

    public void OtherclassSave2(IXPathNavigable nav)
    {
        XmlNode node = (XmlNode)(nav.CreateNavigator().UnderlyingObject);

    }

This way i get my node in the other method and FxCop is happy, but i really don't see the improvement and i need a node to add things in it, not something to read.

I though about changing void SaveInThisNode(XmlNode) into a XmlNode GetMeTheNode() but to create nodes via CreateElements, i need the XmlDocument-object which i am not allowed to use as a parameter, but i could create new XmlDocuments in every step, fine.

My solution was simple and worked fine for everything i wanted it to do, but FxCop does not seem to allow solutions that are not obviously worse and more complicated.

My understanding is that you're supposed to be throwing around that interface instead of the classes that implement it. Pretty common concept. Sometimes it makes things easier, other times, not so much. It's one of those "meh" guidelines. — Zenexer, Feb 08 '12 at 09:35
`XmlNode` implements `IXPathNavigable`, see y answer. You could fix the warning very easily. — ken2k, Feb 08 '12 at 10:08
You could also make the method internal to solve your problem, I guess. — Stefan Paul Noack, Feb 08 '12 at 10:24

ken2k · Answer 1 · 2012-02-08T10:25:48.223

FxCop is saying that you should use the interface instead of the concrete implementation of the interface. It probably detected that in your OtherclassSave2 method the parameter nav could be used as a IXPathNavigable without specifying the concrete implementation (only members exposed by IXPathNavigable are used).

As XmlNode implements IXPathNavigable, you should be able to write:

public void Save()
{
      XmlDocument doc = new XmlDocument();
      XmlNode XmlNodeJob = doc.CreateElement("Job");
      doc.AppendChild(XmlNodeJob);
      OtherclassSave2(XmlNodeJob);
 }

public void OtherclassSave2(IXPathNavigable node)
{
    // Deal with node using the interface only
}

Just to clarify why FxCop is saying that, here is the most common example of the issue FxCop detected:

Say you have:

public int Sum(List<int> parameter)
{
    int tmp = 0;
    foreach (int i in parameter)
    {
        tmp += i;
    }

    return i;
}

List<int> lst = new List<int> {3, 4, 5};
int sum = Sum(lst);

As Sum implementation does not use specific methods of the List<T> type, it's not a good idea to set the type of parameter as List<int> because it will limit the usage of your Sum method. As Sum implementation only use a foreach, it's preferable to write:

public int Sum(IEnumerable<int> parameter)
{
    int tmp = 0;
    foreach (int i in parameter)
    {
        tmp += i;
    }

    return i;
}

so you can call Sum with other types that List<T>: ObservableCollection<T>...etc.

score 2 · Answer 2 · edited Feb 08 '12 at 10:20

2

It is just suggesting that you do not couple yourself with the specific implementation of XmlNode in the method signature. This allows you to change the internal implementation without affecting anything using the class.

It is suggested that you can ignore the warning if you need specific functionality from the concrete class. If this is a public facing API you should try to decouple as much as you can which will give you the freedom to change implementation with less chance of changing the method signatures and thus forcing consumers of the API to change their implementation.

CA1059: Members should not expose certain concrete types

edited Feb 08 '12 at 10:20

John Saunders

160,644
26
247
397

answered Feb 08 '12 at 09:55

Bronumski

14,009
6
49
77

Since me and one other person sitting less than two meters away from me will be the only ones in the world using that method ever, i will just ignore that warning. Thank you =) – Otterprinz Feb 08 '12 at 09:59
I really think FxCop wouldn't have thrown this warning if specific functionalities from the concrete class were used. – ken2k Feb 08 '12 at 10:09
The interface itself does not have the required functionalities of the concrete class, i would need to explicitly convert it back into XmlNode to use it. – Otterprinz Feb 08 '12 at 10:33
@IswMA It is good practice to try and adhere to things such as FxCop where it makes sense such as public facing APIs (public facing could be within the same company) but it is not the be all and end all. If there are certain rules you don't agree with just exclude them. – Bronumski Feb 08 '12 at 12:09
And the likelihood of `XmlNode` being redefined is how close to zero? – ajeh Jan 18 '17 at 21:21

score 0 · Answer 3 · answered Feb 09 '12 at 10:45

0

I found LinqToXml using XElements to be exactly was i was looking for in a more simple and more powerful way with less FxCop and interface-trouble.

answered Feb 09 '12 at 10:45

Otterprinz

459
3
10

FxCop Complaint: Exposed concrete xml types and a bad improvement

3 Answers3