Getters/Setters and object oriented design? Is everything my school teaches wrong?

[Disch]

This thread is getting nuts. These examples are academic and quite impractical.

You guys are trying to create a class that is too complicated for what should be incredibly simple. We are talking about 2 different things here: Cartesian coordinates and polar coordinates. Yes, you will likely want a way to convert between the two... but that does not mean they should be in the same class. They are two very different things and have different capabilities.

Encapsulating this class just so you can combine polar/cartesian under the same roof is a bad approach. To use the class effectively, the user is going to need to know which kind point they are dealing with. IE, if the user is moving points around linearly a lot, they're going to want the implementation to be cartesian. Likewise I'm sure there's some kind of operation that works better with polar coordinates, but I never use them so I can't think of one. =P

Let the programmer choose their tool. Don't try to make an all-in-one.

The programmer can choose between vector/map/list/deque. Yes, they all do more or less the same thing, but their implementation is different. Some are better at some operations than others. There's no general 'container' class that tries to guess which underlying storage mechanism would be best. That'd be ridiculous. Just as there shouldn't be a general 'point' class which tries to guess whether or not polar/cartesian storage would be best.


Not every class needs to be encapsulated. It's not always a good idea. Here is one such instance where encapsulation does not help, but rather just obfuscates the code and makes the class much clunkier to use.

MiiniPaa wrote:

Many questions abous "Should I use getters/setters" are void if you design classes beginning with public interface first.

I agree. And this kind of goes with what I was trying to say originally: if your idea of encapsulation is just writing getters/setters for all your members, you are not encapsulating. To do it properly, you have to think of the interface first, and the implementation afterwards.

It should be immutable (aside from assigment). So no mutators at all.

I'm not a fan of the immutable paradigm. It makes classes harder to use.

LB wrote:

@Disch your record class is well designed.

Ugh. It's crap.

You could make the functions virtual and have MemoryRecord, DiskRecord, DatabaseRecord, etc.

There's no value to that. Are MemoryRecords and DiskRecords going to have a different "name" attribute? If not, then why would get/setName need to be virtual? And if yes, then get/setName is a bad function anyway because it's ambiguous. Which name are we getting/setting?

[LB]

@Disch I don't understand what you're saying with the name attribute. A MemoryRecord would be your class as it is now. A DiskRecord would go to the file system for each call of get/set. A DatabaseRecord would go to the database for each call to get/set. A NetworkRecord would contact a server for each all to get/set. It doesn't matter whether it's the name or some other property. It was also just a quick example.

Here's a more real-world example. I'm the developer of a library called MCModify that lets you modify the various files that Minecraft uses. Basically everything is getters and setters, but I don't go for a plain "make all members public" approach because many getters and setters perform validation or transformations on the data so that you can't do things which Minecraft will not accept.

I have a Region base class which is derived by DiskRegion (goes to the disk each time you get or set a chunk) and MemoryRegion (loads the entire region into memory). All the complicated stuff with compression formats, the offset and timestamp sectors, etc. is handled internally and all you see is a get/set interface (getChunk, getTimestamp, setChunk, setTimestamp).

The base Region class has some protected helper functions and classes to help avoid code duplication since the region format doesn't change whether it's on a disk or in memory, but you could completely ignore those and create NetworkRegion which contacts a server for the get/set function calls and still use the same interface.

[Disch]

@Disch I don't understand what you're saying with the name attribute

The Record class had a member called 'name'. The getter/setter just returned/assigned that member. Making that function virtual is pointless.

A DiskRecord would go to the file system for each call of get/set. A DatabaseRecord would go to the database for each call to get/set. A NetworkRecord would contact a server for each all to get/set. It doesn't matter whether it's the name or some other property. It was also just a quick example.

This would be a class with a different purpose from what I was trying to illustrate.

What I'm illustrating is a container to hold information about a record. What you are illustrating is a stream: Something that actually serializes data. These are two completely different things.

If you are proposing that the container class doesn't actually hold the information, but rather actually serializes it on every get/set -- that's not what I would consider a good idea.

Furthermore... get/set is expected to be quick. For potentially lengthy serialization operations, something like read/write would be more appropriate. IE, something that is clearly not just getting/setting something, but rather is transferring data to somewhere.

Here's a more real-world example. I'm the developer of a library called MCModify that lets you modify the various files that Minecraft uses. Basically everything is getters and setters, but I don't go for a plain "make all members public" approach because many getters and setters perform validation or transformations on the data so that you can't do things which Minecraft will not accept.

Well again... I'm not saying all getters/setters are bad. I'm saying they should be avoided (especially by beginners who don't have the judgement necessary to determine when their use is appropriate).

I have a Region base class which is derived by DiskRegion (goes to the disk each time you get or set a chunk) and MemoryRegion (loads the entire region into memory).

I would question that design decision. It sounds to me like you are making classes that do too much.

[LB]

Yes, my examples are extreme (especially the networking ones). 99% of the time the most a setter will do is ensure that a conversion from an unsafe to a safe is valid and the most a getter will do is perform a conversion from a safe to an unsafe. But anything can happen in that conversion process, such as writing to a log about truncating conversions between safe and unsafe, or throwing exceptions about illegal arguments or an illegal state.

Disch wrote:

I would question that design decision. It sounds to me like you are making classes that do too much.

Care to elaborate? I initially only had the DiskRegion but it was requested to also have a way to load the entire region into memory. Why should existing code care what kind of region it is given access to? When you have a Region you only want to be able to get or set the chunks and their timestamps, and you don't care how it's done.


In general, the decision about getters/setters boils down to one question: does your class need to control its state, or does it only need to maintain its state?

You seem to want all classes everywhere to control their state. I am saying that there are plenty of cases where a class should only maintain its state.

A Record only maintains its state. An Entity controls its state.


It may well be easier to just tell beginners "only make classes that control their state" because it's more complicated to explain when to make classes that only maintain their state. But beginners don't stay beginners forever and if they ask you should be prepared to answer.