Code fixes and improvements #6

michalbundyra · 2016-12-08T14:06:39Z

No description provided.

…xpectExceptionMessage`

weierophinney

Overall, this looks fine. However, I have concerns about two sets of changes:

The change to eliminate goto usage leads to nested conditionals and duplication of a unwieldy ternary... which was the original impetus for using goto in this particular case. I'm not convinced that the changes make the code easier to understand or more maintainable.
Replacing a single setExpectedException($type, $messageContains) call with a set of expectException() + expectExceptionMessage() calls also feels more verbose, and I'm not convinced that doing so makes the code clearer.

Other than those, the other proposed changes make sense to me.

weierophinney · 2016-12-08T14:44:10Z

src/GenerateProgrammaticPipelineFromConfig/Generator.php

@@ -241,32 +241,20 @@ private function generateRoutes(array $config)
                $route = empty($spec['name'])
                    ? sprintf(self::TEMPLATE_ROUTED_NO_METHOD_NO_NAME, $path, $middleware)
                    : sprintf(self::TEMPLATE_ROUTED_NO_METHOD_WITH_NAME, $path, $middleware, $spec['name']);
-
-                goto options;


I know many folks do not like goto, but in this case, it eliminated a lot of code duplication. I find the revised version quite more cumbersome to read, due to additional nesting and duplication of the $route assigment ternary across three conditions.

I'm one of these folks who doesn't like goto :)

if..elseif..else statement doesn't produce more code there. There is no any duplication right now, as it was also with goto instruction. So basicaly now this 'if...elsestatement is to get correctroute` - so I think it should be extracted to separate method, as I just did in 8088334.

Do you still think it is harder to understand and less maintainable?

weierophinney · 2016-12-08T14:46:26Z

test/GenerateProgrammaticPipelineFromConfig/GeneratorTest.php

-        $this->setExpectedException(GeneratorException::class, 'not found');
+
+        $this->expectException(GeneratorException::class);
+        $this->expectExceptionMessage('not found');


This seems overly verbose, compared to the original. What prompted this change, exactly?

Since PHPUnit 5.2 setExpectedException method is deprecated. PHPUnit is development requirements so it's better to avoid using deprecated methods. It will be worth in future when we want migrate to newer version, where this method will be removed.

I was unaware of that; thanks for pointing it out!

weierophinney · 2016-12-08T14:47:39Z

test/ScanForErrorMiddleware/CommandTest.php

@@ -8,8 +8,10 @@
 namespace ZendTest\Expressive\Tooling\ScanForErrorMiddleware;

 use org\bovigo\vfs\vfsStream;
+use org\bovigo\vfs\vfsStreamDirectory;


Why are you importing this? It's not referenced anywhere...

Line 38: /** @var vfsStreamDirectory */

weierophinney · 2016-12-08T14:48:11Z

test/ScanForErrorMiddleware/ErrorMiddlewareFilterTest.php

@@ -10,6 +10,7 @@
 use ArrayIterator;
 use DirectoryIterator;
 use org\bovigo\vfs\vfsStream;
+use org\bovigo\vfs\vfsStreamDirectory;


Same question here: it's not used anywhere, and tests pass without importing it...

Line 22: /** @var vfsStreamDirectory */. We can use there FQCN but I think it's better when it's imported. Then in IDE we can get suggestions etc.

IDEs can get suggestions when you use a FQCN as well:

/** @var \org\bovigo\vfs\vfsStreamDirectory */

I'd argue we could do the same with ProphecyInterface. Since they're only being used in annotations for typehints, I'd prefer that.

weierophinney · 2016-12-08T14:48:29Z

test/ScanForErrorMiddleware/ScannerTest.php

@@ -10,8 +10,10 @@
 use Countable;
 use IteratorAggregate;
 use org\bovigo\vfs\vfsStream;
+use org\bovigo\vfs\vfsStreamDirectory;


Line 87: /** @var vfsStreamDirectory */

weierophinney · 2016-12-08T14:50:44Z

test/ScanForErrorMiddleware/ScannerTest.php

