Code architecture - CMV

following5 commented 5 years ago

This post is mostly obsolete, see the next post.

There are some classes like ApplicationContainer and OcConfig, which represent static content, but are instantiated dynamically by an instance() method. This is error-prone, as a developer could call a class method directly instead of using instance(), which would use or return bad (non-initialized) data. Also, it allows to create confusing methods like these in ApplicationContainer:

    public function getLoggedUser()
    {
        return $this->loggedUser;
    }

    public static function GetAuthorizedUser(){
        return self::Instance()->getLoggedUser();
    }

This makes the calling code unreadable. It is not clear what the calls to getLoggedUser() and GetAuthorizedUser() do, without reading the ApplicationContainer souce code. Actually, getLoggedUser() is something like getLoggedUserOrNullIfThisIsNotInitialized().

OcConfig has the same issue. Some methods need instanciation, others will work without instance and initialize static class data dynamically. This makes the calling code hard to understand - it is not clear if a OcConfig method call needs instanciation or not.

Instead, I would prefer a solution like this: The class is not instanciated, but has an init() method, which is called internally for each method that may require initialization. This is very fast - will have NO performance issues because of redundant init() calls. Developers that use the class cannot make any mistakes, and all the class's methods consistently behave the same way. The static data represented by the class is always valid; methods cannot return bad data. (For OcConfig, there may still be multiple initializations for different groups of config data as an optimization.)

(For OcDb however, it can be necessary in the future to create multiple instance for users with different rights or to access different databases. So instanciating is reasonable there.)

following5 commented 5 years ago

Oops, sorry - I missed that some of the methods are declared static and others not. The static ones all call self::instance(), and the other can only called by an instance. So there should be way to make an error.

Just the naming of the those ApplicationContainer methods is suboptimal.

kojoty commented 5 years ago

@following5 I think that ApplicationContainer as well as OcConfig are not the final solutions from my point of view.

I think that in general:

all controllers should inherit from BaseController
all models should inherit from BaseModel
all views should use code by vars which was set in Controller
utils should be separated from the "base" code - there should be just utils

OcConfig should be refactored and cleanup - there are a few different approach - I think it should work "dynamically":

we have many config files
static OcConfig class loads only requested configs - not all config files at once
we want to remove old-style configs settings.inc
most of the config should be public in repo

following5 commented 5 years ago

@kojoty I have some detail questions on this:

I think that ApplicationContainer as well as OcConfig are not the final solutions from my point of view.

Does it make sense to refactor $usr['admin'] now to ApplicationController::GetAuthorizedUser()->hasOcTeamRole()?

all views should use code by vars which was set in Controller

May the views call getter methods of passed objects, or should the controller do that and pass just the values to the view?

utils should be separated from the "base" code - there should be just utils

What about accessing OcConfig from utils? This is frequently done, i.e. Utils are based upon lib/Objects. Should it stay this way?

following5 commented 5 years ago

And: Would BaseModel be the same as the current BaseObject?

kojoty commented 5 years ago

Does it make sense to refactor $usr['admin'] now to ApplicationController::GetAuthorizedUser()->hasOcTeamRole()?

from my point of view - YES! This close the information inside the object - access to such information is much more clear if this is done by function than by global var

My final solution should is this is close in BaseController code - and all the decisions where you need to know if current user has some role should be based on some thing like: $this->getCurrentuser()->hasOcTeamRole() within controller

But I know that we can't do "great refactor" at one step - I believe we can do small steps and refactor next parts of code - i see it as a process of improvements. In my final vision there are no global vars like $usr[]

May the views call getter methods of passed objects, or should the controller do that and pass just the values to the view?

I think we don't need copy-paste every small value - I think view can use object (model) methods to read some values - it should be as simple as possible - but I think there shouldn't be any significant "decisions" in the view - flow should be controlled by controller - view should only decide about the details of the UI (I hope you understand what I mean - If I not describe it clear please forgive me - then I try to explain it one again)

utils should be separated from the "base" code - there should be just utils

What about accessing OcConfig from utils? This is frequently done, i.e. Utils are based upon lib/Objects. Should it stay this way?

yeah, the discussion about the best structure of the code is something what come back from time to time and we don't have a "one-final-vision"

Some time ago I made small descriptions here: https://docs.google.com/document/d/1UH0tJ3GXRsDqgPZf8eMsYbE--cVpKPlYuwwK-1yTz6A/edit but we also discussed about closing all the code in /app folder + static content in /static (to prevent access on the server side to the "code" folders, and so on... (see #1513) I don't know what is the best approach - we are also open for your suggestions :)

Config-access class is something "special" - lots of code needs access to it - I'm not sure if this is a pure "Util" - maybe it can be store in other place - I don't know :) I'm sure that I would like to remove all the code from folders:

"/lib" - because it was a container for "everything",
"/utils" - this is old code - in many places very raw, buggy or not-working

"/lib/Objects" shoudl be one day moved to "/Objects" or even "/Models"

I also think that our "models" are not a good "models" (let's forget about pure definition of MVC) Our models has too many code - UserModel or Geocache are too long.

Maybe we need to introduce something like (let's call it) "services" - service as a container for code which do some operations on model - many of methods from our models could be migrated to such service classes.

@following5 I'm aware that there is no "clear-strict-architecture-vision" - I don't know what is a best-optimal solution to develop this OCPL code - we are in the way and don't know all the solutions now. We all are open for discussion and looking for the best :)

Many thanks for your involvement! I'm interesting in your opinion.

kojoty commented 5 years ago

And: Would BaseModel be the same as the current BaseObject?

Of course - your right - I mean BaseObject - in my mind object = model

following5 commented 5 years ago

Thanks for explanation. I think so far I have understood it all, and I like your approach to improving OCPL code and do OOP refactoring.

I'm aware that there is no "clear-strict-architecture-vision"

I am fine with that. My experience is that strict top-down achitecture models fail as often as bottom-up "spaghetti coding" fails. The real world often is too complex to fit into a simple and clear architecture ...

But I have too little experience yet with OCPL and PHP website development to really contribute to the code architecture discussion. (My background is hardcore C++ development, and Okapi is only little OOP.) Will have to learn more from you first.

following5 commented 5 years ago

There are some classes like ApplicationContainer and OcConfig, which represent static content, but are instantiated dynamically by an instance() method. This is error-prone, as a developer could call a class method directly instead of using instance()

Here it happened: 13a7e7b4. Missed it while testing, because I tested only with own entries.

kojoty commented 5 years ago

There are some classes like ApplicationContainer and OcConfig, which represent static content, but are instantiated dynamically by an instance() method. This is error-prone, as a developer could call a class method directly instead of using instance()

Here it happened: 13a7e7b. Missed it while testing, because I tested only with own entries.

Hmm..., but every call should generate warning what should be seen in logs and fixed... that was a dead code?

In PHP 7, calling non-static methods statically is deprecated, and will generate an E_DEPRECATED warning. Support for calling non-static methods statically may be removed in the future.

following5 commented 5 years ago

The Applicationcontainer:: call is only executed if $commentDbRow['userId'] != $callingUser (due to optimization of boolean expressions). So it was not executed during my tests, because I tested with own comment entries.

[Deleted - again the same false conclusion as in my first posting.]

following5 commented 5 years ago

Again, I confused PHP and C++. It's not an OC code issue, but a general PHP language issue: The PHP parser will not detect if a developer calls a static method by $this or a nonstatic method by Class::.

kojoty commented 5 years ago

Again, I confused PHP and C++. It's not an OC code issue, but a general PHP language issue: The PHP parser will not detect if a developer calls a static method by $this or a nonstatic method by Class::.

yeah, definitely PHP is much more "relaxed" in comparison to C++ - I'm not a PHP expert but it seems that it becomes more strict - see http://php.net/manual/en/language.oop5.static.php There is a warning I mentioned above - and it seems that calling non-static method in static way will be prohibited in next PHP versions.

opencaching / opencaching-pl

Code architecture - CMV #1821