User tests: Successful: Unsuccessful:
More cleaning.
Labels |
Added:
?
|
Status | Pending | ⇒ | Information Required |
Category | ⇒ | Libraries |
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.
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;
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?
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.
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.
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.
@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
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;
}
Yep. So let's just tackle at least one of them each day. Then we only need 274 years to finish the job!
In that case, I hope Google's "end-to-death" project works out.
Oh, one more thing:
$offset = strlen($this->buffers[$this->name]) + $offset;
Can change to
$offset += strlen($this->buffers[$this->name]);
Oops missed that one. Got it this time. =^D
Nice one
I love constructive discussions
@test: All good as far as I could test
Status | Information Required | ⇒ | Pending |
@Mathewlenning Where is this file called? In other words, how can I test this PR?
Thank you @Mathewlenning!
Status | Pending | ⇒ | Fixed in Code Base |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-05-08 09:27:21 |
Closed_By | ⇒ | Kubik-Rubik |
@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.