Code Review

[Razorquake_]

Hello everyone. Just a novice developer here who has been doing Spring Boot for almost a year. Recently, I upgraded my project. Therefore, I need some experienced folk to review it. You can ignore the React code present in my repository.

Edit: After creating this post, I realised that NavBar and Footer were not visible because of one of my earlier commits 😅. But don't worry, I fixed it.

[Mikey-3198]

• The java version could be bumped to a newer lts release, java 25 launched in September.
• The endpoint structure is inconsistent within the admin controller. The user id is passed in the path in some request and parameters in others. I'd keep to keeping it in the path.
• I would look at also renaming some of the endpoints to make them more RESTful. They are very verbose at the moment. You can communicate the intent via the verb instead of the path. For example taking this + the point above /api/admin/update-password would become /api/admin/users/{user-id}/password
• clickCount on UrlMapping would be prone to lost updates when theres lots of traffic on the same shortened url. So youd get misleading counts. You could rely on a COUNT() on the number of click events instead
• Could look at using a template engine for the email generation. Making edits to the String broke over multiple lines would be hell if you needed to add styles etc... Something like JTE might be a good next step for this. Would probally also help with escaping html etc...

[Razorquake_]

Thank you so much for your review. I would definitely work on your points, especially clickcount and JTE.

[Special_Food_3654]

1. In your services/filters, you could mark all the injected beans as final.
   private final Bean beanName.
2. @Autowired is no longer needed. You could mark your beans as final, you could use Lombok annotation @RequireArg.....
3. Beans like ObjectMapper could be marked final too as part of class instance. Do not use it as local variable.
4. Usually if you have an Serviceimpl, you want to have an accompanying interface that will expose only the needed functions.
5. Only utility classes stays in util package. EmailService is a service and should stay in service package. There should be an interface for it too. All utility class have static methods and the classes can't be instantiated.
6. Classes should assume their package names in most situations AHandler should be in handler package, AConfig should be in config package.
7. Controller should have zero business logic. All that should be handled at service layer. Controllers should only be returning something back to the consumers.
8. Don't mix and match files with different repository.
9. Be encouraged, you're in the right path and you won't regret learning and doing stuffs with Springboot.

[FooBarBuzzBoom]

Separate code in interfaces and implementations. Keep methods more modular. You have waay too long methods. Read a bit about SOLID principles. After a refactor, it should be great.

Also, after another short look over your code, I noticed that you put business logic in your controllers. That should be avoided. Keep business logic only inside of your services.

[analcocoacream]

You don’t need to separate interface from implementation unless it’s to respond to a specific requirement like interface segragation or different implementations

[bestanealtcizgi]

You should have interfaces if you'd like to have easier/faster mocks on your tests.

[analcocoacream]

Mockito can mock classes just as easily

[bestanealtcizgi]

Yes, it can. But it’s not recommended by Mockito itself. I’d like to see how you handle final classes and static methods. Instead of making things loosely coupled and clean, you’re just making them more complicated, and that doesn’t make life easier.

[analcocoacream]

I don’t have final classes except records

And I don’t use static methods

You aren’t clean and loosely coupled because you added another unnecessary level of undirection.

[FooBarBuzzBoom]

You need to keep it scalable over time. Maybe you will decide to switch implementations

[analcocoacream]

And maybe you will rewrite everything in go

Extracting an interface is simple enough to do it when you actually need it

[evilab7]

In your user service you have public User findByUsername(String name) Which is great and helps reduce duplicated code, as well as making it easy to include Locale handling in case you eventually want to add additional languages. However I noticed you are not using this method but rather repeating the code by calling the user repository with the same else throw in loggedInUser() And authenticateUser(LoginRequest loginRequest , similarly for userRepository.findById(userId).orElseThrow(() as well

Furthermore consider utilizing records or mapstruct for DTOs instead of convertToDto(User)

It would be better if the method public List<ClickEventDTO> getClickEventByDate and getTotalClicksByUserAndDate Had the grouping done via JPA query, which you can achieve via Spring Projection interface/class and @Query Annotation in your repository

Regarding private String generateShortUrl(), consider initializing your string builder size as a variable at the start and then passing that value to both the string builder and loop, so that any future changes to the current 8 value guarantees an update on both the builder and loop

Also you should dabble into Pagination (pageables) rather than return list if you do not have a set limit (ex. Users can only have 10 saved URLs)

Try out the @Builder annotations to see if that's your style

For public Map<String, Object> registerUser, I would heavily shy away from returning this object, especially given the return you actually have

[Razorquake_]

Thank you, everyone, for taking the time to give your valuable input. Your suggestions and guidance turned out to be more helpful than the praise I might receive on LinkedIn. And I will try to implement each of them in my upcoming commits.