-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create rule S7197: Circular file imports should be resolved #4645
Open
github-actions
wants to merge
3
commits into
master
Choose a base branch
from
rule/add-RSPEC-S7197
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
{ | ||
"title": "Circular file imports should be resolved", | ||
"type": "CODE_SMELL", | ||
"status": "ready", | ||
"remediation": { | ||
"func": "Constant\/Issue", | ||
"constantCost": "0min" | ||
}, | ||
"tags": [ | ||
"architecture", | ||
"design" | ||
], | ||
"defaultSeverity": "Major", | ||
"ruleSpecification": "RSPEC-7197", | ||
"sqKey": "S7197", | ||
"scope": "All", | ||
"defaultQualityProfiles": ["Sonar way"], | ||
"quickfix": "infeasible", | ||
"code": { | ||
"impacts": { | ||
"MAINTAINABILITY": "HIGH" | ||
}, | ||
"attribute": "MODULAR" | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,100 @@ | ||
This rule reports circular dependencies between source files caused by circular imports. | ||
|
||
== Why is this an issue? | ||
|
||
Circular dependencies occur when two or more source files import each other, either directly or indirectly. | ||
This creates a dependency structure that lacks a clear hierarchy, making the codebase harder to understand and maintain. | ||
Additionally, the order in which circular imports are resolved is not guaranteed, which can lead to unpredictable behavior and runtime errors. | ||
|
||
=== What is the potential impact? | ||
|
||
Circular dependencies increase the complexity of the code architecture, reducing readability, extensibility, and maintainability. | ||
As the project grows, these dependencies can spread, further complicating the architecture and increasing technical debt. | ||
Over time, resolving these dependencies becomes increasingly difficult. | ||
|
||
== How to fix it | ||
|
||
1. **Refactor shared functionality**: If multiple files share similar functionality, consider moving that functionality to a separate module that both can import. This allows each file to depend on the shared module rather than on each other. | ||
|
||
2. **Use dependency inversion:** Instead of directly importing modules that create circular dependencies, use dependency inversion by passing necessary functions or objects as parameters. This breaks the circular reference and makes the code more modular and testable. | ||
|
||
3. **Split responsibilities**: Evaluate whether each file is handling too many responsibilities. If so, break them down into smaller, more focused modules. This reduces circular dependencies and ensures that your code is easier to manage and extend. | ||
|
||
=== Code examples | ||
|
||
==== Noncompliant code example | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel the example is confusing and incomplete, this is also the case in Java IMO. What do you think about something like this below (no clue if the ASCII diagram would work though). Non-compliant: // ┌──────┐ ┌──────┐
// │ A │ ─────▶ │ B │
// └──────┘ └──────┘
// ▲ │
// └───────────────┘
// main.js
import { A } from "./A.js";
A();
// A.js
import { B } from "./B.js";
export function A() {
console.log("A calls B");
B();
}
// B.js
import { A } from "./A.js";
export function B() {
console.log("B calls A");
A();
} Compliant: // ┌──────┐ ┌────────┐ ┌──────┐
// │ A │ ──▶ │ Shared │ ◀── │ B │
// └──────┘ └────────┘ └──────┘
// main.js
import { A } from "./A.js";
A();
// A.js
import { callB } from "./shared.js";
export function A() {
console.log("A calls B");
callB();
}
// B.js
import { callA } from "./shared.js";
export function B() {
console.log("B calls A");
callA();
}
// shared.js
import { A } from "./A.js";
import { B } from "./B.js";
export function callA() { A(); }
export function callB() { B(); } |
||
|
||
The following code contains a circular dependency: `order.js` → `customer.js` → `order.js`, and `order.js` → `product.js` → `order.js`. | ||
Both cycles are connected through `order.js`, forming a tangle of two cycles. | ||
|
||
[source,javascript,diff-id=1,diff-type=noncompliant] | ||
---- | ||
// order.js | ||
import { Customer } from './customer.js'; | ||
import { Product } from './product.js'; | ||
|
||
export class Order { | ||
constructor() { | ||
this.customer = new Customer(); | ||
this.products = []; | ||
} | ||
} | ||
|
||
// customer.js | ||
import { Order } from './order.js'; | ||
|
||
export class Customer { | ||
constructor() { | ||
this.orders = []; | ||
} | ||
} | ||
|
||
// product.js | ||
import { Order } from './order.js'; | ||
|
||
export class Product { | ||
constructor() { | ||
this.orders = []; | ||
} | ||
} | ||
---- | ||
|
||
==== Compliant solution | ||
|
||
The issue can be resolved by refactoring the structure. | ||
Two service functions can replace the dependencies `customer.js` → `order.js` and `product.js` → `order.js`. | ||
|
||
[source,javascript,diff-id=1,diff-type=compliant] | ||
---- | ||
// order.js | ||
import { Customer } from './customer.js'; | ||
import { Product } from './product.js'; | ||
|
||
export class Order { | ||
constructor() { | ||
this.customer = new Customer(); | ||
this.products = []; | ||
} | ||
} | ||
|
||
// customer.js | ||
export class Customer { } | ||
|
||
// product.js | ||
export class Product { } | ||
|
||
// orderService.js | ||
export function getOrdersByCustomer(customer) { | ||
// Implementation to get orders by customer | ||
} | ||
|
||
export function getOrdersByProduct(product) { | ||
// Implementation to get orders by product | ||
} | ||
---- | ||
|
||
== Resources | ||
|
||
- Wikipedia - https://en.wikipedia.org/wiki/Acyclic_dependencies_principle[Acyclic dependencies principle] | ||
- STAN - https://stan4j.com/advanced/adp/[Acyclic dependencies principle] | ||
- RSPEC - https://sonarsource.github.io/rspec/#/rspec/S7091/java[S7091: Circular dependencies between classes across package boundaries should be resolved] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
{ | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we can mention that the rule detects both direct cycles and indirect cycles, as these can be less intuitive but more prevalent. The example we provide is an indirect one.
Perhaps something like: