Skip to content

Commit 87e81ec

Browse files
committed
Add example illustrating how more advanced method-to-method dependencies can be checked
Based on an example provided in #235, this example aims to help people who are working on similar use cases in their projects. Signed-off-by: Per Lundberg <[email protected]>
1 parent 3e01795 commit 87e81ec

File tree

5 files changed

+176
-0
lines changed

5 files changed

+176
-0
lines changed
Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
package com.tngtech.archunit.example.singleton;
2+
3+
import java.util.concurrent.ConcurrentHashMap;
4+
5+
/**
6+
* @author Per Lundberg
7+
*/
8+
public class SingletonClass {
9+
10+
// Poor man's reimplementation of Guava's memoization support. If your project supports Guava,
11+
// replace the stuff below with the following:
12+
//
13+
// private static final Supplier<SingletonClass> INSTANCE = Suppliers.memoize( SingletonClass::new );
14+
//
15+
//
16+
// ...and retrieve the (singleton) instance by calling `INSTANCE.get()`
17+
//
18+
private static final ConcurrentHashMap<Integer, SingletonClass> INSTANCE_MAP = new ConcurrentHashMap<>();
19+
private static final int INSTANCE_KEY = 1;
20+
21+
22+
private SingletonClass() {
23+
// Private constructor to prevent construction
24+
}
25+
26+
public void doSomething() {
27+
throw new UnsupportedOperationException();
28+
}
29+
30+
public static SingletonClass getInstance() {
31+
return INSTANCE_MAP.computeIfAbsent( INSTANCE_KEY, unused -> new SingletonClass() );
32+
}
33+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
package com.tngtech.archunit.example.singleton;
2+
3+
/**
4+
* An example of how {@link SingletonClass} is used incorrectly. This is expected to be detected by
5+
* the corresponding ArchUnit test.
6+
*
7+
* @author Per Lundberg
8+
*/
9+
public class SingletonClassInvalidConsumer {
10+
11+
void doSomething() {
12+
// This pattern (of calling getInstance() in the midst of a method) is both unadvisable and
13+
// dangerous for the following reasons:
14+
//
15+
// - It makes it impossible to override the dependency for tests, which can lead to
16+
// unnecessarily excessive object mocking.
17+
//
18+
// - It postpones object initialization to an unnecessarily late stage (runtime instead of
19+
// startup-time). Problems with classes that cannot be instantiated risks being "hidden"
20+
// until runtime, defeating the purpose of the "fail fast" philosophy.
21+
SingletonClass.getInstance().doSomething();
22+
}
23+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
package com.tngtech.archunit.example.singleton;
2+
3+
/**
4+
* An example of how {@link SingletonClass} is used correctly
5+
*
6+
* @author Per Lundberg
7+
*/
8+
public class SingletonClassValidConsumer {
9+
10+
private static SingletonClassValidConsumer instance;
11+
12+
private final SingletonClass singletonClass;
13+
14+
public SingletonClassValidConsumer( SingletonClass singletonClass ) {
15+
this.singletonClass = singletonClass;
16+
}
17+
18+
public static SingletonClassValidConsumer getInstance() {
19+
// Valid way to call getInstance() on another class:
20+
//
21+
// - We retrieve the instance for the dependency in our own getInstance() method. It is
22+
// passed to our constructor as a constructor parameter, making it easy to override it for
23+
// tests.
24+
if ( instance == null ) {
25+
instance = new SingletonClassValidConsumer( SingletonClass.getInstance() );
26+
}
27+
28+
return instance;
29+
}
30+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
package com.tngtech.archunit.example.singleton;
2+
3+
/**
4+
* An example of how {@link SingletonClass} is used incorrectly, but explicitly whitelisted.
5+
*
6+
* @author Per Lundberg
7+
*/
8+
public class SingletonClassWhitelistedInvalidConsumer {
9+
10+
void doSomething() {
11+
// This pattern (of calling getInstance() in the midst of a method) is both unadvisable and
12+
// dangerous for reasons described in SingleTonClassInvalidConsumer.
13+
SingletonClass.getInstance().doSomething();
14+
}
15+
}
Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
package com.tngtech.archunit.exampletest;
2+
3+
import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.methods;
4+
5+
import java.util.Arrays;
6+
import java.util.Objects;
7+
import java.util.Optional;
8+
9+
import org.junit.Test;
10+
import org.junit.experimental.categories.Category;
11+
12+
import com.tngtech.archunit.core.domain.JavaClasses;
13+
import com.tngtech.archunit.core.domain.JavaMethod;
14+
import com.tngtech.archunit.core.domain.JavaModifier;
15+
import com.tngtech.archunit.core.importer.ClassFileImporter;
16+
import com.tngtech.archunit.example.singleton.SingletonClass;
17+
import com.tngtech.archunit.lang.ArchCondition;
18+
import com.tngtech.archunit.lang.ConditionEvents;
19+
import com.tngtech.archunit.lang.SimpleConditionEvent;
20+
21+
/**
22+
* @author Per Lundberg
23+
*/
24+
@Category(Example.class)
25+
public class SingletonTest {
26+
private static final JavaClasses classes = new ClassFileImporter().importPackagesOf( SingletonClass.class);
27+
28+
@Test
29+
public void getInstance_is_not_used_from_inside_methods() {
30+
methods()
31+
.that().haveName( "getInstance" )
32+
.and().areStatic()
33+
34+
// Note: this is a convoluted way to say "no parameters".
35+
.and().haveRawParameterTypes( new String[0] )
36+
37+
.should( onlyBeCalledFromWhitelistedOrigins(
38+
// The class below will trigger a violation unless present as a parameter here.
39+
"com.tngtech.archunit.example.singleton.SingletonClassWhitelistedInvalidConsumer"
40+
) )
41+
.because( "" +
42+
"getInstance() calls should not be spread out through the methods of a class. This makes it hard/impossible " +
43+
"to override the dependencies for tests, and also means the dependencies are much harder to identify when " +
44+
"quickly looking at the code. Instead, move all getInstance() calls to the INSTANCE supplier and pass the " +
45+
"dependency to the constructor that way. If this is impossible for a particular case, add the class name to " +
46+
"the whitelist in " + getClass().getName() )
47+
.check( classes );
48+
}
49+
50+
private ArchCondition<JavaMethod> onlyBeCalledFromWhitelistedOrigins( String... whitelistedOrigins ) {
51+
return new ArchCondition<JavaMethod>( "only be called by whitelisted methods" ) {
52+
@Override
53+
public void check( JavaMethod method, ConditionEvents events ) {
54+
method.getCallsOfSelf().stream()
55+
// TODO: Add your own exceptions as needed here, if you have particular
56+
// TODO: use cases where getInstance() calls are permissible.
57+
// Static getInstance() methods are always allowed to call getInstance. This
58+
// does not break dependency injection and does not come with significant
59+
// design flaws.
60+
.filter( call -> !( Objects.equals( call.getOrigin().getName(), "getInstance" ) && call.getOrigin().getModifiers().contains( JavaModifier.STATIC ) ) )
61+
62+
// Anything not handled by the exceptions above must be explicitly listed in
63+
// the whitelistedOrigins parameter.
64+
.filter( call -> {
65+
Optional<String> result = Arrays.stream( whitelistedOrigins )
66+
.filter( o -> call.getOrigin().getFullName().startsWith( o ) )
67+
.findFirst();
68+
69+
return !result.isPresent();
70+
} )
71+
.forEach( call -> events.add( SimpleConditionEvent.violated( method, call.getDescription() ) ) );
72+
}
73+
};
74+
}
75+
}

0 commit comments

Comments
 (0)