Junior Code Review: Cleaning Up Laravel CRUD

แชร์
ฝัง

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

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

    Hello good day. I am a SAP Developer with more than 10 years of experience. And I have started with laravel 1 year ago. When I'm developing in Laravel, I don't have "logic" issues, but "best practices" issues. And I get stuck trying to find the best way to do something.
    Your videos are helping me a lot in that matter.
    I find your trainings very interesting and helpful.
    Thank you.

    • @steamkocheaterreport
      @steamkocheaterreport 2 ปีที่แล้ว

      Im on same page. Constantly asking myself "is this the best way to do this?" when code already works... Another one being absolutely terrible documentation, not the never ending texts for reading like lara-docs provide but actual documentation for functions available. And yet another one being terrible code suggestion support where i know function exists but vscode or other editor just dont suggest a thing.

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

    All of the review are so amazing, never get bored when watching your video

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

    This is a great series, I get so much value from it. Also, I do like the idea at the end of focusing on a specific type of refactor on each episode. That sounds like a fantastic direction to go.

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

    Thank you, Povilas. Your code review videos are very educational.

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

    Hey this is my student! In my opinion he is pro not junior :) I’m glad that I have learned him so much. Greetings from pingdevs dot com, Macedonia.

    • @NB-ph6cv
      @NB-ph6cv 3 ปีที่แล้ว

      Bravo!

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

    repetition is the mother of learning ))

  • @bijportal5643
    @bijportal5643 2 ปีที่แล้ว

    My New favourite channel on TH-cam 🔥🔥 especially these code reviews 🙏🏾

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

    Another thing I noticed very briefly:
    Unless I missed another use of $staticpages in the homepage view that needed all staticpages, the loop around $staticpages was calling ->take(4) which means they were querying ALL static pages, but only needing 4.
    If that is the case, you should not be using a straight up all() and instead be using ->limit(4)->get() or ->take(4)->get()
    Getting all then only getting 4 from there is VERY expensive on the database. And databases in scaled applications are often not on the same server as the web server, meaning there's a larger load on the network now.

    •  3 ปีที่แล้ว

      I saw that too. Also, he/she's doing select * and use only 2 property in the view.

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

    because your videos... php storm is become my next IDE :D

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

      Not worth the money, I’m all for Sublime.

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

      Hell no, IDE >>>>>>> Text editor (even with ton of plugins)

    • @themrflibbleuk
      @themrflibbleuk 3 ปีที่แล้ว

      I will say for the low cost of Sublime, I get very close to an IDE at a much lower cost.
      You can also argue, without every IDE function you learn and remember more.

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

      PHPStorm is the best IDE. It has a seemingly endless number of features. They add more all the time. The more you use it the more you start using the added functionality. Sublime is good but only if you want to save money. If you really want to save money use Notepad++ :)

    • @themrflibbleuk
      @themrflibbleuk 3 ปีที่แล้ว

      @@Steviec63 Notepad++ and Sublime are worlds apart.

  • @alicenNorwood
    @alicenNorwood 3 ปีที่แล้ว

    You'll probably be a man of the year in a laravel community soon

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

    Quick note about the first issue, rather than having to rename the class and file name, you could just anonymise the migration class: `return new class extends Migration`. I believe in newer Laravel versions this is actually the default so my note may be a bit outdated 😅

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

    cool I didn't know about the spatie media library ! Thanks for that ! ^^ I will use it for one of my project !

  • @Steviec63
    @Steviec63 3 ปีที่แล้ว

    Thank you for the Spatie Media Library tip!!! I've just watched their tutorial. It is amazing.

  • @Дольган-т4щ
    @Дольган-т4щ 3 ปีที่แล้ว +4

    Hi from russia. Its very useful, especially for me.
    Keep it up, your doing well 👍🏻

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

    On 12:40 he could use inside the function (Request $request, User $id) instead everytime to FindOrFail the user id

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

    Thank you for video. 9:36 you deleted "check.user:Admin". But all users still can register and right a way have access to admin panel!!! Should to prevent other user to have registration on system or not delete "check.user:Admin" middleware ;)

    • @doremidon
      @doremidon 3 ปีที่แล้ว

      For this actioon code like " Auth::routes(['register' => false]); "

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

      Hm, interesting, I was debating it with myself, because there are no actual routes for non-admin users, so it doesn't make sense to have admin routes/middleware without non-admin routes.
      But technically, I agree with you, you're right.

    • @mctiggaz
      @mctiggaz 3 ปีที่แล้ว

      @@LaravelDaily during the class I’d wanted my students to learn about middleware also so keep it mind that this student switched his carrier from to programming. For only 6 months he learned everything and made this cms for only 2 months. He is good! :) I wish him successful carrier.

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

    Povilas your job is awesome! You could do a more advance code review too? A thank you from Brazil!

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

      I sometimes do advanced reviews, thanks for kind words :)

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

    latest() orders by created_at date, not by ID. In this case it might be fine but generally it's wrong. It should be latest('id') instead.

    • @munthirzikre2401
      @munthirzikre2401 3 ปีที่แล้ว

      I think the result is the same for both

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

      @@munthirzikre2401 In this case yes. But what if they create a record with `created_at` in the past, for example. If 2 bits of code return the same value doesn't mean they work the same, you are just lucky enough at the moment.

    • @A1s2d341673214
      @A1s2d341673214 3 ปีที่แล้ว

      The created_at and id, follows the same order. I guess.

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

    8:36 its shorter way to set this variables, you may use Constructor Property Promotion (new in php 8):
    public function __construct() {
    private $settings = Settings::findOrFail(1);
    }

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

    6:48 he selects all static pages but as i understand method take he only uses 4 of them? So he should select only 4 static pages in controller, not all.

    • @JouvaMoufette
      @JouvaMoufette 3 ปีที่แล้ว

      Yeah there's a big big difference between ->take(4)->get() and ->get()->take(4) and it'll be especially noticeable in production as the product grows and has more entries, and starts doing things to scale like having a database on a separate host and now all of the records have to come back as network traffic

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

    What Laravel admin dashboard would you recommend? I've used Vuetify in the past, and I liked it. I was looking for something more lightweight, but with VueJS functionality.

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

    Thank you!

  • @LoganathanNatarajanlogudotcom
    @LoganathanNatarajanlogudotcom 6 หลายเดือนก่อน

    Thanks sir

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

    0:41. I like to follow the RoR-way of naming migrations. E.g., if I want to add the field `name` to the Contact model, I will call the migration AddNameToContact. That way, there should be no collisions, and the file name very clearly describes what the migration does.

    • @andreich1980
      @andreich1980 3 ปีที่แล้ว

      Unless you add a field then decide to delete it then add again ;)

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

      @@andreich1980 Haha true! Guess I haven't had that need yet :)

  • @pvaitonis
    @pvaitonis 3 ปีที่แล้ว

    Great idea, nice job.

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

    cool I didn't know about the spatie media library ! Thanks for that ! ^^ I will use it for one of my project !
    I would like to know if you are able to talk also about scoping in Laravel. I've been in a company that have a senior developer, and use a loooot of scopes through the hole project. And he never uses repositories (just one or two), even so it was fully needed because of the bunch of requests he made in model.
    I like the repository design pattern for complexe relationships, I often use repositories (when it's necessary of course). But do you have a simple example when using scopes, when using repositories ?
    And by the way, could you tell also what is the difference between a trait and a helper ? ^^
    Thanks for reading me, and keep going, I love to watch your videos :)

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

      1. Scopes and repositories are kind of different things, two different topics, you can use scopes without repositories, repository pattern is misused very often in Laravel and therefore I don't like it very much. Search my channel for "repositories" for a few videos.
      2. Trait examples from my new project: laravelexamples.com/tag/traits
      Helper examples: laravelexamples.com/tag/helpers
      From those, you should understand/feel the difference.

  • @sakchanno601
    @sakchanno601 3 ปีที่แล้ว

    I' m realy like your tutorial.

  • @enigmatics-lives
    @enigmatics-lives 3 ปีที่แล้ว

    The ‘=‘ argument in where() is not required, but occasionally not writing it may cause an error, that will be hard to debug. If search term in where will be “=“ it will cause an error of Builder, for not writing the third parameter. Of course, it will happen with input from user more likely, but, who knows.

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

      if the table created by laravel migration the '=' may not be needed. but if have table before, the '=' are very needed. based on my experience. CMIIW

  • @adebajooluwaseyi2124
    @adebajooluwaseyi2124 3 ปีที่แล้ว

    wonderful video

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

    Hi!
    I'm enjoying all your coder reviews.
    Question with validation: I started to do things with LiveWire. In the docs of LiveWire "You might be wondering if you can use Laravel's "FormRequest"s. Due to the nature of Livewire, hooking into the http request wouldn't make sense. For now, this functionality is not possible or recommended."
    What do you think? Is more secure or has better performance do the backand things without Livewire and use FormRequest and validated type of checking the Request data or can I do with LiveWire without any issues on long term?

    • @LaravelDaily
      @LaravelDaily  3 ปีที่แล้ว

      It's a personal preference, so whatever you prefer, depending on how you want to use Livewire - as a full-page component, or just as a part of the form for some fields. There's no security/performance difference, it's about structuring Livewire components how you want.

    • @GergelyCsermely
      @GergelyCsermely 3 ปีที่แล้ว

      ​@@LaravelDaily Thanks. For me is more elegant your solution, but is not supported from LW. :(

  • @codecreeper
    @codecreeper 2 ปีที่แล้ว

    thanks

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

    Call a function in array declaration is not a good idea

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

    find(1) to First()

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

    Hey can you make a video about Porto pattern (apiato) ? Thanks

    • @LaravelDaily
      @LaravelDaily  3 ปีที่แล้ว

      Not in plans currently, sorry.

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

    hello .. what's do you use to fill de forms with data? thanks

  • @jersoncarin1952
    @jersoncarin1952 3 ปีที่แล้ว

    nice explaination

  • @sanchopansa7732
    @sanchopansa7732 3 ปีที่แล้ว

    Probably it's worth noticing that during development it doesn't make sens to make additional migrations just to add a new field in the table. It wouldn't make more sense to just add those fiels in the original migration and be done with it? PS I am just starting with laravel so please correct me if I am wrong.

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

      If you're working alone by yourself and locally, and if you can afford to run "php artisan migrate:fresh" on every change, re-creating the whole database, then yes, it's ok to edit older migrations.
      But as soon as you have real data, or you deployed project on some other server where other developers work, it's better to create new migrations.

  • @llBestBoyll
    @llBestBoyll 2 ปีที่แล้ว

    gg man 🔥👌🏼

  • @winmodif
    @winmodif 3 ปีที่แล้ว

    Thx for the video! Could you or someone please help me with the following; I have a 'catch all route' with "->where('menu', '.*')" and in my controller i'm loading in a bunch of variables though not all variables are nessecary for each single page. It depends on certains fields and/or relationships. I'm pretty sure there is a way to conditionally load in variables based on that. Could you point me in the right direction please?

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

    I liked the admin theme, What is it?

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

      if im not mistaken, its material design bootstrap

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

      Material design bootstrap, free version.

  • @devsbuddy
    @devsbuddy 3 ปีที่แล้ว

    Actually I create trait "CanUpload" in every project for file upload and use whenever and wherever I need it.

  • @mohammedrizwan8123
    @mohammedrizwan8123 3 ปีที่แล้ว

    3:57 which chrome extension is it??

  • @shreydadhaniya2587
    @shreydadhaniya2587 3 ปีที่แล้ว

    Can anyone explain me that how can i set up project2.test url for my project

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

      It's done locally by my Laravel Valet server

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

    PHPStorme theme ?

  • @empty7999
    @empty7999 3 ปีที่แล้ว

    Hello Mr Povilas , first thank you so much for your great content , my question is do we need csrf token when using LiveWire ?

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

      We don't need to do anything with it. It just works.

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

      @@andreich1980 Thank you so much Andrew ✌🏻

  • @1234matthewjohnson
    @1234matthewjohnson 3 ปีที่แล้ว +1

    Thank u very much. Can i ask what SQL tool you use?

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

      Sequel Ace, I believe. github.com/Sequel-Ace/Sequel-Ace

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

      +1

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

      Check the DataGrip made by Jetbrains. I think it is the best from the MySQL admins what i seen before.

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

      Sequel Pro

  • @anisurrahman6634
    @anisurrahman6634 3 ปีที่แล้ว

    nice

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

    The email address of the guy is in the address bar...

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

      Yeah, missed it, edited it out now. Thanks for the notice.

  • @rehabragab5937
    @rehabragab5937 3 ปีที่แล้ว

    Great tutorial,may I send my code for review?

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

      Currently I'm done with junior code reviews for now, but I will start PUBLIC code review program next week. So watch the new videos on this channel on Monday-Tuesday and you will get all the details.

  • @necmttn
    @necmttn 3 ปีที่แล้ว

    Awesome content, Thank you!
    Would be nice to have an example for the Laravel project which has both blade front end and also sanctum CRUD API access from other places.
    public function store(CreateJobRequest $request)
    {
    $job = $request->validated();
    $job = new Job($job);
    $job->author()->associate($request->user());
    $job->save();
    if($request->expectsJson()) {
    return $this->success($job);
    }
    return redirect()->route('job.show', $job->slug);
    }
    right now i'm doing this but not sure it's the best way of doing it or not ?

    • @LaravelDaily
      @LaravelDaily  3 ปีที่แล้ว

      It depends on the project, but in my experience, if the project is both web and API, they should be separate in different controllers, web and API ones. But again, it depends on the project, if your method works for you, use your method.

  • @dfordemo981
    @dfordemo981 3 ปีที่แล้ว

    2:53 what is the name of this mysql software?

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

      Sequel Ace, I believe. github.com/Sequel-Ace/Sequel-Ace

    • @dfordemo981
      @dfordemo981 3 ปีที่แล้ว

      @@travholt thanks, i'm windows user 🙁🙁

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

      Sequel Pro

    • @dfordemo981
      @dfordemo981 3 ปีที่แล้ว

      @@LaravelDaily thanks

  • @alila3883
    @alila3883 3 ปีที่แล้ว

    good

  • @sloppyprogrammer4373
    @sloppyprogrammer4373 2 ปีที่แล้ว

    find the declaration of $data = [ ]; in each controller quite pointless when you can just do something along the lines of view('view')->with([ '' => '']) or view('view', [ '' => '' ])
    It's not really proper handling of (large) view/result/response data collections anyway, but if you must really make logic like this just keep it simple and pass all finals inside the ->with() method or directly inside the view() $data parameter, instead of declaring a variable you're never going to modify in the first place.
    I think it's bad altogether to generate a lot of response arrays inside the controller, while usually you can segregate this into a defined object, but in some situations something along the lines of this is acceptable since it's easy to read and easy to identify it being a final/result collection of data. Also it is accepted for very generic controllers that have next to no logic to split up into an object of it's own and where you want to keep this logic inside the controller since it can invoke unnecessary complexity if you decide to OOP when you don't have to OOP.
    // Note don't declare a variable for result collections if it's behavior won't change
    $data = [
    'posts' => Post::all(),
    'meta' => MetaData::render(),
    // ... many other things
    ];
    // While scanning the code I'm expecting something to happen here, but nothing happened...
    return view('view', $data);
    // A little bit better.. It is short, easy to read : but keep track of the complexity of your controller and consider making a service
    return view('view', [
    'posts' => Post::all(),
    'meta' => MetaData::render(), // Shouldn't be served from here, consider view::share()
    // ... many other things
    ]);

  • @donthatedonate
    @donthatedonate 3 ปีที่แล้ว

    Hey! What Phpstorm theme and color scheme is this?

  • @iamriwash7943
    @iamriwash7943 3 ปีที่แล้ว

    He can put that setting in provider using blind

  • @Paltibenlaish
    @Paltibenlaish 3 ปีที่แล้ว

    is not good to do ::all(), maybe some eager loading ?
    optimization problems ahead ....

  • @marcusgaius
    @marcusgaius 3 ปีที่แล้ว

    While you are definitely providing useful information to many, I think you shouldn't be doing QA for code that hadn't been tested even once.
    Speaking of site settings/config, I would appreciate your take on making such a package, or if you already have one in mind, could you mention it in a reply, please.
    Keep up the good work. 👌🏻

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

      Why shouldn't I be doing QA for the code if a person asked for it himself?
      Regarding settings/config, this one is good: github.com/spatie/laravel-settings

    • @marcusgaius
      @marcusgaius 3 ปีที่แล้ว

      ​@@LaravelDaily My bad, I guess I had a different idea about what the series was about. The information provided is very valuable, I just think that you had to say "I won't repeat myself" about different things too many times in this video.
      That to me indicates the time used on making this video could've been better spent, but that's just my perception of it. And the impression I got from your reactions. NHF, of course.
      Thank you very much for the link.

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

    I'm working with some legacy code right now (procedural php that's 15 years old, not laravel) and the programmer not only put retrieved all data and put it into variables, he put them into variables named differently than the data being put in so signTypeID is put into modelID, then he checks the data and puts it into an array in an array element named model. That's then put into a form element named signID, when it's saved it's put into a json variable named modelNumber and at the receiving code it's renamed multiple more times. I have no way to know what data I'm working with at any time. That's just a single actual example, he did this with practically every pace of data pulled out of the database. I actually had one function that was over 3000 lines of spaghetti code littered with renamed data, if thens, repeated manipulations just copy/pasted instead of calls to a single function that did the work, no case switch statements where they were obviously begging for them. It's actually kind of impressive that it even worked, though it worked poorly. The sound of...let's call it frustration...coming out of my office is pretty constant. Don't do it people, for the sanity of the guy who has to come after you and fix your code.

  • @rajabhishek2936
    @rajabhishek2936 3 ปีที่แล้ว

    Awosem content

  • @olehtalanov6697
    @olehtalanov6697 3 ปีที่แล้ว

    You should not rename migrations this way!

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

      What would you have done in my situation, where migrations just don't work and are in conflict?

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

      It would be helpful if you explained why.

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

      +1 Explain please

    • @out_fur_blood
      @out_fur_blood 3 ปีที่แล้ว

      Migrations are also stored in the database, so if you change the file name, you break the migration I think.

    • @karlebh
      @karlebh 3 ปีที่แล้ว

      @@out_fur_blood if you refresh your database, the migration files get rewritten.

  •  3 ปีที่แล้ว

    3:56 he/she is not validating.

  • @micosair
    @micosair 3 ปีที่แล้ว

    It feels to me like the junior is kinda of an a-hole,trying to get you to fix his sht or just trolling to waste your time.