3
\$\begingroup\$

Shape represents an abstract class and the other two classes inherits from it. This looks insufficiently abstract to me. How could I improve it?

import math
class Point:

    def __init__(self,x,y):
        self.x = x
        self.y = y

    def move(self,x,y):
        self.x += x
        self.y += y

    def __str__(self):
        return "<"+ str(self.x) + "," + str(self.y) + ">"


class Shape:        
    def __init__(self, centrePoint, colour, width, height):
        self.centrePoint = centrePoint
        self.colour = colour
        self.width = width
        self.height = height
        self.type = "Square"

    def __init__(self, centrePoint, radius, colour):
        self.type = "Circle"
        self.radius = radius
        self.colour = colour
        self.centrePoint = centrePoint

    def move(self,x,y):
        self.centrePoint.move(x,y)

    def getArea(self):
        if (self.type == "Square"):
            return self.width * self.height
        elif (self.type == "Circle"):
            return math.pi*(self.radius**2)

    def __str__(self):
        return "Center Point: " + str(self.centrePoint) + "\nColour: "+ self.Colour + "\nType: " + self.type + "\nArea: " + self.getArea()


class Rectangle (Shape):

    def scale(self, factor):
        self.scaleVertically(factor)
        self.scaleHorizontally(factor)

    def scaleVertically(self, factor):
        self.height *= factor

    def scaleHorizontally(self, factor):
        self.width *= factor

class Circle (Shape):

    def scale(self, factor):
        self.radius * factor
\$\endgroup\$
2
\$\begingroup\$

Your class Shape knows about what kind of shape it is (circle/square). This makes it tied to the two child classes. It should not know any thing about the composition. Rather just provide methods with empty implementations that can be overridden by child classes.

The only thing I can see that is common across all classes is the concept of a center point and a color. So

import abc

class Shape(metaclass=abc.ABCMeta):        
    def __init__(self, centrePoint, colour):
        self.centrePoint = centrePoint
        self.colour = colour

Since move depends only on center point, it is ok to define it in base class.

    def move(self,x,y):
        self.centrePoint.move(x,y)

This is certainly one that should be implemented by the child classes because it requires knowledge about what kind of a shape it is.

    @abc.abstractmethod
    def getArea(self):
        return

or if you dont want to use it

    def getArea(self):
        raise NotImplementedError('Shape.getArea is not overridden')

see abc in so

And the to string method is another that should be done in child because it holds details of what shape it is.

    @abc.abstractmethod
    def __str__(self):
        return

However, if you are going to use the same format for all shapes, then you can provide the default implementation (as below.)

    def __str__(self):
        return "Center Point: " + str(self.centrePoint) + "\nColour: "+ self.Colour + "\nType: " + self.type() + "\nArea: " + self.getArea()

I would also recommend scale as part of the base class because all shapes can be scaled.

    def scale(self, factor):
        return

Basically all operations that make sense on any shapes should have an abstract method on the base class.

Now for child classes

class Rectangle (Shape):
    def __init__(self, cp, color, width, height):
        super(Rectangle, self).__init__(cp, color)
        self.width = width
        self.height = height

    def scale(self, factor):
        self.scaleVertically(factor)
        self.scaleHorizontally(factor)

    def scaleVertically(self, factor):
        self.height *= factor

    def scaleHorizontally(self, factor):
        self.width *= factor

    def getArea(self):
        return self.width * self.height

    def type(self):
        return "Rectangle"

class Circle (Shape):
    def __init__(self, cp, color, radius):
        super(Rectangle, self).__init__(cp, color)
        self.radius = radius

    def scale(self, factor):
        self.radius * factor

    def getArea(self):
        return math.pi*(self.radius**2)

    def type(self):
        return "Circle"
\$\endgroup\$

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.