Skip to content

Conversation

@kasperk81
Copy link
Contributor

complex linq expressions, multiple dictionaries and recursion is hardly necessary for this simple command

@ghost ghost added Area-CLI untriaged Request triage from a team member labels Jan 8, 2025
@kasperk81
Copy link
Contributor Author

@Forgind @baronfel

Copy link
Contributor

@Forgind Forgind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really have a strong preference between these two versions, to be honest. I think edvilme's version is a bit more understandable, especially as I normally think tree structures lend themselves well to recursive procedures. kasperk81's version should be faster and allocate less.

@kasperk81
Copy link
Contributor Author

I don't really have a strong preference between these two versions, to be honest. I think edvilme's version is a bit more understandable, especially as I normally think tree structures lend themselves well to recursive procedures. kasperk81's version should be faster and allocate less.

using .Where.GroupBy.ToDictionary then keeping track of caches in other dictionaries with a recursive call is significantly worse than this simple loop in every regard. this version offers implementation that has less lines of codes, less calls, performant and reduced allocations

nonFolderDescendants += current.Files?.Count ?? 0;
foreach (var child in solution.SolutionItems)
{
if (child is { Parent: var parent } && parent == current)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice property pattern :)

@edvilme
Copy link
Contributor

edvilme commented Jan 9, 2025

This looks good to me, my main concern would most likely be readability, but I understand how many of the operations can come at a cost, especially for really large solution files

@kasperk81
Copy link
Contributor Author

i'm not sure i'm following. this version is more readable with a single loop compared to multiple linq expressions, caching and recursion

@edvilme edvilme merged commit 6319104 into dotnet:release/9.0.2xx Jan 9, 2025
29 of 31 checks passed
@kasperk81 kasperk81 deleted the patch-7 branch January 10, 2025 03:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-CLI untriaged Request triage from a team member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants