12
\$\begingroup\$

First I've to indicate that this is my homework.

An example of a given input is:

s1 = {'p1': (x1, y1), 'p2': (x2, y2)}

where all names and coordinations are user input, only the input format is predefined; also p1 is the upper leftmost point and p2 is the lower rightmost point as shown below:

enter image description here

I can go on to get area, surrounding, midpoint, height and width based on this input.

I wrote a code that works as demanded but I don't like it. it doesn't seem I followed the Pythonic approach here; how can I improve this?

class rectangle:
    def __init__(self, dct):
        # ['name', [x, y]]
        names = list(dct.keys())
        coords = list(dct.values())
        start = [names[0], coords[0]]
        end = [names[1], coords[1]]
        self.start = start
        self.end = end

        # int
        start_coords = self.start[1]
        end_coords = self.end[1]
        width = end_coords[0] - start_coords[0]
        height = end_coords[1] - start_coords[1]
        self.width = width
        self.height = height

        # [x, y]
        midpoint = [self.width / 2, self.height / 2]
        self.midpoint = midpoint

        # int
        area = (self.width + self.height) * 2
        surr = self.width * self.height
        self.area = area
        self.surr = surr

s1 = rectangle({'p1': (1,1), 'p2': (2,2)})
print('point one: ', s1.start)
print('point two: ', s1.end)
print('width: ', s1.width)
print('height: ', s1.height)
print('midpoint: ', s1.midpoint)
print('area: ', s1.area)
print('surrounding: ', s1.surr)
\$\endgroup\$
  • \$\begingroup\$ What is those code supposed to do? Calculate the area of the rectangle? I don't see any output in your program. Please add more details about what your code is supposed to do. \$\endgroup\$ – Linny Mar 16 at 22:19
  • \$\begingroup\$ @Linny I did, thanks. yes, it is supposed to calculate and show the values I mentioned in the title. \$\endgroup\$ – Nitwit Mar 16 at 22:31
  • 6
    \$\begingroup\$ Your area property is called 'perimeter' in mathematics, while your surr property is actually called the an 'area'. \$\endgroup\$ – Emanuel Vintilă Mar 17 at 10:36
15
\$\begingroup\$

PEP-8

  • Class names should be CapWords, so instead of rectangle you should have Rectangle.
  • Commas should be followed by 1 space. You've mostly followed this, except in s1 = rectangle({'p1': (1,1), 'p2': (2,2)})

Bugs

  • The formula for "area" is not twice the sum of width & height.
  • I don't know what "surrounding" is, but the formula for perimeter is not width times height.
  • The midpoint (centre?) of a rectangle should be within the bounds of the rectangle. Consider the rectangles with corners (10, 10) and (12, 12). The centre would be (11, 11), not (1, 1) as calculated.

Awkward initialization

This code:

        names = list(dct.keys())
        coords = list(dct.values())
        start = [names[0], coords[0]]
        end = [names[1], coords[1]]
        self.start = start
        self.end = end

relies on the dictionary's ordering of keys. It can break in Python 3.6 and earlier (CPython 3.5 and earlier). It does not enforce the key names p1 and p2; any two keys will work. And self.start[0] and self.end[0] are never used, so storing the key names in these entries is unnecessary.

The code could simply and safely read:

         self.start = dct['p1']
         self.end = dct['p2']

with suitable modifications of the usage of self.start and self.end.

Class with no methods

A class should have methods. Without any methods, you'd be better off with a namedtuple for constant data, or a dict for mutable data.

So let's give your class some methods:

    def width(self):
        return self.end[0] - self.start[0]

    def height(self):
        return self.end[1] - self.start[1]

As mentioned by Peilonrayz, you may wish to use abs(...) here.

You can use these methods externally:

print('width: ', s1.width())
print('height: ', s1.height())

as well as in other members of this class:

    def area(self):
        return (self.width() + self.height()) * 2    # Note: Formula is still incorrect

An Over-Engineered Solution

Do not submit this as your home-work solution! You would likely fail or be expelled! This illustrates some advanced concepts like the @dataclass and the NamedTuple, type hints and the typing module, as well as read-only @property attributes, a @classmethod, and """docstrings""". You may find these interesting to study in your free time.

from typing import NamedTuple
from dataclasses import dataclass

class Point(NamedTuple):
    x: float
    y: float

@dataclass
class Rectangle:
    """A class for calculations on a Rectangle"""

    p1: Point
    p2: Point

    @classmethod
    def from_dict(cls, dct):
        """
        Constructs a Rectangle from a dictionary with "p1" and "p2" keys.
        These keys must contain a tuple or list of two numeric values.
        """
        return Rectangle(Point._make(dct['p1']), Point._make(dct['p2']))

    @property
    def width(self):
        """
        Computes the width of the rectangle.
        """
        return abs(self.p2.x - self.p1.x)

    @property
    def height(self):
        """
        Computes the height of the rectangle.
        """
        return abs(self.p2.y - self.p1.y)

    @property
    def area(self):
        """
        Incorrectly computes the area of the rectangle.
        """
        return (self.width + self.height) * 2     # Note: still the incorrect formula

s1 = Rectangle.from_dict({'p1': (1,1), 'p2': (2,2)})
print('point one: ', s1.p1)
print('point two: ', s1.p2)
print('width: ', s1.width)
print('height: ', s1.height)
print('area: ', s1.area)

Output:

point one:  Point(x=1, y=1)
point two:  Point(x=2, y=2)
width:  1
height:  1
area:  4
| improve this answer | |
\$\endgroup\$
  • 4
    \$\begingroup\$ Just to slightly extend the "Class with no methods" feedback; storing everything in variable is also going to cause you to synchronize this data constantly, for mutable objects. For example, if I were to change one of the parameters of the rectangle, you'd need to recalculate the area/perimeter/midpoint. By using a method, you defer the calculation to when it is actually needed. That means I can change the parameters as much as I want, and only when I want to fetch the information (e.g. midpoint) does the calculation take place with the current values. \$\endgroup\$ – Flater Mar 17 at 13:59
  • 3
    \$\begingroup\$ You're not going to be failed for over-complicating your homework; my strong impression is that if OP is being asked to implement a Rectangle class, they'll probably get at least a C for anything that works. \$\endgroup\$ – ShapeOfMatter Mar 17 at 14:02
  • 2
    \$\begingroup\$ I would also advocate against being scared of typing or namedtuple. While OP probably doesn't need every tool they're using, use of language features for their intended purposes (particularly typing) should be encouraged. \$\endgroup\$ – ShapeOfMatter Mar 17 at 14:04
  • 3
    \$\begingroup\$ @ShapeOfMatter The OP tagged their post with "beginner" and "homework". Suddenly using 7 language features which have not been taught would raise questions by any grader. If the OP cannot explain what they do or how they work, it would be obvious the code was not their own. Thus my caution not to use the over-engineered solution. It, however, is followed by an invitation to study these in their free time, which should be taken as encouragement to look into these advanced features and use them where appropriate. \$\endgroup\$ – AJNeufeld Mar 17 at 14:16
8
\$\begingroup\$
  • Class should begin with capital, so Rectangle
  • You are creating local variables just to set them to object on next line using self, assign calculation to self.xyz variable directly. For example:
        self.area = (self.width + self.height) * 2
        self.surr = self.width * self.height
  • Creating list of keys, then list of values to then map it to different structure seems very obscure. Take a look at items. I think you can change your code to:
        items = dct.items()
        start = items[0]
        end = items[1]

Still, I don't understand, why are you doing this. I'd just access those values directly from original dct, I find it more readable and clean, ex:

width = dct["p2"][0] - dct["p1"][0]
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ because point names are user input, i can't assure it's 'p1' or 'p2' \$\endgroup\$ – Nitwit Mar 17 at 13:08
  • \$\begingroup\$ You need to specify that. I assumed that part is given from the assignment. \$\endgroup\$ – K.H. Mar 17 at 13:12
7
\$\begingroup\$

Following the advice from K.H. we get:

class Rectangle:
    def __init__(self, dct):
        items = dict.items()
        self.start = items[0]
        self.end = items[1]

        self.width = self.end[1][0] - self.start[1][0]
        self.height = self.end[1][1] - self.start[1][1]
        self.midpoint = [self.width / 2, self.height / 2]
        self.area = (self.width + self.height) * 2
        self.surr = self.width * self.height

This still has a couple of problems:

  • By passing dct as a dict you have made two assumptions:

    1. Dictionaries are ordered by default now but on 3.5 and before they are not.
    2. A user will always enter the start as the first value of the dictionary.

    These are bad because you've made some assumptions without being explicit. To solve this you can just pass start and end to Rectangle.

  • Start is assumed to have lower values than end. This means self.width and self.height can be negative values, if this assumption no longer holds. A negative width or height don't make much sense.

    This assumption also goes on to effects self.area and self.surr.

  • Start and end don't make too much sense to return a key, that isn't ever used in Rectangle.
  • Area and surr are wrong.

    • The equation for area is \$ab\$ not \$2(a + b)\$.
    • The equation for surface area, perimeter, is \$2(a + b)\$ not \$ab\$.
class Rectangle:
    def __init__(self, start, end):
        self.start = start
        self.end = end

        self.width = abs(self.end[0] - self.start[0])
        self.height = abs(self.end[1] - self.start[1])
        self.midpoint = [self.width / 2, self.height / 2]
        self.area = self.width * self.height
        self.surr = (self.width + self.height) * 2
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ self.start = items[0] gives me TypeError: 'dict_items' object is not subscriptable \$\endgroup\$ – Nitwit Mar 17 at 13:06
  • 1
    \$\begingroup\$ @Nitwit I didn't make that as a suggestion, please take that up with K.H. \$\endgroup\$ – Peilonrayz Mar 17 at 13:16

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.