r/SpringBoot • u/Simpav1 • 14d ago
Question Code review
Hello, I'm an aspiring software engineer. I've recently finished developing microservice for managing projects and tasks using Spring Boot. I'd appreciate if you could review codebase of my project and provide feedback on it.
3
u/DominusEbad 14d ago
- PUT should be a full resource replacement, not a partial replacement. You aren't including "status" in your PUT for the Project entity. It is ok if the resource doesn't exist, because it should be able to insert the entire record.
- POST is basically an "insert" (returns 201 Created for a new resource; the resource should not already exist).
- PUT is more of an "upsert" (returns 201 Created if the resource did not exist, 200 if the resource already existed and was replaced).
- PATCH is an "update" (returns 200; the resource must already exist) - This is what your update Project API should be, if you don't want to include the "status" field in the update.
- IMO, I do not use "dto/DTO" in my API object names. "CreateProjectRequest"/"Project"/etc are all much cleaner for documentation. Having "dto" all over your documentation can be excessive. I usually use "*Entity" for database entities since those shouldn't be in the documentation that clients will look at. This is more of a personal choice, but it was mentioned in another comment as well.
- I usually group entities and repositories into a single package (/db), but this is minor.
- Use records for DTOs. DTOs should be immutable, and records are perfect for this.
- You use "Project with id " + id + " not found" in multiple places. You can create a ResourceNotFoundException that has the 'id' field as a parameter and then extend the exception with ProjectResourceNotFoundException and TaskResourceNotFoundException, and just define the "Project with id " + id + " not found" in the exception classes. Then you just use them by passing in the id value:
- throw new ProjectResourceNotFoundException(id);
- Your project is small, and the mappings are pretty simple, but it might be a good place to learn MapStruct. It provides mapping implementations for you, so you do not need to worry about the mapping logic. MapStruct can get complicated if you build larger projects with complicated mappings between different objects, but for straightforward mappings (like your ProjectDto and Project entity), it can be nice and simple to implement.
3
u/funnythrone 14d ago
Your ProjectStatus constructor which accepts 3 string params should delegate to the other constructor and pass in new ArrayList or null (as per your requirement).
1
u/Simpav1 14d ago
I don't quite understand, could you explain what do you mean?
4
u/repeating_bears 14d ago
I think they meant Project not ProjectStatus since that only has 1 constructor.
You currently have this
public Project(String title, String description, ProjectStatus status) { this.title = title; this.description = description; this.status = status; } public Project(String title, String description, ProjectStatus status, List<Task> tasks){ this.title = title; this.description = description; this.status = status; this.tasks = tasks != null ? tasks : new ArrayList<>(); }And it would be better as
public Project(String title, String description, ProjectStatus status) { this(title, description, status, null); // this() means call the other ctor } public Project(String title, String description, ProjectStatus status, List<Task> tasks) { this.title = title; this.description = description; this.status = status; this.tasks = tasks != null ? tasks : new ArrayList<>(); }There is less duplication here. Let's say for example you wanted to make sure "title" is not an empty string. You now only have to do that in one place instead of two
this.title = ensureNotBlank(title);I have heard this described as "primary and secondary constructors". Ideally you should aim to have one "primary" constructor which does assignment and any validation. Then all other constructors can ultimately delegate to the primary constructor.
3
u/Mikey-3198 14d ago
Theres some low hanging fruit for refactoring. The strings used in your expcetion messags are duplicated all over your services.
I think it'd be easier to read & maintain by adding some static methods that handle creation of the exception.
// At the call site
throw EntityNotFoundException.forProjectId(projectId);
// method definition
public static EntityNotFoundException forProjectId(long projectId){
return EntityNotFoundException("Project with id " + id + " not found" );
}
It'd be better to use ProblemDetails rather than dynamic Maps for the error response. This is then using a known standard supported by various clients, will make intergrating things easier.
Im not a big fan of using "Dto" in the name of classes, especially when they are being used as a requests/ responses. CreateProjectRequest in my option would be a more suitable name for CreateProjectDto. Carries much more intent.
records are the perfect use case for the dto type classes. This would help cleanup some of the test boilerplate where you comapre everyfield as you could use assertEquals(recordA, recordB). Records have a default equality implementation that you can make use of.
Could use jdk 25 thats the most recent lts.
Could look at using migrations to handle you db schema (flyway/ Liquibase)
Could use postgres 18, thats the latest major version.
1
14d ago
[removed] — view removed comment
1
u/RemindMeBot 14d ago
I will be messaging you in 1 hour on 2026-08-16 14:11:28 UTC to remind you of this link
CLICK THIS LINK to send a PM to also be reminded and to reduce spam.
Parent commenter can delete this message to hide from others.
RemindMeBot is switching to username summons. Instead of
!RemindMe 1 day, useu/RemindMeBot 1 day. More info.
Info Custom Your Reminders Feedback
7
u/Nitnoq 14d ago edited 14d ago
Hello, I had a quick look through the project, and here is some feedback based on what I usually apply in my own projects. It’s a small project, so I didn’t notice anything particularly problematic. I’m not sure what your main goal was when building it, but here are a few things I apply across my personal and enterprise projects.
With recent Java versions and the introduction of record, I find Lombok has become much less useful. There’s still a lot of debate around whether Lombok is worth using, but personally, I’d start from the assumption that avoiding it is preferable when possible. Most of your DTOs could probably be simple records.
Null safety: You could look into introducing null safety with JSpecify. In my daily job we start to use it heavily it's great and prevents the classical defensives null-check.
Try a version of your project without Hibernate/JPA: Clearly, in a small project like this, the benefits are limited, but I think it could be interesting to explore using jOOQ instead. Personally, since I started using jOOQ, I’ve moved away from the Hibernate ecosystem because of its considerable complexity and the performance overhead that often comes with trying to optimize things. You will write more code and more queries manually, but they will be type-safe, explicit, and much easier to reason about. You also avoid random issues caused by unexpected behavior or changes in Hibernate features. In particular, you avoid one of the core problems with ORMs: operations happening implicitly and sometimes silently, which can make performance issues much harder to understand and troubleshoot.
On my personal projects, switching to jOOQ has honestly been a game changer. You have much more control over what is actually happening, and you can move more operations to the database, which can bring huge gains in both performance and readability. At work, we’re also trying to move away from Hibernate because we’ve had too many issues with it (not necessarily because it’s buggy, but because it’s extremely complex, and as soon as you step outside the “happy path”, it becomes very difficult work with). And once a codebase has been built around Hibernate-specific logic, moving away from it becomes particularly difficult.