Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Roadmap for 3.0 #39

Open
4 tasks
mariano opened this issue Mar 7, 2017 · 5 comments
Open
4 tasks

Roadmap for 3.0 #39

mariano opened this issue Mar 7, 2017 · 5 comments

Comments

@mariano
Copy link
Owner

mariano commented Mar 7, 2017

I'd like to release 3.0 soon. On the list of to-do I see:

I'd also like to make php7 the minimum requirement, and consequently move to full strict typing.

What are your feelings retarding this roadmap and particularly the new minimum php version to be supported?

@mariano
Copy link
Owner Author

mariano commented Mar 7, 2017

Food for thought @kaecyra @Revisor @dominics @dwoofhub and everyone else

@Revisor
Copy link

Revisor commented Mar 7, 2017

There is a place I would like to see improved. It confused me when I first contributed to Disque-php and it still confuses me. I think it's hard to maintain and to add new code.

It's the way how Commands, Arguments and Responses are structured.

Commands contain

  1. A command name
  2. Options
  3. Mapping of options to Disque arguments
  4. Argument validation
    1. Either explicitly as traits from Command\Arguments
    2. Or processed by BaseCommand, in that case the behavior is defined by a constant
  5. A response interpreter from Command\Response, defined by a class name in a protected property

Going by the list, there are several points that could be improved in my opinion.

Ad 2. Options + 3. Command arguments + 4. Validation

I suggest moving the whole code around Disque-php commands and arguments, their validation and their mapping to Disque arguments to their own object.
For example the AddJob command, which is quite complicated as to what it accepts, could accept AddJobArguments. All required arguments would be required in the constructor, each optional argument would have its own method:
Instead of

addJob(string $queue, string $payload, array $options = [])

one would call it like this:

$addJobArguments = new AddJobArguments($queue, $payload); // These arguments are always required
$addJobArguments->setTimeout($timeout); // These arguments are optional
$addJobArguments->setDelay($delay);
$disque->addJob($addJobArguments); // Go

Advantages

  • The validation could occur immediately, it could even be partly enforced by PHP, because we could say eg. that the argument for setTimeout must be an integer.
  • The command would only get already validated arguments
  • Not only that, the argument object could also build the whole argument string for Disque
  • It would be easier for the library user to use the command, because they would be guided by method signatures instead of array keys

I also wouldn't use traits at all as they make the code more confusing than composition, in my opinion (and even more so when traits are hierarchical in that they also use other traits).

Ad 4. Validation

Some method names used in validation could be more explicit.
Command\Argument\ArrayChecker::checkFixedArray($elements, $count, $atLeast) does two things:

  • assertElementCount($array, $expectedCount)
  • assertMinimumElementCount($array, $expectedMinimumCount)

Command\Argument\StringChecker::checkStringArguments($arguments) = assertAllArgumentsAreStrings() ("check.../validate...", the verb is not as important as to describe precisely what the method does)
Command\Argument\StringChecker::checkStringArgument($arguments, $numberOfElements) has a very similar name to the previous method and does two things at once

Similarly the methods in Command\Argument\OptionChecker could also be named more explicitly.

Ad 5. A response interpreter

I would inject the proper interpreter explicitly into the command instead of instantiating it inside from a class name

new AddJob(new AddJobResponse())

I would clean up the inheritance tree of responses. It's too complicated and used for code reuse, which might be done better with composition.

Smaller stuff

  • Command\CommandInterface::parse($body) could be Command\CommandInterface::parse($responseBody). There are several "bodies" in Disque-php, and even though it's clear contextually which one is here, I would be extra explicit in order to save the reader precious brain energy
  • Command\Response\ResponseInterface::setBody($body) and Command\Response\ResponseInterface::parse() could be combined into one method Command\Response\ResponseInterface::parse($reponseBody)
  • I would prefix abstract classes with the word "Abstract" instead of "Base"

All in all I would avoid the pattern, where constants and properties in a class change the behavior of the class. That's probably the most confusing thing in Commands, I find. Instead I would make all behavior explicit, and if shared, shared explicitly by composition.

Hopefully the suggestions are understandable, I wrote them more or less as a stream of thoughts.

What do you think about this?

Regarding the move to PHP7, I support it. The new language features could help eg. with some of the validation.

@mariano
Copy link
Owner Author

mariano commented Mar 7, 2017

I like your approach to command arguments, and indeed validation would be much simpler with scalars. I too feel uneasy about how things are structured now, so I'm open to improving it. Great suggestions @Revisor

@dominics
Copy link
Contributor

@mariano Can you add #43 to the 3.0 roadmap?

@mariano
Copy link
Owner Author

mariano commented Mar 11, 2017

@dominics done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants