Skip to content

implemented CycleDetectionII code in LinkedList #1482

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Jan 4, 2024
Merged

implemented CycleDetectionII code in LinkedList #1482

merged 13 commits into from
Jan 4, 2024

Conversation

Akshay73c
Copy link
Contributor

@Akshay73c Akshay73c commented Oct 8, 2023 •
edited
Loading

Hey maintainers,
I've implemented the code for getting the starting node of cycle in a linked list which i think will be useful for understanding of cycle-type-questions.
I've also added the tests to check the correctness.
It'll be my first open source contribution and also in Hacktoberfest. Hoping a success :)

Changes:

  • Added a code to get the starting node of cycle in LinkedList

Checklist:

I've read the CONTRIBUTING.md file and also checked the testcases using npm as asked to.
This pr is all my own work.

@Akshay73c Akshay73c changed the title implemented CycleTectionII code implemented CycleDetectionII code in LinkedList Oct 8, 2023
appgurueu
appgurueu reviewed Oct 9, 2023
View reviewed changes
Copy link
Collaborator

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

  • The code is still messy (mostly formatting-wise, the lengthCycle function should be defined before use for better readability, IMO). Please add comments, and give better variable names (i.e. I assume f and s are fast and slow; why do you need them when you already have fast and slow?).
  • As for the tests, remove the 3 redundant comments there which just repeat test case names.

@Akshay73c
Copy link
Contributor Author

Dear maintainers,
Thanks for the feedback !! I appreciate you taking the time to look over the code and provide suggestions.
I've made the following changes as suggested:

  • Added comments explaining the logic
  • you're absolutely right that using f and s is redundant when I already have fast and slow pointers
  • removed the redundant comments in tests
  • In my opinion, for this problem the key focus is detecting the cycle start node, so I wanted to keep that logic upfront in detectCycleNode(). Defining the helper lengthCycle() function below separates it out from the main detection logic. That said, I'm open to rearranging if you feel strongly that defining lengthCycle() first improves the overall structure.

Let me know! And thanks again for the review :)

I wanted to reach out to let you know that while attempting my first contribution to this open source project, I ran into some issues with git push. (That's why so many commits) After doing some research, I was able to resolve the issues and successfully contribute my changes.
I realize this may not have been the most elegant first contribution, and I wanted to apologize in advance if I caused any inconvenience. As a first time contributor, I was still learning the contribution workflow.

@Akshay73c
Copy link
Contributor Author

Hey maintainers, I'd be really glad if you could review this ;)

appgurueu
appgurueu requested changes Dec 2, 2023
View reviewed changes
Copy link
Collaborator

@appgurueu appgurueu left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Of course! This seems to work, but there is definitely dead / unnecessary code, and why the code is doing what it is doing is not exactly clear. Also some minor stylistic remarks.

@Akshay73c
Copy link
Contributor Author

Dear maintainers,
Thanks for reviewing again! I appreciate you taking this much time to provide suggestions for a beginner like me.
I've made few minor changes to the code and also explained my thought process and addressed to your valuable reviews. Please let me know if anything is required.
And am really sorry for the delays as i was occupied with campus interviews and exams. (am looking for my first internship)

@Akshay73c
Copy link
Contributor Author

Please let me know if there's anything more that needs to be addressed. Fingers crossed for the merge ;)

appgurueu
appgurueu reviewed Jan 3, 2024
View reviewed changes
appgurueu
appgurueu previously approved these changes Jan 3, 2024
View reviewed changes
@appgurueu
Copy link
Collaborator

Sorry for the long delay. I simplified the code a bit and renamed some variables. I hope you're fine with this, otherwise feel free to complain :)

LGTM!

@Akshay73c
Copy link
Contributor Author

Sorry for the long delay. I simplified the code a bit and renamed some variables. I hope you're fine with this, otherwise feel free to complain :)

LGTM!

LGTM too! Thanks for simplifying things, and no complaints here :))

@raklaptudirm raklaptudirm merged commit 5a7e8d1 into TheAlgorithms:master Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@appgurueu appgurueu appgurueu approved these changes

@raklaptudirm raklaptudirm raklaptudirm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Akshay73c @appgurueu @raklaptudirm