2

I marked an immutable data model class as final to make sure the only way to change its values is to create a new instance. (Unfortunately, the fields cannot be final because they needs to be populated by Hibernate.)

This worked well until I wanted to check another class throws the correct exception when called with an invalid instance of the model. The constructor of the model validates the arguments so reflection must be used to set the fields. This is extremely clumsy since the model have quite a few fields and the field names have to be hard-coded.

I can't mock the model either due to it being final. (Is it also debatable whether an interface should be used to enable mocking while keeping the class immutable. By having an interface, there's no way to programmatically mandate the methods must return the same value throughout the life of the instance.)

What do people usually do in this case? Is there any standard approach to this?

billc.cn
  • 7,187
  • 3
  • 39
  • 79
  • 3
    Note: `final` on a class definition does not do the same thing it does in variable declarations. In particular, it doesn't make your type immutable, except inasmuch as it prevents the existence of subclasses that violate the promise of immutability. – cHao Nov 04 '12 at 22:00
  • You actually can mock a final class via instrumentation by using Mockito – rees Nov 04 '12 at 22:08
  • The value of `final` classes is often overstated. It's preventing you from easily testing your code, which seems to me like an argument for removing it. What's the argument for _keeping_ it? – Alan Krueger Nov 04 '12 at 22:08
  • So what you're basically saying is that there's no way to construct an invalid instance of your model class (because, if I follow, the constructor would throw an exception when it's invalid) - so why exactly do you have a test scenario that would imply an invalid model injected into another class? – Guillaume Nov 04 '12 at 22:12
  • 1
    @AlanKrueger On the contrary. I think it is a very valuable construct. You should always make a class `final` if it is not designed to be extended. If you want to mock a class it means that it, with a high probability, should implement an interface or extend an abstract class. – Jagger Nov 04 '12 at 22:15
  • If you are not able to mock your model then make it so it can. Extract your methods to an interface and make the model implement this interface. – Jagger Nov 04 '12 at 22:17
  • In theory, it can help you avoid problems. In practice, it generally keeps you from doing perfectly valid extensions the original designer never thought of. I've never personally encountered a problem it would have solved, and I've hit plenty of instances where we had to use copy-and-paste reuse to work around it. This is likely a religious issue, so YMMV. – Alan Krueger Nov 04 '12 at 23:00
  • @cHao @ AlanKrueger The class is made immutable by not providing mutators. However, this is not enough to guarantee that subclasses won't change the fields, so I have to make it final. @ Guillaume The fields can be populated by Hibernate which will not use the constructor to check the values. @ Jagger I have discussed the problem with interfaces. You can only put the immutable constraints in the JavaDoc of an interface. There's nothing to prevent somebody from implementing a mutable implementation. – billc.cn Nov 04 '12 at 23:21
  • @cHao The intention is that all access to the database should go through this class so the validity of the data can be checked. However, there's nothing to prevent users with the database password to go in and change the data directly. The classes that uses this model are not expected to handle problematic data, but should throw if the data is obviously wrong. Also, please don't swear. – billc.cn Nov 05 '12 at 00:59
  • @billc.cn: ...You do realize you *just told me how to create an invalid instance*, right? Just break the data in the DB. – cHao Nov 05 '12 at 01:03
  • @cHao More or less. In general we trust the integrity of the data in the database (as access to it is very tight controlled) and use it directly without further validation. However, in a few places the database data is double checked (mainly because they're copied to another class that have separated validation code). If invalid data cannot be created, then there's no way to test these validations. – billc.cn Nov 05 '12 at 02:03
  • If invalid data can not be created, then by definition, *your instances will always be valid*, and the validation becomes unnecessary. Seems to me you should be focusing on the DB and ensuring that it rejects invalid data, rather than trying to create an invalid object that should never exist. – cHao Nov 05 '12 at 02:07

2 Answers2

3

Generally speaking, you shouldn't want to mock data objects. Data objects should have no logic and no external dependencies, so there's not really much use to mocking the objects. Instead make it very easy to create fake instances that you can populate in methods as you'd like.

Furthermore, there are a few other reasons you might want to avoid treating a Hibernate-persisted object as immutable:

  • Hibernate-provided objects are inherently not thread-safe and therefore lose the thread-safety advantages that immutable value objects typically provide.
  • You may find your objects are actually proxies, possibly undercutting the final semantics.
  • Hibernate-controlled objects operate completely differently whether their session is still open (attached vs detached) making them a very poor choice for an immutable object. If your immutable object depends on session lifetime, it's not really immutable.
  • It sounds like some objects may be valid or invalid at the application layer, beyond database-layer validation. That makes it a little harder to encapsulate your validation concerns.
  • You are required to have a public no-arg constructor, which is antithetical to the kind of instance control typical of immutable value objects.
  • Because the objects are inherently mutable, it is more complicated to override equals and hashCode.

My advice? If you need more immutability and data validation guarantees than a Hibernate DAO can grant you, then create a real final immutable class with final fields (or a private constructor and static factory method), and then make a constructor (or static factory method) that copies in values from your Hibernate DAO.

If you decide this option, you are stuck with the overhead of having two data objects that change roughly in parallel, but you also get the benefit of separating concerns (in case the Hibernate object should diverge) and the ease of a truly-immutable, equals-and-hashcode-overriding, session-agnostic, guaranteed-valid object that you can easily create for tests.

Community
  • 1
  • 1
Jeff Bowman
  • 90,959
  • 16
  • 217
  • 251
  • I don't quite understand your first point. Calling `get()` with the same ID returns the same object is more or less a preferred behaviour. (It is very clear in Hibernate documentation that a Session is not intended to be shared between threads.) You however reminded me about the potential problem if Hibernate needs to proxy this class for lazy loading. I may have to use the two sets of classes approach. – billc.cn Nov 05 '12 at 19:16
  • @billc.cn Sorry, let me re-word that to be clearer. The shared-instances bit is distracting form the thread-safety point. – Jeff Bowman Nov 05 '12 at 19:26
0

For clarity, making a class final prevents it from being sub-classed. This is good in cases where the class doesn't need to be further refined.

Marking a class level variable as final means that it will only get assigned once. For primitives and immutable objects like String, this has the side effect of making the variable immutable (by default).

However, for mutable objects like Date, your variable will always reference the same instance, but others with access to that instance would still be able to change it's state. For example if you had a method 

public Date getCreatedDate(){
     return this.created; // class variable declared as private final Date created...;
}

Then any caller could access the created instance and change it's state. You would be better to only return truly immutable values, or return a clone.

public Date getCreatedDate(){
     return this.created.clone();
}

EDIT

"I marked an immutable data model class as final to make sure the only way to change its values is to create a new instance"

Your issue as I understand it is that Class A has a dependency on Class B. You wish to test class A and you are unable to mock class B, as you have marked it as final. You marked Class B as final to make it immutable (preventing it's internal state being changed). This is incorrect, as marking a class final prevents it from being sub-classed. It has nothing to do with the ability to change the internal state of an instance.

Your use of final does not have the desired effect. Marking the fields as final is not an option, and would not make the class immutable for the reasons stated above. The only way to protect your data is to prevent clients of your data from having access to the objects that make up it's internal state.

Assuming, that you won't be the only developer, you need to protect the users of your data from unintentional updates. Ensuring that you return clones from getters is one approach. Having team members sub-class and change data is just bad programming, not unintentional, and could be managed through policy and code review.

If you wish to protect your code from external interference by unknown developers (for example writing code that utilises the same namespace to inject their code), then other approaches are available such as package sealing.

Romski
  • 1,912
  • 1
  • 12
  • 27
  • This reasoning about making the class instead of the fields final has been explained in the questions. Essentially the fields cannot be final due to Hibernate, so I removed all the setters. Also, making all the fields final does not ensure the class is immutablable as a sub-class can return arbitrary value in the getters. – billc.cn Nov 05 '12 at 19:24