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

Some commits with minor improvements #30

Merged
merged 6 commits into from Feb 17, 2014
Merged

Some commits with minor improvements #30

merged 6 commits into from Feb 17, 2014

Conversation

pupi1985
Copy link
Contributor

@pupi1985 pupi1985 commented Feb 3, 2014

I have a branch with a few minor improvements that I thought it would make sense to share.

The only one that may make sense to explain is 2b4c4d5. The idea is basically to avoid calling qa_load_modules_with, which calls qa_list_modules($type) which ends up calling array_keys too many times. I'd say it should be faster but I haven't benchmarked it. Anyway, it is more readable to have qa_load_all_modules_with function rather than having to iterate over them all.

Additionally, I thought that removing the global variable $qa_modules from functions that only read it would provide more encapsulation. I would even add items to it by a function encapsulating as much as possible, but as I mentioned before, I'm just throwing some ideas.

@svivian svivian merged commit 45a66c6 into q2a:dev Feb 17, 2014
@svivian
Copy link
Collaborator

svivian commented Feb 17, 2014

Thanks for the updates. A few notes:

  • I renamed the encapsulation function to qa_list_modules_info to keep the naming inline with the other functions (they all retrieve registered modules so the function name doesn't need to contain registered).

  • I reverted the body_script placement. Script order is important so we don't want it to fail for anyone who has overridden that function. It's only 2 lines of JS so there is no drawback to being at the top.

  • Just a heads-up for future development: I'm using braces for any control statements that span multiple lines even if not necessary. In other words:

    // old/current style
    foreach ($arr as $var)
        if (something)
            do_something();
    
    // new preferred style
    foreach ($arr as $var) {
        if (something)
            do_something();
    }
    

I'll add more coding style details to the contributing.md document soon.

@pupi1985
Copy link
Contributor Author

Great, thanks. I'll keep the formatting in mind next time :)

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.

None yet

2 participants