Remove parentheses and make more Ruby standard #3

Merged
merged 46 commits into from Mar 5, 2014

Conversation

Projects
None yet
4 participants
Contributor

tjchambers commented Mar 4, 2014

I am going to try and make use of this SDK and I thought I would contribute back by making some "improvements". I intend to continue making changes and you are welcome to accept or deny these. I am going to also add some spec tests and integrate Travis CI to test with.

Owner

asposeforcloud commented Mar 4, 2014

@tim,

Thank you for taking time to suggest and contribute. Any improvement suggestions are warmly welcomed. Our developer will shortly go through the pull request and take appropriate action.

tim commented Mar 4, 2014

@asposeforcloud I think you meant @tjchambers.

Good start on making the code more Ruby-esque 👍

Owner

asposeforcloud commented Mar 4, 2014

Sure, I meant @tjchambers :)

Contributor

tjchambers commented Mar 4, 2014

I am going to make a slew of additional changes. I have found at least 3 visible bugs. And it appears (no disparagement intended) that someone with PHP background wrote the Ruby code. It will not work as anticipated in many "if" conditions where Ruby and PHP diverge. In addition there is much excess code.

My concern is I am cleaning up code that has no unit tests AFAICT. So I have no proof it worked before and none after. So I should write the unit tests first.

I think I will stop where I am - push what I have changed in as piecemeal sections as I can. And then endeavor to write unit tests for what I need. I cannot afford the time to write unit tests for everything, but there are some core items that have obvious bugs that need to work for the app I need to function.

Contributor

tjchambers commented Mar 4, 2014

Just to be clear I really only need a relatively small part - the part that converts a docx to PDF. The support for storage, and the overall URI management will be integral to that.

tjchambers added some commits Mar 4, 2014

@tjchambers tjchambers Base file is commonly needed
... so raise exception immediately if not provided during object initialization
c616765
@tjchambers tjchambers Ensure request is set to something so it can be tested later 4bea719
@tjchambers tjchambers Code clean up 768afd4
@tjchambers tjchambers Simplify URI sign 8437b3a
@tjchambers tjchambers Simplify code adb9cf4
@tjchambers tjchambers Bugfix: misspelled true "ture" 29605ae
@tjchambers tjchambers Use natural ruby return 19d27ca
@tjchambers tjchambers Reformat array to make more readable 5ebb4e8
@tjchambers tjchambers Code spacing changes 1b336c6
@tjchambers tjchambers Use natural Ruby return 6fc0ac9
@tjchambers tjchambers Remove unnecessary initialize block 7d6fbf0
@tjchambers tjchambers Cleanup code ef11c04
@tjchambers tjchambers Bugfix: string append did nothing 59e0cef
@tjchambers tjchambers Simplify return code 9318708
@tjchambers tjchambers Simplify return code bdf66f7
@tjchambers tjchambers Simplify return code daf195a
@tjchambers tjchambers Cleanup code and formatting a1ef12c
@tjchambers tjchambers Code format improvements 690521e
@tjchambers tjchambers Mandate proper filename during object creation eeefa31
@tjchambers tjchambers Code format cleanup 1d52055
@tjchambers tjchambers Correct use of of !if a76e6e3
@tjchambers tjchambers Filename is required - detect empty during initialization 2a3c55d
@tjchambers tjchambers Simplify return logic 9869520
@tjchambers tjchambers Fix formatting in method 24df652
@tjchambers tjchambers If remote folder empty that's fine d8466c2
@tjchambers tjchambers Simplify parameter precheck a712dea
@tjchambers tjchambers Code reformatting b623281
@tjchambers tjchambers Simplify return value handling 618d7de
@tjchambers tjchambers Simplify parameter appending 9e770d5
@tjchambers tjchambers Code reformatting c2b5a55
@tjchambers tjchambers Blank line cleanup 4b37d85
@tjchambers tjchambers Simplify return value handling a846961
@tjchambers tjchambers Cleanup code spacing 8477953
@tjchambers tjchambers Simplify parameter appending 6116dab
@tjchambers tjchambers Code format cleanup a8e4d75
@tjchambers tjchambers Code format cleanup 333f072
@tjchambers tjchambers Revamp parameter prechecks 9254d90
@tjchambers tjchambers Simplify return processing 411db0c
@tjchambers tjchambers File name is always needed - ensure it is provided 8d79b24
@tjchambers tjchambers Simplify return processing f12ec13
@tjchambers tjchambers Improve parameter prechecking d36c43e
@tjchambers tjchambers Improve parameter prechecking 61f3af8

Just wanted to point out that theoretically output_filename cannot reach here and be empty. SO when I made the fix I eliminated this code which by my estimation is unreachable. This may be a bug in the design though.

Contributor

tjchambers commented Mar 5, 2014

Just wanted to say I am done for now. A couple thoughts:

  • It is possible that the protocol for consistency returns false/true or empty strings, but that is somewhat inconsistent in this Ruby API and empty strings are not common as return parameters in Ruby AFAIK.
  • There does seem to be some hardcoding of HTTP response codes (i.e. 200) that should use constants from a library.
  • The voluminous checking for empty - which I did not change, does not account for passing NIL which would not get caught. IMHO that should also be addressed.
  • Note that PHP treats empty strings and zeros as well as null as false. Empty strings in Ruby and Zero are true and nil is falsy and false is falsy. Not sure that was consistently translated (my inference is the Ruby was a rewrite of PHP code).
Owner

asposeforcloud commented Mar 5, 2014

@tjchambers,

Thank you for a comprehensive analysis.

Yes, you are absolutely correct in your findings related with PHP rewrite. In fact, this SDK followed the following porting path:

.NET -> Java -> PHP -> Ruby

This was obviously not a correct path so eventually we ended up with issues and we are to fix those issues. Thank you for your detailed analysis which is quite helpful.

A developer from the team has been assigned the task of revamping the Ruby SDK based on your thoughts. So, you will soon observe the progress.

Once again, thank you for your quite helpful review.

Collaborator

assadvirgo commented Mar 5, 2014

@tjchambers Thanks for your analysis I'm reviewing all the changes you suggested. I'll keep in touch with you for further reviews from your side. Once again thank you so much for being so kind.

assadvirgo merged commit ecaec6d into asposeforcloud:master Mar 5, 2014

Collaborator

assadvirgo commented Mar 5, 2014

@tjchambers we've accepted all your changes and upon initial testing changes didn't break anything. Now we are going write some unit tests and then implement same improvements across all remaining modules in SDK.Once again thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment