Hopefully, my last blog post convinced you to avoid long lines. The php community has a nice coding style guide called PSR-2 that says the following:
There MUST NOT be a hard limit on line length; the soft limit MUST be 120 characters; lines SHOULD be 80 characters or less.
Basically this means: do whatever you want, but your editor will warn if you go above 120 characters. The 80 characters limit is probably here to encourage people to add linebreaks even if their line is just 121 chars instead of saying "come on, it's only 1 character".
Some projects take advantage of the limit being soft and disregard the soft limit for method signatures and will write this:
<?php
public function connect(?string $sourceId, $sourceValue, ?string $destId, $destValue = null, string $reference = null, bool $lazy = false, bool $weak = false);
rather than this:
<?php
public function connect(
?string $sourceId,
$sourceValue,
?string $destId,
$destValue = null,
string $reference = null,
bool $lazy = false,
bool $weak = false
);
Behind that decision are many arguments that I'd like to discuss here. Just to be clear though: I'm not suggesting you should always use multiline signatures. In my opinion, they should only be used when the line length exceeds 120 characters.
Method signatures are not real code
The main argument seems to be that method signatures are not "real code", because they declare things instead of doing things, and should thus not pollute the screen real estate. Except that they describe the interface of your method, and changes to them are very important, and should be reviewed carefully, because that is where most BC-breaks happen. Also, since php 7.0, function signatures are getting far more type hinting than before, which makes them longer, but also more useful, because they describe what type of variables you are dealing with. This is not always obvious unless you use hungarian notation, which you absolutely shouldn't.
Having multiline signatures is not only useful for reviewers, but for people that edit that part of the code. If one argument maps to one line, navigating from argument to argument is suddenly way easier than whatever IDE shortcuts you would use otherwise.
Still not convinced? Consider using code folding instead of making the code uglier for everyone else, including and especially reviewers. If this suggestion sounds stupid, maybe it is and the signatures do matter after all?
Parameter types are already described in the phpdoc
Then remove the phpdoc, unless you wrote something very interesting in it. Forcing people to describe each and every parameter in the phpdoc always results in the same paraphrasing comments in my experience, which has a very bad consequence: people stop reading the comments. Surely you can give the real estate you gave to the phpdoc to a piece of code? Most IDEs display comments in light gray nowadays, probably because of this, but I feel comments should not be mainly aimed at doc-extracting tools or annotation libraries. They should be helpful messages for your team, when your code fails to achieve enough clarity.
Long method signatures are an alarm signal about having too many arguments
That one is really moot, because if you have a 100000-character long line, it will still be one line, whereas if your screen is filled with a signature, the alarm signal will only be louder, and tools like phpmd will warn about your method having too many arguments.
The first step towards realizing how wrong things are for this method is definitely using a multiline signature:
<?php
/**
* Product constructor.
* @param \Magento\Framework\Model\Context $context
* @param \Magento\Framework\Registry $registry
* @param \Magento\Framework\Api\ExtensionAttributesFactory $extensionFactory
* @param AttributeValueFactory $customAttributeFactory
* @param \Magento\Store\Model\StoreManagerInterface $storeManager
* @param \Magento\Catalog\Api\ProductAttributeRepositoryInterface $metadataService
* @param Product\Url $url
* @param Product\Link $productLink
* @param Product\Configuration\Item\OptionFactory $itemOptionFactory
* @param \Magento\CatalogInventory\Api\Data\StockItemInterfaceFactory $stockItemFactory
* @param Product\OptionFactory $catalogProductOptionFactory
* @param Product\Visibility $catalogProductVisibility
* @param Product\Attribute\Source\Status $catalogProductStatus
* @param Product\Media\Config $catalogProductMediaConfig
* @param Product\Type $catalogProductType
* @param \Magento\Framework\Module\Manager $moduleManager
* @param \Magento\Catalog\Helper\Product $catalogProduct
* @param ResourceModel\Product $resource
* @param ResourceModel\Product\Collection $resourceCollection
* @param \Magento\Framework\Data\CollectionFactory $collectionFactory
* @param \Magento\Framework\Filesystem $filesystem
* @param \Magento\Framework\Indexer\IndexerRegistry $indexerRegistry
* @param Indexer\Product\Flat\Processor $productFlatIndexerProcessor
* @param Indexer\Product\Price\Processor $productPriceIndexerProcessor
* @param Indexer\Product\Eav\Processor $productEavIndexerProcessor
* @param CategoryRepositoryInterface $categoryRepository
* @param Product\Image\CacheFactory $imageCacheFactory
* @param ProductLink\CollectionProvider $entityCollectionProvider
* @param Product\LinkTypeProvider $linkTypeProvider
* @param \Magento\Catalog\Api\Data\ProductLinkInterfaceFactory $productLinkFactory
* @param \Magento\Catalog\Api\Data\ProductLinkExtensionFactory $productLinkExtensionFactory
* @param EntryConverterPool $mediaGalleryEntryConverterPool
* @param \Magento\Framework\Api\DataObjectHelper $dataObjectHelper
* @param \Magento\Framework\Api\ExtensionAttribute\JoinProcessorInterface $joinProcessor
* @param array $data
*
* @SuppressWarnings(PHPMD.ExcessiveParameterList)
* @SuppressWarnings(PHPMD.UnusedFormalParameter)
*/
public function __construct(
\Magento\Framework\Model\Context $context,
\Magento\Framework\Registry $registry,
\Magento\Framework\Api\ExtensionAttributesFactory $extensionFactory,
AttributeValueFactory $customAttributeFactory,
\Magento\Store\Model\StoreManagerInterface $storeManager,
\Magento\Catalog\Api\ProductAttributeRepositoryInterface $metadataService,
Product\Url $url,
Product\Link $productLink,
\Magento\Catalog\Model\Product\Configuration\Item\OptionFactory $itemOptionFactory,
\Magento\CatalogInventory\Api\Data\StockItemInterfaceFactory $stockItemFactory,
\Magento\Catalog\Model\Product\OptionFactory $catalogProductOptionFactory,
\Magento\Catalog\Model\Product\Visibility $catalogProductVisibility,
\Magento\Catalog\Model\Product\Attribute\Source\Status $catalogProductStatus,
\Magento\Catalog\Model\Product\Media\Config $catalogProductMediaConfig,
Product\Type $catalogProductType,
\Magento\Framework\Module\Manager $moduleManager,
\Magento\Catalog\Helper\Product $catalogProduct,
\Magento\Catalog\Model\ResourceModel\Product $resource,
\Magento\Catalog\Model\ResourceModel\Product\Collection $resourceCollection,
\Magento\Framework\Data\CollectionFactory $collectionFactory,
\Magento\Framework\Filesystem $filesystem,
\Magento\Framework\Indexer\IndexerRegistry $indexerRegistry,
\Magento\Catalog\Model\Indexer\Product\Flat\Processor $productFlatIndexerProcessor,
\Magento\Catalog\Model\Indexer\Product\Price\Processor $productPriceIndexerProcessor,
\Magento\Catalog\Model\Indexer\Product\Eav\Processor $productEavIndexerProcessor,
CategoryRepositoryInterface $categoryRepository,
Product\Image\CacheFactory $imageCacheFactory,
\Magento\Catalog\Model\ProductLink\CollectionProvider $entityCollectionProvider,
\Magento\Catalog\Model\Product\LinkTypeProvider $linkTypeProvider,
\Magento\Catalog\Api\Data\ProductLinkInterfaceFactory $productLinkFactory,
\Magento\Catalog\Api\Data\ProductLinkExtensionFactory $productLinkExtensionFactory,
EntryConverterPool $mediaGalleryEntryConverterPool,
\Magento\Framework\Api\DataObjectHelper $dataObjectHelper,
\Magento\Framework\Api\ExtensionAttribute\JoinProcessorInterface $joinProcessor,
array $data = []
) {
Besides, with the php 7.0 type hinting, long signatures are going to happen way more often, and you may want to know if the cause is the number of arguments, the length of the type hints, the length of the variable names or a combination of all of these. If you address the real problem behind that, you might end up with more little methods with fewer arguments, which is usually easier to reason about. Don't sweep things under the rug, heed the warnings and design better code. Symfony's Options Resolver Component might help with that if your arguments are actually interchangeable configuration options for something.
Framework X, which my library uses, forbids multiline signatures
You should strive to make your library framework-agnostic, and segregate it from framework-integrations layers. Style is part of that, use your own, don't let bad decisions spread like as many diseases, especially if you can't help but having many arguments with long names and long type hints because you love Java.
Switching to multiline signatures will be the source of many conflicts
I'm a maintainer of some libraries that do not use multiline signatures. Guess where a big part of the conflicts happen? That's right, they happen on long lines, which nowadays practically only occur with long signatures. So maybe the conflicts are worth it in the long run.
Also, consider that switching to multiline signatures does not have to be done in one-shot: just allow switching to them, and enforce them on newly created methods when they are too long.
Multiline signatures look weird
Remember switching to PSR-2? I remember, it felt horrible at first to have to write
<?php
if (defractulatorIsReady()) {
defraculate();
}
instead of
<?php
if (defractulatorIsReady())
{
defraculate();
}
like some of us did for years. But it was also nice to finally have a standard everyone could agree with, so I went with it anyway, and it felt horrible… for about half a day, and that was it. So if multiline signatures feel horrible, please try them a bit longer, instead of clinging to old habits, the language evolves, and so should you!
Top comments (6)
I voted a unicorn because it's you. :-)
Your old one line signatures' enemy will try to answer you. <3
Be prepared, you will be surprised for some parts. ;-)
This is true. This is not about a debate here, it's a fact.
Well, this is a little bit more complicated than that in my opinion. I'll try to explain clearly.
You are quite right, since php 7, most of the phpdoc type-hinting became duplicate and useless. But some only.
For basic type, they are, for sure. But for object, it could be more complicated. Sometime you have to specify some sub-classes on the phpdoc to get IDE completion working.
So you will tell me that we can only specify those special parameter. Well, it could be but two points:
So now you will maybe tell me that in this case, we should put the phpdoc only when some parameters needs to be specified and nothing for the other.
It's an idea indeed, but here too I see two points:
I'll conclude this part because the PHPDoc can be a good discussion for another article: At this time, I think both are great.
Well, I think you will lie if you deny to talk about Sonata Project as an example. :-)
I don't totally agree with your argument. For the case of Sonata, we don't have a lot of time maintaining the repositories and I really think following a big standard is give more benefices than define our own.
And, as 99% of our projects are Symfony bundles, it makes sense to follow SF standard.
We have indeed some necessary exceptions, like short array for example. Yeah, no need to tell me, I opened a trolling hole here. :-D
Beside the comment I did, it's a great article. Really. And I agree especially for that point: PHP 7 type hinting give us more flexibility and force us to get new reflection and debate about multi-line signature and the usage of PHPDoc.
Also one concern for my case: Beside the rule you choose (multi or single line), I think this should apply every where, like the
,for the multi-lined arrays. You know, for git conflicts? :-)Best regards.
Great answer thanks!
Let me address a few of your points:
Indeed, I will
The typing is far, far more reliable than the comments. IDEs should strive to use the former if they do not already. Comments rot, and are often wrong, as phpstan shows.
And I think they can take it. Not a big deal if they themselves feel that the comments are a bit redundant, which they very often are.
Not only! Api-platform gives the same reason when asked, but yeah, Sonata is of course the example I had in mind, but I didn't want to point fingers (except for Magento :P)
A bot can hardly determine if a parameter needs a comment, since now the typing is described, all there is to describe is non-obvious behaviors, which you cannot know about unless you understand the code.
We have linters to cherry-pick the rules we want. We could do that once and for all, and that wouldn't be a big deal. The Symfony standard minus the rules we don't like could be great.
I don't think we should go that far, even if that kind of stuff can indeed be the source of some conflicts.
I'm not talking about IDE relying, but comment generation. ;-)
I think that feature is now useless and should be removed. It just empowers devs to generate what often ends up being useless noise.
Maybe it can be improved, but not removed.
Otherwise, I'll just use ed. :-P
I think it should be completely okay to "clutter" an interface with signatures, because you are definitely not supposed to have many methods in an interface, see the ISP. Plus, even if you don't change them often, they should still be easy to read too, especially in the Github editor.