Refactor module

[ksiamro-netz]

Hey, I received request to check the module code and provide some info how to write it better. I also refactored the code a bit. Especially the Controller Classes and one of the Observer.

Basically, code standards for Magento2 can be found here:
https://devdocs.magento.com/guides/v2.3/coding-standards/bk-coding-standards.html
Which is bare minimum imo.

In addition:
https://developer.adobe.com/commerce/php/best-practices/extensions/

For better understanding Magento and rules:
https://developer.adobe.com/commerce/php/development/
https://developer.adobe.com/commerce/frontend-core/guide/

In addition, some Libs can be used to check code due to the standards. For example
https://github.com/magento/magento-coding-standard
https://github.com/PHP-CS-Fixer/PHP-CS-Fixer
https://github.com/squizlabs/PHP_CodeSniffer
Those libs will help with keep the code more structured and more clean. Obviously, the libs should be called manually to check code.

Of course, would be great to have also Unit and Integration tests.

Please read my comments, usually mentioned as #.
Please add PhpDocs everywhere and in general comply with PSRs.

Important note: Please move the Observer for sales_order_shipment_save_after event, to Cronjob.
Currently, every product in order may be loaded twice, which may greatly increase the process time. Using Cron, we would move the responsibility to another, external process, that will be ran in the background. To mark order, that were processed, You may add a new column to sales_oprder table.

Big help would be to use PHPStorm with installed plugins like PhpStan, Plugin for Magento, or even PHP Inspection.
PHPStorm can display deprecated method and may show badly written code. To fully use the functionality, try to work with the module on an already running shop instance locally, with access to the Magento Core Code.

Moreover, consider using Interfaces for Services or Models. Especially for the API Model, which connects to Review service.
In PHP Files, do not use use for catalogs. Define whole path to required Class. It will add some additional lines in use section, but it is safer and better to work with.

Consider moving repeatable code to another class like Model or so.

Adapt code to use PHP in version 8.1 at least. Lower PHP versions are not supported anymore, therefore there is no sense to support it.

Do not load products one by one if not required. Every Product load will require connection to DB. With for few products is no issue, but makes a lot of trouble with many products. Product Collection can be used to load multiple products faster. For loading one entity use Repositories, no Models.

In Feed Controller, there may be problem with timeouts anyway. Especially for shops with thousands or a hundred thousand products.
I think about 2 solutions now:

• Use pagination as for API Controller - I have already prepared code for that
• Create Cron, that will collect all data once or few times per day and put it to a custom Table. The Feed Controller would only fetch and display entries from the Custom table. Can also be used by API Controller.

Do not use ob_start and set_time_limit. If You need set_time_limit(0), it means, the code is wrong written and optimized. In the worst case, not only the Timeout may be thrown, but also it may create some issues with the shop, if the code creates high loads and chains of loads and so on.

Additionally, when You create code, keep in mind scalability -> there are places where, behavior will be different per shop. For example, collecting prices or availability of product, or even, when the Order is Closed / Delivered.
Create events or functions to use plugins for managing some behavior if necessary.
Otherwise, it may be necessary for developers to override the whole classes with whole code, to just change one line to change the small behavior and adapt it to shop requirements (which basically is not best approach).

On top of that, please keep module.xml file updated as well as require in compose.json.

Also, its okay to move some code to another private functions. Using proper naming and PhpDocs it may make the code more clear. One big methods that do everything usually are not good approach. Using more methods also helps with working with the code for another developer and may help with using plugins or so.

Do not use ObjectManager to create objects. I can not image situation, when You really need to use it. Comply with injecting required classes in __construct(), even if will be used just once.