diff --git a/README.md b/README.md index ebbdd144..d461e54f 100644 --- a/README.md +++ b/README.md @@ -85,6 +85,7 @@ A solution proposal will be published here every day during the `Advent Of Craft - [Day 15: Put a code under tests.](solution/day15/docs/step-by-step.md) - [Day 16: Make this code immutable.](solution/day16/docs/step-by-step.md) - [Day 17: Design one test that has the impact of thousands.](solution/day17/docs/step-by-step.md) +- [Day 18: Automatically detect Linguistic Anti-Patterns (LAP).](solution/day18/docs/step-by-step.md) ## Contributors diff --git a/solution/day18/docs/challenge-done.md b/solution/day18/docs/challenge-done.md new file mode 100644 index 00000000..406d8a59 --- /dev/null +++ b/solution/day18/docs/challenge-done.md @@ -0,0 +1,69 @@ +## Day 18: Automatically detect Linguistic Anti-Patterns (LAP). + +`What's the problem with this code?` (except the name's class 😁) + +This kind of issues in the code are [`Linguistic Anti Patterns`](https://www.veneraarnaoudova.ca/linguistic-anti-pattern-detector-lapd/las/) as named by [Venera Arnaoudova](https://www.veneraarnaoudova.ca/). + +> How could we simply detect these issues? + +### Architecture Unit Tests +Take a look at [this page](https://xtrem-tdd.netlify.app/Flavours/archunit) describing this concept. + +- We can use the library [`archunit`](https://www.archunit.org/) in java to describe and enforce architecture / design rules. + +- We can now create a test class to materialize this kind of guideline + - We use the `@AnalyzeClasses` annotation and configure which packages need to be analyzed + +```java +@AnalyzeClasses(packages = "lap", importOptions = ImportOption.DoNotIncludeTests.class) +public class TeamRules { +} +``` + +- Even if we are not expert in this library, its discoverability makes it really easy to use and learn + - Read more about [`Dot Driven Development`](https://blog.ploeh.dk/2012/05/25/Designpatternsacrossparadigms/#ebe4a8c5ba664c6fb5ea07c8b7e18555) + +#### No getter can return `void` +- We write a skeleton of this rule +- We generate the `notBeVoid` method +- Once done, we run the tests + - The anti-pattern is detected + +- We can check that it works correctly by fixing the code and run the tests again + +```java +public int getData() { + return 42; +} +``` + +- The test is green, our LA detection is working like a charm + +#### Iser / haser must return a `boolean` +- We work on `is` detection now + +#### Let's detect bad names +- Based on Simon Butler's work, we can try to detect bad naming in our code + - The ones which increase drastically our cognitive load while reading code... + +- We can write 2 rules to detect `underscore anomalies` +- We refactor our `rules` for centralizing the logic on the field + - We extract a `method` then the `parameters` + - Our IDE is smart and make the replacement in the other rule for us 🤩 + +#### Our Team Rules +Here are the `rules` we end up with: +- no_getter_can_return_void +- iser_haser_must_return_boolean +- detect_consecutives_underscores +- detect_external_underscores + +>**Tip of the day: Proactively testing your architecture will avoid design problems.** + +### Share your experience + +How this technique could be useful for you? + +What rules would you need? + +Please let everyone know in the discord. diff --git a/solution/day18/docs/img/a2.png b/solution/day18/docs/img/a2.png new file mode 100644 index 00000000..fd22b246 Binary files /dev/null and b/solution/day18/docs/img/a2.png differ diff --git a/solution/day18/docs/img/dot-driven-development.png b/solution/day18/docs/img/dot-driven-development.png new file mode 100644 index 00000000..1ce56391 Binary files /dev/null and b/solution/day18/docs/img/dot-driven-development.png differ diff --git a/solution/day18/docs/img/extract-parameter.png b/solution/day18/docs/img/extract-parameter.png new file mode 100644 index 00000000..244fbc11 Binary files /dev/null and b/solution/day18/docs/img/extract-parameter.png differ diff --git a/solution/day18/docs/img/getter-void.png b/solution/day18/docs/img/getter-void.png new file mode 100644 index 00000000..b3ce9015 Binary files /dev/null and b/solution/day18/docs/img/getter-void.png differ diff --git a/solution/day18/docs/img/lap-archunit.png b/solution/day18/docs/img/lap-archunit.png new file mode 100644 index 00000000..f838b94c Binary files /dev/null and b/solution/day18/docs/img/lap-archunit.png differ diff --git a/solution/day18/docs/img/lap.png b/solution/day18/docs/img/lap.png new file mode 100644 index 00000000..b4c93d71 Binary files /dev/null and b/solution/day18/docs/img/lap.png differ diff --git a/solution/day18/docs/snippet.png b/solution/day18/docs/snippet.png new file mode 100644 index 00000000..502d2c80 Binary files /dev/null and b/solution/day18/docs/snippet.png differ diff --git a/solution/day18/docs/step-by-step.md b/solution/day18/docs/step-by-step.md new file mode 100644 index 00000000..1039460f --- /dev/null +++ b/solution/day18/docs/step-by-step.md @@ -0,0 +1,234 @@ +## Day 18: Automatically detect Linguistic Anti-Patterns (LAP). + +`What's the problem with this code?` (except the name's class 😁) + +```java +public class ShittyClass { + // getter that returns void... + public void getData() { + } + + // an is method returning an integer... + private int isTrue() { + return 42; + } +} +``` + +This kind of issues in the code are [`Linguistic Anti Patterns`](https://www.veneraarnaoudova.ca/linguistic-anti-pattern-detector-lapd/las/) as named by [Venera Arnaoudova](https://www.veneraarnaoudova.ca/). + +![A2](img/a2.png) + +> How could we simply detect these issues? + +### Architecture Unit Tests +Take a look at [this page](https://xtrem-tdd.netlify.app/Flavours/archunit) describing this concept. + +- We can use the library [`archunit`](https://www.archunit.org/) in java to describe and enforce architecture / design rules. + +```xml + + 1.2.0 + + + + + com.tngtech.archunit + archunit-junit5 + ${archunit-junit5.version} + test + + +``` + +- We can now create a test class to materialize this kind of guideline + - We use the `@AnalyzeClasses` annotation and configure which packages need to be analyzed + +```java +@AnalyzeClasses(packages = "lap", importOptions = ImportOption.DoNotIncludeTests.class) +public class TeamRules { +} +``` + +- Even if we are not expert in this library, its discoverability makes it really easy to use and learn + - Read more about [`Dot Driven Development`](https://blog.ploeh.dk/2012/05/25/Designpatternsacrossparadigms/#ebe4a8c5ba664c6fb5ea07c8b7e18555) + +![Dot Driven Development](img/dot-driven-development.png) + +#### No getter can return `void` +- We write a skeleton of this rule + +```java +@ArchTest +public static final ArchRule no_getter_can_return_void = + ArchRuleDefinition + // Work on the methods + .methods().that() + // Specify a regex to select the ones we are interested in + .haveNameMatching("get.*") + // Set our expectation + .should(notBeVoid()) + .because("any method which gets something should actually return something"); +``` + +- We generate the `notBeVoid` method + - Then adapt it like this: + +```java +public static ArchCondition notBeVoid() { + return new ArchCondition<>("not return void") { + @Override + public void check(JavaMethod method, ConditionEvents events) { + final var matches = !"void".equals(method.getRawReturnType().getName()); + final var message = method.getFullName() + " returns " + method.getRawReturnType().getName(); + events.add(new SimpleConditionEvent(method, matches, message)); + } + }; +} +``` + +- Once done, we run the tests + - The anti-pattern is detected + +![Void get detection](img/getter-void.png) + +- We can check that it works correctly by fixing the code and run the tests again + +```java +public int getData() { + return 42; +} +``` + +- The test is green, our LA detection is working like a charm + +#### Iser / haser must return a `boolean` +- We work on `is` detection now + +```java +@ArchTest +private static final ArchRule iser_haser_must_return_boolean = + methods().that().haveNameMatching("is[A-Z].*") + .or().haveNameMatching("has[A-Z].*") + .should().haveRawReturnType(Boolean.class) + .orShould().haveRawReturnType(boolean.class) + .because("any method which fetch a state should actually return something (a boolean)"); +``` + +#### Let's detect bad names +- Based on Simon Butler's work, we can try to detect bad naming in our code + - The ones which increase drastically our cognitive load while reading code... + +| Name | Description | Example of a bad name | +| ------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------- | +| Capitalization Anomaly | Identifiers should use proper capitalization. | page counter | +| Consecutive underscores | Identifiers should not contain multiple consecutive underscores. | page__counter | +| Dictionary words | Identifiers should consist of words, and only use abbreviations when they are more commonly used than the full words. | pag_countr | +| Number of Words | Identifiers should be composed of between two and four words. | page_counter_
converted_and_
normalized_value | +| Excessive words | Identifiers should not be composed out of more than four words. | page_counter_
converted_and_
normalized_value | +| Short Identifier Name | Identifiers should not consist of fewer than eight characters, except for c, d, e, g, i, in, inOut, j, k, m, n, o, out, t, x, y, z. | P, page | +| Enumeration Identifier Declaration Order | Unless there are clear reasons to do so, enumeration types should be declared in alphabetical order. | CardValue = {ACE, EIGHT, FIVE, FOUR, JACK, KING...} | +| External Underscores | Identifiers should not begin or end in an underscore. | __page_counter_ | +| Identifier Encoding | Type information should not be encoded in identifier names using Hungarian notation or similar. | int_page_counter | +| Long Identifier Name | Long identifier names should be avoided where possible. | page_counter_
converted_and_
normalized_value | +| Naming Convention Anomaly | Identifiers should not combine uppercase and lowercase characters in non-standard ways. | Page_counter | +| Numeric Identifier Name | Identifiers should not be composed entirely of numeric words or numbers. | FIFTY | + +> More explanations regarding those concepts [here](https://www.researchgate.net/scientific-contributions/Simon-Butler-2163532599) + +- We write 2 rules to detect `underscore anomalies` + +```java +@ArchTest +private static final ArchRule detect_consecutives_underscores = + fields().that() + .haveNameMatching(".*__.*") + .should(notExist("not contain consecutive underscores...")) + .because("it ruins readability"); + +@ArchTest +private static final ArchRule detect_external_underscores = + fields().that() + .haveNameMatching("_.*|\\w+._") + .should(notExist("not contain external underscores...")) + .because("it ruins readability"); + +private static ArchCondition notExist(String reason) { + return new ArchCondition<>(reason) { + @Override + public void check(JavaField field, ConditionEvents events) { + final var message = field.getName() + " is not a valid name"; + events.add(new SimpleConditionEvent(field, false, message)); + } + }; +} +``` + +- We refactor our `rules` for centralizing the logic on the field + - We extract a `method` then the `parameters` + - Our IDE is smart and make the replacement in the other rule for us 🤩 + +![extract parameter](img/extract-parameter.png) + +#### Our Team Rules +Here are the `rules` we end up with: + +```java +@AnalyzeClasses(packages = "lap", importOptions = ImportOption.DoNotIncludeTests.class) +public class TeamRules { + @ArchTest + private static final ArchRule no_getter_can_return_void = + methods().that() + .haveNameMatching("get.*") + .should(notBeVoid()) + .because("any method which gets something should actually return something"); + + @ArchTest + private static final ArchRule iser_haser_must_return_boolean = + methods().that() + .haveNameMatching("is[A-Z].*").or() + .haveNameMatching("has[A-Z].*").should() + .haveRawReturnType(Boolean.class).orShould() + .haveRawReturnType(boolean.class) + .because("any method which fetch a state should actually return something (a boolean)"); + + public static ArchCondition notBeVoid() { + return new ArchCondition<>("not return void") { + @Override + public void check(JavaMethod method, ConditionEvents events) { + final var matches = !"void".equals(method.getRawReturnType().getName()); + final var message = method.getFullName() + " returns " + method.getRawReturnType().getName(); + events.add(new SimpleConditionEvent(method, matches, message)); + } + }; + } + + @ArchTest + private static final ArchRule detect_consecutives_underscores = + nameAnomaly(".*__.*", "consecutive underscores"); + + @ArchTest + private static final ArchRule detect_external_underscores = + nameAnomaly("_.*|\\w+._", "external underscores"); + + private static ArchRule nameAnomaly(String regex, String reason) { + return fields().that() + .haveNameMatching(regex) + .should(notExist("not contain " + reason + "...")) + .because("it ruins readability"); + } + + private static ArchCondition notExist(String reason) { + return new ArchCondition<>(reason) { + @Override + public void check(JavaField field, ConditionEvents events) { + final var message = field.getName() + " is not a valid name"; + events.add(new SimpleConditionEvent(field, false, message)); + } + }; + } +} +``` + +- How this technique could be useful for you? +- What rules would you need? \ No newline at end of file diff --git a/solution/day18/pom.xml b/solution/day18/pom.xml new file mode 100644 index 00000000..b69de279 --- /dev/null +++ b/solution/day18/pom.xml @@ -0,0 +1,28 @@ + + + 4.0.0 + + + com.advent-of-craft + advent-of-craft2023 + 1.0-SNAPSHOT + + + lap + 1.0-SNAPSHOT + + 1.2.0 + + + + + com.tngtech.archunit + archunit-junit5 + ${archunit-junit5.version} + test + + + + \ No newline at end of file diff --git a/solution/day18/src/main/java/lap/ShittyClass.java b/solution/day18/src/main/java/lap/ShittyClass.java new file mode 100644 index 00000000..41a93a2c --- /dev/null +++ b/solution/day18/src/main/java/lap/ShittyClass.java @@ -0,0 +1,13 @@ +package lap; + +public class ShittyClass { + private final String consecutive__underscores__kill__readability = "detect me if you can 😬"; + private final String _external_underscores_kill_it_too = "detect me if you can 🚀"; + + public void getData() { + } + + private int isTrue() { + return 42; + } +} \ No newline at end of file diff --git a/solution/day18/src/test/java/lap/TeamRules.java b/solution/day18/src/test/java/lap/TeamRules.java new file mode 100644 index 00000000..b8f148a6 --- /dev/null +++ b/solution/day18/src/test/java/lap/TeamRules.java @@ -0,0 +1,69 @@ +package lap; + +import com.tngtech.archunit.core.domain.JavaField; +import com.tngtech.archunit.core.domain.JavaMethod; +import com.tngtech.archunit.core.importer.ImportOption; +import com.tngtech.archunit.junit.AnalyzeClasses; +import com.tngtech.archunit.junit.ArchTest; +import com.tngtech.archunit.lang.ArchCondition; +import com.tngtech.archunit.lang.ArchRule; +import com.tngtech.archunit.lang.ConditionEvents; +import com.tngtech.archunit.lang.SimpleConditionEvent; + +import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.fields; +import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.methods; + +@AnalyzeClasses(packages = "lap", importOptions = ImportOption.DoNotIncludeTests.class) +public class TeamRules { + @ArchTest + private static final ArchRule no_getter_can_return_void = + methods().that() + .haveNameMatching("get.*") + .should(notBeVoid()) + .because("any method which gets something should actually return something"); + + @ArchTest + private static final ArchRule iser_haser_must_return_boolean = + methods().that() + .haveNameMatching("is[A-Z].*").or() + .haveNameMatching("has[A-Z].*").should() + .haveRawReturnType(Boolean.class).orShould() + .haveRawReturnType(boolean.class) + .because("any method which fetch a state should actually return something (a boolean)"); + + public static ArchCondition notBeVoid() { + return new ArchCondition<>("not return void") { + @Override + public void check(JavaMethod method, ConditionEvents events) { + final var matches = !"void".equals(method.getRawReturnType().getName()); + final var message = method.getFullName() + " returns " + method.getRawReturnType().getName(); + events.add(new SimpleConditionEvent(method, matches, message)); + } + }; + } + + @ArchTest + private static final ArchRule detect_consecutives_underscores = + nameAnomaly(".*__.*", "consecutive underscores"); + + @ArchTest + private static final ArchRule detect_external_underscores = + nameAnomaly("_.*|\\w+._", "external underscores"); + + private static ArchRule nameAnomaly(String regex, String reason) { + return fields().that() + .haveNameMatching(regex) + .should(notExist("not contain " + reason + "...")) + .because("it ruins readability"); + } + + private static ArchCondition notExist(String reason) { + return new ArchCondition<>(reason) { + @Override + public void check(JavaField field, ConditionEvents events) { + final var message = field.getName() + " is not a valid name"; + events.add(new SimpleConditionEvent(field, false, message)); + } + }; + } +} \ No newline at end of file diff --git a/solution/day18/src/test/resources/archunit.properties b/solution/day18/src/test/resources/archunit.properties new file mode 100644 index 00000000..afbba999 --- /dev/null +++ b/solution/day18/src/test/resources/archunit.properties @@ -0,0 +1 @@ +archRule.failOnEmptyShould=false \ No newline at end of file diff --git a/solution/pom.xml b/solution/pom.xml index aafa06dc..9b01f072 100644 --- a/solution/pom.xml +++ b/solution/pom.xml @@ -26,6 +26,7 @@ day15 day16 day17 + day18