Simple Encryption for Strings I have written this code for practice. Here we will enter the number by which we want to shift alphabets.
For eg.

  Enter by how many numbers you want to shift alphabets: 4

  Enter string: Programmer C++

  Encrypted String: Lnkcnwiian Y++

  Decryted String: Programmer C++

Review this code and suggest better C++11 and C++14 alternatives.

#include <iostream>
#include <string>
#include <vector>
#include <unordered_map>

class Cipher
{
    unsigned n;
    const std::vector<char> alphabetL { 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H',
      'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W',
      'X', 'Y', 'Z' };
    const std::vector<char> alphabetS { 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h',
      'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w',
      'x', 'y', 'z' };
    std::unordered_map<char, char> shiftByNL;
    std::unordered_map<char, char> shiftByNS;

  public:
    Cipher(unsigned n = 0);
    std::string encrypt(std::string&);
    std::string decrypt(std::string&);

  private:
    char byValue(const char); //Find by value
};

Cipher::Cipher(unsigned n)
{
    for (int i = 0; i < n; i++)
    {
        shiftByNL.insert(std::make_pair(alphabetL[i], alphabetL[(26 - n) + i]));
        shiftByNS.insert(std::make_pair(alphabetS[i], alphabetS[(26 - n) + i]));
    }
    for (int i = n; i < 26; i++)
    {
        shiftByNL.insert(std::make_pair(alphabetL[i], alphabetL[i - n]));
        shiftByNS.insert(std::make_pair(alphabetS[i], alphabetS[i - n]));
    }
}

char Cipher::byValue(const char value)
{
    for (auto itL = shiftByNL.begin(), itS = shiftByNS.begin();
          itL != shiftByNL.end(), itS != shiftByNS.end(); ++itL, ++itS)
    {
        if (itL->second == value)
        {
            return itL->first;
        }
        else if (itS->second == value)
        {
            return itS->first;
        }
    }
    //not an alphabet
    return value;
}

std::string Cipher::encrypt(std::string& original)
{
    std::string encrypted;
    std::size_t len = original.length();
    for (std::size_t i = 0; i < len; i++)
    {
        if (original[i] == ' ')
        {
            encrypted = encrypted + ' ';
        }
        else if (shiftByNS.find(original[i]) != shiftByNS.end())
        {
            auto search = shiftByNS.find(original[i]);
            encrypted = encrypted + search->second;
        }
        else if (shiftByNL.find(original[i]) != shiftByNL.end())
        {
            auto search = shiftByNL.find(original[i]);
            encrypted = encrypted + search->second;
        }
        else //not an alphabet
        {
            encrypted = encrypted + original[i];
        }
    }
    return encrypted;
}

std::string Cipher::decrypt(std::string& encrypted)
{
    std::string decrypted;
    std::size_t len = encrypted.length();
    for (std::size_t i = 0; i < len; i++)
    {
        if (encrypted[i] == ' ')
        {
            decrypted = decrypted + ' ';
        }
        else
        {
            char key = byValue(encrypted[i]);
            decrypted = decrypted + key;
        }
    }
    return decrypted;
}

int main()
{
    unsigned num;
    std::cout << "Enter by how many numbers you want to shift alphabets: ";
    std::cin >> num;
    std::cin.ignore();
    Cipher cipher(num);
    std::string str;
    std::cout << "Enter string: ";
    std::getline(std::cin, str);
    std::string res = cipher.encrypt(str);
    std::cout << "Encrypted String: ";
    std::cout << res << '\n';
    std::cout << "Decryted String: ";
    std::string decrypted = cipher.decrypt(res);
    std::cout << decrypted << '\n';
}

I don't think you should hard-code the length of alphabet as 26 here:
for (int i = n; i < 26; i++)

Doing so will make it harder to adapt this code to languages with a different number of letters.  Instead, prefer to use alphabetL.size() and alphabetS.size() (and probably use separate loops, as some languages don't have the same number of upper-case and lower-case letters).

It's probably advisable to test that n is less than the alphabet length (or to reduce it modulo length before using), and perhaps reject a zero shift (which is what we call a "weak key" for this cipher).

decrypt() is much slower than encrypt, because it does a search for every character.  What you really want is a pair of maps, but not lowercase and uppercase - instead you want forward and reverse (with both upper and lower in the same map).  In fact, I'd go further; allow negative values for the shift, and then we can create a separate Cipher object for the decryption.

Constructor that can accept a single argument should be explicit.  I don't think a default of 0 is a good choice (see my comment about weak keys).  Also, prefer initialization to assignment in constructors (GCC will warn, with -Weffc++).

Cipher::n is never initialized or used - we can drop that member.  Also, the two constant vectors don't need to be members - they are used only in the constructor, where they can be shared between all instances.  We can simplify further by making them string literals.

encrypt() and decrypt() both take a modifiable reference to their input, but it seems they should not modify the input string.  You could pass a reference to const string, but I think it might be easier to accept a copy by value, and modify the copy.
Also, they shouldn't be modifying the Cipher object, so they can be declared const.

Modified code
Here's what I came up with:
#include <algorithm>
#include <unordered_map>

class Cipher
{
    const std::unordered_map<char, char> map;

public:
    explicit Cipher(int n);
    std::string encrypt(std::string) const;

private:
    static std::unordered_map<char, char> make_map(int n);
};

Cipher::Cipher(int n)
    : map{make_map(n)}
{}

std::unordered_map<char, char> Cipher::make_map(int n)
{
    // helper function to give a positive value for a%b
    auto mod = [](int a, int b){ a %= b; return a < 0 ? a + b : a; };

    std::unordered_map<char, char> map;
    {
        static const char alphabet[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
        int const m = sizeof alphabet - 1; // subtract the final NUL
        for (int i = 0;  i < m;  ++i) {
            map.insert(std::make_pair(alphabet[i], alphabet[mod(i+n,m)]));
        }
    }
    {
        static const char alphabet[] = "abcdefghijklmnopqrstuvwxyz";
        int const m = sizeof alphabet - 1; // subtract the final NUL
        for (int i = 0; i < m; i++) {
            map.insert(std::make_pair(alphabet[i], alphabet[mod(i+n,m)]));
        }
    }

    return map;
}

std::string Cipher::encrypt(std::string s) const
{
    std::transform(s.begin(), s.end(), s.begin(),
                   [=](char c){ auto it = map.find(c); return it == map.end() ? c : it->second; });
    return s;
}

I slightly modified the test program to allow it to accept command-line arguments (easier for test scripts):
#include <cstdlib>
#include <iostream>
int main(int argc, char **argv)
{
    int num;
    if (argc > 1) {
        char* pos;
        num = std::strtol(argv[1], &pos, 0);
        if (*pos || pos == argv[1]) {
            std::cerr << "Usage: " << argv[0] << " NUM WORD" << std::endl;
            return 1;
        }
    } else {
        std::cout << "Enter by how many numbers you want to shift alphabets: ";
        std::cin >> num;
        std::cin.ignore();
    }
    Cipher cipher(num);
    Cipher decipher(-num);

    std::string str;
    if (argc > 2) {
        str = argv[2];
    } else {
        std::cout << "Enter string: ";
        std::getline(std::cin, str);
    }

    std::string res = cipher.encrypt(str);
    std::string decrypted = decipher.encrypt(res);
    std::cout << "Encrypted String: "
              << res << '\n'
              << "Decrypted String: "
              << decrypted << '\n';
}


Laravel - super long relation, it doesn't feel right Okay, This is my method:

public function edit(Request $request, $ent, $room, $obj)
{
    $input = $request->except(['_token']);

    Enterprise::where('bedrijfsnaam', $ent)->first()->rooms()->where('name', $room)->first()->objects()->where('name', $obj)->first()->update($input);

    return redirect('/enterprise/'.$ent.'/room/'.$room);
}

As you can see, at Enterprise::where() I have a really long relation, But it doesn't feel right to have such a long relation. Is it just okay to have one this large or is there a better way to do it?

I suggest you figure out a way to view the execution history for all of the SQL statements across your application. This can be an absolute god-send when trying to untangle or optimize your queries.

I'd recommend using https://github.com/barryvdh/laravel-debugbar . It has a lot of other features as well but presents everything very neatly. There are ways to view the history with custom event listeners and such (https://stackoverflow.com/a/27753889/3224736) but the extra effort for debugbar would be worth it, IMO.

As for your specific query, I believe it'd compile into four separate queries. Three for each time you call first() and one for the update.

Eloquent does allow defining distant relationships with properties like hasManyThrough (https://laravel.com/docs/5.2/eloquent-relationships#has-many-through) but they can get as messy as manually querying the relationships. I'd give it a try, at least.

The other option is to use joins for the three selects. Something like

Enterprise::where('bedrijfsnaam', $ent)
    ->join('rooms', function ($join) {
        $join->on('enterprise.id', '=', 'rooms.enterprise_id')
             ->where('rooms.name', '=', $room);
    })
    ->join('objects', function ($join) {
        $join->on('rooms.id', '=', 'objects.room_id')
             ->where('objects.name', '=', $obj);
    })->first();

(That's just me guessing at your schema, though. You can modify that as needed).

Using one big join would compile into one query, meaning one round trip to the database. Just be sure you have proper indexes on your columns or the query could get bogged down.

-

Or you can investigate eager loading and lazy eager loading your relationships.

That would at least reduce the apparent complexity of your query. But those are most useful if you anticipate iterating the relationships, rather than just selecting one of them. They wouldn't likely do much to speed up your current query.

As complete tangents, I'd recommend investigating two items.

Make sure your models define fillable arrays. Without them, passing the entire $input into it, even if you try to manually prune it, can lead to nasty security/integrity holes.
See if you can use a route or action instead of a raw redirect. Laravel is built to use MVC through-and-through so you might as well learn how to use it early on, rather than patch all your manual stuff later.


Counting sort using arrays This is my implantation of counting sort, but I see myself using way too many for loops. Is this necessary?

    using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace ConsoleApplication1
{
    class Program
    {
        public static int[] a = { 0,0,0,5,4,8,9,9,7,3, 3, 2, 1 };
        public static void Sorting()
        {
            int j = 0, i = 0, smallestvalue = 0, largestvalue = 0, n = a.Length, lengthof_B = 0, temp = 0, anothersmallestvalue;
            smallestvalue = largestvalue = a[0];
            for (i = 0; i < n; i++)
            {
                if (smallestvalue > a[i])
                {
                    smallestvalue = a[i];
                }
                else if (largestvalue < a[i])
                {
                    largestvalue = a[i];
                }
            }
            int x = anothersmallestvalue = smallestvalue;

            lengthof_B = largestvalue - smallestvalue + 1;
            int[] b = new int[lengthof_B];
            for (i = 0; i < lengthof_B && smallestvalue <= largestvalue; i++)
            {
                for (j = 0; j < n; j++)
                {
                    if (smallestvalue == a[j])
                    {
                        b[i] = b[i] + 1;
                    }
                }
                b[i] = temp + b[i];
                temp = b[i];
                smallestvalue++;
            }
            int[] c = new int[a.Length];
            for (i = n - 1; i >= 0; i--)
            {
                anothersmallestvalue = x;
                for (j = 0; j <= lengthof_B ; j++)        //{3,3,2,1}
                {
                    if (a[i] == anothersmallestvalue)
                    {
                        temp = b[j];
                        c[temp-1] = anothersmallestvalue;
                        b[j] = b[j] - 1;
                    }
                    anothersmallestvalue++;
                }
            }
            for (i = 0; i < c.Length; i++)
            {
                Console.WriteLine("c[i] : " + c[i]);
            }
        }
    }
    class Demo
    {
        static void Main(string[] args)
        {
            Program.Sorting();
            Console.ReadLine();
        }
    }
}

I'm sorry, but I think this is the time where it's easier to start from scratch then try to fix your code.
Let me comment on some of your styling choices:

Don't use words like smallest/anothersmallest/largest etc - use min/max.
Respect the CamelCase for long variable names.
Don't initialize your loop counter in the beginning where you initialize all other variables.
n is unnecessary and while you created it - you don't use it everywhere.

Logic
The first loop where you calculate min and max is fine.
The second loop inside the loop is not necessary. Instead you could just use something like (assuming you would use min for smallest value and counts for temp array):
for (int i=0; i<a.length; i++) {
   counts[a[i]-min]++
}

The third loop where you populate result back on what you counted so far can be written like:
int j=0;
for (int i=0; i<counts.length; i++) {
   while (counts[i] > 0) {
      result[j] = i + min;
      counts[i]--;
      j++;
   }
}


Retrieve unique child objects from an array I'm using TypeScript but this example is in plain JavaScript. I have a class rank with a child-class master. I need an ordered collection of unique masters. Here's what I have but it seems a little "kak":

function getMasters() {
    var gms = [];
    _.each(this.ranks, (r) => {
        if (gms[r.master.id])
            return;
        gms[r.master.id] = new ExtendedRank(r.master.id r.master.code, r.master.name);
    });

    return _.without(gms, undefined).sort(this.sort);
}

This is the sort function (which I'm relatively happy with):

function sort(l: ExtendedRank, r: ExtendedRank) {
    return l.sortString.localeCompare(r.sortString);
}

Here's a more functional solution, which based on your example is what you're looking for.  It relies on the uniqueness of object keys.  Note it's untested, but the idea should work:

function getMasters() {
  var extended = r => new ExtendedRank(r.master.id r.master.code, r.master.name),
  return Object.values(
    this.ranks.reduce((m,r) => Object.assign(m, {[r.master.id]: extended(r)}), {});
  ).sort(this.sort);
}

If you're looking to go even further in this direction, I recommend checking out ramda as a better, more functional alternative to underscore.


Binary tree in Python As a Python newbie, I have coded this binary tree as best as I could to follow Python
idioms. However, I know it is still very verbose and not up to standard. I would like Python experts to tear this apart and identify its weaknesses.

class TreeNode(object):
  """ applicable to all tree nodes including root node """
  def __init__(self, value, left=None, right=None,root=False):
    self.value = value
    TreeNode.check(left)
    TreeNode.check(right)
    self.left = left
    self.right = right
    self.root = root

  def inorder(self):
    s = list()
    if not self.left:
      s.append('')
    else:
      s.append(self.left.inorder())

    s.append(str(self.value))

    if not self.right:
      s.append('')
    else:
      s.append(self.right.inorder())

    return ' '.join(s).strip()

  # returns True if success, False otherwise
  def insert( self, new_value ):
    # TODO ?? what happens if we are inserting strings
    if not new_value and new_value != 0:
      return False

    if new_value == self.value:
      return False

    # need to find the right location in terms of binary tree ordering
    if new_value < self.value:
      if not self.left:
        self.left = TreeNode(new_value)
        return True
      else:
        return self.left.insert( new_value )
    elif new_value > self.value:
      if not self.right:
        self.right = TreeNode(new_value)
        return True
      return self.right.insert( new_value )

    return False

  @staticmethod
  def check(node):
    if not node:
      return
    if not isinstance(node, TreeNode):
      raise TypeError('only instances of TreeNode are allowed')

  def __repr__(self):
    return '(' + repr(self.left) + ',' + \
      str(self.value) + ',' + repr(self.right) + ')'

Firstly, the official python style guide recommends 4-space indentation, and you are using 2. (No big deal but following the style guide is helpful)

class TreeNode(object):
  """ applicable to all tree nodes including root node """
  def __init__(self, value, left=None, right=None,root=False):

Your code never passes a left/right parameter. There are also doesn't seem to be a good reason why you would pass left/right parameters in the construction of the tree. I assume you would use this class by constructing an empty tree and inserting values into it, not by composing trees. As a result, I suggest eliminating left/right.

Also, you have a root parameter which is never used. It looks like something that you should eliminate.

Additionally, you cannot create an empty tree with this method.

    self.value = value
    TreeNode.check(left)
    TreeNode.check(right)
    self.left = left
    self.right = right
    self.root = root

  def inorder(self):
    s = list()

Use s = [], also don't use single letter variable names.

    if not self.left:

When checking for None, recommened practice is to do it like: if self.left is None:

      s.append('')
    else:
      s.append(self.left.inorder())

    s.append(str(self.value))

    if not self.right:
      s.append('')
    else:
      s.append(self.right.inorder())

    return ' '.join(s).strip()

The fact that you are trying to construct a string is kinda wierd. I'd expect this method to produce either an iterator or a list containing all the elements.

  # returns True if success, False otherwise
  def insert( self, new_value ):
    # TODO ?? what happens if we are inserting strings
    if not new_value and new_value != 0:
      return False

This is why you should "is None"

    if new_value == self.value:
      return False

    # need to find the right location in terms of binary tree ordering
    if new_value < self.value:
      if not self.left:
        self.left = TreeNode(new_value)
        return True
      else:
        return self.left.insert( new_value )
    elif new_value > self.value:

You've already established that this expression must be true, just use else

      if not self.right:
        self.right = TreeNode(new_value)
        return True
      return self.right.insert( new_value )

The previous branch had this inside an else. You should at least be consistent between branches

    return False

  @staticmethod
  def check(node):
    if not node:
      return
    if not isinstance(node, TreeNode):
      raise TypeError('only instances of TreeNode are allowed')

Explicitly checking types is frowned upon. All you manage is converting an AttributeError into a TypeError. However, a check method could usefully verify the invariants of a data structure. (Just only do it while debugging).

  def __repr__(self):
    return '(' + repr(self.left) + ',' + \
      str(self.value) + ',' + repr(self.right) + ')'

TreeNode seems to be a collection class, one that holds elements. The internal structure shouldn't matter to the world. However, repr spills the internal structure for the world to see. I'd suggest calling inorder to produce all of the elements inside the string and show that. Also repr's value should make it clear what class is being repr, by including "TreeMap" somewhere in its output.


Optimizing display of various famous quotes If it is possible, I'd like to be able to mash all of my arrays into one array, but still have the same functionality within my function (pick quote, pick creator, set padding).

Pastebin

var quotes = [
    /*48px*/"The true sign of intelligence is not knowledge but imagination.",
    /*48px*/"A people without the knowledge of their past history, origin and culture is like a tree without roots.",
    /*48px*/"Real knowledge is to know the extent of one's ignorance.",
    /*48px*/"Ignorance is the curse of God; knowledge is the wing wherewith we fly to heaven.",
    /*48px*/"Human behavior flows from three main sources: desire, emotion, and knowledge.",
    /*80px*/"If money is your hope for independence you will never have it. The only real security that a man will have in this world is a reserve of knowledge, experience, and ability.",
    /*80px*/"Knowledge is power. Information is liberating. Education is the premise of progress, in every society, in every family.",
    /*48px*/"Knowledge has to be improved, challenged, and increased constantly, or it vanishes.",
    /*112px*/"I'm hungry for knowledge. The whole thing is to learn every day, to get brighter and brighter. That's what this world is about. You look at someone like Gandhi, and he glowed. Martin Luther King glowed. Muhammad Ali glows. I think that's from being bright all the time, and trying to be brighter.",
    /*112px*/"Wisdom is the right use of knowledge. To know is not to be wise. Many men know a great deal, and are all the greater fools for it. There is no fool so great a fool as a knowing fool. But to know how to use knowledge is to have wisdom.",
    /*48px*/"To know, is to know that you know nothing. That is the meaning of true knowledge.",
    /*48px*/"The best advice I ever got was that knowledge is power and to keep reading.",
    /*48px*/"He who knows nothing is closer to the truth than he whose mind is filled with falsehoods and errors.",
    /*48px*/"To know what you know and what you do not know, that is true knowledge.",
    /*80px*/"I was bold in the pursuit of knowledge, never fearing to follow truth and reason to whatever results they led, and bearding every authority which stood in their way."
];
var creators = [
    "Albert Einstein",
    "Marcus Garvey",
    "Confucius",
    "William Shakespeare",
    "Plato",
    "Henry Ford",
    "Kofi Annan",
    "Peter Drucker",
    "Jay-Z",
    "Charles Spurgeon",
    "Socrates",
    "David Bailey",
    "Thomas Jefferson",
    "Confucius",
    "Thomas Jefferson"
];
var margins = [
    "48",
    "48",
    "48",
    "48",
    "48",
    "80",
    "80",
    "48",
    "112",
    "112",
    "48",
    "48",
    "48",
    "48",
    "80"
];
function quote() {
    var rand = Math.floor(Math.random() * quotes.length);
    $("p.quote").fadeOut(300).delay(1000).fadeIn(300);
    $("p.creator").fadeOut(300).delay(1000).fadeIn(300);
    $("p.quote").text(quotes[rand]);
    $("p.creator").text(creators[rand]);
    var marginTop = ($(".quotes .quotes-container").height() - margins[rand]) / 2 + "px";
    $(".quotes .quotes-container .content").attr("style","padding-top:" + marginTop);
};

It seems to me like quotes, creators and margins all belong to the same object; your intuition is correct, merging them into one is the right thing to do.

I don't do much web development, so I'm not going to post example code, but I would have the quotes on the server side, and have the script request them through AJAX.

The server-side would probably define an object such as this:

public class QuoteViewModel
{
    public string QuoteText { get; set; }
    public string Author { get; set; }
    public int Margin { get; set; }
}

The data could come from a database, a web service, an xml file on the server, whatever. When the jquery client requests a quote, the server side serializes a QuoteViewModel into a json string and sends it to the client.

This way all you need to change when you want to modify the quotes displayed, is the data itself.

I don't think Margin belongs on that type though. I'm pretty sure there's a way to use css so that you can only request the QuoteText and Author from the server (in a Json string), and achieve the same result.

Consequently, the first thing I'd do would be to delete the margins array and see what needs to change in order to do without; I warmly recommend you to include the html & css into your question, I'm sure there's something wrong with your markup for you to think you need to recentrize .content depending on what quote is picked.


Finding a pair of words from two files with a particular MD5 I have two txt files. Both contain 60k+ words in a list. I put together some code to loop through each file and concatenate the strings

import hashlib
with open('1.txt') as f:
    a= [line.rstrip() for line in f]
with open('2.txt') as f:
    b= [line.rstrip() for line in f]

for y in a:
    for z in b:
        if hashlib.md5(y+z).hexdigest()=='XXXTHEXXXHASHXXX':
            print y+z
            break

So line 1 in file 1 gets concatenated with line 1 in file 2, then line 2 etc. Line 2 in file  gets.... you get where this is going...

Is there a cleaner way to do this? Not only that but how can I edit the script to use more cores?

In general, no, this would not be the fastest approach. However, assuming that all the lines are quite small, I think you won't do better by the 'general' fast approach.

You could use hashlib.md5().copy().

import hashlib
with open('1.txt') as f:
    a= [line.rstrip() for line in f]
with open('2.txt') as f:
    b= [line.rstrip() for line in f]

for y in a:
    prefix_hash = hashlib.md5(y)
    for z in b:
        if prefix_hash.copy().update(z).hexdigest() == 'XXXTHEXXXHASHXXX':
            print y + z
            break

Again, assuming len(y) is not that large, this is unlikely to do any better, and might even do worse because of the .copy() now used. But you're welcome to benchmark it.


zipWith function in Scala My implementation works, but it looks very ugly. Curious to see how it's better implemented.

The zipWith function takes a list of lists (List[List[A]]), or a row with columns if you like. It will then transform this into a column with rows and applies a function List[A]=>B to obtain a List[B]. Hopefully the unit test below explains this better.

  test("zipWith"){
    val input = List(List(1,2,3), List(4,5,6), List(7,8,9))
    val expected = List("1::4::7", "2::5::8", "3::6::9")
    assert(zipWith(input)(_.mkString("::")) === expected)
  }

and the implementation is:

  def zipWith[A, B](lists: List[List[A]])(f: List[A] => B): List[B] = {
    @tailrec
    def loop(acc: List[B], input: List[List[A]]): List[B] = {
      val init = (Nil: List[List[A]], Nil: List[A])
      val (left, zipped) = input.foldLeft(init)((z, a) => {
        val (tails, heads) = z
        (a.tail :: tails, a.head :: heads)
      })
      if (left.foldLeft(false)(_ || _.isEmpty))
        f(zipped.reverse) :: acc
      else
        loop(f(zipped.reverse) :: acc, left.reverse)
    }

    loop(Nil, lists).reverse
  }

How about this?

def zipWith[A, B](ts: Traversable[Traversable[A]])(f: Traversable[A] => B): Traversable[B] = {
  ts.transpose.map(f)
}

Example:

scala> zipWith(input)(_.mkString("::"))
res10: Traversable[String] = List(1::4::7, 2::5::8, 3::6::9)


Extracting maximum values from each of the subsets of an array Problem description:

I have a numpy array cps from which I want to extract the maximum value in each of the
subsets of the array (a subset is defined as non-zero values which fall between zeros). The input and how the output should look like are illustrated in the following figure.

Note: cps only contains positive values and zeros.

for the moment, I am doing this as below:

## cps is a numpy array which contains values as observed in the figure

idx = np.nonzero(cps)
subsets = np.array_split(idx[0],np.where(np.diff(idx[0])!=1)[0]+1)
lst = np.full(len(cps), 0)

for subset in subsets:
    min = subset[0]   ## index of the start of subset (the smallest index number)
    max = subset[-1]  ## index of the end of subset (the largest index number)

    index_of_change_point_with_max_value = min + np.argmax(cps[min:max+1])

    lst[index_of_change_point_with_max_value] = cps[index_of_change_point_with_max_value]

However, I was wondering if there is anyway to do this in a vectorized manner and getting
rid of the for loop?

Single pass algorithm (~2n)

You can do it with a single pass over the array (which I shall denote as lst).

Maintain a maximum value (I shall denote this as max_val) once you hit a non-zero value. When we reach a zero, add (i, max_val) to the list of maximum values and reset the maximum value.
Build the new list using the list of maximum values.

This should give you pretty good performance (~2n since you iterate 2 times). However, if you need a parallel version...

Parallel algorithm

This is the first parallel algorithm I've ever designed, so pardon me if I screwed up somewhere. We use the MapReduce model which is quite simple to write.

Let k be the number of processes which you want to divide the work among and i be the number of each process where 0 <= i < k.

In the Map() procedure:

Each process starts at n/k * i indice and iterates till it hits n/k * (i+1).
Follow the algorithm as described in step 1 of the single pass algorithm.
If the first or last value is non-zero, it indicates that the values we have obtained may not be the maximum values of the subset as we may have only monitored a portion of the subset. Hence, we add it to the portions list.

When we flush, we mark the start and end indice that we have scanned and the maximum value, so as to indicate that we have not finished scanning that subarray.

For k = 3 and our array being [0, 0, 0, 0, 1] + [2, 3, 4, 0, 0] + [0, 2, 1, 0, 2] with a length of 15 (split into 3 segments for convenience):

i = 0: lst[0] = 0
       lst[1] = 0
       lst[2] = 0
       lst[3] = 0
       lst[4] = 1 # is_portion = True, max_val = 1
                  # add (4, 5, (4, 1)) to the list of portions
       return [] and [(4, 5, (4, 1))]
i = 1: lst[5] = 2 # is_portion = True, max_val = 2
       lst[6] = 3 # max_val = 3
       lst[7] = 4 # max_val = 4
       lst[8] = 0 # add (5, 8, (8, 4)) to the list of portions
       lst[9] = 0
       return [] and [(5, 8, (8, 4))]
i = 2: lst[10] = 0
       lst[11] = 2 # max_val = 2
       lst[12] = 1
       lst[13] = 0 # add (11, 2) to the list of maximum values
       lst[14] = 2 # is_portion = True, max_val = 2
                   # add (14, 15, (14, 2)) to the list of portions
       return [(11, 2)] and [(14, 15, (14, 2))]

In the Reduce() procedure, we merge the arrays we have together to form two lists:

1. Maximum values: [(11, 2)]
2. Portions not yet finished scanning: [(4, 5, (4, 1)), (5, 8, (8, 4)), (14, 15, (14, 2))]
  We merge the tuples (4, 5, (4, 1)) and (5, 8, (8, 4)) together and compare the maximum value found. (8, 4) > (4, 1), so we get a maximum value (8, 4).
  The last tuple, (14, 15, (14, 2)), ends at the boundary of the array, so we immediately return the maximum value (14, 2).

We now get [(11, 2), (8, 4), (14, 2)]. We build the list using step 2 and we're done!

The time complexity should be around ~2n if you choose a good k value.


Checking if a string parameter contains only digits public void setMobile(String mobile) {
      char[] mobileList = mobile.toCharArray();
      boolean isValid = true;
      for(char x : mobileList)
      {
          if(!Character.isDigit(x))
          {
              isValid = false;
          }
      }
      if(isValid)
      {
          this.mobile = mobile;
      }
    }

Is this the best way of doing things? Cheers.

There is one major flaw that will come back and haunt you with this code. If you enter an invalid number, there is absolutely no indication whatsoever that it was invalid.

If it is not valid, you should do this:

throw new IllegalArgumentException("Invalid mobile number: " + mobile);

Incorporating Heslacher's suggestions I would do this:

public void setMobile(String mobile) {
    if (!containsOnlyDigits(mobile)) {
        throw new IllegalArgumentException("Invalid mobile number: " + mobile);
    }
    this.mobile = mobile;
}

Another edge case that should be handled is the empty string. I assume you don't want to classify that as a valid mobile number? Then handle it also in containsOnlyDigits or handle it in setMobile.

if (mobile.isEmpty()) {
    throw new IllegalArgumentException("Empty string is not a valid mobile number");
}


Merge duplicated cells in bulk per each column VBA Need to merge data per column if records in the first column are the same.

I wrote a VBA code and it works but it works very slow as it executes "merge" function per each row no matter if consequent cells are the same or different.

Sub DuplicateValues()

'Declare All Variables:

Dim myCell As Range
Dim myRow As Integer
Dim myRange As Range
Dim myCol As Integer
Dim i As Integer
Dim k As Integer

Dim myFirstRow As Long
Dim myFirstColumn As Long

Dim rngCopy As Range, rngPaste As Range

'Count Number of Rows and Columns:

Application.DisplayAlerts = False

myRow = Range(Cells(3, 1), Cells(3, 1).End(xlDown)).Count + 2 'first two rows are headers
myCol = Range(Cells(2, 1), Cells(2, 1).End(xlToRight)).Count

'Apply formatting to the end of the table:

With ActiveSheet
Set rngCopy = .Range(.Range("A3"), .Cells(myRow, myCol))

 With rngCopy.Borders
        .LineStyle = xlContinuous
        .ColorIndex = 0
        .Weight = xlThin
    End With
End With

'Merge duplicated cells per Row:

myFirstRow = 3
myFirstColumn = 1

Set iRow = Cells(myFirstRow, myFirstColumn)
Set n = Cells(myFirstRow + 1, myFirstColumn)

For i = 1 To myRow

    If iRow <> n Then

                Range(Cells(myFirstRow, 1), Cells(myFirstRow, myCol - 3)).HorizontalAlignment = xlCenter
                Range(Cells(myFirstRow, 1), Cells(myFirstRow, myCol - 3)).VerticalAlignment = xlCenter
                Range(Cells(myFirstRow, 8), Cells(myFirstRow, 8)).WrapText = True
                Range(Cells(myFirstRow, 1), Cells(myFirstRow, myCol - 3)).EntireRow.AutoFit

                iRow = Cells(myFirstRow + i, myFirstColumn)
                n = Cells(myFirstRow + i + 1, myFirstColumn)

                Else
                n = Cells(myFirstRow + i + 1, myFirstColumn)

                    For k = 1 To myCol - 3 'need to merge data per column but don't need to merge data in the last 3 columns

                        Range(Cells(myFirstRow + i - 1, k), Cells(myFirstRow + i, k)).Merge
                        Range(Cells(myFirstRow + i - 1, k), Cells(myFirstRow + i, k)).HorizontalAlignment = xlCenter
                        Range(Cells(myFirstRow + i - 1, k), Cells(myFirstRow + i, k)).VerticalAlignment = xlCenter
                        Range(Cells(myFirstRow + i - 1, 8), Cells(myFirstRow + i, 8)).WrapText = True
                        Range(Cells(myFirstRow + i - 1, k), Cells(myFirstRow + i, k)).EntireRow.AutoFit

                    Next

    End If
Next

End Sub

I  tried to create a different logic to initiate "merge" by blocks (skip merging the same value cells in the first column right away):
merging only when the "base/initial" cell is not the same as a "check" cell and merge all appropriate cells all together (not by each row), but I can't find a way to make it work.

Would be very grateful for any code optimizations!

'Declare All Variables:

Dim myCell As Range
Dim myRow As Integer
Dim myRange As Range
Dim myCol As Integer
Dim i As Integer
Dim k As Integer

Dim myFirstRow As Long
Dim myFirstColumn As Long

Dim rngCopy As Range, rngPaste As Range

Don't do this. This procedure is somewhere between 2 and 3 screens high on my laptop: when I'm at the bottom of the procedure, I don't know what I'm looking at, so I scroll back up to this wall of declarations, parse the chunk of code, locate the variable I'm looking for, then scroll back down to where I was, ...and that gets very annoying, very fast.

People defending this "declare everything at the top of the procedure" habit usually defend it pretty hard, and they're wrong. "I can see everything that's used in the procedure at once" sounds great on paper. Truth is, it only makes you scroll back and forth all the time for no reason, and makes it much harder than it should be, to know whether or not a variable is used, let alone where it's used.

If you declare variables where you first assign them, as you need to introduce them, then you simply can't miss the fact that myCell, myRange, rngPaste are never assigned or referenced anywhere... and that iRow and n aren't declared at all. This is extremely bug-prone, you don't want to allow VBA code to run with undeclared variables. Fortunately you can completely prevent this, by simply specifying Option Explicit at the very top of every module you ever write any VBA code in.

Proper indentation also helps, massively. It started well:

For i = 1 To myRow

    If iRow <> n Then

...and then suddenly went way out there:

                Range(Cells(myFirstRow, 1), Cells(myFirstRow, myCol - 3)).HorizontalAlignment = xlCenter

...to seemingly random:

            Else
            n = Cells(myFirstRow + i + 1, myFirstColumn)

                For k = 1 To myCol - 3 'need to merge data per column but don't need to merge data in the last 3 columns

Use an indenter if you're unsure how to keep indentation consistent.

Row numbers should always be As Long. An Integer is a 16-bit signed integer type, so its maximum possible value is 32,767. This means as soon as your data involves row 32,768 and beyond, your code breaks with an "Overflow" run-time error. In fact, there's little to no reason at all to ever declare anything As Integer in 2019: processors are optimized to deal with 32-bit integer types, and that's a Long in VBA.

I like this:

With ActiveSheet

I makes ActiveSheet references explicit, and that's very good.

However you're implicitly referring to whatever the ActiveSheet is everywhere else outside this With block.

myRow = Range(Cells(3, 1), Cells(3, 1).End(xlDown)).Count + 2 'first two rows are headers
myCol = Range(Cells(2, 1), Cells(2, 1).End(xlToRight)).Count

Unqualified like this, Range and Cells are implicitly referring to the ActiveSheet: the With ActiveSheet block is therefore redundant, and makes accesses to that active sheet inconsistent: some are explicit, some are implicit - and that's very bug-prone.

A note about this:

myRow = Range(Cells(3, 1), Cells(3, 1).End(xlDown)).Count + 2 'first two rows are headers

Looks like you're trying to get the last used row; this is not a reliable way to do it. Consider flipping the logic around and going up from the last row on the sheet to get the .Row of that specific cell, rather than the Count of cells in a range:

myRow = ws.Range("A" & ws.Rows.Count).End(xlUp).Row

Note how the fist two rows being headers, doesn't matter any more.

I'd strongly advise against using my as a prefix for anything, regardless of how often you see it in documentation. Same goes for rng and o and whatever other prefixing scheme you might read anywhere. Use meaningful names instead. "myRow" doesn't say what's important about it: it's the last row with data. lastRow would already be a much better name for it.

rngCopy is assigned, its borders are formatted, ...but it's not being copied anywhere. The name strongly suggests it's a source, especially since it's declared right next to the unused rngPaste, which strongly suggests that would be a destination for an operation involving the clipboard. Pretty misleading, since none of that is happening anywhere in the procedure.

I already mentioned n isn't declared. Its usage is a problem.

Set n = Cells(myFirstRow + 1, myFirstColumn)

It's Set-assigned to a Range. Thus, its runtime type at that point is Variant/Range.

If iRow <> n Then

Now it's being compared to iRow, which is another undeclared Variant/Range variable, and the two objects are only comparable through implicit let-coercion involving hidden default member calls - making it all explicit would look like this:

If iRow.Value <> n.Value Then

And while that's now explicit, it's wrong. It's wrong, because they shouldn't be Range objects. They shouldn't be Range objects, because they're later being Let-assigned to plain values:

        iRow = Cells(myFirstRow + i, myFirstColumn)
        n = Cells(myFirstRow + i + 1, myFirstColumn)

Beyond that point, the data type of iRow and n is up in the air. If you're lucky, you're looking at two Variant/Double values. Otherwise, you're looking at one or more Variant/Error values (that would be the case if the worksheet contains error values), and everything blows up with a type mismatch error.

Not declaring variables is one thing, reusing variables is another, but reusing undeclared variables and assigning them a new runtime type, makes everything very hard to follow. Declare them with an explicit data type, and let them be values, instead of objects that behave like values thanks to implicit hidden code.

As for performance, the reason it's slow is because it's doing the single slowest thing any code can do: interact with worksheet cells directly, in a loop.

You can't help it, interacting with worksheet cells is precisely what this macro needs to do. All you can do is tell Excel to stop trying to keep up as you modify the worksheet.

Every time a cell is modified, Excel evaluates whether a recalc is needed, and will perform it if it has to. Switch calculation to manual before you start manipulating the worksheet, and toggle it back to automatic when you're done: Excel will only recalculate once. That's Application.Calculation (set to xlCalculationManual, then back to xlCalculationAutomatic when you're ready).

Every time a cell is modified, Excel fires worksheet events (e.g. Worksheet.Change), and if there's a handler procedure for that event, then that code will run before the next instruction does. Turn off application events before you start manipulating the worksheet, and toggle it back on when you're done: no worksheet events will be fired; that's Application.EnableEvents (set to False, then back to True when you're ready).

Every time a cell is modified, Excel tries to repaint itself. While that's normally pretty fast, when you're modifying cells in a loop you don't want Excel to even try to keep up - you want to make your changes, and have Excel repaint itself when you're done. Do that by toggling Application.ScreenUpdating (set to False, then back to True when you're ready).

Whenever you toggle this global Application state, make sure you handle runtime errors, and have your error-handling subroutine ensure that the global state gets properly reset whether the procedure succeeds or fails, whatever the reason for failing might be.


Using QuickCheck to Verify Free Monad's Functor Instance Given the Free Monad, and my Eq, Show, and Functor instances, I attempted to verify the first Functor law using QuickCheck:

data Free f a = Var a
               | Node (f (Free f a))

I defined the following Eq and Show instances (credit to duplode for helping me out on the Eq instance:

instance (Eq (f (Free f a)), Eq a) => Eq (Free f a) where
    (==) (Var x) (Var y)       = x == y
    (==) (Node fu1) (Node fu2) = fu1 == fu2
    (==) _ _                   = False

instance (Show (f (Free f a)), Show a) => Show (Free f a) where
  show (Var x)  = "Var " ++ (show x)
  show (Node x) = "Node " ++ (show x)

Then, I implemented a Functor instance:

instance Functor f => Functor (Free f) where
  fmap g (Var x)  = Var (g x)
  fmap g (Node x) = Node $ fmap (\y -> fmap g y) x

And now the QuickCheck work:

instance Arbitrary (Free Maybe Int) where
    arbitrary = do
        x <- arbitrary :: Gen Int
        y <- arbitrary :: Gen Int
        elements [Var x, Var y, Node (Nothing), Node (Just (Var y))]

--fmap id = id

functor_id_law ::  Free Maybe Int -> Bool
functor_id_law x = (fmap id x) == (id x)

Finally, run it in QuickCheck:

ghci> quickCheck functor_id_law
+++ OK, passed 100 tests.

However, I haven't included other Functor's, such as [], etc. Nor have I used other types, i.e. Char, String, etc.

What's a more rigorous approach to verifying that my definition of the Free Monad's Functor instance obeys the first Functor Law?

Use Eq1 to implement Eq, Show1 to implement Show

While it's a very recent addition, you should implement Eq via Eq1 and Show via Show1. That removes the UndecidableInstances pragma from your code:

import Data.Functor.Classes

data Free f a = Var a
               | Node (f (Free f a))

instance (Eq1 f) => Eq1 (Free f) where
  liftEq f (Var x)    (Var y)    = f x y
  liftEq f (Node fu1) (Node fu2) = liftEq (liftEq f) fu1 fu2
  liftEq _ _ _                   = False

instance (Eq1 f, Eq a) => Eq (Free f a) where
  (==) = eq1

instance Show1 f => Show1 (Free f) where
  liftShowsPrec sp sl = go
    where
      go d (Var a) = showsUnaryWith sp "Var" d a
      go d (Node fa) = showsUnaryWith (liftShowsPrec go (liftShowList sp sl)) "Node" d fa

instance (Show1 f, Show a) => Show (Free f a) where
  showsPrec = showsPrec1

We need to lift twice: once through the functor f, and once through the next Free.

QuickCheck

Your instance isn't very flexible. Furthermore, QuickCheck cannot set its size. Instead of a fixed size, use sized:

instance Arbitrary (Free Maybe Int) where
    arbitrary = sized go
      where
        go 0 = Var <$> arbitrary
        go n = Node <$> oneof [pure Nothing, Just <$> go (n - 1)]

However, that only yields a single instance. As with Show and Eq, there exists an Arbitrary1 class we can use instead:

instance Arbitrary1 f => Arbitrary1 (Free f) where
  liftArbitrary a = sized (go . min 3)
    where
      go 0 = Var <$> a
      go n = Node <$> liftArbitrary (go (n - 1))

instance (Arbitrary1 f, Arbitrary a) => Arbitrary (Free f a) where
  arbitrary = arbitrary1

We can now check the functor id law easily for many combinations.

The more rigorous approach

The more rigorous approach with functor laws is to proof them by hand. So let us assume that functor laws hold for the functor f in Free f a. Then we have to show that

fmap id value = value

for any value :: Free f a. There are two cases. Either value is a Var x for some x :: a. Then

fmap id value = fmap id (Var x) = Var (id x) = Var x = value

and the law holds. Or value is a Node y for some y :: f (Free f a). Then

fmap id value = fmap id (Node y)
              = Node (fmap id y)
              = Node (id y)      -- used functor law for `f'
              = Node y
              = value

Therefore, the first functor law holds for your instance.


Simplifying a repetitive event handler Is there a more efficient way of writing this? It seems like so much redundancy that this can be greatly reduced. Basically the only difference is if isNodeWebkit is true then run a function using key code 113 or 112

    if (isNodeWebkit) {
        $(document).keyup(function (e) {
            if (e.which == 113) {
                $.ajax({
                    type: "POST",
                    url: "/submit/role/",
                    data: {},
                    dataType: "json",
                    success: function (admin) {
                        if (admin == true) {
                            window.location = urllink;
                        }
                    }
                });
            };
        });
    }
    else {
        $(document).keyup(function (e) {
            if (e.which == 112) {
                $.ajax({
                    type: "POST",
                    url: "/submit/role/",
                    data: {},
                    dataType: "json",
                    success: function (admin) {
                        if (admin == true) {
                            window.location = urllink;
                        }
                    }
                });
            };
        });
    };

Yes! Introduce a keyCode variable to hold the one difference between the two otherwise identical code blocks.

var keyCode = isNodeWebkit ? 113 : 112;
$(document).keyup(function (e) {
    if (e.which == keyCode) {
        $.ajax({
            type: "POST",
            url: "/submit/role/",
            data: {},
            dataType: "json",
            success: function (admin) {
                if (admin == true) {
                    window.location = urllink;
                }
            }
        });
    };
});

Edit: Reverted equality tests since I don't know what you expect to appear in admin, and heck, perhaps some browsers convert the key code to a string!


Calculating Fibonnaci sequence lazily Implementation:

IEnumerable<BigInteger> Fibs()
{
    BigInteger a = 0;
    BigInteger b = 1;
    while(true)
    {
        b = a + (a = b);
        yield return a;
    }
}

Usage:

void Main()
{
    // take first 100 fib numbers
    var fibs = Fibs().Take(100);
}

How can I improve this?

That's a pretty clever implementation. To improve this, I would take some of the cleverness out of it. In particular:

while (true)
{
    BigInteger next = a + b;
    a = b;
    b = next;
    yield return next;
}

2 lines longer, but no unnecessary cleverness. Perfectly clear, nice and simple, and should be just as efficient. It's good general rule to not try to be too clever. Keep it simple.

Also, FibonacciSequence would be a better name than Fibs.

Finally, as a tiny remark, put a space before opening parenthese of the while condition (like I did above, different from the original code).


Showing/hiding a box containing a Facebook profile I see that there are quite some words that are appearing double or more times in my code.

I'm thinking there might be a more efficient way of writing this code, only I don't have the knowledge to do so.

function transitionData(data) {
    if($("div#facebook div#notice").is(":visible")) {
        $("div#facebook div#heading *").fadeOut();
        $("div#facebook div#notice").animate({"opacity":"0"}, function() {
            $("div#facebook div.wrapper > img:first-child").attr("src", "http://graph.facebook.com/"+data.id+"/picture?type=normal");
            $("div#facebook h3").html("Facebook account of "+data.name+"");
            $("div#facebook div#heading a").attr("href", "http://www.facebook.com/"+data.username);
            $("div#facebook tr:first-child td:first-child b").text(data.id);
            $("div#facebook tr:first-child+tr td:first-child b").text(data.first_name);
            $("div#facebook tr:first-child+tr+tr td:first-child b").text(data.last_name);
            $("div#facebook tr:first-child td:first-child+td b").text(data.gender.charAt(0).toUpperCase() + data.gender.slice(1));
            $("div#facebook tr:first-child+tr td:first-child+td b").text(data.locale);
            $("div#facebook tr:first-child+tr+tr td:first-child+td b").text(data.username);

            $("div#facebook div#notice").slideUp();
            $("div#facebook div.wrapper").slideDown();
            $("div#facebook div#heading *").fadeIn();
        });
    } else {
        $("div#facebook div#heading *, div#facebook div.wrapper").fadeOut(function() {
            $("div#facebook div.wrapper > img:first-child").attr("src", "http://graph.facebook.com/"+data.id+"/picture?type=normal");
            $("div#facebook h3").html("Facebook account of "+data.name+"");
            $("div#facebook div#heading a").attr("href", "http://www.facebook.com/"+data.username);
            $("div#facebook tr:first-child td:first-child b").text(data.id);
            $("div#facebook tr:first-child+tr td:first-child b").text(data.first_name);
            $("div#facebook tr:first-child+tr+tr td:first-child b").text(data.last_name);
            $("div#facebook tr:first-child td:first-child+td b").text(data.gender.charAt(0).toUpperCase() + data.gender.slice(1));
            $("div#facebook tr:first-child+tr td:first-child+td b").text(data.locale);
            $("div#facebook tr:first-child+tr+tr td:first-child+td b").text(data.username);

            $("div#facebook div#heading *, div#facebook div.wrapper").fadeIn();
        });
    }
}

From a once over,

You have far more than a few words that are copy pasted, you could extract all the common statements into a common function:

function transitionDataDone( data ){
    $("div#facebook div.wrapper > img:first-child").attr("src", "http://graph.facebook.com/"+data.id+"/picture?type=normal");
    $("div#facebook h3").html("Facebook account of "+data.name+"");
    $("div#facebook div#heading a").attr("href", "http://www.facebook.com/"+data.username);
    $("div#facebook tr:first-child td:first-child b").text(data.id);
    $("div#facebook tr:first-child+tr td:first-child b").text(data.first_name);
    $("div#facebook tr:first-child+tr+tr td:first-child b").text(data.last_name);
    $("div#facebook tr:first-child td:first-child+td b").text(data.gender.charAt(0).toUpperCase() + data.gender.slice(1));
    $("div#facebook tr:first-child+tr td:first-child+td b").text(data.locale);
    $("div#facebook tr:first-child+tr+tr td:first-child+td b").text(data.username);
}

Once you have done that, you will notice that you are not repeating too many jQuery selectors. The only one that stands out to me is $("div#facebook div#notice"); If you cache this selector and apply the previous code, you get something like this (not tested, but you should get the gist):

function transitionData(data) {
    var $divNotice = $("div#facebook div#notice");
    if($divNotice.is(":visible")) {
        $("div#facebook div#heading *").fadeOut();
        $divNotice.animate({"opacity":"0"}, function() {
            transitionDataDone( data )
            $divNotice.slideUp();
            $("div#facebook div.wrapper").slideDown();
            $("div#facebook div#heading *").fadeIn();
        });
    } else {
        $("div#facebook div#heading *, div#facebook div.wrapper").fadeOut(function() {
            transitionDataDone( data );
            $("div#facebook div#heading *, div#facebook div.wrapper").fadeIn();
        });
    }
}

function transitionDataDone( data ){
    $("div#facebook div.wrapper > img:first-child").attr("src", "http://graph.facebook.com/"+data.id+"/picture?type=normal");
    $("div#facebook h3").html("Facebook account of "+data.name+"");
    $("div#facebook div#heading a").attr("href", "http://www.facebook.com/"+data.username);
    $("div#facebook tr:first-child td:first-child b").text(data.id);
    $("div#facebook tr:first-child+tr td:first-child b").text(data.first_name);
    $("div#facebook tr:first-child+tr+tr td:first-child b").text(data.last_name);
    $("div#facebook tr:first-child td:first-child+td b").text(data.gender.charAt(0).toUpperCase() + data.gender.slice(1));
    $("div#facebook tr:first-child+tr td:first-child+td b").text(data.locale);
    $("div#facebook tr:first-child+tr+tr td:first-child+td b").text(data.username);
}

Finally, I must say, your approach is terrible. Your elements should have unique id's ( first_name, username etc. etc. ), the way you select elements now is a maintenance nightmare. Re-ordering the elements means re-writing the JavaScript code!


Sum-based sliding rate limiting with Redis and Lua As I'm new to Redis, I would like to get a review / improvement suggestions from Redis / Lua experts on the following problem and the solution I have found so far.

Problem

The context is: an e-commerce system used by multiple users purchasing various items at different prices. Because of regulation, user purchases must be capped to a certain amount during a certain sliding period.

For example, no more than 10€ can be spent by a purchasing user during a sliding window of 1 week.

The problem to solve is: using StackExchange.Redis (Redis client .NET library) and Redis v3, implement a sliding window rate limiting mechanism, based not on max count of requests (many solutions for this particular problem can be found on web), but on a total sum of purchases done during a sliding period.

Solution design

A single purchase is represented by a purchase ID. It is stored as a string data type with the associated purchase amount (2.5 in the example below). Because we don't need to keep it forever, we'll expire it after 1 week (604800 seconds).

SETEX purchaseid:98db31bf9b 604800 2.5

Users and their corresponding purchases are represented by a sorted list where the key is the user ID, scores are numbers of seconds since 1970 (Epoch time), and values are purchase IDs. Likewise, the expiration is one week (renewed with each new purchase):

ZADD userid:hello@example.com 1443719939 purchaseid:98db31bf9b
EXPIRE userid:hello@example.com 604800

Fig1.: Data model (example)

Because several Redis calls might be required to implement this, I used a pre-loaded Lua script (using EVALSHA) in order to execute several Redis commands in one shot, so to avoid race conditions and to reduce network exchanges (please correct me if I'm wrong here).

Every time a user tries to purchase something, this script is called by the client application, and returns 1 if the purchase is accepted and registered, 0 otherwise.

However, I'm not sure how this approach will work in the context of a Redis Cluster. Will the data created through the Lua script be auto-partitioned over multiple masters? If yes, will the Lua script be able to read it back?

Solution code (Lua)

--[[
Initialization: StackExchange.Redis library allows passing
named parameters tagged with @
--]]
local userKey = 'userid:' .. @curUserId
local curPurchaseKey = 'purchaseid:' .. @curPurchaseId
local sum = 0
--[[
Cleaning up old entries up to @minSecond (in our example,
minSecond = Now (Epoch time in secs) - 604800 (nb of secs in a week)):
--]]
redis.call('ZREMRANGEBYSCORE', userKey, '-inf', @minSecond)

--[[
Getting the list of purchases done by user since @minSecond,
summing them up, returning 0 (failed) if @maxAmount is reached (e.g., 10€):
--]]
local pastEntries = redis.call('ZRANGEBYSCORE', userKey, @minSecond, '+inf')
for i = 1, #pastEntries, 1 do
    local purchaseKey = pastEntries[i]
    local amount = tonumber(redis.call('GET', purchaseKey) or 0)
    sum = sum + amount
end

if sum >= tonumber(@maxAmount) then
    return 0
end

--[[
Saving the current purchase ID + amount (e.g., 2.5€),
updating the sorted list of the user's purchases,
returning 1 (success):
--]]
redis.call('SETEX', curPurchaseKey, @expInSec, @curAmount)
redis.call('ZADD', userKey, @curSecond, curPurchaseKey)
redis.call('EXPIRE', userKey, @expInSec)
return 1

There might be one correctness issue here:

If a customer makes just one purchase all week with the transaction amount exceeding the cap, do you still want to throttle the transaction? Just wanted to bring that up in case you missed this corner case.

Since you talk about performance, you will want to consider which of the two are important to you:

request latency - each request completing within a fixed time
throughput - number of requests served from the cache in a time period.

You can have higher latency, but still maintain a high throughput as long as the transactions are not serialized.

One suggestion I had that might help with both is that you can pull the following:

--[[
Cleaning up old entries up to @minSecond (in our example,
minSecond = Now (Epoch time in secs) - 604800 (nb of secs in a week)):
--]]
redis.call('ZREMRANGEBYSCORE', userKey, '-inf', @minSecond)

outside into a separate call or issue it lazily.

Lazy techniques can be:

GC once every 'N' calls,
run GC as a cron / batch job once a day for all customers
Do GC only if the size of the sorted set is greater than a fixed threshold

This will make individual transactions potentially faster at the expense of a larger memory footprint.

This might have some performance implication, so I would do some benchmarking just for that one call.

If you're okay with being slightly inaccurate about the cap on spending per customer, there are two things you can potentially play with:

Use Exponential Moving Averages. This will allow you to not do a range query on the sorted set, and instead do a constant time query. There's plenty of resources on this topic on the inter-webs.
Use daily aggregates. Whenever you get a query, you will need to look-up exactly 7 key-value pairs. Ofcourse this will not work if you want precision.

Otherwise, your existing approach will get the job done.


Detect single-character xor (OCaml) I have been working through the Cryptopals challenges in Ocaml. My interest in Ocaml is to better learn functional programming techniques and effective usage of the type system.

Set 1 Challenge 4 says 'One of the 60-character strings in this file has been encrypted by single-character XOR. Find it.'

My code finds the correct string. I tried using user-defined types (e.g. xor_input, xor_output) to avoid passing around lots of string types that could easily be muddled. However, I am not sure of what is an appropriate level of type annotation. For example:

is there any merit in make_ranking specifying a return type of score if it ends in |> Score? My understanding of Haskell is that I could use the function's type annotation to specify that it returns a Score and the int returned from the fold would automatically be wrapped in a score type - is there something similar in Ocaml? |> Score seems a cumbersome way to specify the return type.
I was also unsure how to convert to and from a user-defined type and its unwrapped value (e.g. score and int). My approach in sort_results and output (destructuring) seems inelegant to me.
in sort_results I destructure the arguments to get the scores. Does this not mean that any type with a score field will be accepted?

(Note that Hexx is a module I wrote to convert a base64 encoded string to a hex string. Assume the input is a file containing a list of strings.)

open Core

let english = "etaoin srhldcumfpgwybvkxjqzETAOINSRHLDCUMFPGWYBVKXJQZ0123456789.?!"

type score = Score of int
type xor_input = Xor_input of string
type xor_output = Xor_output of string
type result =
  {
    test_char : char;
    score : score;
    xor_input : xor_input;
    xor_output : xor_output;
  }

let xor c (Xor_input input) : xor_output =
  String.map input ~f:(fun l -> int_of_char l |> (lxor) c |> char_of_int) |> Xor_output

let make_ranking (Xor_output str) : score =
  String.fold str ~init:0 ~f:(fun total c ->
      total + match String.index english c with
              | Some v -> v
              | None -> 100 (* arbitary value for non-letters *)
    ) |> Score

let make_result (input : xor_input) c : result =
  let output = xor c input in
  { test_char = char_of_int c; score = make_ranking output; xor_input = input; xor_output = output }

let sort_results { score = first; _ } { score = second; _ } =
  let (Score first_val) = first in
  let (Score second_val) = second in
  first_val - second_val

let find_best_match results : result =
  let best = List.sort ~cmp:sort_results results |> List.hd in
  match best with
  | Some x -> x
  | None -> failwith "No best result found"

let xor_line line =
  let decoded = Hexx.decode line in
  List.range 0 255
  |> List.map ~f:(make_result (Xor_input decoded))
  |> find_best_match

let output {test_char; score; xor_input; xor_output} =
  let (Score s) = score in
  let (Xor_input input) = xor_input in
  let (Xor_output out) = xor_output in
  fprintf stdout "Char: %c, Score: %d, Original: %s, Output: %s\n" test_char s input out

let () =
  In_channel.read_lines "4.txt"
  |> List.map ~f:xor_line
  |> find_best_match
  |> output

Congrats on tackling cryptopals! These are great exercises and very interesting to learn new languages and techniques.

Syntax

Remove the type annotations

In most cases in OCaml, you don't need any type annotations. So instead of:

let make_result (input : xor_input) c : result =

It's more usual to write:

let make_result input c =

The reason is that for exposed functions, the type will be in the .mli file, and for local functions, it's only one key binding away in your editor thanks to merlin.

Pipelines

The pipe operator is powerful and works well in some cases like your main function, but combined with partial application and infix operators, it can become hard to read.

I'd rewrite:

fun l -> int_of_char l |> (lxor) c |> char_of_int

As:

fun l -> char_of_int (c lxor (int_of_char l))

Pattern tricks

In places like this:

let (Xor_input input) =

You can omit the parens and just write:

let Xor_input input =

You can also perform several layers of pattern matching in one step. So rather than writing

let sort_results { score = first; _ } { score = second; _ } =
  let (Score first_val) = first in
  let (Score second_val) = second in

You can write:

let sort_results { score = Score first_val; _ } { score = Score second_val; _ } =

Record shorthand syntax

When building a record, parts that look like label = label can be written just label. So you can rename variables and change:

let make_result input c =
  let output = xor c input in
  { test_char = char_of_int c; score = make_ranking output; xor_input = input; xor_output = output }

To:

let make_result xor_input c =
  let xor_output = xor c xor_input in
  { test_char = char_of_int c; score = make_ranking output; xor_input; xor_output }

Type design

I don't think that the xor_input and xor_output bring a lot here. You're probably be as OK with plain strings.

Other remarks

You are sorting a whole list just to get the minimal element. There is probably a function to do that in core (and it will be linear).

Instead of fprintf stdout, you can just use printf.

That's it! Have a nice journey learning OCaml and please post other examples on this site :)


Logging errors with the same exception type I wrote some code which simply retrieves the HTTP status code of the passed in URL.  There are multiple potential causes of the same error.  I am hoping to get some all around feedback on this code, but my primary concern is that I have low readability in an attempt to provide good feedback in the event of an error.

private static final String HOST_NAME = "proxy.mycompany.com";
private static final int PORT = 5555;
private static final Logger LOGGER = LoggerFactory
        .getLogger("HttpStatusCode");

public static int getHttpStatusCode(URL url) {
    Proxy proxy = new Proxy(Proxy.Type.HTTP, new InetSocketAddress(
                  HOST_NAME, PORT));
    HttpURLConnection connection = null;
    try {
           connection = (HttpURLConnection) url.openConnection(proxy);
    } catch (IOException e) {
           LOGGER.error("URL.openConnection(Proxy) failed in HttpStatusCode.getHttpStatusCode(URL)!");
           LOGGER.error("URL attempted: " + url);
    }
    int responseCode = -1;
    try {
           responseCode = connection.getResponseCode();
    } catch (IOException e) {
           LOGGER.error("HttpURLConnection.getResponseCode() failed in HttpStatusCode.getHttpStatusCode(URL)!");
           LOGGER.error("URL attempted: " + url);
    }
    connection.disconnect();
    return responseCode;
}

My original attempt was the following, but my co-worker advised that I change it to the former for better error handling:

private static final String HOST_NAME = "proxy.mycompany.com";
private static final int PORT = 5555;
private static final Logger LOGGER = LoggerFactory
        .getLogger("HttpStatusCode");

public static int getHttpStatusCode(URL url) {
    Proxy proxy = new Proxy(Proxy.Type.HTTP, new InetSocketAddress(
            HOST_NAME, PORT));
    try {
        HttpURLConnection connection = (HttpURLConnection) url
                .openConnection(proxy);
        int responseCode = connection.getResponseCode();
        connection.disconnect();
        return responseCode;
    }
    catch (IOException e) {
        LOGGER.error("IOException in HttpStatusCode.getHttpStatusCode(URL)!");
        LOGGER.error("URL attempted: " + url);
        return -1;
    }
}

Just to be clear, this code compiles and runs error free based on my testing.  I am looking for a review on the way I am handling potential errors.

Your log output shouldn't also be detailing the code that failed, because the JVM is going to generate a more detailed stacktrace, and you should print that instead.

LOGGER.error("..." + url, e);

Instead, it will be more beneficial to give a more descriptive message. Borrowing your original code...

try {
       connection = (HttpURLConnection) url.openConnection(proxy);
} catch (IOException e) {
       LOGGER.error("Unable to open a connection to " + proxy, e);
}
int responseCode = -1;
try {
       responseCode = connection.getResponseCode();
} catch (IOException e) {
       LOGGER.error("Could not get response code from " + proxy, e);
}


Calculating protein mass This question is part of a series solving the Rosalind challenges. For the previous question in this series, see A sequence of mistakes. The repository with all my up-to-date solutions so far can be found here.

Problem: PRTM

  In a weighted alphabet, every symbol is assigned a positive real number called a weight. A string formed from a weighted alphabet is called a weighted string, and its weight is equal to the sum of the weights of its symbols.

  The standard weight assigned to each member of the 20-symbol amino acid alphabet is the monoisotopic mass of the corresponding amino acid.

Given:

  A protein string \$P\$ of length at most 1000 aa.

Return:

  The total weight of \$P\$. Consult the monoisotopic mass table.

Sample Dataset:

SKADYEK

Sample Output:

821.392

My solution solves the sample dataset and the actual dataset given.

Dataset:

FQLCECNPKWNEEKRQYGACNQRNEYQDMKMVLRLNTRALAFLVEKFCTQATANSHGWQYRIENPLQLNHLPLTICRHGFDEDRIQRYQDDHNGVHIHVAMQITHSEWVLETWASSHQHANISPLSGNSQRYYPANHEGRRIQCCQFDCESHLMEDLWREATRSHESKVTRFKHDHNTPLHFTVFWPSDMEHDNGKTEYKSGWQLMFKIEGSVGKECELHTSYFSMKVKYTTVEIQTNPIERMCYAELVYWLRAEMEGVAKDRDHKWAMEHEQYIYSPMEGLINSVEYHMVYPQMARSIAKDRYWTPATLRRIHVWYCREIVQIMWIDHPKGLVCVMPDSNVGALIKWYTHYTDVKLETHQTCRKNNAGMVRDGRKKVAAEVFIIYEMGMPEYKPHEDANRDYRIPIDHPWRPHNNMCFGISGEWRRSCMKHQIQLSRWWANITKIMPMRWSIESGTTLRFVQGMVYRVNRFDDRYPRMNDCYWDAIWVCGAWTMHLTVPDLLMQMHHCMGAIHDVVFSEMGVKIAYQAVWFASVRIPASFQDWFCHEDHFQRGLRKCKLEHNDVLYETWIAMEPASYHTVHHCWHWREVFCHPCVFIHQMYSYPEFHIPDVELSLHYSQIWMNRETGWSPIRMELMCTSAPPNPDGGTPLLSFWYFGFIIQRHCGCETIMIGQWDTCFNSHHDVWAVWVHPENSQFTKERICCASFTDNSDTRECKQTVQHSSPGPTELWADSKTQVQWWYISQWLGKGLMYSEFQVSISGLSTPMLVYWLPNSMIEMNDVMEILNSPAGAQITTCIAGVNFRMIFETSWRTWYINRH

Output:

97283.202

PRTM.rb:

module Rosalind
  RESIDUE_BY_MONOISOTOPIC_MASS = {
    71.03711  => ["A"],
    103.00919 => ["C"],
    115.02694 => ["D"],
    129.04259 => ["E"],
    147.06841 => ["F"],
    57.02146  => ["G"],
    137.05891 => ["H"],
    113.08406 => ["I", "L"],
    128.09496 => ["K"],
    131.04049 => ["M"],
    114.04293 => ["N"],
    97.05276  => ["P"],
    128.05858 => ["Q"],
    156.10111 => ["R"],
    87.03203  => ["S"],
    101.04768 => ["T"],
    99.06841  => ["V"],
    186.07931 => ["W"],
    163.06333 => ["Y"]
  }

  MONOISOTOPIC_MASS_BY_RESIDUE = RESIDUE_BY_MONOISOTOPIC_MASS.
    flat_map { |c, as| as.map { |a| [a, c] } }.to_h

  def self.problem_prtm(aminoacids_string)
    aminoacids_string.
      scan(/./).
      map { |residue| MONOISOTOPIC_MASS_BY_RESIDUE[residue] }.
      take_while { |aminoacid| aminoacid != "" }.
      inject(0){ |sum, x| sum + x }
  end
end

user_input = gets.chomp
puts '%.3f' % Rosalind.problem_prtm(user_input)

The problem statement is quite similar to PROT, so the code looks quite similar to the answers given there.

Basically, I declare a bunch of value-key pairs which get inverted. This saves me from having the same value multiple times in the hash (when multiple residue weigh the same amount).

I'm mainly looking for idiomaticness and re-usability. I'm pretty sure my take_while could be written better and my naming is always open for critique.

About your code:

I am glad you liked the idea of building the inverse structure programmatically, but here I don't think it's necessary :) Only two aminoacids have the same weight, I'd write the reversed hash.
Why residue instead of aminoacid?
I don't see why you need take_while here.
xs.reduce(0) { |acc, x| acc.method(x) } -> xs.reduce(0, :method).
Minor detail: personally I prefer explicit uses of $stdin.readline instead of gets.

I'd write:

module Rosalind
  MONOISOTOPIC_MASS_BY_AMINOACID = {
    "A" => 71.03711,
    "C" => 103.00919,
    "D" => 115.02694,
    "E" => 129.04259,
    "F" => 147.06841,
    "G" => 57.02146,
    "H" => 137.05891,
    "I" => 113.08406,
    "L" => 113.08406,
    "K" => 128.09496,
    "M" => 131.04049,
    "N" => 114.04293,
    "P" => 97.05276,
    "Q" => 128.05858,
    "R" => 156.10111,
    "S" => 87.03203,
    "T" => 101.04768,
    "V" => 99.06841,
    "W" => 186.07931,
    "Y" => 163.06333,
  }

  def self.problem_prtm(aminoacids_string)
    aminoacids_string.
      chars.
      map { |aminoacid| MONOISOTOPIC_MASS_BY_AMINOACID[aminoacid] }.
      reduce(0, :+)
  end
end

protein = $stdin.readline.rstrip
weight = '%.3f' % Rosalind.problem_prtm(protein)
puts(weight)

Same idea, slightly different approach:

module Rosalind
  ...
  def self.problem_prtm(aminoacids_string)
    aminoacids = aminoacids_string.chars
    masses = MONOISOTOPIC_MASS_BY_AMINOACID.values_at(*aminoacids)
    masses.reduce(0, :+)
  end
end


External "bucket" sort I'm working on a CTags plugin for Sublime Text. One of the open issues revolves around the fact that an in-memory sort of a tag file can cause some form of memory or buffer overflow errors (the stack in Sublime Text is limited to ~25 MB apparently). I've devised a simple external bucket sort to resolve this issue:

#/usr/bin/env python
#
# CSV external bucket sort

import codecs
import tempfile
import os

# column indexes
SYMBOL = 0
FILENAME = 1

TAG_FILE = 'tags'
OUT_FILE = ''.join([TAG_FILE, '_sorted_by_file'])

def sort():
    """External bucket sort of tab delimited CTag files"""
    temp_files = {}

    def get_file(filename):
        """Get a file from the store of files"""
        if not filename in temp_files:
            temp_files[filename] = tempfile.NamedTemporaryFile(delete=False)
            # close and reopen using codecs to avoid problems described here:
            #   http://stackoverflow.com/a/10490859/613428
            temp_files[filename].close()
            temp_files[filename] = codecs.open(
                temp_files[filename].name, 'w+', 'utf-8', 'ignore')
        return temp_files[filename]

    try:
        with codecs.open(TAG_FILE, 'r+', 'utf-8', 'ignore') as file_o:
            for _ in range(6):  # skip the header
                next(file_o)
            for line in file_o:
                temp_file_o = get_file(line.split('\t')[FILENAME])
                split = line.split('\t')
                split[FILENAME] = split[FILENAME].lstrip('.\\')
                temp_file_o.write('\t'.join(split))

        with codecs.open(OUT_FILE, 'w+', 'utf-8', 'ignore') as file_o:
            # we only need to sort the file names - the symbols were already
            # sorted!
            for key in sorted(temp_files):
                temp_files[key].seek(0)
                file_o.write(temp_files[key].read())
    finally:
        for key in temp_files:
            temp_files[key].close()
            os.remove(temp_files[key].name)

    os.remove(OUT_FILE)  # just for testing - remove when done

Here's what I intended to do:

For each line (a.k.a. tag) in a tag file
    Read line into memory
    Get the filename from the line.
    Use filename as key to a temp file and write whole this line to said file
Sort final list of keys (i.e. filenames)
For each file in sorted key list
    Append contents of file to sorted tag file
Close all files and delete temp files

And the performance:

$ python -m timeit -n 100 'import external_sort; external_sort.sort()'
100 loops, best of 3: 12 msec per loop

My question is this: is this as performant as it could be? Could I improve it in any way, e.g. missing corner cases?

NOTE: I'm aware that it's not really a true bucket sort as the buckets aren't arbitrary or equally sized (a project with many small files and few large files would result in unbalanced buckets). However, I'm counting on there being many average sized files and none big-enough to fill memory by themselves.

It is not necessary to decode and encode UTF-8, as split('\t') and lstrip('.\\') work fine on UTF-8-encoded strings.
The programs keeps an unlimited number of temporary files open simultaneously and will fail after exceeding the platform's maximum for open files. An approach where only a certain number of most recently used files are kept open could be appropriate here.
file_o.write(temp_files[key].read()) reads the entire file in memory. To conserve memory, use a loop and give a block size argument to read.


Maintainable and Adaptable FizzBuzz implementation Something I haven't often seen in FizzBuzz implementation is a focus on making it flexible and maintainable. I have attempted to create a version that prioritizes these, such as by making it easier to change the conditions and add more conditions. What else could I do to work towards these criteria?

public class V3_ifFunc
{
   public static void main(String[] args)
   {
       String out;
       int i;
       for(i=0; i<=100; i++)
       {
           out = "";
           if(divisible(i, 3)){
               out += "Fizz";
           }
           if(divisible(i, 5)){
               out += "Buzz";
           }
           if(out.equals("")){
               out += Integer.toString(i);
           }
           System.out.println(out);
       }
   }
   public static boolean divisible(int i, int by)
   {
       if(i%by==0){
           return true;
       }else{
           return false;
       }
   }
}

You could make some sort of struct or class that stores the text printout and the number i should be divisible by. Then you just need to create as many instances of the class as you need and loop through them.

One other approach is to have a hashtable and store the keys in an array that you loop through. You can use whichever one you feel makes the most sense but the first method should technically use up less memory because you only store the text once for each condition. I might be wrong though, I don't really use Java.

Example (Not tested and not written by a Java developer):

class Condition {
    String printout;
    int div;
}

Condition[] conditions = { {"Fizz", 3}, {"Buzz", 5} };
int loops = 100;

public static void main() {
   for(int i = 0; i < loops; i++) {
       String printOut = "";
       for(int j = 0; j < conditions.length; i++) {
           if(i % conditions[j] == 0) {
              printOut += conditions[j].printout;
           }
       }
       if(printOut == "") {
          System.out.println(Integer.toString(i));
       } else {
          System.out.println(printOut);
       }
   }
}

Then if you want to you could wrap it in a function and call it with the conditions array and number of loops as parameters.

You can also shorten your divisible to:

public static boolean divisible(int a, int b) {
   return ((a % b) == 0);
}

But I think that you don't really need that function because you can just use the % operator and your code would be cleaner and quicker to understand. Just for future consideration.

Hope this answers your question.


Transforming from spherical coordinates to Cartesian coordinates using Eigen I need to transform the coordinates from spherical to Cartesian space using the Eigen C++ Library. The following code serves the purpose:

const int size = 1000;
    Eigen::Array<std::pair<float, float>, Eigen::Dynamic, 1> direction(size);
    for(int i=0; i<direction.size();i++)
    {
            direction(i).first = (i+10)%360; // some value for this example (denoting the azimuth angle)
            direction(i).second = (i+20)%360; // some value for this example (denoting the elevation angle)

     }

    Eigen::MatrixX<T1> transformedMatrix(3, direction.size());
    for(int i=0; i<transformedMatrix.cols(); i++)
    {
        const T1 azimuthAngle = direction(i).first*M_PI/180;    //converting to radians
        const T1 elevationAngle = direction(i).second*M_PI/180; //converting to radians

        transformedMatrix(0,i) = std::cos(azimuthAngle)*std::cos(elevationAngle);
        transformedMatrix(1,i) = std::sin(azimuthAngle)*std::cos(elevationAngle);
        transformedMatrix(2,i) = std::sin(elevationAngle);
    }

I would like to know a better implementation is possible to improve the speed. I know that Eigen has supporting functions for Geometrical transformations. But I am yet to see a clear example to implement the same. Is it also possible to vectorize the code to improve the performance?

Note: This is a duplicate posting. I think that the question will be more relevant on this site.

There's not really much to review here. While I have some experience with Eigen, I have no idea what SSPL is. I'm going to assume SSPL:MatrixX is basically Eigen::Matrix3Xf.

const int size = 1000;

This should probably use constexpr rather than const.

for(int i=0; i<direction.size();i++)

It's been a while since I've used Eigen, but I believe you have a bug in this for prologue. If I recall, the return type of size() for Array types is not int. I think it is actually std::ptrdiff_t (or possibly std::size_t), but it's user-customizable. If it is std::ptrdiff_t (for example), and std::ptrdiff_t is larger than int (as it is on some platforms, I think including 64-bit Windows), then you will get UB if the values get cut off.

The way to fix this is to use decltype:

for (decltype(direction.size()) i = 0; i < direction.size(); ++i)

Now the first loop is just generating test data, so let's skip down to the next loop.

const T1 azimuthAngle = direction(i).first*M_PI/180;    //converting to radians

What's infinitely better than:

auto y = /* expression with x */; // convert x to foo

is:

auto y = convert_to_foo(x);

In other words, since you're converting to radians, you should have:

constexpr auto to_radians(float v) noexcept
{
    return (v * pi<float>) / 180.0f;
}

Now, in addition, M_PI is not actually portable. If you don't care, fine, but if you care about portability, you can define a π constant either as:

constexpr auto pi = 3.14159265358979f;

or, better, as:

template <typename T>
constexpr auto pi = T(3.14159265358979L); // add as many digits of precision as you please

in which case you can even make the conversion function a template:

template <typename T>
// possibly constrain T
constexpr auto to_radians(T const& v) noexcept((v * pi<T>) / T(180))
{
    return (v * pi<T>) / T(180);
}

Any way you do it, you should end up with:

const T1 azimuthAngle = to_radians(direction(i).first);
const T1 elevationAngle = to_radians(direction(i).second);

Now in the next few lines you need the sin and cos of the azimuth and elevation. You might as well precalculate them - especially since you reuse some of them:

auto const cos_azimuth = std::cos(azimuthAngle);
auto const sin_azimuth = std::sin(azimuthAngle);
auto const cos_elevation = std::cos(elevationAngle);
auto const sin_elevation = std::sin(elevationAngle);

transformedMatrix(0,i) = cos_azimuth * cos_elevation;
transformedMatrix(1,i) = sin_azimuth * cos_elevation;
transformedMatrix(2,i) = sin_elevation;

But transforming coordinate systems seems both like something you can reuse and - more importantly - something you can test in isolation. So this should be a function:

template <typename Pair>
auto spherical_to_cartesian(Pair const& spherical)
{
    const auto azimuthAngle = to_radians(std::get<0>(spherical));
    const auto elevationAngle = to_radians(std::get<1>(spherical));

    auto const cos_azimuth = std::cos(azimuthAngle);
    auto const sin_azimuth = std::sin(azimuthAngle);
    auto const cos_elevation = std::cos(elevationAngle);
    auto const sin_elevation = std::sin(elevationAngle);

    return std::tuple{cos_azimuth * cos_elevation, sin_azimuth * cos_elevation, sin_elevation};
}

Which makes your loop:

for (decltype(transformedMatrix.cols()) i = 0; i < transformedMatrix.cols(); ++i)
{
    std::tie(
            transformedMatrix(0, i),
            transformedMatrix(1, i),
            transformedMatrix(2, i))
        = spherical_to_cartesian(direction(i));
}

Now, if you're asking about how to do geometric transforms with Eigen, that seems more like a Stack Overflow question. As for vectorization, that depends on what SSPL::MatrixX is. But the loop above can be very easily parallelized, because each transform is independent. As for how, the standard way would require that Eigen::Array and SSPL::MatrixX could be used with standard algorithms, in which case the answer would simply be:

// Hypothetical code.
std::transform(std::par_unseq, begin(direction), end(direction), begin(transformedMatrix), spherical_to_cartesian);

Or you could look into OpenMP and parallel for.


Basic implicit tree implementation in Java I tried starting off with something like below. Note that traversal etc. is not included, this is the portion for storage and modifications. It feels like this could be much more elegant at places but not exactly sure how. I'm mainly trying to improve how the code almost repeats for getters and setters for left and right subtrees.

import org.apache.commons.lang3.ArrayUtils;

public class Tree {
  private int[] treeArray;
  private int depth;

  public Tree(int treeDepth) {
    this.depth = treeDepth;
    this.treeArray =  new int[(int) Math.pow(2, treeDepth)];
    treeArray[0] = Integer.MIN_VALUE;
  }

  public int returnRoot() {
    return treeArray[1];
  }

  public int[] getTreeArray() {
    return treeArray;
  }

  public int getDepth() {
    return depth;
  }

  public void setTreeArray(int[] treeArray) {
    this.treeArray = treeArray;
  }

  public Tree returnLeftSubTree() {
    if (depth == 0) return null;

    Tree ret = new Tree(depth - 1);
    for (int i  = 1; i < depth; i ++) {
      for (int j  = 0; j < depth - 1; j ++) {
        ret.getTreeArray()[(int) Math.pow(2, i - 1) + j]
            = treeArray[(int) Math.pow(2, i) + j];
      }
    }

    return ret;
  }

  public Tree returnRightSubTree() {
    if (depth == 0) return null;

    Tree ret = new Tree(depth - 1);
    for (int i  = 1; i < depth; i ++) {
      for (int j  = 0; j < depth - 1; j ++) {
        ret.getTreeArray()[(int) Math.pow(2, i - 1) + j]
            = treeArray[(int) (Math.pow(2, i) + Math.pow(2, i - 1)) + j];
      }
    }

    return ret;
  }

  public void setLeftSubTree(Tree leftSub) {
    if (leftSub.getDepth() >= depth) {
      this.treeArray = ArrayUtils.addAll(
          this.treeArray,
          new int[(int) (Math.pow(2, leftSub.getDepth() + 1) - Math.pow(2, depth))]);
      this.depth = leftSub.getDepth() + 1;
    }

    for (int i  = 1; i < depth; i ++) {
      for (int j  = 0; j < depth - 1; j ++) {
        treeArray[(int) Math.pow(2, i) + j] =
            leftSub.getTreeArray()[(int) Math.pow(2, i - 1) + j];
      }
    }
  }

  public void setRightSubTree(Tree rightSub) {
    if (rightSub.getDepth() >= depth) {
      this.treeArray = ArrayUtils.addAll(
          this.treeArray,
          new int[(int) (Math.pow(2, rightSub.getDepth() + 1) - Math.pow(2, depth))]);
      this.depth = rightSub.getDepth() + 1;
    }

    for (int i  = 1; i < depth; i ++) {
      for (int j  = 0; j < depth - 1; j ++) {
        treeArray[(int) (Math.pow(2, i) + Math.pow(2, i - 1)) + j]
            = rightSub.getTreeArray()[(int) Math.pow(2, i - 1) + j];
      }
    }
  }
}

More efficient and accurate is a bit shift.

this.treeArray =  new int[(int) Math.pow(2, treeDepth)];
treeArray =  new int[1 << treeDepth];

A test is in order for the top:

if (0 > treeDepth || treeDepth > 31) {
    throw new IllegalArgumentException();
}

The name returnRightSubTree is a bit irritating because of that return, synonymical yieldRightSubTree or get/create.

  public Tree returnLeftSubTree() {
    return yieldSubTree(false);
  }

  public Tree returnRightSubTree() {
    return yieldSubTree(true);
  }

  private Tree yieldSubTree(boolean right) {
    if (depth == 0) return null;

    Tree ret = new Tree(depth - 1);
    int[] trees = ret.getTreeArray();
    for (int i  = 1; i < depth; i++) {
      int iTarget = 1 << (i - 1);
      int iSource = (1 << i) + (right ? 1 << (i - 1)) : 0);
      for (int j  = 0; j < depth - 1; j ++) {
        trees[iTarget + j] = trees[iSource + j];
      }
    }
    return ret;
  }

which could be faster using System.arraycopy:

  private Tree yieldSubTree(boolean right) {
    if (depth == 0) return null;

    Tree ret = new Tree(depth - 1);
    int[] trees = ret.getTreeArray();
    for (int i  = 1; i < depth; i++) {
      int iTarget = 1 << (i - 1);
      int iSource = (1 << i) + (right ? 1 << (i - 1)) : 0);
      System.arraycopy(trees, iSource, trees, iTarget, depth - 1);
    }
    return ret;
  }

The i dependent expression before the j loop. As you see, just as i++
one could have (but not necessarily):

    for (int i  = 1, iT = 1; i < depth; i++, iT <<= 1) {
      //int iTarget = iT;
      int iSource = (iT << 1) + (right ? iT : 0);

For the rest:

Tab indentation in java is 4. Historically a tab size of 3 was often for C/C++,
and for academic reasons: "there should be less long code with nesting in favour of
more methods" 4 was chosen; also as 8 was the archaic system tab size.

A tab size of 2 is still seen in XML/HTML, but I would refrain from that. Especially
as with lambdas one gets more unreadable indentations.


Factorisation code running slow I'm still in the process of learning Python and am doing some of the Project Euler problems.

I have made a factorisation algorithm that works; however, it runs really slowly, and I'm not too well-versed on coding for performance. If someone could give me some pointers on how to improve the algorithm's efficiency it would be a huge help.

def factorise(num_to_factorize):
    """This method returns a list of factors for a particular number passed in to the parameters."""
    factors = []
    for i in range(1,math.floor(num_to_factorize/2)+1):  # Loops from 1 to half the number, floors for odd numbers.
        if num_to_factorize % i == 0:  # Factor check.
            factors.append(i)  # Append on to an array to be checked.
    factors.append(num_to_factorize)  # Append the number itself as it is a factor.
    return factors

def prime_factorise(num_to_prime_factorize):
    """This method returns the prime factors of the number passed in to the method as a parameter."""
    prime_factors = []
    for factor in factorise(num_to_prime_factorize):  # Loop through each factor.
        if factorise(factor) == [1, factor] or factorise(factor) == [1]:  # if prime factors
            prime_factors.append(factor)
    return prime_factors

I'll give you some hints/comments:

factorise(factor) == [1, factor] or factorise(factor) calculates factorise(factor) twice, consider saving its result as a variable and re-using it instead of incurring the cost twice.
You're treating 1 as a special case (in factorise(factor) == [1]), but it isn't. 1 can be factored into anything, but should not be considered a prime factor. Why? Because 1 isn't a prime.
I think your algorithm might be slightly off. For example, if I say prime_factorise(8) I get [1, 2], but I believe the answer should be [2, 2, 2] since 2 is prime, and 2*2*2 = 8.
Overall, my biggest hint would be try to improve your algorithm by thinking of the operation as a reduction from some number, into its prime factors. Think _how can I transform num_to_prime_factorize into a list of its prime factors ("transform" being key).

I don't want to give away the answer, so, hopefully these tips will suffice to get you in the right direction.


FizzBuzz implementation in Java without modulus operator I was trying to implement FizzBuzz without modulus operator.

Areas of concern: inner for loop maybe not needed? seq[] array maybe not needed?

class FizzBuzzWithoutModulus{
    public static void main(String[] args){

    int[] fizzbuzz = new int[101];
    int[] buzz = new int[101];
    int[] fizz = new int[101];

    int[] seq = new int[101];
    for(int i=1; i<=100; i++){
      seq[i] = i;
    }

    for(int i=1; i<=100; i++){
      for(int j=1; j<seq.length; j++){
        if((i/15.0) == seq[j]){
          fizzbuzz[i] = i;
        }
        else if((i/5.0) == seq[j]){
          buzz[i] = i;
        }
        else if((i/3.0) == seq[j]){
          fizz[i] = i;
        }
      }
    }
    for(int i=0; i<=100; i++){
      if(fizzbuzz[i]!=0){
        System.out.println(fizzbuzz[i] + " fizzbuzz");
      }
      else if(buzz[i]!=0){
        System.out.println(buzz[i] + " buzz");
      }
      else if(fizz[i]!=0){
        System.out.println(fizz[i] + " fizz");
      }
    }
  }
}

I think this implementation is really too confusion for the problem at hand, although I think it is quite an interesting solution.

As you said it yourself, integer division loses the fractional part (rounds down towards 0). You can actually use this for your solution! Because if a number is not evenly divisible, multiplying with the division result will not give you back the original number, it will be smaller!

So a way to do this would be:

public static String fizzBuzz(final int number) {
    if (number / 15 * 15 == number) {
        return "FizzBuzz";
    } else if (number / 5 * 5 == number) {
        return "Buzz";
    } else if (number / 3 * 3 == number) {
        return "Fizz";
    }
    return Integer.toString(i);
}

Note that a number divisible by 3 and 5 is also divisible by 15.


Dynamic Routing PHP MVC I'm creating a simple MVC framework for a new version of my website I'm working on. Currently I have simple routing, such as /about-us etc ...

I've just implemented 'my version' of dynamic routing. I have no idea whether or not my implementation is idiotic or not. I made it up as I went along, so I've no idea if this is a horrible way to implement dynamic routing or not.

I'd be grateful if someone could review my implementation and provide some feedback.

  Here's 'Routes.php', this stores the user-defined routes.

// This is the index page. The first route.
Route::set('', function() {
  View::make('Root');
});
// This is a test route, leading to an about-us page.
Route::set('about-us', function() {
  View::make('AboutUs');
});
/*
 * This is an example of a dynamic route. In this example,
 * this route would display a users profile page.
*/
Route::set('user/<1>', function() {
  Route::dyn('user/<1>');
  View::make('UserProfile');
});

The method View::make() has nothing to do with the routing. It's just designed to create the HTML page and the Controller for the page.

The method Route::set() is how I create all routes. Simple routes are created entirely with Route::set(). The dynamic routes are created by executing the closure. Route::set() executes the closure for simple and dynamic routes, for simple routes the closure creates the view and for dynamic routes it swaps <1> for a parameter from the URL and then creates the view.

The reason the syntax is odd, <1>, is because if you want multiple dynamic parameters you can use <2> etc ...

  Here's an example

In this example the URL string might look like this user/francis/settings/something-else

Route::set('user/<1>/settings/<2>', function() {
  Route::dyn('user/<1>/settings/<2>');
  View::make('UserProfile');
});

  Here's the implementation of the Route::dyn() method

public static function dyn($dyn_routes) {
$route_components = explode('/', $dyn_routes);
$uri_components = explode('/', substr($_SERVER['REQUEST_URI'], strlen(BASEDIR)-1));
for ($i = 0; $i < count($route_components); $i++) {
  if ($i+1 <= count($uri_components)-1) {
    $route_components[$i] = str_replace("<$i>", $uri_components[$i+1], $route_components[$i]);
  }
}
$route = implode($route_components, '/');
self::registerRoute($route);
}

$route_components is what we pass to Route::dyn(). $uri_components is the string we get from the URL, i.e user/francis/settings/something-else. This function converts $route_components so that it is the same as $uri_components. The reason for doing so is that I add all routes to a global array called $Routes. When someone visits a page the URL is checked against the valid routes in the $Routes array.

When the user visits user/francis I want the method Route::set('user/<1>') to run so that the profile page is displayed. It all works with my implementation, I'm just wondering if there is a better way.

Thanks,
Francis

Looking pretty neat!

But it is missing some important functionality:

Ability to define the method of request. There are many kinds of HTTP request methods. Click here to learn more.
The fact that you have to call the "dyn" method for dynamic URLs is just not my taste. Other routers are capable of determining if the URL is dynamic or not. This is pretty easy to implement. You could do it like this. You can use REGEX, specifically, you can use the pattern /^<+(.*)+>$/. What you do is, you split the URL with / and use the above REGEX pattern to determine if  that particular part of the URL is dynamic or not. You can then get the value by getting the string in the same spot from the typed in URL comparing with the URL route defined.
Another great feature would be Request and Response objects. What you would do is, in each request, you pass a Request object with all the HTTP data and that controller method would return an instance of a Response object  that would tell the router what to do. Basically, it could be what data to return, HTTP Status Code, etc.

Otherwise, good job!