Thread-safe "singleton" that destroys object when not used anymore I'm working on some legacy code which uses the singleton pattern. The problem I have with the traditional singleton is that the instance lives until the program terminates. This is messing up the order in which things need to get destroyed and makes it difficult to write independent unit tests. Unfortunately, I can't get rid of the singleton by constructing a single instance and passing it around since this would break APIs.

I wrote the following wrapper around a class T that hands out shared pointers to a single instance of T. When the last shared pointer goes out of scope the object gets destroyed.

#include <memory>
#include <type_traits>
#include <mutex>

template <class T>
class refCountedSingleton
{
  static_assert(!std::is_default_constructible<T>::value, "T must have a private/protected constructor to ensure it can only be constructed by refCountedSingleton");
  static_assert(!std::is_copy_constructible<T>::value, "T must have a private/protected copy constructor to ensure instances handed out by refCountedSingleton cannot be copied");
  static_assert(!std::is_copy_assignable<T>::value, "T must have a private/protected copy assignment operator to ensure instances handed out by refCountedSingleton cannot be copied");
  static std::weak_ptr<T> p;
  static std::mutex mu;
public:
  static std::shared_ptr<T> getShared();
};

template <class T>
std::weak_ptr<T> refCountedSingleton<T>::p;
template <class T>
std::mutex refCountedSingleton<T>::mu;

template <class T>
std::shared_ptr<T> refCountedSingleton<T>::getShared()
{
    std::lock_guard<std::mutex> lock(mu);
    std::shared_ptr<T> temp = p.lock();
    if (!temp) {
      temp.reset(new T());
      p = temp;
    }
  return temp;
}

The following class could be used as T:

class obj {
  friend class refCountedSingleton<obj>;
private:
  obj() {}
  obj(obj const&);
  obj& operator=(obj const&);
};

Example use case:

{
  std::shared_ptr<obj> pm1 = refCountedSingleton<obj>::getShared();
  {
    // calling getShared() multiple times yields pointers to the same object
    std::shared_ptr<obj> pm2 = refCountedSingleton<obj>::getShared();
  }
} // instance of obj gets destroyed here
{
   // a new instance of obj gets greated
   std::shared_ptr<obj> pm3 = refCountedSingleton<obj>::getShared();
}

My singleton needs to be thread safe which is why I'm locking the mutex in getShared(). With the traditional singleton pattern performance can be improved by avoiding the expensive locking most of the time by leveraging double-checked locking. I'm wondering if something similar could be done here as well.

The problem with the above code is that it doing two things with one piece of code.

1: Singleton Creation.
2: Singelton LifeSpan.

I would separate these out into two separate classes.

The creation is done via an abstract factory (MySingletonFactory) but lifespan is controlled via explicit factory classes that are defined for the situation (so your code does not define the life span of the singleton).

The other issue of the above code is that I have seen this not working because of the way global variables are created in libraries. When these libraries are linked together it is not required to remove duplicate variables (as all variables were already resolved as much as possible during compilation). The linking phase will only try and resolve unresolved dependencies not all linkers will try and remove duplicated global variables (though I suppose this will depend on how sophisticated the linker is).

Here is how I would resolve the problem (by removing globals (using static storage function scope variables)).

Interfaces

#include <memory>
#include <iostream>
#include <cstdlib>

class MySingleInterface
{
    public:
        virtual ~MySingleInterface() {}

        virtual int doWork() = 0;
};
class MySingleFactoryInterface
{
    public:
        virtual ~MySingleFactoryInterface() {}
        virtual std::shared_ptr<MySingleInterface>  createTheObject() = 0;
};

Abstract Factory (I think?)

class MySingletonFactory
{
    private:
        friend class MySingleInterface;
        static MySingleFactoryInterface& getCurrentFactory(MySingleFactoryInterface* replace)
        {
            // Do not strictly need a default factory.
            // But it is nice to have one for standard (default)
            // operations. Then you only specify one in non standard
            // situations like testing.
            static MyDefaultSingeltonFactory           defaultFactory;

            // The current factory we are using.
            static MySingleFactoryInterface*           currentFactory = &defaultFactory;
            if (replace)
            {
                std::swap(replace, currentFactory);
            }
            else
            {
                replace = currentFactory;
            }
            return *replace;
        }
    public:
        static std::shared_ptr<MySingleInterface> getInstance()
        {
            return getCurrentFactory(nullptr).createTheObject();
        }
        static MySingleFactoryInterface& setFactory(MySingleFactoryInterface& alternativeFactory)
        {
             return getCurrentFactory(&alternativeFactory);
        }
};

Example Default Singleton

class MyDefaultSingeltonFactory;
class MyDefaultSingleton: public MySingleInterface
{
    private:
        friend class MyDefaultSingeltonFactory;
        MyDefaultSingleton(){}
    public:
        virtual int doWork() override {static int value; return value++;}   // Normal operation.
};
class MyDefaultSingeltonFactory: public MySingleFactoryInterface
{
    // A Normal Singleton factory.
    // The singelton lives from creation until the end of the program.
    // Note: This is the behavior of this factory does not need to
    //       to be universal behavior.
    public:
        std::shared_ptr<MySingleInterface>  createTheObject() override
        {
            std::shared_ptr<MySingleInterface>  instance = std::shared_ptr<MySingleInterface>(new MyDefaultSingleton());
            return instance;
        }
};

Example Test Singelton

class MyTestSingleton: public MySingleInterface
{
    int value;
    public:
    MyTestSingleton()
        : value(rand())
    {}
    public:
        virtual int doWork() override {return value;}        // Operation for testing.
};

class MyTestSingletonFactory: public MySingleFactoryInterface
{
    // This factory creates a new instance each time.
    // Can be useful for some types of test.
    public:
        std::shared_ptr<MySingleInterface>  createTheObject() override
        {
            return std::make_shared<MyTestSingleton>();
        }
};

class MyTestPersistantSingletonFactory: public MySingleFactoryInterface
{
    // As long as the code holds a shared pointer it will be re-used.
    // Useful if you want to create the singleton during "setUp()" and
    // destroy it during "tearDown()" you get a brand new singleton for
    // each individual test.
    public:
        std::shared_ptr<MySingleInterface>  createTheObject() override
        {
            // I leave the exercise of locking to you.
            static std::weak_ptr<MySingleInterface>     persist;
            auto result = persist.lock();
            if (!result)
            {
                result = std::make_shared<MyTestSingleton>();
                persist = result;
            }
            return result;
        }
};

Test Harness

int main()
{
    srand(0);

    // Incrementing results.
    std::cout << MySingletonFactory::getInstance()->doWork() << "\n";
    std::cout << MySingletonFactory::getInstance()->doWork() << "\n";
    std::cout << MySingletonFactory::getInstance()->doWork() << "\n";
    std::cout << MySingletonFactory::getInstance()->doWork() << "\n";
    std::cout << MySingletonFactory::getInstance()->doWork() << "\n";
    std::cout << MySingletonFactory::getInstance()->doWork() << "\n";
    std::cout << MySingletonFactory::getInstance()->doWork() << "\n";
    std::cout << MySingletonFactory::getInstance()->doWork() << "\n";
    std::cout << MySingletonFactory::getInstance()->doWork() << "\n";

    // Each result indapendant.
    MyTestSingletonFactory  testFactory;
    MySingletonFactory::setFactory(testFactory);
    std::cout << MySingletonFactory::getInstance()->doWork() << "\n";
    std::cout << MySingletonFactory::getInstance()->doWork() << "\n";
    std::cout << MySingletonFactory::getInstance()->doWork() << "\n";
    std::cout << MySingletonFactory::getInstance()->doWork() << "\n";

    MyTestPersistantSingletonFactory    persistantFactory;
    MySingletonFactory::setFactory(persistantFactory);
    {
        // As long as we hold a reference then
        // we should get the same result.
        std::shared_ptr<MySingleInterface> local = MySingletonFactory::getInstance();
        std::cout << "L: " << local->doWork() << "\n";
        std::cout << "L: " << MySingletonFactory::getInstance()->doWork() << "\n";
        std::cout << "L: " << local->doWork() << "\n";
        std::cout << "L: " << MySingletonFactory::getInstance()->doWork() << "\n";
    }
    // No local reference so we will create a new value.
    std::cout << "N: " << MySingletonFactory::getInstance()->doWork() << "\n";
}

Output

> ./a.out
0
1
2
3
4
5
6
7
8
520932930
28925691
822784415
890459872
L: 145532761
L: 145532761
L: 145532761
L: 145532761
N: 2132723841


Validating input against rules I have created a method that validates inputs based on a set of rules. If an error is found, it is put into an array which can later on be used to display the errors to the user.

Below is my code:

register.php

<?php
    require_once('core/init.php');

    if(input::inputExists()){
        $validate = new validate();
        $validate->checkInput($_POST, array(
            'username' => array(
                'formatted_name' => 'username',
                'required' => 'true',
                'min' => 4,
                'max' => 25,
                'unique' => 'users'
                ),
            'password' => array(
                'formatted_name' => 'password',
                'required' => 'true',
                'min' => 5
                ),
            'confirm_password' => array(
                'formatted_name' => 'password confirmation',
                'matches' => 'password',
                'required' => 'true'
                )
        ));
        if ($validate->validationPassed()) {
            // Register new member...
        } else {
            // Return Errors....
            print_r($validate->getErrors());
        }
    }

?>

<form method="POST" autocomplete="off">

            Username

        <input type="text" name="username" id="username" />

            Password

        <input type="password" name="password" id="password" />

            Confirm Password

        <input type="password" name="confirm_password" id="confirm_password" />

        <input type="submit" />

</form>

validate.class.php

<?php
class validate {
    private $errors;
    private $passed = 'false';

    // public function __construct(){}

    public function checkInput($data, $items = array()) {
        foreach($items as $item => $rules){
            foreach($rules as $rule => $rule_value){
                if ($rule != 'formatted_name'){
                    if ($rule === 'required' && empty($data[$item])){
                        $this->addError("{$rules['formatted_name']} is required.");
                    }
                    if ($rule === 'min' && strlen($data[$item]) < $rule_value){
                        $this->addError("{$rules['formatted_name']} must be at least {$rule_value} characters long.");
                    }
                    if ($rule === 'max' && strlen($data[$item]) > $rule_value){
                        $this->addError("{$rules['formatted_name']} can be a maximum of {$rule_value} characters long.");
                    }
                    if ($rule === 'matches' && $data[$item] != $rule_value){
                        $this->addError("{$rules['formatted_name']} does not match {$items[$rule_value]['formatted_name']}");
                    }
                }
            }

        }
    }

    private function addError($error){
        $this->errors[] = $error;
    }

    public function validationPassed(){
        return empty($this->errors);
    }

    public function getErrors(){
        return $this->errors;
    }
}

Any improvements, suggestions or advice you can give me?

Right well first off, I'd consider looking into a framework such as Symfony, Laravel, or some other PHP web framework if you want to get serious about doing web design. Right now there is a lot less baggage in rolling your own framework, and the learning curve can be a little steep on them, but once you understand how they work you can spend way less time writing the framework and much more time implementing the actual features you care about.

There are a lot of suggestions I'd consider making, but most involve using routing/middleware layers to accomplish what you are doing here in order to make that form validation logic re-usable in register.php. The logic itself looks fine, but as your needs grow more and more complex you're going to run into some huge pitfalls with the architecture, or lack thereof, you are currently using.

Either way, I'd consider taking a look at Symfony's Validator component. It does a lot of what you're trying to accomplish, but in a really concise and powerful way and eliminates the need to write all of that rule logic in validate.class.php.

From a stylistic point of view, class names should be uppercased and given a noun not a verb. So your class validate -> Validator. Furthermore, your file names should be named after the class they represent and upper cased so validate.class.php -> Validator.php.

That being said, the code is organized quite nicely and is easy to read. Keep up the good work!


Printing command line arguments in C++ I'm learning C++ and here I've written a little toy program that is supposed to list all the arguments passed to it through the command line.

#include <iostream>;
using namespace std;

int main (int argc, char *argv[]) {
  for (int number = 1; number < argc; ++number) {
    cout << argv[number] << endl;
  }
  return 0;
}

It is possible to use indexing instead and rewrite it in the following way:

#include <iostream>
using namespace std;

int main (int argc, char *argv[]) {
  int i;

  for (int j = 1; j < argc; ++j) {
    i = 0;
    while (argv[j][i]) {
      cout << argv[j][i];
      ++i;
    }
    cout << endl;
  }
   return 0;
}

I personally like the first version more. It doesn't need one more variable and it doesn't need one more cycle, so making use of pointers here seems like a good idea: it helps improve readability and as far as I know performance.

Is this actually a good alternative to the second version that uses indexing? And if so, how could it be improved?

Is this actually a good alternative to the second version that uses indexing? And if so, how could it be improved?

I'm not sure what you're asking here. Obviously the first version is fine; obviously the second version is pretty silly (because it does the same thing as the first version but in a more confusing and verbose way).

As for "using indexing", doesn't the first version also use indexing? What do you think argv[i] is, if not "indexing"? It sure seems like you've got the hang of "indexing" well enough. :)

Your second program is also silly in that it uses a while loop instead of a for loop, even though you have all three pieces of a for-loop (initialization, condition, and increment) in close proximity. Why didn't you use a for loop? Just for practice with different kinds of loops? In that case, I'd dock you points for not using a do-while loop! ;)

To nitpick as long as I'm here:

Everyone will tell you (so you might as well start listening now) that using namespace std is a bad habit. Write std::cout and std::endl explicitly.
You have a stray ; on line 1 — a transcription error, since the compiler wouldn't accept it if you actually tried to compile that.
Your second program uses saner loop index names: i and j. Naming a loop control variable number is more likely to confuse the reader than help them.
Explicit return 0 from main is unnecessary in C++ (and also in C, as of 18 years ago). Omitting it from simple programs like this can help the reader focus on the important stuff instead of the boilerplate.

Putting it all together:

#include <iostream>

int main(int argc, char **argv)
{
    for (int i = 1; i < argc; ++i) {
        std::cout << argv[i] << std::endl;
    }
}


Correctly coding an OOP object I have a couple of functions in WordPress that I'm rewriting to OOP. All these functions are interlinked in some way and also reuse code from other functions.

This is an example of the first class I'm trying. I'm very new to OOP, but I have learned about separation of concern, so basically each concern get its own class.

interface Single_Post_Referrer_Interface {

    function init();

    function get_is_author_referrer();

    function get_is_date_referrer();

    function set_is_author_referrer( $is_author_referrer );

    function set_is_date_referrer( $is_date_referrer );

}

class Single_Post_Referrer_Class implements Single_Post_Referrer_Interface {

    private $is_author_referrer;
    private $is_date_referrer;

    function __construct() {

        $this->init();

    }

    function init() {

        $is_author_referrer = isset( $_GET['aq'] );
        $is_date_referrer   = isset( $_GET['dq'] );

        $this->set_is_author_referrer( $is_author_referrer );
        $this->set_is_date_referrer( $is_date_referrer );

    }

    function get_is_author_referrer() {

        return $this->is_author_referrer;

    }

    function get_is_date_referrer() {

        return $this->is_date_referrer;

    }

    function set_is_author_referrer( $is_author_referrer ) {

        $this->is_author_referrer = $is_author_referrer ;

        return $this;

    }

    function set_is_date_referrer( $is_date_referrer ) {

        $this->is_date_referrer = $is_date_referrer ;

        return $this;

    }

}

$a = new Single_Post_Referrer_Class();

This class basically is a lot of conditional checks (checks if a certain query variable is in the URL and sets the appropriate condition) and return the conditional checks with a boolean value which will be used by other classes.

As this is my first attempt, I would like to know if this is correctly done and if not, where I can improve. Just a note: I correctly get the correct values if I do a var_dump( $a );.

Separation of concern
You mention "separation of concern" in your question. It's fantastic that you actually know about it, and are trying to adopt good practices whilst learning about OO. However, allow me to be a pedantic tw*t for a second, and stress that SoC is not the same as SRP (Single Responsibility Principle).
It's the SRP that states that a class can have one, and only one reason to change. That's a fancy way of saying that 1 class == 1 job. SoC operates on a higher level. You may, or may not have, stumbled across buzzwords or terms like MVC. SoC is the main reason for architectural patterns like the MVC pattern: Each component (module) can, and often does, consist of a multitude of classes. Each of these classes have a single job that can be grouped under a specific module (ie: the "V" module (view) consists of a renderer class, a class that manages the cache, a class that manages the output buffers etc...).
Just a bit of a pedantic lecture, as an aside :-P
At first glance:
Well, the first thing that strikes me as, erm..., off is your failing to specify the visibility of your class/interface members. While it's true that PHP will default to public access unless you specify otherwise, and that interface members have to be public by definition, it's best to be specific, therefore change your methods from:
function init();//in interface

To:
public function init();

Another niggle I have is with your coding style. It might seem irrelevant at first, or a matter of personal preference (which it is, to a certain degree), but there are such things as coding standards. They exist for good reason, and should really be adhered to as much as possible. In this case, this means that:
function get_is_author_referrer();
//should become
public function getIsAuthorReferrer();
//or perhaps better still:
public function isAuthorReferrer();

In general, methods that start with get are getter methods  (methods that will return the value of a member/property). set methods are setters to set that member. If a method should return a boolean, it's quite common to see methods with an is prefix. Just like it's fairly common to see classes that implement a has method, to check the value of a given property, take this basic example:
class Foo
{
    /**
     * @var array|null
     */
    protected $someArray = null;

    /**
     * @param array|null $vals = null
     */
    public function __construct(array $vals = null)
    {
        $this->someArray = $vals;
    }

    /**
     * @return array|null
     */
    public function getVals()
    {
        return $this->someArray;
    }

    /**
     * @param array $vals
     * @return $this
     */
    public function setVals(array $vals)
    {
        $this->someArray = $vals;
        return $this;
    }

    /**
     * @return bool
     */
    public function isArray()
    {
        return is_array($this->someArray);
    }

    /**
     * @return bool
     */
    public function hasVals()
    {
        return !empty($this->someArray);
    }
}

I can check to see if the $someArray property of a given instance of Foo is null, or an array (using isArray). I can also check to see if that property isn't empty. I can get the value of the property, or set it using the getter and setter methods just fine. There's no real point in using additional properties for this, so I'd suggest you do something along the lines of:
protected $authorReferrer = null;
//in the init method:
$this->authorReferrer = isset( $_GET['aq'] ) ? $_GET['aq'] : null;
//add these methods:
public function isAuthorReferrer()
{
    return $this->authorReferrer !== null;//if not null
}
public function getAuthorReferrer()
{
    return $this->authorReferrer;
}

The upshot is that your class can now be used both to check if certain params were set in the $_GET superglobal (more on your use of this later), The instance can also be used to retrieve the values, and possibly validate or normalize them.
Try not to support people on EOL versions
Your class, and interface seem to be following the PSR-0 autoloading standard (to some degree). This standard states that the _ (underscores) in names like Single_Post_Referrer_Class will be replaced with directory separators. It was commonplace to use this in PHP versions prior to PHP 5.3 (which introduced namespace support). However, PSR-0 is now deprecated in favour of the PSR-4 standard, simply because there is no supported PHP version left that does not support namespaces. Heck, even 5.3 has been EOL'd about half a year ago.
What does this mean for you? Well, put simply: Shorter class names:
class Single_Post_Referrer_Class {}
//now becomes:
<?php
namespace MainNS\Single\Post;
class Referrer {}

Where MainNS is the root namespace for your project. I ditched the Class because that's a reserved keyword, and a rather dodgy class name anyway.
TL;TR: Embrace namespaces

