2
\$\begingroup\$

This was mostly made for fun, so I don't see it having any real world purpose any time soon.

<?php

namespace Junky\Components\Cookie;

use Symfony\Component\Yaml\Exception\ParseException;
use Symfony\Component\Yaml\Yaml;

/**
 * Class Cookie
 *
 * @package Junky\Components\Cookie
 */
class Cookie
{
    /**
     * The YAML full config path
     *
     * @var string
     */
    protected $configPath = null;

    /**
     * The cookie value parameter
     *
     * @see http://php.net/setcookie
     *
     * @var
     */
    protected $value = null;

    /**
     * The cookie name parameter
     *
     * @see http://php.net/setcookie
     *
     * @var
     */
    protected $name = null;

    /**
     * The cookie expiration date parameter
     *
     * @see http://php.net/setcookie
     *
     * @var
     */
    protected $expire = null;

    /**
     * The cookie path parameter
     *
     * @see http://php.net/setcookie
     *
     * @var
     */
    protected $path = null;

    /**
     * The cookie domain parameter
     *
     * @var
     */
    protected $domain = null;

    /**
     * The cookie secure parameter
     *
     * @see http://php.net/setcookie
     *
     * @var
     */
    protected $secure = false;

    /**
     * The cookie httponly parameter
     *
     * @see http://php.net/setcookie
     * @var
     */
    protected $httponly = false;

    /**
     * If this is true, the config values will be used
     * Using config values will overwrite any values set
     * before the config was loaded
     *
     * Default is false
     * If the config is used, make sure that it's a valid
     * YAML file and has the proper fields
     *
     *  Default YAML values:
     *
     *   expire: 3600
     *   path: /
     *   domain: /
     *   secure: false
     *   httponly: false
     *   prefix: null
     *   cache: false
     *   cachePath: null
     *
     * @var null
     */
    protected $usesConfig = false;

    /**
     * The cookie prefix
     * If it's false the cookie name will have no prefix
     *
     * @var false
     */
    protected $prefix = false;

    /**
     * The cache path file
     * Should be a full name
     *
     * @var null
     */
    protected $cachePath = null;

    /**
     * Should results be cached?
     *
     * @var bool
     */
    protected $cache = false;

    ///////////////////
    // Begin Setters //
    //////////////////

    /**
     * Tells Cookie weather to use caching or not
     *
     * Do note that caching is mostly for debugging
     * purposes, it doesn't have any real world uses
     *
     * @param bool|false $cache
     *
     * @return $this
     */
    public function useCache($cache = false)
    {
        $this->cache = (bool)$cache;

        return $this;
    }

    /**
     * If the is set to true then the values
     * in the YAML config will be used
     *
     * @param $usesConfig
     *
     * @return $this
     */
    public function useConfig($usesConfig)
    {
        $this->usesConfig = $usesConfig;

        return $this;
    }

    /**
     * Sets a custom path for the cache file
     * Normally this is left as the default
     *
     * @param $cachePath
     *
     * @return $this
     */
    public function setCachePath($cachePath)
    {
        if( ! file_exists($cachePath) || ! is_writable($cachePath) || is_dir($cachePath))
        {
            throw new \InvalidArgumentException('Cache path is invalid. Make sure that the file exists, is writable and is not a directory');
        }

        $this->cachePath = $cachePath;

        return $this;
    }

    /**
     * Sets a prefix and validates said prefix
     *
     * @param null $prefix
     *
     * @return $this
     */
    public function setPrefix($prefix)
    {
        $this->validateName($prefix);

        $this->prefix = $prefix;

        return $this;
    }

    /**
     * Removes the previously set prefix
     *
     * @return $this
     */
    public function removePrefix()
    {
        $this->prefix = false;

        return $this;
    }

    /**
     * @param $configPath
     *
     * @return $this
     */
    public function setConfigPath($configPath)
    {
        $this->configPath = $configPath;

        return $this;
    }

    /**
     * Validates and sets the cookie name
     *
     * @param mixed $name
     *
     * @return $this
     */
    public function setName($name)
    {
        $this->validateName($name);

        $this->name = $name;

        return $this;
    }

    /**
     * Sets the cookie value
     *
     * @param mixed $value
     *
     * @return $this
     */
    public function setValue($value)
    {
        $this->value = $value;

        return $this;
    }

    /**
     * Sets the expire date for the cookie
     *
     * @param $expire int | string | \DateTime | \DateTimeInterface
     *
     * @throws \InvalidArgumentException
     *
     * @return $this
     */
    public function setExpire($expire)
    {
        if($expire instanceof \DateTime || $expire instanceof \DateTimeInterface)
        {
            $expire = $expire->format('U');
        }
        elseif( ! is_numeric($expire))
        {
            $expire = strtotime($expire);

            if(false === $expire || -1 === $expire)
            {
                throw new \InvalidArgumentException('The cookie expiration time is not valid.');
            }
        }

        $this->expire = $expire;

        return $this;
    }

    /**
     * @param mixed $httponly
     *
     * @return $this
     */
    public function setHttponly($httponly)
    {
        $this->httponly = (bool)$httponly;

        return $this;
    }

    /**
     * @param mixed $secure
     *
     * @return $this
     */
    public function setSecure($secure)
    {
        $this->secure = (bool)$secure;

        return $this;
    }

    /**
     * @param mixed $domain
     *
     * @return $this
     */
    public function setDomain($domain)
    {
        $this->domain = $domain;

        return $this;
    }

    /**
     * @param mixed $path
     *
     * @return $this
     */
    public function setPath($path)
    {
        $this->path = empty($path) ? '/' : $path;;

        return $this;
    }

    /////////////////
    // End Setters //
    /////////////////

    ///////////////////
    // Begin Getters //
    ///////////////////

    /**
     * @return mixed
     */
    public function getPath()
    {
        return $this->path;
    }

    /**
     * @return mixed
     */
    public function getExpire()
    {
        return $this->expire;
    }

    /**
     * @return mixed
     */
    public function getDomain()
    {
        return $this->domain;
    }

    /**
     * @return mixed
     */
    public function isSecure()
    {
        return (bool)$this->secure;
    }

    /**
     * @return mixed
     */
    public function isHttponly()
    {
        return (bool)$this->httponly;
    }

    /**
     * @return string
     */
    public function getConfigPath()
    {
        return $this->configPath;
    }

    /**
     * @return mixed
     */
    public function getValue()
    {
        return $this->value;
    }

    /**
     * @return mixed
     */
    public function getName()
    {
        return $this->name;
    }

    /**
     * @return null
     */
    public function getUsesConfig()
    {
        return $this->usesConfig;
    }

    /**
     * @return null
     */
    public function getPrefix()
    {
        return $this->prefix;
    }

    /**
     * @return mixed
     */
    public function getCachePath()
    {
        return $this->cachePath;
    }

    /**
     * @return boolean
     */
    public function usesCache()
    {
        return $this->cache;
    }

    /////////////////
    // End Getters //
    /////////////////

    //////////////////////////////
    // Begin Internal Functions //
    /////////////////////////////
    /**
     * @return mixed
     * @throws \Exception
     */
    private function buildCachePath()
    {
        if(is_null($this->getCachePath()) || empty(trim($this->getCachePath())))
        {
            $this->setCachePath(__DIR__ . DIRECTORY_SEPARATOR . 'cache' . DIRECTORY_SEPARATOR . 'cookies');
        }

        return $this->cachePath;
    }

    /**
     * @return mixed|string
     * @throws \Exception
     */
    private function readFromCache()
    {
        $cachedItems = file_get_contents($this->buildCachePath());

        if(false === $cachedItems)
        {
            // The flow cannot continue and an exception is in order
            throw new \Exception('Unable to read cached data');
        }

        if(empty($cachedItems))
        {
            return '';
        }

        return unserialize($cachedItems);
    }

    /**
     * @param $data
     *
     * @throws \Exception
     */
    private function writeToCache($data)
    {
        if(false === file_put_contents($this->buildCachePath(), serialize($data)))
        {
            // The flow cannot continue, again, an exception is in order
            throw new \Exception('Unable to write to cache file');
        }
    }

    /**
     * @return mixed|string
     */
    private function buildCookieName()
    {
        if(false !== $this->prefix)
        {
            $cookieName = $this->prefix . $this->name;
        }
        else
        {
            $cookieName = $this->name;
        }

        return $cookieName;
    }

    /**
     * The cookie name should not contain any of the
     * characters listed below, else PHP will get mad
     *
     * @param $name
     */
    private function validateName($name)
    {
        if(is_array($name))
        {
            if( ! empty(preg_grep("#[=,; \t\r\n\013\014]#", $name)))
            {
                throw new \InvalidArgumentException(sprintf('The cookie name "%s" contains invalid characters.', $name));
            }
        }
        else
        {
            if(preg_match("#[=,; \t\r\n\013\014]#", $name))
            {
                throw new \InvalidArgumentException(sprintf('The cookie name "%s" contains invalid characters.', $name));
            }
        }

        if(empty($name))
        {
            throw new \InvalidArgumentException('The cookie name cannot be empty.');
        }
    }

    /**
     * Parses the YAML config an sets certain properties
     * of the class if they exist in the YAML
     *
     * @return bool
     */
    private function loadConfig()
    {
        if( ! is_null($this->configPath))
        {
            try
            {
                $yamlValues = Yaml::parse(file_get_contents(__DIR__ . DIRECTORY_SEPARATOR . 'config.yaml'));
            }
            catch(ParseException $e)
            {
                throw new ParseException('Cannot parse YAML config file');
            }

            foreach($yamlValues as $key => $value)
            {
                if(property_exists($this, $key))
                {
                    $this->$key = $value;
                }
            }

            $this->usesConfig = true;
        }
        elseif(file_exists(__DIR__ . DIRECTORY_SEPARATOR . 'config.yaml'))
        {
            try
            {
                $yamlValues = Yaml::parse(file_get_contents(__DIR__ . DIRECTORY_SEPARATOR . 'config.yaml'));
            }
            catch(ParseException $e)
            {
                throw new ParseException('Cannot parse YAML config file');
            }

            if(empty($yamlValues))
            {
                return false;
            }

            foreach($yamlValues as $key => $value)
            {
                if(property_exists($this, $key))
                {
                    $this->$key = $value;
                }
            }

            $this->usesConfig = true;
        }

        return true;
    }

    ////////////////////////////
    // End Internal Functions //
    ///////////////////////////

    ///////////////////////////
    // Start Cache Functions //
    ///////////////////////////
    /**
     * @throws \Exception
     */
    public function cachePut()
    {
        if(
            is_null($this->name) ||
            is_null($this->value)
        )
        {
            throw new \Exception('Unable to cache cookie. Cookie name and cookie value are empty');
        }

        $cachedItems = $this->readFromCache();

        if(empty($cachedItems))
        {
            $cachedItems = [];
        }

        // Keep other values in the cache, but if the cookie name already exists, overwrite it
        $cachedItems[ $this->buildCookieName() ] = [
            'name'     => $this->buildCookieName(),
            'value'    => $this->value,
            'expire'   => $this->expire,
            'path'     => is_null($this->path) ? 'null' : $this->path,
            'domain'   => is_null($this->domain) ? 'null' : $this->domain,
            'secure'   => is_null($this->secure) || false === $this->secure ? 'false' : $this->secure,
            'httponly' => is_null($this->httponly) || false === $this->httponly ? 'false' : $this->httponly,
        ];

        $this->writeToCache($cachedItems);

        return $this;
    }

