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:
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)
3 Answers 3
PEP-8
- Class names should be
CapWords
, so instead ofrectangle
you should haveRectangle
. - 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
-
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– Flater2020年03月17日 13:59:46 +00:00Commented Mar 17, 2020 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– ShapeOfMatter2020年03月17日 14:02:35 +00:00Commented Mar 17, 2020 at 14:02 -
2\$\begingroup\$ I would also advocate against being scared of
typing
ornamedtuple
. 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– ShapeOfMatter2020年03月17日 14:04:05 +00:00Commented Mar 17, 2020 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– AJNeufeld2020年03月17日 14:16:16 +00:00Commented Mar 17, 2020 at 14:16
- 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 toself.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]
-
\$\begingroup\$ because point names are user input, i can't assure it's
'p1'
or'p2'
\$\endgroup\$nitwit– nitwit2020年03月17日 13:08:01 +00:00Commented Mar 17, 2020 at 13:08 -
\$\begingroup\$ You need to specify that. I assumed that part is given from the assignment. \$\endgroup\$K.H.– K.H.2020年03月17日 13:12:50 +00:00Commented Mar 17, 2020 at 13:12
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 adict
you have made two assumptions:- Dictionaries are ordered by default now but on 3.5 and before they are not.
- 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
andend
toRectangle
.Start is assumed to have lower values than end. This means
self.width
andself.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
andself.surr
.- Start and end don't make too much sense to return a key, that isn't ever used in
Rectangle
. -
- 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
-
\$\begingroup\$
self.start = items[0]
gives meTypeError: 'dict_items' object is not subscriptable
\$\endgroup\$nitwit– nitwit2020年03月17日 13:06:26 +00:00Commented Mar 17, 2020 at 13:06 -
1\$\begingroup\$ @Nitwit I didn't make that as a suggestion, please take that up with K.H. \$\endgroup\$2020年03月17日 13:16:08 +00:00Commented Mar 17, 2020 at 13:16
Explore related questions
See similar questions with these tags.
area
property is called 'perimeter' in mathematics, while yoursurr
property is actually called the an 'area'. \$\endgroup\$