Duplicate Code

[TheFox]

Why do you use so much duplicate code?

• For example, you always use each table name as a string, and not the tableName() function of each record. Why not calling this function instead of writing the same table name over and over again? Beside the mirgrations, you call tableName() only on 5 different places. You could reuse tableName() from the record classes at least at over 90 places in the code.
• Why do you not use traits? For example, you have properties in Product Record, the same properties in Product Element and the same properties in the Product Query.
• For example, in ProductQuery, function beforePrepare(), you have at least 20 'commerce_products' strings in 20 lines. Even more. You could just reuse one string for all places.

And those are just some parts I looked at. I could probably write here another 90 examples..

When writing code you shouldn't repeat yourself (DRY). It seems like your main business is not programming. I don't want to teach you programming, but this code looks like it's written by some interns and you even sell this product.

[lukeholder]

@TheFox

You are correct we could reference the Record::tableName() function, but we are also following convention in the Craft CMS core. For example:

https://github.com/craftcms/cms/blob/882a87247f64a7f6a14c7e1924912e26300fce24/src/services/Sites.php#L1084-L1090

You will notice in most cases we are not using active record queries, and instead, write the query ourselves which we found is a performance improvement.

Personally, doing lots of string interpolation (to put the table name in) within a custom query string annoys me.

You are welcome to use the tableName() in your own code, but the cost of doing a find and replace if we ever rename a table is so insignificant it is not worth making it any DRY'er IMHO.

In the future, we might change the convention, but for now, we would rather it be consistent.

Thanks for the feedback.

[lukeholder]

BTW, I just made a commit to be consistent and use the table name string in those places where we using tableName().

956d363

Thanks :)

[andris-sevcenko]

For example, you always use each table name as a string, and not the tableName() function of each record. Why not calling this function instead of writing the same table name over and over again? Beside the mirgrations, you call tableName() only on 5 different places. You could reuse tableName() from the record classes at least at over 90 places in the code.

Mostly performance reasons. DB actions are localized in the service layer and table names are pretty unlikely to change, so introducing an overhead there just does not seem like the right move.

Why do you not use traits? For example, you have properties in Product Record, the same properties in Product Element and the same properties in the Product Query.

Product Record does not have actual properties you refer to (https://github.com/craftcms/commerce/blob/develop/src/records/Product.php#L17-L37), because it depends on the __set and __get functions for active record logic. Having record fields as actual properties would mean explicitly defining a list of attributes as well, so Yii knows which properties to attempt to save to database and which not. This, ironically, would violate DRY.

As far as Product Element goes, those properties should be defined similar to how it's in the Product Record to avoid ambiguity, but even if that wasn't the case, not all properties for the Product should be queryable and stuffing this all under a trait unnecessary couples the product and it's query. Furthermore, the vast majority of the properties don't even have the same definition. The id on the Product is always going to be a single integer, while the same property on the Query can be also an array of ids. The expiry date on the Product is a DateTime, while on the query it can be a string such as >= 2020-04-01. So using traits just introduces more complexity, instead of reducing it.

For example, in ProductQuery, function beforePrepare(), you have at least 20 'commerce_products' strings in 20 lines. Even more. You could just reuse one string for all places.

'commerce_products' is a table alias there, as the actual table can have a prefix. (https://github.com/craftcms/cms/blob/develop/src/elements/db/ElementQuery.php#L1550-L1552)

---

Sticking to DRY is nice, but sometimes duplication is far cheaper than the wrong abstraction.

[brandonkelly]

@TheFox Please stop posting issues here and other Craft CMS repos if you are unable to follow the code of conduct.