Return to Answer

MustModify did a great review of your code as a whole. I won't retread all that but rather look at the overall cleanliness and style of the code.

MustModify did a great review of your code as a whole. I won't retread all that but rather look at the overall cleanliness and style of the code.

• Your indentation is all over the place, making it really hard to read at times. Ruby convention is to use 2 spaces of indentation. Spaces; not tabs.

• You could do with more horizontal whitespace, again for readability. I.e. x = 2 not x=2, and x == foo not x==foo. The whitespace you do have is also all over the place.

• return is implicit at a method's exit point. You only need explicit returns if you're returning early. And for early returns you have post-fixed contionals, such as return 0 if d.zero? (notice also the use of #zero? instead of == 0, but read on why you shouldn't do that either)

• Don't use zero as a NULL value. Ruby has nil and the Object#nil? check method. So your min and max variables should be initialized to nil - not integer zero.

• Don't use "magic" strings when you have Symbols. I.e. "max" and "min" should rather be :max and :min

• Don't do assignments in conditions (e.g. if (x = foo()) == bar; there's really no reason to, and it makes it harder to follow what's happening. Compact code ≠ faster code.

• Don't abbreviate variables (yes, I'm referring to the Node ivars in particular). Again, hard to read and follow. Just call 'em left, right, and value.

• Don't specify an empty block for Struct.new - it's optional, so leave it out.

def insert(value, type, node = nil)
 return Node.new(value) if node.nil? # struct ivars are auto-initialized to nil
 if (type == :max && value > node.value) || (type == :min && value < node.value)
 tmp = node.value
 node.value = value
 node.left = insert(tmp, type, node.left)
 else
 node.left = insert(value, type, node.left)
 end
 node
end

• Your indentation is all over the place, making it really hard to read at times. Ruby convention is to use 2 spaces of indentation. Spaces; not tabs.

• You could do with more horizontal whitespace, again for readability. I.e. x = 2 not x=2, and x == foo not x==foo. The whitespace you do have is also all over the place.

• return is implicit at a method's exit point. You only need explicit returns if you're returning early. And for early returns you have post-fixed contionals, such as return 0 if d.zero? (notice also the use of #zero? instead of == 0, but read on why you shouldn't do that either)

• Don't use zero as a NULL value. Ruby has nil and the Object#nil? check method.nil is also false'y, so in many cases you can skip the explicit nil? check. So your min and max variables should be initialized to nil - not integer zero.

• Don't use "magic" strings when you have Symbols. I.e. "max" and "min" should rather be :max and :min

• Don't do assignments in conditions (e.g. if (x = foo()) == bar; there's really no reason to, and it makes it harder to follow what's happening. Compact code ≠ faster code.

• Don't abbreviate variables (yes, I'm referring to the Node ivars in particular). Again, hard to read and follow. Just call 'em left, right, and value.

• Don't specify an empty block for Struct.new - it's optional, so leave it out.

def insert(value, type, node = nil)
 return Node.new(value) if node.nil? # struct ivars are auto-initialized to nil
 # could also say:
 # return Node.new(value) unless node
 if (type == :max && value > node.value) || (type == :min && value < node.value)
 tmp = node.value
 node.value = value
 node.left = insert(tmp, type, node.left)
 else
 node.left = insert(value, type, node.left)
 end
 node
end