brianlmoon / GearmanManager

PHP daemon for managing gearman workers
http://brian.moonspot.net/GearmanManager
BSD 2-Clause "Simplified" License
682 stars 190 forks source link

Hacky code #153

Closed ReduktorSpalin closed 5 years ago

ReduktorSpalin commented 5 years ago

Code in revision 2.1.1 does not follow best practices for writing OOP software.

First of all when we use abstract class we cannot use (possibly undefined) functions. If function does not contain body, it should become abstract function. That way we force this kind of interface on inheriting class, making sure not to use code that smells bad.

I am referring to definitions of start_lib_worker and validate_lib_workers. The are missing in this revision, but (because of PHP allowing such incorrect usage) it works, because either way GearmanPeclManager and GearmanPearManager implement those functions.

brianlmoon commented 5 years ago

Your PR is valid. I would tone down the rhetoric about what “cannot” be done. Or what is “incorrect” usage. And what is “Hacky”. It rubs people the wrong way. If not for years of therapy and experience, I would have rejected your PR for being insulting. I have learned that it is not always what we say that people have a problem with, but how we say it that.

ReduktorSpalin commented 5 years ago

Sorry for the content of PR. I never meant to offend you or any contributors in any way. Great thing you create that project. When I referred to "code smell" I though of technical "slang" for what my IDE (I use Intellij PHPStorm) and any other tool call.

I will consider your advice about the way I write Issue content. The moment I wrote it I thought only in "problem - solution" context (pointing out fragments of code that still requires improvement), not trying to insult you.

Many thanks for understanding and pointing out the flaws of way I write Issues.

Thanks once again.