Page 1 of 2

Here guys - adeframework.com

Posted: Thu Jun 12, 2008 5:49 pm
by Benjamin
Tear it apart.

http://adeframework.com

This is sooo not ready to be looked at. I threw that site up in about 3 hours.

Re: Here guys - adeframework.com

Posted: Thu Jun 12, 2008 6:31 pm
by Christopher
What is the license? What does "not yet public domain" mean?

Re: Here guys - adeframework.com

Posted: Thu Jun 12, 2008 6:41 pm
by Benjamin
It means there is no license because I haven't decided to make it public domain yet or not. I guess it doesn't matter if nobody likes it though;) The code base is available for peer review.

Re: Here guys - adeframework.com

Posted: Thu Jun 12, 2008 7:13 pm
by Christopher
Well if there is no license then it is proprietary, which makes it difficult to look at.

Re: Here guys - adeframework.com

Posted: Thu Jun 12, 2008 7:38 pm
by Benjamin
arborint, honestly I value your opinion, but I'm not going to license it just so people can look at it. If people are too scared to look at it fearing a lawsuit that is their choice. Based on everything you have said so far to me today however, I really don't think you would have anything nice to say anyway. It would be nice if you could focus on the positive and how to improve it rather than identify everything that you feel is absolutely horrible. I don't know if you're just a grumpy old man, having a bad day, or if you just don't like me, but it's not nice.

I know that the code is far from perfect, but I really like it, and I'm not going to suddenly dislike it and throw it away because you say it's bad.

Re: Here guys - adeframework.com

Posted: Thu Jun 12, 2008 7:43 pm
by Christopher
Much as you might want to make is about feelings and attitude, it is really about simple professionalism. You are asking people to contribute to something that belongs to you -- for free. You will benefit from improvements, but you decline to state a license. It would be trivially easy for you to select a license. Why not do so to make it clear and easy for people to look at the code.

Re: Here guys - adeframework.com

Posted: Thu Jun 12, 2008 7:56 pm
by Benjamin
I'm just showing some cool code to others, gathering opinions. Seems to me that is the whole point of the devnet site. Anyway, I look forward to any constructive criticism and comments from others who are interested.

Re: Here guys - adeframework.com

Posted: Thu Jun 12, 2008 8:39 pm
by Eran
Hey guys, why can't all just get along nicely? :)

Anyway, I downloaded the framework and had a look. I have to say, it has some interesting concepts and obviously I can see how it can save you (its developer) development time. It is actually somewhat similar to how an OS works.
However, for using OO syntax it fails to capitalize on several things that make OO useful. Basically, it is completely procedural code in OO clothing - which there is nothing wrong with, it's just that OO can give you so much more in terms of code-reuse, structure and modularity, maintainability and extendability.

First of all, all the classes (that I've looked at it) go only one deep, meaning there is no class inheritance structure to speak of. For example, looking at the mysql db (dbmysql and dbmysqli) classes, they share many similar functions (some completely identical) and both could have inherited from the same parent class (reusing much of the code).

Furthermore, all the classes rely heavily on the Kernel class to work, setting up deep dependencies inside the framework (meaning they can not work independently). The heavy use of the Kernel and magical functions (__get(), __set()) also obscures the chain of command and makes it hard to track down where a method is actually defined.

You have some PHP4 conventions in there that aren't considered good practices anymore, such as the use of define's and global's. Your Kernel class is a singleton implementation, why not use its static getInstance() method instead of using global?

Having said all of this, I found your code to be very readable and consistent in coding style (though I prefer a different style, but that's another matter). Too bad there is no inline documentation.

Re: Here guys - adeframework.com

Posted: Thu Jun 12, 2008 10:41 pm
by Benjamin
Good stuff pytrin.

Your absolutely correct about the database classes. That is something that has been in the back of my mind.

As for the globals I'm not sure why I chose those over get_instance(). I'm pretty sure there was a reason (because I advocate against using them), but yet I did anyway. I should have commented that.

The framework isn't something I set out to design. It's kind of evolved over the years into what it is. Bits and pieces of different code that have somehow all globed together into a cohesive unit. As you can see it's incredibly fast, which is something that really stands out in IE. The pages aren't being cached either, because it sends headers to disable that.

I've tested the memory usage which is very low as well. That was something I was going to integrate into the framework but I discovered that the memory usage functions aren't universally available.

There is some older code in there that is due for refactoring. There's quite a bit of stuff I want to add to the framework but time is limited.

A major feature that isn't included in the version I released is a panel which allows you to create validation code from a database schema. That can turn 2 hours worth of work into 15 minutes as you only have to make minor changes to the validation code once you create it.

Anyway, thank you for your comments, I appreciate them.

Re: Here guys - adeframework.com

Posted: Fri Jun 13, 2008 2:28 am
by onion2k
"Accelerated Development Environment Framework Framework" is a weird name. Assuming the page <title> tag is correct. :wink:

Re: Here guys - adeframework.com

Posted: Fri Jun 13, 2008 12:43 pm
by Benjamin
Yeah I'll fix that.

Re: Here guys - adeframework.com

Posted: Fri Jun 13, 2008 1:48 pm
by alex.barylski
I don't mind looking at frameworks, regardless of license. It's not like I'm going to use your framework, basically for the same reasons you don't use others. But if I can steal an idea, it's worth my time. :P

Questions/Suggestions:

1) Why do you have the cron folder inside your your framework? Doesn't linux already provide a cron folder? Or is that where you store maintenance scripts? if the latter, I would rename that to make it more clear.

2) What the heck is $_KERNEL about? I see 2 problems with using that. One, $_ I think is reserved for built in PHP super globals so I would stay away (personally). Two, Kernel is a bit of a misnomer. I see kernel and I think it's somehow wrapping actual Kernel functions in PHP.

3) What or where is your boot strap? includes/core/init.php is your bootstrap? I see a kernel::get_instance() which I assume is your interpretation of a front controller? Seem's it's actually more than a front controller though, as the object returned also initializes your templates, etc??? index.php also calls a rendering function on the kernel object.

Personally I think your delegating to much work to one object. Seems like you have the KERNEL object acting as a master class which handles requests (front controller) views (view manager of sorts) template initializastion, etc. You have something of a super facade going on.

includes/core/fuctions.php

A lot of generic functions in there get, post, etc. I'd suggest switching over to a request object to keep things more OOP.

You also have some functions in there which generate FORM code. Personally I would store those functions in separate well named files for clarity. Anything that generates HTML in my own framework is done in a template, always. I think I have encountered one situation where I needed to actually output HTML in code that wasn't template based.

That file also has functions which would be best in a response object, but your redirect() has a dependency on KERNEL so you might have some serious refactoring/redesigning to do if you wanted a more pure OOP approach.

I like your pagination class, at least in how you avoid any HTML inside the actual class and essentially just have the class initialize some variables which can then be used by a template to generate the actual pager HTML. Although it seems to have a concrete dependency on DB object which is dependent on KERNEL.

The classes buffer, compiler...are they for templates?

dbconnection class is redundant. Why not use PDO and save yourself a tonne of time and increase efficiency?

I notice you have the following in every class:

Code: Select all

$this->kernel = kernel::get_instance();
two concerns:

1) You don't explicitly create the member variable and set it's access control modifier.
2) You do this in each and every class.

Although it became pretty clear, pretty quick that the $this->kernel was an dynamic/expando property I still prefer to see the access control modifier for explicitness and clarity. I'm not sure if the property can be accessed publically in these cases, but I rarely (if ever) allow properties to be publically accessible -- private or protected at best.

My biggest conern with your classes in the amount of coupling and dependencies. Although they are simple classes on their own, things get wildly complex once you start trying to peice togather how the work to form a complete system. The coupling would make it fragile to change and likely require a re-write in order to change anything drastically.

Your file structure is a little, well, typical. I hate redundancy.

I notice you have:

Code: Select all

includes/classes/{empty}
includes/core/classes/{files}
I can't stand when projects do that, just a personal beef I have. In my applications I strive for absolute clarity and simplicity. If I'm looking for a class I know I have only one directory to look under, etc.

Most of what I complain about isn't really important to the original developer, but for someone just coming onto a project, these are always my concerns.

I do like the fact that your directory tree isn't 25 levels deep, that is another pet peeve of mine.

One last beef. index.php it's so empty??? Personally I find that such a waste as that is the first place I start when I enter a new project. I find it irritating when it starts calling includes and a single function. Why not keep the include expanded inside index.php -- unless there is an overwhelming reason to keep it as a external include???

I'm still quite bothered by your KERNEL object. After reading your docs breifly, it seems now that it's almost acting as a registry, when you say:
The kernel allows you to easily call any class or method from anywhere in the code base
Sounds like a registry.
It will autoload classes located in includes/classes and includes/core/classes.
Now I'm thinking service locator.

What are event triggers all about? I didn't bother trying to figure those out. What purpose do they serve? Are they hooks to allow core customiztion without tinkering with core code? Did you borrow that idea from CodeIgnitor?

Off Topic: While I agree arborint sometimes sounds like a grumpy old man. :lol: His devils advocate approach to answering questions is usually very helpful. I hate to say it (just teasing) but I don't think there is a single person on these boards who has taught me more about my own best practices than him. If not directly, indirectly by at least making me second guess myself and try something different and sometimes that results in something better.

I do agree with the sentiment, that you want people to look at your code probono but have not made it publically usable. On the flip side, I don't really think in your case (astions) a license is a big deal. It's not like anyone is going to run off and use your code, anymore than arborints framework, my framework, or anyone else's on here.

If anyone made a PHP framework private in that regard and attempted to sell it, I'd laugh as all frameworks have their sucky parts and good parts.

Now how about looking over my application and offering some opinions? :lol:

I appreciate your sharing your code with us, I learn something new each time I look at code.

Cheers :)

Re: Here guys - adeframework.com

Posted: Fri Jun 13, 2008 3:14 pm
by Benjamin
Glad to see you poke your head in here.
Hockey wrote:1) Why do you have the cron folder inside your your framework? Doesn't linux already provide a cron folder? Or is that where you store maintenance scripts? if the latter, I would rename that to make it more clear.
Yes, it's just a place to store cron scripts. What would you rename it to? cron_scripts?
Hockey wrote: 2) What the heck is $_KERNEL about? I see 2 problems with using that. One, $_ I think is reserved for built in PHP super globals so I would stay away (personally). Two, Kernel is a bit of a misnomer. I see kernel and I think it's somehow wrapping actual Kernel functions in PHP.
The kernel is basically a registry object that will also instantiate classes if they don't exist yet. It contains a method that checks it can locate a class, which needs to be hooked to a resource locator, that I haven't created yet.
Hockey wrote: 3) What or where is your boot strap? includes/core/init.php is your bootstrap? I see a kernel::get_instance() which I assume is your interpretation of a front controller? Seem's it's actually more than a front controller though, as the object returned also initializes your templates, etc??? index.php also calls a rendering function on the kernel object.
The init file includes all the files required for the framework to run. kernel::get_instance(); is just initializing the $_KERNEL var for later usage. init.php is also setting the default template.
Hockey wrote: Personally I think your delegating to much work to one object. Seems like you have the KERNEL object acting as a master class which handles requests (front controller) views (view manager of sorts) template initializastion, etc. You have something of a super facade going on.
The $_KERNEL doesn't do anything, you can just call other methods and classes *through* it.
Hockey wrote: includes/core/fuctions.php

A lot of generic functions in there get, post, etc. I'd suggest switching over to a request object to keep things more OOP.
Most of those functions have very little code in them and server very simple purposes. They are designed to save typing. Like this:

Code: Select all

 
# instead of typing
echo (isset($_POST['var'])) ? $_POST['var'] : '';
 
# I can do this
echo post('var');
 
That saves me a lot of time, especially on pages that contain lots of post or get variables.
Hockey wrote: You also have some functions in there which generate FORM code. Personally I would store those functions in separate well named files for clarity. Anything that generates HTML in my own framework is done in a template, always. I think I have encountered one situation where I needed to actually output HTML in code that wasn't template based.
Again, these are to simply save time. I really can't justify the overhead of using a template for an input box. These work fine until you need to start doing things such as adding javascript. You can just pass classes as an argument. If an input field has an error flagged in the form_error class it will be assigned an error class.

Code: Select all

 
echo input('var_name');
 
Hockey wrote: That file also has functions which would be best in a response object, but your redirect() has a dependency on KERNEL so you might have some serious refactoring/redesigning to do if you wanted a more pure OOP approach.
Again I don't see a need to put something as simple as a redirect in a class. It calls the kernel so that the system can shutdown correctly. The problem is that PHP is destructing some of the classes required by other classes before their __destruct method had a chance to execute. Having the kernel shut down these classes first solves the problem.
Hockey wrote: I like your pagination class, at least in how you avoid any HTML inside the actual class and essentially just have the class initialize some variables which can then be used by a template to generate the actual pager HTML. Although it seems to have a concrete dependency on DB object which is dependent on KERNEL.
Yeah it needs some work but it's a real time saver. There are two issues with it. 1 is that it has a small bug I believe and 2 I would like to add the option to have it automatically add a limit clause rather than seeking to rows in large results set. Seeking to rows or using limit both have their drawbacks, but at least then the programmer could decide. It depends on the kernel because it requires a database connection. You can pass it your own db connection if you wanted it to use a mysql slave server for example.
Hockey wrote: The classes buffer, compiler...are they for templates?
They build the webpage.
Hockey wrote: dbconnection class is redundant. Why not use PDO and save yourself a tonne of time and increase efficiency?
I like my database class better than PDO.
Hockey wrote:

Code: Select all

$this->kernel = kernel::get_instance();
1) You don't explicitly create the member variable and set it's access control modifier.
2) You do this in each and every class.
This is how the classes access other classes - through the kernel. It would not be a bad idea to set this as private in each class.
Hockey wrote: My biggest conern with your classes in the amount of coupling and dependencies. The coupling would make it fragile to change and likely
require a re-write in order to change anything drastically.
I'm not fond of dependencies either, but this is a mute point. I think all frameworks are like this.
Hockey wrote: I notice you have:

Code: Select all

includes/classes/{empty}
includes/core/classes/{files}
There's nothing preventing them from being placed into a single file. It's purely to keep the framework classes separate from yours.
Hockey wrote: One last beef. index.php it's so empty???
That is all it needs. Did you expect to see 5 require_once(); :wink:
Hockey wrote: What are event triggers all about? I didn't bother trying to figure those out. What purpose do they serve? Are they hooks to allow core customiztion without tinkering with core code? Did you borrow that idea from CodeIgnitor?
I haven't borrowed anything. The trigger system is something I was going to add so you execute classes or classes and methods based on events. For example you could add a page_not_found event that would execute a class which would then do something based on the requested resource.

My issue with that was that I didn't want to sprinkle $kernel->events->run('event_name_here'); all over the place, but I haven't found a better way yet.
Hockey wrote: Now how about looking over my application and offering some opinions? :lol:
Sure where's it at?

Re: Here guys - adeframework.com

Posted: Fri Jun 13, 2008 3:58 pm
by alex.barylski
Yes, it's just a place to store cron scripts. What would you rename it to? cron_scripts?
probably just 'scripts' personally.
The kernel is basically a registry object that will also instantiate classes if they don't exist yet. It contains a method that checks it can locate a class, which needs to be hooked to a resource locator, that I haven't created yet.
I figured that out eventually...see where I'm going with this? I would refactor that KERNEL into a few smaller objects. A registry, service locator perhaps.
The $_KERNEL doesn't do anything, you can just call other methods and classes *through* it.
I figured that out too eventually...but I'm confused as to the purpose? Why not just include the class file, instantiate the object and call it's members. It would be more efficient.

I suppose if your KERNEL is a service locator and automates the include and instantiation of an object and then doubles as a registry/object stash -- it becomes more useful.

When you have the KERNEL instantiate objects though, how would you ever perform any kind of constructor injection? I dislike setter's -- I prefer to initialize an object with it's ctor(). I think there was a disscussion around here a while back about intelligent DI libraries which as I understand, acted like service locators, initializing the objects at construciton time via a config file.
Again I don't see a need to put something as simple as a redirect in a class
I didn't either, until CC contributed to a thread I started. His reason was purely OOP but after implementing a response object I have since discovered many other really handy situations in which it has helped greatly.

You need to build a full fledge application I think to truely appreciate all these little objects working as a system.
The problem is that PHP is destructing some of the classes required by other classes before their __destruct method had a chance to execute. Having the kernel shut down these classes first solves the problem.
I'm pretty sure PHP and most other languages implement GC as a LIFO stack. That is, object A is created first, object B is created second, object B is destroyed first, object A is destroyed last.

I think if your experiencings these problems, thats a design issue being introduced because of the coupling you have between classes.
I like my database class better than PDO.
While I find PDO a serious PITA -- I guess it depends on what your after. If it ain't broke, don't fix it. :lol: Personally I like to break things on purpose and then try and improve on them. I never had toys for very long as a kid.
I'm not fond of dependencies either, but this is a mute point. I think all frameworks are like this
Yes they do, I agree, but some dependencies are better than others, either for the sake of clarity or simplicity. Other dependencies are abstract (IoC/DI) and others are concrete, such as having each class call upon your KERNEL class. Sorry I keep CAPS the KERNEL it's just force of habit after looking at your code. :lol:
That is all it needs. Did you expect to see 5 require_once();
Haha...no thats no better. I just hate finding the index.php only to find a single line and now I have to follow through countless functions or includes to learn what or how the system pulls it self up from it's boot straps. :P
I haven't borrowed anything. The trigger system is something I was going to add so you execute classes or classes and methods based on events. For example you could add a page_not_found event that would execute a class which would then do something based on the requested resource.
I'm pretty sure it's CodeIgnitor which does that as well. Your framework is very similar, it too uses something like your kernel object to access all it's objects. I have ot be honest, I don't like that choice, but it obviously works for some people, CI is pretty popular. :)

Re: Here guys - adeframework.com

Posted: Fri Jun 13, 2008 5:06 pm
by Benjamin
So I guess what I'm getting here is that a lot of you aren't fond of the $_KERNEL? Would it be better to call it something else?

This is an awesome framework. Creating a site with it using MVC would be trivially easy, the template system is easy to use and it's got a lot of other things going for it.

I'm surprised you guys aren't jumping on it. I'd worry more about what it does for the programmer than the code base. The core code base can always be changed/refactored, but it works virtually flawlessly as it is right now.

Is anyone interested in adding features/improving the code base? If so I'll open source it and throw it in google svn.