Adding sorting algorithms #29
Conversation
|
Please have a look at PR #22 that I made but is a draft right now due to the lack of comments. |
@dlesnoff Nice to meet you, and I am really sorry that I did not see your pull request because I made my PR on a train so I wanted to submit it as quickly as possible and did not check if there are similar PRs. However, will this still be merged because of your previous pull request? If not, I will restore my fork and start working on other algorithms. |
adding "case" to comments
There was a problem hiding this comment.
@dlesnoff Nice to meet you, and I am really sorry that I did not see your pull request because I made my PR on a train so I wanted to submit it as quickly as possible and did not check if there are similar PRs.
I like to code when in the train too ;) Do not worry, that happens.
Thanks for your kind first contribution, but I think that many issues must be addressed before merging them. I plan to merge your implementation when these are addressed. I like having a second implementation of these algorithms giving a comparison to mine.
In general, the comments duplicates what a developer sees from the code.
They should rather give insights on the «why» of the algorithm. Why two for loops?
Which part of the bubble sort corresponds to a bubble? What can be guaranteed after a «bubble» (a pass) on the list (give invariants)?
Most importantly, it seems like you have overlooked the CONTRIBUTING.md document that we have carefully written to give first contributors an idea of the general structure of a program.
The when isMainModule is important for a file to be shipped inside a big project without messing the imports.
You confused high(arr) for len(arr).
The tests do not reflect the genericity of the algorithm. Even if the procedure has been written for integers only, there should at least be a zero and negative numbers in them. Some small random tests would be very much appreciated. Your tests are very similar and could be templated too.
| @@ -0,0 +1,52 @@ | |||
| ## Bubble Sort | |||
| ## | |||
| ## https://en.wikipedia.org/wiki/Bubble_sort | |||
There was a problem hiding this comment.
Sources and links should be at the end of the docstring bloc.
There was a problem hiding this comment.
Please, add a description of the algorithm too. Here is the one I put in my implementation.
« Bubble sort, sometimes referred to as sinking sort, is a simple sorting algorithm that repeatedly steps through the input list element by element, comparing the current element with the one after it, swapping their values if needed. These passes through the list are repeated until no swaps had to be performed during a pass, meaning that the list has become fully sorted. »
Something more concise would be greatly appreciated.
| ## Bubble Sort | ||
| ## | ||
| ## https://en.wikipedia.org/wiki/Bubble_sort | ||
| ## Time complexity: O(n^2) |
There was a problem hiding this comment.
Best, worst, average cases ?
It should be put forward that not only the time complexity is far from optimal, but it is even the slowest algorithm (with the constant factor) among the algorithms with quadratic complexity.
| ## Import unit testing for testing purposes | ||
| import unittest | ||
|
|
||
| ## Define function, bubble_sort() |
There was a problem hiding this comment.
What is the point of this comment ? It looks redundant with the function header.
| ## Import unit testing for testing purposes | ||
| import unittest |
There was a problem hiding this comment.
Look at other implementations or the CONTRIBUTING.md file.
Imports are rather put under a when isMainModule: block.
| import unittest | ||
|
|
||
| ## Define function, bubble_sort() | ||
| ## Pass in array "arr" as parameter |
There was a problem hiding this comment.
Same. Comment does not bring much. A short explanation of the openArray type is preferable.
Avoid comments that duplicate code. See, e.g. SO blog post: best practices for writing code comments or even Don't write comments video by CodeAesthetic
| ## Length is the length of arr | ||
| var length = high(arr) |
There was a problem hiding this comment.
Iit is clearly stated in the very first Nim tutorial that:
The built-in len proc returns the array's length. low(a) returns the lowest valid index for the array a and high(a) the highest valid index.
| ## Length is the length of arr | |
| var length = high(arr) | |
| var length = len(arr) |
or:
| ## Length is the length of arr | |
| var length = high(arr) | |
| # high returns the last index of an array | |
| var last_index = high(arr) |
| var length = high(arr) | ||
|
|
||
| if length <= 1: | ||
| ## If length is or is less than one, no execution |
There was a problem hiding this comment.
Your comment should not duplicate code and give algorithm insight. Avoid speaking of execution, if, while.
| ## If length is or is less than one, no execution | |
| ## Array is too small, nothing to sort. | |
| ## In these cases, the array is considered already sorted. |
| ## Iterate through array | ||
| for i in 1..high(arr): | ||
|
|
||
| ## You can treat "key" as a temporary variable |
There was a problem hiding this comment.
| ## You can treat "key" as a temporary variable | |
| ## "key" is a pivot element for which we seek the right position |
| var j = i - 1 | ||
| while j >= 0 and key < arr[j] : | ||
| arr[j + 1] = arr[j] | ||
| j -= 1 |
There was a problem hiding this comment.
Another Nim's syntax sugar that I like. You can resolve this conversation if you do not like this notation.
| j -= 1 | |
| dec j |
| j -= 1 | ||
| arr[j + 1] = key | ||
|
|
||
| ## Test |
|
I guess I should not do any duplicated work, and I will move on from sorting algorithms to searching algorithms as you are almost done with sortings! Thank you for the problems you pointed out, it really helps me, and I will follow the contribution guidelines, write better comments, and take notice of the code I am writing. |
Algorithms Nim seems like a very new project with few contributions. I added two new sorting algorithms, Insertion sort, and Bubble sort. I will try to contribute more algorithms to this project, and I desire to see this project grow!