[Laravel] Write better code
After reviewing the code for the new team members, I found long/wrong codes. I write it down here.
1. Use default params when getting value from request
2. Use Eloquent when function
3. Use Eloquent updateOrCreate
4. Use map instead of bundle if/else
5. Use collection when you can
6. Stop calling query in loop
7. Stop printing raw user inputted values in blade.
8. Be careful when running javascript with user input.
1. Use default params when getting value from request
$search_type = SearchTypeConstant::TITLE; if($request->has('search_type')){ $search_type = $request->get('search_type'); }
$search_type = $request->get('search_type', SearchTypeConstant::TITLE);
2. Use Eloquent when function
$query = User::active(); if($first_name = $request->get('first_name')){ $query = $query->where('first_name', $first_name); } $users = $query->get();
We can remove temp $query variable
$users = User::active()->when($first_name = $request->get('first_name'), function($first_name){ return $q->where('first_name', $first_name); })->get();
3. Use Eloquent updateOrCreate
if($user->profile){ $user->profile->update($request->get('profile')->only('phone', 'address')); } else { $user->profile()->create($request->get('profile')->only('phone', 'address')); }
$user->profile()->updateOrCreate([], $request->get('profile')->only('phone', 'address'));
4. Use map instead of bundle if/else
function get_status_i18n($status){ if($status == STATUS::COMING){ return 'coming'; } if($status == STATUS::PUBLISH){ return 'publish'; } return 'draft'; }
private const MAP_STATUS_I18N = [ STATUS::COMING => 'coming', STATUS::PUBLISH => 'publish' ]; function get_status_i18n($status){ return self::MAP_STATUS_I18N[$status] ?? 'draft'; }
5. Use collection when you can
function get_car_equipment_names($car){ $names = []; foreach($car->equipments as $equipment){ if($equipment->name){ $names[] = $equipment->name; } } return $names; }
function get_car_equipment_names($car){ return $car->equipments()->pluck('name')->filter(); }
6. Stop calling query in loop
$books = Book::all(); // *1 query* foreach ($books as $book) { echo $book->author->name; // Make one query like *select \* from authors where id = ${book.author_id}* every time it is called }
$books = Book::with('author')->get(); // Only two queries will be called. // select * from books // select * from authors where id in (1, 2, 3, 4, 5, ...) foreach ($books as $book) { echo $book->author->name; }
Simple implementation of $books = Book::with('author')->get() code as the bellow.
$books = Book::all(); //*1 query* $books_authors = Author::whereIn('id', $books->pluck('author_id'))->get()->keyBy('author_id'); // *1 query* foreach($books as $book){ $books->author = $books_authors[$book->author_id] ?? null; }
$books = Book::all(); // *1 query* foreach($books as $book){ echo $book->comments()->count(); // Make one query like *select count(1) from comments where book_id = ${book.id}* every time it is called }
$books = Book::withCount('comments')->get(); // Only two queries will be called. foreach($books as $book){ echo $book->comments_count; }
Simple implementation of $books = Book::withCount('comments')->get() code as the bellow.
$books = Book::all(); // *1 query* $books_counts= Comment::whereIn('book_id', $books->pluck('id'))->groupBy('book_id')->select(['count(1) as cnt', 'book_id'] )->pluck('book_id', cnt); // *1 query* foreach($books as $book){ $book->comments_count = $likes[$book->id] ?? 0; }
Note: Read more about eager-loading In some frameworks, like phoenix, lazy-load is disabled by default to stop incorrect usage.
7. Stop printing raw user inputted values in blade.
<div>{!! nl2br($post->comment) !!}</div>
There is a XSS security issue
<div>{{ $post->comment }}</div>
Note: if you want to keep new line in div, you can use white-space: pre-wrap Rules: Don't use printing raw({!! !}}) with user input values.
8. Be careful when running javascript with user input.
function linkify(string){ return string.replace(/((http|https)?:\/\/[^\s]+)/g, "<a href="%241">$1</a>") } const $post_content = $("#content_post"); $post_content.html(linkify($post_content.text()));
There is a XSS security issue. Easy hack with input is http:deptrai.com<a href="javascript:alert('hacked!');">stackoverflow.com</a> or http:deptrai.com<img src="1" alt="image">
function linkify(string){ return string.replace(/((http|https)?:\/\/[^\s]+)/g, "<a href="%241">$1</a>") } const post = $("#content_post").get(0); post.innerHTML = linkify(post.innerHTML)
Bonus: simple sanitize function Note: Don't use unescaped user input to innerHTML. Almost javascript frameworks will sanitize input before print it into Dom by default. So, be careful when using some functions like react.dangerouslySetInnerHTML or jquery.append() for user inputted values.
In my test results, WAF(Using our provider's default rules) can prevent server-side XSS quite well, but not good with Dom-Base XSS.
Rules: Be careful when exec scripts that contains user input values.
When using morph, we cannot use foreign key relationship and when the table is grow big we will face performance issues.
posts id - integer name - string videos id - integer name - string tags id - integer name - string taggables tag_id - integer taggable_id - integer taggable_type - string
posts id - integer name - string videos id - integer name - string tags id - integer name - string posts_tags tag_id - integer post_id - integer videos_tags tag_id - integer video_id - integer
