Skip to content

Conversation

@oyvindeide
Copy link
Collaborator

No description provided.

Copy link
Collaborator Author

@oyvindeide oyvindeide left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution, I think it is a good piece of work, I have some comments.

Comment on lines +32 to +34
def test_slope_calculator_without_parameterize():
assert calculate_slope(*[1, 2, 1, 2]) == 1
assert calculate_slope(*[1, 2, 1, 4]) == 3
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a duplicate of the test above, I would suggest keeping just one of the implementations.

return coordinates


def calculate_slope(x1, x2, y1, y2):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The ordering of the input arguments are a bit hard to read, perhaps we should reorder them?

Secondly, having arguments with 1 and 2 can very often lead to bugs, so perhaps we should try to avoid that also?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One option is to just keep the tuples, that might make it more readable.

return (y2 - y1) / (x2 - x1)


def treasure_hunt(input_coordinates):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would be good to add a docstring, as it is not immediately apparent. Also the name is not very descriptive.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Your style check is failing, one reason is because we are missing type hints in this function.



def calculate_slope(x1, x2, y1, y2):
return (y2 - y1) / (x2 - x1)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is a potential bug here if the x-coordinate does not change. Should also add a test for that.



def treasure_hunt(input_coordinates):
for i, (x2, y2) in enumerate(input_coordinates[1:]):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This line is a bit confusing, perhaps it is worth rewriting?

for i, (x2, y2) in enumerate(input_coordinates[1:]):
x1, y1 = input_coordinates[i]
if calculate_slope(x1, x2, y1, y2) > 1:
return x2, y2
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
return x2, y2
return (x2, y2)

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.

1 participant