I wrote a module that calculates the area of shapes and I wanted to get feedback. Am I on the right track?

import math

# A python program to calculate the area of shapes

# area of a square is length multiplied by width
def area_of_square(length, width):
    _check_if_values_are_strings(length = length, width = width)
    _check_if_values_are_negitive(lenth = length, width = width)

    return length * width

# area of a triangle is base multiplied by height divided by 2
def area_of_triangle(base_value, height):
    _check_if_values_are_strings(base_value = base_value, height = height)
    _check_if_values_are_negitive(base_value = base_value, height = height)

    return (base_value * height) / 2

# area of a circle is radius squared times pi
def area_of_circle(radius):
    _check_if_values_are_strings(radius = radius)
    _check_if_values_are_negitive(radius = radius)

    return math.pow(radius,2) * math.pi

def _check_if_values_are_strings(**values):
    for k,v in values.items():
        if (isinstance(v,str)):
            raise TypeError("{0} is a string".format(k))

def _check_if_values_are_negitive(**values):
    for k,v in values.items():
        if v < 0:
            raise ValueError("{0} is a negitive".format(k))

My main concern is the use of the _check_if_values_are_strings and _check_if_values_are_negitive methods. Am I using them correctly, or is there a better approach to verifying the parameters before preforming the calculation?

  • 1
    \$\begingroup\$ Doesn't affect the code, but "negitive" should be spelled "negative" \$\endgroup\$
    – Carson
    Mar 6 '18 at 1:45
  • \$\begingroup\$ (Welcome to CR!) (Should affect the code: lenth = length.) Not using any keyword in "the checks", why are you not using a single asterisk (args instead of kwargs)? \$\endgroup\$
    – greybeard
    Mar 6 '18 at 7:12

Instead of having multiple _check_if_values_are_somethings, just have a single check for the type of values you want. You want the values to be positive real number, just assert them:

def ensure_positive_reals(*values):
    assert all(val >= 0 for val in values)

which will raise ValueError if any of the val was a string and an AssertionError if any of them is negative (Thanks Mathias). To use it, you have:

ensure_positive_reals(length, width)
ensure_positive_reals(base_value, height)

Instead of math.pow, use the exponentiation expression **:

math.pi * (radius ** 2)

which is faster.

  • 1
    \$\begingroup\$ Except float(val) will not error out on a string like "42" but then area_of_square("42", 2) will return '4242' instead of the expected 84… Why not just use assert all(val >= 0 for val in values) instead and let Python tell you about TypeErrors if any val is not a number? \$\endgroup\$ Mar 6 '18 at 9:42
  • \$\begingroup\$ @MathiasEttinger ah yes. I didn't notice that. Thanks :) \$\endgroup\$
    – hjpotter92
    Mar 6 '18 at 9:49

A few things:

  1. Squares don't have width and height, those are rectangles
  2. You check to see that your values are not strings, what if they are lists (or other types of objects)?
  3. Your check functions error if their title is true, this is unintuitive. Something like ensure_value_is_positive would be a better name.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.