Laravel PostController

Question

I'm new at Laravel and I got an assessment task from company where they want me to create a mini-blog with Users and Posts.

I was using following methods: index, create, show, store, edit and destroy in PostController. When I finished I sent it to them but they told me:

We still want you have a look at some parts of your code:

• The index function of your PostController

• Your delete function of your PostController

I am not sure what can I improve in index and delete methods. Here are how they looks like in controller:

public function index()
 {
 $post = Post::all();
 return view('blog.index')
 ->with('posts', Post::orderBy('created_at', 'DESC')->get());
 }
public function destroy($id)
 {
 $post = Post::where('id',$id);
 $post->delete();
 
 return redirect('/blog')
 ->with('message', 'Your post has been deleted!');
 }

Here are the routes (web.php):

Route::get('/', [PagesController::class, 'index']);
Route::resource('/blog', PostsController::class);
Auth::routes();

Do you have any suggestions on what to improve, or maybe do we have any best practices related to mentioned methods?

• Sᴀᴍ Onᴇᴌᴀ: Welcome to Code Review! Was this assessment part of an interview? If so, the tag interview-questions could be added.

Answer 1

Thanks for migrating this from Stackoverflow! Let's take a look at your code:

public function index() {
 $post = Post::all();
 return view('blog.index')->with('posts', Post::orderBy('created_at', 'DESC')->get());
}

This isn't bad, but the main issue is that $post is never used. You can remove that line completely. Also, ->with() works, but there are other syntaxes available, such as simply passing an array as the 2nd parameter of ->view(). Also, orderBy('created_at', 'DESC') can simply be replaced by ::latest():

public function index() {
 return view('blog.index', [
 'posts' => Post::latest()->get()
 ]);
}

The destroy() method also isn't bad, but, since you're using a ResourceController, you shouldn't need to do Post::find(); that should be implied:

public function destroy(Post $post) {
 $post->delete();
 
 return redirect()->route('blogs.index')->with([
 'message' => 'Your Post has been deleted'
 ]);
}

This should work, but your definition in routes/web.php seems a bit odd. I would recommend:

Route::resource('/posts', PostsController::class);

And anywhere you have blog, replace with post (or posts). You'll also need to move anything in resources/views/blogs to resources/views/posts. Resource Controllers ideally should reference the same name as the model they are representing.

Answer 2

Review

While the question may not (yet) have the tag interview-questions it does seem like it is part of an interview process. If I was evaluating this code written by a potential developer, who may work independently or with others, I would consider aspects such as:

• how readable the code is
• how well the code takes advantage of language features
• how well the code takes advantage of framework features

For a user that is new to using laravel the code is not bad, however the sections below offer suggestions that utilize language and framework features.

Suggestions

Readability

While there are only two methods listed, and it may be due to copy/paste techniques, it seems that indentation is not always consistent.

Let us look at the first three lines of the controller code:

public function index()
 {
 $post = Post::all();

It appears there is one indentation level for the opening brace, then one additional indentation level for lines within the block. Idiomatic PHP code has no additional level for the opening brace for a function / method. Speaking of idiomatic code, many PHP developers follow the PHP Standards Recommendations e.g. PSR-1: Basic Coding Standard, PSR-12: Extended Coding Style. PSR-12 Section 4.4 is congruent with this convention:

4.4 Methods and Functions

Visibility MUST be declared on all methods.

Method names MUST NOT be prefixed with a single underscore to indicate protected or private visibility. That is, an underscore prefix explicitly has no meaning.

Method and function names MUST NOT be declared with space after the method name. The opening brace MUST go on its own line, and the closing brace MUST go on the next line following the body. There MUST NOT be a space after the opening parenthesis, and there MUST NOT be a space before the closing parenthesis.

A method declaration looks like the following. Note the placement of parentheses, commas, spaces, and braces:

<?php
namespace Vendor\Package;
class ClassName
{
 public function fooBarBaz($arg1, &$arg2, $arg3 = [])
 {
 // method body
 }
}

¹

In the index method the last line has an extra indentation level, since it is a chained call:

 return view('blog.index')
 ->with('posts', Post::orderBy('created_at', 'DESC')->get());

However the same is not true for the destroy method - the final line does not have an extra indentation level:

 return redirect('/blog')
 ->with('message', 'Your post has been deleted!');

Docblocks

While PSR-5 PHPDoc is still in draft mode it is still helpful to anyone reading the code (including your future self) to have quick descriptions of methods, functions, etc. as well as any parameters, return values, etc.

Language features

Type declarations can be added to function arguments, return values, and, as of PHP 7.4.0, class properties. They ensure that the value is of the specified type at call time, otherwise a TypeError is thrown.⁸ .

These could be added to arguments - e.g. $id in the destroy() method.

Framework features

As the answer by Tim Lewis implies Route model Binding could be used to simplify the destroy() method if the Post model is injected by accepting a Post $post instead of $id, though be aware that if no such model is found then a 404 response would be generated¹⁰

Single use variable

In that destroy() method $post is only used once after it is assigned:

$post = Post::where('id',$id);
$post->delete();

Unless the variable is needed for debugging or makes subsequent lines shorter, it can be eliminated. While memory may not be an issue, it is wise to reduce memory consumption.

Post::where('id',$id)->delete();

Variable names

In the index() method a collection of model records is fetched with the all() method:

$post = Post::all();

Perhaps a more descriptive variable name would be $posts.

When calling where on a model class a query builder I.e. Illuminate\Database\Query\Builder Is returned. Thus perhaps a better name than $post in the destroy() method would be $query.

Helper method to delete records

As the documentation suggests the destroy() method could be used to delete a record without needing to call the where() method:

Post::destroy($id);

Though be aware that this may be slower and consume more resources, depending on what handlers for events may be defined on the model.

The destroy method loads each model individually and calls the delete method so that the deleting and deleted events are properly dispatched for each model. ²

• Tim Lewis: Great answer too! Very informative 🙂 My only gripe is $post vs $posts; the variable casing should match what is returned, and id, being primary, should only ever return 1 record. $post is appropriate for ->first(), ->find(), etc. $posts is appropriate for ::all(), ->get(), etc.; closures that return Multiple. That being said, in the last scenario (delete), you actually wouldn't assign a variable at all. Typical code would be Post::where('id', $id)->delete(), or Post::find($id)->delete(), etc. The subsequent code is a redirect, which doesn't reference $post at all.
• Sᴀᴍ Onᴇᴌᴀ: Thank you for the constructive criticism! I have revised my post to clarify a few things. Initially I should have been more clear about the where() method returning a query builder. Yes in most cases a variable wouldn’t be needed during a delete operation though there could be some customer (I’ve worked for some like it) that wants to have a resource of the deleted model.
• Tim Lewis: Excellent! I only brought up naming as I see this too often: $post = Post::all(), then $post->id; that doesn't work, since $post is a Collection of multiple Post objects, and trying to access ->id fails (a Collection doesn't have an id, and the code won't know which id you're trying to access). Also, $posts = Post::find($id). If you tried to iterate $posts, it would work, but iterating a Model in Laravel loops over each column from its associated database table. Being aware, naming and casing of variables is super important, especially when using frameworks like Laravel 🙂