Skip to content
This repository has been archived by the owner on Mar 16, 2020. It is now read-only.

Comments for specific files #17

Open
Quetzacoalt91 opened this issue Sep 26, 2014 · 1 comment
Open

Comments for specific files #17

Quetzacoalt91 opened this issue Sep 26, 2014 · 1 comment

Comments

@Quetzacoalt91
Copy link
Contributor

  • /config.api.php
    • l.49 to 56: Why not using PS_MODULE_DIR or DPD_POLAND_MODULE_DIR ?
  • /dpdpoland.lang.php
    • Why this new layer of translation has been implemented ?
  • /dpdpoland.php
    • l.226 to 240: Using PrestaShop's ObjectModel manage date_add and data_upd for you.
    • Registering the hook paymentTop do not need to be in a if().
    • Checking if soap is activated on the server is not needed in getContent, because it has already been checked in install()
    • function getContent: Strange execution with <controller>::init() before new <controller>. Why don't you call init() in the controller construct ?
    • function getPaymentModules(): On PrestaShop 1.4, you can make the following call:
Hook::getModulesFromHook(Hook::get('payment'));
  • /classes/arrange_pickup.controller.php
    • l.200 & 202: Braces misplaced
      and / or
    • l.200: Is it the properly test ? --> *if(!extra_time_from || $extra_time_from ... *
  • /classes/configuration.controller.php
    • function getZonesForCarriers(): Code executed 3 times, Should be merged in a new function, which will be called 3 times with the needed arguments
  • /classes/countryList.controller.php
    • l.140: Why do you rewrite the existing id ?
  • /classes/dpd.classic.service.php, /classes/dpd.standard.service.php, /classes/dpd.standard_cod.service.php
    • They are almost the same. You should create a single parent class and your existing controllers will extend it. The different values would then be stored in each subclass and then used by the superclass.
  • /classes/manifestList.Controller.php
    • l.145 & 146: $pdf->addPDF(...) of the same file is made twice.
  • /classes/packageList.controller.php
    • **l.35: Affectation of a test result unused elsewhere. Could be written directly in the if().
    • l.51, 54, 78, 90, 128, 161: Because we have arrays here, you should better something like count($int_packages)
    • l.130 to 158, l.163 to 190: These lines could be merged and the different values stored in a array (package list & file name). Then, a foreach would executed the merged content.
  • /css/backoffice.css
    • The attributes in this file may changed unattended parts parts of the page. You should add a class specific to you module to stay in your perimeter.
@Quetzacoalt91
Copy link
Contributor Author

Updated

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

1 participant