Skip to main content
added 3203 characters in body
Source Link
Kevin Peno
  • 233
  • 1
  • 6

In both cases you will need to pass the parts that matter to the Database constructor. This reduces Coupling which, in turn, reduces the amount of rewrite needed during refactoringrefactoring.

There is little more I can offer except questions. For example, what is Security doing? Is there only the one function? What else is declared in it? The same goes for Misc. Both of these classes sound like they don't really belong and are classes for the sake of being classes.

EDIT

... the config class sounds interesting. Do you have any good places where I should start looking for a decent way?

The Zend_Config documentation, part of the Zend Framework, would probably be a good place to see how people are using something like this.

This is my Misc class:

You could keep this I suppose, but Misc is a poor name for it. Calling some thing Misc is just asking for disorganization. If you really want this to be a function, and you can support PHP 5.3, I would wrap this in a namespace and treat the namespace similar to what most people would call a module.

Example:

<?php
namespace Utilities
{
    function validateEmail( $email )
    {
        // Code
    }
}

However, you should really consider refreshing yourself on the PHP documentation, specifically the newer stuff that's been added since 4.0. A perfect example of this is the use of the Filter extension. In your case, FILTER_VALIDATE_EMAIL will do wonders.

Security:

This class, I won't really comment on because I'm not sure the use case or need. I'll assume it is necessary as is.

Similar to Database you have high Coupling by putting the private key directly into the class. Depending on the needs of the key, it could exist in a config file and be treated just like the Database config attributes. On initialization, you would pass the config var for passkey into Security::__constructor.

Doing so means that you could have a different passkey per user instead of a single passkey. Expanding this to a public+private key system in the future would also be easy (just add another parameter).

User:

This class is also plagued with high Coupling because of the self loading of Database. There are a few ways to handle dependency injection and I'm not going to sway you either way on this matter. However, I'd suggest you spend some time looking through the many articles your favorite search engine will provide (or Bing! and decide...lol).

I would also keep from naming functions in all lower case. It is common for people to name functions either in CamelCase or underlines _ between words. Not doing so can impact readability.

Aside form that, this class is the first I've seen you provide that is acting as a Model (thinking in terms of MVC). That's a good thing, so nice job. I would expand your understanding of MVC if this is the path you are headed for so that you don't fall into many of the common pitfalls.

In both cases you will need to pass the parts that matter to the Database constructor. This reduces Coupling which, in turn, reduces the amount of rewrite needed during refactoring.

There is little more I can offer except questions. For example, what is Security doing? Is there only the one function? What else is declared in it? The same goes for Misc. Both of these classes sound like they don't really belong and are classes for the sake of being classes.

In both cases you will need to pass the parts that matter to the Database constructor. This reduces Coupling which, in turn, reduces the amount of rewrite needed during refactoring.

There is little more I can offer except questions. For example, what is Security doing? Is there only the one function? What else is declared in it? The same goes for Misc. Both of these classes sound like they don't really belong and are classes for the sake of being classes.

EDIT

... the config class sounds interesting. Do you have any good places where I should start looking for a decent way?

The Zend_Config documentation, part of the Zend Framework, would probably be a good place to see how people are using something like this.

This is my Misc class:

You could keep this I suppose, but Misc is a poor name for it. Calling some thing Misc is just asking for disorganization. If you really want this to be a function, and you can support PHP 5.3, I would wrap this in a namespace and treat the namespace similar to what most people would call a module.

Example:

<?php
namespace Utilities
{
    function validateEmail( $email )
    {
        // Code
    }
}

However, you should really consider refreshing yourself on the PHP documentation, specifically the newer stuff that's been added since 4.0. A perfect example of this is the use of the Filter extension. In your case, FILTER_VALIDATE_EMAIL will do wonders.

Security:

This class, I won't really comment on because I'm not sure the use case or need. I'll assume it is necessary as is.

Similar to Database you have high Coupling by putting the private key directly into the class. Depending on the needs of the key, it could exist in a config file and be treated just like the Database config attributes. On initialization, you would pass the config var for passkey into Security::__constructor.

Doing so means that you could have a different passkey per user instead of a single passkey. Expanding this to a public+private key system in the future would also be easy (just add another parameter).

User:

This class is also plagued with high Coupling because of the self loading of Database. There are a few ways to handle dependency injection and I'm not going to sway you either way on this matter. However, I'd suggest you spend some time looking through the many articles your favorite search engine will provide (or Bing! and decide...lol).

I would also keep from naming functions in all lower case. It is common for people to name functions either in CamelCase or underlines _ between words. Not doing so can impact readability.

Aside form that, this class is the first I've seen you provide that is acting as a Model (thinking in terms of MVC). That's a good thing, so nice job. I would expand your understanding of MVC if this is the path you are headed for so that you don't fall into many of the common pitfalls.

Source Link
Kevin Peno
  • 233
  • 1
  • 6

The only thing I see so far is that you have high Coupling between the Database class and the location of the config file. What you should do is either:

  • Create a config class that handles config parsing and setting
  • Or, at minimum, load the config file somewhere else

In both cases you will need to pass the parts that matter to the Database constructor. This reduces Coupling which, in turn, reduces the amount of rewrite needed during refactoring.

There is little more I can offer except questions. For example, what is Security doing? Is there only the one function? What else is declared in it? The same goes for Misc. Both of these classes sound like they don't really belong and are classes for the sake of being classes.