Skip to content

PR #6#6

Open
CheezItMan wants to merge 1 commit intomasterfrom
pr-6
Open

PR #6#6
CheezItMan wants to merge 1 commit intomasterfrom
pr-6

Conversation

@CheezItMan
Copy link
Copy Markdown

This method sorts an array

Comment thread sort.rb
while other_index < array.length
if array[index] > array[other_index]
temp = array[index]
array[index] = array[other_index]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Consider renaming. Index / Other_index can get a little confusing.

Comment thread sort.rb
@@ -0,0 +1,25 @@

def sort(array)
if array == nil || array.length <= 1
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nice base case!

Comment thread sort.rb
Comment on lines +13 to +17
if array[index] > array[other_index]
temp = array[index]
array[index] = array[other_index]
array[other_index] = temp
swapped = true
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

great use of temp variable here

Comment thread sort.rb
array[index] = array[other_index]
array[other_index] = temp
swapped = true
end
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is a bit confusing here, could we add some clarifying comments?

Comment thread sort.rb
other_index = index + 1
while other_index < array.length
if array[index] > array[other_index]
temp = array[index]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Consider using a more descriptive variable name than "temp"

Comment thread sort.rb

def sort(array)
if array == nil || array.length <= 1
return array # nothing to sort
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

should it return nil if there is no array?

Comment thread sort.rb
Comment on lines +7 to +22
index = 0
swapped = true
while index < array.length && swapped
swapped = false
other_index = index + 1
while other_index < array.length
if array[index] > array[other_index]
temp = array[index]
array[index] = array[other_index]
array[other_index] = temp
swapped = true
end
other_index += 1
end
index += 1
end
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bubble sort? A few comments might be helpful for those of us who don't understand what this is doing at first glance.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

i agree

Comment thread sort.rb
swapped = true
while index < array.length && swapped
swapped = false
other_index = index + 1
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

maybe since it's the next index the variable should be called next_index instead of other_index

Comment thread sort.rb
array[index] = array[other_index]
array[other_index] = temp
swapped = true
end
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

maybe parallel assignment would make this more readable instead of using temp

Comment thread sort.rb
@@ -0,0 +1,25 @@

def sort(array)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

a comment describing the sorting algorithm could be helpful for comprehension

Comment thread sort.rb
other_index = index + 1
while other_index < array.length
if array[index] > array[other_index]
temp = array[index]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can you rename temp to a more descriptive variable?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants