The app/core holds some helpers(Json Serialization) and handlers(error handler).Also not to forget it holds the database configuration and an abstract class that will have use in the model.

The view hold JSON encoding and it's the end point

The model hold the functions and communicates to the database

The controller hold the controller and it's an extension to the router.

The thing that concerns me the most is everything :P
But I would like if someone would check out my model.
So basically the main mode for example Test.php ,hold functions for certain actions.It extends those actions from Test/classes
Each of those classes holds a function and extends an class.

1 Answer
1

There seem to be a number of strange design and/or naming conventions that you are using here. It makes your intention hard to understand at times.

What are you building here? Are you building a framework or an application? Your file structure of most everything living in an app directory seems at odds with your decision to have some classes nested underneath labelled within a core library. If you have a core framework, should that not exist outside the construct of any given app that that leverages the framework?

I think you could benefit greatly in continuing to understand the proper use of Composer with regards to library/dependency management (including frameworks). If for nothing else, it would be useful to understand good patterns around structuring your frameworks/libraries/dependencies relative to applications that use these dependencies. You might ultimately find yourself building your framework as a totally separate codebase from your actual applications that use them. This is a proper separation of concerns to allow you to maximize re-use of the framework parts.

Additionally, you should really look at PSR-4 compliant structuring of your code to allow for autoloading. This prescribes a particular scheme for structuring your code and naming your class files. You are almost there, but your classnames are wrong.

Use meaningful names. This code is littered with examples of questionable naming choices, starting with the name of the package itself (MVCTest). Is this a framework for testing? Just because it may be experimental in your mind at this point doesn't mean it shouldn't be named according to what you intend it to be. Think about it. What have you named your Github repository (hint - the word test is nowhere to be found)? Why introduce this test nomenclature throughout your codebase (which could end up confusing a reader of your code or someone who might be considering using your Composer package)?

Why is the main application bootstrap class called Test? Why do you have models nested under a Test directory? This is now causing you to introduce Test into your namespaces even. This is bad naming. Name these things what they are.

Why would your main application class (Test as currently named) live in a Model directory? Don't confuse main application functionality with models used within the application.

I do not at all understand having separate classes like AddUser, GetUser, UpdateUser etc. These do no seem like models at all, but rather controller actions and/or methods that would live on a proper user model. Why do you need concrete instantiations of what amount to methods?

I don't understand how the update action would exist in a static context, which is, in essence, what your call pattern is (even though you are instantiating an object to make an update). I would think the user object to be updated would have already been marshalled based on one of the methods shown above and would simply have the means to update itself.

In your diagram, you talk about having a router, but I see no evidence of there being a router from either the code or directory structure you have shown. This is a critical piece of functionality for a framework. Where is it?

I am concerned that you have config files buried within your code base. These typically should not be part of your code package (other than maybe providing a template). For example, you should not be including DB credentials within your repository at all (and you absolutely should not be using root/root DB credential for an application EVER) (note to other readers, this was seen in GitHub repo, since code is not here). You have you DB credentials out on GitHub!!! Change them now and get this file out of your repo!

In your Test class. Your methods are unclear in intent, possibly due to poor parameter naming.

Why would a caller be passing a $userObject(s) to these methods and then have totally different objects instantiated inside the methods? What is happening here? Why would a get* method get objects passed to it when the intent is to get objects returned from it? Are you really passing ID's here (meaning perhaps the parameters should be renamed)?

You have an odd pattern of using these main() methods which seem like a paradigm from other languages you may have used in the past. main() has no special meaning in PHP. If there is code you want to be run upon object instantiation in PHP, that code needs to be in the constructor. Why make the caller set up objects in proper state? In other words, consider this using this sort of instantiation:

$object = new someObject($data);

instead of:

$object = new someObject();
$object->main($data);

With your DB-related classes, you seem to have fallen into a classic anti-pattern in working with DB objects in PHP. For some reason (probably because there are thousands of bad code examples out there on the web), almost every developer picking up PHP tries to write some DB abstraction or DB connection class. 99% of the time these classes add no value above and beyond working with the underlying PDO, Mysqli or similar classes.

In your case, what value do DBHandler and ModelHelper add? These classes present a number of problems:

They don't provide any sort of connection management (as there is nothing here to stop a caller from spawning any number of DB connections).

They don't provide any meaningful error recovery/handling (in fact they swallow exceptions, hiding error conditions from the caller which is likely better positioned to determine what to do in such conditions).

They severely break encapsulation, in that you require a caller to instantiate and pass a PDOStatement object to the query() method.

They wander off course of the Single Responsibility Principle, in that these classes are beginning to implement functionality that should either be in controllers (i.e. determining response codes) and/or models (like determining which rows from a database resource to retrieve).

I would honestly abandon these classes altogether in favor of having a true model base class (not a ModelHelper) which simply accepts a PDO object as a dependency. Do not do things like have model classes inherit from DB classes like you seem to be moving towards here when you start having your "model" classes in turn inherit from ModelHelper. DB connections and Users (for example) are two entirely different objects in your system and should not be part of the same class inheritance structure. Providing dependencies through inheritance is a poor plan. What happens for example when your model has a dependency on both a DB connection and a system logger? PHP does not support multiple inheritance, so you are out of luck. You really need to embrace the "composition over inheritance" mantra. Compose your dependencies into your classes rather than inherit them.

Why are you mixing snake_case and camelCase? I know PHP, as a language, is not really good about this (mainly from its legacy roots), but that doesn't mean libraries that you are writing should not be consistent amongst themselves with regards to the symbols you are defining.

\$\begingroup\$I'd like to pay you for reviewing my code if you would agree on it?@Mike Brant\$\endgroup\$
– DaAmidzaJun 4 '17 at 18:58

\$\begingroup\$Since this helps me a lot ,and your first answer moved me out of the blind zone.Thanks a lot\$\endgroup\$
– DaAmidzaJun 4 '17 at 19:01

\$\begingroup\$the router i used is simple route and it's in index.I'l change that part....\$\endgroup\$
– DaAmidzaJun 4 '17 at 19:22

\$\begingroup\$@Arslan.H I don't think you are at the point where you should be worried about paying for code reviews. There is still a lot of free material/resources out there on the Internet. Not to mention why pay to improve a code base that really has no commercial value at this point :)\$\endgroup\$
– Mike BrantJun 5 '17 at 2:31