How to modify object properties?

Question

I've recently read a great article about unit testing. There was an example of a bad method which is not well designed. It looks like this

public static string GetTimeOfDay()
{
    DateTime time = DateTime.Now;
    if (time.Hour >= 0 && time.Hour < 6)
    {
        return "Night";
    }
    if (time.Hour >= 6 && time.Hour < 12)
    {
        return "Morning";
    }
    if (time.Hour >= 12 && time.Hour < 18)
    {
        return "Afternoon";
    }
    return "Evening";
}

There are some things which author pointed as those are anti-patterns:

1. It is tightly coupled to the concrete data source. (it reads current datetime from machine it runs on)
2. It violates the Single Responsibility Principle (SRP).
3. It lies about the information required to get its job done. Developers must read every line of the actual source code to understand what hidden inputs are used and where they come from. The method signature alone is not enough to understand the method’s behaviour.

I code mainly in Python and after this article I feel like using self in most cases violates those points too.

class Car:
    def __init__(self, power):
        self.power = power
        self.speed = 0
        
    def accelerate(self, acceleration_time):
        self.speed = self.calculate_acceleration(acceleration_time, self.power)

1. accelerate has hidden input: self.power
2. The method signature alone is not enough to understand the method’s behaviour. There's hidden output (?) self.speed

It's small method and it's easy to read but what about methods with hundred of lines which reads and assigns to self in many places? If those are not named properly developer will have big troubles to understand what it does and even if those are named properly developer should read whole implementation to know if it does modify some self stuff, or if additional context is injected with self.

On the other hand when I'll try to code every method without using self, with input (arguments) and output (return value) then I'll end up passing one variable through many methods and I'll repeat myself.

So how to deal with self and how to use it properly? How to make it clear what method uses as input and what it modifies (output)?

Answer 1

Eeh, it's best not to get overly extreme. Yes it's true that small pure functions with no explicit data flows are much easier to test than mutating operations that result in some action at a distance. But within reason, mutability, impurity, and dependencies aren't a problem. They make some stuff far more convenient.

As a rule of thumb: the closer some code is to the business logic of some software, the more pure, immutable, functional-ish, explicit, and testable it should become. The closer some code is to the outer layers of the application, the less functionality there is that's worth unit-testing carefully, and so less testable designs are OK. For example, code that just wraps some external API cannot be reasonably unit-tested.

As an example for the problems of impurity, many programming introductions have you create domain objects that directly produce output:

class Cat(Animal):
  def make_noise(self):
    print("meow")

That's not a good design, because the output is tightly coupled with the sys.stdout stream. More testable designs would include returning a string instead of printing it directly like
def noise(self): return "meow"
or passing in a file that can be printed to:
def make_noise(self, stream): print("meow", file=stream).

In your example, you have a mutating operation car.accelerate(t). This isn't a problem! This operation doesn't threaten testability because the result can be easily asserted:

car = Car(10)
assert car.speed == 0
car.accelerate(5)
assert car.speed == 50

The name accelerate() also makes it sufficiently clear that this is a mutating operation. Other languages also encode this in the type system (e.g. fn accelerate(&mut self) in Rust) or in the naming convention (e.g. accelerate! in Ruby). Keeping a distinction between mutating commands and pure queries tends to be useful, even though it doesn't always work in practice.

If there's a problem in your code, it's not that the accelerate() method assigns to self, but the self.calculate_acceleration(time, self.power) method. This method receives data from self twice: once as the object the method it's invoked on, another time via the second parameter. This makes the data flows intransparent – there is no reason for this to be a method unless self would be mutated within the method. Changing the design like this can be helpful:

def calculate_acceleration(time, power):
  ...

class Car:
  def __init__(self, power):
    ...
        
  def accelerate(self, acceleration_time):
    self.speed = calculate_acceleration(acceleration_time, self.power)

