Program to retrieve key/message from a multiple times used one time pad I wrote a program to retrieve the key/messages from 10 different ciphers which were all encrypted with the same key with an xor one-time-pad method via crib dragging.

To do this, I wrote a python script which turns a string to test to hex code. Then it xors the 10 cipher strings (200 hex characters each), two at a time. After that it, xors the test string with the 5 xor hashes on each position and then looks via a website whether this is a reasonably good string to go further and try again.

I already tested it and got positive results.

Now when I do it with the real ciphers I got nowhere so far, and I am not sure whether this is because of the script or me not being able to expand on retrieved valid string fragments.

Here is the code, I know that the website lookup is very bad, but I found no other way this fast/easy (to implement, not to run).

EDIT: Since I do not think overwriting the code is good, I put it here in case somebody wants to see it: Gist link

# -*- coding: UTF-8 -*-
import codecs
from urllib.error import URLError
import urllib.request

forbiddenSigns = ['{', '}', '[', ']', '(', ')', '?', '=', '%', '_', ';', '-', '^', ':', '<', '>', '|', '!', '`', '´', "'", '\\']

with open("testCiphers.txt") as f:
    ciphers = f.readlines()

ciphers = [s.strip() for s in ciphers]

crib = "the"
cribList = []

for c in crib:
    cribList.append(hex(ord(c))[2:])

cribHash = ''.join(cribList)
cribLength = len(cribHash)

cipherHashList = []
for i in range(0, len(ciphers) - 1):
    cipherHashList.append(int(ciphers[i], 16) ^ int(ciphers[i + 1], 16))

hashResultList = [str(hex(hr))[2:] for hr in cipherHashList]

hexList = []

for hashResultString in hashResultList:
    for i in range(0, len(hashResultString) - cribLength + 1, 1):
        hexList.append(hashResultString[i:i + cribLength])

for ha in hexList:
    cribAttempt = int(cribHash, 16) ^ int(ha, 16)
    cribAttempt = (hex(cribAttempt))[2:].zfill(cribLength)
    clearText = codecs.decode(cribAttempt, "hex")
    if any(s not in str(clearText) for s in forbiddenSigns) and "\\x" not in str(clearText):
        url = 'http://www.morewords.com/contains/' + str(clearText, "ASCII")
        try:
            siteRequest = str(urllib.request.urlopen(url).read())
            if "words found.
" in siteRequest:
                print(str(clearText, "ASCII"), end="\n")
        except URLError :
            "error getting webpage"

List comprehensions

You use list comprehensions sometimes, and sometimes you use for ...: someList.append(...). This is not consistent. Use list comprehensions as much as you can since you are already building lists (and list comprehensions are slightly faster than for loops):

cribList = [hex(ord(letter)[2:] for letter in crib]

cipherHashList = [int(ciphers[i],16)^int(ciphers[i+1],16) for i in range(len(ciphers)-1)]

hexList = [hashResultString[i:i+cribLength] for hashResultString in hashResultList for i in range(len(hashResultString) - cribLength + 1)]

You can speed things up a little more (maybe at the cost of readability) by using more complex expressions in the list comprehension to avoid creating intermediate lists:

hashResultList = [hex(int(ciphers[i],16)^int(ciphers[i+1],16))[2:] for i in range(len(ciphers)-1)]

Naming

Check PEP 8 for the rationale but you’d better use UPPERCASE names for constants such as FORBIDDEN_SIGNS or CRIB and lower_snake_case for variables such as hex_list or crib_length.

Some of the variable are named thingList and might be improved using things.

Reusability

Make functions to ease testing into the interpreter and/or reusability:

def get_crib_hash():
    return ''.join(hex(ord(letter))[2:] for letter in CRIB)

change the value of CRIB in the interpreter and voilà, every time you (or any function) use get_crib_hash() the value is easily updated without having to recompute everything.

For efficiency reasons I recommend using a generator function for file reading.

Flaws

On my computer, instead of returning b'test':

>>> codecs.decode("74657374","hex")
Traceback (most recent call last):
  File "<pyshell#39>", line 1, in <module>
    codecs.decode("74657374","hex")
LookupError: unknown encoding: hex

You can use a more robust approach in the name of binascii.unhexlify which is doing exactly what you want.

For your issue with “real ciphers”, you might want to print the clear text first instead of trying to lookup words through a web API. It’ll help understand what is happening.

Small improvements

forbiddenSigns = ['{', '}', '[', ']', '(', ')', '?', '=', '%', '_', ';', '-', '^', ':', '<', '>', '|', '!', '`', '´', "'", '\\']

is better written (more resource friendly) as:

FORBIDDEN_SIGNS = '{}[]()?=%_;-^:<>|!`´\'\\'

and you will still be able to iterate over each symbol.
range() does not need a 0 as first argument (unless you want to specify a step); similarly a step of 1 is the default.
compute once if you can; e.g: str(clearText, "ASCII").
consider using an if __name__ == '__main__': construct to emphasize on reusability and improve testing in the interactive interpreter.
format is often more efficient for building strings; it can even replace hex for your use case.

Proposed improvements

import binascii

FORBIDDEN_SIGNS = '{}[]()?=%_;-^:<>|!`´\'\\'
CRIB = 'the'

def get_crib_hash():
    return ''.join(format(ord(letter), '02x') for letter in CRIB)

def hash_file(filename):
    with open(filename) as f:
        previous = None
        for line in f:
            line = line.strip()
            try:
                yield format(int(previous, 16) ^ int(line, 16), 'x')
            except TypeError: # for int(None, 16) at first pass
                pass
            previous = line # Yeah, I’ll compute int(this_thing, 16) a second time…
            # Avoiding it is left as an exercise for the reader.

def decipher_file(filename):
    crib_hash = get_crib_hash()
    crib_length = len(crib_hash)
    crib_hash = int(crib_hash, 16)

    for hash_result in hash_file(filename):
        for i in range(len(hash_result) - crib_length + 1):
            attempt = crib_hash ^ int(hash_result[i:i+crib_length], 16)
            attempt = '{0:0{fill}x}'.format(attempt, fill=crib_length)
            yield str(binascii.unhexlify(attempt), 'ASCII')

if __name__ == '__main__':
    from urllib.error import URLError
    from urllib.request import urlopen

    for clear_text in decipher_file('testCiphers.txt'):
        # or just print(clear_text)…
        if '\\x' not in clear_text and any(s not in clear_text
                                           for s in FORBIDDEN_SIGNS):
            url = 'http://www.morewords.com/contains/{}'.format(clear_text)
            try:
                response = str(urlopen(url).read())
                if 'words found.
' in response:
                    print(clear_text)
            except URLError :
                'error getting webpage'


Inserting filename, reading data, inserting regex, and testing if each line matches - version 2 This is a improved version of the program seen in my older question.

This program first displays a menu which shows some options that the user can select. Each option will the respective functions which do some things.

The user can insert a filename, read data from it, enter a regex pattern, test if each line of the file matches the given regex pattern.

Please provide feedback on how I can improve my program.

import re

def test_data(readData, regex):
    '''Checks if each line of data matches the regex'''

    if not regex:
        print('Empty regex')
        print('')
        return

    try:
        pattern = re.compile(regex)
    except:
        print('Invalid regex')
        print('')
        return

    if not readData:
        print('Read data is empty')
        print('')
        return

    for i, line in enumerate(readData, 1):
        line = line.strip()  #Strip off the newline character from each line
        if pattern.match(line):
            print(' Line ', i, ':"', line,'"', ' matches current regex', sep='')
        else:
            print(' Line ', i, ':"', line,'"', ' does not match current regex', sep='')
    print('')

def change_regex(regex):
    '''Sets the regex variable'''

    regex = '"%s"' % regex if regex else 'empty'
    print('Current regex is', regex)

    regex = input('Enter new regex: ')
    print('')
    return regex

def read_file(filename):
    '''Reads data from file if possible'''

    try:
        with open(filename, "r") as file:
            readData = file.readlines()
            print('Data read successfully!')
            return readData
    except TypeError:
        print('filename is empty!')
    except FileNotFoundError:
        print('File not found!')

    print('')

def change_filename(filename):
    '''Sets the filename variable'''

    filename = '"%s"' % filename if filename else 'empty'
    print('Current filename is', filename)

    filename = input('Enter new filename (with extension): ')
    print('')
    return filename

def main():
    filename = regex = readData = None
    while True:
        print('Enter 1 to change filename')
        print('Enter 2 to read file')
        print('Enter 3 to change regex')
        print('Enter 4 to test if read data matches given regex')
        print('Enter 5 to exit')

        try:
            option = int(input('Option: '))
        except:
            print('Invalid input')
            print('Please enter a valid option')
            print('')
            continue

        if option == 1:
            filename = change_filename(filename)
        elif option == 2:
            readData = read_file(filename)
        elif option == 3:
            regex = change_regex(regex)
        elif option == 4:
            test_data(readData, regex)
        elif option == 5:
            break
        else:
            print('Invalid input')
            print('Please enter a valid option')
            print('')

if __name__ == "__main__":
    main()

Pure functions

All of your functions both "do work" and print. You should separate logic and In/Out.

print('') is poor practice

If you want an extra newline just concat it to the previous message:

print('Empty regex\n')
return

Avoid noise

Maybe in a file of 1000 lines, my regex will match 5 lines, why must I watch 995 useless lines of output? Just output lines where it does match.

Prefer a dictionary or indexes to an array to a long if elif chain

if option == 1:
    filename = change_filename(filename)
elif option == 2:
    readData = read_file(filename)
elif option == 3:
    regex = change_regex(regex)
elif option == 4:
    test_data(readData, regex)
elif option == 5:
    break
else:
    print('Invalid input')
    print('Please enter a valid option')
    print('')

In all sincerity I don't know how to do this in this particular case, but iit would also give the  benefit of generating the printing message on the fly, instead of repeating the same thing twice.

Command-line?

But maybe all of that effort for the menu is counterproductive and you should allow the user to call the programme like:

python regex_script.py <regex> <filename>

by using sys.argv or argparse.

My take

This is my implementation of this program, it separates Input/Output and logic and can be used concisely from the command line:

import re
import sys

def lines_matched(regex, text):
    for line in text.splitlines():
        if re.search(regex, line):
            yield line

def main():
    if len(sys.argv) != 3:
        print("Usage: python {} <regex> <filename>".format(__file__))
        return 1
    regex, filename = sys.argv[1], sys.argv[2]
    with open(filename) as f:
        print('\n'.join(lines_matched(regex, f.read())))

if __name__ == "__main__":
    main()

argparse would make it more user friendly.


Addition of two number using array of unequal size I tried my level best to get it done. But I would like to have a better solution in all terms.

  Given 2 huge numbers as seperate digits, store them in array and
  process them and calculate the sum of 2 numbers and store the result
  in an array and print the sum.

  Input:

Number of digits:12

9 2 8 1 3 5 6 7 3 1 1 6
Number of digits:9
7 8 4 6 2 1 9 9 7

  Output:

9 2 8 9 2 0 2 9 5 1 1 3

    #include<stdio.h>
    #include<stdlib.h>
    int main()
    {
        int num1[]={9,9,9,9,9,9,9,9,9};
        int num2[]={1,1,1};
        int num1_n=sizeof(num1)/sizeof(int);
        int num2_n=sizeof(num2)/sizeof(int);
        int z=num1_n+num2_n;

        int carry=0;
        int digi=0;
        int i=0;
        int j=0;
        int *num3;
        if(num1_n>num2_n)
        {
            num3=(int*)malloc(sizeof(int)*(num1_n+1));
            for(i=0;i<num1_n+1;i++)
            {
                num3[i]=0;
            }
            for(i=num1_n-1,j=num2_n-1;i>=0 || j>=0 ;i--)
            {
                num3[i+1]=num3[i+1]+num1[i]+num2[j];
                printf("i %d j %d num3[i] %d num1[i] %d num2[i] %d\n",i,j,num3[i],num1[i],num2[j]);
                if(num3[i+1]>9)
                {
                    carry=1;
                    num3[i+1]=num3[i+1]%10;
                }
                if(carry==1)
                {
                    num3[i]=1;
                    carry=0;
                }
                if(j>=0)
                {
                    j--;
                }
            }
        }
        for(i=0;i<num1_n+1;i++)
        {
            printf("%d ",num3[i]);
        }
        return 0;
    }

Define an API

The first thing you should do is define a function to wrap the implementation.  This gives it a name and a defined interface.  You can then set expectations such as the first number is the largest, the caller needs to pre-allocate a buffer for the result, etc.

Naming

It would be easier to follow your code if i and j had more meaningful names.  num3 could be target_number or sum etc.

Unused variables

You define variables z and digi, but don't use them.  If you don't need them, get rid of them, they simply add unnecessary noise.

carry?

Do you really need your carry flag?  You could simply combine the code that sets it with the code that checks it:

if(num3[i+1]>9)
{
    num3[i+1]=num3[i+1]%10;
    num3[i]=1;
}

Bugs

Your code has two major issues.

As pointed out by @chux in the comments, if num1 has the same number or more digits than num2 your code skips the addition step and then accesses num3 to print it out.  This will probably crash because num3 hasn't been allocated.
If num1 has more digits than num2, you access a negative index of num2.  Once you pass the left side of the shortest number you should stop adding from it.


Connect graph from 2D array I just completed Project Euler problem 18 which is a dynamic programming question through a graph of a triangle of numbers. The goal is to find the path from top to bottom of the triangle that has the largest sum.

In the example below the path is 3->7->4->9.

Say the numbers in the triangle are:

   3
  7 4
 2 4 6
8 5 9 3

I built a jagged array that looks like:

int[][] triangleNumbers = new int[4][]
{
    new int[] {3},
    new int[] {7, 4},
    new int[] {2, 4, 6},
    new int[] {8, 5, 9, 3},
};

Then I made a custom object TriangleNode and converted the numbers into TriangleNodes. Then I went through and connected the lower left and lower right nodes to their parent so I could graph traverse them easily.

//build triangle graph
TriangleNode[][] triangleGraph = new TriangleNode[triangleNumbers.Length][];

for(int row = 0; row < triangleNumbers.Length; row++)
{
    triangleGraph[row] = new TriangleNode[triangleNumbers[row].Length];
    for(int col = 0; col < triangleNumbers[row].Length; col++)
    {
            //create the Node and save the value
        triangleGraph[row][col] = new TriangleNode(triangleNumbers[row][col]);
    }
}

//connect triangle graph
for(int row = 0; row < triangleGraph.Length; row++)
{
    for(int col = 0; col < triangleGraph[row].Length; col++)
    {
        if(row + 1 == triangleGraph.Length)
        {
                    //bool for easy graph traversing to check if I'm at the bottom row
            triangleGraph[row][col].IsEnd = true;
        }
        else
        {
                    //hook up the Left/Right pointers
            triangleGraph[row][col].Left = triangleGraph[row + 1][col];
            triangleGraph[row][col].Right = triangleGraph[row + 1][col + 1];
        }
    }
}

Obviously this is a ton of code just to build the graph.

Is there an easier way to build this graph? The requirements are that each TriangleNode holds its value and Left/Right pointers to the Nodes in the row directly below. I was thinking there might be some voodoo magic via LINQ to turn the triangleNumbers[][] into the triangleGraph[][], then some more voodoo to hook up the Left/Right pointers.

I don't know about "LINQ voodoo", but you could do this in one pass if you reversed the order you're processing the rows. Something like this:

        //build triangle graph
        var triangleGraph = new TriangleNode[triangleNumbers.Length][];

        for (int row = triangleNumbers.Length - 1; row >= 0; row--)
        {
            triangleGraph[row] = new TriangleNode[triangleNumbers[row].Length];
            for (int col = 0; col < triangleGraph[row].Length; col++)
            {
                //create the Node and save the value
                bool isEnd = row == triangleNumbers.Length - 1;
                triangleGraph[row][col] = new TriangleNode(triangleNumbers[row][col])
                                              {
                                                  IsEnd = isEnd,
                                                  Left = isEnd ? null : triangleGraph[row + 1][col],
                                                  Right = isEnd ? null : triangleGraph[row + 1][col + 1]
                                              };
            }
        }

I'll admit, I didn't test


Print-based debugging #include <iostream>

const bool DEBUG = false;

template <typename ... Args>
void debug(Args ... args);

template <typename ... Args>
void _debug(Args ... args) {
    std::cout << "[" << __FILE__ << ":" << __LINE__ << "] ";
    (std::cout << ... << args) << std::endl;
}

#define debug(args...) if (DEBUG) _debug(args);

Suppose you are running ACM or you are somewhere without a well-crafted development environment. You have written some code and wondering what is going wrong.

Goal here is to find out what cause the problem as quickly as possible by placing tracing prints and turn them off all together by preventing code generation for them. So you can  submit your code with DEBUG = false and it will not cause any performance issues done by calling debug(...).

Although it's a short function/macro, there are a number of problems here:

__FILE__ and __LINE__ are expanded in the function definition, rather than at the call site.  We need an interface that passes those in, so that the macro is
  #define debug(args...) if (DEBUG) _debug(__FILE__, __LINE__, args);

That's a non-standard variadic macro expansion.  The standard way is to write the parameter as ... and expand it using __VA_ARGS__.

Identifiers in global scope beginning with underscore are reserved for the implementation.  Use a namespace instead (e.g. debugging::debug).

Debug information should go to std::clog, not std::cout.

The macro doesn't play nicely in if/else statements - use the do...while(0) idiom to make it statement-like.

Conventional style is to use all-caps for macros (to indicate their dangers) and nothing else.  We have this exactly backwards here, with debug and DEBUG.

Improved version
#include <iostream>

namespace debugging
{
#ifdef ENABLE_DEBUG
    constexpr bool debug = true;
#else
    constexpr bool debug = false;
#endif

    template <typename... Args>
    void print(const char* file, int line, Args... args) {
        (std::clog << "[" << file << ":" << line << "] "
                  << ... << args) << std::endl;
    }
}

#define DEBUG(...)                                              \
    do {                                                        \
        if (debugging::debug)                                   \
            debugging::print(__FILE__, __LINE__, __VA_ARGS__);  \
    } while (0)

And a quick test (that demonstrates that the arguments are evaluated only when we're debugging):
int main()
{
    DEBUG("Started main");
    int status = 1;
    DEBUG("Leaving main, status=", status=0);
    return status;
}

Addendum
We don't need the debug constant - just change the definition of the macro according to whether or not we're debugging:
#ifndef ENABLE_DEBUG
#define DEBUG(...)  ((void)0)
#else
#include <iostream>

namespace debugging
{
    template<typename... Args>
    void print(const char* file, int line, Args... args) {
        (std::clog << "[" << file << ":" << line << "] "
                  << ... << args) << std::endl;
    }
}

#define DEBUG(...)  debugging::print(__FILE__, __LINE__, __VA_ARGS__)
#endif


Schema design for user profile management Is this a good design? I would like to create a view on user table to get a list of users based on filtered conditions. Please suggest ways of improving this schema design.

A view example that I would like to add on this schema:

Select all users who belong to group and all groups created after that group.
Select all users with last message, status, location and media URLs included.

CREATE TABLE IF NOT EXISTS users
  (
     userid     TEXT NOT NULL,
     name       TEXT NULL,
     lmessage   INTEGER NULL,
     statusid   INTEGER NULL,  /* statusid should refer to last status of the user in status table*/
     locationid INTEGER NULL,  /* locationid should refer to last status of the user in locations table */
     registered INTEGER NOT NULL,
     tinypic    INTEGER NULL /* this refers to media id in media table */,
     largepic   INTEGER NULL /* this also refers to media id in media table */,
     groupid    INTEGER NULL /* this refers to id in groups table */ ,
     PRIMARY KEY (userid)
  );

CREATE TABLE IF NOT EXISTS locations
  (
     serial INTEGER,
     locationid TEXT NOT NULL,
     userid TEXT NOT NULL,
     time     INTEGER NULL,
     PRIMARY KEY (serial)
  );

CREATE TABLE IF NOT EXISTS status
  (
     serial INTEGER,
     userid TEXT NULL,
     message  TEXT NOT NULL,
     time     INTEGER NULL,
     PRIMARY KEY (serial)
  );

CREATE TABLE IF NOT EXISTS messages
  (
     sno       INTEGER,
     messageid INTEGER NOT NULL,
     sender    TEXT NOT NULL,
     receiver  TEXT NOT NULL,
     time      INTEGER NULL,
     message   TEXT NULL,
     image     INTEGER NULL,
     video     INTEGER NULL,
     audio     INTEGER NULL,
     PRIMARY KEY (sno)
  );

CREATE TABLE IF NOT EXISTS media
  (
     mediaid    TEXT NOT NULL UNIQUE,
     url        TEXT NULL,
     downloaded INTEGER NULL,
     thumbnail  TEXT NULL,
     PRIMARY KEY (mediaid)
  );

CREATE TABLE IF NOT EXISTS groups
  (
     serial INTEGER,
     name TEXT NOT NULL,
     id INTEGER NOT NULL
     PRIMARY KEY(serial)
  );

CREATE UNIQUE INDEX IF NOT EXISTS id_unique ON users (userid ASC);

CREATE UNIQUE INDEX IF NOT EXISTS serial_unique ON status (serial ASC);

CREATE UNIQUE INDEX IF NOT EXISTS id_unique ON messages (sno ASC);

CREATE UNIQUE INDEX IF NOT EXISTS serial_unique ON patterns (serial DESC);

CREATE UNIQUE INDEX IF NOT EXISTS mediaid_unique ON media (mediaid ASC);

Is this a good design?

No, not really.
Some of the tables are not well designed.
Take a look at the responsibilities of the users table and the media table.

The media table has one clear responsibility:
store the attributes of media items.
Every record is self-contained, complete.
You will probably never update the records in this table:
you will only insert or delete. That's nice and simple
The users table has two responsibilities:

Store the attributes of users
Store the relationship of users and other objects

Every record is only half-complete (user attributes),
to get the full picture you need to follow the references to other tables.
You will probably never update the records very frequently,
for setting the last status and location ids

It would be better to make the users table in charge of user attributes only.
You could add additional tables to track the relations with other objects.

The circular dependencies are especially ugly:

users depends on status for the last status, and status depends on users to associate all statuses with a user
Same for locations

I recommend this kind of structure:

user - in charge of user attributes (name, registered date)
status - in charge of status attributes (message, date)
user_status - in charge of linking user and status
user_last_status - in charge of linking user and last status, with unique key on user

And similarly for locations.
It's more work, but the end result will be more robust and flexible.

A reasonable compromise can be to not create user_status,
but include user_id in the status table (with a similar treatment for locations too).

  Select all users who belong to group and all groups created after that group.

To find users by a given group id fast,
you need an index on the group id column in the users table.
Below I explain how to create foreign keys.
When you make user.group_id a foreign key referencing group.group_id,
creating such index is one of the steps,
and will ensure fast lookups.

In addition,
you might also find a group fast by name.
In that case you might want to create an index on group.name column.
However, if you won't have many groups than that would be overkill,
and not recommended by the documentation.

  Select all users with last message, status, location and media URLs included.

Generally speaking, For good performance of queries like this,
you want to make sure that all fields used in the WHERE and JOIN clauses are indexed.
If you follow my suggestions,
this will be automatically the case for most of these elements you mention,
and for the rest you can probably figure out yourself.

Another issue with the design is that some of the fields are not clear enough, for example:

users

userid: it would be good to know if this will be a technical id or a something like a UNIX username. If the latter, I'd rename to username
name: NULL is allowed, but why?
lmessage: an integer field, but not clear which other table it will reference
registered: I suppose the date when the user was created. I recommend using the type datetime, even if internally it might be just an integer

locations

serial: technical id I suppose?
locationid: text? That's a bit surprising. How is it different from serial?
time: perhaps there can be a better name for this. And again, I recommend using the type datetime

status: similar objections as with locations
messages

sno: technical id I suppose? I recommend to standardize the name of technical ids in all tables
sender, receiver: I'm wondering if these have any relation to other tables

media: this is good and clear. The UNIQUE keyword on mediaid is pointless when you add a PRIMARY KEY constraint on the same column

The answers to the questions I raised would be good in comments.
Some example values for the columns would be useful too,
especially for the ones where the meaning and motivation is not obvious.

Declare PRIMARY KEY on the field's line

Instead of declaring PRIMARY KEY at the end,
it's more compact to declare it on the field itself, like this:

CREATE TABLE IF NOT EXISTS locations
  (
     serial INTEGER NOT NULL PRIMARY KEY,
     -- ...
  );

I also added NOT NULL for the field, though it's not strictly necessary.
When a field is primary key in sqlite,
it's treated as auto-incrementing,
so even if you try to explicitly insert NULL into it,
it will actually use max(col) + 1 instead.
But I like to write it this way anyway to make it explicit:
looking at this schema there will be no doubt that the field will never contain NULL.

Pointless unique indexes

Many of the indexes you're adding seem completely pointless:

CREATE UNIQUE INDEX IF NOT EXISTS id_unique ON users (userid ASC);

CREATE UNIQUE INDEX IF NOT EXISTS serial_unique ON status (serial ASC);

CREATE UNIQUE INDEX IF NOT EXISTS id_unique ON messages (sno ASC);

CREATE UNIQUE INDEX IF NOT EXISTS mediaid_unique ON media (mediaid ASC);

In all these commands, the target fields are already marked as PRIMARY KEY,
so adding a unique index will do absolutely nothing.

From the docs:

  In most cases, UNIQUE and PRIMARY KEY constraints are implemented by
  creating a unique index in the database. (The exceptions are INTEGER
  PRIMARY KEY and PRIMARY KEYs on WITHOUT ROWID tables.) Hence, the
  following schemas are logically equivalent:

CREATE TABLE t1(a, b UNIQUE);

CREATE TABLE t1(a, b PRIMARY KEY);

CREATE TABLE t1(a, b);
CREATE UNIQUE INDEX t1b ON t1(b);

There is one more unique index in your post:

CREATE UNIQUE INDEX IF NOT EXISTS serial_unique ON patterns (serial DESC);

But the schema of the patterns table is not in your post.
Curiously, this unique index uses the DESC keyword,
which would set it apart from the other examples which all used the default ASC.
In this case, I'm wondering what difference this will make on a table where the target column is already a primary key.
That might be an interesting question on stackoverflow.com

Although in this case it seems the unique indexes are redundant and can be removed,
if ever you need to make a column unique,
I recommend declaring UNIQUE on the column's line when creating the table,
like this:

CREATE TABLE IF NOT EXISTS status
  (
     serial INTEGER NOT NULL PRIMARY KEY,
     userid TEXT NULL UNIQUE,
     -- ...
  );

Or else,
if you really want to use a unique index with DESC sorting order,
then create the index right after creating the table.
It's not a good idea to separate the declarations of tables and indexes like in your post. The indexes are closely tied to their tables,
and the code is most readable when dependent elements are closed to each other.

Also keep in mind that only versions of SQLite 3.3.0 understand indexes with descending sorting order.

Use foreign keys

It seems you wanted to use foreign keys just didn't know how :)
Here's an example:

CREATE TABLE IF NOT EXISTS users
  (
     userid     TEXT NOT NULL PRIMARY KEY,
     locationid INTEGER NULL REFERENCES locations (locationid)
  );

CREATE TABLE IF NOT EXISTS locations
  (
     serial INTEGER NOT NULL PRIMARY KEY,
     locationid TEXT NOT NULL UNIQUE,
  );

CREATE INDEX users_location_index ON users(locationid);

Notice the UNIQUE constraint on the referenced column:
this is a requirement of foreign keys.
(For more details, see the docs.)

Lastly, notice the index on users.locationid.
Although not required, this is recommended by the docs.

Indicate the column sizes

Although SQLite understands and ignores VARCHAR(N),
treating it as TEXT,
I recommend to use VARCHAR(N) anyway for non-text fields.
This is to indicate your design intentions.
It will also make it easier to port your schema to other databases in the future.
To be safe, add a comment to remind that SQLite doesn't enforce the specified sizes.

Naming and style

I recommend using id column names like user_id, status_id instead of userid and statusid because this seems a common practice.

I would borrow some ideas from Django:

Use singular instead of plural for table names: for example user instead of users
Use id as the auto-incrementing technical id field consistently in all tables
Very cosmetic thing but anyway: write SQL types like INTEGER and TEXT using lowercase letters


Checking if CLI arguments are valid files/directories in Python I frequently write Python scripts that require three command line arguments:

config_file — A CSV configuration file
input_dir — The directory containing the input files to be processed
output_dir — The directory where the output files should be stored

I found myself copying/pasting two functions all the time: is_valid_file and is_valid_directory.

Questions

How can I consolidate the is_valid_file and is_valid_directory functions to eliminate duplicate code?
Is there a better way to check that files/directories provided in CLI arguments actually exist?

Code

Here is the code for process_the_data.py:

if __name__ == "__main__":
    # Process the arguments
    import argparse
    import arghelper
    parser = argparse.ArgumentParser(
        description='Process the data.')
    parser.add_argument(
        'config_file',
        help='CSV configuration file.',
        metavar='FILE', type=lambda x: arghelper.is_valid_file(parser, x))
    parser.add_argument(
        'input_dir',
        help='Directory containing the input files.',
        metavar='DIR', type=lambda x: arghelper.is_valid_directory(parser, x))
    parser.add_argument(
        'output_dir',
        help='Directory where the output files should be saved.',
        metavar='DIR', type=lambda x: arghelper.is_valid_directory(parser, x))
    args = parser.parse_args()

Here is the code for arghelper.py:

import os

def is_valid_file(parser, arg):
    if not os.path.isfile(arg):
        parser.error('The file {} does not exist!'.format(arg))
    else:
        # File exists so return the filename
        return arg

def is_valid_directory(parser, arg):
    if not os.path.isdir(arg):
        parser.error('The directory {} does not exist!'.format(arg))
    else:
        # File exists so return the directory
        return arg

You can address some of this by subclassing ArgumentParser.  This will allow you hide the lambda calls that may turn off some and leave you scratching your head if you come back to that file later.

Your Script

if __name__ == "__main__":
    # Process the arguments
    #import argparse
    import arghelper
    parser = arghelper.FileArgumentParser(
        description='Process the data.')
    parser.add_argument_with_check(
        'config_file',
        help='CSV configuration file.',
        metavar='FILE')
    parser.add_argument_with_check(
        'input_dir',
        help='Directory containing the input files.',
        metavar='DIR')
    parser.add_argument_with_check(
        'output_dir',
        help='Directory where the output files should be saved.',
        metavar='DIR')
    args = parser.parse_args()
    print args

New ArgHelper

import argparse
import os

class FileArgumentParser(argparse.ArgumentParser):
    def __is_valid_file(self, parser, arg):
        if not os.path.isfile(arg):
            parser.error('The file {} does not exist!'.format(arg))
        else:
            # File exists so return the filename
            return arg

    def __is_valid_directory(self, parser, arg):
        if not os.path.isdir(arg):
            parser.error('The directory {} does not exist!'.format(arg))
        else:
            # File exists so return the directory
            return arg

    def add_argument_with_check(self, *args, **kwargs):
        # Look for your FILE or DIR settings
        if 'metavar' in kwargs and 'type' not in kwargs:
            if kwargs['metavar'] is 'FILE':
                type=lambda x: self.__is_valid_file(self, x)
                kwargs['type'] = type
            elif kwargs['metavar'] is 'DIR':
                type=lambda x: self.__is_valid_directory(self, x)
                kwargs['type'] = type
        self.add_argument(*args, **kwargs)


Best way to work out angle between points? I have the following JavaScript code to work out the angle between two points (clockwise). The code I have seems a bit "hacky" to me, as I have a while loop ensuring the number is not negative (to make the range 0-360), as well as I have to subtract 270 degrees to get it to work correctly. Is there a better way to do this?

this.angle = function(point)
{
    var delta = point.subtract(this);
    var aR = Math.atan2(delta.y, delta.x);
    var aD = aR * (180 / Math.PI);
    aD -= 270;
    while (aD < 0)
    {
        aD += 360;
    }
    return aD;
};

First of all, let's define the problem precisely.  According to your comment, this is the behavior you want:

  You are correct in believing I am expecting it to be relative to 12 o'clock, and for it to move clockwise.

However, your code doesn't actually behave that way.  Your angle() function, as written, computes the orientation of point relative to this, with 6 o'clock being 0°, angles increasing counterclockwise, with return values in the range [0, 360).  (Alternatively stated, your angle() computes the orientation of this relative to point, with 12 o'clock being 0°, angles increasing counterclockwise, with return values in the range [0, 360).)  So, I'm reviewing erroneous code.

As evidenced by the comments, many people had problems understanding what you meant by the "angle between two points".  That's a vocabulary problem.  I suggest that you borrow a term from navigation: bearing.  In this review, I'll use the term "bearing" to refer to your desired behavior: when point is taken relative to this, 12 o'clock is considered to have a bearing of 0°, with values increasing clockwise, up to 360°.  I'll continue to use the term angle to refer to the erroneous behavior of your angle() function.

First of all, let's see how we can calculate the angle without having to do aD -= 270.  We can accomplish this by mapping the axes strategically when calling atan2().  The direction for which we want the arctangent to be 0 is an "x′ axis" of our mapped plane, and the direction for which we want the arctangent to be π/2 we will call our "y′ axis".

Here it is written out:

// Same behavior as angle(), but implemented without aD -= 270.
Point.prototype.nicerAngle = function(point) {
    var delta = point.subtract(this);
    var aR = Math.atan2(delta.x, -delta.y); // Mapped axes!
    var aD = aR * (180 / Math.PI);
    if (aD < 0) {
        aD += 360;
    }
    return aD;
};

Note that your while loop can simply be an if conditional.

Note that Math.atan2() is guaranteed to return a value in the range [-Math.PI, +Math.PI].  (See footnote below.)  We should now ask, is it possible to get rid of the conditional altogether?

One reason to get rid of the conditional is to reduce the number of code paths — fewer code paths means fewer cases to worry about in code-coverage tests.  Another reason is performance — if the CPU can always execute the code sequentially, then it doesn't have to do branch prediction.  When faced with a branch, CPUs speculatively execute code in advance, then throw away the results from the branch not taken.  (That's true for a low-level language like C.  It probably wouldn't matter at all for a naïve JavaScript interpreter.  It might make a difference on a highly optimized JavaScript JIT.  Anyway, the code-path-reduction argument alone should be a good reason to try to eliminate the conditional.)

Well, if we always add π to the value returned by Math.atan2(), then the result would be in the range [0, 2 * Math.PI].  It's a simple matter of mapping the axes to make that work for us.  We want to map the axes such that 12 o'clock yields an atan2 of 0° (our x″ axis), increasing counterclockwise such that 9 o'clock is 90° (our y″ axis).  If we can accomplish that, then adding 180° would map 6 o'clock to 0° and 3 o'clock to 90°.  Here is a diagram:

… and a solution:

// Same behavior as nicerAngle(), but implemented without if-statement.
Point.prototype.unconditionalAngle = function(point) {
    var delta = point.subtract(this);
    var aR = Math.atan2(-delta.x, delta.y);
    return 180 + aR * (180 / Math.PI);
};

So, what about computing bearing?  Once again, we simply have to remap the axes when calling atan2().  Using the same ideas as before,

// Computes orientation of point relative to this, with 12 o'clock being 0 degrees, angles
// increasing clockwise, with return values in the range [0, 360].  Implemented like
// unconditionalAngle().
Point.prototype.bearing = function(point) {
    var delta = point.subtract(this);
    return 180 + 180 / Math.PI * Math.atan2(-delta.x, -delta.y);
};

Of course, if you really wanted top performance, you could make this a one-liner by doing away with the intermediate delta object.

Footnote about unconditionalAngle() and my unconditional implementation of bearing()

Note that I said that Math.atan2() returns values in the range [-Math.PI, +Math.PI], not [-Math.PI, +Math.PI).  Therefore, with the unconditional implementations, it is possible to get a result of 360°, for example, with ORIGIN.unconditionalAngle(new Point(-0, -1)) and ORIGIN.bearing(new Point(-0, 1)).  All I'll say here is that proper IEEE floating point math has a useful concept of negative zero.  I'll leave it to you to decide whether you want to deliberately stamp out this manifestation of that concept.

Here's a jsFiddle of all the code above.


Finding 10-unique-digit number with i-first-digits divisible by i I was posed this question by a friend:

  I am a natural number written with 10 different digits such that for all \$i ∈ {1, . . ., 10}\$ the number formed by my \$i\$ first digits (from left to right) is divisible by \$i\$. Who am I?

I came up with a brute-force solution over the next hour or so (a few attempts failed) and came eventually finished with this:

# !/usr/bin/python
# -*- coding: utf-8 -*-

# build all possible permutations
import itertools
base_number = ['1','2','3','4','5','6','7','8','9','0']
perms = itertools.permutations(base_number)

# join the permutation lists into single numbers by joining each into a string
all_nums = [''.join(item[0:10]) for item in perms]

'''
Check whether each item is divisible by i (from 10-->2) for subset of item,
item[0:i]. For example;

    i = 10, number = 1234567890
    number[0:10]   = 1234567890

        is this divisible by i(10)? Yes.

    i = 9, number = 1234567890
    number[0:9]   = 123456789

        is this divisible by i(9)? Yes.

    i = 8, number = 1234567890
    number[0:8]   = 12345678

        is this divisible by i(8)? No.

We do not check whether the number is divisible by 1 as all integers are
divisible by 1.

'''
for i in range(10,1,-1):
    legits = [x for x in all_nums if int(x[0:i])%i==0]
    all_nums = legits
    print "%-10d # of result can be divided by %d" % (len(legits),i)

print legits

I considered that a recursive algorithm could perform this task much more efficiently but have not considered how to actually code the pseudo-code'ish solution I was shown;

f(1) = {1, 2, 3, 4, 5, 6, 7, 8, 9}
f(2) = for n in f(1):
       find solutions to i for $$n * 10 + i = 0 (mod 2)$$
       where i isn't a digit of n.

I welcome all feedback, but I'm specifically hoping to hear about my coding-style/practices and the algorithm I developed.

Style (and related tools)

A few things are wrong concerning the style (mostly matter of whitespace). Python has a coding guideline called PEP 8. You can use pep8 (or its online version) to check it automatically and autopep8 to try to fix this automatically. You'll find various other convenient tools to check your code such as pylint, pyflakes or pychecker.

Making things more concise

You can use string.digits to have all digits.

Also, you are using more variables that you actually need.

Finally, you don't need to precise 0 in your string slicing and all your permutations will have 10 characters so you don't have to try to take the fist 10 :

Your code becomes :

# join the permutation lists into single numbers by joining each into a string
nums = [''.join(item) for item in itertools.permutations(string.digits)]

for i in range(10, 1, -1):
    nums = [x for x in nums if int(x[:i]) % i == 0]
    print "%-10d # of result can be divided by %d" % (len(nums), i)

print nums

Solution - algorithm, math, etc

Your solution looks interesting in that sense that you iterate through less and less numbers (also, starting from 10 to remove many solutions from the very beginning is a nice touch). However, the issue is that you have to check all iterations at the very beginning and this is quite costly. Also, you keep a list of them in memory.

Let's try to do things differently.

For a start, we know that the number has to be divisible by 10. Therefore, we know that the 0 will be in last position.

>>> print(len(list(itertools.permutations(['1','2','3','4','5','6','7','8','9','0']))))
3628800
>>> print(len(list(itertools.permutations(['1','2','3','4','5','6','7','8','9']))))
362880

Your first iteration should be 10 times faster and your code is indeed roughly 10 times faster.

# join the permutation lists into single numbers by joining each into a string
nums = [''.join(item) for item in itertools.permutations(['1','2','3','4','5','6','7','8','9'])]

for i in range(9, 1, -1):
    nums = [x for x in nums if int(x[:i]) % i == 0]
    print "%-10d # of result can be divided by %d" % (len(nums), i)

print [n + '0' for n in nums]

Simimarly, the divisibility by 9 does not need to be checked because of the following rule (and the fact that the sum of the first 10 numbers is divisible by 9) : remove that check and you have yet another performance gain.

However, for an actual gain, you could go the other wat round : trying to build number by looking at the different candidates of length 1, 2, and so on.

Here is how I did it and it seems to be really fast :

cand = {0}
for i in range(1, 10):
    cand = { n for n in (10 * c + d for c in cand for d in range(1, 10) if str(d) not in str(c)) if n % i == 0}
print [n * 10 for n in cand]

Also, if you were to do things manually, after notificing that 0 has to be in 10th position so that the number is divisible by 10, you can notice that it leaves only 5 to be in 5th position so that the number is divisible by 5. You have 2, 4, 6, 8 left for 2, 4, 6 and 8th position and 1, 3, 7, 9 for 1, 3, 7 and 9th position. You can do this programmatically for yet another performance optimisation.

cand = {0}
for i in range(1, 10+1):
    r = [0] if i == 10 else \
        [5] if i == 5 else \
        [1, 3, 7, 9] if i % 2 else \
        [2, 4, 6, 8]
    cand = { n for n in (10 * c + d for c in cand for d in r if (str(d) not in str(c))) if n % i == 0}
    print(i, len(cand), cand)
print cand


Topological sort in Java I am learning algorithms in graphs.  I have implemented topological sort using Java. Kindly review my code and provide me with feedback.

import java.util.LinkedList;
import java.util.Queue;
import java.util.Stack;

public class TopologicalSortGraph {

    /**
     *  This Topological Sort implementation takes the example graph in
     *  Version 1: implementation with unweighted
     *  Assumption : Graph is directed
     */
    TopologicalSortGraph Graph = new TopologicalSortGraph();
    //public LinkedList<Node> nodes  = new LinkedList<Node>();

    public static void  topologicalSort(Graph graph) {
        Queue<Node> q = new LinkedList<Node>();
        int vertexProcessesCtr = 0;
        for(Node m : graph.nodes){
            if(m.inDegree==0){
                ++vertexProcessesCtr;
                q.add(m);
                System.out.println(m.data);
            }
        }
        while(!q.isEmpty()) {
            Node m = q.poll();

            //System.out.println(m.data);
            for(Node child : m.AdjacenctNode){
                --child.inDegree;
                if(child.inDegree==0){
                    q.add(child);
                    ++vertexProcessesCtr;
                    System.out.println(child.data);
                }
            }

        }
        if(vertexProcessesCtr > graph.vertices) {
            System.out.println();
        }

    }

    public static void main(String[] args) {
        Graph g= new Graph();
        g.vertices=8;

    Node TEN = new Node("10");
    Node ELEVEN = new Node("11");
    Node TWO = new Node("2");
    Node THREE = new Node("3");
    Node FIVE = new Node("5");
    Node SEVEN = new Node("7");
    Node EIGHT = new Node("8");
    Node NINE = new Node("9");

    SEVEN.AdjacenctNode.add(ELEVEN);
    ELEVEN.inDegree++;
    SEVEN.AdjacenctNode.add(EIGHT);
    EIGHT.inDegree++;
    FIVE.AdjacenctNode.add(ELEVEN);
    ELEVEN.inDegree++;
    THREE.AdjacenctNode.add(EIGHT);
    EIGHT.inDegree++;
    THREE.AdjacenctNode.add(TEN);
    TEN.inDegree++;
    ELEVEN.AdjacenctNode.add(TEN);
    TEN.inDegree++;
    ELEVEN.AdjacenctNode.add(TWO);
    TWO.inDegree++;
    ELEVEN.AdjacenctNode.add(NINE);
    NINE.inDegree++;
    EIGHT.AdjacenctNode.add(NINE);
    NINE.inDegree++;

    g.nodes.add(TWO);
    g.nodes.add(THREE);
    g.nodes.add(FIVE);
    g.nodes.add(SEVEN);
    g.nodes.add(EIGHT);
    g.nodes.add(NINE);

    System.out.println("Now calling the topologial sorts");

    topologicalSort(g);

    }
}

Graph class:

class Graph {
    public int vertices;
    LinkedList<Node> nodes = new LinkedList<Node>();
}

Node Class:

class Node {
    public String data;
    public int dist;
    public int inDegree;
    LinkedList<Node> AdjacenctNode = new LinkedList<Node>( );

    public void addAdjNode(final Node Child){
        AdjacenctNode.add(Child);
        Child.inDegree++;
    }

    public Node(String data) {
        super();
        this.data = data;
    }
}

class Graph {
    public int vertices;
    LinkedList<Node> nodes = new LinkedList<Node>();
}

Graph

Graph, as other data structures in general, should be declared
public. Because you want them to be able to be used outside of the
package they are declared in.

nodes, vertices should be private so that you can know that they
are not changed outside the Graph class.

nodes should not be a LinkedList. You must depend on abstractions as
much as possible. You should prefer interfaces such as List over
specific implementations LinkedList.

Nodes of a graph is not a List, it is a Set. A standard graph cannot
have multiple copies of a node. You should prefer a Set to represent a
set unless you have a good reason.

Node

All of the above points also apply to Node. Apart from those:

AdjacenctNode should be named adjacentNode by Java naming
convention.

Feel free to remove parameterless call to super();, although Eclipse
adds it by default, it's just noise.

TopologicalSortGraph

Remove unused code :

TopologicalSortGraph Graph = new TopologicalSortGraph();

Always remove commented code. If you need to see previous versions of a
code use a version control system. :

//public LinkedList<Node> nodes  = new LinkedList<Node>();

Do not put more than one space between tokens, use autoformat of your
IDE to fix formatting after changing a piece of code:

public static void  topologicalSort(Graph graph) {

The snippets :

if(child.inDegree==0){
    q.add(child);
    ++vertexProcessesCtr;
    System.out.println(child.data);
}

and

if(m.inDegree==0){
    ++vertexProcessesCtr;
    q.add(m);
    System.out.println(m.data);
}

are duplicates. They should be extracted to a private method. You are
missing some kind of abstraction there.

You are changing the internals of an object passed in as a parameter:
--child.inDegree; I do not expect my graph to change after I ask to
see its nodes printed in topological order. What if I want to print
them again?

You are mixing the calculation and the printing out of the calculation result,
I do not expect to see printlns in a method implementing an algorithm:

System.out.println( .... );

What if I want the result to be printed somewhere other than System.out?
What if I do not want the result to be printed at all and want it to be
used as an intermediate step in a bigger calculation instead?

You probably want to return a List<Node> from a topological sort
algorithm. List is the standard return type when you are sorting some
collection, that is when the result is a collection whose order is
important. You can then print that list as many times as you want or
pass it as a parameter to wherever you like.

public static void main(String[] args) {

You should separate test code from your main code. If your actual class
is named TopologicalSortGraph put your test code in
TopologicalSortGraphTest.

Use de facto standard JUnit so that instead of one big main you can
have many small tests. You can run any one of them or all of them easily
from within your IDE or from the command line.

You should try to separate the test code into separate source
directories or even into separate projects. Your implementation code
should not need or know about your test code to compile.

Another spacing (indentation) problem :

Node TEN = new Node("10");

Your code should align well. So that it reads neatly top to bottom and
scopes in it are easily identified.

TEN should be ten by Java naming convention.

Instead of these two you should use your addAdjNode method instead:

SEVEN.AdjacenctNode.add(ELEVEN);
ELEVEN.inDegree++;

Also access chains like node.AdjacenctNode.add(otherNode) or x.y.z
are usually a sign that your encapsulation is not good enough. In this
case you are modifying a collection of one class from another class.
It's a problem waiting to happen. Same encapsulation problem is present
in ELEVEN.inDegree++, too.

The root problem here is adjacency is a property of the graph -Remember
G = (V, E) from school?- and not of the nodes themselves. Instead of
node.addAdjNode(otherNode), you should use a method like
graph.addEdge(node, otherNode).

Same problem also exists here: g.nodes.add(TWO); You should have a
Graph.addNode(node) method instead.

Coming back to G = (V, E); it says, if you listen carefully, you need
a set of vertices and a set of edges to have a graph and should not add
nodes one by one. Ideally, Graph would have a constructor like this
public Graph(Set<Node> vertices, Set<Edge> edges).


Implementing my own shared-ownership double-indirecting smart-pointer I am implementing my own double-indirecting shared-ownership smart-pointer because I needed a way to replace the managed object a group of smart-pointers is pointing to with a new object (it's the method reset_all in the code below).

I tried to keep this as close as I could to the std::shared_ptr's interface, while avoiding things I don't understand / need like aliasing and conversion from other smart-pointer types.

//(it's all in an header file)
#pragma once

template <typename T>
class Manager {
    template <typename U>
    friend class SharedPtr;

    T* managed;
    unsigned int counter;

    Manager(T* new_ptr):
        managed(new_ptr), counter(1) {}
    ~Manager() = default;

    Manager<T>* own() {
        ++counter;
        return this;
    }
    void disown() {
        if (!(--counter)) {
            delete managed;
            delete this;
        }
    }
};

template <typename T>
class SharedPtr {
    public:
    SharedPtr() = default;
    SharedPtr(T* new_ptr) {
        if (new_ptr) {
            manager = new Manager<T>(new_ptr);
        }
    }
    SharedPtr(const SharedPtr &rhs):
        manager(rhs.manager->own()) {}
    SharedPtr(SharedPtr &&rhs):
        manager(rhs.manager) {
        rhs.manager = nullptr;
    }

    SharedPtr& operator=(const SharedPtr &rhs) {
        if (manager) {
            manager->disown();
        }
        manager = rhs.manager->own();
        return *this;
    }
    SharedPtr& operator=(SharedPtr &&rhs) {
        if (manager) {
            manager->disown();
        }
        manager = rhs.manager;
        rhs.manager = nullptr;
        return *this;
    }

    void swap(SharedPtr &rhs) {
        Manager<T>* tmp = manager;
        manager = rhs.manager;
        rhs.manager = tmp;
    }

    void reset() {
        if (manager) {
            manager->disown();
        }
        manager = nullptr;
    }
    void reset(T* new_ptr) {
        if (manager) {
            manager->disown();
        }
        manager = new Manager<T>(new_ptr);
    }

    T* get() {
        if (manager) {
            return manager->managed;
        }
        return nullptr;
    }

    T& operator*() {
        return *(manager->managed);
    }

    T* operator->() {
        return manager->managed;
    }

    unsigned int use_count() {
        if (manager) {
            return manager->counter;
        }
        return 0;
    }

    bool unique() {
        return use_count() == 1;
    }

    operator bool() {
        return (bool) manager;
    }

    void reset_all(T* new_ptr) {
        if (manager) {
            delete manager->managed;
            manager->managed = new_ptr;
            return;
        }
        manager = new Manager<T>(new_ptr);
    }

    ~SharedPtr() {
        if (manager) {
            manager->disown();
        }
    }

    private:
    Manager<T>* manager = nullptr;
};

I wouldn't call it SharedPtr because the contrast to a std::shared_ptr is too big, and the name too similar.
Maybe DoublePtr or alike to emphasize the double-indirection which is the crucial feature...
I would make Manager a private non-template member of your class, because it's an implementation-detail. Also, I would make it a dumb struct and put the useful function it contains (disown) into the private part of DoublePtr.
While having a default-ctor without allocation might be enough of a boon to allow a resourceless state (it probably is), there's no reason to disallow creating a pointer-group which starts as nullptr.
Thus I would remove the check in the ctor.
op= is wrong, try self-assignment. It should be disown after own.
op=(&&) would be better as a simple swap.
op(bool) is wrong: Use get() != nullptr.
reset and reset_all could easily be upgraded from basic to strong exception-safety (commit-or-rollback), just create a new DoublePtr and swap.
Many of your functions can and should be marked noexcept.
Also, pure observing methods should be marked const.


Creating an app to simulate load I am writing an app that is supposed to simulate load on our web api and NServiceBus(particular) service.  This is the first time I am writing an application like this.  It appears to run ok, however it takes a few seconds before anything shows up in the console window.  Is there a better way to do this?
class Program
{
    static void Main(string[] args)
    {
        Console.WriteLine("Press any key to start...");
        Console.ReadKey();
        Console.WriteLine();

        RunLoad(100);

        Console.WriteLine("Press any key to exit...");
        Console.ReadKey();
    }

    private static void RunLoad(int loadSize)
    {
        Parallel.For(0, loadSize, i =>
        {
            RunTasks();
        });
    }

    public static async Task RunTasks()
    {
        var tasks = new List<Task>
        {
            GetLoanByLoanId("7000002050"),
            GetEnvelopesForLoan("7000002077"),
            GetLoanDataByLoanId("7000002072")

        };

        await Task.WhenAll(tasks);
    }

    private static async Task GetLoanByLoanId(string loanNumber)
    {
        await Task.Run(() => {
            var svc = new WebApiService();
            var msg1 = svc.GetLoanByLoanId(loanNumber);
            if (string.IsNullOrWhiteSpace(msg1)) return;
            Console.WriteLine($"GL {loanNumber}: {msg1.Length}");
        });
    }

    private static async Task GetLoanDataByLoanId(string loanNumber)
    {
        await Task.Run(() => {
            var svc = new WebApiService();
            var msg1 = svc.GetLoanDataByLoanId(loanNumber);
            if (string.IsNullOrWhiteSpace(msg1)) return;
            Console.WriteLine($"GLD {loanNumber}: {msg1.Length}");
        });
    }

    private static async Task GetEnvelopesForLoan(string loanNumber)
    {
        await Task.Run(() => {
            var svc = new WebApiService();
            var msg1 = svc.GetEnvelopesForLoan(loanNumber);
            if (string.IsNullOrWhiteSpace(msg1)) return;
            Console.WriteLine($"GEFL {loanNumber}: {msg1.Length}");
        });
    }

}
}

I am trying to mimic the number to users hitting the system through the RunLoad method.
Also this is running in .Net Framework 4.8..

There are several improvement areas. Let's review them one-by-one from bottom up:
GetXYZ
private static async Task GetXYZ(string parameter)
{
     await Task.Run(() => {
         var svc = new WebApiService();
         var msg1 = svc.GeXYZ(parameter);
         if (string.IsNullOrWhiteSpace(parameter)) return;
            Console.WriteLine($"XYZ {parameter}");
     });
}

A Task can represent either an asynchronous work or a parallel work. An async work can be considered as non-blocking I/O operation. Whereas the parallel work can be treated as (blocking) CPU operation. (It is oversimplified but it works for us now)
In the above code the Task.Run tells to .NET runtime that this piece of code should run a dedicated thread (in reality this is not as simple as this, but let me simplify the model now). It means that the passed delegate should run a dedicated CPU core.
But inside your delegate you are making a blocking I/O operation. So you have created a delegate, you have moved that code to a dedicated core, which will block and do nothing until the network driver will finish the I/O operation.
You are just wasting a lot of valuable resources. A better alternative would look like this:
private static async Task GetXYZ(string parameter)
{
     var svc = new WebApiService();
     var msg1 = await svc.GeXYZAsync(parameter);
     if (string.IsNullOrWhiteSpace(parameter)) return;
        Console.WriteLine($"XYZ {parameter}");
}

Here you are making a non-blocking I/O operation. The network driver fires of a network call asynchronously and returns immediately. When it completes it will notify the ThreadPool. (Because here we are calling await against an asynchronous work that's why we are not blocking the caller Thread.)
In short: Network driver can do let's say 1000 concurrent network operations at the same time. While the CPU can perform as many operations in parallel as many cores it has.
RunTasks
(I guess you can find a better name for this.)
Here with the Task.WhenAll you are already asking the .NET runtime to run them in parallel. If they are asynchronous operations then the network driver will take care of them and try to run them in parallel. In case of parallel work CPU tries to schedule them on different cores but there is no guarantee that they will run in parallel. They might run only concurrently (with context-switches).
RunLoad
(I guess you can find a better name for this.)
Here with the Parallel.For you try to create as many blocking parallel task as many the loadSize value. If you have 100 Tasks for 8 CPU cores then it is inevitable to have context-switches. Again your current implementation is highly CPU-bound even though you try to perform massive I/O operations.
A better alternative:
private static async Task RunLoad(int loadSize)
{
    var clients = new List<Task>();
    for (int j = 0; j < loadSize; j++)
    {
        clients.Add(RunTasks());
    }

    await Task.WhenAll(clients);
}

In this way all your client's all requests might run in parallel if your network driver supports that many outgoing requests.
This WhenAll will be finished when all operation has finished. If you want to monitor them in "real-time" when any of them finished then you can rewrite your method in the following way:
static async Task Main(string[] args)
{
    Console.WriteLine("Press any key to start...");
    Console.ReadKey();
    Console.WriteLine();

    await foreach (var test in RunTests(10))
    {
        //NOOP
    }

    Console.WriteLine("Press any key to exit...");
    Console.ReadKey();
}

private static async IAsyncEnumerable<Task> RunTests(int loadSize)
{
    var requests = new List<Task>();
    for (int j = 0; j < loadSize; j++)
    {
        requests.Add(GetLoanByLoanId("7000002050"));
        requests.Add(GetEnvelopesForLoan("7000002077"));
        requests.Add(GetLoanDataByLoanId("7000002072"));
    }

    while (requests.Any())
    {
        var finishedRequest = await Task.WhenAny(requests);
        requests.Remove(finishedRequest);
        yield return finishedRequest;
    }
}


Sieve of Atkin in Python I recently implemented the Sieve of Atkin prime generating algorithm in Python. Though I know the term "pythonic" isn't exactly set in stone, I can tell that my program doesn't quite take advantage of python's inherent traits; my program looks like it was written with "C" in mind.

My question is two fold; (1) how can I make this more pythonic, and (2) how can I improve its performance?

I suspect the solution lies in making less use of lists in lieu of generators, but I'm not sure how to do so here:

def atkin_sieve(limit):
    sieve_list = [-1]*limit
    sieve_list[2] *= -1
    sieve_list[3] *= -1
    x = 1
    # Part I: preliminary work
    while(x*x < limit):
        y = 1
        while(y*y < limit):
            n = (4*x*x)+(y*y)
            if n <= limit and (n%12==5 or n%12==1):
                sieve_list[n] *= -1

            n = (3*x*x)+(y*y)
            if n <= limit and n%12==7:
                sieve_list[n] *= -1

            n = (3*x*x)-(y*y)
            if n <= limit and n%12==11 and x>y:
                sieve_list[n] *= -1
            y += 1
        x += 1
    # Part II: Remove the squares of primes (and their multiples)
    r = 5
    while r*r < limit:
        if sieve_list[r] > 0:
            i = r*r
            while i < limit:
                sieve_list[i] = -1
                i += r*r
        r += 1
    # Part III: Append everything into a list
    results = []
    x = 0
    for p in sieve_list:
        if p > 0:
            results.append(x)
        x += 1
    return results

So I recast your code to make it a bit more pythonic. For reference, all of the code is at the bottom of this post. I played around a bit looking for some performance improvements, but found nothing significant.

Some notes:

Pep8:

First thing to do is get a style/lint checker.  I use the pycharm ide which will show you style and compile issues right in the editor.  When you get a chance you should also read through pep8 which is the official python style guide.

Native Types:

I changed all of the +/-1 to True/False

Bug at a boundary:

The sieve_list list was one element too short.  I discovered this with a quick sanity test of:

print atkin_sieve(28)
print atkin_sieve(29)
print atkin_sieve(30)

Use python iterators:

This:

i = r*r
while i < limit:
    sieve_list[i] = -1
    i += r*r

became:

for n in range(r_squared, len(sieve_list), r_squared):
    sieve_list[n] = False

And this:

results = []
x = 0
for p in sieve_list:
    if p > 0:
        results.append(x)
    x += 1
return results

became:

return [x for x, p in enumerate(sieve_list) if p]

Code:

def atkin_sieve(limit):
    assert limit > 3
    sieve_list = [False] * (limit + 1)
    sieve_list[2:4] = (True, True)

    # Part I: preliminary work
    x = x_squared = 1
    while x_squared < limit:
        y = y_squared = 1
        while y_squared < limit:
            n = 4 * x_squared + y_squared
            if n <= limit and n % 12 in (1, 5):
                sieve_list[n] = not sieve_list[n]

            n = 3 * x_squared + y_squared
            if n <= limit and n % 12 == 7:
                sieve_list[n] = not sieve_list[n]

            if x > y:
                n = 3 * x_squared - y_squared
                if n <= limit and n % 12 == 11:
                    sieve_list[n] = not sieve_list[n]
            y += 1
            y_squared = y * y
        x += 1
        x_squared = x * x

    # Part II: Remove the squares of primes (and their multiples)
    r = 5
    r_squared = r * r
    while r_squared < limit:
        if sieve_list[r]:
            for n in range(r_squared, len(sieve_list), r_squared):
                sieve_list[n] = False
        r += 1
        r_squared = r * r

    # Part III: Append everything into a list
    return [x for x, p in enumerate(sieve_list) if p]


Aggregate by several keys I have this rather verbose piece of code. How can I get it shorter and simpler?

This function aggregate results of test by population and by different keys:

def aggregate(f):
  """ Aggregate population by several keys """
  byMerchant  = col.defaultdict(lambda: col.defaultdict(lambda: [0,0,0,0]))
  byPlat      = col.defaultdict(lambda: col.defaultdict(lambda: [0,0,0,0]))
  byDay       = col.defaultdict(lambda: col.defaultdict(lambda: [0,0,0,0]))
  byAffiliate = col.defaultdict(lambda: col.defaultdict(lambda: [0,0,0,0]))
  byCountry   = col.defaultdict(lambda: col.defaultdict(lambda: [0,0,0,0]))
  byType      = col.defaultdict(lambda: col.defaultdict(lambda: [0,0,0,0]))
  byPop       = col.defaultdict(lambda: [0,0,0,0])
  for line in f:
    fields = line.strip().split("^A")
    if fields[2] == '\\N':  continue

    day          = fields[0]
    popid        = int(fields[1])
    affiliate_id = int(fields[2])
    merchant_id  = int(fields[3])
    clicks       = int(fields[4])
    displays     = int(fields[5])
    sales        = float(fields[6])
    gross        = float(fields[7])

    merchant_plat = partners_info[merchant_id][1]
    merchant_name = partners_info[merchant_id][0]

    publisher_name    = publisher_info[affiliate_id][0]
    publisher_country = publisher_info[affiliate_id][1]
    publisher_type    = publisher_info[affiliate_id][2]

    data = [clicks,displays,sales,gross]
    for k in xrange(len(data)):
      byMerchant[merchant_name][popid][k]    += data[k]
      byPlat[merchant_plat][popid][k]        += data[k]
      byAffiliate[publisher_name][popid][k]  += data[k]
      byCountry[publisher_country][popid][k] += data[k]
      byDay[day][popid][k]                   += data[k]
      byType[publisher_type][popid][k]       += data[k]
      byPop[popid][k]                        += data[k]

return byPop, byMerchant, byPlat, byDay, byAffiliate, byCountry, byType

If I try to split on the empty string I get an error:

>>> 'a b c'.split('')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: empty separator

Perhaps you meant .split()?

Anyway, here are some ideas for simplifying your code.

Use from collections import defaultdict to avoid the need to refer to col (I presume you have import collections as col).
Put a dummy key into the byPop aggregation so that its structure matches the others.
Put the aggregates into a dictionary instead of having a bunch of variables. That way you can manipulate them systematically in loops.
Use enumerate in your last loop to get the index as well as the value from the data list.

That leads to this:

from collections import defaultdict

def aggregate(f):
    """
    Aggregate population by several keys.
    """
    aggregates = 'pop merchant plat day affiliate country type'.split()
    result = dict()
    for a in aggregates:
        result[a] = defaultdict(lambda: defaultdict(lambda: [0,0,0,0]))

    for line in f:
        fields = line.strip().split()
        if fields[2] == '\\N': continue

        # Decode fields from this record.
        day          = fields[0]
        popid        = int(fields[1])
        affiliate_id = int(fields[2])
        merchant_id  = int(fields[3])
        clicks       = int(fields[4])
        displays     = int(fields[5])
        sales        = float(fields[6])
        gross        = float(fields[7])

        # Keys under which to aggregate the data from this record.
        keys = dict(merchant = partners_info[merchant_id][0],
                    plat = partners_info[merchant_id][1],
                    day = day,
                    affiliate = publisher_info[affiliate_id][0],
                    country = publisher_info[affiliate_id][1],
                    type = publisher_info[affiliate_id][2],
                    pop = 0)       # dummy key

        # Update all aggregates.
        for i, value in enumerate([clicks, displays, sales, gross]):
            for a in aggregates:
                result[a][keys[a]][popid][i] += value

    return (result['pop'][0],) + tuple(result[a] for a in aggregates[1:])

It's not really a vast improvement.


Performance shifting for SQL Database and Linq memory grouping I am messing around with shifting between stored procedures and business layer EF getting data rapidly then processing in memory.

This works and on a static data range of 3 months I get execution time of 2.3 seconds

Is there anything I can do to help the compiler increase CPU efficiency or even a better way to get data without worrying about locks, just get it in one quick go, it doesn't  change any way?

public static List<DashGraph> GetTimeOverviewData(DateTime start, DateTime end)
        {
            List<DashHelper> totals;
            List<DashHelper> visitors;
            List<DashHelper> visits;
            List<DashHelper> calls;

            Stopwatch sw = new Stopwatch();
            sw.Start();

            //Works like NOLOCK?
            using (new TransactionScope(
                    TransactionScopeOption.Required,
                    new TransactionOptions { IsolationLevel = IsolationLevel.ReadUncommitted }))
            {
                using (var db = new Track_LOpEntities())
                {
                    visitors = db.JTrack_Visitors.AsNoTracking()
                        .Where(v => v.CreatedOn >= start & v.CreatedOn <= end)
                        .Select(v => new DashHelper() { DateAndTime = v.CreatedOn.Value, Source = 1 })
                        .ToList();

                    visits = db.JTrack_Visits.AsNoTracking()
                        .Where(v => v.DateTimeStart >= start & v.DateTimeStart <= end)
                        .Select(v => new DashHelper() { DateAndTime = v.DateTimeStart.Value, Source = 2 })
                        .ToList();

                    calls = db.JTrack_Calls.AsNoTracking()
                        .Where(v => v.calldate >= start & v.calldate <= end)
                        .Select(v => new DashHelper() { DateAndTime = v.calldate.Value, Source = 3 })
                        .ToList();
                }
            }

            totals = visitors.Concat(visits).Concat(calls).ToList();

            var visitorsGroup = totals.GroupBy(q => new { q.DateAndTime.Date, q.DateAndTime.Hour });

            List<DashGraph> dg = new List<DashGraph>();

            foreach (var g in visitorsGroup)
            {
                dg.Add(new DashGraph()
                {
                    Date = g.Key.Date,
                    Hour = g.Key.Hour,
                    TotalVisitors = g.Where(x => x.Source == 1).Count(),
                    TotalPageViews = g.Where(x => x.Source == 2).Count(),
                    TotalCalls = g.Where(x => x.Source == 3).Count()
                });
            }

            sw.Stop();
            var exectionTime = TimeSpan.FromMilliseconds(sw.ElapsedMilliseconds);

            return dg;
        }

It's hard to tell if this will work without trying it but I'd at least give this a shot. By calling ToList on your calls, visits and visitors and then concating them you are losing the database query structure, try this instead:

            using (var db = new Track_LOpEntities())
            {
                var visitors = db.JTrack_Visitors.AsNoTracking()
                    .Where(v => v.CreatedOn >= start & v.CreatedOn <= end)
                    .Select(v => new DashHelper() { DateAndTime = v.CreatedOn.Value, Source = 1 });

                var visits = db.JTrack_Visits.AsNoTracking()
                    .Where(v => v.DateTimeStart >= start & v.DateTimeStart <= end)
                    .Select(v => new DashHelper() { DateAndTime = v.DateTimeStart.Value, Source = 2 });

                var calls = db.JTrack_Calls.AsNoTracking()
                    .Where(v => v.calldate >= start & v.calldate <= end)
                    .Select(v => new DashHelper() { DateAndTime = v.calldate.Value, Source = 3 });
               var totals = visits.Union(calls).Union(visitors);
               var visitorsGroup = totals.GroupBy(q => new { q.DateAndTime.Date, q.DateAndTime.Hour });

        List<DashGraph> dg = new List<DashGraph>();

        foreach (var g in visitorsGroup)
        {
            dg.Add(new DashGraph()
            {
                Date = g.Key.Date,
                Hour = g.Key.Hour,
                TotalVisitors = g.Where(x => x.Source == 1).Count(),
                TotalPageViews = g.Where(x => x.Source == 2).Count(),
                TotalCalls = g.Where(x => x.Source == 3).Count()
            });
        }

            }

Also I'm not sure why you need a transaction scope, your not doing any updates.


Filtering index using has_many through relationships Both Cities and Positions are associated to Jobs through a has_many :through relationship.

On the Jobs#Index page, I have a form, which when submitted allows a user to filter jobs based on a combination of city and positions.

I got this to work, but know the code can be a lot better.

This is my Jobs#Index controller:

  def index
    @cities = City.all
    @positions = Position.all

    @city = City.find(params[:city_id]) if params[:city_id] && params[:city_id] != "0"
    @position = Position.find(params[:position_id]) if (params[:position_id] && params[:position_id] != "0")

    @jobs = Job.all
    @jobs = @jobs.includes(:cities).where(cities: { id: @city }) if @city
    @jobs = @jobs.includes(:positions).where(positions: { id: @position }) if @position
    @jobs = @jobs.paginate(page: params[:page], per_page: 5)

  end

This is my Jobs#Index View:

- provide(:title, 'All Jobs')

.thinstripe_career
  .darken
    .container
      .row
        .span10.offset1
          .jobs_header
            .row
              .span7
                h2 All #{@city.name if @city} #{@position.name if @position} Jobs
              .span3
                == link_to "Sign up for Job Alerts!", "#", :class => "button"

- if current_user && current_user.admin?
  == render 'shared/dashboard_header'

.container
  .row
    .span10.offset1
      .job_search
        == form_tag jobs_path, :method => 'get', :id => "jobs_filter" do
          h2 I'd like jobs in
          .filter_sect
            == select_tag :city_id, content_tag(:option,'all cities...', :value=>"0") + options_from_collection_for_select(@cities, "id", "name", params[:city_id].to_i  ), :class => "basic"
          .filter_sect
            == select_tag :position_id, content_tag(:option,'all roles...',:value=>"0") + options_from_collection_for_select(@positions, "id", "name", params[:position_id].to_i ), :class => "basic"
          .filter_sect.filter_search
            == submit_tag "Search", :class => "button search_button button_black", :name => nil

      ul.jobs
        - if @jobs.any?
          == render @jobs
        - else
          li.non_founds
            h2 No jobs founds.
            p There are currently no jobs founds for these combinations. Sign up to requests

      == will_paginate

In models/job.rb, you can add scopes and use them chained on the controller:

class Job < ActiveRecord::Model
  scope :with_cities, ->(city) { includes(:cities).where(cities: { id: @city })  if city }
  scope :with_positions, ->(position) { includes(:positions).where(positions: { id: @position }) if position }

  # rest of the code ...
end

For JobsController, you can:

Add a before_filter to load all the data, and if you need to use that data on other actions, you just need to append the name of that action too.
Use memoization to load the city and the position.

In controllers/jobs_controller.rb:

class JobsController < ApplicationController
  before_filter :load_data, only: [:index]

  def index
    @jobs = Job.with_cities(city).with_positions(position).paginate(page: params[:page], per_page: 5)
  end

  # ...

  protected:

  def load_data
    @cities = City.all
    @positions = Position.all
  end

  def city
    @city ||= City.find(params[:city_id]) unless params[:city_id].empty?
  end

  def position
    @position ||= Position.find(params[:position_id]) unless params[:position_id].empty?
  end

On the view:

Use try, so instead of:

    - if current_user && current_user.admin?
      == render 'shared/dashboard_header'

you could write:

    == render 'shared/dashboard_header' if current_user.try(:admin?)

Use include_blank and prompt options in select_tag.
This not only simplifies the views, but also allows to use params[city_id].empty? on the controller.

select_tag :city_id,
  options_from_collection_for_select(@cities, :id, :name, params[:city_id].to_i),
    include_blank: true, prompt: "all cities...", class: "basic"

Finally, you could extract ul.jobs into a partial:

= render partial: 'jobs', locals: { jobs: @jobs }

Or even the .job_search div, especially if you'll be needing this somewhere else.


Possible security issues in email validation I need feedback on this code regarding security issues.

<?php
if(!isset($_POST['submit']))
{
    //This page should not be accessed directly. Need to submit the form.
    echo "error; you need to submit the form!";
}
$name = $_POST['name'];
$visitor_email = $_POST['email'];
$message = $_POST['message'];

//Validate first
if(empty($name)||empty($visitor_email))
{
    echo "Name and email are mandatory!";
    exit;
}

if(IsInjected($visitor_email))
{
    echo "Bad email value!";
    exit;
}

$email_from = 'someemail@some.com';//<== update the email address
$email_subject = "New Form submission";
$email_body = "You have received a new message from the user $name.\n".
    "Here is the message:\n $message".

$to = "someemail@some.com";//<== update the email address
$headers = "From: $email_from \r\n";
$headers .= "Reply-To: $visitor_email \r\n";
//Send the email!
mail($to,$email_subject,$email_body,$headers);
//done. redirect to thank-you page.
header('Location: thank-you.html');

I'm using this IsInjected($str) function to validate email addresses:

// Function to validate against any email injection attempts
function IsInjected($str)
{
  $injections = array('(\n+)',
              '(\r+)',
              '(\t+)',
              '(%0A+)',
              '(%0D+)',
              '(%08+)',
              '(%09+)'
              );
  $inject = join('|', $injections);
  $inject = "/$inject/i";
  if(preg_match($inject,$str))
    {
    return true;
  }
  else
    {
    return false;
  }
}

?>

In your first validation, I think you forgot to exit, you probably meant:

if(!isset($_POST['submit']))
{
    //This page should not be accessed directly. Need to submit the form.
    echo "error; you need to submit the form!";
    exit;
}

To validate email addresses, you can use the filter_var method, like this:

function is_valid_email($str) {
    return filter_var($str, FILTER_VALIDATE_EMAIL);
}

For example:

function check_email($email) {
    printf("checking '%s': %s\n", $email, is_valid_email($email) ? 'OK' : 'INVALID');
}
check_email('joe@example.com');
check_email('bogus');

will output:

checking 'joe@example.com': OK
checking 'bogus': INVALID

For more details and (caveats!), see the docs. Also some more examples here.

Your original IsInjected method might still be useful for other purposes, that's why I created a new method for validating emails. If you keep it I would rename it to hasInjectedChars or has_injected_chars. And these lines can be improved:

  if(preg_match($inject,$str))
    {
    return true;
  }
  else
    {
    return false;
  }

you could write simply:

return preg_match($inject, $str);


Vue component over-complication I'm building a date range selector, and while this works, I feel like I'm making it more complicated than it needs to be. Is there a more elegant way of writing this?

Possibly using computed values or watches?

<template>
<form>
    <select  @change="rangeSelection">
            <option
                v-for="(option, key) in rangeOptions"
                :key="key"
                :value="key">
                {{option.display}}
            </option>
    </select>
    </form>
</template>

<script>
import moment from 'moment';

export default {
  name: 'DateRangeChooser',
  data: () => {
    return {
        selectedRange: "last7Days",
        startDate: moment().subtract(8, 'days'),
        endDate: moment().subtract(1, 'days'),
        rangeOptions: {
            last7Days: {
                display: 'Last 7 Days',
                startDate: moment().subtract(8, 'days'),
                endDate: moment().subtract(1, 'days')
            },
            lastWeek: {
                display: 'Last Week',
                startDate: moment().startOf('week').subtract(1, 'week'),
                endDate: moment().endOf('week').subtract(1, 'week')
            },
            last30days: {
                display: 'Last 30 days',
                startDate: moment().subtract(31, 'days'),
                endDate: moment().subtract(1, 'days')
            }
        }
    }
  },
  methods: {
    rangeSelection: function(e){
        this.endDate = this.rangeOptions[e.currentTarget.value].endDate;
        this.startDate = this.rangeOptions[e.currentTarget.value].startDate;
    }
  }
};
</script>

Yes you can use computed properties:

computed: {
    endDate: function() {
      //determine endDate based on value of selectedRange
    },
    startDate: function() {
      //determine startDate based on value of selectedRange
    }
}

And v-model can be used to bind the value of the select list to that data property:

<select v-model="selectedRange">

Then use that property in the computed properties:

  computed: {
    endDate: function() {
      return this.rangeOptions[this.selectedRange].endDate;
    },
    startDate: function() {
      return this.rangeOptions[this.selectedRange].startDate;
    }
  }

With this approach there is no need to define the rangeSelection method and bind it to the onchange property.

rangeOptions could also be moved outside the component, and then the first key could be used to select the default value of selectedRange instead of hard coding it. See the snippet below.

const rangeOptions = {
  last7Days: {
    display: 'Last 7 Days',
    startDate: moment().subtract(8, 'days'),
    endDate: moment().subtract(1, 'days')
  },
  lastWeek: {
    display: 'Last Week',
    startDate: moment().startOf('week').subtract(1, 'week'),
    endDate: moment().endOf('week').subtract(1, 'week')
  },
  last30days: {
    display: 'Last 30 days',
    startDate: moment().subtract(31, 'days'),
    endDate: moment().subtract(1, 'days')
  }
};
const form = new Vue({
  el: '#DateRangeChooser',
  data: () => {
    return {
      selectedRange: Object.keys(rangeOptions)[0],
      rangeOptions
    }
  },
  computed: {
    endDate: function() {
      return rangeOptions[this.selectedRange].endDate;
    },
    startDate: function() {
      return rangeOptions[this.selectedRange].startDate;
    }
  }
});

  <select v-model="selectedRange">
    <option v-for="(option, key) in rangeOptions" :key="key" :value="key">
      {{option.display}}
    </option>
  </select>

Start: {{startDate.format('LL')}}

End: {{endDate.format('LL')}}

Edit

@blindman67 brought up an interesting point:

  Sorry my VUE knowledge is limited but I thought computed properties were cached. Could be a problem in some edge cases, eg time near midnight and user selects, then changes their mind waits till after midnight selects again?1

That is true:

  ... computed properties are cached based on their dependencies2

if caching is an issue, then you may need to look at using a watcher or just calculate the computed property each time instead of referencing values within this.rangeOptions.

1Vue component over-complication

2https://vuejs.org/v2/guide/computed.html#Computed-Caching-vs-Methods


Codeigniter view within a view I'm calling a view into a view. Am I doing correctly, or is this a bad practice?

                 Nuevo Usuario

        <?php $this->load->view('modules/user/form_add_user'); ?>

For your controller, a method like this would be best to call all your variables:

$data = array(
    "add_user" => $this->load->view('form_add_user'),
    "username" => $username,
    "admin"    => $admin
);

(Variables are made up just for example)

Then, within your code, do this:

                 Nuevo Usuario

        <?=$add_user;?>


Script to monitor a folder and compress the content We have a Debian Squeeze server where we upload files through SFTP, and I wrote this script to automatically compress uploaded files.

The script is scheduled through a cron job in an unprivileged user's crontab.

As of now, the script works and does its job.

But I'm pretty novice to Bash scripting, and I'm wondering if it has any major flaws. (e.g. If the file name contains " what happens? Does it have security flaws?)

Also, am I doing it right, or there are better ways to do it?

#!/bin/bash

# Use the newline character
# as the field separator.
IFS=$'\n'

# The folder to watch.
dir="/path"

# Get the folder's content, recurively.
for item in $(find "${dir}")
do

        # If the item is a regular file.
        if [ -f "${item}" ]
        then

                # Check if the file is in use
                # by someone else. E.g. The SFTP
                # daemon because the user is still
                # uploading it.
                lsof -n "${item}" > /dev/null

                # If no one is using the file.
                if [ "$?" -ne 0 ]
                then

                        # Get the file extension.
                        ext=$(echo "${item}" | awk -F "." '{ print $NF }')

                        # If the file is not a zip archive
                        # and does not exists a zip archive for it
                        if [ "${ext}" != "zip" -a ! -f "${item}.zip" ]
                        then
                                # Create the zip archive.
                                zip "${item}.zip" "${item}" > /dev/null
                        fi

                fi

        fi

done

Slightly different than @kojiro's answer, using find for everything:

find /path \
     -type f \
     ! -iname "*.zip" \
     ! -exec /usr/sbin/lsof -n "{}" \; \
     ! -exec test -f "{}.zip" \; \
     -exec zip -q "{}.zip" "{}" \; \
     -exec rm "{}" \;

(last step only if the removal of the original is required).


Synchronous and asynchronous methods in a client library I am working on a library in which I need to make synchronous and asynchronous methods in my client library.

My library does this:

The customer will use our library and they will call it by passing user_id. We will then construct a URL by using that userId and make an HTTP client call to that URL. We will then get a JSON string back after hitting the URL. And after we get the response back as a JSON string, then we will send that JSON String back to our customer as it is.

Now I need to have synchronous and asynchronous methods. Some customer will call the executeSynchronous method to get the same feature and some customer will call our executeAsynchronous method and with the executeAsynchronous method, they will call future.get in there code itself.

Interface:

public interface Client {

    // for synchronous
    public String executeSynchronous(final String userId);

    // for asynchronous
    public Future<String> executeAsynchronous(final String userId);
}

And then I have SmartClient which implements the Client interface:

public class SmartClient implements Client {

    private RestTemplate restTemplate = new RestTemplate();
    private ExecutorService service = Executors.newFixedThreadPool(5);

    // for synchronous call
    @Override
    public String executeSynchronous(String userId) {

        String response = null;

        try {
            Future<String> handle = executeAsynchronous(userId);
            response = handle.get(500, TimeUnit.MILLISECONDS);
        } catch (TimeoutException e) {
            e.printStackTrace();
        }

        return response;
    }

    //for asynchronous call
    @Override
    public Future<String> executeAsynchronous(String userId) {
        Future<String> future = null;

        try {
            Task task = new Task(userId, restTemplate);
            future = executor.submit(task);
        } catch (Exception ex) {
            e.printStackTrace();
        }

        return future;
    }
}

This simple class will perform the actual task:

class Task implements Callable<String> {

    private final String userId;
    private RestTemplate restTemplate;

    public Task(String userId, RestTemplate restTemplate) {
        this.userId = userId;
        this.restTemplate = restTemplate;
    }

    public String call() throws Exception {

        String url = createURL(userId);

        // make a HTTP call to the URL
        String jsonResponse = restTemplate.exchange(url, HttpMethod.GET, null, String.class);

        return jsonResponse;
    }

    // create a URL
    private String createURL(String userId) {
        String generateURL = somecode;

        return generateURL;
    }
}

Is this the correct and efficient way of solving this problem? Can we generalize my interface?
How about the exception handling?

I need to have both synchronous and asynchronous methods so that the customer can call any method they like to call. Some customers might call the asynchronous method and then they will do future.get in their own library. Some can call the synchronous method, which internally can call the asynchronous method and do a future.get on it to return the response.

public String executeSynchronous(final String userId);

Drop this final as it helps nobody.

  How about the exception handling?

Exception handling is always a pain, especially with checked exceptions. Usually, the best way is the simplest one, i.e., do nothing. Declare the exception to be thrown and let it blow. Someone up the stack will take care of it (it may be you) in a place where this makes sense.

e.printStackTrace();

This is rather wrong. You should log the message, printing to stderr may be redirected to somewhere where nobody sees it. What's worse, your method returns null and the caller has no way to find out what went wrong. Even worse, the caller does not expect null and will get an NPE one day (usually, when it creates the biggest damage).

@Override
public String executeSynchronous(String userId) {

    String response = null;

    try {
        Future<String> handle = executeAsynchronous(userId);
        response = handle.get(500, TimeUnit.MILLISECONDS);
    } catch (TimeoutException e) {
        e.printStackTrace();
    }

    return response;
}

You're actually using two threads (the current one and one in the executor) where just one is needed. It should go the other way round: Let executeSynchronous do the work and submit a Callable calling it when asynchronous execution is needed.

  Is this the correct and efficient way of doing this problem?

I guess, it's correct, but concerning efficiency it's no good to block two threads instead of one (and there's also the small overhead of executor, which is useless in the synchronous case).

  ... Can we generalize interface?

Maybe... I guess you want many such methods... and an interface containing both versions would be twice as big; no good idea. There's

com.google.common.util.concurrent.Futures.immediateFuture(@Nullable V value)

in Guava, which could help. Instead of two methods, use two implementations of a single interface (synchronous and asynchronous).

public interface Client {
    public Future<String> executeAsynchronous(String userId);
}

The synchronous implementation does the real job normally, but returns an immediateFuture instead of the value itself.

The asynchronous implementation calls the synchronous one using the executor. In case of 10+ methods I'd consider using a dynamic proxy.

Alternatively, you could add an argument

enum Synchronicity {SYNCHRONOUS, ASYNCHRONOUS}

(don't use a boolean as its meaning is not obvious) to all the interface methods.


Python use glob to search for files by extension This code works, but I feel bad about it. Did I achieve Python Zen or create a terrible monster?
Note: it doesn't have to be super-fast or work on super-huge directory trees.
import glob
from typing import List

# Provide pathname without a trailing /
def find_files_with_extension(pathname: str, extension: str) -> List[str]:
    return list({item for sublist in [glob.iglob(s + "**." + extension) for s in glob.iglob(pathname + "/**/", recursive=True)] for item in sublist})

A few suggestions:

This doesn't need to be a one line function. Writing it on one line makes it much harder to understand and makes finding mistakes more difficult.
Use os.path.join to assemble your full pathname + wildcard + extension. That way, you don't need to worry about whether or not your pathname is given with or without a trailing /
If you're going to worry about a trailing / in your pathname, you should be equally worried about a preceding . in your extension.
glob.glob already returns a list with the matching files. I don't see any need for the complicated, nested list comprehensions you're using. Your function should essentially be a wrapper for calling glob.glob that simply makes combining the pathname + extension a little easier for the user.

That being said, here's how I might re-write this function:
import glob
import os

from typing import List

def find_files_with_extension(path_name: str, extension: str) -> List[str]:
    extension_wildcard = "*"

    if extension[0] != ".": # suggestion #3
        extension_wildcard += "."
    extension_wildcard += extension

    path_name_wildcard = os.path.join(path_name, "**", extension_wildcard) # suggestion #2

    return glob.glob(path_name_wildcard, recursive=True)