    /**
     * @return mixed|null|string
     * @throws \Exception
     */
    public function cacheGetAll()
    {
        $cache = $this->readFromCache();

        return empty($cache) ? '' : $cache;
    }

    /**
     * Returns an item from the cache based on its name
     * The name is whatever the cookie name is
     *
     * @param $name
     *
     * @return null
     * @throws \Exception
     */
    public function cacheGet($name)
    {
        $cachedItems = $this->readFromCache();

        if(empty($cachedItems))
        {
            return null;
        }

        if(is_array($name))
        {
            if(count($name) !== count($name, COUNT_RECURSIVE))
            {
                throw new \InvalidArgumentException('The $name argument cannot be a multidimensional array');
            }

            $this->validateName($name);

            $data = [];

            foreach($cachedItems as $key => $value)
            {
                if(in_array($key, $name))
                {
                    if(isset($cachedItems[ $key ]))
                    {
                        $data[ $key ] = $value;
                    }
                    else
                    {
                        $data[ $key ] = null;
                    }
                }
            }

            return $data;
        }

        if(empty($cachedItems) || ! isset($cachedItems[ $name ]))
        {
            return null;
        }

        return $cachedItems[ $name ];
    }

    /**
     * @param $name
     *
     * @return bool
     * @throws \Exception
     */
    public function cacheDelete($name)
    {
        $cachedItems = $this->readFromCache();

        if(empty($cachedItems))
        {
            return false;
        }

        if(is_array($name))
        {
            if(count($name) !== count($name, COUNT_RECURSIVE))
            {
                throw new \InvalidArgumentException('Unable to accept multidimensional arrays');
            }

            $this->validateName($name);

            foreach($cachedItems as $key => $value)
            {
                if(in_array($key, $name))
                {
                    unset($cachedItems[ $name ]);
                }
            }
        }
        else
        {
            $this->validateName($name);

            if( ! isset($cachedItems[ $name ]))
            {
                return false;
            }

            unset($cachedItems[ $name ]);
        }

        $this->writeToCache($cachedItems);

        return true;
    }

    /**
     * @throws \Exception
     */
    public function cacheClear()
    {
        $this->writeToCache('');

        return $this;
    }

    /////////////////////////
    // End Cache Functions //
    /////////////////////////

    ////////////////////////////
    // Begin Cookie Functions //
    ///////////////////////////

    /**
     * @param null $name
     * @param null $value
     * @param null $expire
     * @param null $path
     * @param null $domain
     * @param null $secure
     * @param null $httponly
     *
     * @return $this
     * @throws \Exception
     */
    public function set($name, $value, $expire = null, $path = null, $domain = null, $secure = null, $httponly = null)
    {
        // Use the values passed in to the function
        if( ! $this->usesConfig)
        {
            if(is_null($this->expire) || is_null($expire))
            {
                $this->setExpire($this->forever());
            }
            else
            {
                $this->setExpire($expire);
            }

            $this->setPath($path);
            $this->setDomain($domain);
            $this->setSecure($secure);
            $this->setHttponly($httponly);
        }
        // If config is used load the values from there
        else
        {
            $this->loadConfig();
        }

        $this->setName($name);
        $this->setValue($value);

        // Cache the request even if setcookie() fails
        if($this->cache)
        {
            $this->cachePut();
        }

        // Set a cookie
        if( ! setcookie(
            $this->buildCookieName(),
            $this->value,
            $this->expire,
            $this->path,
            $this->domain,
            $this->secure,
            $this->httponly
        )
        )
        {
            throw new \Exception(sprintf('Unable to set cookie %s'), $this->name);
        }

        return $this;
    }

