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:
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
}
}
1
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.8.
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 generated10
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.
2