Review: Use delegation instead of inheritance

... as it allows for better encapsulation and a cleaner API Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
TNG · Feb 5, 2023 · 8514897 · 8514897
1 parent 014c442
commit 8514897
Show file tree

Hide file tree

Showing 4 changed files with 67 additions and 38 deletions.
diff --git a/archunit/src/main/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStore.java b/archunit/src/main/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStore.java
@@ -25,18 +25,17 @@
 import java.util.UUID;
 import java.util.regex.Pattern;
 
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
 import com.google.common.base.Joiner;
 import com.google.common.base.Splitter;
 import com.tngtech.archunit.PublicAPI;
 import com.tngtech.archunit.lang.ArchRule;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkState;
 import static com.google.common.io.Files.toByteArray;
-import static com.tngtech.archunit.PublicAPI.Usage.INHERITANCE;
+import static com.tngtech.archunit.PublicAPI.Usage.ACCESS;
 import static com.tngtech.archunit.library.freeze.FreezingArchRule.ensureUnixLineBreaks;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static java.util.stream.Collectors.toList;
@@ -49,8 +48,8 @@
  *     simply uses a UUID. However, this can be overridden by passing in an instance of {@link TextFileBasedViolationStore.FileNameStrategy} to the appropriate constructor.
  * </p>
  */
-@PublicAPI(usage = INHERITANCE)
-public class TextFileBasedViolationStore implements ViolationStore {
+@PublicAPI(usage = ACCESS)
+public final class TextFileBasedViolationStore implements ViolationStore {
     private static final Logger log = LoggerFactory.getLogger(TextFileBasedViolationStore.class);
 
     private static final Pattern UNESCAPED_LINE_BREAK_PATTERN = Pattern.compile("(?<!\\\\)\n");
@@ -103,7 +102,7 @@ public TextFileBasedViolationStore(FileNameStrategy fileNameStrategy) {
     }
 
     @Override
-    public final void initialize(Properties properties) {
+    public void initialize(Properties properties) {
         storeCreationAllowed = Boolean.parseBoolean(properties.getProperty(ALLOW_STORE_CREATION_PROPERTY_NAME, ALLOW_STORE_CREATION_DEFAULT));
         storeUpdateAllowed = Boolean.parseBoolean(properties.getProperty(ALLOW_STORE_UPDATE_PROPERTY_NAME, ALLOW_STORE_UPDATE_DEFAULT));
         String path = properties.getProperty(STORE_PATH_PROPERTY_NAME, STORE_PATH_DEFAULT);
@@ -136,12 +135,12 @@ private void checkInitialization(boolean initializationSuccessful, String messag
     }
 
     @Override
-    public final boolean contains(ArchRule rule) {
+    public boolean contains(ArchRule rule) {
         return storedRules.containsKey(rule.getDescription());
     }
 
     @Override
-    public final void save(ArchRule rule, List<String> violations) {
+    public void save(ArchRule rule, List<String> violations) {
         log.debug("Storing evaluated rule '{}' with {} violations: {}", rule.getDescription(), violations.size(), violations);
         if (!storeUpdateAllowed) {
             throw new StoreUpdateFailedException(String.format(

diff --git a/archunit/src/main/java/com/tngtech/archunit/library/freeze/ViolationStore.java b/archunit/src/main/java/com/tngtech/archunit/library/freeze/ViolationStore.java
@@ -72,4 +72,33 @@ public interface ViolationStore {
      * @return The lines of violations currently stored for the passed {@link ArchRule}
      */
     List<String> getViolations(ArchRule rule);
+
+    @PublicAPI(usage = INHERITANCE)
+    class Delegate implements ViolationStore {
+        private final ViolationStore delegate;
+
+        public Delegate(ViolationStore delegate) {
+            this.delegate = delegate;
+        }
+
+        @Override
+        public void initialize(Properties properties) {
+            delegate.initialize(properties);
+        }
+
+        @Override
+        public boolean contains(ArchRule rule) {
+            return delegate.contains(rule);
+        }
+
+        @Override
+        public void save(ArchRule rule, List<String> violations) {
+            delegate.save(rule, violations);
+        }
+
+        @Override
+        public List<String> getViolations(ArchRule rule) {
+            return delegate.getViolations(rule);
+        }
+    }
 }
diff --git a/archunit/src/test/java/com/tngtech/archunit/library/freeze/FreezingArchRuleTest.java b/archunit/src/test/java/com/tngtech/archunit/library/freeze/FreezingArchRuleTest.java
@@ -51,6 +51,7 @@
 @RunWith(DataProviderRunner.class)
 public class FreezingArchRuleTest {
 
+    private static final String STORE_PROPERTY_NAME = "freeze.store";
     private static final String STORE_DEFAULT_PATH_PROPERTY_NAME = "freeze.store.default.path";
     private static final String ALLOW_STORE_CREATION_PROPERTY_NAME = "freeze.store.default.allowStoreCreation";
     private static final String ALLOW_STORE_UPDATE_PROPERTY_NAME = "freeze.store.default.allowStoreUpdate";
@@ -311,7 +312,7 @@ public void works_with_multi_line_violations_with_different_line_separators(Stri
 
     @Test
     public void allows_to_customize_ViolationStore_by_configuration() {
-        ArchConfiguration.get().setProperty("freeze.store", TestViolationStore.class.getName());
+        ArchConfiguration.get().setProperty(STORE_PROPERTY_NAME, TestViolationStore.class.getName());
         ArchConfiguration.get().setProperty("freeze.store.first.property", "first value");
         ArchConfiguration.get().setProperty("freeze.store.second.property", "second value");
         ArchConfiguration.get().setProperty("freeze.unrelated", "unrelated value");
@@ -481,6 +482,21 @@ public void can_prevent_default_ViolationStore_from_updating_existing_rules() th
         expectStoreUpdateDisabledException(() -> frozenRule.check(importClasses(getClass())));
     }
 
+    @Test
+    public void allows_to_adjust_default_store_file_names_via_delegation() throws IOException {
+        // GIVEN
+        File storeFolder = useTemporaryDefaultStorePath();
+        ArchConfiguration.get().setProperty(STORE_PROPERTY_NAME, MyTextFileBasedViolationStore.class.getName());
+        ArchConfiguration.get().setProperty(ALLOW_STORE_CREATION_PROPERTY_NAME, "true");
+
+        // WHEN
+        ArchRule rule = rule("some rule").withViolations("irrelevant").create();
+        freeze(rule).check(importClasses(getClass()));
+
+        // THEN
+        assertThat(storeFolder.list()).contains("some rule" + MyTextFileBasedViolationStore.FILE_NAME_SUFFIX);
+    }
+
     @Test
     public void violations_ignored_by_archunit_ignore_patterns_are_omitted_from_the_store() throws IOException {
         Path ignoreFile = null;
@@ -516,9 +532,10 @@ private void createFrozen(TestViolationStore violationStore, ArchRule rule) {
                 .hasNoViolation();
     }
 
-    private void useTemporaryDefaultStorePath() throws IOException {
+    private File useTemporaryDefaultStorePath() throws IOException {
         File folder = temporaryFolder.newFolder();
         ArchConfiguration.get().setProperty(STORE_DEFAULT_PATH_PROPERTY_NAME, folder.getAbsolutePath());
+        return folder;
     }
 
     private static RuleCreator rule(String description) {
@@ -629,6 +646,14 @@ private StoredRule(List<String> violations) {
         }
     }
 
+    static class MyTextFileBasedViolationStore extends ViolationStore.Delegate {
+        static final String FILE_NAME_SUFFIX = " test";
+
+        MyTextFileBasedViolationStore() {
+            super(new TextFileBasedViolationStore(description -> description + FILE_NAME_SUFFIX));
+        }
+    }
+
     private static class ConsiderAllLinesWithTheSameStartLetterTheSame implements ViolationLineMatcher {
         @Override
         public boolean matches(String lineFromFirstViolation, String lineFromSecondViolation) {

diff --git a/...it/src/test/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStoreTest.java b/...it/src/test/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStoreTest.java
@@ -7,15 +7,14 @@
 import java.util.List;
 import java.util.Properties;
 
+import com.google.common.collect.ImmutableList;
+import com.google.common.io.Files;
+import com.tngtech.archunit.lang.ArchRule;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
 
-import com.google.common.collect.ImmutableList;
-import com.google.common.io.Files;
-import com.tngtech.archunit.lang.ArchRule;
-
 import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.classes;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static java.util.Arrays.asList;
@@ -119,29 +118,6 @@ public void stores_violations_with_line_breaks() {
         assertThat(violations).as("stored violations").containsExactlyElementsOf(expected);
     }
 
-    @Test
-    public void class_can_be_overridden_and_rule_file_name_strategy_supplied() {
-
-        // given
-        class MySubclassOfTextFileBasedViolationStore extends TextFileBasedViolationStore {
-            public MySubclassOfTextFileBasedViolationStore() {
-                super(s -> s + " xxx");
-            }
-        };
-        TextFileBasedViolationStore store = new MySubclassOfTextFileBasedViolationStore();
-        store.initialize(propertiesOf(
-                "default.path", configuredFolder.getAbsolutePath(),
-                "default.allowStoreCreation", String.valueOf(true)));
-
-        // when
-        ArchRule firstRule = rule("first rule");
-        store.save(firstRule, ImmutableList.of("first violation1", "first violation2"));
-
-        // then
-        String fileName =  configuredFolder.listFiles()[0].getName();
-        assertThat(fileName).isEqualTo("first rule xxx");
-    }
-
     private Properties readProperties(File file) throws IOException {
         Properties properties = new Properties();
         try (FileInputStream inputStream = new FileInputStream(file)) {