Are public accessors a CodeSmell?

For the uninitiated, accessors are methods that let you read and write the value of an instance variable of an object.

They are also known as setters and getters.

For some, whenever a public accessor is found in a codebase, AlarmBells start to go off.


'''The Original Argument'''


Public accessors indicate that the data and the behavior of a class are not kept together.

This is seen as a an indication of higher coupling and lower coherence.

In Java, accessors force you to treat each variable as a separate entity and this leads to lots of code with the only purpose of shuffling data back and forth.

In most cases, public accessors are just as smelly as direct public access to member variables.

If that is what you need, why don't you skip the accessors altogether?

They are not going to give you much protection.

As a guideline assume that whenever you add an accessor to an object, you did something wrong.

Ask your compiler (or tests) about who uses the accessor, and then determine whether this is a good use.

Typically, one can categorize most accessor uses into one of two situations.


    * A client of the object makes a decision based upon the state of the object. 

	Ask yourself: Why can't the object itself make this decision?

    * A client of the class needs to move the data to some external medium. 

	Examples include: Database, files, network transport and user interface.

'''So how to externalize data without violating encapsulation'''

This is very close to the question answered by the MementoPattern.

The memento pattern does not seem to apply to user interfaces or network traffic, although it could probably be extended this way easily.

The general approach of my suggested solution makes the object responsible for driving the process. 

A demonstration of idea using Java code follows:

  interface DataTarget {
	public void putData(String key, Object value);
  }


  interface DataSource {
	public Object getData(String key);
  }


  class ObjectToExternalize {
	public void externalizeTo(DataTarget? target) {
	target.putData("myFirstFieldName", myFirstField);
	target.putData("mySecondFieldName", mySecondField);
	target.putData("myThirdFieldName", myThirdField);
	}


	public void readFrom(DataSource source) {
	myFirstField = source.getData("myFirstFieldName");
	mySecondField = source.getData("mySeconFieldName");
	myThirdField = source.getData("myThirdFieldName");
	}
  }


  class DatabaseGateway implements DataTarget, DataSource {
	java.sql.ResultSet rowset;


	public void putData(String key, Object value) {
	rowset.updateObject(key, value);
	}


	public Object getData(String key) {
	rowset.getObject(key);
	}
  }


  class DatabaseMediator {
	public ObjectToExternalize doYourStuff() {
	ObjectToExternalize newObject = new ObjectToExternalize();
	DatabaseGateway data = new DatabaseGateway();
	// Initialize the gateway with a suitable rowset
	newObject.readFrom(data);


	newObject.doYourStuff();


	newObject.externalizeTo(data);


	// commit the update
	}
  }


  class Dialog implements DataSource, DataTarget? {
	// Maps from Strings to java.swing.JTextComponent or something similar
	java.util.Map fieldNamesToControls;	


	public void putData(String key, Object value) {
	JTextComponent component = (JTextComponent)fieldNamesToControls.get(key);
	component.setText(value.toString());
	}


	public Object getData(String key) {
	JTextComponent component = (JTextComponent)fieldNamesToControls.get(key);
	return component.getText(key);
	}
  }

Some ideas for further exploration:

    * One interface DataExchange for both reading and writing. This is roughly equivalent to the MicrosoftFoundationClasses' idea of DataExchange
    * A field name mapper to handle the situation that the internally used field names are different from the external ones (see DecoratorPattern)
    * Specialized interfaces for situations where the semantic behaviour of the fields must be preserved
    * Use of an identity field to ease the mapping to a relational database
    * A spike to see whether the use of String-based keys has any performance impact.
    * A solution for translating from different Object classes to Strings.

-- JohannesBrodwall



''' Feedback '''


We use a similar architecture for getting the data from the model objects to the user interface in our web application. It works well, although I'd like to generalize it, particularly in regards to collections and other composite objects. Our current approach is a mistake and full of DuplicatedCode, but the ReFactored solution that I'm thinking of seems like a reinvention of XML. I'm still toying with these problems. -- MarkAddleman


