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

refactor: RotateListRight.js and added tests #1101

Merged
merged 3 commits into from
Sep 22, 2022

Conversation

10kartik
Copy link
Contributor

Open in Gitpod know more

Describe your change:

  • Add an algorithm?
  • Fix a bug or typo in an existing algorithm?
  • Documentation change?

Checklist:

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • All new JavaScript files are placed inside an existing directory.
  • All filenames should use the UpperCamelCase (PascalCase) style. There should be no spaces in filenames.
    Example:UserProfile.js is allowed but userprofile.js,Userprofile.js,user-Profile.js,userProfile.js are not
  • All new algorithms have a URL in its comments that points to Wikipedia or other similar explanation.
  • If this pull request resolves one or more open issues then the commit message contains Fixes: #{$ISSUE_NO}.

appgurueu
appgurueu previously approved these changes Sep 14, 2022
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.

Looks good. Ideally this should be a method of the SinglyLinkedList class though; then you would also be able to leverage the get method to obtain an array rather than having to do the tedious .head. You could additionally add a constructor that takes a list to shorten your list creation.

@appgurueu appgurueu added code quality Code quality improvement tests Adds or fixes tests; issue that points out bugs in the tests labels Sep 14, 2022
@10kartik 10kartik changed the title Refactored RotatedListRight.js and added its tests Refactored RotateListRight.js and added its tests Sep 14, 2022
@raklaptudirm
Copy link
Member

@appgurueu Should I await your suggestion being implemented before merging?

@appgurueu
Copy link
Collaborator

@appgurueu Should I await your suggestion being implemented before merging?

Not sure. The PR is definitely already an improvement in its current state, but the ideal solution would be what I have suggested.

@raklaptudirm
Copy link
Member

@10kartik Can you implement the suggested?

@10kartik
Copy link
Contributor Author

Yeah @raklaptudirm, I'll give it a try... But I'm a bit doubtful about the suggested implementation.

Ideally this should be a method of the SinglyLinkedList class though; then you would also be able to leverage the get method to obtain an array rather than having to do the tedious .head.

As per my understanding @appgurueu, adding a method RotateListRight to LinkedList class will narrow it's usage.

Let's say in a list of length 10, I want to rotate the list after the 4th node, so I'd simply pass the reference of the 5th node as head and k steps. But if I'll add a method in LinkedList class that only accepts k which rotates the list right from the first node, the method will simply lose the essence behind it.

Please correct me if I'm wrong or missing something here.

You could additionally add a constructor that takes a list to shorten your list creation.

For this, I must add a constructor to the LinkedList class, pass an array/list as an argument to it and call addLast to create a list, right?

@appgurueu
Copy link
Collaborator

As per my understanding @appgurueu, adding a method RotateListRight to LinkedList class will narrow it's usage.

Let's say in a list of length 10, I want to rotate the list after the 4th node, so I'd simply pass the reference of the 5th node as head and k steps. But if I'll add a method in LinkedList class that only accepts k which rotates the list right from the first node, the method will simply lose the essence behind it.

Please correct me if I'm wrong or missing something here.

Valid point.

The clean solution to that would be allowing "views" into sublists, comparable to Python or Go slices (although those work on array-based lists). Views would obviously again be linked lists and could thus have rotateListRight applied to them.

You could additionally add a constructor that takes a list to shorten your list creation.

For this, I must add a constructor to the LinkedList class, pass an array/list as an argument to it and call addLast to create a list, right?

Yes.

@10kartik
Copy link
Contributor Author

10kartik commented Sep 15, 2022

The clean solution to that would be allowing "views" into sublists, comparable to Python or Go slices (although those work on array-based lists). Views would obviously again be linked lists and could thus have rotateListRight applied to them.

Right! seems reasonable. Okay, so not sure how should I proceed with that 🤔. Should I shift the rotateRightList function logic to LinkedList 's method, rotate list by k nodes and reset the head accordingly? Before that, I will add a constructor for creating LinkedList... Sounds good?

@appgurueu
Copy link
Collaborator

The clean solution to that would be allowing "views" into sublists, comparable to Python or Go slices (although those work on array-based lists). Views would obviously again be linked lists and could thus have rotateListRight applied to them.

Right! seems reasonable. Okay, so not sure how should I proceed with that thinking. Should I shift the rotateRightList function logic to LinkedList 's method, rotate list by k nodes and reset the head accordingly? Before that, I will add a constructor for creating LinkedList... Sounds good?

Yes, sounds good. Methods to create views / sublists can come later (or not at all).

appgurueu
appgurueu previously approved these changes Sep 19, 2022
Data-Structures/Linked-List/SinglyLinkedList.js Outdated Show resolved Hide resolved
@raklaptudirm raklaptudirm changed the title Refactored RotateListRight.js and added its tests refactor: RotateListRight.js and added tests Sep 22, 2022
@raklaptudirm raklaptudirm merged commit 9bcf16b into TheAlgorithms:master Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Code quality improvement tests Adds or fixes tests; issue that points out bugs in the tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants