Can you review?

Question

PHP

I decided to write my bike (client) for telegram. Not all functionality has been implemented there yet, but I would like some constructive criticism. Can you help with this?
link to turnip

Answer 1

Eyes rejoice when you see such code, no need to ask - where is the one who wrote it
? -something complicated in such a notation ->where()->where()->group(). If something just returns another object, it might be a factory. I mean even be called a factory. Builder means "there are so many parameters here that it's stupid not to put them in one place, and you need to take one from here, another from here, combine cleverly" and so on. so he's a builder.
3. There is a place where you really like if ($a instanceof Class) and times 20. You can throw the class names as keys into an array and stupidly check isset(). Unless you plan to inherit, then isset () will not be very good. But in any case, forich will simplify this code by 30 lines
4. It’s as if it doesn’t turn into a holivar for the actions, but feel free to throw the standard Phpsh actions with a small remark - wrap them in your namespace (in this case, the categories will remain standard, and your module will throw, as it were, "their own"). For cases when you really need to log something and pull it out of the exception, I would provide, like you have a ClientException, but instead of Response, just put ... $arguments there. What you throw is what you get later. That is, the standard messages, code and the previous one will be like all the exceptions - and the arguments - to log them. I don’t think that you benefited much from the fact that you made an action in which the first parameter is necessarily a response. Now it can only be used here and nowhere else. If the exception results in any action other than "catch", "throw", "notify" or "log" - it is very likely that this is not an exception. Although this option may be why not.
5. Interfaces lie together with classes, this is not bad, but next to the interface there is no same class, but there are others that implement the interface. It looks like an abstraction that could be. ActionInterface.php, Action.php and the Action folder where Actions are already located. But it's also cosmetic. In the actions themselves, I would still add the word Action, despite the fact that the namespace is also Action. If you open a lot of files in the editor, then the same names can be duplicated on top, and it's clear that here we are talking about an action, but not about an action.
6. That's what seemed strange to me - you have a bunch of "actions" implying that this is an "action". But inside there lie entities called "Action". Well, that is, you have created many classes whose task is to check the input arguments by type (throw InvalidArgumentException if the cant, in fact), and nothing more. Well they have getters and properties. This state does not change in any way, it is simply set and validated. Can. Looks nice, neat. But a ghost is running behind me, which hints that Zarraza, he created so many files, he didn’t get sick? However, if the bot's apishka is unpredictable - it returns one thing for one, the other for the second and there is nothing in common - then this method of validation is acceptable. Unfortunately, except for the developer who will sculpt on the basis of this thing, no one will be able to react to them in any way, because. InvalidArgumentException is, first of all, "to notify the developer that something that should not have got here", such an error will not work to catch and translate into several languages. That is why they resort to validation where this thing will be used by a less experienced developer or user.
Probably everything, you can chat in the same cart, if you are interested in more details. Generally liked

Answer 2

Even the code is reluctant to look, commits like refactoring part 2 already say everything. What for to think, why describe the changes, if you can just do: refactoring part 2, refactoring part 3, code part 1, code part 2, my mom love pizza