5
\$\begingroup\$

I am trying to create a very basic class: Line as follows:

class Line:

    def __init__(self, x1, y1, x2=None, y2=None, dx=None, dy=None):
        self._x1 = x1
        self._y1 = y1

        if dx is None:
            if x2 is None:
                raise ValueError("You must specify either x2 or dx. both argument are not passed")
            else:
                self._x2 = x2
                self._dx = dx
        else:
            if x2 is None:
                self._x2 =  self._x1 + dx
                self._dx = dx
            else:
                raise ValueError("Both x2 and dx are passed. You must supply only one of them")

        if dy is None:
            if y2 is None:
                raise ValueError("You must specify either y2 or dy. both argument are not passed")
            else:
                self._y2 = y2
                self._dy = dy
        else:
            if y2 is None:
                self._y2 =  self._y1 + dy
                self._dy = dy
            else:
                raise ValueError("Both y2 and dy are passed. You must supply only one of them")


    def x1(self):
        return self._x1

    def x2(self):
        return self._x2

    def y1(self):
        return self._y1

    def y2(self):
        return self._y2     

    def dx(self):
        return self._dx

    def dy(self):
        return self._dy

x1 and y1 arguments are mandatory. But I want:

  • Only x2 or dx is passed not both.
  • Only y2 or dy is passed not both.

In the code above I feel that I am repeating my self. How can I achieve that with minimum conditons?

I appreciate your help

\$\endgroup\$
6
\$\begingroup\$

I noticed with dx and dy, you're assigning them to self.dx and self.dy regardless of what they are. Those assignments can be moved up with x1 and x2 which thins out the checks a bit.

Unless you really want all four combinations to happen using the same function though, I'd probably break this down into some "pseudo-constructors" that defer to the main constructor. Instead of allowing all possible combinations of data then error checking after, I'd just try to limit up front what options are available. There are four possible input combinations: dx, dy, dx, y2, x2, y2, and x2, dy. You could have four static methods that explicitly request a certain combination, and construct a line accordingly.

In the below code, I also decided to use a NamedTuple here. They're basically a shortcut way of creating an immutable class, complete with a generated constructor, a __repr__ implementation and some other nice features. See the comments doc page and the comments.

Now, I'll admit that this code that I ended up with isn't quite as nice as what I originally saw in my head. I think it is still quite an improvement though in terms of readability:

from typing import NamedTuple, Optional

# NamedTuple automatically creates a constructor that handles all the annoying self._x2 = x2 lines
# It also makes Line immutable. This gets rid of the need for the getters you have
class Line(NamedTuple):
    x1: int  # "Declare" the fields that the tuple will have
    y1: int

    x2: int  
    y2: int

    dx: Optional[int] = None  # With defaults of None for some
    dy: Optional[int] = None

    # staticmethod essentially gets rid of the `self` parameter and makes the method 
    #  intended to be called on the class itself instead of on an instance.
    @staticmethod
    def from_dx_dy(x1, y1, dx, dy):
        return Line(x1, y1, dx=dx, dy=dy, x2=x1 + dx, y2=y1 + dy)

    @staticmethod
    def from_dx_y2(x1, y1, dx, y2):
        return Line(x1, y1, dx=dx, y2=y2, x2=x1 + dx)

    @staticmethod
    def from_x2_dy(x1, y1, x2, dy):
        return Line(x1, y1, x2=x2, dy=dy, y2=y1 + dy)

    @staticmethod
    def from_x2_y2(x1, y1, x2, y2):
        return Line(x1, y1, x2=x2, y2=y2)

Then, as an example:

>>> Line.from_dx_dy(1, 2, 3, 4)
Line(x1=1, y1=2, x2=4, y2=6, dx=3, dy=4)

>>> Line.from_x2_dy(1, 2, 3, 4)
Line(x1=1, y1=2, x2=3, y2=6, dx=None, dy=4)

It still has an unfortunate amount of duplication, but I think it's easier to make sense of.



As mentioned in the comments, since I'm referring to the class itself in the static methods, they should arguably be classmethods instead:

@classmethod
def from_dx_dy(cls, x1, y1, dx, dy):
    return cls(x1, y1, dx=dx, dy=dy, x2=x1 + dx, y2=y1 + dy)

@classmethod
def from_dx_y2(cls, x1, y1, dx, y2):
    return cls(x1, y1, dx=dx, y2=y2, x2=x1 + dx)

@classmethod
def from_x2_dy(cls, x1, y1, x2, dy):
    return cls(x1, y1, x2=x2, dy=dy, y2=y1 + dy)

@classmethod
def from_x2_y2(cls, x1, y1, x2, y2):
    return cls(x1, y1, x2=x2, y2=y2)

Where cls is referring to the current class (Line). I don't like the look of this as much, but it is the intent of class methods.

| improve this answer | |
\$\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.