current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

I am currently writing a simple PHP script that uses themes and I found myself writing these methods as part of a class that handles theme config files:

public function getArrayOfThemeNames()
{
    $this->loadUnfilteredDirectoryListing();
    $this->loadThemeFiles();
    return $this->theme_files;
}

private function loadUnfilteredDirectoryListing()
{
    $this->unfiltered_dir_listing = scandir($this->theme_config_directory);
}

private function loadThemeFiles()
{
    foreach($this->unfiltered_dir_listing as $file)
    {
        if($file != ".." and $file != ".")
            $this->theme_files[] = $file;
    }
}

Does the getArrayOfThemeNames() method violate SRP, as it could be argued it does two (or potentially even three)? It loads the theme files array and then returns it.

Am I over-thinking this or should this be split into two methods -- one that fills the themes array, and another that returns it?

share|improve this question
1  
I'm no PHP guru, but is there any reason why you're using class members to pass values around instead of just local variables? – MetaFight Jun 11 '13 at 14:31
    
My view -- and it may be wrong, I'd be happy to have thoughts on this -- was that it avoiding unnecessary use of parameters which wouldn't contribute anything. – Racktash Jun 11 '13 at 16:36
    
On further consideration -- I am now edging towards using arguments, as the variables are modified and used only in one place. – Racktash Jun 11 '13 at 18:57

1 Answer 1

up vote 1 down vote accepted

Well, the function kind-of sort-of violates SRP...but according to Uncle Bob, I think your class violates the SRP moreso than the function.

Your class above, has three functions within it: getArrayOfThemeNames(), loadUnfilteredDirectoryListing(), and loadThemeFiles(). One of them scans the directory, one of theme creates an array of theme names, and another loads the theme files. One of those methods should not be in there: loadUnfilteredDirectoryListing should be in its own class called DirectoryLister{} or something. Which means, by extension, that getArrayOfThemeNames() is in violation of SRP. To quote Atwood - SRP is about "choosing a single, clearly defined goal for any particular bit of code: Do One Thing ... But in choosing one thing, you are ruling out an infinite universe of other possible things you could have done. It also means consciously choosing what your code won't do.". So in the end, its up to you whether this violates SRP and whether you would choose to follow SRP so deeply that you violate OOP or DRY.

Perhaps someone with more experience can chime in or prove me wrong (heck, always good to learn something from someone else)

EDIT: See (http://www.oodesign.com/single-responsibility-principle.html) article as well for clarification.

REVISIT 01/26/2015: Revisiting this answer, I would rewrite the code as follows:

class Theme {
    public $theme_directory;
    public $theme_files;

    public function __construct($theme_directory){
        $this->theme_directory = $theme_directory;
    }

    public function get_theme_names() /* Array */ {
        $this->load_theme_files();
        return $this->theme_files;
    }

    private function load_theme_files(Directory $directory) /* Null */ {
        $directory->directory = $this->theme_directory;
        foreach($directory->load_unfiltered() as $file) {
            if($file != '..' && $file != '.') {
                $this->theme_files[] = $file;
            }
        }
    }
}

class Directory {
    public $directory;

    public function __construct() {
        $this->directory = $directory;
    }

    private function load_unfiltered() /* array */ {
        return  scandir($this->directory);
    }
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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