In this particular case there's no real impact on testability, but in other cases it might now be possible to test the calculation directly, without having to go through the object's interface. Whereas in other languages private static helper methods are normal, that's not an appropriate approach for Python – just use a free function.

One possible criticism of methods is that it's not clear which fields are consumed. E.g. this kind of data flow would be bonkers even though it's arguably “Clean Code” compliant:

class ReallyWeirdObject:
  def __init__(self, x, y):
    self.x = x
    self.y = y
    self.z = None
    self.use_x = False

  def _helper(self):
    self.z = self.x + self.y

  def some_operation(self):
    if self.use_x:
      return self.x
    else:
      self._helper()
      return 2 * self.z

weirdo = ReallyWeirdObject(1, 2)
weirdo.use_x = True
print(weirdo.some_operation())

But the WTF in this code is that z is used to communicate internal results, or that use_x is a field when it should likely be an optional keyword argument to some_operation().

What isn't a problem is that some_operation() consumes fields of the object it was called on. That's like … the entire point. As long as the data in this object is reasonably small and manageable, such operations are fine. If you want to get fancy, you could call this an instance of the “interface segregation principle”. Issues arise mostly for really unwieldy god objects that have dozens of fields.

The question shouldn't be whether the external caller of the method knows which fields of the object will be used. The caller shouldn't have to know this, the object should be one encapsulated thing. A more important question is whether these dependencies and relationships are clear from within the object. Having many fields implies many opportunities for things to get out of sync.

Answer 2

