#4: Implemented PackageRuleTest#17
Conversation
pull from Master
hohwille
left a comment
hohwille
left a comment
There was a problem hiding this comment.
@moritzLanger thanks for your PR. Looks good to me. 👍
Can you have a look at my review comments and see if you can improve accordingly?
| return new ArchCondition<JavaClass>("check for the package structure to be valid", new Object(){}) { | ||
| @Override | ||
| public void check(JavaClass javaClass, ConditionEvents events) { | ||
| Pattern pattern = Pattern.compile(DEFAULT_PATTERN); |
There was a problem hiding this comment.
Pattern compilation consumes time. Use a constant for the Pattern to prevent compilation for every checked class.
| events.add(new SimpleConditionEvent(javaClass, | ||
| matcher.find(), "true if valid, flase if invalid")); |
There was a problem hiding this comment.
I would suggest using matcher.matches() here as find is for finding a sub-pattern that may occur multiple times in a String to check.
Further, I do not understand this event approach. So we always create and event with a boolean flag (true = OK, false = NOK). Will archunit raise an error displaying all violations then? Fine, if so - I am just curious as I have not seen this feature before...
| /*Components */ | ||
| public static final String COMPONENT_GENERAL = "general"; | ||
|
|
||
| public static final String COMPONENT_TASK = "task"; |
There was a problem hiding this comment.
I am not sure if it is a good idea to hardcode the components into the regex.
We only need the actual component names in the component dependency rules but for the package rule there is no point in restricting the components and listing all of them here as constants... As a result this rule will not need to be customized by the projects.
If we need the component name we can get it from a capturing group of the regex.
When implementing #3 we will see if we want to build it on top of this and your regex or if we use archunit features directly.
| + LAYER_DATA_ACCESS + "|" + LAYER_SERVICE + "|" + LAYER_LOGIC + "|" + LAYER_BATCH + "|" | ||
| + LAYER_CLIENT; | ||
|
|
||
| private static final String COMPONENT_LAYERS = COMPONENT_GENERAL + "|" + COMPONENT_TASK; |
There was a problem hiding this comment.
Components are orthogonal to layers so the term COMPONENT_LAYERS is IMHO missleading.
|
|
||
| private static final String COMPONENT_LAYERS = COMPONENT_GENERAL + "|" + COMPONENT_TASK; | ||
|
|
||
| public static final String DEFAULT_PATTERN = "(" + PATTERN_SEGMENT + ")\\.(" + COMPONENT_LAYERS + ")\\.(" + PATTERN_LAYERS + ")\\.(" + PATTERN_SEGMENT + ")*"; |
There was a problem hiding this comment.
The root package typically has multiple segments (e.g. com.customer.app).
I would create a separate capturing group for the entire root package as `"(" + PATTERN_SEGMENT + "(\." + PATTERN_SEGMENT + ")*)".
In case you will actually use capturing groups with according index in the regex code below, a best practice is to add a comment on top of the regex numbering the groups. Example:
https://github.com/devonfw/cobigen/blob/8db8c40c53fe2acedfe15010c69f7fcbee3bce7c/cobigen-plugins/cobigen-templateengines/cobigen-tempeng-agnostic/src/main/java/com/devonfw/cobigen/tempeng/agnostic/AgnosticTemplateEngine.java#L51-L52
|
|
||
|
|
There was a problem hiding this comment.
remove these blank lines :)
| @Override | ||
| public void check(JavaClass javaClass, ConditionEvents events) { | ||
| Pattern pattern = Pattern.compile(DEFAULT_PATTERN); | ||
| Matcher matcher = pattern.matcher(javaClass.getName()); |
There was a problem hiding this comment.
Actually, you should instead do javaClass.getPackageName() instead of javaClass.getName().
| LAYER_LOGIC, LAYER_SERVICE, LAYER_BATCH); | ||
|
|
||
| /* Pattern */ | ||
| private static final String PATTERN_SEGMENT = "[a-zA-Z0-9_]+"; |
There was a problem hiding this comment.
while it is technically possible to use UPPERCASE in package segments, it is strictly discouraged.
We should not allow this explicitly as it is sick to do this in Java.
hohwille
left a comment
hohwille
left a comment
There was a problem hiding this comment.
@moritzLanger thanks. Now everything looks perfect 👍
Ready for merge.
p.s.: We should later see how we can align this with the layer rules that defines the same layers redundantly that you have here as constants and later also with the component rules...
2 participants
Implements story #4.