Check if modelitem refers to currently logged in user with mvc identity I'm using MVC in order to generate a couple of views within my webpage. For a way of logging in, I've decided to use Asp.net's Identity, which I am very happy with.

However, I'm trying to find an efficient/ 'normal' way of testing if the user is logged in, and are looking at info that refers to them.

As a test, I found that:

 if (User.Identity.Name == (Html.DisplayFor(modelItem => item.Email).ToString()))
        {

THIS IS ME

        }

razor code works for this.

However, this seems quite inefficient, and I was wondering if there is a more appropriate way of telling 'is this their row in which they can edit'.

I would add the logic in the controller

 public ActionResult(){
    ApplicationDbContext db = new ApplicationDbContext();
    List<ApplicationUser> model = db.Users.ToList();

    // Viewbag just being an example for a model. It would be better to
    // pass this into a property in the model that is being passed to the view.
    foreach (var item in model)
    {
        ViewBag.UserEmail = System.Web.HttpContext.Current.User.Identity.Name
                         == item.Email ? item.Email : null;
    }
  // If we were returning a viewmodel we could add this check into a
  // ApplicationUser ( public ApplicationUser AppUser {get; set;})
  // property which could then be accessed in the model as Model.AppUser. This
  // would hold all the values for the application user. If that is
  // something you would need

 return View(model);
 }

This will assign the ViewBag.UserEmail to either the currently logged in users email or null. I added the full namespace because sometimes Current is not directly accessible

And in the view

 if(Viewbag.UserEmail != null)
 {

 THIS IS ME

 }

This then just checks if the ViewBag is null and displays a code block if it is not.

This will still work in a loop. Because the ViewBag holds the current users email which can be checked against in a foreach

@foreach ( var item in Model){

@item.Name

       if(ViewBag.UserEmail == item.Email){
          <button>Edit </button> // Should only be visible to logged in user
       }

       // Or Like
        @( ViewBag.User == item.Email ?
          Html.Raw("<button class='btn btn-primary'>Edit</button>") :
          Html.Raw("<button class='btn btn-primary'>View</button>"))
}


Promise-driven animation Inspired by this question on Stack Overflow, I've attempted to code such animation, mostly to get some more practice with async, promises and Q.js:

(Live demo)

function addOutput(s) {
  $('<div>').text(s).appendTo(wnd);
  //return Q.defer().promise;
  return Q.delay(100).then(function() { return addPrompt(); });
}

function addInput(s) {
  var l = $('.prompt:last');
  return addLettersRecursive(l, s);
}

function addPrompt() {
  var prompt = "kos@codepen % ";
  var l = $('<div>').text(prompt).addClass('prompt').appendTo(wnd);
  return Q.delay(900);
}

function addLettersRecursive(container, s) {
  container.append(s.charAt(0)); // dangerous :(
  var row_complete = Q.defer();
  Q.delay(100).then(function() {
    if (s.length <= 1) {
      Q.delay(300).then(function() {
        row_complete.resolve();
      });
    }
    addLettersRecursive(container, s.substr(1)).then(function() {
      row_complete.resolve();
    })
  });
  return row_complete.promise;
}

// Usage

addPrompt(">>> ")
.then(function() { return addInput("whoami"); })
.then(function() { return addOutput("kos"); })
.then(function() { return addInput("uname -a"); })
.then(function() { return addOutput("Javascript in codepen.io, powered by Q.js"); })
.then(function() { return addInput("raz dwa"); })
.then(function() { return addOutput("zsh: command not found: raz"); })
.then(function() { return addInput("trzy cztery"); })
.then(function() { return addOutput("zsh: command not found: trzy"); })
.done();

I'm not really satisfied by this implementation, though. How can I simplify it? Here are the specific concerns I have:

I implemented addInput in terms of a recursive helper function addLettersRecursive which still is overly complicated according to my gut feeling.
There's this .then(function() { return function_that_returns_promise(args); }); pattern all over, which seems like a very verbose way of chaining behaviour.

What I have tried:

I attempted to unwind the recursion by stacking the promises one on top of another in a loop, which was a bit tricky to implement because of the selective closure involved. There's some improvement but still looks messy:

function addInput(s) {
  var container = $('.prompt:last');
  var d = Q.delay(0);
  for (var i=0; i<s.length; ++i) {
    d = d.then(function(i) { return function() { // pass i by value
      container.append(s.charAt(i));
      return Q.delay(100);
    }}(i));
  }
  return d.then(function() { return Q.delay(300); });
}

I'm not sure if this kind of pattern is typical to promise-driven code, or I'm just doing something sub-optimally. One thing that came to my mind is this substitution:

  /* before */ .then(function() { return addInput("whoami"); })
  /* after  */ .then(addInput.bind(null, "whoami"))

  /* before */ return d.then(function() { return Q.delay(300); });
  /* after  */ return d.then(Q.delay.bind(Q, 300));

Is it a good track?

Generally I haven't seen much "real life" uses of promises yet except the book cases, so if anything else looks out of the ordinary or sub-optimal, please raise it.

This stuff is hard to get right, at this point good promise driven code seems more like an art than engineering.

From a CodeReview perspective, your code is fairly easy to follow, except for one thing which got me stumped for a little while, row_complete should really be named char_complete, and then suddenly addLettersRecursive will make more sense.

There are at least 3 additional ways you can deal with .then(function() { return function_that_returns_promise(args); });

Change your functions ( like addInput ) to return a function, like this:

function addInput(s) {
  return function addInputWrapper(){
    var l = $('.prompt:last');
    return addLettersRecursive(l, s);
  }
}

Then you can call simply .then( addInput( 'whoami' ) )
Create a generic wrapper function like this:

function wrap( f ){
  var args = Array.prototype.slice.call(arguments,1);
  return function wrapper(){
    return f.apply( this, args );
  }
}

Then you can call .then( wrap( addInput, 'whoami' ) )
Generate a lambdafy function like this :

function lambdafy( f )
{
  var lambda = function(){
    var args = Array.prototype.slice.call(arguments);
    return function(){
      f.apply( this , args );
    }
  }
  return lambda;
}

Then you can addInput = lambdafy( addInput ) and .then( addInput( 'whoami' ) )


Singleton MongoClient instance provider A Java service that connects to MongoDB in production, but opened connection count is too much. It affects on MongoDb performance, so I have come up with a solution that provides only one MongoClient instance so which will reduce the connections.
Here is my code, is it a good way to provide a singleton object from 3rd party library?
import com.mongodb.client.MongoClient;
import com.mongodb.client.MongoClients;

public class MongoClientFactory {

    private static volatile MongoClient instance = null;

    private MongoClientFactory() {}

    public static MongoClient getInstance(String connectionString) {
        if (instance == null) {
            synchronized(MongoClientFactory.class) {
                if (instance == null) {
                    instance = MongoClients.create(connectionString);
                }
            }
        }
        return instance;
    }
}

There are already links on this page to reasons why singletons should not be used. I will not repeat those.
is it a good way to provide a singleton object from 3rd party library?
No. Your singleton has an undocumented internal state which affects the processing of the input parameters. On the first invocation, the getInstance(String connectionString) methods returns a connection to the database requested by the caller. On subsequent invocations it returns a connection to the database requested by the first caller, regardless of what connection string is provided to it. You could document it but it doesn't fix the issue of this being a very error prone solution: your method does something unexpected if something that cannot be verified during run time happened before it was called.


Mergesort with an inversion counter I'm new to Lisp and I'm yet to wrap my head around the lisp way of writing programs. Any comments regarding approach, style, missed opportunities appreciated:

In particular, please advice if I build results list correctly ((setf merged-list (append merged-list (list a)))).

;;;; Count inversions
(defun small-list (list)
  (or (null list) (null (rest list))))

(defun split-in-half (list)
  (let ((mid (ceiling (length list) 2)))
    (values (subseq list 0 mid)
            (subseq list mid))))

(defun count-inversions (list)
  (if (small-list list) (list list 0)
    (multiple-value-bind (lower upper) (split-in-half list)
      (merge-inversions
        (count-inversions lower)
        (count-inversions upper)))))

(defun merge-inversions (lower-pair upper-pair)
  (let ((lower (first lower-pair))
        (upper (first upper-pair))
        (merged-list '())
        (num-inversions 0))
    (loop while (not (and (null lower) (null upper)))
          do (cond
               ((null lower) (let ((a (first upper)))
                               (setf merged-list (append merged-list (list a)))
                               (setf upper (rest upper))))
               ((null upper) (let ((a (first lower)))
                               (setf merged-list (append merged-list (list a)))
                               (setf lower (rest lower))))
               ((< (first lower)
                   (first upper)) (let ((a (first lower)))
                   (setf merged-list (append merged-list (list a)))
                   (setf lower (rest lower))) )

               (t (let ((a (first upper)))
                    (setf merged-list (append merged-list (list a)))
                    (setf upper (rest upper))
                    (incf num-inversions (length lower))))))
    (list merged-list (+ (second lower-pair) (second upper-pair) num-inversions)))

Use nconc instead of append for speed in

(setf merged-list (append merged-list (list a)))

Note that you need setf with nconc only if merged-list is nil.
Use pop: instead of

(let ((a (first lower)))
  (setf merged-list (append merged-list (list a)))
  (setf lower (rest lower))))

you can write

(setf merged-list (nconc merged-list (list (pop lower))))
The combination of let and loop creates extra indentation for no good reason. You can use with clause in loop to create bindings instead, or use the do macro.
Usually predicates are named with a -p suffix, so I suggest that you rename your small-list to small-list-p.
Please fix line breaks and indentation in the third cond clause.


Display class containing a Canvas in a JFrame I've been using this class for so long, I never thought about how terribly big and how bad it looks. Looking back on it, this class looks so out-of-date, and nothing explains itself; I want to get it looking much better, not with all the extra junk that it doesn't need.
Here's the class:

import java.awt.Canvas;
import java.awt.Dimension;

import javax.swing.JFrame;

public class Display {

    private JFrame frame;
    private Canvas canvas;

    private String title;
    private int width, height;

    public Display(String title, int width, int height){
        this.title = title;
        this.width = width;
        this.height = height;

        createDisplay();
    }

    private void createDisplay(){
        frame = new JFrame(title);
        frame.setSize(width, height);
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.setResizable(false);
        frame.setLocationRelativeTo(null);
        frame.setVisible(true);

        canvas = new Canvas();
        canvas.setPreferredSize(new Dimension(width, height));
        canvas.setMaximumSize(new Dimension(width, height));
        canvas.setMinimumSize(new Dimension(width, height));
        canvas.setFocusable(false);

        frame.add(canvas);
        frame.pack();
    }

    public Canvas getCanvas(){
        return canvas;
    }

    public JFrame getFrame(){
        return frame;
    }

}

Is there anything here I can condense, or any way to make it look cleaner?

one minor issue - no redundant code...

canvas = new Canvas();
canvas.setPreferredSize(new Dimension(width, height));
canvas.setMaximumSize(new Dimension(width, height));
canvas.setMinimumSize(new Dimension(width, height));
canvas.setFocusable(false);

you should use

canvas.setPreferredSize(dim); //see below
canvas.setMaximumSize(dim);
canvas.setMinimumSize(dim);

that leads to another issue:
create a default constructor that doesn't require a size

private static final int DEFAULT_WIDTH = 100;
private static final int DEFAULT_HEIGHT = 100;
public Display(String title){
    this(title, DEFAULT_WIDTH, DEFAULT_HEIGHT);
}

and use this parametrized constructor:

//private int width, height; TODO remove: no more required
private static final Dimension dim;
public Display(String title, int width, int height){
    this.dim = new Dimension(width, height);
    createDisplay();
}

another issue would be removing unrequired attributes - whats the sense of keeping the title? just forward it...

//private String title; TODO remove: no more required
public Display(String title, int width, int height){
    this.dim = new Dimension(width, height);
    createDisplay(title);
}

private void createDisplay(final String title){
    frame = new JFrame(title);
    ...
}


Checking and returning Xml from a byte array I have a byte array in a C# program.

I want to determine as quickly as possible if the content is Xml. If it's the case, I also want to return the Xml itself.

By now I have this method :

    protected bool TryGetXElement(byte[] body, out XElement el)
    {
        el = null;
        // if there is no data, this is not xml :)
        if (body == null || body.Length == 0)
        {
            return false;
        }

        try
        {
            // Load the data into a memory stream
            using (var ms = new MemoryStream(body))
            {
                using (var sr = new StreamReader(ms))
                {
                    // if the first character is not <, this can't be xml
                    var firstChar = (char)sr.Peek();
                    if (firstChar != '<')
                    {
                        return false;
                    }
                    else
                    {
                        // ultimately, we try to parse the XML
                        el = XElement.Load(sr);
                        return true;
                    }
                }
            }
        }
        catch
        {
            // if it failed, we suppose this is not XML
            return false;
        }
    }

Is there potential improvement?

Why not verify if a root node exists rather than check for a '<' ?

    public bool GetRootNode(XmlReader reader)
    {
        bool isValid;
        try
        {
            while (reader.Read())
            {
                if (reader.NodeType == XmlNodeType.Element)
                {
                    isValid = true;
                    break;
                }
            }
        }
        catch (XmlException x)
        {
            throw new XmlException(x.Message);
        }

        return isValid;
    }

You could also use the chain of responsibilities pattern to determine which parser you want to use.

EDIT

Not sure if this will work, but here is an example on using the above code:

protected bool TryGetXElement(byte[] body, out XElement el)
{
    el = null;
    // if there is no data, this is not xml :)
    if (body == null || body.Length == 0)
    {
        return false;
    }

    try
    {
        // Load the data into a memory stream
        using (var ms = new MemoryStream(body))
        {
            using (var sr = new StreamReader(ms))
            {
                XmlReaderSettings settings = XmlReaderSettings { CheckCharacters = true; };
                using(XmlReader reader = XmlReader.Create(ms, settings))
                {
                    if (!GetRootNode(reader))
                    {
                        return false;
                    }
                    else
                    {
                        // ultimately, we try to parse the XML
                        el = XElement.Load(sr);
                        return true;
                    }
                }
            }

        }
    }
    catch
    {
        // if it failed, we suppose this is not XML
        return false;
    }
}


Greedy algorithm in Ruby - any suggestions? Here is my implementation of the greedy algorithim in Ruby. Do you have any suggestions?

class Greedy
  def initialize(unit, total, *coins)
    @total_coins = 0
    @unit = unit
    @total = total
    @currency = coins.sort
    @currency = @currency.reverse
    unless @currency.include?(1)
      @currency.push(1)
    end
  end
  def sort_coins
    @currency.each do |x|
      @pos = @total / x
      @pos = @pos.floor
      @total_coins += @pos
      @total -= x * @pos
      puts "#{@pos}: #{x} #{@unit}"
    end
    puts "#{@total_coins} total coins"
  end
end

@currency could be renamed @currencies, or perhaps better, @denominations.  Naming an array in the plural leads to easy naming when iterating, e.g.:

@currencies.each do |currency|

instead of:

@currency.each do |x|

The name of method sort_coins doesn't match what it's doing.  Whether or not it's sorting, it's also printing.  A better name might be print_coins.

@quotient might be a better name for @pos.

Most of the variables in sort_coins do not need to be member variables: @pos and @total_coins should lose the @.  And, if you decide to make it an argument to sort_coins (see below), @total as well.

This:

@currency = coins.sort
@currency = @currency.reverse

Could be this:

@currency = coins.sort.reverse

The initialization of @total_coins needs to move from the constructor to sort_coins.  This prevents a bug triggered by calling sort_coins more than once.

Consider passing total to sort_coins rather than to the constructor.  This would allow sort_coins to be called on an instance of Greedy with a different total each time, and no need to create a new instance of Greedy for each.  It would also get the variable's initialization closer to where it is used, making for simpler code.

Consider the constructor argument for the set of coins bing simply coins rather than *coins, because it makes the calling code easier to understand.  E.g.,

Greedy.new('cents', 71, 1, [5, 10, 25, 50]).sort_coins

vs.

Greedy.new('cents', 71, 1, 5, 10, 25, 50).sort_coins

Put a line of white space between methods, and at the top and bottom of the class definition.

If all acted upon, these suggestions yield:

class Greedy

  def initialize(unit, coins)
    @unit = unit
    @demoninations = coins.sort.reverse
    unless @demoninations.include?(1)
      @demoninations.push(1)
    end
  end

  def print_coins(total)
    total_coins = 0
    @demoninations.each do |demonination|
      quotient = (total / demonination).floor
      total_coins += quotient
      total -= demonination * quotient
      puts "#{quotient}: #{demonination} #{@unit}"
    end
    puts "#{total_coins} total coins"
  end

end

Greedy.new('cents', [1, 5, 10, 25, 50]).print_coins(72)
#=> 1: 50 cents
#=> 0: 25 cents
#=> 2: 10 cents
#=> 0: 5 cents
#=> 2: 1 cents
#=> 5 total coins


Finite State Machine in C++ I made a state machine.

You can press 0 and 1 to switch between the hypothetical menu and playing state.

I hope you can help me improve it further.

StateManager.h

#pragma once

#ifndef StateManager_H
#define StateManager_H

#include <iostream>
#include "State.h"

/*
State classes
*/

class StateManager {
public:
    // Define m_current_state upon the creation of this class (since that prevents there being a nullptr error in change_state
    StateManager(State* state)
        : m_current_state(state)
    {}

    void change_state(State* state) {
        m_current_state->on_exit();
        m_current_state = state;
        m_current_state->on_enter();
    }

    void update_state(StateManager* manager) {
        m_current_state->on_update(manager);
    }

    State* get_state() {
        return m_current_state;
    }

private:
    State* m_current_state;
};

#endif

State.h

#pragma once

#ifndef State_H
#define State_H

#include "StateManager.h"

// I'm using forward declaration here since I've had a problem with circular dependency
class StateManager;

class State {
public:
    // All states have to have these three functions
    virtual void on_enter() = 0;
    virtual void on_update(StateManager* state_manager) = 0;
    virtual void on_exit() = 0;
};

#endif

IntroState.h

#pragma once

#ifndef IntroState_H
#define IntroState_H

#include "StateManager.h"
#include "State.h"
#include "GameState.h"

/*
Basic test state for demonstration
*/

class MenuState : public State {
public:
    MenuState();

    virtual void on_enter() override;

    virtual void on_update(StateManager* state_manager) override;

    virtual void on_exit() override;

private:
    bool should_change_state;
};

#endif

Introstate.cpp

#include "IntroState.h"

MenuState::MenuState()
    : should_change_state(false)
{}

void MenuState::on_enter(){
    std::cout << "Entering the menu state" << std::endl;
}

void MenuState::on_update(StateManager* state_manager){
    // update the menu
    std::cout << "Should the state be changed to the playing state" << std::endl;
    std::cin >> should_change_state;
    if (should_change_state == true) {
        state_manager->change_state(new PlayingState());
    }
}

void MenuState::on_exit(){
    std::cout << "Exiting the menu state" << std::endl;
}

GameState.h

#pragma once

#ifndef GameState_H
#define GameState_H

#include "IntroState.h"
#include "State.h"

/*
Basic test state for demonstration
*/

class PlayingState : public State {
public:
    PlayingState();

    virtual void on_enter() override;

    virtual void on_update(StateManager* state_manager) override;

    virtual void on_exit() override;

private:
    bool should_change_state;
};

#endif

GameState.cpp

#include "GameState.h"

PlayingState::PlayingState()
    : should_change_state(false)
{}

void PlayingState::on_enter() {
    std::cout << "Entering the playing state" << std::endl;
}

void PlayingState::on_update(StateManager* state_manager) {
    // update the menu
    std::cout << "Should the state be changed to the menu state" << std::endl;
    std::cin >> should_change_state;
    if (should_change_state == true) {
        state_manager->change_state(new MenuState());
    }
}

void PlayingState::on_exit() {
    std::cout << "Exiting the playing state" << std::endl;
}

Source.cpp

#include "StateManager.h"
#include "State.h"
#include "IntroState.h"

int main() {
    StateManager* manager = new StateManager(new MenuState());

    int exit = 0;
    while (exit != 1) {
        manager->update_state(manager);
    }

    return 0;
}

Thanks!

Last things first

Before I forget about it (because it's peripheral in your code), I'll point out the new StateManager(new MenuState());, which is among the don't of C++. Raw pointers are dangerous! if you don't delete the allocated memory afterwards, you'll leave a leak (and you didn't). So use smart pointers (std::unique_ptr or std::shared_ptr) instead. That's quite an interesting topic, but you'll have to study it for yourself. Or better still, allocate your objects on the stack (State_manager manager;)

The design

  manager->update_state(manager);

is a bit weird, don't you think? What would it mean to write manager->update_state(other_manager)? Probably not much. So you could rewrite the update_state method like this:

void update_state() {
    m_current_state->on_update(this); // instead of current->on_update(removed_argument)
}

It's cleaner and completely equivalent. But then you have to write such things as:

while (exit != 1) {
    manager->update_state();
}

It isn't obvious. Where are the external input taken from? Following the thread, I see that the the state itself is responsible for managing the transition: it will ask user input and control if changing from the current state (i.e itself) to the next state (that it will have to determine too) is feasible. Without saying if it is a good design or not, I wonder whether it is useful in anyway to have a state manager at all. Its only role seems to call on_enter and on_exit, but baking that into the State class isn't difficult since on_enter and on_exit are part of its interface (or you could even dispense with it; just add instructions at the beginning and the end of the update function). It also keeps track of the current state, but a simple current_state variable would do the trick:

std::unique_ptr<State> current_state = std::make_unique<Intro_state>();
while (exit != 1) current_state->update();

But then you might also wonder why this exit is outside of the state machine. If there is an intro-state, a game-state

  At that point I want to take a break and tell you that naming your file with a synonym or a paraphrases for the class it contains is a really bad idea. The Game_state class should be in Game_state.h, not in gaming_state.h, that doesn't make any sense.

then why isn't there an exit-state? It could do some clean-up, display credits, I don't know. So now we have:

std::unique_ptr<State> current_state = std::make_unique<Intro_state>();
current_state->update();

But then the current_state variable itself, and the allocation that goes along with it, are also superfluous. Intro_state().update(); would look weird, but what about intro();? So we have a function void intro() that asks if it needs to call void game() or void exit(), and a void game() that asks if it needs to call void intro() again or void exit(). The design pattern has disappeared, and yet we haven't removed any functionality or usability feature, only simplified. From that observation I feel I can conclude safely that there is a problem in your design.

My 3 cents

Many C++ manuals insist on the object-oriented part of the language and the so-called design patterns. It isn't, IMHO (which, that said, isn't really my own but one taken by most of the prominent figures of the C++ scene), a good way to learn C++. Try rather to explore the robustness of the language or its versatility.


Find median of list of integers This code is meant to find the median of a list of integers. The list is assumed to be of odd length.

This is based of this HackerRank question.

I'm doing this to improve my style and to improve my knowledge of fundamental algorithms/data structures for an upcoming coding interview.

from heapq import heappush, heappop

def median(l):
    heap = []
    for i in l:
        heappush(heap, i)
    cur_int = None
    mid = len(l) // 2
    for i in range(0, mid+1):
        cur_int = heappop(heap)
    median = cur_int
    return median

I think this algorithm is \$O(n)\$ time and space complexity. Please correct me if I'm wrong. I'm also open to any suggestions to improving its complexities or general performance.

To clean up the code a bit and increase performance, try this one line that does everything your code does :-)

from statistics import median

As far as your algorithm goes, I think that the speed would likely be improved if you just sorted the list in the first place, then selected the middle one (or ones and took the average). You don't correctly handle the case where the size of the list is even.


Tag Cloud using 2 queries (one nested), and a foreach loop: Is there a better way? I have a database that, among other things, stores publications and publication tags.  There is a many-to-many relationship between publications and publication tags.  Simply put, I query the database for all the publication tags and the corresponding id.  While looping through that query, I have a nested query to count the number of times each tag is used.  Once I have all the information, I then loop through all results to display the tags.  That seems inefficient, but I can't think of a better way.  Thoughts?

//get the tag id and tag names
try {
  $stmt = $pdo->prepare('SELECT publicationTagId, tagName FROM publicationTags WHERE EXISTS (SELECT publicationTags_publicationTagId FROM publications_have_publicationTags WHERE publicationTags_publicationTagId = publicationTagId)ORDER BY tagName');
  $stmt->execute();
} catch(PDOException $e) {
    echo "
Oops!
";
}
$tags = $stmt->fetchAll();

$tagCount = array();
//iterate through the tags and get usage counts
foreach($tags as $tag) {
  try {
    $stmt = $pdo->prepare('SELECT count(publicationTags_publicationTagId) FROM publications_have_publicationTags WHERE publicationTags_publicationTagId = :pubId');
    $stmt->execute(array(':pubId'=>$tag[0]));
    while($tagCounter = $stmt->fetch()) {
      $tagCount[] = $tagCounter[0];

    }
  } catch(PDOException $e) {
    echo "
Oops!
";
  }
}

//get the min and max usages
$minUse = min($tagCount);
$maxUse = max($tagCount);

//max and min font size
$maxPercent = 150;
$minPercent = 100;

//prevent divide by zero
$divisor = ($maxUse == $minUse) ? 1 : $maxUse - $minUse;

//multiplier
$multiplier = ($maxPercent - $minPercent) / $divisor;

//set up list
echo "
";

$counter = 0;
//loop through tags
foreach($tags as $tag) {
    $size = $minPercent + ($tagCount[$counter][0]-$minUse)*$multiplier;
    echo "
$tag[1]
";
    $counter++;
}

//reset link
echo "
View All
";

//close the list
echo "
";

Note: Your table and especially column names are quite verbose. I would remove the table name from each column as a first start and be more consistent with the names. I'll use better names below. You'll have to map it out yourself.

You can get the used tags with their counts in a single query with this statement:

select
    tag_id,
    tag_name,
    count(publication_id)
from
    tag join publication_tag using (tag_id)
group by
    tag_id


Optimize matlab triple for loop I have a large matrix samples, I want to make another matrix sampleProb which is the same size, but modified as below:

%samples is a 1000*1000 matrix, M = 1000
sampleProb = zeros(1000);
for i = 1:size(samples, 2)
  for j = 1:M
    for k = 1:M
      if (k ~= j)
        sampleProb(j, i) = sampleProb(j, i) + normcdf(samples(j, i) - samples(k, i));
      end
    end
  end
end
sampleProb = sampleProb./M;

I know it's best to avoid for loops when possible in Matlab and operations should be vectorised, but I can't figure out what to do. How can I optimise this for performance?

Introduction and solution code

Since you mentioned making the output matrix sampleProb, I am assuming it to be initialized as all zeros. So, here's the vectorized implementation with the mighty bsxfun -

%// Get the subtractions
sub1 = bsxfun(@minus,samples,permute(samples,[3 2 1]))

%// Calculate the sum of normcdf's in a vectorized fashion
sampleProb = bsxfun(@minus,sum(normcdf(sub1),3),normcdf(sub1(1,:,1)))./M

Benchmarking

Benchmarking Code with M = 100 -

M = 100;
samples =  rand(M,M);
sampleProb1 =  zeros(M,M);

disp('---------------------------------------- With Original Approach')
tic
for i = 1:size(samples, 2)
    for j = 1:M
        for k = 1:M
            if (k ~= j)
                sampleProb1(j, i) = sampleProb1(j, i) + normcdf(samples(j, i) - samples(k, i));
            end
        end
    end
end
sampleProb1 = sampleProb1./M;
toc, clear sampleProb1 i j k

disp('---------------------------------------- With Proposed Approach')
tic
%// Get the subtractions
sub1 = bsxfun(@minus,samples,permute(samples,[3 2 1]));

%// Calculate the sum of normcdf's in a vectorized fashion
sampleProb = bsxfun(@minus,sum(normcdf(sub1),3),normcdf(sub1(1,:,1)))./M;
toc

Runtime results -

---------------------------------------- With Original Approach
Elapsed time is 21.425729 seconds.
---------------------------------------- With Proposed Approach
Elapsed time is 0.046284 seconds.


Build an entity, based on a defined model by comparing table column to entity names and building Can I optimize the BuildEntity method more?

protected IEnumerable<GroupTitle> BuildProductGroup()
    {
        var productGroups = new List<GroupTitle>();
        using (var connection = new SqlConnection(_dbConnection))
        using (var command = new SqlCommand(_getProductGroups, connection))
        {
            connection.Open();
            command.CommandType = CommandType.StoredProcedure;
            using (var reader = command.ExecuteReader())
                while(reader.Read())
                {
                    var productGroup = new GroupTitle();
                    BuildEntity<GroupTitle>(reader, ref productGroup);
                    productGroups.Add(productGroup);
                }
        }

        return productGroups;
    }

    protected TEntity BuildEntity<TEntity>(IDataReader reader, TEntity model)
    {
        var type = model.GetType();
        var table = Enumerable.Range(0, reader.FieldCount).Select(reader.GetName).ToArray();

        foreach (var column in table)
        {
            var matchColumnToProperty = type.GetProperties().FirstOrDefault(property => String.Compare(property.Name, column, true) == 0);
            if (matchColumnToProperty != null && !reader.IsDBNull(reader.GetOrdinal(matchColumnToProperty.Name)))
                matchColumnToProperty.SetValue(model, reader.GetValue(reader.GetOrdinal(matchColumnToProperty.Name)), null);
        }

        return model;
    }

You can cache the properties to improve performance. No need to retrieve them repeatedly within the body of that for loop.

Also, simplify the LINQ expression to gather all the matching properties, and then iterate over the result set in one fell swoop.

protected IEnumerable<GroupTitle> BuildProductGroup()
{
    var productGroups = new List<GroupTitle>();
    using (var connection = new SqlConnection(_dbConnection))
    using (var command = new SqlCommand(_getProductGroups, connection))
    {
        connection.Open();
        command.CommandType = CommandType.StoredProcedure;
        using (var reader = command.ExecuteReader())
        {
            while(reader.Read())
            {
                var productGroup = new GroupTitle();
                BuildEntity<GroupTitle>(reader, ref productGroup);
                productGroups.Add(productGroup);
            }
        }
    }

    return productGroups;
}

protected TEntity BuildEntity<TEntity>(IDataReader reader, TEntity model)
{
    var type = model.GetType();
    var table = Enumerable.Range(0, reader.FieldCount).Select(reader.GetName).ToArray();

    var properties = type.GetProperties();
    var matchedColumns = properties.Where(property => string.Compare(property.Name,
                                                                     column,
                                                                     true) == 0
                                                      && !reader.IsDBNull(reader.GetOrdinal(matchColumnToProperty.Name)));

    foreach (var property in matchedColumns)
    {
        var value = reader.GetValue(reader.GetOrdinal(matchColumnToProperty.Name));
        matchColumnToProperty.SetValue(model, value, null);
    }

    return model;
}


malloc / free implementation Purpose: Educational
Requirement: Given 64KB of memory, implement your own malloc and free
Method: First-Fit

struct Header
{
    unsigned short next; //in blocks of sizeof(Header)
    unsigned short size; //in blocks of sizeof(Header)
};

static Header* _dBuffer;
static Header* _dTail;

static public void dInit()
{
    _dBuffer = getMemBlock(64 * 1024);
    _dTail = _dBuffer;
}

static public void* dMalloc(unsigned short nbytes)
{
    if (_dTail == NULL) //empty free list
        return NULL;

    Header* prev = NULL;
    Header* current = _dTail;

    // below is equivalent to ceil((nbytes + sizeof(Header)) / sizeof(Header))
    int nblocks = (nbytes + sizeof(Header) - 1) / sizeof(Header) + 1;
    while (current->next != 0 && current->size < nblocks)
    {
        prev = current;
        current = _dBuffer + current->next;
    }

    if (current->size < nblocks)
        //we've reached the end of the free list without finding a chunk big enough
        return NULL;

    if (current->size == nblocks)
    {
        //we've found an exact match, remove the chunk from the free list
        if (prev == NULL) //remove the first chunk
            _dTail = NULL;
        else
            prev->next = current->next;
        return current + 1;
    }

    //current->size > nblocks
    current->size -= nblocks;
    current+= current->size;
    current->size = nblocks;
    return current + 1;
}

static bool coalesce(Header* block)
{
    Header* nextBlock = _dBuffer + block->next;
    if (block + block->size != nextBlock)
        return false;

    block->size += nextBlock->size;
    block->next = nextBlock->next;
    return true;
}

static public void dFree(void* p)
{
    Header* released = (Header*)p - 1;
    Header* before = NULL;
    Header* after = _dTail;

    if (_dTail == NULL) //the free list is empty
    {
        released->next = 0;
        _dTail = released;
        return;
    }

    //find where released is located in the free list
    while (after->next != 0 && after < released)
    {
        before = after;
        after = _dBuffer + after->next;
    }

    if (after < released) //released chunk after the end of the free list
    {
        released->next = 0;
        after->next = released - _dBuffer;
        coalesce(after);
        return;
    }

    if (before == NULL) //released chunk before the beginning of the free list
    {
        released->next = _dTail - _dBuffer;
        _dTail = released;
        coalesce(released);
        return;
    }

    //released chunk is somewhere in the middle of the list
    released->next = before->next;
    before->next = released - _dBuffer;
    if (coalesce(before)) //before was coalesced with released
        coalesce(before); //now try to coalesce it with after
    else
        coalesce(released); //otherwise, try coalescing released with after
}

