1
\$\begingroup\$

Debugged my page load (was taking 4s...) and I was running 94 queries. 30 of them were the same. I've refactored a little by caching results, but how can I further refactor this class?

The project is only half finished, and its already getting... messy.

<?php

class User extends Authenticatable
{
    use Notifiable;

    protected $fillable = [
        'name', 'email', 'password',
    ];

    protected $hidden = [
        'password', 'remember_token',
    ];


    protected $casts = [
        'email_verified_at' => 'datetime',
    ];

    public function courseEntries() {
        return $this->hasMany('App\Models\CourseEntry');
    }

    public function lessonEntries() {
        return $this->hasMany('App\Models\CourseLessonEntry');
    }

    public function lastCourseEntry() {
        $this->courseEntries()->orderBy('last_interaction', 'DESC')->first();
    }

    public function classrooms() {
        return $this->hasMany('App\Models\Classroom', 'teacher_id', 'id');
    }

    public function lastLessonEntry() {
        if ($this->lastCourseEntry() == null) {
            return null;
        }

        return $this->lessonEntries()->whereCourseId($this->lastCourseEntry()->course_id)->orderBy('last_interaction', 'DESC')->first();
    }

    public function lessonsCompleted() {
        if ($this->lastCourseEntry() == null) {
            return 0;
        }

        return $this->lessonEntries()->whereCourseId($this->lastCourseEntry()->course_id)->whereNotNull('completed_at')->count();
    }

    public function lastProgress() {
        $lastCourse = $this->lastCourseEntry();
        return $lastCourse == null ? 0 : $this->lessonsCompleted() / $lastCourse->course->lessons()->count() * 100;
    }

    public function hasCourse($courseId) {
        if ($this->isTeacher()) {
            if ($this->lastClassroom() == null) {
                return null;
            }
            return $this->courseEntries()->where('course_id', $courseId)->whereClassroomId($this->lastClassroom()->id)->count() > 0;
        }

        return $this->courseEntries()->where('course_id', $courseId)->count() > 0;
    }

    public function hasCompletedCourse($courseId) {
        if ($this->isTeacher()) {
            if ($this->lastClassroom() == null) {
                return null;
            }
            return $this->courseEntries()->where('course_id', $courseId)->whereClassroomId($this->lastClassroom()->id)->whereNotNull('completed_at')->count() > 0;
        }

        return $this->courseEntries()->where('course_id', $courseId)->whereNotNull('completed_at')->count() > 0;
    }

    public function hasLesson($courseId, $lessonId) {
        if ($this->isTeacher()) {
            return $this->lessonEntries()->whereClassroomId($this->lastClassroom()->id)->where('course_id', $courseId)->where('lesson_id', $lessonId)->count() > 0;
        }

        return $this->lessonEntries()->where('course_id', $courseId)->where('lesson_id', $lessonId)->count() > 0;
    }

    public function hasCompletedLesson($courseId, $lessonId) {
        $entry = $this->getLessonEntry($courseId, $lessonId);
        return $entry == null ? false : $entry->count() > 0;
    }

    public function getLessonEntry($courseId, $lessonId) {
        if ($this->isTeacher()) {
            return $this->lessonEntries()->where('course_id', $courseId)->where('lesson_id', $lessonId)->whereClassroomId($this->lastClassroom()->id)->whereNotNull('completed_at')->first();
        }

        return $this->lessonEntries()->where('course_id', $courseId)->where('lesson_id', $lessonId)->whereNotNull('completed_at')->first();
    }

    public function rankInstance() {
        return $this->hasOne('App\Models\UserRank', 'id', 'rank');
    }

    public function completedCourses() {
        return $this->courseEntries()->whereNotNull('completed_at')->get();
    }

    public function notifications() {
        return $this->hasMany('App\Models\UserNotification');
    }

    public function notificationCount() {
        return $this->notifications()->where('seen', false)->count();
    }

    public function lastClassroom() {
        $lastClassroomId = Auth::user()->last_classroom;

        if ($lastClassroomId != 0) {
            $classroom = Classroom::find($lastClassroomId);

            if ($classroom != null) {
                return $classroom;
            }
        }

        $classroom = $this->classrooms()->orderBy('id', 'DESC')->first();
        return $classroom;
    }

    public function lastCourseForClassroom() {
        if ($this->lastClassroom() == null) {
            return null;
        }

        return $this->courseEntries()->whereClassroomId($this->lastClassroom()->id)->orderBy('last_interaction', 'DESC')->first();
    }

    public function lastLessonForClassroom() {
        if ($this->lastCourseForClassroom() == null) {
            return null;
        }

        return $this->lessonEntries()->where('course_id', $this->lastCourseForClassroom()->course_id)->whereClassroomId($this->lastClassroom()->id)->orderBy('last_interaction', 'DESC')->first();
    }

    public function getCourses() {
        return Auth::user()->courseEntries()->where();
    }

