PR 3: find_max method #3
Conversation
|
looks so good Chris |
| def find_max(array, max_index) | ||
| max = nil | ||
| if max_index == 0 | ||
| puts "Array is empty" |
There was a problem hiding this comment.
I'm not sure, but maybe "return" be more appropriate than "puts"? Otherwise, the code will continue to run after the print statement and then try to return nil
There was a problem hiding this comment.
I think it's great idea, thank you for pointing it out.
| if max < array[index] | ||
| max = array[index] | ||
| end |
There was a problem hiding this comment.
| if max < array[index] | |
| max = array[index] | |
| end | |
| max = array[index] if max < array[index] |
There was a problem hiding this comment.
is another shorter option to consider!
| index += 1 | ||
| end | ||
| end | ||
| return max |
There was a problem hiding this comment.
nice job! I think this would correctly return the max. Have you considered writing tests for this method?
| def find_max(array, max_index) | ||
| max = nil | ||
| if max_index == 0 | ||
| puts "Array is empty" |
There was a problem hiding this comment.
I think a return statement would be better than a puts, then the code wouldn't continue to run and would just return immediately.
ie: return nil
| def find_max(array, max_index) | ||
| max = nil | ||
| if max_index == 0 | ||
| puts "Array is empty" |
There was a problem hiding this comment.
Is it possible to add a return instead of a puts statement for this part? That way the code would stop running in case the array is empty.
|
|
||
| # Find the maximum element between index 0 and max_index, exclusive | ||
|
|
||
| def find_max(array, max_index) |
There was a problem hiding this comment.
Would it be more readable to rename variable max_index into length?
| def find_max(array, max_index) | |
| def find_max(array, length) |
| if max_index == 0 | ||
| puts "Array is empty" | ||
| else | ||
| max = array[0] |
There was a problem hiding this comment.
Max variable is initially assigned to nil and then to the array[0], is there a purpose to do that or initial value can be set to array[0]?
| @@ -0,0 +1,19 @@ | |||
|
|
|||
| # Find the maximum element between index 0 and max_index, exclusive | |||
There was a problem hiding this comment.
This comment is helpful, but you might consider (even if this is a small change) getting into the practice of adding a comment to your PR discussing what you changed/added, how it improves/impacts your code base -- for reviewer folx to be able to better understand your code's goals and scope quickly
| index += 1 | ||
| end | ||
| end | ||
| return max |
There was a problem hiding this comment.
if max_index == 0, it prints "Array is empty" and returns max (ie nil), is that what we want?
No description provided.