Be part of JetBrains PHPverse 2026 on June 9 – a free online event bringing PHP devs worldwide together.

Michel414's avatar

Request review for correct methodology (new to lavarel)

New to lavarel, managed to get something done but unsure if I did this the correct way. -> Can somebody please review my code so I can learn how to do this properly?

Goal: I would like to add users to administrations, for this I have created the following:

The model: (after alot of reading I get in to alot of trouble if I added functions to the model, so I left this blank, i'm unsure of how to add my functions here and call them properly from the controller, if anybody has a live example, I would be happy to give it another bash)

<?php

namespace App;

use Illuminate\Database\Eloquent\Model;
use App\administration;

class Administrations_has_users extends Model
{
    protected $table = 'core_administrations_has_users';
}
?>

migration database tables:

<?php
Schema::create('core_administrations', function (Blueprint $table) {
            $table->increments('id');
            $table->integer('user_create')->nullable();
            $table->integer('user_update')->nullable();
            $table->string('name')->unique();
            $table->text('description')->nullable();
            $table->binary('image')->nullable() ; // for blob
            $table->integer('type')->nullable();
            /*
            Types:
            1 = ?
            2 = ?
            3 = ?
            */
            $table->timestamps();
        });
Schema::create('core_administrations_has_users', function (Blueprint $table) {
            $table->increments('id');
            $table->integer('userId');
            $table->integer('administrationId');
            $table->integer('type');
            /*
            Types:
            1 = member
            2 = owner
            */
            $table->timestamps();
        });
?>
  • users table is default of the lavarel

administrationController:

<?php

namespace App\Http\Controllers;

use Illuminate\Http\Request;
use App\Administration;
use App\Administrations_has_users;
use Auth;
use Session;
use App\User;
use App\Sysevent;
use DB;

//sf administrations group users together and are also commonly used by apps. These will mostly represent companies.

class AdministrationController extends Controller
{
    public function index()
    {
        $posts = Administration::orderBy('id', 'desc')->paginate(10);
        
        //need users and administration list for the add to administration form
        $users = User::all();
        $administrations = Administration::all();
        
        return view('administrations.index')->withPosts($posts)->withUsers($users)->withAdministrations($administrations);
    }

    //HIDDEN OTHER FUNCTIONS THAT I WILL NOT BE NEEDING FOR THIS
    
    public function assign(Request $request)
    {
        //get params
        $user = $request->userId;
        $administration = $request->administrationId;
        $type = $request->administrationType;
        
        //check if user exists
        $checkUser = user::where('id', '=', $user)->first();
        if ($checkUser === null) {
            Session::flash('success', 'User does not exist');
            return redirect()->route('administration.index');
        }
        
        //check if administration exists
        $checkAdministration = Administration::where('id', '=', $administration)->first();
        if ($checkAdministration === null) {
            Session::flash('success', 'Administration does not exist');
            return redirect()->route('administration.index');
        }
        
        //check if user is already part of the administration
        if ($checkAdministration = DB::table('core_administrations_has_users')->where([
            ['administrationId', '=', $administration],
            ['userID', '=', $user],
            ])->exists()) 
        {
            Session::flash('success', 'User is already part of that administration');
            return redirect()->route('administration.index');
        }
        
        //are you trying to make an owner type? if so, you need to make sure there isn't one already
        if ($type == '2' && $checkOwnerExists = DB::table('core_administrations_has_users')->where([
            ['administrationId', '=', $administration],
            ['type', '=', '2'],
        ])->count() > 0)
        {   
            Session::flash('success', 'An owner has already been assigned');
            return redirect()->route('administration.index');
        }
        
        //if all of the above past then add the user to the administration
        $input = new Administrations_has_users;
        
        $input->userId = $user;
        $input->administrationId = $administration;
        $input->type = $type;   
        $input->save();
        
        //create System event
        $sysevent = new Sysevent;
        $sysevent->event = 'User assigned to administration';
        $sysevent->type = 2;
        $sysevent->save();
        
        //set flash data with success message
        Session::flash('success', 'User assigned to administration');
        
        return redirect()->route('administration.index');
    }
}
?>

administration index view:

<?php
        <div class="row">
        <div class="col-md-12">
            <div class="card-box widget-box-1 bg-white">
                <h4>Add user to administration</h4>
                <form method="POST" action="{{ route('administration.assign') }}" accept-charset="UTF-8">
                    
                    {{ csrf_field() }}
                    
                    <label for="name">Select user: </label>
                    <select name="userId" class="selectpicker">
                        <option value="" selected="selected">Please select a user</option>
                        @foreach ($users as $user)
                        <option value="{{ $user->id }}">{{ $user->name }} - {{ $user->email }}</option>
                        @endforeach
                    </select>
                    <br>
                    
                    <label for="name">Select administration: </label>
                    <select name="administrationId" class="selectpicker">
                        <option value="" selected="selected">Please select an administration</option>
                        @foreach ($administrations as $administration)
                        <option value="{{ $administration->id }}">{{ $administration->name }}</option>
                        @endforeach
                    </select>
                    <br>
                    
                    <label for="name">As: </label>
                    <select name="administrationType" class="selectpicker">
                        <option value="1" selected="selected">a member</option>
                        <option value="2" selected="selected">the owner</option>
                    </select>
                    <br>
                    
                    <button type="submit" class="btn btn-sm">Execute</button>
                    
                </form>
            </div>
        </div>
    </div>
?>
0 likes
6 replies
al0mie's avatar

you can use markup language to past code

Michel414's avatar

@al0mie, hey there thanks for the quick response, i'm new so I'm figuring it out using the guidelines I just found via google :) give me a sec as I adapt the post.

Alright I'm ready, can you perhaps give me some pointers?

al0mie's avatar

I don't run this code, but at the first look there can be some notices.

  • Try to follow the standards of psr, it is not required, but it's common practice.
  • Try to use the same style for variable names.
  • Try don't use magic constants.
  • It's strange to see variables in if statement, because if will be true there will be return statement. Also you use sometimes DB queries, sometimes Eloquent model for one purposes.
  • Maybe the level of validation will look better if you implement it through the standard rules of validation requests in laravel https://laravel.com/docs/5.4/validation#creating-form-requests
  • It seems you do not need to use the Adminations_has_users model directly, if you specify a relationship, so you can simply attach the model with pivot column.
  • Also params from request preferebly to get via getter method, because in this way you can accidentally get native properties of the request object.
1 like
Michel414's avatar

@al0mie, thanks a bunch, It raised more questions :) I'll have to inspect each item you've just sent and find out more about it

al0mie's avatar

and don't close php tags, if there nothing except php code. Also php tags strange looks in your template.

Please or to participate in this conversation.