One of my main issues when I first started learning code, was not being able to understand the logic behind the code. And everyone teaching back then only focused on making sure we memorized the style of the code instead of the logic behind it. - THIS for me, is one of the BEST part of your videos. Since you explain exactly what that code does and then, when needed, fix it, explaining exactly what the fix does. If there was a dedicated series on this, it would be amazing!! fantastic work. love what you do.
@AmigosCode, i really love the codereview, please can you make in the future some e2e tensting tutorials, as well as CI/CD, and Cloud (AWS or GCP) pleaseee, and keep going
I thought the same, especially because in this example, an ID (random UUID) is assigned to the newly created object. One can argue whether to return the entire object or just the URL location, but the client should receive some reference to the stored object.
I’m not a Java programmer so please take this comment lightly. I find that at 19:50, you refactored to not return anything but I think this differs depending on the context. If the frontend needs to be aware of multiple bookings, it should return that saved booking id instead of void. This is so that the frontend can be aware of the saved booking id to then maybe later in the same form send a patch request to update the booking.
That would be unclean code as you're adding multiple logic to one method. What you should do in this method is only book a room, like the name implies, and if you need the id, create a DAO to retrieve the id by the bookings name and optionally the start and ending date.
Wow, I never learned so much in one video. I think the value here is you are tackling it from a professional perspective, which some of us newbies lack. Tutorials many time go around teaching things like syntax and that kind of stuff, but I believe learning best practices is really valuable. Just loved it
Disagree with listAll The class is called booking service and it only gets bookings. Calling it getAll is better because it is less verbose and you can still figure out the meaning from the class name. Same thing with delete,find,create
Agree. Same as naming every date in entity with date: startDate, bookedDate, createdDate... It is inferred from type information, no need to type it twice.
Fair cleanup, but there are some points here that caught me off guard. 7:40 - I'm curious as to why it's best practice to only import the used types and not wildcard said package. The only "issue" there is, is that it clutters the local namespace and in reality could ONLY cause you to receive a compile-time error if there were to be conflicting type names, which can be resolved on the spot. Other than the mentioned "issue", there is no obvious reason not to do this if you're certain of the type namings. You also decrease the amount of unnecessary lines of code. 9:50 - 'listAll', 'all' or 'findAll' would be fine considering the fact that it's in a type of 'BookingService'. The type of listing is already made clear in the name so IMO 'BookingService#findAllBookings' seems very exhaustive and explicit. 10:36 - same point for this as the one mentioned above. 'BookingService' already indicate that it handles bookings and therefore, 'deleteByCode' references the code of booking(s). 29:29 - why would one NEVER reference the 'Collection' type? It's useful in many cases, and quite frankly required a lot. Feel free to prove me wrong if there are reasonable doubt and arguments to the points mentioned. As a backend programmer myself, I would be glad to receive said thoughts as we can never stop learning. Other than that; fun video, Amigos!
about the 9:50 and 10:36, it's never wrong or useless to be more explicit. You've got IDE helping you write all long names, but when it comes to reading it and tracing back it's always good to be more explicit, even if it's inside a type
@@StarzzDrak Of course. Don't get me wrong, I'm not saying it's not; what I am saying is that it is not a better practise nor is it needed. Quite frankly, I feel it's unnecessarily long. If you prefer to name it that way, go for it, but it's already indicated so in reality it's all good. In this case there was even multiple repositories, so even inheritance would do good with the original naming.
@@StarzzDrak Being explicit is good, but being concise is also important. I can't claim to be an authority on the best balance of explicit vs concise, but there is such a thing as being too explicit. It's the same as in 7:40. I don't think it is necessary to explicitly state every class you're importing, the domain names are explicit enough while being far more concise. To me, "listAll" is explicit enough, because this is the class "BookingService", it is the controller for the class "BookingDTO" and all it's functions implicitly refer to booking DTO. If for some reason listAll returned anything other than booking DTO I would agree that it needed to be more explicit.
@@StarzzDrak "it's never wrong or useless to be more explicit" I strongly disagree. For example, if you have a class Person with fields like "name", "age", "address", it is better to have methods like "getName()", "getAge()" and "getAddress()", and not "getPersonName()", "getPersonAge()" and "getPersonAddress()". when the code is "person.getName()", it is clear to the user what the name field is refering to, however "person.getPersonName()" just makes the code inflated for no good reason. This is just a simple example but of course it applies to the case covered in the video as well. if you have a booking service, bookingService.getById(), bookingService.getByCode() is clear enough, no need to inflate the code with the obvious context (booking)
20:30 honestly calling findAll on a repository and doing filtering in code is a huge code smell. you're loading an entire DB table into memory just to filter out one single entry. best case: it's just bad code, worst case: your operating cost explodes because of the memory consumption, also latency. the filter itself should be contained within a specialized DB query to the likes of (pseudo-SQL:) "WHERE startDate before now AND confirmed == true" then "ORDER BY start date ASC" and "LIMIT 1",. the repository method returns an Optional, since you're actually only interesented in one entry (which could not exist, e.g. if you don't have any bookins yet, therefore Optional). BOOM, your code just became extremely trivial. also don't forget DB indexes on startDate and confirmed DB fields fro proper DB query performance ;)
also, regarding RESTfulness: when adding new resources, you don't return void and 200 OK, you do return the saved entity with the generated UUID and respond with a 201 Created. a HTTP DELETE is supposed to be idempotent, which means that performing the same request several times should have the same effect. thefefore you shouldnt be returning a 404 when the entity to be deleted doesn't exist, just give a 204 No Content response (since the effect that the request asked for happened: the entity to be deleted does not exist after the request is completed).
but apart from that, great video, i pretty much agree with the way you structure and format the code and your way of thinking, love that you use assertJ. great educational stuff!
New to the channel but I love this series. I haven’t used java since 1st year of uni and it’s nice to learn about declarative patterns that weren’t taught in uni.
Awesome video man! I'm an experienced Java developer and I'm planning to take one of your online courses, because I'm learning a lot with you. Now, I want to argue with you in one thing. I think that the BookingService should actually return a BookingDTO upon insertion. This is because, when you send a 200 CREATED status, usually you send back a link to the newly created resource, so the client can actually send a GET request afterwards. In fact, mostly of the time the client will insert the new resource and do something with it afterwards(adding to a list on the UI, for example). So, sending the created entity avoids having to do a second request. If returning the whole Entity/DTO may cause a overhead, then at least the generated ID should be returned, so you can build the link for the client. Well, this is my opinion. Congratulations again for the video!
On the delete booking by code (and other situations like that), rather than have them void, I like to return a bool stating where the delete was successful, for instance if an unknown code is passed it may be useful when using that function to know whether the action was successful or not and structure the logic accordingly
Great video. For other watching, if you use findAll and filter afterwards its going to impact performance on a large dataset, always try to put your filtering inside your query (jparepository). You can even put your min max inside there.
The cranberry juice caught me off guard. Instant like and sub But seriously, thank you for this resource. Everything from the shortcuts, the use of optional, and just the way you were critical about naming conventions really challenged what I've learned over the last few years in school and the bits of work where I've scripted or dabbled in some code snippets. Awesome video!
Hello, Would be nice to have a Spring's course about Relation between tables, the cascades, constraints, when to use REST microservice or direct relation in module, joins, and other sql stuff ;)
Very nice code review!! I find writing static test classes that setup the entities that you will constantly test against makes life easier. I would probably also use some verifications in the test as well
@13:44, amazing, java already have throw something if null on its chaining method, just like Linq's FirstOrDefault which requires you an additional code for null checking
thank you for this, i ;ietrally dont even know some of the built in methods used here so now i have some cool features to look into. i appreciate this. its in a way bettdr than tutorials i think
Sir Amigo, i love your segment with this reviewing some created codes, its really so informative. I wish you can have this added as part of your weekly content. Thanks so much, you are so awesome. Ive learned so much about Java language. Thanks again and more power to your YT Channel.
Assalamu aleykum It’s awesome, bro! We need more your short lessons about refactoring, some practice lessons where we shoud use conditions operator, where map, streams…
Nice video, but I think if you refactor some code like that you should write the tests first and then refactor. Here you did refactor a method and wrote a test for that method and the test passes, but you might not know if you refactored the logic correctly.
At 21:00 would you really do all the filtering then and there "by hand" or would it be preferred to do the filtering right in the query to the underlying DB? I'm kind of torn apart between the DB usually being something slow one wants to reduce interaction with and on the other side the potential instantiation of a hugmongus amount of objects just to be filtered out right away. Would appreciate your thoughts @Amigoscode
Join the community: amigoscode.com/p/join-community
Premium programming courses amigoscode.com/courses
How to add icon build? Look at it funny ^^
I need your help plz if you don't mind. How we can stop people publish ing an image of us on Facebook and whatisup?
alhamdullillah da'wah and programming at the same time hehe
We want more of these series . It's very useful
True, please keep them coming 🙏🏽
True
Definitely learned a lot from this. I can tell you know the fundamentals very well and that goes a long way.
True
One of my main issues when I first started learning code, was not being able to understand the logic behind the code. And everyone teaching back then only focused on making sure we memorized the style of the code instead of the logic behind it. - THIS for me, is one of the BEST part of your videos. Since you explain exactly what that code does and then, when needed, fix it, explaining exactly what the fix does.
If there was a dedicated series on this, it would be amazing!!
fantastic work. love what you do.
Just more of this. This was literally amazing.
I enjoyed every single second of it.
Actually regarding the comparator you wrote. You could just do
min(Comparator.comparing(booking -> ...))
Then this would even be smaller and neater.
@AmigosCode, i really love the codereview, please can you make in the future some e2e tensting tutorials, as well as CI/CD, and Cloud (AWS or GCP) pleaseee, and keep going
19:25 Isn't REST standard to return 201 Created and include newly created object in the response body and URL in location header?
I thought the same, especially because in this example, an ID (random UUID) is assigned to the newly created object. One can argue whether to return the entire object or just the URL location, but the client should receive some reference to the stored object.
Yes, 201 with object is standard
Yup. 201 with the entity created. Otherwise how would the client know what GUID was saved in the db?
Yes 201 with object
@@mishikookropiridze Yeah, make sense to me.
This is amazing. More of this! Watching a senior dev correcting lower tier devs is so nice and a valuable lesson for us all.
I’m not a Java programmer so please take this comment lightly.
I find that at 19:50, you refactored to not return anything but I think this differs depending on the context.
If the frontend needs to be aware of multiple bookings, it should return that saved booking id instead of void.
This is so that the frontend can be aware of the saved booking id to then maybe later in the same form send a patch request to update the booking.
That would be unclean code as you're adding multiple logic to one method. What you should do in this method is only book a room, like the name implies, and if you need the id, create a DAO to retrieve the id by the bookings name and optionally the start and ending date.
Wow, I never learned so much in one video. I think the value here is you are tackling it from a professional perspective, which some of us newbies lack. Tutorials many time go around teaching things like syntax and that kind of stuff, but I believe learning best practices is really valuable. Just loved it
You can tell he has robust understanding of Data structures and Algorithms.
You are the best Trainer on Java so far in my experience.
Keep rocking. Big Fan
Disagree with listAll
The class is called booking service and it only gets bookings.
Calling it getAll is better because it is less verbose and you can still figure out the meaning from the class name.
Same thing with delete,find,create
Yep exactly
Agree. Same as naming every date in entity with date: startDate, bookedDate, createdDate... It is inferred from type information, no need to type it twice.
I think no RUclipsr had done this. This code reviews is so good to see. Please continue to.
ArjanCodes does some nice code reviews too
Well, I must say that modern Java looks quite neat. Greeting from dotnet :)
Fair cleanup, but there are some points here that caught me off guard.
7:40 - I'm curious as to why it's best practice to only import the used types and not wildcard said package. The only "issue" there is, is that it clutters the local namespace and in reality could ONLY cause you to receive a compile-time error if there were to be conflicting type names, which can be resolved on the spot. Other than the mentioned "issue", there is no obvious reason not to do this if you're certain of the type namings. You also decrease the amount of unnecessary lines of code.
9:50 - 'listAll', 'all' or 'findAll' would be fine considering the fact that it's in a type of 'BookingService'. The type of listing is already made clear in the name so IMO 'BookingService#findAllBookings' seems very exhaustive and explicit.
10:36 - same point for this as the one mentioned above. 'BookingService' already indicate that it handles bookings and therefore, 'deleteByCode' references the code of booking(s).
29:29 - why would one NEVER reference the 'Collection' type? It's useful in many cases, and quite frankly required a lot.
Feel free to prove me wrong if there are reasonable doubt and arguments to the points mentioned. As a backend programmer myself, I would be glad to receive said thoughts as we can never stop learning. Other than that; fun video, Amigos!
about the 9:50 and 10:36, it's never wrong or useless to be more explicit. You've got IDE helping you write all long names, but when it comes to reading it and tracing back it's always good to be more explicit, even if it's inside a type
@@StarzzDrak Of course. Don't get me wrong, I'm not saying it's not; what I am saying is that it is not a better practise nor is it needed. Quite frankly, I feel it's unnecessarily long. If you prefer to name it that way, go for it, but it's already indicated so in reality it's all good. In this case there was even multiple repositories, so even inheritance would do good with the original naming.
@@StarzzDrak Being explicit is good, but being concise is also important. I can't claim to be an authority on the best balance of explicit vs concise, but there is such a thing as being too explicit.
It's the same as in 7:40. I don't think it is necessary to explicitly state every class you're importing, the domain names are explicit enough while being far more concise.
To me, "listAll" is explicit enough, because this is the class "BookingService", it is the controller for the class "BookingDTO" and all it's functions implicitly refer to booking DTO. If for some reason listAll returned anything other than booking DTO I would agree that it needed to be more explicit.
7:40 couldn't be more right. Explicit imports are just noise in your code and don't contribute anything
@@StarzzDrak "it's never wrong or useless to be more explicit"
I strongly disagree. For example, if you have a class Person with fields like "name", "age", "address", it is better to have methods like "getName()", "getAge()" and "getAddress()", and not "getPersonName()", "getPersonAge()" and "getPersonAddress()".
when the code is "person.getName()", it is clear to the user what the name field is refering to, however "person.getPersonName()" just makes the code inflated for no good reason.
This is just a simple example but of course it applies to the case covered in the video as well. if you have a booking service, bookingService.getById(), bookingService.getByCode() is clear enough, no need to inflate the code with the obvious context (booking)
The code review/refactor series is great, keep it 👍
This is awesome! I would suggest writing the test first and then re-factoring.
I was going to comment that opinion as well 😀
WOW, I started as a junior java dev about a year ago and I learned more from these videos than most of my time at work!
Man I'm just loving such kind of videos. This teaches you so much practical stuff.
20:30
honestly calling findAll on a repository and doing filtering in code is a huge code smell.
you're loading an entire DB table into memory just to filter out one single entry.
best case: it's just bad code, worst case: your operating cost explodes because of the memory consumption, also latency.
the filter itself should be contained within a specialized DB query to the likes of (pseudo-SQL:) "WHERE startDate before now AND confirmed == true" then "ORDER BY start date ASC" and "LIMIT 1",. the repository method returns an Optional, since you're actually only interesented in one entry (which could not exist, e.g. if you don't have any bookins yet, therefore Optional).
BOOM, your code just became extremely trivial.
also don't forget DB indexes on startDate and confirmed DB fields fro proper DB query performance ;)
also, regarding RESTfulness:
when adding new resources, you don't return void and 200 OK, you do return the saved entity with the generated UUID and respond with a 201 Created.
a HTTP DELETE is supposed to be idempotent, which means that performing the same request several times should have the same effect. thefefore you shouldnt be returning a 404 when the entity to be deleted doesn't exist, just give a 204 No Content response (since the effect that the request asked for happened: the entity to be deleted does not exist after the request is completed).
but apart from that, great video, i pretty much agree with the way you structure and format the code and your way of thinking, love that you use assertJ. great educational stuff!
Thanks for the tips. Appreciate it
@@adamk2251 Yes. I was about to comment on this, but glad someone else did it before me :D
So basically keep a properly organized database instead of bodging in code?
I love the subtle arabic words you throw in
New to the channel but I love this series. I haven’t used java since 1st year of uni and it’s nice to learn about declarative patterns that weren’t taught in uni.
Thank you Sir for these videos. As junior for me your code review serie is more helpful than any courses I've every seen.
Even if I'am not learning Java I'am benefiting from this video, jazakAllahu khairan brother
Awesome video man! I'm an experienced Java developer and I'm planning to take one of your online courses, because I'm learning a lot with you.
Now, I want to argue with you in one thing. I think that the BookingService should actually return a BookingDTO upon insertion. This is because, when you send a 200 CREATED status, usually you send back a link to the newly created resource, so the client can actually send a GET request afterwards.
In fact, mostly of the time the client will insert the new resource and do something with it afterwards(adding to a list on the UI, for example). So, sending the created entity avoids having to do a second request.
If returning the whole Entity/DTO may cause a overhead, then at least the generated ID should be returned, so you can build the link for the client. Well, this is my opinion.
Congratulations again for the video!
Please go on posting this kind of videos because they are amazing and full of tremendous concepts
On the delete booking by code (and other situations like that), rather than have them void, I like to return a bool stating where the delete was successful, for instance if an unknown code is passed it may be useful when using that function to know whether the action was successful or not and structure the logic accordingly
This helped me refactor my own project
Walaikumsalaam warehmatullahi wabarakaatuhuu...Love from Kashmir💖
Great video. For other watching, if you use findAll and filter afterwards its going to impact performance on a large dataset, always try to put your filtering inside your query (jparepository). You can even put your min max inside there.
When you "Assalamualikum", it made me so happy!!
The Legendary Hero walking around at 1st flat of the dungeon, hanging with the children:d
Best video on RUclips. your videos inspire me and make me more professional. Thank you a lot
The cranberry juice caught me off guard. Instant like and sub
But seriously, thank you for this resource. Everything from the shortcuts, the use of optional, and just the way you were critical about naming conventions really challenged what I've learned over the last few years in school and the bits of work where I've scripted or dabbled in some code snippets. Awesome video!
@Amigoscode this was a fantastic video. It's great to see you at work.
Amazing thanks for the help
This way my first spring boot project and ive learned a lot from you !
Learning through errors correction is one of the best way to fix new things in mind ! well done
Hello, Would be nice to have a Spring's course about Relation between tables, the cascades, constraints, when to use REST microservice or direct relation in module, joins, and other sql stuff ;)
Very nice code review!! I find writing static test classes that setup the entities that you will constantly test against makes life easier. I would probably also use some verifications in the test as well
Best coder around!
@13:44, amazing, java already have throw something if null on its chaining method, just like Linq's FirstOrDefault which requires you an additional code for null checking
It doesn't take 2 seconds to like this video. I see you, I like it. Simple :)
I would love to see more code refactoring videos like this.
It would be really nice to see the keyboard to know the shortcuts you are using
Can you make a course on dart and flutter 🎯
Keep up these series. This is worth every second!!
It is satisfying to watch it. Please continue 🙏
Extremely useful!! I don't see much video content like this... It's practical.
It's awesome to see that is going to be a new series :) Code review is the best way to exchange knowledge
السلام عليكم..
hello brother.. thanks for everything you did for people..
suliman from SaudiArabia ❤
We need more of this. Learnt a lot,gonna apply this in future for sure💯
This video cured my athlete's foot.
thank you for this, i ;ietrally dont even know some of the built in methods used here so now i have some cool features to look into. i appreciate this. its in a way bettdr than tutorials i think
Amazing stuff akhi, can’t wait to get on that level in sha Allah😍🔥
this was really awesome, I felt like all code I've ever written was being reviewed, and it was great.
Please bring again same videos for code reviews asap. These are amazing and you are doing great work. Thanks a lot!! Please bring asap...
Walikum Alsalam, Gaza’s Allah Khair…. Loving you code ❤️😘
This is amazing, we need more videos like this!
Hugs from Brazil!
hello, thanks for your demonstration how we can be better coding.
Amigos thanks for taking the time to explain this topic
Thanks. Please continue this series.
I really appreciate this kind of videos, man you are awesome!
Do an example on how to use functional programming/streams while using a logger!
more videos just like this, but dive deep into each of the service classes
Sir Amigo, i love your segment with this reviewing some created codes, its really so informative. I wish you can have this added as part of your weekly content. Thanks so much, you are so awesome. Ive learned so much about Java language. Thanks again and more power to your YT Channel.
Thank you, great video!!!
I have one question: You say never use a Collection always use a List. Why? What's the problem with using a Collection?
This is great stuff. I'm tempted to do a kotlin equivalent.
Hey,
Thanks for sharing your knowledge.
I have a question about using dtp on service layer.
What are the pros and cons please?
Regards
This is really good. Plz do more of such stuff.
I don't even use Java, but I like your videos.
I love this goofy format!
I always like coding reviews and thank you very much for the topic.
It's amazing. I always learn something new from your videos.
Amazing. How do I get my prize?
Wow this is a great video, also I love your hat! You just earned a new subscriber :D
Here comes the incredible Nelson. Kuddos to you bro you are the best
Very usefull. Next time the code review could be a Spring Rest controller test with mocking.
Powerful Teaching Methods Here, Thanks very much!
More of this series please!!! 👍
never new i'd be very interested in refactoring Thank you !
There is a shortcut: Command+Option+(left arrow) to move to last edited place
Nice video, please make more of these, I'm hooked!... A series of these would actually make us better java developers🙂💯!!!!;
Thank you for making more of these!
Love this series! Please continue!
Love your videos and this series a lot. Which font do you use for intelliJ? Is that the default JetBrains Mono?
Wow. Thanks a lot for this video. This is more helpful. Please drop more videos like this.
This is great stuff! Thanks for these!
isn't calling filter and then min slower? the original code does both in one pass, but idk
It is amazing! Looking forward to see more!
Whao. I would love more of these. Also, would you mind displaying the InteliJ shortcuts you use?
It’s there on bottom in green
Thank you for your contribution 🙏
Assalamu aleykum
It’s awesome, bro!
We need more your short lessons about refactoring, some practice lessons where we shoud use conditions operator, where map, streams…
I like your good heart Sir
Well done
Nice video, but I think if you refactor some code like that you should write the tests first and then refactor. Here you did refactor a method and wrote a test for that method and the test passes, but you might not know if you refactored the logic correctly.
Amazing video, very constructive criticizing. Please have more videos like this.
This is great, please make more of these
I liked his Berserk pic)) (on the first repository)
Thanks you so much amigoscode for this kind of videos
At 21:00 would you really do all the filtering then and there "by hand" or would it be preferred to do the filtering right in the query to the underlying DB? I'm kind of torn apart between the DB usually being something slow one wants to reduce interaction with and on the other side the potential instantiation of a hugmongus amount of objects just to be filtered out right away. Would appreciate your thoughts @Amigoscode
DB slow? Fetching all data and filtering in the application code is a huge anti-pattern. You can do this in toy projects but not in real life.
It was amazing and I learn alot from your video, keep grinding
I love this channel btw -- Subscribed