views:

219

answers:

7

I'm currently rewriting an e-shop - but only the client side, i.e. the CMS remains mostly in tact. I am not using a pre-built framework, as the system has to retain backwards compatibility with the CMS and I have to have full freedom of the structure of code.

The new system is purely MVC based and I have a Bootstrapper which loads controllers based on the current uri and the latter use models for the real work - both with sessions and the database.

tl;dr It's my first project without a pre-built framework.

I am very inexperienced when it comes to design patterns. I know how do most of the popular ones work but have had never put them to use.

Now I am suspecting code smells because all of my models are classes that consist purely of static methods. I can find no advantages of doing them in a different manner. I routinely need some of the methods in various places through out the code. I.e. I need to fetch the logged in user in the main layout, check user rights to see current page in the bootstraper, display user panel by the controller. I'd need to re-instantiate an object each time or keep a global one if I wasn't using statics. There also won't be a need for more than one such class at a time.

I must be missing something, because even though I use OOP, some my classes are just meaningless containers for their methods (and sometimes a couple of private variables). I could have just been using PHP4 and simple functions.

Any comments or advice would be highly appreciated.

EDIT: in spite of all these educated answers, I remain unconvinced. Even though it's most probably because of my lack of experience, I still don't foresee anything going wrong with the current setup. I mean I don't even fathom a situation where I'd have any inconveniences due to the code architecture as it is now. I hope I don't get a harsh lesson when it's too late to change anything...

+5  A: 

If you are mainly only using static classes then you've really taken out the object out of object oriented programming. I am not saying you are doing things incorrectly, I am saying maybe your system shouldn't lend itself to OOP. Maybe it is a simple app that requires some basic utility functions (sends email, etc). In this case most of your code becomes very procedural.

If you are dealing with databases you could have a static db class, and a simple business layer, and your php app interacts with your business layer which in turn interacts with your database layer. This becomes your typical 3-tier architecture (some people like to refer to this as 4 t-iers and seperate the actual database from the data layer, same thing).

If you are not really calling methods that require an object than what is the point of all of these static classes, just a question to ask yourself.

JonH
+1  A: 

I would be cautious about using static variables and classes in web applications. If you really must share an instance between users, then it should be ok to have a single instance (lookup "Singleton" design pattern).

However, if you trying to maintain state across pages, you should do this through either utilising the ViewState or by using the Session object.

If you were to have a global static variable, you could have a situation where concurrent users are fighting to update the same value.

Jamie Chapman
I'm not using ASP.NET. Your last paragraph was lost on me too.
Raveren
In the last paragraph he is saying that if you have more than 1 user active on the site at once, and they are both hitting the site, that variable being static will be shared, they both have the same variable and will cause concurrency issues. If you do not know what concurrency is you MUST learn about it or you will have severe security issues on a payment portal.
Jeremy B.
That's what I thought you meant, but PHP variables - static or otherwise - are not shared between user sessions.
Raveren
A: 

There are advantages to using static methods. One being that since you cannot inherit them they perform better. But using them all of the time limits you. The whole OOP paradigm is based on re-usability of base classes thorough the use of inheritance.

Joe Pitz
Well I do use inheritance in controller classes, but every model is too unique to share any properties among each other. +1 for the performance tip.
Raveren
+3  A: 

One thing you may notice is that if you plan on doing any kind of unit testing with mocking/stubbing you are probably going to have a hard time since static classes and methods are not easy to mock, stub or test.

Otávio Décio
No, I won't do unit testing, but thanks for the note.
Raveren
+1  A: 

Short answer: It's ok but you are foregoing the benefits of OOP.

One reasoning behind using objects is that most of the time there is more than one type of object that performs a role. For example you can swap your DBVendor1 data access object with a DBVendor2 data access object that has the same interface. This especially handy if you are using unit tests and need to swap objects that do real work with dummy objects (mocks and stubs). Think of your objects with the same interface as Lego bricks with different colors that fit together and are easily interchangeable. And you simply can't do that with static objects.