Specific critiques
Now for some more specific critiques/issues I have with your code:
A constructor mustn't be abused
You're certainly not the only person to abuse a constructor. And to be honest, I've seen a lot worse that your code (honestly, it's not half bad), but let's be honest:
public function __construct()
{//standards: this { goes on the next line
    $this->init();
}

Your constructor calls a second method, and yes, all the method seemingly does is to initialize the instance, but look at your class from a user standpoint (the person using your code):
$x = new Referrer();

Does his code accurately reflect what is going on? Of course not! Can you force the user to only create instances of your class when there is a $_GET variable available? No, you can't. Someone might, for example, choose to write a CLI script using your codebase and (owing to name conflicts or for whatever reason) create an instance of your class, which relies entirely on their being a $_GET variable available.
I know there are many, many more complete definitions of a class out there, but here's what I think a class essentially is/should be:

"A class is a self-contained unit of code that couples data and functionality together in order to perform a single, task, regardless of the context in which task is to be performed"

What does that mean? Well, take the simplest of classes for example: a data model:
class Person
{
    /**
     * @var string
     */
    protected $name = null;

    /**
     * @var string
     */
    protected $email = null;

    /**
     * @var int
     */
     protected $age = null;

    /**
     * @param string $name
     * @param string $email
     * @param int $age
     */
    public function __construct($name, $email, $age)
    {
        $this->setName($name)
             ->setEmail($email)
             ->setAge($age);
    }

    /**
     * Basic, but somewhat unsafe setter (Relies on user to pass expected value)
     * @param string $name
     * @return $this
     */
    public function setName($name)
    {
        $this->name = $name;
        return $this;
    }

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

    /**
     * Example of validating setter: throws exceptions if value isn't valid
     * @param string $name
     * @return $this
     * @throws \InvalidArgumentException if email is not a valid address
     */
    public function setEmail($email)
    {
        if (!filter_var($email, \FILTER_VALIDATE_EMAIL))
        {//<-- allman brackets, not standard, I know... :-P
            throw new \InvalidArgumentException(
                sprintf(
                    '%s expects $email to be valid email address, %s is not',
                    __METHOD__,
                    $email
                )
            );
        }
        $this->email = $email;
        return $this;
    }

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

    /**
     * Example of setter that attempts some basic normalization
     * @param int $age
     * @return $this
     */
    public function setAge($age)
    {
        $this->age = abs((int) $age);//use abs in case of negative vals, cast to int
        return $this;
    }

    /**
     * @return int
     */
    public function getAge()
    {
        return $this->age;
    }
}

This class does what it says on the tin: It represents a person, and all of its associated data (a name, an age, and an email address). The setters perform some normalization or validation, and regardless of where this class is used, or where the data comes from: this class will behave perfectly predictable:
$person = new Person('Foo Bar', 'foo@bar.com', 33);
echo $person->getName(), ' is ', $person->getAge(), ' years of age', PHP_EOL,
    ' and can be reached via: ', $person->getEmail();
try {
    $person->setEmail($person->getAge());//try 33 as email address...
} catch (\InvalidArgumentException $e) {
    //Person::setEmail expects $email to be valid email address 33 is not
    echo 'Rather predictable: ', $e->getMessage();
}

I could (and probably would) change this class a tad, making the constructor optional, or allow the user of the class to set the properties through an associative array, or an object, by simply adding this method:
/**
 * @param array|\stdClass|\Traversable $mixed
 * @return $this
 * @throws \InvalidArgumentException
public function setBuld($mixed)
{
    if (!is_array($mixed) && !$mixed instanceof \stdClass && !$mixed instanceof \Traversable)
    {
        throw new \InvalidArgumentException(
            sprintf(
                '%s needs either an array, or an instance of \\stdClass or \\Traversable to be passed, instead saw %s',
                __METHOD__,
                is_object($mixed) ? get_class($mixed) : gettype($mixed)
            );
        );
    }
    foreach ($mixed as $name => $value)
    {//create setter from $name
        $setter = 'set' . ucfirst(strtolower($name));
        if ($setter !== 'setBulk' && method_exists($this, $setter))
        {//ensure no recursive calls here, and make sure the setter exists:
            $this->{$setter}($value);//set value
        }
    }
    return $this;
}

Using this approach, you change your class and interface to be a bit more forgiving, and (more importantly) make it so that your class no longer relies on super globals like $_GET:
namespace MainNS\Single\Post;
interface ReferrerInterface
{
    /**
     * @param array $values
     * @return $this
     */
    public function init(array $values);
    /**
     * @return bool
     */
    public function isAuthorReferrer();
    /**
     * @return bool
     */
    public function isDateReferrer();
    /**
     * @param mixed $authorReferrer
     * @return $this
     */
    public function setAuthorReferrer( $authorReferrer );
    /**
     * @param string|\DateTime $authorReferrer
     * @return $this
     */
    public function setDateReferrer( $dateReferrer );
}

Here, I simply reworked your interface a bit, to ensure the public function init allows the user to pass an array of values to the instance upon which the method is called. So in your class, I'd rewrite a thing or two, too:
namespace MainNS\Single\Post;
class Referrer implements ReferrerInterface
{
    /**
     * @var mixed (string?)
     */
    protected $authorReferrer = null;
    /**
     * @var \DateTime
     */
    protected $dateReferrer = null;
    /**
     * @param array $values = null;
     */
     public function __construct(array $values = null)
     {
         if ($values)
             $this->init($values);
     }

    /**
     * {@inheritdoc}
     */
    public function init(array $values)
    {//or implement the setBulk I've shown earlier
        if (isset($values['dateReferrer']))
            $this->setDateReferrer($values['dateReferrer']);
        if (isset($values['authorReferrer']))
            $this->setAuthorReferrer($values['authorReferrer']);
        return $this;
    }

    /**
     * @param mixed $authorDate
     * @return $this
     */
     public function setDateReferrer($dr)
     {
         if (!$dr instanceof \DateTime)
         {
              $dr = new \DateTime($dr);
         }
         $this->dateReferrer = $dr;
         return $this;
     }
     //implement rest of the interface here...
}

The way you implement the interface is, of course, down to you entirely. However, keep in mind that PHP is quite forgiving in terms of breaches of contract (more so than other languages). That doesn't mean that breaking the inherited or implemented contracts is anything else than wrong (I'd be happy to go into this in a bit more detail if you want me to, just let me know in a comment).
It's therefore important to understand, and religiously follow the liskov principles at all times.
Anyway, I've been going on for some time now, and I guess I'll leave it at this for now. Hope this helps you on your way


Sorting an List of tiles based on size and row limitation We are using the below code to optimally display our menu items (metro style).  We are looping through an array of menu items and automatically shifting them based on size.

Our tiles could be a size of 1, 2, 3.  The row length is 6

Could you help me optimize the code?

private void SortTiles(ref List<MenuItems> t)
    {
        int rowSize = 0;
        var n = new List<MenuItems>();

        while (t.Count > 0)
        {
            if (6 - rowSize < t[0].Size) //Too Big
            {
                bool suitableTileFound = false;

                for (int j = 1; j < t.Count; j++) //Go find smaller tile
                {
                    if (6 - rowSize >= t[j].Size) //Use this one
                    {
                        suitableTileFound = true;
                        n.Add(t[j]);
                        rowSize += t[j].Size;
                        t.RemoveAt(j);
                        break;
                    }
                }
                if (!suitableTileFound)
                    rowSize = 0;
            }
            else
            {
                //Add normally
                n.Add(t[0]);
                rowSize += t[0].Size;
                t.RemoveAt(0);
            }

        }
        t = n;
    }

MenuItem class

public class MenuItems
{
    public int MenuId { get; set; }

    public string MenuTitle { get; set; }

    public string MenuUrl { get; set; }

    public int SuiteId { get; set; }

    public string Target { get; set; }

    public int? Count { get; set; }

    public List<MenuItems> SubMenuItems { get; set; }

    public int Size { get; set; }
}

Magic numbers
You use 6 - rowSize and Mr.Maintainer has no idea what the 6 is standing for. Extract it to a meaningful named constant.

Naming
MenuItems seems to be plural of MenuItem, so what is a List<MenuItems> ?
One letter parameter names for counter/index fields are ok, but for a List<T> you should use something else neither t nor n.

Brackets {}
You should always use brackets for if statements at least if you use a new line for the action to take.

Refactoring
Let us name the parameter and local field in a better way and like already told, we should refactor the magic number to a const. As of the missing context this needs to be renamed by you later

private void SortTiles(ref List<MenuItems> menuItems)
{
    const int magicNumber6 = 6;
    int rowSize = 0;
    var sortedMenuItems = new List<MenuItems>();
}

Instead of using a while() loop, we should use a for loop, so we can skip the two calls to menuItems.RemoveAt() and just call menuItems.Clear() after the loop.

Next we should refactor the magicNumber6 - rowSize outside of the inner loop, as this needs to be calculated only once for the inner loop. Next a small adjustment of the inner loop startindex to int j = i + 1  and we have this

private void SortTiles(ref List<MenuItems> menuItems)
{
    const int magicNumber6 = 6;
    int rowSize = 0;
    var sortedMenuItems = new List<MenuItems>();

    while (menuItems.Count > 0)
    {
        int minimumSize = magicNumber6 - rowSize;
        MenuItems menuItem = menuItems[0];
        if (minimumSize < menuItem.Size)
        {
            bool suitableTileFound = false;

            for (int j = 1; j < menuItems.Count; j++)
            {
                menuItem = menuItems[j];
                if (minimumSize >= menuItem.Size)
                {
                    suitableTileFound = true;
                    sortedMenuItems.Add(menuItem);
                    rowSize += menuItem.Size;
                    menuItems.RemoveAt(j);
                    break;
                }
            }
            if (!suitableTileFound)
            {
                rowSize = 0;
            }
        }
        else
        {
            sortedMenuItems.Add(menuItem);
            rowSize += menuItem.Size;
            menuItems.RemoveAt(0);
        }

    }
    menuItems = sortedMenuItems;
}

but wait, we can still do better. We can use the List<T>.Find() method to remove the inner loop at all and while we do this we can also get rid of some of the fields we had used and as change the const to the correct name maxRowSize

private void SortTiles(ref List<MenuItems> menuItems)
{
    const int maxRowSize = 6;
    int rowSize = 0;
    var sortedMenuItems = new List<MenuItems>();

    while (menuItems.Count > 0)
    {
        int minimumSize = maxRowSize - rowSize;
        MenuItems menuItem = menuItems[0];
        if (minimumSize < menuItem.Size)
        {
            menuItem = menuItems.Find(item => minimumSize >= item.Size);
            if (menuItem != null)
            {
                sortedMenuItems.Add(menuItem);
                rowSize += menuItem.Size;
                menuItems.Remove(menuItem);
            }
            else
            {
                rowSize = 0;
            }
        }
        else
        {
            sortedMenuItems.Add(menuItem);
            rowSize += menuItem.Size;
            menuItems.RemoveAt(0);
        }
    }
    menuItems = sortedMenuItems;
}

but what if we need to adjust the maxRowSize...? Let us pass it as a parameter then. Because we don't want now to change the code using this method, we add an overloaded method also.

private void SortTiles(ref List<MenuItems> menuItems)
{
    const int maxRowSize = 6;

    SortTiles(ref menuItems, maxRowSize);
}

private void SortTiles(ref List<MenuItems> menuItems,int maxRowSize)
{
    int rowSize = 0;
    var sortedMenuItems = new List<MenuItems>();

    while (menuItems.Count > 0)
    {
        int minimumSize = maxRowSize - rowSize;
        MenuItems menuItem = menuItems[0];
        if (minimumSize < menuItem.Size)
        {
            menuItem = menuItems.Find(item => minimumSize >= item.Size);

            if (menuItem != null)
            {
                sortedMenuItems.Add(menuItem);
                rowSize += menuItem.Size;
                menuItems.Remove(menuItem);
            }
            else
            {
                rowSize = 0;
            }
        }
        else
        {
            sortedMenuItems.Add(menuItem);
            rowSize += menuItem.Size;
            menuItems.RemoveAt(0);
        }

    }
    menuItems = sortedMenuItems;
}


Rewrite BehaviorSuject that takes either localStorage or Observable in Angular I have this code
employee = this.identityService.me;
employeeToken: Observable<string> = this.employee.pipe(map((e) => e.token));

currentHead = new BehaviorSubject(localStorage.getItem('head'));

What I need to do is to change the currentHead if the local storage is empty. So I do this in the ngOnInit
ngOnInit() {
    this.employeeToken.subscribe(e => {
        if (!this.currentHead.value || this.currentHead.value === '') {
            this.currentHead.next(e);
        }
    });
}

It works, but somehow I feel like this should e feasible with rxjs in the initialization of currentHead, no?
Where I failed is, that the local storage is not an observable but this.employee is, so I struggle to compare them. I tried some convert observable to behaviorsubject but that didn't work either.

I think the ngOnInit() is the best suitable way for that. Clean and easy to spot. The initialization of the other two variables should also be part of ngOnInit (employee and employeeToken).
Something like that:
employee: Empoyee;
employeeToken: Observable<string>;
currentHead: BehaviorSubject<ADD-TYPE-HERE>;

...

ngOnInit() {
    const headFromStorage = localStorage.getItem('head');
    employee = this.identityService.me;
    employeeToken = this.employee.pipe(map((e) => e.token));
    this.currentHead = new BehaviorSubject(localStorage.getItem('head'));

    this.employeeToken.subscribe(e => this.currentHead.next(e));
}

Not sure you need this:
if (!this.currentHead.value || this.currentHead.value === '') {

Because this is a subscription. Every time the empoloyeeToken change, you have to update it, right?


Toggle code using jQuery I have here some code which is working but it seems to be a lot of coding to do here. I would really appreciate if someone could make this simpler.

click

Close

                <?php get_search_form(); ?>

Your recent searches

search 1

search 1

search 1

search 1

search 1

click

Close

favourite page

favourite page

favourite page

favourite page

favourite page

and here is my js

$(".icontap").toggle(function(){
    $("#search").animate({height:40},200).css( "overflow", "hidden" );
    $(".search-w .widget-close").css("visibility","visible");
    $(".search-w .biz-widget").css("border-color", "#335ccc");
},function(){
    $("#search").animate({height: $("#search").get(0).scrollHeight}, 200 );
    $(".search-w .widget-close").css("visibility","visible");
    $(".search-w .biz-widget").css("border-color", "#335ccc");
});

$(".closeme").click(function(){
    $("#search").animate({height:0},200);
    $(".search-w .biz-widget").css("border-color", "#fff");
    $(".search-w .widget-close").css("visibility","hidden");
});

/*******/

$(".icontap2").toggle(function(){
    $("#favourite").animate({height:40},200).css( "overflow", "hidden" );
    $(".favourite-w .widget-close").css("visibility","visible");
    $(".favourite-w .biz-widget").css("border-color", "#335ccc");
},function(){
    $("#favourite").animate({height: $("#favourite").get(0).scrollHeight}, 200 );
    $(".favourite-w .widget-close").css("visibility","visible");
    $(".favourite-w .biz-widget").css("border-color", "#335ccc");
});

$(".closeme2").click(function(){
    $("#favourite").animate({height:0},200);
    $(".favourite-w .biz-widget").css("border-color", "#fff");
    $(".favourite-w .widget-close").css("visibility","hidden");
});

Or see this in fiddle.

The way I see it there are two accordion styles.

Expand and Contract (ec) the one Section where multiple Sections can be
Expanded or Contracted.
Only one Section is Expanded at any given time. (ecp ec paired)

HTML

<button class="link" type="button" onclick ="ec('s1')">Button</button>

<button class="link" type="button" onclick ="ec('s2')">Button</button>

JS

I have two functions one for each the two EC methods.
function ec() is for EC method 1
functions ecp() is for EC method 2, pairs the current selected section with the previous.
Normally obe one of these functions is used on a web page.

The div id="s999" is for the initial value for prev so there is no null value to prev.

The reason I have var disp = div.style.display; is for when the same section is selected as previously.  I do not care if prev is expanded or contracted just contracting it is much quicker than an else if to check its display state.  But I need to remember what I am doing with the passed id.

If the same section is clicked and I toggle the display state, if it is being expanded the prev toggle would contract it again.

The line of code:

var y=div.offsetTop;window.scrollTo(0, y-64);

Takes the expanded section and puts it at the top of the Browser window. When all are contracted, it scrolls to the very top or the page.

The reason I use an array for the toggle is to eliminate the else if that would otherwise be required.

Working Snippet

  var toggle=[]
 toggle[''] = 'block';
    toggle['none'] = 'block';
    toggle['block'] = 'none';
    function ec(id){
      div = document.getElementById(id);
      div.style.display=toggle[div.style.display];
      var y=div.offsetTop;window.scrollTo(0, y-24);
    }

    var prev = document.getElementById('s999');
    function ecp(id){
      div = document.getElementById(id);
      var disp = div.style.display;
      prev.style.display='none';
      div.style.display=toggle[disp];
      prev=div;
      var y=div.offsetTop;window.scrollTo(0, y-24);
    }
body{background:#333;font-size:.8em;}
.btn1,.btn2{padding: .2em;margin:.2em 0 0 0;border-radius: 3px 3px 3px 3px;font: 700 1em Arial,Helvetica,Calibri,sans-serif;overflow: visible;border:1px solid #2985EA;color: #fff;background-color:#2985EA;
background-image: -o-linear-gradient(bottom, #2ef 0%, #02f 100%);
background-image: -moz-linear-gradient(bottom, #2ef 0%, #02f 100%);
background-image: -webkit-linear-gradient(bottom, #2ef 0%, #02f 100%);
background-image: -ms-linear-gradient(bottom, #2ef 0%, #02f 100%);
background-image: linear-gradient(to bottom, #2ef 0%, #02f 100%);}
.btn1{width:96%;}
.btn2{width:48%;}
h3{margin:0;}
p{margin:.1em .4em;}
.s1,.s2,.s3,.s4{display:none;padding:0;margin:0;}
.s1{color:#f0f;}
.s2{color:#0f0;}
.s3{color:#f00;}
.s4{color:#0ff;}
#s999{min-height:40em;}
<!DOCTYPE html>
<title>Expand Contract</title>
<button class="btn1" type="button" onclick ="ec('s1')">Section One </button>

This Section toggles when Section One button is clicked

Section 1 content
goes here.

<button class="btn1" type="button" onclick ="ec('s2')">Section Two</button>

This Section toggles when Section Two button is clicked

Section 2 content
goes here.

<button class="btn2" type="button" onclick ="ecp('s3')">Section Three </button>
<button class="btn2" type="button" onclick ="ecp('s4')">Section Four</button>

This Section toggles when Section Three button is clicked
And if Expanded will Contract when Button Four is clicked

Section 3 content
goes here.

This Section toggles when Section Four button is clicked
And if Expanded will Contract when Button Three is clicked

Section 4 content
goes here.

&#x2002;


Ruby script that deletes servers from Red Hat Satellite This script uses the Red Hat Satellite v5.4 API to delete servers which are read in from a file. The servers are listed in the file by hostname but the Satellite API deletes servers by ID number. The script runs the getId call using the hostname and stores the ID ( val[0]["id"] ) in the system_ids array (assuming it is not empty). It then deletes the servers in bulk.

The script assumes the server name is in the host.sub-domain.domain format. One improvement that I can make is to check each hostname and kick it out if the format is not correct. This would make the need to test for an empty system_ids array prior to attempting to delete the servers.

I could also add some error handling, but that is low on my list of priorities.

It works as designed. I'm just hoping to get some tips on streamlining or improving the code.

#!/usr/bin/ruby

require "xmlrpc/client"
require "io/console"

print "Username: "
user = STDIN.gets.chomp
print "Password: "
pass = STDIN.noecho(&:gets).chomp
puts

# These are the parameters we use to create our XMLRPC client
params = {
  host:  "tcecapm1r6cm01.ecap.cciio",
  path:  "/rpc/api",
  use_ssl:  "false",
  user:  user,
  pass:  pass
}

# Get the file we are reading from the command line and
# initialize the array that is used for storing system IDs
server_list = ARGV[0]
system_ids = Array.new

# Create our client using the parameters above
client = XMLRPC::Client.new(params[:host],params[:path])

# Create our Satellite API session
session = client.call("auth.login",params[:user],params[:pass])

# Open the file containing the servers to be deleted and build
# an array of their system IDs for further processing
File.open(server_list, "r") do |file|
  file.each_line do |line|
    val = client.call("system.getId", session, line)
    if val.nil?
      next
    else
      system_ids.push(val[0]["id"]) unless val.empty?
    end
  end
end

client.call("system.deleteSystems", session, system_ids)

The params hash seems like a verbose way to write four variables.  You never use params as a whole.  Furthermore, params[:use_ssl] is completely unused.

Using XMLRPC::Client.new_from_uri(…) lets you specify the host, path, and protocol in just one variable.

Instead of opening ARGV[0] yourself, just use ARGF — it's even better, in that it can also read its input from STDIN.  Instead of using line as the block parameter name, use something descriptive like system_name.

system_ids = Array.new followed by many system_ids.push(…) calls would be better written using map, in my opinion.

def prompt_auth():
  …
end

user, pass = prompt_auth()
uri = 'http://tcecapm1r6cm01.ecap.cciio/rpc/api'

client = XMLRPC::Client.new_from_uri(uri)
session = client.call('auth.login', user, pass)

system_ids = ARGF.map do |system_name|
  system = client.call('system.getId', session, system_name)
  system[0]['id'] unless system.nil? || system.empty?
end.compact
client.call('system.deleteSystems', session, system_ids)


Binary tree in Python exercise I'm practicing Python. Being completely new to the language I'd like to hear someone else thoughts on the way I use the language. I'm coming from (mainly) Java and C so I'd expect new ways of doing stuff offered by Python which I am surely not aware of because I'm still learning.

I came up with a very simple implementation for a binary tree. The idea is to practice with classes in Python. The tree supports just inserting elements and traversing pre-order, in-order and post-order. I'm not concerned about the functionality but more about comments.

from __future__ import print_function
import math
import os

class BinaryTree:
    __left = None
    __right = None
    __value = None

    def __init__(self):
        return

    def addValue(self, value):
        if self.__value is None:
            self.__value = value

        elif value > self.__value:
            if self.__right is None:
                self.__right = BinaryTree()
            self.__right.addValue(value)

        elif value < self.__value:
            if self.__left is None:
                self.__left = BinaryTree()
            self.__left.addValue(value)

        return

    def preOrderTraverse(self):
        """
        Performs a pre-order traversal of the tree.
        Returns the list of elements
        """
        elements = []
        if self.__value is not None:
            # Visit the root node first
            elements.append(self.__value)
            # Then visit the left node if present
            if self.__left is not None:
                elements += self.__left.preOrderTraverse()
            # Then visit the right node if present
            if self.__right is not None:
                elements += self.__right.preOrderTraverse()

        return elements

    def inOrderTraverse(self):
        """
        Performs a in-order traversal of the tree.
        Returns the list of elements
        """
        elements = []
        if self.__value is not None:
            # Visit the left node first if present
            if self.__left is not None:
                elements += self.__left.inOrderTraverse()
            # Then visit the root node
            elements.append(self.__value)
            # Then visit the right node if present
            if self.__right is not None:
                elements += self.__right.inOrderTraverse()

        return elements

    def postOrderTraverse(self):
        """
        Performs a post-order traversal of the tree.
        Returns the list of elements
        """
        elements = []
        if self.__value is not None:
            # Visit the left node first if present
            if self.__left is not None:
                elements += self.__left.postOrderTraverse()
            # Then visit the right node if present
            if self.__right is not None:
                elements += self.__right.postOrderTraverse()
            # Then visit the root node
            elements.append(self.__value)

        return elements

if __name__ == "__main__":
    binarytree = BinaryTree()
    binarytree.addValue(23)
    binarytree.addValue(15)
    binarytree.addValue(19)
    binarytree.addValue(8)
    binarytree.addValue(7)
    binarytree.addValue(42)
    binarytree.addValue(0)
    binarytree.addValue(1)
    elements = binarytree.preOrderTraverse()

    print("Pre order traversal: ", end="")
    for val in elements:
        print(val, end=" ")

    print("")

    elements = binarytree.inOrderTraverse()

    print("In order traversal: ", end="")
    for val in elements:
        print(val, end=" ")

    print("")

    elements = binarytree.postOrderTraverse()

    print("Post order traversal: ", end="")
    for val in elements:
        print(val, end=" ")

    print("")

PEP 8 says

You have minor issues with the code layout:

PEP 8 requires two empty lines before the class definition,
Modules os and math are not used,
In addValue the return statement is unnecessary,
PEP 8 proposes the method names in this_format; rename addValue to add_value.

Improving the code

I suggest you try to take use of a visitor pattern: in your traversal methods add new argument taking a function that is called with a current binary tree node. I had this in mind:

from __future__ import print_function

class BinaryTree:
    def __init__(self):
        self.__left = None
        self.__right = None
        self.__value = None

    def add_value(self, value):
        if self.__value is None:
            self.__value = value

        elif value > self.__value:
            if self.__right is None:
                self.__right = BinaryTree()
            self.__right.add_value(value)

        elif value < self.__value:
            if self.__left is None:
                self.__left = BinaryTree()
            self.__left.add_value(value)

    def pre_order_traverse(self, visitor_func=None):
        """
        Performs a pre-order traversal of the tree.
        Returns the list of elements
        """
        if self.__value is not None:
            # Visit the root node first
            if visitor_func:
                visitor_func(self.__value)
            # Then visit the left node if present
            if self.__left is not None:
                self.__left.pre_order_traverse(visitor_func)
            # Then visit the right node if present
            if self.__right is not None:
                self.__right.pre_order_traverse(visitor_func)

    def in_order_traverse(self, visitor_func=None):
        """
        Performs a in-order traversal of the tree.
        Returns the list of elements
        """
        if self.__value is not None:
            # Visit the left node first if present
            if self.__left is not None:
                self.__left.in_order_traverse(visitor_func)
            # Then visit the root node
            if visitor_func:
                visitor_func(self.__value)
            # Then visit the right node if present
            if self.__right is not None:
                self.__right.in_order_traverse(visitor_func)

    def post_order_traverse(self, visitor_func=None):
        """
        Performs a post-order traversal of the tree.
        Returns the list of elements
        """
        if self.__value is not None:
            # Visit the left node first if present
            if self.__left is not None:
                self.__left.post_order_traverse(visitor_func)
            # Then visit the right node if present
            if self.__right is not None:
                self.__right.post_order_traverse(visitor_func)
            # Then visit the root node
            if visitor_func:
                visitor_func(self.__value)

if __name__ == "__main__":
    binarytree = BinaryTree()
    binarytree.add_value(23)
    binarytree.add_value(15)
    binarytree.add_value(19)
    binarytree.add_value(8)
    binarytree.add_value(7)
    binarytree.add_value(42)
    binarytree.add_value(0)
    binarytree.add_value(1)

    def visitor1(value):
        print(value)

    class ElementVisitor:
        def __init__(self):
            self.elements = []

        def add(self, value):
            self.elements.append(value)

    ev = ElementVisitor()
    binarytree.pre_order_traverse(ev.add)
    print(ev.elements)

    print("")

    binarytree.in_order_traverse(visitor1)

    print("")

    binarytree.post_order_traverse(visitor1)

As you can see, above you can both print the nodes and collect them in a list; both by simply passing an appropriate function/object method.

Hope that helps.


Counting unique visitors in access log I'm aware I am probably reinventing the wheel somewhat here, but I am trying to teach myself simple bash coding by completing simple tasks such as parsing files.

To that end I am looking to learn what elephants in the room I could be missing or if there are better ways to use the core functionality of bash without installing any additional tools.

This simple code returns a list of unique IP addresses that have hit the index of my site along with a count of the hits.

a="access.log"; for b in $(cat $a | awk '{print $1}' | sort | uniq);do echo $b;grep $a -e "GET / HTTP" | grep -c $b;done;

Assumptions:
access.log is in the current directory and is in the regular format

Any advice or suggestions for improvement are greatly appreciated

Well, your code is hardly a bash solution, is it? You use sort, awk, grep, and echo....

Additionally, your code is dumped on a single line, and it makes it hard to read. Why not put it in a script, and have separate commands on separate lines.... like:

#!/bin/bash

a="access.log"
for b in $(cat $a | awk '{print $1}' | sort | uniq); do
  echo $b;
  grep $a -e "GET / HTTP" | grep -c $b;
done;

Those variable names.... ouch. a and b make it hard to separate from the -c and -e too.... and they mean nothing otherwise. Why not use meaningful names like ip and log?

Then, when I ran the code, I got a lot of funny results.... like:

54.69.125.145
1
61.240.144.65
0
64.14.99.254
0
66.196.235.78
0
66.249.64.188
0
74.208.152.232
0

Why are there 0 counts.... oh, that's because those are IP's that are not accessing the home page, but are accessing other pages... they appear as $b but don't actually "GET" /.

I would consider making it more a study of bash and use the native bash structures to get things right.... no grep, awk, etc.

#!/bin/bash

# use first commandline argument if supplied
log="access.log"
if [ $1 ] ; then
    log="$1"
fi

# set a variable to match in a regular expression
match="GET / HTTP"

# create a named array.
declare -A counts

# read the file line-by-line
while IFS='' read -r line || [[ -n "$line" ]]; do

  # find lines that access GET / HTTP
  if [[ $line =~ $match ]] ; then

    # get just the IP of the client
    ip=${line%% *}
    # get the previous count, default to 0
    prev=${counts[$ip]:-0}
    # increment the count for this IP
    counts[$ip]=$(($prev + 1))
  fi
done < "$log"

for ip in "${!counts[@]}" ; do
    echo "IP $ip visited ${counts[$ip]} times"
done

EDIT: About the ${line%% *} variable substitution. The possibilities when doing variables in bash are remarkably powerful. I recommend looking at the document Parameter Substitution for details, and the man page for bash is good as well (but does not have the examples). The %% token indicates that there should be a pattern search backward from the end of $line for a space  followed by any characters (the * - this is a "glob" expression, not a regex). This pattern essentially looks for the first space, and removes it and any charaters after it. The man page document says:

${parameter%word}
${parameter%%word}

    The word is expanded to produce a pattern just as in filename expansion.
    If the pattern matches a trailing portion of the expanded value of
    parameter, then the result of the expansion is the value of parameter
    with the shortest matching pattern (the ‘%’ case) or the longest matching
    pattern (the ‘%%’ case) deleted. If parameter is ‘@’ or ‘*’, the pattern
    removal operation is applied to each positional parameter in turn, and the
    expansion is the resultant list. If parameter is an array variable
    subscripted with ‘@’ or ‘*’, the pattern removal operation is applied to
    each member of the array in turn, and the expansion is the resultant list.


Cancelling ongoing http requests when next one is started - web app search feature This piece of code is aimed to cancel any ongoing search requests sent from the web application if next request is started, e.g. after user updates the search term.

I tested it in the final application and everything looks fine. It has already been deployed to the remote environment in our organization.

It appears to work fine, but are there any pitfalls in the code?

const ongoingRequests = [];
const fetchData = (text, activePage, itemsPerPage) => async dispatch => {
dispatch({ type: FETCH_ENTITIES });
const page = activePage - 1;

let response;

// cancel any ongoing search requests
if (ongoingRequests.length > 0) {
    ongoingRequests.map(x => x.cancel('next request started'));
    ongoingRequests.length = 0;
}

// start a new request
response = await (async () => {
    const cts = axios.CancelToken.source();
    ongoingRequests.push(cts);

    let path =
    text !== ''
        ? `/Search?PageNumber=${page}&PageSize=${itemsPerPage}&SearchTerm=${text}`
        : `?QueryLead=true&PageNumber=${page}&PageSize=${itemsPerPage}`;

    return axios
    .get(`${env_urls.api.entity}Entity${path}`, {
        cancelToken: cts.token,
    })
    .then(response => {
        return response;
    })
    .catch(error => {
        if (
        typeof cts !== 'undefined' &&
        typeof cts.token !== 'undefined' &&
        typeof cts.token.reason !== 'undefined' &&
        cts.token.reason.message === 'next request started'
        ) {
            // request cancelled, everything ok
        } else {
        throw error;
        }
    });
})();

// response is undefined if the request has been cancelled
if (typeof response !== 'undefined') {
    const data = response.data;

    dispatch({ type: FETCH_ENTITIES_SUCCESS, data });
}
};

You can rewritte this piece of code:

.catch(error => {
    if (
    cts !== 'undefined' &&
    cts.token !== 'undefined' &&
    cts.token.reason !== 'undefined' &&
    cts.token.reason.message === 'next request started'
    ) {
    } else {
    throw error;
    }
}

To:

.catch(error => {
    if (
        typeof cts === 'undefined' ||
        typeof cts.token === 'undefined' ||
        typeof cts.token.reason === 'undefined' ||
        cts.token.reason.message !== 'next request started'
    ) {
        throw error;
    }
}

And you can go further into the simplification using this trick.


Custom std::shared_ptr implementation I'd like to get a few pointers about my code.
I know that shared_ptr is up and running, and I'm reinventing the wheel but you can't say no to the school assignment.

template <typename T> class SharedPointer;

template <typename T>
void swap(SharedPointer<T> &lhs, SharedPointer<T> &rhs)
{
    lhs.swap(rhs);
}

template <typename T>
class SharedPointer {
    friend void swap<T>(SharedPointer &lhs, SharedPointer &rhs);

public:
    SharedPointer(): use_c(new std::size_t(0)), p(nullptr), deleter(Deleter()) { }
    SharedPointer(T *ptr): p(ptr), use_c(new std::size_t(1)), deleter(Deleter()) { }
    SharedPointer(T *ptr, const std::function<void(*)> &del):
            p(ptr), use_c(new std::size_t(1)), deleter(del) { }
    SharedPointer(const SharedPointer &rhs):
            use_c(rhs.use_c), p(rhs.p), deleter(rhs.deleter) { ++*use_c; }

    SharedPointer(SharedPointer &&) noexcept;
    SharedPointer& operator=(const SharedPointer &);
    SharedPointer& operator=(SharedPointer &&) noexcept;
    ~SharedPointer() { free(); }

    std::size_t use_count() { return *use_c; }

    bool unique() const { return *use_c == 1; }

    operator bool() const { return p != nullptr; }

    void reset() noexcept { free(); use_c = new std::size_t(0); };
    void reset(T*);
    void reset(T*, const std::function<void(T*)>&);

    void swap(SharedPointer<T>&);

    T* get() const { return p; }

    T& operator*() const { return *p; }
    T* operator->() const { return &*p; }

private:
    std::size_t *use_c;
    T *p;
    std::function<void(T*)> deleter;

    void free();
};

template <typename T>
SharedPointer<T>::SharedPointer(SharedPointer &&rhs) noexcept:
        use_c(rhs.use_c), p(rhs.p), deleter(std::move(rhs.deleter))
{
    rhs.use_c = new std::size_t(0);
    rhs.p = nullptr;
}

template <typename T>
SharedPointer<T> &SharedPointer<T>::operator=(const SharedPointer &rhs)
{
    ++*rhs.use_c;
    free();
    this->use_c = rhs.use_c;
    this->p = rhs.p;
    this->deleter = rhs.deleter;
    return *this;
}

template <typename T>
SharedPointer<T> &SharedPointer<T>::operator=(SharedPointer &&rhs) noexcept
{
    swap(rhs);
    rhs.free();
    rhs.use_c = new std::size_t(0);
    return *this;
}

template <typename T>
void SharedPointer<T>::reset(T *rhs)
{
    if (p != rhs) {
        free();
        p = rhs;
        use_c = new std::size_t(1);
    }
}

template <typename T>
void SharedPointer<T>::reset(T *rhs, const std::function<void(T *)> &del)
{
    if (p != rhs) {
        reset(rhs);
        deleter = del;
    }
}

template <typename T>
void SharedPointer<T>::swap(SharedPointer<T> &rhs)
{
    using std::swap;
    swap(this->use_c, rhs.use_c);
    swap(this->p, rhs.p);
    swap(this->deleter, rhs.deleter);
}

template <typename T>
void SharedPointer<T>::free()
{
    if (p && --*use_c == 0) {
        deleter(p);
        delete use_c;
    } else if (!p) {
        delete use_c;
    }
    use_c = nullptr;
    p = nullptr;
}

I've yet to add here some non-member operators, but I guess that's trivial and it's no use to litter this post so much.
I've tried to test it here and there and it looks like it works but I'm unsure about copy-and-swap and few other things.
It uses this functor class as the default deleter, if anyone is wondering what's that about.

class Deleter {
public:
    template <typename T> void operator()(T *p) const;
};

template <typename T> void Deleter::operator()(T *p) const
{
    delete p;
}

So basically I'd like to ask your help with the copy-and-swap idiom - if this implementation is correct and if it's needed at all and the overall coding style.

Nice first post!

There are things that std::unique_ptr provides which are absent here, notably the aliasing constructor (so you can own the containing object of what's referenced) and weak pointers (so cached values don't cause unwanted object retention) but otherwise, this is pretty good.

Omissions

I had a couple of compilation errors, because it's missing #include <functional>.  Also, the deleter ought to have a specialization for array types (which need delete[] p; instead of delete p;).

Naming

On naming, I'd probably choose something like release() for what you've called free() - but that's probably just because of half a lifetime writing C.

Copy and swap

You mention the copy-and-swap idiom; this is how you'd use it for your assignment operator:

template<typename T>
SharedPointer<T>& SharedPointer<T>::operator=(SharedPointer rhs)
{
    swap(rhs);
    return *this;
}

Notice that we pass rhs by value, which means we're getting the copy constructor to do all the work for us.  Having swapped, rhs has our old value, which is then destructed at the end of the function, so we don't have to duplicate the work of the constructor.

Move-assignment

Move assignment doesn't need to reset rhs - just perform the swap, and expect the moved-from value to be destructed soon:

template <typename T>
SharedPointer<T> &SharedPointer<T>::operator=(SharedPointer &&rhs) noexcept
{
    swap(rhs);
    return *this;
}

Swap

The swap function doesn't need to be a friend, as it uses only the public interface of the shared pointer.

Use default arguments to simplify

You can reduce the number of constructors, like this:

SharedPointer(T *ptr = nullptr,
              const std::function<void(*)> &del = Deleter())
    : p(ptr),
      use_c(new std::size_t(ptr!=nullptr)),
      deleter(del)
{ }

This is also true for the reset() method.

Reset should change deleter

The condition in reset() skips changing the deleter if there's no change to the pointer.  But we might be calling reset() for exactly that purpose.  Also, if a null pointer is passed as the new value, it's treated the same as a non-null pointer.  Both of these problems can be avoided by using swap() and default arguments (to re-use our constructor and destructor):

    // declaration
    void reset(T* = nullptr, const std::function<void(T*)>& = Deleter());

template <typename T>
void SharedPointer<T>::reset(T *rhs, const std::function<void(T*)>& del)
{
    swap(SharedPointer(rhs, del));
}


Program Arcade Games: Ch 16 Worksheet 2D-Array Algorithm The check_celebrity() function below takes an n*n grid as its parameter and prints the celebrity if there is one.
A celebrity is a person who is known by everybody but doesn't know anybody except himself.
If grid [ i ] [ j ] equals 1,  "i" knows "j".
If grid [ i ] [ j ] equals 0,  "i" doesn't know "j".
However, I am not satisfied with this one because there are too many loops inside loops and if checks. Are there any ways I can improve it?
def check_celebrity(grid):
    known = 0
    unknown = 0

    for i in range(len(grid)):
       ## Reset these variable for every person i
        known = 0
        unknown = 0
        for j in range(len(grid)):
            if grid[i][j] == 0:
                unknown += 1

            ## if he doesn't know anybody,
            ## check if every other "j" knows him
            if unknown == len(grid)-1:
                for j in range(len(grid)):
                    if grid[j][i] == 1:
                        known += 1

                    if known == len(grid):
                        print("#{} is the one.".format(i))
                        break

#These can be used as test cases

##  0 is a celebrity.
grid = [ [1, 0, 0, 0],
         [1, 0, 0, 0],
         [1, 0, 1, 0],
         [1, 0, 1, 0] ]

## 2 is a celebrity
grid = [ [0, 0, 1, 0, 0],
         [0, 1, 1, 0, 1],
         [0, 0, 1, 0, 0],
         [0, 0, 1, 0, 0],
         [1, 0, 1, 1, 1] ]

Welcome to Code Review, few suggestions:

Instead of calling .format() you can use f-Strings: print(f"#{i} is the one.")
Is better to return the result, instead of printing in the method and break
You can use all() to test the celebrity and make the code more compact.

Applying the suggestions:
def check_celebrity(grid):
    for i in range(len(grid)):
        # Check if i is a candidate celebrity
        if grid[i][i] == 1 and grid[i].count(1) == 1:
            # Ensure that i is a celebrity
            if all(grid[j][i] == 1 for j in range(len(grid))):
                return i

Test:
grid = [ [1, 0, 0, 0],
         [1, 0, 0, 0],
         [1, 0, 1, 0],
         [1, 0, 1, 0] ]

print(f"#{check_celebrity(grid)} is the one.")
# Outputs: #0 is a celebrity


Determine if a triangle is equilateral, isosceles, or scalene I've been fiddling around with some easy code challenges and there's one about determining if a triangle is equilateral, isosceles, or scalene.

I've come up with a working solution, but I feel this could be greatly improved and/or simplified.

The sides of a triangle come in a list e.g. [2, 2, 2] or [0, 0, 0].

Here's what I've got.

def is_triangle(sides):
    if min(sides) <= 0:
        return False
    if sum(sorted(sides)[:-1]) < sorted(sides)[-1]:
        return False
    return True

def equilateral(sides):
    triangle = is_triangle(sides)
    if triangle:
        x, y, z = sides
        return x == y == z
    else:
        return False

def isosceles(sides):
    triangle = is_triangle(sides)
    if triangle:
        x, y, z = sides
        return x == y or y == z or z == x
    else:
        return False

def scalene(sides):
    if equilateral(sides) or isosceles(sides):
        return False
    return is_triangle(sides)

Also, I'm adding a simple unit test module.

import unittest

from triangle import equilateral, isosceles, scalene

class TestEquilateralTriangle(unittest.TestCase):
    def test_all_sides_are_equal(self):
        self.assertIs(equilateral([2, 2, 2]), True)

    def test_all_zero_sides_is_not_a_triangle(self):
        self.assertIs(equilateral([0, 0, 0]), False)

    def test_third_triangle_inequality_violation(self):
        self.assertIs(isosceles([3, 1, 1]), False)

    def test_sides_may_be_floats(self):
        self.assertIs(equilateral([0.5, 0.5, 0.5]), True)

class TestIsoscelesTriangle(unittest.TestCase):
    def test_last_two_sides_are_equal(self):
        self.assertIs(isosceles([3, 4, 4]), True)

    def test_equilateral_triangles_are_also_isosceles(self):
        self.assertIs(isosceles([4, 4, 4]), True)

    def test_third_triangle_inequality_violation(self):
        self.assertIs(isosceles([3, 1, 1]), False)

    def test_sides_may_be_floats(self):
        self.assertIs(isosceles([0.5, 0.4, 0.5]), True)

class TestScaleneTriangle(unittest.TestCase):
    def test_no_sides_are_equal(self):
        self.assertIs(scalene([5, 4, 6]), True)

    def test_all_sides_are_equal(self):
        self.assertIs(scalene([4, 4, 4]), False)

    def test_third_triangle_inequality_violation(self):
        self.assertIs(isosceles([3, 1, 1]), False)

if __name__ == "__main__":
    unittest.main()

I'd appreciate some feedback on my code.

A triangle has, by definition, three sides. I find it therefore weird to take a single sides argument, which could be of any size. This opens you up to obscure bugs, such as these ones, which are not covered in your tests:

>>> is_triangle([1,2,3,4])
True    # ?
>>> is_triangle([1, 1])
True    # ???
>>> is_triangle([float('nan')])
True    # WTF?

Instead, just explicitly take three arguments. The sides of a triangle are customarily called a, b, c.

def is_triangle(a, b, c):
    a, b, c = sorted([a, b, c])
    return a > 0 and a + b > c

This uses the fact that after the sorted, a is always the smallest side, as mentioned in the comments.

The only thing you need to change in your calling code is to call this with is_triangle(*sides), i.e. use tuple unpacking.

Your other functions can also be shortened a bit. Try to put multiple checks in one line to return right away (but don't push it if it gets too complicated).

def equilateral(a, b, c):
    return is_triangle(a, b, c) and a == b == c

Instead of manually checking all combinations of sides for equality, just use set to get rid of multiples:

def isosceles(a, b, c):
    return is_triangle(a, b, c) and len(set([a, b, c])) <= 2

def scalene(a, b, c):
    return is_triangle(a, b, c) and len(set([a, b, c])) == 3

Note that all functions need to use is_triangle. You could define a decorator that makes sure the input is a triangle:

from functools import wraps

def ensure_triangle(func):
    @wraps(func)
    def wrapper(a, b, c):
        return is_triangle(a, b, c) and func(a, b, c)
    return wrapper

@ensure_triangle
def equilateral(a, b, c):
    return a == b == c

@ensure_triangle
def scalene(a, b, c):
    return len(set([a, b, c])) == 3

@ensure_triangle
def isosceles(a, b, c):
    return len(set([a, b, c])) <= 2


100 Locker Problem Expanded The locker problem for 100 lockers is simple:

  A school has 100 lockers and 100 students. All lockers are closed on the first day of school. As the students enter, the first student, denoted S1, opens every locker. Then the second student, S2, begins with the second locker, denoted L2, and closes every other locker. Student S3 begins with the third locker and changes every third locker (closes it if it was open, and opens it if it was closed). Student S4 begins with locker L4 and changes every fourth locker. Student S5 starts with L5 and changes every fifth locker, and so on, until student S100 changes L100.

  Which lockers will remain open?

Now my program in Python uses a trick hidden in this problem which is that all  lockers whose numbers have an odd number of divisors will remain open. In other words, lockers whose numbers are perfect squares. This allowed me to expand my program to solve the problem for any \$x\$ amount of lockers, provided by the user:

# 100 Locker Problem

counter_open = 0
counter_close = 0
open_list = []

def locker(num_lockers):
    global counter_open, counter_close
    num = 1
    while num**2 < num_lockers:
        open_list.append(num**2)
        num += 1
    counter_open = len(open_list)
    counter_close = num_lockers - len(open_list)

def run():
    global open_list, counter_close, counter_open
    open_list = []
    counter_open = 0
    counter_close = 0
    endings = ["st", "nd", "rd", "th"]
    try:
        user_input = int(raw_input("How many lockers will there be: "))
    except ValueError:
        print "Please insert an integer"
    else:
        locker(user_input)
        return_str = "The "
        for index in range(0, len(open_list)):
            if index == (len(open_list) - 1):
                if str(open_list[index])[-1:] == "1":
                    return_str += ("and " + str(open_list[index]) + endings[0])
                elif str(open_list[index])[-1:] == "2":
                    return_str += ("and " + str(open_list[index]) + endings[1])
                elif str(open_list[index])[-1:] == "3":
                    return_str += ("and " + str(open_list[index]) + endings[2])
                else:
                    return_str += ("and " + str(open_list[index]) + endings[3])
            else:
                if str(open_list[index])[-1:] == "1":
                    return_str += (str(open_list[index]) + endings[0] + ", ")
                elif str(open_list[index])[-1:] == "2":
                    return_str += (str(open_list[index]) + endings[1] + ", ")
                elif str(open_list[index])[-1:] == "3":
                    return_str += (str(open_list[index]) + endings[2] + ", ")
                else:
                    return_str += (str(open_list[index]) + endings[3] + ", ")
        return_str += " locker(s) will be open."
        print return_str

while True:
    run()
    print "Open Lockers: " + str(counter_open)
    print "Closed Locker: " + str(counter_close)
    print "----------------------------"

Yes, I do realize that I am using global in my functions and I have 8 if/elif statements for grammar but accuracy in the result (and PEP 8) is key to me (keep that in mind) . How do I improve the efficiency of the code and/or shorten the code?

Algorithm

The beginning of the mathematical analysis is very nice and well explained. Maybe it would be nice to have it in your code as a comment.

Also, please note that if what you really care about is the number of lockers open (or closed), it is just a matter of computing a square root.

Separation of concerns

The biggest issue in your code is that you have a function doing everything : handling user input, string formatting, updating globals... This makes your code hard to follow and hard to test. A better way to do it would be to have smaller units (functions or sometimes classes and modules) with a single responsability.

By doing so, you could get rid of the globals and write the code like this :

# 100 Locker Problem

def get_open_lockers(num_lockers):
    open_list = []
    num = 1
    while num**2 < num_lockers:
        open_list.append(num**2)
        num += 1
    return open_list

def format_output(nb_lockers, open_list):
    endings = ["st", "nd", "rd", "th"]
    counter_open = len(open_list)
    counter_close = nb_lockers - counter_open
    return_str = "The "
    for index in range(0, len(open_list)):
        if index == (len(open_list) - 1):
            if str(open_list[index])[-1:] == "1":
                return_str += ("and " + str(open_list[index]) + endings[0])
            elif str(open_list[index])[-1:] == "2":
                return_str += ("and " + str(open_list[index]) + endings[1])
            elif str(open_list[index])[-1:] == "3":
                return_str += ("and " + str(open_list[index]) + endings[2])
            else:
                return_str += ("and " + str(open_list[index]) + endings[3])
        else:
            if str(open_list[index])[-1:] == "1":
                return_str += (str(open_list[index]) + endings[0] + ", ")
            elif str(open_list[index])[-1:] == "2":
                return_str += (str(open_list[index]) + endings[1] + ", ")
            elif str(open_list[index])[-1:] == "3":
                return_str += (str(open_list[index]) + endings[2] + ", ")
            else:
                return_str += (str(open_list[index]) + endings[3] + ", ")
    return_str += " locker(s) will be open."
    print return_str
    print "Open Lockers: " + str(counter_open)
    print "Closed Locker: " + str(counter_close)
    print "----------------------------"

while True:
    try:
        nb_lockers = int(raw_input("How many lockers will there be: "))
    except ValueError:
        print "Please insert an integer"
    else:
        open_list = get_open_lockers(nb_lockers)
        format_output(nb_lockers, open_list)

Look like a native

Almost everytime you do range(len(list)), you are not doing things the right way. I highly recommend Ned Batchelder's excellent talk called "Loop like a native".

In your case, the code becomes (l stands for locker which is probably not the best name):

def format_output(nb_lockers, open_list):
    endings = ["st", "nd", "rd", "th"]
    counter_open = len(open_list)
    counter_close = nb_lockers - counter_open
    return_str = "The "
    for i, l in enumerate(open_list):
        if i == (len(open_list) - 1):
            if str(l)[-1:] == "1":
                return_str += ("and " + str(l) + endings[0])
            elif str(l)[-1:] == "2":
                return_str += ("and " + str(l) + endings[1])
            elif str(l)[-1:] == "3":
                return_str += ("and " + str(l) + endings[2])
            else:
                return_str += ("and " + str(l) + endings[3])
        else:
            if str(l)[-1:] == "1":
                return_str += (str(l) + endings[0] + ", ")
            elif str(l)[-1:] == "2":
                return_str += (str(l) + endings[1] + ", ")
            elif str(l)[-1:] == "3":
                return_str += (str(l) + endings[2] + ", ")
            else:
                return_str += (str(open_list[i]) + endings[3] + ", ")
    return_str += " locker(s) will be open."
    print return_str
    print "Open Lockers: " + str(counter_open)
    print "Closed Locker: " + str(counter_close)
    print "----------------------------"

Extra Operations (functions calls and slicing)

You keep converting l to str. Maybe you could it once and for all.

Similarly, you keep accessing string[-1:] : it would be nice to do it only once. By the way, the usual way to get the last elements of the list is just [-1] (no need for colon).

def format_output(nb_lockers, open_list):
    endings = ["st", "nd", "rd", "th"]
    counter_open = len(open_list)
    counter_close = nb_lockers - counter_open
    return_str = "The "
    for i, l in enumerate(open_list):
        s = str(l)
        last_dig = s[-1]
        if i == (len(open_list) - 1):
            if last_dig == "1":
                return_str += ("and " + s + endings[0])
            elif last_dig == "2":
                return_str += ("and " + s + endings[1])
            elif last_dig == "3":
                return_str += ("and " + s + endings[2])
            else:
                return_str += ("and " + s + endings[3])
        else:
            if last_dig == "1":
                return_str += (s + endings[0] + ", ")
            elif last_dig == "2":
                return_str += (s + endings[1] + ", ")
            elif last_dig == "3":
                return_str += (s + endings[2] + ", ")
            else:
                return_str += (s + endings[3] + ", ")
    return_str += " locker(s) will be open."
    print return_str
    print "Open Lockers: " + str(counter_open)
    print "Closed Locker: " + str(counter_close)
    print "----------------------------"

Don't repeat yourself

Your have similar logic in the then and else blocks in format_output. This could be factorised out. Also, it would be a good idea to take this chance to extract it out in a function on its own.

def int_to_ordinal(n):
    endings = ["st", "nd", "rd", "th"]
    s = str(n)
    last_dig = s[-1]
    if last_dig == "1":
        return s + endings[0]
    elif last_dig == "2":
        return s + endings[1]
    elif last_dig == "3":
        return s + endings[2]
    else:
        return s + endings[3]

def format_output(nb_lockers, open_list):
    counter_open = len(open_list)
    counter_close = nb_lockers - counter_open
    return_str = "The "
    for i, l in enumerate(open_list):
        if i == (len(open_list) - 1):
            return_str += ("and " + int_to_ordinal(l))
        else:
            return_str += (int_to_ordinal(l) + ", ")
    return_str += " locker(s) will be open."
    print return_str
    print "Open Lockers: " + str(counter_open)
    print "Closed Locker: " + str(counter_close)
    print "----------------------------"

Tiny bug

I've just discovered one bug as I was trying to go further. I imagine it was in the original code but I might be wrong. It happens when there is only one lock open (the first), the formatting gives : "The and 1st locker(s) will be open." which looks a bit weird.

PEP8 and join

There is a style guide for Python code called PEP 8 : it is worth reading (and worth following as much as possible). Your code seems to follow the PEP 8 style for most parts but there is a part you'd be interested in:

  For example, do not rely on CPython's efficient implementation of in-place string concatenation for statements in the form a += b or a = a + b . This optimization is fragile even in CPython (it only works for some types) and isn't present at all in implementations that don't use refcounting. In performance sensitive parts of the library, the ''.join() form should be used instead. This will ensure that concatenation occurs in linear time across various implementations.

Even though I can safely assume that your code performance is not that important, the join builtin, on top of making your code faster, can make it clearer. You could write something like:

def format_output(nb_lockers, open_list):
    counter_open = len(open_list)
    counter_close = nb_lockers - counter_open
    print "The " + ", ".join(int_to_ordinal(l) for l in open_list) + " locker(s) will be open."
    print "Open Lockers: " + str(counter_open)
    print "Closed Locker: " + str(counter_close)
    print "----------------------------"

(I removed the part with the "And" because it seemed buggy anyway).

If main

In Python, it is recommended to put the part of your code that actually does something (instead of merely defining something) behind an if __name__ == "__main__": so that you can easily run your code as a main program but also import it to be able to reuse the functions/classes you've defined.

if __name__ == "__main__":
    while True:
        try:
            nb_lockers = int(raw_input("How many lockers will there be: "))
        except ValueError:
            print "Please insert an integer"
        else:
            open_list = get_open_lockers(nb_lockers)
            format_output(nb_lockers, open_list)

Optimisation in get_open_lockers

In get_open_locks, you can easily compute the number of iterations you'll need. Also, you could use a list comprehension to avoid calling append many times.

import math

def get_open_lockers(num_lockers):
    if num_lockers <= 0:
        return []
    sqrt = int(math.sqrt(num_lockers-1))
    return [i*i for i in range(1, sqrt+1)]

(It is easy to write tests to ensure that the function returns the same resultats before and after)

Final version of the code

My final code looks like (it can still probably be improved):

import math

def get_open_lockers(num_lockers):
    if num_lockers <= 0:
        return []
    sqrt = int(math.sqrt(num_lockers-1))
    return [i*i for i in range(1, sqrt+1)]

def int_to_ordinal(n):
    endings = ["st", "nd", "rd", "th"]
    s = str(n)
    last_dig = s[-1]
    if last_dig == "1":
        return s + endings[0]
    elif last_dig == "2":
        return s + endings[1]
    elif last_dig == "3":
        return s + endings[2]
    else:
        return s + endings[3]

def format_output(nb_lockers, open_list):
    counter_open = len(open_list)
    counter_close = nb_lockers - counter_open
    print "The " + ", ".join(int_to_ordinal(l) for l in open_list) + " locker(s) will be open."
    print "Open Lockers: " + str(counter_open)
    print "Closed Locker: " + str(counter_close)
    print "----------------------------"

if __name__ == "__main__":
    while True:
        try:
            nb_lockers = int(raw_input("How many lockers will there be: "))
        except ValueError:
            print "Please insert an integer"
        else:
            open_list = get_open_lockers(nb_lockers)
            format_output(nb_lockers, open_list)


Factory pattern in F# for a web scraper I'm trying to learn F# by creating a little web scraper that will do custom scraping based on the url domain. For this, I need to create and select the correct kind of scraper. I figure I would use a Factory that would determine the correct kind of scraper for me.

So here is the interface for my scraper

type IExtractor =

    /// determines if the extractor will work for the url
    /// returns true if the extrator can handle the url
    abstract member Suitable : string -> bool

    /// name of the extractor
    abstract ExtractorName : string with get

Here is the instance of one of the scrapers

type CustomExtractor1() =

    /// <summary>
    /// tests whether the input url is valid
    /// </summary>
    /// <param name="url"></param>
    let (|ValidUrl|_|) url =
        let regexExpression = "^http(s)?://www\.customsite\.com.*\?fd="
        // regex helper that returns string option
        Common.UrlMatch regexExpression url

    interface IExtractor with
        member this.Suitable url =
            match url with
            | ValidUrl url -> true
            | _ -> false

        member this.ExtractorName with get() = "Custom extractor"

Here is the Factory

module Extractor =

    let private _extractors: IExtractor list = [new CustomExtractor1(); new CustomExtractor2()]

    let create url =
        let isSuitable (input: string) (ex:IExtractor)  = ex.Suitable input
        let one (input:string) (ex:IExtractor) = true
        match (_extractors |> List.tryFind (isSuitable url) ) with
        | Some ex -> Some ex
        | _ -> None

Here is the usage

 let input = "https://www.customsite.com?fd=cWz5d"
            let extractor = Extractor.create input

            let extractorOption =
                match Extractor.create input with
                | Some ex -> Some ex
                | _ ->
                     (
                     raise (Error ("no extractor found. Exiting"))
                     )

            let extractor = extractorOption.Value

            // do stuff with extractor
            printfn "%s" extractor.ExtractorName

The flow for this usage somehow feels more like C#, so it seems a little off. Especially in the usage where I'm using extractorOption.Value. I am currently investigating using ROP instead of exception handling for better control flow.

What do you think — is there a way to improve this?

Instead of defining the IExtractor interface, wouldn't it be simpler with a function?

In this question, I assume that ExtractorName is a stand-in for something you'd really want the extractor to do, because otherwise, as the OP is stated, the extractor doesn't really do anything. In this case, an extractor is nothing by a string, but I assume that in reality, it'd be a function.

This would enable you to make the Extractor module simpler:

module Extractor =

    let private fooExtractor url =
        if url = "foo"
        then Some "Foo Extractor" // Stand-in for actual implementation
        else None
    let private barExtractor url =
        if url = "bar"
        then Some "Bar Extractor" // Stand-in for actual implementation
        else None

    let private extractors = [fooExtractor; barExtractor]

    let create url =
        extractors
        |> List.choose (fun candidate -> candidate url)
        |> List.tryHead

Each element of extractors is a function with the type string -> string option. In order to be useful, the return value should probably be another function that you can subsequently call; something like string -> ('a -> 'b) option.

Note the use of List.choose to pick only those functions that return Some 'a. Instead of the object-oriented Try/Parse pattern, you can indicate match success or failure with an option value.

Usage can be simplified as well:

let input = "https://www.customsite.com?fd=cWz5d"
match Extractor.create input with
| Some x ->
    // do stuff with extractor
    printfn "%s" x
| None -> printfn "%s" "No extractor found; exiting."

Here, we're simply printing the name of the extractor, since x is a string, but if you imagine that x was instead a function, you'd be able to call it.


Vigenere Cipher - Haskell Implementation I'm reading 'Haskell Programming from First Principles' book written by Christopher Allen and Julie Moronuki. As part of the exercises, I'm supposed to implement encryption and decryption functions for Vigenere cipher.

This is my implementation. I'm using recursion for every letter in the plain text.

shift :: (Int -> Int -> Int) -> Char -> Char -> Char
-- shift applies a arithmetic function to given characters
-- after converting it from ASCII value to Int
-- This can be used to Encrypt or Decrypt the character
shift f i k = let i' = ord i - 65
                  k' = ord k - 65
              in  chr $ ((f i' k') `mod` 26) + 65

crypt :: (Int -> Int -> Int) -> (String, Int) -> (String, Int) -> String -> String
-- crypt applies given artithmetic function and does Vigenere encryption/decryption.
-- This function moves forward on the string, encrypts the
-- current char using corresponding char in key. It maintains
-- current position of the string and key it is working on. When
-- it encounters space, it skips the en(de)cryption and doen't move forward
-- in the key. After encrypting the current char, it recursively calls
-- itself by passing next positions in string to be encrypted along with
-- position of key.
crypt cryptFunc (words', wpos) (key, kpos) cipherText
    | length words' <= wpos =  cipherText
    | otherwise =
        let c  = words' !! wpos
            c' = case c of
                    ' ' -> ' '
                    _ -> shift cryptFunc c $  key !! kpos
            kpos' = case c of
                    ' ' -> kpos `mod` (length key)
                    _   -> (kpos + 1) `mod` (length key)
            in crypt cryptFunc (words', wpos + 1) (key, kpos') (cipherText ++ [c'])

encrypt :: [Char] -> [Char] -> [Char]
encrypt i k = crypt (+) (i, 0) (k, 0) []

decrypt :: [Char] -> [Char] -> [Char]
decrypt i k = crypt (-) (i, 0) (k, 0) []

-- input = "MEET AT DAWN"
-- key = "ALLY"
-- output = "MPPR AE OYWY"

Do you think the implementation is efficient and obvious way to do it? Please tell if there are any improvements I can do.

Do you think the implementation is efficient

No, sorry. You're heading in the right direction, but !! makes your algorithm \$\mathcal O(n^2)\$, and if it wasn't for !!, appending c' to the cipherText would also lead to \$\mathcal O(n^2)\$ due to ++. Or taking the length all the time. Well, basically any function that you use in each iteration that needs to (possibly) traverse the complete list makes your function non-efficient.

But let us start at the top. I suggest you to put your documentation before the type signature. That way, the type and the names are still close, which makes your function easier to understand if height is limited:

-- shift applies a arithmetic function to given characters
-- after converting it from ASCII value to Int
-- This can be used to Encrypt or Decrypt the character
shift :: (Int -> Int -> Int) -> Char -> Char -> Char
shift f i k = let i' = ord i - 65
                  k' = ord k - 65
              in  chr $ ((f i' k') `mod` 26) + 65

Next, you seem to use a very specific interpretation of Char as Int, namely the position in the alphabet. This is fine, but if you use - 65 and + 65 all the time, it's going to be hard to update it to a larger alphabet. A pair of additional functions and values can help:

toInt :: Char -> Int
toInt c = ord c - 65

fromInt :: Int -> Char
fromInt i = chr i + 65

alphabetSize :: Int
alphabetSize = 26

-- feel free to add your documentation here
shift :: (Int -> Int -> Int) -> Char -> Char -> Char
shift f i k = let i' = toInt i
                  k' = toInt k
              in  fromInt $ f i' k' `mod` alphabetSize

By the way, with on from Data.Function one could write

shift :: (Int -> Int -> Int) -> Char -> Char -> Char
shift f i k = let f' = f `on` toInt
              in  fromInt $ f i k `mod` alphabetSize

but that's just a remark (Exercise: try to guess on's type. How would a valid implementation look like?).

Next, we head over to crypt. Let us assume for a second that the key is longer than the text we want to encrypt. We want to encrypt each character on its own. So if we have a string, we can pattern match. We start with the easier case:

crypt' :: (Int -> Int -> Int) -> String -> String -> String
crypt' _ [] _ = []

If there's nothing to encrypt, there's nothing to return. Now, what should we do if there is at least one character? Well, we check whether it is a space. If it is a space, we add a space to our result and continue on the rest:

crypt' f (' ':ws) ks = ' ' : crypt f ws ks

A space goes in, a space goes out. We do not change the key. So now there is only one case missing: the one where we have a character that's not a space:

crypt' f (w:ws) (k:ks) = shift f w k : crypt f ws ks

A character goes in, a character from the key goes in, we use the function to shift and create the result, and then we continue on our other words and keys.

Here's crypt' at once:

crypt' :: (Int -> Int -> Int) -> String -> String -> String
crypt' _ []       _      = []
crypt' f (' ':ws) ks     = ' '         : crypt' f ws ks
crypt' f (w:ws)   (k:ks) = shift f w k : crypt' f ws ks

However, this does only work if the key is longer than the text. How do we write crypt, which may take a smaller key? We use cycle:

crypt :: (Int -> Int -> Int) -> String -> String -> String
crypt f w k = crypt' f w (cycle k)

That's not necessary by the way, you could have defined a function inside of crypt, check whether the "local" key is [] and then start over:

crypt f w k = go w k
  where
    go ...

but that's left as another exercise.

Further remarks

  Do you think the implementation is … [an] obvious way to do it?

It's an obvious way in imperative languages, which have a string as an array or similar data structure, since index-wise accessing is fast, and .push_back or other "append-single-character-at-end" functions are usually (amortized) constant.

So if you're coming from an imperative language, yes, that would be the obvious way to implement it there (well, aside from the recursiveness).

But in Haskell keep in mind that lists are, well, lists. If you want to access the 20th element, you have to skip the first 19. !! isn't a care-free operation like a vector access in several other operations. Neither is length. That's why it's usually a good idea to pattern match (or use higher-order-functions) and create the list with :.

Exercises

Adjust encrypt and decrypt to crypt's new type.
What is the type signature of cycle? Can you implement it yourself?
(*) Instead of (Int -> Int -> Int), have crypt use (Char -> Char -> Char). (Why?)
(*) Make it possible to use both upper and lower characters in your text.
(*) Keep unsupported characters
(**) Instead of (Char -> Char -> Char), have crypt use (a -> a -> a). (Is this possible? What do you have to change to make it possible? Why?)
(***) Make it possible to encrypt/decrypt arbitrary alphabets, if given "nice" toInt and fromInt functions.