Junior Developer Sent Me A PR For Review

แชร์
ฝัง
  • เผยแพร่เมื่อ 13 ต.ค. 2024
  • In this video, we'll take a look at a pull request (PR) submitted by a junior developer for review. As a more experienced developer, it's important to provide constructive feedback to help junior developers learn and grow. We'll walk through the PR step-by-step and discuss best practices for reviewing code changes.
    Whether you're a junior developer looking to learn more about code review, or an experienced developer looking to improve your reviewing skills, this video has something for everyone. Join us as we explore the world of code review and help junior developers take their coding skills to the next level!
    Join waiting list here: amigoscode.com...
    Don't Forget to
    ===========================================
    💯 Subscribe to Amigoscode - bit.ly/2HpF5V8
    💯 Courses Available for free here - amigoscode.com...
    💯 Join Private Facebook Group and Discord - amigoscode.com...
    🙊 Here are the goods for all my videos video 🙊
    ► Recommended Books
    ===========================================
    Clean Code - amzn.to/2UGDPlX
    HTTP: The Definitive Guide - amzn.to/2JDVi8s
    Clean Architecture - amzn.to/2xOBNXW
    ► Computer and Monitor
    ===========================================
    New Apple MacBook Pro - amzn.to/3464Mmn
    Dell 27 INCH Ultrasharp U2719D Monitor - amzn.to/2xM3nW1
    Double Arm Stand Desk Mount - amzn.to/3aYKKfs
    USB C Hub Multiport Adapter - amzn.to/2Jz7NlL
    ► Camera Gear
    =============================================
    Sony ILCE7M3B Full Frame Mirrorless Camera - amzn.to/346QIJn
    Sigma 16 mm F1.4 DC DN - amzn.to/2wbic3Q
    Sigma 33B965 30 mm F1.4 DC DC - amzn.to/39G37Fd
    ► IDE & Tools I use for coding 💻 🎒
    ===========================================
    ITerm
    VsCode
    GoLand
    IntelliJ Ultimate
    Sublime
    P.S
    ===========================================
    💯 Don't forget to subscribe | bit.ly/2HpF5V8
    💯 Join Private Facebook Group and Discord - amigoscode.com...
    💯 Follow me on Instagram | bit.ly/2TSkA9w
    ❤️ Thanks for watching
  • วิทยาศาสตร์และเทคโนโลยี

ความคิดเห็น • 276

  • @amigoscode
    @amigoscode  ปีที่แล้ว +11

    Join the waiting list here folks: amigoscode.com/p/full-stack-professional

    • @nothiiiiiiiing
      @nothiiiiiiiing ปีที่แล้ว

      For me it's better getting 800£ a month and learning programming at a real place than taking a 500$ course

    • @spirothegamer
      @spirothegamer ปีที่แล้ว

      🤣

  • @dimpho.ngache
    @dimpho.ngache ปีที่แล้ว +316

    This got me thinking that maybe interviewing developers by allowing them to review a PR like this and explaining their suggestion will reveal a lot about their competence.

    • @emasmach
      @emasmach ปีที่แล้ว +9

      never thought about that

    • @ProduccionesLukaz
      @ProduccionesLukaz ปีที่แล้ว +16

      I actually got a couple of interviews that were like that

    • @NathanHedglin
      @NathanHedglin ปีที่แล้ว +6

      I've done interviews where I had debug code. Never a proper PR though.

    • @aaqibjavedz2569
      @aaqibjavedz2569 ปีที่แล้ว +5

      Idk its highly subjective i think 💭

    • @MyWatermelonz
      @MyWatermelonz ปีที่แล้ว +20

      No bro, you have to do two sum and reverse the linked list. It's the only way

  • @petroniobonavides3530
    @petroniobonavides3530 ปีที่แล้ว +110

    This is awesome!!!! Watching this video we can see , what a senior developer, think about a junior developer code. And... Paying attention in those tips/movements, we can improve!!!!!! Our code!!! EUREKA!!! Actually, It deserve a playlist

  • @suikast420
    @suikast420 ปีที่แล้ว +60

    6:57 You suggest a RuntimeExcpetion as base class. It's ok at first. But every Project should have at least one base type of it's BusinessException which extends from RuntimeExpcetion. This makes the error handling in the rest controlelers and the transaction rollback handling for specific use cases relay easier.

  • @Elvis-is-king-l3s
    @Elvis-is-king-l3s ปีที่แล้ว +43

    Always good to watch PR reviews but IMO it Would be good to add explanation on top of code changes in the comments otherwise it is hard/impossible to understand why the changes are required/suggested

    • @niklas3128
      @niklas3128 ปีที่แล้ว

      If you have a specific question you can ask here and I'll try to answer. I thought, that his comments and suggestions were very detailed and thought it's because some of his mentee are new to Spring. This video took 20 minutes for a rather short PR. IMO The time a junior dev spends on researching your suggestions is valuable and senior dev has better things to do then explain. Besides, he could also just DM him on DC and ask.

    • @BramHarmsen
      @BramHarmsen ปีที่แล้ว +1

      I was exactly thinking the same thing. I might give too much explanations and sources sometimes. Buy it only takes s couple second to at least at one sentence explaining why the suggested change is required or recommended.
      Other then that great video, useful to see someone else's thought process while doing a code review.

    • @tiskahar9738
      @tiskahar9738 ปีที่แล้ว +6

      @@niklas3128 why even make it possible to generate a DM? That takes even more time than a simple explanation of suggestions. This type of review isn't helpful at all for juniors. They need to learn the *why*s. He also fails to justify his disagreements. "Returning the entity to a create is wrong" is an opinion, and one he fails to justify in any way. The purpose of a senior engineer is twofold: produce code and mentor juniors. If you treat juniors like they're a bother, then you're not doing your job.

    • @ouwyukha
      @ouwyukha ปีที่แล้ว

      or give them this video to watch 😂

    • @blighthornsteelmace820
      @blighthornsteelmace820 ปีที่แล้ว +2

      @@niklas3128 you must be joking. "This is wrong" is not detailed explanation. LOL

  • @videohighlights6066
    @videohighlights6066 ปีที่แล้ว +27

    The code is returning entity via REST, which is BAD. Not sure why wasn't it called out. As the project progresses and the User will have `password` field in it, it will also be returned back via REST. The entity should be converted to DTO.
    Also, Lombok should be used to validate field values. No need to validate them in service layer i.e., if email exist or not. Service layer can be used to validate business logic.

    • @uxweb
      @uxweb ปีที่แล้ว +1

      I agree, returning the raw entity as is could be dangerous if it contains sensitive information. I think that's why a resource layer is needed to transform an entity to a response, within that resource object it can be decided which data from the entity should be returned or serialized

    • @maksym.pavlenko
      @maksym.pavlenko ปีที่แล้ว +1

      I'd prefer simply annotating fields that shouldn't be serialized to exclude them and have a test in place in case someone removes annotations by mistake. It's just that creating DTO in this case produces a lot of repetitive code. Thoughts?
      EDIT:
      come to think about it, in this particular use-case since the data is so sensitive it makes sense to use a DTO without a password, since the risk is too high

    • @videohighlights6066
      @videohighlights6066 ปีที่แล้ว

      @@maksym.pavlenko for simple use case it would work i.e., when you know for sure that password field doesn't need to be serialized. But you would still need some object which can serialize incoming object to a class. For example, a user wants to update the password. If you have put an anootation on password not to serialize, how will you translate this request to an object which can then be persisted?
      In simpler terms, Entitys and VOs/DTOS has different purposes and different logic is applied there. These objects shouldn't be shared.

    • @luisjavierpalacio3019
      @luisjavierpalacio3019 ปีที่แล้ว +1

      To check if the email exists you need to use the repository so you have to perform that validation in the service layer. Nulls and date formats could be validated with just annotations in a dto. Is that right?

  • @jorgeromero4680
    @jorgeromero4680 ปีที่แล้ว +36

    9:13 Actually the HTTP Protocol nowhere states that you shouldn't return the resource back. You can return the created resource in this case "User" as long as you provide the identifier with it.

    • @maksym.pavlenko
      @maksym.pavlenko ปีที่แล้ว +5

      I agree. I'd say it depends on how often the API user will make a follow up GET to retrieve the user data

    • @bardas5
      @bardas5 ปีที่แล้ว

      Hateoas

    • @younekekohota
      @younekekohota ปีที่แล้ว

      i agree

  • @Norua2007
    @Norua2007 ปีที่แล้ว +33

    Also it is a good practice of returnung DTO's instead of entities

    • @bart7176
      @bart7176 ปีที่แล้ว +7

      I'd say it's even more than just a good practice - a must-have, especially when working on users.

    • @slicex3408
      @slicex3408 ปีที่แล้ว +1

      "DTOs" should technically only be used in service to service communication, not as a return payload of a http request

    • @Norua2007
      @Norua2007 ปีที่แล้ว

      @@slicex3408 , rly? So it is better to return a whole entities on GET requests? ))

    • @badripaudel77
      @badripaudel77 ปีที่แล้ว +1

      @@Norua2007 no , never. When we return entity, all the fields are returned which is unnecessary and increases load time. With DTO, we can only return the response we need.
      Create DTO if possible, it also helps to avoid LazyLoading Exception.

    • @Norua2007
      @Norua2007 ปีที่แล้ว +3

      @@badripaudel77 , I know it, but dude above thinks that DTO's are for interservice communication only :)

  • @christopherbryant703
    @christopherbryant703 ปีที่แล้ว +66

    You should do this more often! Very helpful!! Thanks

  • @ogookafor2137
    @ogookafor2137 ปีที่แล้ว +8

    There are no videos like this out there. Please more of these videos. This was very interesting.

  • @caffeinejavacode1475
    @caffeinejavacode1475 ปีที่แล้ว +5

    Review took 15 min
    I can't even imagine how much time it takes to review sophisticated business logic with 15+ pages or more

    • @NathanHedglin
      @NathanHedglin ปีที่แล้ว +2

      If someone's PR is that big, I'll simply reject it. Unless it is a major refactor or something that has to be that large.

  • @Fortuna272
    @Fortuna272 ปีที่แล้ว +7

    As a junior developer, I made note that a lot of the comments/suggestions you left on the PR didn't have explanations why lines should be modified, added or removed. Is this something you talk about at length outside the PR to teach your developers why those changes should be made?

    • @tejeshreddy6252
      @tejeshreddy6252 ปีที่แล้ว +2

      Depends on the person and kind of changes. It is expected that juniors ask questions when something doesn't make sense in PRs. If it's very business specific usually people leave in comments, otherwise you're expected to figure it out or ask

    • @Fortuna272
      @Fortuna272 ปีที่แล้ว +2

      @@tejeshreddy6252 That somewhat reinforces my experience. Tech lead often doesn't explain what's wrong, just tells me to change the code. Conversely, one of my colleagues has been really helpful in learning and teaching the way of work in my company. Being personally proactive has helped a ton as well.

    • @queenofpain6483
      @queenofpain6483 ปีที่แล้ว +1

      It’s not their job to teach you how to approach things. If you happen to have someone that coaches you, then you are lucky. But most of the time they just want to explain things to you so you don’t ask them the same question over and over again.

    • @Fortuna272
      @Fortuna272 ปีที่แล้ว

      @@queenofpain6483 Which is why I ask a lot of questions but I try not to ask it twice. Having a written answer helps because I can refer back to it.

    • @brooksfire9580
      @brooksfire9580 ปีที่แล้ว +1

      As a developer for 30+ years, I wouldn't be this detailed on a PR. I would give a note such as 'Look into changing your mappings. Start with a users mapping, and inside this, create an empty post mapping for registrations and a {id} mapping for users. This would simplify things. Contact me if you have any questions'. Something along those lines. I'd like the junior developer to figure this out and learn something. What will happen when a junior developer sees this PR, they'll just blindly go with his suggestions, not know why. They'll be happy to get an approved PR and move on to something else.

  • @dad9424
    @dad9424 ปีที่แล้ว +4

    Had a little chuckle at the end, "theres no way this PR makes it's way into the main branch.". I work at a major bank, and you'd be amazed at some of the shit that gets yolo merged...
    This video should be a mandatory watch, even for seasoned devs

  • @matheusnico1as
    @matheusnico1as ปีที่แล้ว +15

    I would suggest to remove the code with endpoint comments because swagger can doc the endpoints with more details (request, response, endpoint, etc) but it's not a big deal (I'm sorry if looks like nitpicking, that's not the idea). Awesome video, I'm a developer for 6 years and I'm focusing on leave Brazil and move to UK or Ireland in the next months, I really appreciate your content, thanks for that!

    • @matheusnico1as
      @matheusnico1as ปีที่แล้ว +1

      @@mofobo it’s possible, but most companies are from USA. It can be possible to work from Brazil, but for European companies is more restrict based on my experience

    • @danielj8408
      @danielj8408 ปีที่แล้ว +1

      Boa sorte mano

  • @endieposts
    @endieposts ปีที่แล้ว +3

    At 1:46
    "Develop: In progress
    Architecture diagram: In progress
    Create ERD: Done
    User Stories: Todo"
    Designing and building the project before they have any user stories: it's just like at a real company! :shobon:

  • @aayush481
    @aayush481 ปีที่แล้ว +2

    Thanks buddy, watching code reviews really helps us improve!

  • @saifsushi
    @saifsushi ปีที่แล้ว +2

    the spacing is a company issue not a developer issue. each project should have some form of formatting (ex: prettier) and the dev should have his env set up to format based on what is provided for the project. or just have a pre-commit tool like husky that will auto format the code with prettier. That way no matter what dev touches the code its format stays consistent.

    • @mohamedchehimi98
      @mohamedchehimi98 ปีที่แล้ว

      Use a linter that will be helpfull

    • @maksym.pavlenko
      @maksym.pavlenko ปีที่แล้ว

      from experience, I have pre-commit auto-format disabled since sometimes it doesn't do a good job and there a couple of ways to make it work even by its own rules. But yeah, without possibility to use autoformat, life turns into pain

  • @uxweb
    @uxweb ปีที่แล้ว +2

    Nice video. I would like to know why returning the whole resource is wrong when creating a resource? In my opinion, returning the whole new resource is more convenient, it can save the consumer to submit an additional request to get the resource. Could you elaborate on the wrong argument? Thanks!

  • @deshpasheeto192
    @deshpasheeto192 ปีที่แล้ว +1

    Hiya! Would really like to watch more of such videos where you do a live Review with us. This is really very helpful. Please keep them coming. Thank you

  • @Im_not_scareddd
    @Im_not_scareddd ปีที่แล้ว +2

    This is awesome bro. It was my first time to see code reviewing. It really motivated me bro. tnx a bundle

  • @abu-dukhan
    @abu-dukhan ปีที่แล้ว +12

    Maasha Allah, that's so interesting, even though I'm not a pro but I understand and I'll also take note of these mistakes, I really wanted to grow my java knowledge because when I was in university almost everyone hates Java, and I don't know how Allah makes it easier to me, and following your tutorial really opened my eyes to what Java really is, but still I wasn't able to purchase even one of your course due to our economy situation, but believe me I have almost all your TH-cam videos (Spring boot related) download on my PC.
    And in Sha Allah I'll try to share my work based in what I learnt from just the TH-cam videos, but that'll be after you permit me to.
    I don't have enough words to show my gratitude to you but only Allah will reward you abundantly. JazaakAllahu khairan
    Yunus from Nigeria

  • @sweets7092
    @sweets7092 ปีที่แล้ว +3

    About createUser method. Shouldn't he use some dto / request class rather than entity User class? 👿 Normally we don't want to use entity class as param, right? So something like CreateUserRequest would be preferred 🤔

  • @shaded4438
    @shaded4438 ปีที่แล้ว +1

    On your first point, i personally like spacing rtc, however its down to your lint rules. If your project doesn't have lint rules then its not particularly fair to criticize a junior for not following linting. Its the responsibility of the seniors

  • @uxweb
    @uxweb ปีที่แล้ว

    I agree with the part of not throwing an exception when a resource is not found, unless that exception is handled by the framework and transformed into an 404 http response, otherwise it is more convenient to return a 404 response when the resource is not found.

  • @adambright5416
    @adambright5416 ปีที่แล้ว +2

    It was interesting, but i think your review lack descriptions. Specifically, why? Why do you need space there? Why should it extend this class and not that? How can you avoid this kind of mistakes later? Yes, you answered some of it in the video, but not everything. These kind of "suggestions" will lead to people either blindling doing the same stuff without even thinking, and thus making more mistakes later, when those "suggestions" are no more applicable; or it will lead to a pointless and rather negative argument later. When i was junior at python, i had my portion of blank reviews with "just to it like that" or "change it". Man, how many hours were wasted on fighting that person, before we could understand each other.

    • @adambright5416
      @adambright5416 ปีที่แล้ว +1

      For example, i tend to use type annotations, and the code base was old enough, that was written when they weren't supported. Guy said i must remove them. That's it. Just "remove". Only few weeks later i figured that he was concerned more about code to have the same style everywhere, rather than for it to gradually become more type safe.

  • @ringishpil
    @ringishpil ปีที่แล้ว +2

    Hey Amigos! Greetings from a fellow senior developer. I have to say I completely disagree with how you did the review. Considering this is a fellow junior engineer, all you did is point where he did the mistake and not why he did it. At the end when you suggested to check if email is taken, you should’ve suggested how. Is that a new func? Is it an ON CONFLICT query? And what if its taken? Would you leak out that information to the client?
    I think the most important thing at PR review is the WHY which according to me you failed to deliver

  • @matthiasrudingsdorfer7688
    @matthiasrudingsdorfer7688 ปีที่แล้ว +6

    Since when should a http post request (create) not return the entire entity? You said that the user endpoint should only return the id

    • @tobyjacobs1310
      @tobyjacobs1310 ปีที่แล้ว +2

      Was thinking exactly this. I thought PUT was for creating resources? Also while we're on it, I'm a bit mystified about avoiding returning a user object and instead only returning an id. DTO, sure instead of a bare instance but otherwise seems ok to return a more complete object to avoid additional unnecessary calls ...

  • @codestorm1298
    @codestorm1298 11 หลายเดือนก่อน

    Hi, little suggestion (regarding 7:34 ). We can use a code formatter whatever the IDE we use, right? So those spaces, new line issues can be fixed with that. 😊.
    Anyway good to see these kinds of videos and very interesting how you explain them.🥳

  • @justinf1343
    @justinf1343 ปีที่แล้ว +1

    As someone who’s coded Java since 1999, I don’t follow regarding the ‘no space’ argument between the ) and {
    Just look at the Java source code and it’s ) { and not ){
    That’s a poor example of a PR review. That just pisses your colleagues off and adds zero value to the PR.

    • @satejbidvai
      @satejbidvai ปีที่แล้ว +1

      He told him to add a space not remove it. I think you did a mistake watching the video.

  • @DeGoya
    @DeGoya ปีที่แล้ว +3

    what's the reason for only returning the ID instead of the user object?

    • @loydako6285
      @loydako6285 ปีที่แล้ว

      you have this GET /api/v1/users/{id}
      id + HttpStatus is enough response for user creation

  • @scottserage9022
    @scottserage9022 8 หลายเดือนก่อน

    The thumbnail really worked for this lol, I probably overlook all sorts of bad coding practices and redundant or overly complicated code but that no new line eof drives me bonkers 😂

  • @mohammedyasir6560
    @mohammedyasir6560 ปีที่แล้ว +3

    Assalamu alaikum nelson..You are videos are very useful, Jazhakhalla. Can u put a video about api version and tell some tips for coding like the one you do in code reviews.

  • @gabrieldragone
    @gabrieldragone ปีที่แล้ว +6

    These videos are very good, it would be really cool if they happened more often

  • @gosnooky
    @gosnooky ปีที่แล้ว

    I think at 14:30, it's more semantically correct to throw a "422 Unprocessable Entity" rather than a "400 Bad Request", as it provides more context to the error.

  • @danescucc
    @danescucc ปีที่แล้ว +2

    Thank you! We want more code reviews!❤

  • @andrewawadike7560
    @andrewawadike7560 ปีที่แล้ว

    Kudos Amigo! keep doing code review it's eye opening to silly mistakes we can make in our codes

  • @emasmach
    @emasmach ปีที่แล้ว +1

    that was good! yes, i know it is not very easy to write excellent code, so it is a good idea what you saud in the video and in the code review.

  • @caiolopes7871
    @caiolopes7871 ปีที่แล้ว +3

    Hey Nelson, As-Salamu Alaykum. I don't understand how the groups with projects works, can you explain it?
    I am brazilian software engineer student and this type of project will be amazing for my career. So i am very interesting on this mentorship.

    • @amigoscode
      @amigoscode  ปีที่แล้ว +1

      Join waiting list

  • @SushmithaSathyanarayanan
    @SushmithaSathyanarayanan 3 หลายเดือนก่อน

    Very informative video, would be helpful for junior developers to make all the checks before submitting PR. Can we validate the user data in controller layer, instead of user service implementation for createUser()?

  • @Ogrizkov
    @Ogrizkov ปีที่แล้ว

    Thanks for video, actually pretty fair request for changes in the end

  • @sanskarkaazi3830
    @sanskarkaazi3830 ปีที่แล้ว +1

    You telling about returning the identifier in the HTTP Post request after creating an entity.
    I too return the entity itself . Does it not depend on what the client wants in the response. Could you provide the reason why returning its identifier after the entity being created in the response is a good practice?
    Thank you!! :)

    • @blighthornsteelmace820
      @blighthornsteelmace820 ปีที่แล้ว

      ye, I think it depends. Also some subset of entity data may be needed, not whole.

  • @youssefbouchara1179
    @youssefbouchara1179 ปีที่แล้ว +1

    Why shouldn't the custom exception extend Exception and make it a checked one? what's the advantage of using an unchecked runtimeexception? I'd rather handle it myself

  • @TheDenisTopG
    @TheDenisTopG ปีที่แล้ว +1

    I found it interesting to see this side of software engineering

  • @andrewdremov2835
    @andrewdremov2835 ปีที่แล้ว +2

    would it be a better approach to check for mandatory fields (bad request) immediately in the controller? or maybe it would even be done by spring if you specify the fields in request body object as non-nullable?🤔

    • @blasttrash
      @blasttrash ปีที่แล้ว

      You have check it within controller with manual code. Or you could use some library like hibernate-validator etc which would make it declarative. depends on the team

    • @andrewdremov2835
      @andrewdremov2835 ปีที่แล้ว

      @@blasttrash I thought Spring would automatically respond with 404 if some of the required parameters are missing or have different type🤔

    • @blasttrash
      @blasttrash ปีที่แล้ว

      @@andrewdremov2835 parameters could be part of both queryparams or request body(even GET requests can have request body). But that said, for such requests usually people return 400 bad request, not 404.
      Since the endpoint that consumers hit, actually exists in our app. Its just that they did not form the full request properly(missing fields or blank strings or null values etc)

  • @helpy852
    @helpy852 10 หลายเดือนก่อน

    Review comments should follow a questioning manner rather saying "do this" we can say " can we do this? what do you think? Juniors will take this in a more productive manner. Also complement the good things of the juniors in the code that really boost their motivation.

  • @umer-media
    @umer-media ปีที่แล้ว

    Hey Nelson. I want to take Microservice and Distributed systems course. What are prerequisites for that course.

  • @calebopeyemi2498
    @calebopeyemi2498 ปีที่แล้ว

    I learnt so much from the lesson. God bless you with more wisdom and trying to get one of your course

  • @deadlyecho
    @deadlyecho ปีที่แล้ว +1

    Just to point out also using Nullability annotations and formating rules would be awsome as well.

  • @TathagataDuttaghetegho
    @TathagataDuttaghetegho ปีที่แล้ว

    Hi, am a regular follower of your channel.... Just a question... At 12.46, shouldn't the arrow be -> instead of =>

    • @mitch0210
      @mitch0210 ปีที่แล้ว +2

      Yeah, you're right

  • @patrickbelanger6253
    @patrickbelanger6253 ปีที่แล้ว

    That would be a nice you could make a video about how to deal with PR when developers are challenging your suggestions and comments.

  • @simpleandminimalmaybe
    @simpleandminimalmaybe ปีที่แล้ว

    very helpful, i would to join these cohort once i have my finances in order probably end of next year. I have a question re the deletion of code, is it not safer to mark as @Depracated or i am mistaken. thank you for these sessions.

  • @Daikini0
    @Daikini0 ปีที่แล้ว

    8:11 I would request to change the User entity to have 'id' instead of 'customer_id'. Also if they use liquibase or flyway or equivalent to migrate the data. After months or maybe years, nobody will understand why is it customer_id, or they may mix it with a badly written FK

  • @ionutanghel6337
    @ionutanghel6337 ปีที่แล้ว

    At 5:53, when you added a suggestion for the @GetMapping endpoint, you forgot a “/“.
    Very nice video, tho

  • @reddev149
    @reddev149 ปีที่แล้ว +1

    Wow. This video is amazing, but I have some questions 😁😁
    1. Can you use the @Autowired annotation instead of including "final" in the private UserRepository userRepository?
    2. That is the difference between the two?
    3. When do we use the @Autowired annotation

    • @DeGoya
      @DeGoya ปีที่แล้ว +3

      1. since we use @AllArgsConstructor we're basically doing constructor injection. There's no need to use @Autowired in that case.
      2. There's no difference. You could also just use field injection instead of constructor injection.
      3. When using field injection, multiple constructors, or @Autowired(required = false) when you don't want to autowire certain parameters in your constructor injection

    • @anunymous7
      @anunymous7 ปีที่แล้ว +1

      @@DeGoya thank you!

    • @patrickstorm8334
      @patrickstorm8334 ปีที่แล้ว +1

      @@DeGoya I will add one more thing here
      @Autowired is considered deprecated and it will not be used in future versions and it's good practice is to use dependency injection via Constructors (especially if the project will suffer an version upgrade in the future)
      I usually do it via @RequiredArgsConstructor

    • @DeGoya
      @DeGoya ปีที่แล้ว

      @@patrickstorm8334 correct! you can even use
      @RequiredArgsConstructor(onConstructor = @__(@Autowired))
      When you want to use Lombok, but only autowire certain variables, that are declared with final static.

  • @sebon11
    @sebon11 ปีที่แล้ว

    This video is really great! Would love to see more content with senior commenting on junior code.

  • @ganjargingintahyudin9774
    @ganjargingintahyudin9774 ปีที่แล้ว

    you're such a great mentor! really appreciated

  • @Chris-qg6kc
    @Chris-qg6kc ปีที่แล้ว

    I'm on your list and in a fb group. I wonder why I never heard or got notification of your group project?

  • @bertrodgers2420
    @bertrodgers2420 ปีที่แล้ว +1

    Why would a user not existing throw an error, wouldn't a simple optional type be better here

  • @ukaszkiepas57
    @ukaszkiepas57 ปีที่แล้ว

    Thanks a lot. Keep on this series. Extremly helpful

  • @patrickstorm8334
    @patrickstorm8334 ปีที่แล้ว +4

    Quick question shouldn't the API signature be (according to REST)
    /users/user/{id}
    instead of
    /users/{id} -> this case would suggest for me a list of users not a single user
    And I agree with people who said such videos of code reviews are indeed a good idea
    and a very good practice especially for beginners (but not only) so they can follow certain guide rules - we also can discuss certain points covered by you in the comments and everybody can bring their input from their practice (in that way people may learn something new and the way they were doing things maybe is not the most correct or recommended)
    Big thumbs up for this video

    • @-Chris_
      @-Chris_ ปีที่แล้ว +2

      No, according to rest /users/id is right

  • @uxweb
    @uxweb ปีที่แล้ว +1

    The part of making the repository throw an exception when it don't find the user seems excessive to me, it would be similar to a database returning an error when your query didn't return any records. In my opinion it would be more convenient to return a value that represents the absence of a result, like a null object or value.

    • @klickgeil8442
      @klickgeil8442 ปีที่แล้ว

      No HTTP and Rest exist so the appropriate stuscode should be used.

  • @ZeePintor
    @ZeePintor ปีที่แล้ว +3

    Honest question: why use Long with user id over the user object?

    • @bylatt
      @bylatt ปีที่แล้ว +1

      +1, still don't understand why returning back user object is wrong.

    • @nithishkumar6641
      @nithishkumar6641 ปีที่แล้ว

      Return user Object is not a wrong. User Id enough for future reference that's why he suggest to return userid (Long)

    • @nilavkhatiwada2076
      @nilavkhatiwada2076 ปีที่แล้ว +4

      In my view using just long with userId have many advantages:
      1. is more secure in the sense it doesn't need to provide whole user data to the client by limiting the user information
      2. If clients wants then client can use the id to retrieve the userInformation. So it gives more control to the client.
      3. Also, in some case the user object might be big or contains lots of data. So returning whole object might effect the performance.

    • @nithishkumar6641
      @nithishkumar6641 ปีที่แล้ว +1

      @@nilavkhatiwada2076 Good Explanation bro👍

    • @viktormykhasko9406
      @viktormykhasko9406 ปีที่แล้ว +1

      If you use a user object as result of POST , on next step -> you can compare result with expectation in Int Test (TDD) and can check controller/service/repository and all logic is right, if you use only id you can check only status 2XX or 4XX and all logic in repository you should check by handle ...

  • @Bloome_BlueFolf
    @Bloome_BlueFolf ปีที่แล้ว

    As Junior this was very interesting to watch :)

  • @tienta8053
    @tienta8053 ปีที่แล้ว +1

    Can you tell me why not return user object but return user id?
    With the request to return the newly created user information, how should it be handled?

  • @hampusandersson9375
    @hampusandersson9375 ปีที่แล้ว +1

    I've just built my first Spring Boot Backend and this was incredible! Why is it wrong to return the user when its created? Appreciate the content!
    Edit: Is it ok to return an ObjectID?

    • @AiwaLee
      @AiwaLee ปีที่แล้ว +2

      Also interested in the response here. He says it's wrong but doesn't provide a justification. That's nonsense. Returning the Entity it perfectly fine imo. Unless he means that you need to define a DTO and not return the object itself, in which case he is not being clear in his commentary.

    • @SegNode
      @SegNode ปีที่แล้ว +4

      Probably don't need all of that data to be returned, just returning the ID is simpler.

    • @haruntrako794
      @haruntrako794 ปีที่แล้ว

      @razermoon I unterdtanded it that way also

    • @hampusandersson9375
      @hampusandersson9375 ปีที่แล้ว +1

      @@SegNode thank you for the response, had that interpretation from his explanation aswell! But still sparked my interest!

    • @TsukiCTF
      @TsukiCTF ปีที่แล้ว +1

      I'd return just an ID, as well. Or a DTO or an abstraction to the real User object. The client usually doesn't need to know every field of the User.

  • @acyutaanime1983
    @acyutaanime1983 ปีที่แล้ว

    Hello, the idea of building a full blown application is amazing. I’d love to be a part of this, please let me know if you need one more Java Developer in the team. Regards!

  • @mhasan-cse
    @mhasan-cse ปีที่แล้ว

    Learned so many things, Jazakallh Nelson

  • @kamilczubaszek6850
    @kamilczubaszek6850 ปีที่แล้ว +1

    There was important thing not mentioned, AllArgsConstructor for private final should not be used, it's should be RequiredArgsConstructor, i personaly prefered just make constructor

    • @DeGoya
      @DeGoya ปีที่แล้ว

      I like to do this one
      @requiredargsconstructor(onconstructor = @__(@autowired))

    • @kamilczubaszek6850
      @kamilczubaszek6850 ปีที่แล้ว

      @@DeGoya if You have one constructor there is no need for that

    • @DeGoya
      @DeGoya ปีที่แล้ว

      @@kamilczubaszek6850 yes, but sometimes you want to use regular variables, instead of Autowiring them when using Lombok

  • @harshwardhanparmar8258
    @harshwardhanparmar8258 ปีที่แล้ว

    Amazing video, it gave me clear idea about how code reviewer will think. It will help me alot, Thank you.

  • @benedictcumberbatch8801
    @benedictcumberbatch8801 ปีที่แล้ว +3

    If we are returning id instead of entire resource but we care about performance, wouldn't it be a problem if we make two API calls instead of one?

    • @leoperrin7771
      @leoperrin7771 ปีที่แล้ว +1

      Actualy it depend of the next steps. If you need to display or edit the entity after creation, it's better to return the full entity. But if you don't, the id is lighter. 2 endpoints can be a solution

  • @pouyan021
    @pouyan021 ปีที่แล้ว

    I think the service could be an implementation from the start as well, why should the service be an interface? It's against the YAGNI principle

  • @konstantinoskokkorogiannis4756
    @konstantinoskokkorogiannis4756 ปีที่แล้ว

    Great video, can we have please some more details about your mentorship program and maybe a description of the projects we can contribute?

  • @Slayz38
    @Slayz38 ปีที่แล้ว +1

    I do follow you and like your videos. However, I have to disagree on some points like no explaining the reasons of the change (you are explaining them on the video, but the PR author needs to know them too)
    Additionally, we need to be as objective as possible and not try to enforce our own style of coding.

  • @ibrahimhlc
    @ibrahimhlc ปีที่แล้ว

    what about creating a video about the testing you mention in the last? thanks.

  • @nicholascameron_tf
    @nicholascameron_tf ปีที่แล้ว

    i love these videos. You are an excellent teacher. Thank you

  • @derekmorgan110
    @derekmorgan110 ปีที่แล้ว

    Thank-you, this was so helpful. I agree with everyone else, this idea would make an awesomely helpful playlist.

  • @calebopeyemi2498
    @calebopeyemi2498 ปีที่แล้ว

    I'm a junior Dev (Java) need a video on runtime exception,custom exception and so on. The chain that connect this exceptions together.thank you

  • @gzoechi
    @gzoechi ปีที่แล้ว

    Code formatting issues like missing spaces in code reviews seems quite odd. The pipeline should fail on such issues and the developer can fix these before someone reviews the code.

  • @saronistu
    @saronistu ปีที่แล้ว

    I was curious about the thumbnail. What's the deal with the end of the file line?

  • @justanaveragebalkan
    @justanaveragebalkan ปีที่แล้ว +1

    Alot of the mentioned advice is really obscure, not a lot of details to instruct your collogue, vague description of problems that are complicated for a junior developer. From architecture point of view this project seems like a huge mess, no wonder people are writing stuff without even considering the issue. Which senior developer decided it's a nice idea to have a UserService that calls a UserRepository, why would you do validation inside a service, validators should be decoupled from entities in any scenario and unless that user service is doing anything different then basic crud you can probably just leave it to the repository.
    Your criticism is valid, all the User nonsense and exception handling was indeed wrong but as a senior developer you should be able to go into details regarding the issues because they are apparent to you, not the case for someone that has no experience overall, just saying Y should be X doesn't cut it, try to give a reasoning for why it's wrong, otherwise code reviews make no sense they are implemented to learn from the mistakes not to point the fix.
    If a person doesn't understand the problem, there is no point understanding the solution and it does more harm than not telling them at all. I personally really believe that the person that wrote it should be the person that fixed it and never provide code snippets to PR request but would rather spend some time either in person or over a call with the developer so I can see that he truly understands what's going on.

    • @blighthornsteelmace820
      @blighthornsteelmace820 ปีที่แล้ว

      Isn't service layer the proper stage for business logic and validations before save?
      Where it should be done instead?

    • @justanaveragebalkan
      @justanaveragebalkan ปีที่แล้ว

      ​@@blighthornsteelmace820 Certain things such as Entity related validations can be done in the entity itself, before submit, they have no place inside the abstraction of the repository.
      Most ORMs have methods that can be overridden for that purpose even sprint boot has a validator extension that you can use to mark an object and it even allows your code to skip going to the service layer before the request comes in.
      For stuff like common validator in case checking for a string format e.g email, phone number, vat number ect. You can abstract those into another service and inherit the implementation where you need it, this way you wont replicate code all over the place and your repository will do what it has to do, basic CRUD. That's from the S in SOLID, one method should serve one purpose and do one thing.
      Imagine having 50 fields on an entity, and you going if else if else 50 times in the repository to validate the entity? even with short hard syntax in any language those are 50 lines of code a best that just sit there for no practical reason.
      It will also allow you to test your validators separately from your services to make sure they function properly on each new release.

  • @almeidaGGG_
    @almeidaGGG_ ปีที่แล้ว +1

    why put the service and repository as "final"?

  • @muurimc
    @muurimc ปีที่แล้ว

    This was great video. Would like to see more PR process related videos.

  • @hajaressaidi7021
    @hajaressaidi7021 ปีที่แล้ว

    Hello, amigoscode family, how can I join the mentorship please 🙏😊

  • @emasmach
    @emasmach ปีที่แล้ว

    Can you do the same with PRs written in other languages? I would be very interested in watching a video about code reviewing python code. Maybe a PR for a project built with django.

  • @SplashDmg2011
    @SplashDmg2011 ปีที่แล้ว

    Returning the entity in Controller is bad practice, you should map it to DTO instead and return the DTO.

  • @RizwanAli-xt7mr
    @RizwanAli-xt7mr ปีที่แล้ว

    Sir please mention why you do some changes I am talking about technical reasons as you removed User return type as long.

  • @feloniousmonkey
    @feloniousmonkey ปีที่แล้ว

    Consider not versioning your controllers to version your apis...so remove /api/v1. Api versioning is a broad topic but there are proper ways...

  • @NikoWicaksono
    @NikoWicaksono ปีที่แล้ว

    Brother please doing video that why we should stick with java instead of kotlin

  • @glaze4629
    @glaze4629 ปีที่แล้ว

    I'm new to this world of git, it seems like a lot of changes, is it any rule of how long a PR should be?

    • @blighthornsteelmace820
      @blighthornsteelmace820 ปีที่แล้ว

      just keep in mind that if PR is very big, then noone is willing to read all that ;)

    • @MarcoS-mx1vj
      @MarcoS-mx1vj ปีที่แล้ว

      @@blighthornsteelmace820 Well i already met developers, who spent hours on code reviews for large PRs :D

  • @Billy_98
    @Billy_98 ปีที่แล้ว +1

    Amazing job!

  • @ThanHtutZaw3
    @ThanHtutZaw3 ปีที่แล้ว

    so senior should know the right code , without running it . What if senior missed code mistake in PR review and error happened in production ?

  • @Akash-xv5sk
    @Akash-xv5sk ปีที่แล้ว

    How to append an image in existing form for example i have multiple input fields and I'm doing post method

  • @abdulfatorma348
    @abdulfatorma348 ปีที่แล้ว

    Good afternoon sir, after the completion of the course is the certificate accredited with any college in England
    Please reply me sir

  • @gorandev
    @gorandev ปีที่แล้ว +2

    And it's Cyprus, not Cyrous :)

  • @Talaria.School
    @Talaria.School ปีที่แล้ว

    Interesting content I would have préfèred you handle error using problèmdetails of the rfc 7807 to get a formatted and i18n errors response instead of delegating this to obscure plumber with indigest stack trace error.

  • @programalol
    @programalol ปีที่แล้ว

    Me gusto mucho este video, se deben de seguir las mejores practicas de ser posible.

  • @user-tk2jy8xr8b
    @user-tk2jy8xr8b ปีที่แล้ว

    Returning `User` instead of a user ID (why is it `long`, BTW? smells like `primitive obsession`) from POST makes sense if there are default or computed values to save on a subsequent call to GET. However, in this case the `new user` model seems to be matching the `existing user` model, so yeah, not much sense.

    • @blighthornsteelmace820
      @blighthornsteelmace820 ปีที่แล้ว

      returned data was nothing new beside customer id either.

    • @user-tk2jy8xr8b
      @user-tk2jy8xr8b ปีที่แล้ว

      @@blighthornsteelmace820 this may be not so in general or evolve with time

  • @shekarroy5044
    @shekarroy5044 ปีที่แล้ว

    don't we flag wildcard imports in PR review? i remember there was wildcard import in this PR

  • @federico42o
    @federico42o ปีที่แล้ว

    this kind of video is very useful, i learned a lot, thanks

  • @ashwinraghunath4453
    @ashwinraghunath4453 ปีที่แล้ว

    why was 'throws UserDoesNotExistException' removed from controller ?