First, it's worth noting that the example in the article somewhat contrived (for practical reasons), and that context matters when it comes to these things. E.g. if you're writing a small, one-off tool, there's little reason to bother too much with design. But let's say this is a part of some longer term project, and that you can reasonably expect that this code would benefit from some design changes (or that you've already had to implement changes that clash with the current design), and let's examine it in that context.

Here's the code for reference:

public static string GetTimeOfDay()
{
    DateTime time = DateTime.Now;
    if (time.Hour >= 0 && time.Hour < 6)
    {
        return "Night";
    }
    if (time.Hour >= 6 && time.Hour < 12)
    {
        return "Morning";
    }
    if (time.Hour >= 12 && time.Hour < 18)
    {
        return "Afternoon";
    }
    return "Evening";
}

In C#, the static keyword essentially means that this is a free function (i.e., not an instance method on an object). This is relevant within the context of your question, since you ask how these considerations apply to objects.

The author of the article raises several points; let me first address 1. (tightly coupled to the date-providing service - the DateTime class) and 3. (misleads about dependencies). The problem this creates is that, while the function works well under the circumstances it was originally created for, is not usable in other contexts.

E.g., what if I need to support a UI that allows users to see the "time of day" category for some future date (again, this "Morning/Afternoon/Evening/Night" example is contrived, but suppose it returns some business-relevant category instead, something of interest to the users).

Another such context is, of course, testing, where you want to be able to plug in predefined values (currently not possible) and check for results (from the perspective of a test, the function is non-deterministic - you can't tell what to expect).

This is easily fixed by making the date-time be a parameter:

public static string GetTimeOfDay(DateTime dateTime)
{
    // same code, except that it uses the dateTime param...
}

Now, regarding SRP violation (point 2.) - the problem is, it's not very meaningful to talk about it in abstract terms. What I mean by that is that it's not very meaningful to just look at the code in isolation and consider a bunch of "what if" scenarios. Sure, there are some general things you can say about SRP in this way, but if you don't consider how your code is actually changing, and the actual design needs, you'll end up with a loot of wasted effort and with overly complicated (read "over-engineered") code.

This means that while you can and should apply SRP initially based on a couple of educated guesses and reasonable assumptions, you'll have to reconsider your design over several iterations/sprints as your understanding of the actual responsibilities and change patterns increases, as you work on this code.

Now, the author says that the function "consumes the information and also processes it". That's too vague to be useful, you could say that about any function. And even if a function delegates the processing to lower level code, at the end of the chain, there has to be something that "consumes the information and also processes it".

The thing is, if this part of the codebase changes very rarely (or never), then you don't really need to consider SRP. You could come up with any number of different reasons to change, but if those changes never happen, you've paid the design costs without getting any benefits. E.g., perhaps the strings returned should be available in different languages (maybe the function should return a key to some dictionary to support localization). Or maybe the threshold values for different times of day can vary - maybe they should be read from a database. Or maybe these values change throughout the year. Or maybe this entire logic isn't universal, so maybe some sort of strategy should be injected into the function (the Strategy pattern). What about a design that needs to support all of the above?

See what I mean by a bunch of "what if" scenarios? What you should do instead is develop an understanding of the problem domain and the codebase, and apply SRP so that the most prominent change axes (kinds of changes, responsibilities) are well supported.

The concept of a seam

So, when you design functions or classes (or libraries and frameworks, for that matter), you often provide some extensibility points - places where client code can plug something in, or otherwise parametrize the provided behavior. Michael Feathers (in Working Effectively with Legacy Code) calls these "seams" - a seam is a place where you can join two software components together. Making datetime be a parameter is a very simple seam. Dependency injection is also a way to create seams. E.g., you could also inject a function or an object that can return a datetime instance (this may or may not be an overkill in the context of this particular example).

What about objects?

So far, we've been considering things at the level of a free function; objects provide another organizational level. So you now have to consider the object as a whole, as objects have their own mechanisms for introducing seams.

The typical way to do so is through constructor injection (as this results in a ready-to-use object)¹. A (Python) class that's equivalent to the example code above would be:

class DateTimeServices:
  def __init__(self):
    self.datetime = datetime;    # from datetime import datetime

  def get_time_of_day(self):
    now = self.datetime.now()
    if 0 <= now.hour < 6:
      return "Night"
    if 6 <= now.hour < 12:
      return "Morning"
    if 12 <= now.hour < 18:
      return "Afternoon"
    return "Evening"

This has the same issues, but the problem now isn't the method itself, it's the fact that the class constructor creates the datetime dependency internally, and it doesn't offer an explicit way to plug in something else. There's no built in seam for this purpose. It's not easy to reuse the class a different scenario.

Here's the same class, but now the constructor takes a "datetime provider":

class DateTimeServices:
  def __init__(self, datetimeProvider):
    self.datetimeProvider = datetimeProvider;

  def get_time_of_day(self):
    now = self.datetimeProvider.now()
    if 0 <= now.hour < 6:
      return "Night"
    if 6 <= now.hour < 12:
      return "Morning"
    if 12 <= now.hour < 18:
      return "Afternoon"
    return "Evening"

# elsewhere:
dts = DateTimeServices(datetime)
dts.get_time_of_day()

Now you can plug in different things, as long as the thing that plays the role of datetimeProvider satisfies the required interface (which, in this case, consists only of the now() method that returns a datetime instance). E.g.:

class FakeDateTimeProvider:
  def __init__(self, year, month, day, hour, minute = 0, second = 0):
    self.datetime = datetime(year, month, day, hour, minute, second)

  def now(self):
    return self.datetime

# then:
dts = DateTimeServices(FakeDateTimeProvider(2020, 8, 18, 8))
dts.get_time_of_day()

# always returns "Morning"

This addresses concerns 1. & 3. from before (with the same considerations regarding concern 2. (SRP)). So, you see, the use of self isn't the problem in itself, it has more to do with the design of the class. As other answers have mentioned, when you use a class (or more precisely, an object), you know what that object represents conceptually, and it isn't surprising to you, the programmer, that the class has and uses its internal state.

class Car:
    def __init__(self, power):
        self.power = power
        self.speed = 0
        
    def accelerate(self, acceleration_time):
        self.speed = self.calculate_acceleration(acceleration_time, self.power)

From my understanding of the class Car, from the naming of the method, and perhaps from documentation, it isn't surprising to me that accelerate changes the state of the instance. This is not something unexpected for objects.

What's problematic is if the class has hidden dependencies that are somehow relevant to your work, making things harder for you.

That said, what can be confusing (in light of the above) is that often instance methods need to take their own parameters. Think of these as accepting additional contextual information that's not directly related to the core responsibility of the class. E.g., it's not something that you can pass once to the constructor, but something that may change on every call. One classic toy example is shapes (circles, triangles, rectangles) that can draw themselves (or, instead of shapes, these could be UI elements (buttons, labels, etc), or game entities (say, 2D sprites)). One way to do it is to have a parameterless draw() method, that does all the drawing internally. But what if you want to draw the same thing in a completely different part of a UI, on a separate drawing surface? Or on a different buffer so that you can do special effects like portals or mirrors? The more flexible alternative is to pass in the drawing surface (or some kind of a graphics object) as a parameter to the draw method.

but what about methods with hundred of lines which reads and assigns to self in many places?

Take that code and burn it with fire.

If those are not named properly developer will have big troubles to understand what it does and even if those are named properly developer should read whole implementation to know if it does modify some self stuff, or if additional context is injected with self.

Yeah. Exactly. Don't write methods with hundreds of lines of code.

Now, on a more serious note, sometimes, you'll end up with large methods. But most of the time, strive to decompose your code into smaller methods and small classes.

If you do have a large method like the one you're describing, one that you can't make the heads or tails of, that methods suffers from all kinds of design problems that you're not going to solve by changing its signature. It's not about self, or about what parameter it takes - this method has bigger issues. You have to refactor it, find things that are generalizable, and break it down into smaller, more understandable and more dependable chunks (methods that you don't have to look into in order to understand the method that calls them). You may even end up putting those chunks in completely different classes.

On the other hand when I'll try to code every method without using self, with input (arguments) and output (return value) then I'll end up passing one variable through many methods and I'll repeat myself.

Well, don't go to either extreme. Write relatively small classes, try to find useful abstractions, and be deliberate about what you pass in as a parameter/dependency of the object itself, and what you want to provide as contextual information to individual methods. Consider if instances of your class should appear in scenarios other than the one you originally intended, and see if your design can accommodate them.

How to make it clear what method uses as input and what it modifies (output)?

Again, when it comes to objects, what you want is to do is make clear what the object itself represents. For object level dependencies, use (preferably) constructor injection and make it clear what the class represents conceptually, what it does, and how it's meant to be used. For instance methods, use good naming, describe what they do and make use of contextual parameters when required. As for class methods and static methods, threat them more as free functions that are somehow closely related to the concept represented by the containing class (these are often things like helper methods and factories).

---

¹ Sometimes constructor injection is not feasible (e.g. a framework may require a parameterless constructor), so dependencies are injected via methods or properties instead, but this is less ideal.

Answer 3

These sorts of questions can usually be answered by looking at the code using the method.

acceleration_time = 5000 # in milliseconds
car.accelerate(acceleration_time)

print(car.speed) # <-- what do you as a programmer expect the speed to be?

While we want to write testable code, we do use code outside of unit tests. Unit tests verify the public facing behavior. The internal behavior of a class is not something a unit test needs to explicitly verify.

When I see the word "accelerate" I expect something to be faster after acceleration has completed. This implies a change to the runtime value of self.speed.

Contrast that with a class modeling physics, like VehicleAccelerationPhysics. I would expect a calculate_acceleration method to return a value, not modify a value. But an accelerate method on a Car would not surprise me if car.speed gets changed — I would expect it to be changed.

Therefore your code is not violating any best practices as far as unit testing is concerned.

accelerate has hidden input: self.power

The current value of self.power is an implementation detail, not "hidden input." If instead you desire to accelerate to a specific speed, your Car class needs an accelerate_to_speed method that calculates the proper acceleration time based on the car's current power.

The method signature alone is not enough to understand the method’s behaviour.

Seems find to me. A car can accelerate. After acceleration the speed is greater than it was before. That's all I need to know.

Answer 4

The basic approach is to put as much of the logic as possible in functions that live outside the class (or are static), then concisely call them in the methods that depend on a state. (These calls still technically need to hide the passed-in property from their signature, but that's kind of the point of OOP, to have a persistent state separate from whatever else methods needs; they're not just in-a-vacuum functions.) The other main point I want to make is that there are other issues we should be addressing first.

With your first example, it helps to first edit it to address another concern, that it's hard to unit-test. Ideally, we want something like

public static string GetTimeOfDay() => get_time_of_day(DateTime.Now.Hour);

// Helper function that's easy to unit test, & can live outside a class
public static get_time_of_day(hour)
{
    if (hour >= 0 && hour < 6)
        return "Night";
    if (hour >= 6 && hour < 12)
        return "Morning";
    if (hour >= 12 && hour < 18)
        return "Afternoon";
    return "Evening";
}

This approach still falls afoul of the tight-coupling criticism. But we can fix this by giving GetTimeOfDay an argument, which I've made optional in the example below:

 public static string GetTimeOfDay(DateTime now=DateTime.Now) => get_time_of_day(now.Hour);

In your second example, I'll change your power terminology. The accelerate method is strange in that it passes a property of the class instance to a method which, because it lives non-statically in the class, can call that property anyway, as if it's a hybrid between hiding two such calls & hiding neither of them. It can be changed as thus:

class Car:
    def __init__(self, acceleration):
        self.acceleration = acceleration
        self.speed = 0
        
    def accelerate(self, acceleration_time):
        self.speed += acceleration_time*self.acceleration

This is easy to test, e.g.

car = Car(3)
car.accelerate(4)
assert car.speed == 12

(feel free to reformat that however you like). But it still depends on self.acceleration, so you might prefer e.g.

    def accelerate(self, acceleration_time):
        self.speed += delta_speed(self.acceleration, acceleration_time)

def delta_speed(acceleration, acceleration_time): return acceleration*acceleration_time

Note delta_speed is at the same indentation level as Car because it doesn't live in a class, so it has none of the hidden parameters that bother you. (As an exercise, you may rewrite this approach to use = instead of +=; it's irrelevant to the point made here.)

Answer 5

There's validity to some (if not most) of your observations, but the conclusions you draw from them are too extreme.

1. It is tightly coupled to the concrete data source. (it reads current datetime from machine it runs on)

Correct. The date value should either be passed as a parameter or a clock-like dependency should be injected.

Note that dependency injection requires a non-static class and method. More on that later.

Take note of the latter suggestion (injecting a dependency). Your question argues against this idea, and that's where your observation goes off the rails. More on that later.

2. It violates the Single Responsibility Principle (SRP).

I don't see how it does, and you didn't justify why you think it does either. This method does one thing. SRP does not focus on whether dependencies are injected, SRP focuses on the logic contained within the class. This class has one strictly defined purpose: generate a human-friendly label for the current time of day.

Just to be clear: the code can be improved, but SRP isn't what comes to mind as a violation here.

The argument that fetching the datetime value is a discrete responsibility is a strenuous argument. Any responsibility can be subdivided into smaller responsibilities - but there is a line drawn between what's reasonable and what's overkill. Assuming the method conveys that the current time of day is being evaluated, this is not an SRP violation.

3. It lies about the information required to get its job done. Developers must read every line of the actual source code to understand what hidden inputs are used...

That's arguable. When I see GetTimeOfDay and it does not clearly take in a datetime value (either as method parameter or dependency), then the logical inference is that the current time is being used.
Even semantically, "getting the time of day" suggests that you are getting the current time, so I don't quite see an issue here with the naming.

...and where they come from. ...

This, I do agree on. You have no idea whether it's relying on the system clock, or a cloud-based API or ... This is solved when you inject it as a dependency or add it as a method parameter.

The method signature alone is not enough to understand the method’s behaviour.

Most OOP principles (SOLID among others) focus on classes, not methods. You should not observe methods by themselves, you should view them as operations on a class, and more specifically on a known instance of that class.

As far as code readability is concerned, you may assume that whoever calls a class method on an instance (object) of that class is also aware of how that object was constructed in the first place. That is not always the case, but when it is not the case this means that the caller has consented to delegate the object construction.

That is not your responsibility (you = the designer of the consumed class). You cannot and should not try to manage how your consumers delegate their own work internally.

When the source of the datetime value has been refactored to be an injected dependency or a method parameter, then the issue pointed out in your third bullet point is null and void.

So how to deal with self...?

"deal with" implies that it's a problem or unwanted item. Your discourse on self and the alleged issues with it carries an undertone of dislike for the concept of object-oriented state.

If that is how you feel, and you don't want to shift your way of thinking, that's okay too. Programming is an abstract concept of the mind, and different approaches exist to solve the same problem. In that case, you should consider moving to functional programming instead of object-oriented programming, for one major reason:

self is at the heart of OOP.

Objects track state. That is what they do. If they didn't, then your codebase exists only of methods, and then all of those methods could be made static.

self is the keyword that allows you to access the current object's state. Without self, you're effectively unable to actually store and retrieve object state, and thus we would revert back to a system where everything is just a collection of static methods.

Note: in your question, you've conveyed that you judge each method individually. That is actually in line with how you work with static methods, but it's not compatible with how you should think about object-oriented code.

... and how to use it properly?

This goes back to the part where I said that you need to observe things at a class level, not at a method level.

The easiest way to think of it is that the state stored in an object (i.e. via self, usually done via the constructor) has been configured once and is reusably accessible by all of that class' methods. For example:

public class Clock
{
    public DateTime GetDateTime()
    {
        return DateTime.Now;
    }
}

public class SundayChecker
{
    private Clock clock;

    public SundayChecker(Clock clock)
    {
        this.clock = clock;
    }

    public bool IsItSunday()
    {
        var now = this.clock.GetDateTime();
        return now.DayOfWeek == DayOfWeek.Sunday;
    }
}

Notice how I only had to tell the SundayChecker which clock it should use once, but I am then able to repeatedly check the current time and confirm whether it's Sunday or not.

This is just a simple example, but it showcases the basic nature of OOP.

Note: there are many more arguments in favor of using object state, but this is the easiest one to grasp in order to shift your mind into an OOP-compatible frame.

This is much too broad for an in-depth explanation on OOP and how it should be used. I suggest you research OOP tutorials and exercises that teach you to use (and in turn know how to leverage) object-oriented code.

It's small method and it's easy to read but what about methods with hundred of lines which reads and assigns to self in many places?

Anything can be overkilled. Just because OOP has its uses does not mean that it cannot be abused or badly written.

• OOP or not, methods with hundreds of lines are inherently a code quality issue.
• While object state can be manipulated and that's not inherently a bad idea, having an object whose state is constantly being altered to the point of no longer being able to keep track of what its state is, is also a code quality issue.

But these are not arguments against using OOP as a blanket rule. That's like saying that no one should ever use a hammer because you've seen your dad hit his thumb with a hammer.
Mistakes happen but the existence of mistakes does not disprove the concept as a whole.

Answer 6

It is bad to call time of day "now" within a method that also computes something like the time of day string as you've shown.  This is because,

• if you want to know the time of day string from some other time than now, you simply can't use this method — that makes this method much less useful and you have to repeat its logic to use that logic in another way.

• if you want to know the time of day string but also want the actual time of day now, you end up calling time of day now twice, and the two separate calls to "now" could easily be different values, where authors of the code are most likely expecting them to have matched exactly.

Ideally if you need the time of day "now", then that is obtained only once (per whatever) and passed as parameter to any code that is dealing with the "current" time.