Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I wrote this a while back: https://github.com/chintanparikh/Serene

I'm looking for feedback on the architecture/Object Oriented Design of the framework.

What I thought was neat: In the COre folder, I think the Route/Router/Dispatcher combo is a good example of software architecture, because it allows you to completely rewrite Route without changing anything else.

I also think my Config and ConfigParsers combo is pretty neat, because you can drop in a new config parser, change one line in Config, and support a new type.

Am I correct? What could I have done better?

share|improve this question
2  
Unfortunately questions without inline code are off-topic on this site. Could you include some code into the question? (Otherwise I guess it will be closed.) –  palacsint Mar 13 '13 at 19:01
add comment

closed as off topic by Jeff Vanzella, Corbin, Brian Reichle, palacsint, svick Mar 15 '13 at 1:39

Questions on Code Review Stack Exchange are expected to relate to code review request within the scope defined by the community. Consider editing the question or leaving comments for improvement if you believe the question can be reworded to fit within the scope. Read more about reopening questions here.If this question can be reworded to fit the rules in the help center, please edit the question.

1 Answer

Please, go and learn what MVC actually is, because what you have written IS NOT an MVC or MVC-inspired design pattern.

  • Model is not any single class or object. Model is a layer that contains all of the business logic, that is distributed to different groups of structures, each with different area of responsibilities: domain logic, storage logic, application logic.

  • View is not a template. Views are instances, which contain all of the UI logic. They acquire data from model layer, and then, based on that info, choose which templates to combine for creating response. Or even if a complicated response need to be sent. Maybe it is enough to send an HTTP location header.

  • Your autoloader is a sad joke. I don't even see the point in having a class there. You could just use a namespace function to achieve same result.

  • There is a singleton in database class, which is simply a thin wrapper around incorrectly initialized PDO instance.

  • The way you have created configuration files pollutes the scope in which the are included.

  • Controllers in MVC are responsible for altering state of model layer, not for creating output.

  • And all but the most basic classes in your codebase are tightly coupled to some other name of class.

share|improve this answer
    
1. I know, what I have in Models is an example model –  user1045696 Mar 13 '13 at 21:11
    
2. Mind going into a little more detail? Views look like templates in Rails, or am I interpreting this incorrectly? –  user1045696 Mar 13 '13 at 21:12
    
3. The class is there in case I ever need to extend or change the autoloader - and it does autoload everything I need to. Again, mind going into a bit more detail? –  user1045696 Mar 13 '13 at 21:17
    
4. What's wrong with a singleton in the database class? How am I initializing the PDO instance incorrectly? –  user1045696 Mar 13 '13 at 21:17
    
5. The config files are really just there to demo how you can use a config file. If this was ever going to be used, I'd stick to one format –  user1045696 Mar 13 '13 at 21:18
show 2 more comments

Not the answer you're looking for? Browse other questions tagged or ask your own question.