Personally, I've never had a problem with Accessors, ever. For a spin on this subject from a diametrically opposed opinion, see http://web.archive.org/web/20040708065354/http://www.scs.carleton.ca/~deugo/95540/Notes/Foundation/foundation.html or http://www.rolemodelsoftware.com/moreAboutUs/publications/articles/self-enc.php for a revised edition.

There's a good article on this subject in KentBecksGuideToBetterSmalltalk, "To accessor or not to accessor". I don't think accessors are evil (yet), but seeing a class with 30 fields that do nothing but read and write a privatized variables does have a slight stench about it. The attractiveness of this idea is that it brings us closer to qwon. -- AaronSevivas

Is it just me, or does almost all demo code explaining JavaBeans (the non-Enterprise kind) fit this description?

Usually that's the case, but half of JavaBeans is dynamic property scanning & access through introspection... (the other half being pub/sub events). So the accessors in this case don't necessarily stink since data-setting is the whole point to JavaBeans.

Generally I think this is a good rule of thumb to use except when your task is to (literally) extract data, for a database or GUI. That's why frameworks like JavaBeans, ValueModel & AspectAdaptor (both in VisualWorksSmalltalk), exist... they provide a data-access abstraction that tends to preserve encapsulation. -- StuCharlton

 But are accessors really the best way to move bulk amounts of data around? -- JohannesBrodwall

Accessors are not evil, EvilIsEvil. Too many accessors is bad design, but too many people get confused and decide that more than 0 must be too many. Can there be no middle path? -- JohnFarrell

  I am afraid I will have to go ahead and disagree with you, John.
  Even though I use the phrase to spark a discussion more than violent agreement,
  I must admit that there is more than a core of truth to it. I have yet
  to see an example of use of accessors that did not smell to some extent. 
  I don't think it is realistic (or even valuable)
  to decide that more than 0 is forbidden. Sometimes you have to live with evil,
  but it is still evil. -- JohannesBrodwall (donning the extremist's cape)

Took me a little while to understand the opinion here. Seems fair to say that accessors are used where state is moving between objects. The evilness of this fact depends upon the constraints involved.

Consider that any parameterized function, which might return a value, may be considered equivalent to a setter or a getter, or both, from the point of view of the client. Getters/setters are not privileged in any respect.

I tend to place functionality at the level of minimum explicit coupling. If a function only involves a single object then the function should rest in its class. If the functionality involves two (or more) disparate objects then placing the functionality in either object creates a dependency, so a third object/context is appropriate. This might mean a 'getter/setter', or a direct access. Thus I would have a Complex class and a ComplexOp class which defines the op's on Complex numbers etc. Not very OO I know, but then I prefer my types to remain relatively stable.

For the examples above someone noted that they are 'reinventing XML', and that is a good point. When two objects interact, if that interaction is to be more than an explicit coupling between two classes, then an intermediate protocol is being established, with an intermediate message format. Not only are we reinventing XML, but PUT/GET too. These are universals. It isn't a bad thing at all IMO. Take a look at a paper by AlistairCockburn, The Interaction of Social Issues and Software Architectures (it's only in PDF) at http://alistair.cockburn.us/crystal/articles/isiasa/ACM%20interaction%20of%20soc%20iss%20sw%20arch.pdf

He argues for both ways, and describes a time when accessors paid off. Roughly speaking, he writes:

    * Accessor methods are good, as they provide a constant interface should the data access technique change.
    * Accessor methods are bad, as they slow the software and add complexity to the object's interface.
    * Using accessor methods for every instance variable is good, making the code uniform, hence easier to read and modify (the Consistency principle).
    * Using accessor methods for every instance variable is bad, exposing more of an object's inside than may be desirable (the Abstraction principle).
    * They are controversial: some people swear by them, some swear at them.

Therefore, in the case he describes: Require "persistence accessor methods", getters and setters for the persistent instance variables of persistent objects. They are property of the infrastructure, not the domain designers, and may be regenerated at any time. Access to other instance variables is considered a "local matter" to each design group.