    public function lessonsCompletedForClassroom() {
        if ($this->lastCourseForClassroom() == null) {
            return 0;
        }

        return $this->lessonEntries()->whereClassroomId($this->lastClassroom()->id)->where('course_id', $this->lastCourseForClassroom()->course->id)->whereNotNull('completed_at')->count();
    }

    public function lastProgressForClassroom() {
        if ($this->lastCourseForClassroom() == null) {
            return 0;
        }

        $lastCourse = $this->lastCourseForClassroom()->course;

        return $lastCourse == null ? 0 : $this->lessonsCompletedForClassroom() / $lastCourse->lessons()->count() * 100;
    }

    public function completedCoursesForClassroom() {
        if ($this->lastClassroom() == null) {
            return null;
        }

        return $this->courseEntries()->whereClassroomId($this->lastClassroom()->id)->whereNotNull('completed_at')->get();
    }

    public function courseEntriesForClassrooom() {
        if ($this->lastClassroom() == null) {
            return null;
        }

        return $this->courseEntries()->whereClassroomId($this->lastClassroom()->id)->get();
    }

    public function isAdmin() {
        return $this->rankInstance == null ? false : $this->rankInstance->is_admin == 1;
    }

    public function isTeacher() {
        return $this->rankInstance == null ? false : ($this->rankInstance->is_admin == 1 || $this->rankInstance->is_teacher == 1);
    }

    public function classroomEntries() {
        return $this->hasMany('App\Models\UserClassroomEntry', 'user_id', 'id');
    }

    public function hasInviteToClass($classId) {
        $this->classroomEntries()->where('classroom_id', $classId)->count();
    }
}
\$\endgroup\$
  • \$\begingroup\$ The title is too common of a request for this site, since many users are interested in refactoring their code, and many posts utilize laravel and eloquent. Please make the title of the question describe what the code does via edit. Maybe you missed the placeholder on the title element: "State the task that your code accomplishes. Make your title distinctive.". Also from How to Ask: "State what your code does in your title, not your main concerns about it.". \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ May 2 '19 at 18:04
  • \$\begingroup\$ 4 seconds is a bit too much for 100 queries. 100000 queries would be a more reasonable number. So it seems the problem is not in the quantity but in the quality. Run Laravel profiler and see what's wrong with your queries. \$\endgroup\$ – Your Common Sense May 2 '19 at 18:44
  • 1
    \$\begingroup\$ If you want to talk about a particular page load then you'll need to include the controller and view code as well. \$\endgroup\$ – nzt May 5 '19 at 1:09
  • \$\begingroup\$ However from the code I'm guessing you might want to look into the difference between the Eloquent relationship methods and dynamic properties. This will allow you to use eager loading properly. \$\endgroup\$ – nzt May 5 '19 at 1:15
2
\$\begingroup\$

You are not describing the business logic, so for class User that you provided it's possible to give only very general recommendations.

Do not use Authentication in the Model, it's not understandable. Use it in controller or even better in Route:

Route::group(['middleware' => ['web', 'auth']], function () {
   //put here all routes that need of authenticated user 
});

It will redirect Guests to login page.

The methods for models Course, Lesson, Classroom and Notification are more proper to put into appropriate models. In model class User remain just methods for foreign keys where you used hasMany() relation.

Basing on structure as I offer you'll get data from Models using Controllers and not like now using one 'global' model.

Method courseEntries() is good in controller for getting users with course entries as:

$usersWithCourseEntries = User::with('courseEntries')->get();

Refer to. Also remember, this query will return from DB all users who have course entries and there may be a huge number of results.

Using this method in model as you made:

public function lastCourseEntry() {
        $this->courseEntries()->orderBy('last_interaction', 'DESC')->first();
    }

may drive your program to fail. Maybe it's a bottleneck that gives you 4 seconds query execution, but it's just assumption. I didn't try it.

Next:

If you need data returned by some method, for example, coursesEntriesEnded() and in the same controller the data like lastCourseEntryEnded() as last record of data set and similar operations for other models you may not to call these both methods and use something like this to get data from collection that you already have:

$courseModel = new Course();
$endedCourses = $courseModel->coursesEntriesEnded;
$lastCourseEntryEnded = $endedCourses->orderBy('last_interaction')->last();

it will reduce the DB queries count.

Of course this review is not complete, but for deeper analysis it's needed to know tasks, business logic and to see the rest code of app.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for providing an answer. Some of the comments on the answer also indicate that more information would help provide a better review. In a case like this I would recommend you vote to close as lacks context instead of providing an answer. Then after more context/code has been added you can add an answer. Protip: there are many other unanswered questions - e.g. ~345 in PHP alone. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jun 6 '19 at 17:19
  • \$\begingroup\$ @sam-onela, yes, I agree about vote to close as lacks context and will do it next time \$\endgroup\$ – Vlad Jun 6 '19 at 18:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.