Pr-3

[CheezItMan]

This method find the maximum value of an array.

[ranuseh]

Nothing to add. All good.

[cyndilopez]

what if max index is a negative number? then the max should be 0 and should return 0 instead of nil

[carlabosco]

You can do better than this!

[griffifam]

Suggested change

	def find_max(array, max_index)
	def find_max(array)
	if array.nil?
	return "Array has no max element"
	end
	max = nil
	idx = 0
	while max < array.length
	if max < array[idx]
	max = array[idx]
	end
	idx ++
	end
	return "Max element is " + max
	end

[shubha-rajan]

Please see my comments. I left a few suggestions for improving the clarity of output messaging and handling edge cases.

[shubha-rajan]

Array is empty might be a confusing message to print out since a user can call the method on an array that isn't empty, but set the max index to zero or negative.

Suggested change

	puts "Array is empty"
	if max_index == 0
	puts "Array has no elements in the given range"
	return nil
	else if max_index < 0
	puts "Invalid range: Max index can't be less than zero"
	return nil

[shubha-rajan]

the only cases in which max array should return nil are where max_index is zero or negative. There is explicit messaging for max_index==0, but nothing for negative. Maybe return nil in both those cases (with explicit messaging) and initialize max later in the else condition?

Suggested change

max = nil

[shubha-rajan]

What happens here if max_index is greater than array.length?

[tatsqui]

max_index as a variable name sounds misleading, is this variable supposed to represent the length of the array?

[griffifam]

Edge case for if array is nil

[griffifam]

+- does max_index need to be a param? Can you define it inside method if array provided?

[shubha-rajan]

does it make sense here to return array[0] if max_index is negative?

[tatsqui]

Suggested change

	puts "Array is empty"
	# will also need to check first that array.length != nil
	return array[0]

[MyriamWD]

max is initialized as nil at the beginning and then reassigned when the else statement gets executed. It might be ok to either initialize it at the beginning as max = array[0] and delete line 9.

[MyriamWD]

If we check for max_index > 0 here and then execute the code in the else statement, max can be declared and returned from within the same statement.

[amythetester]

Might want to consider some of these changes, we might also want to look for more edge cases.

[amythetester]

Suggested change

	puts "Array is empty"
	return array[0]

[amythetester]

Suggested change

	while index < max_index
	while index <= max_index && index <= array.length - 1

[amythetester]

This seems not needed.

[amythetester]

Suggested change

	max = nil
	if array.length == 0
	return "Array is empty"
	end

[aphunk]

Consider finding the end of the array programmatically using length.

[aphunk]

this is redundant since it's defined again in theelse statement

[amyesh]

max_index is an unclear variable name - would length be more readable?

[amyesh]

it's not necessary to declare max at this point since it's declared on line 9 as well.

[msfinnan]

do we need to define this here or can we wait and define it on line 9?

[msfinnan]

is max_index the max index of array? if so, could we just find that inside the method instead of passing it in as a parameter?

[Kimberly-Fasbender]

Do we want to change this function name to something more descriptive? Maybe 'find_max_value', or 'find_max_array_value'?

[Kimberly-Fasbender]

Should this be <= instead of < ?