Removed the nested conditional switch construct

[Mathewlenning]

More cleaning.

[zero-24]

@Mathewlenning can you add more information for your change? e.g. how we can this if needed etc. Thanks!

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/6338.}

[Mathewlenning]

@zero-24 I just removed a switch that had 3 options with nested conditionals in each option from the stream_seek method of this utility class.

None of the inputs/outputs have changed, so using it will be the same as before.

Does that answer your question?

[sovainfo]

Don't think so. Assume the question is how to test this. Maybe someone intimate with the platform/framework can confirm this: Looks like this can be tested when changing configuration with the FTP Layer in effect. It should work. Installing an extension with FTP Layer should work.

[Mathewlenning]

@sovainfo thanks =^D I'm not sure where this is being used. Just opened the file and started looking for clean code easy wins.

[ghost]

I think this would be better:

switch ($whence)
{
    case SEEK_SET:
        if ($offset < strlen($this->buffers[$this->name]) && $offset >= 0)
        {
            $this->position = $offset;

            return true;
        }
        break;

    case SEEK_CUR:
        if ($offset >= 0)
        {
            $this->position += $offset;

            return true;
        }
        break;

    case SEEK_END:
        if (strlen($this->buffers[$this->name]) + $offset >= 0)
        {
            $this->position = strlen($this->buffers[$this->name]) + $offset;

            return true;
        }
        break;
}

return false;

[Mathewlenning]

@nonumber

That is the same construct that I re-factored out.

Could you explain the reasoning behind your thinking? Is there something that I've overlooked?

[ghost]

I guess it is down to taste.
A switch is very good to do different things/checks based on the same variable.

In this case, the 3 different values for $whence.

The cleanup that can be made is to remove the useless elses.

What is the advantage of not using the switch but 3 separate ifs in your view?

[Mathewlenning]

In general I try to avoid the switch, simply because it is a sign that I'm doing too much in one function.

I'd actually like to break this method into three private methods. Something like

public function stream_seek($offset, $whence)
{
    if(!in_array($whence, array('SEEK_SET','SEEK_CUR', 'SEEK_END'))
    {
         return false;
    }

    $whence = strtolower($whence);
    return $this->$whence($offset);
}

private method seek_set($offset)

private method seek_cur($offset)

private method seek_end($offset)

But when you make that much change, chances of getting merged seems unlikely.

The only real benefits of the 3 separate if clauses is it shifting the method back to one indentation and uses less lines to accomplish the same logic.

[Bakual]

I agree with @nonumber here. Switches aren't bad as long as you do simple compares like this one here. They are very easy to understand.
Your "cleaner" code has indeed less indentation, but this doesn't make the code always easier to read. In this case I actually think it's harder to see what it does.

[ghost]

I agree with @Mathewlenning that splitting it up would be even better. I'd go with:

public function stream_seek($offset, $whence)
{
    if(!in_array($whence, array('SEEK_SET', 'SEEK_CUR', 'SEEK_END'))
    {
        return false;
    }

    switch ($whence)
    {
        case SEEK_SET:
            return $this->stream_seek_set($offset);

        case SEEK_CUR:
            return $this->stream_seek_cur($offset);

        case SEEK_END:
            return $this->stream_seek_end($offset);
    }
}

private method seek_set($offset)
{
...
}

private method seek_cur($offset)
{
...
}

private method seek_end($offset)
{
...
}

Reason I try to stay away from $this->$foobar is because IDEs have a hard time of figuring out what is going on (can't jump to functions from call).
And I believe that if it is to hard for IDEs, then it is probably too difficult for people to quickly see what is going on too.

[Mathewlenning]

@nonumber good reasoning.

I hate it when my IDE cannot tell me what is going on. It means I have to stop and think about it.

What if we got rid of that opening conditional and just returned false at the end.

public function stream_seek($offset, $whence)
{
    switch ($whence)
    {
        case SEEK_SET:
            return $this->stream_seek_set($offset);

        case SEEK_CUR:
            return $this->stream_seek_cur($offset);

        case SEEK_END:
            return $this->stream_seek_end($offset);
    }

    return false;
}

private method seek_set($offset)
{
...
}

private method seek_cur($offset)
{
...
}

private method seek_end($offset)
{
...
}

This would also get rid of the need to strtolower, which is always a +1

[Mathewlenning]

@Bakual it isn't really the switch that I have a problem with, its the switch + conditional that tells me something isn't right.

[ghost]

Yes, seems good!

And something like:

private method seek_set($offset)
{
    if ($offset < 0 || $offset >= strlen($this->buffers[$this->name]))
    {
        return false;
    }

    $this->position = $offset;

    return true;
}

private method seek_cur($offset)
{
    if ($offset < 0)
    {
        return false;
    }

    $this->position += $offset;

    return true;
}

private method seek_end($offset)
{
    $offset += strlen($this->buffers[$this->name]);

    if ($offset < 0)
    {
        return false;
    }

     $this->position = $offset;

    return true;
}

[Mathewlenning]

@nonumber B-E-A-utiful.

Now we only have like 100,000 more clean ups to do and Joomla will be purring lol.

I'll make the changes.

[ghost]

Yep. So let's just tackle at least one of them each day. Then we only need 274 years to finish the job!

[Mathewlenning]

In that case, I hope Google's "end-to-death" project works out.

[ghost]

Oh, one more thing:

$offset = strlen($this->buffers[$this->name]) + $offset;

Can change to

$offset += strlen($this->buffers[$this->name]);

[Mathewlenning]

Oops missed that one. Got it this time. =^D

[Bakual]

Nice one 👍
I love constructive discussions 😄

[ghost]

@test: All good as far as I could test

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/6338.}

[roland-d]

@Mathewlenning Where is this file called? In other words, how can I test this PR?

[Kubik-Rubik]

Thank you @Mathewlenning!