Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvement: Defensive Programming - Assertions #473

Open
miili opened this issue Feb 20, 2018 · 4 comments
Open

Improvement: Defensive Programming - Assertions #473

miili opened this issue Feb 20, 2018 · 4 comments

Comments

@miili
Copy link

@miili miili commented Feb 20, 2018

function normalize_rectangle is not easy to comprehend for a beginner. Why should a rectangle be normalized?

http://swcarpentry.github.io/python-novice-inflammation/08-defensive/

I propose a more simple function scale_rectangle:

def scale_rectangle(rect, factor):
    ''' Scale a rectangle from its lower left corner '''
    assert len(rect) == 4, "Rectangle must have four coordinates x1, y1, x2, y2"
    x1, y1, x2, y2 = rect
    
    assert x1 <= x2, "Invalid X coordinates"
    assert y1 <= y2, "Invalid Y coordinates"

    return (x1, y1, x2*factor, y2*factor)
@annefou
Copy link
Contributor

@annefou annefou commented Feb 22, 2018

I agree with you. I am not sure it is necessary to have such complex function. However, the name of the function scale_rectangle looses its meaning with your simplified version. I guess the idea was also to have a meaningful function (in terms of content). I labeled this issue as both enhancement and discussion to get more inputs from others.

Loading

@maxim-belkin
Copy link
Contributor

@maxim-belkin maxim-belkin commented Mar 28, 2018

Interesting suggestion! fixing the function is straightforward (given its description):

return (x1, y1, x1 + (x2-x1)*factor, y1 + (y2-y1)*factor)

Of course, that's assuming factor is positive...
However, do we need to check that x1<x2 and y1<y2? What if we use min(x1, x2) or min( (x1, y1), (x2, y2) )? Would that increase cognitive load?

Loading

@skuschel
Copy link

@skuschel skuschel commented Feb 14, 2019

related to #622

Loading

@skuschel
Copy link

@skuschel skuschel commented Feb 16, 2019

I believe that neither the summing numbers nor the rectangle example show good practice for a few reasons:

  • Why would we teach someone, that only positive values can be summed? It is very pythonic, that a sum function can sum up any kind of object for which the + operator is defined.
  • A rectangle is defined by any two points in the coordinate system. Swapping the order will not change the rectangle. If the function cannot handle this, the first place to mention this would be in the docstring and as a result of the definition the coordinates should be checked. If they are unordered, a ValueError should be raised.

A good example would be:

  • Check the length of the list, which describes the rectangle. If the length is longer than 4, it should raise an IndexError (a shorter list would cause the same error in the line x1, y1, x2, y2 = rect).

I actually find it quite hard to come up with a good example myself, because most errors are raised in the function anyways without manual checking before. From my experience the typical case is checking the number of arguments.

Loading

fmichonneau added a commit to fmichonneau/python-novice-inflammation that referenced this issue Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants