Junior java dev mistakes from real-world project
I always heard that it’s super funny when you look at your code after some period of time (especially the one which you wrote at the beginning of your career!). And the funnier it seems to you, the better progress you made.
Finally, I had the chance to experience it for myself and wanted to share with you about it. Unfortunately, I will not able to describe all of my bad decisions and mistakes cause that post would be way too long :) I decided that I will describe mistakes that are pretty common among entry-level java developers. I think it might be a good source of knowledge and I wish I have seen such a post before I started writing my project.
The project I will be talking about is called AppointmentScheduler. Today it’s been 1.5 years (as of December 2020) since I created it. I will not describe each discovered problem in many details, I will just provide the concept and you can google the rest yourself. The main idea is to make you aware of which mistakes junior java developers make very often and gather all of them in one place so you could avoid it.
0. Not using static code analyzer
This is of course not mandatory but I wish I knew earlier that there is something like SonarLint, SonarQube or FindBugs. For example, you can download SonarLint to IntelliJ and it will analyze your code and highlight all common mistakes. Some of the issues mentioned below could be easily avoided if I have used these tools. Trust me that it is worth using one of these and the sooner you start to use it the better for you.
1. Wrong usage of Optional class
I used spring data which returns classes wrapped in Optional. I didn’t understand the concept and I just knew that I need to use .get() function to get the value. Spend some time to learn about optional and get to know all the functions related to it otherwise you will end up with something like I did:
WRONG:
@Override
public Appointment getAppointmentById(int id) {
Optional<Appointment> result = appointmentRepository.findById(id);
Appointment appointment = null;
if (result.isPresent()) {
appointment = result.get();
} else {
throw new AppointmentNotFoundException();
}
return appointment;
}CORRECT:
@Override
public Appointment getAppointmentById(int id) {
return appointmentRepository.findById(id).orElseThrow(AppointmentNotFoundException::new);
}Example from AppointmentServiceImpl.java
2. Not using Java 8 stream API
Many tutorials and books are still not using Java 8 stream API and are iterating over collections with for loops. I also did the same cause I was learning from some old tutorials and couldn’t understand why I should use streams. Once I learned few useful methods like map(), filter(), collect() I loved it and can’t look at for loops anymore :) Here is a quick example:
UGLY:
@GetMapping("/availableHours/{providerId}/{workId}/{date}")
public List<AppointmentRegisterForm> getAvailableHours(@PathVariable("providerId") int providerId, @PathVariable("workId") int workId, @PathVariable("date") String date, @AuthenticationPrincipal CustomUserDetails currentUser) {
LocalDate localDate = LocalDate.parse(date);
List<TimePeroid> peroids = appointmentService.getAvailableHours(providerId, currentUser.getId(), workId, localDate);
List<AppointmentRegisterForm> appointments = new ArrayList<>();
for (TimePeroid peroid : peroids) {
appointments.add(new AppointmentRegisterForm(workId, providerId, peroid.getStart().atDate(localDate), peroid.getEnd().atDate(localDate)));
}
return appointments;
}BOOTIFUL:
@GetMapping("/availableHours/{providerId}/{workId}/{date}")
public List<AppointmentRegisterForm> getAvailableHours(@PathVariable("providerId") int providerId, @PathVariable("workId") int workId, @PathVariable("date") String date, @AuthenticationPrincipal CustomUserDetails currentUser) {
LocalDate localDate = LocalDate.parse(date);
return appointmentService.getAvailableHours(providerId, currentUser.getId(), workId, localDate)
.stream()
.map(timePeriod -> new AppointmentRegisterForm(workId, providerId, timePeriod.getStart().atDate(localDate), timePeriod.getEnd().atDate(localDate)))
.collect(Collectors.toList());
}Example from AjaxController.java
3. Field injection instead of constructor injection
I used field injection instead of constructor injection. It’s easier to inject mocks in test and easier to spot cyclic dependencies if you use constructor injection.
WRONG:
@Controller
@RequestMapping("/appointments")
public class AppointmentController {
private final String REJECTION_CONFIRMATION_VIEW = "appointments/rejectionConfirmation";
@Autowired
private WorkService workService;
@Autowired
private UserService userService;
@Autowired
private AppointmentService appointmentService;
@Autowired
private ExchangeService exchangeService;
//....
}CORRECT:
@Controller
@RequestMapping("/appointments")
public class AppointmentController {
private final String REJECTION_CONFIRMATION_VIEW = "appointments/rejectionConfirmation";
private final WorkService workService;
private final UserService userService;
private final AppointmentService appointmentService;
private final ExchangeService exchangeService;
public AppointmentController(WorkService workService, UserService userService, AppointmentService appointmentService, ExchangeService exchangeService) {
this.workService = workService;
this.userService = userService;
this.appointmentService = appointmentService;
this.exchangeService = exchangeService;
}
//....
}Example from AppointmentController.java
4. Return null value
I was returning null in some cases. Always return default/empty values.
WRONG:
@GetMapping("/user/{userId}/appointments")
List<Appointment> getAppointmentsForUser(@PathVariable("userId") int userId, @AuthenticationPrincipal CustomUserDetails currentUser) {
if (currentUser.hasRole("ROLE_CUSTOMER")) {
return appointmentService.getAppointmentByCustomerId(userId);
} else if (currentUser.hasRole("ROLE_PROVIDER"))
return appointmentService.getAppointmentByProviderId(userId);
else if (currentUser.hasRole("ROLE_ADMIN"))
return appointmentService.getAllAppointments();
else return null;
}CORRECT:
@GetMapping("/user/{userId}/appointments")
public List<Appointment> getAppointmentsForUser(@PathVariable("userId") int userId, @AuthenticationPrincipal CustomUserDetails currentUser) {
if (currentUser.hasRole("ROLE_CUSTOMER")) {
return appointmentService.getAppointmentByCustomerId(userId);
} else if (currentUser.hasRole("ROLE_PROVIDER"))
return appointmentService.getAppointmentByProviderId(userId);
else if (currentUser.hasRole("ROLE_ADMIN"))
return appointmentService.getAllAppointments();
else return Lists.newArrayList();
}Example from AjaxController.java
5. @RequestMapping instead of the specific HTTP method
I used everywhere @RequestMapping instead of specific HTTP method annotation like @PostMapping.
WRONG:
@RequestMapping("/paid/{invoiceId}")
public String changeStatusToPaid(@PathVariable("invoiceId") int invoiceId) {
invoiceService.changeInvoiceStatusToPaid(invoiceId);
return "redirect:/invoices/all";
}CORRECT:
@PostMapping("/paid/{invoiceId}")
public String changeStatusToPaid(@PathVariable("invoiceId") int invoiceId) {
invoiceService.changeInvoiceStatusToPaid(invoiceId);
return "redirect:/invoices/all";
}Example from InvoiceController.java
6. Magic numbers
I pretty often used “magic numbers”. Instead of writing if(age > 65) write something like if( age > RETIREMENT_AGE) and define that var as static field
WRONG:
//...
else if (getCanceledAppointmentsByCustomerIdForCurrentMonth(userId).size() >= 1) {
return "You can't cancel this appointment because you exceeded maximum number of cancellations in this month.";
}
//...CORRECT:
private final static int NUMBER_OF_ALLOWED_CANCELATIONS_PER_MONTH = 1;
//..
else if (getCanceledAppointmentsByCustomerIdForCurrentMonth(userId).size() >= NUMBER_OF_ALLOWED_CANCELATIONS_PER_MONTH) {
return "You can't cancel this appointment because you exceeded maximum number of cancellations in this month.";
}
//..Example from AppointmentServiceImpl.java
7. System.out.println() instead of logger
I was using System.out.println() to print exceptions messages or debug information or even worse I was catching exception and did nothing with it. More useful information about this topic here.
WRONG:
} catch (MessagingException e) {
e.printStackTrace();
}CORRECT:
} catch (MessagingException e) {
log.error("Error while adding attachment to email, error is {}", e);
}Example from EmailServiceImpl.java
8. Breaking equals() and hashCode() contract
If you override equals() then you should always override hashCode() and meet these two conditions:
- If two objects are equal, then their hash codes should be equal.
- If two objects have the same hash code, then they may or may not be equal.
Example from User.java
9. Not using git workflow
I used git but didn’t use other branches than master. I think it was like day 2 of my professional career when I learned that there is git workflow. I really recommend reading this article and stick to this convention in your projects, that will make your development easier.
10. Unnecessary comments
I thought that adding a lot of comments will make my code better and easier to understand. I was wrong. Always try to write code that will be self-explanatory, if you need to add a comment to your code it might be an indicator that it’s too complex.
Example from AppointmentServiceImpl.java
11. Lack of auto-formatting
This is very trivial but in the first months I didn’t use auto-formatting. I recommend enabling this very nice feature in IntelliJ to reformat code and optimize imports before every commit. You will no longer forget about doing it :)
12. Using System.out.println for debugging :)
Try to familiarize yourself with debugger and breakpoints. I just put logging here and there to see what is the value if something was not working. I thought that setting up a debugger requires a lot of time. I couldn’t be more wrong, in IntelliJ you just press the debug button instead of a run button and that’s it.
13. Not using ENUMs
If you have some finite set of types that you need to represent in code always use enum! Here I had different values for appointment statuses like ‘SCHEDULED’, ‘CONFIRMED’… As you can see at the beginning I used String as type for status field which was a huge mistake! More on that subject can be found here.
VERY WRONG:
@Override
public boolean acceptRejection(int appointmentId, int customerId) {
if (isProviderAllowedToAcceptRejection(customerId, appointmentId)) {
Appointment appointment = getAppointmentByIdWithAuthorization(appointmentId);
appointment.setStatus("rejected");
updateAppointment(appointment);
notificationService.newAppointmentRejectionAcceptedNotification(appointment, true);
return true;
} else {
return false;
}
}CORRECT:
@Override
public boolean acceptRejection(int appointmentId, int customerId) {
if (isProviderAllowedToAcceptRejection(customerId, appointmentId)) {
Appointment appointment = getAppointmentByIdWithAuthorization(appointmentId);
appointment.setStatus(AppointmentStatus.REJECTED);
updateAppointment(appointment);
notificationService.newAppointmentRejectionAcceptedNotification(appointment, true);
return true;
} else {
return false;
}
}Example from AppointmentServiceImpl.java
Now it’s your turn!
There are much more bad practices in current version of AppointmentScheduler. As I said it was my first serious project in Java so it is full of such mistakes. I really encourage you to check it and write in comment section which bad practice did you find? I’m really curious what it will be and will be happy to add it to this post so others could learn from it as well.