Euler Project 12# - highly divisible triangular number I'm an amateur programmer, new to Java and while attempting the Project Euler Archive 12 (Highly divisible triangular number) I ran into extremely long run time, with no result as of yet.

Is it efficient and what should I do to improve it? Is there a special method to follow when sorting factors of numbers?

Basically I need to find the first triangle number with over 500 divisors.

public class Divisor {

    public static void main(String[] args) {
        int f = 0; //divisors
        int m = 500; //max divisors
        int j = 1; //current number
        int z = 0; //sum (last run achieved: 135878572)
        int a = 1; //current denominator
        String t = ""; //total divisors

            while (f<=m) {
                f = 0;
                z += j;
                j++;
                System.out.println("------");
                System.out.println("t: " + z);
                //Now get factors of each, the first to have over 500 is the answer
                while (a <= z){
                    if ((z % a) == 0) {
                        t += (String.valueOf(a) + "|");
                        f++;
                    }
                    a++;
                }
                System.out.println("f: " + t);
                t="";
                System.out.println("d: " + f);
                a = 1;
            }
            System.out.print("Answer: " + z);
    }

}

Here is an example of my output (first 3 triangle numbers):

------
t: 1   <--- Triangle Number
f: 1|  <--- Factors (Divisors)
d: 1   <--- Total Factors (Total Divisors)
------
t: 3
f: 1|3|
d: 2
------
t: 6
f: 1|2|3|6|
d: 4
------

Also please note: I'm using NetBeans IDE 7.3.1, with Java 1.7

Before we get to the slowness aspect of this code, there are a few other things we should straighten out first:

Variable names

In order to better understand your code, it is helpful to have better variable names.

Whenever you see yourself adding a comment after each variable name to describe it, you are doing something wrong:

int f = 0; //divisors
int m = 500; //max divisors
int j = 1; //current number
int z = 0; //sum (last run achieved: 135878572)
int a = 1; //current denominator
String t = ""; //total divisors

Not to mention that this is just confusing:

System.out.println("t: " + z);
...
System.out.println("f: " + t);
...
System.out.println("d: " + f);
...
System.out.print("Answer: " + z);

I would have expected something like:

System.out.println("t: " + t);
...
System.out.println("f: " + f);
...
System.out.println("d: " + d);

But your output is not very informative at all about what it actually means.

Now, how about we do this?

int divisors = 0;
int maxDivisors = 500;
int currentNumber = 1;
int sum = 0;
int currentDenominator = 1;
String totalDivisors = "";

Now there's no longer a need for the comments describing each variable, and now it will be easier to understand your code.

String concatenation and System.out

A major bottleneck in your code is all the System.out.println messages. You can take the fastest code in the world, and add a bunch of calls to System.out.println to it and it will become a lot slower.

Additionally, actually storing and outputting all the divisors is a major bottleneck. You are storing the divisors by using String concatenation. String concatenation by using the += operator is slow, as a new String object is created every time. It is slightly faster to use the StringBuilder class to perform string concatenation. However, in this case I would recommend getting rid of this output entirely. Once you have checked that the calculation of the number of divisors is correct, you don't need to know what the exact divisors are anymore.

Algorithm

Now to the fun part. Your way of calculating the number of divisors is the good old brute-force-ish way. Loop from 1 to x and see if it is divisible. Makes sense.

But there is a much much much faster way.

Let's take a look at some numbers, shall we?

Number    Divisors
6         4
28        6
36        9
66        8
120       16

Let's take a look at the prime factorization for those numbers

Number    Divisors  Prime Factorization
6         4         2*3
28        6         2*2*7
36        9         2*2*3*3
66        8         2*3*11
120       16        2*2*2*3*5

In how many different ways can we pick the prime factorizations for each of these numbers?

Let's take a look at 36. There are 2x 2's in the prime factorization and 2x 3's. So we can pick 0-2 2's (three combinations) and 0-2 3's (three combinations). 3 combinations * 3 combinations = 9 !!

Coincidence? Let's take a look at 120. There are three 2's, one 3, and one 5. So there's four combinations to pick 2's, two combinations to pick 3's and two combinations to pick 5's. 4*2*2 = 16.

Coincidence? Absolutely not.

Note that we don't actually need to do the actual prime factorization, we just need to know in how many different ways we can pick the prime factors.

Assume that the prime factorization is x*x*x*y*y*z, then there will be 4*3*2 = 24 divisors for that number. No matter what the values of x, y and z are.

