Resolved Why my recursive function calling itself again after returning the value ?

[ram_rocks]

The problem is after the node.parentNode is null it's returning but it's going back to the GetPath function and executing and returning it. so what should I do? Due to this my program is running for a very long time

public static List<string> GetPath(XmlNode node, List<string> parentList)
        {
            if (node.ParentNode == null)
            {
                return parentList;
            }
            else
            {
                if (node.FirstChild.Name == "SHORT-NAME")
                {
                    parentList.Add("/" + node.FirstChild.InnerText);
                }

                parentList = GetPath(node.ParentNode, parentList); //RECURSIVE
                return parentList;
            }
        }

 

[Skydiver]

That's the nature of recursive functions. It'll call itself (line 14) until it hits a base condition (e.g. your lines 3-6), and then it has to unroll the call stack that got it to that base condition (lines 15).

Unless you have a very deep structure or you keep on calling this method multiple times, I doubt that this method is the cause of your slow runtime. It's a simple walk up the tree and should have O(log n) runtime characteristics. Or is the code that you presented above just a minimal repro code? In your actual code, are you doing a lot more between lines 9-12?

 

[ram_rocks]

That's the nature of recursive functions. It'll call itself (line 14) until it hits a base condition (e.g. your lines 3-6), and then it has to unroll the call stack that got it to that base condition (lines 15).

Unless you have a very deep structure or you keep on calling this method multiple times, I doubt that this method is the cause of your slow runtime. It's a simple walk up the tree and should have O(log n) runtime characteristics. Or is the code that you presented above just a minimal repro code? In your actual code, are you doing a lot more between lines 9-12?

The above code is the actual code and yes I am calling this in for loop like approximately 500 times..

 

[Skydiver]

I suspect that the higher up the tree you go, this gets larger and larger: node.FirstChild.InnerText.

Furthermore, you are redoing work that has already been done. Consider the following tree:

<A>
  <B>
    <SHORT-NAME>Child of B</SHORT-NAME>
    <C/>
    <D/>
    <E/>
  </B>
</A>

When <B> is processed, "Child of B" is added to the list. When <C> is processed, it'll go up to <B>, and then "Child of B" is added to the list again. This happens again for <D>, and <E>.

Now imagine if <C> had children, grand children, and great grand children. "Child of B" would be added for each of those descendants as well.

 

[jmcilhinney]

I cannot recommend strongly enough that you pick up a pen and paper and perform this process manually to see what the actual steps are, then use the same pen and paper to write those steps down and formalise them into an algorithm. You can then write code specifically to implement that algorithm, independent of the result you're trying to achieve. That way, when you debug your code, you can always compare the code and what it does to the algorithm and what it does to confirm that your code is as it should be. If you do that then you should not end up in the situation described above.

 

[ram_rocks]

Can you please tell me is the below code is a correct way to implement ?

Previous Implementation: Basically If the Inner text of the children element matches then I need all the parent element Inner text to create a path for my further use so In the previous code (mentioned above) I used a recursive function like if inner text matches then I am going all the way to top and storing all the parent related inner text in a list. Running this recursive function approx 500 times So as Skydiver mentioned higher up the tree you go, this gets larger and larger and this is causing delay.
Updated Implementation: I Implemented with for loop and it worked but I think the code is not perfect one because I thought to use ancestors() and to use this method
- I have to use XElement class so I have to first convert XMLNode to XElement -> [ XElement.Parse(node.OwnerDocument.OuterXml) ]
- Used Descendants to go through one by one until I find my node name and if I found then within that node I have go to firstnode and check Innertext, if it matches then going for ancestors.

I think I am doing something like you know up and down, up and down scenerio

public static List<string> GetPathList(XmlNode node, string signal)  // gets specific node
{
    List<string> pathList = new();
    XElement element = XElement.Parse(node.OwnerDocument.OuterXml);  // to convert Xelement and I used node.OwnerDocument.OuterXml to perform ancestors operation
    foreach (XElement elem in element.DescendantsAndSelf()) // even though I get specific node from one of the parameter I have to come again from top of the file and checking node name matches with my actual node coming from
    {                                                                                             // parameter because if use node.OuterXml on the top I will not get parent elements
        if (elem.Name.LocalName == node.Name)
        {
              XElement els = elem.Descendants().First();
              if ((els.Name.LocalName == "SHORT-NAME") && (els.Value == signal))
              {
                   foreach (XElement el in elem.Ancestors())
                   {
                        XElement snm = el.Descendants().First();
                        if (snm.Name.LocalName == "SHORT-NAME")
                               pathList .Add("/" + snm.Value);
                   }
                   break;
               }
          }
     }
      pathList.Reverse();  // need to reverse because from above it stores bottom to top
      return pathList;
}

I actually worked on easy step: when I placed a break point on elem I saw FirstNode and within firstNode -> Name (this is name of the first node of parent node) IF I use this I can eliminate elem.Descendants().First() and el.Descendants().First() line but the problem is when I am writing elem.FirstNode.Name it shows me error because of Inheritance. So I have only above code option to implement.

 

[Skydiver]

Please put your code in code tags. Press the icon on the toolbar that looks like </> and paste in your code.

 

[ram_rocks]

Please put your code in code tags. Press the icon on the toolbar that looks like </> and paste in your code.

Sorry I don't know that Edited in above window

 

[Skydiver]

I already edited it for you.

One thing that may help us out is if you used more meaningful names, or at least explain what you mean by the names you've chosen. You named your method GetPath(), but it doesn't return a single path. It returns a list of strings that are supposedly paths. Within the code, you have a list called parentList, but it's not a list of parent nodes, it looks a list of strings that contain paths. But the paths are not the paths of the parents, they look to be some kind of artificial path where each path string is a slash followed by the text value of the first child.

 

[ram_rocks]

I already edited it for you.

One thing that may help us out is if you used more meaningful names, or at least explain what you mean by the names you've chosen. You named your method GetPath(), but it doesn't return a single path. It returns a list of strings that are supposedly paths. Within the code, you have a list called parentList, but it's not a list of parent nodes, it looks a list of strings that contain paths. But the paths are not the paths of the parents, they look to be some kind of artificial path where each path string is a slash followed by the text value of the first child.

Please check code and naming too Does it makes sense now ?? or else give me some suggestions

 

[NoUserHere]

pathList.Reverse(); // need to reverse because from above it stores bottom to top

If you want the list in a given order, would you not be best using a collection that does that for you upon populating it instead of reversing the order after its populated?
Using the right type prevents you from having to reverse the order, if you use the proper collection type for the order you want.

 

[Skydiver]

In the future, please post new code as a new post. It will help with the continuity of a thread. This is a forum. It is not a Question and Answer site like Stack Overflow where you keep on massaging the question based on the comments and answers posted in an asynchronous manner.

Now, my post #9 looks like I was on drugs talking about some that doesn't exist. I'm not going on the good drugs until a couple of weeks from now when I go for surgery.

 

[Skydiver]

What is the significance of the signal parameter? When you call your new GetPathList() 500+ times, in your old code you obviously called it with 500+ different XMLNodes as the first parameter. Is the second parameter also 500+ different strings, or is it a single fixed string?

 

[ram_rocks]

String is a fixed one
Before coming to the Getpath first I am checking if the signal name is same as node firstchild (shortname) inner text then i am calling getpath to get path of the node (something like root text/children/children/node name)
The reason why i use it again is because working around XmlNode to Xelement conversion I have to again come from top to bottom as mentioned in above post.
Why conversion ? because i want to use ancestors() and its method of Xelement class

Working around XmlNode to Xelement is like we need to write some extra code for conversion

 

[Skydiver]

If it's a fixed string, don't pass it in as a parameter. Hard code it into the code to make your intent known.

But now you are doing the conversion 500+ times from XmlElement to XElement. Ouch!

And it's unclear how you will hit the correct node. Consider the following XML:

<PACKAGE>
    <SHORT-NAME>Oscar</SHORT-NAME>
    <PACKAGES>
        <PACKAGE>
            <SHORT-NAME>Oscar</SHORT-NAME>
        </PACKAGE>
    </PACKAGES>
</PACKAGE>

If you call GetPathList(node, "Oscar) where node was referencing the inner node "/PACKAGE/PACKAGES/PACKAGE", it would match the top level node "/PACKAGE".

It's still also unclear why you consider "/Oscar" a path as built up by line 16. Maybe I'm conflating XPath paths with what you are calling a path.