Getter-Setter Pattern Considered Harmful | Hacker News Reader

Getter-Setter Pattern Considered Harmful

wolfgang-ziegler.com

42 pointsgm67886 comments

This misses the main problem with get/set methods.

In OO othodoxy, you aren't supposed to make state public, because it violates encapsulation. If you want to modify your Point object to use polar coordinates instead of cartesian, then if you expose your cartesian state (x and y), then all the code that relies on x and y is now broken.

So what getX() and setX() do is allow your x and y fields to be encapsulated. Problem solved! Except that once you change to polar coordinates you have your choice of problems:

1) Find all the getX() and getY() uses, and rewrite the affected code to use getR() and getTheta(), since you are now using polar coordinates. In other words, you have violated encapsulation by having getX() and getY() methods, and your get/set methodology hasn't achieved anything.

2) Re-implement getX() and getY() in terms of polar coordinates. That's doable. But do you also now have getR() and getTheta()? Why?

Having getFoo() and setFoo() methods for each field foo is this dumb idea that began, I think, with JavaBeans. It has nothing to do with OO as I understand it. The idea behind OO encapsulation is to separate your object's behavior (the methods) from private state. You can change state at will as long as you maintain behavior. Mechanically adding behavior (get/set methods) to track your private state has always been a stupid idea that has nothing to do with OO except to violate encapsulation.

> Mechanically adding behavior (get/set methods) to track your private state has always been a stupid idea that has nothing to do with OO except to violate encapsulation.

What should be private state is a matter of debate.

If you have a point, it's not too ridiculous to ask "where is it?" Cartesian coordinates may or may not be how the Point object represents where it is, and that internal detail should be hidden from callers.

> But do you also now have getR() and getTheta()? Why?

It seems like a Point class should hold its internally state in the "best" implementation for how it is usually used, and provide its state out to callers in all of the convenient forms that would be used more than occasionally.

Of course, one should avoid purely exposing internal details. And sometimes you might encapsulate more to encourage callers to use the pointA.getDistanceAndBearing(pointB) call. (But I don't think this makes sense for points; knowing their location is too fundamental).

They enable your option 2 and allow the implementation to change behind the public API. Should theta/R have getters is entirely up to your API decisions.

The point of getters and setters is they're the bare minimum of encapsulation. The advice is to ensure even reading a field should be abstracted so implementation details do not cause large reactors of downstream code.

How this is conflated with 'every field should have getters and setters' like it is in this discussion I do not know but that is not the intention.

OO design is ultimately API design.

Unfortunately, it's often taught with toy noun examples that have obvious/actual internal fields and data structures.

It'd be better taught solely using abstract nouns, to better nudge learners into explicitly thinking about external/API design vs internal state.

Or, in other words, getters and setters are subcases where the external/API exposed is 1:1 with internal state.

> Should theta/R have getters is entirely up to your API decisions

But it often isn't. The blog post starts out talking about how that field -> getter/setter pattern is very widespread, and tools (JavaBeans tools for example), make that pattern unavoidable sometimes. Yes, it should be an API decision, but for a variety of reasons -- tools, patterns, coding standards, cargo cult programming -- it isn't.

What's the point of this article if those are unavoidable and unaddressed?

Or:

3) implement getX() and getY() alongside getR() and getTheta() and allow user code to decide which to use. Double the functionality without having to expose internal state.

4) implement a PolarPoint class with a way to convert between that and Point. This is more applicable if the concepts are not orthogonal but also not as strongly correlated as coordinate systems.

The two ways of handling you describe are not how I would actually handle any of this in real situations.

Getters and setters meanwhile allow you to do things like recalculate internal state whenever you call setX() or setR() to amortize computational costs or do any other kind of change tracking without having to rely on some more language-specific feature to do so. Plus getters and setters work across network boundaries unlike modifying state directly.

Point 2) feels very weak. You could easily have getR and getTheta, but critically you could also always have had them - if they make sense as APIs, and your state is encapsulated, you can expose them at any time.

The benefit of having getters is that API consumers don’t privilege public state above any other API component, because they don’t know what’s actually the real state and what’s just under contract.

Given your Coord example, can you come up with a better OO version than the article's advice of "can I make it immutable"?

The immutable version would let you have all four fields. A setter on any of the 4 items (should you desire it) will return a new object whose four fields remain consistent with one another.

...and how precise do the switches between coordinate systems have to be? depending on your format for floats, you will land on areas that are only expressible with perfect precision in one or the other coordinate system. I can imagine a fixed point that moves around by being repeatedly queried in one or the other system.

Point object could be either, since the coordinate systems are truly interchangeable, e.g. the class could just store everything as Cartesian internally and convert to/from polar as needed. In a class with more mutually exclusive properties I can see how this wouldn’t work so well.

To answer a few sibling responses:

The point is that mechanically maintaining get/set methods for each field is completely at odds with encapsulation. To continue the example, sure, go ahead, have any or all of get/setX, get/setY, get/setR, get/setTheta if they make sense for your application. But writing them just because you've created a field, and the coding standards say they should be there is dumb. The coding standards that say this (like JavaBeans) may have their reasons, but OO ain't it.

C# has the with part as a language built in; we use it at work with our immutable objects.

    var y = x with { things I want to change }

https://learn.microsoft.com/en-us/dotnet/csharp/language-ref...

JS/TS has

    y = { ...x, change: 'b' }

And Swift avoids the mutability-problem all together, by providing structs with copy-on-write and self-mutating methods:

https://docs.swift.org/swift-book/documentation/the-swift-pr...

I think C# borrowed this feature from functional languages like Haskell or OCaml.

I would credit F# even more. It's been a main source of new C# features for a while now.

From F#, most likely! C# converges more and more on F# with every release.

What terrible advice. Your public variables are so much easier to refactor if/when your data changes when you have simple accessors methods around them. Having accessors for data have saved my ass multiple times when I've had to do things such as a) migrate the data transparently b) remove data and use computation instead c) debug reads or writes of data.

The last point, sure you can have a breakpoint on a memory address but if you have a getter/setter method its trivial to put a breakpoint in there, but I bet that if you have to set a memory brekapoint you have to waste time googling how do that (at least with GDB)

Raw data POD type structs are useful but only when the scope is limited.

The article doesn't suggest making the properties public, it makes valid suggestions IMHO (though I don't agree with all). I would recommend reading the article.

It does this in the end, though

I feel like this is one of the things python got right. You can start off with plain old data:

    def Foo:
        def __init__(self, foo):
            self.foo=foo

