Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

code cleanup #4

Closed

Conversation

TheDigitalOrchard
Copy link

@TheDigitalOrchard TheDigitalOrchard commented Mar 11, 2018

This is the official Zoho API SDK, yet its coding style falls well below today's standards.

Simple things like the following should be addressed:

  1. Files ending in PHP code do not need the closing ?> PHP tag.
  2. Whitespace should be consistent.
  3. Using __DIR__ instead of dirname(__FILE__).
  4. Using single quotes around strings that don't need parsing or have single quotes within.
  5. Using switch() where appropriate, instead of endless if/else() statements.
  6. Using typecasting instead of $str = "" . $var; for strings and $num = $var + 0; for numbers.

About item (6), the results of that + 0 technique are unpredictable. It depends on what value is in the variable. Look over this to see why:

Input             Output
'2' + 0           2 (int)
'2.34' + 0        2.34 (float)
'0.3454545' + 0   0.3454545 (float)

So it's always better to use (int) or (float) typecasts whenever a specific type is needed.

There are many third-party SDK's for Zoho's API, and one reason may be that this official library does not live up to today's standards. I will provide such improvements in a pull-request if you will review and accept the changes. Please acknowledge this and I will proceed.

Please see the changes made in this pull-request for example updates.

@TheDigitalOrchard
Copy link
Author

Also including a minor bug fix in EntityAPIHandler.php where the getTaxListAsJSON() function was missing a $ on the return $taxes; line.

@TheDigitalOrchard
Copy link
Author

Is anybody watching for these pull requests? Would love to get some kind of acknowledgement.

@devla
Copy link

devla commented Mar 21, 2018

I'll take a look in the morning, fully agree with you. Coding standards are at a low level, it should not even be the subject of discussion. After a few days of use, there seem to be serious bugs, especially bad is the way how settings are stored in the vendor folder, also logs, database credentials are hardcoded, when access tokens are stored in the file, somehow email address is missing and later authentication does not pass... Some methods return the record ID as float, refers to 6 and probably a lot more ...

Due to all of this, I have serious problems to deploy my custom Drupal integration to Pantheon and there is no other way than to try to improve the SDK. I wish I had never met Pantheon and Zoho SDK, but it's not my choice.

@dylanlawrence
Copy link

Dear @sumanthchilka,

Config in vendor?
Do we write in books at the library? -Why would we write in library code?
In fact many of us have composer install as part of the build process so we don't muck up our repo.

require_once('rtfm.php')
We have had namespace for almost a decade now php5.3 -2009.
When was this code written?
Is it some sort of auto generated java porting?

@TheDigitalOrchard
Copy link
Author

TheDigitalOrchard commented May 7, 2018

Fixed merge conflicts, and incorporated latest changes from master branch. Glad to see some work being done here, even if there's a long way to go before the code style is solid. :)

@zhujinxuan
Copy link

zhujinxuan commented Jan 17, 2019

Why not use phpcs to do some linting? Though phpcbf is not a good tool, it is OK to do some linting like consistent white space and the ending ?>

@pravesh-a
Copy link
Collaborator

Code cleanup will be done in the upcoming versions of the SDK

@pravesh-a pravesh-a closed this Nov 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants