1
\$\begingroup\$

I am trying to get a complete tree in a single loop using an iterative function. For instance, this is what I have:

var someVariable = GetAllChildPositions(posId) //Method called from a console method.



public ICollection<Position> GetAllChildPositions(int posId)
{
    ICollection<Position> posCollection = new Collection<Position>();
    posCollection= GetChildPos(posId,posCollection);
    return posCollection;
}

public ICollection<Position> GetChildPos(int posId, ICollection<Position> posCollection) 
{
    var posObject= serviceRepo.Position.GetById(posId);
    posCollection.Add(posObject);
    if (serviceRepo.Position.GetAll().Where(x => x.ParentPosId == posId).Any())
    {
        var childPositions = serviceRepo.Position().GetAll().Where(x => x.ParentPosId == posId);
        foreach (var childPos in childPositions )
        {
            GetChildRows(childPos.Id, posCollection);
        }
    }
    return posCollection;
}

I am trying to get a list/collection child positions for a given parent position Id. This method works quite well and gets the job done albeit a little slowly. I am interested in knowing if there is a better way of doing this.

\$\endgroup\$
6
  • \$\begingroup\$ Please ensure that you have posted real working code, or an excerpt of real working code. Obfuscation is discouraged; see How to Ask. \$\endgroup\$ Jul 27, 2017 at 1:06
  • \$\begingroup\$ The only thing I have obfuscated in the code is the variable names. Apart from that rest all code are actually that is present on the testing server which is undergoing testing. Can you point me where the code is broken or not written ? I am adding some updates to the code let me know if it helps. \$\endgroup\$
    – vikramjb
    Jul 28, 2017 at 2:59
  • \$\begingroup\$ I had closed the question because DatabsaeCall() was a clear sign that this was fake code. Thanks for editing; I have reopened the question. \$\endgroup\$ Jul 28, 2017 at 3:27
  • 1
    \$\begingroup\$ @t3chb0t: I think his code does work structurally - he passes in reference to a collection to which objects are added and then he passes that reference back. I agree that it's ugly and not necessary but it still works. And Collection<T> does exist. \$\endgroup\$
    – ChrisWue
    Aug 7, 2017 at 0:37
  • \$\begingroup\$ @ChrisWue ok, my bad, sorry ;-] \$\endgroup\$
    – t3chb0t
    Aug 7, 2017 at 4:14

3 Answers 3

3
\$\begingroup\$

If Position and Position() in your code is the same and you supposed to call GetChildPos instead of GetChildRows in the inner loop then your code can be significantly simplified:

var someVariable = GetChildPos(posId).ToList();

public IEnumerable<Position> GetChildPos(int posId)
{
    yield return serviceRepo.Position.GetById(posId);

    foreach (var pos in serviceRepo.Position().GetAll()
                                              .Where(x => x.ParentPosId == posId)
                                              .SelectMany(x => GetChildPos(x.Id)))
    {
        yield return pos;
    }
}

Also it can improve performance because in your original code you do this instructions

serviceRepo.Position().GetAll().Where(x => x.ParentPosId == posId)

two times that can cause slowing down if some method in this chain is not lazy and executes heavy code even for Any().

\$\endgroup\$
6
  • \$\begingroup\$ This can be even further improved by passing the actual Position object as parameter instead of having to call GetById every single time. Just needs a wrapper methods which calls GetById for the root once \$\endgroup\$
    – ChrisWue
    Aug 7, 2017 at 0:51
  • \$\begingroup\$ @ChrisWue Yes, agree with you. Thanks for the note. \$\endgroup\$
    – Maxim
    Aug 7, 2017 at 1:20
  • \$\begingroup\$ Thank you for that. While your code is way cleaner than what I have done, it would seems DawidP is way faster than the yield method. I am gonna try and follow @ChrisWue suggestion and see if it makes a difference \$\endgroup\$
    – vikramjb
    Aug 7, 2017 at 4:55
  • \$\begingroup\$ Did you test that my code slower? I don't see anything that can make it to run slowly than solution of @DawidP. He use LINQ query as well and also has the same query repeated twice as in your code :) \$\endgroup\$
    – Maxim
    Aug 7, 2017 at 5:46
  • \$\begingroup\$ @Maxim, that's what made it so strange. I am checking it further to make sure I did not make stupid mistakes \$\endgroup\$
    – vikramjb
    Aug 8, 2017 at 1:12
2
\$\begingroup\$

I think @Maxim's answer should yield a reasonable improvement and as mentioned in the comment you could pass in the Position object directly saving a call to GetById every single time.

One consideration not mentioned: If GetAll actually returns all Positions in a non-lazy fashion then you'd be better off putting the Positions into a Dictionary<int, Position> keyed by their id and performing the search in a single loop by either using a stack or a queue (depth first vs breadth first)

\$\endgroup\$
1
  • \$\begingroup\$ It did improve compared to my method but falls short of DawidP method. I am going to try your suggestion of passing the object and see if it makes a difference. \$\endgroup\$
    – vikramjb
    Aug 7, 2017 at 4:57
2
\$\begingroup\$

What really would speed this up would be to store the result of serviceRepo.Position().GetAll() into a variable and pass this as an argument to the desired method. In its current state you query for the "root" item at least one time(.Any()) but only if there aren't any children.

If there are children, you make this call a second time and for each child you do at least one more call.
So for a tree like this

      A
     / \
    B   C
   / \
  D   E
     / \
    F   G

This call would happen 10 times.


If we would integrate this into @Maxim's answer we would get

public IEnumerable<Position> GetChildPos(int posId)
{
    var positions = serviceRepo.Position().GetAll()
    return GetChildPos(posId, positions);
}  

public IEnumerable<Position> GetChildPos(int posId, IEnumerable<SomeObject> positions)
{
    yield return serviceRepo.Position.GetById(posId);

    foreach (var pos in positions.Where(x => x.ParentPosId == posId)
                                 .SelectMany(x => GetChildPos(x.Id, positions)))
    {
        yield return pos;
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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