2
\$\begingroup\$

I am currently creating an analyzer using the roslyn sdk. I ran into a case where it was simpler to check child tokens for Wait()/.Result.

The code i am sharing works perfectly fine - However i was unable to find a better way on google nor through browsing the docs.

This way of getting the previous sibling of a node feels very weird - Is there a better way?

        if (context.Node is IdentifierNameSyntax identifierName)
        {
            if (identifierName.Identifier.Text == TaskIdentifierWait || identifierName.Identifier.Text == TaskIdentifierResult)
            {
                var siblingsAndSelf = identifierName.Parent.ChildNodes().ToImmutableArray();
                var selfIndex = siblingsAndSelf.LastIndexOf(identifierName);
                if (selfIndex <= 0)
                    return;

                var previousSibling = siblingsAndSelf[selfIndex-1];
\$\endgroup\$

closed as off-topic by t3chb0t, Sᴀᴍ Onᴇᴌᴀ, Zeta, IEatBagels, Jamal Oct 27 '18 at 23:13

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – t3chb0t, Sᴀᴍ Onᴇᴌᴀ, Zeta, IEatBagels, Jamal
If this question can be reworded to fit the rules in the help center, please edit the question.

2
\$\begingroup\$

You could iterate over the ChildNodes only once with the Zip extension where the first collection starts at the first element and the other one at the second element with Skip(1). This will produce tuples with pairs such as Previous & Current. Now you can filter it and pick at the same time the Last element that satisfies the condition:

var childNodes = new[] { "a", "a", "b", "c", "a" };

var previousSibling =
    childNodes
        .Zip(childNodes.Skip(1), (p, c) => (Previous: p, Current: c))
        .Last(t => t.Current == "c").Previous; // b
\$\endgroup\$
  • \$\begingroup\$ I'm pretty sure you still iterate twice over childNodes. If I understand Zip correctly, it iterates over both arguments seperately, no matter if if's the same collection. So performance wise there is probably not much of an advantage, though readability is IMHO much less clear. \$\endgroup\$ – crazy_crank Oct 26 '18 at 9:56
  • \$\begingroup\$ @crazy_crank yeah, you're right, it does iterate twice or rather at the same time but without testing you cannot say which way is better especially that the question severly lacks context and I shouldn't even have answerd it in the first place so I VTC - I don't agree or readability because it's much better (unless someone doesn't get linq) \$\endgroup\$ – t3chb0t Oct 26 '18 at 10:00
  • 1
    \$\begingroup\$ totally correct. Using your method now in my context anyway. Wanna take back what I said about readillity though, your version is definitily cleaner. I just always find the Zip operator hard to read, but that's probably just because I use it so rarely \$\endgroup\$ – crazy_crank Oct 30 '18 at 9:00
  • 1
    \$\begingroup\$ @crazy_crank cool ;-) I think I'll create my own Zip that returns a tuple automatically becuase doing (p, c) => (Previous: p, Current: c) is such a common task that I wonder it's not already there and writing this every time feels unnecessary. \$\endgroup\$ – t3chb0t Oct 30 '18 at 9:05
  • 1
    \$\begingroup\$ @crazy_crank I think you should encapsulate this ugly expression with to extensions: node.Current() and node.Previous() - now your code is pretty again ;-] \$\endgroup\$ – t3chb0t Oct 30 '18 at 10:27

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