I have this code with some classes and the inheritance tree. What I can do better in this code? If we look in the direction of designing classes.

CAR_TYPES = {
    'Car': 'Car',
    'Truck': 'Truck',
    'SpecMachine': 'SpecMachine'
}


class CarBase:
    def __init__(self, brand, photo_file_name, carrying):
        self.car_type = None
        self.photo_file_name = photo_file_name
        self.brand = brand
        self.carrying = carrying

    def get_photo_file_ext(self):
        return self.photo_file_name.split(".")[-1]

    def __str__(self):
        return f"Car type: {self.car_type} | Brand: {self.brand} | Carrying: {self.carrying}"


class Truck(CarBase):
    def __init__(self, photo_file_name, brand, carrying, body_lwh):
        super().__init__(photo_file_name, brand, carrying)
        self.car_type = CAR_TYPES['Truck']
        self.body_lwh = body_lwh
        self.body_length = 0
        self.body_width = 0
        self.body_height = 0
        self.body_volume = 0

        if body_lwh:
            self._set_lwh()
            self._set_body_volume()

    def __str__(self):
        return f"{super().__str__()} | Length: {self.body_length} | Width: {self.body_width}, " \
           f"| Height {self.body_height}, | Volume: {self.body_volume}"

    def _set_lwh(self):
        try:
            self.body_length, self.body_width, self.body_height = map(float, self.body_lwh.split('x'))
        except ValueError:
            self.body_length, self.body_width, self.body_height = 0.0, 0.0, 0.0
            print("Value Error. Check your values and try again!")

    def _get_body_volume(self):
        return self.body_length * self.body_width * self.body_height

    def _set_body_volume(self):
        self.body_volume = self._get_body_volume()


class Car(CarBase):
    def __init__(self, photo_file_name, brand, carrying, passenger_seats_count):
        super().__init__(photo_file_name, brand, carrying)
        self.car_type = CAR_TYPES['Car']
        self.passenger_seats_count = passenger_seats_count

    def __str__(self):
        return f"{super().__str__()} | Passenger seats count: {self.passenger_seats_count}"


class SpecMachine(CarBase):
    def __init__(self, photo_file_name, brand, carrying, extra):
        super().__init__(photo_file_name, brand, carrying)
        self.car_type = CAR_TYPES['SpecMachine']
        self.extra = extra

    def __str__(self):
        return f"{super().__str__()} | Extra: {self.extra}"

I want to make this code more readable and more scalable, but I don't have any experience in this field and I want to learn it.

For example, what I can do with car_type variable? I tried to put car_type to CarBase class, but I don't know, how I can assign it later and to make it right from the design side

CodePudding user response：

Lots of things depend on your design goals, so we would need to know what your car classes need to be doing for their clients to comment more on their design, but I can comment on your use of types.

First, the class name CarBase suggests that it is not meant to be instantiated, that it is just a base for your derived classes. If this is the intent of your design, i would suggest looking into python's abc.abstractclass so that you can make this an abstract class.

Second, to answer your question about the car_type variable: I will claim that you don't need at all, and even that you shouldn't have it. Usually, the purpose of inheritance is so that your clients don't need to care what your type is, they can just know that you are some kind of car, and that you will take care of how that matters for your specific type. This is called polymorphism.

CodePudding user response：

Abstracting to make things more readable actually has an opposite effect most of the time.

First off, you're doing really well here. You're functions/methods are all small - only a few lines. Which means its much easier to absorb what that method does then go back to the rest of the code. Plus, your names are pretty descriptive - they don't use variable types in the name (a good thing!!) and they don't use acronyms. The only abbreviation you used was lwh which is ... pretty easy to grasp.

It looks like you're using a dictionary of terms for CAR_TYPE and if this is a completely self contained system that is plenty fine. If this is code that others will be working on as well, you might use Litterals for this instead:

class CarType():
    MAZARATI = "Mazarati"
    FERRARI = "Ferrari"
    SPORTS_CAR = "Performance"

ect ect. If you define Litterals like this in a class, outside the __init__ constructor, you can access them directly without having to instantiate a class. These become module constants, and you could reference them like:

import CarType

if car_type == CarType.SPORTS_CAR:
    print("Zooooomy!")

which can look more professional, plus also Intellisense will pick these up and provide them in any decent IDE, where as a dictionary will not show its available Keys.

Overall - I don't think there is much you have to worry about here for refactoring. The main idea with refactoring is to make your code DRY - Don't Repeat Yourself -- and you pretty much are not repeating yourself. The other side of refactoring is to make the code more Readable - As a general rule any given dev of the same or more experience as you (and to some extent, less!) should be able to simply read your code like it was a book (hence the praise above that your methods are small, and your method names and variable names are actually readable and dont make me have to puzzle it out)

Don't worry too much about refactoring if most of your stuff is 1 or 2 lines. As my first line here said, there is a point where too much refactoring too much can actually make it worse. If I have to jump to 10 different methods in order to figure out whats happening it is more of a detriment than anything else (This is also why some opinions are that your method that are called inside another should always be defined after and below, so you are reading down the page like you would read a book. This is just an opinion however - some people disagree and think it should be the opposite - kinda sometimes depends on the language)

Overall - if this came across my docket as a Pull Request to review with one of my jr devs I'd be very pleased!

Some stuff you might try for future implementations:

1. Any of your attribtues that are default set to None, you could also default with a kwarg.

   def init(self, brand, photo_file_name, carrying, car_type=None): self.car_type = car_type

this way when someone instantiates the car they can go ahead and set the car_type during the instantiation instead of having to use a second line.

2. If you get more than 4 or 5 properties to pass in, you might consider using dictionaries (if this is internal) or converting some or all them all to kwargs so that they have to be tagged in the instantiation (car_base = CarBase(brand=A, photo_file_name=B.jpg, ...) which when you have more than a couple properties can help clear things up.

Great work! keep it up!