    /**
     * @param            $name
     * @param bool|false $tryCache
     * @param null       $default
     *
     * @return null
     */
    public function get($name, $tryCache = false, $default = null)
    {
        if( ! isset($_COOKIE[ $name ]))
        {
            if($tryCache)
            {
                $cached = $this->cacheGet($name);

                if(is_null($cached))
                {
                    return $default;
                }

                return $cached;
            }

            return null;
        }

        return $_COOKIE[ $name ];
    }

    /**
     * @param $name
     *
     * @return null
     */
    public function expire($name)
    {
        // try to delete from cache
        // but don't return false it fails
        $this->cacheDelete($name);

        // try to delete from superglobal $_COOKIE
        // but don't return false it fails
        if(isset($_COOKIE[ $name ]))
        {
            unset($_COOKIE[ $name ]);
        }

        setcookie($name, '', time() - $this->expire);
        setcookie($name, '', time() - $this->expire, '/');

        return true;
    }

    /**
     * @return $this
     */
    public function expireAll()
    {
        // try to "unset" any cookies from the cache
        $cachedItems = $this->cacheGetAll();

        if( ! empty($cachedItems))
        {
            foreach($cachedItems as $name => $item)
            {
                $this->expire($name);
            }
        }

        // clear the cookies from headers
        if(isset($_SERVER['HTTP_COOKIE']))
        {
            $cookies = explode(';', $_SERVER['HTTP_COOKIE']);
            if( ! empty($cookies))
            {
                foreach($cookies as $cookie)
                {
                    $parts = explode('=', $cookie);
                    $name = trim($parts[0]);

                    $this->expire($name);
                }
            }
        }

        $this->cacheClear();

        return $this;
    }

    /**
     * @return $this
     * @throws \Exception
     */
    public function forever()
    {
        $this->setExpire(strtotime('+1 year', time()));

        return $this;
    }

    /**
     * @return $this
     */
    public function once()
    {
        $this->setExpire(0);

        return $this;
    }

    /**
     * @return $this
     */
    public function oneDay()
    {
        $this->setExpire(strtotime('+1 day', time()));

        return $this;
    }

    /**
     * @return $this
     */
    public function oneWeek()
    {
        $this->setExpire(strtotime('+1 week', time()));

        return $this;
    }

    //////////////////////////
    // End Cookie Functions //
    //////////////////////////

    ////////////////////////////
    // Begin Header Functions //
    ///////////////////////////
    /**
     * @param bool|true  $toArray
     * @param bool|false $timestamp
     *
     * @return array
     */
    public function getFromHeader($toArray = true, $timestamp = true)
    {
        $cookies = [];
        $headers = headers_list();

        // see http://tools.ietf.org/html/rfc6265#section-4.1.1
        foreach($headers as $header)
        {
            if(strpos($header, 'Set-Cookie: ') === 0)
            {
                $pair = [];

                // Get only what's after 'Set-cookie: ' header
                $value = str_replace('&', urlencode('&'), substr($header, 12));

                if($toArray)
                {
                    // explode all the values by ;
                    // normally a header for cookies looks like this
                    // Set-Cookie: a_name=another_value; expires=Thu, 05-Oct-2017 01:01:21 GMT; Max-Age=31536000; path=/; domain=test.com; secure; httponly
                    $values = explode(';', $value);

                    // get the cookie name, aka the first value in the Set-Cookie header
                    $explodeForName = explode('=', $values[0]);

                    // get the actual cookie name
                    $cookieName = $explodeForName[0];

                    // set the cookie name as the key of the array
                    $cookies[ $cookieName ][ $cookieName ] = trim($explodeForName[1]);

                    // unset the first value in the values array
                    // it's no longer needed
                    unset($values[0]);

                    foreach($values as $key => $item)
                    {
                        // create a temporary array by exploding using =
                        $temp = explode('=', $item);

                        // this is for values such as "secure" and "httponly"
                        // if index 1 doesen't exit it means that one of the 2 is the culprit
                        if( ! isset($temp[1]))
                        {
                            $cookies[ $cookieName ][ trim($temp[0]) ] = true;
                        }
                        else
                        {
                            // set the first value in the temp array as the key
                            // and the second as the value
                            // such as expires=Thu, 05-Oct-2017 01:01:21 GMT
                            // results in ['expires' => 'Thu, 05-Oct-2017 01:01:21 GMT']

                            if($timestamp)
                            {
                                if(trim($temp[0]) == 'expires')
                                {
                                    // create a unix time stamp
                                    $cookies[ $cookieName ][ trim($temp[0]) ] = strtotime(trim($temp[1]));
                                }
                                else
                                {
                                    $cookies[ $cookieName ][ trim($temp[0]) ] = trim($temp[1]);
                                }
                            }
                            else
                            {
                                $cookies[ $cookieName ][ trim($temp[0]) ] = trim($temp[1]);
                            }
                        }
                    }
                }
                else
                {
                    $pair[] = current(explode(';', $value, 1));
                    $cookies = array_merge_recursive($cookies, $pair);
                }

            }
        }

        return $cookies;
    }

