Quote:
Originally Posted by Salathe
I've not read thoroughly through the entire article (time is money) but I do however have a few comments to offer up. Firstly, the filesystem structure itself; why is there a folder named includes when some things in there aren't simple PHP files to be included into others (e.g. the js folder)? A more suitable name would suffice, or indeed a complete separation of groups of files serving different purposes (javascript, css might be better suited outwith the PHP files' folders).
|
Because then in your templates when you "include" a css file, you use:
Code:
<link charset="utf-8" title="no title" media="screen" type="text/css" href="includes/gfx/layout.css" rel="stylesheet">
or
Code:
<script charset="utf-8" type="text/javascript" src="includes/js/mootools.js"></script>
So you can say that you include the file to be used, and thus it should be placed in includes..
Quote:
Originally Posted by Salathe
A next point, and one I won't labour too much, is the way you have written the various DB-related classes. To me at least, whether you did this knowingly or not, it all seems very MySQL-centric in how things are done. You could certainly do with working with a wider group of databases and seeing which common methods are really needed, and those which are more tailored to working with MySQL in particular.
|
Yep. And that's because of the simple reason, that I use MySQL and not anything else. And this is why I've made the factory cause people can then write their own database classes to support THEIR database-type.
Quote:
Originally Posted by Salathe
Next, your autoloading and factory method. Why have both? Why separate the two? It's not my point to provide answers right now but it's something to consider. I will, however, note that you can only define a single __autoload function. It might (read: will) be easier to use the SPL library (e.g. spl_autoload_register) to keep the DB autoloading separate from any other autoloading which might be needed elsewhere in your project.
|
I've used this method for ages, and I use it for almost every single system I code as a core. The thing is, you SHOULD NEVER create an instance of an object OUTSIDE of classes.php. That's the purpose of classes.php, to instanciate all the classes. Config includes classes.php, so you only have to include the config file and you'll have access to every class.
And within classes.php, you always include DB.php, thus I've placed it there. However, I could place the autoload function in classes.php instead if you like that, but it doesn't really matter the way I see it if it's in DB.php or in classes.php.
But I'll take a look at spl autoload!
[quote=Salathe;17443]
For the configuration file, I'd like to see things simplified much more. Have separate config files for different jobs. For example, one for the MySQL database settings, another for SQLite database settings, yet another for caching settings (if that were to be part of your application), etc. Keep the variable names within those files simple (just plain
$config works fine) and make use of the include-scope when loading/using the configurations (if that's over your head, I'll explain at a later date).
[quote]
This is all a matter of preferences ofcourse. But you are all free to create your own variable names for your configuration file. I'm just mearly showing that you should have a variable for the dbhost, one for dbuser, etc.
The thing about several configuration files, will actually make it harder, at least to me. Why not keep it all placed in one file, which will make it alot easier to find and read the config. Instead of having to go through 10 configuration files just to find the setting that you're looking for if you're not sure where the setting is.
And about the seperate DBsettings. You should only have 1 database, so thus you can easially change that by changing the variable that contains the value for "dbtype" from "mysql" to "whateveryouwant".
Quote:
Originally Posted by Salathe
Anyhow, there are some comments to tide you over until I can really sink my teeth into this come september! :)
|
Thanks for comments
