Требую больше immutability!

Вообщем тривиальная слоган, его можно прочитать даже в достаточно старой Joshua Bloch: Effective Java, Item 15: Minimize mutability.

Под катом чутка банальности, так как сегодня доделал и наболело.

Сейчас занимался рефакторингом проекта, в частности улучшением/уменьшением кода синхронизации. И как одно из решений, несколько классов, которые должны были быть immutable - сделал таковыми. В большинстве случаев было просто, но есть один классик, который используется почти во всех частях системы. После удаления из него mutator'ов, получил 600 ошибок компиляции, после их разрешения, получил более полусотни падающих тестов. И вот тут самое интересное, что меня заставило написать пост. А именно, обнаружились тесты, которые были совершенно неправильные, они ничего не тестировали и скорее всего бы не упали при присутствие проблемы. Вот упрощённый примерчик, кое-чего подобного:

copy to clipboardподсветка кода

1. public static enum Status {  
2.         APPROVE, PROCESSED  
3.     }  
4.   
5.     public static class Bean {  
6.   
7.         private int id;  
8.   
9.         private Status status = Status.APPROVE;  
10.   
11.         public Bean(int id) {  
12.             this.id = id;  
13.         }  
14.   
15.         public void markProcessed() {  
16.             status = Status.PROCESSED;  
17.         }  
18.   
19.         public int getId() {  
20.             return id;  
21.         }  
22.   
23.         public Status getStatus() {  
24.             return status;  
25.         }  
26.   
27.         ....                
28.   
29.     }  
30.   
31.     public static class BeanQueue {  
32.   
33.         private Map queue = new TreeMap();  
34.   
35.         public void enqueue(Bean bean) {  
36.             if (bean == null) {  
37.                 return;  
38.             }  
39.             queue.put(bean.getId(), bean);  
40.         }  
41.   
42.         public void commit(List ids) {  
43.             for (Integer id : ids) {  
44.                 if (id != null && queue.containsKey(id)) {  
45.                     queue.get(id).markProcessed();  
46.                 }  
47.             }  
48.         }  
49.   
50.         public Bean get(Integer id) {  
51.             if (id == null || !queue.containsKey(id)) {  
52.                 return null;  
53.             }  
54.             return queue.get(id);  
55.         }  
56.         ....
57.   
58.     }  

Основной смысл: есть какой-то объект, который переходит из разных статусов и очередь этих объектов.

Вроде даже с большего выглядит не страшно. И его тестирует такой тест

 

copy to clipboardподсветка кода

1. @Test  
2. public void testCommit() {  
3.     BeanQueue queue = new BeanQueue();  
4.   
5.     Bean bean1 = new Bean(1);  
6.     queue.enqueue(bean1);  
7.   
8.     Bean bean2 = new Bean(2);  
9.     queue.enqueue(bean2);  
10.   
11.     queue.commit(asList(1));  
12.   
13.     assertEquals(Status.PROCESSED, bean1.getStatus());  
14.     assertEquals(Status.APPROVE, bean2.getStatus());  
15. }  

Тест конечно зелёненький, но если задуматься над его смыслом, это какой-то Ад кромешный.

А если мы вот так изменим метод commit

copy to clipboardподсветка кода

1. public void commit(List ids) {  
2.     for (Integer id : ids) {  
3.         if (id != null && queue.containsKey(id)) {  
4.             Bean bean = queue.get(id);  
5.             queue.remove(id);  
6.             bean.markProcessed();                      
7.         }  
8.     }  
9. }  

Тест продолжает гореть ярко зелёным цветом, и мы по прежнему уверены что у нас всё гуд.

Правда, надо отдать должное, как только я сделал immutable Bean, тест покраснел :)

 

copy to clipboardподсветка кода

1. public static class Bean {  
2.   
3.         private final int id;  
4.   
5.         private final Status status;  
6.   
7.         public Bean(final int id) {  
8.             this(id, Status.APPROVE);  
9.         }  
10.   
11.         public Bean(final int id, final Status status) {  
12.             this.id = id;  
13.             this.status = status;  
14.         }  
15.   
16.         public Bean createProcessedBean() {  
17.             return new Bean(id, Status.PROCESSED);  
18.         }  
19.   
20.         public int getId() {  
21.             return id;  
22.         }  
23.   
24.         public Status getStatus() {  
25.             return status;  
26.         }          
27.   
28.     }  

Конечно же теперь тест падает

 

copy to clipboardподсветка кода

1. java.lang.AssertionError: expected: but was:
2.     at org.junit.Assert.fail(Assert.java:91)  
3.     at org.junit.Assert.failNotEquals(Assert.java:645)  
4.     at org.junit.Assert.assertEquals(Assert.java:126)  
5.     at org.junit.Assert.assertEquals(Assert.java:145)  
6.     at Test.testCommit(Test.java:108)     

