Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactored and Included Tests for TopoSortFromBfsNodeMap #1133

Conversation

nathannaveen
Copy link
Contributor

@nathannaveen nathannaveen commented Aug 7, 2023

Description of the PR

  • Refactored the code for TopoSortFromBfsNodeMap for readability and time complexity purposes.
  • Included tests for TopoSortFromBfsNodeMap

In regards of improving the time complexity here is what I did:

I changed parentsMap from a map[string] []string to a map[string] parent, with parent being:

type parent struct {  
	parents map[string]bool // Consider the map[string]bool as a set, the value doesn't matter just the key  
}

So basically now parentsMap = map[string] map[string] bool. This so that we don't have to loop through every value in parentsMap. Doing this change will improve the time complexity by a lot, instead of looping through every value, we will only be doing an O(1) operation to remove each id from the maps.

I also added a map[string] []string called childrensMap, it is a map of parents to a list of children. This makes it so that we know which children have which parents. We can use this to only loop through the nodes that have a certain parent and not just loop through each and every node.

This may be affected by: #1132

PR Checklist

  • All commits have a Developer Certificate of Origin (DCO) -- they are generated using -s flag to git commit.
  • All new changes are covered by tests
  • If GraphQL schema is changed, make generate has been run
  • If collectsub protobuf has been changed, make proto has been run
  • All CI checks are passing (tests and formatting)
  • All dependent PRs have already been merged

@nathannaveen nathannaveen force-pushed the nathan/feat/refactorAndAddedTestsForToposort branch 3 times, most recently from 1f0a3dd to 9407373 Compare August 9, 2023 19:18
@nathannaveen nathannaveen force-pushed the nathan/feat/refactorAndAddedTestsForToposort branch from 9407373 to fc6ebca Compare August 9, 2023 19:25
@pxp928
Copy link
Collaborator

pxp928 commented Aug 9, 2023

@rmetzman Did you want to review this one as well?

@rmetzman
Copy link
Contributor

rmetzman commented Aug 9, 2023

Thank you! LGTM

Copy link
Collaborator

@pxp928 pxp928 left a comment

Choose a reason for hiding this comment

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

LGTM

@nathannaveen
Copy link
Contributor Author

@lumjjb, @mihaimaruseac 👀

pkg/guacanalytics/toposort_test.go Show resolved Hide resolved
@pxp928
Copy link
Collaborator

pxp928 commented Aug 15, 2023

@nathannaveen rebase will auto merge.

* Refactored the code for TopoSortFromBfsNodeMap for readability and time complexity purposes.
* Included tests for TopoSortFromBfsNodeMap

Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>
Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>
@nathannaveen nathannaveen force-pushed the nathan/feat/refactorAndAddedTestsForToposort branch from 82511e2 to a2178b5 Compare August 15, 2023 20:39
@nathannaveen
Copy link
Contributor Author

@nathannaveen rebase will auto merge.

Done, Thanks!

@kodiakhq kodiakhq bot merged commit 021655e into guacsec:main Aug 15, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants