Skip to content
This repository has been archived by the owner on Feb 25, 2018. It is now read-only.

Reason for inclusion of all files? #148

Open
scarstens opened this issue Apr 29, 2014 · 2 comments
Open

Reason for inclusion of all files? #148

scarstens opened this issue Apr 29, 2014 · 2 comments

Comments

@scarstens
Copy link

https://github.com/apotropaic/parse.com-php-library/blob/master/parse.php#L2

  1. These are class files, why are we not using "include_once" here?
  2. Is there a reason why you are including all files? Its very inefficient to load everything they way you do. All of the extension classes should instead include the parent class, and/or any other dependencies.

Example:

  • parse.php (should have no additional include files... except for Config, though I'm not thrilled about the use of the config class in general and think the config params should be part of the class instantiation.
  • parseObject.php (should include_once the parse.php file)
    Result: I can include parseObject.php, it will load the dependency class, and I can then work with objects, without loading all the other classes I'm not using.

If there is not some reason for this, I'll include the fixes to this in my next pull request.

@andrewscofield
Copy link
Owner

I actually left off working on an autoloading solution, so the include file will work for now, but won't be necessary when I get that new version up and going.

As for config files, I've gone back and forth on this. V1 did loading throw class instantiation:
https://github.com/apotropaic/parse.com-php-library/blob/parse.com-php-library_v1/parse.php

And there were a couple reasons why I switch to a config file model in v2. But one developer got me thinking about this a few months back and I proposed this idea, but never really landed on it:
eba2017

With v2 I wanted to make the entire library more flexible to use as much or a little as you wanted of it. Which ironically I botched as you've pointed out with all my include statements haha!

One reason I went to the config file model was it seemed to make the post sense to achieve this line here: https://github.com/apotropaic/parse.com-php-library/blob/master/parseObject.php#L15

So, I can call just $object = new ParseObject(); without having to include the app id and keys every single time. Maybe there is a different or better way of doing this. I'm totally open to feedback and change.

Like I said with the new version, I'm looking at adopting modern standards, and make this entire library a composer package. Config files are handled much different there and should solve problems there.

@scarstens
Copy link
Author

I like the new config idea better then the existing, however it still requires that you "create a file and place it in the library". I didn't develop a way around the config file yet, so I don't have a "solution" to provide you with. It is convenient to not have to pass all that info, as you mention. But what if I wanted to connect to 2 different Parse.com instances? I think its impossible with the current setup, but you can only have 1 config. I'll have to give the config concept some more thought.

I did however reverse the file inclusion to follow the "class dependency" method, and added it to my pull request: #149

Thanks for the responses, its been refreshing to meet another active developer on Github... instead of the build and abandon projects I usually run across.

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

No branches or pull requests

2 participants