HackerRank, Cracking the Coding Interview: Arrays - Left Rotation I just wanted to get some constructive feedback on my solution for this problem taken from the HackerRank Cracking the Coding Interview series.

CTCI Arrays Left Rotation Problem Statement

#include <map>
#include <set>
#include <list>
#include <cmath>
#include <ctime>
#include <deque>
#include <queue>
#include <stack>
#include <string>
#include <bitset>
#include <cstdio>
#include <limits>
#include <vector>
#include <climits>
#include <cstring>
#include <cstdlib>
#include <fstream>
#include <numeric>
#include <sstream>
#include <iostream>
#include <algorithm>
#include <unordered_map>

using namespace std;

vector<int> array_left_rotation(vector<int> a, int n, int k) {
    vector<int> b(a.size());
    for(int i = 0, len = a.size(); i < len; i++) {
        int swapValue = (i + len - k) % len;
        b[swapValue] = a[i];
    }

    return b;
}

int main(){
    int n;
    int k;
    cin >> n >> k;
    vector<int> a(n);
    for(int a_i = 0;a_i < n;a_i++){
        cin >> a[a_i];
    }
    vector<int> output = array_left_rotation(a, n, k);
    for(int i = 0; i < n;i++)
        cout << output[i] << " ";
    cout << endl;
    return 0;
}

Don't use:

using namespace std;

See: Why is “using namespace std” considered bad practice?

Only include the headers you need. Also organizing them by length of name does not seem to have any benefits. Personally I group them by function others sort them alphabetically. Pick something logical.

Pass large objects by const reference rather than value.

vector<int> array_left_rotation(vector<int> const& a, int n, int k) {
                                      ///   ^^^^^^

If you pass by value as in the code above. The compiler needs to add code to copy the object from the main function into array_left_rotation().

Prefer pre-increment:

for(int i = 0, len = a.size(); i < len; i++)
                                     // ^^^ prefer ++i

When using integers it makes no difference. But for other types it can potentially make a difference. So by using the pre-increment version you guarantee that you always use the most efficient version no matter what the type.

Not sure why we add len here:

int swapValue = (i + len - k) % len;

This is exactly the same as:

int swapValue = (i - k) % len;

Prefer to always use braces: '{' and '}' when you have a sub statements.

for(int i = 0; i < n;i++)
    cout << output[i] << " ";

I would have used this:

for(int i = 0; i < n;i++) {
    cout << output[i] << " ";
}

Prefer \n over std::endl. THe only difference between the two is that std::endl will flush the stream. Since the code will automatically flush the stream at appropriate points (doing it manually usually results in efficiencies).


How to prevent SQL injection PHP and MySQL? I have been reading about SQL injection and I want to secure my code.

I am not asking anyone to write me a code, but I just want to learn it in simple terms.  The best way for me to learn is to edit my code so I can compare them.

<?php
if (isset ($_POST['email']))  {
    //Connect to the database through our include
    include_once "config/connect.php";
    $email = stripslashes($_POST['email']);
    $email = strip_tags($email);
    $email = mysqli_real_escape_string($db_conx, $email);
    $password = preg_replace("[^A-Za-z0-9]", "", $_POST['password']); // filter everything but numbers and letters
    $password = md5($password);
    // Make query and then register all database data that -
    // cannot be changed by member into SESSION variables.
    // Data that you want member to be able to change -
    // should never be set into a SESSION variable.
    $sql = "SELECT * FROM members WHERE email='$email' AND password='$password'";
    $query = mysqli_query($db_conx, $sql);
    $login_check = mysqli_num_rows($query);
    if($login_check > 0){
        while($row = mysqli_fetch_array($query, MYSQLI_ASSOC)){
            // Get member ID into a session variable
            $id = $row["id"];
            session_register('id');
            $_SESSION['id'] = $id;
            // Get member username into a session variable
            $username = $row["username"];
            $email = $row["email"];
            $password = $row["password"];
            $firstname = $row["firstname"];
            $lastname = $row["lastname"];
                    session_register('username');
            session_register('firstname');
            session_register('lastname');
            // Update last_log_date field for this member now
            $sql = "UPDATE members SET lastlogin=now() WHERE id='$id'";
            $query = mysqli_query($db_conx, $sql);
            // Print success message here if all went well then exit the script
            header("location: members/index.php?id=$id");
            exit();
        } // close while
    } else {
    // Print login failure message to the user and link them back to your login page
    header("location: login.php");
        exit();
    }
}
?>

So, you want to defend against possible injection attacks. That's great. But I struggle to understand why you're bothering to escape all parameters in various ways, but then let the whole thing down by not using the easiest, fastest and rather safe tool you have: prepared statements.

First off, are prepared statements 100% safe, all of the time? No, I'm not going to postulate that. Simply because I don't have a solid basis (as in proof) that it is.
Have I seen injection attacks of the first order that managed to circumvent the security gotten from prepared statements? Nope. Never. So, as long as you're not using mysqli_multi_query, don't use user-data in tandem with data gotten from another query and you don't allow yourself to get bogged down in endless sub-queries or JOIN queries without aliases, you should be pretty safe.

So, first step in defending against inject: use prepared statements.

Now, look at what you're doing with your data:

$email = stripslashes($_POST['email']);
$email = strip_tags($email);
$email = mysqli_real_escape_string($db_conx, $email);
$password = preg_replace("[^A-Za-z0-9]", "", $_POST['password']);
$password = md5($password);

Email:
What's wrong with how you process the email? A couple of things.

stripslashes: can potentially change email addresses that are perfectly valid! However unusual an email address is, you have the responsibility to write code that can handle valid input.
This is a valid email address: "very.(),:;<>[]\".VERY.\"very@\\ \"very\".unusual"@strange.example.com, believe it or not. stripslashes, then, is NOT an option. See other wacky email addresses here.
striptags: I take it that, from the example here, and on the wiki I linked to, it's self-evident that this function call is right out, for the same reasons.

What to do, then? Simple. PHP comes with an email validator function out of the box, simply use that:

if (filter_var($_POST['email'], FILTER_VALIDATE_EMAIL))
    $email = $_POST['email'];
else
    //error.

If you want to be lenient towards your users, which I'd encourage, you can add a sanitation call to this code. If the email address posted contains invalid chars, then notify the user and ask him/her if he meant to write a valid version of the address:

if (!filter_var($_POST['email'], FILTER_VALIDATE_EMAIL))
{//not the !<-- not valid
    echo $_POST['email'], ' is not a valid email address. Did you mean: ',
        filter_var($_POST['email'], FILTER_SANITIZE_EMAIL);//<-- sanitize
}

Password:
What's wrong with the password? A few things.

first off: you're going to hash the password. An md5 hash is guaranteed to contain only hex chars (0-9a-f). These chars pose no risk at all in terms of injection.
If you want to inform the user about him/her using an "invalid" password, however, a regex can come in handy. However, the one you're using now is not going to work!
preg_replace: You are not replacing what you thing you are. In PHP, a regex has to be delimited by special chars (for example: /a/ matches a). Of course, delimiters like [ and ] are allowed, too. That means that PHP doesn't interpret your pattern as a character class, but rather.

It is translated to this:

preg_replace('/^A-Za-z0-9/', '', $string);

You're matching the beginning of the string, followed by the literal string: A-Za-z0-9. This will be replaced, the rest is left as is.
Just test:

echo preg_replace("[^A-Za-z0-9]", '', 'Hello World!');

Which should replace the space and exclamation mark. The actual output, however, is: Hello World!.
I take it you were actually going for something along the lines of:

echo preg_replace('/[^A-Za-Z0-9]/', '', 'Hello World!');

Which echoes HelloWorld. But actually, it's a good thing this code doesn't work. You're actually prohibiting users from entering a safer password. Which pass, according to you (and according to common sense) would be most likely to be cracked using a simple dictionary attack:

mySimplePass
//or
|\/|y=5!mpL3_P@55

Allow complex passes, you have no reason not to.

md5: Ok, it's good to see you're hashing passwords. It really is. But md5 is deemed unsafe. Collisions have been spotted in the wild, and thus it's likely that there are several passwords that will be accepted as valid for a single hash.
sha1 is a safer option, but best switch to blowfish.

Another niggle when using hash functions is: never simply hash the pass. use salt. If I were to create a dummy account, and at some point saw the hash you were using, I could figure out what type of hashing algorithm you are using, and I would know you're not using salt.
In that case, all I need is to see someone elses salt and set my machine to work, generating random strings and computing the hash. For every match I find, I have access to another users' account on your site.

Couple this to your policy of forcing people to use weak passes and you must admit, you have a huge security problem on your hands.
In case you don't know what I mean by salt:

$hash = sha1($pass, 'S0me->53cr3t<>5tr!~');

That way, I'd first have to generate random strings, in the off chance I find the salt, which is of unknown length, can contain any char, and even when I get a matching hash (in case of md5 especially), I could be dealing with a false positive (collision...).

Moving on: Like I said: use prepared statements, instead of just stringing arguments into a query string, for starters. Then, do away with the mysqli_num_rows call. All your code is contained within a while loop. If there are no results to be found, the loop body won't be executed. Instead, I'd simply write a single query:

$stmt = mysqli_prepare(
    $db_conx,
    'UPDATE members
        SET lastlogin=NOW()
    WHERE email = ?
      AND password = ?'
);
//after validation, of course
mysqli_stmt_bind_param($stmt, 'ss', $email, $pass);
mysqli_stmt_execute($stmt);
if (mysqli_affected_rows($db_conx))
{
    mysqli_stmt_close($stmt);//<-- CLEAN UP AFTER YOURSELF!
    //update was successful
}

Of course, this code doesn't select username, first and last name from the table. If you want to do so, then you'll have to keep the second query, still.

There, too, your code can be improved on. SELECT * is something that can, and should be avoided as much as possible: only SELECT what you need!
If the supplied email and pass are valid, you already have that data, so there's no reason to query for it again. You only need 4 fields: the id, user-, first- and last name.
Not only is selecting what you need easier on your server, it makes life of the people who end up maintaining your code (and that includes you, yourself) a lot easier if ever something needs to be changed/added a couple of weeks from now:

$stmt = mysqli_prepare(
    $db_conx,
    'SELECT id, username, firstname, lastname
     FROM members
     WHERE email = ?
       AND password = ?'
);
mysqli_bind_param($stmt, 'ss', $email, $pass);
if (!mysqli_stmt_execute($stmt))//returns false on success
    throw new RuntimeException('Failed to execute statement');
//get the result first
$result = mysqli_stmt_get_result($stmt);
//or $row = mysqli_fetch_assoc($result)
if ($row = mysqli_fetch_array($result, MYSQLI_ASSOC))
{
    $_SESSION['id'] = $row['id'];//and so on
    //AND CLEAN UP!!
    mysqli_free_result($result);
    mysqli_stmt_close($stmt);
}

As you can see, the mysqli_* extension offers a lot of powerful features, and tools. In return, it does require you to take care of a couple of things: freeing the results, closing the statements when you're done playing with them etc...
It takes some getting used to at first, but it's not that hard, really.

If you find this a bit too wearisome (can't blame you if you do), then take a look at the alternative extension PDO.
It offers a cleaner, less verbose API. The trade-off: it's OO-only (IMO, a good thing), slightly less powerful and marginally slower, but not as if you'd notice. Other things are far more likely to be the bottleneck.


Windows filepath and filename validation I have a Windows program that prompts the user to input a file path and filename. I then check that the file exists with Directory.Exists. Then the user inputs a filename and I check it using this answer to check whether the filename is valid or not (it may or may not yet exist).

using System;
using System.IO;
using System.Text.RegularExpressions;

public static string GetFilePath()
{
    string filePath = " ";
    bool directoryFound = true;
    do
    {
        if (!directoryFound)
        {
            Console.WriteLine("This directory not found:\n{0}\nPlease double check file path. \nPlease Double check that this program has read and write access to the directory.\n", filePath);
        }

        Console.WriteLine("Please specify file location:");
        filePath = @Console.ReadLine();
        Console.Clear();

        directoryFound = Directory.Exists(filePath);
        if (filePath.Length >= 240) //This looks like a bad solution
        {
            directoryFound = false;
            Console.WriteLine("Please keep the filepath under 240 chars so that you still are able to provide a name for the file.");
        }

    } while (!directoryFound);

    filePath = GetFileName(filePath);

    return filePath ;
}

    public static string GetFileName(string filePath)
    {
        string fileName = " ";
        bool isValidName = true;
        int fileNameMaxLength = 0;
        do
        {
            if (!isValidName)
            {

                Console.WriteLine("{0} \nis not a valid filename. Make sure you enter a valid filename.\nYour max filename length should be {1}", fileName, fileNameMaxLength);
            }

            Console.WriteLine("Please enter the name of the file:");
            fileName = Console.ReadLine();
            Console.Clear();

            string illegalChars = @"^(?!^(PRN|AUX|CLOCK\$|NUL|CON|COM\d|LPT\d|\..*)(\..+)?$)[^\x00-\x1f\\?*:\"";|/]+$";

            isValidName = Regex.IsMatch(fileName, illegalChars, RegexOptions.CultureInvariant);

            if (fileName.Length + filePath.Length > 254) //254 instead of 260 in order to account for file extension
            {
                isValidName = false;
                fileNameMaxLength = 254 - filePath.Length;
            }

        } while (!isValidName);
        return Path.Combine(filePath, (fileName + ".ppm"));
    }

}

How could I make this be more elegant? I especially don't like:

if(filePath.Length >= 240)

I need to make sure that:

The filepath exists
The filename is valid
That filename + filepath <= 260 (this is max char limit on Windows)
That my program doesn't crash while doing this

For the if(filePath.Length >= 240) part, you can use the Path object.
It throws a PathTooLongException.

You can then change to this :

try
{
    filePath = Path.GetFullPath(filePath);
}
catch (PathTooLongException ex)
{
    directoryFound = false;
    Console.WriteLine("Please keep the filepath under 240 chars so that you still are able to provide a name for the file.");
}

Note : you can find references to Path object here and see all exception you have to handle invalid path input.

Now for GetFileName(), you can use the same object Path.GetFileName(). It will check if the name is valid.

  ArgumentException : path contains one or more of the invalid characters defined in GetInvalidPathChars.

...
fileName = Console.ReadLine();
            Console.Clear();
try
{
    fileName = Path.GetFileName(fileName);
}
catch (ArgumentException ex)
{
    // Whatever exception handling you want.
}

Note : Since you have to check more than one time if the Path is valid, I would create a method to do this.

public static bool PathIsValid(string inputPath)
{
    try
    {
        Path.GetFullPath(inputPath);
    }
    catch(PathTooLongException ex)
    {
        return false;
    }
    return true;
}


Function simulating single elevator movement I am trying to write code for simulating movement of single elevator. This was a question to me and was pretty straightforward as the use-case was explained properly. So the problem contains following data:

  inputFloors : all floors elevator should go to. e.g. {5, 2, 9, 3, 8, 1}

  currentFloor : current floor where elevator is standing/user is entering the elevator. e.g. 6

  direction : direction of elevator movement like UP or DOWN.

Now since the user is entering at floor 6 and wants to go down then the route of the elevator should be 5, 3, 2, 1, 8, 9. That means the elevator should first go down the lesser floors in descending order and then go up the higher floors in ascending order. Similarly if the user wants to go up then the route should be 8, 9, 5, 3, 2, 1.

I wrote the code and it looked like a pretty simple use-case to me. But I wanted to see if there is any better way to do this. I am brushing up my c++ basics so this is the best solution i could think of.

My Code :

std::vector<int> singleElevatorSystem (int inputFloors[], int size, int currentFloor, string direction) {
  std::vector<int> High, Low;

  for (int i = 0; i < size; i++) {
    if (currentFloor < inputFloors[i]) {
      High.push_back (inputFloors[i]);
    } else if (currentFloor > inputFloors[i]) {
      Low.push_back (inputFloors[i]);
    }
  }

  std::sort (High.begin(), High.end());
  std::sort (Low.begin(), Low.end());

  if (direction.compare("UP") == 0) {
    High.insert(High.end(), Low.rbegin(), Low.rend());
    return High;
  } else if (direction.compare("DOWN") == 0) {
    std::reverse (Low.begin(), Low.end());
    Low.insert (Low.end(), High.begin(), High.end());
    return Low;
  }
}

int main () {
  int inputFloors[] = {5, 2, 9, 3, 8, 1};
  int currentFloor = 6;
  //string direction("UP");
  string direction("DOWN");

  std::vector<int> outputFloors = singleElevatorSystem (inputFloors, 6, currentFloor, direction);
  std::cout << "Elevator will stop at requested floors in sequence : ";
  for (int i = 0; i < 6; i++) {
    std::cout << outputFloors.at(i) << "  ";
  }

  std::cin.get();
  return 0;
}

I hardcoded the size of array and current floor value.

Any comments or updates please??

As a C++ learning exercise, you will benefit from creating an Elevator class in this case. Make it a class that has a the current floor and direction as a data members (those are the current states of the elevator), and a function PlanRoute that gets input floors as input, and returns the output floors.

The h-file will look something like this:

class Elevator {
public:
  Elevator();
  void setCurrentFloor(int currentFloor);
  void setDirection(bool goingDown);
  std::vector<int> planRoute(const std::vector<int>& inputFloors)
private:
  bool goingDown;
  int currentFloor;
}


Shorter code for adding checkboxes How can I make this code shorter?

function tblcheckboxes(){
    var a= 0;
    var b= 0;
    var c= 0;
    var d= 0;
    var e= 0;
    var f= 0;
    var g= 0;

    if ($('#tcbx1').is(":checked")) {
         a = parseFloat($("#tcbx1").val(), 10);
    }
    if ($('#tcbx2').is(":checked")) {
         b = parseFloat($("#tcbx2").val(), 10);
    }
    if ($('#tcbx3').is(":checked")) {
         c = parseFloat($("#tcbx3").val(), 10);
    }
    if ($('#tcbx4').is(":checked")) {
         d = parseFloat($("#tcbx4").val(), 10);
    }
    if ($('#tcbx5').is(":checked")) {
         e = parseFloat($("#tcbx5").val(), 10);
    }
    if ($('#tcbx6').is(":checked")) {
         f = parseFloat($("#tcbx6").val(), 10);
    }
    if ($('#tcbx7').is(":checked")) {
         g = parseFloat($("#tcbx7").val(), 10);
    }

    var total = a + b + c + d + e + f + g;
    $('.txt7').val(total.toFixed(2));
}

$(document).ready(function(){
        $('#tcbx1').click(function(){
                tblcheckboxes();
                grandtotal();
        });
        $('#tcbx2').click(function(){
                tblcheckboxes();
                grandtotal();
        });
        $('#tcbx3').click(function(){
                tblcheckboxes();
                grandtotal();
        });
        $('#tcbx4').click(function(){
                tblcheckboxes();
                grandtotal();
        });
        $('#tcbx5').click(function(){
                tblcheckboxes();
                grandtotal();
        });
        $('#tcbx6').click(function(){
                tblcheckboxes();
                grandtotal();
        });
        $('#tcbx7').click(function(){
                tblcheckboxes();
                grandtotal();
        });
});

You could try to do the whole thing on the fly, like this:

var total = 0;
$("input.tcbx").change(function(){
    if($(this).is(":checked")) {
        total += parseFloat($(this).val(), 10);
    } else {
        total -= parseFloat($(this).val(), 10);
    }
    $('.txt7').val(total.toFixed(2));
});

Just add a class to all your checkboxes, and then either add the value to your total or reduce it as they get selected. If you have preselected values you might want to enter the preselected total already, otherwise you could use an each() to first check all the values.

$(document).ready(function(){
    var total = 0;
    $("input.tcbx").change(function(){
     if($(this).is(":checked")) {
      total += parseFloat($(this).val(), 10);
     } else {
      total -= parseFloat($(this).val(), 10);
     }
     $('#result').val(total.toFixed(2));
    });
});

<input type="checkbox" value="1" id="check-1" class="tcbx" />
1
<input type="checkbox" value="2" id="check-2" class="tcbx" />
2
<input type="checkbox" value="3" id="check-3" class="tcbx" />
3
<input type="text" id="result">0


Convert float price to integer price without commas My payment gateway wants the prices this way: "1050" instead of 10.50.
So I created this function:

https://repl.it/HgHI/3

def price_format_gateway(price):
    price = "{0:.2f}".format(price)
    price = price.split(".")
    try:
      if len(price[1]) > 2:
        decimals = str(price[1][0:2])
      else:
        decimals = price[1]
    except IndexError:
      pass

    return str(price[0]) + str(decimals)

price_format_gateway(10) # Expected -> 1000
price_format_gateway(10.1) # Expected -> 1010
price_format_gateway(10.15765) # Expected -> 1016

Is there another way more elegant, or cleaner? I just want to improve.

For me at least, it would be more natural to think of the operation as a multiplication by 100:

def price_format_gateway(price):
    return '{:.0f}'.format(100 * price)


Seconds -> Days, Hours, Minutes, and Seconds This is just a simple function to convert seconds to days, hours, minutes and seconds for a larger app I'm going to be building. I'm just looking for thoughts on it.

import math

def convert_seconds(x):

    # Day's
    days = x / 86400
    fraction, whole = math.modf(days)
    days = int(whole)

    # Hours
    hours = fraction * 24
    fraction, whole = math.modf(hours)
    hours = int(whole)

    # Minutes
    minutes = fraction * 60
    fraction, whole = math.modf(minutes)
    minutes = int(whole)

    # Seconds
    seconds = fraction * 60
    fraction, whole = math.modf(seconds)
    seconds = int(whole)

    return days, hours, minutes, seconds

if True:
    print(convert_seconds(1043573894573948573))

Your code is lacking in the regards that you do floating arithmetic when you don't need to, and this potentially introduces smaller errors as you multiply back the different intervals.

With that being said your code looks clean enough, well spaced and good variable names. Only thing missing as such would be a docstring to document the functions return value as the name is somewhat ambiguous.

To avoid the floating arithmetic, and ease the calculation I would flip the order of doing divmod operations, and then we end up with the following code:

def convert_to_d_h_m_s(seconds):
    """Return the tuple of days, hours, minutes and seconds."""

    minutes, seconds = divmod(seconds, 60)
    hours, minutes = divmod(minutes, 60)
    days, hours = divmod(hours, 24)

    return days, hours, minutes, seconds

if __name__ == '__main__':

    for seconds in [ 11, 61, 3601, 3668, 86400, 86401, 90061, 1043573894573948573 ]:
        print("{0} seconds: {1[0]} days, {1[1]} hours, {1[2]} minutes, {1[3]} seconds".format(
              seconds, convert_to_d_h_m_s(seconds)))

Notice how this also uses the if __name__ == '__main__': idiom to pack the test code used. This would allow for the convert function to be imported from another script if imported. The output of this code is:

11 seconds: 0 days, 0 hours, 0 minutes, 11 seconds
61 seconds: 0 days, 0 hours, 1 minutes, 1 seconds
3601 seconds: 0 days, 1 hours, 0 minutes, 1 seconds
3668 seconds: 0 days, 1 hours, 1 minutes, 8 seconds
86400 seconds: 1 days, 0 hours, 0 minutes, 0 seconds
86401 seconds: 1 days, 0 hours, 0 minutes, 1 seconds
90061 seconds: 1 days, 1 hours, 1 minutes, 1 seconds
1043573894573948573 seconds: 12078401557568 days, 20 hours, 22 minutes, 53 seconds


Generating the pretty bit at the end of a URL I'm having a dip into F# and am attempting not to write it like it's C#.

One area that bothers me is using members of System.String. These often need to be chained together, but the only way to do so that I've seen so far is pretty much the same as C#.

For example, I've written a function to generate the pretty bit at the end of a URL (similar to SE):

module String =

    open System.Text.RegularExpressions

    ///Makes a string suitable for use as part of a URL:
    ///Removes syntax, replaces spaces with dashes and lowers case,
    ///then trims first word after 30 chars
    let prettify (x:string) =

        let parsed = Regex.Replace(x.Trim(),"[^0-9a-zA-Z ]","").Replace(" ","-").ToLower()

        match String.length parsed with
            | m when m > 30 ->
                match parsed.IndexOf("-",30-1) with
                    | p when p < 0 -> parsed
                    | p -> parsed.[..p]
            | _ -> parsed

Is there a way to refactor this to be "better F#"?  I'm particularly concerned about how to better handle the methods on System.String and Regex.Replace. (The match part is a little convoluted to handle the edge case of the last word being the one that takes it over 30 chars.)

F# does have a String module, but it's almost empty. If you want to make string manipulations look more idiomatic, you'll have write functions that wrap System.String methods yourself (quick search didn't find anything existing):

let trim (input:string) =
    input.Trim()

let regexReplace (pattern:string) (replacement:string) (input:string) =
    Regex.Replace(input, pattern, replacement)

let replace (pattern:string) (replacement:string) (input:string) =
    input.Replace(pattern, replacement)

let toLower (input:string) =
    input.ToLower()

…

let parsed =
    x
    |> trim
    |> regexReplace "[^0-9a-zA-Z ]" ""
    |> replace " " "-"
    |> toLower

match String.length parsed with
    | m when m > 30 -> …
    | _ -> parsed

You don't need to try to differentiate from C# so hard. if would be more appropriate here:

if String.length parsed > 30 then …
else parsed

match parsed.IndexOf("-",30-1) with
    | p when p < 0 -> parsed
    | p -> parsed.[..p]

You can rewrite this to avoid having to reference p in both branches:

match parsed.IndexOf("-",30-1) with
    | p when p >= 0 -> parsed.[..p]
    | _ -> parsed

Also, use more descriptive names than p, like index or maybe pos.

Also, this code usually returns a string that's over 30 characters and ends with -. Is that what you intended?


Calculate total miles per gallon and average per trip Assignment: Develop a Java application that will input the number of miles driven and gallons used (both as integers) for each trip. The program should calculate and display the miles per gallon obtained for each trip and print the combined miles per gallon obtained for all trips up to this point. All averaging calculations should use floating point results.

See my implementation below.
YES the class name is weird but I didn't feel like changing it.
Does the code satisfy the assignment requirements?
Have I missed the target completely, and if so how far off was I?

 import java.util.Scanner;

    public class MagicNumber
    {
        public static void main(String[] args)
        {
            float AvgMPG = 0;
            int MilesDriven = 0;
            int totalTrips = 0;
            int GallonsUsed = 0;
            int totalMilesPerGallon = 0;
            int MilesPerGallon = 0;

            Scanner input = new Scanner(System.in);
            System.out.println("Enter Miles Driven or -1 to quit:  ");
            MilesDriven = input.nextInt();

            System.out.println("Enter Gallons used to fill tank or -1 to quit:  ");
            GallonsUsed = input.nextInt();

            while ( MilesDriven != -1)
            {
                MilesPerGallon = MilesDriven / GallonsUsed;
                System.out.println("Miles Per Gallon for this trip:  "           +MilesPerGallon);

                totalMilesPerGallon = MilesPerGallon + totalMilesPerGallon;

                totalTrips = totalTrips + 1;

                System.out.println("Enter Miles Driven or -1 to quit:  ");
                MilesDriven = input.nextInt();

                System.out.println("Enter Gallons used to fill tank or -1 to quit:  ");
                GallonsUsed = input.nextInt();
            }
        if (totalTrips != 0)
        {
          System.out.println("Number of trips taken:  "+ totalTrips);

          AvgMPG = (float) totalMilesPerGallon / totalTrips;

          System.out.println("Total Miles Per Gallon for all trips is :"             +totalMilesPerGallon);
          System.out.println("Average Miles Per Gallon Per Trip is :"  +AvgMPG);

                }
                else
                System.out.println("No data entered");

        }
    }

One thing you need to do is methodically test your code.  So far you have only tested your code to 'work', but the point of testing is to find what doesn't work (as intended).

It's something you can also hand in and it's a good practice to get into - you will need it if you program professionally.  As your code grows you might need to look at splitting the user interface of your code from your business logic so that you can test them separately using a test program.

For small pieces of code a test plan is 'good enough' for your needs.

Logic testing - look at your looping and branching conditions to work out which inputs should bring them to each branch.  For example your UI says that inputting -1 gallons used will exit - does it?  Could your program be given zero trips, how should it handle that?

Validation testing - Could you get invalid inputs that make your code do something you didn't expect?  For example is there an opportunity for division by zero given bad input?  If user input could generate an exception then it should be mitigated.

Computational testing - Do you get an appropriate level of accuracy when you are mixing data types?  (another answer has pointed out a bug in that department.)  Are the calculation results what you expect if you work them out manually?  Do you get the correct output (for example at each loop the spec requests the average so far.)

Since your program is quite simple it would be very easy to test well.  But each time you find a bug then don't consider it an isolated problem - when you have 1 bug it usually means you have found a class of bugs and you should check if that bug crops up elsewhere, it frequently does.

Looking at your variables in this code:

totalTrips should probably be just trips since it's just a counter.
totalMilesPerGallon is just added to but you are adding an average.  Either your naming is wrong or your logic is.

This class could use a more meaningful name than MagicNumber as
   well, choosing meaningful names is a clear indicator that you have
   fully understood the problem or need your code is trying to address.
   It's also a handy 'red-flag' that if you can't think up a name easily
   that you may need to check that you understand what you want your code to do more
   clearly before you write any (more).

Looking at code reuse, do you really need all your inputs listed twice?  Is there another way it could be coded to avoid the repetition of the inputs?

Finally, you might want to double-check your formatting, your braces {...} should match up if that's the convention you're following.  Most IDE's will do this for you.

All that should give you a good handle on what you're missing and how your code can improve for now.


Vectorized numpy version of arange with multiple start stop Given a single start and stop, numpy.arange is a good solution for building a NumPy array of evenly spaced values.  However, given an array of start and an array of stop values, I would like to build an array of concatenated evenly spaced values, and do so in C speed (no looping).  Here is my current solution, though I am wondering if there is a NumPy/SciPy function I missed that already does this.

def vrange(starts, lengths):
    """ Create concatenated ranges of integers for multiple start/length

    Args:
        starts (numpy.array): starts for each range
        lengths (numpy.array): lengths for each range (same length as starts)

    Returns:
        numpy.array: concatenated ranges

    See the following illustrative example:

        starts = np.array([1, 3, 4, 6])
        lengths = np.array([0, 2, 3, 0])

        print vrange(starts, lengths)
        >>> [3 4 4 5 6]

    """

    # Repeat start position index length times and concatenate
    cat_start = np.repeat(starts, lengths)

    # Create group counter that resets for each start/length
    cat_counter = np.arange(lengths.sum()) - np.repeat(lengths.cumsum() - lengths, lengths)

    # Add group counter to group specific starts
    cat_range = cat_start + cat_counter

    return cat_range

If you are curious why I need this, it's for building a 1-to-many mapping of intervals to contained positions.

This code looks good to me: the docstring is clear and the implementation is simple and efficient. So I have only a few minor points.

The code doesn't fit in 80 columns, meaning that we have to scroll it horizontally to read it here on Code Review.
The docstring contains an example. If it were formatted like this:

>>> starts = np.array([1, 3, 4, 6])
>>> lengths = np.array([0, 2, 3, 0])
>>> vrange(starts, lengths)
array([3, 4, 4, 5, 6])

then it could be run using the doctest module, allowing you to automatically check that it is correct.
The example in the docstring uses the print statement and so is not compatible with Python 3.
I think it would be clearer to take a stops array instead of a lengths array. It would then have an interface that corresponds closely to range and numpy.arange. Possibly lengths is more convenient for your application, but you can easily calculate stops = starts + lengths.
The docstring says that starts and lengths must be "numpy.array", by which I think you mean numpy.ndarray, but in fact it's OK for starts to be an array_like because the code doesn't call any methods on it.
It would be possible to allow lengths to be array_like too, by calling numpy.asarray.
The implementation requires starts and lengths to be 1-dimensional, so the docstring should mention this.
The implementation carries out a sum that includes (among other terms):

np.repeat(starts, lengths) - np.repeat(lengths.cumsum() - lengths, lengths)

this is the same as:

np.repeat(starts - lengths.cumsum() + lengths, lengths)

which saves a call to numpy.repeat, and this is the same as:

np.repeat(stops - lengths.cumsum(), lengths)

Putting all that together, I get:

def vrange(starts, stops):
    """Create concatenated ranges of integers for multiple start/stop

    Parameters:
        starts (1-D array_like): starts for each range
        stops (1-D array_like): stops for each range (same shape as starts)

    Returns:
        numpy.ndarray: concatenated ranges

    For example:

        >>> starts = [1, 3, 4, 6]
        >>> stops  = [1, 5, 7, 6]
        >>> vrange(starts, stops)
        array([3, 4, 4, 5, 6])

    """
    stops = np.asarray(stops)
    l = stops - starts # Lengths of each range.
    return np.repeat(stops - l.cumsum(), l) + np.arange(l.sum())

This is not quite as clear as your implementation: it's hard to give a concise explanation of what stops - l.cumsum() means. So I can see an argument for preferring the more explanatory version even if it does have an extra call to numpy.repeat.


Rating normalization algorithm I have a rating system where users can vote between 1-6. I'd like to normalize the votes a bit to better accommodate for extreme voting, where individuals tend to vote as high or as low as possible. To do this, I have the following algorithm which seems to work well. However, it's extremely inefficient and not very well written.

What might I do to improve it?

protected function calculateWeightedAverage($ratings)
{
    $values = array_values($ratings);
    sort($values);

    $count = count($values);
    $weights = array_fill(0, $count, 1);

    $out = (int) ($count / 3);
    $out2 = pow($out + 1, 2);

    $max = $count - 1;
    $min = 0;

    for ($i = 0; $i < $out; $i++) {
        $j = $i + 1;
        $weights[$min + $i] = $j * $j / $out2;
        $weights[$max - $i] = $j * $j / $out2;
    }

    $sum = array_sum($weights);

    for ($i = 0; $i < $count; $i++) {
        $weights[$i] /= $sum;
    }

    $rating = 0;
    for ($i = 0; $i < $count; $i++) {
        $rating += $values[$i] * $weights[$i];
    }

    return $rating;
}

Your code is really good and I can't see any significant way to improve it. One minor improvement is to reduce the number of loops from 3 to 2:

protected function calculateWeightedAverage($ratings)
{
  $sum = 0;
  $rating = 0;

  $values = array_values($ratings);
  sort($values);

  $count = count($values);
  $out = (int) ($count / 3);
  $out2 = pow($out + 1, 2);

  for ($i=0; $i<$count; $i++) {
    if ($i < $out) {
      $weights[$i] = pow(($i+1),2)/$out2;
    } elseif ($i > $count-$out-1) {
      $weights[$i] = pow($count-$i,2)/$out2;
    } else {
      $weights[$i] = 1;
    }
    $sum += $weights[$i];
  }

  for ($i = 0; $i < $count; $i++) {
    $rating += $values[$i] * $weights[$i]/$sum;
  }

  return $rating;
}


View model with properties that represent years and months I have a view model with properties that represent years and months:

public IEnumerable<SelectListItem> Years
{
    get
    {
        return new SelectList(
            Enumerable.Range(1900, 112)
            .OrderByDescending(year => year)
            .Select(year => new SelectListItem
            {
                Value = year.ToString(CultureInfo.InvariantCulture),
                Text = year.ToString(CultureInfo.InvariantCulture)
            }
        ), "Value", "Text");
    }
}

public IEnumerable<SelectListItem> Months
{
    get
    {
        return new SelectList(
            Enumerable.Range(1, 12)
            .OrderByDescending(month => month)
            .Select(month => new SelectListItem
            {
                Value = month.ToString(CultureInfo.InvariantCulture),
                Text = month < 10 ? string.Format("0{0}", month) : month.ToString(CultureInfo.InvariantCulture)
            }
        ), "Value", "Text");
    }
}

I definitely use copy & paste approach here.  How can I refactor this code? Maybe somehow passing the numbers as parameters to some helper method?

You can use a format string to ensure that there are at least 2 digits in the format without having to check first.  The format: {0:00} ensures there are at least 2 digits, but allows for more.  It will prefix 0 to any single-digit input.

string.Format also has an overload which takes in an IFormatProvider, so my personal preference is just to collapse that into the string.Format call.  It also happens to line up nicely :)

With that, you can create a method with appropriate parameters which can handle both cases:

  public IEnumerable<SelectListItem> Years2 { get { return BuildList (1900, 112); } }
  public IEnumerable<SelectListItem> Months2 { get { return BuildList (1, 12); } }

  public IEnumerable<SelectListItem> BuildList (int start, int count)
  {
     return new SelectList (
         Enumerable.Range (start, count)
         .OrderByDescending (val => val)
         .Select (val => new SelectListItem
         {
            Value = val.ToString (CultureInfo.InvariantCulture),
            Text = string.Format (CultureInfo.InvariantCulture, "{0:00}", val)
         }
     ), "Value", "Text");
  }

Note: I suffixed my properties with 2 so I could run them side-by-side and compare results to be sure I didn't miss anything.

Note2: as Jesse suggests, you should ideally cache the results.


In DRY Python code, when should I catch/raise exceptions? Given python's principle of duck-typing, when is it best to raise exceptions?

In a simplified case below, say I want to code a couple functions to return booleans based on an input object product. Rather than catching an exception if an unexpected object was entered, I'm splitting functionality to keep things DRY.

However, should I also be catching the raised exception in functions that call the abstracted code?

For the options below, which would be the better approach? Are there better approaches still?

Option 1

TAX_RATE = 0.2

def calculate_price_after_tax(product):
    """Returns Decimal"""
    try:
        net_price = product.price
    except AttributeError:
        raise TypeError('Expected an input of type Product, but received: {0}.'.format(type(product))
    return net_price * TAX_RATE

def is_price_above_100(product):
    """Returns bool"""
    return calculate_price_after_tax(product) > 100

def is_price_above_200(product):
    """Returns bool"""
    return calculate_price_after_tax(product) > 200

Option 2

TAX_RATE = 0.2

def calculate_price_after_tax(product):
    """Returns Decimal"""
    try:
        net_price = product.price
    except AttributeError:
        raise TypeError('Expected an input of type Product, but received: {0}.'.format(type(product))
    return net_price * TAX_RATE

def is_price_above_100(product):
    """Returns bool"""
    try:
        calculate_price_after_tax(product) > 100
    except TypeError as e:
        raise TypeError(e)

def is_price_above_200(product):
    """Returns bool"""
    try:
        calculate_price_after_tax(product) > 200
    except TypeError as e:
        raise TypeError(e)

Your Option 1 and Option 2 are almmost exactly the same (except that Option 2 is more confusing)

This is because if you don't catch an exception, it will just rise to the top, where it will halt your program. Catching and immediately re-raising it is (almost) the same as never catching it at all.

The only difference can be seen here:

In [1]: def f(a, b):
   ...:     return a / b
   ...:

In [2]: def g(a, b):
   ...:     try:
   ...:         return f(a, b)
   ...:     except ZeroDivisionError as e:
   ...:         raise ZeroDivisionError(e)

When calling g(2, 0), the traceback shows us the place where the error was last raised, which is in the function g:

In [3]: g(2, 0)
---------------------------------------------------------------------------
ZeroDivisionError                         Traceback (most recent call last)
<ipython-input-3-9920cf4d7d78> in <module>()
----> 1 g(2, 0)

<ipython-input-2-a48ddeb59e34> in g(a, b)
      3         return f(a, b)
      4     except ZeroDivisionError as e:
----> 5         raise ZeroDivisionError(e)
      6

ZeroDivisionError: integer division or modulo by zero

When defining a third function h, that does not re-raise the exception, this does not happen and we see the full context of calling f from within h:

In [4]: def h(a, b):
   ...:     return f(a, b)
   ...:

In [5]: h(2, 0)
---------------------------------------------------------------------------
ZeroDivisionError                         Traceback (most recent call last)
<ipython-input-5-b455367d475f> in <module>()
----> 1 h(2, 0)

<ipython-input-4-7bfa64a165c7> in h(a, b)
      1 def h(a, b):
----> 2     return f(a, b)

<ipython-input-1-44ecddb27ac3> in f(a, b)
      1 def f(a, b):
----> 2     return a / b

ZeroDivisionError: integer division or modulo by zero

Finally note that if you use just a raise (without the explicit exception type and passing it the exception), it will also behave like the function h:

In [6]: def k(a, b):
   ...:     try:
   ...:         return f(a,b)
   ...:     except ZeroDivisionError:
   ...:         raise
   ...:

In [7]: k(2, 0)
---------------------------------------------------------------------------
ZeroDivisionError                         Traceback (most recent call last)
<ipython-input-7-545f9b93e361> in <module>()
----> 1 k(2, 0)

<ipython-input-6-5891fde4d79f> in k(a, b)
      1 def k(a, b):
      2     try:
----> 3         return f(a,b)
      4     except ZeroDivisionError:
      5         raise

<ipython-input-1-44ecddb27ac3> in f(a, b)
      1 def f(a, b):
----> 2     return a / b

ZeroDivisionError: integer division or modulo by zero

So re-raising the exception in this way just obfuscates where the exception originally occurred. This can be a good or a bad thing, but I would not recommend using it unless that is your intention.


Flash LEDS for POV setup Below is a python script that I wrote to flash neopixel leds at a very fast rate using a raspberry pi0. The main while loops runs at a semi reasonable rate, although faster would be much better. In addition, the button thread slows down the script considerably.  Any ideas on how to make this run faster? While running, it uses 96% of my CPU already.

I am especially hoping for input on how to make this faster.

import time
import csv
from pprint import pprint
from neopixel import *
import RPi.GPIO as GPIO
import threading

B1 = 23
B2 = 24
B3 = 25
GPIO.setmode(GPIO.BCM)
GPIO.setup(B1, GPIO.IN)
GPIO.setup(B2, GPIO.IN)
GPIO.setup(B3, GPIO.IN)

numpats = 4
cpat = 0
DELAY = 0
shuffle = False

def ButtonThread():
    global cpat
    global DELAY
    global shuffle

    while True:
        if GPIO.input(B1):
            cpat = cpat + 1
            if cpat >= numpats:
                cpat = 0
            print(cpat)
            time.sleep(0.5)

        if GPIO.input(B2):
            DELAY = DELAY + .0001
            time.sleep(0.1)

            if DELAY > .05:
                DELAY = 0

        if GPIO.input(B3):
            shuffle = not shuffle
            print(shuffle)
            time.sleep(0.5)

LED_COUNT      = 300      # Number of LED pixels.
NUMPIX =  100
LED_PIN        = 18      # GPIO pin connected to the pixels (18 uses PWM!).
LED_FREQ_HZ    = 800000  # LED signal frequency in hertz (usually 800khz)
LED_DMA        = 5       # DMA channel to use for generating signal (try 5)
LED_BRIGHTNESS = 25     # Set to 0 for darkest and 255 for brightest
LED_INVERT     = False   # True to invert the signal (when using NPN transistor level shift)
LED_CHANNEL    = 0       # set to '1' for GPIOs 13, 19, 41, 45 or 53
LED_STRIP      = ws.WS2811_STRIP_GRB   # Strip type and colour ordering

#LOAD PATTERNS
text = []
pattern = []
with open('patterns.txt')as inputfile:
    for line in inputfile:
        text.append(line.strip().split(','))
    for p in text:
    pattern.append([])
        for d in p:
            pattern[cpat].append(int(float(d)))
    cpat = cpat + 1

cpat = 0
colors = []
r,g,b = 0.0,0.0,0.0
for p in pattern:
    cnt=0
    colors.append([])
        for x in p:
            if cnt%3 == 0:
            r = x
        elif cnt %3 == 1:
            g = x
        else :
            b = x
            colors[cpat].append(Color(r,g,b))
        cnt = cnt + 1
    cpat = cpat + 1

if __name__ == '__main__':
    cpat = 0
    strip = Adafruit_NeoPixel(LED_COUNT, LED_PIN, LED_FREQ_HZ, LED_DMA, LED_INVERT, LED_BRIGHTNESS, LED_CHANNEL, LED_STRIP)
    strip.begin()

    buttThread = threading.Thread(target=ButtonThread, args=())
    buttThread.daemon = True
    buttThread.start()

    cpat = 0
    print(strip.numPixels())

    while True:
        cnt=0
        for x in colors[cpat]:
            led = cnt%(NUMPIX)
            strip.setPixelColor(led, x)
            strip.setPixelColor(NUMPIX + led, x)
            strip.setPixelColor(2*NUMPIX + led, x)
            cnt = cnt +1

            if led == NUMPIX-1:
                strip.show()
                cnt=0
                time.sleep(DELAY)

A couple of suggestions.

Throttle Infinite Loops:

The button loop has no throttle on its CPU usage.  If no buttons are pressed it is constantly doing IO.  If you reformulate the loop without any of the buttons being pressed the effective code looks like:

while True:
    GPIO.input(B1)
    GPIO.input(B2)
    GPIO.input(B3)

This code never gives the CPU a rest.  It should probably be more like:

while True:
    GPIO.input(B1)
    GPIO.input(B2)
    GPIO.input(B3)
    time.sleep(0.05)

Picking the actual sleep() time will depend on the system, but hopefully this demonstrates the likely problem.

So the loop with the if statements will look like:

while True:
    if GPIO.input(B1):
        ...

    if GPIO.input(B2):
        ...

    if GPIO.input(B3):
        ...

    time.sleep(0.05)

Use List Properties for Length Management

This code is using a variable to get the current pattern:

cpat = cpat + 1
if cpat >= numpats:
    cpat = 0

This can be more cleanly done with:

cpat = (cpat + 1) % len(colors)

The change removes two lines of code, and sets the variable limit based on the list indices, which can avoid bugs later.


Filtering Arrays by values in a specific Column The following function, as evidenced by its name KeepOrRemoveArrayRowsWhereComparisonIsTrue is probably trying to do too much.

How can I better structure/refactor/split it to be cleaner whilst retaining the original intent: "Here's an array, filter it based on the values in this column"?

Public Function KeepOrRemoveArrayRowsWhereComparisonIsTrue(ByRef sourceArray As Variant, ByVal colIndex As Long, ByVal operator As ComparisonOperator, ByVal comparisonValue As Variant, ByVal hasHeaders As Boolean, ByVal keepOrRemoveOnTrue As KeepOrRemove, Optional ByRef arrayOfRemovedRows As Variant) As Variant

        Dim LB1 As Long, UB1 As Long
        AssignArrayBounds sourceArray, LB1, UB1

        Dim rowsToBeRemoved As Variant, removeCounter As Long
        rowsToBeRemoved = Array()
        ReDim rowsToBeRemoved(1 To 1)

        Dim ix As Long, startRow As Long
        If hasHeaders Then startRow = LB1 + 1 Else startRow = LB1

        Dim sourceValue As Variant

        Select Case keepOrRemoveOnTrue

            Case KeepOrRemove.keep
                removeCounter = 0
                For ix = startRow To UB1
                    sourceValue = sourceArray(ix, colIndex)
                    If IsNull(sourceValue) Then sourceValue = 0
                    If IsNull(comparisonValue) Then comparisonValue = 0
                        If Not ComparisonIsTrue(sourceValue, operator, comparisonValue) Then
                            removeCounter = removeCounter + 1
                            ReDim Preserve rowsToBeRemoved(1 To removeCounter)
                            rowsToBeRemoved(removeCounter) = ix
                        End If
                Next ix

            Case KeepOrRemove.Remove
                removeCounter = 0
                For ix = startRow To UB1
                    sourceValue = sourceArray(ix, colIndex)
                    If IsNull(sourceValue) Then sourceValue = 0
                    If IsNull(comparisonValue) Then comparisonValue = 0
                        If ComparisonIsTrue(sourceValue, operator, comparisonValue) Then
                            removeCounter = removeCounter + 1
                            ReDim Preserve rowsToBeRemoved(1 To removeCounter)
                            rowsToBeRemoved(removeCounter) = ix
                        End If
                Next ix

        End Select

        sourceArray = Remove2DArrayRows(sourceArray, rowsToBeRemoved, arrayOfRemovedRows)

        KeepOrRemoveArrayRowsWhereComparisonIsTrue = sourceArray

End Function

As mentioned in a comment (and in chat) already, your method does too many things at once

Do one thing and do that well. Any method that takes more than three arguments stinks to the heavens. At two arguments you should already be thinking whether you actually need them all.

Let's just look at the name for a second: KeepOrRemoveArrayRowsWhereComparisonIsTrue ...

Why is there an Or inside?
Why are you talking about the Comparison?
Why are you talking about ArrayRows?

The latter two are easily fixed. Remove that unneeded fluff:

KeepOrRemoveWhere( ... )

wow.. that's... significantly shorter. It also opens the door for the next change. Split the method at it's responsibilities.

Flags are dangerous, because they're magical and they allow you to pack state-machines into arguments where they don't belong.
Instead of passing in a boolean telling you whether you remove or keep, and which becomes a lie as soon as you change the variable name, you should separate these two methods into one responsible for removing, and one for keeping.

This is often known as Remove and Retain (though Keep works just as well.

Now we have  the following signatures:

Public Function KeepWhere(ByRef sourceArray As Variant, ByVal colIndex As Long, ByVal operator As ComparisonOperator, ByVal comparisonValue As Variant, ByVal hasHeaders As Boolean, Optional ByRef arrayOfRemovedRows As Variant) As Variant

and

Public Function RemoveWhere(ByRef sourceArray As Variant, ByVal colIndex As Long, ByVal operator As ComparisonOperator, ByVal comparisonValue As Variant, ByVal hasHeaders As Boolean, Optional ByRef arrayOfRemovedRows As Variant) As Variant

now we include the changes to your ComparisonOperator construction from here, to replace operator and comparisonValue by a single predicate.

The last thing that bugs me is hasHeaders. This is something the calling code should've taken care of by skipping the first row.

I daresay that this is not something this function should take care of. It's yet another responsibility, and one that shouldn't be handed down the callchain, but processed early.

We end up with:

 Public Function KeepWhere(ByRef sourceArray As Variant, ByVal colIndex As Long, ByVal predicate As Predicate, Optional ByRef arrayOfRemovedRows As Variant) As Variant

Additionally (and lastly) I'm more used to returning the removed elements from such a function (since you still have a reference to the sourceArray anyways). As such the last parameter falls away and we're left with following implementation

Public Function KeepWhere (ByRef sourceArray As Variant, ByVal colIndex As Long, ByVal predicate As Predicate) As Variant
    Dim LB1 As Long, UB1 As Long
    AssignArrayBounds sourceArray, LB1, UB1

    Dim rowsToBeRemoved As Variant, removeCounter As Long
    rowsToBeRemoved = Array()
    ReDim rowsToBeRemoved(1 To 1)

    Dim sourceValue As Variant

    removeCounter = 0
    For ix = startRow To UB1
        sourceValue = sourceArray(ix, colIndex)
        If IsNull(sourceValue) Then sourceValue = 0
        If Not predicate.Test(sourceValue) Then
            removeCounter = removeCounter + 1
            ReDim Preserve rowsToBeRemoved(1 To removeCounter)
            rowsToBeRemoved(removeCounter) = ix
        End If
    Next ix

    sourceArray = Remove2DArrayRows(sourceArray, rowsToBeRemoved, arrayOfRemovedRows)

    KeepOrRemoveArrayRowsWhereComparisonIsTrue = arrayOfRemovedRows
End Function

and similarly for RemoveWhere...
It may be interesting to talk about a way to "Mark" things for rows. then you could actually make this even cleaner by using some flow like the following stub code:

Public Function KeepWhere (sourceArray, colIndex, predicate)
    rowsToBeRemoved = MarkRows (sourceArray, colIndex, predicate.Invert)
    KeepWhere = Remove2DArrayRows (sourceArray, rowsToBeRemoved)
End Function


Merge two arrays of hashes and map them I would like to refactor that code.

def experiences
  work_experience_array = @coach.delete(:work_experience)
  education_array = @coach.delete(:education)

  array = []
  array << work_experience_array.map do |experience|
    Db::Experiences::Work.new(experience.for_indexing)
  end
  array << education_array.map do |experience|
    Db::Experiences::Education.new(experience.for_indexing)
  end
  array.flatten
end

Some notes:

If you use += instead of <<, you don't need to flatten the array.
DRY code by identifying common patterns and create suitable data structures accordingly.

A rewrite capturing the pattern:

def experiences
  fields = [
    [:work_experience, Db::Experiences::Work],
    [:education, Db::Experiences::Education],
  ]

  fields.flat_map do |field, db_class|
    @coach.delete(field).map do |experience|
      db_class.new(experience.for_indexing)
    end
  end
end


v2 - Adding a duplicate entry randomly into a list in haskell using random monad Next version of Adding a duplicate entry randomly into a list in haskell using random monad

I wrote this trying to set up a Haskell testcase. The aim is to take a list and add a single duplicate from any place in the list to anywhere else in the list.  I'm trying to learn to use the Random Monad properly so the main aims should be clear, simple, idiomatic and pure code. However any recommendations for improvement are appreciated.

The code here has most of the improvements suggested by @Petr in the review of the previous version, however

I'm still using lists rather than sequences because the code I plan to test uses lists
I still have an infinite list function because I want to be able to use it easily from IO code where the number of strings to be generated isn't known in advance.  I believe that since the Random Monad uses incremental state based on the State Monad, it should be okay to generate infinite lists with it.

-- DataListDuplicator.hs by Michael De La Rue 2014
-- licensed to StackExchange codereview under cc by-sa 3.0
-- may be used under AGPLv3

module DataListDuplicator where

import Control.Monad.Random

infiniteDuplicateLists :: (MonadRandom m) => [a] -> m [[a]]
infiniteDuplicateLists = mapM addRandomDuplicate . repeat

addRandomDuplicate :: MonadRandom m => [a] -> m [a]
addRandomDuplicate genlist = do
     frompos <- getRandomR (0 ,llen - 1)
     topos   <- getRandomR (0 ,llen)
     let  newlist = listEntryDuplicate frompos topos genlist
     return newlist
   where
     llen = length genlist

listEntryDuplicate from to list =
      start ++ [repeat] ++ end
    where
      repeat = list !! from
      (start, end) = splitAt to list

Here's a little program which drives that:

-- duplicate-to-list-randmonad.hs by Michael De La Rue 2014
-- licensed to StackExchange codereview under cc by-sa 3.0
-- may also be used under AGPLv3
-- N.B. Trivial copying of code fragments does not normally require any license.

import Data.List
import Control.Monad.Random
import DataListDuplicator

main :: IO ()
main = do
    putStrLn $ "list comparison " ++ prettyList list
    g <- getStdGen
    let shuffled = evalRand (infiniteDuplicateLists list) g
    putStrLn $ "lists after \n" ++ intercalate "\n" ( map prettyList (take 5 shuffled))
    where
    list = ["a","b","c"]

prettyList :: (Show a) => [a] -> String
prettyList list = " [ " ++ intercalate "," (map show list) ++ " ] "

I'd again advise against using monadic computations that produce lazy infinite lists. For most monads (including State) this does not work. For MonadRandom it works only very special cases, when the infinite list is the last randomized computation (if you add another randomized computation after the infinite list, it will diverge).

I can think of two possible solutions:

Find a way to properly formalize this idea of infinite randomized lists and get support from the library authors (or send patches yourself). Since this is for me an interesting problem as well, I started a discussion on this topic.
Use conduits or a similar pipe-like library (such as pipes). This way, you can separate the producer of your values, which can be infinite, and a consumer, that takes only as many values as it needs. The library then "switches" between them: each time a consumer requests input, the producer is asked to provide a new value and is suspended again. In your case this could be used as follows:

import qualified Data.Conduit as C
import qualified Data.Conduit.List as C
import Data.List
import Control.Monad
import Control.Monad.Trans
import Control.Monad.Random

-- ...

-- Produces an infinite source of randomized lists.
duplicateListsSource :: (MonadRandom m) => [a] -> C.Source m [a]
duplicateListsSource xs =
    forever $ lift (addRandomDuplicate xs) >>= C.yield

main :: IO ()
main = do
    putStrLn $ "list comparison " ++ prettyList list
    g <- getStdGen
    shuffled <- evalRandT (duplicateListsSource list C.$$ C.take 5) g
    putStrLn $ "lists after \n"
               ++ intercalate "\n" (map prettyList shuffled)
  where
    count = 5
    list = ["a","b","c"]


Exposing .NET's internal StorageRoot via Reflection for accessing file/stream-based Structured Storage .NET tantalizingly exposes StorageInfo and StreamInfo as public classes, but hides the seminal StorageRoot as an internal class, leaving the user no way of creating or opening a Structured Storage from a file or stream.

Microsoft's Reference Source details the source behind StorageRoot which inherits from StorageInfo, so rather than copy the code, I've chosen to expose the static methods of StorageRoot via reflection, so that I can get a reference to the root storage of a File or Stream. I've also exposed a Close method to close an open root StorageInfo.

using System;
using System.IO;
using System.IO.Packaging;
using System.Reflection;

namespace StructuredStoreageTest
{
    public static class StorageRoot
    {
        public static StorageInfo CreateOnStream(Stream baseStream)
        {
            return (StorageInfo)InvokeStorageRootMethod(null, "CreateOnStream", baseStream);
        }

        public static StorageInfo CreateOnStream(Stream baseStream, FileMode mode)
        {
            return (StorageInfo)InvokeStorageRootMethod(null, "CreateOnStream", baseStream, mode);
        }

        public static StorageInfo Open(string path)
        {
            return (StorageInfo)InvokeStorageRootMethod(null, "Open", path);
        }

        public static StorageInfo Open(string path, FileMode mode)
        {
            return (StorageInfo)InvokeStorageRootMethod(null, "Open", path, mode);
        }

        public static StorageInfo Open(string path, FileMode mode, FileAccess access)
        {
            return (StorageInfo)InvokeStorageRootMethod(null, "Open", path, mode, access);
        }

        public static StorageInfo Open(string path, FileMode mode, FileAccess access, FileShare share)
        {
            return (StorageInfo)InvokeStorageRootMethod(null, "Open", path, mode, access, share);
        }

        public static StorageInfo Open(string path, FileMode mode, FileAccess access, FileShare share, int sectorSize)
        {
            return (StorageInfo)InvokeStorageRootMethod(null, "Open", path, mode, access, share, sectorSize);
        }

        public static void Close(StorageInfo storageRoot)
        {
            InvokeStorageRootMethod(storageRoot, "Close");
        }

        private static object InvokeStorageRootMethod(StorageInfo storageRoot, string methodName, params object[] methodArgs)
        {
            Type storageRootType = typeof(StorageInfo).Assembly.GetType("System.IO.Packaging.StorageRoot", true, false);
            return storageRootType.InvokeMember(methodName,
                BindingFlags.Static | BindingFlags.Instance | BindingFlags.Public |
                BindingFlags.NonPublic | BindingFlags.InvokeMethod,
                null, storageRoot, methodArgs);
        }
    }
}

I can then use StorageRoot as follows:

MemoryStream myStream = new MemoryStream(streamBytes);
StorageInfo myRoot = StorageRoot.CreateOnStream(myStream);

StreamInfo dataStreamInfo = myRoot.GetStreamInfo("data");
Stream dataStream = dataStreamInfo.GetStream();
//read/process the stream
dataStream.Dispose();

StorageRoot.Close(myRoot);

Is relying upon Reflection the best alternative here? I could reproduce the StorageRoot code from Reference Source, and make StorageRoot an instance class, I suppose.

A stream-like class that doesn't implement IDisposable and needs to be closed manually, and worked with static methods, breaks POLA/POLS.

I would expect to be able to do this:

using(var ms = new MemoryStream(bytes))
using(var root = new StorageRoot(ms))
{
    // do stuff
}

I like the reflection idea to access these otherwise hidden gems, however in a code base that uses objects in a standard/idiomatic way, I see this one sticking out like a sore thumb.

I'd definitely do whatever it takes to make it work with a proper instance.


Connect-Four winner-checking algorithm I'm trying to practice some functional techniques. I haven't worked much in Haskell, so any language-specific tips would be very appreciated, but what I care for even more are general tips towards my approach that I can carry between functional languages. Performance is not a big concern of mine right now.
I implemented the winner-checking algorithm for the connect-four game. Basically, given a 6x7 grid, I need to be able to determine if a player has successfully placed four consecutive tiles down (horizontally, vertically, or diagonally). If so, I need to return which player it was.
Here is my implementation:
module Logic (Player(..), Board, getWinner) where
import qualified Control.Monad as Monad

data Player = Player1 | Player2

type BoardEntry = Maybe Player

type Board = [[BoardEntry]]

type Coord a = (a, a)

instance Eq Player where
  Player1 == Player1 = True
  Player2 == Player2 = True
  x == y = False

map2dList :: (a -> b) -> [[a]] -> [[b]]
map2dList fn = map (map fn)

getElementAt :: [a] -> Int -> Maybe a
getElementAt (el:_) 0 = Just el
getElementAt (_:rest) n = getElementAt rest (n - 1)
getElementAt [] _ = Nothing

getBoardEntryAt :: Board -> Coord Int -> Maybe BoardEntry
getBoardEntryAt board (x, y) =
  getElementAt board y >>= flip getElementAt x

getBoardDimensions :: Board -> (Int, Int)
getBoardDimensions board =
  ((length . head) board, length board)

addCoords :: Coord Int -> Coord Int -> Coord Int
addCoords (x1, y1) (x2, y2) =
  (x1 + x2, y1 + y2)

isCoordInRegion :: (Int, Int) -> Coord Int -> Bool
isCoordInRegion (width, height) (x, y) =
  x >= 0 && x < width &&
  y >= 0 && y < height

getAllCoordsInRegion :: (Int, Int) -> [Coord Int]
getAllCoordsInRegion (width, height) =
  [(x,y) | x <- [0..width-1], y <- [0..height-1]]

getWinner :: Board -> Maybe Player
getWinner board =
  let
    fourInRowsFromOrigin =
      [
        [(0, 0), (1, 0), (2, 0), (3, 0)],
        [(0, 0), (1, 1), (2, 2), (3, 3)],
        [(0, 0), (0, 1), (0, 2), (0, 3)],
        [(0, 0), (-1, 1), (-2, 2), (-3, 3)]
      ]
    playerPieceAt = Monad.join . getBoardEntryAt board
    results = concat $ do
      coord <- (getAllCoordsInRegion . getBoardDimensions) board
      fourInRow <- map2dList (addCoords coord) fourInRowsFromOrigin
      player <- [Player1, Player2]
      return [player | all ((== Just player) . playerPieceAt) fourInRow]
  in case results of
    [winner] -> Just winner
    [] -> Nothing
    _ -> undefined

And here is a sample driver, to help see that it works:
import Logic (Player(Player1, Player2), Board, getWinner)

getWinnerAsString :: Maybe Player -> [Char]
getWinnerAsString Nothing = "No one has won"
getWinnerAsString (Just Player1) = "Player 1"
getWinnerAsString (Just Player2) = "Player 2"

printWinner :: Board -> IO ()
printWinner = putStrLn . getWinnerAsString . getWinner

exampleBoard :: Board
exampleBoard =
  let
    __ = Nothing
    p1 = Just Player1
    p2 = Just Player2
  in [
    [__, __, __, __, __, __, __],
    [__, __, __, __, __, __, __],
    [__, __, p2, p1, __, __, __],
    [__, __, p1, p2, __, __, __],
    [__, p1, p1, p2, p1, __, __],
    [p1, p2, p2, p2, p1, __, __]
  ]

main :: IO ()
main = printWinner exampleBoard

All the indexing can be deleted. You only need the transpose function plus a custom function that grabs the diagonals of a matrix (list of lists, all of the same length).
import Data.List (transpose)

-- | The 'diagonals' function returns the diagonals of a matrix
diagonals :: [[a]] -> [[a]]
diagonals matrix = lowerDiags matrix ++ upperDiags matrix
    where lowerDiags = reverse . transpose . zipWith drop [1..]
          upperDiags = transpose . zipWith drop [0..] . transpose

You can invoke it as shown below. If any of the vectors has four in a row, you have a winner.
board = ...
vectors = board ++ transpose board ++ diagonals board ++ diagonals (reverse board)


Saving and loading parts of configuration I have a piece of code where I have string pairs (eg. 'a' and 'AA'), where both have their own unique ids. Id for first object is configured by user and second id is received at some point during runtime.

Example code:
http://repl.it/BE5N

Is there smarter way to load/save second ids from/to a file?
It feels somewhat unnecessary to loop through all configs every time new id is found (even if it doesn't happen that often)

And if I added completely new pair-configs during runtime, how should I add them to user's config file while still keeping it human readable?

Initial setup:

var fs = require('fs');

// config pair for first and second
function config (first, second, first_id) {
    this.first    = first;
    this.second   = second;
    // use name as an id for first if none is given
    this.first_id = first_id ? first_id : first;
}

var lookup = function (type, name, arr) {
    return arr.filter(function (obj) {
        return obj[type] === name;
    })[0];
};

// initial config file (few lines out of many)
// user doesn't know second ids
configs = [
    new config('a', 'AA', 'a_id'),
    new config('b', 'BB'),
    new config('c', 'C'),
    new config('d', 'D', 'd_id'),
    new config('e', 'EEE')
];

// read ids for seconds from file if it exists
// read json from file
input_json = {"AA":123,"C":321,"EEE":456};
for (var i=0; i<configs.length; ++i) {
    var key = configs[i].second;
    if (key in input_json) {
        configs[i].second_id = input_json[key];
    }
}

While program is running and new id's are found:

// add new id for 'BB'
BB_config = lookup('second', 'BB', configs);
BB_config.second_id = 654;

// write edited json to file
var output_json = {};
for (var i=0; i<configs.length; ++i) {
    if (configs[i].second_id) {
        output_json[configs[i].second] = configs[i].second_id;
    }
}

console.log(JSON.stringify(output_json));

How about storing your data in sqlite database? All of your operations can be implemented as relational database queries/commands.

Your table will have columns:

FIRST
SECOND
FIRST_ID
SECOND_ID

To perform updates on the second_id based on an object, just use (this is pseudo-code):

for (second in input_json) {
  ...run UPDATE CONFIGS SET SECOND_ID = {input_json[second]} WHERE SECOND = {second} ...

You can make this run efficiently by creating an index on the SECOND column.

Lookup are just SELECT * FROM CONFIGS WHERE ...

For instance, to select only those configs which have second id:

SELECT * FROM CONFIGS WHERE SECOND_ID <> ""

It all depends on how many configs you have. For a large number using a sqlite database could be a win.


Find words from dictionary method. Performance improvements I've implemented a method to solve the following problem:

  Given a dictionary and a text string, find all words from the
  dictionary that are present in the text. One important point is
  that if the string is in double quotes, I should treat it a "single"
  value. If no matches found, the method should return 0.

So for this input:

const text = `Hi. I am a developer. A "Hello world" program was my first code.`;
const dictionary = "hi developer world";

I should get this:

hi
developer
hello
world

This is my current solution:

const fn = (text, dictionary) => {
    const knownWords = dictionary.toLowerCase().split(' ');
    const words = text.toLowerCase().match(/\w+|"[^"]+"/gi);

    const result = words.reduce((acc, word) => {
        if (word[0] == '"') {
            const wordsNoQuotes = word.replace(/"/g, "").split(' ');
            const shouldInclude = wordsNoQuotes.find(word => knownWords.includes(word));
            if (shouldInclude) {
                return acc.concat(wordsNoQuotes);
            }
        } else if (knownWords.includes(word)) {
            acc.push(word)
        }

        return acc;
    }, []);

    return result.length === 0 ? 0 : [...new Set(result)].join('\n');
}

It works ok, but I would like to get some performance related improvements. I am happy to change the code (i.e using indexOf over includes) and so on. Any suggestions are welcome! Thanks.

The main potential performance improvement I can see is turning the array of words into a Set of words instead. For every match in the string, you're doing:

const shouldInclude = wordsNoQuotes.find(word => knownWords.includes(word));

The .find is O(n) (it iterates over all words in the array, worst-case), and the .includes is O(n) (it also iterates through all words in its array, worst-case). So this is an O(n ^ 2) operation which is already inside a loop. That could be a bit expensive when either or both arrays are large. Using a Set for one of the arrays instead will allow you to use Set.prototype.has, which is an O(1) operation. The else part also uses .includes, which can be changed to .has:

const knownWordsSet = new Set(dictionary.toLowerCase().split(' '));
const words = text.toLowerCase().match(/\w+|"[^"]+"/gi);
const result = words.reduce((acc, word) => {
  if (word[0] == '"') {
    const wordsNoQuotes = word.replace(/"/g, "").split(' ');
    const shouldInclude = wordsNoQuotes.find(word => knownWordsSet.has(word));
    if (shouldInclude) {
      return acc.concat(wordsNoQuotes);
    }
  } else if (knownWordsSet.has(word)) {

const fn = (text, dictionary) => {
  const knownWordsSet = new Set(dictionary.toLowerCase().split(' '));
  const words = text.toLowerCase().match(/\w+|"[^"]+"/gi);
  const result = words.reduce((acc, word) => {
    if (word[0] == '"') {
      const wordsNoQuotes = word.replace(/"/g, "").split(' ');
      const shouldInclude = wordsNoQuotes.find(word => knownWordsSet.has(word));
      if (shouldInclude) {
        return acc.concat(wordsNoQuotes);
      }
    } else if (knownWordsSet.has(word)) {
      acc.push(word)
    }

    return acc;
  }, []);

  return result.length === 0 ? 0 : [...new Set(result)].join('\n');
}
console.log(fn(
  `Hi. I am a developer. A "Hello world" program was my first code.`,
  "hi developer world"
));

But Sets have a small overhead as well, so while this will definitely be the best strategy for large inputs, it might do slightly worse for tiny inputs when there are only one or a couple of words. (But for such tiny inputs, performance is not an issue to worry about anyway)

You're currently pushing to an array inside the loop, then the array only gets used to create a Set to deduplicate at the end of the function. It would be more efficient to use a Set of the result words from the beginning, instead of using an array and converting to one after the fact (converting an array to a Set and the other way around is an O(n) process). At this point, since the accumulator would be the same Set every time, .reduce would arguably not be appropriate, so use a plain loop instead.

A potential issue in your current code is that if the global .match has no matches, it will return null, not an empty array (unfortunately). So, to avoid occasionally throwing errors, you have to check to see if the match exists first before iterating over it.

You're using .find to check if there are any elements in the wordsNoQuotes array which fulfill the condition. But you don't care about which element fulfills the condition - you just want to see if any element does. So, rather than using .find (which returns the matching element), it would be more semantically appropriate to use .some (which returns true if any elements pass a callback test, and false otherwise)

When the match is a quote string, your

word.replace(/"/g, "").split(' ');

iterates over and checks every character of the string twice - once to check for "s to remove, and again to identify the positions of spaces. Since you already know the positions of the "s are the first and last character, you could use slice instead:

word.slice(1, word.length - 1).split(' ');

But your word variable isn't necessarily a word - it may well be a quoted string which contains multiple words. To improve clarity, maybe call it match instead.

Edited version in full:

const fn = (text, dictionary) => {
  const knownWordsSet = new Set(dictionary.toLowerCase().split(' '));
  const matches = text.toLowerCase().match(/\w+|"[^"]+"/gi);
  if (!matches) return 0;
  const resultSet = new Set();
  for (const match of matches) {
    if (match[0] == '"') {
      const wordsNoQuotes = match.slice(1, match.length - 1).split(' ');
      const shouldInclude = wordsNoQuotes.some(word => knownWordsSet.has(word));
      if (shouldInclude) {
        for (const word of wordsNoQuotes) {
          resultSet.add(word);
        }
      }
    } else if (knownWordsSet.has(match)) {
      resultSet.add(match);
    }
  }
  return resultSet.size === 0 ? 0 : [...resultSet].join('\n');
}
console.log(fn(
  `Hi. I am a developer. A "Hello world" program was my first code.`,
  "hi developer world"
));


Solution discussion on the best way to reverse a number in Python I'm practicing some python exercise and I have some doubts about the solution of reversing a number. Example, 42 -> 24, -314 -> -413.
The book has this solution:

def reverse(x: int) -> int:
  result, x_remaining = 0, abs(x)
  while x_remaining:
      result = result * 10 + x_remaining % 10
      x_remaining //= 10
  return -result if x < 0 else result

My solution:
def reverse_digit(x: int) -> int:
  s = str(abs(x))
  result = s[::-1]
  if x >= 0:
    return int(result)
  else:
    return int("-" + result)

The book says that converting the number to string would be a brute-force solution but don't they have the same time complexity, O(n)?
Is the book solution that much better than mine (that in my opinion is much simpler and easy to read)?

First, a note on complexity. Both your implementation and the reference implementation do indeed have O(n) complexity in the number of input digits in the best case, but O(log(n)) in the range of the input number.
This depends, mind you, on the expected range of the input number. For "typical values" below the limit of machine precision, which these days will be \$2^{63}\$ for most people, the contents of the reference implementation's inner loop will be constant-time. However, Python has automatic support for arbitrary precision, and if that support is used, the reference implementation will end up being super-linear in the worst case.
As for reversal of a negative number, I think it makes less sense to automatically abs() than just to raise, because - if you interpret the number as a string - negative numbers are non-reversible. I say this knowing that it violates the reference implementation if you interpret it as a specification, but I don't think the specification makes much sense.


API for SQL queries with PHP PDO I'm writing a very simple CRUD application, and I'm wondering if the way I'm using static methods throughout the code makes sense. I'd very much like to simplify the code if possible.
Any feedback and criticism is very much welcome!

<?php

class Database {

    private const DB_DSN = "127.0.0.1";
    private const DB_USER = "root";
    private const DB_PASSWORD = "root";
    private const DB_NAME = "testDB";
    private const DB_PORT = 8889;

    static function connect() {
        return new PDO(
            "mysql:host=".self::DB_DSN.";port=".self::DB_PORT.
            ";dbname=".self::DB_NAME,
            self::DB_USER,
            self::DB_PASSWORD,
            array(PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION),
        );
    }

    static function create() {
        $pdo = new PDO(
            "mysql:host=".self::DB_DSN.";port=".self::DB_PORT,
            self::DB_USER,
            self::DB_PASSWORD,
            array(PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION),
        );
        $pdo->exec("CREATE DATABASE IF NOT EXISTS ".self::DB_NAME);
        return self::connect();
    }
}

class User {

    private const TABLE_NAME = "users";

    public $uid;
    public $username;
    public $email;
    public $hash;
    public $created;
    public $verified;
    public $pdo;

    private function __construct() {
    }

    public static function signUp($pdo, $username, $email, $password) {

        if (self::exists($pdo, $username)) {
            return false;
        }

        $hash = password_hash($password, PASSWORD_DEFAULT);
        $stmt = $pdo->prepare("INSERT INTO `users` SET `username`=?, `email`=?, `hash`=?");
        $stmt->execute([$username, $email, $hash]);
        return self::fromUId($pdo, $pdo->lastInsertId());
    }

    public static function login($pdo, $username, $password) {

        if (!self::exists($pdo, $username)) {
            return false;
        }

        $stmt = $pdo->prepare("SELECT * FROM `users` WHERE `username` = ?");
        $stmt->execute([$username]);
        $user = $stmt->fetchObject("User");
        $user->pdo = $pdo;
        return password_verify($password, $user->hash) ? $user : false;
    }

    public static function exists($pdo, $username) {

        $stmt = $pdo->prepare("SELECT `uid` FROM `users` WHERE `username` = ?");
        $stmt->execute([$username]);
        return $stmt->fetch(PDO::FETCH_COLUMN);
    }

    public static function fromUId($pdo, $uid) {

        $stmt = $pdo->prepare("SELECT * FROM `users` WHERE `uid` = ?");
        $stmt->execute([$uid]);
        $user = $stmt->fetchObject("User");
        $user->pdo = $pdo;
        return $user;
    }

    public function verify() {

        $stmt = $this->pdo->prepare("UPDATE `users` SET `verified` = 1 WHERE `uid` = ?");
        if ($stmt->execute([$this->uid])) {
            $this->verified = true;
            return true;
        } else {
            return false;
        }
    }
}

$db = Database::create();

$db->exec(
    "CREATE TABLE IF NOT EXISTS `users` (
        `uid` int NOT NULL PRIMARY KEY AUTO_INCREMENT,
        `username` varchar(100) NOT NULL UNIQUE,
        `email` varchar(100) NOT NULL UNIQUE,
        `verified` boolean DEFAULT 0,
        `hash` varchar(255) NOT NULL,
        `created` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP
    )"
);

$db->exec(
    "CREATE TABLE IF NOT EXISTS `images` (
        `id` int NOT NULL PRIMARY KEY AUTO_INCREMENT,
        `uid` int NOT NULL,
        `image` varchar(255) NOT NULL,
        `like_count` int NOT NULL DEFAULT 0
    )"
);

$user = User::signUp($db, 'JohnDoe', 'john.doe@sample.org', '12345');

?>

On the Database class
In my experience, a language layer never creates a database. It would require a create database privilege for one thing. Although it's sort of OK for the local environment but would you like your live application to connect the database under the root account? I highly doubt so.
Hence I find the create() method superfluous. But just for sake of refactoring I would make the connect method to accept a parameter that would tell it to use or not to use the dbname DSN argument so it could connect with or without setting the database.
What I would rather add to the database class is a handy "prepare and execute in one go" method like:
public function query($sql, $data = []) {
    $stmt = $this->pdo->prepare($sql);
    $stmt->execute($data);
    return $stmt;
}

as it will make all methods of the User class to lose a lot of weight. As a general rule, when you see repetitive code, think of creating a function to do all the repetitions in one go.
Also it would make sense to make $pdo a public property of the database class.
Now to the User class
First of all, I think that all your doubts with static class is for naught. Yes, in theory it's simple to call User::something() from anywhere without the need to pass around the instance of the user class. But... you need to pass around the instance of PDO anyway! The same problem, different angle.
So you can make your code much cleaner and doubtless by making all methods dynamic, and putting the pdo (or rather a Database class) instance inside. Then you will have to pass around the instance of the user class instead of the instance of the PDO class.
Another repetition that can be spotted is two methods that select a user by username. That's nearly identical code! Always try to reuse the code that already exists in your class. Moreover, for some reason login method runs these two identical queries one after another. But why? Is once not enough?
Another problem is signup function. It's a no-go when it just silently returns false when a user exists. It should be vocal about that. The generic solution is to throw a custom built exception that can be caught and conveyed to the user.
On the return values: Most of time it's not needed. After all, what would you do with that return value from the verify() method?
On the names: it is Better to be more explicit with the function names. "verify" is ambiguous - you never can tell what does it verify and why. The same goes for "exists".
The ActiveRecord vs. DataMapper controversy
From the way you are asking, I can tell you already feel the ambiguity of your class, which serves for two purposes at once, being both a user and a User persistence layer. That's always so with the ActiveRecord pattern to which your class belongs even if you don't probably realize that. You even tried to mae a distinction between these two roles, where static methods are manipulating the database and dynamic methods and properties are manipulating the class data.
You need to make just a logical step further and separate these roles explicitly. A better approach to be considered is to split this class in two - User class and a UserMapper class, where the latter one would do all the database work.
the Username controversy
As long as there is an email to identify the user, there is absolutely no sense in using a dedicated username field. In practice, it adds helluvalot of confusion. A user signs up with a user name and n email, then forgets it and signs up again with email as a username and everything becomes so entangled that it makes it really hard for the support staff. Make it only email to identify the user and get rid of that username nonsense completely. In case you plan to use it as a display name then make it a display name that is not used in the login process at all.
The code
So according to the above suggestion I would create three classes. The User itself
<?php
class User {
    public $uid;
    public $username;
    public $email;
    public $hash;
    public $created;
    public $verified;

    public function login($password)
    {
        return password_verify($password, $this->hash);
    }

}

A class with a custom exception
class UserException Extends InvalidArgumentException {}

And a UserMapper class
class UserMapper
{
    private $db;
    private $table = 'users';
    private $class = 'User';

    public function __construct(Database $db) {
        $this->db = $db;
    }
    public function signUp($email, $password)
    {
        if ($this->getByEmail($email)) {
            throw new UserException("User already exists");
        }
        $hash = password_hash($password, PASSWORD_DEFAULT);
        $sql = "INSERT INTO `$this->table` SET `email`=?, `hash`=?";
        $this->db->query($sql, [$email, $hash]);
        return $this->getById($this->db->pdo->lastInsertId());
    }
    public function getByEmail($email)
    {
        $sql = "SELECT * FROM `$this->table` WHERE `email` = ?";
        return $this->db->query($sql, [$email])->fetchObject($this->class);
    }
    public function getById($uid)
    {
        $sql = "SELECT * FROM `$this->table` WHERE `uid` = ?";
        return $this->db->query($sql, [$uid])->fetchObject($this->class);
    }
    public function setVerified()
    {
        $sqk = "UPDATE `$this->table` SET `verified` = 1 WHERE `uid` = ?";
        $this->db->query($sql, [$this->uid]);
    }
}

then it can be used like this
$userMapper = new UserMapper($db);
try {
    $user = UserMapper->signUp('john.doe@sample.org', '12345');
} catch (UserException $e) {
    $error = $e->getMessage;
    // show it back to user, etc
}

$user = $userMapper->getByEmail($_POST['email']);
if ($user && $user->login($_POST['password'])) {
     // authorize
} else {
     // invalid login or password
}


HackerRank: Encryption Given a two-dimensional list, say some_list = ["ABCD", "EFGH", "IJ"], I am supposed to encrypt it by reading each column top to bottom, thus:

A B C D

E F G H

I J

Would become:

AEI BFJ CG DH

My initial try had “index out of range” problem so I did this:

def encrypt_list(some_list):
    encrpted_list = []
    for i in range(c):
        string = ""
        for j in range(f):
            string += my_list[j][i:i + 1]
        encrpted_list.append(string)
    return encrpted_list

Which looks like a hack. Is there a more Pythonic way?

(This is essentially the Encryption problem from HackerRank.)

Complete working code:

import math

s = "feedthedog"
length_s = len(s)

f = math.floor(math.sqrt(length_s))
c = math.ceil(math.sqrt(length_s))

if c * f < length_s:
    f = c

def chunks(string, split_point):
    for start in range(0, len(string), split_point):
        yield s[start:start + split_point]

split_list = [chunk for chunk in chunks(s, c)]

def encrypt_list(my_list):
    encrpted_list = []
    for i in range(c):
        string = ""
        for j in range(f):
            string += my_list[j][i:i + 1]
        encrpted_list.append(string)
    return encrpted_list

solution_list = encrypt_list(split_list)

for item in solution_list:
    print(item, end=" ")

There's an app for that

The nice thing about the python standard library, especially itertools is that there's always a function for what you want to do. In this case, you want to take chunks like:

['ABCD', 'EFGH', 'IJ']

and turn them into chunks like:

['AEI', 'BFJ', 'CG', 'DH']

That is, we're round robining through each row. There's a recipe for that:

def roundrobin(*iterables):
    "roundrobin('ABC', 'D', 'EF') --> A D E B F C"
    # Recipe credited to George Sakkis
    pending = len(iterables)
    nexts = cycle(iter(it).__next__ for it in iterables)
    while pending:
        try:
            for next in nexts:
                yield next()
        except StopIteration:
            pending -= 1
            nexts = cycle(islice(nexts, pending))

So ultimately, we have just a couple steps:

Get our chunks.
Round robin through them.
Join the result

def chunks(l, n):
    """Yield successive n-sized chunks from l."""
    for i in range(0, len(l), n):
        yield l[i:i+n]

def encrypt(s):
    n = int(math.sqrt(len(s))
    chunked = chunks(s, n)
    return ''.join(roundrobin(*chunked))

Prefer to join

In your solution, you have:

string = ""
for j in range(f):
    string += my_list[j][i:i + 1]
encrpted_list.append(string)

This is inefficient though. When we're building up strings, it's much better to join them:

encrpted_list.append(''.join(my_list[j][i:i+1] for j in range(f)))

Evaluate lazily

This is inefficient:

split_list = [chunk for chunk in chunks(s, c)]

You likely only evaluated out chunks due to the way you wrote your encrypter, but you want to keep things as generators for as long as possible. It's more efficient to pass in chunks(s,c) into the zip_longest() than it would be to fully evaluate it. Though if you do, it's shorter to just write:

split_list = list(chunks(s,c))

Minor points

Excessive amounts of vertical spacing here.
encrpted_list is misspelled: should be encrypted_list.
Why do you have encrypt_list that takes a list and returns a list? You should have encrypt that takes a string and returns a string.
You don't need f,c... you can use int to get the floor, and add 1 if necessary. And avoid single-letter names.


Python program to check if a set of points is at land or at sea I have written a program that divides an geographical area up in cells with size 0.002 degrees longitude by 0.001 degrees latitude, and check every cell whether it's a land area or a sea area. I've done this to effectively to get all the sea area of that area. This is probably insane from a GIS standpoint. Please review my code, call me an insane idiot and help me to get it better.

"""Function to iterate with decimal step size"""
def seq(start, end, step):
    assert(step != 0)
    sample_count = abs(end - start) / step
    return itertools.islice(itertools.count(start, step), sample_count)

if __name__ == "__main__":
    bottomlat = 70.390829
    toplat = 70.855549
    bottomlong = 27.692793
    toplong =  28.728254
    gridsize = 0.001
    map = Basemap(
    projection="merc",
    resolution = 'h', area_thresh = 0.001,
    llcrnrlon=bottomlong, llcrnrlat=bottomlat,
    urcrnrlon=toplong, urcrnrlat=toplat)
    map.drawcoastlines(color='black')
    """Divide global area into grid cells"""
    for i in seq(bottomlat, toplat, gridsize):
        for j in seq(bottomlong, toplong, gridsize*2):
            lon, lat = map(j,i)
            if not map.is_land(lon,lat):
                plt.scatter(lon,lat, s=0.1, color='c', edgecolors='none')
    plt.show()

The ultimate use case is:
I've god millions of lat/long positions from ships. I want to see how large percentage of the sea that has a lat/long position from a ship (cells with a ship position divided by total cell of ocean). So my initial thought was to check each cell if its sea, then perform a check if there is a ship position there.

Style

Your docstring at the top of the seq function should be moved underneath the function signature, like this:

def seq(start, end, step):
    """Function to iterate with decimal step size"""
    ...

The variable map, should be written like this, to improve clarity and readability:

map = Basemap(
    projection="merc",
    resolution = 'h',
    area_thresh = 0.001,
    llcrnrlon=bottomlong,
    llcrnrlat=bottomlat,
    urcrnrlon=toplong,
    urcrnrlat=toplat
)

All these variables:

bottomlat = 70.390829
toplat = 70.855549
bottomlong = 27.692793
toplong =  28.728254
gridsize = 0.001

Should be renamed so they appropriately display the fact that they are constants.

BOTTOM_LAT = 70.390829
TOP_LAT = 70.855549
BOTTOM_LONG = 27.692793
TOP_LONG =  28.728254
GRID_SIZE = 0.001

This comment:

"""Divide global area into grid cells"""

Should be changed into an inline comment:

# Divide global area into grid cells

For more help with style, visit PEP8, Python's official style guide.

Error handling in seq

You shouldn't be using assert to check if a variable meets certain constraints unless you're doing testing. The proper way would be to do something like this:

if step != 0:
    sample_count = abs(end - start) / step
    return itertools.islice(itertools.count(start, step), sample_count)
raise ValueError("Step must not be equal to zero.")


Random Engine - follow up I have ported the code from my previous question "Generate a random numbers class" to C++14 and made a few modifications.

How can I improve it further?

#include <iostream>
#include <random>

template<typename T>
class Random
{
    template <typename U>
    static auto dist() -> typename std::enable_if_t<std::is_integral<U>::value, std::uniform_int_distribution<U>>{};

    template <typename U>
    static auto dist() -> typename std::enable_if_t<std::is_floating_point<U>::value, std::uniform_real_distribution<U>>{};

public:
    Random()
        : mRandomEngine(std::random_device()())
    {}

    auto operator()(T max)
    {
        decltype(dist<T>()) uniformDistribution(0, max - 1);
        return uniformDistribution(mRandomEngine);
    }

    auto operator()(T min, T max)
    {
        decltype(dist<T>()) uniformDistribution(min, max);
        return uniformDistribution(mRandomEngine);
    }

private:
    std::mt19937            mRandomEngine;
};

int main()
{
    Random<int> random;
    for (int i = 0; i < 9; ++i)
        std::cout << random(1, 8) << '\n';

    Random<float> randomf;
    for (int i = 0; i < 9; ++i)
        std::cout << randomf(1.f, 8.f) << '\n';
}

Asserting our preconditions

Your class requires T to either be an integral type or a floating point type. If it's not, you'll get a compile error at point of use on operator() instead of at point of declaration. Let's fix that:

static_assert(std::is_integral<T>::value || std::is_floating_point<T>::value, "!");

Unnecessary Complexity

We are conditionally selecting a distribution type based on whether or not T is integral or floating point. Once we assert that it's one or the other, we can simply use std::conditional:

using dist_type = std::conditional_t<
                      std::is_integral<T>::value,
                      std::uniform_int_distribution<T>,
                      std::uniform_real_distribution<T>>;

And just use that:

T operator()(T min, T max)
{
    dist_type uniformDistribution(min, max);
    return uniformDistribution(mRandomEngine);
}

This is a lot more direct. No extra template arguments. Also auto on return type is best reserved for those cases where you need it. This is a random engine generating Ts, so it really had better return a T.

Single-value?

Consider the difference between our distributions. uniform_int_distribution is closed:

  Produces random integer values i, uniformly distributed on the closed interval [a, b]

but uniform_real_distribution is open:

  Produces random floating-point values i, uniformly distributed on the interval [a, b)

This suggests one of two potential implementations

Pass this difference onto your users. They should be aware that for floating point Ts, it will be open on the top end.
Make your implementation always open on top end of the distribution.

Right now, we're half and half. It's fairly unexpected that:

Random<int> r;
r(1, 6); // can return 6
r(0, 6); // can return 6
r(6);    // cannot return 6

I'd recommend option (1):

T operator()(T max) {
    return (*this)(0, max);
}

Prefer Braces

Where you have:

: mRandomEngine(std::random_device()())

Prefer to brace-initialize the random_device:

: mRandomEngine(std::random_device{}())