Skip to content

Commit 813988b

Browse files
benjchristensenbenjchristensen
committed
Naming and Class Conventions
- make concrete implementations final as extending them is dangerous (use composition and implement Subscription instead) - deprecated long get/setSubscription methods in favor of short verbs (add/get/set/clear/remove) - updated unit tests with changes
1 parent 996e78f commit 813988b

File tree

7 files changed

+101
-96
lines changed

7 files changed

+101
-96
lines changed

‎rxjava-core/src/main/java/rx/subscriptions/BooleanSubscription.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@
2626
*
2727
* @see <a href="http://msdn.microsoft.com/en-us/library/system.reactive.disposables.booleandisposable(v=vs.103).aspx">Rx.Net equivalent BooleanDisposable</a>
2828
*/
29-
public class BooleanSubscription implements Subscription {
29+
public final class BooleanSubscription implements Subscription {
3030

3131
private final AtomicBoolean unsubscribed = new AtomicBoolean(false);
3232
private final Action0 action;

‎rxjava-core/src/main/java/rx/subscriptions/CompositeSubscription.java

Lines changed: 23 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@
3030
*
3131
* @see <a href="http://msdn.microsoft.com/en-us/library/system.reactive.disposables.compositedisposable(v=vs.103).aspx">Rx.Net equivalent CompositeDisposable</a>
3232
*/
33-
public class CompositeSubscription implements Subscription {
33+
public final class CompositeSubscription implements Subscription {
3434

3535
private final AtomicReference<State> state = new AtomicReference<State>();
3636

@@ -75,64 +75,62 @@ public boolean isUnsubscribed() {
7575
}
7676

7777
public void add(final Subscription s) {
78-
State current;
78+
State oldState;
7979
State newState;
8080
do {
81-
current = state.get();
82-
if (current.isUnsubscribed) {
81+
oldState = state.get();
82+
if (oldState.isUnsubscribed) {
8383
s.unsubscribe();
8484
return;
8585
} else {
86-
newState = current.add(s);
86+
newState = oldState.add(s);
8787
}
88-
} while (!state.compareAndSet(current, newState));
88+
} while (!state.compareAndSet(oldState, newState));
8989
}
9090

9191
public void remove(final Subscription s) {
92-
State current;
92+
State oldState;
9393
State newState;
9494
do {
95-
current = state.get();
96-
if (current.isUnsubscribed) {
95+
oldState = state.get();
96+
if (oldState.isUnsubscribed) {
9797
return;
9898
} else {
99-
newState = current.remove(s);
99+
newState = oldState.remove(s);
100100
}
101-
} while (!state.compareAndSet(current, newState));
101+
} while (!state.compareAndSet(oldState, newState));
102102
// if we removed successfully we then need to call unsubscribe on it
103103
s.unsubscribe();
104104
}
105105

106106
public void clear() {
107-
State current;
107+
State oldState;
108108
State newState;
109109
do {
110-
current = state.get();
111-
if (current.isUnsubscribed) {
110+
oldState = state.get();
111+
if (oldState.isUnsubscribed) {
112112
return;
113113
} else {
114-
newState = current.clear();
114+
newState = oldState.clear();
115115
}
116-
} while (!state.compareAndSet(current, newState));
116+
} while (!state.compareAndSet(oldState, newState));
117117
// if we cleared successfully we then need to call unsubscribe on all previous
118-
// current is now "previous"
119-
unsubscribeFromAll(current.subscriptions);
118+
unsubscribeFromAll(oldState.subscriptions);
120119
}
121120

122121
@Override
123122
public void unsubscribe() {
124-
State current;
123+
State oldState;
125124
State newState;
126125
do {
127-
current = state.get();
128-
if (current.isUnsubscribed) {
126+
oldState = state.get();
127+
if (oldState.isUnsubscribed) {
129128
return;
130129
} else {
131-
newState = current.unsubscribe();
130+
newState = oldState.unsubscribe();
132131
}
133-
} while (!state.compareAndSet(current, newState));
134-
// current is now "previous"
135-
unsubscribeFromAll(current.subscriptions);
132+
} while (!state.compareAndSet(oldState, newState));
133+
unsubscribeFromAll(oldState.subscriptions);
136134
}
137135

138136
private static void unsubscribeFromAll(Collection<Subscription> subscriptions) {

‎rxjava-core/src/main/java/rx/subscriptions/RefCountSubscription.java

Lines changed: 22 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,8 @@
2626
*
2727
* @see <a href='http://msdn.microsoft.com/en-us/library/system.reactive.disposables.refcountdisposable.aspx'>MSDN RefCountDisposable</a>
2828
*/
29-
public class RefCountSubscription implements Subscription {
30-
/** The reference to the actual subscription. */
29+
public final class RefCountSubscription implements Subscription {
3130
private final Subscription actual;
32-
/** Counts the number of subscriptions (1 parent + multiple children) */
3331
private final AtomicReference<State> state = new AtomicReference<State>(new State(false, 0));
3432

3533
private static final class State {
@@ -67,20 +65,25 @@ public RefCountSubscription(Subscription s) {
6765
this.actual = s;
6866
}
6967

68+
@Deprecated
69+
public Subscription getSubscription() {
70+
return get();
71+
}
72+
7073
/**
7174
* Returns a new sub-subscription.
7275
*/
73-
public Subscription getSubscription() {
74-
State current;
76+
public Subscription get() {
77+
State oldState;
7578
State newState;
7679
do {
77-
current = state.get();
78-
if (current.isUnsubscribed) {
80+
oldState = state.get();
81+
if (oldState.isUnsubscribed) {
7982
return Subscriptions.empty();
8083
} else {
81-
newState = current.addChild();
84+
newState = oldState.addChild();
8285
}
83-
} while (!state.compareAndSet(current, newState));
86+
} while (!state.compareAndSet(oldState, newState));
8487

8588
return new InnerSubscription();
8689
}
@@ -94,15 +97,15 @@ public boolean isUnsubscribed() {
9497

9598
@Override
9699
public void unsubscribe() {
97-
State current;
100+
State oldState;
98101
State newState;
99102
do {
100-
current = state.get();
101-
if (current.isUnsubscribed) {
103+
oldState = state.get();
104+
if (oldState.isUnsubscribed) {
102105
return;
103106
}
104-
newState = current.unsubscribe();
105-
} while (!state.compareAndSet(current, newState));
107+
newState = oldState.unsubscribe();
108+
} while (!state.compareAndSet(oldState, newState));
106109
unsubscribeActualIfApplicable(newState);
107110
}
108111

@@ -113,18 +116,18 @@ private void unsubscribeActualIfApplicable(State state) {
113116
}
114117

115118
/** The individual sub-subscriptions. */
116-
class InnerSubscription implements Subscription {
119+
private final class InnerSubscription implements Subscription {
117120
final AtomicBoolean innerDone = new AtomicBoolean();
118121

119122
@Override
120123
public void unsubscribe() {
121124
if (innerDone.compareAndSet(false, true)) {
122-
State current;
125+
State oldState;
123126
State newState;
124127
do {
125-
current = state.get();
126-
newState = current.removeChild();
127-
} while (!state.compareAndSet(current, newState));
128+
oldState = state.get();
129+
newState = oldState.removeChild();
130+
} while (!state.compareAndSet(oldState, newState));
128131
unsubscribeActualIfApplicable(newState);
129132
}
130133
}

‎rxjava-core/src/main/java/rx/subscriptions/Subscriptions.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424
/**
2525
* Helper methods and utilities for creating and working with {@link Subscription} objects
2626
*/
27-
public class Subscriptions {
27+
public final class Subscriptions {
2828
/**
2929
* A {@link Subscription} that does nothing.
3030
*

‎rxjava-core/src/test/java/rx/subscriptions/MultipleAssignmentSubscriptionTest.java

Lines changed: 32 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -15,54 +15,66 @@
1515
*/
1616
package rx.subscriptions;
1717

18+
import static org.mockito.Mockito.*;
19+
import static rx.subscriptions.Subscriptions.*;
1820
import junit.framework.Assert;
21+
1922
import org.junit.Before;
2023
import org.junit.Test;
21-
import static org.mockito.Mockito.mock;
22-
import static org.mockito.Mockito.never;
23-
import static org.mockito.Mockito.times;
24-
import static org.mockito.Mockito.verify;
24+
2525
import rx.Subscription;
26-
import static rx.subscriptions.Subscriptions.create;
2726
import rx.util.functions.Action0;
2827

2928
public class MultipleAssignmentSubscriptionTest {
29+
3030
Action0 unsubscribe;
3131
Subscription s;
32+
3233
@Before
3334
public void before() {
34-
unsubscribe = mock(Action0.class);
35-
s = create(unsubscribe);
35+
unsubscribe = mock(Action0.class);
36+
s = create(unsubscribe);
3637
}
38+
3739
@Test
3840
public void testNoUnsubscribeWhenReplaced() {
3941
MultipleAssignmentSubscription mas = new MultipleAssignmentSubscription();
40-
41-
mas.setSubscription(s);
42-
mas.setSubscription(null);
43-
mas.unsubscribe();
44-
42+
43+
mas.set(s);
44+
mas.set(Subscriptions.empty());
45+
mas.unsubscribe();
46+
4547
verify(unsubscribe, never()).call();
46-
4748
}
49+
4850
@Test
4951
public void testUnsubscribeWhenParentUnsubscribes() {
5052
MultipleAssignmentSubscription mas = new MultipleAssignmentSubscription();
51-
mas.setSubscription(s);
53+
mas.set(s);
5254
mas.unsubscribe();
5355
mas.unsubscribe();
54-
56+
5557
verify(unsubscribe, times(1)).call();
56-
58+
5759
Assert.assertEquals(true, mas.isUnsubscribed());
5860
}
61+
5962
@Test
60-
public void testUnsubscribedDoesntLeakSentinel() {
63+
public void subscribingWhenUnsubscribedCausesImmediateUnsubscription() {
6164
MultipleAssignmentSubscription mas = new MultipleAssignmentSubscription();
65+
mas.unsubscribe();
66+
Subscription underlying = mock(Subscription.class);
67+
mas.set(underlying);
68+
verify(underlying).unsubscribe();
69+
}
6270

63-
mas.setSubscription(s);
71+
@Test
72+
public void testSubscriptionRemainsAfterUnsubscribe() {
73+
MultipleAssignmentSubscription mas = new MultipleAssignmentSubscription();
74+
75+
mas.set(s);
6476
mas.unsubscribe();
65-
66-
Assert.assertEquals(true, mas.getSubscription() == Subscriptions.empty());
77+
78+
Assert.assertEquals(true, mas.get() == s);
6779
}
6880
}

‎rxjava-core/src/test/java/rx/subscriptions/RefCountSubscriptionTest.java

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ public void testImmediateUnsubscribe() {
4747
public void testRCSUnsubscribeBeforeClient() {
4848
InOrder inOrder = inOrder(main);
4949

50-
Subscription s = rcs.getSubscription();
50+
Subscription s = rcs.get();
5151

5252
rcs.unsubscribe();
5353

@@ -67,8 +67,8 @@ public void testRCSUnsubscribeBeforeClient() {
6767
public void testMultipleClientsUnsubscribeFirst() {
6868
InOrder inOrder = inOrder(main);
6969

70-
Subscription s1 = rcs.getSubscription();
71-
Subscription s2 = rcs.getSubscription();
70+
Subscription s1 = rcs.get();
71+
Subscription s2 = rcs.get();
7272

7373
s1.unsubscribe();
7474
inOrder.verify(main, never()).call();
@@ -88,8 +88,8 @@ public void testMultipleClientsUnsubscribeFirst() {
8888
public void testMultipleClientsMainUnsubscribeFirst() {
8989
InOrder inOrder = inOrder(main);
9090

91-
Subscription s1 = rcs.getSubscription();
92-
Subscription s2 = rcs.getSubscription();
91+
Subscription s1 = rcs.get();
92+
Subscription s2 = rcs.get();
9393

9494
rcs.unsubscribe();
9595
inOrder.verify(main, never()).call();

0 commit comments

Comments
 (0)