Reducing complexity of execute method in SuggestCommand #27

Idrinth · 2018-03-20T23:00:06Z

I took the liberty of ripping appart the execute method to make other goals(like testing) easier to achieve.

The following was done:

moved duplicate code to methods
extracted classes to wrap logically related functionality or data

Idrinth · 2018-03-20T23:40:10Z

just wondering, should I reformat the whole thing to match psr2? would look cleaner obviously and could be tested automatically, on the other hand it might makie diffs across that change harder to read.

tomzx · 2018-04-13T17:59:53Z

Please keep the style as is for now. I'd rather not have style change because of an unrelated PR.

tomzx · 2018-04-13T18:00:51Z

bin/php-semver-checker-git

-		require $path;
-		$found = true;
-		break;
+		require($path);


This is a bit unrelated to this PR.

agreed, not quite sure why I committed this to this branch. Should I remove it?

Yes. Please keep the PR contained 😉.

tomzx · 2018-04-13T18:02:15Z

src/PHPSemVerCheckerGit/Console/Command/SuggestCommand.php

 use Symfony\Component\Console\Input\InputInterface;
 use Symfony\Component\Console\Input\InputOption;
 use Symfony\Component\Console\Output\OutputInterface;
 use vierbergenlars\SemVer\expression as SemanticExpression;
 use vierbergenlars\SemVer\SemVerException as SemanticVersionException;
 use vierbergenlars\SemVer\version as SemanticVersion;
+use PHPSemVerChecker\Report\Report;


Please follow alphabetical ordering.

tomzx · 2018-04-13T18:04:49Z

test/Console/Command/SuggestCommandTest.php

+            array(Level::PATCH, $this->getMockedVersion(1,0,0), '1.0.1'),
+            array(Level::MINOR, $this->getMockedVersion(1,0,0), '1.1.0'),
+            array(Level::MAJOR, $this->getMockedVersion(1,0,0), '2.0.0'),
+            //v2.3.4


Anything in particular not tested by the v1.0.0 case?

making sure the incrementing does zero out the next lower version number. Might be a bit more than required.

tomzx · 2018-04-13T18:05:30Z

test/Console/Command/SuggestCommandTest.php

+     * @test
+     * @dataProvider provideGetNextTag
+     */
+    public function testGetNextTag($level, version $version, $expected) {


This would probably be simpler to read using mockery.

tomzx · 2018-04-13T18:05:50Z

test/Console/Command/SuggestCommandTest.php

+use PHPSemVerChecker\SemanticVersioning\Level;
+use vierbergenlars\SemVer\version;
+
+class SuggestCommandTest extends TestCase


👍 for the added tests.

tomzx · 2018-04-13T18:09:55Z

src/PHPSemVerCheckerGit/ProcessedFileList.php

+     * @param string[] $files
+     * @param Scanner $scanner
+     */
+    public function __construct($originalAmount, array &$files, Scanner &$scanner)


I'd rather have the list of original (unfiltered) files here than the count.

I don't think there's any usage of that list after this point, but I'll adjust the class.

tomzx · 2018-04-13T18:10:07Z

src/PHPSemVerCheckerGit/ProcessedFileList.php

+     * @param string[] $files
+     * @param Scanner $scanner
+     */
+    public function __construct($originalAmount, array &$files, Scanner &$scanner)


Why a reference to the files array?

tomzx · 2018-04-13T18:10:46Z

src/PHPSemVerCheckerGit/SourceFileProcessor.php

+     * @param $exclude
+     * @return ProcessedFileList
+     */
+    public function processFileList(


Why is this on multiple lines?

tomzx · 2018-04-13T18:11:01Z

src/PHPSemVerCheckerGit/SourceFileProcessor.php

+     * @param Scanner $scanner
+     * @param array $files
+     */
+    private function scanFileList(Scanner &$scanner, array &$files)


Why references?

adjusting ordering of use statements in SuggestCommand adjusting ProcessedFileList to hold the actual file lists removing useless linebreaks from method definition extracting reflecting in test for easier readability

tomzx · 2018-04-14T15:11:14Z

src/PHPSemVerCheckerGit/Console/Command/SuggestCommand.php

@@ -50,8 +50,8 @@ private function getRepository($directory)
    }

 	/**
-	 * @param \Symfony\Component\Console\Input\InputInterface   $input
-	 * @param \Symfony\Component\Console\Output\OutputInterface $output
+	 * @param InputInterface   $input


Be careful, we still want the FQCN (fully qualified class name).

having FQN in PHPdoc looks inconsistent to me when they aren't also used elsewhere, Will fix those.

tomzx · 2018-04-14T15:12:26Z

src/PHPSemVerCheckerGit/ProcessedFileList.php

    /**
     * @return int
     */
-    public function getOriginalAmount()
+    public function getFilteredAmount()


Use "count" instead of "amount" (see https://english.stackexchange.com/questions/141564/differences-between-amount-count-number-and-quantity)

very useful, thank you.

changing Amount to Count for list size

Idrinth · 2018-07-24T06:06:22Z

is there any todo left @tomzx ?

tomzx · 2018-07-24T12:08:37Z

I'll review this again over the next few days. Thanks for the ping.

Idrinth · 2018-07-29T08:51:01Z

thanks :)

Idrinth added 10 commits March 20, 2018 22:26

splitting execute method

e42dc82

reducing duplicate code by extracting it into methods

4cdd1e6

extracting source file list processing into a class of it's own

9183fb2

removing variables, that are better provided by ProcessedFileList

46ce682

adding missing use statements

494ef03

adding missing use statement

88d5295

adding missing use statement

7e5eaeb

using use statement

de62b48

using exit code of run properly

0e425a0

adding some simple tests version incrementing

c565d4e

Idrinth added 2 commits March 21, 2018 00:41

adjusting unit test folder in config

61743aa

adding pre-release testcases

8d873fd

tomzx reviewed Apr 13, 2018

View reviewed changes

Idrinth added 2 commits April 14, 2018 13:36

removing useless references

49cf635

adjusting ordering of use statements in SuggestCommand adjusting ProcessedFileList to hold the actual file lists removing useless linebreaks from method definition extracting reflecting in test for easier readability

fixing issues found by scrutinizer

0899176

tomzx reviewed Apr 14, 2018

View reviewed changes

Idrinth added 2 commits April 14, 2018 18:46

reverting file to f0abc36

1645081

restoring FQN for phpdoc

54c5324

changing Amount to Count for list size

tomzx merged commit 54c5324 into tomzx:master Jul 29, 2018

Reducing complexity of execute method in SuggestCommand #27

Reducing complexity of execute method in SuggestCommand #27

Uh oh!

Conversation

Idrinth commented Mar 20, 2018

Uh oh!

Idrinth commented Mar 20, 2018

Uh oh!

tomzx commented Apr 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Idrinth Apr 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Idrinth commented Jul 24, 2018

Uh oh!

tomzx commented Jul 24, 2018

Uh oh!

Idrinth commented Jul 29, 2018

Uh oh!

Uh oh!

Idrinth Apr 14, 2018 •

edited

Loading