Of course, the increased flexibility of the objects comes at a price: The initialization of the objects and putting them together is more work (like you wrote) and leads to more code and objects that put together other objects. This is where creational design patterns like builder and factory come into play.

If you want to go that route, I advise you to read about dependency injection and using a DI framework.

chiborg
+3  A: 

You are right, it's a code smell and everybody will tell you it's baaaad.

So here I suggest rather to make a self-assessment of the severity of the problem:

  • Do you have classes with many getter and setter?

  • Are your static functions like the one below?

    If yes, try to move the logic in the class MyClass that will be already way more OO. That's a classic mistake from procedural/scripting world.


 static void myMethod( MyClass anObject )
 {
     // get value from anObject
     // do some business logic
     // set value of anObject
 }

  • Do you have a lot of global state, such as data you fetch from the current session?

    If yes, make an assessment whether you want to change it. The OO way would be to pass the session down the call chain. But in practice, it's convenient to access the session as a global object. But it impedes testability. Try to remove some global state and turn that into regular object that you pass and manipulate in methods.

Make this assessment, and try to identify utility classes, services classes and the business objects. Utility class are helper classes with utility methods (e.g. formatting, conversion, etc.) which can be static. Service class do some business logic but they should be stateless and one instance suffice. Business objects are user, products, article, etc. is where you must concentrate your effort. Try to turn plain data into objects with embed some behavior.

Have a look at should entity be dumb. Even if it's for java, the concepts are general.

EDIT

Here is my analysis based on your comment:

  • You don't have a domain model with entities. You manipulate the database directly.
  • What you call your model, is what I call services and is where you perform the business logic that manipulate data. Service classes are stateless, which is correct. As you pointed out in the question, you then either need to constantly re-create them, create one global instance, or use static methods.

The OO paradigm would say that you should try to have a domain model where you map your database with entities. At least have an anemic domain model where entities are dull data container that are loaded/persisted in database. Then the OO paradigm would also say to put a bit of logic in the entities if possible.

It would also say to turn the services into objects to ease composition and reuse. If it was the case you could for instance wrap all services with an interceptor to start/stop transactions or do some security check, which you won't be able to do with static methods.

What you describe (no entities + stateless procedural services) is not considered a great OO design. I would suggest you introduce an anemic domain model at least and DAO. Regarding the sateless procedural services, this is actually the reality of many web applications -- if you don't need more you can stick to it.

My 2 cents

ewernli
None of the three statements apply to me. And I only use session data were it needs to be used anyway - like during user and shopping cart processing - and even then, crucial data resides in db. I don't need entities like `products` or `user` either. They'd have no methods. Only thing I need to do with a product, for eg., is to fetch data about it from db. Bootstraper calls an action of a controller and it calls some methods from models to set and get required data. I have no more types of classes - except stuff like db wrapper.
Raveren
@Raveren - if that is the case this is the basis that you should take - that this system not be OOP. I know you mentioned Users dont need to be a class, but it could very well be if you wanted. Your user class one user object => goes back to one user record in your database.
JonH
Wow that's a lot to wrap my head around. I wish I had a professional relationship with someone with such experience, it's really hard for me to learn from textbooks - not real world examples or practice... I opted out of making 1:1 *models to database tables* relationship because our db was designed by a moron and I wanted the stupidity to be handled centrally, so yeah, I have service classes for most entities, like `gifts`. These handle all associated operations. Frankly I can hardly see any gain in *making* wrapper classes for each db table, even if the structure is logical...
Raveren
ewernli
+1  A: 

Technically there is nothing wrong in doing it. But practically you are loosing lot of the benefits of object oriented programming. Also write the code/functionality where it belong to.. for example:

 user.doSomeTask()

on the user object makes more sense than

 UserUtils.doSomeTask(User user)

Using OOP concepts you abstract the functionality where it belongs to and in future it helps you change your code, extend the functionality more easily than using the static methods.

Teja Kantamneni