@@ -125,10 +139,13 @@ public function testScanningEmitsInfoToConsoleWhenEncounteringFilesOfInterest()
        $this->console
            ->writeLine(
                Argument::that(function ($arg) {
-                    if (! strstr($arg, 'src/ErrorMiddleware.php')) {
+                    if (false === strpos($arg, 'src/ErrorMiddleware.php')) {


The original was correct; strstr returns boolean false if the needle is not found in the haystack. The only reason it might emit a false positive is if it matches exactly at the beginning, as it would return '' in that case; in this particular comparison, that would never happen...

As far as I know strpos is more efficient, uses less memory because it returns int or false. strstr returns portion of string or false. It's just a good practice to use strpos instead (when possible).

weierophinney · 2016-12-08T14:51:17Z

test/ScanForErrorMiddleware/ScannerTest.php

                        return false;
                    }
-                    if (! strstr($arg, sprintf('<error>implementing %s</error>', ErrorMiddlewareInterface::class))) {
+                    if (false === strpos(


On the flip side, this one would occur, so the proposed change is more correct.

On consideration, changing all occurrences of ! strstr to false === strpos would make it more internally consistent, so I'm fine with these changes.

- previously it was part with "goto" instruction, extreacted to separate method to improve readability and maintainable and also to eliminate "goto" instruction

weierophinney · 2016-12-08T16:25:08Z

src/GenerateProgrammaticPipelineFromConfig/Generator.php

+     * @param array $spec
+     * @return string
+     */
+    private function getRoute(array $spec)


much better! Thanks!

Code fixes and improvements

Close #6

weierophinney · 2016-12-08T16:29:38Z

Thanks, @webimpress

michalbundyra added 11 commits December 8, 2016 13:52

Eliminated "goto" instruction

50bb91e

Removed redundant semicolon after if statement

0dfb42d

Import classes

c848062

Changes "strstr" to "strpos" - save memory

5bee4d0

Format change - consistency

9f9ecce

Added private properties in tests

23325b8

Added @param tags in tests with dataProvider

286b45e

Changed deprecated setExpectedException to expectException and `e…

913edc3

…xpectExceptionMessage`

Changed assertTrue(file_exists()) to assertFileExists()

c6ff258

Removed redundant variable

6c9a0fc

Bumped PHPUnit to ^5.7 in composer.json

293d3e7

weierophinney suggested changes Dec 8, 2016

View reviewed changes

Extract method "getRoute" from spec array

8088334

- previously it was part with "goto" instruction, extreacted to separate method to improve readability and maintainable and also to eliminate "goto" instruction

weierophinney reviewed Dec 8, 2016

View reviewed changes

weierophinney approved these changes Dec 8, 2016

View reviewed changes

weierophinney added the enhancement label Dec 8, 2016

weierophinney self-assigned this Dec 8, 2016

weierophinney merged commit 8088334 into zendframework:master Dec 8, 2016

weierophinney added a commit that referenced this pull request Dec 8, 2016

Merge pull request #6 from webimpress/fix/improvements

07ec47f

Code fixes and improvements

weierophinney added a commit that referenced this pull request Dec 8, 2016

Added CHANGELOG for #6

3f5765d

weierophinney added a commit that referenced this pull request Dec 8, 2016

Merge branch 'hotfix/6'

3866285

Close #6

michalbundyra deleted the fix/improvements branch December 8, 2016 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code fixes and improvements #6

Code fixes and improvements #6

michalbundyra commented Dec 8, 2016

weierophinney left a comment

weierophinney Dec 8, 2016

michalbundyra Dec 8, 2016 •

edited

Loading

weierophinney Dec 8, 2016

michalbundyra Dec 8, 2016

weierophinney Dec 8, 2016

weierophinney Dec 8, 2016

michalbundyra Dec 8, 2016

weierophinney Dec 8, 2016

michalbundyra Dec 8, 2016

weierophinney Dec 8, 2016

weierophinney Dec 8, 2016

michalbundyra Dec 8, 2016

weierophinney Dec 8, 2016

michalbundyra Dec 8, 2016

weierophinney Dec 8, 2016

weierophinney Dec 8, 2016

weierophinney Dec 8, 2016

weierophinney commented Dec 8, 2016

Code fixes and improvements #6

Code fixes and improvements #6

Conversation

michalbundyra commented Dec 8, 2016

weierophinney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalbundyra Dec 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney commented Dec 8, 2016

michalbundyra Dec 8, 2016 •

edited

Loading