-
-
Notifications
You must be signed in to change notification settings - Fork 26.9k
Onion Architecture #3266
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
base: master
Are you sure you want to change the base?
Onion Architecture #3266
Conversation
PR SummaryThis PR implements the Onion Architecture pattern in a Java application. The application manages a TODO list, demonstrating the architecture's layered structure with separation of concerns. The core domain logic is independent of infrastructure concerns, promoting testability and maintainability. Changes
autogenerated by presubmit.ai |
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.
✅ LGTM!
Review Summary
Commits Considered (1)
- d91d3d8: Onion Architecture
Files Processed (8)
- onion-architecture/README.md (1 hunk)
- onion-architecture/src/main/Main.java (1 hunk)
- onion-architecture/src/main/application/TodoService.java (1 hunk)
- onion-architecture/src/main/domain/TodoItem.java (1 hunk)
- onion-architecture/src/main/domain/TodoRepository.java (1 hunk)
- onion-architecture/src/main/infrastructure/TodoRepositoryImpl.java (1 hunk)
- onion-architecture/src/test/application/TodoServiceTest.java (1 hunk)
- onion-architecture/src/test/domain/TodoItemTest.java (1 hunk)
Actionable Comments (0)
Skipped Comments (0)
|
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.
Addionally, please add onion-architecture module to the parent pom.xml, otherwise it's not build by CI.
import infrastructure.TodoRepositoryImpl; | ||
|
||
import java.util.Scanner; | ||
|
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.
Here above the Main
, explain the pattern and describe how this code example implements it.
Scanner scanner = new Scanner(System.in); | ||
TodoService service = new TodoService(new TodoRepositoryImpl()); | ||
|
||
System.out.println("Welcome to the TODO App!"); |
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.
Use Lombok's @slf4j logging utility
public TodoItem(int id, String title) { | ||
this.id = id; | ||
this.title = title; | ||
this.isCompleted = false; | ||
} |
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.
You can use Lombok's @AllArgsConstructor
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.
... and @Getter
public class TodoServiceTest { | ||
public static void main(String[] args) { | ||
TodoRepositoryImpl fakeRepo = new TodoRepositoryImpl(); | ||
TodoService service = new TodoService(fakeRepo); | ||
|
||
service.createTodo("Learn onion architecture"); | ||
service.createTodo("Write unit tests"); | ||
|
||
List<TodoItem> todos = service.getTodos(); | ||
|
||
assert todos.size() == 2 : "Should have 2 todos"; | ||
assert todos.get(0).getTitle().equals("Learn onion architecture"); | ||
assert !todos.get(0).isCompleted(); | ||
|
||
int idToComplete = todos.get(0).getId(); | ||
service.completeTodo(idToComplete); | ||
|
||
todos = service.getTodos(); | ||
assert todos.get(0).isCompleted() : "First item should be completed"; | ||
|
||
System.out.println("TodoServiceTest passed"); | ||
} | ||
} |
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.
It doesn't look like a valid JUnit test. You should mark it with @test annotation.
public class TodoItemTest { | ||
public static void main(String[] args) { | ||
TodoItem item = new TodoItem(1, "Write tests"); | ||
|
||
assert item.getId() == 1 : "ID should be 1"; | ||
assert item.getTitle().equals("Write tests") : "Title should match"; | ||
assert !item.isCompleted() : "Item should not be completed initially"; | ||
|
||
item.markCompleted(); | ||
|
||
assert item.isCompleted() : "Item should be marked as completed"; | ||
|
||
System.out.println("TodoItemTest passed"); | ||
} | ||
} |
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.
Same here. Not a valid test.
System.out.println("\nWhat would you like to do?"); | ||
System.out.println("[a] Add tasks"); | ||
System.out.println("[d] Mark tasks as done"); | ||
System.out.println("[v] View tasks"); |
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.
Maybe add "delete task" function as well?
Pull Request
What does this PR do?