split up a string into whitespace-seperated fields I am trying to improve my C skills, and I hope someone might be able to provide me with some feedback on the following code. I avoid strtok function intentionally.

#define MAX_SIZE 1000
int split_up(char* string, char* argv[])
{
    char* p = string;
    int argc = 0;

    while(*p != '\0')
    {
        while(isspace(*p))
            ++p;

        if(*p != '\0')
            argv[argc++] = p;
        else
            break;

        while(*p != '\0' && !isspace(*p))
            p++;
        if(*p != '\0')
        {
            *p = '\0';
            ++p;
        }

    }

    return argc;
}

int main()
{
    char *av[MAX_SIZE];
    char string[] = "this is a test";
    //int i, ac = makeargv(string, av, 2);
    int i;
    int ac = split_up(string, av);
    printf("The number of token is: %d\n", ac);
    for(i = 0; i < ac; i++)
        printf("\"%s\"\n", av[i]);

    return 0;
}

Your code looks quite nice, but here are a few nitpicking comments, for what they are worth.

Missing headers, but I assume you just didn't paste them.
split_up should be static and get a better name
variable names are odd. argc/argv are the usual names for main() (missing here) but not for functions. I'd use tokens instead of argv, and maybe n instead of argc. Okay, n is not at all descriptive, but for such a short function it need not be.
assigning string to p gains nothing, as the original value is not required later on.
perhaps use braces, even for 1-line statements following while, for, if etc
I prefer to leave a space after keywords (while, if, etc)
when breaking out of the loop, why not do it immediately:

if (*p == '\0') {
    break;
}
argv[argc++] = p;

The while loop following this knows that *p is initially not \0, so put the check for that 2nd. Also it is better to add brackets round conditions, even if you think the precedence is obvious:

while(!isspace(*p) && (*p != '\0')) {
    p++;
}

you could also just check for *p instead of *p == '\0'.  But many will disagree.
you also have two blank lines near the end of split_up that make the code look a little messy.

Note also that you don't check for overflow of the argv array.  Okay, you gave it 1000 places for the purpose of the example and that is unlikely to overflow. But in practice you will use somewhat less than 1000 and overflow will be more likely.

And finally, my preference is not to nest loops. Your two nested while loops could be extracted into inline functions - they might be useful in themselves, after all. Here is an example with the loops extracted; you don't have to like it - it is just an example. I find this easier to read, but yours might well be slightly quicker, if that matters.

static inline char* next_token(char *p)
{
    while (isspace(*p)) {
        ++p;
    }
    return *p ? p : NULL;
}

static inline char* next_space(char *p)
{
    while (!isspace(*p) && *p) {
        ++p;
    }
    return *p ? p : NULL;
}

static int string_split(char *p, char* token[])
{
    int n = 0;
    while ((p = next_token(p)) != NULL) {
        token[n++] = p;
        if ((p = next_space(p)) == NULL) {
            break;
        }
        *p++ = '\0';
    }
    return n;
}

Or you could consider using strspn and strcspn and define your own whitespace characters...


All balanced parentheses for a given N Write a function (in Haskell) which makes a list of strings
  representing all of the ways you can balance N pairs of parentheses:

  Example:

balancedParens 0
[""]
balancedParens 1
["()"]
balancedParens 2
["()()","(())"]
balancedParens 3
["()()()","(())()","()(())","(()())","((()))"]

My code produces the right answer but is very slow.

I would appreciate any help on how to speed it up (I have little Haskell experience, so the main struggle for me was implementing the proper algorithm).

module Balanced.Parens where

import Data.List
import Data.List.Split

mutateBP :: String -> [String]
mutateBP "()" = ["(())", "()()"]
mutateBP  xs  = [xs ++ "()", "()" ++ xs, "(" ++ xs ++ ")"]

continuousSubSeqs :: String -> [String]
continuousSubSeqs = filter (not . null) . concatMap inits . tails

indicesOfSubStr :: String -> String -> [Int]
indicesOfSubStr []  _   = []
indicesOfSubStr sub str = filter (\i -> sub `isPrefixOf` drop i str) $ head sub `elemIndices` str

splitBP :: String -> String -> [[String]]
splitBP s xs = chunksOf 2 (concat (map (\i -> [take (i + 1) xs, drop (i + 1) xs]) (indicesOfSubStr s xs)))

get1st :: (a,b,c) -> a
get1st (a,_,_) = a

get2st :: (a,b,c) -> b
get2st (_,b,_) = b

get3st :: (a,b,c) -> c
get3st (_,_,c) = c

zipWithNums' :: String -> [(Char, Int, Int)]
zipWithNums' xs = zip3 xs (parensNum xs) [0..]

findMatch "" = (0, "", "", 0)
findMatch xs = (length xs, group, tail', matchIdx)
  where zipWithNums = zip3 xs (parensNum xs) [0..]
        matchNum = 1 + (get2st $ head zipWithNums)
        matchPar = if '(' == (get1st . head) zipWithNums then ')' else '('
        matchAndTail = dropWhile (\x -> (get1st x /= matchPar) || (get2st x /= matchNum) ) (tail zipWithNums)
        matchIdx = if 0 == length matchAndTail then 0 else (get3st $ head matchAndTail) + 1
        group = take matchIdx xs
        tail' = drop matchIdx xs

groups xs = nub $ filter (\x -> (length $ get2st x) /= 0) [(take (xs_l - l) xs, g, t) | (l, g, t, idx) <- map findMatch (tails xs)]
  where xs_l = length xs

parensNum :: String -> [Int]
parensNum xs = scanl (\acc x -> if '(' == x then acc + 1 else acc - 1) 1 xs

isValidBP :: String -> Bool
isValidBP "" = True
isValidBP xs = (length valid_parens_num) - 1 == length xs
  where parens_num = parensNum xs
        valid_parens_num = takeWhile (>0) parens_num

balancedParens :: Int -> [String]
balancedParens 0 = [""]
balancedParens 1 = ["()"]
balancedParens n = nub $ concat $ map (\x -> [p ++ z ++ s| p <- [get1st x], z <- mutateBP $ get2st x, s <- [get3st x]]) gr
  where gr = concat $ map groups (balancedParens (n - 1))

The code can be sped up, and it can also be simplified. For n > 0, there is a unique way to write a balanced string as (a)b where a and b are balanced strings.

a and b must have n-1 pairs of parentheses, total. This leads us to the following solution:

balancedParens 0 = [""]
balancedParens n = ["(" ++ a ++ ")" ++ b | i <- [0 .. n - 1], a <- balancedParens i, b <- balancedParens (n - i - 1)]


Simple PacketHandler class I can recently coded an emulation system and in this project I have a PacketHandler for each player connected to a socket server, and each player receives packets (data) from the client application.

Can someone please let me know if this code can be improved in any way? I will post the code below for the PlayerPacketHandler class, if you need any other classes that are used in this class let me know and I can add them.

Thanks.

How is ProcessPacketData called?

private void IncomingPacket(IAsyncResult iAr)
{
    try
    {
        var bytesReceived = _playerSocket.EndReceive(iAr);

        if (bytesReceived == 0)
        {
            Dispose();
            return;
        }

        var packet = new byte[bytesReceived];
        Array.Copy(_buffer, packet, bytesReceived);
        _playerPacketHandler.ProcessPacketData(packet);
    }
    catch (Exception)
    {
        Dispose();
    }
    finally
    {
        try
        {
            _playerSocket.BeginReceive(_buffer, 0, _buffer.Length, SocketFlags.None, IncomingPacket, _playerSocket);
        }
        catch
        {
            Dispose();
        }
    }
}

The class I am asking to be reviewed:

using System;
using System.IO;
using Sahara.Core.Logging;

namespace Sahara.Base.Game.Players
{
    internal class PlayerPacketHandler
    {
        private readonly Player _player;
        private readonly LogManager _logManager;
        private bool _decryptedData;
        private byte[] _dataReceived;
        private bool _dataCompleted;

        public PlayerPacketHandler(Player player)
        {
            _player = player;
            _logManager = Sahara.GetServer().GetLogManager();
        }

        public void ProcessPacketData(byte[] data)
        {
            try
            {
                if (_player.RC4Client != null && !_decryptedData)
                {
                    _player.RC4Client.Decrypt(ref data);
                    _decryptedData = true;
                }

                if (_dataCompleted)
                {
                    byte[] fullDataRcv = new byte[_dataReceived.Length + data.Length];
                    Buffer.BlockCopy(_dataReceived, 0, fullDataRcv, 0, _dataReceived.Length);
                    Buffer.BlockCopy(data, 0, fullDataRcv, _dataReceived.Length, data.Length);

                    ProcessPacketData(fullDataRcv);
                    _dataCompleted = false;

                    return;
                }

                using (var reader = new BinaryReader(new MemoryStream(data)))
                {
                    if (data.Length < 4)
                        return;

                    int messageLength = HabboEncoding.DecodeInt32(reader.ReadBytes(4));

                    if ((reader.BaseStream.Length - 4) < messageLength)
                    {
                        _dataReceived = data;
                        _dataCompleted = true;
                        return;
                    }

                    if (messageLength < 0 || messageLength > 5120)
                        return;

                    var packet = reader.ReadBytes(messageLength);

                    using (var binaryReader = new BinaryReader(new MemoryStream(packet)))
                    {
                        int packetHeader = HabboEncoding.DecodeInt16(binaryReader.ReadBytes(2));

                        byte[] Content = new byte[packet.Length - 2];
                        Buffer.BlockCopy(packet, 2, Content, 0, packet.Length - 2);

                        IncomingPacket clientPacket = new IncomingPacket(packetHeader, Content);
                        onNewPacket.Invoke(clientPacket);

                        _decryptedData = false;
                    }
                }
            }
            catch (Exception exception)
            {
                var method = System.Reflection.MethodBase.GetCurrentMethod().Name;
                _logManager.Log("Error in " + method + ": " + exception.Message, LogType.Error);
                _logManager.Log(exception.StackTrace, LogType.Error);
            }
        }
    }
}

My two cents : MemoryStream is a disposable object (inherited from Stream)

using (var reader = new BinaryReader(new MemoryStream(data)))

should grant you with a warning CA2000.

You may check this point with something like

Stream stream = null ;
try
{
   Stream = new MemoryStream(data) ;
   using (var reader = new BinaryReader(stream))
...
}
finally
{
    if (stream != null)
    {
        stream.Dispose() ;
    }
}

This should ensure you are freeing the stream in time.

Hope this helps !


Validating CSV headers against an expected list My application (legacy code) reads a CSV input file with an expected format with regards to Column names as follows:

1st column - Should say "Marker"
2nd column - Should say "Category"
3rd column ... to ... N columns - These are the actual data columns. Should have number starting with 1 and increasing by 1 going right.

In other words, 3rd column should have "1", 4th column header should have "2".

Example CSV file with 10 data columns (total of 12 columns in file):

Marker,Category,1,2,3,4,5,6,7,8,9,10
TODD,Sea,0.001062344,0.001931534,0.002334951,0.002738369,0.003141786,0.003545203,0.004788256,0.005917489,0.007025671,0.008078547
JAMIE,Fork,0.00133847,0.002433581,0.002941854,0.003450128,0.003958402,0.004466676,0.006032825,0.007455569,0.00885179,0.010178331
LENNY,PITCH,0.001686365,0.003066119,0.003706504,0.004346889,0.004987274,0.005627659,0.007600883,0.009393428,0.011152557,0.012823893

My code that does this validation (and it works) is as follows:

fileStream = new FileStream(delimitedFileName, FileMode.Open,
   FileAccess.Read, FileShare.Read);
csvStreamReader = new StreamReader(fileStream);
string headerLine = csvStreamReader.ReadLine();
var headers = headerLine.Split(delimiter, '\\');

if (!string.Equals(headers[0], "Marker", StringComparison.InvariantCultureIgnoreCase)
|| (!string.Equals(headers[1], "Category", StringComparison.InvariantCultureIgnoreCase)))
{
    throw new Exception("Invalid file format. Please use template (MarkerTemplate.csv).");
}

for (int i = 2; i < headers.Length; i++)
{
    if (!string.Equals(headers[i], (i-1).ToString(),
        StringComparison.InvariantCultureIgnoreCase))
    {
    throw new Exception("Invalid file format. Please use template (MarkerTemplate.csv).");
    }
}

I want to avoid doing it like this, in 2 steps.

Is there a way to do some sort of delegate or Action to do the 2nd section, i.e. validate that the data column names are correct, for a variable number of data columns?

I want to avoid doing it in 2 separate sections as shown above, and have duplicate throw new Exception code for essentially the same exception.

Move the data column validation into a function

bool invalidDataColumns(string[] headers) {
    int[] numbers = headers.Skip(2).Select(s => int.TryParse(s, out var n) ? n : 0).ToArray();
    return !(numbers[0] == 1 && numbers.Zip(numbers.Skip(1), (previous, current) => current - previous).All(diff => diff == 1));
}

And call it within the if condition.

//...

if (!string.Equals(headers[0], "Marker", StringComparison.InvariantCultureIgnoreCase)
    || (!string.Equals(headers[1], "Category", StringComparison.InvariantCultureIgnoreCase))
    || invalidDataColumns(headers)
    ) {
    throw new Exception("Invalid file format. Please use template (MarkerTemplate.csv).");
}

//...

The condition could be reduced further by moving the entire thing into its own function as well

bool invalidHeaders(string[] headers) {
    return !string.Equals(headers[0], "Marker", StringComparison.InvariantCultureIgnoreCase)
        || (!string.Equals(headers[1], "Category", StringComparison.InvariantCultureIgnoreCase))
        || invalidDataColumns(headers);
}

which now simplifies the code to

//...

if (invalidHeaders(headers)) {
    throw new Exception("Invalid file format. Please use template (MarkerTemplate.csv).");
}

//...

Unit test used to verify expected behavior

[TestClass]
public class InvalidDataColumnsTests {
    [TestMethod]
    [DataRow("Marker,Category,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15", false)]
    [DataRow("Marker,Category,1,2,3,4,5,6,7,8,9,10", false)]
    [DataRow("Marker,Category,1,2,3,4,5", false)]
    [DataRow("Marker,Category,x,2,3,4,5,6,7,8,9,10", true)]
    [DataRow("Category,1,2,3,4,5", true)]
    public void Should_Validate_Columns(string headerLine, bool expected) {
        //string headerLine = "Marker,Category,1,2,3,4,5,6,7,8,9,10";
        string[] headers = headerLine.Split(new[] { "," }, StringSplitOptions.RemoveEmptyEntries);
        invalidHeaders(headers).Should().Be(expected);

        ////Example Use case
        //if (invalidHeaders(headers)) {
        //    throw new Exception("Invalid file format. Please use template (MarkerTemplate.csv).");
        //}
    }

    bool invalidHeaders(string[] headers) {
        return !string.Equals(headers[0], "Marker", StringComparison.InvariantCultureIgnoreCase)
            || (!string.Equals(headers[1], "Category", StringComparison.InvariantCultureIgnoreCase))
            || invalidDataColumns(headers);
    }

    bool invalidDataColumns(string[] headers) {
        int[] numbers = headers.Skip(2).Select(s => int.TryParse(s, out var n) ? n : 0).ToArray();
        return !(numbers[0] == 1 && numbers.Zip(numbers.Skip(1), (previous, current) => current - previous).All(diff => diff == 1));
    }
}


Which of these two is better in terms of readability and function? I have two different ways of populating a List with querying another List. They are:

var list = _surveyList.Where(x => x.Date == selectedDate).Select(s => s.Joint).ToList();

And:

var list = from s in _surveyList
           where s.Date.Equals(selectedDate)
           select s.Joint;

They functionally do the same thing. My question is which is better in terms of readability, and function? Does one have more overhead than the other? Is one way more preferred than the other?

These aren't completely the same.  The first will return a List<T> and the second will return an IEnumerable<T>.

You would need to make this change in order for these to be equivalent.

var list = (from s in _surveyList
            where s.Date.Equals(selectedDate)
            select s.Joint).ToList();

But to answer your question it really comes down to personal preference.  What do you feel more comfortable with?  What has your team agreed on?

There are cases where you have to use Method Syntax over Query Syntax.  And there are some cases when one will be more readable then others.  You will have to make that determination yourself.

Check out this question over on Stack Overflow.

https://stackoverflow.com/questions/8037677/linq-query-syntax-vs-method-chains-lambda

That question was ultimately closed as being not constructive (opinion related) as I'm sure this one will be also.


Replacing values in json file with jq I have a JSON file like

{
    "Changes": [{
        "Action": "UPSERT",
        "ResourceRecordSet": {
            "Name": "RESOURCE_NAME",
            "Type": "A",
            "TTL": 60,
            "ResourceRecords": [{
                "Value": "RESOURCE_HOSTS"
            }]
        }
    }]
}

where I need to replace .Changes[].ResourceRecordSet.Name and .Changes[].ResourceRecordSet.ResourceRecords[] dynamically.

So, I wrote a script to replace required values with jq.

#!/bin/bash -xe

RESOURCE_NAME="example.com"
RESOURCE_HOSTS="1.1.1.1,2.2.2.2,"

cat resource-changes.json | \
jq '.Changes[].ResourceRecordSet.Name=env.RESOURCE_NAME' | \
jq 'del(.Changes[].ResourceRecordSet.ResourceRecords[])' \
> resource-changes.$$.json && \
mv resource-changes.$$.json resource-changes.json

for host in $(echo "$RESOURCE_HOSTS" | tr "," "\n"); do
  cat resource-changes.json | \
  jq --arg theHost $host '.Changes[].ResourceRecordSet.ResourceRecords |= (. + [{"Value":$theHost}])' \
  > resource-changes.$$.json
  mv resource-changes.$$.json resource-changes.json
done

jq '.' resource-changes.json

Which works fine and I can get required replacements. Though the script looks quite long and complicated IMO. Also, I do not really like joggling with temporary files. I'm looking for a way to simplify the script. Appreciate any comments!

Accessing environment variables in jq

This doesn't really work as it is...:

RESOURCE_NAME="example.com"
# ...

cat resource-changes.json | \
jq '.Changes[].ResourceRecordSet.Name=env.RESOURCE_NAME' | \
# ...

jq cannot access RESOURCE_NAME if it's not exported.
The fix is simple:

export RESOURCE_NAME="example.com"

Iterating over a list of values

This is a very hacky way to iterate over a list of hosts:

RESOURCE_HOSTS="1.1.1.1,2.2.2.2,"

# ...

for host in $(echo "$RESOURCE_HOSTS" | tr "," "\n"); do

If the host names will never contain space,
you could use a simple space-separated string:

RESOURCE_HOSTS="1.1.1.1 2.2.2.2"

# ...

for host in $RESOURCE_HOSTS; do

Or for maximum flexibility you could use a proper Bash array:

RESOURCE_HOSTS=(1.1.1.1 2.2.2.2)

# ...

for host in "${RESOURCE_HOSTS[@]}"; do

Streamlining by eliminating the loop

The current script creates many temporary files in the process,
which is very ugly.
The biggest obstacle to streamlining this into a single nice pipeline is the looping logic.
You could prepare the host values into a JSON list and let jq take care of the rest.

Here's one way to create a JSON list from an array of hosts:

hosts=(1.1.1.1 2.2.2.2)

values="\"${hosts[0]}\""
for host in "${hosts[@]:1}"; do
    values="$values,\"$host\""
done

This assumes that there is at least one host.
It initializes values with the first one,
and then appends the others with a comma,
adding double-quotes to make it valid JSON.

With this new variable,
you can chain the jq calls like this:

cat resource-changes.json | \
jq '.Changes[].ResourceRecordSet.Name=env.RESOURCE_NAME' | \
jq 'del(.Changes[].ResourceRecordSet.ResourceRecords[])' | \
jq ".Changes[].ResourceRecordSet.ResourceRecords = ([$values] | map({\"Value\":.}))"

The cat command at the front is a bit ugly, because the filename could be a parameter of the first jq call, or replaced with an input redirection.
But that would break the current nice aesthetic flow of the jq calls.
This minor issue could be fixed by grouping and input redirection:

{
jq '.Changes[].ResourceRecordSet.Name=env.RESOURCE_NAME' | \
jq 'del(.Changes[].ResourceRecordSet.ResourceRecords[])' | \
jq ".Changes[].ResourceRecordSet.ResourceRecords = ([$values] | map({\"Value\":.}))"
} < resource-changes.json


Striping a matrix concentrically, anticlockwise Given a matrix you shall stripe the matrix, such that every number that has equal distance to the matrix border will be filled into an array.

For examples see code comments.

typedef int matrixOptimizationValue;
/*
Striping a matrix by packing numbers who have the same distance from matrix border.
Note that the packing is anti-clockwise.
 E.g:

 Example 1:
 ___      ___
 . 1 1 1 1  .         Field:
 . 2 2 2 2  .          [1,2,3,3,3,3,2,1,1,1]
 . 3 3 3 3  .  -->     [2,2]
 ___      ___

 Example 2:
 ___      ___         Field:
 . 1 1 1 4 1  .        [1,2,3,1,2,4,5,6,3,2,1,4,1,1,1]
 . 2 2 2 3 2  .        [2,3,3,3,2,3,2,3,2,2]
 . 3 3 3 2 3  .  -->
 . 1 2 4 5 6  .
 ___      ___

 returns:
 return_value      - Field of Arrays; unpacked Matrix
 arr_lengths       - Since fields arrays have a variable size, "*array_lengths" contains the following information.
                     {Amount of arrays in the field, sizes_of_array01, sizes_of_array03 ... }
                     Therefore "*array_lengths" size must be *array_lengths[0]+1.

 in:
 const int* matrix - Matrix to be unpacked
 int rows          - rows of the matrix
 int columns       - columns of the matrix

 Possible Errors:
    1. Matrix has no measurments.

 */
matrixOptimizationValue** strip(const matrixOptimizationValue* matrix, const int rows, const int columns, int** arr_lengths){
    //Checking if Matrix has measurments.
    int* lengths = *arr_lengths;
    if(rows == 0 || columns == 0){
        printf("\nMatrix has no measurments. Error. \n");
        exit(0);
    }

    //Calculating maximal distance of an element to borders.
    const int max_dis = (columns > rows) ? round((double)(rows)/2) : round((double)(columns)/2);
    //Creating future return_value
    matrixOptimizationValue** unstriped_matrix = malloc(max_dis*sizeof(int*));
    (lengths) = malloc((max_dis+1) * sizeof(matrixOptimizationValue));
    (lengths)[0] = max_dis;
    //Packing...
    int x_minborder = 0; //Borders
    int y_minborder = 0;
    int x_maxborder = columns;
    int y_maxborder = rows;

    for(int i = 0; i < max_dis; i++){
        int elements_size=0; //Unfortunately, making "const" causes really bad syntax...
        if(x_maxborder-x_minborder <= 1 || y_maxborder-y_minborder <=1){
            if(x_maxborder-x_minborder <= 1 && y_maxborder-y_minborder <=1){
                elements_size = 1;
            } else {
                elements_size = (x_maxborder-x_minborder == 1) ?  y_maxborder-y_minborder : x_maxborder-x_minborder;
            }
        } else {
            elements_size = 2*(x_maxborder-x_minborder + y_maxborder-y_minborder)-4;
        }
        matrixOptimizationValue* array = malloc(elements_size * sizeof(int));
        (lengths)[i+1]=elements_size;
        //Initializing coordinates
        int x = i; //It is certain, that this coordinates will always belong to fields array_i;
        int y = i;

        int dir_x = 0; //Direction
        int dir_y = 1;

        for(int j = 0; j < elements_size; j++){
            array[j] = matrix[y*columns + x];
            if(x+dir_x >= x_maxborder || x+dir_x < x_minborder || y+dir_y >= y_maxborder || y+dir_y < y_minborder){
                /*Rotate Direction-vector using Algebra
                         x
                         y
                 0  1    y
                -1  0   -x
                 */
                const int clone = dir_x;
                dir_x = dir_y;
                dir_y = -clone;
            }
            x += dir_x;
            y += dir_y;
        }
        x_minborder++;
        y_minborder++;
        x_maxborder--;
        y_maxborder--;
        unstriped_matrix[i]=array;
    }
    return unstriped_matrix;
}

How would you improve the syntax?
Assume high-performance needs, how would you improve the code?

Here are some things that may help you improve your code.

Fix the bug

The code allocates memory for the passed arr_lengths but never returns a pointer to the newly allocated memory to the caller.  The code currently has this:

int* lengths = *arr_lengths;
// more code
(lengths) = malloc((max_dis+1) * sizeof(matrixOptimizationValue));

Instead it should be this:

// more code
*arr_lengths = malloc((max_dis+1) * sizeof(matrixOptimizationValue));
int* lengths = *arr_lengths;

Provide complete code to reviewers

This is not so much a change to the code as a change in how you present it to other people.  Without the full context of the code and an example of how to use it, it takes more effort for other people to understand your code.  This affects not only code reviews, but also maintenance of the code in the future, by you or by others. The code comments are good, but a sample program would be better.

Use the required #includes

The code uses printf which means that it should #include <stdio.h>.  It was not difficult to infer, but it helps reviewers if the code is complete.  I had to add these three lines:

#include <stdio.h>
#include <math.h>
#include <stdlib.h>

Avoid the use of floating point math

On many computers, floating point mathematics is slower than integer math.  For this reason, it's often better to avoid if you can.  In this case, it's easily done.  We can replace this:

const int max_dis = (columns > rows) ? round((double)(rows)/2) : round((double)(columns)/2);

with this:

const int max_dis = (rows < cols ? rows+1 : cols+1) / 2;

I also changed the variable columns to cols but that's a personal preference.  The longer name is arguably more descriptive.

Check return values for errors

The calls to malloc can fail.  You must check the return values to make sure they haven't or your program may crash (or worse) when given malformed input or due to low system resources.  Rigorous error handling is the difference between mostly working versus bug-free software.  You should strive for the latter.

Choose descriptive names

Most of the variables and function are well-named and clear in their usage and intent, but I found matrixOptimizationValue both somewhat misleading (it's not a value, but rather a type) and also overly long.  I renamed it DataType which is also not a brilliant name, but is, at least, shorter.

Minimize system calls

If you're looking for performance, it's often a good idea to avoid system calls, such as for malloc.  In this case, I'd suggest changing the code to do only one single allocation.  This has the benefits of speed, simplification of error checking and handling, and simplification of cleanup after the call (only one call to free is required).

Consider separating I/O from the algorithm

The strip function uses printf to print to the console and then exits via exit if the input is malformed.  Consider changing that to return NULL on error and letting the caller figure out what to do.

Simplify the algorithm

I found the length of the code and many variables something of an impediment to my understanding of the algorithm.  It can be simplified considerably by remembering that the passed variables may also be used directly.  In the rewrite I created, pointers are used extensively to simplify the code.  It also uses the fact that we can calculate in advance when to change direction.

Results

Following all of these suggestions, I created the following rewrite:

#include <stdlib.h>
#include <stdio.h>
#include <stddef.h>

typedef unsigned short DataType;
#define DataTypeStr "u"

DataType* strip(const DataType* matrix, int rows, int cols, int** arr_lengths){
    if (rows == 0 || cols == 0) {
        return NULL;
    }
    const int max_dis = (rows < cols ? rows+1 : cols+1) / 2;
    // return value is a single vector of the same size as
    // the input, followed by an int value n, which is the
    // count of arrays represented, followed by n ints which
    // represent the lengths of those sub-arrays.
    DataType *retval = malloc(rows * cols * sizeof(DataType*)
            + (1 + max_dis) * sizeof(int));
    if (retval == NULL) {  // bail out on allocation error
        return NULL;
    }
    *arr_lengths = (int *)(&retval[rows*cols]);
    DataType *out = retval;
    int *len = *arr_lengths;
    *len++ = max_dis;
    // precalculate down, right, up, left
    enum compass { DOWN, RIGHT, UP, LEFT, DIRCOUNT };
    const int dirs[DIRCOUNT] = { cols, +1, -cols, -1 };
    // because we move and then fetch a value,
    // move one row above matrix
    matrix += dirs[UP];

    for (int matnum=0; matnum < max_dis; ++matnum) {
        if (rows > 1 && cols > 1) {
            *len++ = 2 * (rows + cols - 2);
        } else {
            *len++ = rows + cols - 1;
        }
        // start by moving down
        int dir = DOWN;
        while (rows && cols && dir < DIRCOUNT) {
            // down and up
            for (int i = rows; i; --i) {
                matrix += dirs[dir];
                *out++ = *matrix;
            }
            --rows;
            --cols;
            ++dir;
            // right and left
            for (int i = cols; i; --i) {
                matrix += dirs[dir];
                *out++ = *matrix;
            }
            ++dir;
        }
    }
    return retval;
}

int main(int argc, char *argv[]) {
    if (argc != 3) {
        puts("Usage: matstripe rows cols\n");
    }
    int rows = atoi(argv[1]);
    int columns = atoi(argv[2]);
    printf("Input is a %d x %d matrix\n", rows, columns);
    DataType *input = malloc(rows * columns * sizeof(DataType));
    if (input == NULL) {
        puts("Out of memory error, quitting program.");
        return 1;
    }
    int colcounter = columns;
    for (int i = 0; i < rows*columns; ++i) {
        input[i] = i+1;
        printf("%3" DataTypeStr " ", input[i]);
        if (--colcounter == 0) {
            printf("\n");
            colcounter = columns;
        }
    }
    int *arr_lengths;
    DataType* answer = strip(input, rows, columns, &arr_lengths);
    free(input);
    puts("Output:");
    if (answer) {
        DataType* mat = answer;
        for (int matnum = 0; matnum < *arr_lengths; ++matnum) {
            printf("[%" DataTypeStr, *mat++);
            for (int i = arr_lengths[matnum+1] - 1; i; --i) {
                printf(",%" DataTypeStr, *mat++);
            }
            puts("]");
        }
        free(answer);
    }
}

This takes two command line parameters for the nmber of rows and number of columns  and constructs a test input matrix of that size filled with cardinal numbers.  Here's an example of output.

Input is a 5 x 4 matrix
  1   2   3   4
  5   6   7   8
  9  10  11  12
 13  14  15  16
 17  18  19  20
Output:
[1,5,9,13,17,18,19,20,16,12,8,4,3,2]
[6,10,14,15,11,7]


Is this P/Invoke code standard? I have two projects, a native C++ DLL (pinvokelib.dll), and a C# console application (pinvoketest.exe) which calls the DLL. I'm curious if the way I do P/Invoke is considered best practice.

Here is my C++ code:

extern "C" __declspec(dllexport) int test(int val);

int test(int val)
{
    return val * 2;
}

And my C#:

using System;
using System.Runtime.InteropServices;

namespace pinvoketest
{
    class Program
    {
        static void Main(string[] args)
        {
            Console.WriteLine(test(42));
            Console.ReadLine();
        }

        [DllImport("pinvokelib.dll", CallingConvention=CallingConvention.Cdecl)]
        static extern int test(int val);
    }
}

I'm using Visual Studio 2012 Express to compile both.

Is there any way that the code can be improved? Is there something redundant? I'm especially unsure about the first line of the C++ code, since I don't understand everything about it very well.

If you don't call your function recursively from C, you don't need to have separate defition and declaration:

extern "C" __declspec(dllexport) int test(int val)
{
    return val * 2;
}

And if you have more than one function you want to export, you can use extern "C" as a block:

extern "C"
{
    __declspec(dllexport) int test(int val)
    {
        return val * 2;
    }

    // more functions here
}

As to what does it all mean:

extern "C" tells the compiler not to mangle the name of the function; this is normally necessary to support C++ function overloading
__declspec(dllexport) means that this function will be exported from the DLL, so that you can use it from outside this library


Random 5-character token generator I have an Android app that uses randomly generated tokens to log in.  They're 5 characters long, are sent to a user's email address, and have a short lifespan, so I don't think I need anything that's extremely secure.

I'm getting odd result that suggest I'm not really generating random numbers.  That, or I'm seeing patterns that don't really exist.

For example, the last few tokens generated were:

0pqrH
UMOPQ
d789r
jmnde

These have patterns:

pqr
789
all caps
all lowercase

There are some tokens generated with scattered characters:

f80!#
NLog#
Z6kpJ

It just seems that there are too many patterns to be coincidence.

static public String getToken(int chars) {
    String CharSet = "abcdefghijkmnopqrstuvwxyzABCDEFGHJKLMNOPQRSTUVWXYZ234567890!@#$";
    String Token = "";
    for (int a = 1; a <= chars; a++) {
        Token += CharSet.charAt(new Random().nextInt(CharSet.length()));
    }
    return Token;
}

Am I generating random numbers correctly? Should I be picking a seed? Is this in my imagination?

This is not entirely only in your imagination

Random objects are meant to be reused. Recreating a Random object each time may cause a specific pattern in the result. This depends a bit on the Java version. There is no need to pick a seed, Java picks a seed for you based on a varying number, and the current system time (see Java 6 source). Newer versions of Java uses a better seedUniquifier method. I am not sure about what implementation the Android platform uses.

Additionally, Java variable names should start with lowercase letters by the coding conventions.

I would also use a StringBuilder for better efficiency and lower memory usage.

To avoid confusion with the Charset class, I would name your String variable chars and your int variable length.

private static final Random random = new Random();
private static final String CHARS = "abcdefghijkmnopqrstuvwxyzABCDEFGHJKLMNOPQRSTUVWXYZ234567890!@#$";

public static String getToken(int length) {
    StringBuilder token = new StringBuilder(length);
    for (int i = 0; i < length; i++) {
        token.append(CHARS.charAt(random.nextInt(CHARS.length())));
    }
    return token.toString();
}


Scraping data from tables connected to each dot on a map I've written a script in python with selenium to scrape different table data lie within different dots on a map in a certain website. Each table is connected to each dot. The table is activated once either of the dots is clicked. However, my script is able to open that webpage, traverse the map, click each dot to activate each table and finally parse the data of each table available on that map. Any input on this to make it more robust  will be highly appreciated.

Here is what I've written to do the whole thing:

import time
from selenium import webdriver
from selenium.webdriver.common.by import By
from selenium.webdriver.support.ui import WebDriverWait
from selenium.webdriver.support import expected_conditions as EC
from selenium.webdriver.common.action_chains import ActionChains

driver = webdriver.Chrome()
driver.get("https://acwi.gov/monitoring/vm/programs/vm_map.html")
wait = WebDriverWait(driver, 10)
wait.until(EC.presence_of_element_located((By.TAG_NAME, 'iframe')))

#Using iframe link to get to the map

driver.get(driver.find_element_by_tag_name("iframe").get_attribute("src"))
wait = WebDriverWait(driver, 10)
wait.until(EC.presence_of_element_located((By.CSS_SELECTOR, "div#mapDiv_zoom_slider")))

#Zooming in for the browser to locate elements properly

driver.find_element_by_css_selector("div#mapDiv_zoom_slider").click()

#Hardcoded time to wait until certain moment to avoid stale element error

time.sleep(5)

#Finding each tag to click to get the table

for item in driver.find_elements_by_tag_name("circle"):
    ActionChains(driver).move_to_element(item).click().perform()

    #Go for the next item to click

    elem = driver.find_element_by_css_selector(".titleButton.next")
    if elem.is_displayed():
        elem.click()
        time.sleep(5)

        #Finding table element

        items = wait.until(EC.presence_of_element_located((By.CSS_SELECTOR, "table.attrTable")))
        wait.until(EC.presence_of_element_located((By.CSS_SELECTOR, "tr")))
        list_of_data = [[item.text for item in data.find_elements_by_css_selector('td')]
                    for data in items.find_elements_by_css_selector('tr')]
        for tab_data in list_of_data:
            print(tab_data)

driver.quit()

Here is a link to an image to give the clarity of my above description:
https://www.dropbox.com/s/axd66fvozexbefp/Untitled.jpg?dl=0

Some key things to take into account when using selenium:

if you have an id of an element - use it - it's the fastest way to locate an element
wait.until() returns a WebElement with most of the Expected Conditions - there is no need to find the element again if you want to use the element you've been waiting for
WebDriverWait() can be instantiated once per driver and reused
time.sleep() waits have to be avoided - they are making the code slower than needed most of the time, they are unreliable and not tied to any condition on a page, and sometimes the delay you set is not enough

You don't have to click through every circle on the map
We can seriously improve on web-scraping speed by extracting the "token" (let's call it this way) from the iframe URL and then making a REST API request to a specific endpoint with the data using requests:
from urllib.parse import parse_qs, urlparse

import requests
from selenium import webdriver
from selenium.webdriver.common.by import By
from selenium.webdriver.support.ui import WebDriverWait
from selenium.webdriver.support import expected_conditions as EC

driver = webdriver.Chrome()
driver.get("https://acwi.gov/monitoring/vm/programs/vm_map.html")

wait = WebDriverWait(driver, 10)

# get the token from the frame src
frame = wait.until(EC.presence_of_element_located((By.TAG_NAME, 'iframe')))
token = parse_qs(urlparse(frame.get_attribute("src")).query)['webmap'][0]

driver.close()

# parse data
response = requests.get("http://www.arcgis.com/sharing/rest/content/items/{token}/data?f=json".format(token=token))
data = response.json()

for item in data["operationalLayers"][0]["featureCollection"]["layers"][0]["featureSet"]["features"]:
    coordinates = item["geometry"]["x"], item["geometry"]["y"]
    attributes = item["attributes"]

    print(coordinates, attributes["MPN"])


Finding the closest value and matching it with the correct item in the list I have list values with their IDs like this:

ID   Value
A    1.2
B    4.2
C    7.2
D    10.2

In C#, I want to find the ID with the closest value to a given value say 3.9. In this example, B will be the answer as it has the value(4.2) closest to the given value.

My solution to this problem involved following steps:

Put the values in an list of objects (where each object has a property ID and a value) or a dictionary with string ID and decimal value
Loop through the list or dictionary and find the minimum value
If the value is found or their difference is minimum, print the item ID (in this case B)

I want to know if there is better solution in terms of time complexity as here it seems to be \$O(n)\$.

public class SampleClass {
        public string ID { get; set; }
        public double Value { get; set; }
        public SampleClass(string _id, double _value){
        this.ID = _id;
        this.Value = _value;
        }
        public static void Main(string[] args){
            SampleClass input1 = new SampleClass("A",1.20);
            SampleClass input2 = new SampleClass("B", 4.20);
            SampleClass input3 = new SampleClass("C", 7.20);
            SampleClass input4 = new SampleClass("D", 10.20);
            double givenValue = 9.9;
            List<SampleClass> sampleClassList = new List<SampleClass>();
            sampleClassList.Add(input1);
            sampleClassList.Add(input2);
            sampleClassList.Add(input3);
            sampleClassList.Add(input4);
            double cVal = double.MaxValue;
            string cID = string.Empty;
            double diff = 0;
            foreach (SampleClass item in sampleClassList){
              if (item.Value == givenValue){
                  Console.WriteLine(item.ID);
                  return;
              }
             diff = Math.Abs(item.Value - givenValue);
             if (diff < cVal) {
                 cVal = diff;
                 cID = item.ID;
             }
           }
            Console.WriteLine(cID);
        }
    }

I am aware if there are list of values which is sorted, then you can find the closest value through binary search on this sorted list and it will give you a more efficient time complexity.

But in my case, after finding the closest value, I have to match this value to its ID. Also, these values can be duplicate, so I cannot use them as key in dictionary.

Is there more efficient way of doing this, or am I missing something?

You cannot use a dictionary but you can use the GroupBy and then search for the closest key. The values of the group will be the items with same values.

var values = new[]
{
    new Foo{ Id ="A", Value = 1.2 },
    new Foo{ Id ="B", Value = 4.2 },
    new Foo{ Id ="C", Value = 7.2 },
    new Foo{ Id ="D", Value = 4.2 }
}
.GroupBy(x => x.Value);

where

struct Foo
{
    public string Id;
    public double Value;
}


Determinant of a matrix I'm visiting some code that I wrote for one of my finals projects and wanted to know whether there were a more optimal, more elegant way to do this so it does not look so "hard-coded".

The problem was that I needed to calculate the Determinant of a matrix, and, at the time (some of it was rushed) I did not know of a better way, using loops or re-usable code that could make the code a little better. Here is the code:

namespace Determinant {

    template<int X>
    float determinant(std::vector<Vector> &data)
    {

        float deter = 0.0;
        if(X == 2)
        {
            float a = data[0][0];
            float b = data[1][0];
            float c = data[1][1];

            deter = (a + c) * (a + c) -4 * (a*c-b*b);
        }else if(X == 3)
        {

            float determinant1 = (data[1][1] * data[2][2]) - (data[2][1] * data[1][2]);
            float determinant2 = (data[1][0] * data[2][2]) - (data[2][0] * data[1][2]);
            float determinant3 = (data[1][0] * data[2][1]) - (data[2][0] * data[1][1]);

            deter = (data[0][0] * determinant1) - (data[0][1] * determinant2) + (data[0][2] * determinant3);
        }
        return deter;
    }
}

As you can see the Determinant is very hard-coded which is probably not the right way -- But are there any other alternatives that isn't hard-coded? I want to start to use a design pattern, as I feel one would be useful here but can't seem to figure out which one.

You don't really need a design pattern for determinants, just a better algorithm. Generally one of the easiest (and fastest) ways of calculating a matrix determinant is by using what is known as LU-Decomposition. This factors a matrix into two matrices, a lower triangular and an upper triangular matrix. From these, the determinant can simply be calculated as the product of diagonal elements.

Note that you have to be careful when calculating determinants of large matrices; for a 100x100 matrix, it can easily overflow the maximum size of a float (or double). For this reason it's often better to calculate a log-determinant.

On to the actual code you've presented:

data should be passed by const& since it isn't (and shouldn't be) modified:

float determinant(const std::vector<Vector>& data)

Using a template int parameter to choose between determinant sizes is really odd, and is potentially easily misused. What if I use Determinant::determinant<2>(...) on a 3x3 matrix? It'll give me the wrong answer. You should generally try to make your code easy to use and hard to misuse. In this case, that means calculating the determinant size based on the row/column size of the passed parameter. Better yet would be creating a matrix class to encapsulate all of this information.

These days, it generally doesn't make a lot of sense to use float over double unless you really need the speed (and even then, it is often no faster, and can sometimes even be slower on modern hardware). Stick to using double by default.


Efficiency of grid generated by for-in loop I've spend the last several hours working on a loop designed to take items in an array and arrange them as a so to speak "gird" of UIButtons. As far as I've been able to see through testing this code works perfectly.

However, I am new to creating loops and I was wondering if anyone could give me any pointers on how I could make this code cleaner/more efficient for future coding purposes.

I know my question is very similar to Optimize sorting of Array according to Distance (CLLocation), but I'm really hoping to get some tips on a "per case" basis. Thank you for your time.

- (void)myMethod
{
    NSArray *array = [[NSArray alloc] initWithObjects:@"Item #1", @"Item #2", @"Item #3", @"Item #4", @"Item #5", @"Item #6", @"Item #7", @"Item #8",  nil];

    BOOL leftOkay = NO;
    BOOL centerOkay = NO;
    BOOL rightOkay = NO;
    int left = 0;
    int center = 1;
    int right = 2;
    int yOrigin = 0;
    int spaceBetweenItems = 5;
    UIButton *leftButton;
    UIButton *centerButton;
    UIButton *rightButton;

    for (int i = 0; i < [array count]; i++)
    {
        if (left < [array count]) {
            leftButton = [[UIButton alloc] initWithFrame:CGRectMake(spaceBetweenItems, yOrigin + spaceBetweenItems, 100, 150)];
            [leftButton setTitle:[array objectAtIndex:left] forState:UIControlStateNormal];
            [leftButton setTitleColor:[UIColor redColor] forState:UIControlStateNormal];
            [leftButton setBackgroundColor:[UIColor cyanColor]];
            [myScrollView addSubview:leftButton];
            leftOkay = YES;
        }
        if (center < [array count]) {
            centerButton = [[UIButton alloc] initWithFrame:CGRectMake(110, yOrigin + spaceBetweenItems, 100, 150)];
            [centerButton setTitle:[array objectAtIndex:center] forState:UIControlStateNormal];
            [centerButton setTitleColor:[UIColor blueColor] forState:UIControlStateNormal];
            [centerButton setBackgroundColor:[UIColor yellowColor]];
            [myScrollView addSubview:centerButton];
            centerOkay = YES;
        }
        if (right < [array count]) {
            rightButton = [[UIButton alloc] initWithFrame:CGRectMake(215, yOrigin + spaceBetweenItems, 100, 150)];
            [rightButton setTitle:[array objectAtIndex:right] forState:UIControlStateNormal];
            [rightButton setTitleColor:[UIColor greenColor] forState:UIControlStateNormal];
            [rightButton setBackgroundColor:[UIColor purpleColor]];
            [myScrollView addSubview:rightButton];
            rightOkay = YES;
        }
        if (leftOkay == YES && centerOkay == YES && rightOkay == YES) {
            yOrigin = yOrigin + leftButton.frame.size.height + (spaceBetweenItems * 2);
            leftOkay = NO;
            centerOkay = NO;
            rightOkay = NO;
        }

        left    = left    + 3;
        center  = center  + 3;
        right   = right   + 3;
    }
    myScrollView.contentSize = CGSizeMake(myScrollView.frame.size.width, yOrigin + leftButton.frame.size.height + (spaceBetweenItems * 2));
}

you can write this code dramatically shorter like

self.view = [[UIView alloc] initWithFrame:CGRectMake(0, 0, 320, 480)];

NSArray *array = [[NSArray alloc] initWithObjects:@"Item #1", @"Item #2", @"Item #3", @"Item #4", @"Item #5", @"Item #6", @"Item #7", @"Item #8",  nil];

NSUInteger buttonWidth = 100;
NSUInteger buttonHight = 100;
NSUInteger spacer = 5;
NSUInteger leftOffset = 5;
NSUInteger topOffset = 5;
[array enumerateObjectsUsingBlock:^(NSString *obj, NSUInteger idx, BOOL *stop) {
    int row = idx / 3;
    int column = idx %3;

    UIButton *button = [UIButton buttonWithType:UIButtonTypeRoundedRect];
    button.frame = CGRectMake((buttonWidth + spacer)*column+leftOffset, (buttonHight+spacer)*row +topOffset , buttonWidth    , buttonHight);
    [button setTitle:obj forState:UIControlStateNormal];
    [self.view addSubview:button];
}];

Note: ARC enabled


Maintaining a column for each datatype I have a function that is suppose to store variables based on their datatype, and this is table I have come up with to store those variables into.

Is it smarter to have everything broken down like this or would be wiser to only have columns for the largest for the largest datatype of a group (such as string datatypes only having a MEDIUMTEXT)? I am expecting this table to become quite large and I would like to keep the tables storage space demands as small as possible.

CREATE TABLE IF NOT EXISTS form_part_detail(
form_part_detail_id  INT NOT NULL    AUTO_INCREMENT  PRIMARY KEY,
form_part_detail_type_id   INT NOT NULL,
label   VARCHAR(255) NOT NULL,
`string`    VARCHAR(255)  NULL,
`text`   TEXT   NULL,
`mediumtext`    MEDIUMTEXT NULL,
`integer`   INT NULL,
`boolean`   TINYINT(1)  NULL,
`float` FLOAT NULL,
`double`    DOUBLE NULL,
FOREIGN KEY (form_part) REFERENCES `form_part` (form_part_id)
FOREIGN KEY (form_part_detail_type_id) REFERENCES `form_part_detail_type` (form_part_detail_type_id)
);

I would probably tackle it like this:

The database would have a few columns:

id
label
data (of some type, say, varchar for now)

The Wrapper class would be used to store/retrieve data from the database:

Responsible for conversion (serialization) from whatever type is necessary to the backend data store

The Wrapper class could have some sanity checks so that you couldn't ask for a int if it really was a string, etc etc.

This probably doesn't satisfy your "keeping things as small as possible" requirement, but disk is cheep.

You could create an object and basically serialize that and store the serialized version in the database.  You could then deserialize that when you needed to.


Extended PDO class for MySQL I recently started with object oriented programming in PHP and I am wondering if my database class is on the right track.

Am I doing something wrong? If so, how can I improve it?

Here is the class:

    <?php
    class PDOC extends PDO{
        private $myselhost;
        private $dbname;
        private $myseluser;
        private $mysqlpass;

        public function __construct($mysqlhost, $dbname, $mysqluser, $mysqlpass){
            try{
                parent::__construct("mysql:dbname={$dbname};host={$mysqlhost}", $mysqluser, $mysqlpass);
                $this->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            }catch(PDOException $e){
                return $e->getMessage();
                //die('Something went wrong, please try again.');
            }
        }

        public function queryInsert($tableName, $insertKey, $insertData, $executeData){
            $prepare = parent::prepare("INSERT INTO {$tableName} ({$insertKey}) VALUES ({$insertData})");

            try{
                $prepare->execute(
                    $executeData
                );
            }catch(PDOException $e){
                return $e->getMessage();
            }
        }

        public function queryUpdate($tableName, $insertData, $executeData, $insertWhereKey, $insertWhereValue){
            $prepare = parent::prepare("UPDATE {$tableName} SET {$insertData} WHERE {$insertWhereKey} = {$insertWhereValue}");

            try{
                $prepare->execute(
                    $executeData
                );
            }catch(PDOException $e){
                return $e->getMessage();
            }
        }

        public function queryDelete($tableName, $deleteWhereKey, $deleteWhereValue){
            $prepare = parent::prepare("DELETE FROM {$tableName} WHERE {$deleteWhereKey} = {$deleteWhereValue}");

            try{
                $prepare->execute();
            }catch(PDOException $e){
                return $e->getMessage();
            }
        }

        public function querySelect($tableName, $selectData, $sellectWhereKey = null, $selectWhereValue = null, $orderByKey = null, $orderByValue = null){
            if(isset($selectWhereKey, $selectWhereValue)){
                $prepare = parent::prepare("SELECT {$selectData} FROM {$tableName} WHERE $insertWhere");
            }
            elseif(isset($orderByKey, $orderByValue)){
                $prepare = parent::prepare("SELECT {$selectData} FROM {$tableName} ORDER BY {$orderByKey} {$orderByValue}");
            }
            elseif(isset($selectWhereKey, $selectWhereValue, $orderByKey, $orderByValue)){
                $prepare = parent::prepare("SELECT {$selectData} FROM {$tableName} WHERE {$selectWhereKey} = {$selectWhereValue} ORDER BY {$orderByKey} {$orderByValue}");
            }
            else{
                $prepare = parent::prepare("SELECT {$selectData} FROM {$tableName}");
            }

            try{
                $prepare->execute();
            }catch(PDOException $e){
                return $e->getMessage();
            }
        }
    }
?>

Your approach vs PDO

First of, you take perfectionally acceptable exceptions, and transform them into error strings, which are harder to handle.

And I'm also not sure if all your the methods really make things easier.

Let's take insert as an example:

// with your code:
$result = $db->queryInsert("myTable", "key1, key2, key3", ":value1, :value2, :value3", ["value1", "value2", "value3"]);
if ($result == "well, what do I even check here?") {
    // handle error
}

// without your code:
$prepare = $db->prepare("INSERT INTO myTable (key1, key2, key3) VALUES (:value1, :value2, :value3)");
try {
    $prepare->execute(["value1", "value2", "value3"]);
} catch(PDOException $e) {
    // handle error
}

Without your code, it's not that much more to write, and it is a lot clearer what is actually happening.

Improving your approach

Now, I'm not generally a fan of wrapping/extending PDO like this, but you can improve your methods to make it a bit more usable.

First of, documentation. It is completely unclear from the outside how to use your code. This can also easily lead to security issues, as $insertKey and $insertData should not be user-supplied.

Secondly, it's really not needed that a user passes $insertData themselves. It makes your method harder to use, it doesn't provide any additional functionality for a user, and as said above, it can easily lead to vulnerabilities.

Thirdly, as mentioned, use exceptions instead of error strings.

A better method signature might be this:

public function queryInsert($tableName, $insertKey, $insertData) throws PDOException

Now it can be used like this:

try {
    $db->queryInsert("myTable", "key1, key2, key3", ["value1", "value2", "value3"]);
} catch(PDOException $e) {
    // handle error
}

That's already a bit nicer, but really not that much simpler than the original query (and harder to use, as it's not really clear what the passed values need to be), so I'm not sure if it's worth it.

I would probably stick to the well-known and simple PDO interface. If you want to simplify it, or don't like to write plain SQL queries, something like a query builder or an orm might be worth looking into.

Security

Your approach completely disguises which values are safe and which are not, which will lead to security issues.

Basically, only $executeData may contain user input.

For some of the other values, a user may not expect it to be secure (such as $tableName), but just to be safe, I would still validate it.

For some values, this is devestating. Having such an interface as yours, I would completely expect eg $insertWhereValue to be secure.

Misc

Bug: $insertWhere doesn't exist.
You have a couple of spelling mistakes, such as sellect or mysel.


Downloading big PDF files I am trying to optimize my code when downloading around 50 pdf files from a website. All of the pdf are big in size (average 25mb). Is there any way to optimize further to download the pdf faster?

for (String pdfUrl : pdfUrls) {
    URLConnection request;
    request = new URL(pdfUrl).openConnection();
    InputStream in = request.getInputStream();
    File downloadedFile = File.createTempFile("download-pdf-" + System.currentTimeMillis(), ".pdf");
    FileOutputStream out = new FileOutputStream(downloadedFile);
    byte[] buffer = new byte[10240];
    int len = 0;
    int bytesBuffered = 0;
    while ((len = in.read(buffer)) != -1) {
        out.write(buffer, 0, len);
        bytesBuffered += len;
        if (bytesBuffered > 1024 * 1024) {
            out.flush();
        }
    }
    in.close();
    out.close();
}

Correctness

Files over 10240 bytes will fail to download because the streams are closed after one iteration of the while loop(Edit: Was paste error by OP). As of java 7, it is highly preferable to use a try-with-resources block when working with AutoCloseable objects.

Implementation

Wrapping your FileOutputStream in a BufferedOutputStream would be preferable to manually flushing.

Performance

You may see some performance gain by running each file download on its own thread. You'd need to take your existing code and move much of it into a Runnable or Callable. Use an Executor to perform some or all of the downloads concurrently.

If you made these modifications, the code might look more like:

import java.io.BufferedOutputStream;
import java.io.File;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.net.URL;
import java.net.URLConnection;
import java.util.Objects;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;

public final class FileDownloader {

    // or newFixedThreadPool() to control the number of threads.
    private final ExecutorService executor = Executors.newCachedThreadPool();

    public FileDownloader() {
        super();
    }

    public void downloadPdf(final URL targetUrl) throws IOException {
        final File destination = File.createTempFile("download-pdf-" + System.currentTimeMillis(), ".pdf");
        this.executor.submit(new DownloadCallable(targetUrl, destination));
    }

    private static final class DownloadCallable implements Callable<File> {

        private static final int BUFFER_SIZE = 1024 * 1024;

        private final URL targetUrl;
        private final File destination;

        public DownloadCallable(final URL targetUrl, final File destination) {
            this.targetUrl = targetUrl;
            Objects.requireNonNull(targetUrl);

            this.destination = destination;
            Objects.requireNonNull(destination);
        }

        @Override
        public File call() throws IOException {
            final URLConnection request = this.targetUrl.openConnection();
            try (final InputStream inputStream = request.getInputStream();
                    final FileOutputStream fileStream = new FileOutputStream(this.destination);
                    final BufferedOutputStream outputStream = new BufferedOutputStream(fileStream, BUFFER_SIZE);) {

                final byte[] data = new byte[10240];
                int bytesRead = 0;
                while ((bytesRead = inputStream.read(data)) != -1) {
                    outputStream.write(data, 0, bytesRead);
                }
            }
            return this.destination;
        }
    }
}


Derive integer lengths for given areas I want to build a function that, given an area size (target), returns a list of three lengths. The last item of the list (my_list[-1]) corresponds to the square plate.

The first two lengths in the list correspond to an orthogonal plate whose length is approximately 1.5 times larger than the one of the square plate and whose width is 1.5 times smaller. I want the lengths to be whole numbers.

The function I wrote for the task is given below, but I am sure there is a much better/faster solution. Keep in mind that when it comes to rounding floats, the bigger size for the length must be chosen since it is more conservative, but the main consideration is to not overshoot the target more than I have to.

import math

def get_plates(target):
    square = math.ceil(target ** 0.5)
    ortho1 = [math.ceil(target ** 0.5 * 1.5), math.floor(target ** 0.5 * 1.5)]
    ortho2 = [math.ceil(target ** 0.5 / 1.5), math.floor(target ** 0.5 / 1.5)]
    temp = list(zip([ortho1[0] for _ in ortho2], ortho2))
    temp.extend(list(zip([ortho1[1] for _ in ortho2], ortho2)))
    temp = [x for x in temp if x[0]*x[1] >= target]
    temp.sort(key=lambda x: abs(x[0] * x[1] - target))
    return [*temp[0], square]

For example, for a target = 700 the get_plates(700) correctly returns [39, 18, 27] and not [40, 18, 27] since 39x18 = 702 whereas 40x18 = 720 and 40x17 = 680

How can the above be made more efficient?

You don't need your temp list, you can just use itertools.product to build it:

>>> list(itertools.product(ortho1, ortho2))
[(41, 18), (41, 17), (40, 18), (40, 17)]

Also, you should use some more descriptive names.

You can use the fact that tuples are also iterable and can be written like so: t = 0, 1, saving you the unneeded list brackets.

You can also compute the square root of target once and save it.

You should add a docstring describing what your function does.

I would separate the two concerns of your function. One is to find the side length of a square most closely matching target, the other one is finding a rectangle with ratio about 1:2.

You can use min and re-use your key function for that, since you only need the pair with the smallest distance and don't care about all others. min is \$\mathcal{O}(n)\$, whereas sort is usually \$\mathcal{O}(n \log n)\$. The abs is also not needed anymore, since we made sure that only those pairs with a product greater or equal the target are still left.

With this, your code becomes:

import math
import itertools

def get_plates_square(target):
    """Returns the integer side length of a square with an area at least `target`."""
    return math.ceil(target ** 0.5)

def get_plates(target):
    """
    Return a rectangle with integer sides with a ratio of about 2:1,
    which is closest to the given `target` area.
    """
    target_sqrt = target ** 0.5
    sides_a = math.ceil(target_sqrt * 1.5), math.floor(target_sqrt * 1.5)
    sides_b = math.ceil(target_sqrt / 1.5), math.floor(target_sqrt / 1.5)
    rectangles = [(a, b)
                  for a, b in itertools.product(sides_a, sides_b)
                  if a * b >= target]
    return min(rectangles, key=lambda x: x[0] * x[1] - target)


Avoiding pointers in this C++ linked list As I know it is not good idea to have so many pointers in c++ code.
But I have written this simple implementation of linked list in c++ and there are many pointers in it, how can I improve that?
Also as I tried I can not use functions in algorithm module on this linked list, how can I make this class compatible with those?

template <typename T>
class linkedlist {
private:
    struct node {
        node() : T() {
        }

        node(node * prev, node * next, const T & data) :
                prev(prev), next(next), data(data) {
        }

        node * next;
        node * prev;
        T data = T();
    };

    node first;
    node last;

public:
    linkedlist() :
            first(nullptr, &last, T()),
            last(&first, &last, T()) {
    }

    class iterator {
        friend class linkedlist;
    private:
        node * current_node;

    public:
        iterator(node * current_node) : current_node(current_node) {
        }

        T & operator*() {
            return current_node->data;
        }

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

        iterator & operator++() {
            current_node = current_node->next;
            return *this;
        }

        bool operator!=(const iterator & other) {
            return current_node != other.current_node;
        }
    };

    void add(const T & data) {
        node * new_node = new node(last.prev, &last, data);
        new_node->prev->next = new_node;
        new_node->next->prev = new_node;
    }

    bool remove(iterator & iterator) {
        if (iterator.current_node == &first || iterator.current_node == &last)
            return false;
        node *to_remove = iterator.current_node;
        to_remove->prev->next = to_remove->next;
        to_remove->next->prev = to_remove->prev;
        delete to_remove;
        return true;
    }

    iterator begin() {
        return iterator(first.next);
    }

    iterator end() {
        return iterator(&last);
    }

    iterator find(const T & data) {
        for (iterator it = begin(); it != end(); ++it) {
            if (*it == data) {
                return it;
            }
        }
        return end();
    }
};

I see some things that may help you improve your code.

Write member initializers in declaration order

One of the constructors for node looks like this:

node(node * prev, node * next, const T & data) :
        prev(prev), next(next), data(data) {
}

That looks fine, but in fact, next will be initialized before prev because members are always initialized in declaration order and next is declared before prev in this class. To avoid misleading another programmer, you should swap the order of those such that it says instead:

node(node * prev, node * next, const T & data) :
        next(next), prev(prev), data(data) {
}

This way the initialization actually proceeds from left to right as one might expect at first glance.

Eliminate the default constructor

At the moment, one of the constructors for node is written like this:

node() : T() { }

However, this is not good at all since, if it were ever used, it would leave next and prev uninitialized.  Better would be to explicitly delete it like this:

node()  = delete;

Add a destructor

Because you're allocating memory with new you must also deallocate it with delete or your program will leak memory.  Here's one way to write it:

virtual ~linkedlist() {
    for (auto it = begin(); it != end(); it = begin()) {
        remove(it);
    }
}

Provide move semantics for iterators

Right now, it's not possible to do this:

linkedlist<std::complex<float>> ll;
ll.remove(ll.begin());

The reason is that ll.begin() is an "rvalue" and so can't be passed as a reference.  To fix this, create a version of remove with move semantics:

bool remove(iterator &&iterator) {
    if (iterator.current_node == &first || iterator.current_node == &last)
        return false;
    node *to_remove = iterator.current_node;
    to_remove->prev->next = to_remove->next;
    to_remove->next->prev = to_remove->prev;
    delete to_remove;
    return true;
}

Provide const versions of iterators

The cbegin() and cend() forms of the iterators are missing.  Here's a simple cbegin():

iterator cbegin() const {
    return iterator(first.next);
}

Provide an initializer_list constructor

At the moment, this form of the constructor doesn't work:

linkedlist<std::complex<float>> ll{{3,4}, {5,12}};

It's simple to add, however:

linkedlist(std::initializer_list<T> list) :
        first(nullptr, &last, T()),
        last(&first, &last, T())
{
    for (auto &item : list) {
        add(item);
    }
}

Provide copy function or delete it

At the moment, a copy function is compiler generated but almost certainly not what you want because it will only copy pointers and not the underlying data.  Either delete the function explicitly or provide one.

Provide member functions to suit the data structure

A linked list is not a very good data structure for many purposes, but one thing that is efficient is the ability to insert one linked list within another (a splice).  Unfortunately, this function is not provided, so any advantage is unrealized.

Provide a full iterator implementation

In order to use the algorithms in <algorithm>, you will need first class iterators, but although somewhat useful, yours are missing a number of pieces.  See this reference page for details on what a real iterator provides.


Read characters from stdin into an resizeable array As first steps in C, I'm trying to write a program that reads from stdin into an array allocated on the free store, until an exclamation mark ! is entered. The array should be extended to prevent overflow.

Here is what I've written:

#include <stdio.h>
#include <stdlib.h>

char* increase_buffer_size(char *buff_ptr, size_t multiplier);

//==================================================================
int main () {
    // initial buffer size
    size_t buffer_size = 32;

    // allocate 32 bytes of heap memory
    char *buffer_pointer = (char*) malloc(buffer_size);

    // input loop variables
    char input_var;
    size_t counter = 0;
    char sentinel = '!';

    // input loop
    printf("Type input\n>>");
    while (input_var = getchar()) {

        // check termination condition
        if (input_var == sentinel) break;

        // check capacity and double it if maximum is reached
        if (counter == buffer_size - 1) buffer_pointer = increase_buffer_size(buffer_pointer, buffer_size *= 2);

        // populate buffer
        buffer_pointer[counter++] = input_var;
    }

    getchar();

    // print input values
    printf("Buffer content: \n");
    for (auto i = 0; i < counter; ++i) printf("%c %c", buffer_pointer[i], ' ');

    getchar();
}

//==================================================================

char* increase_buffer_size(char *buff_ptr, size_t new_size) {

    char* new_ptr = (char*) realloc(buff_ptr, new_size);

    return new_ptr;
}

As I already mentioned these are my first steps in C, so I was wondering:

Is the above code written in accordance with C conventions?

Is there anything that can be skipped or written better?

Unnecessary function

The only difference between increase_buffer_size() and realloc() is that the former returns a char* and the latter returns a void*. But you can just manually do the cast anyway, which will still be shorter to type.

buffer_pointer = (char*)realloc(buffer_pointer, buffer_size *= 2);

Avoid putting lots of things on on one line

Your reallocation line does three things: checks if you need to reallocate, doubles the buffer size, and reallocates. It'd be much better to just split that up. Don't write the bodies of your if statements on the same line either, unless the body is trivial (e.g. return;). Using {}s is a good habit to get into:

// check capacity and double it if maximum is reached
if (counter == buffer_size - 1) {
    buffer_size *= 2;
    buffer_pointer = (char*)realloc(buffer_pointer, buffer_size);
}

What if realloc fails?

It's possible for it to return a null pointer after all.


XML splitting query very slow I have a query to split a delimited string into to multiple columns. The query is extremely slow and takes more than 5 minutes to handle 80444 rows. How can I improve execution time?

  WITH cte (Execution_ID,Intake_Generic_ID, [File_Name],F1,Report_Attributes) AS ( SELECT
    Execution_ID,  Intake_Generic_ID,
    [File_Name],  F1,
    CONVERT(XML,'<Report><Attribute>'
        + REPLACE(F1,'~', '</Attribute><Attribute>')
        + '</Attribute></Report>') AS Report_Attributes FROM [dbo].[Intake_Generic] )

SELECT
    Execution_ID,  Intake_Generic_ID,
    [File_Name],
    LTRIM(RTRIM(Report.value('Attribute[1]','varchar(50)'))) AS [TransactionDate],
    LTRIM(RTRIM(Report.value('Attribute[2]','varchar(50)'))) AS [TransactionNum],
    LTRIM(RTRIM(Report.value('Attribute[3]','varchar(50)'))) AS [Interchange],
    LTRIM(RTRIM(Report.value('Attribute[4]','varchar(50)'))) AS [Gantry],
    LTRIM(RTRIM(Report.value('Attribute[6]','varchar(50)'))) AS [GantryDirection],
    LTRIM(RTRIM(Report.value('Attribute[7]','varchar(50)'))) AS [GantryEntryExit],
    LTRIM(RTRIM(Report.value('Attribute[8]','varchar(50)'))) AS [RTCID],
    LTRIM(RTRIM(Report.value('Attribute[9]','varchar(100)'))) AS [Vehicle_Classification_Cd],
    LTRIM(RTRIM(Report.value('Attribute[10]','varchar(100)'))) AS [VehicleClassification],
    LTRIM(RTRIM(Report.value('Attribute[11]','varchar(100)'))) AS [Txn_Iden_Cd],
    LTRIM(RTRIM(Report.value('Attribute[12]','varchar(100)'))) AS [TransactionIdentification] FROM  cte S
        CROSS APPLY Report_Attributes.nodes('/Report') AS Tbl(Report)  order by Intake_Generic_ID

This won't do much for the performance, but maintaining this kind of T-SQL is painful, so proper formatting is essential.

The FROM clause is hidden, and one has to pay very close attention to notice the ORDER BY clause as well. Consider:

WITH cte (Execution_ID,Intake_Generic_ID, [File_Name],F1,Report_Attributes) AS (
    SELECT
         Execution_ID
        ,Intake_Generic_ID
        ,[File_Name]
        ,F1
        ,CONVERT(XML,
            '<Report><Attribute>' +
            REPLACE(F1,'~', '</Attribute <Attribute>') +
            '</Attribute></Report>') AS Report_Attributes
    FROM [dbo].[Intake_Generic]
)
SELECT
     Execution_ID
    ,Intake_Generic_ID
    ,[File_Name]
    ,LTRIM(RTRIM(Report.value('Attribute[1]','varchar(50)'))) AS [TransactionDate]
    ,LTRIM(RTRIM(Report.value('Attribute[2]','varchar(50)'))) AS [TransactionNum]
    ,LTRIM(RTRIM(Report.value('Attribute[3]','varchar(50)'))) AS [Interchange]
    ,LTRIM(RTRIM(Report.value('Attribute[4]','varchar(50)'))) AS [Gantry]
    ,LTRIM(RTRIM(Report.value('Attribute[6]','varchar(50)'))) AS [GantryDirection]
    ,LTRIM(RTRIM(Report.value('Attribute[7]','varchar(50)'))) AS [GantryEntryExit]
    ,LTRIM(RTRIM(Report.value('Attribute[8]','varchar(50)'))) AS [RTCID]
    ,LTRIM(RTRIM(Report.value('Attribute[9]','varchar(100)'))) AS [Vehicle_Classification_Cd]
    ,LTRIM(RTRIM(Report.value('Attribute[10]','varchar(100)'))) AS [VehicleClassification]
    ,LTRIM(RTRIM(Report.value('Attribute[11]','varchar(100)'))) AS [Txn_Iden_Cd]
    ,LTRIM(RTRIM(Report.value('Attribute[12]','varchar(100)'))) AS [TransactionIdentification]
FROM  cte S
    CROSS APPLY Report_Attributes.nodes('/Report') AS Tbl(Report)
ORDER BY Intake_Generic_ID

XQuery is slower than "normal" T-SQL querying, pretty much by definition.

If the results don't need to be sorted, or if they can be sorted by whoever is consuming this data, then removing the ORDER BY clause should give you a little boost, but I suspect the CROSS APPLY is much more expensive than the sorting.

Looking more closely at what the query is doing, I'd say the XQuery isn't required for this to work - it was clever, but as you saw, a total performance killer.

    ,F1
    ,CONVERT(XML,
        '<Report><Attribute>' +
        REPLACE(F1,'~', '</Attribute <Attribute>') +
        '</Attribute></Report>') AS Report_Attributes

You're building the XML from the contents of F1, by replacing occurrences of ~ within the varchar value.

If the data is shaped anything like a list of comma tilde-separated values, you could write a table-valued function that returns a table with 12 records given that F1 string:

declare @result as table (
     AttributeIndex int
    ,AttributeValue varchar(50)
);

Your CTE could be CROSS APPLY-ing that function with the contents of Intake_Generic, and the actual SELECT wouldn't have much work left to do. And without XML involved, I'm willing to bet that you'd get the results much, much faster.


Generating numbers that are a product of consecutive primes I have implemented a correct but horribly coded solution to Project Euler Problem 293.

  An even positive integer N will be called admissible, if it is a power
  of 2 or its distinct prime factors are consecutive primes. The first
  twelve admissible numbers are 2,4,6,8,12,16,18,24,30,32,36,48.

  If N is admissible, the smallest integer M > 1 such that N+M is prime,
  will be called the pseudo-Fortunate number for N.

  For example, N=630 is admissible since it is even and its distinct
  prime factors are the consecutive primes 2,3,5 and 7. The next prime
  number after 631 is 641; hence, the pseudo-Fortunate number for 630 is
  M=11. It can also be seen that the pseudo-Fortunate number for 16 is
  3.

  Find the sum of all distinct pseudo-Fortunate numbers for admissible
  numbers N less than 109.

I use several nested loops that I wish to make into some nicer functions or such. The following is the first three sections of the code, out of 10 sections required to solve the problem. Each has one more nested loop, and while the time is not an issue I would like to improve this code, but I am not sure how to implement this algorithm in a more concise manner.

What the nth section does is generate all numbers less than 1e9, that contain only the first n prime numbers(and then add numbers relating to the problem to a set).

I have tried for example to have a list of exponents for all primes, and incrementing the outermost nonzero exponent, and then backtracking when the product is larger than 1e9, however I have not been able to do anything successful.

pseudofortunate=set()

pr=generate_primes(24)

num1=1
while num1<1e9/2:
    num1*=pr[0]
    num2=num1
    while num2<1e9/3:
        num2*=pr[1]
        m=num2+3
        while True:
            if is_prime(m):
                pseudofortunate.add(m-num2)
                break
            m+=2

num1=1
while num1<1e9/2:
    num1*=pr[0]
    num2=num1
    while num2<1e9/3:
        num2*=pr[1]
        num3=num2
        while num3<1e9/5:
            num3*=pr[2]
            m=num3+3
            while True:
                if is_prime(m):
                    pseudofortunate.add(m-num3)
                    break
                m+=2

num1=1
while num1<1e9/2:
    num1*=pr[0]
    num2=num1
    while num2<1e9/3:
        num2*=pr[1]
        num3=num2
        while num3<1e9/5:
            num3*=pr[2]
            num4=num3
            while num4<1e9/7:
                num4*=pr[3]
                m=num4+3

                while True:
                    if is_prime(m):
                        pseudofortunate.add(m-num4)
                        break
                    m+=2

Ok so I figured this one out, by doing the nested loops recursively as a generator. Below is a MUCH nicer code that yields the same answer.

def loop(n,depth):
    """generates numbers containing "depth" primes, up to n"""
    if depth==0:            #generates the powers of two
        num1=1
        while num1<n/2:
            num1*=pr[0]
            yield num1
    else:
        for i in loop(n,depth-1):           #generates all numbers containing the first n prime numbers
            num_depth=i                     #number containing all previous primes
            while num_depth<n/pr[depth]:
                num_depth*=primes[depth]            #successively multiplies the nth prime
                yield num_depth

def admissiblenums(n=1e9):
    """generates a set of all admissible numbers not exceeding 1e9"""
    admissibleset=set()
    for i in range(len(pr)):
        nth_admissible_prime=set(loop(n,i))
        admissibleset|=nth_admissible_prime     #union of admissibles up to nth prime
    return admissibleset

def pseudofortunate(n):
    """returns the pseudofortunate number of n
    (the difference between n, and the first prime larger than or equal to n)"""
    prime_candidate=n+3#defined as prime larger than n+1, so starting the search for a prime at n+3, since admissible numbers are always even
    while True:
        if is_prime(m):
            return prime_candidate-n        #returns the difference(pseudo-fortunate number) if prime_candidate is prime
        prime_candidate+=2      #check odd numbers
t1=time()
primes=generate_primes(24)      #the primes less than 24, since the primorial needs to be less than 1e9
pseudofortunates=set()
for i in admissiblenums():
    pseudofortunates.add(pseudofortunate(i))    #adds psuedo-fortunate numbers to set
print sum(pseudofortunates)     #sum of unique pseudo-fortunate numbers
t2=time()
print t2-t1


Foo - Control | Naming conventions update The code below is for modern browsers only.

Naming conventions are as follows:

m or M - Model

v or V - View

c or C - Controller

   /********************************************************************************************
     ********************************************************************************************
     *( C )ontrol
     *  Cin -  Input Path
     *  COut - Ouput Path
     ********************************************************************************************
     *******************************************************************************************/

    /**
     *COut
     */

    var COut = ( function ()
    {
        var messages =
        {
            name:         'Please enter a valid name',
            email:        'Please enter a valid email',
            email_s:      'Please enter a valid email.',
            pass:         'Please enter password, 6-40 characters',
            url:          'Please enter a valid url',
            title:        'Please enter a valid title',
            tweet:        'Please enter a valid tweet',
            empty:        'Please complete all fields',
            same:         'Please make emails equal',
            taken:        'Sorry, that email is taken',
            validate:     'Please contact support to reset your password'

.

..

...

....

The code within the functions seems clear and simple to me, so my answer mostly focus on the naming conventions:

Why call your variables m..., v..., c... when you could call them "model", "view", "controller"? This would make your code immediately clearer to anyone who reads it.
The functions under "CIn" are also prefixed with "c", which is redundant. Since COut is a controller, it is obvious, that its functions are controller functions so they could simply be called "tryIt", "block", etc.
Strictly speaking, in MVC, the role of a controller is to handle user input and (optionally) to output data. There is not one controller for input and another one for output, both should be handled by the same controller. It might make sense however to wrap output operations in a separate class. In Kohana for instance there's a "Response" object. This is pretty much what you've done with COut, except that you shouldn't call it a controller since that's not really what it is. You could call it "Response" or "Output" for example. "HtmlResponse" might be a good choice too as that would allow you to also have "JsonResponse", "XmlResponse", etc.


Web Component prevent multiple API fetches I work on a small Web Component that converts an amount of money to another currency. For that, I use an API that returns the exchange rates.
I want to use the component multiple times on a page and thought that it would be a waste of traffic if every component fetches the same exchange rates. So I wrote some logic that only the first handled component fetches it, stores the results in a window variable. The other components notice that some already loads the data and store a callback in that window variable. When the fetching is done all callbacks will be resolved.
This works great but I wonder if this is a bad practice or whether this is even unnecessary because maybe it is cached etc.
Code (the described part is in the "connectedCallback" function):
const template = document.createElement('template');
template.innerHTML = `

  `;

class CurrencyConverter extends HTMLElement {
  constructor() {
    super();
    this.attachShadow({ mode: 'open' });
    this.shadowRoot.appendChild(template.content.cloneNode(true));
  }

  async fetchApi() {
    return new Promise((resolve, reject) => {
      fetch(`https://api.exchangeratesapi.io/latest?base=${this.baseCurrency}`)
        .then((response) => response.json())
        .then((data) => resolve(data))
        .catch((err) => reject(err));
    });
  }

  renderData() {
    const browserLocale = navigator.language;
    const originFormat = new Intl.NumberFormat(browserLocale, {
      style: 'currency',
      currency: this.baseCurrency,
    });
    const conversionFormat = new Intl.NumberFormat(browserLocale, {
      style: 'currency',
      currency: this.conversionCurrency,
    });

    const originAmount = originFormat.format(this.value);

    this.convertedAmount =
      this.value *
      window.rates[this.baseCurrency].data.rates[this.conversionCurrency];

    const conversionAmount = conversionFormat.format(this.convertedAmount);

    this.shadowRoot.querySelector(
      '.curr-shell'
    ).innerText = `${originAmount} | ${conversionAmount}`;
  }

  async connectedCallback() {
    this.baseCurrency = this.getAttribute('base-currency');
    this.value = parseFloat(this.getAttribute('value'));
    this.conversionCurrency = this.getAttribute('conversion-currency');

    if (!window.rates) window.rates = {};

    if (!window.rates[this.baseCurrency]) {
      // first one -> fetch api
      window.rates[this.baseCurrency] = {
        status: 'fetching',
        data: null,
        callbacks: [],
      };

      window.rates[this.baseCurrency].data = await this.fetchApi();
      window.rates[this.baseCurrency].status = 'loaded';
      this.renderData();
      // resolve all callbacks from the waiting ones
      window.rates[this.baseCurrency].callbacks.forEach((cb) => cb());
    } else if (window.rates[this.baseCurrency].status === 'fetching') {
      // currently some else is fetching -> add callback to be called when done
      window.rates[this.baseCurrency].callbacks.push(() => this.renderData());
    } else {
      // all data loaded
      this.renderData();
    }
  }
}

window.customElements.define('currency-converter', CurrencyConverter);

Demo: https://phartenfeller.github.io/currency-converter-wc/demo/

From a super short review;

I would attach the style tag to the head element, especially if you can have this element several times on the page (and ensure that you only add it once)

When all a fat arrow function does is call an another function with the provided parameter(s), then you might as well
  .then(resolve)
  .catch(reject);

instead of
  .then((data) => resolve(data))
  .catch((err) => reject(err));

This will not work in a rainy day scenario;
 window.rates[this.baseCurrency].data.rates[this.conversionCurrency];

Consider how you would handle this if window.rates[this.baseCurrency] is missing.


Reading the contents at three URLs using Promises I'm learning promises and now I'm trying to figure out if something in this code can be improved. This code is expected 3 urls and then async parallel calls should be done. When all requests are finished thet just show each request's data.

const hyperquest = require('hyperquest');
const BufferList = require('bl');
const Promise = require('bluebird');

var tasks = [];
for (var i = 0; i < 3; i++) {
    new function(index) {
        tasks[index] = new Promise(function(resolve) {
            makeRequest(resolve, index);
        });
    }(i);
}

Promise.all(tasks).then(function(result) {
    result.forEach(function(item) {
        console.log(item.bl.toString());
    })
});

function makeRequest(resolve, step) {
    var bl = new BufferList();
    var req = hyperquest(process.argv[step + 2]);
    req.on('end', function() {
        resolve({data: bl});
    });
    req.pipe(bl);
}

The main question is it right usage of Promise.
Should I use this

tasks[index] = new Promise(function(resolve) { makeRequest(resolve, index); });

or it can be simplified?

Also req.on('end', function() { resolve({data: bl}); }); is a direct call of resolver callback but maybe it's possible without it?

On the whole it does not look very elegant, especially new function, building functions in a loop and building the Promise array.

I would suggest you invest time in the bluebird Promise.map function and write something like this:

var indexes = [0,1,2];
Promise.map(indexes, function(index) {
    return new Promise(function(resolve) {
        makeRequest(resolve, index);
    });
}).then(function(result) {
    result.forEach(function(item) {
        console.log(item.bl.toString());
    })
});

Other than that, you seem very light on the failure handling, make sure to use catch and reject.


Minimum Scalar Product Here's the problem statement for Minimum Scalar Product

You are given two vectors v1=(x1,x2,...,xn) and v2=(y1,y2,...,yn). The scalar product of these vectors is a single number, calculated as x1y1+x2y2+...+xnyn.

Suppose you are allowed to permute the coordinates of each vector as you wish. Choose two permutations such that the scalar product of your two new vectors
is the smallest possible, and output that minimum scalar product.

Please review my code.

#include<iostream>
#include<vector>
#include<algorithm>

int main() {
    int t;
    std::cin >> t;
    while (t--) {
        std::size_t n;
        std::cin >> n;
        if (n != 0) {
            int product = 0;
            std::vector<int> x(n);
            for (std::size_t x_index = 0; x_index < n; ++x_index) {
                std::cin >> x[x_index];
            }
            std::vector<int> temp(n);
            for (std::size_t t_index = 0; t_index < n; ++t_index) {
                std::cin >> temp[t_index];
            }
            sort(x.begin(),x.end());
            sort(temp.begin(),temp.end());
            std::vector<int> y(n);
            for (std::size_t y_index = 0; y_index < n; ++y_index) {
                y[y_index] = temp[temp.size()-1-y_index];
            }
            for (std::size_t i = 0; i < n; ++i) {
                product += x[i]*y[i];
            }
            std::cout << product << '\n';
        }
    }
}

How can I make this code better and faster? Particularly without using an additional vector? And is there a better solution?

Perhaps you should mention that your code takes advantage of the Rearrangement inequality which states
that the scalar product of any permutation of the vectors
\$ (x_1,x_2,...,x_n) \$ and \$ (y_1,y_2,...,y_n) \$ is minimal if the elements of one vector
are in increasing order and the elements of the other in decreasing order.

I see two possible simplifications: First, instead of reading the second
vector into a temporary vector, sorting it and then reversing it "manually", you can sort in decreasing order by supplying a comparator to the sort:

for (std::size_t y_index = 0; y_index < n; ++y_index) {
    std::cin >> y[y_index];
}
sort(y.begin(),y.end(), std::greater<int>());

Second, the scalar product can be computed with

#include <numeric>

int product = std::inner_product(x.begin(), x.end(), y.begin(), 0);


Displaying a user's social media information This is for a page where it displays some social media information for that user.

I would like to improve the following code that I posted below.  It does the job perfectly, but I just feel like it could definitely be improved.  I could easily leave it the way it is but I am trying to learn and that's why I would like to get suggestions from experienced programmers.

<?php
if($facebook && !$twitter_username || !$facebook && $twitter_username) {
    $width = "490";
} else {
    $width = "930";
}
?>

<div style="width:<?=$width;?>px;margin:auto;">
    <?php if($facebook) { ?>

Like Us
on Facebook

<?php } ?>
    <?php if($twitter_username) { ?>

Follow Us
on Twitter
@<?=$twitter_username;?>
<?php } ?>

<?php
if($twitter_username) {
    if($display_tweets) {
 ?>

Latest Tweet

<?php
    }

}
if(!$display_tweets || !$twitter_username) {
    if($website_url) {
?>

Visit Our Website
www.<?=$website_url;?>

<?php
    }
}
?>

In the database there are 4 fields:

Facebook (NULL or 1)
Twitter username (NULL or username)
display_feed (NULL or 1)
Website URL (NULL or URL domain.com format)

Style (PHP)

At the beginning, the test seems pretty weird but in any case, I usually prefer

$var = (condition) ? val1 : val2;

over

if(condition) {
    $var = val1;
} else {
    $var = val2;
}

You can replace

if(condition1) {
    if(condition2) {
        stuff();
    }
}

with

if(condition1 && condition2) {
    stuff();
}

On top of that, the if(!$display_tweets || !$twitter_username) test would then be useless as it would correspond to the else case.

There's no point in closing php tags just before reopening them. Remove the ? > < ? php tags.

Style (HTML)

Do you really want to use the em markup inside the h2 one?

What you are actually doing/trying to do

The code at the beginning seems to be assuming that we have a least facebook or twitter (sometimes both).

If this is indeed the case, the
$width = ($facebook && !$twitter_username || !$facebook && $twitter_username) ? "490" : "930";
at the beginning could become
$width = ($facebook && $twitter_username) ? "930" : "490";
If it is not the case, could the beginning be behind an if (twitter || facebook) test as creating an empty div (and giving it an arbitrary width) might not be relevant. If you want to keep it, for the definition of the width, I'd rather have someting like :
$width = ($facebook && $twitter_username) ? "930" : (($facebook || $twitter_username) ? "490" : "1");

It might be interesting to use <a href="<?= $website_url; ?>" > or something like that to display the url as an actual link to make things easier from a user point of view (here I assumed that the input has been sanitized properly). On top of that, I don't understand why you hardcode the www. string (have a look at your current adress bar for more details). If there was anything to be hardcoded (and I personally wouldn't go for that), it's more likely to be the http:// part.


Jolly jumpers programming challenge I have posted my solution to the jolly jumpers programming challenge(detailed here) for your review:

  A sequence of n > 0 integers is called a jolly jumper if the absolute
  values of the differences between successive elements take on all
  possible values 1 through n - 1. For instance,

  1 4 2 3

  is a jolly jumper, because the absolute differences are 3, 2, and 1,
  respectively. The definition implies that any sequence of a single
  integer is a jolly jumper. Write a program to determine whether each
  of a number of sequences is a jolly jumper.

  Input

  Each line of input contains an integer n < 3, 000 followed by n
  integers representing the sequence.

  Output

  For each line of input generate a line of output saying Jolly or
  Not jolly.

  Sample Input

4 1 4 2 3
5 1 4 2 -1 6

  Sample Output

 Jolly
 Not jolly

//platform specific code
#ifdef WINDOWS
#include "stdafx.h"
#endif // WINDOWS

#include <iostream>
#include <vector>

using std::cout;
using std::cin;
using std::vector;

const int MAX_SEQUENCE_LENGTH = 3000;

int main()
{
    bool not_jolly;
    vector<int> sequence(MAX_SEQUENCE_LENGTH, 0);
    vector<bool> in_sequence(MAX_SEQUENCE_LENGTH, false);
    int sequence_length;
    while (cin >> sequence_length) {
        not_jolly = false;
        std::fill(sequence.begin(), sequence.end(), 0);
        std::fill(in_sequence.begin(), in_sequence.end(), false);
        for (int i = 0;i < sequence_length; i++) {
            cin >> sequence[i];
        }

        for (int i = 1;i < sequence_length; i++) {
            in_sequence[abs(sequence[i] - sequence[i - 1])] = true;
        }

        for (int i = 1;i < sequence_length;i++) {
            if (!in_sequence[i]) {
                not_jolly = true;
                break;
            }
        }

        if (not_jolly == false) {
            cout << "Jolly" << "\n";
        } else {
            cout << "Not jolly" << "\n";
        }
    }
}

The program already looks correct and simple enough, yet there are some thing that can be improved.

First and most important, you should have a unit test for the important part of the program. To extract this part, create a function bool is_jolly(const std::vector<int> &numbers). Then you can easily write a test that calls this function and checks whether the returned value is what you expect.

There is a bug. When the sequence of number contains two numbers whose difference is 2147483648, the abs function will return a negative value, making your program crash. The difference might also be any other number, so you need to check whether the array index is valid. For a start, you could replace array[index] with array.at(index), which throws an exception instead of crashing.

When you loop over an array, the array index should be of type std::size_t instead of it. Enable the compiler warnings, and it will complain about a comparison between signed and unsigned.

Variable names should not contain the word not. Just try to evaluate if (!not_jolly != false) in your head, or the double negation not_jolly == false. It would be easier to read if it were just if (is_jolly) { … }.

Nitpick: after a semicolon, there should be a space (see the for loops).

To use the algorithm std::fill, you have to include its header. From the top of my head, it should be <algorithm>.

When writing a line to the output, you can merge the strings, e.g. std::cout << "Jolly\n"; — the form you are currently using is perfectly fine, I'm just mentioning this as a shorter alternative.

What you did great:

Indenting your source code consistenly, so that it is easily readable
Importing only a few names from the std namespace
Checking for errors when reading from std::cin (at least in one case)
Writing short and easy to understand code