OK i need to change your code. SORRY

let's just right click and run fingers crossed fingers crossed fingers crossed what what is going on guys assalamu alaikum welcome to amigos code in this video i want to pick a project which was sent on the code reviews channel and this is because of the previous video that i've done two weeks ago you guys seem to be really interesting in this sort of stuff so i was like okay fine let me just pick someone else's um repository and this time what i'm going to do is actually dig deep into one of these classes and i'm going to refactor and even add a test so that you can see the overall process if you're new to my channel literally just take two seconds and smash that like button literally just take two seconds so while i drink this um cranberry juice this is amazing so good especially when it's really cold so yeah so hopefully by now you should have subscribed and if you're not part of the um discord community go ahead and join so the community is growing i think around 15 000 people and growing and also we have a private facebook group which um very soon everybody's going to be shifting to discord because discord is amazing for communities so without further ado let's kick off this video right so in here i've got this um person right here i really don't know um his name i think it's that's his nickname basically mint so he said hello i really loved your video or your last video and thought i would share my code for review i'm a final year software engineer student and this project is for last year internship and basically give me the repo yada yada but i'm going to go over and basically clone this which i actually cloned already i'm going to review some of these classes and whatnot and add a test one or two maybe let's see um how we get so i actually haven't i haven't looked into this so this is my first time looking into this uh repository so hopefully should be not that bad hopefully so there's a bunch of people that actually sent me uh more stuff really interesting projects uh really sorry if i cannot review right now because there's so many of you but let me know if you want me to you know continue the series then i can go over some of these repos and you know give you some feedback basically okey dokey so i already cloned and i've got intellij open so let me just open up intellij and in here the first thing that i can tell you is that you don't have the readme so from last week i said that read me it's really important that you have readme because i can just look at this and know exactly what this project is about so here i can see you've named the main folder reservation but then the module you called it booking system which is yeah so yeah i know i know naming is hard but you can do better right so then i can see you've got some github stuff so workflows build okay that's fine let's have a look at this so on pull requests opened reopened then you set up java 11 you check out the code and you use some caching and basically you're using sauna cloud so this is mainly for checking whether you've got the right text coverage code smells and whatnot which i kind of like and cool so this is nothing nothing special here but it's awesome because um you know by just pushing and opening up a pull request you can basically just just see how the code looks like right so let's uh basically close this so we don't need this and what i want to focus here really i'm just going to go straight into what matters right let's just dive into tests all right so i can see some tests fine now i'm not going to look into these tests what i'm going to do is actually right click and then run tests with coverage so write test with coverage and let's see if they'll pass ah what you've got you got tests which are currently broken what is this what is this man right so let's have a look even at these tests so okay context loads so there's something wrong with your code so let's go into this test so this failed and basically let's have a look at the coverage in here so the coverage for uh reservation is quite low to be honest have a look so yeah this is this is like pretty much really really low so it to me it looks like you don't have any tests so let's just have a look so let's open up this it's empty okay cool so this was at your internship so i would expect you to do some testing but let's just get rid of this because this is just causing us problems right now entities i don't know why you test entities no one does so let's open it and yeah it's empty so we don't need this as well and oh hold on was that it alright so you've got a package here which is not tested yes that is it okay cool so let's just get rid of that and this package as well so hopefully i can basically write one test so you can see the overall process now what i want to do is let's just go into src main and then java and you can even see so intellij is quite nice so it gives you the code coverage which is really really great so 12 of classes one line covered and you can see all these classes or packages my bad now what i'm interested is so i'm going to go straight into so i'm not interested in config nor down or dto no entities no exceptions no mappers nor security but i'm interested in the service because this is where the business logic lives so let's let's let's just open it up service okay so you've got a bunch of classes in here so i wish i could you know spend my entire day just going over this refactoring give you some feedback but let's just pick the very first class which is booking service all right so if you see in here have a look so this is kind of nice right so you can see these red lines in here so this is intellij telling you what's been tested so currently if it's red it hasn't been tested another look so all of this not tested and hopefully i can give you some feedback and let's see if we can improve on this now i'm going to try and destroy all of this sorry now i'm going to try and destroy all of this if i have to literally that is the goal so you can see how i would go about and refactor everything okay so the first thing i can see here so let's just have a look so you've got your imports right here and then you have java.util so i usually don't like to have star in here so it's always best if you have all imports which are being used instead of having everything so this is the best practice then next i can see that you're using lombok that's fine uh at service that's good transactional required all rx constructor which is fine but then one thing which i i can see immediately why is this public why it doesn't have to be public right so these instead they should be private and also what is what is what is what's this like like yeah format your code like literally format your code so here this should be like that like this and then here let's just select public and let's select everything and let's make this as private there we go and you can see this is much neater right then you have user repository which is not being used so this um can go and right here so you have public list booking dto list all okay list all list or what just imagine if every single method out there is named list all right then yeah so that light in there the blue light it just it just keeps on you know flashing um but if every single method out there was named list all then you can imagine that it becomes so difficult to navigate around because you know when you build projects uh big applications you have to use distinctive names because it's not just you working on it you've got a bunch of other engineers searching refactoring and you know find usages and and all these things and if i want to search um all the methods and everything is called list all like come on just work a little bit on on the naming basically right so here booking repository so here uh maybe you can say uh fine so what i'm gonna do is refactor this so i'm gonna say find all bookings so this is much better right then this line can go and [Music] this is good okay so maybe one thing here um if you want you can you can basically extract this to a variable and you can say bookings and then you can basically return the converter entity to dto and then you pass the bookings so this is much easier to read next let's have a look delete by code so delete what right so again naming in here so never start the cl the method name with uppercase so this should be camo case and common case goes like this so delete and then here this should be booking by code yeah so delete booking by code and then you have your book repository delete by code which is fine and then you pass the code awesome the next method let's have a look so in here this one so get by id this throws an exception not found so if this is empty then you throw this exception okay so i can see right here that you're not using this properly so let's have a look not found exception so this should be a runtime exception and not a checked exception yeah because you're not forcing the caller of that method to deal with that exception right away right because your code can compile so this is a runtime exception because you know an error might occur during the normal execution of your application okay so this should be runtime exception and i'm not i'm not sure whether you're using spring or not but then this by default returns a 500 so i think you're using spring yeah so here i think you can say at response status and you can say no found so this is now returning the correct response back to the client so 404 let's go back so here again we don't need this in here and this right here we can do better so what we can do is so here let's just rewrite this so we can say booking repository dot find by code we pass id now this guy right here returns a booking now what i'm going to do so if i go inside here you have booking and then find booking by code what you could do is you could encapsulate this into an optional if you want so you could say optional of booking and we'll fix these three problems in a second so then what you do is in here you say right so now i can say dot and then you can see that you are throwing the exception right so if this is not um present you can say or else throw and this is a lambda like that and then let's just get rid of that and what is this so this should be string id uh oh actually sorry it's fine by id my bad so i'm actually refactoring the wrong thing so let's just go back in here so it's not that one and it's this one here oh this actually already returns an option which is fine so beautiful so it returns an optional and we are passing the id in there so find by id so here it should be find by id so i was using the wrong one or else you throw that exception just like that and then what i'm going to do is so just before i say or else here you actually map so you convert your record that you find from the database to a detail so here we can actually say map so let me just put this right here and here we can say dot map and then we're going to take this guy right here and here we get a book so uh or booking i'm gonna call it booking and then paste that now instead of saying booking.get i can just say booking and have a look so this now is much neater so this is return and have a look and i can even take a step further which is replaced with method reference and have a look this is much nicer right and one other thing which i can see here is reservation not found or not available so here uh which reservation right so you need to be specific reservation and then here we could just um you could use format and whatnot but let me just use plus in here id not found so here you are giving more information to the client so you're saying reservation and let's just say call them in here and then not available or not found for example so this is much later all right let's move into the next one shall we so here i think this is fine again um let's just say booking in here and i'm just doing this so that it's easy to read now here you don't have any validation right you don't have any validation now i would say if you don't have any validation then in your booking dto in here so you should basically enforce that or on the controller you should say right so this has to be present which i'm assuming you're doing right i'm not going to check but i assume that you are doing so ends you're not performing uh whether it's blank here or not but this i think is taking shape right so have a look how we we are improving all of this so here let's just get rid of that space all right cool then book a room so you're going to book a room so you're going to pass the name local date start and whatnot now here this should be before so room so i'm going to say room and then here all of this should be a name like that and then start so work on the namings in here and they should be end just like this okay then this should be name lowercase and you have the room now if this is no then what's gonna happen right you need to account for that so here let's just say because you know using optionals so here i've looked this is your interface and you know using optionals we could say optional dot of nullable or we could just return an optional in here so let's just say optional and again if there is any problem so six problems okay i don't fix all those six problems right now let's just keep it simple so here i'm going to say optional dot of nullable because it can't be null dot or else we're going to throw one exception so let's just throw an exception so again um not found exception cool we can just reuse this so new not found exception this is now returning the right thing to the client and you can pass a message in here so room with uh plus name so name uh oh actually um it should be not found not found just like that okie dokie so i just have to add the actual name in here there we go and this is much better right so room like that so room and then pass the name not found then if this is the case right so now you can start your booking so you create the booking you set the room set the start date and you set the code now i think this this logic is is wrong i think this should return a void right so think about rest just literally think about rest wait uh book a room and then so what what do you do with this you just you just find the room and then you set you set right so i think i think you you're using jpa so yeah so this is jpa and i think you're expecting this to save okay cool so think about rest so you are posting you are you are adding a new resource so why are you returning an object you you don't okay so this should be just 200 status code and then you let the client if it wants to get that data back the client can get it back right so let me just have my cranberry juice bismillah so here what i'm saying is don't return anything and problem solved this is much better now we have a problem in here and i guess is because yes it's because you're passing this from another class and let's have a look booking and then you say booking.save uh yeah so i think this is this is incorrect okay but let's just go back in here so you basically should shouldn't return i'm just going to leave it like this so it doesn't break anything but make it void yeah then next booking so this should be lowercase n then okay one thing i can see here is have a look you say next booking doesn't mean anything next booking or find basically just a name just the name needs to be better right so let's let's just move on so here then you have a final books you create a next booking which is empty main value i don't know what what this is doing here oh right so mean value is what is this number okay so do this so this should be underscore and the score and now it's easy to read also never use numbers like this so this should be a constant so this should be a constant refactor and then here introduce constant so public static and you can even make it private if you want which is better so here i'm going to say min value and let me go to it and it should be private so this is much better okay now let's go back to it because i think this needs some work and why is it complaining oh yeah it's complaining because i didn't refactor correctly so here next booking like that and then you have some casting which is not needed and then right here you have next which is a new object and then you're basically mutating this whenever i see stuff like this this is yelling at me streams stream streams and streams but also here next booking but then you return the code right surely this should return the actual booking and not the code so i think the name here let's just refactor once more so get next booking code i think this is much better okay then all right so let's just try and refactor all of this so in here what we're gonna do is we're going to say booking repository dot find all then we're going to stream then what you do we're going to filter because right here this is your filter have a look if so i'm just going to copy all of that command w a couple of times all good command c and then here we're going to take booking and i'm going to paste that in so have a look this is now coming together and also um you should always pass clock so that you have access to the time in here right because when you test then it's easy for you to move the clock forwards or backwards so this is my filter then what do we have so okay so i think what you're trying to do here is duration you get the duration for okay and then okay so you're trying to find the minimum and then you say the minimum if the duration is less than less and less than the minimum then you update that value all right cool so i think we can do this we can take all of this in here and in here so new line and i can say dot and then min this takes a comparator one and o two in here so instead of one and o two i'm gonna say uh b1 and then b2 so for booking okay now i'm gonna paste this duration in here because this is what you are trying to do now um i'm going to say b1 oh let's just let's just not be lazy so booking one and then booking two and then here let's just say booking one and i don't think you need to cast this do you oh okay so you're casting two to be an integer so this could be a long actually so i think that returns a long okay cool so duration one and then let's just have the duration two in here and instead of booking one this is booking two booking two in here and in here what we're going to say is we're going to return so duration 1 greater than duration two so this is using comparator if this is the case we're going to return one otherwise zero there we go so you can see this is much neater now what do you do next then then you have a system.out.printline so we don't need that but the last thing that you return is code so what we do here is we're going to say dot and we're going to map we're going to get the actual code so get code by using method reference and i think so here and this should actually return an optional because if this is null then things will break so here i'm just going to say or else or else another string so the other string will be no now have a look so this is much neater and i can just say return and all of this can go have a look all of this can go just like that so this is beautiful stuff now and obviously uh because i changed this i wanna i wanna make sure that indeed it does work yeah i'm gonna write a test for this um so let's just have a look so here uh get all get all by room name so let's just re factor this again so this will be oops get all by room name again so what you're doing here so you're checking whether this is null okay so let's just try and do this let's just try and replace this with so let's see so okay so this is a room okay i'm going to say optional dot of nullable because this can be no let's just take this pass it inside and i'm going to say dot or else we're going to throw the exception so here and this should be like this and then new and then not found exception and let's add in the room so plus plus just like this okay um and in fact i just have to remove that from there and then pass the name so name and [Music] there we go now what you have in here what okay so you say if it's if it's now you throw an exception else you get the bookings blah blah blah okay so we can just say uh dot and then map so always use functional style or declarative instead of you having to define every single business logic in here so otherwise we're going to map so this is a room we're going to say room dot get bookings in here and [Music] let's also take this oh actually we could just return so we could just say this basically right so we could just say this in here just like that and there we go so you could do this or if i go back once more so dot map so this now will give us bookings and if i paste that and then pass bookings inside so you can do either have a look now this is much cleaner and then return and all of this now can go beautiful stuff now here never use a collection use list always and i guess it's because of it's because of what is it because of this uh it shouldn't be uh nothing breaks i think right let's just improve this with method reference method reference here as well whoops and have a look so this is much neater now so i think we are almost there so get user by booking id so here i'm going to say oops refactor this get user so the name should be lowercase again no need to throw in here and get by id this throws the exception that's fine so if we go back so that there's a shortcut that allows you to go back to the last edited um place but so i think we were here right so here yeah i think we can just in line this just like that there we go okie dokie so here again no need for this and you convert to dto you set the code you check the availability and i think and then you and then yeah okay so i'm just see whether i should complete this or not because the video is already becoming too long so i think basically um this is the wrong approach so when you save just say void and return a status code 200 and again use a functional programming here confirm booking okay this seems this seems plausible that's fine and here you use lowercase c which is fine and then okay this looks okay just a space there and then delete booking so why are you getting okay so you're not using anything here okay so i think you could just do what you've done here oh get my id all right so get by id and also get by id so i think we could just reuse our get by id in here because that will throw an exception that will throw an exception so this will give us a book in dto if it's not found it throws an exception but the problem here is that you kind of just throw throwing around objects without even using them right so i'm just gonna leave like this but basically you should see whether it exists and maybe return it true so here i think you have um find by id or no find all oh yeah uh get by id let's have a look yeah so this is this is exactly what you're using so i think let's just get rid of that and then this is um basically if it's now then throw the exception basically whatever i've been doing right okay so let me just let me just ignore this for now so what i want to do is i want to quickly write a test i think this was the main thing here that we refracted so let me just write a test for this so here command shift t so command shift t create a test and i think what's the name of it next spoken code next booking code select that let's also have the setup and okay i'm going to cancel so this gives me the skeleton which i'm going to need i don't use this by the way i use the search for j now let me just say can get next booking code okay now let's have a look at the dependencies in here so we have three dependencies so let's just take this and also this is not used anymore let's just get rid of it so here let's just basically have all these in here and let's just select everything i'm going to say mock so if it's an external dependency we should mock so at mock and this doesn't have to be final anymore let's have the booking service so private booking service i'm going to call it under test and before each the setup i'm going to say under test equals to new so we have a new instance each time passing the book repository the this should be booking converter so booking converter booking converter oh should be converter um i'll refract this in a second and then room repository so you can see how clean this looks right so also i like to always put these on a new line so just like that and also we need to initialize the mox so mokito annotations dot open marks this because otherwise we're going to have a bunch of no pointer exceptions now we need to test so given so the test that we want to test or the method that we want to test is under test dot so when under test buy or i think it's booking next booking code all right so when this is actually invoked so i'm going to say actual here so this is the actual code that we get so when this is when this is invoked we get this code and then we have to perform some assertions now here let's have a look we need to mock this so let's just take this and i'm going to say given and let's import static method from mokito so given the booking repository final dot will return list dot off and these are the bookings okay now let's just extract this to a variable i'm going to say bookings and within bookings we can have new and then booking so id will be one l and local date oh yeah we need to refactor a bunch of things so the naming here but you get a point so local date time dot and then yeah because we can't really control the clock let's just say dot plus and then um 10 minutes and you'll see why in a second so let's just basically put this on a new line so you see everything then i think so the end date so that'll be the same but let's just say i'm not even sure what is that so 50 minutes and this is testing by the way description i'm going to say fool and then here code will be full code and then boolean confirm this will be let's say this is true and then the user i'm just going to pass no because i don't think we use the user in there and then the room will also be no so this is one booking we need the next we need another booking so here let's just take all of this and then put it there and this will be bar so let's just select this one going to be bar and then here so let's just say that this is um 15 minutes so the start date which is now plus 15 minutes right so we are expecting to basically let's just take one other booking so two and then a third booking and this is the final one so just like that and then here i'm gonna say bingo and this will be bingo code not that i play bingo but this id will be three now let's just say that this will be five minutes so we are expecting this to be the actual uh booking right so the expected one oh actually um bingo code i think this is what we're expecting all right so when that happens so given the repository final return the bookings when we try and find the next booking or the next booking code then assert that import static method from assert assertions dot assert that from core api i'm going to say actual so i think it's actual that we have to pass yeah so actual is what we get dot equals to expected well let's just put var in here expected equals to and this will be i think i said bingo code so bingo code just like that and then here bingo oops bingo oh expected right cool so this is our test so let's see whether this works shall we let's just right click and run fingers crossed fingers crossed fingers crossed yes first time oh my goodness it's so satisfying trust me it's amazing when tests just work first time around right so this is the happy path now let's basically have a second one and usually with tests what we do is we just basically copy them around there we go and cannot get next booking code when empty so when the list is empty so when booking or bookings are empty so here we can get rid of all of this and here let's just say list dot off and in fact let me just basically let's just do this bookings there and we're going to basically just remove this so empty list and then here this should be no so i can get rid of that actual is no all right let's give give a go again fingers crossed fingers crossed fingers crossed hooray it's working beautiful stuff uh nice i think one more um i think i think you could have maybe just when it's one but i think this is this is working now what i'm going to do is i'm going to um basically right click and then run with coverage and let me replace active suits in here there we go and all i want to see is let's just go back in here for a second on the class which is being tested and have a look so we have green this is beautiful we have green cool so you can see that we change couple things and we are testing as we change and refract which is what you should be doing so obviously i didn't refactor these other two methods but this is it and have a look um also you have a bunch of other classes department service which i'm not going to go into because as i said i wish i could spend hours and hours and you know refactoring and testing all of this but we've done it we've literally done it so this is pretty much everything for this video let me know what you thought about this video also let me know what you want to see next literally just let me know what you want to see next and comment down below it was fun and um yeah i'll catch you on the next one you

Channel: Amigoscode

Views: 149,596

Keywords: amigoscode, code review, code review best practices, code review in software engineering, code review github, pull request github, pr review best practices, pr review meme, pr review github, java tutorial, java for beginners, spring boot tutorial

Id: BBSQ8ZKTR4c

Length: 43min 18sec (2598 seconds)

Published: Thu Mar 17 2022