I think this is a bug:

if (current->size == nblocks)
{
    //we've found an exact match, remove the chunk from the free list
    if (prev == NULL) //remove the first chunk
        _dTail = NULL;
        ^^^^^^^^^^^^^^^
        You are just removing the head of the list
        Not NULL(ing) the list.
        I think the line should be:

        _dTail = current->next;

    else
        prev->next = current->next;
    return current + 1;
}

The only thing I don't like is:

unsigned short next; //in blocks of sizeof(Header)

This because you have to keep doing maths to covert it to/from a pointer. I think your code would become much simpler if you just convert this too:

Header* next; // next block in list

I would also remove the assert:

assert(nbytes % sizeof(Header) == 0);

and replace it with code to move the size up.

if (nbytes % sizeof(Header) != 0)
{
    nbytes += (sizeof(Header) - (nbytes % sizeof(Header)));
}

I think you can simplify your code by quite a bit by using sentinel values on your list. See this answer about lists read the bit about sentinals and how it makes things easier.


Securing file uploading I'm working on this snippet of code within a PHP file. The upload function is working but I suspect it is not secure. I move all the files into an upload folder within the site folder. I think that some of this could be tricked into uploading malicious files but I'm not sure how it would be done and more importantly, how to adjust this code to be more secure.

Basically what I'm trying to do is:

Allow Word and PDF documents to be uploaded to a sitefolder's upload folder.
Append the date to the filename.
Move the file from the PHP temporary directory into the destination direction mentioned in step 1.

Other note: I've read on some similar threads that files should not be uploaded to the wwwroot folder, but I'm not sure why that is the case. I understand that if a hacker gains access to some folder X and Y resides within X, that the hacker will have access to X and Y, but would the hacker necessarily be able to gain access to files up the www root directories?

<?php

$allowedExts = array("docx", "DOCX", "DOC", "doc","pdf","PDF");
$temp = explode(".", $_FILES["file".$x]["name"]);
$extension = end($temp);
$destinationdirectory= "C:\inetpub\wwwroot\sitefolder\upload\\";
$date=date("m-d-Y-his", time());
$originalname=(string)$_FILES["file".$x]["name"];
$newfilename=$date." ".$originalname;
$newfilename=preg_replace('/\s+/', '', $newfilename);//Need to remove all white space to make url friendly
//Validation of files types
if ((($_FILES["file".$x]["type"] == "application/pdf") || ($_FILES["file".$x]["type"] == "application/msword")) && in_array($extension, $allowedExts)) {
      if ($_FILES["file".$x]["error"] > 0) {
            echo "Return Code: " . $_FILES["file".$x]["error"] . "<br>";
      } else {
          if (file_exists($destinationdirectory . $_FILES["file".$x]["name"])) {
              echo $_FILES["file".$x]["name"] . " already exists. ";
          } else {
              move_uploaded_file($_FILES["file".$x]["tmp_name"], destinationdirectory . $newfilename);
              //Now that the file is validated, renamed and uploaded to the server, get the vars together to store them inside of the array.
              $filelocation="http://mywebsite.com/upload/".$newfilename;
          }//File moving success loop.
      }
} //IF FILETYPE IS CORRECT AND ALLOWED.
else {
    //Upload error or no file chosen for upload, add it to the array.
    array_push($uploadserrors,$actiondoctypesindex[$x]);
}

?>

Readability

It's a bit difficult to comment on the security aspects of code that's hard to read.
There are several readability issues here:

The indentation is very messy and inconsistent throughout.
It would be good to clean that up and use consistent indenting,
for example use 4 spaces inside every code block (inside { ... })
$_FILES["file".$x] is duplicated in many places,
which makes the code look very noisy.
You could save it in a local variable and the whole thing will look a lot simpler.
It's good to put a space around operators, for example:

instead of: $newfilename=$date." ".$originalname;
write like this: $newfilename = $date . " " . $originalname;

Simplify

Other simplifications are possible.

Instead of listing up all the lowercase and uppercase variants of extensions like this:

$allowedExts = array("docx", "DOCX", "DOC", "doc","pdf","PDF");

You could use list just in lowercase, and when you need to check the extension,
just wrap that in strtolower, like this:

in_array(strtolower($extension), $allowedExts)

You could reduce one nesting level in the conditions. Instead of this:

  if ($_FILES["file".$x]["error"] > 0) {
    // ...
  }
  else {
    if (file_exists($destinationdirectory . $_FILES["file".$x]["name"])) {
      // ...
    } else {
      // ...
    }
  }

You could use an elseif:

  if ($_FILES["file".$x]["error"] > 0) {
      // ...
  } elseif (file_exists($destinationdirectory . $_FILES["file".$x]["name"])) {
      // ...
  } else {
      // ...
  }


Using Scanner.nextInt() whilst ignoring rest of line I'm contemplating how to read an int using Scanner.nextInt(), and at the same time ignore the rest of the line or flush the input buffer to be ready for inputting the next number. Typical usage would be to read a number where the user is likely to add units or similar after the number, and I want the scanner to be cleared out before I output a new message and ready for the next number.

In the following code I firstly use Scanner.nextLine() to read rest of line, and in the second I attempted to use the Scanner.hasNext() to detect if anything was present. But it originally failed, so to get it to work I had to add a bug-fix related to writing the text "quit". The third read is just to finish off with a final read of an integer.

import java.util.Scanner;

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

        Scanner scanner = new Scanner(System.in);

        System.out.print("1) Enter an integer (with some junk like units): ");
        int myFirstInt = scanner.nextInt();
        // Ignore rest of line
        scanner.nextLine();

        System.out.print("2) Enter an integer (and some junk afterwards): ");
        int mySecondInt = scanner.nextInt();

        // Clear out scanner before next read...
        String line;                    // Fix to avoid endless loop
        while (scanner.hasNext()) {
            line = scanner.next();
            if (line.equals("quit")) {  // Fix to avoid endless loop
                break;                  // Fix to avoid endless loop
            }                           // Fix to avoid endless loop
        }

        System.out.print("3) Enter the third integer: ");
        int myThirdInt = scanner.nextInt();

        System.out.printf("\nFirst: %d, Second: %d, Third: %d\n",
                          myFirstInt, mySecondInt, myThirdInt);
    }
}

A typical run with this code:

1) Enter an integer (with some junk like units):  123 kg
2) Enter an integer (and some junk afterwards):  45 stones 5 pound
 quit
3) Enter the third integer:  238

First: 123, Second: 45, Third: 238

The code now works (with the need for entering "quit" at the second prompt), but I'm wondering whether there is a safer or better method for clearing out the scanner, so I can get it ready for reading the next input. Hopefully with the out endless loop fix. The output is as expected.

In order to remove the junk after the integer, you can make use of Scanner.skip(Pattern pattern). It allows you to skip over any set of characters that match a pattern:

scanner.skip(".*");

This method call will cause the Scanner to skip over any characters that match the pattern ".*"  .  The period character in a regular expression matches any single character.  The repetition character * causes matching zero or more of the previous character.  So the pattern .* will match zero or more of any characters.  Thus, it will skip over any characters currently on the stream. So now our code will look like:

    int myFirstInt = scanner.nextInt();
    // Ignore rest of line
    scanner.skip(".*");

    System.out.print("2) Enter an integer (and some junk afterwards): ");
    int mySecondInt = scanner.nextInt();
    scanner.skip(".*");
    System.out.print("3) Enter the third integer: ");
    int myThirdInt = scanner.nextInt();

    System.out.printf("\nFirst: %d, Second: %d, Third: %d\n",
                      myFirstInt, mySecondInt, myThirdInt);

It is a simple way to flush the stream so that it is clear of bad characters, and ready to be reused!


Project Euler #127 abc-hits Problem (Rephrased from here):

  The radical of \$n\$, \$rad(n)\$, is the product of distinct prime factors of \$n\$. For example, \$504 = 2^3 × 3^2 × 7\$, so \$rad(504) = 2 × 3 × 7 = 42\$.

  We shall define the triplet of positive integers \$(a, b, c)\$ to be an abc-hit if:

  \$GCD(a, b) = GCD(a, c) = GCD(b, c) = 1\$
  \$a < b\$
  \$a + b = c\$
  \$rad(abc) < c\$

  If \$(a,b,c)\$ is an abc-hit such that \$c<120000\$, Find sum of all \$c\$.

I have this code which runs in ~25.727 seconds, which I would like to write in a better way.

Firstly I noted these:

If GCD(a,b)=1 then GCD(a,a+b)=1 and GCD(b,a+b)=1
rad(a*b*c)=rad(a)*rad(b)*rad(c) if GCD(a,b,c)=1

Code:

private static double[] rad;
private static int[] primes;

public static void main(String[] args) {
    long t1 = System.currentTimeMillis();
    System.out.println(solve());
    long t2 = System.currentTimeMillis();
    System.out.println((double) (t2 - t1) / 1000.0);
}

private static long solve() {
    final int lim = 1000;
    long sumc = 0;
    sievePrimesArray(lim);
    sieveRadArray(lim);
    for (int a = 1; a < lim; a++) {
        // The value of `b` is minimum `a+1`, so `notcoprime` is an
        // array which tells if `i` and `a` have some common prime factor.

        boolean[] notcoprime = new boolean[lim - a - 1];
        for (int i = 0; i < primes.length && primes[i] <= a; i++) {
            if (a % primes[i] == 0) {
                // We need `primes[i]*j>=a+1`

                for (int j = (a + 1) / primes[i] + (((a + 1) / primes[i] < a + 1) ? 1 : 0);
                     primes[i] * j < lim; j++) {
                    notcoprime[primes[i] * j - (a + 1)] = true;
                }
            }
        }
        for (int b = a + 1; b + a < lim; b++) {
            int c = a + b;
            if (!notcoprime[b - (a + 1)]) {

            // Here to avoid overflow I used `double` and since `BigInteger` was slow.
            // The maximum value of `rad` rad reached is `119999` and
            // `rad[a-1]*rad[b-1]<=119999*119999=14399760001` which is still in `double`

                if (rad[a-1] * rad[b-1] < (double) c / rad[c-1]) {
                    sumc += c;
                }
            }
        }
    }
    return sumc;
}

Rather than finding all prime factors of a number, I multiply rad[x*p] by p where p is prime, once.

public static void sieveRadArray(int lim) {
    rad = new double[lim];
    Arrays.fill(rad, 1);
    for (int x : primes) {
        for (int j = 1; x * j < lim; j++) {
            rad[j * x - 1] *= x;
        }
    }
}