Но он сигнализирует только о том, что он написан совершенно не правильно. Вообщем поинтов тут несколько. Главный: Item 15: Minimize mutability! Кроме того стоит отметить, что к unit тестам надо относиться серъёзно, их код должен быть ничуть не хуже, чем код приложения, иначе это впустую потраченное время и силы. Ну и по тестам конечно стоит в обязательном порядке прочитать Test Driven Development by Kent Beck, потому как это больше книга не по unit тестированию, а по архитектуре приложений. Если unit тесты допиливать как попало, когда уже код готов, это мало чем поможет, лучше чем ничего конечно, но...

Да, и так между прочим, паттер Builder, просто не заменим при работе с immutable объектами, он очень помог при наследовании. Если посмотреть на immutable версию Bean класса, то видна проблема, которая возникнет при наследовании. Если мы у потомка вызовем метод createProcessedBean, то он нам вернёт не копию потомка с измененным статусом, а папочку, т.о. мы потеряем не только тип, но и данные хранившиеся в потомке. Конечно, можно запретить final'ом наследование для immutable классов, но это не по джедайски. Так что был создан внутренний builder класс для Bean.

copy to clipboardподсветка кода

1. protected static abstract class AbstractBeanBuilder<T extends Bean> {   
2.   
3.     // mandatory   
4.     protected final int id;   
5.   
6.     // optional   
7.     protected Status status;   
8.   
9.     public AbstractBeanBuilder(T bean) {   
10.         this.id = bean.id;   
11.         this.status = bean.status;   
12.     }   
13.   
14.     public AbstractBeanBuilder<T> status(final Status status) {   
15.         this.status = status;   
16.         return this;   
17.     }   
18.   
19.     public abstract T build();   
20.   
21. }  

Можем по цепочки менять все поля, а потом методом build сгенерировать immutable Bean. Просто прекрасно, ну и с учётом этого внесём поправки в наш bean class

copy to clipboardподсветка кода

1. public Bean createProcessedBean() {   
2.     return builder().status(Status.PROCESSED).build();   
3. }   
4.    
5. protected AbstractBeanBuilder<? extends Bean> builder() {   
6.     return new BeanBuilder(this);   
7. }   
8.   
9. private static final class BeanBuilder extends AbstractBeanBuilder<Bean> {   
10.   
11.     public BeanBuilder(Bean bean) {   
12.         super(bean);   
13.     }   
14.   
15.     @Override   
16.     public Bean build() {   
17.         return new Bean(id, status);   
18.     }   
19.   
20. }  

Во-первых, когда например добавится ещё одно поле, придётся поменять лишь слегка Builder класс, куча методов, которые создают изменённые копии останутся в том же виде.

Во-вторых, самое главное, для чего всё это и делалось, если мы создадим наследника, и переопределим у него метод builder, который будет возвращать правильный Builder класс, то теперь наш метод createProcessedBean при вызове на потомке, будет возвращать именно копию потомка.

Примерчик:

copy to clipboardподсветка кода

1. public class MyBean extends Bean {   
2.    
3.     private final String info;   
4.    
5.     public MyBean(final int id, final String info) {   
6.         super(id);   
7.         this.info = info;   
8.     }   
9.    
10.     public MyBean(final int id, final Status status, final String info) {   
11.         super(id, status);   
12.         this.info = info;   
13.     }   
14.    
15.     public MyBean createMyBeanWithInfo(final String info) {   
16.         return builder().info(info).build();   
17.     }   
18.    
19.     public String getInfo() {   
20.         return info;   
21.     }   
22.    
23.     @Override   
24.     protected MyBeanBuilder builder() {   
25.         return new MyBeanBuilder(this);   
26.     }   
27.    
28.     private static final class MyBeanBuilder extends AbstractBeanBuilder<MyBean> {   
29.    
30.         private String info;   
31.    
32.         public MyBeanBuilder(MyBean bean) {   
33.             super(bean);   
34.             this.info = bean.info;   
35.         }   
36.    
37.         public MyBeanBuilder info(final String info) {   
38.             this.info = info;   
39.             return this;   
40.         }   
41.    
42.         @Override   
43.         public MyBean build() {   
44.             return new MyBean(id, status, info);   
45.         }   
46.    
47.     }   
48. }   

Всё достаточно просто, делаем свой билдер, потомок абстрактного и переопределяем метод builder, чтобы возвращать наш. Т.о., теперь когда мы вызовем у нашего потомка метод createProcessedBean, он нам вернёт копию нашего потомка с новым статусом. Вот пример кода и его результат.

 

copy to clipboardподсветка кода

1.     public static void main(String[] args) {   
2.         MyBean m = new MyBean(1, "test");   
3.         System.out.println(m);   
4.         m = (MyBean) m.createProcessedBean();   
5.         System.out.println(m);   
6.     }   
7.    
8. output:   
9. MyBean [info=test, {Bean [id=1, status=APPROVE]}]   
10. MyBean [info=test, {Bean [id=1, status=PROCESSED]}]