Query a product from a list of brands I am quite new to F# and just wrote my first program. It checks if the query exists in the brand list and returns the matching brand.

Query is the string you are searching for in the brand list. So someone could be looking for "Miso Power Washer X1000" the idea is that my function analyses the query to see if there is a matching brand. Later the remainder ("Power Washer X1000") could be parsed as well. In this way I am able to "parse" the query of the user and give it context. This meta data will be used to give better search results when the database is queried.

I used stringInArray in order to split the logic into a separate function. Of course I could do something like Array.filter(fun (elem: string) -> query.IndexOf(elem) > -1)) brands But I felt it was less readable.

Sometimes when learning a new language you get things to work. However someone more experienced say: you can use this or that which is more common, faster etc. I am looking for this kind of feedback before creating a bunch of code which turns out to be rubbish through the eyes of an experienced F# programmer.

Is this the correct way to use F# function structure?

Code

module Program =

    [<EntryPoint>]
    let main argv =
        let query = "Miso Power Washer X1000"

        let brands = [|
            "Hayo"
            "Miso"
            "The Master"
            "Vector"
        |]

        let stringInArray = fun (elem: string) ->
            query.IndexOf(elem) > -1

        let getBrands (query: string): string[] =
            Array.filter(stringInArray) brands

        let result = getBrands query
        printfn "%A" result

        0

Output

[|"Miso"|]

Unfortunately, there is at least one blatant issue with the program in its current state: unused arguments. Let's have a look at it and find out why that's important.

Before that, a short disclaimer, though: I'm not a F# developer. However, I know functional programming (e.g. Haskell). I don't know the .NET lands by heart. Take this review with a grain of salt on the arguments that concern F#.

Functions and the world

When we write a function in a context, it gains knowledge of that context. For example, we bound five to addFive in the following example:

let y = 5
let addFive x = x + y

However, there is a possible issue with the code above: we might accidentally use y at a place we didn't intend, for example:

let add x z = x + y

This is exactly what happened in your functions:

        let stringInArray = fun (elem: string) ->
            query.IndexOf(elem) > -1

        let getBrands (query: string): string[] =
            Array.filter(stringInArray) brands

Note how stringInArray just uses query? And how getBrands completely ignores the given query? This means that we could use let result = getBrands "" and still end up with [|"Miso"|]. That's not what we intended!

Instead, let's go back to back to the drawing board. We need to make sure that the query gets used. So we need to add at least one argument to stringInArray:

        let stringInArray (query: string) (elem: string) =
            query.IndexOf(elem) > -1

Now we can use query in getBrands:

        let getBrands query =
            Array.filter(stringInArray query) brands

Great! Now let result = getBrands "" leads to an empty array. Success!

Names and tales

However, now that we changed stringInArray, we note that the name isn't quite fitting: if we add type signatures, we and up with:

        let stringInArray (query: string) (elem: string) =
            query.IndexOf(elem) > -1

Neither of the arguments is an array. We should call this function contains or similar. However, we could introduce another function that gets matching elements from an array:

        let isSubstringOf (haystack: string) (needle: string) =
            haystack.IndexOf(needle) > -1

        let matchingElements arr haystack =
            Array.filter(isSubstringOf haystack) arr

        let getBrands query = matchingElements brands query
        // or even
        //  getBrands = matchingElements brands

Note that with this approach we can keep the definition of getBrands to a minimum:

        let getBrands = matchingElements brands

General purpose functions and the world

Now that we used proper naming and split the functionality of our functions, it's time to re-evaluate whether they really belong in main. Remember how functions have their context saved? They provide a closure. It's therefore a good idea to keep the context small.

What functions should we therefore move out of main? We have the following at hand:

isSubstringOf, which searches in a string for another string
matchingElements, which filters an array of strings whether they are contained in the second argument
getBrands, which filters brands given a query

The first two functions sound very generic, so let's move them out of main:

module Program =

    let isSubstringOf (haystack: string) (needle: string) =
        haystack.IndexOf(needle) > -1

    let matchingElements arr haystack =
        Array.filter(isSubstringOf haystack) arr

    [<EntryPoint>]
    let main argv =
        let query = "Miso Power Washer X1000"

        let brands = [|
            "Hayo"
            "Miso"
            "The Master"
            "Vector"
        |]

        let getBrands = matchingElements brands

        let result = getBrands query
        printfn "%A" result

        0

Note how short our main got. It only contains the essential elements: query, brands, getBrands and result. One could argue that we can just replace getBrands by its definition, but premature brevity in source code is the source of future confusion, so let's keep it a little bit more verbose but self-explanatory.

Moving the functions might seem like an overkill, but note that this approach immediately had shown an error if we followed it right from the beginning. If we now use we now use query accidentally in isSubstringOf, we immediately get a compiler error (and probably an IntelliJ warning/note/error). That can be a huge boon in finding errors!

Furthermore, this approach makes it easy to unit test the functions later. Maybe we want to improve the isSubstringOf to use fuzzy logic so that it also works for "Mizo Power Washer". (Unit) tests can make sure that we don't accidentally break old functionality on the way.


Execute script on remote host and obtain variables on localhost This executes a predefined script on a remote host, and somehow retrieve variable values without affecting STDOUT and STDERR.

My idea was to encode the data in JSON, write it to a file, then read the data in a separate request.

The cons of this method are that:

I would need to add the following code (file_put_contents) to the bottom of every script.
It requires a separate request (albeit, a pretty light request)

This is a dumbed down example, but inside the script to be run remotely:

<?php
$data = array(...);
$json = json_encode($data);
file_put_contents('tmp.json', $json);

On the local side:

<?php
include('Net/SSH2.php');

$ssh = new Net_SSH2('www.domain.tld');
if (!$ssh->login('username', 'password')) {
    exit('Login Failed');
}

echo $ssh->exec('php script.php');
$json = $ssh->exec('cat tmp.json');

I'm just looking for some input or alternative ways of doing this.

This has numerous problems with it, one of which is the race condition. Imagine two clients connect to the server at the same time. Which client's tmp.json wins? The other client gets bad data.

Another problem with the current way of doing things is that you start a new PHP interpreter for each request. So, it's not as lightweight as you're envisioning.

You really need to develop a protocol for sending the two data streams back, and then implement that protocol. A temporary file should not be necessary.


Implementing quicksort with pivot of first, median or random I am attempting to implement the quick sort algorithm whereby the pivot will be chosen by the user, and can either be median, first, or random. I am supposed to return a long that measures the number of times comp.compare() is invoked. I am wondering if you could review for me.

/**
     * The quick sort
     * @param arr the collection array
     * @param comp is the comparator
     * @param pivottype specifies where the pivot is going to be at, can either be "first", "middle", or "random"
     * @return returns the number of comparisons performed during the execution of the sorting algorithm
     */
    public static <E> long quickSort(E[] arr, Comparator<? super E> comp, String pivottype)
    {
        recursiveQuickSort(arr,0, arr.length -1,  pivottype, cp );
        return quickSortCounter;

    }
    private static<E> void recursiveQuickSort(E[] array, int first, int last, String pivottype, Comparator<? super E> comp)
    {
        if(first < last)
        {
            int pivotPosition = partition(array, first, last, cp, pivottype);
            if(pivotPosition > 1)
                recursiveQuickSort(array, first, pivotPosition-1, pivottype, cp);
            if(pivotPosition + 1 < last)
                recursiveQuickSort(array, pivotPosition + 1, last, pivottype, cp);
        }

    }
    private static <E> int partition(E[] collectionArray, int firstIndex, int lastIndex, Comparator<? super E> comp, String pivottype)
    {
        int pivotLocation;
        // counts the number of times the cp.compare() is called
        quickSortCounter = 0;

        if(pivottype.equals("first"))
        {
            pivotLocation = firstIndex;
        }
        else if(pivottype.equals("middle"))
        {
            pivotLocation = (firstIndex + lastIndex)/2;
        }
        else
        {

            pivotLocation = rand.nextInt(lastIndex - firstIndex) + firstIndex;
        }

        E pivotElement = collectionArray[pivotLocation];

        while(firstIndex <= lastIndex)
        {
            //scans right;
            while((firstIndex <= lastIndex) && (cp.compare(collectionArray[firstIndex], pivotElement) == -1))
            {
                firstIndex ++;
                quickSortCounter++;

            }
            //scans left
            while((lastIndex >= firstIndex) && (cp.compare(collectionArray[lastIndex], pivotElement) == 1) ||
                    (lastIndex >= firstIndex) && (cp.compare(collectionArray[lastIndex], pivotElement) == 0))
            {
                lastIndex--;
                quickSortCounter++;
            }
            if(firstIndex >  lastIndex)
            {
                swap(collectionArray, pivotLocation, lastIndex);
            }
            else
            {
                swap(collectionArray, firstIndex, lastIndex);
            }

        }
        return lastIndex;

    }
    private static <E> void swap(E[] array, int i, int j)
    {
        E temp = array[i];
        array[i] = array[j];
        array[j] = temp;
    }

}

quickSortCounter is reset too often
If the goal of the program is to count compares, the counter you are using is being reset in the wrong place.  Right now, you are going to end up with only the number of compares in the last call to partition(), which will likely be close to 0.  You need to move the quickSortCounter = 0 line to right before you call recursiveQuickSort().  That way you will add up all the compares in the entire sort.
You can also do it a different way without using a static variable.  You just need to have partition() be able to return the count to its caller.  One way of doing this would be to have partition() return an int [2] (one int for the partition index and the other int for the number of compares).  Another way is to pass an int [1] or Integer variable into partition() and write the compare count into that variable before returning.
Naming
One thing confused me about your variable names.  You used array as you array everywhere, but then you called it collectionArray in partition().  Why "collection"?


Validator class for PHP I've made this validation class in PHP that would take inputs, on initialization sanitize them and validate them specific to the type. It works fine, but I would like to know if there's some bad practice involved and how I can improve it.
I would also love to hear if there's anything that makes the code awful or hard to read. Does it follow good coding standards? If not, how can I improve it?

Class Validate {
    //Take the user input into the data put it to a class.
    private $data; //Function argument
    private $err; // boolean
    public $empty;
    private $errMsg;

    public function __construct($data) { //Not sure about this one. Have to check it out later and decide

        //perform some basic level validation like checking for emptiness.
        foreach ($data as $k =>$v) {
            $data[$k]= $v;
            if($this->checkEmpty($v)==1) {
                $this->empty=true;

            }
        }

    }

    private function checkEmpty($data) {
        if($data==""|| empty($data) || !isset($data)) {
    return 1;
        }
    }
    private function basicSanitize($data) { //Basic level input santization to be used by other functions only.

    $data=htmlspecialchars($data);
    $data=trim($data);
    $data=stripslashes($data);

    return $data;
    }

    public function sanitize($data) { // Sanitization at a massive level to sanitize a lot of inputs at one go.
        foreach ($data as $k =>$v) {
            $this->data[$k]= $this->basicSanitize($v);
            }

            return $this->data;
    }

    public function name($data) {

        if(strlen($data)<3) {

            $this->errMsg = "Name too short.";
        }

        else if(strlen($data)>100) {
            $this->errMsg = "Name too big.";
        }

        if(isset($this->errMsg)) {
            return $this->errMsg;
        }

        else return true;
    }

    public function email($data) {
        if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
             $this->errMsg = "Invalid email format";
        }

        if(isset($this->errMsg)) {
            return $this->errMsg;
        }

        else return true;
    }

}

And here is calling the class:

    $name="J";
    $email="mephone@2d.com";

    $input= array (
        'name'=>$name,
        'email'=>$email
        );

    $val=new Validate($input);
    if($val->empty==false) {

        $clean=$val->sanitize($input);

        $nameErr = $val->name($clean['name']);
        if($nameErr)  {
        echo $nameErr;
        }

        else echo $clean['name'];

Should I be calling the error messages like that? What could be a more systematic and better way to do that?

Sanitize Functions

First of all, note that your class does two very different things:

It checks if data is valid, returning either true or false (name, email).
It changes the data so that it is valid (sanitize).

Combining both of these functionalities in the same class can get confusing quickly.

construct

Why does the constructor do sanitation? You already have basicSanitize, shouldn't that be responsible? And where is the result actually checked?

basicSanitize

The name alone already suggests that this isn't a great function. It's so generic, this function could be doing anything.

Let's look at what it is doing (adapted from my answer here, as these kinds of functions are quite often combined, even though they shouldn't be):

trim: removes whitespace from beginning and end of string. This might be useful in some situations, and doesn't hurt in too many situations. It doesn't really affect security.
stripslashes: as the name suggests, it removes slashes. This is only useful if magic quotes is on (which is the functionality that adds slashes), so you should check that first, as otherwise you may corrupt your data. If magic quotes is on (it shouldn't be), using stripslashes is recommended, as otherwise you will have corrupted data.
htmlspecialchars again, this might corrupt your data (as in, you are not saving your original data in the database, but changed data). It is proper defense against XSS in most (but not all) situations (see here for exceptions, and use ENT_QUOTES), but it really should not happen before inserting data into the database, for two reasons: 1. corrupted data 2. This approach makes it easy to introduce XSS vulnerabilities. XSS is a vulnerability that is introduced when echoing data, so that is when it should be prevented. Otherwise, it will be hard to remember which data is secure and which is not.

To summarize: Your approach changes input data before it is inserted into the database, which could impact usability. This doesn't even add that much security, and is certainly not considered a best practice approach to security.

name

This functions doesn't really have anything to do with names. I'm glad that you are not too restrictive, but really, this is nothing more than a length check, so it might as well be called checkLength($data, $min, $max).

email

This is fine. FILTER_VALIDATE_EMAIL doesn't follow any email specs, but as it's PHPs inbuilt filter, it seems ok to use it.

Style / Coding Standards

I'm glad that you asked about this, as there are a couple of issues:

Your indentation is off, making your code difficult to read.
use curly brackets for all statements, even one-line statements.
Your spacing is off.
Your function level comments could be PHPDoc comments.

Use any IDE to fix most of these issues automatically.

Structure

Your class has state, but doesn't really need it, as your private fields are never used across functions. So they are just making your class unnecessarily complex.

You can just make your functions static. For example, your email function could just as well look like this:

public function email($data) {
    if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
         return "Invalid email format";
    }
    return true;
}

And your name function:

public function name($data) {
    if(strlen($data) < 3) {
        return "Name too short.";
    } else if(strlen($data) > 100) {
        return "Name too big.";
    }
    return true;
}

API

Your class is very awkward to use. I imagine it works something like this:

$validate = new Validate($_GET); // this seems to make sense, but actually will not do anything
$validatedName = $validate->name($_GET['name']);
if (!$validatedName) {
    // handle $validatedName, which contains an error message
} else {
    // everything is fine
}

You'll notice that this doesn't actually work!

The reason for this is that strings as well as true evaluate to true with weak comparison. So instead, I'll need $validatedName !== true. And I will need to remember to use strict comparison each time, as I will otherwise have a - possibly quite serious - bug in my code.

The main problem is that you are sending error messages via the same mean as your return value, which just complicates things.

There are three different solutions that come to mind:

get rid of the error message and just return true or false. For many filters this will make perfect sense (if the email filter failed, it's because the input isn't an email, no need to explain).
store the error message in a field, which can later be accessed if needed.
throw an exception containing the error message on failure.


Excel VBA code midStr() function using left and right position instead of length used with mid() In the early days of Basic (1970's when i started coding) the midStr() function used left and right positions directly rather than left and length we now use with mid(). I always found it more efficient in my mind to code using the old midStr() method, so I finally just created some code to do it. It's certainly not as efficient as mid(), and if I am dealing with lots of data I'd use mid() directly instead, but for simplicity, I do like the old way better, it's more direct to simply code rightPos rather then rightpos - leftPos + 1, and to test for null.
I also made the rightPos optional, and when not used, the length of the textin is used instead, so it will simply return the remainder of the text starting at the leftPos.
Any kind critique is welcome, thank you.
Function midStr(ByVal textin As String, _
                ByVal leftPos As Long, _
       Optional ByVal rightPos As Variant) As Variant
   'midStr returns textin string using leftPos and rightPos (rather than num_chars).
   'rightPos is optional, len(textin) is used for the rightPos when it's not otherwise defined.

   On Error GoTo midStrErr
   If IsMissing(rightPos) Then rightPos = Len(textin)
   If rightPos < leftPos Then
      midStr = vbNullString
   Else
      midStr = Mid(textin, leftPos, rightPos - leftPos + 1)
   End If
   Exit Function

midStrErr:
   On Error Resume Next
   midStr = CVErr(xlErrValue) '#VALUE!
End Function

While the Optional ByVal rightPos As Variant is a cool idea because you can use the IsMissing function, it introduces unnecessary errors. For example, Debug.Print midStr("Hello World", 1, Nothing) will print "Error 2015" in the Immediate window. That line should not be allowed to compile.
I would declare the rightPos parameter as Long. The compiler will take care of some of the issues - for example you won't be able to pass an Object. The Long will default to 0 so you can replace If IsMissing(rightPos) Then rightPos = Len(textin) with If rightPos = 0 Then rightPos = Len(textin).
The generally accepted convention in VBA is that method names are written with PascalCase and variable names are written with camelCase. Moreover, an Excel User Defined Function (UDF) is usually written with UPPERCASE.
This function seems to be designed as an Excel UDF. What if you need to use it in VBA? The Excel errors mean nothing to another VBA method. I would write this as a separate VBA only function that will work in any VBA-capable application host (no need to return a Variant):
Public Function MidStr(ByVal textin As String, _
                       ByVal leftPos As Long, _
              Optional ByVal rightPos As Long _
) As String
    Const methodName As String = "MidStr"

    If leftPos < 0 Then Err.Raise 5, methodName, "Invalid left position"
    If rightPos < 0 Then Err.Raise 5, methodName, "Invalid right position"

    If rightPos = 0 Then rightPos = Len(textin)
    If rightPos < leftPos Then
        MidStr = vbNullString
    Else
        MidStr = VBA.Mid$(textin, leftPos, rightPos - leftPos + 1)
    End If
End Function

I would then have an Excel UDF:
Public Function MID_STR(ByVal textin As String, _
                        ByVal leftPos As Long, _
               Optional ByVal rightPos As Long _
) As Variant
    On Error GoTo ErrorHandler
    MID_STR = MidStr(textin, leftPos, rightPos)
Exit Function
ErrorHandler:
    MID_STR = CVErr(xlErrValue)
End Function

This allows the use of MidStr function in other VBA methods and the MID_STR is only going to be called via the Excel interface.


Trying to Find a Loop to use for a specific MySQL query I'm trying to see if there is a loop I can use in order to pull off this code a little more elegantly. I was previously using a foreach($row as $cell) with $row = mysqli_fetch_row($result) but because of that I couldn't access the data from the 'user' column in order to check it against the session variables in the if statment. Here's the code:

$sql = "SELECT * FROM tbl_name ORDER BY subject, description LIMIT {$startpoint},{$limit}";
$result = $mysqli->query($sql);
$num_rows = mysqli_num_rows($result);

if($num_rows>0){
$field_num = $mysqli->field_count;
echo "
HERE ARE SOME RESULTS:
";
echo "
";

while($row = mysqli_fetch_array($result))
{
    echo"
";
    echo"".$row['subject']."	";
    echo"".$row['description']."	";
    echo"".$row['user']."	";
    if($row['user'] == $_SESSION['firstname']." ".$_SESSION['lastname']){
    echo"You can delete this	";
    }
    else{
    echo"Code didn't work	";
    }
    echo "
\n";
}
mysqli_free_result($result);
}
else{
echo 'There are no results!';
}
?>

Is there a loop I can run that can also allow me to access data from each row so I don't have to print out a new line of code for each column but keep the if statement in tact? Thanks!

Saving lines of code in search of elegance is not always (in fact hardly ever in my experience) a worthwhile exercise if it comes at the expense of clarity and maintainability. Your current code is better than any solution that iterates over table columns because it is clear and easy to change. For example, combining the data of two columns into one or changing column order are both trivial.

You might consider moving this display logic to a template. Here is an example of a twig template:

{% if results %}

HERE ARE SOME RESULTS

    {% for row in results %}

        {{ row.subject }}
        {{ row.description }}
        {% if row.user == user %}
        You can delete this
        {% else %}
        Code didn't work
        {% endif %}

    {% endfor %}

{% else %}

No results

{% endif %}

The PHP becomes very simple:

$bindings = array(
    'results' => $mysqli->query('SELECT...'),
    'user' => $_SESSION['firstname'] . ' ' . $_SESSION['lastname']
);
$twig->render('template.html', $bindings);


OR vs UNION in message tables I have a dilemma. I have a messages table containing fields id, sender, receiver, body I need to select all users a given user has had a conversation with. A conversation may be a single message without a response.

Here's a show create table for you to see the indexes (as I may have set them up incorrectly):

CREATE TABLE `messages` (
  `id` int(10) unsigned NOT NULL AUTO_INCREMENT,
  `sender` int(10) unsigned NOT NULL,
  `receiver` int(10) unsigned NOT NULL,
  `body` text,
  `datetime` datetime DEFAULT NULL,
  `seen` enum('0','1') NOT NULL DEFAULT '0',
  PRIMARY KEY (`id`),
  KEY `message_receiver` (`receiver`),
  KEY `msg_snd` (`sender`),
  CONSTRAINT `msg_rcv` FOREIGN KEY (`receiver`) REFERENCES `users` (`id`) ON DELETE NO ACTION ON UPDATE CASCADE,
  CONSTRAINT `msg_snd` FOREIGN KEY (`sender`) REFERENCES `users` (`id`) ON DELETE NO ACTION ON UPDATE CASCADE
) ENGINE=InnoDB AUTO_INCREMENT=7 DEFAULT CHARSET=utf8

Here is basically the simplest way I could come up with to get the task done

SELECT DISTINCT
    (
        CASE SENDER
        WHEN 15 THEN
            receiver
        ELSE
            CASE receiver
        WHEN 15 THEN
            sender
        END
        END
    ) AS `user`
FROM
    messages
WHERE
    sender = 15
OR receiver = 15

However, I know that MySQL cannot take full advantage of indexes with OR comparisons (may be wrong), so I came up with an alternative of two queries which can both benefit from the indexes:

SELECT DISTINCT
    `user`
FROM
    (
        SELECT
            DISTINCT sender AS `user`
        FROM
            messages
        WHERE
            receiver = 15
        UNION
            SELECT
                DISTINCT receiver AS `user`
            FROM
                messages
            WHERE
                sender = 15
    ) AS conversations

Which one of these approaches is better in terms of performance and complexity?

Use OR
You should make it a habit to look at your queries' execution plan when in doubt. I recreated the table in SQL Fiddle (albeit, without containing data) and it shows that the query with OR is less complex.
Please note: SQL Fiddle does not support your FOREIGN KEY constraints, so I had to remove them.
First query with OR:

Second query with UNION:

As you can see, the complexity of the 2nd query is higher, which might make it slower (or faster!), albeit the direction (faster or slower) and how much difference it will have depends on a lot of other things too.
In MySQL (and many other DBMS), it's actually (and counter-intuitively) the UNION ALL is usually more efficient than the OR version. Of course, that's because the UNION ALL does not have to remove duplicate rows. In many cases, including this one, we have use UNION (so UNION DISTINCT).

The edit below has been suggested by @ypercube, from dba.SE, and voluntarily added by OP:
Also note that your second query has a unnecessary level and an extra DISTINCT that is redundant which will almost certainly drag efficiency down. Here are two simplifications:
 -- query 2b:  SELECT DISTINCT - UNION DISTINCT - SELECT DISTINCT
SELECT
    DISTINCT sender AS `user`
FROM
    messages
WHERE
    receiver = 15
UNION
    SELECT
        DISTINCT receiver
    FROM
        messages
    WHERE
        sender = 15 ;

-- query 2c: SELECT ALL - UNION DISTINCT - SELECT ALL
SELECT
    sender AS `user`
FROM
    messages
WHERE
    receiver = 15
UNION
    SELECT
        receiver
    FROM
        messages
    WHERE
        sender = 15 ;


Banking: Calculate the Capital Requirement to calculate Risk-Weighted Assets I know it's not the perfect code, but it outputs the correct values. Could anyone give me some pointers on how to make the operation more Pythonic?

def capital_requirement(r, lgd, pd, madj, tenor):
    '''Returns the capital requirement.'''
    madjcoeff = 1.5
    tsub = 2.5
    a1 = (stat.norm.ppf(pd) + r**0.5 * stat.norm.ppf(0.999)) / ((1 - r)**0.5)
    b1 = lgd * stat.norm.cdf(a1) - lgd * pd
    return b1 * (1 + ((tenor - tsub) * madj)) / (1- madjcoeff * madj)

See:
https://en.wikipedia.org/wiki/Capital_requirement

The function name is very nice. The docstring adds nothing to that. Consider describing one or more arguments, especially if there are tricky details about their validity. Definitely add a URL describing the math approach.

Sorry, I didn't notice how the wikipedia page on Capital_requirement relates to the details of the formula.

Overall the code is clear. If you really wanted to delete the b1 temp variable for some reason you might write this two-liner:

return ((lgd * stat.norm.cdf(a1) - lgd * pd)
        * (1 + ((tenor - tsub) * madj)) / (1- madjcoeff * madj))

(I suppose lgd could be factored out, if desired.) In any event, the a1 temp definitely helps with readability, you'll want to hang on to that. Something to keep in mind is that subexpressions can raise exceptions. If you're concerned that might happen, it may be useful to break out several temp variables, each on its own line, so the stack trace will pinpoint where things went south.

tsub is a perfectly nice concise name to choose, but please add a comment:

tsub = 2.5  # tenor subtrahend

Not sure how "constant" these two are, but you might allow callers to change them:

def capital_requirement(r, lgd, pd, madj, tenor, madjcoeff=1.5, tsub=2.5):

It's pretty clear that stat.norm.ppf(0.999) is not implementing the .99 given in your formula.

You might validate r:

assert 0 < r < 1

If out-of-range values sneak in, be aware that math.sqrt() will raise ValueError, which you probably want, while ** 0.5 will return an imaginary root.


Create a generator that returns every permutation (unique arrangement) of positive integers up to n I watched this video from The Unqualified Engineer on YouTube and tried out the challenge. The challenge (or interview question) is to build a generator that returns all the permutations of the positive integers up to \$n\$ (that is, \$1,2,3,\ldots,n\$).

I know about itertools.permutations but want to implement it myself.

def gen_perm(n):
    l=tuple(range(1,n+1))
    yield l
    while True:
        l=increment_lex(l)
        if(not l):
            raise StopIteration
            break
        yield l

def increment_lex(l):
    for i in range(len(l)-1):
        x=len(l)-(i+1)
        y=len(l)-(i+2)
        if l[x] > l[y]:
            m=min([i for i in l[x:] if i > l[y]])
            cl=list(l[x:])
            cl[cl.index(m)] = l[y]
            il=tuple(l[:y] + (m,) + tuple(cl[::-1]))
            return il
    return

for i in gen_perm(3):
    print(i)

This outputs:

(1, 2, 3)
(1, 3, 2)
(2, 1, 3)
(2, 3, 1)
(3, 1, 2)
(3, 2, 1)

In order to extend this to the other direction as well, however I couldn't think of a way to do it without adding two additional functions:

def gen_perm_reverse(n):
    l=tuple(range(n,0,-1))
    yield l
    while True:
        l=decrement_lex(l)
        if not l:
            raise StopIteration
            break
        yield l

def decrement_lex(l):
    for i in range(len(l)-1):
        x=len(l)-(i+1)
        y=len(l)-(i+2)
        if l[x] < l[y]:
            m=max([i for i in l[x:] if i < l[y]])
            cl=list(l[x:])
            cl[cl.index(m)] = l[y]
            il=tuple(l[:y] + (m,) + tuple(cl[::-1]))
            return il
    return

Apologies for the use of l as an identifier. It works nicely on my editor font but not here.

Style
Readability is impaired because you don't use the space bar enough. Put a space around = for assignments, after ,, around binary operators. Parenthesis are also not required in if statements.
You also use 1-letter variable names which doesn't convey any meaning, which makes it harder to understand the code.
Duplicated work
gen_perm and gen_perm_reverse are basically the same function with some minor tweaks: the range and the "increment" function to call. Increments functions are the same too, only the comparison (< vs > and min vs max) change.
You can merge the two of them and parametrize it with a reverse parameter that default to False.
import operator

def permutations(n, reverse=False):
    numbers = range(1, n+1)
    permutation = tuple(reversed(numbers) if reverse else numbers)

    yield permutation
    while True:
        permutation = next_permutation(permutation, reverse)
        if not permutation:
            return
        yield permutation

def next_permutation(permutation, reverse=False):
    compare = operator.lt if reverse else operator.gt
    top = max if reverse else min

    max_length = len(permutation) - 1
    for i in range(max_length):
        x = max_length - i
        permutable = permutation[x - 1]
        if compare(permutation[x], permutable):
            m = top(i for i in permutation[x:] if compare(i, permutable))
            cl = list(permutation[x:])
            cl[cl.index(m)] = permutable
            return permutation[:x-1] + (m,) + tuple(cl[::-1])

A couple of changes here:

Instead of raising StopIteration, you can just return from a generator. In any case, having a break after a raise is poor practice since it is unreachable code.
I computed len(permutation) (len(l)) less time in next_permutation and assigned l[y] to a variable to reduce the cost of retrieving it.
I removed the call to tuple when creating the object to be returned since it already it a tuple.
The last return was also unnecessary as it is implicit anyway.

The drawback of this approach is that you now perform much more comparisons to reverse to be able to get the right comparison functions. You can get rid of it either by passing the functions as parameters or by combining both functions:
import operator

def permutations(n, reverse=False):
    numbers = range(1, n+1)
    compare = operator.lt if reverse else operator.gt
    top = max if reverse else min
    permutation = tuple(reversed(numbers) if reverse else numbers)
    max_length = n - 1

    yield permutation
    while True:
        for i in range(max_length):
            x = max_length - i
            permutable = permutation[x - 1]
            if compare(permutation[x], permutable):
                m = top(i for i in permutation[x:] if compare(i, permutable))
                cl = list(permutation[x:])
                cl[cl.index(m)] = permutable
                permutation = permutation[:x-1] + (m,) + tuple(cl[::-1])
                break
        else:
            # We didn't break -> no more permutations available
            return
        yield permutation

Here I used the for .. else construct to detect when we couldn't produce an new permutation and stop the generation accordingly.
Building on my comment
As I said in a comment, the most straighforward way to achieve that in python is by using itertools.permutation:
import itertools

def permutations(n):
    return itertools.permutations(range(1, n+1))

the advantage, beside its simplicity of use, is that the reverse version is straighforward too:
def permutations(n, reverse=False):
    numbers = range(1, n+1)
    return itertools.permutations(reversed(numbers) if reverse else numbers)

But being able to use both the forward and reverse iteration had me thinking about defining a class:
import itertools

class Permutations:
    def __init__(self, n):
        self.numbers = range(1, n+1)

    def __iter__(self):
        return itertools.permutations(self.numbers)

    def __reverse__(self):
        return itertools.permutations(reversed(self.numbers))

Usage being:
perm_3 = Permutations(3)
for i in perm_3:
    print(i)
for i in reversed(perm_3):
    print(i)

The advantage here is that, if you really don't wan't to use itertools, you can plug in your custom function easily:
import operator

class Permutations:
    def __init__(self, n):
        self.numbers = range(1, n+1)

    @staticmethod
    def _permutation_helper(permutation, compare, top):
        max_length = len(permutation) - 1

        yield permutation
        while True:
            for i in range(max_length):
                x = max_length - i
                permutable = permutation[x - 1]
                if compare(permutation[x], permutable):
                    m = top(i for i in permutation[x:] if compare(i, permutable))
                    cl = list(permutation[x:])
                    cl[cl.index(m)] = permutable
                    permutation = permutation[:x-1] + (m,) + tuple(cl[::-1])
                    break
            else:
                # We didn't break -> no more permutations available
                return
            yield permutation

    def __iter__(self):
        return self._permutation_helper(tuple(self.numbers), operator.gt, min)

    def __reverse__(self):
        return self._permutation_helper(tuple(reversed(self.numbers)), operator.lt, max)


Reviewing custom array List class of java I am using JDK 12 and  I have developed the custom array list in java with a basic understanding Please let me know if there are any flaws in it that I can correct the same

First the POJO class

public class Employee {
    private String id;
    private String name;

    public Employee(String id, String name) {
        this.id = id;
        this.name = name;
    }

    @Override
    public String toString() {
        return "Employee[id=" + id + ", name=" + name + "] ";
    }

}

Then come the main logic class of ArrayList, which is the heart

class CustomArrayList<E> {

    private static final int INITIAL_CAPACITY = 10;
    private Object[] elementData;
    private int size = 0;

    public customArrayList() {
        elementData = new Object[INITIAL_CAPACITY];
    }

    private void ensureCapacity() {
        int newIncreasedCapacity = elementData.length * 2;
        elementData = Arrays.copyOf(elementData, newIncreasedCapacity);
    }

    public void add(E e) {
        if (size == elementData.length) {
            ensureCapacity();
        }
        elementData[size++] = e;
    }

    @SuppressWarnings("unchecked")
    public E get(int index) {
        if (index < 0 || index >= size) {
            throw new IndexOutOfBoundsException("Index: " + index + ", Size " + index);
        }
        return (E) elementData[index];
    }

    public Object remove(int index) {
        if (index < 0 || index >= size) {
            throw new IndexOutOfBoundsException("Index: " + index + ", Size " + index);
        }

        Object removedElement = elementData[index];
        for (int i = index; i < size - 1; i++) {
            elementData[i] = elementData[i + 1];
        }
        size--;   //reduce size of ArrayListCustom after removal of element.
        return removedElement;
    }

    /**
     * method displays all the elements in list.
     */
    public void display() {
        System.out.print("Displaying list : ");
        for (int i = 0; i < size; i++) {
            System.out.print(elementData[i] + " ");
        }
    }

}

and finally, the test class to test the custom implementation of the ArrayList

public class ArrayListTest {

    public static void main(String... a) {
        CustomArrayList<Employee> list = new CustomArrayList<>();
        list.add(new Employee("1", "saral"));
        list.add(new Employee("2", "amit"));
        list.add(new Employee("3", "dinesh"));
        list.add(null);
        list.display();
        System.out.printf("\n element at index %d = %s%n", 1, list.get(1));
        System.out.printf("element removed from index %d = %s%n", 1, list.remove(1));

        System.out.println("\n" +
                "let's display list again after removal at index 1");
        list.display();

    }

}

The CustomArrayList API is inconsistent because I can declare a list of type E to add elements of type E, but when I remove an element from it, I get an Object element:

public Object remove(int index) { /* ... */ }

Inside CustomArrayList you work with Object[] elementData. From the book Effective Java; Item 26 you could change your code to:

class CustomArrayList<E> {

  private E[] elementData;
  /* ... */

  public CustomArrayList() {
    elementData = (E[]) new Object[INITIAL_CAPACITY];
  }

  /* ... */

}

Inside remove you modify elementData in an imperative way. You could use System.arraycopy which would be more deklarative.

public Object remove(int index) {
 /* ... */
 Object removedElement = elementData[index];
 for (int i = index; i < size - 1; i++) {
   elementData[i] = elementData[i + 1];
 }
 /* ... */
}

public E remove(int index) {
  /* ... */

  if (size - 1 - index >= 0)
    System.arraycopy(elementData, index + 1, elementData, index, size - 1 - index);

  /* ... */
}

The methods get and remove contain a code duplication:

public E get(int index) {
 if (index < 0 || index >= size) {
   throw new IndexOutOfBoundsException("Index: " + index + ", Size " + index);
  }
 return (E) elementData[index];
}

public Object remove(int index) {
 if (index < 0 || index >= size) {
   throw new IndexOutOfBoundsException("Index: " + index + ", Size " + index);
 }

 Object removedElement = elementData[index];

 /* ... */

 return removedElement;
}

It is possible to invoke get inside remove like:

public E remove(int index) {
  E elementToRemove = get(index);

  if (size - 1 - index >= 0)
    System.arraycopy(elementData, index + 1, elementData, index, size - 1 - index);

  size--;

  return elementToRemove;
}


Count number of unique adjacent cells in a matrix I want to write an algorithm to count how many islands are in a given matrix. Consider for example:

A = [
        [1, 2, 1, 3],
        [2, 2, 3, 2],
        [3, 3, 2, 3]
    ]

Directly adjacent (north, south, east, west, but not diagonally) numbers constitute an island, in this example matrix we have 9.

I've written the following code which works and performs fine. Any suggestions on how could I write this more cleanly and to make it work even faster?

def clean_neighbours(matrix, this_row, this_col):
    cell_value = matrix[this_row][this_col]
    if cell_value == 0:
        return

    matrix[this_row][this_col] = 0
    number_of_rows = len(matrix)
    number_of_columns = len(matrix[0])

    for shift in (
        (-1,0), (1,0), (0,-1), (0,1)
    ):
        row, col = [x+y for x,y in zip((this_row, this_col), shift)]
        if (row >= 0 and row < number_of_rows) and (
            col >= 0 and col < number_of_columns
        ):
            if matrix[row][col] == cell_value:
                clean_neighbours(matrix, row, col)

def count_adjacent_islands(matrix):
    number_of_islands = 0
    for row_index, row in enumerate(matrix):
        for column_index, _ in enumerate(row):
            if matrix[row_index][column_index] != 0:
                number_of_countries += 1
                clean_neighbours(matrix, row_index, column_index)
    return number_of_countries

import random
A = [ [random.randint(-1000,1000) for e in range(0,1000)] for e in range(0,1000) ]
print(count_adjacent_islands(A))

bug

in count_adjacent_islands, number_of_islands = 0 should be number_of_countries = 0

mutate original argument

Most of the time, it's a bad idea to change any of the arguments to a function unless explicitly expected. So you better take a copy of the matrix first:

matrix_copy = [row[:] for row in matrix]

tuple unpacking

instead of for shift in ((-1,0), (1,0), (0,-1), (0,1)):, you can do for dx, dy in ((-1, 0), (1, 0), (0, -1), (0, 1)):, then row, col = [x+y for x,y in zip((this_row, this_col), shift)] can be expressed a lot clearer: row, col = x + dx, y + dy

continue

instead of keep nesting if conditions, you can break out of that iteration earlier if the conditions are not fulfilled

for row_index, row in enumerate(matrix):
    for column_index, _ in enumerate(row):
        if matrix[row_index][column_index] != 0:
            number_of_islands += 1
            clean_neighbours(matrix, row_index, column_index)

can become:

for row_index, row in enumerate(matrix_copy):
    for column_index, _ in enumerate(row):
        if matrix_copy[row_index][column_index] == 0:
            continue
        number_of_islands += 1
        clean_neighbours2(matrix_copy, row_index, column_index)

saving 1 level of indentation on the code that actually does the lifting. This is not much in this particular case, but with larger nested conditions, this can make things a lot clearer, and save a lot of horizontal screen estate

recursion

If there are some larger islands, you will run into the recursion limit. Better would be to transform this to a queue and a loop

from collections import deque
def clean_neighbours2(matrix, x, y):
    cell_value = matrix[x][y]
    if cell_value == 0:
        return

    matrix[x][y] = 0

    queue = deque([(x,y)])

    while queue:
        x, y = queue.pop()
        for dx, dy in ((-1, 0), (1, 0), (0, -1), (0, 1)):
            row, col = x + dx, y + dy
            if (
                0 <= row < len(matrix)
                and 0 <= col < len(matrix[0])
                and not matrix[row][col] == 0
            ):
                continue
            if matrix[row][col] == cell_value:
                queue.append((row, col))
                matrix[row][col] = 0

def count_adjacent_islands2(matrix):
    matrix_copy = [row[:] for row in matrix]
    number_of_islands = 0
    for row_index, row in enumerate(matrix_copy):
        for column_index, _ in enumerate(row):
            if matrix_copy[row_index][column_index] == 0:
                continue
            number_of_islands += 1
            clean_neighbours2(matrix_copy, row_index, column_index)
    return number_of_islands

For the sample data you provided, this code took 3s compared to 4s for the original on my machine

alternative approach

Using numba and numpy, and a slight rewrite to accomodate for numba compatibilities:

from numba import jit
import numpy as np

@jit()
def clean_neighbours_jit(matrix, x, y):
    cell_value = matrix[x, y]
    if cell_value == 0:
        return

    matrix[x, y] = 0

    queue = [(x, y)]
    row_length, column_length = matrix.shape

    while queue:
        x, y = queue.pop()
        for dx, dy in ((-1, 0), (1, 0), (0, -1), (0, 1)):
            row, col = x + dx, y + dy
            if (
                not 0 <= row < row_length
                or not 0 <= col < column_length
                or matrix[row, col] != cell_value
            ):
                continue
            queue.append((row, col))
            matrix[row, col] = 0

@jit()
def count_adjacent_islands_jit(matrix):
    matrix_copy = matrix.copy()
    number_of_islands = 0
    row_length, column_length = matrix_copy.shape
    for row_index in range(row_length):
        for column_index in range(column_length):
            if matrix_copy[row_index, column_index] == 0:
                continue
            number_of_islands += 1
            clean_neighbours_jit(matrix_copy, row_index, column_index)
    return number_of_islands

This expects a numpy array as matrix, (for example: count_adjacent_islands_jit(np.array(A))) but does the job in about 200 to 300ms, (about 80ms spent on converting A to an np.array), so more than 10x speedup.


Sieve of Eratosthenes for prime generation I was originally going to write this intro as just being the bare-bones "here's my problem, here are some ideas, what do you recommend" format (which I am still going to follow, mind you), but I've decided to also add some backstory to it.

In high school I would write inefficient programs that generated mathematically significant results, at least to me, and then have hours of fun optimizing and revising the code until it was as clean and fast as I could make it. While I was writing this sieve, I was exploring the generation of primes.

Currently, the code will generate a range of natural numbers from \$1\$ to \$N\$ and store it in a list. Then the program will iterate through that list, marking each multiple of a prime as \$0\$ and storing each prime in a secondary list.

While this solution works just fine, one of the optimizations that I could never wrap my head around writing the code for was removing the multiples of primes instead of simply marking them as \$0\$.

The benefits of doing this are as follows:

Faster execution
Less memory usage
No need for cleanup
No if statements required

The problem is that I'm having a hard time wrapping my head around how I could perform this optimization and still remove multiples of each prime efficiently. If anyone has any ideas on how to do this in TI-Basic, please mention it.

What other optimizations could I make to the code as it is now?

Note: I do use an indentation system here because I loved the readability that indentation provides to languages like Python. Could this have any noticeable impact on the performance of the program?

:ClrList L2
:Input "END: ",E
:E->dim(L1:Fill(1,L1:cumSum(L1->L1
:For(A,2,E)
::If L1(A:Then
:::L1(A->L2(dim(L2)+1
:::For(B,2A,E,A
::::0->L1(B
:::End
::End
:End
:L2

Clarification:

E->dim(L1:Fill(1,L1:cumSum(L1->L1 generates the range from \$1\$ to \$N\$.

Removing a single number from a TI-BASIC list takes O(N) time where N is the size of the list. It's simply not practical. Here is the fastest idiom for removing the Xth element:

seq(L₁(A+(A≥X)),A,1,dim(L₁)-1→L₁

As for the indentation, the TI-BASIC interpreter needs to step through one colon at a time, which is relatively fast but not instantaneous. I expect each extra colon to take a TI-84 series calc about 0.1 ms each time through. If you want to maximize speed, get rid of the indentation.

As for other optimizations:

Rather than using E->dim(L1:Fill(1,L1:cumSum(L1->L1 to generate the list, use

cumSum(binomcdf(E-1,0→L₁

which has the same effect.

You can prescan for numbers not divisible by 2, etc; try this:

augment({2},1+2cumSum(binomcdf(int(.5E),0

to generate only the odd numbers.

As a different approach, you can generate prime numbers by adding to, not removing from, a list:

{2
For(,3,E,2)       ;close parenthesis to fix parser bug; step by 2 to catch odd numbers
If min(fPart(/L₁
n→L₁(1+dim(L₁
End
L₁

It should be of comparable speed or faster.

As a small tip, use the sequence variable n instead of a loop index. It takes as much as 0.5 milliseconds less to access (depending on how many other variables are in your VAT).


Cancelable thread worker My goal was to create class, that can run external method in non blocking manner.
Second requirement was to be able to abort method run if it is needed (i.e. running user script).
I decided to do that with Thread instead of Task, because only Thread can be truly aborted.
Because Thread have big creation overhead, it is initialized and started in advance and resumed if work is needed.

What do you think about my implementation ?

Example usage:

//generic type param is used as external method argument (instead of object)
AbortableStepWorker<int> worker = new AbortableStepWorker<int>(someMethodWithIntArgument);
worker.Start();
worker.DoWork(someArgument);
while(!worker.WaitForWorkDone(someTimeout))
    if(cancelWasRequested)
    {
        worker.Stop(lastChanceTimeout);
        return false;
    }

//if we've managed to get here, then work was done
//run result:
var result = !worker.Stopped;

and class implementation:

public class AbortableStepWorker<T> : IDisposable
    {
        public AbortableStepWorker(Func<T, bool> externalWork)
        {
            if (externalWork == null) throw new ArgumentNullException("workMethod");
            this.externalWork = externalWork;

            ThreadInstance = new Thread(Worker) { Priority = ThreadPriority.BelowNormal };
        }

        private readonly Func<T, bool> externalWork;
        private T externalWorkParam;
        private void Worker()
        {
            try
            {
                while (true)
                {
                    resumeEvent.WaitOne(Timeout.Infinite);

                    if (stopRequested)
                        return;

                    if (!externalWork(externalWorkParam))
                        return;

                    workDoneEvent.Set();
                }
            }
            catch (ThreadAbortException) { }
            finally { workDoneEvent.Set(); }
        }

        private bool stopRequested = false;
        private AutoResetEvent resumeEvent = new AutoResetEvent(false);
        private ManualResetEvent workDoneEvent = new ManualResetEvent(false);

        public Thread ThreadInstance { get; private set; }

        public void Start()
        {
            ThreadInstance.Start();
        }

        /// <summary>
        /// Allows inner thread to do externalWork call.
        /// </summary>
        public void DoWork(T param)
        {
            externalWorkParam = param;
            workDoneEvent.Reset();
            resumeEvent.Set();
        }

        /// <summary>
        /// Waits for DoWork to finish.
        /// </summary>
        /// <param name="millisecondsTimeout">Timeout.</param>
        /// <returns>True if work is not still running.</returns>
        public bool WaitForWorkDone(int millisecondsTimeout)
        {
            return workDoneEvent.WaitOne(millisecondsTimeout);
        }

        /// <summary>
        /// Waits specified time for worker method to end. If time elapsed, aborts thread.
        /// </summary>
        /// <param name="millisecondsTimeout">Timeout before thread abort.</param>
        public void Stop(int millisecondsTimeout = 0)
        {
            if (Stopped)
                return;

            bool wasWorking = !workDoneEvent.WaitOne(0);
            stopRequested = true;
            resumeEvent.Set();

            if (!ThreadInstance.Join(wasWorking ? millisecondsTimeout : Timeout.Infinite))
                ThreadInstance.Abort();
        }

        public bool Stopped
        {
            get
            {
                return ThreadInstance.Join(0);
            }
        }

        #region IDisposable
        public void Dispose()
        {
            if (ThreadInstance.ThreadState != ThreadState.Unstarted)
            {
                Stop();
                ThreadInstance.Join();
            }
            resumeEvent.Dispose();
            workDoneEvent.Dispose();
        }
        #endregion //IDisposable
    }

You should never use Thread.Abort(). If you don't have control over the code that's executing in the thread, then you don't know what will that abort do. It's quite possible it will leave some object in an inconsistent state, or something like that. If you do have control over the code, you should instead use cooperative cancellation using CancellationToken.


Netflix Login script What are my options on how the loginToNetflix method is called? Is there a more clean and elegant way of accomplishing this?

loginToNetflix(credentials.username, credentials.password);

function loginToNetflix (username, password) {
casper.start('https://www.netflix.com/Login?locale=en-US', function() {
    this.echo('Attempting login to Netflix - filling and submitting login form');

    this.fill('form#login-form', {
            'email':    username,
         'password':    password,
       'RememberMe':    true
    }, true);
});

casper.then(function() {
    this.echo('Login Complete - Please check the cookie as defined in command line');
    //save the cookie for use by netflix scraper api
    var cookies = JSON.stringify(phantom.cookies);
    fs.write("netflix-cookies.txt", cookies, 644);
});

casper.run(function () {
    this.echo('Done - Exiting!');
    casper.exit();
});
}

Looks alright to me. A small personal nuisance for me,  but a possible improvement would be to pass the credentials in an object

var credentials = {
    username: "xxx",
    password: "yyy"
};

That would make the signature of your function smaller, and after all, the 2 variables are tightly coupled, semantically speaking.

But as I say, peccata minuta. Looks good to me.


Removing neighbors in a point cloud I have written a program to optimize a point cloud in dependency of their distances to each other. The code works very well for smaller number of points. For 1700 points it takes ca. 6 minutes. But I have 300000 points in the point cloud. The code is still running after almost 30 hours.

I am sure there is a pythonic way to optimze the code. But I do not know how can I reduce the calculation time and make the performance better. I have read alot about multithreding, chunk size etc. But I think there is no need for this size of data. I think, I am using the memory in a very bad way. Am I right? Here is the code:

import pickle
import math
import itertools
import gc

def dist(p0, p1):
    return math.sqrt((p0[0] - p1[0])**2 + (p0[1] - p1[1])**2)

def join_pair(points, r):
    for p, q in itertools.combinations(points, 2):
        if dist(p, q) < r:
            points.remove(q)
            return True
    return False

if __name__ == "__main__":
    filename = 'points.txt'
    filename2 = 'points_modified.txt'
    mynumbers = []
    with open(filename) as f:
        for line in f:
            mynumbers.append([float(n) for n in line.strip().split(' ')])
        mynumbers = sorted(mynumbers, key=lambda tup: (tup[1], tup[0]))
    gc.collect()
    while join_pair(mynumbers, 3.6):
        pass

    with open (filename2,"w")as fp2:
        for j in range(len(mynumbers)):
            fp2.write(' '.join(map(repr, mynumbers[j]))+'\n')

and the input data is like the following:

661234.58 5763766.03 72.63
661254.81 5763765.08 75.04
661244.86 5763764.14 74.99
661234.90 5763763.21 74.94
661225.13 5763762.29 74.89

1. Introduction

It is not a good plan to leave code running for 30 hours. Until you have analyzed the code, and have figured out how its runtime varies according to the size of the input, then you have no idea how long it will take. Maybe it will take 31 hours, but maybe it will take a million hours.

In this situation you need to (i) analyze the algorithm to get an expression for its runtime complexity (that is, how its runtime depends on the size of the input); (ii) measure the performance of the code on small inputs; (iii) use the results of (i) and (ii) to extrapolate to the runtime for the full data set.

2. Review

There are no docstrings. What are these functions supposed to do?
dist can be simplified using math.hypot:

def dist(p, q):
    "Return the Euclidean distance between points p and q."
    return math.hypot(p[0] - q[0], p[1] - q[1])

The name join_pairs is misleading because it does not join anything. I am sure you could pick a better name.
It would be useful (for testing and performance measurement) to have a function that calls join_pairs in a loop until it returns False. I am going to use this function:

def sparse_subset1(points, r):
    """Return a maximal list of elements of points such that no pairs of
    points in the result have distance less than r.

    """
    points = points[:]
    while join_pairs(points, r):
        pass
    return points

The reason that I am taking a copy of points (rather than modifying the original) is so that I can call this function for testing or performance measurement without destroying the input data. See below for some examples.

3. Performance

Here's a test case of 1,000 random points:

import random
POINTS = [[random.random() for _ in range(2)] for _ in range(1000)]

The code in the post takes about 11 seconds to reduce this to a sparse subset of 286 points at least 0.04 apart:

>>> from timeit import timeit
>>> timeit(lambda:sparse_subset1(POINTS, 0.04), number=1)
10.972541099996306

Why does it take so long? The problem is that join_pairs removes just one point each time it is called. This means that you have to call it in a loop, but each time you call it it starts over comparing the same pairs of points that were already compared in the previous iteration. As a result of this algorithm the runtime is \$O(n^3)\$: that is, the runtime grows like the cube of the length of the input.

We can see this cubic behaviour by measuring the time \$t\$ taken by the function on a range of sizes \$n\$ and finding a cubic equation \$t = an^3\$ of best-fit:

>>> import numpy as np
>>> n = np.arange(100, 1001, 100)
>>> t = [timeit(lambda:sparse_subset1(POINTS[:i], 0.04), number=1) for i in n]
>>> a, = np.linalg.lstsq(n[:, np.newaxis] ** 3, t)[0]

We can plot this on a log-log graph using matplotlib:

>>> import matplotlib.pyplot as plt
>>> plt.xlabel("n")
>>> plt.ylabel("t (seconds)")
>>> plt.loglog(n, t, 'r+', label="data")
>>> plt.loglog(n, a * t**3, label="least squares fit to $t = an^3$")
>>> plt.legend()
>>> plt.grid(alpha=0.25)
>>> plt.show()

So extrapolating from this analysis and measurement, we estimate that if \$n=300000\$ then the time taken will be roughly:

>>> a * 300000**3
339550658.803593

That's about ten years! Looking at the graph, it seems that the slope of the data is not quite as steep as the line of best-fit, so maybe the true exponent is a bit less than 3 and the extrapolated time somewhat less than ten years. But as a rough analysis this tells us that there's no hope for it finish in a few more hours, and so we have to find a better approach.
It would be better to do the work in a single pass over the list of points, like this:

def sparse_subset2(points, r):
    """Return a maximal list of elements of points such that no pairs of
    points in the result have distance less than r.

    """
    result = []
    for p in points:
        if all(dist(p, q) >= r for q in result):
            result.append(p)
    return result

This builds up the modified point cloud in the list result and returns it (instead of modifying the input points). The code considers each point in the input, and adds it to the result so long as it is sufficiently far from all the points that have been previously added to the result.

To check that this is correct, we can compare the output against the original version of the code:

>>> sparse_subset1(POINTS, 0.04) == sparse_subset2(POINTS, 0.04)
True

This revised code takes \$O(nm)\$, where \$n\$ is the number of points in the input and \$m\$ is the number of points in the output. In the worst case \$m = n\$ and the code has to compare every pair of points. On the 1000-points test case, this is hundreds of times faster than the code in the post:

>>> timeit(lambda:sparse_subset2(POINTS, 0.04), number=1)
0.04830290700192563

To further improve the performance, we need a spatial index, that is, a data structure supporting efficient nearest-neighbour queries. For this problem, I'm going to use the Rtree package from the Python Package Index, which implements the R-tree data structure. An R-tree maintains a collection of rectangles (or hyper-rectangles if the search space has more than two dimensions), and can efficiently find the rectangles in the collection that intersect with a query rectangle.

import rtree

def sparse_subset3(points, r):
    """Return a maximal list of elements of points such that no pairs of
    points in the result have distance less than r.

    """
    result = []
    index = rtree.index.Index()
    for i, p in enumerate(points):
        px, py = p
        nearby = index.intersection((px - r, py - r, px + r, py + r))
        if all(dist(p, points[j]) >= r for j in nearby):
            result.append(p)
            index.insert(i, (px, py, px, py))
    return result

When this code appends a point to the result list, it also adds a corresponding rectangle to the R-tree. This rectangle has zero width and height, so it represents a single point. When considering a point p for inclusion in the result set, the code queries a rectangle centred on p with width and height 2 * r. All points within a distance r of p are inside this rectangle, but there might also be some points in the corners of the rectangle that are more distant than r. That's why the code has to further check each point in the query rectangle.

Again, we need to check that this is correct:

>>> sparse_subset1(POINTS, 0.04) == sparse_subset3(POINTS, 0.04)
True

An R-tree takes \$O(\log m)\$ to find an intersecting rectangle in an index of \$m\$ rectangles, so we expect the runtime of sparse_subset3 to be \$O(n\log m)\$, where \$n\$ is the number of points in the input and \$m\$ is the number of points in the output.

Let's make a much bigger test case, with 100,000 points:

POINTS = [[random.random() for _ in range(2)] for _ in range(10**5)]

The performance of sparse_subset2 and sparse_subset3 depends on \$m\$, the number of points in the result. We expect that when r is big, \$m\$ will be small and so there won't be much difference. Here we use r=0.05 and get a subset of 287 points, and the two algorithms have almost identical runtime:

>>> timeit(lambda:sparse_subset2(POINTS, 0.05), number=1)
3.5980678769992664
>>> timeit(lambda:sparse_subset3(POINTS, 0.05), number=1)
3.598255231976509

But if we reduce r to 0.01, getting a subset of 5994 points, then the R-tree shines:

>>> timeit(lambda:sparse_subset2(POINTS, 0.01), number=1)
87.03980600900832
>>> timeit(lambda:sparse_subset3(POINTS, 0.01), number=1)
4.6150341169850435

(I didn't try sparse_subset1 on this test case as it will take many hours.)

The worst case for sparse_subset3 is when r is zero and so all the points in the input end up in the result:

>>> timeit(lambda:sparse_subset3(POINTS, 0), number=1)
10.394442071992671

Extrapolating from this, you should be able to run your 300,000-point case in under a minute, which is a big saving on ten years.


C# program which measures the distance between the average and maximum/minimum temperature The program below takes inputs for the temperature in various cities in a span of multiple days. It then outputs the ones where the minimal temperature is closer to the average than the maximal temperature, prefaced by the number of cities where this happens.

For example:
"3 5
10 15 12 10 10

11 11 11 11 20

12 16 16 16 20"

returns "2 1 2".

I've ran it with various particularly large inputs a few times, and the program seems to slow down with larger numbers. Is there any way to make the program faster?

class Program
    {
        static void Main(string[] args)
        {
            string input = Console.ReadLine();
            string[] inputs = input.Split();
            int cities = Convert.ToInt32(inputs[0]);
            int days = Convert.ToInt32(inputs[1]);
            int[] content = new int[days];
            int i = 0;
            int y = 0;
            float max = 0;
            float average = 0;
            float min = 50;
            int how_many = 0;
            List<int> which = new List<int>();
            for (i = 0; i < cities; i++)
            {
                input = Console.ReadLine();
                inputs = input.Split();
                for (y = 0; y < days; y++)
                {
                    content[y] = Convert.ToInt32(inputs[y]);
                    average += content[y];
                    if (content[y] > max)
                    {
                        max = content[y];
                    }
                    if (content[y] < min)
                    {
                        min = content[y];
                    }
                }
                average = (float)average / days;

                if (max - average > average - min)
                {
                    how_many += 1;
                    int position = i + 1;
                    which.Add(position);
                }
                max = 0;
                average = 0;
                min = 50;
                y = 0;

            }
            Console.Write(how_many + " ");
            i = 0;
            for (i = 0; i < which.Count(); ++i)
            {
                Console.Write(which[i] + " ");
            }

            Console.ReadKey();
        }
    }

If you want to see how performance is you need to get the user input out of the loop.  I'm assuming you don't want to measure the time it takes someone to type in or paste in the data elements.

You don't need the variable how_many as it will always be the same as the Count of which, which by the way isn't a good name for a variable even something like results would be better name.

You also don't need the context array variable. Just need to store the current value not save each element as it loops through.

The variables i, y, max, average, min can all be declared in the loop scope and not need to be reset.

Calling Count() on  which.Count() is the Linq Count method, should just call the list Count property

I'm also assuming this is a beginner question and won't get into Console as input and validating the data coming from it is valid.

I personally didn't want to type in all the data so I made the computer generate random data, if in debug mode

    static void Main(string[] args)
    {
#if DEBUG
        var random = new Random();
        var inputs = "1000 1000".Split();
#else
        var inputs = Console.ReadLine().Split();
#endif
        var cities = Convert.ToInt32(inputs[0]);
        var days = Convert.ToInt32(inputs[1]);

        var data = new List<int[]>(cities);
        for (var i = 0; i < cities; i++)
        {
#if DEBUG
            var cityData = Enumerable.Range(0, days)
                .Select(_ => random.Next(100))
                .ToArray();
#else
            var cityData = Console.ReadLine()
                .Split()
                .Select(x => Convert.ToInt32(x))
                .ToArray();
#endif
            if (cityData.Length != days)
            {
                Array.Resize(ref cityData, days);
            }
            data.Add(cityData);
        }

        var timer = new Stopwatch();
        timer.Start();
        var results = new List<int>();
        for (var i = 0; i < cities; i++)
        {
            float max = float.MinValue;
            float average = 0;
            float min = float.MaxValue;
            for (var y = 0; y < days; y++)
            {
                var content = data[i][y];
                average += content;
                if (content > max)
                {
                    max = content;
                }
                if (content < min)
                {
                    min = content;
                }
            }
            average = average / days;

            if (max - average > average - min)
            {
                int position = i + 1;
                results.Add(position);
            }
        }

        Console.Write(results.Count + " ");
        Console.WriteLine(string.Join(" ", results));
        timer.Stop();
        Console.WriteLine();
        Console.WriteLine($"Total Processing Time: {timer.Elapsed}");
        Console.ReadKey();
    }

This code runs in less than a second on my machine.  I assume waiting for user input was what was taking the longest time in pervious one and not actually the processing of the data.


A defined macro to copy selected values of std::vector to an array using std::copy Thought I share this piece of code to the world. But be aware, I am not sure if this piece of code is safe and efficient. Feel free to improve it or give some feedback and suggestions.

#pragma region DOCUMENTATION ON: STD_COPY_VECTOR_TO_ARRAY
//////////////////////////////////////////////////////////////////////////////
// MACRO: STD_COPY_VECTOR_TO_ARRAY( Vector, Position, Length, Array )       //
//                                                                          //
// DESCRIPTION:                                                             //
// A defined macro that is created to copy selected values of std::vector   //
// an array. This defined macro takes 4 arguments.                          //
//                                                                          //
// ARGUMENTS:                                                               //
// Vector - The std::vector that contains the values that will be copied.   //
// Position - Position of the first value from Vector to be copied.         //
// Note: The first value of Position is marked by the value of 0 and not 1. //
// Length - Amount of values to be copied from Vector.                      //
// Array - The Array that the values will be copied to.                     //
//                                                                          //
// TIPS:                                                                    //
// #1 - To copy all values within Vector, set the value of Position to 0    //
// and set the value of Length to the size of Vector (std::vector::size).   //
//////////////////////////////////////////////////////////////////////////////
#pragma endregion
#define STD_COPY_VECTOR_TO_ARRAY( Vector, Position, Length, Array )         \
                                  std::copy( Vector.begin( ) + Position,    \
                                  Vector.begin( ) + Position + Length ,     \
                                  Array )

Basically, there are very few places in C++ where you want to use Macros. A lot of old C code used function-like macros because of performance reasons - calling a function requires creating a new stack frame. They were also used to get away from C's type system. With C++, inline functions and templates remove the need for this 99% of the time.

template <typename T>
inline void copy_vector_to_array(const std::vector<T>& vec,
                                 std::size_t position,
                                 std::size_t length,
                                 T* array)
{
    std::copy(vec.begin() + position, vec.begin() + position + length, array);
}

In C++11, we can do better still - an erroneous length passed in above will cause problems, and will be hard to detect:

#include <vector>
#include <array>
#include <iterator>
#include <cassert>

template <typename T, std::size_t N>
inline void copy_vector_to_array(const std::vector<T>& vec,
                                 std::size_t position,
                                 std::size_t length,
                                 std::array<T, N>& array)
{
    assert(length <= N);
    std::copy(std::begin(vec) + position, std::begin(vec) + position + length,
              std::begin(array));
}

This will be just as efficient as the macro version, however, we also gain type safety, as well as (easy) bounds checking.

We can also improve on it. So that it handles other container types:

template <typename C, std::size_t N>
inline void copy_container_to_array(const C& container,
                                 std::size_t position,
                                 std::size_t length,
                                 std::array<typename C::value_type, N>& array)
{
    assert(length <= N);
    std::copy(std::next(std::begin(vec), position), std::next(std::begin(vec), position + length),
              std::begin(array));
}


Extracting an arbitrary element from each row of a np.array I have a large numpy array of shape (n,m). I need to extract one element from each row, and I have another array of shape (n,) that gives the column index of the element I need. The following code does this, but it requires an explicit loop (in the form of a list comprehension.):

import numpy as np

arr = np.array(range(12))
arr = arr.reshape((4,3))
keys = np.array([1,0,1,2])
#This is the line that I'd like to optimize
answers = np.array([arr[i,keys[i]] for i in range(len(keys))])
print(answers)
# [ 1  3  7 11]

Is there a built-in numpy (or pandas?) function that could do this more efficiently?

The best way to do this is how @GarethRees suggested in the comments:

>>> arr[np.arange(4), keys]
array([1, 3, 7, 11])

There is another (not as good) solution that is a better alternative to using arr[i, keys[i]] for i in range(len(keys)).  Whenever you want both the index and the item from some iterable, you should generally use the enumerate function:

>>> np.array([arr[i, item] for i, item in enumerate(keys)])
array([1, 3, 7, 11])


VBA macro - searches through a filtered table, stops when entry passes criteria I have written several subs which are then called from a main sub. Individual subs run very quickly, most are instantaneous (the DoFind sub takes a few seconds to run due to the large amounts of data in the table) however when I run the main sub it takes up to a minute to execute. Any ideas/tips on why this is the case?

Note, I haven't had much experience with VBA (all has been learnt in the past week). There are other macros used, but they are not shown since even the test sub takes approximately 1 minute

Sub DoFind()

    Dim i As Long

    i = 1

        Do While Sheets("Temp").Cells(i, "A").Value <> Empty

                Dim BearingArray(6) As String

                BearingArray(0) = Sheets("Temp").Cells(i, "A").Value
                BearingArray(1) = Sheets("Temp").Cells(i, "B").Value
                BearingArray(2) = Sheets("Temp").Cells(i, "C").Value
                BearingArray(3) = Sheets("Temp").Cells(i, "D").Value
                BearingArray(4) = Sheets("Temp").Cells(i, "E").Value
                BearingArray(5) = Sheets("Temp").Cells(i, "F").Value
                BearingArray(6) = Sheets("Temp").Cells(i, "G").Value

                With Sheets("Calculations")
                    .Cells(17, "K").Value = BearingArray(0)
                    .Cells(19, "O").Value = BearingArray(1)
                    .Cells(20, "O").Value = BearingArray(2)
                    .Cells(23, "O").Value = BearingArray(3)
                    .Cells(22, "O").Value = BearingArray(4)
                    .Cells(26, "O").Value = BearingArray(5)
                    .Cells(17, "L").Value = BearingArray(6)
                End With

                i = i + 1

                    If Sheets("Calculations").Cells(17, "M").Value = "PASS" Then
        Exit Do
                    Else
                    End If
        Loop
                    If Sheets("Temp").Cells(i, "A").Value = Empty Then
                        MsgBox "No available bearing."

                    End If

End Sub

Sub Create_Sheet_Temp()

    ThisWorkbook.Sheets.Add
    ActiveSheet.Name = "Temp"

' This creates a new worksheet called "Temp"

End Sub

Sub Copy_Paste()

    Dim NewTable As ListObject
    Sheets("Calculations").Activate

    Set NewTable = Sheets("Calculations").ListObjects("Full_Bearings_List")

    NewTable.Range.SpecialCells(xlCellTypeVisible).Select
    NewTable.DataBodyRange.SpecialCells(xlCellTypeVisible).Copy
    Sheets("Temp").Range("A1").PasteSpecial xlPasteAll
    Application.CutCopyMode = False

        'This sub copies all visible cells from a filtered table and pastes them to the new sheet called "Temp"

End Sub

Sub test()
    Create_Sheet_Temp
    Copy_Paste
    DoFind

End Sub

First, I'd just like to say: This is decent code for someone who's brand new to VBA. Great work.

That aside, let's begin:

Option Explicit

If that's not at the top of all your code modules, put it in. Go to Tools --> Options --> Require Variable Declaration to have it auto-inserted into every new module you create.

Easy Performance Improvements

Sub DoThing()

    With Application
        .ScreenUpdating = False
        .EnableEvents = False
        .StatusBar = False
        .Calculation = xlCalculationManual
    End With

    ...

    ...

    Code

    ...

    ...

    With Application
        .ScreenUpdating = True
        .EnableEvents = True
        .StatusBar = True
        .Calculation = xlCalculationAutomatic
    End With

End Sub

Each of those settings will provide large performance improvements. Especially ScreenUpdating. Always check that you're not reliant on the thing you're disabling though. For instance, DoFind() is relying on a formula to change, so you shouldn't disable Calculation.

Consistent Indenting

Pick an indenting rule and stick to it. Personally, I use a single Tab for each logical level (If, While, With, etc.) and it's a widespread convention with VBA. This way, I can easily identify which statements are operating at what level.

Something like this is particularly confusing:

               If Sheets("Calculations").Cells(17, "M").Value = "PASS" Then
    Exit Do
               Else
               End If

It's much easier to "see" what's going on when it's expressed like this:

If Sheets("Calculations").Cells(17, "M").Value = "PASS" Then
    Exit Do
End If

I would re-indent your DoFind sub like so:

Sub DoFind()

    Dim i As Long
    i = 1

    Do While Sheets("Temp").Cells(i, "A").Value <> Empty

        Dim BearingArray(6) As String
        BearingArray(0) = Sheets("Temp").Cells(i, "A").Value
        BearingArray(1) = Sheets("Temp").Cells(i, "B").Value
        BearingArray(2) = Sheets("Temp").Cells(i, "C").Value
        BearingArray(3) = Sheets("Temp").Cells(i, "D").Value
        BearingArray(4) = Sheets("Temp").Cells(i, "E").Value
        BearingArray(5) = Sheets("Temp").Cells(i, "F").Value
        BearingArray(6) = Sheets("Temp").Cells(i, "G").Value

        With Sheets("Calculations")
            .Cells(17, "K").Value = BearingArray(0)
            .Cells(19, "O").Value = BearingArray(1)
            .Cells(20, "O").Value = BearingArray(2)
            .Cells(23, "O").Value = BearingArray(3)
            .Cells(22, "O").Value = BearingArray(4)
            .Cells(26, "O").Value = BearingArray(5)
            .Cells(17, "L").Value = BearingArray(6)
        End With

        i = i + 1

        If Sheets("Calculations").Cells(17, "M").Value = "PASS" Then
            Exit Do
        End If

    Loop

    If Sheets("Temp").Cells(i, "A").Value = Empty Then
        MsgBox "No available bearing."
    End If

End Sub

Much easier to follow.

Naming

I recommend the Excellent, Classic article on naming by Joel spolsky.

Naming is, famously, one of the 2 hardest things in software development. It's also one of the most important. Code should be written for other people (including future you) to understand as easily and completely as possible.

Names should be descriptive, then unambiguous, and only then concise.
Variables should sound like what they are. I should be able to tell from the name of a Sub/Function exactly what it does.

DoFind. This tells me nothing.

FindBearing would be better. I'd offer you an even better name, but there's not enough context to be sure what the rest of your code/workbook is doing and why.

Create_Sheet_Temp. Don't use _ in procedure names. _ in VBA is used for event triggers e.g. Workbook_Open or Button_OnClick. Avoid it in your own names to avoid confusion.

Copy_Paste. Useless. Copy what? Paste what? Paste where?.
PasteBearingDataToTempSheet. Sure, it's a bit verbose, but it's also useful. You should always aim for useful names. If you can make them short as well, great, but it's not the priority.

Use the Object Model

The great power of VBA for manipulating MS Office is the extensive Object Model it's integrated with.

There are objects for everything. Workbooks, Worksheets, ListObjects, Ranges etc.

Take this for instance:

        BearingArray(0) = Sheets("Temp").Cells(i, "A").Value
        BearingArray(1) = Sheets("Temp").Cells(i, "B").Value
        BearingArray(2) = Sheets("Temp").Cells(i, "C").Value
        BearingArray(3) = Sheets("Temp").Cells(i, "D").Value
        BearingArray(4) = Sheets("Temp").Cells(i, "E").Value
        BearingArray(5) = Sheets("Temp").Cells(i, "F").Value
        BearingArray(6) = Sheets("Temp").Cells(i, "G").Value

What happens if/when your temp sheet has a different name than "Temp"? How about if someone renames the "Calculations" sheet? Not only will your code break, but you'll have to change those strings everywhere in your code. Maybe you'll get them all. Maybe you won't. Chances are, it will introduce new errors.

Instead, make some Worksheet objects and use those:

Sub NewTest()

    Dim tempSheet As Worksheet
    Set tempSheet = ThisWorkbook.Sheets.Add

    Dim calculationSheet As Worksheet
    Set calculationSheet = ThisWorkbook.Sheets("Calculations")

    PasteBearingDataToTempSheet tempSheet, calculationSheet

    FindBearing tempSheet, calculationSheet

End Sub

Sub PasteBearingDataToTempSheet(ByRef tempSheet As Worksheet, ByRef calculationSheet As Worksheet)

    calculationSheet.Activate

    Dim NewTable As ListObject
    Set NewTable = calculationSheet.ListObjects("Full_Bearings_List")

    NewTable.Range.SpecialCells(xlCellTypeVisible).Select
    NewTable.DataBodyRange.SpecialCells(xlCellTypeVisible).Copy

    tempSheet.Range("A1").PasteSpecial xlPasteAll

    Application.CutCopyMode = False

    'This sub copies all visible cells from a filtered table and pastes them to the new sheet called "Temp"

End Sub

Sub FindBearing(ByRef dataSheet As Worksheet, ByRef calculationSheet As Worksheet)

    Dim i As Long
    i = 1

    Do While dataSheet.Cells(i, "A").Value <> Empty

        Dim BearingArray(6) As String
        With dataSheet
            BearingArray(0) = .Cells(i, "A").Value
            BearingArray(1) = .Cells(i, "B").Value
            BearingArray(2) = .Cells(i, "C").Value
            BearingArray(3) = .Cells(i, "D").Value
            BearingArray(4) = .Cells(i, "E").Value
            BearingArray(5) = .Cells(i, "F").Value
            BearingArray(6) = .Cells(i, "G").Value
        End With

        With calculationSheet
            .Cells(17, "K").Value = BearingArray(0)
            .Cells(19, "O").Value = BearingArray(1)
            .Cells(20, "O").Value = BearingArray(2)
            .Cells(23, "O").Value = BearingArray(3)
            .Cells(22, "O").Value = BearingArray(4)
            .Cells(26, "O").Value = BearingArray(5)
            .Cells(17, "L").Value = BearingArray(6)
        End With

        i = i + 1

        If calculationSheet.Cells(17, "M").Value = "PASS" Then
            Exit Do
        End If

    Loop

    If dataSheet.Cells(i, "A").Value = Empty Then
        MsgBox "No available bearing."
    End If

End Sub

Now, if either of those names change, you only have to change the declaration in one place, and the rest of your code will continue running perfectly. Notice how we don't even have to name the tempSheet. We just create it directly into a worksheet object.

And how about this:

        If calculationSheet.Cells(17, "M").Value = "PASS" Then
            Exit Do
        End If

What is the significance of the particular cell? How do we know what it's supposed to be? And how do we check that it is what it's supposed to be?

Depending on your workbook, my first suggestion would be to make that cell a Named Range.

If we named that cell "ValidateBearing" then we could do the following:

    Dim validationCell As Range
    Set validationCell = ThisWorkbook.Names("ValidateBearing").RefersToRange

    Const VALIDATION_PASS_TEXT As String = "PASS"

    Do While dataSheet.Cells(i, "A").Value <> Empty

        ...

        ...

        If validationCell.Text = VALIDATION_PASS_TEXT Then
            Exit Do
        End If

And now, not only have we made it clear what that cell is and what it's doing, but that cell can now move around as much as it likes. As long as nobody actually deletes the row/column/sheet it's in, that code will always point to the right place.

Other tips & Tricks

Do While dataSheet.Cells(i, "A").Value <> Empty

should be

Do While Not IsEmpty(dataSheet.Cells(i, 1))

    If dataSheet.Cells(i, "A").Value = Empty Then
        MsgBox "No available bearing."
    End If

This is hacky. Add a proper Boolean variable to check for conditions:

    Dim foundBearing As Boolean
    foundBearing = False

    Do While Not IsEmpty(dataSheet.Cells(i, 1))

        ...

        ...

        If validationCell.Text = VALIDATION_PASS_TEXT Then
            foundBearing = True
            Exit Do
        End If

    Loop

    If Not foundBearing Then
        MsgBox "No available bearing."
    End If

You can drop .Select everywhere and always.

A.B.Select
Selection.DoThing

Is always the same as just doing

A.B.DoThing

Here:

    NewTable.Range.SpecialCells(xlCellTypeVisible).Select
    NewTable.DataBodyRange.SpecialCells(xlCellTypeVisible).Copy

You can drop the first line entirely and it will still run perfectly fine.

Get used to using Sheet.Cells(row, column) instead of Sheet.Range("[Address]"). Not only is it faster, but it's semantically closer to what cells actually are, and you can't do things like this anywhere near as easily with addresses:

Dim startRow As Long
startRow = 1

Dim finalRow  As Long
finalRow = [Get final Row]

Dim startColumn As Long
startColumn = 1

Dim finalColumn As Long
finalColumn = [Get final Column]

Dim iRow As Long
Dim iColumn As Long

For iRow = startRow To finalRow
    For iColumn = startColumn To finalColumn

        With [Sheet].Cells(iRow, iColumn)

            [Do Stuff]

        End With

    Next iColumn
Next iRow


Generic class to generate combinations As junior java programmer I am working with Euler project and I would like to create a general purpose class which generates all the combinations from the length and number of choosen elements. The class returns a list of boolean arrays with length of length. Each array has choosen true value which highlight the chosen elements the others are false. I have a working solution (I mean it is tested to small numbers) but I wonder is there any opportunity to improve the performance or improve some other aspect. Here it is:

public static List<boolean[]> generate(int length, int choosen){
    List<boolean[]> variationList = new ArrayList<>();
    boolean[] actual = initArray(length, choosen);
    variationList.add(actual.clone());
    while(incrementArray(actual)) {
        variationList.add(actual.clone());
    }
    return variationList;
}

private static boolean[] initArray(int length, int choosen) {
    boolean[] array = new boolean[length];
    for (int index=0; index<choosen; index++) {
        array[index] = true;
    }
    return array;
}

private static boolean incrementArray(boolean[] actual) {
    for(int index = actual.length-1; index > -1 ; index--) {
        if(actual[index]) {
            if(moveForward(actual, index)) {
                return true;
            }
        }
    }
    return false;
}

private static boolean moveForward(boolean[] actual, int position) {
    if(position<actual.length-1 && !actual[position+1]) {
        actual[position] = false;
        actual[position+1] = true;
        copyBack(actual, position);
        return true;
    }else {
        return false;
    }
}

private static boolean copyBack(boolean[] actual, int position) {
    boolean answer = false;
    int delay = 2;
    for (int index = position+2; index<actual.length;index++) {
        if(actual[index]) {
            actual[index] = false;
            actual[position + delay] = true;
            delay++;
            answer = true;
        }
    }
    return answer;
}

Edit

Euler project was only the motive I would like to find a way independently. My aim was produce all of the combinations (by the way my solution for the original problem needed them all (not just the number of them) but there is a workaround which no need combination at all).
Some example of the method:

CombinationGenerator.generate(5, 1);

The result:

true false false false false
false true false false false
false false true false false
false false false true false
false false false false true

Invocation:

CombinationGenerator.generate(5, 3)

The result:

true true true false false
true true false true false
true true false false true
true false true true false
true false true false true
true false false true true
false true true true false
false true true false true
false true false true true
false false true true true

BitSet

A BitSet is deliberately designed to be a memory efficient way to hold Boolean values.

It also has initialize methods (clear and set) already, so no need for initArray.  In combination with nextSetBit and nextClearBit (or the previous variants), those methods can replace moveForward.  And copyBack isn't necessary without moveForward.

The single increment for loop in incrementArray would not be necessary with a BitSet.  Again, the nextSetBit and previousSetBit methods would allow you to jump through multiple steps at once.

I haven't compared performance, but generally built-in methods are more efficient than custom methods.  The counter-argument here might be that a BitSet is more space efficient than time efficient.  Or that using the built-in methods involves algorithmic tradeoffs.  You'd have to test to see.

Nitpick

Choosen is not a word.  Although the base word is choose, the past participle is chosen with a single o.  But the variable doesn't really hold what has been chosen.  It holds a count of how many choices should be set to true.  The BitSet definition suggests that the best name for this is cardinality.  At least that's the name of the BitSet method.

BitSet also uses size to represent the total number of choices.  And uses length for something else.  Consider switching the parameter name length to size to match.  Not because length is a bad name, just inconsistent with BitSet.


Classifying rows of data by the number of days to close DataTable looks like below

I would like to simplify this code in a better manner.

DataTable dt = results.Results[0].DataTable;
dt.Columns.Add("Averagedays", typeof(string));
dt.Columns.Add("0to3", typeof(string));
dt.Columns.Add("4to7", typeof(string));
dt.Columns.Add("Gtrthan7", typeof(string));

foreach (DataRow row in dt.Rows)
{
    if (row["total_daysclose"] != System.DBNull.Value && row["closed_in0to3_count"] != System.DBNull.Value
        && row["closed_in4to7_count"] != System.DBNull.Value && row["closedGtr7_count"] != System.DBNull.Value
        && row["Closed"] != System.DBNull.Value)
    {
        if (Convert.ToDecimal(row["closed"]) > 0)
        {
            decimal value = Convert.ToDecimal(row["total_daysclose"]) / Convert.ToDecimal(row["Closed"]);
            row["Averagedays"] = string.Format("{0:0.#}", Math.Floor(value * 10) / 10);
            row["0to3"] = string.Format("{0:P1}", Convert.ToDecimal(row["closed_in0to3_count"]) / Convert.ToDecimal(row["Closed"]));
            row["4to7"] = string.Format("{0:P1}", Convert.ToDecimal(row["closed_in4to7_count"]) / Convert.ToDecimal(row["Closed"]));
            row["Gtrthan7"] = string.Format("{0:P1}", Convert.ToDecimal(row["closedGtr7_count"]) / Convert.ToDecimal(row["Closed"]));

        }
        else
        {
            row["Averagedays"] = string.Format("{0:0.#}", 0);
            row["0to3"] = string.Format("{0:P1}", 0);
            row["4to7"] = string.Format("{0:P1}", 0);
            row["Gtrthan7"] = string.Format("{0:P1}", 0);

        }
    }
}

In addition to Heslacher's review, I'd also move the initial if-check to a separate function:

private bool IsRowInvalid(DataRow row)
{
    return row["total_daysclose"] == System.DBNull.Value
           || row["closed_in0to3_count"] == System.DBNull.Value
           ||row["closed_in4to7_count"] == System.DBNull.Value
           || row["closedGtr7_count"] == System.DBNull.Value
           ||row["Closed"] == System.DBNull.Value;
}

This method you then call like this:

foreach (DataRow row in dt.Rows)
{
    if (IsRowInvalid(row))
    {
        continue;
    }

    // other logic here
}

I would also rethink names like "0to3" and "total_daysclose" if possible: these naming styles are inconsistent and IMHO bad; I'd much prefer "ZeroToThree" and "TotalDaysClose". "Gtrthan7" is even worse, since it contains an abbreviation and I really can't see any reason why.

Note that I would also move column names to constants, perhaps even move those to a static class, e.g.

internal static class ColumnName
{
    public const string AverageDays = "AverageDays";
    public const string Closed = "Closed";
}

I didn't implement this in the method above because it was too much work.

I just noticed your edit, and I'm wondering whether the NULL checks are even necessary, since I don't see any NULLs in your sample output. I also agree with the comments that this code indicates issues elsewhere, and that simply focusing on this method is not enough.


Creating zip but for dictionaries I found myself needed zip but for dictionaries, and wanted to know if there is any way I can make this shorter and more concise, and/or faster. Also I cannot figure out what the type hinting might be for the return (I would like to put it in the reStructuredText).

def dict_zip(*args):
    """The zip utility but for dictionaries

    :param args:
        A bunch of **identical** dicts
    :type args:
        dict
    :return:
        A dict of all the keys in the input dict,
        mapped to a list of all the values fro that key
    :rtype:
        dict
    """
    out = {i: [] for i in args[0]}
    for dic in args:
        for key in out:
            out[key].append(dic[key])
    return out

if __name__ == "__main__":
    dict1 = {"first": True, "name": "John"}
    dict2 = {"first": False, "name": "sue"}
    print dict_zip(dict1, dict2)

Output:

{'name': ['John', 'sue'], 'first': [True, False]}

A shorter version, functionally equivalent to yours:

def dict_zip(*dicts):
    return {k: [d[k] for d in dicts] for k in args[0].keys()}

That's assuming all dicts have the same keys, or more exactly, all dicts have at least all the keys present in the first dict.

To make it more robust and handle cases when dicts don't have the same keys:

def dict_zip(*dicts):
    all_keys = {k for d in dicts for k in d.keys()}
    return {k: [d[k] for d in dicts if k in d] for k in all_keys}

Regarding type hints: not sure, but it might be something based on typing.MutableMapping. But that's Python 3.

EDIT

To add padding for keys which are not present in all dicts:

def dict_zip(*dicts, fillvalue=None):
    all_keys = {k for d in dicts for k in d.keys()}
    return {k: [d.get(k, fillvalue) for d in dicts] for k in all_keys}

If you have to use Python 2 (if you can, upgrade to Py3 for your own good):

def dict_zip(*dicts, **kwargs):
    fillvalue = kwargs.get('fillvalue', None)
    all_keys = {k for d in dicts for k in d.keys()}
    return {k: [d.get(k, fillvalue) for d in dicts] for k in all_keys}

Notice that you could just do kwargs.get('fillvalue') and if 'fillvalue' is not in kwargs, get would return None anyways. But explicit is better than implicit.