3
\$\begingroup\$

I created a class that I plan on using to autoload my project classes (20-30 classes) maximum. I wonder if there is anything, anything at all, that I can improve on this class to improve it any further.

Please let me know your thoughts below.

<?php
/**
 * YodaCMS, content management system.
 * 
 * @info This class autoloads project classes. 
 * @author Someone Someone <[email protected]>
 * @version 0.0.1
 * @package YodaCMS
 *
 */
class autoLoader {
 private static $loadedLibrarys;
 public function __construct() {
 $loadedLibrarys = array();
 }
 public static function loadLibrarys() {
 newLibrary("testClass", new testClass());
 }
 private function newLibrary($lName, $lValue) {
 if (!$this->tryLoadLibrary($lName, $lValue)) {
 print_r('Error loading library: ' . $lName);
 }
 }
 public function tryLoadLibrary($lName, $lValue) {
 include(ROOT . '/applocation/base/classes/'.$lName.'.class.php');
 $this->loadedLibrarys[$lName] = $lValue;
 return true;
 }
}
200_success
145k22 gold badges190 silver badges478 bronze badges
asked Mar 8, 2016 at 8:42
\$\endgroup\$
1
  • \$\begingroup\$ Any reason why you don't use the PSR-4 autoloader? php-fig.org/psr/psr-4 ? \$\endgroup\$ Commented Apr 3, 2016 at 23:13

1 Answer 1

1
\$\begingroup\$

I don't think that you need newLibrary and tryLoadLibrary. The names don't really make clear what the distinction is, and I can't imagine that you'd ever want to load a library but not know if it failed or not.

Also, instead of printing, I would throw an exception. That way, the calling code can handle the case (what if it's an irrelevant library? Now the user is still annoyed with a meaningless message; but what if it's a highly relevant library? Is a small message really enough?).

I would also get rid of the distinction between lName and lValue. If you follow conventions, they should be the same anyways. If they are not, it might get confusing, especially because it's not just this call, but also how they are stored in loadedLibrarys[$lName] = $lValue;,

I would also rename tryLoadLibrary to loadAdditionalLibrary. try is not really needed (we can assume that it will only try), and additional makes it more fitting with loadLibrarys. lName and lValue are also not all that clear; if I didn't read your code, I'd have no idea what l is, or what name and what a value is. libraryClassName and libraryFileName would be clearer (you can also skip the library part, as it's clear from context.

Finally, I would always check for directory traversal when including with a variable. It shouldn't really be necessary in this situation, but it's just good practice, and doesn't cost that much.

answered Mar 8, 2016 at 9:52
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.