Merge pull request #131 from Cognifide/issue-116

Issue 116 Name and Reuse Repeating Conditions

README.md

@@ -90,6 +90,9 @@ Below you will find descriptions of all rules available in **AEM Rules for Sonar
 - **HTL-2** HTL Templates should be placed in separate files.
   - HTL Templates should be placed in separate files. This helps to understand which code is meant to render a component and which code is re-used as a template.
 
+- **HTL-4** Name and re-use Repeating Conditions
+  - Consider caching data-sly-test conditions and reduce code duplication.
+
 ## Possible bugs
 
 - **AEM-3** Non-thread safe object used as a field of Servlet / Filter etc.

src/main/java/com/cognifide/aemrules/htl/checks/NamingAndReusingConditionsCheck.java

@@ -0,0 +1,112 @@
+/*-
+ * #%L
+ * AEM Rules for SonarQube
+ * %%
+ * Copyright (C) 2015-2018 Cognifide Limited
+ * %%
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ * #L%
+ */
+package com.cognifide.aemrules.htl.checks;
+
+import com.cognifide.aemrules.htl.api.ParsingErrorRule;
+import com.cognifide.aemrules.metadata.Metadata;
+import com.cognifide.aemrules.tag.Tags;
+import com.cognifide.aemrules.version.AemVersion;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.sling.scripting.sightly.compiler.expression.Expression;
+import org.sonar.check.Priority;
+import org.sonar.check.Rule;
+import org.sonar.plugins.html.node.Attribute;
+import org.sonar.plugins.html.node.TagNode;
+
+@Rule(
+    key = NamingAndReusingConditionsCheck.RULE_KEY,
+    name = NamingAndReusingConditionsCheck.RULE_MESSAGE,
+    priority = Priority.INFO,
+    tags = Tags.AEM
+)
+@AemVersion(
+    all = true
+)
+@Metadata(
+    technicalDebt = "10min"
+)
+@ParsingErrorRule
+public class NamingAndReusingConditionsCheck extends AbstractHtlCheck {
+
+    static final String RULE_KEY = "HTL-4";
+
+    static final String RULE_MESSAGE = "Consider caching data-sly-test conditions";
+
+    private static final String SLY_TEST = "data-sly-test";
+
+    private static final int SLY_TEST_LENGTH = 14;
+
+    private Set<String> unnamedConditions = new HashSet<>();
+
+    private Set<String> namedConditions = new HashSet<>();
+
+    @Override
+    public void startHtlElement(List<Expression> expressions, TagNode node) {
+        if (!isConditionReusedCorrectly(expressions, node)) {
+            createViolation(node.getStartLinePosition(), RULE_MESSAGE);
+        }
+        updateConditionSets(expressions, node);
+    }
+
+    private boolean isConditionReusedCorrectly(List<Expression> expressions, TagNode node) {
+        String condition = clearExpressions(expressions).stream()
+            .findFirst()
+            .orElse("");
+
+        return !(isUnnamedConditionReused(condition) && isNewUnnamedConditionDeclared(node));
+    }
+
+    private boolean isNewUnnamedConditionDeclared(TagNode node) {
+        return node.getAttributes().stream()
+            .map(Attribute::getName)
+            .anyMatch(SLY_TEST::equals);
+    }
+
+    private boolean isUnnamedConditionReused(String condition) {
+        return unnamedConditions.stream()
+            .anyMatch(condition::equals);
+    }
+
+    private void updateConditionSets(List<Expression> expressions, TagNode node) {
+        String condition = node.getAttributes().stream()
+            .map(Attribute::getName)
+            .filter(text -> text.contains(SLY_TEST))
+            .findFirst()
+            .orElse("");
+        if (!SLY_TEST.equals(condition) && SLY_TEST_LENGTH < condition.length()) {
+            condition = condition.substring(SLY_TEST_LENGTH);
+            namedConditions.add(condition);
+        } else {
+            unnamedConditions.addAll(clearExpressions(expressions).stream()
+                .filter(text -> !namedConditions.contains(text))
+                .collect(Collectors.toSet()));
+        }
+    }
+
+    private Set<String> clearExpressions(List<Expression> expressions){
+        return expressions.stream()
+            .map(Expression::getRawText)
+            .map(text -> text.replaceAll("[${}]", ""))
+            .collect(Collectors.toSet());
+    }
+}

src/main/java/com/cognifide/aemrules/htl/rules/HtlCheckClasses.java

@@ -19,10 +19,11 @@
  */
 package com.cognifide.aemrules.htl.rules;
 
-import com.cognifide.aemrules.htl.checks.HtlAttributesShouldBeAtTheEndCheck;
-import com.cognifide.aemrules.htl.checks.ParsingErrorCheck;
 import com.cognifide.aemrules.htl.Htl;
 import com.cognifide.aemrules.htl.api.HtlCheck;
+import com.cognifide.aemrules.htl.checks.HtlAttributesShouldBeAtTheEndCheck;
+import com.cognifide.aemrules.htl.checks.NamingAndReusingConditionsCheck;
+import com.cognifide.aemrules.htl.checks.ParsingErrorCheck;
 import com.cognifide.aemrules.htl.checks.PlaceTemplatesInSeparateFilesCheck;
 import com.google.common.collect.ImmutableList;
 import java.util.List;
@@ -37,8 +38,9 @@ public final class HtlCheckClasses {
 
     private static final List<Class<? extends HtlCheck>> CLASSES = ImmutableList.of(
         ParsingErrorCheck.class,
+        PlaceTemplatesInSeparateFilesCheck.class,
         HtlAttributesShouldBeAtTheEndCheck.class,
-        PlaceTemplatesInSeparateFilesCheck.class
+        NamingAndReusingConditionsCheck.class
     );
 
     private HtlCheckClasses() {

src/main/resources/rules/HTL-4.md

@@ -0,0 +1,24 @@
+Always try to re-use existing conditions, so the code is more readable.
+
+== Noncompliant Code Example
+```
+<!--/* Bad - the same condition is evaluated multiple times */-->
+<span class="uber-mode__top-bar" data-sly-test="${uberModeHelper.uberModeEnabled || forceUberMode}">
+    <div class="my-component">
+        Some text
+    </div>
+</span>
+<span class="uber-mode__bottom-bar" data-sly-test="${uberModeHelper.uberModeEnabled || forceUberMode}"></span>
+```
+
+
+
+== Compliant Solution
+```
+<span class="uber-mode__top-bar" data-sly-test.uberMode="${uberModeHelper.uberModeEnabled || forceUberMode}">
+    <div class="my-component">
+        Some text
+    </div>
+</span>
+<span class="uber-mode__bottom-bar" data-sly-test="${uberMode}"></span>
+```

src/test/files/checks/htl/HtlAttributesShouldBeAtTheEndCheck.html

@@ -1,22 +1 @@
-<!--
-
-    #%L
-    AEM Rules for SonarQube
-    %%
-    Copyright (C) 2015-2018 Cognifide Limited
-    %%
-    Licensed under the Apache License, Version 2.0 (the "License");
-    you may not use this file except in compliance with the License.
-    You may obtain a copy of the License at
-
-         http://www.apache.org/licenses/LICENSE-2.0
-
-    Unless required by applicable law or agreed to in writing, software
-    distributed under the License is distributed on an "AS IS" BASIS,
-    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-    See the License for the specific language governing permissions and
-    limitations under the License.
-    #L%
-
--->
 <div data-sly-list="${model.value}" class="list"></div> <!--/* Non-Compliant */-->

src/test/files/checks/htl/NamingAndReusingConditionsCheck.html

@@ -0,0 +1,22 @@
+<span class="uber-mode__top-bar" data-sly-test="${uberModeHelper.uberModeEnabled || forceUberMode}">
+    <div class="my-component">
+        Text
+    </div>
+</span>
+
+<span class="uber-mode__bottom-bar" data-sly-test="${uberModeHelper.uberModeEnabled || forceUberMode}"></span> <!--/* Non-Compliant */-->
+
+<span class="uber-mode__top-bar" data-sly-test.uberMode="${uberModeHelper.uberModeEnabled || forceUberMode}">
+    <div class="my-component">
+        Text
+    </div>
+</span>
+<span class="uber-mode__top-bar" data-sly-test="${uberMode}"></span>
+
+<span class="uber-mode__bottom-bar" data-sly-test="${uberModeHelper.uberModeEnabled || forceUberMode}"> <!--/* Non-Compliant */-->
+    <div class="my-component">
+        Text
+    </div>
+</span>
+
+<span class="uber-mode__bottom-bar" data-sly-test="${uberMode}"></span>

src/test/java/com/cognifide/aemrules/htl/HtlProfileTest.java

@@ -20,6 +20,7 @@
 package com.cognifide.aemrules.htl;
 
 import static org.fest.assertions.Assertions.assertThat;
+
 import java.util.Collection;
 import java.util.Map;
 import java.util.Map.Entry;
@@ -42,6 +43,16 @@ public class HtlProfileTest {
     public void setUp() {
         profile = new HtlProfile();
         this.context = new BuiltInQualityProfilesDefinition.Context();
+
+    }
+
+    @Test
+    public void sanity() {
+        profile.define(context);
+        Map<String, BuiltInActiveRule> activeRules = getActiveRulesByRuleKey(context);
+
+        assertThat(activeRules.size()).isEqualTo(4);
+        assertThat(activeRules.keySet()).contains("HTL-0");
     }
 
     @Test

src/test/java/com/cognifide/aemrules/htl/checks/NamingAndReusingConditionsCheckTest.java

@@ -0,0 +1,33 @@
+/*-
+ * #%L
+ * AEM Rules for SonarQube
+ * %%
+ * Copyright (C) 2015-2018 Cognifide Limited
+ * %%
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ * #L%
+ */
+package com.cognifide.aemrules.htl.checks;
+
+import com.cognifide.aemrules.htl.AbstractBaseTest;
+import org.junit.Test;
+
+public class NamingAndReusingConditionsCheckTest extends AbstractBaseTest {
+
+    @Test
+    public void checkHtlAttributesOrder() {
+        check = new NamingAndReusingConditionsCheck();
+        filename = "src/test/files/checks/htl/NamingAndReusingConditionsCheck.html";
+        verify();
+    }
+}