This is just a push in the right direction, now I will leave the fun part of implementing the code up to you (or, if you really really just want the codes, which I don't recommend, you can take a look at one of my previous questions in which I have implemented this fast approach)


Tweeting statistics on shared links I tend to be rather prolific in my sharing of things I find online. I'm interested in tracking my own habits to make them more effective. Therefore, I submit the above to be critiqued by the lot of you. Have at it.

import argparse
from backports.lzma import LZMAFile
import csv
import datetime
import time
import tweepy

parser = argparse.ArgumentParser(description='Links report on twitter -- some basic metrics as to how many links I shared today')
parser.add_argument('API_KEY', action='store', metavar='API_KEY', help='Twitter API key')
parser.add_argument('API_SECRET', action='store', metavar='API_SECRET', help='Twitter API secret')
parser.add_argument('ACCESS_TOKEN', action='store', metavar='ACCESS_TOKEN', help='Twitter access token')
parser.add_argument('ACCESS_SECRET', action='store', metavar='ACCESS_SECRET', help='Twitter access secret')
parsed = parser.parse_args()

auth = tweepy.OAuthHandler(parsed.API_KEY, parsed.API_SECRET)
auth.set_access_token(parsed.ACCESS_TOKEN, parsed.ACCESS_SECRET)
api = tweepy.API(auth)

tweets = []

today = datetime.datetime.today() - datetime.timedelta(hours=24)
today.replace(hour=0, minute=0, second=0, microsecond=0)
today = today.timetuple()

def links_today(link):
    date_of_link = time.localtime(long(link[0]))
    return today < date_of_link

with LZMAFile('/home/ec2-user/public_html/links.csv.xz') as fin:
    reader = csv.reader(fin.readlines()[1:])
    tweets = filter(links_today, reader)

status = 'In the last 24 hours, I sent {0} links to {1} unique recipients.'.format(len(tweets[1]), len(set(tweets[2])))

if len(tweets) != 0:
    api.update_status(status=status)

A pair of style comments based on PEP 8:

It’s good that you sort your module imports alphabetically, but you should also split out third-party imports from standard library modules. Specifically, this means moving backports and tweepy into their own group. (Imports)
Wrap lines to 79 characters. (Maximum Line Length)

Now on to the meat of the program:

Get your secret information from the keychain, not the command line.

I’m not a big fan of passing in passwords or secret information over the command line. They’ll show up in your command history, and you probably need to copy/paste them from somewhere every time you use them (Twitter’s access tokens are not particularly easy to remember).

I prefer to use the keyring module, which stores and accesses passwords via the system keychain.

That’s how I store my secret tokens for Twitter, which I think is a bit more secure and more convenient. Here’s a snippet from my Twitter backup script which gets my tokens this way:

import keyring
import tweepy

def setup_api():
    """Authorise the use of the Twitter API."""
    a = {attr: keyring.get_password("twitter", attr) for attr in [
        'consumerKey',
        'consumerSecret',
        'token',
        'tokenSecret'
    ]}
    auth = tweepy.OAuthHandler(a['consumerKey'], a['consumerSecret'])
    auth.set_access_token(a['token'], a['tokenSecret'])
    return tweepy.API(auth)

and here’s how I would initially set the tokens:

import keyring

keyring.set_password("twitter", "consumerKey", 'abcdefg1234567')
# repeat for other secrets/tokens as appropriate

Tidy up your date handling code.

There are several things I’d change about this part:

Use a different name for the today variable. Not only is it incorrect (because the datetime object it creates points to yesterday), but it also has the potential for confusion with the today() method.
Add some comments to explain the purpose of this variable – what does it represent, and why are you doing it this way?
The today.replace() line has no effect on the program. The replace() method returns a new datetime object; it doesn’t modify in-place. Here’s a short snippet that demonstrates the difference:

mydate = datetime.datetime.today()
mydate.replace(hour=0, minute=0, second=0, microsecond=0)
print mydate  # 2015-04-29 19:29:54.572354

mydate2 = datetime.datetime.today()
mydate2 = mydate.replace(hour=0, minute=0, second=0, microsecond=0)
print mydate2 # 2015-04-29 00:00:00

You need to add today = to the start of the line.

(Although it’s not clear to me why you need this line, anyway. It means you’ll get links from the last 24 hours and a bit. If you’re not using it, remove this line from the script.)

Clarify the links_today() function.

You should add a docstring to the function to explain what it does, and what sort of input it expects. It’s not clear what link is supposed to be, or why you're taking the 0th index to get the time.
The name of the function suggests to me it might return a list of links from today. Instead it tells me whether a link was posted today. I’d give the function a more descriptive name, e.g. was_posted_today().

Don’t hard code the path to your csv.xz file halfway down the program. It would be better to put it as a global variable right at the top of the script: that way, if you ever want to change it, it’s easy to find.
It would be good to add some comments about the format of file, and what each line of the CSV looks like. That would help to explain the seemingly random indices of tweet that you take later.
For the status string, I would precompute the number of links and recipients, accompanied with a comment about why you take index 1 and 2, respectively. Otherwise these seem like magic numbers.

Also note that if either of these numbers are 1, you will have a pluralisation bug. Oops.
Wrap your code in a main() function.

Rather than have all your program code in the top-level, define a series of functions, concluding in main(), which is where your main program flow goes. Then put this at the end of your script:

if __name__ == '__main__':
    main()

Now, if the script is run as a standalone program, everything in main(), but you can import the functions from this script in another file as well. It makes it easier to reuse code.


ActiveAdmin: Hide a whole panel in a smart way In order to only show relevant information to my users I want to disable the whole panel when all fields inside that panel are empty.

Currently I do this hacky method to get to this result:

if resource.phone.present? or
    resource.fax.present? or
    resource.email.present? or
    resource.website.present?
  panel I18n.t('contact_details') do
    attributes_table_for resource do
      row :phone if resource.phone.present?
      row :fax if resource.fax.present?
      row :email if resource.email.present?
      row :website if resource.website.present?
    end
  end
end

So I need to check all fields in order to know if I display the panel.
Then once I display the panel I need to check all the fields again in order to display the given row or not.

Now the user sees only what is relevant but my admin code is cluttered and no longer as maintainable.

I don't know ActiveAdmin, so I can only offer a local improvement.  That said, a little metaprogramming could work here.  The idea is to have the list of resource items in one place:

RESOURCES_ITEMS = [
  :phone,
  :fax,
  :email,
  :resource,
]

and then use that list with Object#public_send and Enumerable#any?:

if RESOURCE_ITEMS.any? { |item| resource.public_send(item).present? }
  panel I18n.t('contact_details') do
    attributes_table_for resource do
      RESOURCE_ITEMS.each do |item|
        if resource.public_send(item).present?
          row.public_send(item)
        end
      end
    end
  end
end

While not a huge improvement, this does DRY the list of resource items.


Clojure - substring? function Trying to write a substring function in Clojure. I am sure there is a more idiomatic way. Can anyone enlighten me?

Otherwise, here is my version. Thoughts?

(defn substring?
  "is 'sub' in 'str'?"
  [sub str]
  (if (not= (.indexOf str sub) -1)
    true
    false))

As you wrote in your comments, (.contains "str" "sub") is perfectly fine. it is indeed java interop - it runs the method contains on String object "str".

two more comments, first, passing str as a var name isnt so good, since str is a function, so you should consider giving it a different name. Second, in your implementation, its quite redundant to write

(defn substring? [sub st]
  (if (not= (.indexOf st sub) -1)
   true
   false))

You could simply write

(defn substring? [sub st]
 (not= (.indexOf st sub) -1))


Flood-clearing routine for a JavaFx Minesweeper I really hate copy and pasting my code from one method to another. It signals something in my brain, "This could be done either more efficiently or more readable". This is used in a minesweeper program I am implementing with a JavaFx GUI (that's not very relevant, but it might help to know). Hopefully this is enough context!

@Contract(pure = true)
private int setMinX(int x){ return (x == 0 ? 0:(x-1)); }
@Contract(pure = true)
private int setMinY(int y){ return (y == 0 ? 0:(y-1)); }
@Contract(pure = true)
private int setMaxX(int x){ return (x == this.width-1 ? this.width-1 : x+1); }
@Contract(pure = true)
private int setMaxY(int y){ return (y == this.height-1 ? this.height-1 : y+1);}

private void adjacents(int x, int y){
    //calculate adjacents for the just placed mine
    int minX = setMinX(x);
    int minY = setMinY(y);
    int maxX = setMaxX(x);
    int maxY = setMaxY(y);

    if (this.mineField[minX][minY] != -1){ this.mineField[minX][minY] += 1; } //left top corner
    if (this.mineField[maxX][minY] != -1){ this.mineField[maxX][minY] += 1; } //right top corner
    if (this.mineField[maxX][maxY] != -1){ this.mineField[maxX][maxY] += 1; } // bottom right corner
    if (this.mineField[minX][maxY] != -1){ this.mineField[minX][maxY] += 1; } // bottom left corner

    if(x > 0 && x < this.width-1){
        if (this.mineField[x][minY] != -1){ this.mineField[x][minY] += 1; } // top-middle
        if (this.mineField[x][maxY] != -1){ this.mineField[x][maxY] += 1; } //bottom-middle
    }

    if(y > 0 && y < this.height-1){
        if (this.mineField[minX][y] != -1){ this.mineField[minX][y] += 1; } // left-middle
        if (this.mineField[maxX][y] != -1){ this.mineField[maxX][y] += 1; } //right-middle
    }
}

public void floodClear(int x, int y) {

    int minX = setMinX(x);
    int minY = setMinY(y);
    int maxX = setMaxX(x);
    int maxY = setMaxY(y);

    if (this.mineField[x][y] >= 0 && !this.liveGame[x][y]) {
        //IMPLEMENT WITH GUI
        //show(x,y);
        if (this.mineField[minX][minY] == 0) {
            floodClear(minX, minY);
        }
        if (this.mineField[maxX][minY] == 0) {
            floodClear(maxX, minY);
        }
        if (this.mineField[maxX][maxY] == 0) {
            floodClear(maxX, maxY);
        }
        if (this.mineField[minX][maxY] == 0) {
            floodClear(minX, maxY);
        }

        if (x > 0 && x < this.width - 1) {
            if (this.mineField[x][minY] == 0) {
                floodClear(x, minY);
            } // top-middle
            if (this.mineField[x][maxY] == 0) {
                floodClear(x, maxY);
            } //bottom-middle
        }

        if (y > 0 && y < this.height - 1) {
            if (this.mineField[minX][y] == 0) {
                floodClear(minX, y);
            } // left-middle
            if (this.mineField[maxX][y] == 0) {
                floodClear(maxX, y);
            }  //right-middle
        }
    }
}

Iterating through adjacents

There are two main approaches to take while iterating through the neighbors in Minesweeper.

The first and most common option is to use a nested for loop:

for (int y = -1; y <= 1; y++) {
    for (int x = -1; x <= 1; x++) {
        if (x == 0 && y == 0) {
            continue; // We don't want to consider the field itself as a neighbor
        }
        increaseAdjacentMineCounter(mineX + x, mineY + y);
    }
}

The second approach, and the one that I normally use and like better, is to use an array to define which fields should be used as your neighbors.

int[][] neighbors = {{ -1, -1 }, { -1, 0 }, { -1, 1 }, { 0, -1 },
    { 0, 1 }, { 1, -1 }, { 1, 0 }, { 1, 1 }};
for (int[] neighbor : neighbors) {
    int x = neighbor[0];
    int y = neighbor[1];
    increaseAdjacentMineCounter(mineX + x, mineY + y);
}

The reason for why I like the second approach better is because it becomes more data-oriented. You can change the way you consider neighbors without changing your code. Changing the way your neighbors are considered can be a bit entertaining but very confusing :)


Liveupdate phone number forms and test for international numbers I took a base from here and built on it. You call the function with onkeypress (hence live changing).

The function gives output like this for domestic numbers:

  (123) 456–7890

When the numbers pass the above character limit (13), the function outputs international numbers like this:

  +12 (345) 678-9012

It doesn't handle backspacing elegantly (reformatting), which is on my to-do list.  I'm looking for feedback on how to improve.

 var mask = function(content) {
      var val = content.value.split('');
      if (val.length > 13) {
        tel = '+'
      }
      else {tel = '('}
      for(var i=0; i< val.length; i++) {
        if(val[i] == '+'){
          val[i]='';
        }
        if(val[i] =='('){
          val[i]='';
        }
        if(val[i]==')'){
          val[i]='';
        }
        if(val[i]==' '){
          val[i]='';
        }
        if(val[i]=='-'){
          val[i]='';
        }
      }
      for(var i=0; i<val.length; i++){
          if (i==3){
            if (val.length>13) {
            val[i]= ' (' + val[i];
            }
            else {
            val[i]=val[i]+') ';
            }
          }
          if (i==7){
            if (val.length>13) {
            val[i]= val[i] + ') ';
              if (val[i+2] == '–') {
                val[i+2]='';
                val[i+5]='–'
              }
            }
          }
          if(i==8 && val[i+1] != '–'){
            if (val.length <= 13) {
            val[i]=val[i]+'–';
            }
          }
          tel=tel+val[i];
      }
      content.value=tel;
    };

Regular expressions are a quicker way to get it done. Here's a naïve implementation that's (I believe) functionally equivalent to yours:

var mask = function (content) {
  var val = content.value.replace(/[^\d]/g, ''); // Remove anything that's not a digit
  if( val.length <= 10 ) { // US number
    content.value = val.replace(/^(\d{3})(\d{3})(\d*)$/, '($1) $2-$3');
  } else { // Other
    content.value = val.replace(/^(\d{2})(\d{3})(\d{3})(\d*)$/, '+$1 ($2) $3-$4');
  }
};

Here's a live demo

But this is far, far from perfect. There are many, many things to consider.

For starters, non-US numbers aren't necessarily longer than US ones. My own number is just 8 digits (or 10, if you count the country digits - or 12 if you use "00xx" instead of "+xx" for the country code). So if I wrote my phone number (as I normally would) as "+xx xx xx xx xx", it'd be formatted as a US number - which it isn't. If I wrote "00xx xx xx xx xx", I'd get "+00 ...", which is also wrong since it's an invalid country code.

On top of that, there's a ton of different local conventions for how to write phone numbers.

Point is that you can't go by the phone-number-length alone.

Regardless of how far you want to go with it, though, regular expressions will come in handy. Here's a nice interactive reg exp demo/tester to get you started.


Pulling people out of a database I have this class that pulls people out of a database. I want it to be a little prettier and function better.

<?php
class comps extends db_config {

    public $output = array();
    private $arr = array();
    public $area;
    public $me;
    public $options = array();

    public function __construct() {
        $this->pdo = new PDO('mysql:host='.$this->_host.';dbname='.$this->_db.';charset='.$this->_charset, $this->_user, $this->_pass);
    }

    public function __toString() {
        return var_dump($this->result());
    }

    public function result() {
        return $this->output;
    }

    public function setoption($option, $value) {
        array_push($this->options, array($option=>$value));
    }

    public function run() {

        $where = array();
        $limit = '';
        $showMe = '';

        if (isset($this->options['admin'])) {
            $where[] = "admin = '".$this->options['admin']."'";
        }

        if (isset($this->options['area'])) {
            $where[] = "area = '".$this->area."'";
        }

        if (isset($this->options['showMe']) && $this->options['showMe'] = true) {
            $showMe = "AND missionary <> '".$this->me."'";
        }

        if (isset($this->options['limit'])) {
            $limit = " LIMIT ".$this->options['admin'];
        }

        if (count($where) > 0) {

            $where = implode(' AND ', $where);

        } else {
            $where = '';
        }

        $area = isset($this->area) ? $this->area : '';

        if ($area != '') {
            $sth = $this->pdo->prepare("SELECT missionary FROM missionary_areas WHERE area_uid = :area AND missionary_released = '0' ".$showMe);
            $sth->bindParam(':area', $area);
            $sth->execute();
            $sth->setFetchMode(PDO::FETCH_OBJ);

            while ($mis = $sth->fetch()) {
                $sth2 = $this->pdo->prepare("SELECT * FROM missionarios WHERE ".$where." mid = :mid LIMIT 1");
                $sth2->bindParam(':mid', $mis->missionary);
                $sth2->execute();

                $this->arr[] = $sth2->fetch(PDO::FETCH_ASSOC);
            }

            foreach($this->arr as $a) {
                array_push($this->output, $a);
            }
        } else {
            $sth = $this->pdo->prepare("SELECT * FROM missionarios ".$where.$limit);
            $sth->execute();
            $res = $sth->fetchAll(PDO::FETCH_ASSOC);

            foreach($res as $a) {
                array_push($this->output, $a);
            }
        }

    }
}

Here's the code smells:

The new keyword in a class definition
A result() method and $output attribute for data created by single method
A very long, ambiguous method: run
Two mostly-different SQL statements separated by an if-else

And here's what I'd do to eliminate them:

Move the PDO object to a constructor argument (see below) so you're not violating the Law of Demeter (see #1 and 2 here)
Remove result() altogether and simply return the result from the method in which it's built.
Break this into smaller, more meaningful methods based on responsibility (see #4 below)
Create getMissionaryByArea() and getAllMissionaries() methods with optional parameters

Here's what my code might look like:

<?php

/**
 * This class name is ambiguous.
 * I'm not sure what it means, so I cannot suggest a better name.
 */
class comps {

    public function __construct(PDO $pdo) {
        $this->pdo = $pdo;
    }

    public function getMissionariesByArea($area, $admin_id=false, $omit_missionary_id=false, $limit=100) {
        $sql = <<<'ENDSQL'
        SELECT m.*
        FROM missionarios m
        JOIN missionary_areas ma ON ma.missionary = m.mid
        WHERE ma.area_uid = :area
        AND ma.missionary_released = '0'
        LIMIT :limit
ENDSQL;
        $params = [
            'area' => $area,
            'limit' => $limit
        ];

        if ($omit_missionary_id) {
            $sql .= ' AND ma.missionary <> :omit_missionary_id';
            $params['omit_missionary_id'] = $omit_missionary_id;
        }

        if ($admin_id) {
            $sql .= ' AND m.admin = :admin_id';
            $params['admin_id'] = $admin_id;
        }

        $sth = $this->pdo->prepare($sql);
        $sth->execute($params);
        return $sth->fetchAll();
    }

    public function getAllMissionaries($admin_id=false, $limit=100) {
        $sql = 'SELECT * FROM missionarios';
        $params = ['limit' => $limit];

        if ($admin_id) {
            $sql .= ' AND admin = :admin_id';
            $params['admin_id'] = $admin_id;
        }

        $sth = $this->pdo->prepare($sql);
        $sth->execute($params);
        return $sth->fetchAll();
    }
}

I can revise if I've missed something in my refactoring.


Quick Sort - implementation Can anything be improved in this code?

#include <iostream>
using namespace std;

void print(int *a, int n)
{
    int i=0;
    while(i<n){
        cout<<a[i]<<",";
        i++;
    }
}

void swap(int i,int j, int *a){
    int temp = a[i];
    a[i] = a[j];
    a[j] = temp;
}

void quicksort(int *arr, int left, int right){
    int min = (left+right)/2;
    cout<<"QS:"<<left<<","<<right<<"\n";

    int i = left;
    int j = right;
    int pivot = arr[min];

    while(left<j || i<right)
    {
        while(arr[i]<pivot)
        i++;
        while(arr[j]>pivot)
        j--;

        if(i<=j){
            swap(i,j,arr);
            i++;
            j--;
        }
        else{
            if(left<j)
                quicksort(arr, left, j);
            if(i<right)
                quicksort(arr,i,right);
            return;
        }
    }
}

int main() {
    int arr[8] = {110, 5, 10,3 ,22, 100, 1, 23};
    quicksort(arr, 0, (sizeof(arr)/sizeof(arr[0]))-1);
    print(arr, (sizeof(arr)/sizeof(arr[0])));
    return 0;
}

In general, the code you have is neat, and easy to follow. The QuickSort algorithm is relatively traditional, and the pieces I expect to see are about where I expect them to be.

Let's go through the issues that I see though... and some of them are serious...

Namespaces

Using namespace::std is generally a poor idea. The possibility of namespace pollution is real. In fact, you have implemented a swap function and there already is one std::swap, but we'll get to that.

Style

You have a variable min, but this should be mid.
sometimes you put the function parameters with the left/right indices before the array, and sometimes after. You have:

void quicksort(int *arr, int left, int right) {

and you also have:

void swap(int i,int j, int *a) {

pick one, and stick with it. I would personally recommend putting the array first, and the indices afterwards.
Use whitespace appropriately. Operators like the << operator on cout are operators like any others. Use the space to improve readability:

cout<<"QS:"<<left<<","<<right<<"\n";

should be:

std::cout << "QS:" << left << "," << right << "\n";

Bugs

You have a few bugs in here which should be addressed:

There's the potential for integer overflow when calculating the mid point. This is a 'famous' bug. Your code int min = (left+right)/2; should be done as:

int mid = left + (right - left) / 2;

The above solution will not overflow.
You should, when partitioning the data, consider values equal to the pivot value, to be either left or right of the pivot. Your code you use  a strict < or > depending on whether you are on the right or left. One of those should include =. Your code, as it is, will run through the actual pivot value and do some funky business with it. You end up moving the pivot around in various ways.
You have a potential overrun (buffer overflow) in your loop conditions. It is posible, when you get to this line in the pivoting:

    while(arr[i]<pivot)
        i++;

for i to run off the end of the array. If all the remaining values are less than the pivot, there's nothing stopping it from going off. You still need to check against j in these loops.

Swap

C++ has a swap function, use it. To get it, from C++11 #include<utility> and before that #include<algorithm>

Algorithm

The classic quick-sort is done in 5 stages:

find a 'pivot value'.
move all values less than (or equal to) the pivot value to 'the left'.
move all values larger than the pivot to 'the right'.
quick-sort the values less than(or equal)
quick-sort the values larger than.

Note that many text books extract the first 3 stages in to a 'partitioning' function. The purpose of that function is to identify the pivot value, move the candidates around, and then insert the pivot value back in to the data at 'the right place'.

That last part is key, it leaves the pivot value in the exact place it is supposed to be. This means you never have to include that pivot value in the sorts again.

Let's break that logic down in to it's methods, then, with the assumption that' there's a 'pivoting' function that does the first 3 steps. That leavs a simpler quicksort that looks like:

void quicksort(int *arr, const int left, const int right){

    if (left >= right) {
        return;
    }

    int part = partition(arr, left, right);

    quicksort(arr, left, part - 1, sz);
    quicksort(arr, part + 1, right, sz);
}

Notice, in the above, that the check to make sure the inputs are valid are done on entry to the recursive function. This simplifies the last part of the function. The same code could, alternatively, be written similar to yours, as:

void quicksort(int *arr, const int left, const int right, const int sz){

    int part = partition(arr, left, right);
    std::cout << "QSC:" << left << "," << right << " part=" << part << "\n";
    print (arr, sz);

    if (left < part - 1) {
        quicksort(arr, left, part - 1, sz);
    }
    if (part + 1 < right) {
        quicksort(arr, part + 1, right, sz);
    }
}

I prefer the first.... it makes it more easy to spot the recursion terminator.

So, that's now a simpler quicksort, partition the data, sort each partition (but not the actual partitioning value which is in the right place).

How do you partition the data?

The trick here is to swap the pivot value to the front of the sequence, partition the rest of the values, and then swap the pivot value back to where it belongs:

int partition(int *arr, const int left, const int right) {
    const int mid = left + (right - left) / 2;
    const int pivot = arr[mid];
    // move the mid point value to the front.
    std::swap(arr[mid],arr[left]);
    int i = left + 1;
    int j = right;
    while (i <= j) {
        while(i <= j && arr[i] <= pivot) {
            i++;
        }

        while(i <= j && arr[j] > pivot) {
            j--;
        }

        if (i < j) {
            std::swap(arr[i], arr[j]);
        }
    }
    std::swap(arr[i - 1],arr[left]);
    return i - 1;
}

Note how the code above double-checks the buffer overflow?

Putting this all together, and leaving in some of the debug statements you have, I would have the code:

#include <iostream>
#include <algorithm>

void print(int *a, int n)
{
    int i = 0;
    while(i < n){
        std::cout << a[i] << ",";
        i++;
    }
    std::cout << "\n";
}

int partition(int *arr, const int left, const int right) {
    const int mid = left + (right - left) / 2;
    const int pivot = arr[mid];
    // move the mid point value to the front.
    std::swap(arr[mid],arr[left]);
    int i = left + 1;
    int j = right;
    while (i <= j) {
        while(i <= j && arr[i] <= pivot) {
            i++;
        }

        while(i <= j && arr[j] > pivot) {
            j--;
        }

        if (i < j) {
            std::swap(arr[i], arr[j]);
        }
    }
    std::swap(arr[i - 1],arr[left]);
    return i - 1;
}

void quicksort(int *arr, const int left, const int right, const int sz){

    if (left >= right) {
        return;
    }

    int part = partition(arr, left, right);
    std::cout << "QSC:" << left << "," << right << " part=" << part << "\n";
    print (arr, sz);

    quicksort(arr, left, part - 1, sz);
    quicksort(arr, part + 1, right, sz);
}

int main() {
    int arr[8] = {110, 5, 10,3 ,22, 100, 1, 23};
    int sz = sizeof(arr)/sizeof(arr[0]);
    print(arr, sz);
    quicksort(arr, 0, sz - 1, sz);
    print(arr, sz);
    return 0;
}

I have put this in ideone too.


Implementing an ArrayList I implemented ArrayList functionality in C as follows:

#include <stdlib.h>
#include <assert.h>
#include "ArrayList.h"

struct _arraylist {
    size_t size;
    void ** data;
};

struct _arraylist *arraylist_create() {
    /* Allocate Memory */
    struct _arraylist *list = malloc(sizeof(struct _arraylist));
    assert(list != NULL);
    list->size = 0;
    list->data = calloc(2, sizeof(void *));
    assert(list->data != NULL);
    list->data[0] = NULL;
    return list;
}

void arraylist_setdata(struct _arraylist *list, void ** data, int max, int clear_data) {
    /* Sets the internal array of the arraylist */
    clear_data ? arraylist_clear(list) : NULL;
    list->data = data;
    list->size = max;
}

void arraylist_add(struct _arraylist *list, void *elem) {
    /* Adds one element of generic pointer type to the internal array */
    void ** new_data = realloc(list->data, arraylist_getsizeof(list));
    assert(new_data != NULL);
    new_data[list->size] = elem;
    arraylist_setdata(list, new_data, list->size + 1, 0);
}

void *arraylist_get(struct _arraylist *list, int index) {
    /* Gets an member of the array at an index */
    return list->data[index];
}

size_t arraylist_getsizeof(struct _arraylist *list) {
    /* Returns the size of the internal array in memory */
    return sizeof(*list->data);
}
size_t arraylist_getsize(struct _arraylist *list) {
    /* Returns the number of elements in the arraylist */
    return list->size;
}

void arraylist_remove(struct _arraylist *list, int index, int freeit) {
    /* Removes one element at and index */
    if (index > list->size - 1)
        return;
    if (list->size == 1) {
        arraylist_clear(list);
        return;
    }
    if (freeit)
        free(arraylist_get(list, index));
    for ( int i = index; i < list->size; ++i ) {
        if (i == list->size - 1)
            list->data[i] = NULL;
        else
            list->data[i] = list->data[i + 1];
    }
    void ** new_data = realloc(list->data, arraylist_getsizeof(list));
    --list->size;
    assert(new_data != NULL);
    arraylist_setdata(list, new_data, list->size, 0);
}

void arraylist_clear(struct _arraylist *list) {
    /* Clears the internal array */
    list->size = 0;
    free(list->data);
    list->data = NULL;
}

void arraylist_deallocate(struct _arraylist *list) {
    /* De-allocates the arraylist from memory
    No usage of the arraylist is allowed after this function call */
    if (list->data != NULL)
        free(list->data);
    free(list);
}

int arraylist_getindex(struct _arraylist *list, void *elem) {
    /* Looks for elem in list and returns the index or -1 if not found */
    for(int i = 0; i < list->size; ++i)
        if (elem == arraylist_get(list, i))
            return i;
    return -1;
}

I'm testing it like this:

#include <stdio.h>
#include "ArrayList.h"

int main(int argc, char const *argv[])
{
    ArrayList *list = arraylist_create();
    int i;
    for(i = 0; i < 100; ++i)
        arraylist_add(list, &i);
    for(i = 0; i < 100; ++i)
        printf("i: %d\n", *(int *)arraylist_get(list, i));
    for(i = 0; i < 100; ++i)
        arraylist_remove(list, i, 0);
    arraylist_deallocate(list);
    return 0;
}

It works fine, though realloc sometimes crash the program. Can I improve this ?

First, a word on naming:

The name you've chosen for your type, _arraylist is a bad name for a library interface
type. Names starting with _ are not pleasant to work with in user code. They are commonly
used inside library internals. Better names would be ArrayList or array_list.

Actually, in your usage example, you have ArrayList. Does this mean that in the header,
which is not included here, you have something like this?

typedef _arraylist ArrayList;

If you did define an opaque type in the header, like above, that would be a good practice.
But then you should not use any reference to _arraylist in your code. Use always the typedef'd name to avoid confusion.

The function name prefix should also follow exactly the name of the type, so for ArrayList all functions should be prefixed the ArrayList_, e.g.:

ArrayList * ArrayList_create();

Also, I would suggest that you avoid tightlypacked names, like in arraylist_getsize(). Adding an
underscore to separate words makes them a lot more readable. E.g.: ArrayList_get_size().

Problems with memory:

Lets look at arraylist_create():

struct _arraylist *arraylist_create() {
    struct _arraylist *list = malloc(sizeof(struct _arraylist));
    assert(list != NULL);
    list->size = 0;
    list->data = calloc(2, sizeof(void *));
    assert(list->data != NULL);
    list->data[0] = NULL;
    return list;
}

First thing unusual here is the assertions. Assertions are not the proper way to
handle a memory allocation failure. Plus, they are commonly disabled on release builds,
so on release, if you'd happen to run out of memory, the program would just crash silently.
You should probably return a NULL in this case (maybe also log to stderr) and let the caller handle this error as he/she sees fit.

Second problem here is with calloc(). You are allocating 2 void pointers, however, size is set to zero.
I don't really get the point of this. Since your structure is more like and array of arrays then a list,
what you should do is allocate the array of pointers with some predefined default size, then allocate the
individual arrays as needed. Growing the array of pointers on demand. How arraylist_create() should look like:

ArrayList * ArrayList_create() {
    ArrayList *list = malloc(sizeof *list);
    if (list == NULL) {
        return NULL;
    }

    list->size = 0;
    list->data = calloc(INITIAL_BASE_ARRAY_SIZE, sizeof(void *));
    if (list->data == NULL) {
        free(list); // Don't leek memory here!
        return NULL;
    }

    return list;
}

Another big memory issue is the constant re-allocations done by arraylist_add() and arraylist_remove().

Remove should not shrink the sequence. Keep that space around if the array grows again in the future. You can add an explicit way to let the user shrink the storage if necessary (a la std::vector::shrink_to_fit()).

Adding to the array can be made to run in amortised-constant time if you pre-allocate storage with a larger size then the requested. (Again inspired by the STL vector).

sizeof mistake:

This will not return what you expect:

size_t arraylist_getsizeof(struct _arraylist *list) {
    /* Returns the size of the internal array in memory */
    return sizeof(*list->data);
}

The sizeof operator always returns the size of the type it is applied to.
It cannot infer the size of an array pointed by a pointer, because it is a
compile-time operation. arraylist_getsizeof() will always return the same value,
the size of a void pointer, which will be 4 or 8, depending on the architecture.

Use assertions to check for invariants:

You should assert that the *list parameter of every function is valid. This is a precondition of every function, they cannot work without a valid ArrayList instance, so you should assert that once the function enters.

Miscellaneous:

You don't need to check if the pointer is null before freeing it. In arraylist_deallocate() the if (list->data != NULL) check is uneeded.

arraylist_deallocate would be more symmetric with arraylist_create if named arraylist_destroy.


Simple input and output in assembly x86_64 I am new assembly programming in Linux (x86_64) and I want to make sure that I am programing in a correct way. I wrote a program that just takes an input from the user and then writes his input to stdout
SYS_WRITE   equ 1 ; write text to stdout
SYS_READ    equ 0 ; read text from stdin
SYS_EXIT    equ 60 ; terminate the program
STDOUT      equ 1 ; stdout

section .bss
    uinput resb 24 ; 24 bytes for user string
    uinput_len equ $ - uinput ; get length of user input

section .data
    text db "You wroted: "
    text_len equ $ - text

section .text
    global _start

_start:
    mov rax, SYS_READ
    mov rdi, STDOUT
    mov rsi, uinput
    mov rdx, uinput_len
    syscall

    mov rax, SYS_WRITE
    mov rdi, STDOUT
    mov rsi, text
    mov rdx, text_len
    syscall

    mov rax, SYS_WRITE
    mov rdi, STDOUT
    mov rsi, uinput
    mov rdx, uinput_len
    syscall

    mov rax, SYS_EXIT
    mov rsi, 0 ; successful exit
    syscall

Am I doing this experiment correctly?
Which suggestions do you find to improve this code?
Could you please provide some resources to deepen in good practices (and if posible more features or effective techniques)?
Thank you.

Three small errors

section .data
   text db "You wroted: "
   text_len equ $ - text

A small spelling error (typo). Correct is: "You wrote: " without the d.

mov rax, SYS_READ
mov rdi, STDOUT
mov rsi, uinput
mov rdx, uinput_len
syscall

For SYS_READ you need to use STDIN instead of STDOUT.

mov rax, SYS_EXIT
mov rsi, 0 ; successful exit
syscall

The first parameter goes in the RDI register instead of RSI.
Three small improvements

section .bss
   uinput resb 24 ; 24 bytes for user string
   uinput_len equ $ - uinput ; get length of user input

It's strange to see a calculation for the uinput_len variable given that the length is a hardcoded 24. What you can write is:
section .bss
    uinput_len equ     24         ; 24 bytes for user input
    uinput     resb    uinput_len

Be nice for the person that uses your program and show a prompt of some kind before expecting an input.

For the final result you currently show the whole inputbuffer. What if the user didn't input that much characters? Best to only show the characters that were effectively inputted. You obtain this count in the RAX register upon returning from SYS_READ. e.g. If the user inputs 5 characters then RAX will hold 6. Those 5 characters plus the terminating newline character (0Ah).
Same code, different style
You should offset you tail comments so that they all start in the same column. This will improve readability.
And because readability is very very important, I've applied the same rule to the labels, mnemonics, and operands.
    SYS_READ   equ     0          ; read text from stdin
    SYS_WRITE  equ     1          ; write text to stdout
    SYS_EXIT   equ     60         ; terminate the program
    STDIN      equ     0          ; standard input
    STDOUT     equ     1          ; standard output
; --------------------------------
section .bss
    uinput_len equ     24         ; 24 bytes for user input
    uinput     resb    uinput_len ; buffer for user input
; --------------------------------
section .data
    prompt     db      "Please input some text: "
    prompt_len equ     $ - prompt
    text       db      10, "You wrote: "
    text_len   equ     $ - text
; --------------------------------
section .text
    global _start

_start:
    mov     rdx, prompt_len
    mov     rsi, prompt
    mov     rdi, STDOUT
    mov     rax, SYS_WRITE
    syscall

    mov     rdx, uinput_len
    mov     rsi, uinput
    mov     rdi, STDIN
    mov     rax, SYS_READ
    syscall                      ; -> RAX
    push    rax                  ; (1)

    mov     rdx, text_len
    mov     rsi, text
    mov     rdi, STDOUT
    mov     rax, SYS_WRITE
    syscall

    pop     rdx                  ; (1)
    mov     rsi, uinput
    mov     rdi, STDOUT
    mov     rax, SYS_WRITE
    syscall

    xor     edi, edi             ; successful exit
    mov     rax, SYS_EXIT
    syscall

Instead of mov rdi, 0, I've used xor edi, edi which is shorter and faster and leaves the same result (0) in the RDI register.
I always prefer to write the function number directly above the syscall instruction. I find this clearer. As a consequence I've also inversed the order of the other parameters, again for clarity.

You can learn a lot about 64-bit Linux programming from the .PDF that you can download here
It provides good examples that deal with console input and console output and more.


Android app with config and customization classes I have an Android app with three different Activity classes. Each of the Activities uses and changes config and user-customization data stored in a wrapped SharedPreferences. The code could stand some refactoring, so I'm thinking that rather than have each Activity instantiate the SharedPreferences by calling PreferenceManager.getDefault... in their constructors, I can make a 'base' abstract Activity to serve as a parent class and then have each of the Activities subclass the abstract Activity and wrap the SharedPreferences in an inner class of the abstract Activity, like so:

public abstract class AbstractBaseActivity extends AppCompatActivity
{
    private static AppPrefs instance;

    private interface PrefKeys
    {
        String KEY_BOOL_SETUP =  "KEY_BOOL_SETUP";
        String KEY_STR_USER_EMAIL = "KEY_STR_USER_EMAIL";
        String KEY_LONG_LAST_EMAIL_SENT = "KEY_LONG_LAST_USE";
        //...remaining keys omitted...//
    }

public static class AppPrefs implements PrefKeys
{
  private SharedPreferences prefs;
  private String no_email;

        private AppPrefs(AbstractBaseActivity activity)
        {
            prefs = PreferenceManager.getDefaultSharedPreferences(activity);
            no_email = activity.getString(R.string.no_email);
        }

        public static AppPrefs getInstance(AbstractBaseActivity activity)
        {
            if(instance == null)
            {
                instance = new AppPrefs(activity);
            }
            return instance;
        }

        public String getUserEmail()
        {
          return prefs.getString(AppPrefs.KEY_STR_USER_EMAIL, no_email);
        }

        public boolean setUserEmail(String email)
        {
          return prefs.edit().putString(AppPrefs.KEY_STR_USER_EMAIL, email).commit();
        }

         //... getter/setter methods to modify the SharedPreferences ...//
    }
}

I like this approach because it simplifies getting the SharedPreferences. I just call AppPrefs.getInstance(this) from within the Activities or AppPrefs.getInstance(getActivity()) from within a Fragment. Currently, my SharedPreferences are wrapped in a custom Object -- AppPrefs -- which uses obviously named getters and setters to interact with my SharedPreference data without actually having to muck about with key-value pairs and remembering to call commit() when changing data, but it is a stand-alone class. It seems to make sense to make the AppPrefs class an inner class of the AbstractBaseActivity class, but I'd welcome any criticisms of the approach.

I'm also uncertain about how I'm dealing with getting the AppPrefs object -- is there a better way to implement instantiation?

What are the downsides I'm not seeing?

Public class, private Interface?

I'm pretty confused by this setup.
The meaning is always trying working towards the most highest object( this case interface) possible.
I'm thinking that you created this setup so having the keys as constants.
But there is nothing wrong to have those keys as private static final String in your class self.

Java singleton.

I can't repeat it enough but if you want a singleton, please think enum in stead of private constructor.
Your singleton isn't thread safe or Serializable in this case, while if you use enum it is, and it's even easier to created it.

Abstract class root of all evil?

While I do use abstract classes, I'm always thinking also about putting the abstract methods in an interface and use that interface in the constructor.
This has multiple advantages of the abstract class.
First of all, you can create the base class final so no unexpected behavior could be implemented later on.
Then the interface you can swap at runtime (don't forget a check on null) and your class behave different but still in the correct way.

As I don't see the whole picture here I can't say it's good or not in this case.
But still, think about it, see the advantages/disadvantages and make your conclusion if it's good in this case or not.


Singletons with eager and lazy instantiation I've found more than one implementation of the Singleton Pattern in Java (more like 5-6). I want to know if both of the following snippets of code (written by me) are valid implementations of Singleton Pattern in Java.

//Eagerly intialization
public final class LoggerEagerSingleton {

private static final LoggerEagerSingleton logger = new LoggerEagerSingleton();

private LoggerEagerSingleton() {
}

public static LoggerEagerSingleton getInstance() {
    return logger;
}
}

//Lazy Initialization
public final class LoggerLazySingleton {

private LoggerLazySingleton() {
}

public static LoggerLazySingleton getInstance() {
    return LazyHelper.logger;
}

private static class LazyHelper {
    private static final LoggerLazySingleton logger = new LoggerLazySingleton();
}
}

The singleton pattern guarantees that only one instance of a class will be created into the application context.

So, in your code I see that:

You are create a static final class.
You declare the selfclass propertie as final: private static final LoggerEagerSingleton logger

Both of these last points are not necessary because the singleton pattern guarantees that just one will be create and nobody could be modify the unique instance of the singleton class.

The singleton patterns suggest the following rules:

A private construct.
A Static private property of the self class.
A Static method to get the unique instance, where it create the instance if it has not been created.

    public class SimpleSingleton {

        private SimpleSingleton logger;

        private SimpleSingleton() {

        }

        public static SimpleSingleton getInstance() {

            if(logger == null)
                logger = new LoggerEagerSingleton();

            return logger;
        }
    }

As you can see the property logger is create in the static method getInstance() and no other methods give access or alter this property, and the method give you a way to get the unique instance of the class.

So if you apply these rules, your classes could be rewritten like:

public class LoggerEagerSingleton {

    private static LoggerEagerSingleton logger = null;

    private LoggerEagerSingleton() {
    }

    public static LoggerEagerSingleton getInstance() {
        if(logger == null)
            logger = new LoggerEagerSingleton();
        return logger;
    }

    //TODO: All the remain methods
    public void method A()
    {

    }

}

And it could be used as:

LoggerEagerSingleton ref = LoggerEagerSingleton.getInstance();
ref.void methodA();

But remember, a patter is a generic design that has been used to solve some issues that are common in several projects. So you can implement the pattern as it is presented or make some modifications without altering the essence of the pattern.


Making PHP composable and look functional I've found myself on numerous occasions needing to with large text-files, where joining this on that, and filtering this out on arbitrary conditions.

The first couple of times, this turned into a loop in a loop in a loop mess which was indented more times than I care to either remember or admit.

More recently, I've attempted to make something that looks and behaves more "functional". Below is the basic class and a simple example.

I'm primarily concerned with extensibility. I'm not sure the approach I use will lend itself to extensibility in the future. I though, for example, about trying to stream results to disc, and I'm not sure that's doable at all without a fundamental change. Similarly it would be useful to add a simple ->not() for negating a filter, which I also don't see easily doable here.

A secondary concern is performance. PHP is perhaps not the best choice for a functional style. I'm guessing the performance of all these function calls isn't fantastic. Anything obvious I can do to improve it? If "real" performance is a requirement, the obvious suggestion might just to be to not use PHP, but if there are cheap ways to stretch this further, it would be nice.

Code:

class Functional {
  protected $result;
  protected $extra_data;

  public function __construct($array = NULL) {
    $this->result = $array;
  }

  public function reduce($callback, $i = NULL) {
    if ($i === NULL) {
      $i = $this->result;
    }

    $result = array_reduce($i, $callback);
    $this->result = $result;
    return $this;
  }

  public function walk($callback, $i = NULL) {
    if ($i === NULL) {
      $i = $this->result;
    }

    array_walk($i, $callback);
    $this->result = $i;
    return $this;
  }

  public function filter($callback, $i = NULL) {
    if ($i === NULL) {
      $i = $this->result;
    }

    $this->result = array_filter($i, $callback);
    return $this;
  }

  public function result() {
    return $this->result;
  }

  public function gather($callback, $key) {
    $this->extra[$key] = $callback($this->result)
    return $this;
  }

  public function extra($key) {
    return $this->extra[$key];
  }
}

Example usage:

$a = [1, 2, 3, 4];

$f = new Functional($a);
$result = $f->walk(function(&$i) { return $i *= 2; })
  ->reduce(function($i, $j) { return $i + $j; })
  ->result();

var_dump($result);

int(20)

I propose you a more compact version with code duplication removal:

You can compact all of your call with an apply method, as shown in the following code:

<?php

class Functional
{
    protected $result;
    protected $extra_data;

    public function __construct($array = NULL)
    {
        $this->result = $array;
    }

    protected function apply($fn, $callback, $value)
    {
        $input = (is_null($value)) ? $this->result : $value;
        call_user_func($fn, $input, $callback);
        return $this;
    }

    public function reduce($callback, $i = NULL)
    {
        return $this->apply('array_reduce', $callback, $i);
    }

    public function walk($callback, $i = NULL)
    {
        return $this->apply('array_walk', $callback, $i);
    }

    public function filter($callback, $i = NULL)
    {
        return $this->apply('array_filter', $callback, $i);
    }

    public function result()
    {
        return $this->result;
    }

    public function gather($callback, $key)
    {
        $this->extra[$key] = $callback($this->result);
        return $this;
    }

    public function extra($key)
    {
        return $this->extra[$key];
    }
}


implementation of Message queue I have implemented a message queue for inter thread communication just like POSIX message queue. I want to know whether there is any scope for performance improvement and also any bugs which need to be corrected. This is my code:
#include <iostream>
#include <cstring>
#include <list>
#include <vector>
#include <thread>
#include <mutex>
#include <condition_variable>

std::mutex mu;
std::condition_variable cv;
std::list<std::vector<char>> msg_queue;

void recv() {
    char buff[2048];

    while(true) {
        std::unique_lock<std::mutex> ul { mu };

        while(msg_queue.empty())
            cv.wait(ul);

        std::vector<char>& queue_msg = msg_queue.front();
        if(queue_msg.size() < sizeof(buff)) {
            std::memcpy(buff, queue_msg.data(), queue_msg.size());
            buff[queue_msg.size()] = '\0';
        }

        msg_queue.pop_front();

        ul.unlock();

        std::cout << buff << std::endl;
    }
}

int32_t main() {
    const char *data = "Hello world!";
    std::thread th(recv);

    while(true) {
        mu.lock();

        msg_queue.emplace_back(std::vector<char>(data, data + strlen(data)));
        cv.notify_one();

        mu.unlock();

        std::this_thread::sleep_for(std::chrono::seconds(1));
    }

    th.join();

    return EXIT_SUCCESS;
}

Since you’re primarily interested in efficiency, that’s what I’m going to focus on in my review.
However, first I do need to point out that int32_t main() is not legal C++. I’m mildly surprised it even compiles. There are only two legal forms of main():

int main(); and
int main(int, char*[]).

I should also mention that a message queue is probably better done as a class, with the mutex and other implementation details as private data members, rather than as global variables, and details like notifying the condition variable handled automatically. But that isn’t a performance issue, just a design one.
std::list<std::vector<char>> msg_queue;

Using a std::list for the actual queue is an odd choice. It’s not wrong; it’ll work perfectly fine. And std::list is not the most performant container, but its inefficiencies will be absolutely dwarfed by all the thread context switching and locking/unlocking.
Normally for a message queue like this you’d use a std::vector, or maybe a std::deque. Or sometimes you’d actually use a std::array (or a std::vector with a fixed size), and either treat it like a ring buffer—so if there are too many unhandled messages, older messages get lost—or block any new messages until some older messages are handled.
As I said, std::list isn’t wrong. It’ll work. I just don’t see any benefits to using it, because at least in the example code, you don’t really benefit from iterator stability (which is the primary reason to prefer a std::list), and you’ll pay a lot more for allocations and cache misses.
char buff[2048];

Is there a reason you use a fixed-size buffer? I don’t see any performance gains from it, and you’ll just lose any messages longer than 2k.
I’ll offer alternative suggestions later.
while(true) {

You have a bug in your program in that the loops are infinite. Infinite loops are UB. (Correction: Infinite loops are UB if and only if they have no observable side effects. These loops have side effects (like printing to standard out, and locking mutexes). However, there is no portable way to end the program, and—more importantly (because it’s the subject of the review)—the message queue. If you tried to put this message queue as-is in an existing program, like the example one, it will never end; it will deadlock while waiting for the thread to join, while the thread loops and waits on the condition variable.)
What you really need is a way for the recv() function to realize the program’s ending. For example, you could use a message "done" as a signal to wind things up:
// in recv()
while (true)
{
    // ... [snip] ...

    if (buf == "done")
        break;
}

// in main()
for (auto n = 0; n < 10000; ++n) // instead of while (true)
{
    // ... [snip] ...
}

// artificial scope
{
    auto lock = std::scoped_lock(mu);
    msg_queue.emplace_back({'d', 'o', 'n', 'e'});
}

cv.notify_one();

th.join();

It doesn’t need to be a special message to signal the end, but if you use something else, like a flag, then you might need to be sure you finish handling all the messages before ending the recv() loop, and you need to consider what happens if the flag is set while the queue is empty (and the condition variable is waiting on it to not be empty).
while(msg_queue.empty())
    cv.wait(ul);

Rather than a manual loop like this, it’s cleaner and less likely to cause problems if you use the other form of wait() that takes a lambda:
cv.wait(ul, [] { return not msg_queue.empty(); });

This removes the temptation for some later code monkey to fool with the loop in some way. Raw loops are a code smell, because you should generally prefer algorithms. They’re fine in the use cases here, but you never know what some future coder might think to do. Also, this reads much clearer, even to someone who doesn’t understand condition variables (who might ask, “why a loop and a call to a wait function?”).
std::vector<char>& queue_msg = msg_queue.front();
if(queue_msg.size() < sizeof(buff)) {
    std::memcpy(buff, queue_msg.data(), queue_msg.size());
    buff[queue_msg.size()] = '\0';
}

msg_queue.pop_front();

Okay, here’s where the biggest inefficiencies lie. I don’t see a reason—at least in this code—why you copy the message from the message queue into a buffer, then delete the message from the queue. Why not just take the message right from the queue and use it?
In other words, why not do this:
void recv()
{
    while (true)
    {
        std::unique_lock<std::mutex> ul { mu };
        cv.wait(ul, [] { return not msg_queue.empty(); });

        // *TAKE* the message from the queue. This will be *LIGHTNING* quick;
        // possibly *thousands* of times faster than copying the message into a
        // buffer.
        auto queue_msg = std::move(msg_queue.front());

        // Remove it from the list.
        msg_queue.pop_front();

        // No need to hold the lock anymore.
        ul.unlock();

        // Now you have the message as queue_msg, and you can do whatever you want
        // with it. For example, print it:
        std::cout.write(queue_msg.data(), queue_msg.size());
        std::cout << '\n';
    }
}

But that’s just the tip of the performance iceberg. It’s very wasteful to do all that locking and so on for ONE message at a time. What if there are several messages in the queue? Why not handle them all at once?
For example:
void recv()
{
    while (true)
    {
        std::unique_lock<std::mutex> ul { mu };
        cv.wait(ul, [] { return not msg_queue.empty(); });

        // Take the ENTIRE QUEUE. Even if there are multiple messages, this could
        // still be thousands of times faster than copying a SINGLE message into a
        // buffer, and that's not even taking into account the gains you get from
        // avoiding multiple locking and unlocking and waiting cycles.
        auto messages = std::vector<std::vector<char>>{};
        messages.resize(msg_queue.size()); // make space for the messages
        std::move(msg_queue.begin(), msg_queue.end(), messages.begin()); // move them all out of the queue
        msg_queue.clear(); // clear the queue of the (now empty, moved-from) messages

        // No need for the lock anymore.
        ul.unlock();

        // Now we can handle all the messages at our leisure.
        std::for_each(messages.begin(), messages.end(),
            [](auto&& message)
            {
                std::cout.write(message.data(), message.size());
                std::cout << '\n';
            });
    }
}

As an optimization, you could also move messages out of the loop, and call messages.clear() at the end of the loop. The advantage of this is that it will mean that messages.resize() will almost never need to allocate because you can reuse the memory. That means that that entire block of code in the middle of the function above will amount to a few measly pointer copies… which will be FAST… far faster than copying entire strings, even with std::memcpy(). And that in turn means much less time spent with the mutex locked, which means you should be able to get a MUCH higher throughput for your messages.


Hangman game with hard-coded word or phrase I feel that this implementation can still be simplified.  I've tried to keep variable scopes low and have used as much of the standard library as I could.

It currently works with a hard-coded word or phrase, but I eventually plan on having it work with random words or phrases from a file.

My main concern is in regards to my functions.  Am I using too few or too many of them?  I still feel that main() is doing a little too much.

Note: I have not tagged this as c++11 because there's no utilization of it here, but I am open to any recommendations from C++11.

#include <iostream>
#include <string>

bool isCorrectGuess(const char guess, std::string const& word)
{
    return (word.find(guess) != std::string::npos);
}

std::string updateWrongGuesses(const char guess, std::string const& word, std::string wrongGuesses)
{
    return wrongGuesses += guess;
}

std::string updateCorrectGuesses(const char guess, std::string const& word, std::string correctGuesses)
{
    for (std::string::size_type i = 0; i != word.size(); ++i)
    {
        if (word[i] == guess)
        {
            correctGuesses[i] = guess;
        }
    }

    return correctGuesses;
}

void displayStatus(const unsigned int triesLeft, std::string const& guessedWord, std::string const& wrongGuesses)
{
    std::cout << "\n\nRemaining Tries: " << triesLeft;
    std::cout << "\nWrong Guesses  : " << wrongGuesses;
    std::cout << "\n\n" << guessedWord << "\n\n";
}

int main()
{
    const std::string word = "rabbit rabbit";

    std::string guessedWord;

    // set up placeholders for word to be guessed
    for (std::string::size_type i = 0; i != word.size(); ++i)
    {
        guessedWord += (isspace(word[i])) ? ' ' : '*';
    }

    unsigned int triesLeft = 8;
    std::string wrongGuesses;

    displayStatus(triesLeft, guessedWord, wrongGuesses);

    std::string attemptedGuesses;

    while (triesLeft > 0 && guessedWord != word)
    {
        std::cout << "\nGuess a letter: ";
        char guess;
        std::cin >> guess;
        guess = tolower(guess);
        attemptedGuesses += guess;

        if (isCorrectGuess(guess, word))
        {
            guessedWord = updateCorrectGuesses(guess, word, guessedWord);
        }
        else
        {
            wrongGuesses = updateWrongGuesses(guess, word, wrongGuesses);
            triesLeft--;
        }

        displayStatus(triesLeft, guessedWord, wrongGuesses);
    }

    if (guessedWord == word)
    {
        std::cout << "\n\nYou've guessed the word/phrase!\n\n";
    }
    else
    {
        std::cout << "\n\nYou've been hanged...\n\n";
        std::cout << "The word/phrase is: " << word;
    }
}

The first thing that nearly jumps out at me is this:

std::string updateWrongGuesses(const char guess, std::string const& word, std::string wrongGuesses)
{
    return wrongGuesses += guess;
}

It's not at all clear why you pass word to this at all, since it doesn't use or modify it. Worse, it looks like it's actually more work to call the function than to re-implement what (little) the function does. At least to me:

wrongGuesses += guess;

...seems simpler and more readable than:

 wrongGuesses = updateWrongGuesses(guess, word, wrongGuesses);

I think I probably would use a class (or possibly two) in writing this code. My first attempt would be something on this general order:

#include <iostream>
#include <string>

class hangman{
    static const int max_wrong = 8;
    std::string word;
    std::string display;
    std::string wrong;
public:
    hangman(std::string const &word) : word(word) {
        for (char ch : word)
            display += isspace(ch) ? ' ' : '*';
    }

    void enter_guess(char ch) {
        bool correct = false;

        for (std::string::size_type i = 0; i != word.size(); ++i)
            if (word[i] == ch) {
                display[i] = ch;
                correct = true;
            }
        if (!correct)
            wrong += ch;
    }

    bool is_over() {
        return wrong.size() == max_wrong || display == word;
    }

    std::string final() {
        return "The word was: " + word + (display == word ? "\nYou win!" : "\nYou lose");
    }

    std::string status() {
        return "\nwrong guesses: " + wrong + "\nCurrent word: " + display;
    }
};

char get_guess() {
    std::cout << "Please enter your guess: ";
    char ch;
    std::cin >> ch;
    return ch;
}

int main()
{
    hangman game("rabbit rabbit");

    do {
        std::cout << game.status() << "\n";
        game.enter_guess(get_guess());
    } while (!game.is_over());
    std::cout << game.final();
}

As far as how much of your code is in main: I try not to treat main any differently from any other function. IMO, you shouldn't strive for main to be as small, or large, or anything else in particular as possible. Rather, main should be a function just about like any other. It should have accomplish something useful. Like any other function, it should have a clearly defined level of abstraction at which it operates, and (for the most part) it should try to stick to it.

As a natural consequence of being bigger, a bigger program will typically have more layers between main and the lowest levels of manipulation, so in a large program it'll typically have little code beyond creating objects and/or calling functions. I wouldn't consider that a particularly worthy goal in itself though. If it fits well with the size and type of program you're writing great--but there's not necessarily a good reason to force it if it doesn't happen to fit or work out well that way.


Multi-line input reader I wrote two functions to read multi-line user input.

from os import linesep

def input_lines(prompt=None):
    """Yields input lines from user until EOFError is raised."""

    while True:
        try:
            yield input() if prompt is None else input(prompt)
        except EOFError:
            break
        else:
            prompt = None   # Only display prompt while reading first line.

def multiline_input(prompt=None):
    """Reads a multi-line input from the user."""

    return linesep.join(input_lines(prompt=prompt))

if __name__ == '__main__':
    print(multiline_input('Gimme your lines: '))

Is there something I can improve by maybe invoking some library method I overlooked?

There's not much code, however, I'd change input_lines in the following ways:

  Only display prompt while reading first line.

If you're only going to do something once, then just add it as an initial return.
It's simpler IMO to have a try that wraps the entire code of the function, this way you know it will keep doing something until EOFError is raised. Rather than possibly do something on each iteration.

As noted in the comments by OP, you can instead use contextlib.suppress, which can make the code easier to read.
You may want to use yield from and iter, if you think that's cleaner to read than a while loop.

This can result in either of:

def input_lines(prompt=None):
    try:
        if prompt is not None:
            yield input(prompt)
        while True:
            yield input()
    except EOFError:
        pass

from contextlib import suppress

def input_lines(prompt=None):
    with suppress(EOFError):
        if prompt is not None:
            yield input(prompt)
        yield from iter(input, None)


Assigning variables in a block It's common pattern when you need to assign a variable inside a block

def query(sql)
  logger.debug "Db: Executing query #{sql}"
  result = nil
  ts = Benchmark.realtime do
    result = @db.exec sql
  end
  logger.debug "Db: Query completed in #{ts}"
  result
end

Is there a more elegant way to rewrite this snippet?

I am afraid your code cannot be simplified further due to the imperative nature of Benchmark.realtime (it would have been better if it also returned the result of the block). Anyway, I think the best solution is to abstract it and write a wrapper method. For example:

def benchmark(msg)
  logger.debug(msg)
  result = nil
  ts = Benchmark.realtime(&block) { result = yield }
  logger.debug("Completed in #{ts} seconds")
  result
end

def query(sql)
  benchmark("DB: Executing query #{sql}") do
    @db.exec(sql)
  end
end

Now, if your question was on a more generic plane, let's say, that you have lots of methods that work like realtime which force you write a lot of boilerplate, I'd propose this generic Object#capture wrapper:

class Object
  def capture(method, *args)
    result2 = nil
    result1 = send(method, *args) { |*bargs| result2 = yield(*bargs) }
    [result1, result2]
  end
end

realtime_output, block_output = Benchmark.capture(:realtime) do
  "my output"
end #=> [7.263e-06, "my output"]


Simple Grid Builder for Future Maze Applications I'm new to Clojure and decided to read the book, "Mazes for Programmers". My first task, without reading any of the book, was to think of how I would create a maze. This is my first result and I'd like good, honest and even scathing critiques:

(defrecord Cell [visited? valid-dirs open-dirs])
(defrecord Maze [cells])

(defn create-cell
      "Creates an unvisited cell with no valid directions."
      ([] (Cell. false #{} #{}))
      ([valid-dirs] (Cell. false valid-dirs #{})))

(defn create-corner-cell [corner]
      "Creates a corner cell which has two valid directions."
      (case corner
            (:top-left) (create-cell #{:east :south})
            (:top-right) (create-cell #{:west :south})
            (:bottom-left) (create-cell #{:east :north})
            (:bottom-right) (create-cell #{:west :north})))

(defn create-wall-cell [side]
      "Creates a wall cell which has three valid directions."
      (case side
            (:top) (create-cell #{:west :south :east})
            (:left) (create-cell #{:north :south :east})
            (:bottom) (create-cell #{:west :north :east})
            (:right) (create-cell #{:north :south :west})))

(defn create-unbound-cell []
      "Creates a cell that has no invalid directions."
      (create-cell #{:north :east :south :west}))

(defn visited? [cell]
      "Determines whether or not a cell has been visisted."
      (true? (:visited? cell)))

(defn corner-cell? [index size]
  "Determines whether the index is a corner cell of #{top-left top-right bottom-left bottom-right} in a grid
  of size*size dimensions. Returns both whether or not the index is a corner cell, but
  also which corner of the grid it's on."
      (assert (not= 0 size))
      (cond
            (= index 0) [true :top-left]
            (= index (dec size)) [true :top-right]
            (= index (- (* size size) size)) [true :bottom-left]
            (= index (dec (* size size))) [true :bottom-right]
            :else [false]))

(defn wall-cell? [index size]
  "Determines whether the index is a wall cell of #{top left right bottom} in a grid
   of size*size dimensions. Returns both whether or not the index is a wall cell, but
   also which side of the grid it's on."
      (assert (not= 0 size))
  (let [contains-index (partial seq-contains index)
        top-row (range 0 size)
        bottom-row (range (- (* size size) size) (* size size))
        left-col (take-while (partial > (* size size)) (iterate #(+ % size) 0))
        right-col (take-while (partial > (* size size)) (iterate #(+ % size) (dec size)))]
    (cond
      (contains-index top-row) [true :top]
      (contains-index bottom-row) [true :bottom]
      (contains-index left-col) [true :left]
      (contains-index right-col) [true :right])))

(def m-corner-cell? (memoize corner-cell?))
(def m-wall-cell? (memoize wall-cell?))

(defn create-maze-cell [index size]
      "Creates a cell depending on its location in the maze."
      (let [[is-corner-cell corner] (m-corner-cell? index size)
            [is-wall-cell side] (m-wall-cell? index size)]
            (cond
                  (true? is-corner-cell) (create-corner-cell corner)
                  (true? is-wall-cell) (create-wall-cell side)
                  :else (create-unbound-cell))))

(defn create-maze [size]
      "Create a maze of size*size dimensions."
      (if (= 1 size) (create-cell)
        (let [total-cells (* size size)]
              (map-indexed
                    (fn [idx _] (create-maze-cell idx size))
                    (range total-cells)))))

Docstrings go BEFORE the arglist

(defn my-function
  "with aweseome documentation"
  [all the args]
  :insert-code-here)

Visited really shouldn't be a property of a cell.  Instead of mutating your cell, you should maintain a set of visited nodes.  Maybe you want to run two different path finding algorithms over the same data?

Your case corners are a little strange.  Whenever you see this, you should probably think map:

(def corner-cells
  {:top-left #{:east :south}
   :top-right #{:west :south}
   :bottom-left #{:east :north}
   :bottom-right #{:west :north}})

(defn create-corner-cell
  "Creates a corner cell which has two valid directions."
  [corner]
  (create-cell (corner-cells corner)))

Do you really need :top-left and all those combinations?  Consider instead:

(def open-block #{:north :east :south :west})
(def wall-for-side
  {:top :north
   :bottom :south
   :left :west
   :right :east})

(defn add-walls [block & sides]
  (clojure.set/difference block
                          (map wall-for-side sides)))

mazer.core> (add-walls open-block :top)
#{:west :south :east}
mazer.core> (add-walls open-block :top :left)
#{:south :east}
mazer.core> (add-walls open-block :bottom :right)
#{:west :north}

A function like corner-cell? doesn't need to return a list like that.
 Simply return nil for false and the direction for true. You can use (if-let [dir (corner-cell? ...)] :something-with-dir :something-without-dir) to differentiate.  And, since this isn't strictly a predicate, you really shouldn't name it with a question mark.  Maybe corner-cell-type.

I wouldn't use memoization here. You almost never need to memoize. If you REALLY think you need to cache this, that's the kind of information that probably would go on the cell.  But I think you should just convert the index to an x/y (math is fast) and compare that.


Parsing ODataQueryOptions to Expression> I am following up on this answer for a scenario that I am currently working on. Like the OP, I am too concerned about the longevity of the code.

public IQueryable<TEntity> EmptyEnumerable()
{
    return Enumerable.Empty<TEntity>().AsQueryable();
}

private Expression<Func<TEntity, bool>> GetFilterExpression(FilterQueryOption filter)
{
    var enumerable = this.EmptyEnumerable();
    var param = Expression.Parameter(typeof(TEntity));
    if(filter != null)
    {
        enumerable = (IQueryable<TEntity>)filter.ApplyTo(enumerable, new ODataQuerySettings());

        var mce = enumerable.Expression as MethodCallExpression;
        if(mce != null)
        {
            var quote = mce.Arguments[1] as UnaryExpression;
            if(quote != null)
            {
                return quote.Operand as Expression<Func<TEntity, bool>>;
            }
        }
    }
    return Expression.Lambda<Func<TEntity, bool>>(Expression.Constant(true), param);
}

My questions are:

Is there anything that I should be checking, which I am not already checking?
Is returning a truth expression a sensible default?

Any refactorings/improvements are welcome.

I guess that you're assuming that the MethodCall expression should be a call to Queryable.Where(...) ?

You should check that explicitly via something like :

// yea, that's ugly. But that's for the demo.
//   I'm pretty sure you'll find the proper way to get the method info with your favorite method
static readonly MethodInfo QueryableWhere = ((MethodCallExpression)(
   (Expression<Func<IQueryable<string>>>)
    (() => ((IQueryable<string>)null).Where(x => true)))
   .Body.Unquote())
   .Method.GetGenericMethodDefinition();

and in your code

 if(!mce.Method.IsGeneric || mce.Method.GetGenericMethodDefinition() != QueryableWhere)
     throw an exception;

Unary expressions are not necessarily quotes! (a negation is an unary, for instance). Thus, you want to check mce.Arguments[1].NodeType instead of checking its type.

Moreover, quote un-quoting is quite common, so it can be useful to replace.

var quote = mce.Arguments[1] as UnaryExpression;
if(quote != null)
{
    return quote.Operand as Expression<Func<TEntity, bool>>;
}

by

return mce.Arguments[1].Unquote();

with

public static Expression Unquote(this Expression quote)
{
    if(quote.NodeType == ExpressionType.Quote)
        return ((UnaryExpression)quote).Operand.Unquote();
    return quote;
}

Returning a lambda is OK in my opinion. No problem with that IF AND ONLY IF there is no chance that the enclosed predicate does not contains any reference to an outer parameter (or variable), which is the case here, so it's okay.


Reverse doubly linked list Are there any corner cases missing here?

class Node
{
    int data;
    Node next;
    Node prev;
}

Node reverseDLL(Node head)
{
    if(head == null || head.next == null)
       return head;

    Node previousNode = head.prev;
    Node currentNode = head;
    Node nextNode;

    while(currentNode!=null)
    {
       nextNode = currentNode.next;

       currentNode.next = previousNode;
       currentNode.prev = nextNode;

       previousNode = currentNode;
       currentNode = nextNode;
    }

    head = previousNode;
    return  head;
}

If we are allowed to pass the tail node to the reversal method (Node reverseDLL(Node head, Node tail)), we can perform the reversal by reversing the data integer fields instead of restructuring the list. All in all, I had this in mind:

import java.util.ArrayList;
import java.util.List;

class Node {
    int data;
    Node next;
    Node prev;

    Node(int data) {
        this.data = data;
    }
}

public class ReverseDLL {

    static void reverseDLL2(Node head, Node tail) {
        Node left = head;
        Node right = tail;

        while (true) {
            if (left == right) {
                return;
            }

            int tmp = left.data;
            left.data = right.data;
            right.data = tmp;

            if (left.next == right) {
                return;
            }

            left = left.next;
            right = right.prev;
        }
    }

    static void printList(Node head) {
        for (Node current = head; current != null; current = current.next) {
            System.out.print(current.data);
            System.out.print(" ");
        }
    }

    public static void main(String[] args) {
        List<Node> nodes = new ArrayList<>();

        for (int i = 0; i < 10; ++i) {
            nodes.add(new Node(i));
        }

        for (int i = 0; i < 9; ++i) {
            nodes.get(i).next = nodes.get(i + 1);
        }

        for (int i = 1; i < 10; ++i) {
            nodes.get(i).prev = nodes.get(i - 1);
        }

        printList(nodes.get(0));
        reverseDLL2(nodes.get(0), nodes.get(9));
        System.out.println();
        printList(nodes.get(0));
        reverseDLL2(null, null);
    }
}

Hope that helps.


PHP array with array value into multiple arrays with same key Very difficult to explain in a sentence sorry so here is example.
I have this array:
$setParams = [
    'repeat_delay' => [23, 1],
    'repeat_type' => ['Hour', 'Day'],
];

that I want to turn into this
$wanted = [
    [
        'repeat_delay' => 23,
        'repeat_type' => 'Hour'
    ],
    [
        'repeat_delay' => 1,
        'repeat_type' => 'Day'
    ]
];

I have it working but wanting to know if there is a cleaner way, or even another way.
$newArray = [];
$setKeys = array_keys($setParams);
$setValues = array_values($setParams);

$isSetValueArray = array_filter($setValues,
    static function ($setValue) {
        return is_array($setValue);
    });

if (!empty($isSetValueArray)) {
    foreach ($setKeys as $keyCount => $key) {
        foreach ($setValues as $valueCount => $value) {
            if (is_array($value)) {
                foreach ($value as $k => $v) {
                    if ($keyCount === $valueCount) {
                        $newArray[$k][$key] = $v;
                    }
                }
            }
        }
    }
}

Some more information to help others. I am sorry I didn't provide sooner as I know context is everything.
Here is a link to my snippet with more info and complete example.
https://gitlab.mrwilde.solutions/-/snippets/18

Assuming you have the same number of items for each key, here are two alternative approaches:
Foreach:
<?php
$setParams = [
    'repeat_delay' => [23, 1],
    'repeat_type'  => ['Hour', 'Day'],
];

$i = 0;
foreach($setParams['repeat_delay'] as $k => $v) {
    $result[$i]['repeat_delay'] = $v;
    $result[$i]['repeat_type']  = $setParams['repeat_type'][$k];
    $i++;
}

Array map:
$result = array_map(null, ...array_values($setParams));
$keys   = array_keys($setParams);
$result = array_map(function($v) use ($keys) { return array_combine($keys, $v);}, $result);

Both result in $result holding the following:
array (
  0 =>
  array (
    'repeat_delay' => 23,
    'repeat_type' => 'Hour',
  ),
  1 =>
  array (
    'repeat_delay' => 1,
    'repeat_type' => 'Day',
  ),
)

You could rewrite the last array_map with the array_combine to:
$result = array_map(fn($v) => array_combine($keys, $v), $result);

Using a short arrow function for brevity.


Session handling using Python Requests client I'm using this code to login to an experiment login system created by me for this purpose.

import requests
import re

def get_page_data(regex, req):
    match = re.compile(regex).search(req.text)
    if match != None:
        return match.group(1)
    return 'no match found on {}'.format(regex)

def print_req_data(req, req_name):
    print('{} status code: {}'.format(req_name, req.status_code))
    print('{} title: {}'.format(req_name, get_page_data('<title>(.*?)</title>', req)))
    print('{} content:\n{}'.format(req_name, req.text))

form_url = 'http://migueldvl.com/heya/login/'
post_url = 'http://migueldvl.com/heya/login/process.php'
headers = {'User-agent' : 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:7.0.1) Gecko/20100101 Firefox/7.0.1'}

with requests.Session() as s:
    # get form page, so the cookies are set (form token)
    logged_out_req = s.get(form_url, headers=headers)
    print_req_data(logged_out_req, 'Logged out request')
    title_logout = get_page_data('<title>(.*?)</title>', logged_out_req)
    # getting the form token
    token = get_page_data('<input type="hidden" name="token" value="(.*?)">', logged_out_req)
    login_data = {'password' : 'password', 'username' : 'miguel', 'token': token}
    # posting the data to the post url
    post_req = s.post(post_url, data=login_data, headers=headers)
    print_req_data(post_req, 'Post request (redirect page)')
    check_post_title = get_page_data('<title>(.*?)</title>', post_req)
    # comparing the titles (logged out title with redirect page title) so i see if login success
    if(check_post_title != title_logout):
        print('SUCCESS\n[+] Checking if still logged in...')
        # going to see if i'm still loggin, see if our loggedin session is permanent
        logged_req = s.get(form_url, headers=headers)
        print_req_data(logged_req, 'Check if still logged request')
        title_check = get_page_data('<title>(.*?)</title>', logged_req)
        if(title_check == check_post_title):
            print('You are still loogedin')
        else:
            print('Not loggeding anymore')
    else:
        print('FAIL LOGIN')
        print_req_data(post_req, 'Post request (redirect page)')

I was wondering if there is a better way to do this, especially with regard to:

Is there a better way to get the form token? I tried checking the headers,

logged_out_page = s.get(form_url, headers=headers)
print(logged_out_page.headers) # headers the server sent back to us
print(logged_out_page.request.headers) # headers we sent to the server

I know that session values are not stored on our machine (browser) unlike normal cookies, those are stored in the server so the real session value is not on the headers (just some id key to the server, so it can fetch the real data stored on it). But is there anyway besides using regex to extract the session value? PS: (session cookie produced with php, $_SESSION['token] = md5(time())).

Is there a way (besides doing a string comparison of the page title, or some other element on the page) to check if the login was successful?

Any other comments and suggestions are, of course, welcome. If you'd like to test this code (python3, using the links above, form_url and post_url) with the username miguel and the password password.

PS: Bear in mind that this code is based on the loggedin system mechanic that I created (the most usual, i guess). I don't wish any improvements on this code based on another

Please don't use regexes

Regular expressions are power tools. You can use them for many situations, but they tend to be inherently fragile. They're especially fragile for parsing HTML, in part because of how lenient most browsers will be. I'd recommend using a dedicated parser for it.  If you decided that since this is a page you control you don't need to be that careful that's fine, just be aware.

main

You shouldn't execute any code in the main body of your program - anything that should execute should be inside of an if __name__ == '__main__': block, and that should still be broken up into functions for ease of use.

get_page_data

Don't return a value if nothing was found; this would be very unexpected (and it would be annoying to check for, with a few edge cases). Instead raise an appropriate exception.

You also don't need to do if match != None:, just do if match:

class NoMatchFoundException(Exception): pass

def get_page_data(regex, req):
    match = re.compile(regex).search(req.text)
    if match:
        return match.group(1)
    raise NoMatchFoundException("No match found on {}".format(regex))

Favor good names over comments

Comments are almost always a sin. A necessary sin, but they mean that you haven't been able to sufficiently express yourself in code. Most of your comments would be great function names, e.g.

# get form page, so the cookies are set (form token)
logged_out_req = s.get(form_url, headers=headers)
print_req_data(logged_out_req, 'Logged out request')
title_logout = get_page_data('<title>(.*?)</title>', logged_out_req)

becomes

def get_form_page(session, url, headers):
    return session.get(url, headers=headers)

def get_page_title(page):
    return get_page_data('<title>(.*?)</title>', page)

logged_out_req = get_form_page(s, form_url, headers)
logout_page_title = get_page_title(logged_out_req)

Also you desperately need whitespace - add newlines liberally to group like sections of code. This is also a good way to see where you should create a new function.

I've left out the printing, because...

Don't print willy nilly

Most of the time you have a bunch of print statements they are intended to be debugging output; remove that before you actually use this. Occasionally if you need some sort of admin script or you need to know the status that's fine, but make sure you're printing out the bare minimum of what you need, and a log file is still going to be better.

I ended up with something like this. A lot of the functions end up not doing a whole lot, and if you don't think they're adding a lot of value remove them - the point of them, however, is to make it clear what each step is intended to do.  You'll note that I never do any error handling - I don't know what you actually want to be doing when the match fails.

import requests
import re

class NoMatchFoundException(Exception): pass

def get_page_data(regex, req):
    match = re.compile(regex).search(req.text)
    if match:
        return match.group(1)
    raise NoMatchFoundException("No match found on {}".format(regex))

def get_page_title(page):
    return get_page_data("<title>(.*?)</title>", page)

def get_page(session, url, headers, data=None):
    data = data if data else {}
    return session.get(url, headers=headers, data=data)

def print_req_data(req, req_name):
    print('{} status code: {}'.format(req_name, req.status_code))
    print('{} title: {}'.format(req_name, get_page_title(req)))
    print('{} content:\n{}'.format(req_name, req.text))

def get_form_token(page):
    return get_page_data('<input type="hidden" name="token" value="(.*?)">', page)

def logged_in(logout, logged):
    return get_page_title(logout) != get_page_title(logged)

def same_page(first, second):
    return get_page_title(first) == get_page_title(second)

def test_login(form_url, post_url, headers, username, password):
    with requests.Session() as s:
        logout_page = get_page(s, form_url, headers)

        form_token = get_form_token(logout_page)
        login_data = {'password' : password, 'username' : username 'token': form_token}

        logged_in_page = get_page(s, post_url, headers, login_data)

        if logged_in(logout_page, logged_in_page):
            logged_in_test = get_page(s, form_url, headers)

            if same_page(logged_in_test, logged_in_page):
                print('You are still logged-in')
            else:
                print('Not logged-in anymore')
        else:
            print('FAIL LOGIN')
            print_req_data(post_req, 'Post request (redirect page)')

if __name__ == '__main__':
    form_url = 'http://migueldvl.com/heya/login/'
    post_url = 'http://migueldvl.com/heya/login/process.php'
    headers = {'User-agent' : 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:7.0.1) Gecko/20100101 Firefox/7.0.1'}
    username = 'miguel'
    password = 'password'

    test_login(form_url, post_url, headers, username, password)


Separate models for rest API POST and GET I'm currently all about separating things where it makes sense. We've come to conclusion that small, slim, only-what-really-need models are the only one accepeatable.

How about this scenario?

public class EmailModel
{
  public int Id {get; set;}
  public string Subject {get; set;}
  public string Body {get; set;}
  public List<AttachmentModel> Attachments {get; set;}
  ...
}

To create a new email, user makes HTTP POST request:

POST api/v1/emails
{
  "subject": "email subject",
  "body": "some email content .. "
}

There is additional api for atttachments ofcourse api/v1/emails/attachments with POST to upload attachment and GET to retrieve email attachments.

GET email returns something like this:

GET api/v1/emails/1
{
  "id": "1",
  "subject": "email subject",
  "body": "some email content .. "
  "attachments": []
}

Is it better to have two models then?

public class CreateEmailModel // for http post
{
  public string Subject {get; set;}
  public string Body {get; set;}
}
public class EmailModel : CreateEmailModel // for http get
{
  public int Id {get; set;}
  public List<AttachmentModel> Attachments {get; set;}
}

Or is this complicating things for no reason, but still when another developer sees the code it's immediately clear what the model does. No model property is unneccessary.

I would not over-complicate the matter by splitting it up in several models. But if you want to stick to the choice of splitting it up, I would define an abstract base class and create a Create and Read model.

Because in the current situation I find the naming not 100% clear. Certainly because EmailModel (which sounds generic/basic) derives from CreateEmail (which is specific). It should be the other way around, a specific class should inherit from a generic/basic class.

Here's a rough implementation:

public abstract class EmailModel
{
    public string Subject {get; set;}
    public string Body {get; set;}
}

public class CreateEmailModel : EmailModel { }

public class ReadEmailModel : EmailModel
{
    public int Id {get; set;}
    public List<AttachmentModel> Attachments {get; set;}
}