6

Consider a Message object in Java that stores some text.

public class Message { private String text; private boolean containsDigit; public Message() { //constructor } public String getText() { return text; } public void setText(String text) { this.text = text; } public boolean isContainsDigit() { return containsDigit; } }

This is a persisted object.

I have no problem with data being set in the constructors but the Message's text field can be set after the object is created and when the text is set, the containsDigit field should also be query-able thereafter. The obvious way to do this is in the setter:

public void setText(String text) { // presume existence of method to check for digit if(text.containsDigit()) this.containsDigit = true; this.text = text; }

But does this result in any "best practice" alarms bells going off due to having logic within a setter method?

Would anyone suggest an alternative implementation?

EDIT

I should probably add that the containsDigit field is required because the object is persisted so the containsDigit field can be queried subsequently. Also, in an application using the Spring/Hibernate engine, this setter is constantly called when re-reading/writing the object so was wondering about the practicality/efficiency of this also.

Improve this question
8
  • 1
    Well, thats the whole purpose of using seters and getters - so as to encapsulate and prevent direct access to fields that might not be consistent. Commented Mar 25, 2014 at 13:33
  • 1
    That is sort of the point of a setter :) Commented Mar 25, 2014 at 13:33
  • You could refer this: programmers.stackexchange.com/questions/177133/… programmers.stackexchange.com/questions/126721/… I hope this will help. Commented Mar 25, 2014 at 13:36
  • 1
    To the "this is the point of getters and setters" people. There is also another narrative on this whereby "experts" say that they should be used as assessors ONLY and so purely for access to the variable's value. Otherwise they can "hide" issues/bugs in the code. Commented Mar 25, 2014 at 15:01
  • Could you point out a reference to this narrative. I suspect it applies to immutability which was not part of your question. Commented Mar 25, 2014 at 16:21

5 Answers 5

Reset to default
7

Your case is the very reason for using setters and getters. If you weren't allowed to have logic in a setter, then you might as well access the fields directly!

Improve this answer
Sign up to request clarification or add additional context in comments.

1 Comment

Yeah, but this is what happens in day to day enterprise software - getters and setters are just generated methods of java beans with 0 logic and the expectation is that setter just sets a value and you get surprised when a unit test you've just written fails because there is some "hidden" logic in the setter..
2

It is probably not the best practice, however sometimes the life is stronger than best practices.

Probably better approach is to remove field containsDigit and move the logic you added to setter into isContainsDigit(): 

public class Message { private final static Pattern d = Pattern.compile("\\d"); private String text; public String getText() { return text; } public void setText(String text) { this.text = text; } public boolean isContainsDigit() { return text == null ? false : d.matcher(text).find(); } }
Improve this answer

6 Comments

This is the preferred solution in best practice with Java standards. Just in case there's another way text can be modified, this solution will prevent you from manually having to set containsDigit.
@DesertIvy, u r right, but this is even good because this field cannot be set separately from the text. It should be computed automatically by definition.
Remember that this is a persisted object. Therefore there is a need to have a containsDigit member variable to satisfy the ORM requirements so, unless I'm missing something, it rules this suggestion out.
@dre, persistency is not a reason to create bad design or data duplication. JPA for example supports @PostLoad and @PreUpdate annotations that help to initiate fields automatically. In this case even this is not needed: we just do not have to persist the flag isConainsDigit() unless there are DB queries based on this flag.
@AlexR Firstly, remember that this is just a hypothetical example and the reason I'm here is to look for good design suggestions. The idea of @PostLoad and @PreUpdate are exactly the kind of suggestions I'm looking for except they are limited to a JPA implementation. Oh and presume there is a need to query the flag.
|
1

Implement the method public boolean isContainsDigit() as following:

public boolean isContainsDigit() { return getText().containsDigit(); }

This way you do not have to keep them both in sync, while having to reevaluate it again and again. On the other side, never performance optimize your code, if you have no need to do it. The method setText() and isContainsDigit() fall under racing conditions, if they are accessed concurrently by two threads. Perhaps you have to synchronize them if you want to address this issue.

Improve this answer

1 Comment

This is not just a requirement for a method to query the text for a digit, the result also needs to be persisted in the database and so the member variable is mandatory.
1

@dre - really its too bizarre. neither of the links that Saint linked to differentiate themselves substantially from your question other than to argue about some kind of design elegance or purity. If you have a "setter" then the whole point is to be able to surround the setting of the variable with some logic that ensures that the variable gets set correctly taking into account dependencies that exist to other attributes in the class provided such need arises. If "setters" are going to be "pure", then get rid of them and make the variable public. There might actually be something in Robert Martin's book "clean code" that explicitly speaks to the principle of the occam's razor or the principle of parsimony. Perhaps this link helps : http://effectivesoftwaredesign.com/2013/08/05/simplicity-in-software-design-kiss-yagni-and-occams-razor/. Finally it should be noted that it is not at all recommended to make an attribute public since going back and making an attribute private from public is a nightmare. i reiterate - first choose to make your class immutable with all member attributes private. If users of the class scream, ask them to justify why they need to mutate it. Once their justification is taken into account, create a setter, do not make the variable public. That should work as a set of design rules. Hibernate or JPA should have nothing to do with this design decision.

To make a class that has many parameters immutable use the builder pattern - When would you use the Builder Pattern?

Improve this answer

3 Comments

"to argue about some kind of design elegance or purity" exactly! Thats what I'm asking, is there any substance to such an argument?? You seem to think I'm arguing for or trying to justify a decision to make a member variable public. Definitely not! I think the question is just about answered for me at this point anyway, if you shouldn't add logic to a setter, then what is the point in having one? Of course, I mention hibernate as it is a requirement of hibernate to provide a setter (although I think you can make it private).
@dre - no, i dont think so, i do know ;-) a computer scientist is all knowing:-)
"a computer scientist is all knowing". That'll do, I can accept that. :-)
0

I can say that this is "good enough". However, if you want to follow object oriented design completely you will implement something like event onTextChange, that will be fired whenever set changes text. On the other hand you will implement handler for this event that will update containsDigit field. For your simple example this is really unnecessary but if you expect adding new logic in future in setText() you can consider this.

Improve this answer

4 Comments

I do not agree. isContainsDigit() is a derived information, so as long as you do not have any need of caching the result, emitting an event is over-design in my opinion.
I wrote that in this particular case this is not something I would do. Even if isContainsDigit is derived information it is cached so if we assume that calculation of this es expensive I would still use this approach. Calculating this on every get sometimes is not very good. In this case it is OK, but we are considering just example and we can think about all possibilities.
Sure, if you have to cache because some computation is very expansive I have to agree with the notification design. Either an observer or emitting an event is the most obvious thing one should do.
Have to agree with @Harmlezz, this seems overkill and further unnecessary abstraction of functionality.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.