Use it in all the ways you like. If then later you discover a need for a getter/setter type setup (because you're now e.g. storing foo in a different format internally) you can switch without any of the code depending on the Foo class needing to change:

    def Foo:
        def __init__(self, foo):
            self.foo=foo

        @property
        def foo(self):
            return unmunge(self._foo)
    
        @foo.setter
        def foo(self, v):
            self._foo = munge(v)

I'll grant that the syntax is slightly wonky, but its really nice to not have to think about getter/setters when initially designing a class. You can add them when and where you need them without breaking other code.

    def __init__(self, foo):
            self.foo=foo

should, in this example, probably be:

    def __init__(self, foo):
            self.foo=munge(foo)

Ah yeah missed that. Added the munge/unmunge to it as a final pass because I figured it'd clarify the use case slightly better.

I don't think this kind of article is that helpful.

There are aspects of a valid point in there, but it's buried and confused.

That builder pattern makes me sick to my stomach.

Also, it starts with claiming state is part of the problem with getters/setters, but does not mention it again. (It seems to be conflating mutability and state, which, while often related in practice, are not necessarily that tightly coupled -- an immutable thing can nevertheless be state, and you can have mutability without introducing state. State is really about things that live beyond the immediate context/scope. So really nothing to do with getter/setter.)

Tell me about something which is mutable and non-stateful.

I think you should take a minute to think about it.

Let me turn it around on you: Give me a simple example in code of something that is mutable and not state.

I'll come back later to check your answer and provide one of my own, if necessary.

OK, here it is:

    int addOne(int value) {
        value += 1;
        return value;
    }

value is clearly mutable, yet not state.

State is a value you can refer to later. Here, "value" doesn't outlive the function, so can't be referred to later.

State is a complication because you have to worry about whether it remains up-to-date.

(Mutable state is even more complex because you also have to worry about if/when it may be changed.)

I always thought blank / barebones getters and setters were kind of dumb and genuinely pointless, some obscure ceremonial thing that. C# added syntactic sugar for them.

If you are not validating any of the input you might as well just have a public variable.

I thought there were two reasons. You can add validation for all setters, and abstract it as an interface later if needed.

I’ve written a lot of C# and have heard the proclaimed reasons. I’ve stopped doing this over time. You almost never need the interface and when you do, I haven’t found it hard to add.

Maybe you do this by default if you are developing an external API for versioning reasons but that’s such a niche use case.

When you change from fields to properties with getters and setters you break binary compatibility with previous versions, which is very bad for libraries.

How much legacy code have you maintained/added features to vs. writing new code?

I find that getters and setters seem like pointless overhead when I have an idea that I want to test empirically as soon as possible, but if I don't, I end up with a lot of regret years later when I have to track down all the references and update them instead of just changing code in the one place.

I'm not a fan of lazy loading, but I know a lot of people are, and using the getter/setter pattern also enables fairly straightforward ways to lazy-load properties from persistent storage (or add that capability down the road if it becomes advantageous).

Also lets you maintain your invariants. It's particularly useful for cache invalidation. Like if your object exposes a hash of all its public properties, you can invalidate the cached hash when a prop setter is called with a new value.

You just reminded me of one of my main reasons to avoid getters, which the article didn't mention.

Accessing a field will not invoke other code.

Unless it's synchronized. Then you could deadlock.

This got me curious, so I tried it out.

I was unable to put a synchronized onto the field directly:

  error: modifier synchronized not allowed here
    public synchronized final Object field;

But when I locked it in code:

  synchronized (field) {
    while(true) {
      ..

I was still able to access the synchronized field directly without triggering a deadlock - whereas a getter() could be synchronized and trigger a deadlock.

In any case, the article already made a strong points for immutability so you don't need to mess around with deadlocking crap.

They're saying multiple things can synchronize on the same public field. Same reason you don't want to use this.

Although I suppose they could mean the concept and not the keyword. A field might not be thread safe and there's no way for an object to guarantee thread safe access of to a field.

Deadlocking was raised as the objection to using public instead of getter. But in regard to deadlocking, using public is strictly better than a getter.

You have to take a lock in order to deadlock in the public version. In the getter version, you can take a lock without realising it and then deadlock.

nope, sorry, that's my mistake. I'm misremembering java, it has been too long :) thanks for the correction!

To me that's just a matter of having standards that have clear requirements of what's okay to put in a getter - imho, refreshing invalidated caches (as long as they're lightweight and not like network communication) is perfectly appropriate. Anything with a side-effect that can be observed from outside of the object (creating the case where you get code just hitting setters and getters to cause side effects) is right out.

I mean, Lazy<T> is a great example of use of getters.

I never understood this argument for statically typed languages like Java.

> You can add validation for all setters, and abstract it as an interface later if needed.

Sure, but why not just postpone the introduction of setters until you actually need them. 90% of time you don't, in which case we just saved a ton of boilerplate and ceremony. The 10% of time that you do, converting public variables to a setter method is about 2 IDE clicks these days.

The only argument I can think of is if you are exposing a public library API, in which case you need to worry about backwards API compatibility.

Generating getters is actually 2 IDE clicks that will protect you down the road.

Trying to refactor fields that made it into a public API is a nightmare. If you're shipping code people use, you won't get to refactor their projects from your upstream library.

Also, no one is suggesting everything needs a setter. Where is this coming from?

If I could I would make this two IDE clicks behind a big mean warning. In my experience people tends to autopilot it without thinking about what that entails.

> abstract it as an interface later if needed

this later literally never comes. if you didn't have an ontology when you wrote the code, it never magically unexpectedly appears.

This article sort of reads like they think everything should be immutable, which feels kind of dogmatic. Using `with` for everything by default seems like overkill. But in C#, lately I have been using `readonly and `init` wherever vars should not be changed after initialization, which is most of them tbh. For small, data-only objects that can easily be immutable and get passed around a lot, this makes sense to me as a way to avoid the kind of bugs they are talking about.

I’m on board with the With and Builder patterns where it makes sense, but the final suggestion to make the fields public is just bad advice.

The author recognizes that mutable state gives you a lot to worry about, which I agree with. But public mutable fields make it even worse. Getters and setters at least minimize the surface area and give you some control over mutability.

There are many reasons why this is not the common practice, people have learned these lessons before.

I find immutable state to be useful when appropriate. But sometimes you really do want mutability, and it is a feature. Performance of updating player.coordinates, ability to create temporary but potentially complex structures inside the function, lack of need to create a new immutable copy of a bank account’s ledger to add another transaction, all good reasons to allow mutability.

It seems that there are some people who really just think of everything as a state machine and want state transitions to happen only in very controlled and specific ways. And other people seem to focus more on how to organize frequently evolutions of mutable state in a way that makes it easy to reason about. I’m not sure if there is some sort of commonality to these approaches. Different parts of the industry maybe? Different exposure to Haskell?

> lack of need to create a new immutable copy of a bank account’s ledger to add another transaction, all good reasons to allow mutability.

The immutability kool-aid was already drunk by the banking industry 5000 years ago. That's why you're appending transactions on a ledger instead of mutating bank balances (behind getters and setters, so it's "encapsulated" /s).

Sure but your account balance does mutate and so does the length of the ledger. (I know this was sarcasm, but I do think those are important considerations).

If your account balance and ledger disagree, which one is right?

This pattern is really only used with @Entity’s. Things that actually are mutable and rightfully need to be. With @Entity’s setters are exactly what you want because it allows you to add validation, and check error states before persisting.

Arguing immutability for the sake of immutability is considered harmful imo. Only make things immutable when they actually are immutable or when concurrency is at play (which it probably isn’t, 99% of all code is single threaded).

Why not just prevent the construction of bad states instead of constructing bad states and later checking them?

Often, imo, this depends on where you want to put the validation complexity. You can move validation to the front end and "prevent constructing bad states" through a combination of good UI and front end data validation, i.e. validate all the pieces of an entity to make sure the eventual Entity will be valid. But the opposite trade off would be to keep your front end simple, and have the data represent anything that is possible in the UI, and then you can validate anywhere in your stack that you want or where it makes sense for any engineering reason. You can still validate the UI models on the front end before sending them over the wire. You can validate at the endpoint and return HTTP codes. You can run logic in your database to massage/correct/fix the data any way you want. You could even throw things into a queue for post-processing later on, or send to another server for processing, whatever you want.

In theory it sounds great to say "just prevent construction of bad states," but this is a complex problem in professional software that involves coordination between programmers, designers, and product managers to get the UI features into the right place where preventing the construction of bad states doesn't lead to a frustrating UI experience for users. It's often far simpler, more pragmatic, and less complex in terms of coupling in both software and humans to just accept whatever state the UI gives you and figure things out somewhere else where you aren't bothering your users so much.

> Often, imo, this depends on where you want to put the validation complexity. You can move validation to the front end

This has nothing to do with the frontend/backend split since a server will receive arbitrary requests, not just the ones you hope you programmed the frontend to send.

> In theory it sounds great to say "just prevent construction of bad states," but this is a complex problem in professional software

Defeatism.

If you have a User in your domain, and your domain dictates that Users have a UserId, simply do not allow construction of a User without a UserId. What's a UserId? Is it a String, or is it a UUID? If you think the King James Bible translated into Chinese makes for a good UserId, then a String is a perfectly good representation for that. Otherwise pick something more suitable.

I mean I was just trying to have a conversation you don't have to try and roast me with quote-quips and absurdist comparisons... What is happening to this platform these days?

I grounded it.

Any problem can be swept away with "you don't know what you're talking about", "you'll come around to my way of thinking when you're older", "X has nothing to do with Y", etc.

So, make things real with an example.

I think that makes a bad assumption that _you are constructing bad states_ when that isn’t true. You most likely don’t have all the data you need at construction time and the state of the object is still valid. Some data can be null, but later when it is set you want to validate that the data is correct.

For instance when making a person or user you most likely only have their email when you initially create them and then it’s the users job to complete the rest of the data at a later time. Some might want to fill in their age or some might want to fill in their sex. You may never get this data because it’s optional, but when you set it you’ll want to validate it.

And as always in programming _it depends_. It depends on your workflow, requirements, and domain. Being dogmatic about anything is just going to result in worse code.

Use a builder flow, only construct when you have everything you need, don't have any setters, only getters.

That's what I do, anyway.

I personally don’t really like builders. They work in some situations. But if I’m pulling the record from the DB and I only need to set some data on it that is overkill and more complex than need be. The object is already constructed in a valid state.

> The object is already constructed in a valid state.

But that's the entire value proposition of do-not-allow-construction-of-invalid-state!

If a User is actually a User, then you can say "the object is already constructed in a valid state".

If you decide to be 'less dogmatic' and instead construct Users-which-may-or-may-not-be-users-just-throw-an-ex-if-theyre-no-good, then you cannot say that the object is already constructed in a valid state.

I genuinely don't know what you're trying to say here. Perhaps you can show me in code.

That's not the situation described above where data is partial.

In your case I would copy rather than mutate, unless I'm actually getting noticably higher profits from lower memory usage during the time between getting the object and GC. This hasn't happened to me yet.

Maybe I'm not getting what you're saying but the data is partial? In the situation above we have a valid record created by someone else. This is pseudocode obviously, but I genuinely don't know what copying or building or immutability is going to buy me here other than being more complex. There is no worrying about the record being in an invalid state because it could have never gotten to the database in an invalid state.

    class User {
        Long? id;
        String email;
        String? name;
        Integer? age;
        Sex? sex;
        Avatar? avatar;
        
        User(String email) {
             this.email = email;
        }
    }

    record ProfileVm(String? name, Integer? age, Sex? sex, Avatar? avatar) {} 

    // some ui controller
    var user = findById(id)
    if (user == null) {
        return notFound();
    }

    var vm = ProfileVm.fromJson(request.json());
    user.setName(vm.name());
    user.setAge(vm.age());
    user.setSex(vm.sex());
    user.setAvatar(vm.avatar());
    user.save();

    return ok();

Entities are also the one place where you would need a way to express instance private state and not class private state.

This is weird. I use getter/setter regularly for DynamoDB backed DTOs. This helps catch setters at the source of a problem before it gets persisted. Also the exceptions will help pinpoint code paths where these things can happen.

I agree with the article for the most part (except the last snippet where the author reverted to bare, mutable fields), but the point was very badly argued.

I think we really need to consider that sometimes you want a place to hold together data, and some other times you are encoding some behavioral trait, and prior to records Java did not have a way of distinguishing between them.

My rule of thumb is to avoid exposing data in behavioral classes, and prefer immutable patterns for data holders. But sometimes it's not possible to not have setters (e.g. for builders and for JPA entities).

Some decades ago as a child I read a Bjarne Stroustrup interview where he said you use those things to enforce invariants and if you don’t you keep access simple. In the intervening decades, I’ve found that when I have a new invariant I often need to confirm the call sites are compliant anyway as he says. So I just use that, keeping accessors only where required or where interface compliance syntactically requires it.

https://www.artima.com/articles/the-c-style-sweet-spot

> Notice the subtle difference? Dropping those get prefixes for the access methods has significant benefits. Let's think about it: What value was added by the prefix? Correct, none!

Methods and functions do something rather than are something so naming them as the action they complete seems much better form to me. 3 characters is a fair price for clarity.

> Drop the getters and setters. They don't add any value

They add the ability for the underlying object to change without breaking compatibility. This is particularly important on shared libraries where you are not going to be able to refactor every single call sight.

For instance we have an object in our model representing a building, we've gone through *three* different structures of what a building is, but we've managed to make those changes while maintaining compatibility through leaving deprecated methods that translate the calls.

I get it, immutability is nice. But doesn’t it come with memory and runtime overheads for having to create and store new objects?

Depends on the language, some have runtime optimizations to make copies fast (usually FPs with purely immutable data). Not sure if that’s the case in something like Java.

For immutable objects you can use new ‘record’. Another option is to use Lombok which provides an elegant way for reducing code.

Summary: X is bad. How to do something without X? Don't do X

Missed the obvious: age should be replaced with date of birth from which an up-to-date age can always be calculated.

Only when you're a Java programmer can you find that "Builder" suggestion attractive.

is there anything out there that that someone does not consider harmful?

Reads like AI garbage.

The field is final, not it’s contents. It’s not a ”final list” on that field, it’s a (mutable) list that is the final assignment of a field.

Mutability or immutability is part of the type not the field so you have 4 combinations in total

List<T> list;

final List<T> list;

final ImmutableList<T> list;

ImmutableList<T> list;

And that’s not even considering whether each T in the list is mutable or not.

OOP separates coders from craftsmen.

In what sense?

I can't count how many bugs I found and fixed by removing setters.

I avoided Spring for years because of its asinine favoring of setters over correct construction

I dont agree with the post, but wait til you hear about `record`.

please for the love of Anders Hejlsberg just add properties to the language already we've had JavaBeans for like a century now we just need properties like any sane language would have added please bro i promise bro i swear just add properties to the langauge so the expression is a valid right hand side and left hand side look at javabeans bro i promise it'll be ok it's not bad to admit that anders was right on this one it'll be ok bro just add properties please bro no more builders just lets add properties bro

This is a waste of time. No One I've ever seen uses getName.

It's always name.

Also, most of this is useless overkill.

If I have, in this case, a person object, I'm doing two things with it. Displaying it. Editing it.

If displaying, who gives a crap of its mutable? If I'm editing it, why are you wasting memory making a copy?

This just makes life way more difficult in most business apps.

Sure, if you NEED this, go for it. But for everyday bog standard business apps? Get the hell away from me.

To me, much of this just gives away a lack of experience.

Have you never hit a bug because an object mutated unexpectedly? Please do keep this in mind when you do, and spend hours of your life trying to hunt it down.

When you've done this 8 or 9 times after wasting days of your life debugging, you become enlightened to the joy of immutability. It's worth the tiny memory overhead.

Beyond that, get* is everywhere in of corporate OO especially in more classical OO languages. I'd argue the right way to do things as well, naming methods as actions, but that's a separate issue.

This was born from needs in the bog where standard business software lives.

Sure, you can have naked attributes, but you'll be sorry once they aren't enough and you now have two parallel ideologies seeping through your software, increasing the risk of bugs. Concurrency and collection attributes are the common reasons you'll want getters, possibly also setters.

Reply on news.ycombinator.com