Accessors are not evil because:

   1. logical properties - the get/set interface has nothing to do with your public members. By exposing public members you've locked your self into a set of attributes that may not even exist in later versions, yet the information at the class level has is not the same. It is encapsulation to present an interface for the class that is independent of the underlying implementation.

          For example, in a communication system the underlying data are stored in a serialized buffer for quick transmission. There are set accessors that write directly into the buffer. There is no corresponding attribute to publicly expose.

          Take for example a date attribute in a class. It's no business of a user how you store the date. By exposing a date object you've locked yourself in.

   2. validation - without a degree of indirection you can't insert your own validation layer. The date object, for example, can't know the domain validations associated with the class so the validations can't really be put in that date class.

   3. event propagation - if you have observers you need to say the object has changed. Nobody wants to know a contained date object has changed.

How much of this is a language issue? Some languages allow one to swap attributes with methods without changing the interface (existing calling code). YagNi would dictate making attributes be attributes, and only alter them if something more complicated is later needed. But, if a language requires you to change the calling signature in order to "upgrade" to a method, then you may have to create a bunch of accessors up front to avoid overhauling code upon attribute-to-method changes.

While AccessorsMutatorsViolateOop, they are more a sign of the weakness of the paradigm - or rather, of the particular application of it - than a sign of bad design. They are to circumvent encapsulation while staying within the letter of the law. They exist because OOP is not suited for some types of projects, particularly those which are DataDriven on a very fine scale. Accessors and mutators are only a symptom; the real misuse of tools is coming at a higher level.

Hrmm, while I try to practice avoiding value-laden words, the premise of this page is outright silly. There are typically only two mechanisms for accessing an object's attributes (RubyLanguage only has one, I'd be curious if there are more in odd languages) - 1) direct access to public member variables, and 2) using getters and setters. Suggesting that all instances of 2) smell requires accessing an object's attributes would ideally only be done through 1). Which is laughable because this means that your object cannot validate its attribute values (function setX(v) { if (v < 0) throw FooError; this.v = v; }), cache transparently computed values (function getX() { if (this.x == null) loadXFromDisk(); return X; }), refactor its innards and maintain a consistent interface (var X -> var square_of_X; function getX() return this.X.sqrt(); }). And, there are instances where the object would not be able to guarantee a consistent internal state--sometimes attributes must agree with each other (Example: RightTriangle has three attributes--leg1_length, leg2_length, hypotenuse_length).

More generally, accessing public instance variables can be thought of as less-functional a specialization of getters and setters with a unique interface. (Except in RubyLanguage, where it getters and setters are enforced, but also made easy.)

What _does_ smell about accessors is a whole lot of monotonous code (function getX() { return this.X; } function setX(X) { this.X = X; } function getY() { ...). ThePragmaticProgrammer recommends to use a CodeGenerator in this case. (Except in Ruby, where it is built in :)

ThePragmaticProgrammer recommends always using getters and setters, because it is the more general interface and will weather change. I agree.

-- JasonFelice

Er... transparent getters and setters, and even deleters, are easy to implement with PythonLanguage's "properties". Ruby's not the only cool language out there in this respect. :-) -- ElizabethWiethoff

 "There are typically only two mechanisms for accessing an object's attributes ..."


Wrong. There's a third one: don't. And that's the point of this page. By the way, the title is a hyperbole so take it with a grain a salt. Now, having received this clue, try to parse it again. Related pages are TellDontAsk, LawOfDemeter (accessors invite LOD violations), LawOfDemeterRevisited. -- CostinCozianu

Don't!? I read that before, but did not believe that you were serious. I guess I'm confused, then. Given the following scenarios, are they CodeSmell or just evidence of the hyperbole of the claim? If they are CodeSmell, how else would you implement them?

    * The Point class in most GUI and text UI interfaces: You would simply not access the X and Y, neither by accessor nor by direct access.
    * Many class libraries have a "Complex" class. How would you write a program not accessing the real or imaginary parts?
    * ObjectRelationalMapping's row attributes. Construct replacement objects to serialize to the database whenever the user requests the update of some attribute? Flag the old one as deleted. (But you can't check whether the old one was deleted!)

