? Pending

User tests: Successful: Unsuccessful:

avatar Mathewlenning
Mathewlenning
6 Mar 2015

More cleaning.

avatar Mathewlenning Mathewlenning - open - 6 Mar 2015
avatar joomla-cms-bot joomla-cms-bot - change - 6 Mar 2015
Labels Added: ?
avatar zero-24 zero-24 - change - 6 Mar 2015
Status Pending Information Required
avatar zero-24 zero-24 - change - 6 Mar 2015
Category Libraries
avatar zero-24
zero-24 - comment - 6 Mar 2015

@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.
avatar Mathewlenning
Mathewlenning - comment - 7 Mar 2015

@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?

avatar sovainfo
sovainfo - comment - 7 Mar 2015

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.

avatar Mathewlenning
Mathewlenning - comment - 12 Mar 2015

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

avatar nonumber
nonumber - comment - 16 Mar 2015

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;
avatar Mathewlenning
Mathewlenning - comment - 16 Mar 2015

@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?

avatar nonumber
nonumber - comment - 16 Mar 2015

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?

avatar Mathewlenning
Mathewlenning - comment - 16 Mar 2015

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.

avatar Bakual
Bakual - comment - 16 Mar 2015

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.

avatar nonumber
nonumber - comment - 16 Mar 2015

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.

avatar Mathewlenning
Mathewlenning - comment - 16 Mar 2015

@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

avatar Mathewlenning
Mathewlenning - comment - 16 Mar 2015

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

avatar nonumber
nonumber - comment - 16 Mar 2015

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;
}
avatar Mathewlenning
Mathewlenning - comment - 16 Mar 2015

@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.

avatar nonumber
nonumber - comment - 16 Mar 2015

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

avatar Mathewlenning
Mathewlenning - comment - 16 Mar 2015

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

avatar nonumber
nonumber - comment - 16 Mar 2015

Oh, one more thing:

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

Can change to

$offset += strlen($this->buffers[$this->name]);
avatar Mathewlenning
Mathewlenning - comment - 16 Mar 2015

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

avatar Bakual
Bakual - comment - 16 Mar 2015

Nice one :+1:
I love constructive discussions :smile:

avatar nonumber
nonumber - comment - 16 Mar 2015

@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.
avatar nonumber nonumber - test_item - 16 Mar 2015 - Tested successfully
avatar brianteeman brianteeman - change - 19 Jul 2015
Status Information Required Pending
avatar roland-d roland-d - alter_testresult - 15 Apr 2016 - nonumber: Tested successfully
avatar roland-d
roland-d - comment - 15 Apr 2016

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

avatar Kubik-Rubik
Kubik-Rubik - comment - 8 May 2016

Thank you @Mathewlenning!

avatar Kubik-Rubik Kubik-Rubik - change - 8 May 2016
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2016-05-08 09:27:21
Closed_By Kubik-Rubik
avatar Kubik-Rubik Kubik-Rubik - close - 8 May 2016
avatar Kubik-Rubik Kubik-Rubik - merge - 8 May 2016
avatar Kubik-Rubik Kubik-Rubik - reference | fa07f78 - 8 May 16
avatar Kubik-Rubik Kubik-Rubik - merge - 8 May 2016
avatar Kubik-Rubik Kubik-Rubik - close - 8 May 2016
avatar roland-d roland-d - reference | 47ec21a - 8 May 16

Add a Comment

Login with GitHub to post a comment