private static void sievePrimesArray(int lim) {
    boolean[] isPrime = new boolean[lim + 1];
    Arrays.fill(isPrime, true);
    for (int i = 2; i * i <= lim; i++) {
        if (isPrime[i]) {
            for (int j = i; i * j <= lim; j++) {
                isPrime[i * j] = false;
            }
        }
    }
    ArrayList<Integer> prime_list = new ArrayList<Integer>();
    prime_list.add(2);
    for (int j = 3; j < lim; j++) {
        if (isPrime[j]) {
            prime_list.add(j);
        }
    }
    primes = prime_list.stream().mapToInt(i -> i).toArray();
}

Kudos for doing math groundwork, particularly for noticing multiplicative property of \$rad\$.
I see no reason to resort to double. 14399760001 is well within the long range.
Computing notcoprime costs a lot. Replacing it with

for (int a = 1; a < lim; a++) {
    for (int b = a + 1; b + a < lim; b++) {
        if(gcd(a, b) == 1) {
            ....

reduces the execution time by the factor of 1.5 even for the most naive gcd implementation.
On my system your code as is runs for about 0.06 sec. The reported 25+ sec must have some other explanation. What system do you have?


Am I optimally closing this modal window? I have a modal window that needs to be closed, obviously.  I've created this piece of code that works okay. But is there a way to optimize it?

// close the modal window
$('.modal-close').click(function(e) {
    $('#modal').hide();
    $('#modal-overlay').hide();

    e.preventDefault();
});

$(document).keydown(function(e) {
    if (e.keyCode == 27) { // if Esc key is pressed
        $('#modal').hide();
        $('#modal-overlay').hide();
    }

    if (e.keyCode == 9) { // cancel the Tab key
        return false;
    }
});

You can combine a couple of things: The close functionality can be function, and jQuery can operate on several elements at once:

function closeModal(event) { // passing in an event obj is optional
  $("#modal, #modal-overlay").hide();
  if(event) {
    event.preventDefault();
  }
}

$(".modal-close").click(closeModal);

$(document).keydown(function(event) {
  switch(event.keyCode) {
    case 27: // esc
      closeModal();
      break;
    case 9: // tab
      event.preventDefault();
      break;
    default: // everything else
      break;
  }
}

I've switched the keydown handler to use a switch statement, but that's not critically important. The point is just that since the keycode will be either one number or the other, there's no need for multiple ifs, and a switch is simple to extend if you need to handle more keycodes.


Spelling a word with chemical elements in Rust I tried solving this /r/dailyprogrammer challenge in Rust.

I'm a complete Rust beginner, and I'm sure I've made a lot of mistakes. I would be very thankful for any tips or help you might be able to provide.

Here are a couple of points that I found awkward/confusing:

In show_result, what I basically wanted to do was something like

symbols = spelling.map {|e| e.symbol}.join("")
elements = spelling.map {|e| e.name}.join(", ")

but that didn't seem to work.
I'm not exactly sure why I have to clone the elements to put them into the result vector. Can I/should I avoid this?
Coming from a dynamically typed garbage collected language, I had a hard time deciding when and where to use references.

Thank you very much!

extern crate csv;

use std::env;
use std::path::Path;
use std::collections::HashMap;
use std::option::Option;
use std::vec::Vec;

#[derive(Clone)]
struct Element {
    number: u16,
    symbol: String,
    name: String
}

impl Element {
    fn new (number: u16, symbol: &str, name: &str) -> Element {
        Element {
            number: number,
            symbol: symbol.to_string(),
            name: name.to_string()
        }
    }
}

// Read all elements from CSV file into a hash map
fn read_elements() -> HashMap<String, Element> {
    let mut elem_map = HashMap::new();

    let path = Path::new("./elements.csv");
    let mut csv_reader = csv::Reader::from_file(path).unwrap();
    for record in csv_reader.decode() {
        let (number, symbol, name): (u16, String, String) = record.unwrap();
        let symbol = symbol.trim();
        let name = name.trim();
        elem_map.insert(symbol.to_lowercase().to_string(), Element::new(number, symbol, name));
    };

    return elem_map;
}

// Try to spell target_word using elements from elem_map
fn spell_word(target_word: &str, elem_map: HashMap<String, Element>) -> Option<Vec<Element>> {
    let mut result: Vec<Element> = Vec::new();
    let target_word = target_word.to_lowercase().to_string();

    // How far we've matched so far
    let mut current: usize = 0;

    while current < target_word.len() {
        // Try matching the next two letters first
        if current + 2 <= target_word.len() {
            let next_two = &target_word[current..current + 2];
            if let Some(ref elem) = elem_map.get(next_two) {
                result.push((*elem).clone());
                current += 2;
                continue;
            }
        }

        // Try matching one letter if two letters failed
        let next_one = &target_word[current..current + 1];

        if let Some(ref elem) = elem_map.get(next_one) {
            result.push((*elem).clone());
        } else {
            // If no one letter element could be found,
            // this word cannot be spelled using elements
            return None;
        }
        current += 1;
    }

    return Some(result);
}

// Print the found spelling
// e.g. PoISON (polonium, iodine, sulfur, oxygen, nitrogen)
fn show_result(spelling: Vec<Element>) {
    let mut symbols: String = String::new();
    let mut elements: Vec<String> = Vec::new();

    for elem in spelling {
        symbols.push_str(&elem.symbol);
        elements.push(elem.name.to_lowercase());
    }

    println!("{} ({})", symbols, elements.join(", "));
}

fn main() {
    let args: Vec<String> = env::args().collect();
    let elements = read_elements();

    if args.len() <= 1 {
        println!("Please provide a word to spell.");
    }

    let target_word: &str = &args[1];

    match spell_word(target_word, elements) {
        Some(spelling) => show_result(spelling),
        None => println!("Could not find a match for word \"{}\".", target_word)
    }
}

Overall

Don't explicitly import Vec or Option, they are part of the prelude.
Basically always want to implement Debug on any type.
I see two types that aren't there: something that holds the elements, something that contains results.
This allows you to implement Display for Spelling. This is more flexible and participates in the ecosystem better.

read_elements

No need to use Path explicitly, &str implements AsRef<Path>.
Can collect iterator of tuples into a HashMap, no need to muck about with a mutable variable.
Don't use unwrap. Prefer expect or return a Result. expect has words that will help you or the user find the problem. Note that we can collect into a Result.
Don't hardcode the filename inside the function.

spell_word

Should not take elem_map by value. Take a reference as we aren't making use of the allocated data.
I'd split the string and just track the string pieces, not the distance into the string.
No explicit return at the end of function.
Return references to the element instead of cloning.

show_result

Don't specify the types on a variable binding; let inference take care of it.
Don't take ownership of Vec, instead take a slice.
I'd avoid allocating in the printing function. I used Itertools to help with the comma separating.

main

Don't specify the type of a collection, let inference take care of it (let foo: Vec<_> = bar.collect()).
Don't check for the argument and then implicitly check again (via indexing). A match or unwrap would be better.
No need to collect into a vector. Instead, skip the program name and just iterate over all the arguments - bonus feature!
No need to explicitly convert String to &str, a &String can be coerced to a &str.
Use raw strings to avoid escaping quotes

extern crate csv;
extern crate itertools;

use std::{env, fmt};
use std::collections::HashMap;

use itertools::Itertools;

type Error = Box<std::error::Error>;

#[derive(Debug, Clone)]
struct Element {
    number: u16,
    symbol: String,
    name: String
}

impl Element {
    fn new (number: u16, symbol: &str, name: &str) -> Element {
        Element {
            number: number,
            symbol: symbol.to_string(),
            name: name.to_string()
        }
    }
}

struct Elements {
    elem_map: HashMap<String, Element>
}

impl Elements {
    fn read_from_file(name: &str) -> Result<Self, Error> {
        let mut csv_reader = csv::Reader::from_file(name)?;

        let elem_map: Result<_, Error> = csv_reader.decode().map(|record| {
            let (number, symbol, name): (u16, String, String) = record?;
            let symbol = symbol.trim();
            let name = name.trim();
            Ok((symbol.to_lowercase().to_string(), Element::new(number, symbol, name)))
        }).collect();

        Ok(Elements { elem_map: elem_map? })
    }

    // Try to spell target_word using elements from elem_map
    fn spell_word(&self, target_word: &str) -> Option<Spelling> {
        let mut result = Vec::new();
        let target_word = target_word.to_lowercase().to_string();
        let mut target_word = target_word.as_str();

        while !target_word.is_empty() {
            // Try matching the next two letters first
            if target_word.len() >= 2 {
                let (head, tail) = target_word.split_at(2);

                if let Some(elem) = self.elem_map.get(head) {
                    result.push(elem);
                    target_word = tail;
                    continue;
                }
            }

            // Try matching one letter if two letters failed
            let (head, tail) = target_word.split_at(1);

            if let Some(elem) = self.elem_map.get(head) {
                result.push(elem);
                target_word = tail;
                continue
            }

            // If no one letter element could be found,
            // this word cannot be spelled using elements
            return None;
        }

        Some(Spelling(result))
    }
}

struct Spelling<'a>(Vec<&'a Element>);

impl<'a> fmt::Display for Spelling<'a> {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        for elem in &self.0 {
            write!(f, "{}", elem.symbol)?;
        }

        write!(f, " (")?;

        for elem in self.0.iter().map(|e| e.name.as_str()).intersperse(", ") {
            write!(f, "{}", elem)?;
        }

        write!(f, ")")
    }
}

fn main() {
    let elements = Elements::read_from_file("./elements.csv").expect("Unable to read elements");

    for target_word in env::args().skip(1) {
        match elements.spell_word(&target_word) {
            Some(spelling) => println!("{}", spelling),
            None => println!(r#"Could not find a match for word "{}"."#, target_word)
        }
    }
}


Basic User Interaction in Python - syntax, docstrings, performance Hoping for input on the clarity of the docstrings, syntax and performance of these two functions both of which should be self-explanatory:

def tell(paragraph, duration=.1):
    """Print paragraph line by line with pause between each line."""
    lines = paragraph.splitlines()
    for line in lines:
        print line.lstrip()
        time.sleep(duration)

def yinyang(yes, no, prompt = "y/n: ", tries = 3):
    """Return result based on parsed user input with specified number of tries."""
    input = raw_input(prompt)
    word_list = scan(input)
    repeat_prompt = """Does not compute.
                    Please try again to express yes or no."""
    player_decision = parse_sentence(word_list).decision
    try:
        assert(input and tries > 0)
        input = None
        if player_decision == "affirmative":
            return yes
        elif player_decision == "negative":
            return no
        else:
            tries += 1
            tell(repeat_prompt)
            return yinyang(yes, no, prompt, tries)
    except AssertionError:
        return no

The functions, scan() and parse_sentence() are from a module so their docstring (within the module code) should suffice for any needed explanation, right?

Will include them, too:

"""Accept a word string and return a yes, no or error"""

WORD_TYPES = {'affirmative': ['yes', 'y', 'do', 'will', 'sure', 'will', 'ok', 'can', 'would'],
              'negative': ['no', 'n', 'won\'t', 'not', 'nyet', 'can\'t']}

type_words = {word : label for label, words in WORD_TYPES.items() for word in words}

PUNCTUATION = ['.', '?', '!', ',', ':', ';']

def scan(sentence):
    """Strip punctuation and return list containing word and type"""
    sentence_list = sentence.split()
    for idx, word in enumerate(sentence_list):
        for p in PUNCTUATION:
            if p in word:
                word = word.strip(p)
        try:
            lc_word = word.lower()
            word_type = type_words.get(lc_word, None)
            assert word_type is not None
            sentence_list[idx] = (word_type, lc_word)
        except AssertionError:
            lc_word = word.lower()
            sentence_list[idx] = ('error', lc_word)
    return sentence_list

and parse_sentence is here:

class Sentence(object):

    def __init__(self, decision):
        # remember we take ('noun','princess') tuples and convert them
        self.decision = decision[0]

def peek(word_list):
    if word_list:
        word = word_list[0]
        print word[0]
        print word[1]
        return word[0]
    else:
        return None

def match(word_list, expecting):
    if word_list:
        word = word_list.pop(0)

        if word[0] == expecting:
            return word
        else:
            return None
    else:
        return None

def skip(word_list, word_type):
    while peek(word_list) == word_type:
        match(word_list, word_type)

def parse_decision(word_list):
    next_word = peek(word_list)

    if next_word == 'affirmative':
        return match(word_list, 'affirmative')
    elif next_word == 'negative':
        return match(word_list, 'negative')
    else:
        return ('error', 0)

def parse_sentence(word_list):
    decision = parse_decision(word_list)

    return Sentence(decision)

Snippet 1.

The way yinyang is organized is quite unconventional. I presume that

tries += 1

is a typo (tries -= 1 would make much more sense). Even than I don't see a need for a recursion. A simple loop will do.

assert(input) also misses the point. If raw_input() succeeds, an assertion cannot possibly trigger; if not, EOFError will be raised. For the same reason, there's no need to

input = None

Passing yes and no arguments to yinyang need some serious justification. Returning True/False would be as good.

Snippet 2.

PUNCTUATION doesn't feel right: string.punctuation is there for a reason.

word.strip(p) is palliative. What if a punctuation mark is in the middle of the word?

A try/except flow is supposed to deal with problems beyond your control. It is not an if substitute. Besides,

    lc.word = word.lower()
    word_type = type_words.get(lc_word, None)

may not possibly raise AssertionError. Thus they do not belong to the try clause.

Also I guess you are trying too hard with affirmatives. y/n is just than enough.

Snippet 3.

I don't see a reason for class Sentence to exist. BTW it is not documented at all.


Adding a class to an element in a plugin I have the following code, and I just added a Class to the element that uses the plugin. I works perfectly, but I am wondering if there is a better way to do it.

(function ($) {

/**
 * jQuery Plugin Extension
 */
$.fn.extend({

    /**
     * build editable
     *
     * @param {string} saveUrl
     * @param {Object} options
     * @returns {*}
     */
    baseEditable: function (saveUrl, options) {

        var settings = $.extend(true, {
            name: 'value',
            onblur: 'submit',
            placeholder: $.i18n._('msg_click_to_add'),
            ajaxoptions: {
                dataType: 'json'
            }
        }, options);

        return this.each(function () {
            $(this).addClass('editable');
            var baseEditable = $(this).editable(saveUrl, settings);

            $(this).data('baseEditable', baseEditable);
        });

    }
});
})(jQuery);

You can use the addClass method on all the elements instead of creating a new jQuery object for each item:

return this.addClass('editable').each(function () {
  var baseEditable = $(this).editable(saveUrl, settings);
  $(this).data('baseEditable', baseEditable);
});


URLs extraction from specific block This script extracts all urls from a specific HTML div block (with BeautifulSoup 4) :

raw_data = dl("http://somewhere")
links = []
soup = BeautifulSoup(raw_data)
data = str(soup.find_all('div', attrs={'class' : "results"}))
for link in BeautifulSoup(data, parse_only = SoupStrainer('a')):
    links.append(urllib.parse.unquote_plus(link['href']))
return links

Is there a more efficient and clean way to do it ?

AFAIK, you can use List comprehension to make it more efficient

raw_data = dl("http://somewhere")
soup = BeautifulSoup(raw_data)
data = str(soup.find_all('div', attrs={'class' : "results"}))
return  [ urllib.parse.unquote_plus(link['href']) for link in BeautifulSoup(data, parse_only = SoupStrainer('a'))]


Merge a new interval into given array of intervals This is Merge Intervals problem from www.interviewbit.com

  Problem : Given a set of non-overlapping intervals, insert a new interval into the intervals (merge if necessary). You may assume that
  the intervals were initially sorted according to their start times.

  Example 1:

Given intervals [1,3],[6,9] insert and merge [2,5] :
result [1,5],[6,9].

  Example 2:

Given intervals [1,2],[3,5],[6,7],[8,10],[12,16], insert and merge [4,9]
result in [1,2],[3,10],[12,16]

My Approach:

def insert( intervals, new_interval):
    """returns list of intervals after merging"""
    count = 0
    status = False
    final_interval = []
    k = len(intervals)
    new_interval =  (min(new_interval),max(new_interval))
    for i in range(k):

        ## if new interval lesser than given interval
        if min(new_interval) < min(intervals[i]) and max(new_interval) < min(intervals[i]):
            status = False
            break

        ## if new interval falls under given interval
        elif min(new_interval) >= min(intervals[i]) and max(new_interval)<= max(intervals[i]):
            status = True
            break

        ## if given interval falls under new interval
        elif min(new_interval) <= min(intervals[i]) and max(new_interval)>= max(intervals[i]):
            intervals.remove(intervals[i])
            status = True
            intervals.insert(i, new_interval)

        ## part of new interval overlaps with left part of given interval
        elif min(new_interval) <= max(intervals[i]) and min(new_interval) >= min(intervals[i]):
            intervals[i][1]  = max(new_interval)
            new_interval = intervals[i]
            status = True

        ## part of new interval overlaps with right part of given interval
        elif max(new_interval) > min(intervals[i])and max(new_interval) <= max(intervals[i]):

            new_interval[1]  = max(intervals[i])
            intervals.remove(intervals[i])
            status = True
            intervals.insert(i, new_interval)
        count+= 1
    if not status:
        intervals.insert(count,new_interval)

    for elem in intervals:
        if elem not in final_interval:
            final_interval.append(elem)

    return final_interval

Test Cases:

print insert([[1,2],[3,6]],[8,10])
print insert([[3,5],[7,9]],[1,10])
print insert([[3,6],[8,10]],[1,2])
print insert([[1,2],[8,10]],[3,6])
print insert([[1,2],[3,5],[6,7],[8,10],[12,16]],[4,9])
print insert([[1,3],[6,9]],[2,5])

How can this problem be solved in a better way?

Avoid indexing repetitively. You know that you'll want intervals[i] frequently within the loop, so index it once... or better yet, loop on it efficiently in the first place:

for i, (low, high) in enumerate(intervals):
    if new_interval[0] < low and new_interval[1] < low:
        # ...

Along the same lines, you frequently want the know the min and max of the new interval; you already compute these values, why keep it as a tuple that has to be indexed again and again?

new_low, new_high = min(new_interval), max(new_interval)
# ...
for i, (low, high) in enumerate(intervals):
    if new_low < low and new_high < low:
        # ...

Use my_list.pop(i) instead of my_list.remove(my_list[i]). The first requires Python to iterate through the list element by element, check for equality against my_list[i], and remove the first equal element it finds; the second tells it exactly what part of the list you want gone much more efficiently, since you already know the location i.

Better yet, use an approach that doesn't modify intervals at all (no popping or inserting). It's entirely possible to solve this problem by only appending new outputs to final_interval with no other list mutations. That's particularly important if you're iterating on intervals, in which case mutating it within the for loop will break your code.

Your final loop is O(N^2), which is not necessary for this problem:

for elem in intervals:  # O(n)
    if elem not in final_interval:  # O(n)
        final_interval.append(elem)

Since you know that the values in intervals are sorted already, I would instead suggest an approach that stops the first loop after you've output new_interval and all intervals it overlaps with, then the second loop just pumps out the remaining items from where the first loop left off.

And now to actually answer your specific error: you've defined your function to work on a list of lists (or tuples, whatever), and return the same; the actual problem hosted on the site provides a list of Interval objects and expects a list of Interval objects back. This is easy to support:

def insert(self, intervals, new_interval):
    intervals = [(v.start, v.end) for v in intervals]
    new_interval = (new_interval.start, new_interval.end)
    new_low, new_high = min(*new_interval), max(*new_interval)
    # ...
    return [Interval(l, h) for l, h in final_interval]


Calculate percentage with lodash I have data like:

var data =
    [{
        "date":"1-1-2013",
        "answer":"yes"
    },{
        "date":"1-1-2013",
        "answer":"no"
    },{
        "date":"2-1-2013",
        "answer":"no"
    },{
        "date":"2-1-2013",
        "answer":"no"
    },{
        "date":"2-1-2013",
        "answer":" "       //this is not answered!
    },,{
        "date":"2-1-2013",
        "answer":"no"
    }];

I need to return[array] percentages of responses for that day(groupedBy) which have a value of “yes” for the “answer”.

For above the result is: [50, 0...]

I have written this below code, it works but i believe it can be simplified a lot.

var freshData =  _.map(data,function(key){
    if (key.answer == "") return false;
    return key;

});

var reducedData = _.filter(freshData, function(n) {
    return n != false;
});

var groups = _.groupBy(reducedData, function(value){
        return value.date + '#' + value.answer;
});

var result = _.map(groups,function(key){
    if (key[0].answer == "") return false;
    return {
        date : key[0].date,
        answer : key[0].answer,
        length :key.length
    };
});

var result2 = _.chunk(result,2);

var result3 =  _.map(result2,function(key){
    return {
        date:key[0].date,
        yes:key[0].length,
        no:key[1].length,
        percentage : (key[1].length/(key[0].length + key[1].length) * 100)
    }
});

var result4 =  _.map(result3,function(key){
return  key.percentage;
});

console.log(result4);

I need to use lodash.
Thanks.

Create a lodash chain, use _.groupBy() to get a dates map, and then reduce it to percentages:

function getNumOfYes(data) {
  return _(data).groupBy('date').reduce(function(percentage, answers) {
    var numOfYes = _.countBy(answers, 'answer').yes || 0;
    percentage.push(numOfYes / answers.length * 100);
    return percentage;
  }, []);
}

var data = [{
  "date": "1-1-2013",
  "answer": "yes"
}, {
  "date": "1-1-2013",
  "answer": "no"
}, {
  "date": "2-1-2013",
  "answer": "no"
}, {
  "date": "2-1-2013",
  "answer": "no"
}, {
  "date": "2-1-2013",
  "answer": " " //this is not answered!
}, , {
  "date": "2-1-2013",
  "answer": "no"
}];

var numOfYes = getNumOfYes(data);

console.log(numOfYes);

document.getElementById('results').innerText = JSON.stringify(numOfYes);


Reversing a linked list recursively This appears to work. Are there any edge cases I am missing? What are your thoughts on the algorithm and implementation?

template <typename el>
void Linkedlist<el>::reverse() {
    Node<el> * oldHead = head;
    Node<el> * tailAfterReversal = recReverseList( head->getNextNode() );
    oldHead->setNextNode(NULL);
    tailAfterReversal->setNextNode(oldHead);
}

// Reverses part of a linked list starting from toRevesrPortionHead, returns the tail node
// after the reversal operation has been performed
template <typename el>
Node<el> * Linkedlist<el>::recReverseList( Node<el> * toReversePortionHead ) {
    if ( toReversePortionHead->getNextNode() == NULL ) {
        head = toReversePortionHead;
        return toReversePortionHead;
    }

    Node<el> * tailAfterReversal = recReverseList( toReversePortionHead->getNextNode() );
    tailAfterReversal->setNextNode(toReversePortionHead);
    toReversePortionHead->setNextNode(NULL);

    return toReversePortionHead;

}

Algorithm

Since its recursive and O(n), for large n you may get a stack overflow.
It will fail if the head is null (empty).
This part is redundant:

toReversePortionHead->setNextNode(NULL);

Since in each stack call you change the next pointer of the next to the current. For example:

In the base case :

(4) -> (5) -> (9) -> (10) -> nil

First pop:

(4) -> (5) -> (9) <- -> (10)

Second pop:

(4) -> (5) <- -> (9) <- (10)

You will end up with:

(4) <- -> (5) <- (9) <- (10)

So you set the member next of the resulting node to null

nil <- (4) <- (5) <- (9) <- (10)

Node

Your node is generic, you should make it a nested class. Also your get and set methods are too verbose and I would personally omit them.

NULL:

If you are using C++11 you should be using nullptr. If you are curious why is a better alternative you could check out this link https://stackoverflow.com/questions/13816385/what-are-the-advantages-of-using-nullptr

Code :

I would change it to this:

template <typename el>
void Linkedlist<el>::reverse() {

    if(!empty())
       recReverseList(head)-> next = nullptr;
}

template <typename el>
Node * Linkedlist<el>::recReverseList( Node* node ) {

    if ( node->next == nullptr )
        return head = node;

    return recReverseList( node->next )->next = node;
}


Run the most recently requested action after finishing current, skip middle tasks The goals for the code below are in the class summary below. Can anyone see that the code fails at any of those goals? I'm unsure how to thoroughly test it. Is there a way to keep the Queue method thread-safe and remove the lock? Is that lock even necessary?

/// <summary>
/// The goals: only run the most recently queued action.
/// Do not run any actions simultaneously.
/// Skip any tasks that were unstarted if we get a new one.
/// Cancel the currently running task (without aborting the thread).
/// (Rely upon the code in the action to bail early).
/// Support the "IsCanceled" flag on any task that was canceled or skipped.
/// </summary>
public class SingleTaskQueue
{
    private Task _task = TaskUtilities.FromResult(false); // using Task.FromResult in .NET 4.5
    private CancellationTokenSource _cts = new CancellationTokenSource();
    private Action<CancellationToken> _action;
    public Task Queue(Action<CancellationToken> action) // this needs to be thread-safe
    {
        lock (_task) // yes, this changes with each call to Queue
        {
            _action = action;
            var newSource = new CancellationTokenSource();
            var tokenSource = Interlocked.Exchange(ref _cts, newSource);
            tokenSource.Cancel();
            var ret = _task.ContinueWith(justFinished => ProcessLastAction(newSource.Token), newSource.Token);
            _task = ret;
            return ret;
        }
    }

    private void ProcessLastAction(CancellationToken token)
    {
        if (!token.IsCancellationRequested)
        {
            Action<CancellationToken> last = Interlocked.Exchange(ref _action, null);
            if (last != null)
                last.Invoke(token);
        }
    }
}

Don't change the locks - You lock on _task, but inside the synchronized block - you change the reference to _task to a new object! This means that if threadA is in the block, threadB is waiting for the lock to release, and threadC calls the method exactly after threadA has run _task = ret; - threadC will be able to enter the block! This makes your lock mechanism fail.

If you want to lock a block - find a lock which defines the scope of your lock - and maintain it accordingly. In your example, the scope seems to be the instance of SingleTaskQueue. Since it is not advisable to do lock(this), create a private object _lock = new object(); and use it as your lock.

Mind your state - in ProcessLastAction you take your action from _action, to take only the last one, but you take the token as a parameter. This means that if two threads queue a new message before the last task finishes - the second _action may be invoked with the first token - which is already cancelled!

If the two objects are dependent of each other - maintain them together. Create a KeyValuePair<CancellationToken, Action> or a custom class to hold them both.

Is that lock necessary? I can't say if you really need the lock, as I don't know how would Task behave if ContinueWith is called twice on it:

If the last call overrides the previous call (in a thread-safe manner) - you could do away with your whole solution - it could come down to a one-liner :)
If calls are aggregated in an internal queue, and your manual maintenance of the last action is needed, you would have to synchronize your work.

Also, if you have only a single producer thread (only one thread which calls Queue) - you don't need to synchronize.


Simple URL shortener This URL shortener is just for my own use and it runs on my local machine. I created this for fun and I doubt if I would ever use it in production. Is my script any good?

shortener.php

<?php
require_once 'lib/db.php';

function random_string($length, $charset = 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789') {
    $chars_length = strlen($charset) - 1;
    $str = '';

    for ($i = 0; $i < $length; ++$i) {
        $str .= $charset[random_int(0, $chars_length)];
    }

    return $str;
}

$url = $error = '';

if (isset($_GET['id'])) {
    $id = trim($_GET['id']);

    $stmt = $pdo->prepare('SELECT url FROM url WHERE id = ?');
    $stmt->execute([$id]);
    $url = $stmt->fetchColumn();

    header('Location: ' . ($url ?: "http://{$_SERVER['HTTP_HOST']}{$_SERVER['SCRIPT_NAME']}"));
    exit;
}
else {
    if ($_SERVER['REQUEST_METHOD'] === 'POST') {
        $url = $_POST['url'];

        if (filter_var($url, FILTER_VALIDATE_URL) !== FALSE) {
            do {
                $id = random_string(5);

                $stmt = $pdo->prepare('SELECT COUNT(*) FROM url WHERE id = ?');
                $stmt->execute([$id]);
                $count = $stmt->fetchColumn();
            }
            while ($count > 0);

            $stmt = $pdo->prepare('INSERT INTO url (id, url) VALUES (?, ?)');
            $stmt->execute([$id, $url]);

            header("Location: http://{$_SERVER['HTTP_HOST']}{$_SERVER['SCRIPT_NAME']}");
            exit;
        }
        else {
            $error = 'Not a valid URL';
        }
    }

    $last_id = $pdo->query('SELECT id FROM url ORDER BY created DESC LIMIT 1')->fetchColumn();
}
?><!DOCTYPE html>

    <meta charset="utf-8">
    <title>Extremely Simple Short URL Generator</title>

    <form method="post">
        <input type="text" name="url" placeholder="URL here" value="<?= htmlspecialchars($url) ?>">
        <button type="submit">Submit</button>
    </form>
    <?php if ($last_id): ?>
<a href="<?= "{$_SERVER['SCRIPT_NAME']}?", http_build_query(['id' => $last_id]) ?>">Latest Link
<?php endif ?>
    <?php if ($error): ?>
<?= $error ?>
<?php endif ?>

I'm aware that separating the view from the script is better, but for a simple app like this, I don't think it's necessary.

db.php

<?php
$pdo = new PDO(
    'mysql:host=host;dbname=dbname;charset=utf8mb4',
    'username',
    'password',
    [
        PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
        PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
        PDO::ATTR_EMULATE_PREPARES => FALSE,
    ]
);

Any improvement is welcome.

Feedback

The script looks pretty good. For a small script that runs on your local machine it appears to suffice for your needs.

Is the goal to only show the latest link? Or would there be a use for showing previous links?

Suggestions

Variable naming

This variable naming might be misleading:

$chars_length = strlen($charset) - 1;

Because the value is one less than the length of the string. If I was working with that code, I would ask you to rename it to something more appropriate, like:

$max_index = strlen($charset) - 1;

since that value is ultimately used to determine the maximum index used to get a character out of the string.

Use uuid() or uuid_short()

Bearing in mind that you might likely need to alter the length of the id column, instead of generating your own random string, you could consider using MySQL's uuid() or uuid_short() function. That might eliminate the need to repeat the loop of calling random_string() and running a query just to see if the return value isn't used by any records.

One other approach to a really short URL would be to just have an auto-increment integer field starting at 1... up until there are 9,999 records, the id would have 1-4 digits...

Unspecified parameter $charset

$charset doesn't appear to be passed by the one place that the function is called. Unless you plan to utilize that, the default value could be made a constant:

define('CHARSET', 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789');

function random_string($length) {
    $chars_length = strlen(CHARSET) - 1;

for vs foreach

I thought about suggesting you use foreach(range(0, $length) but after reading posts like this it might not be wise to get into a habit of that, in case you work on code that deals with large amounts of data.

If you were more used to using foreach, range() can allow you to use foreach like a for statement:

foreach(range(1, $length) as $i) {
    $str .= $charset[random_int(0, $chars_length)];
}


Generating large testing datasets I have made a small Python script that will generate some test sets for my project.

The script generates 2 datasets with the same dimensions n*m. One contains binary values 0,1 and the other contains floats.

The script runs fine and generates the output I need, but if I want to scale to many dimensions the for-loop in pick_random() slows down my computation time.
How can I get rid of it? Perhaps with some array comprehension using numpy?

What throws my reasoning off is the if-stmt. Because the sampling should occur with a probability.

# Probabilities must sum to 1
AMOUNT1 = {0.6 : get_10_20,
           0.4 : get_20_30}

AMOUNT2 = {0.4 : get_10_20,
           0.6 : get_20_30}

OUTCOMES = [AMOUNT1, AMOUNT2]

def pick_random(prob_dict):
    '''
    Given a probability dictionary, with the first argument being the probability,
    Returns a random number given the probability dictionary
    '''
    r, s = random.random(), 0
    for num in prob_dict:
        s += num
        if s >= r:
            return prob_dict[num]()

def compute_trade_amount(action):
    '''
    Select with a probability, depending on the action.
    '''
    return pick_random(OUTCOMES[action])

ACTIONS = pd.DataFrame(np.random.randint(2, size=(n, m)))
AMOUNTS = ACTIONS.applymap(compute_trade_amount)

There are already tools for choosing elements from a collection with some given probabilities. In the standard library there is random.choices, which takes the values, the probabilities and the number of items you want:

import random

values = ["get_10_20", "get_20_30"]
#p = [0.5 * 0.6 + 0.5 * 0.4, 0.5 * 0.4 + 0.5 * 0.6]
p = [0.5, 0.5]

n = 10
random.choices(values, weights=p, k=n)
# ['get_10_20', 'get_10_20', 'get_10_20', 'get_10_20', 'get_10_20', 'get_20_30',
#  'get_20_30', 'get_20_30', 'get_10_20', 'get_10_20']

The other possibility is to use numpy.random.choice for this, which allows you to directly generate multi-dimensional data:

values = ["get_10_20", "get_20_30"]
p = [0.5, 0.5]

n, m = 2, 3
np.random.choice(values, p=p, size=(n, m))
# array([['get_10_20', 'get_10_20', 'get_20_30'],
#        ['get_20_30', 'get_20_30', 'get_20_30']], dtype='<U9')

Both approaches assume that you can combine the probabilities to a total probability for each value. The numpy method enforces that they sum to one, while the standard library random does that for you, if necessary.


Fraction or percentage regular expression I'm learning regular expression as part of my Java course. Now I know sometimes the best use of regex is to not use regex at all. But since I have to use it for this course and learn it, I figure I might as well learn it properly.

I am matching (##/##) or (##) where # is a number 0-9. All white space is ignored (except for in between the digits ##). In context, I'm matching a fraction (10/20) or a percentage (50).

For example (  ##   / ##    ) and (   ## ) are valid. (# #/#  #) and (#  #) are not.

Code / Explanation:

(?:[(])(?:[ ]*)?([\d][\d])(?:[ ]*)?(?:[\/])?(?:[ ]*)?([\d][\d])?(?:[ ]*)?(?:[)])

(?:[(])          Beginning parathensis (
(?:[ ]*)?        All white space
([\d][\d])       Two digits for the first number ##
(?:[ ]*)?        All white space
(?:[\/])?        Forward slash /
(?:[ ]*)?        All white space
([\d][\d])?      Two digits for the second number ##
(?:[ ]*)?        All white space
(?:[)])          Closing parathensis )

Try it online

Example usage in Java:

String regex = "(?:[(])(?:[ ]*)?([\\d][\\d])(?:[ ]*)?(?:[\\/])?(?:[ ]*)?([\\d][\\d])?(?:[ ]*)?(?:[)])";
String test = "(20/50)";

if (test.matches(regex)) { // true
    System.out.println("Valid.");
else {
    System.out.println("Invalid.");
}

Everything is setup using non-capture groups, except for the two digits. This is so I can reference the capture groups in my code (and simply check if group 2 is null before trying to use it, indicating (##) not (##/##)).

This is basically my first time writing regex from complete scratch. Some questions are:

Should I be wrapping everything in [ ] even when they can be left out? i.e. (?: *) instead of (?:[ ]*).
Is my use of non-capture groups the right way to do things (feels verbose to me)?

Generally, what can I improve on?

Your regex accepts (20/) as valid input, and I suspect that you didn't intend to consider it valid.

Your regex rejects one-digit numbers.  If that is intentional, you might want to write a comment about it in the code.

As you suspected, this is a very "noisy" regex — nearly unreadable.  This expression would do the job:

String regex = "\\( *(\\d{2}) *(?:/ *(\\d{2}) *)?\\)";

There is no need to wrap everything in [ ].  In this particular problem, there aren't even any character classes to speak of.  The only place where the square brackets might be justified is as a hack to formulate the expression with fewer backslashes:

String regex = "[(] *(\\d{2}) *(?:/ *(\\d{2}) *)?[)]";

In (?:[ ]*)?, the square brackets are pointless.  The ? is redundant with *.  So, you can just write a space followed by *.

In ([\d][\d]), the square brackets are pointless, and I would slightly prefer seeing (\d{2}) to (\d\d), since it can more easily be modified to accommodate different numbers of digits.

You might want to consider naming the capture groups for clarity:

String regex = "\\( *(?<numerator>\\d{2}) *(?:/ *(?<denominator>\\d{2}) *)?\\)";


Form validation evaluation to avoid exploits Please advise if the form validation script below is secure enough to avoid most types (all types?) of contact form exploits? I ahve found this script online, added some extra php finctions in hope to make it safer, but not complitely sure if it is good for the purpose.

if ($_SERVER["REQUEST_METHOD"] == "POST" && !empty($_SERVER['HTTP_X_REQUESTED_WITH']) && strtolower($_SERVER['HTTP_X_REQUESTED_WITH']) == 'xmlhttprequest') {
    // Get the form fields and remove whitespace.
    $name = strip_tags(trim($_POST["name"]));
    $name = str_replace(array("\r","\n"),array(" "," "),$name);
    $email = filter_var(trim($_POST["email"]), FILTER_SANITIZE_EMAIL);
    $message = strip_tags(trim($_POST["message"]));

    // Check that data was sent to the mailer.
    if ( empty($name) OR empty($message) OR !filter_var($email, FILTER_VALIDATE_EMAIL)) {
        // Set a 400 (bad request) response code and exit.
        //http_response_code(400);
        echo "Oops! There was a problem with your submission. Please complete the form and try again.";
        exit;
    }

    // Set the recipient email address.
    // FIXME: Update this to your desired email address.
    $recipient = "email_here";

    // Set the email subject.
    $subject = "New contact from $name";

    // Build the email content.
    $email_content = "Name: $name\n";
    $email_content .= "Email: $email\n\n";
    $email_content .= "Message:\n$message\n";

    // Build the email headers.

    $email_headers = "MIME-Version: 1.0\r\n";
    $email_headers .= "Content-type: text/html; charset=utf-8\r\n";
    $email_headers .= "From: $name <$email>\r\n";
    $email_headers .= "Reply-To: $email\r\n";
    $email_headers .= "Return-Path: $email\r\n";
    $email_headers .= "Organization: Bilingual Counselling\r\n";

    // Send the email.
    if (mail($recipient, $subject, $email_content, $email_headers)) {
        // Set a 200 (okay) response code.
        //http_response_code(200);
        echo "Thank You! Your message has been sent.";
    } else {
        // Set a 500 (internal server error) response code.
        //http_response_code(500);
        echo "Oops! Something went wrong and we couldn't send your message.";
    }

}

I think it looks ok - though someone might want to add on if they notice any issues I may have missed. However, a bit of cleanup goes a long way:

# Set a 500 (internal server error) response code.
function outputResponse($override = "", $code = 500){
    # http_response_code($code);
    echo $override ?: "Oops! Something went wrong and we couldn't send your message.";
    exit;
}

# Exit early while doing negative checks
if(empty($_SERVER['HTTP_X_REQUESTED_WITH'] ||
    strtolower($_SERVER['HTTP_X_REQUESTED_WITH']) != 'xmlhttprequest' ||
    $_SERVER["REQUEST_METHOD"] != "POST"){
    outputResponse();
}

# Get the form fields and remove whitespace.
$name = strip_tags(trim($_POST["name"]));
$name = str_replace(array("\r","\n"),array(" "," "),$name);
$email = filter_var(trim($_POST["email"]), FILTER_SANITIZE_EMAIL);
$message = strip_tags(trim($_POST["message"]));

# Check that data was sent to the mailer.
if ( empty($name) OR empty($message) OR !filter_var($email, FILTER_VALIDATE_EMAIL)) {
    # Set a 400 (bad request) response code and exit.
    outputResponse("Oops! There was a problem with your submission. Please complete the form and try again.", 400);
}

# Set the recipient email address.
$recipient = "email_here";

# Set the email subject.
$subject = "New contact from $name";

# Build the email content.
$email_content = "Name: $name\n";
$email_content .= "Email: $email\n\n";
$email_content .= "Message:\n$message\n";

# Build the email headers.
$email_headers = "MIME-Version: 1.0\r\n";
$email_headers .= "Content-type: text/html; charset=utf-8\r\n";
$email_headers .= "From: $name <$email>\r\n";
$email_headers .= "Reply-To: $email\r\n";
$email_headers .= "Return-Path: $email\r\n";
$email_headers .= "Organization: Bilingual Counselling\r\n";

# Send the email.
if (mail($recipient, $subject, $email_content, $email_headers)) {
    # Set a 200 (okay) response code.
    outputResponse("Thank you! Your message has beent sent.", 200)
} else {
    outputResponse();
}


Logging a user in and creating a session I have a bunch of functions that return promises and need to chain them together. However, it's currently difficult to read and I would expect it to be even harder to maintain.

I have a function that performs the following actions:

Logs off any inactive users.
Verifies a user's credentials.
Creates a session and logs the user in.

Is there a better way?

generateToken: function (email, password) {
    return new Promise((resolve, reject) => {
        //Kill inactive sessions first
        module.exports.destroyInactive()
        .then(() => {
            //Verify that the password matches the user
            users.checkPassword(email, password, sharedSecret)
            .then((user) => {
                //Check if there is another user already signed in, if there is then we can't
                //produce another token until they are signed out.
                signedon.getByEmailAddressThisProcess(email)
                .then((result) => {
                    let rows = result[0];

                    if (rows.length > 0)
                        return reject('A user with email: ' + email + ' is already logged in.');

                    //Get the current XPROCESS.
                    xprocess.getByProcessId(process.pid)
                    .then((xproc) => {
                        //Create a brand new session object
                        let session = generateSession(user);

                        //Save the session in the signed on table.
                        signedon.signin(session, xproc.pid)
                        .then(() => {
                            resolve(parseSessionToToken(session));
                        },
                        reject);
                    },
                    reject);
                },
                reject);
            },
            reject);
        },
        reject);
    });
},

Potential improvements

generateToken(email, password) {
  let user, session;

  return module.exports.destroyInactive()
    .then(() => users.checkPassword(email, password, sharedSecret))
    .then(_user => user = _user)
    .then(() => signedon.getByEmailAddressThisProcess(email))
    .then(([rows]) => {
      if (rows.length > 0) {
        return Promise.reject(`A user with email: ${email} is already logged in`);
      }

      return xprocess.getByProcessId(process.pid);
    }).then(xproc => {
      session = generateSession(user);

      return signedon.signin(session, xproc.pid);
    }).then(() => parseSessionToToken(session));
}

Your method falls into the nested promises antipattern, very similar to the classic callback pyramid of doom.

This solution relies on chaining promises. Since we need the variables user and session in non-adjacent promises we need to have references of them outside of the promise chain.

Since you are already using Promises which are an ES6 feature, you can take advantage of extra ES6 goodness. See es6-features.org for more.

ES6 features

Enhanced object method properties

generateToken: function (email, password) can be simplified by using  to generateToken(email, password)

Arrow Function Expression Bodies

users.checkPassword(...) is implicitly returned. Since this is a Promise, the chain won't continue to be executed until the returned Promise is resolved/rejected. This means we can just return the promise and not have to continue to nest using checkPassword(...).then(...)

Array Matching

let rows = result[0] can be simplified to rows = [result], which can then be used as the resolve parameter (([rows]) => ...)

Promise.reject

Returning Promise.reject allows us to reject the Promise without nesting.

Template Literals/String Interpolation

Provide a (subjectively) more readable solution by not having to concatenate strings with variables.

'A user with email: ' + email + ' is already logged in.'

becomes

`A user with email: ${email} is already logged in`

Arrow Function Statement Bodies

Since we are doing multiple things inside the rows Promise,  we use => statement bodies.

Due to this we need to explicitly return the Promise (so that the chain doesn't continue to execute until the Promise is resolved/rejected). Otherwise undefined will be resolved, not waiting for the Promise.

Note:

Nowhere are you catching rejected promises using catch, are you catching a rejected generateToken attempt?

Promises can greatly improve readability and remove nested callbacks when used correctly, but it is important to not allow them to "swallow" errors. At the very least make sure to log rejected Promises. A simple somePromise(...).catch(console.error) is better than nothing. Prefer console.error over console.log when possible since it's more "noisy".