    //////////////////////////
    // End Header Functions //
    /////////////////////////
}

A simple example usage would be like so:

require 'vendor/autoload.php';

$cookie = new \Junky\Components\Cookie\Cookie();

$cookies = $cookie
    ->forever()
    ->set('cookie_name', 'cookie_value', null, '/', 'www.test.com', true, true)
    ->once()
    ->set('another_cookie_name', 'one more cookie value')
    ->cachePut()
    ->oneDay()
    ->setPrefix('cookie_')
    ->set('with_prefix', 'one more cookie value')
    ->removePrefix()
    ->set('no_more_prefix', 'one more cookie value')
    ->getFromHeader(true, true);

print_r($cookies);

Of course you could chain them on forever, with regards to logical usage of course.

If you wanna see it on git, have a look here.

\$\endgroup\$
1
\$\begingroup\$

First off, I think this code is one of the better "professional-level" PHP examples one would expect to see here. So, kudos on writing clean, well-documented, well-thought-out code.

My biggest point of feedback here, is that I am not quite sure what this class is intending to be. Perhaps that is because the scope of the class is quite large. This class seems to want to provide the following functionalities:

  • Acting as object-oriented interface to perform CRUD operations for individual cookies.
  • Overall cookie management class (i.e. cookie collection type of functionalities)
  • A cookie factory (being able to create cookies from headers, for example)
  • A cookie caching mechanism
  • A configuration reader/parser

I would suggest that you might ultimately want to consider splitting this functionality into different classes. Perhaps with classes such as:

  • Cookie - to represent and expose functionality for a single cookie. Also holds low-level interface to $_COOKIE and set_cookie() operations.
  • CookieCollection - to manage the collection as a whole
  • CacheableCookie - perhaps extends Cookie class to override methods as necessary to interact with cache (with perhaps the caching mechanism itself, in the form of an object, passed as a dependency to this class)
  • CookieFactory - to manage creation of cookie objects. This functionality could perhaps just be part of CookieCollection above if this is not a significant amount of logic to where it makes sense to separate.
  • YAMLConfigProvider - class to manage reading of YAML files and constructing a configuration object/structure that the other classes above can use.

Other points of feedback:

You do little to validate that the parameters being passed to your public methods are acceptable to work with. You should strongly consider beginning to apply type hints into the method signatures such that exceptions are thrown when unexpected values are passed. For cases where type-hinting can't help you (like when passing string or integer values), then some basic validation should be the very first thing that you do inside the method (i.e. non-zero length string required, positive integer value required, boolean value required, etc.). Fail fast with an InvalidArgumentException or similar when these criteria are not met.

