Check if a new movie session can be inserted

Question

I have this function in my code, this way I validate if a new movie session can be inserted, it works, but I wanted to find a better way to do it.

public static function validateSession($newSession)
{
    $sessions = Sessions::get();

    foreach($sessions as $session){
        //Validates if the time and date of sessions are the same as the existing ones
        if (
            $newSession['session_date'] == $session->session_date
            && $newSession['room_id'] == $session->room_id
        ) {
            //Validates if the session time is the same as an existing one
            if ($newSession['session_id'] == $session->session_id)
            {
                return false;
            } 
                //Validates if the time of the new session is greater than the start time and less than the end time of existing sessions    
                elseif (
                $newSession['session_hour'] > $session->session->session_hour
                && $newSession['session_hour'] < $session->end_of_session
            ) {
                return false;
            }
                //Validates if the time of the new session is less than the start time and the end time of the new session is greater than the start time of the existing sessions
                elseif (
                $newSession['session_hour'] < $session->session->session_hour
                && $newSession['end_of_session'] > $session->session->session_hour
            ) {
                return false;
            }
        }
    }
    return true;
}

I tried to make a switch structure and even use the spaceship operator, but I was not successful, I wanted to find a better way to do this type of validation.

Answer 1

1. Do these get() and validateSession() methods both exist within the Sessions class? Perhaps ask yourself if you have a good reason to declare both of them as static.

2. Overall, the code styling closely adheres to PSR-12 with only a couple outliers. The foreach() should have a space before and after its parenthetical expression. A couple elseif branches were over-tabbed.

3. I recommend using type declarations on incoming method parameters and return values.

4. sessions is a single-use variable and the variable name does not improve the description of the data that it holds -- just call the static method inside the foreach().

5. Try to keep your code comments concise without losing clarity. Horizontal scrolling inside an IDE can be annoying for developers.

6. You aren't returning different outcomes based on different validation conditions, so combine your conditions to D.R.Y. your code (only type return false; once).

7. Invert the parent if expression and use continue to avoid "arrowhead" code. This restyling will keep more of your code to the left side of your file and improve readability.

8. Opinions vary regarding whether it is good naming convention for a boolean-returning method's name to start with is, has, was, etc. My preference would be to call your method isValidSession() -- then a developer could semantically imply a boolean return value. Alternatively, if the method returned a string that described how the input was invalid, then the name might be reasonSessionInvalid(). As a final theoretical method name, a developer might use reasonsSessionInvalid() to return an array of zero or more violations.

Other than that, it looks pretty tight.

New suggested code:

public static function isValidSession(array $newSession): bool
{
    foreach (Sessions::get() as $session) {
        if (
            // if session date and id do not qualify
            $newSession['session_date'] != $session->session_date
            || $newSession['room_id'] != $session->room_id
        ) {
            continue;
        }

        if (
            // if session time already exists
            $newSession['session_id'] == $session->session_id

            // or if new session time is between the start and end time    
            || (
                $newSession['session_hour'] > $session->session->session_hour
                && $newSession['session_hour'] < $session->end_of_session
            )

            // or if new session time is less than start time
            //    and new session end time is greater than existing session's start time
            || (
                $newSession['session_hour'] < $session->session->session_hour
                && $newSession['end_of_session'] > $session->session->session_hour
            )
        {
            return false;
        }
    }
    return true;
}

Answer 2

You're focusing on aesthetics while missing the main problem:

$sessions = Sessions::get();

Presumably this selects all sessions in the database for this user and brings them to the php app. Then the code throws away most of them. This makes the database do a lot of useless work.

The first thing to do would be to move all the conditions into that SELECT. Then the database could use an index on (date,room) to only look at the sessions on the same day and in the same room as the new session.

With all the checks in the database, either the SELECT will return the conflicting session (could be one or two) or it won't return anything.

It looks like you're trying to check if the new session overlaps with an existing one, so you might want to search "interval overlap condition". If you use Postgres, it has an OVERLAP keyword to do that.

Note this only tells you if there is a conflicting session at the time of the select, but not at the time of the insert. Another user could insert a session between the check and the insert, so that way of doing things is not concurrency safe.

One proper way to do that would be to lock the room with a SELECT FOR UPDATE, then check and insert, then COMMIT the transaction.