Mock user table class I've just created my first tests using mock classes. So far my tests are running much faster and now I guess I'm solely focusing on a single class (in this case I'm testing UserTable and mocking the DatabaseAdapter instance I inject into it)

I haven't proceeded to do the write tests for the other methods of this class (just testing "create" method for now), I was hoping someone could comment on my tests for this method. It simply creates a new record in the user table. I'm trying to test all possibilities that I should be testing. I'm a little new to this and not quite sure if I've got it right yet.

<?php

use app\models\UserTable;
use app\models\User;

/**
*    UserTableTest
*/
class UserTableTest extends PHPUnit_Framework_TestCase
{
    protected $dbAdapter;
    protected $userTable;

    public function setUp()
    {
        // create mock database adapter
        $this->dbAdapter = $this->getMockBuilder('framework\Db\DatabaseAdapter')
            ->disableOriginalConstructor()
            ->getMock();

        // create our instance of the class to be tested
        $this->userTable = new UserTable;
    }

    // create method

    public function testCreateMethodWhenSuccessfullyInsertsRow()
    {
        // set mock class

        // set the mock object's methods for this test
        $this->dbAdapter->expects( $this->any() )
            ->method('insert')
            ->will( $this->returnValue(true) );

        $this->userTable->setDatabaseAdapter($this->dbAdapter);

        // perform test

        // assert user is created when valid argument is passed
        $result = $this->userTable->create( array() );

        $this->assertTrue($result);
    }

    public function testCreateMethodWhenFailsToInsertsRow()
    {
        // set mock class

        // set the mock object's methods for this test
        $this->dbAdapter->expects( $this->any() )
            ->method('insert')
            ->will( $this->returnValue(false) );

        $this->userTable->setDatabaseAdapter($this->dbAdapter);

        // perform test

        // assert user is created when valid argument is passed
        $result = $this->userTable->create( array() );

        $this->assertFalse($result);
    }

    /**
    * @expectedException InvalidArgumentException
    */
    public function testCreateMethodWithInvalidArguments()
    {
        // set mock class

        // set the mock object's methods for this test
        $this->dbAdapter->expects( $this->any() )
            ->method('insert')
            ->will( $this->returnValue(false) );

        $this->userTable->setDatabaseAdapter($this->dbAdapter);

        // perform test

        // pass invalid argument, should be an array of values
        $result = $this->userTable->create('invalid argument type');
    }

    /**
    * @expectedException RuntimeException
    */
    public function testCreateMethodWithMissingDatabaseAdapter()
    {
        // assert user is created
        $result = $this->userTable->create( array() );
    }
}

Except for some (mostly experience related issues) I commend your efforts. Well done!
The Short Version
Specific Points

use app\models\User; is not needed as the User class is not used (or mocked) in this test.
The comments before your asserts don't always seem to match what you are actually asserting.
I think you'd be better of using those comments as the test method name rather that having them as a comment in the function.
On the subject of names, I'd suggest using words like should and when to make the test method name state exactly what it expects the system under test to do. That way when a test fails you'll rarely have to look in the test to understand what is broken.
The first two test methods are basically testing the same thing. Whether or not the output of the create method matches that of the insertRow mock.
This could be simplified into one single test method fed by a dataProvider, more on this below.
You don't need to catch the result of the create in the methods that test for exceptions. Unused variables in code are bad and test code is still code.

Although it is impossible to tell without the class under test, you seem to have covered all logic paths:

Insert Succeeds
Insert Fails
Valid Arguments Provided (implicitly through previous methods)
Invalid Arguments Provided
DbAdapter Provided (implicitly)
DbAdapter Not Provided

You could consider making the implicit tests more explicit.
Incorporating these points your code would look a bit like this:
<?php

namespace app\models;

/**
 * Tests for the UserTable class
 *
 * @coversDefaultClass app\models\UserTable
 * @covers ::<!public>
 */
class UserTableTest extends PHPUnit_Framework_TestCase
{
    ////////////////////////////////// FIXTURES \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\
    protected $userTable;

    /**
     * create our instance of the class to be tested
     */
    final public function setUp()
    {
        $this->userTable = new UserTable;
    }

    /////////////////////////////////// TESTS \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\
    /**
     * @covers ::create
     * @covers ::setDatabaseAdapter
     *
     * @dataprovider provideBooleans
     */
    final public function testUserTableShouldOnlyCreateUserWhenInsertRowSucceeds($insertRowStatus)
    {
        $userTable = $this->userTable;

        // set mock class
        $mockDbAdapter = $this->getMockAdapter();

        // set the mock object's methods for this test
        $mockDbAdapter->expects( $this->any() )
            ->method('insert')
            ->will( $this->returnValue($insertRowStatus) );

        $userTable->setDatabaseAdapter($mockDbAdapter);

        // perform test

        // assert user is created when valid argument is passed
        $actual = $userTable->create( array() );

        $this->assertEquals($insertRowStatus, $actual);
    }

    /**
     * @covers ::create
     * @covers ::setDatabaseAdapter
     *
     * @expectedException InvalidArgumentException
     *
     * @dataprovider provideInvalidArgumentTypes
     */
    final public function testUserTableShouldOnlyCreateUserWhenGivenValidArguments($invalidArgumentType)
    {
        $userTable = $this->userTable;
        // set mock class
        $mockDbAdapter = $this->getMockAdapter();

        // set the mock object's methods for this test
        $mockDbAdapter->expects( $this->any() )
            ->method('insert')
            ->will( $this->returnValue(false) );

        $userTable->setDatabaseAdapter($mockDbAdapter);

        // perform test

        // pass invalid argument, should be an array of values
        $userTable->create($invalidArgumentType);
    }

    /**
     * @covers ::create
     *
     * @expectedException RuntimeException
     */
    final public function testUserTableShouldOnlyCreateUserWhenGivenDatabaseAdapter()
    {
        $userTable = $this->userTable;
        $userTable->create( array() );
    }

    ////////////////////////////// MOCKS AND STUBS \\\\\\\\\\\\\\\\\\\\\\\\\\\\\
    final public function getMockAdapter()
    {
        // create mock database adapter
        $mockDbAdapter = $this->getMockBuilder('framework\Db\DatabaseAdapter')
            ->disableOriginalConstructor()
            ->getMock()
        ;
    }

    /////////////////////////////// DATAPROVIDERS \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\
    final public function provideBooleans()
    {
        return array(
            'true' => array(true),
            'false' => array(false),
        );
    }

    final public function provideInvalidArgumentTypes()
    {
        return array(
            'string' => array('invalid argument type'),
            'boolean - false' => array(false),
            'boolean - true' => array(true),
            'null' => array(null),
        );
    }
}

The Long Version
Should you feel in the mood for a bit more depth and education, I have a more thorough review below.
These are all things I'd point out to my teammates, if this code had passed my desk at work.
This may be a bit much to take in, keep in mind I'm a rather critical developer and I always find room for improvement, even (or maybe especially) in my own code. Feel free to ask for clarity if any point is unclear.
Also, just ignore any points that don't resonate with you. There's no use in taking advice you don't believe in.
Points in General
Don't Start test functions with the word test
Use annotations to tell PHPUnit that this function is a @test. This allow the name of the test function to be more descriptive.

I've actually changed my mind on this one. By starting test method names with test they are both more explicit and recognizable. The same goes for starting provider methods with provides.
Don't create your mock in the setup function
Unless you need to insert the mock into the constructor of your class under test,
don't create your mock until you need it. I'd just create a getDatabaseAdapterMock
function and call that from the test functions. You could consider making specific methods for the various states the mock is in but (depending on the size and complexity of the class under test) that might be a step to far.
Add @covers annotations to your docblocks
Not only does this set you up to understand which parts of your code might still need attention, it also means you don't have to mention the name of the method you are testing in the name of function in your test class.
As of version 4, PHPUnit will tell you if "risky" code has run for a test. That is to say, code that either isn't mentioned by a @covers block or code that is mentioned but didn't run.
Another trick I tend to use is adding a @coversDefaultClass to the class-level docblock along with a @covers ::<!public> (see the @covers docs). That way you only need to specify public API in the test method doc blocks.
Don't use $this->any() when the count is important
A mock is also an assert. You want to be as specific with your assertions of (public) API as you can.
If it does not matter whether insert is called once or a hundred times than it's fine to use any.
In that case you should add a test proving that there is no difference and that no nasty side-effect occur.
If it does matter, for instance double rows might be inserted or existing rows might overwritten several times (causing your application to become slow), then I would strongly advocate using $this->exactly($count). Personally I also prefer using $this->exactly(1) over $this->once() for the sake of things looking the same regardsless of how often a method should be called.
Be specific with native PHP exceptions
Using @expectedException is a good start but, again, being specific really pays of when code breaks and tests start failing.
My advice would be to also add an @expectedExceptionMessage annotation to make sure you are getting the right exception.
If you want to keep things really clean you could put the message of Exceptions you throw yourself in class constants and just use the constant to check against like so (quoting straight from the manual here):
 /**
  * @expectedException        MyException
  * @expectedExceptionMessage MyClass::ERROR_MESSAGE
  */

Misc.
Use a namespace
Your classes live in a namespace, so should your test classes. Some people prefer to have an explicit Test namespace, others (me included) prefer to have the namespace match between the test class and class under test (as the class name already clearly states which one is the test...).
I'd say, pick one and see what you like best.
Avoid $this in the body of test functions.
To some it does not matter, other folks (like me) prefer a "setup" of sorts at the beginning.
That way there are no distracting signatures in the important code. The bit you'll be looking at most will (most likely) look more like it will in
"normal" code (that is to say, non-test code). You can see this in the example code posted above.
Use @dataproviders
If you run into a situation where you feel inclined to use a foreach loop in your test, or feed it just one single value, refactor the code to use a dataprovider instead.
This is especially useful to make a distinction in which datasets are "good" versus "bad".
Another benefit is that developers often only test "happy path", even for things like exceptions.
Instead of merely testing against one invalid argument, for instance, you could use a dataprovider and test against many.
The results may surprise you, especially when testing against scalar values or obtional object parameters when fed a NULL.
It also allows such sets of data to be re-used from several tests functions. (DRY FTW!)


Split a string of hostnames, expand from regex, return non-duplicated names in original order I have the following function. It can be passed a candidate_string such as the following examples:

"device1 device2"
"device1"
"device.*"
"device.region"
"device[123]

Assume that target_list contains at least the following:

['device1', 'device2', 'device3', 'device4']

I want the output of calling the function to return the following:

['device1', 'device2']
['device1']
['device1', 'device2', 'device3', 'device4']
['device.region']
['device1', 'device2', 'device3']

This code works (and I have unit tests for all of the above cases and more), I would like to know if it can be improved on.

I am passing an optional target_list into the function, only in my tests, this is so I don't have to mock that function out.

######################################################################
# sub hostname_regex_expand($host_string, @target_list)
#
# Returns a list of hostnames from $host_string.
#
# $host_string should be a space separated list of 1 or more
# host_names, or regex strings representing host_names. If the
# given host_name contains chars other than [a-zA-Z0-9-], then we
# will try to match the assumed regex against the list of names returned
# by get_hostlist. It is also possible that this is a device name
# of the form device.region or FQDN. In that case,
# we will not match anything from get_hostlist so will use the name
# as provided. If the hostname is just a hostname, then we use it as is.
######################################################################
sub hostname_regex_expand {
    my ($host_string, @target_list) = @_;

    # split by whitespace
    my @host_names = split(' ', $host_string);

    # load hostlist
    if (not @target_list) {
        @target_list = get_hostlist();
    }

    my (%host_hash, @hosts);
    foreach my $host_name (@host_names) {
        my $found = 0;
        foreach my $target (@target_list) {
            if ($target =~ m/^$host_name$/) {
                $found = 1;
                if (not $host_hash{$target}) {
                    $host_hash{$target} = 1;
                    push(@hosts, $target);
                }
            }
        }
        # If we didn't match anything in the target_list, just add this
        # host_name to the hash.
        if (not $found and not $host_hash{$host_name}) {
            push(@hosts, $host_name);
            $host_hash{$host_name} = 1;
        }
    }

    return @hosts;
}

To jump to the conclusion first, I would rewrite the function like this:

sub hostname_regex_expand {
    my ($patterns_ref, $hostnames_ref) = @_;
    my @patterns = @$patterns_ref;
    my @hostnames = $hostnames_ref ? @$hostnames_ref : get_hostlist();

    my %results;
    foreach my $pattern (@patterns) {
        my @found = grep(/$pattern/, @hostnames);
        @found = $pattern unless @found;
        @results{@found} = ();
    }

    return sort keys %results;
}

First of all, the original variable names were not so good: @target_list, %host_hash, and $host_string contain in the name their types (list, hash, string), which is not normal or necessary. What's worse is that @host_names and $host_name are storing host name patterns, not really host names.

In terms of the logic, instead of iterating over each hostname to try to see if it matches a pattern, it would be more straightforward to use a grep.

It seems you really want to use two list parameters:

List of patterns
List of hostnames

So I changed the function to take two list references, it's cleaner than the original.

I took a shortcut that you might not like: instead of adding matched hosts and patterns to @hosts, I use the %results map, and return the sorted keys. The title of your post (and the original) suggests that you want to preserve the original order. If that's a requirement, not a problem, we can refactor like this:

sub hostname_regex_expand {
    my ($patterns_ref, $hostnames_ref) = @_;
    my @patterns = @$patterns_ref;
    my @hostnames = $hostnames_ref ? @$hostnames_ref : get_hostlist();

    my @ordered;
    my %results;
    foreach my $pattern (@patterns) {
        my @found = grep(/$pattern/, @hostnames);
        @found = $pattern unless @found;
        foreach my $item (@found) {
            if (!$results{$item}) {
                push(@ordered, $item);
                $results{$item} = 1;
            }
        }
    }

    return @ordered;
}


User and Database classes in PHP I am learning some OOP on PHP and I have developed some sample code to practise.

Ignoring the functionality, I just want to learn the "best" or "better" way to write OOP codes in PHP.

The following is my code and I hope anyone who has been doing PHP can help to review or comment for any improvements or weaknesses on my code.

I want to develop my style of code in which copy-and paste is minimum and functionality expandability is rich. For example, the execute() function in the UserDB class is basically the same function and can be used widespread in many diff db class. Do I need to copy and paste the code again or should I just moved this piece of code to abstract class?

How can I optimized my code to made it better for future functionalities such as other select, insert, update and delete while maintaining readability and still within OOP standards?

<?php

    require_once "sys_config.php";
    require_once "general.inc";
    $para[":user"]= "%";

    $dataset = UserFactory::getUsers($para);

    var_dump($dataset);

?>

UserFactory class

<?php

    class UserFactory{

        public static function getUsers($para)
        {
            $db = new UserDB("CALL getUsers(:user)",$para); //sp

            $objs = $db->execute();
            if(!empty($objs))
                foreach( $objs as $obj)
                {
                    $users[] = new user($obj);
                }
            return $users;
        }

    }

?>

UserDB class

<?php

    Class UserDB extends Database{

        protected $conn;
        protected $SQL;
        protected $para;

        function __construct($SQL, $para)
        {
            parent::__construct();

            $this->conn = parent::getConn();
            $this->SQL = $SQL;
            $this->para = $para;
        }

        function execute()
        {

            $query = $this->conn->prepare($this->SQL);
            if(!empty($this->para))
                foreach($this->para as $key=>$val)
                {
                    $query->bindParam($key,$val);
                }
            $results = $query->execute();

            $dataset = null;

            if($results)
            {
                while($row = $query->fetch(PDO::FETCH_OBJ))
                {
                    $dataset[]=$row;
                }
            }

            unset($this->conn);

            parent::__destruct();

            return $dataset;

        }

    }

?>

Database abstract class

<?php

    abstract Class Database{

        protected $conn;

        function __construct()
        {
            $this->conn = new PDO ("mysql:host=".DB_LOCATION.";dbname=".DB_NAME,DB_USER,DB_PASS);

        }

        function __destruct()
        {
            unset($this->conn);

        }

        function getConn()
        {
            return $this->conn;
        }

        abstract function execute();
    }

?>

User object class

<?php

    class User{

        private $fullname;
        private $id;

        function __construct($userObj)
        {
            $this->fullname = $userObj->fullName;
            $this->id = $userObj->id;
        }

        function setFullname($fullname)
        {
            $this->fullname = $fullname;
        }

        function getFullname()
        {
            return $this->fullname;
        }

        function setId($id)
        {
            $this->id = $id;
        }

        function getId()
        {
            return $this->id;
        }
    }

?>

Just a few comments on the code as it stands so far:

You're using pretty old-school file dependency management (which isn't the same as dependency management) by explicitly including things.  I'd strongly suggest you look into the SPL autoload system, write an autoloader that can find where your classes are located in the filesystem and rely on the autoloader to handle the file dependency management instead.  It means that only code that's actually going to be used will be included and you won't get into a situation where you have loads of included classes but only a few get used, or where you're not sure what's been included.
You're using static methods.  Generally, statics are frowned upon because they're not considered to be good OO practice.  Sometimes they're necessary, but as a rule you should avoid them.  Any other class that makes use of the static call becomes tightly coupled to the class that implements it, reducing re-usability and testability (it becomes very difficult to test the dependant modules in isolation because you can't easily replace the static with a mock implementation).  I'd advise refactoring your factory to work in a non-static way.
Your Database class seems like it might not be necessary.  It's merely a wrapper around PDO, and actually hides most of the PDO class functionality.  I'd suggest you might be better off using PDO directly, as it's got a well-documented interface and you'll have full access to all the PDO functionality in the consuming classes instead of the small subset your Database class provides.

I would say that on the whole you're on basically the right track though.  It's certainly better than my own early efforts with OOP.  Don't worry, thinking in an "objecty" way is something that comes with practice and experience.


Binary relation interface I'm trying to design a binary relation interface in Java and then implement it, but I'm not sure if I'm doing it right. I'd really appreciate a little feedback just so I know if I'm way in the wrong direction or not.

This is what I've got so far:

Interface:

public boolean compare(Object that, Object ths);
// compares relations to see if they are equal.

public Set<Y> getY(X value);
//return set of all Y values with relation X

public Set<X> getX(Y value);
//return set of all X values with relation Y

public void clear();
//clear relation of values

public Set<Relation<X, Y>> delY(X value);
//remove all relations containing given Y

public Set<Relation<X, Y>> delX(Y value);
//remove all relations containing given X

public String write(Object that);
//output Relation in string format

public Y addPair (X valX, Y valY);
//adds the entry (X, Y) to the Relation

public Y delPair (X valX, Y valY);
//deletes entry (X, Y) from the Relation

Implementation:

int size;

String[][] basket = new String[16][2];
String[][] bag = basket;

public String addPair(String X, String Y){

    basket[size][0] = X;
    basket[size][1] = Y;

    String pair = basket[size][0] + basket[size][1];
    size++;
    return pair;

}

public boolean compare(String[][] that, String[][] ths){
    that = basket;
    ths = bag;
    boolean equal;

    if(that.equals(ths))
        equal = true;
    else
        equal = false;
    return equal;
}

public Set<String> getY(String X){

    Set<String> Y = new TreeSet<String>();

    for(int i = 0;i<size;i++)
    {
        if(basket[i][0]==X)
            Y.add(basket[i][1]);
    }

    return Y;

}
public Set<String> getX(String Y){

    Set<String> X = new TreeSet<String>();

    for(int i = 0;i<size;i++)
    {
        if(basket[i][1]==Y)
            X.add(basket[i][0]);
    }

    return X;

}

public String write(Object o){

    o = basket;
    String out = "";
    out += o.toString();
    System.out.println("OUT: " + out);
    return out;

}

public void clear(){
 for(int i = 0;i<size;i++)
    {
        if(basket[i][0]!= null){
            basket[i][0] = null;
            basket[i][1] = null;
        }
    }
}

public Set<String> delY(String X, Relation r){
    Set<String> Y = new TreeSet<String>();

    for(int i = 0;i<size;i++)
    {
        if(basket[i][0]==X){
            Y.add(basket[i][1]);
            basket[i][1]=null;
        }
    }

    return Y;

}

public Set<String> delX(String Y){
   Set<String> X = new TreeSet<String>();

    for(int i = 0;i<size;i++)
    {
        if(basket[i][1]==Y){
            X.add(basket[i][1]);
            basket[i][1]=null;
        }
    }

   return X;
}

public String delPair (String X, String Y){
    X = "Orange";
    Y = "Fruit";
    for(int i = 0;i<size;i++)
    {
        if(basket[i][0]==X && basket[i][1]==Y){
            basket[i][1]=null;
            basket[i][0]=null;
        }
    }

     return (X + Y);

}

There are a couple of things in here that concern me.... but, the most concerning is:

Use .equals() to compare String values

Code like this:

if(basket[i][0]==X){

will fail when you get data that is not String-constant data.

That line should be:

 if(basket[i][0].equals(X)){

This problem is found all over your code.

In addition, there are other problems:

your arrays are pre-sized. This code:

String[][] basket = new String[16][2];

is potentially OK if you are initializing the array, but what if there are more than 16 pairs?
it would be better to create a 'Pair' class to contain your two strings, than to use the 2-size array on the basket. Then your basket could be something like:

StringPair[] basket = new StringPair[SIZE];

your StringPair class could look something like:

public class StringPair {
    private final String first, second;

    public StringPair(String first, String second) {
        this.first = first;
        this.second = second;
    }

    ..... getFirst() .....
    ..... getSecond() .....

}

compare(String[][] that, String[][] ths) is a method that needs no state data. It should be a static method, or a utility method. Alternatively, it should be a simple 1-value method compare(MyClass that) ... which, for what it is worth, would be very similar to the equals(...) method.

but, that method is pretty broken. It has the two input parameters that and ths, but it then overwrites those values with the internal versions bag and basket.

that = basket;
ths = bag;
boolean equal;

if(that.equals(ths))
    equal = true;
else
    equal = false;
return equal;

now, since you set bag = basket earlier, this will always return true.

Further, that last 5 lines of the method can be replaced with:

return (that.equals(ths));


batch progress visualization In React.js I am slowly digesting React.js, found couple of ways to write the same piece of code:
Please tell where all I went wrong in below code.

I try to put logic inside render(), is that Ok?
Not using React life cycles for simple components?
Use Functional components instead of class components?

File 1: batch_progress.js

Description: Shows batch progress, expects 3 values:

current batch.
target batch count.
state of the batch (based on which the progress bar color changes).

import React, { Component } from 'react';
import ProgressBar from '../progress_bar/progress_bar';

export default class BatchStatus extends Component {
  constructor(props) {
    super(props);
  }

  render() {
    let color;
    let percentage = (this.props.batch.number / this.props.targetBatchCount) * 100;

    switch (this.props.batch.status) {
      case 100:
        color = '#e7e4f1';
        break;
      case 200:
        color = '#c3dcec';
        break;
      case 300:
        color = '#ecc6eb';
        break;
      case 400:
        color = '#ecdec3';
        break;
      case 500:
        color = '#c8ecc7';
        break;
      default:
        color = '#e7e4f1';
    }

    return (
      <ProgressBar foregroundColor={color} percentage={percentage}>
        {this.props.batch.number}&nbsp;/&nbsp;{this.props.targetBatchCount}
      </ProgressBar>
    );
  }
}

File 2: progress_bar.js

import React, { Component } from 'react';
import './progress_bar.css';

export default class ProgressBar extends Component {
  constructor(props) {
    super(props);
  }

  render() {
    let foregroundColor = this.props.foregroundColor || '#e7e4f1';
    let percentage = this.props.percentage || 0;
    let backgroundColor = this.props.backgroundColor || '#eceeef';

    let style = {
      backgroundImage:
        'linear-gradient(to right, ' +
        foregroundColor +
        ' ' +
        percentage +
        '%, ' +
        backgroundColor +
        ' 0%)'
    };

    return (
      <div className="progress-bar-container" style={style}>
        {this.props.children}
      </div>
    );
  }
}

The function render() in only supposed to render. It is not supposed to compute some style, set variables, have any logic other than render: (Functions Names Should Say What They Do). This should respect the SRP (Single Responsiblity Principle), as well as another clean code rule: 'A Function Should Do One Thing'.

What could be code smell in your first file:

A unused constructor should not be there at all,
The percentage calculation should be put in a function of its own
The color choice should be placed in a function of its own

What could be code smell in your second file:

A unused constructor should be removed

For both files, you could also add some prop types verifications using the library 'prop_types'.

As for your questions, with short answers:

Not it's not Ok
Depends on what you intend to do!
This article should help you choose: React Functional or Class components ? - even though you can use hooks now as well :)


Storing and iterating over multiple NSError pointers How would you refactor this kind of code? I've unsuccessfully tried several approaches (NSArray, NSPointerArray, plain C array), but always ran into pointer type checking error.

NSError *error1;
NSError *error2;
NSError *error3;
//imagine MANY of them
[object1 methodWithError: &error1];
[object2 methodWithError: &error2];
[object3 methodWithError: &error3];
BOOL wasErrorOfCode1 = ([error1 code] == 1) || ([error2 code] == 1) || ([error3 code] == 1);
if (wasErrorOfCode1) {
//do stuff

I want to propose a differently structured code, that especially will be easier to adapt for more objects.

NSArray *objects = @[object1, object2, object3];
NSMutableArray *errors = [NSMutableArray array];

[objects enumerateObjectsUsingBlock:^(id obj, NSUInteger idx, BOOL *stop) {
    NSError *error;
    [obj methodWithError: &error];
    if(error){
        [errors addObject: @{@"error":error,@"object":obj}];
    }
}];

Now you can iterate over all errors and examine them along with the object the error occurred on.

[errors enumerateObjectsUsingBlock:^(NSDictionary *errorDict, NSUInteger idx, BOOL *stop) {
    NSError *error = errorDict[@"error"];
    id object = errorDict[@"object"];
    if(…){
        //add an appropriate test
    }
}];

you can't add the errors into an array upfront, as

NSError *error1;

will result into a nil-object for the variable error1. NS(Mutable)Arrays can't handle nil objects. Same for dictionaries.
On the other hand my solution allows adding the errors, as it only add them once an existing objects is assigned to the variable error.


Background task with instant abort capability in c# I'm looking for a better way to make a background operation with abort/cancel support. Right now I use this approach:

Task.Factory.StartNew(() => {
    var cts = new System.Threading.CancellationTokenSource();
    var ct = cts.Token;
    try
    {
        var t = System.Threading.Thread.CurrentThread;
        using (ct.Register(t.Abort))
        {
            LongOperation();
        }
    }
    catch {...} // an exception gets thrown about aborting threads
});

Calling ct.Cancel() stops the operation even if it's something that cannot usually be interrupted. The focus is that it can be stopped instantly.

I don't think this is unsafe if I'm not doing any file writing. So far it's been working well enough for me, but this code doesn't look proper to me. Is there a more canonical way to start background operations and abort them instantly? Are there any safety concerns I have been lucky not to encounter so far?

Update 2 years later: I looked it all up again, and it seems like a better way to avoid problems is to "abort" a process instead of aborting a thread on your app's main process. See this answer by Eric Lippert. to "What is a safe way to stop a running thread?" question.

Disclaimer: Don't do that

Aborting threads usually means that the thread will terminate and will no longer be available to do anything else. While that maybe exactly what you want there are some scenarios where you have to be really careful about aborting threads namely a shutdown process i.e (where you have to do some work before exiting).

It is also worth noting that you are trying to abort a thread from the thread pool, because the TPL uses the thread pool. And those threads should be always ready to get work done. If you abort one of the threads in the worst case then the thread pool will need to create another thread to replace it, meaning that you will have a cycle of threading creation and abortion.

Code Review

You are creating a CancellationTokenSource inside the working thread, meaning that only the working thread will be able to cancel/abort itself.
Cancellation is a mechanism meant to be used across multiple threads, because a thread is already responsible for it's own life-cycle.
So you should use other kinds of mechanisms, i.e time-based using Stopwatch, to determine if you want to leave or not.

If you want to know how to effectively cancel an operation please head to my other answer here

EDIT

Unlike you, I don't think there is something overly complicated on your code.
There should be a minimal set of comprehensible code to achieve some functionality and .NET API AFAIK doesn't have a simpler way to do it. However I would like to point two small things:

You could have used a using statement for System.Threading

You could have used meaningful names. I won't complain too much about cts because sometimes I also use that but I acknowledge that cancelSource is better. However ctcould be easily replaced by at least token or if you want to go a step further cancelToken. And t could be simply threador yet again currentThread.

Another alternative could also be to don't have that many variables I wouldn't mind to write cancelSource.Token and Thread.CurrentThread, if they happen to be used just once.


A recursive solution to symmetirc tree I tried to solve a  symmetric tree problem

  Given a binary tree, check whether it is a mirror of itself (ie, symmetric around its center).

  For example, this binary tree [1,2,2,3,4,4,3] is symmetric:

    1
   / \
  2   2
 / \ / \
3  4 4  3

  But the following [1,2,2,null,3,null,3] is not:

    1
   / \
  2   2
   \   \
   3    3

  Note:
  Bonus points if you could solve it both recursively and iteratively.

My solution with recursion

# Definition for a binary tree node.
# class TreeNode:
#     def __init__(self, x):
#         self.val = x
#         self.left = None
#         self.right = None

class Solution(object):
    def isSymmetric(self, root):
        if not root: return True #None is symmetic
        return self.isMirror(root.left, root.right)

    def isMirror(self, l, r):
        if not l and not r: return True #base case 1
        if not l or not r: return False #base case 2
        if l.val != r.val: return False #base case 3
        #recur case
        left = self.isMirror(l.left, r.right)
        right = self.isMirror(l.right, r.left)
        return left and right

I assumed it as a decent solution to this problem but get a low score

  Runtime: 32 ms, faster than 24.42% of Python online submissions for Symmetric Tree.
  Memory Usage: 12.2 MB, less than 5.08% of Python online submissions for Symmetric Tree.

How could improve the my solution?

Possibly the most obvious part is here

    left = self.isMirror(l.left, r.right)
    right = self.isMirror(l.right, r.left)
    return left and right

there's no need to perform the second test if the first one returns False:

    if not self.isMirror(l.left, r.right): return False
    if not self.isMirror(l.right, r.left): return False

    return True


Finding clients in a 100 km circle I needed a node app that:

reads a list in text file format from URL
parses the rows on the list. (fields: id, name, longitude, latitude)
finds names of the people who live maximum 100 kms away from a set location.
sorts the list by user id

Could you take a look and let me know what I could have done better here?

To run:

nodejs scriptname.js

 var ClientsNearby = (function(){
  'use strict';
  var contacts = [],
    dublinOffice = {
      latitude: 53.3381985,
      longitude: -6.2592576
    };
  var request = require('request');
  function getDistanceFromLatLonInKm(lat2,lon2) {
    var R = 6371; // Radius of the earth in km
    var dLat = toRadians(lat2-dublinOffice.latitude);  // toRadians below
    var dLon = toRadians(lon2-dublinOffice.longitude);
    var a =
      Math.sin(dLat/2) * Math.sin(dLat/2) +
        Math.cos(toRadians(dublinOffice.latitude)) * Math.cos(toRadians(lat2)) *
          Math.sin(dLon/2) * Math.sin(dLon/2);
    var c = 2 * Math.atan2(Math.sqrt(a), Math.sqrt(1-a));
    var d = R * c; // Distance in km
    return d;
  }

  function toRadians(deg) {
    return deg * (Math.PI/180);
  }
  function processClients(contacts) {
    // saving clients living in a 100km radius
    var distanceIncludedClientList =
      contacts.map(client => {
        return {
          latitude: client.latitude,
          longitude: client.longitude,
          user_id: client.user_id,
          name: client.name,
          distance: getDistanceFromLatLonInKm(client.latitude, client.longitude)
        };
      });

    var closeByClients = distanceIncludedClientList.
        filter(client => client.distance <= 100);
    function compareUserID(a,b) {
      if(a.user_id > b.user_id)
        return 1;
      if(a.user_id < b.user_id)
        return -1;
      return 0;
    }
    var closeByClientsSorted = closeByClients.sort(compareUserID);

    report(closeByClientsSorted);
  }
  function report(contacts) {
    console.log('Clients who live max 100 Kms from our office: \n');
    contacts.map(contact => {
      console.log(` id: ${contact.user_id}, Name: ${contact.name}, Distance: ${contact.distance.toFixed(1)}Km`);
    });
  }

  function init() {
    var textFileUrl = 'https://gist.githubusercontent.com/brianw/19896c50afa89ad4dec3/raw/6c11047887a03483c50017c1d451667fd62a53ca/gistfile1.txt';

    request.get(textFileUrl, function (error, response, body){
      if (!error && response.statusCode === 200) {
        contacts = body.split('\n');

        contacts = contacts.map(function(elm){
          elm = JSON.parse(elm);
          return elm;
        });
      }
      processClients(contacts);
    });

  }
  return {
    init: init,
    toRadians: toRadians,
    processClients: processClients,
    getDistanceFromLatLonInKm: getDistanceFromLatLonInKm
  };
})();

ClientsNearby.init();
module.exports = ClientsNearby;

I'll just list things as I read them, so don't expect any sort of meaningful order:

Prefer let over var - let provides variable declaration in block scope, rather than functional scope. It also prevents hoisting. It solves a few language gotchas that var has. Please note that currently, you can only use let in contexts under strict mode (which you already have, good for you!)
Your getDistanceFromLatLonInKm() function has many single letter variables, which makes it a bit hard to follow. What's R? Earth's radius in km? Why not EarthRadius?
If you only need to return a single expression from an arrow function, you don't need the full form.

(arg1, arg2) => { return something }

can be shortened to

(arg1, arg2) => something

in case something is an object literal, you can wrap it with braces like so:

(arg1, arg2) => ({something: here})

Your compareUserID() function can be shortened to

var compareUserID = (a, b) => a.user_id - b.user_id;

Think twice before defining a function within another function, the inner function will be redefined for every function call.
The [].map() function is meant for mapping one array to another, not for logging. If you want to just iterate the array and do something without returning anything, use [].forEach(), also, you don't need to full form of the arrow function for console.log:

contacts.forEach(contact => console.log(`...`));

Node supports object literal shorthands, if the key and the variable name you're defining have the same name, you can do it like this:

return {init, toRadians, processClients, getDistanceFromLatLonInKm};

It would be likely better if the init() function accepted the URL to the file to be processed. That way, if you ever change URLs (which is more than likely, because raw GitHub urls stay relevant for so long), you can leave your module's code untouched.
Node's modules are implicitly wrapped by IIFEs anyway, so your IIFE is redundant. Remove var ClientsNearby = (function(){ and }(); to save yourself a level of indentation across the script. Just assign your object literal with functions to module.export directly.
It's considered good practice to have as few side effects as possible in your modules (such as producing output). Think about how you'd want to use a module. Personally, I'd like to be able to invoke a function and get an array (or a collection) of data, for me to do what I please. Tomorrow you wouldn't want to output the clients, you'd want to log them into a file, or push them into a database.


Toggling element class according checked radio input V2.0 This question is about an improvement of the code from this previous question  (previous changed visibility, new applies a CSS class with a border).

It is a constructor function that attaches a click event to radio inputs using event delegation in order to add, remove CSS classes and enable the text input which contains a data-value equals to the radio value.

Short example

<input type="radio" name="color" value="yellow" checked />Yellow
<input type="text" class="radio-condition visible" data-value="yellow" />

One important fact is that it must work for old browsers such as IE7-8.  I know I'm using methods from EcmaScript 6 (is ES6 right?) such as classList which won't work. But I found this link from hacks.Mozilla.org where it shares a code for older browsers. So I guess I can use those ES6 methods safely, right?

jsFiddle here!

<form>

<input type="radio" name="color" value="yellow" checked />Yellow
<input type="radio" name="color" value="brown" />Brown
<input type="radio" name="color" value="orange" />Orange
<input type="radio" name="color" value="purple" />Purple

<input type="text" class="radio-condition visible" placeholder="Which yellow?" data-value="yellow" />
<input type="text" class="radio-condition" placeholder="Which brown?" data-value="brown" disabled />
<input type="text" class="radio-condition" placeholder="Which Orange?" data-value="orange" disabled />
<input type="text" class="radio-condition" placeholder="Which Purple?" data-value="purple" disabled />

</form>

Let's start with a dry review:

Underscores in JavaScript hurt the eyes. The absolute naming convention of JavaScript is to use camelCase. Your underscore notation, while consistent, hurts the eyes of other JavaScript developers.
Using event delegation is great, but since you've queried the DOM once when you instantiated the element, you can only use the elements which existed in the DOM when you instantiated the object. Boo.
This line

  this.radio_set = document.querySelector('radio-set'),

selects the first <radio-set></radio-set> element.
You're selecting all of the radio buttons on init, what happens if you have other radio elements which aren't related to your module? You'll process them too, which is undesirable.

Now a bit on the approach, I prefer the Revealing Module Pattern for making self-contained modules. It allows you to have public and private properties and methods, and only reveal what you want.

My Take

jsFiddle

 <form id="module">
    <input type="radio" name="color" value="yellow" checked/>Yellow
    <input type="radio" name="color" value="brown"/>Brown
    <input type="radio" name="color" value="orange"/>Orange
    <input type="radio" name="color" value="purple"/>Purple

    <input type="text" class="radio-condition visible" placeholder="Which yellow?" data-value="yellow"/>
    <input type="text" class="radio-condition" placeholder="Which brown?" data-value="brown" disabled/>
    <input type="text" class="radio-condition" placeholder="Which Orange?" data-value="orange" disabled/>
    <input type="text" class="radio-condition" placeholder="Which Purple?" data-value="purple" disabled/>
</form>


How to remove redundancy in code when alternating between methods? Alright, so I've been tasked with recreating the game of pig dice in Java, however the issue that im having with creating a method that switches back and fourth between pc and user is that there's a lot of redundancy with calling methods with different parameters.

How would I be able to condense this alternating code?

if (coin.equals("heads")) {
    if (currentPlayer.equals(pcName)) {
        pcPoints = roll2Dices(kb, random, currentPlayer, pcPoints, userName, pcName, round);
        currentPlayer = userName;
    } else if (currentPlayer.equals(userName)) {
        userPoints = roll2Dices(kb, random, currentPlayer, userPoints, userName, pcName, round);
        currentPlayer = pcName;
    }
} else if (coin.equals("tails")) {
    if (currentPlayer.equals(userName)) {
        userPoints = roll2Dices(kb, random, currentPlayer, userPoints, userName, pcName, round);
        currentPlayer = pcName;
    } else if (currentPlayer.equals(pcName)) {
        pcPoints = roll2Dices(kb, random, currentPlayer, pcPoints, userName, pcName, round);
        currentPlayer = userName;
    }
}

Rethink your function

Instead of using if statements you can store the points in an array and then the current player variable could be an index for the array (so not a string value, use and integer instead).

Try to think in numbers instead of strings.

Code example:

int[2] pointsArray;
int currentPlayer = 0;
pointsArray[currentPlayer] = roolDiceFunction();

If you really want to use strings though you could use a map but I think it will just make your code more complicated.

Coin:

Just do the coin check at the start of your function by setting the start index for current player. For example if you want the user to start if the coin lands on heads which you can represent as 0, you then just set the current player index to a random value between 0 & 1.

Example:

int[2] pointsArray;
int currentPlayer = (int)Math.random(); //Random value between 0 & 1
pointsArray[currentPlayer] = roolDiceFunction();

I'm guessing there's more to your function than this, but you probably want to have it in a loop of some kind and then ask at the end of the function if the user wants to keep rolling, if they don't want to keep rolling you can change the current player index.

More about your functions

Make the rool2Dice function simpler, does it really need that many parameters, you are passing the exact same most of the time.

If you have any more questions just ask away, good luck.


Vertical "moving" text on a LCD-display Following idea:

  You have a LCD-display with 2 rows.

  You write the first line of a "text" to the second row. Then you clean the screen.
  After a delay you write the first line to the first row and a the second line of the text to the second row. Then follows another clean
  and delay.
  Then you write the second line to the first row and the third line to the second row. And so on ...

  When the end of the text is reached the whole process starts again
  from the beginning.

  That way the impression of an "vertical moving" text is accomplished.

The code I have written:

#include <LiquidCrystal.h>

LiquidCrystal lcd(12, 11, 5, 4, 3, 2);

void setDisplay(String lineValue, int lineNumber, int waitUntilClear, boolean clearScreen) {
  lcd.setCursor(0, lineNumber),
  lcd.print(lineValue);

  delay(waitUntilClear);

  if (clearScreen == true) {
   lcd.clear();
  }
}

void setup() {
  lcd.begin(16, 2);
}

void loop() {
  const int DELAY = 3000;
  const int COUNT_ELEMENTS = 12;

  String messages[COUNT_ELEMENTS] = {
    "",
    "Furniture Store",
    " MEYER & MILLER",
    "",
    "  Contact us!",
    "",
    " -- Email ----- ",
    "demo@example.com",
    "",
    " -- Phone ----- ",
    " 000 123456",
    ""
  };

  for (int i = 0; i < COUNT_ELEMENTS - 1; i++) {
    setDisplay(messages[i], 0, 0, false);
    setDisplay(messages[i + 1], 1, DELAY, true);
  }
}

It works fine and by using an additional function I could avoid code redundancies mostly. If someone has an idea how to improve my code or even doing it completely different in a better way, then I would appreciate his/her comment or answer.

waitUntilClear is always 0 when clearScreen is false then boolean flag is redundant:

void setDisplay(String lineValue, int lineNumber, unsigned long msToWaitBeforeClear) {
  lcd.setCursor(0, lineNumber),
  lcd.print(lineValue);

  if (msToWaitBeforeClear > 0) {
   delay(msToWaitBeforeClear);
   lcd.clear();
  }
}

Note that delay() accepts unsigned long instead of int then our parameter should match its type (does it make sense to have a negative delay?)

Now is (should be) clearer that we're calling setDisplay() twice but it's easier to simply call it with the index of the current line:

void displayPage(String messages[], size_t startIndex) {
  for (int8_t rowIndex = 0; rowIndex < NUMBER_OF_DISPLAY_LINES; ++rowIndex) {
      lcd.setCursor(0, rowIndex),
      lcd.print(messages[startIndex + rowIndex]);
  }

   delay(DELAY_BETWEEN_PAGES_IN_MS);
   lcd.clear();
}

Used as:

for (size_t i = 0; i < COUNT_ELEMENTS - 1; ++i) {
  displayPage(messages, i);
}

Now it's time to remove those magic numbers in setup() and lcd declaration. I'd also suggest to move all global constants to the very top, outside functions (probably in a separate file). It's what you want to change to configure your display and it's better if you don't have to search them buried inside code.

One small note: if you manually unroll the loop over NUMBER_OF_DISPLAY_ROWS then you can skip the first call to setCursor() because clear() already do it for you:

void displayPage(String messages[], size_t startIndex) {
    lcd.print(messages[startIndex]);
    lcd.setCursor(0, 1),
    lcd.print(messages[startIndex + 1]);

    delay(DELAY_BETWEEN_PAGES_IN_MS);
    lcd.clear();
}


Checking Object keys set with value against another Object keys I'm willing to check an an object's values in typescript, based on another object's keys.
The mental model is "the key exists, it's not an object, is the value in obj set, otherwise, it's an object, iterate again".
What would be your recommandations about this simple snippet :
/**
 * Returns an array. If the array is empty, the checked obj is fine.
 * Otherwise it's not.
 *
 * @remarks
 * areValuesSet(obj, rules).length === 0 is the test to make.
 *
 * @param obj {object} - object to verify
 * @param rules {object} - object containing the keys to check against
 * @returns {string[]} - array of incorrect keys
 */
const areValuesSet = (
  obj: { [propKey: string]: any },
  rules: { [propKey: string]: any }
): string[] =>
  Object.keys(rules).reduce((prev, key) => {
    if (typeof rules[key] === "object" && typeof obj[key] === "object") {
      return areValuesSet(obj[key], rules[key]);
    }

    return !obj ||
      !obj[key] ||
      (isEmpty(obj[key]) && typeof obj[key] !== 'boolean')
      ? [...prev, key]
      : [...prev];
  }, []);

I would enhance this with a "validator" function which would call areValuesSet like so :
/**
 * Call `rulesParser` and checks the returned value length equals 0
 *
 * @param obj {object} - object to verify
 * @param rulesObj {object} - object containing the keys to check against
 * @returns {boolean}
 */
export const validateRules = (
  obj: { [propKey: string]: any },
  rulesObj: { [propKey: string]: any }
): boolean => rulesParser(obj, rulesObj).length === 0;

Thank you

Function and parameter names The function name areValuesSet could be more precise. It returns an array of incorrect property names, not a boolean of if values are set. Maybe call it getIncorrectProperties? Also, the argument named obj is uninformative. You want to indicate that it's the target value being checked. Maybe call it validateTarget or something similar.
Avoid any any is type-unsafe. Unless you have a really good reason for it, better to type something you don't know about as unknown instead. unknown is similar to any, but requires you to do type-narrowing first. In this case, using unknown will point out a bug in your current logic:
typeof null gives object You have:
if (typeof rules[key] === "object" && typeof obj[key] === "object") {
  return areValuesSet(obj[key], rules[key]);
}

This will result in the recursive call throwing if one of the values happens to be null, since null is an object. Add in and call:
const isObject = (arg: unknown) => typeof arg === 'object' && arg !== null;

!obj[key] excludes false Your current logic permits values of true but forbids values of false due to this check:
!obj[key] || (isEmpty(obj[key]) && typeof obj[key] !== 'boolean')

Is that deliberate? It's OK if it is, but it may well be a logic bug. If false should indeed be excluded, then typeof obj[key] !== 'boolean' simplifies to obj[key] !== true.
Nested property bugs
(1) The recursive call is only entered if both the rules and target are objects. If the rule is an object, and the target is a non-empty non-object, the target will pass, when it probably shouldn't. Eg:
// rules:
{
  prop: { val: 'val' }
}

// target:
{
  prop: 'foo'
}

should probably fail, but won't. If the rule is an object and the target value is not an object, you probably want to push the property to the array of errors.
(2) When a sub-object is found as a value, your reduce callback does:
return areValuesSet(obj[key], rules[key]);

Whatever may have been put into the accumulator array on prior iterations will be lost, since it's not being factored into the return value. For example, if you had a data structure for which the first 5 properties were wrong, but the 6th property is nested and correct, the function would return an empty array.
But reduce arguably isn't very appropriate in these sorts of situations anyway - see that link for a video by Chrome developers on the subject. It introduces an annoying amount of boilerplate code, especially in TS, and even well-formulated reduces like these often aren't entirely trivial to understand at a glance.
Consider creating an array outside the function and pushing to it instead - or use a recursive function which creates the array to return as a default argument when not passed. Including all the other suggestions as well:
type GenericObj = { [propKey: string]: unknown };
const isObject = (arg: unknown): arg is GenericObj => typeof arg === 'object' && arg !== null;

const getIncorrectProperties = (
  validateTarget: GenericObj,
  rules: GenericObj,
  incorrectProperties: string[] = [],
) => {
  for (const [key, ruleValue] of Object.entries(rules)) {
    const targetValue = validateTarget[key];
    if (isObject(ruleValue)) {
      if (!isObject(targetValue)) {
        incorrectProperties.push(key);
      } else {
        getIncorrectProperties(targetValue, ruleValue, incorrectProperties);
      }
    } else if (!targetValue || (isEmpty(targetValue) && targetValue !== true)) {
      incorrectProperties.push(key);
    }
  }
  return incorrectProperties;
};

Live snippet of compiled code including example validation, to show that it's working:

"use strict";
const isObject = (arg) => typeof arg === 'object' && arg !== null;
const getIncorrectProperties = (validateTarget, rules, incorrectProperties = []) => {
    for (const [key, ruleValue] of Object.entries(rules)) {
        const targetValue = validateTarget[key];
        if (isObject(ruleValue)) {
            if (!isObject(targetValue)) {
                incorrectProperties.push(key);
            }
            else {
                getIncorrectProperties(targetValue, ruleValue, incorrectProperties);
            }
        }
        else if (!targetValue || (_.isEmpty(targetValue) && targetValue !== true)) {
            incorrectProperties.push(key);
        }
    }
    return incorrectProperties;
};
console.log(getIncorrectProperties({
    prop2: {
        nested: 'val',
    }
}, {
    prop1: true,
    prop2: {
        nested: true,
        nested2: {
            deeplyNested: true
        }
    },
    prop3: true
}));
<script src="https://cdnjs.cloudflare.com/ajax/libs/lodash.js/4.17.20/lodash.min.js"></script>

The only issue I have with the above code is the incorrectProperties argument. Although the approach works to create an array on initial call and return it at the end, having it as a parameter might be confusing, since the initial call should not take 3 arguments, only 2. If you think that's not acceptable, you could create an array in the function body on every call and return it at the end, and spread the result of the recursive call into that array.


Calculating Chinese zodiac signs based on birthdays I am attempting to write a function to calculate a user's Chinese zodiac sign based on his or her birthday for Drupal.

I originally had a function for calculating based on the Gregorian calendar (1924 = Rat, 1925 = Ox).  This was easy to write, but several of my users have complained that the Chinese zodiac is actually based on the lunar calendar, and a list of relevant start and end dates can be found here.

I started writing the code as follows.  The big problem is that I have an if, else if, and else per year, and at this rate I will have to manually code 100 years worth of birthdays.  On the other hand, I'm unsure of how to use a loop to iterate over this, because the day the lunar year starts and ends each year is different.  Any insight on how to make this more efficient would be highly appreciated.

function mymodule_calculate_chinese_zodiac($birthdate) {
  // Drupal MySQL data looks like this: 1981-07-30 00:00:00
  $year = substr($birthdate, 0, 4);
  $month = substr($birthdate, 5, 2);
  $day = substr($birthdate, 8, 2);

  if ($year == 1924) {
    if ($month > 2) {
      // Rat
      $sign = "1";
    }
    else if (($month == 2) && ($day > 4) {
      $sign = "1";
    }
    else {
      // Pig
      $sign = "12";
    }
  }
  else if ($year == 1925) {
    if ($month > 1) {
      // Ox
      $sign = "2";
    }
    else if (($month == 1) && ($day > 23)) {
      $sign = "2";
    }
    else {
      $sign = "1";
    }
  }
  else if ($year == 1926) {
    if ($month > 2) {
      // Tiger
      $sign = "3";
    }
    else if ($month == 2) && ($day > 12) {
      $sign = "3";
    }
    else {
      $sign = "2";
    }
  }
  //All years until 2020
  return $sign;
}

To start, lets look for something that we can loop through. From your link to relevant dates, we can see that the associated animals loop through 12 animals, in order:

Rat
Ox
Tiger
Rabbit
Dragon
Snake
Horse
Goat
Monkey
Rooster
Dog
Pig

After [Pig], it restarts again with [Rat]. So let's put these into an associative array, with the associated element:

$assocs = array(
    "Rat"     =>  "Yang Wood",
    "Ox"      =>  "Yin Wood",
    "Tiger"   =>  "Yang Fire",
    "Rabbit"  =>  "Yin Fire",
    "Dragon"  =>  "Yang Earth",
    "Snake"   =>  "Yin Earth",
    "Horse"   =>  "Yang Metal",
    "Goat"    =>  "Yin Metal",
    "Monkey"  =>  "Yang Water",
    "Rooster" =>  "Yin Water",
    "Dog"     =>  "Yang Wood",
    "Pig"     =>  "Yin Wood" )
);

Now, to determine the sexagenary cycle of a gregorian year, the algorightm is as follows:

sCycle = (grego - 3) - (60 X ([(grego - 3)] / 60))

So, let's translate that to a PHP function:

function sexagenaryToGregorian($gYear){
    return ($gYear - 3) - (60 * (floor(( $gYear - 3) / 60)));
}

Now, we know that the Sexagenary cycle consists of 60 periods, thus, to determine what animal and element a certain year matches:

$bdayYear = 1989;
$sYear = sexagenaryToGregorian($bdayYear);
// $sYear == 6

For birth year 1989, sYear should be period 6. Now, let's generate a function that will hold the animal-element array above ($assoc) and contain two loops. The outer loop will decrement through the ceiling of the sexagenarian cycle divided by 12, and the inner loop is a foreach() over the animal-element array. This method is accurate all the way back to 4AD.

function generateAnimal($sexaCycle){
    $assocs = array(
        "Rat"     =>  "Yang Wood",
        "Ox"      =>  "Yin Wood",
        "Tiger"   =>  "Yang Fire",
        "Rabbit"  =>  "Yin Fire",
        "Dragon"  =>  "Yang Earth",
        "Snake"   =>  "Yin Earth",
        "Horse"   =>  "Yang Metal",
        "Goat"    =>  "Yin Metal",
        "Monkey"  =>  "Yang Water",
        "Rooster" =>  "Yin Water",
        "Dog"     =>  "Yang Wood",
        "Pig"     =>  "Yin Wood"
    );

    $i = 1;
    while(ceil($sexaCycle / 12) > 0){
        foreach($assocs as $animal => $element){
            if($i == $sexaCycle)
                return array($animal => $element);
            $i++;
        }
    }
}

Let's give it a few runs:

// Let's find the result for the year 2041
$result = generateAnimal(sexagenaryToGregorian(2041));
print_r($result); // Array ( [Rooster] => Yin Water )

// Let's find the result for the year 4
$result = generateAnimal(sexagenaryToGregorian(4));
print_r($result); // Array ( [Rat] => Yang Wood )

// Let's find the result for the year 1989
$result = generateAnimal(sexagenaryToGregorian(1989));
print_r($result); // Array ( [Snake] => Yin Earth )

Looks good to me! Now, if you want to include the dates, that's when you may want to start thinking about just using a database as a static lookup table.


Finding the candidate with the majority of ballots I am doing a code wars problem and so far I have managed to loop through a collection to find the "winner" (defined by the String that appears more than collection.size()/2). However, my code times out and isn't efficient enough. How can I make this more efficient?

import java.util.List;

public class BallotsCounter1 {
    static String winner;
    static String current;
    static int count;

     public static String getWinner(final List<String> listOfBallots) {
         int totalSize = listOfBallots.size();
         count = listOfBallots.size()/2;
         if (listOfBallots.isEmpty() == true){
             return null;
         } else {
             for (int i = 0; i < listOfBallots.size(); i++){
                 int running = 0;
                 current = listOfBallots.get(i);
                 for (int j = 0; j < listOfBallots.size(); j++){
                     if (listOfBallots.get(i) == listOfBallots.get(j)){
                         running++;
                     }
                 }
                 if (running > count){
                     count = running;
                     winner = current;
                 }
             }
         }
         if (count > (listOfBallots.size()/2)){
             return winner;
         } else {
             return null;
         }

     }
}

Before making it faster, it is important to make it better.

Scope of variables

You currently have 3 static variables in your code

static String winner;
static String current;
static int count;

that are only used inside the getWinner method. This is not a good idea:

A variable should have a scope as minimal as possible. This means you only declare that variable when you need it and not a moment before.
They are not private which means that another unrelated class could even access and modify them, thereby completely destroying the algorithm.

You should remove those static variables and have instead:

int count = listOfBallots.size()/2;
// ...
String current = listOfBallots.get(i);
// ...
String winner = null; // before the if/else part

Unused variable

You are storing the size of the input list into the variable totalSize but this variable is unused in the rest of your code. You should either remove it (unused code should be deleted) or make use of it and not call listOfBallots.size() each time it is needed.

Since the current code gets the size of the list multiple times, it would be better to store it like you did and reuse that variable. Example, instead of:

int count = listOfBallots.size()/2;

have

int count = totalSize / 2;

Also, notice the added spaces before the operator / that adds to readability.

Don't compare booleans to true or false

In the case of an empty list, you currently return null with:

if (listOfBallots.isEmpty() == true){
    return null;
}

This explicit check with == true is redundant and even harder to read than a simple

if (listOfBallots.isEmpty()) {
    return null;
}

Early return

In your code, you are returning early in the case of an empty list. But you are not going all the way. You have:

if (listOfBallots.isEmpty()) {
    return null;
} else {
    // ...
}

The else part is not needed and it adds an unnecessary indent to the code. Consider removing it, thereby shifting all the subsequence code one indentation to the left, so making it easier to read for small screens:

if (listOfBallots.isEmpty()) {
    return null;
}
// ...

As noted by Fabian Barney in the comments, you don't even need to return early: when the list is empty, your initial count will be equal to listOfBallots.size()/2 and null will still be returned. Might as well get rid of it and make the code shorter.

Enhanced for loop

Your two loops are written using the traditional index solution:

for (int i = 0; i < totalSize; i++){
    // ...
    String current = listOfBallots.get(i);
    for (int j = 0; j < totalSize; j++){
        if (listOfBallots.get(i) == listOfBallots.get(j)){
            // ...

When you don't need the index, it is better to use the enhanced for loop. It makes the code more compact and easier to read:

for (String current : listOfBallots) {
    // ...
    for (String other : listOfBallots) {
        if (current == other) {

Notice that it also makes another problem appear more clearly:

Do not compare Strings with ==

This won't work like you would expect. To compare Strings, you should really use the equals method and have:

if (current.equals(other))

Returning null

As a last comment, if you explicitely return null when no winner has been found then it would be best to document it clearly. There could be other approaches, like:

Throwing a custom exception.
Returning an Optional<String> (starting with Java 8).

Putting it all together

We all those comments, you could have the following:

public static String getWinner(final List<String> listOfBallots) {
    int totalSize = listOfBallots.size();
    int count = totalSize / 2;
    String winner = null;
    for (String current : listOfBallots) {
        int running = 0;
        for (String other : listOfBallots) {
            if (current.equals(other)) {
                running++;
            }
        }
        if (running > count) {
            count = running;
            winner = current;
        }
    }
    if (count > totalSize / 2) {
        return winner;
    } else {
        return null;
    }
}

Now, let's analyze your current code and spot where the slowness is coming from. The goal of the method is:

  to find the "winner" (defined by the String that appears more than collection.size()/2).

To do that, you are looping over the list, and for each element, you are looping a second time over the list and counting equals element. That is the slow part: for a list of size n, you just made your approach O(n2). You are traversing the list again and again to find the equal elements.

How to improve that? Look at it this way: the goal is to count the number of times the String appears in the list. To do that, we just need to loop over that list only once, but remember the current count.

Take the following list:

A  ---  B  ---  A  ---  D

We first encounter A and we remember that A has been encountered 1 time.
Then we move to B. B was never encountered so we also remember that, now, we encoutered it 1 time.
Then we move to A. But, now, we know that it has already been encoutered 1 time: we remembered it already. So we can just remember that A was encoutered 2 times now.
Finally, we hit D that we never encountered and we remember that we encoutered it 1 time.

In that pattern, the list was only traversed a single time. With the current implementation, it would have been traversed 5 times (the main one and 4 other times to count for how many times each element appeared).

A good data structure to use for that memory is a Map<String, Integer>, that is to say a data structure that maps one String into a count.

There is another optimization that we can make. The winner is the String appearing more than half the time in the initial collection. This means that once we found the winner, there is no need to continue. No other element will be able to appear more than the element that appeared more than half the time. Put another way, if a String A has appeared more than n/2 times, no other element B can appear more times than A: there are less than n/2 elements remaining.

With all this in mind, this is what you could have, that will be a lot more performant:

public static String getWinner(final List<String> listOfBallots) {
    int count = listOfBallots.size() / 2;
    Map<String, Integer> map = new HashMap<>();
    for (String current : listOfBallots) {
        int running = map.containsKey(current) ? map.get(current) + 1 : 1;
        map.put(current, running);
        if (running > count) {
            return current;
        }
    }
    return null;
}

For each element, we determine the number of times it appeared: if it already appeared (so the map contains the key), we add 1; else it is the first time.

Then when we found an element that appeared more than half the time, we return it directly.

With Java 8 constructs

Using Java 8, it is possible to use the merge method on the map and simplify a bit the code. Instead of having

int running = map.containsKey(current) ? map.get(current) + 1 : 1;
map.put(current, running);

we could just have:

int running = map.merge(current, 1, Integer::sum);

And if you're using Java 8, consider returning an Optional<String> instead of null:

public static Optional<String> getWinner(final List<String> listOfBallots) {
    int count = listOfBallots.size() / 2;
    Map<String, Integer> map = new HashMap<>();
    for (String current : listOfBallots) {
        int running = map.merge(current, 1, Integer::sum);
        if (running > count) {
            return Optional.of(current);
        }
    }
    return Optional.empty();
}


multiple api calls through promise update1:

- I need to make two different api calls.
- from the result of first api call I am getting id in the variable firstAPIid,
https://reqres.in/api/users?page=2
- I need to pass this id firstAPIid to the second api call.

can you review my code.
I did multiple api calls through promise.
right way is through promise or async or call back.
providing my code snippet and sandbox below.

https://codesandbox.io/s/redux-async-actions-xjdo7

<FetchButton
          onFetchClick={() => {
            store.dispatch(dispatchFunc => {
              dispatchFunc({ type: "FETCH_DATA_START" });
              axios
                .get("https://reqres.in/api/users?page=2")
                // axios.get('https://jsonplaceholder.typicode.com/posts')
                .then(response => {
                  console.log("response.data.data---->", response.data.data);
                  console.log(
                    "response.data.data[0].id---->",
                    response.data.data[0].id
                  );
                  //First of all we'll create the number of requestes base on the previous Response
                  const promises = response.data.data.reduce(
                    (previousValue, { id }) => {
                      previousValue.push(
                        axios.get(
                          `https://jsonplaceholder.typicode.com/comments?postId=${id}`
                        )
                      );
                      console.log(
                        " promises previousValue---->",
                        previousValue
                      );

                      return previousValue;
                    },
                    []
                  );

                  console.log("promises---->", promises);

                  //We use the built in function to fetch the data
                  axios.all(promises).then(responses => {
                    //Here you have all responses processed
                    const emailsMapped = responses.reduce(
                      (previousValue, { data }) => {
                        const emails = data.map(({ email }) => email);
                        previousValue.push(...emails);
                        return previousValue;
                      },
                      []
                    );
                    //You send the emails you want
                    dispatchFunc({
                      type: "RECEIVED_DATA",
                      payload: emailsMapped
                    });
                    console.log(emailsMapped);
                  });
                })
                .catch(err => {
                  dispatchFunc({ type: "FETCH_DATA_ERROR", payload: err });
                });
            });
          }}
        />

First of all, putting all your code in the props of your JSX is making it unreadable. Extract this into a function, and have your JSX reference that instead.

Next, for debugging, I would recommend using breakpoints to debug code. console.log() is fine for a quick peek, but it introduces a lot of noise in the code.

Also, when you expand a logged object in the console, you see the object's structure at the time of expanding, not at the time of logging. The object's contents may have already changed between the time it was logged and when you expanded it. If you're not aware of this quirk, you'll easily think your code is broken.

For promises, you're really just mapping. Use array.map() instead of array.reduce(). For emailsMapped, which is also just a mapping operation, since you're adding multiple items, use array.flatMap() for that.

Lastly, you can modify your code so that you can chain the promises instead of nesting them. You can return the promise of axios.all() to chain it to your axios.get().

store.dispatch(dispatchFunc => {
  dispatchFunc({ type: "FETCH_DATA_START" });
  axios
    .get("https://reqres.in/api/users?page=2")
    .then(response => {
      const promises = response.data.data.map(({ id }) => {
        return axios.get(`https://.../comments?postId=${id}`)
      })

      return axios.all(promises)
    })
    .then(responses => {
      const emailsMapped = responses.flatMap(({ data }) => {
        return data.map(({ email }) => email)
      })

      dispatchFunc({
        type: "RECEIVED_DATA",
        payload: emailsMapped
      })
    })
    .catch(err => {
      dispatchFunc({
        type: "FETCH_DATA_ERROR",
        payload: err
      })
    })
})

Now that we've established a more linear flow via chaining, we can take this a bit further and convert it to an async function so that we can use await:

// Note the async here
store.dispatch(async dispatchFunc => {
  dispatchFunc({ type: "FETCH_DATA_START" });

  try {
    // Note the various await before each asynchronous function call

    const response = await axios.get("https://reqres.in/api/users?page=2")

    const promises = response.data.data.map(({ id }) => {
      return axios.get(`https://.../comments?postId=${id}`)
    })

    const responses = await axios.all(promises)

    const emailsMapped = responses.flatMap(({ data }) => {
      return data.map(({ email }) => email)
    })

    dispatchFunc({
      type: "RECEIVED_DATA",
      payload: emailsMapped
    })
  } catch (err) {
    dispatchFunc({
      type: "FETCH_DATA_ERROR",
      payload: err
    })
  }
})


Multithreaded non-blocking tiny echo server I wrote this code to make a non-blocking server without using select(), my main concern is to catch as many exceptions as possible for worst case scenarios (so I would appreciate some feedback on cases I missed), and to let each client to be handled separately by its own thread.

Any feedback on style/performance/good or bad coding practice is welcome.

import socket
import logging
import threading
import sys

class EchoServer:
    BUFFER_SIZE = 1024

    # size limit in bytes for the client message to be received
    MAX_MSG_SIZE = 1024 * 5

    # this maps connected clients socket objects returned from accept() to their address tuple
    connected_clients = {}

    def __init__(self, port):
        self.hostname = 'localhost'
        self.port = port

        try:
            self.sockfd = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
            self.sockfd.bind((self.hostname, self.port))
            self.sockfd.listen(10)
            self.sockfd.setblocking(False)

        except socket.error as e:
            logging.critical(e)
            logging.critical('Could not start up server. Exiting.')
            sys.exit(-1)

    def look_for_connections(self):
        while True:
            try:
                client_connfd, client_addr = self.sockfd.accept()
                logging.info('Connection from client {!r}'.format(client_addr))
                self.connected_clients[client_connfd] = client_addr

                client_handling_thread = threading.Thread(target=self.handle_client, args=(client_connfd,))
                client_handling_thread.start()

            except BlockingIOError:
                # no active clients trying to connect, nothing to do
                continue

    def handle_client(self, connfd: socket.socket):
        msg = self.get_client_msg(connfd)
        logging.info('Client: {} sent {} bytes.'.format(self.connected_clients[connfd], len(msg)))

        sent_bytes_size = self.send_client_msg(connfd, msg)
        logging.info('Server sent {} bytes to client: {!r}'.format(sent_bytes_size, self.connected_clients[connfd]))

        del self.connected_clients[connfd]
        connfd.close()

    def startup_server_loop(self):
        # this starts the main event loop (accepting connections from client)
        # each client get handled by its own thread
        server_thread = threading.Thread(target= self.look_for_connections)
        server_thread.start()

    def get_client_msg(self, connfd: socket.socket):
        data = b''

        while True:
            try:
                buffer = connfd.recv(self.BUFFER_SIZE)
                if len(buffer) == 0 or len(data) >= self.MAX_MSG_SIZE:
                    break
                else:
                    data += buffer
            except BlockingIOError:
                break
        return data

    def send_client_msg(self, connfd: socket.socket, msg: str):
        sent_bytes = 0
        total_bytes = len(msg)
        total_sent = 0

        if len(msg) == 0:
            return total_sent

        while True:
            try:
                sent_bytes = connfd.send(msg[sent_bytes: total_bytes])
                total_sent += sent_bytes

                if sent_bytes == 0:
                    return total_sent
            except BlockingIOError:
                return total_sent

            # in case client disconnected before sending the echo
            except ConnectionResetError:
                logging.info('Client {!r} disconnected before sending echo.'.format(self.connected_clients[connfd]))
                return total_sent

    def __del__(self):
        for client in self.connected_clients.keys():
            del self.connected_clients[client]
        self.sockfd.close()

if __name__ == '__main__':
    logging.basicConfig(level=logging.INFO)
    my_server = EchoServer(50000)
    my_server.startup_server_loop()

There seems to be a few misconceptions about blockings calls and the purpose of non-blocking sockets. You wrote in a comment:

I thought this was a good idea for the server to be as responsive as possible for any number of clients connected (isn't that correct?).

Yes, this is a good idea, but no, your implementation does not make it faster. If anything, it will be slightly slower than using blocking sockets.
How is this slower?
Neither your server thread nor your handler threads does anything else than managing their socket. So there is absolutely nothing to be gained from having non-blocking sockets. Besides by using an active loop (while True without sleeping) your threads will eat-up 100% CPU and will never enter a state where it is favorable for the OS to deschedule it, leaving room for other threads, leading to other threads waiting longer before being scheduled.
This is normaly not an issue when a program can make use of multiple cores to spawn several threads concurently. But this is Python, and the GIL prevents us from having more than one thread at once. This means that as long as a thread is handling message from/to clients, the server thread is waiting to be scheduled. And the active loops are not helping for it to be scheduled faster.
But why non-blocking sockets in the first place?
Non-blocking sockets and/or sockets using timeouts are meant to be used in a context where there is other tasks to perform. Either because a single thread performs both the "server" and "handler" part:
def non_blocking_server(port, hostname='localhost'):
    try:
        sockfd = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
    except socket.error as e:
        logging.critical(e)
        logging.critical('Could not open socket. Exiting.')
        sys.exit(1)

    try:
        sockfd.bind((hostname, port))
        sockfd.listen(10)
        sockfd.setblocking(False)
    except socket.error as e:
        logging.critical(e)
        logging.critical('Could not start up server. Exiting.')
        sockfd.close()
        sys.exit(2)

    connected = {}
    receiving = {}
    sending = {}
    while True:
        with suppress(BlockingIOError):
            client, client_addr = sockfd.accept()
            logging.info('Connection from client {!r}'.format(client_addr))
            connected[client] = client_addr
            receiving[client] = b''

        still_receiving = {}
        for client, data in receiving.items():
            try:
                buf = client.recv(BUFFER_SIZE)
                if buf:
                    still_receiving[client] = data + buf
                else:
                    logging.info('Client: {} sent {} bytes.'.format(connected[client], len(data)))
                    sending[client] = (memoryview(data), 0)
            except BlockingIOError:
                still_receiving[client] = data
        receiving = still_receiving

        still_sending = {}
        for client, (message, sent_bytes) in sending.items():
            try:
                sent = client.send(message[sent_bytes:])
                sent_bytes += sent

                if sent:
                    still_sending[client] = (message, sent_bytes)
                else:
                    logging.info('Server sent {} bytes to client: {!r}'.format(sent_bytes, connected[client]))
                    client.close()
                    del connected[client]
            except BlockingIOError:
                still_sending[client] = (message, sent_bytes)
            except ConnectionResetError:
                # in case client disconnected before we send the echo
                logging.info('Client {!r} disconnected before sending echo.'.format(connected[client]))
                logging.info('Server sent {} bytes to client: {!r}'.format(sent_bytes, connected[client]))
                client.close()
                del connected[client]
        sending = still_sending

It would be a shame if a message from the first client was partially received and we would need to wait for a second client to connect before being able to receive and handle the rest. Here non-blocking sockets allow us to perform all three operations (accepting connections, receiving messages, sending messages) sequencially in a single thread without being dependent on external resources (such as client connection or network availability).
An other use case would be to check for incomming connection and/or handle them alongside more heavyweight computation in a loop. It allows to fine tune when things are done instead of relying on the OS to schedule threads for us.
Using threads as non-blocking facility.
So, after having said that non-blocking sockets in threads is bad, let's analyze the alternative of having blocking sockets in threads. In this scenario each thread is suspended while waiting for the blocking system call to finish. This has 2 advantages:

A thread no longer uses 100% CPU while waiting for an event;
The OS can more easily give priority to actually active threads and/or resume threads whose system call just finished faster.

And since you’re using threads, the OS is free to give resources where they are needed which usually manifests itself as "schedule the thread whose blocking call just finished".
Using threads also call for more readable code as it usually will make you write separate entities for separate kind of actions (a server and a handler, for instance).
Misconception of the "blocking" part of sending/receiving data
From the code you wrote, it seems that you think you will get a BlockingIOError if no more data can be read/sent. This is false, it only means that the network is not (yet) available.
In case of a read, a BlockingIOError can mean that:

the client didn't start sending data;
the client stopped sending data (but may be sending more in the future);
the data has been sent by the client but is not yet arrived on the server;
who knows.

When reading data, you have a few possibilities to know when to stop reading but it's a protocol that must be established between the client and the server:

the client can shutdown its write end of the socket, then the server will be notified by a recv that doesn't block but return 0 bytes;
the client can send a fixed-sized payload as a header to its message indicating the length of the incomming message;
the client can send a fixed-length message, filling missing data with whatever makes sense;
the client can end its message with a specific delimiter;
…

In case of a send a BlockingIOError is usually a sign that the network interface is used by another thread and that you should wait before sending your data.
Code review
With that in mind, you might have understood that your get_client_msg and send_client_msg are not quite doing the proper thing. But that's not an issue since we’re removing the non-blocking part of the sockets anyway.
You also have a few functions pertaining to handling the client socket where you pass said socket as parameter. The only reason that you left them in the EchoServer class is to be able to use self.connected_clients[connfd]. Instead, create a new class dedicated to handling a client, and pass the address as a parameter of this class alongside the socket.
Also, to simplify thread management, both classes can inherit from threading.Thread. A first rewrite would look like:
import socket
import logging
import threading
import sys

class EchoHandler(threading.Thread):
    BUFFER_SIZE = 1024
    # size limit in bytes for the client message to be received
    MAX_MSG_SIZE = 1024 * 5

    def __init__(self, sockfd, address):
        super().__init__()
        self.sockfd = sockfd
        self.address = address

    def run(self):
        msg = self.get_client_msg()
        logging.info('Client: %r sent %d bytes.', self.address, len(msg))

        sent_bytes_size = self.send_client_msg(msg)
        logging.info('Server sent %d bytes to client: %r', sent_bytes_size, self.address)

        self.sockfd.close()

    def get_client_msg(self):
        data = b''
        while True:
            buffer = self.sockfd.recv(self.BUFFER_SIZE)
            data += buffer
            if not(buffer) or len(data) >= self.MAX_MSG_SIZE:
                return data

    def send_client_msg(self, msg):
        total_sent = 0
        message = memoryview(msg)

        while True:
            try:
                sent_bytes = self.sockfd.send(message[total_sent:])
                total_sent += sent_bytes

                if sent_bytes == 0:
                    return total_sent
            except ConnectionResetError:
                # in case client disconnected before we send the echo
                logging.info('Client %r disconnected before sending echo.', self.address)
                return total_sent

class EchoServer(threading.Thread):
    def __init__(self, port, hostname='localhost'):
        super().__init__()

        try:
            self.sockfd = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
        except socket.error as e:
            logging.critical(e)
            logging.critical('Could not open socket. Exiting.')
            sys.exit(1)

        try:
            self.sockfd.bind((hostname, port))
            self.sockfd.listen(10)
        except socket.error as e:
            logging.critical(e)
            logging.critical('Could not start up server. Exiting.')
            self.sockfd.close()
            sys.exit(2)

    def run(self):
        while True:
            client_connfd, client_addr = self.sockfd.accept()
            logging.info('Connection from client %r', client_addr)
            self.connected_clients[client_connfd] = client_addr

            EchoHandler(client_connfd, client_addr).start()

    def __del__(self):
        self.sockfd.close()

if __name__ == '__main__':
    logging.basicConfig(level=logging.INFO)
    EchoServer(50000).start()

But this is starting to look a lot like socketserver:
import logging
import socketserver

class EchoHandler(socketserver.BaseRequestHandler):
    BUFFER_SIZE = 1024
    # size limit in bytes for the client message to be received
    MAX_MSG_SIZE = 1024 * 5

    def finish(self):
        self.request.close()

    def handle(self):
        msg = self.get_client_msg()
        logging.info('Client: %s sent %d bytes.', self.client_address, len(msg))

        sent_bytes = self.send_client_msg(msg)
        logging.info('Server sent %d bytes to client: %r', sent_bytes, self.client_address)

    def get_client_msg(self):
        data = b''
        while True:
            buf = self.request.recv(self.BUFFER_SIZE)
            data += buf
            if not buf or len(data) >= self.MAX_MSG_SIZE:
                return data

    def send_client_msg(self, msg):
        total_sent = 0
        message = memoryview(msg)

        while True:
            try:
                sent_bytes = self.request.send(message[total_sent:])
                total_sent += sent_bytes

                if sent_bytes == 0:
                    return total_sent
            except ConnectionResetError:
                # in case client disconnected before we send the echo
                logging.info('Client %r disconnected before sending echo.', self.client_address)
                return total_sent

class EchoServer(socketserver.ThreadingTCPServer):
    allow_reuse_address = True

    def process_request(self, request, client_address):
        logging.info('Connection from client %r', client_address)
        super().process_request(request, client_address)

if __name__ == '__main__':
    logging.basicConfig(level=logging.INFO)
    with EchoServer(('localhost', 50000), EchoHandler) as server:
        server.serve_forever()

This is more to the point because all the boilerplate is already handled by Python.


bash input parameter logic The following works fine and I'm not having to repeat any commands once logic is evaluated. However, it feels as though I'm doing a lot of seperate checks. I'm wondering if I could do this more efficiently?

#!/bin/bash

#The script can have 0, 1 or 2 input parameters
#parameter $1 can be one of 4 values ('foo','bar','chu','test')
#parameter $2 can only be 'test' and only if $1 is present and not 'test'

if ( [ $# == 1 ] && [ $1 == 'test' ] ) || ( [ $# == 2 ] && [ $1 != 'test' ] && [ $2 == 'test' ] ); then
    echo 'enable testing'
fi

if [ $# == 0 ] || ( [ $# == 1 ] && [ $1 == 'test' ] ); then
    echo 'do everything'
    exit 0
fi

if ( [ $# == 1 ] && [ $1 != 'test' ] ) || ( [ $# == 2 ] && [ $2 == 'test' ] ); then
    case "$1" in
        'foo')
            echo 'only do foo'
            exit 0
            ;;
        'bar')
            echo 'only do bar'
            exit 0
            ;;
        'chu')
            echo 'only do chu'
            exit 0
            ;;
        *)
            echo 'not valid'
            exit 1
    esac
else
    echo 'not valid'
    exit 1
fi

exit 0

You can achieve the same but simpler if you reorder the validation steps.

If there are two arguments and both are test, that's invalid:

[ "$1" = test ] && [ "$2" = test ] && invalid

Where invalid is a helper function:

invalid() {
    echo not valid
    exit 1
}

If the second argument is test, then enable testing,
or else if the first argument is test then enable testing and shift the arguments:

if [ "$2" = test ]; then
    echo enable testing
elif [ "$1" = test ]; then
    echo enable testing
    shift
fi

At this point, the should be 0 or 1 arguments, not more.
That was a missing validation step in the original code:

[ $# -le 1 ] || invalid

If we reach this point,
there must be 0 or 1 arguments.
The original case statement can come here as before, without any further if statements. It can use the invalid helper function in the invalid case.


Fastest Windows script to move files from one did to a network dir I wrote a script that moves files older than two days over to a network share (via scheduled task). Windows Task Scheduler doesn't seem to like the script so much, but it works well by itself.

Is there a faster way than what I have?
Is there a way to programmatically make it a scheduled task or service of some type?

Backup.ps1

$today = get-date
$tda= $today.addDays(-2) #two days ago

$files = get-childitem d:\source_path | select fullname, lastwritetime | where-object{$_.lastwritetime -lt $tda}

foreach ($file in $files) {
  Echo "moving " $file.fullname #just so I know it's still running, get rid of this?
  mv $file.fullname 'n:\dest_folder'
  Echo 'done'
}

Okay, I'll start by trying to answer your questions first.

If there is a lot of files and you should look into using runspaces to run several copy-jobs in separate threads. Just be careful, you are still limited by the bandwidth of the network.
We need to see some kind of error to be certain, but I would start by using PowerShell commands instead of old dos commands.

Which leads me to the second part of this answer. The actual code review :)

First thing; if you are going to write a script in PowerShell, use PowerShell commands :) Replace echo with Write-Host and replace mv with Copy-Item.

Second: you don't need | select fullname, lastwritetime. It probably just adds (a little bit of) latency to the execution of the script.

As others have pointed out, you should add some error handling and thinking about what would happen if the destination folder already have a file with the same name. Using -Force is one way, but would overwrite the existing file. So if you don't want this, you will have to handle it differently.

Lastly, if you are new to PowerShell, start commenting your code. You'll thank yourself when you go back to review your scripts 6 months from now :)

Oh, by the way, If you read up on Copy-Item, there is a -Filter parameter that might make the whole Get-ChildItem part of the script redundant.