Skip to main content
deleted 169 characters in body
Source Link
l0b0
  • 9k
  • 21
  • 36
  • When doing the opposite of an action I would use the antonym of the method name. So in this case I would rename erase_value to delete, remove or another antonym of "add".
  • The _first suffix looks redundant - pop and peek are common names for what you are doing.
  • A common pattern for first and second parameters like in _swap_indexes or your lambdas is simply first and second. These are lexicographically more distinct than index1 and index2, so it's much harder to confuse the two.
  • Your _*_index methods seem like they could be useful to someone using this class. I would remove the initial underscore and possibly make them @property methods.
  • When doing the opposite of an action I would use the antonym of the method name. So in this case I would rename erase_value to delete, remove or another antonym of "add".
  • The _first suffix looks redundant - pop and peek are common names for what you are doing.
  • A common pattern for first and second parameters like in _swap_indexes or your lambdas is simply first and second. These are lexicographically more distinct than index1 and index2, so it's much harder to confuse the two.
  • Your _*_index methods seem like they could be useful to someone using this class. I would remove the initial underscore and possibly make them @property methods.
  • When doing the opposite of an action I would use the antonym of the method name. So in this case I would rename erase_value to delete, remove or another antonym of "add".
  • The _first suffix looks redundant - pop and peek are common names for what you are doing.
  • A common pattern for first and second parameters like in _swap_indexes or your lambdas is simply first and second. These are lexicographically more distinct than index1 and index2, so it's much harder to confuse the two.
added 176 characters in body
Source Link
l0b0
  • 9k
  • 21
  • 36
  • Type annotationsType annotations help the IDE make sense of dynamically typed code.
  • Doctest examplesDoctest examples show people how to use your code with theirs.
  • Unit testsUnit tests ensure that future changes are less likely to break functionality.
  • Type annotations
  • Doctest examples
  • Unit tests
  • Type annotations help the IDE make sense of dynamically typed code.
  • Doctest examples show people how to use your code with theirs.
  • Unit tests ensure that future changes are less likely to break functionality.
added 252 characters in body
Source Link
l0b0
  • 9k
  • 21
  • 36

Excellent:

Excellent

Naming issues:

Naming issues

Keeping it simple:

Keeping it simple

  • By all means write comments to clarify things to yourself while programming, but then see if you can simplify the code so much that you can remove it. Comments should explain the "why", not the "how", "what" or "when."
  • I would pull out both lambdas and give them names. That way the "Default results in min-heap." comment becomes redundant and -1 if a < b else 0 if a == b else 1 can be much more legible as multiple lines.
  • Unless binary heaps are special in some way starting indexing at 1 will be extremely confusing to most programmers. Python as a rule does not do 1-based indexing.

Tests

  • Testing should be done by an external process, not every time the program runs.
  • Test naming is tricky. I know almost every guide out there uses the standard "test [function name]" or "test [high level requirement]" naming scheme, but there's a couple tricks to make them much more useful:
    • Start the test name with "should." (In Python you need to configure your test framework to look for should_ or just use test_should_.) This forces you to think about the goal of the test, to prove that the code does what it should do rather than just what sort of data or function is involved in the test. For example, renaming test_erasing_nonexistent to should_return_false_when_removing_nonexistent_number would clarify what the test is trying to prove.
  • I can tell from the tests that they were written after the code, because I used to write tests the same way. With TDD you would discover that there is no point testing 16 values when one, two or three would do to prove the point. This would also make the tests much easier to read, because as a reviewer I would be able to verify with the absolute minimal cognitive load that they do what they say.
  • Each test should ideally contain only one assertion. (The only exception to this that I've found is when a guard assertion results in more useful error messages, such as self.assertEqual(200, request.status_code, request.data).) While resulting in lots more test methods this gives several advantages:
    • Each test is much easier to read.
    • It's much easier to spot redundant, irrelevant or wrong tests.
    • You won't have to play whack-a-mole with assertions to get to the end of the test.

Excellent:

Naming issues:

Keeping it simple:

  • By all means write comments to clarify things to yourself while programming, but then see if you can simplify the code so much that you can remove it. Comments should explain the "why", not the "how", "what" or "when."
  • I would pull out both lambdas and give them names. That way the "Default results in min-heap." comment becomes redundant and -1 if a < b else 0 if a == b else 1 can be much more legible as multiple lines.
  • Unless binary heaps are special in some way starting indexing at 1 will be extremely confusing to most programmers. Python as a rule does not do 1-based indexing.
  • Testing should be done by an external process, not every time the program runs.

Excellent

Naming issues

Keeping it simple

  • By all means write comments to clarify things to yourself while programming, but then see if you can simplify the code so much that you can remove it. Comments should explain the "why", not the "how", "what" or "when."
  • I would pull out both lambdas and give them names. That way the "Default results in min-heap." comment becomes redundant and -1 if a < b else 0 if a == b else 1 can be much more legible as multiple lines.
  • Unless binary heaps are special in some way starting indexing at 1 will be extremely confusing to most programmers. Python as a rule does not do 1-based indexing.

Tests

  • Testing should be done by an external process, not every time the program runs.
  • Test naming is tricky. I know almost every guide out there uses the standard "test [function name]" or "test [high level requirement]" naming scheme, but there's a couple tricks to make them much more useful:
    • Start the test name with "should." (In Python you need to configure your test framework to look for should_ or just use test_should_.) This forces you to think about the goal of the test, to prove that the code does what it should do rather than just what sort of data or function is involved in the test. For example, renaming test_erasing_nonexistent to should_return_false_when_removing_nonexistent_number would clarify what the test is trying to prove.
  • I can tell from the tests that they were written after the code, because I used to write tests the same way. With TDD you would discover that there is no point testing 16 values when one, two or three would do to prove the point. This would also make the tests much easier to read, because as a reviewer I would be able to verify with the absolute minimal cognitive load that they do what they say.
  • Each test should ideally contain only one assertion. (The only exception to this that I've found is when a guard assertion results in more useful error messages, such as self.assertEqual(200, request.status_code, request.data).) While resulting in lots more test methods this gives several advantages:
    • Each test is much easier to read.
    • It's much easier to spot redundant, irrelevant or wrong tests.
    • You won't have to play whack-a-mole with assertions to get to the end of the test.
added 252 characters in body
Source Link
l0b0
  • 9k
  • 21
  • 36
Loading
Source Link
l0b0
  • 9k
  • 21
  • 36
Loading