Which gets me thinking - it suggests to me that perhaps for you this is CodeSmell because the object is mutable, perhaps?

-- JasonFelice

The examples you provided are certainly evidence of the hyperbole in the title, and in addition they are really structures (data), not quite objects - in the sense given in NygaardClassification. In order for the real objects to work, you need all kinds of structs to pass data for the messages between the objects - in languages like Java and Smalltalk that data has to be called an object too, quite confusing perhaps. Whereas in ObjectiveCaml, for example, you may just pass data in place in form of tuples, or declare a record type. Python Ruby and Perl are also quite good at handling plain data. In C++ you'd declare a struct. In Java and Smalltalk you have to declare a class, making them syntactically indistinguishable from the real objects. For all I care, you can make such structs as classes with all fields public.

Oh, and O/R mapping is evil too :)

There is a difference between objects that have "behaviour" and therefore have identity and change their state over time, and values that have no identity and are identified only by their state. Behavioural objects (aka ReferenceObjects or "entities" in DomainDrivenDesign terminology) interact with one another in a TellDontAsk style (following the LawOfDemeter). ValueObjects are processed in a FunctionalProgramming style; they have to be because ValueObjectsShouldBeImmutable. So behavioural objects should not expose accessors while ValueObjects should *only* expose accessors (or transformers that return new objects).

Generalizing to accessors distorts the assessment. Setters and Getters are different breeds of dog.

A setter involves this-here-object modifying the state of that-there-object. This almost always creates maintenance problems and is smelly in the extreme.

If fu needs data from bar because (and only because) bar is the authoritative source or an agent of the authoritative source (users, databases and other outside things need agents), then bar needs to provide a getter interface for fu to use. There are a half-dozen design patterns to choose from that let bar tell any and all interested fus that new or changed data is available and to "come and get it". Since fu is in control, and the coupling is kept to the minimum required for bar to provide its service, it doesn't smell at all. This is the essence of MVC.

-- MarcThibault See also ShouldMemberVariablesBeAccessibleToDerivedClasses ForgetAboutWritingAccessors


Often accessors seem to be used to write procedural code using OO. A good article about this (not all of which I agree with) is http://www.javaworld.com/javaworld/jw-07-1999/jw-07-toolbox.html. -- BenTilly


I can nullify this whole argument using a single counterexample. Imaging you have a class that represents an image; a very typical case in OO UI design. As the image may contain transparent regions, it is useful to have a background color. The background color is a private state of the image class, it is needed internally to perform the "draw" operation, that draws the image on the screen (or somewhere else). However, it makes perfectly sense to set the background color from external code and it makes perfectly sense to ask the image class for the current set background color. I don't know how you would solve this without using an accessor, nor do I see any reason for avoiding this accessor. As with most "... are evil" arguments, people see some pattern being used incorrectly and immediately declare it to be evil. Then they take some (narrow) examples, where it is really possible to avoid the pattern and take that as a proof, that the pattern is unnecessary as a whole. But if you take those patterns to a wide range of examples, you end up with examples where this pattern is unavoidable or where every attempt to avoid this pattern blows up the code complexity by a factor of ten, and guess what, "excessive complex code is evil", about ten times more evil than every anti-pattern I've came across so far. -- Mecki

''The whole "... are evil" thing is hyperbole. It's also a bit cliche these days. It's basically not much more than a "don't use accessors unless you clearly gain something by doing so."''

'''References and Further Reading''''

HexagonalArchitecture, LawOfDemeter, TellDontAsk, Article by AllenHolub in JavaWorld (http://www.javaworld.com/javaworld/jw-09-2003/jw-0905-toolbox.html), RobertMartin argues that sometimes, you can just make the instance variables public (http://www.artima.com/weblogs/viewpost.jsp?thread=36312)

---

''' Refactoring '''

DecemberZeroFive RefactorMe Now before it becomes a ThreadMess: Highlight the main point, show its counterpoint(s), moved references to the top, move discussion to separate page (to be deleted later), rephrase discussion with BlackHat/WhiteHat etc. 

CategoryEvil