clean-up work

[Clean Code](https://www.42lines.net/2012/07/06/clean-code-reducing-wtfs-per-minute/) === Quality Code
fixes #91, #87
<a href='#crh-start'></a><a href='#crh-data-%7B%22processed%22%3A%20%5B%22https%3A//github.com/zencart/zc-v1-series/pull/96%23issuecomment-48860671%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23issuecomment-48861613%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23issuecomment-48862101%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23issuecomment-48863308%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23issuecomment-48864059%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23issuecomment-48970907%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14864288%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14864343%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14864668%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14865168%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14909339%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14909340%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14910603%22%5D%2C%20%22comments%22%3A%20%7B%22General%20Comment%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23issuecomment-48860671%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22%3A-1%3A%5Cr%5Cnplease%20remove%20this%20PR.%5Cr%5CnThe%20tone%20of%20the%20commit%20messages%20is%20arrogant%20and%20disrespectful%20to%20others%22%2C%20%22created_at%22%3A%20%222014-07-14T02%3A35%3A22Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/227354%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/zcwilt%22%7D%7D%2C%20%7B%22body%22%3A%20%22%40zcwilt%20cleaned%20up%20commit%20messages%20via%20%60rebase%60%22%2C%20%22created_at%22%3A%20%222014-07-14T03%3A03%3A41Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1590605%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/texdc%22%7D%7D%2C%20%7B%22body%22%3A%20%22thanks%20for%20updating%20that.%20however%20think%20https%3A//github.com/texdc/ZenCart/commit/6b6638dfac8654e3bd7e4abbf938516e316cd93c%20slipped%20into%20your%20rebase%22%2C%20%22created_at%22%3A%20%222014-07-14T03%3A17%3A50Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/227354%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/zcwilt%22%7D%7D%2C%20%7B%22body%22%3A%20%22%40zcwilt%20%20Yep%2C%20tricky%20things%20happen%20during%20%60rebase%60%3A%20very%20useful%2C%20but%20also%20very%20dangerous%20-%20always%20use%20with%20great%20care.%22%2C%20%22created_at%22%3A%20%222014-07-14T03%3A51%3A54Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1590605%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/texdc%22%7D%7D%2C%20%7B%22body%22%3A%20%22yeps%2C%20had%20that%20happen%20to%20me%20yesterday%2C%20%22%2C%20%22created_at%22%3A%20%222014-07-14T04%3A13%3A02Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/227354%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/zcwilt%22%7D%7D%2C%20%7B%22body%22%3A%20%22%3A%2B1%3A%3Ca%20href%3D%27%23crh-update-none%27%3E%3C/a%3E%22%2C%20%22created_at%22%3A%20%222014-07-14T22%3A57%3A17Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1590605%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/texdc%22%7D%7D%5D%2C%20%22title%22%3A%20%22General%20Comment%22%7D%2C%20%22Pull%206a6b8ae5d0cdcc92786161b8efd334f229391da6%20admin/includes/functions/general.php%2050%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14864343%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Would%20it%20be%20better%20to%20use%20the%20existing%20Zen%20Cart%20methods%20in%20query_factory%20for%20prepared%20statements%20instead%20of%20using%20sprintf%3F%22%2C%20%22created_at%22%3A%20%222014-07-14T04%3A55%3A32Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/2963958%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/lhungil%22%7D%7D%2C%20%7B%22body%22%3A%20%22Ya%2C%20not%20a%20fan%20of%20heredoc%20or%20sprintf%20for%20SQL%20queries%20either%2C%20at%20least%20not%20in%20the%20v1.x%20code%20base.%5Cr%5Cn%5Cr%5CnAlso%2C%20aren%27t%20RIGHT%20JOINs%20considered%20to%20be%20slower%20than%20LEFT%3F%22%2C%20%22created_at%22%3A%20%222014-07-14T05%3A17%3A53Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/404472%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/drbyte%22%7D%7D%2C%20%7B%22body%22%3A%20%22Indeed%2C%20since%20the%20query%20was%20rewritten%20with%20the%20tables%20reversed%2C%20a%20LEFT%20JOIN%20would%20be%20required.%20The%20PR%20brings%20back%20nulls%20for%20attributes%20without%20downloads%2C%20which%20is%20the%20opposite%20of%20the%20desired%20result.%5Cr%5Cn%5Cr%5CnBut%20there%27s%20no%20need%20to%20rewrite%20the%20query%20at%20all.%20The%20original%20query%20is%20fine.%22%2C%20%22created_at%22%3A%20%222014-07-14T05%3A46%3A15Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/404472%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/drbyte%22%7D%7D%2C%20%7B%22body%22%3A%20%22%3A%2B1%3A%3Ca%20href%3D%27%23crh-update-none%27%3E%3C/a%3E%22%2C%20%22created_at%22%3A%20%222014-07-14T22%3A57%3A17Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1590605%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/texdc%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20admin/includes/functions/general.php%3AL2558-2594%22%7D%2C%20%22Pull%206a6b8ae5d0cdcc92786161b8efd334f229391da6%20testFramework/unittests/support/zcAdminTestCase.php%2012%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14864288%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22I%20believe%20using%20an%20%5C%22if%5C%22%20block%20creates%20more%20readable%20code%20than%20an%20implied%20%60if%60%20via%20%60%7C%7C%60.%20I%20would%20recommend%20against%20this%20line%20change%20%28although%20would%20not%20be%20opposed%20to%20the%20addition%20of%20curly%20brackets%20to%20the%20original%20code%29.%22%2C%20%22created_at%22%3A%20%222014-07-14T04%3A50%3A54Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/2963958%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/lhungil%22%7D%7D%2C%20%7B%22body%22%3A%20%22%3A%2B1%3A%3Ca%20href%3D%27%23crh-update-complete%27%3E%3C/a%3E%22%2C%20%22created_at%22%3A%20%222014-07-14T22%3A57%3A17Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1590605%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/texdc%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20testFramework/unittests/support/zcAdminTestCase.php%3AL11-22%22%7D%2C%20%22Pull%20522084e112948161f57f197dc60bae1a70e73118%20admin/includes/functions/general.php%2040%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14910603%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Using%20a%20left%20join%20may%20cause%20rows%20to%20be%20returned%20with%20a%20%5C%22null%5C%22%20value%20when%20no%20matching%20products_attributes_id%20exists%20in%20TABLE_PRODUCTS_ATTRIBUTES_DOWNLOAD.%5Cr%5Cn%5Cr%5CnMay%20need%20to%20modify%20the%20ON%20or%20WHERE%20clause%20to%20exclude%20NULLs%20%28if%20I%20remember%20correctly%20doing%20the%20check%20in%20the%20ON%20clause%20is%20faster%29...%20Or%20switch%20back%20to%20the%20original%20query...%5Cr%5Cn%22%2C%20%22created_at%22%3A%20%222014-07-14T23%3A34%3A43Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/2963958%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/lhungil%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20admin/includes/functions/general.php%3AL2558-2584%22%7D%7D%7D'></a>
<a href='https://www.codereviewhub.com/'><img src='http://www.codereviewhub.com/site/github-bar.png' height=40></a>
- [ ] <a href='#crh-comment-Pull 522084e112948161f57f197dc60bae1a70e73118 admin/includes/functions/general.php 40'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/zencart/zc-v1-series/pull/96#discussion_r14910603'>File: admin/includes/functions/general.php:L2558-2584</a></b>
- <a href='https://github.com/lhungil'><img border=0 src='https://avatars.githubusercontent.com/u/2963958?size=16' height=16 width=16'></a> Using a left join may cause rows to be returned with a "null" value when no matching products_attributes_id exists in TABLE_PRODUCTS_ATTRIBUTES_DOWNLOAD.
May need to modify the ON or WHERE clause to exclude NULLs (if I remember correctly doing the check in the ON clause is faster)... Or switch back to the original query...
- [x] <a href='#crh-comment-General Comment'></a> <img src='http://www.codereviewhub.com/site/github-completed.png' height=16 width=60>&nbsp;<b><a href='https://github.com/zencart/zc-v1-series/pull/96#issuecomment-48860671'>General Comment</a></b>
- <a href='https://github.com/zcwilt'><img border=0 src='https://avatars.githubusercontent.com/u/227354?size=16' height=16 width=16'></a> :-1:
please remove this PR.
The tone of the commit messages is arrogant and disrespectful to others
- <a href='https://github.com/texdc'><img border=0 src='https://avatars.githubusercontent.com/u/1590605?size=16' height=16 width=16'></a> @zcwilt cleaned up commit messages via `rebase`
- <a href='https://github.com/zcwilt'><img border=0 src='https://avatars.githubusercontent.com/u/227354?size=16' height=16 width=16'></a> thanks for updating that. however think https://github.com/texdc/ZenCart/commit/6b6638dfac8654e3bd7e4abbf938516e316cd93c slipped into your rebase
- <a href='https://github.com/texdc'><img border=0 src='https://avatars.githubusercontent.com/u/1590605?size=16' height=16 width=16'></a> @zcwilt  Yep, tricky things happen during `rebase`: very useful, but also very dangerous - always use with great care.
- <a href='https://github.com/zcwilt'><img border=0 src='https://avatars.githubusercontent.com/u/227354?size=16' height=16 width=16'></a> yeps, had that happen to me yesterday,
- [x] <a href='#crh-comment-Pull 6a6b8ae5d0cdcc92786161b8efd334f229391da6 testFramework/unittests/support/zcAdminTestCase.php 12'></a> <img src='http://www.codereviewhub.com/site/github-completed.png' height=16 width=60>&nbsp;<b><a href='https://github.com/zencart/zc-v1-series/pull/96#discussion_r14864288'>File: testFramework/unittests/support/zcAdminTestCase.php:L11-22</a></b>
- <a href='https://github.com/lhungil'><img border=0 src='https://avatars.githubusercontent.com/u/2963958?size=16' height=16 width=16'></a> I believe using an "if" block creates more readable code than an implied `if` via `||`. I would recommend against this line change (although would not be opposed to the addition of curly brackets to the original code).
- [x] <a href='#crh-comment-Pull 6a6b8ae5d0cdcc92786161b8efd334f229391da6 admin/includes/functions/general.php 50'></a> <img src='http://www.codereviewhub.com/site/github-completed.png' height=16 width=60>&nbsp;<b><a href='https://github.com/zencart/zc-v1-series/pull/96#discussion_r14864343'>File: admin/includes/functions/general.php:L2558-2594</a></b>
- <a href='https://github.com/lhungil'><img border=0 src='https://avatars.githubusercontent.com/u/2963958?size=16' height=16 width=16'></a> Would it be better to use the existing Zen Cart methods in query_factory for prepared statements instead of using sprintf?
- <a href='https://github.com/drbyte'><img border=0 src='https://avatars.githubusercontent.com/u/404472?size=16' height=16 width=16'></a> Ya, not a fan of heredoc or sprintf for SQL queries either, at least not in the v1.x code base.
Also, aren't RIGHT JOINs considered to be slower than LEFT?
- <a href='https://github.com/drbyte'><img border=0 src='https://avatars.githubusercontent.com/u/404472?size=16' height=16 width=16'></a> Indeed, since the query was rewritten with the tables reversed, a LEFT JOIN would be required. The PR brings back nulls for attributes without downloads, which is the opposite of the desired result.
But there's no need to rewrite the query at all. The original query is fine.


<a href='https://www.codereviewhub.com/zencart/zc-v1-series/pull/96?mark_as_completed=1'><img src='http://www.codereviewhub.com/site/github-mark-as-completed.png' height=26></a>&nbsp;<a href='https://www.codereviewhub.com/zencart/zc-v1-series/pull/96?approve=1'><img src='http://www.codereviewhub.com/site/github-approve.png' height=26></a>&nbsp;<a href='https://github.com/zencart/zc-v1-series/pull/96'><img src='http://www.codereviewhub.com/site/github-refresh.png' height=26></a>
<a href='#crh-end'></a>
That's the same argument that was used for the `right join`.  Are you sure about that?  Which is correct?  (I've been using `Doctrine`, so my `SQL` is rusty.)