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

tommyc81's avatar

Code refactoring -> Folder structure suggestions (Model, Repository etc.)

I'm upgrading from Laravel 4 to 5 and at the same time refactoring a lot of the spaghetti code to tidy things up (thanks Laracasts for loads of useful info!). However, I'm not sure what the best way to divide and clean up the following is. Currently everything sits in the User model, but should obviously be divided up. Not the full file below, but some relevant excerpts:

class User extends Eloquent
{

<Various Model related stuff; tables, relations etc.>

<Various stuff that should go into the UserRepository>

// CONSTANTS
const LEVEL_DISABLED = 0;
const LEVEL_STUDY = 1;
const LEVEL_GENERATE = 2;
const LEVEL_REVIEW = 3;
const LEVEL_EDIT = 4;
const LEVEL_MANAGE = 5;

// STATIC LEVEL INFORMATION FUNCTIONS
public static function getLevelLabel($level)
{
    switch ($level) {
        case self::LEVEL_DISABLED:
            return 'Disabled';
            break;
        case self::LEVEL_STUDY:
            return 'Study';
            break;
        case self::LEVEL_GENERATE:
            return 'Generate';
            break;
        case self::LEVEL_REVIEW:
            return 'Review';
            break;
        case self::LEVEL_EDIT:
            return 'Edit';
            break;
        case self::LEVEL_MANAGE:
            return 'Manage';
            break;
    }
}

public static function getLevelIcon($level)
{
    switch ($level) {
        case self::LEVEL_DISABLED:
            return '<span class="glyphicon glyphicon-lock"></span> ' . self::getLevelLabel(self::LEVEL_DISABLED);
            break;
        case self::LEVEL_STUDY:
            return '<span class="glyphicon glyphicon-user"></span> ' . self::getLevelLabel(self::LEVEL_STUDY);
            break;
        case self::LEVEL_GENERATE:
            return '<span class="glyphicon glyphicon-share"></span> ' . self::getLevelLabel(self::LEVEL_GENERATE);
            break;
        case self::LEVEL_REVIEW:
            return '<span class="glyphicon glyphicon-comment"></span> ' . self::getLevelLabel(self::LEVEL_REVIEW);
            break;
        case self::LEVEL_EDIT:
            return '<span class="glyphicon glyphicon-edit"></span> ' . self::getLevelLabel(self::LEVEL_EDIT);
            break;
        case self::LEVEL_MANAGE:
            return '<span class="glyphicon glyphicon-star-empty"></span> ' . self::getLevelLabel(self::LEVEL_MANAGE);
            break;
    }
}

public function getRealmLevelIcon($realm_id = null)
{
    if (is_null($realm_id)) {
        $realm_id = PESession::getActiveRealmId();
    }

    foreach ($this->realms as $realm) {
        if ($realm->id == $realm_id) {
            return self::getLevelIcon($realm->pivot->level);
        }
    }

    return self::getLevelIcon(User::LEVEL_DISABLED);
}

public static function getLevelDropdownList($max = null, $validation = false)
{
    $list = array(
        self::LEVEL_DISABLED => self::getLevelLabel(self::LEVEL_DISABLED),
        self::LEVEL_STUDY => self::getLevelLabel(self::LEVEL_STUDY),
        self::LEVEL_GENERATE => self::getLevelLabel(self::LEVEL_GENERATE),
        self::LEVEL_REVIEW => self::getLevelLabel(self::LEVEL_REVIEW),
        self::LEVEL_EDIT => self::getLevelLabel(self::LEVEL_EDIT),
        self::LEVEL_MANAGE => self::getLevelLabel(self::LEVEL_MANAGE),
    );

    if (!is_null($max)) {
        $adjusted_list = array();
        foreach ($list as $key => $value) {
            if ($key <= $max) {
                $adjusted_list[$key] = $value;
            }
        }
        $list = $adjusted_list;
    }

    if ($validation) {
        return implode(',', array_keys($list));
    }

    return $list;
}

As you can see, there are some functions that are related to the User model that I simply don't know where to put. The model seems like the wrong place. Where would you suggest I place them? (Code improvement suggestions are also welcome!)

I use some of the attached functions to format html output as it makes it a bit simpler when looping through them in the view: $user->getActiveRealmIcon(). Not sure what the alternative is?

0 likes
6 replies
michaeldyrynda's avatar

The model should really only concern itself with talking to your persistence layer. That said, you can extract the levels functionality into it's own class somewhere.

class Level {
    const LEVEL_DISABLED = 0;
    const LEVEL_STUDY = 1;
    const LEVEL_GENERATE = 2;
    const LEVEL_REVIEW = 3;
    const LEVEL_EDIT = 4;
    const LEVEL_MANAGE = 5;

    protected $levels;

    public function __construct()
    {
        $this->levels = [
            self::LEVEL_DISABLED => [ 'label' => 'Disabled', 'icon' => 'lock', ],
            self::LEVEL_STUDY => [ 'label' => 'Study', 'icon' => 'user', ],
            self::LEVEL_GENERATE => [ 'label' => 'Generate', 'icon' => 'share', ],
            self::LEVEL_REVIEW => [ 'label' => 'Review', 'icon' => 'comment', ],
            self::LEVEL_EDIT => [ 'label' => 'Edit', 'icon' => 'edit', ],
            self::LEVEL_MANAGE => [ 'label' => 'Manage', 'icon' => 'star-empty', ],
        ];
    }

    private function isValid($level)
    {
        if ( ! isset($this->levels[$level]) )
        {
            throw new InvalidLevelException(sprintf('The level %s does not exist.', e($level));
        }

        return true;
    }

    public function getLabel($level)
    {
        $this->isValid($level);

        return $this->levels[$level]['label'];
    }

    public function getIcon($level)
    {
        $this->isValid($level);

        return $this->levels[$level]['icon'];
    }

    public function getList($max = null, $validation = false)
    {
        $list = [ ];

        foreach ($this->labels as $key => $value)
        {
            if ( is_null($max) || ( $key <= $max ) )
            {
                $list[$key] => $value['label'];
            }
        }

        if ( $validation === true )
        {
            return join(',', array_keys($list));
        }

        return $list;
    }
}

I've omitted the HTML part, that can likely be extracted to a view partial, rather than living in your class. Then just pass the label and icon to that view when rendering it.

Another option would be to initialise the Level class with a single $level passed into the constructor rather than individual methods, that way you only have to check isValid on initialisation.

This is just one way to tackle it (and the first that came to mind at 0630). It may give you a hint on a direction to take (or one to avoid!), depending on your needs. I would certainly avoid putting much of that logic in the model, though. Don't be shy about adding folders in your app structure and separating things out a bit.

3 likes
tommyc81's avatar

Brilliant response!

Completely agree that the Model is overly cluttered with stuff it shouldn't be bothered with. Having too many options can be tricky sometimes, the (folder) structure can be done in so many ways. You definitely pointed me in a new direction, exactly what I needed!

1 like
thepsion5's avatar
Level 25

The Level class idea is a good one, but you should use this really solid enum package for it: https://github.com/myclabs/php-enum

It will make the level class itself dead simple, validation and all.

Next, you should extract most of the non-authentication functionality into a class that extends the User class. I'm not sure what your app is, but I'm pretty sure something like "Participant" or "Player" would more closely describe what you're actually using the User class for.

Third, you should extract the html-related methods from both your user and the user's level into a ParticipantPresenter class. This can include (and probably should) include the level icons.

That should get you well on your way. :)

2 likes
michaeldyrynda's avatar

I knew there was an enum package @thepsion5 but I couldn't remember it and I was heading out the door. Cheers for that.

Glad I was able to point you in the right direction @tommyc81.

1 like
tommyc81's avatar

@thepsion5 Thanks for great suggestions! You and @deringer has really helped me out here!

I'm reading up a bit on the different bits and pieces before I dig into actually re-factoring everything properly.

@thepsion5 Some additional questions;

You suggest extending the User Model class so as to separate the pure database functions from additional functionality, sounds good to me! What do you call these classes that are extended that way? Entities? Just trying to get my head around the general folder structure I use. In addition to that, would you also have the AppServiceProvider bind "User" to the entity "User" (the Entity class rather than Model class), so only the entity class would be used in all places where the Model normally would?

I've seen some examples of different ways to implement enums (several packages on github for instance). I assume the package you've suggested is your preferred one? Do you have a concrete example of how you implement it's usage? Would you, in case I use an entity class that extends the User model, simply "use" the UserLevel enum class in the entity file?

Thanks again for the help, also throw the question up on Reddit and got similar responses: https://www.reddit.com/r/laravel/comments/2vogyg/code_refactoring_folder_structure_suggestions/

tommyc81's avatar

@deringer @thepsion5 I ended up doing something in between, for now at least.

Made an abstract constant class with the following:

<?php namespace App\Constants;

use App\Exceptions\InvalidConstantException;

abstract class Constant
{
    static protected $constants = null;

    static public function isValid($constant)
    {
        if ( ! isset(static::$constants[$constant])) {
            throw new InvalidConstantException(sprintf('A constant with value "%s" does not exist in class %s.', e($constant), e(static::class)));
        }

        return true;
    }

    static public function __callStatic($name, $params){
        $constant = $params[0];

        static::isValid($constant);

        return isset(static::$constants[$constant][$name]) ? static::$constants[$constant][$name] : null;
    }
}

and then I extend it:

<?php namespace App\Constants;

class UserLevel extends Constant
{

    const DISABLED = 0;
    const STUDY = 1;
    const GENERATE = 2;
    const REVIEW = 3;
    const EDIT = 4;
    const MANAGE = 5;

    static protected $constants = [
        self::DISABLED => [ 'label' => 'Disabled', 'icon' => 'lock' ],
        self::STUDY    => [ 'label' => 'Study', 'icon' => 'user' ],
        self::GENERATE => [ 'label' => 'Generate', 'icon' => 'share' ],
        self::REVIEW   => [ 'label' => 'Review', 'icon' => 'comment' ],
        self::EDIT     => [ 'label' => 'Edit', 'icon' => 'edit' ],
        self::MANAGE   => [ 'label' => 'Manage', 'icon' => 'star-empty' ],
    ];
}

It seem to be working fine and it keeps all things related to the constants in one place. I'm probably failing miserably in separating concerns somewhere, but it is simple and a huge improvement from what I'm doing now. If you can see any obvious pitfalls (apart from generally failing to adhere to established coding standards) I'm all ears. Thanks again for the pointers so far, the handling of all constants has been the biggest issue to figure out so far.

Please or to participate in this conversation.