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.

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

Adding sorting algorithms #29

Closed
wants to merge 9 commits into from
Closed

Adding sorting algorithms #29

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a1f09bf
Create bubble_sort.nim
SatinWukerORIG Jun 18, 2023
3336eac
Delete bubble_sort.nim
SatinWukerORIG Jun 18, 2023
b33e5b0
Create bubble_sort.nim
SatinWukerORIG Jun 18, 2023
5e8ef79
Update bubble_sort.nim
SatinWukerORIG Jun 18, 2023
9df2f6f
Create insertion_sort.nim
SatinWukerORIG Jun 18, 2023
806dbb9
fix bug, unittesting -> unittest
SatinWukerORIG Jun 18, 2023
9b2125a
Update insertion_sort.nim
SatinWukerORIG Jun 18, 2023
b0f4779
Update insertion_sort.nim
SatinWukerORIG Jun 18, 2023
40f906e
Merge branch 'main' into main
SatinWukerORIG Jun 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions sorting/bubble_sort.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
## Bubble Sort
##
## https://en.wikipedia.org/wiki/Bubble_sort
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sources and links should be at the end of the docstring bloc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

## Time complexity: O(n^2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

## Auxiliary Space: O(1).

## Import unit testing for testing purposes
import unittest
Comment on lines +7 to +8
Copy link
Collaborator

Choose a reason for hiding this comment

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

Look at other implementations or the CONTRIBUTING.md file.
Imports are rather put under a when isMainModule: block.


## Define function, bubble_sort()
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the point of this comment ? It looks redundant with the function header.

## Pass in array "arr" as parameter
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

proc bubble_sort(arr: var openarray[int])=
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why int only? It is not less pedagogical if the type is generic.

Suggested change
proc bubble_sort(arr: var openarray[int])=
proc bubble_sort[T](arr: var openarray[T])=

## Optimization: if array is already sorted,
## it doesn't need to do this process
Comment on lines +13 to +14
Copy link
Collaborator

Choose a reason for hiding this comment

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

Which process?
Why the array might already be sorted?
Do not hesitate to split into several functions, one with the basic algorithm, adding gradually all optimisations. I like to call bubbleSort the last algorithm containing all optimisations. See PR #29 .

var swapped = false
## Iterate through arr
for i in 0 .. high(arr) - 1:
Copy link
Collaborator

Choose a reason for hiding this comment

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

In Nim, we have syntactic sugar to avoid all these - 1 in upper bounds.

Suggested change
for i in 0 .. high(arr) - 1:
for i in 0 ..< high(arr):

## high(arr) also work but outer loop will
## repeat 1 time more.
## Last i elements are already in place
Comment on lines +18 to +20
Copy link
Collaborator

Choose a reason for hiding this comment

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

This optimization trick is more general than this and can actually be applied at each iteration.

for j in 0 .. high(arr) - i - 1:
Copy link
Collaborator

Choose a reason for hiding this comment

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

The bubble can also be increased rather than decreased.

Suggested change
for j in 0 .. high(arr) - i - 1:
for j in 0 ..< i:

## Go through array from 0 to length of arr - i - 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pay attention, length of array is high(arr) + 1, so this is in fact: length of arr - i.

## Swap if the element found is greater
## than the next element
Comment on lines +23 to +24
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
## Swap if the element found is greater
## than the next element
## Sort two adjacent elements

if arr[j] > arr[j + 1]:
swap arr[j], arr[j + 1]
swapped = true

if not swapped:
## if no need to make a single swap, just exit.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
## if no need to make a single swap, just exit.
## The list is sorted if no swap has been made at the last pass.

return

## Test
Copy link
Collaborator

Choose a reason for hiding this comment

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

See above. Add a when isMainModule block here.

Suggested change
## Test
when isMainModule:
## Test

test "Empty array":
var arr: seq[int]
bubble_sort(arr)
echo repr(arr)
check arr == []


test "one element array":
var arr = @[1]
bubble_sort(arr)
echo repr(arr)
check arr == [1]
Comment on lines +34 to +45
Copy link
Collaborator

Choose a reason for hiding this comment

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

Group them under a suite block to separate these edge cases from more interesting tests.



test "complete array":
var arr = @[5, 6, 2, 1, 3]
bubble_sort(arr)
echo repr(arr)
check arr == @[1, 2, 3, 5, 6]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this work only for positive numbers?

55 changes: 55 additions & 0 deletions sorting/insertion_sort.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
## Insertion Sort Algorithm Implementation
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
## Insertion Sort Algorithm Implementation
## Insertion Sort

##
## https://en.wikipedia.org/wiki/Insertion_sort
Copy link
Collaborator

Choose a reason for hiding this comment

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

Put at the end.

## Worst Case Time Complexity: O(n^2)
## Best Case Time Complexity: O(n)
## Worst Case Space Complexity: O(n)
Comment on lines +4 to +6
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are these worst/best cases corresponding to? (Sorted in order/Sorted in decreasing order)
Average case? (O(n^2))
The useful information that a dev wants to look up and remember are the conditions that make using the insertion sort interesting.
Does this work better than other algorithms on certain list distributions?
In fact, insertion sort is:

  1. Pedagogical
  2. Easy to remember
  3. is quite efficient on small lists (less comparison/swaps than e.g. merge sort for lists of about <20 items).
    People should prefer algorithms with lower average case time complexity (batcher sort, quick sort with random pivot, merge sort) for longer lists.


## Import unit testing for testing purposes
import unittest
Comment on lines +8 to +9
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above. Put that under the when isMainModule: block.


## Define the function
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
## Define the function

proc insertion_sort(arr: var openarray[int]) =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
proc insertion_sort(arr: var openarray[int]) =
proc insertion_sort[T](arr: var openarray[T]) =


## Length is the length of arr
var length = high(arr)
Comment on lines +14 to +15
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Suggested change
## Length is the length of arr
var length = high(arr)
var length = len(arr)

or:

Suggested change
## Length is the length of arr
var length = high(arr)
# high returns the last index of an array
var last_index = high(arr)


if length <= 1:
## If length is or is less than one, no execution
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your comment should not duplicate code and give algorithm insight. Avoid speaking of execution, if, while.

Suggested change
## 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.

return

## Iterate through array
for i in 1..high(arr):

## You can treat "key" as a temporary variable
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
## You can treat "key" as a temporary variable
## "key" is a pivot element for which we seek the right position

var key = arr[i]

## Move elements of arr[0..i-1], that are
## greater than key, to one position ahead
## of their current position
var j = i - 1
while j >= 0 and key < arr[j] :
arr[j + 1] = arr[j]
j -= 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another Nim's syntax sugar that I like. You can resolve this conversation if you do not like this notation.

Suggested change
j -= 1
dec j

arr[j + 1] = key

## Test
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comments as above.

test "Empty array":
var arr: seq[int]
insertion_sort(arr)
echo repr(arr)
check arr == []


test "one element array":
var arr = @[1]
insertion_sort(arr)
echo repr(arr)
check arr == [1]


test "complete array":
var arr = @[5, 6, 2, 1, 3]
insertion_sort(arr)
echo repr(arr)
check arr == @[1, 2, 3, 5, 6]