Also, consider asking the caller to be more specific in passing certain parameters. For example, on your setExpire() method, you allow a variety of input types. Do you really need this flexibility (and the added code complexity)? If the code can operate against anything implementing DateTimeInterface, why not just put that in type hint? Why check for both DateTime and DateTimeInferace when DateTime implements this interface? Do you REALLY need to accept arbitrary input and the edge cases it might expose in your code? Like right now, what happens if I pass a value of 3.14159 to this method? It falls through your conditional and gets set on the property, because it is_numeric() and is not DateTimeInterface object. You probably don't want to have to write 15-20 lines of guarding code for this simple setter operation.

Similarly, with methods like setHttponly() (bad camel case here), you should enforce a boolean parameter as being passed rather than casting the parameter as boolean and potentially introducing unexpected "truthy"/"falsey" behavior. Right now, I could pass an object to this method and have that evaluate as truthy, making this cookie HTTP only, even though the caller's method usage was incorrect and should have caused an exception in your system. Be very specific when you set up the interactions between your classes and class callers. The looser you permit these interactions to be, the more problems you will have with bugs in your code and being able to quickly debug that code.


I don't think that it is appropriate to have this class (or really any of the classes in a proposed multi-class cookie management library), to have to understand how to parse YAML configurations. I really like that you are looking to inject configuration into this class (making it more composable) as opposed to setting up a bunch of class constants, switch cases, if-else branching logic and some of the things that typically go along with trying to maintain configuration in classes.

I just don't think these classes should need to know how that config is created. You should consider just passing a configuration object as a dependency upon instantiation rather than have these classes hold logic on where config files lie. So probably a whole different class or set of classes should be considered for this purpose. Such class(es) would present tremendous opportunity for re-use as other portions of an application might need to be made to read YAML configurations as well. You don't want to repeat code similar to this in all places in your app where you need to read YAML configurations.


Consider using http_parse_cookie() or similar to parse the cookie header string rather than rolling your own solution.


I am concerned about the mutability of many of the properties of your cookie. Should you really even expose methods to change cookie domain, path, security, session vs. long term persistence, prefix, etc. after the cookie has been instantiated from its configuration? That kind of renders your application configuration moot and could be enabling the caller to introduce some very odd (and potentially insecure) behaviors.

\$\endgroup\$
  • \$\begingroup\$ I completely agree with everything you just said. The point of the class is too wide, it tries to do too many thing. I wanted to be used as a standalone class without too many dependencies. I could have created a separate class for reading/writing to a file(or rather a subclass of a "WriterInterface" of sorts), a factory too, a "Bag" class which would be an implementation of an arrayable class and so on, but as I said too many dependencies for just one class. It's mostly made for the hell of it. But i'll refactor some stuff, it's not like I got anything to lose. \$\endgroup\$ – Andrew Oct 5 '16 at 15:34
  • \$\begingroup\$ @Andrew Even if you are building something lightweight or for a narrowly-defined purpose. If the code is going to be used in production, you should still be thinking about class design and code quality concerns. Think of it as a habit that good software developers get into. Now you may say that for the sake of expediency you don't want to build out an entire library, but at least that should be more of a conscious decision on the part of the designer. Code quality concerns, however, like having good doc blocks, defining your interfaces strictly, etc should always be a part of your process. \$\endgroup\$ – Mike Brant Oct 5 '16 at 15:39
  • \$\begingroup\$ Hopefully nobody will use this in production, if they know what's good for them. But you make a fair point non the less. \$\endgroup\$ – Andrew Oct 5 '16 at 15:46
  • \$\begingroup\$ @Andrew Be careful not to select an answer too quickly here on code review. This could end up discouraging others from sharing any feedback they might have. Might be good idea to leave answers unaccepted for a day or two to solicit additional feedback. \$\endgroup\$ – Mike Brant Oct 5 '16 at 15:54
  • \$\begingroup\$ I accepted it mostly because it touches exactly what I was afraid of. So as far as I'm concerned it's quite an acceptable answer, no worries, I plan to rewrite it and post it again. Others will have a fair chance to review it too :P \$\endgroup\$ – Andrew Oct 5 '16 at 15:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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