Linq query performance improvements As I am getting my Linq query to a functional point, I start looking at the query and think about all the "ANY" and wonder if those should be a different method and then I have data conversions going on.

Does anything jump out as being a performance issue? What is recommended to make this more performant? (Yes, I need all the &&.)

etchVector =
    from vio in list
    where excelViolations.Any(excelVio => vio.VioID.Formatted.Equals(excelVio.VioID.ToString()))
    && excelViolations.Any(excelVio => vio.RuleType.Formatted.Equals(excelVio.RuleType))
    && excelViolations.Any(excelVio => vio.VioType.Formatted.Equals(excelVio.VioType))
    && excelViolations.Any(excelVio => vio.EtchVects.Any(x => x.XCoordinate.Equals(excelVio.XCoordinate)))
    && excelViolations.Any(excelVio => vio.EtchVects.Any(y => y.YCoordinate.Equals(excelVio.YCoordinate)))
    select new EtchVectorShapes
    {
        VioID = Convert.ToInt32(vio.EtchVects.Select(x => x.VioID)),
        ObjectType = vio.EtchVects.Select(x => x.ObjectType).ToString(),
        XCoordinate = Convert.ToDouble(vio.EtchVects.Select(x => x.XCoordinate)),
        YCoordinate = Convert.ToDouble(vio.EtchVects.Select(x => x.YCoordinate)),
        Layer = vio.EtchVects.Select(x => x.Layer).ToString()
    };

You can definitely optimize this query. If list has M items and excelViolations has N items, the query will iterate excelViolations M*5N times.

It is possible to reduce this to M*N iterations by consuming excelViolations only once per item in list. You can do this with a subquery on excelViolations that checks all five conditions for each excelVio, then ORs each of the five conditions across all excelVio instances.

If that is not clear, hopefully this walkthrough is:

from vio in list

// Perform all five checks for each excelVio

let checks = excelViolations
    .Select(excelVio => new
    {
        HasVioID = vio.VioID.Formatted.Equals(excelVio.VioID.ToString()),
        HasRuleType = vio.RuleType.Formatted.Equals(excelVio.RuleType),
        HasVioType = vio.VioType.Formatted.Equals(excelVio.VioType),
        HasXCoordinate = vio.EtchVects.Any(x => x.XCoordinate.Equals(excelVio.XCoordinate)),
        HasYCoordinate = vio.EtchVects.Any(y => y.YCoordinate.Equals(excelVio.YCoordinate))
    })

// From left to right, OR each of the five results (equivalent to .Any)

    .Aggregate((left, right) => new
    {
        HasVioID = (left.HasVioID || right.HasVioID),
        HasRuleType = (left.HasRuleType || right.HasRuleType),
        HasVioType = (left.HasVioType || right.HasVioType),
        HasXCoordinate = (left.HasXCoordinate || right.HasXCoordinate),
        HasYCoordinate = (left.HasYCoordinate || right.HasYCoordinate)
    })

// Filter to only those that pass every check

where checks.HasVioID
    && checks.HasRuleType
    && checks.HasVioType
    && checks.HasXCoordinate
    && checks.HasYCoordinate

// Same projection

select new EtchVectorShapes
{
    VioID = Convert.ToInt32(vio.EtchVects.Select(x => x.VioID)),
    ObjectType = vio.EtchVects.Select(x => x.ObjectType).ToString(),
    XCoordinate = Convert.ToDouble(vio.EtchVects.Select(x => x.XCoordinate)),
    YCoordinate = Convert.ToDouble(vio.EtchVects.Select(x => x.YCoordinate)),
    Layer = vio.EtchVects.Select(x => x.Layer).ToString()
};


Displaying information about a ticket purchase My code works perfectly, however, I know my approach isn't best practice practice. I would appreciate the effort of anyone who tries to explain in details on how I could refactor this code using either(presenters, decorators or poros) - I have no deep understanding of either yet.
Here is the code below:

In my events controller, I have a method called tickets_report defined as such

def tickets_report
  @all_bookings = @event.bookings
  @ticket_types = @event.ticket_types
  @attendees = @event.attendees
end

then, I use the @ticket_types instance variable in my views to plot a table as follows:

<% grand_quantity = ticket_left = quantity_sold = total_price = total_amount = 0 %>
<% @tickets.each_with_index do |ticket, index| %>
    <% remains = ticket.quantity.to_i - ticket.user_tickets.count.to_i
       amount = ticket.price.to_f * ticket.user_tickets.count.to_i
       grand_quantity += ticket.quantity
       ticket_left += remains
       quantity_sold += ticket.user_tickets.count
       total_price += ticket.price.to_f
       total_amount += amount
    %>

            <%= index + 1%>
            <%= ticket.name %>
            <%= ticket.quantity %>
            <%= remains %>
            <%= ticket.user_tickets.count %>
            <%= number_to_currency(ticket.price)%>
            <%= number_to_currency(amount)%>

<% end %>

    Grand Total
    <%= grand_quantity %>
    <%= ticket_left %>
    <%= quantity_sold %>
    <%= number_to_currency(total_price)%>
    <%= number_to_currency(total_amount)%>

My problem is this calculations done in the view is really not a good thing, and this is how far I have gone trying to fix this myself

module Events
  class Ticket
    def initialize(ticket)
      @ticket = ticket
    end

    def name
      @ticket.name
    end

    def quantity
      @ticket.quantity
    end

    def price
      @ticket.price.to_f
    end

    def user_count
      @ticket.user_tickets.count.to_i
    end

    def remains
      @ticket.quantity.to_i - user_count
    end

    def amount
      price  * user_count
    end
  end

  class TicketReportPresenter < Ticket
    @@grand_quantity = @@ticket_left = @@quantity_sold = 0
    @@total_price = @@total_amount = 0

    def initialize(ticket)
      @ticket = Ticket.new(ticket)
      @@grand_quantity += @ticket.quantity.to_i
      @@ticket_left += @ticket.remains
      @@quantity_sold += @ticket.user_count
      @@total_price += @ticket.price
      @@total_amount ++ @ticket.amount
    end

    class << self

      def grand_quantity
        @@grand_quantity
      end

      def ticket_left
        @@ticket_left
      end

      def quantity_sold
        @@quantity_sold
      end

      def total_price
        @@total_price
      end

      def total_amount
        @@total_amount
      end
    end
  end
end

The new tickets_report method in my controller

def tickets_report
  @all_bookings = @event.bookings
  @ticket_types = Events::TicketReportPresenter.new(@event.ticket_types.first)
  @attendees = @event.attendees
end

However, things don't seem to be going right at this point. For instance I cannot call ActiveRecord relationships on the tickets object I defined. What is the best approach to refactor the logic present in the view, a detailed description and source code will suffice plus links to other resources.

I finally figured out a better approach to avoid those complex logic in my view by using a presenter pattern. I create a presenter directory in my app folder and subfolder for the event controller and a tickets_report_presenter.rb file with the following code

module Events
  class TicketsReportPresenter
    attr_reader :event

    def initialize(event)
      @event = event
    end

    def all_bookings
      event.bookings.paginate(page: 1, per_page: 20)
    end

    def attendees
      event.attendees.paginate(page: 1, per_page: 20)
    end

    def ticket_types
      event.ticket_types.map do |ticket|
         Report::Ticket.new(ticket)
      end
    end

    def grand_quantity
      ticket_types.map(&:quantity).inject(0, &:+)
    end

    def ticket_left
      ticket_types.map(&:remains).inject(0, &:+)
    end

    def quantity_sold
      ticket_types.map(&:tickets_sold).inject(0, &:+)
    end

    def total_price
      ticket_types.map(&:price).inject(0, &:+)
    end

    def total_amount
      ticket_types.map(&:amount).inject(0, &:+)
    end

  end

  module Report
    class Ticket
      attr_reader :ticket

      def initialize(ticket)
        @ticket = ticket
      end

      def name
        ticket.name
      end

      def quantity
        ticket.quantity.to_i
      end

      def tickets_sold
        ticket.user_tickets.count.to_i
      end

      def price
        ticket.price.to_i
      end

      def remains
        quantity - tickets_sold
      end

      def amount
        price * tickets_sold
      end
    end
  end
end

Then in my tickets_report action, I did this

def tickets_report
    @presenter = Events::TicketsReportPresenter.new(@event)
end

This allows be to call the following methods in my view:

@presenter.all_bookings
@presenter.attendees
@presenter.ticket_types
@presenter.grand_quantity
@presenter.ticket_left

Thanks to anyone who viewed this.


State Machine for Character I come today with a state machine I'm currently working on.

Problem:

  Given any State that the Character is in, a button press or combination of button presses can modify that state in a different way than how the initial state was entered.

I've looked at several different methods of creating a solution, but I feel that my current solution is lacking in its ability to be extendable. Would a Behavior Tree be more suitable for this kind of problem?

public class StateManager<T> where T : Engine.Object
{
    //See notes in the Constructor.
    IState _currentState,_lastState,_nextState,_defaultState;

T _objectReference;

StateCompletion _onCompletion,_nextOnComplete;

public StateManager(T objectRef)
{

    //Creating a "NullState" removes any and all possibilities of a NRE in the event that the next state is considered Null.
    //It can also be used for error handling or testing deadlock cases.
    _defaultState = new NullState();
    _onCompletion = OnComplete;
    _nextOnComplete = OnComplete;

    //This is to store the object that we are currently manipulating with the state machine itself.
    _objectReference = objectRef;

    //The various states of the state machine shift and store the various states it passes through.
    _currentState = _defaultState;
    _nextState = _defaultState;
    _lastState = _currentState;

    //Enter the default "NullState"
    _currentState.Enter(_objectReference,_lastState,_onCompletion);
}

//Used for the default NullState
private void OnComplete(object isComplete)
{
    Debug.LogWarning("Warning! You are acting on a Null State");
}

public void ChangeState<TU>(StateCompletion onComplete) where TU : IState, new()
{
    //Assign next OnComplete Delegate
    _nextOnComplete = onComplete;

    //Assign the appropriate state.
    _nextState = new TU();
    if(_lastState != _currentState) //Set previous state if applicable.
    {
        _lastState = _currentState;
    }
}

public void Execute()
{
    //This is essentially wrapping the inner state machines update loop.
    _currentState.Execute();

    //Check Completion of the current State to catch when the state is finished calculating it's information.
    if(_currentState.IsComplete()) //States can be
    {
        SwitchState();
    }
}

public void CheckModifier(KeyCode key)
{
    //This would pass the keycode into the state and check to see if it could modify its current actions.
    _currentState.CheckStateModifiers();
    //Something about this screams that this is going to lead to loads of repeated codes.
}

public void PhysicsExecute()
{
    _currentState.PhysicsExcecute();
}

private void SwitchState()
{
    //Call the current states OnComplete delegate.
    _onCompletion(_objectReference);
    _onCompletion = _nextOnComplete; //Assign the next delegate based on the last state change request.

    //Begins calculating the next states information in Excecute.
    _currentState = _nextState;
    _currentState.Enter(_objectReference,_lastState,_onCompletion);

    //Set next OnComplete to the Null completion method.
    _nextState = _defaultState;
    _nextOnComplete = OnComplete;
}
}

I think that you have an issue with general approach you have chosen. StateManager can still be improved tho.

Use better and consistent naming. Why _onCompletion but _nextOnComplete? Why _nextState, but _lastState (shouldn't it be "previous"?)? Why ChangeState() when it doesnt change the current state, but sets the next state instead (i'm not sure i understand why it changes last state as well)? What IsCompleted() does? Does it actually checks for completion (then the name is fine) or does it retrieves the result of operation (then it could use a better naming)? Looks like it is the latter. It should be a property either way. Minor stuff like that improves code readability by a lot.
Delegates are clearly coupled with states, so, if for some reason you can not refactor them into events of IState, you can implemet some aggregator-object:

private class StateInfo
{
    public IState State { get; set; }
    public StateCompletion OnExecuted { get; set; }
}

and then instead of

_onCompletion = _nextOnComplete;
_currentState = _nextState;

use _currentStateInfo = _nextStateInfo. This will reduce the number of fields and assignments.
I dont see an issue with _currentState.CheckStateModifiers();. Repeated code should be dealt with by extracting common logic to the base class (StateBase?).
I'm not sure _currentState.Enter (state entering object) makes much sense. Shouldn't object enter state and not vice versa?

It's hard to tell more without knowing implementation details and the logic you need to implement.


Thread pausing/resuming/canceling with Qt I have written this code to be able to suspend (or to cancel) a worker executed in a separate thread in Qt.

To do it, I have used an instance QWaitCondition and QMutex.

#ifndef MYWORKER_H
#define MYWORKER_H

#include <QObject>
#include <QMutex>
#include <QWaitCondition>

class MyWorker : public QObject
{
    Q_OBJECT
public:
    explicit MyWorker(QObject *parent = 0);
    ~MyWorker();
    void restart();
    void pause();

signals:
    void finished();

public slots:
    void doWork();
    void cancelWork();

private:
    QMutex m_continue;
    QWaitCondition m_pauseManager;
    bool m_cancelRequested;
    bool m_pauseRequired;
};

#endif // MYWORKER_H

Here is the code of myworker.cpp

#include "myworker.h"

#include <QDebug>

MyWorker::MyWorker(QObject *parent) :
    QObject(parent),
    m_cancelRequested(false),
    m_pauseRequired(false)
{
}

MyWorker::~MyWorker()
{
}

void MyWorker::restart()
{
    this -> m_pauseRequired = false;
    this -> m_pauseManager.wakeAll();

}

void MyWorker::pause()
{
    this -> m_pauseRequired = true;
}

void MyWorker::doWork()
{
    // Write your code here
    for(int i = 0; i < 4000000000; i++)
    {
        if (i % 100000 == 0) {
            qDebug() << i;
        }

        if (m_pauseRequired) {
            m_pauseManager.wait(&m_continue);
        }

        if (this -> m_cancelRequested()) {
            break;
        }
    }

    // Write your code above"

    emit finished();
}

void MyWorker::cancelWork()
{
    this -> restart();
    this -> m_cancelRequested = true;
}

Here is the snippet to use this code :

this -> m_thread = new QThread();
this -> m_worker = new MyWorker();
this -> m_worker-> moveToThread(this -> m_thread);
connect(m_thread, SIGNAL(started()), m_worker, SLOT(doWork()));
connect(m_worker, SIGNAL(finished()), m_thread, SLOT(quit()));

this -> m_thread -> start();

// To suspend the work
this -> m_worker -> pause();

// To stop the work
this -> m_worker -> cancelWork();

Do you have any advice on improving this code, or do you have a better solution?

The code you have is unsafe, because pause() and cancelWork() are called from outside the worker's thread, and they use no locking when modifying the worker's members.

In general, prefer to interact with objects in other threads by invoking their slots via queued connections, unless there's a pressing reason to act immediately.  This means you don't need any of the synchronization facilities inside the worker.  This simplifies the coding, and makes it easier to test.

To do that, the worker needs to periodically process events.  I prefer to put this in a function of its own:

bool isCancelled() {
    auto dispatcher = QThread::currentThread()->eventDispatcher();
    if (!dispatcher) return false;
    dispatcher->processEvents();
    return m_cancelRequested;
}

In the work method, this can be called every time you want to poll for cancellation.

You need some protection against re-entering doWork() from this event queue.  One way to manage this is with a tiny state-machine:

private:
    enum State { IDLE, RUNNING, PAUSED };
    State state = IDLE;

Used like this:

void MyWorker::doWork()
{
    if (state == RUNNING)
        return;

    state = RUNNING;
    emit started();

Here's my complete, tested version - no locks or atomics, all the synchronization is managed by Qt:

#include <QAbstractEventDispatcher>
#include <QObject>
#include <QThread>
#include <QDebug>

class Worker : public QObject
{
    Q_OBJECT

public:
    explicit Worker(QObject *parent = 0)
        : QObject(parent)
    {
    }

signals:
    void started();
    void finished();

public slots:
    void pause()
    {
        auto const dispatcher = QThread::currentThread()->eventDispatcher();
        if (!dispatcher) {
            qCritical() << "thread with no dispatcher";
            return;
        }

        if (state != RUNNING)
            return;

        state = PAUSED;
        qDebug() << "paused";
        do {
            dispatcher->processEvents(QEventLoop::WaitForMoreEvents);
        } while (state == PAUSED);
    }

    void resume()
    {
        if (state == PAUSED) {
            state = RUNNING;
            qDebug() << "resumed";
        }
    }

    void cancel() {
        if (state != IDLE) {
            state = IDLE;
            qDebug() << "cancelled";
        }
    }

protected:

    enum State { IDLE, RUNNING, PAUSED };
    State state = IDLE;

    bool isCancelled() {
        auto const dispatcher = QThread::currentThread()->eventDispatcher();
        if (!dispatcher) {
            qCritical() << "thread with no dispatcher";
            return false;
        }
        dispatcher->processEvents(QEventLoop::AllEvents);
        return state == IDLE;
    }

};

class MyWorker : public Worker
{
    Q_OBJECT

public:
    explicit MyWorker(QObject *parent = 0)
        : Worker(parent)
    {
    }

    void doWork()
    {
        if (state == PAUSED)
            // treat as resume
            state = RUNNING;

        if (state == RUNNING)
            return;

        state = RUNNING;
        qDebug() << "started";
        emit started();

        // This loop simulates the actual work
        for (auto i = 0u;  state == RUNNING;  ++i) {
            QThread::msleep(100);
            if (isCancelled()) break;
            qDebug() << i;
        }

        qDebug() << "finished";
        emit finished();
    }
};

#include <QCoreApplication>
int main(int argc, char **argv)
{
    QCoreApplication app(argc, argv);

    QThread thread;
    auto worker = new MyWorker();
    worker->moveToThread(&thread);
    QObject::connect(&thread, &QThread::started, worker, &MyWorker::doWork);
    QObject::connect(worker, &MyWorker::finished, worker, &QObject::deleteLater);
    QObject::connect(worker, &QObject::destroyed, &thread, &QThread::quit);
    QObject::connect(&thread, &QThread::finished, &app, &QCoreApplication::quit);

    thread.start();

    // To saved defining more signals, I'll just use invoke() here.
    // Normally, we connect some "driver" object's signals to the
    // pause/resume/cancel slots.
    auto const m = worker->metaObject();
    QThread::sleep(1);
    m->invokeMethod(worker, "pause");
    QThread::sleep(4);
    m->invokeMethod(worker, "resume");
    QThread::sleep(1);
    m->invokeMethod(worker, "cancel");

    app.exec();
}

Sorry this is a bit rushed; I hope the principle is clear.


F# Binary Tree and Tree Traversal So for an side exercise on exercism I implemented a BinarySearchTree.
I was confident in implementing the creation of the binary tree. But I'm very unsure about the traversal of the tree. So I came up with two implementations of the traversal algorithm - which one is the "better" solution?

I'm learning about F# because I want to learn Functional Programming. This is code review - so any criticism is welcome!

module BinarySearchTree

type Node<'a> =
    { Left: Node<'a> option
      Right: Node<'a> option
      Data: 'a }

let left (node: Node<'a>)  = node.Left

let right (node: Node<'a>) = node.Right

let data (node: Node<'a>) : 'a = node.Data

let rec insertNode (root: Node<'a> option) (nextValue: 'a) : Node<'a> =
    match root with
    | Some root -> if nextValue <= root.Data then { root with Left = Some(insertNode root.Left nextValue) }
                   elif nextValue > root.Data then { root with Right = Some(insertNode root.Right nextValue) }
                   else root
    | None -> { Left=None; Right=None; Data=nextValue }

let create (items: 'a list) : Node<'a> =
     items
     |> List.fold (fun (acc: Option<Node<'a>>) (item: 'a) -> Some(insertNode acc item)) None
     |> function
        | None -> failwith "Failed to construct binary tree. You must pass a valid item list."
        | Some root -> root

let sortedData (node: Node<'a>) : ('a list) =
    let rec traverse (acc: 'a list) (node: Node<'a>)  =
        match (node.Left, node.Right) with
        | Some left, Some right -> seq {
                                   yield! node.Data::acc
                                   yield! (traverse [] left)
                                   yield! (traverse [] right)}
                                   |> Seq.toList
        | Some left, None -> traverse (node.Data::acc) left
        | None, Some right -> traverse (node.Data::acc) right
        | None, None -> node.Data::acc

    List.sort (traverse [] node)

And the alternative implementation of sortedData:

let sortedData (node: Node) : (int list) =

 let rec traverse (acc: int list) (node: Node)  =
        match (node.Left, node.Right) with
        | Some left, Some right -> (node.Data::acc)@(traverse [] left)@(traverse [] right)
        | Some left, None -> traverse (node.Data::acc) left
        | None, Some right -> traverse (node.Data::acc) right
        | None, None -> node.Data::acc

    List.sort (traverse [] node)

In general in F# we try to minimize the use of explicit type declaration on function arguments:

let left (node: Node<'a>) = node.Left

becomes

let left node = node.Left

etc.

The function insertNode violates the single responsibility principle in that if root =  None then it creates a new tree/root node instead of actual inserting the value.

Personal I wouldn't allow root to be Option/None

Instead I would provide a function called newTree taking a 'a value as argument and returning a Node<'a>

  let rec insertNode (root: Node<'a> option) (nextValue: 'a) : Node<'a> = ...

In general you should have object in question of the module as the last argument of a function in order to make use of the pipe operator:

let rec insertNode value root =...

root |> BinarySearchTree.insertNode value

sortedData seems overly complicated - maybe from the use of List.sort. In principle it's just a depth-first-search (dfs), where you for each node can concatenate the values in the left child node with the value of the node itself concatenated with the values in the right child node:

For a list it could be:

let toList root =
    let rec dfs optNode =
        match optNode with
        | Some node -> (dfs (left node))@(data node::dfs (right node))
        | None -> []

    dfs (Some root)

Or if you prefer a Sequence (which I do):

let toSeq root =
    let rec dfs optNode =
        match optNode with
        | Some node -> seq {
            yield! dfs (left node)
            yield data node
            yield! dfs (right node)
            }
        | None -> Seq.empty

    dfs (Some root)

I call the functions toList and toSeq. Maybe toOrderedList/Seq are better names.

You could consider to extend the List-module with a toBinaryTree items instead of the create items function. That way it is possible to write:

let data = [ 5; 8; 1; 5; 1; 2; 2; 4; 6; 3 ]
let root = data |> List.toBinaryTree

which may be more intuitive

In the create items function you should consider to sort the items and do a binary insertion in order to create a balanced tree, because if you provide a list of items that is sorted then your function will merely produce a singly linked list of nodes using only one of the child nodes instead of a tree.

You should consider to provide a function that can balance an existing tree.

As an alternative to the Node<'a> type as a record type, you could consider to define it as a discriminated union type with the following definition:

type Node<'a> =
    | Empty
    | Node of Data:'a * Left:Node<'a> * Right:Node<'a>

That is more in line with the functional style of F# and your functions become somewhat more simple and intuitive:

module BinaryTree =

let data = function Empty -> failwith "Empty Tree" | Node (d, _, _) -> d
let left = function Empty -> Empty | Node (_, l, _) -> l
let right = function Empty -> Empty | Node (_, _, r) -> r

let first root =
    let rec goLeft node =
        match node with
        | Empty -> None
        | Node (d, ln, _) ->
            match ln with
            | Empty -> Some d
            | Node _ -> goLeft ln

    goLeft root

let last root =
    let rec goRight node =
        match node with
        | Empty -> None
        | Node (d, _, rn) ->
            match rn with
            | Empty -> Some d
            | Node _ -> goRight rn

    goRight root

let rec insert data root =
    match root with
    | Empty -> Node(data, Empty, Empty)
    | Node (d, l, r) ->
        match data <= d with
        | true -> Node (d, l |> insert data, r)
        | false -> Node (d, l, r |> insert data)

let toOrderedSeq root =
    let rec dfs node =
        match node with
        | Empty -> Seq.empty
        | Node (d, ln, rn) -> seq {
                yield! dfs ln
                yield d
                yield! dfs rn
            }

    dfs root

module List =
    let toBinaryTree data = data |> List.fold (fun n x -> n |> BinaryTree.insert x) Empty

As seen, the use of option is unnecessary and the insert function is consistent, because an Empty node as root is also a Node<'a>/Tree - just without any elements.


Recode C -> C++ trampoline function macros using templates I'm reworking PyCXX (C++ wrapper for Python).

I'm currently examining the following macro code, and wondering whether there is a way to write it using templates rather than macros.

If so, the question becomes: what is the best way to write it? (I personally favour a concise macro solution over an awkward template solution).

#define PYCXX_NOARGS_METHOD_NAME( NAME ) _callNoArgsMethod__##NAME
#define PYCXX_VARARGS_METHOD_NAME( NAME ) _callVarArgsMethod__##NAME
#define PYCXX_KEYWORDS_METHOD_NAME( NAME ) _callKeywordsMethod__##NAME

// - - -

#define PYCXX_DECLARE_METHOD( THIS_CLASS, METHOD_NAME, Ax, NAME_OF_THIS_STATIC_METHOD, ... ) \
    \
    static PyObject* NAME_OF_THIS_STATIC_METHOD( PyObject* _pyObj_caller, PyObject* _a, PyObject* _k ) \
    { \
        DBG_LINE( Ax << #NAME_OF_THIS_STATIC_METHOD << Ax ); \
        try \
        { \
            Py::PythonClassInstance* base_cxx_obj   = reinterpret_cast< Py::PythonClassInstance* >( _pyObj_caller ); \
            THIS_CLASS*              this_class_obj = reinterpret_cast< THIS_CLASS* >             ( base_cxx_obj->m_pycxx_object ); \
            Py::Object result{  (this_class_obj -> METHOD_NAME)(__VA_ARGS__)  }; /* __VA_ARGS__ uses _a and _k (see below) */ \
            return Py::new_reference_to( result.ptr() ); \
        } \
        catch( Py::Exception & ) \
        { \
            DBG_LINE( "! ! !  Exception in PYCXX_DECLARE_METHOD ! ! !" ); \
            return 0; \
        } \
    }

#define PYCXX_NOARGS_METHOD_DECL(   _class, name ) PYCXX_DECLARE_METHOD( _class, name, "  &  "     , PYCXX_NOARGS_METHOD_NAME(   name ) )
#define PYCXX_VARARGS_METHOD_DECL(  _class, name ) PYCXX_DECLARE_METHOD( _class, name, "  & &  "   , PYCXX_VARARGS_METHOD_NAME(  name ), Py::Tuple{_a} )
#define PYCXX_KEYWORDS_METHOD_DECL( _class, name ) PYCXX_DECLARE_METHOD( _class, name, "  & & &  " , PYCXX_KEYWORDS_METHOD_NAME( name ), Py::Tuple{_a}, _k?Py::Dict{_k}:Py::Dict{} )

... and

#define PYCXX_ADD_METHOD( py_name, method_name, docs, flags ) \
    add_method( \
               #py_name, \
               (PyCFunction)method_name, \
               flags, \
               docs \
               )

#define PYCXX_ADD_NOARGS_METHOD(   py_name, name, docs )  PYCXX_ADD_METHOD( py_name, PYCXX_NOARGS_METHOD_NAME(   name ), docs, METH_NOARGS )
#define PYCXX_ADD_VARARGS_METHOD(  py_name, name, docs )  PYCXX_ADD_METHOD( py_name, PYCXX_VARARGS_METHOD_NAME(  name ), docs, METH_VARARGS )
#define PYCXX_ADD_KEYWORDS_METHOD( py_name, name, docs )  PYCXX_ADD_METHOD( py_name, PYCXX_KEYWORDS_METHOD_NAME( name ), docs, METH_VARARGS | METH_KEYWORDS )

It is used in a class to expose certain methods to Python:

Py::Object new_style_class_func_noargs( void )
{
    std::cout << "new_style_class_func_noargs Called."           << std::endl << std::endl;
    std::cout << "value ref count " << m_value.reference_count() << std::endl << std::endl;
    return Py::None();
}
PYCXX_NOARGS_METHOD_DECL( new_style_class, new_style_class_func_noargs )

static void init_type(void)
{
    :
    PYCXX_ADD_NOARGS_METHOD(    func_noargs , new_style_class_func_noargs,  "docs for func_noargs"  );
}

The reason for this architecture is that Python can only fit "extern C" style functions into its tables.  But this code is attempting to run C++ instance member functions.

Hence it has to trampoline off a static member function (as a static member function type casts to an "extern C" style function).

And we require a separate static member function for each method we wish to expose.

Is there any way to use templates to accomplish this?

Surprisingly (to me at any rate) there is a template solution.

It appears that it is possible to do:

template<FuncSignature funcPointer>
myFunc(){...}

And later:

x = myFunc<&MyClass::MyMethod>

You can pick up the trail here: https://stackoverflow.com/questions/26934036/coding-static-to-instance-method-trampoline-function-with-templates


Functions asking for user input, with default prompts I succeeded in the first part of the exercise, this attempt is the second part. I am not sure I have accomplished the goal, and would appreciate the input of more knowledgeable minds. As always, critiques of my techniques welcome.

This is an exercise from the book titled 'Beginning C++ through Game Programming' by Michael Dawson.

Challenge me, despite my novice level of understanding.

The exercise:

  Using default arguments, write a function that asks the user for a number and returns that number. The function should accept a string prompt from the calling code. If the caller doesn't supply a string for the prompt, the function should use a generic prompt.

  Next, using function overloading, write a function that achieves the same results.

My code:

// Chapter 05, Exercise 03: Beginning C++ through Game Programming

/*  Using default arguments, write a function that asks the user for a
    number and returns that number. The function should accept a string
    prompt from the calling code. If the caller doesn't supply a string
    for the prompt, the function should use a generic prompt.

    Next, using function overloading, write a function that achieves
    the same results.
*/

#include <iostream>
#include <string>

int askNumberFn(int number, std::string prompt = "Please enter a number, now: "); // default arguments must come last, in order of excecution
std::string askNumberFn(std::string prompt = "Please type a number, now: "); // default parameters must be different in order to overload Fn

char exitTimeFn();

int main()
{
    askNumberFn("Please type a number: "); // We require the calling code to return a prompt
    askNumberFn(); // We also require the Fn to return a default prompt

    int number = 0;
    number = askNumberFn(number, "Please enter a number: "); // To prove we have overloaded the FN, we require the Fn to also return an int

    number = 0; // We return the value of number to zero, in order to remove any doubt, if we input the same number again
    number = askNumberFn(number); // Lastly, we have to prove our overloaded Fn displays a default prompt

    exitTimeFn();
    return 0;
}

int askNumberFn(int number, std::string prompt)
{
    std::cout << prompt;
    std::cin >> number;
    std::cout << "You entered the number: " << number << std::endl;
    return number;
}

std::string askNumberFn(std::string prompt)
{
    std::string word;
    std::cout << prompt;
    std::cin >> word;
    std::cout << "You typed the number: " << word << std::endl;
    return word;
}

char exitTimeFn()
{
    char exitNow;
    std::cout << "Press 'x' to exit: ";
    std::cin >> exitNow;
    return exitNow;
}

Correctness: There are some cases that aren't handled. What happens if the user enters a word instead of a number? What if the user enters 2.1? The program will exit regardless of what character the user enters when exitTimeFn() is called.

I think your overload was a clever solution to a vague instruction. To make it clear that the extra parameter is irrelevant, you could comment out the name. For example:

int askNumberFn(int /*number*/, std::string prompt)
{
    int number;
    std::cout << prompt;

Because parameters are used to supply data to functions, it doesn't make sense to pass in a value, then immediately re-assign it before use.

Parameters: I would pass the strings by const reference instead of by value.

Naming: No need for Fn at the end of every function. It's obvious that it's a function from how it's used. The names could also be slightly more descriptive: getNumberFromUser, showExitDialogue, for example.

Commenting: Generally, I'd discourage line-by-line comments explaining what each line of code is doing. It's usually a sign you need to write clearer code. The way you use it in your main function to document your testing has some merit, I think. Look into unit testing for a more scalable/professional way of doing the same thing.

If accidentally using the same input for testing is a concern, I'd just use a different variable completely. A unit testing library would provide better tools to handle this.

While I believe this fulfils the question literally, there is an implied task of reacting reasonably to invalid input. I would test more thoroughly and make sure the edges don't do anything surprising.


Assembling a json object from an array Is there a way to make this code shorter and more succinct? Thanks

My objective is to:

Take an array of objects (someArray)
Use get() to get one specific property of each of someArr's objects (which is also an object itself, as I am working with a json object)
And push this object to the 'tabs' array of the newly created object I am returning

function (someArray) {
    var resultObj = {};
    resultObj.tabs = [];
    someArray.map(function (obj) {
        resultObj.tabs.push(obj.get());   // get() gets the dataValues (Sequelize)
    });
    return resultObj;
}

Instead of pushing to an array outside of the map function, just return obj.get() in each callback and use the return value of the map call.


LeetCode 952: Largest Component Size by Common Factor I'm posting my code for a LeetCode problem copied here. If you would like to review, please do so. Thank you for your time!
Problem

Given a non-empty array of unique positive integers A, consider the
following graph:

There are nums.length nodes, labeled nums[0] to nums[nums.length - 1];

There is an edge between nums[i] and nums[j] if and only if nums[i] and nums[j] share a common factor greater than 1.

Return the size of the largest connected component in the graph.

Example 1:

Input: [4,6,15,35]
Output: 4

Example 2:

Input: [20,50,9,63]
Output: 2

Example 3:

Input: [2,3,6,7,4,12,21,39]
Output: 8

Note:

\$1 <= nums.length <= 20000\$
\$1 <= nums[i] <= 100000\$

Inputs
[4,6,15,35]
[20,50,9,63]
[2,3,6,7,4,12,21,39]
[2,3,6,7,4,12,21,39,100,101,102,103,104,1200,123,123,13424]

Outputs
4
2
8
15

Code
#include <cstdint>
#include <vector>
#include <algorithm>
#include <unordered_map>
#include <cmath>

#define max(a, b) (a > b ? a : b) // std::max(a, b)

// Union Find
struct DisjointSets {
    DisjointSets(int_fast64_t n) : parent(n) {
        for (int_fast64_t index = 0; index < n; index++) {
            parent[index] = index;
        }
    }

    void union_ds(const int_fast64_t x, const int_fast64_t y) {
        parent[find_ds(x)] = parent[find_ds(y)];
    }

    const int_fast64_t find_ds(const int_fast64_t x) {
        if (parent[x] != x) {
            parent[x] = find_ds(parent[x]);
        }

        return parent[x];
    }

private:
    std::vector<int> parent;
};

struct Solution {
    static const size_t largestComponentSize(const std::vector<int> &nums) {
        const size_t max_nums = *std::max_element(nums.begin(), nums.end());
        DisjointSets union_find(max_nums + 1);

        for (const auto &num : nums) {
            for (size_t k = 2; k <= std::sqrt(num); k++) {
                if (num % k == 0) {
                    union_find.union_ds(num, k);
                    union_find.union_ds(num, num / k);
                }
            }
        }

        std::unordered_map<int, int> component_map;
        size_t largest_component = 1;

        for (const auto &num : nums) {
            size_t curr_component = ++component_map[union_find.find_ds(num)];
            largest_component = max(largest_component, curr_component);
        }

        return largest_component;
    }
};

References

Problem

Discuss

Solution

Union Find

Observation
I don't understand your solution.
Code Review
#define max(a, b) (a > b ? a : b) // std::max(a, b)

Don't do this. Use std::max() directly.
Long gone are the days that we used macros to inline code like this. The std::max() will work correctly in all situation this macro has edge cased that will fail (especially if a or b are not just simple numbers).

The size_t is coming because you included a C header file cstdint. It is not guaranteed to be in the global namespace (it commonly is but its not guaranteed). So prefer to use std::size_t.
        const size_t max_nums = *std::max_element(nums.begin(), nums.end());


Collecting adjacent cells in a 2D grid implemented as a 1D list The grid is implemented as an ArrayList<Cell> cells with the number of columns as int cols and int length = rows * cols. The id of a Cell is equal to its position in the list. The first element of cells is the top-left cell and the last element is the bottom-right cell.

public ArrayList<Cell> getAdjCells(int id) {
    ArrayList<Cell> cs = new ArrayList<>();
    //check if cell is on an edge
    boolean l = id %  cols > 0;        //has left
    boolean u = id >= cols;            //has upper
    boolean r = id %  cols < cols - 1; //has right
    boolean d = id <  length - cols;   //has lower
    //collect all existing adjacent cells
    if (l)      cs.add(cells.get(id - 1       ));
    if (l && u) cs.add(cells.get(id - 1 - cols));
    if (u)      cs.add(cells.get(id     - cols));
    if (u && r) cs.add(cells.get(id + 1 - cols));
    if (r)      cs.add(cells.get(id + 1       ));
    if (r && d) cs.add(cells.get(id + 1 + cols));
    if (d)      cs.add(cells.get(id     + cols));
    if (d && l) cs.add(cells.get(id - 1 + cols));
    return cs;
}

I may restructure ArrayList<Cell> cells as Cell[] cells later on. Return type can be any type of collection really. I just want this function to be as fast as possible, since I have to use it very often.

Looks good, ship it.

Certainly cols is an accurate name, but in this context sometimes people prefer to refer to it as stride.

It's close to the initial default, but still, since you know the max size you might specify a capacity: new ArrayList<>(8);

Modulo can be slightly expensive. Maybe you want to assign the result to a temp variable, or maybe you trust that JIT already did that for you.

The sequential memory references to {l&u, u, u&r} are cache friendly. The reverse sequence of addresses, {r&d, d, d&l} is perhaps backwards from what you prefer.


Count the number of occurences of X's within a range in Java Given an integer X within the range of 0 to 9, and given two positive

  integers as upper and lower bounds respectively, find the number of
  times X occurs as a digit in an integer within the range, excluding
  the bounds. Print the frequency of occurrence as output.

  Input: The first line of input is an integer T, denoting the number of
  test cases. For each test case, there are two lines of input, first
  consisting of the integer X, whose occurrence has to be counted.
  Second, the lower and upper bound, L and U which are positive
  integers, on the same line separated by a single space, respectively.

  Output: For each test case, there is only one line of output, the
  count of the occurrence of X as a digit in the numbers lying between
  the lower and upper bound, excluding them.

  Constraints: 1<=T<=100 0<=X<=9 0

  Example: Input 5 3 100 250 2 10000 12345 0 20 21 9 899 1000 1 1100
  1345 Output: 35 1120 0 120 398

  Explanation:

  In the first test case, the occurrence of 3 in the numbers starting
  from 101 to 249 is counted and comes out to be 35.

My approach:

/*package whatever //do not write package name here */

import java.io.BufferedReader;
import java.io.InputStreamReader;
import java.io.IOException;

class GFG {

    private static int numOccurences (int X, int low, int up)
        {
            String str;
            int totOcc = 0;

            //Convert a digit 0-9 to a character
            char chk = Character.forDigit(X,10);
            for (int i = low+1; i < up; i++)
                {
                    str = String.valueOf(i);
                    totOcc += numCharsInStr(str,chk);
                }
            return totOcc;
        }

    private static int numCharsInStr (String str, char ch)
        {
            int numOccurs = 0;
            for (int i = 0; i < str.length(); i++)
                {
                    if (str.charAt(i) == ch)
                        {
                            numOccurs++;
                        }
                }
            return numOccurs;
        }

    public static void main (String[] args) throws IOException {
        //code
        BufferedReader br = new BufferedReader (new InputStreamReader (System.in));
        String line = br.readLine();
        String line2;
        String line3;

        int numTests = Integer.parseInt(line);
        int X;
        String [] inps;
        int lower;
        int upper;

        for (int i = 0; i < numTests; i++)
            {
                line2 = br.readLine();
                X = Integer.parseInt(line2);
                line3 = br.readLine();
                inps = line3.split(" ");
                lower = Integer.parseInt(inps[0]);
                upper = Integer.parseInt(inps[1]);
                System.out.println(numOccurences(X,lower,upper));
            }
    }
}

I have the following questions with regards to the code:

How can I further improve my approach?
Is there a better way to solve this question?
Are there any grave code violations that I have committed?
Can space and time complexity be further improved?

Reference

Here are my comments:

Find occurences of digit in String

Instead of iterating on every character, you can harness the power of regular expression.  What we can do is replaceAll characters that are not the one we are interested in with an empty String effectively removing these characters and we are left with a String that contains all occurrences of the character.
For example, say we want to count occurrences of the digit 1 in str:

str.replaceAll("[^1]", "").length()

If you are concerned about performance, you should know that str.replaceAll() is a costly operation because the regex "[^1]" is parsed each time the method is called.  This can be solved: you can parse the regex one time into an instance of java.util.regex.Pattern:

Pattern notDigitPattern = Pattern.compile("[^"+x+"]");  // x is the digit we want to count

This is done one time only.  when you want to find occurences of x in str you do that by constructing an instance of java.util.regex.Matcher and using that to do the replaceAll() operation:

Matcher notDigitMatcher = notDigitPattern.matcher(str);
notDigitMatcher.replaceAll("").length()

Iteration on numbers

The 'main' loop iterates on the range of numbers between low and up.  However, One can calculate in advance the number of occurences of any digit within range of 10 or 100 or 1000 and so on.
let us examine the range of 10. a digit will occur only once in that range except if it is the first digit as well.
The digit 3 apears once for 0x (0-9), 1x (10-19), 2x (20-29), and so on. it apears 11 times in 3x (30-39) 10 times as 1st digit and 1 time as 2nd.
sum of the above for all ranges of 10s (0-99) is 9*1+11=20. this is true for all 10 digits.
If we look at range of 100s than the digit will occur 20 times for all 100 ranges except if it is also the first digit, in this case add 100 occurrences.
sum of the above for all ranges of 100s (0-999) is 9*20+120=300.
you can continue this calculation for 1000s and so on.

When you receive an input of 111 to 3333 you know how many times the digit occurred between 1000 and 1999 and also 2000-2999. so you just need to iterate for the first and last 1000 (between 112 and 999 and 3000 to 3332) and you can also figure out complete 100 ranges. then figure out complete 10 ranges... at the end you will need to iterate on the first and last range of 10s.

I understand this seems unclear (and maybe too complicated) but it does save time complexity for large ranges.

Coding best practices

Closing IO resources
although the stream you open on stdin will be closed when the program finishes, you should get the habbit of explicitly closing all resources, so yuo don't get resource leakage when you develop not-so-trivial programs.  Use Java 7 try-with-resources feature for that.
Declare variables in the scope where they are used
let's look at the variable inps in main().  It is used inside the loop as a kind if temporary variable for parsing the input.  It is better to declare it inside the loop (each pair of curly braces define a new scope).  There are two reasons for that: 1) declaring the variable inside the loop helps readability of your code - other developers will understand that inps is not used anywhere else. and 2) you don't run the risk of someone trying to use inps out of scope.


Bit array implementation I have made a bit array implementation that can be seen below:

unsigned int uint_size();

class Bitarray1D {
private:
    unsigned int * array_;
    int elements_;
    int bits_;

public:
    Bitarray1D(int bits);

    ~Bitarray1D();

    void size(int& bits) const;

    unsigned int count() const;

    void setAll(int value);

    void set(int index, int value);

    int test(int index) const;

    unsigned int operator [](int index) const;

    void flip(int index);

    std::string to_string() const;

private:
    void init();

};

and some of the functions:

void Bitarray1D::init(){
        if (bits_ == 0) throw std::invalid_argument("Can't initialize zero size array");

        int number_of_elements = bits_;
        std::cout << "unsigned int size: " << uint_size() << " bytes" << std::endl;

        elements_ = std::ceil((bits_*1.0)/(uint_size()*1.0));
        array_ = new unsigned int[elements_];
}

int Bitarray1D::test(int index) const {
        if (index < 0 || index >= bits_) throw std::out_of_range("Index out of range");

        int element = index / uint_size();
        int bit = index % uint_size();

        unsigned int temp_element = array_[element];
        temp_element = temp_element >> bit;

        unsigned int mul = 1;
        temp_element = temp_element & mul;
        return temp_element;
}

unsigned int Bitarray1D::operator [] (int index) const {
        if (index < 0 || index >= bits_) throw std::out_of_range("Index out of range");

        return test(index);
}

uint_size() is a function that returns the size of an unsigned int in bytes.

While this works well, i want to implement the assignment in array-like style as well, i.e.: bitarray[i] = 1;

In the current implementation this is not possible. In order to do so, I figured that [] operator in Bitarray should return a "Bit object" with the appropriate operators like below:

class Bit{
  private:
        unsigned int& element_;
        unsigned int index_;

    int getValue() const{
        // get value of bit
    }
public:
    Bit(unsigned int& element, unsigned int index): element_(element), index_(index){}

    Bit& operator [] (int value){
        return *this;
    }

    Bit& operator = (int value){
          //assign value to the bit
    }

    friend std::ostream& operator<<(std::ostream& os, const Bit& bit){
        os << bit.getValue();
        return os;
    }
};

This implementation works, but has two major problems:

Memory leak. In order for the [] operator in Bitarray to return a reference to a Bit object I have to return the following:

Bit& Bitarray1D::operator [] (int index) const {
     return *(new Bit(..., ... ));
}

I could change the array_ of the bitarray to hold an array of class Bit, but then the whole point of the Bitarray is lost, which is smaller memory footprint.

Don't write a function to return a constant

You don't need uint_size(), as the returned value never changes (for a given target platform).  You can replace it with

#include <climits>
static const std::size_t uint_bits = CHAR_BIT * sizeof (unsigned int);

Don't require a call to init()

The constructor should fully initialize the object; just move the body of init() there instead.

Use integer operations on integers

Instead of using std::ceil to round up division, we can do this in integer arithmetic by adding one less than the divisor before dividing:

    elements_ = (bits + uint_bits - 1) / uint_bits;

Given that we know the number of bits, we can always compute the number of elements when we need it; you might consider the trade-off between storing and computing elements_.

Obey the Rule of Five (or Rule of Zero)

You allocate memory in the constructor, but don't provide copy/move constructors, nor an assignment operator, so you run the risk of double delete[].  The easiest fix is to use a standard container as the storage; you might also consider using a smart pointer to the storage.  As a last resort, you could manage the memory yourself, but I'd advise against that.

Use an unsigned type for indexing

You can avoid needing to check for negative index values by accepting an unsigned integer type in the methods.  I recommend std::size_t as the standard type usually used for that.

Return by value when possible

void size(int& bits) const;

is better written as

std::size_t size() const;

My version

Working replacement code:

#include <climits>
#include <stdexcept>
#include <vector>

class Bitarray1D {
    using Element = unsigned int;

    class Bit {
        Element& element;
        const Element mask;
    public:
        Bit(Element& element, Element mask) : element{element}, mask{mask} {}
        Bit(const Bit& other) : Bit{other.element, other.mask} {}
        operator bool() const { return element & mask; }
        Bit& operator=(bool b) { element = b ? element | mask : element & ~mask; return *this; }
        Bit& operator=(const Bit& other) { return this->operator=(bool(other)); }
    };

    static const std::size_t element_bits = CHAR_BIT * sizeof (Element);

    std::size_t bits;
    std::vector<Element> array;

public:
    Bitarray1D(std::size_t bits);
    ~Bitarray1D() = default;

    std::size_t size() const;

    Bit operator[](std::size_t index);

};

Bitarray1D::Bitarray1D(std::size_t bits)
    : bits{bits},
      array{}
{
    // rounded-up division
    array.resize((bits + element_bits - 1) / element_bits);
}

Bitarray1D::Bit Bitarray1D::operator[](std::size_t index) {
        if (index >= bits)
            throw std::out_of_range("Index out of range");
        return Bit(array[index / element_bits], 1u << (index % element_bits));
}

int main()
{
    Bitarray1D bits{15};
    bits[0] = 0;
    bits[2] = bits[1] = 1;
    return bits[0] + bits[1] + bits[2] != 2;
}


SQL cross join and multiple joins Is there a way to improve this SQL query?  It involves multiple cross join and joins.

I have 3 tables, and I want to compute the cardinal product:

table N:
    N
    -
    4

table sums:
    i   S
    -----------
    1   22
    2   26
    3   22

table mults:
    i   j   M
    -------------------
    1   1   122
    1   2   144
    1   3   119
    2   1   144
    2   2   170
    2   3   141
    3   1   119
    3   2   141
    3   3   126

Here is a fiddle.

SELECT  ((N.n*M1.m)-(S1.s*S2.s)) / (SQRT((N.n*M2.m)-(pow(S1.s,2))) * SQRT((N.n*M3.m)-(pow(S2.s,2))))
FROM N
CROSS JOIN sums as S1
CROSS JOIN sums as S2
JOIN mults as M1
     ON M1.i = S1.i
     AND M1.j = S2.i
JOIN mults as M2
     ON M2.i = S1.i
     AND M2.j = S1.i
JOIN mults as M3
     ON M3.i = S2.i
     AND M3.j = S2.i;

This SQL, for what it does, is neat, and concise. There is no obvious place where any optimizations can be made. With the data size as small as it is, there is no reason to recommend indexes, or other improvements.

The only glaring issue has been pointed out already: There are tools other than SQL that are designed to do these types of heavy computation with performance that your database just won't beat.

Apart from anything else, the log functions are notoriously hard to get right, and there are multiple 'standards' for implementation.

Consider math libraries designed for data and computations such as this.


Inefficient Parsec parser to skip unreachable block In a parser written with Parsec 3.1.11, I have a performance issue with parsing (or better, skipping) the unreachable branch of a if-then-else statement.

The syntax to be parsed is as follows:

_if (condition) {
  # block of statements
} _else {
  # block of statements
}

The _else branch is optional and only statically resolvable conditions are allowed.

The parser for the if-then-else block:

type MyParser a = ParsecT T.Text UserState (StateT UserState Identity) a

ifelseStmt :: MyParser -> [Statement]
  = do  reserved "_if"
        e <- parens expression
        if evalBoolExpr e
          then do b <- block
                  option [] $
                    do reserved "_else"
                       unparsedBlock
                  return b
          else do unparsedBlock
                  option [] $
                    do reserved "_else"
                       block

Now, parser unparsedBlock is meant to throw away the entire {...} block of the unreachable branch. Obviously this can in turn contain {..} blocks, which means curly brackets have to be matched.

This is the parser I wrote for it:

unparsedBlock :: MyParser -> [Statement]
  = do braces (many (do { noneOf "{}"; return []}  <|> unparsedBlock))
       return []

It works well, but the profiler shows that way too much time is spent on it, while I'd expect it to be rather efficient.

Can anyone see how I could implement unparsedBlock more efficiently?

First of all, you can batch the skipping of braces (and return ()):

unparsedBlock :: Parser ()
unparsedBlock = braces (skipMany (skipMany (noneOf "{}") <|> unparsedBlock))

Unfortunately, this is also probably quite inefficient, as this still works on a character-by-character basis, and uses stuff like manyAccum under the hood.

If you really need more speed, then you will need to write your own Parser function that does the skipping on the source, while at the same time updating the position in the stream.

That is (one of) the reason(s) attoparsec is much faster : it doesn't keep track of the position in the input stream, and has combinators that work directly on the source (such as skipWhile).


Constraining data in idiomatic Java Playing with a simple Java Point class where I would like to constrain the X and Y values to be doubles that must be in the range -10 to 10, inclusive. I've written some code, but it's been years since I've written java and I would like to know if this is how it would be written in modern Java:

public class Point {
    private double x;
    private double y;

    public Point(double x, double y) {
        constrain("x", x);
        constrain("y", y);
        this.x = x;
        this.y = y;
    }

    // is there a cleaner/shorter way of handling this, such as a direct way of declaring a
    // subtype of double that I could use in method signatures?
    protected static void constrain(String name, double val) {
        if ( val < -10 || val > 10 ) {
            throw new IllegalArgumentException(name + " must be between -10 and 10");
        }
    }

    public double getX() { return x; }

    public void setX(double x) {
        constrain("x", x);
        this.x = x;
    }

    public double getY() { return y; }

    public void setY(double y) {
        constrain("y", y);
        this.y = y;
    }

    @Override
    public String toString() {
        return ("[" + x + "," + y + "]");
    }
}

Remove the magic numbers for the constrained values and give them constants instead:

private static final double MAX_VAL = 10;
private static final double MIN_VAL = -10;

These constants should also be printed in the exception statement instead of hard-coding the values.  Otherwise, if you just change the constants, the code will give inaccurate results.

Regarding the class name, consider renaming it to something like Constrained2DPoint.  Points can also have a 'z' if they're used in 3D space, so the name Point may be ambiguous in either case.


Unordered list in HTML with animated list points I had this idea today, but I don't have any practical usage for it right now. Any feedback or improvements welcome.

Code is found at CodePen, and also presented here as a snippet:

ul {
  list-style-type: none;
  width: 250px;
  border: 2px solid #a9a9a9;
  border-radius: 16px;
  margin: 0 auto;
}

.item {
  margin: 10px 0;
}

.point {
  display: inline-block;
  width: 20px;
  height: 20px;
  margin-right: 20px;
  border-radius: 0 50%;
  transform: rotate(0deg);
}

.point:hover {
  transform: rotate(-45deg);
}

.text {
  display: inline-block;
  width: 200px;
  color: #121212;
  font-family: helvetica;
}

li:nth-of-type(1) div:first-child {
  background-color: rgba(30, 30, 30, 1.0);
}

li:nth-of-type(2) div:first-child {
  background-color: rgba(60, 60, 60, 1.0);
}

li:nth-of-type(3) div:first-child {
  background-color: rgba(90, 90, 90, 1.0);
}

li:nth-of-type(4) div:first-child {
  background-color: rgba(120, 120, 120, 1.0);
}

li:nth-of-type(5) div:first-child {
  background-color: rgba(150, 150, 150, 1.0);
}

Alpha

Beta

Gamma

Delta

Epsilon

Your HTML is far too overloaded.

The .point elements are unnecessary and can be replaced by using the pseudo-element ::before. This requires adding the property content: "".
Generally as a rule, it's wrong to use the same class on all child elements, especially when it's generic like item. Better mark the list itself and use a child selector to select the items, for example .animated-list > li.
The <div class="text"> also seem unnecessary for this example. Setting display: inline-block and width have nothing to do with animated bullets and the color and font-family should be assigned on a higher level element such as the body.

In the CSS, as mentioned above

replace ul with ul.animated-list
replace .item with ul.animated-list > li
replace .point with ul.animated-list > li::before

The sudden use of :first-child instead of .point seems strange. Use ul.animated-list > li::before here too, thus ul.animated-list > li:nth-of-type(1)::before.

Finally, I'd expect the the bullets to animate when hovering over the list item not just the bullet itself, so I'd replace .point:hover with ul.animated-list > li:hover::before.

And for some "flair" consider adding transition: transform 0.1s; to get a smooth animation.

EDIT: Here's my code: http://codepen.io/anon/pen/adoWxp


jQuery Delay slideDown using Queue I have the task to create a function that adds some messages to the page and another to show them in last to first order with a jQuery slideDown() effect.

jsFiddle

jQuery("#input1").click(function()
{
    //Represents the first function to add the messages
    jQuery.each(["
1
", "
2
", "
3
"],
        function(index, item)
        {
            var jDiv =jQuery(item);
            jDiv.hide();
            jQuery(".parent").prepend(jDiv);
        });

    //represents the second function to show them.
    var jParent = jQuery(".parent");
    jParent.children().reverse().each(function()
    {
        var jThis= jQuery(this);
        jParent.queue( function()
       {
           jThis.slideDown(function()
           {
               jParent.dequeue();
           });

       });
    });
});

(reverse from this answer)

jQuery.fn.reverse = [].reverse;

This seems to be an awful bunch of code just to show them one after another. Is there any way to clean up/remove redundant code?

I would use function recursion in place of a jQuery queue.
The central piece is the displayNextHiddenMessagesIn function. As the name implies, it grabs the next hidden message and displays it.  After the message is displayed, it calls this function again... looping until all messages are visible.
To me, this seems much more readable and easier to maintain. Also, it would be trivial to have a another message parent and reuse this same code (i.e. error messages and information messages).
(function($) {

    function createMessageFor(parent, contents) {
        var $parent = $(parent);

        for(var content in contents) {
            var $message = newMessage(contents[content]);

            $parent.prepend($message);
        }

        function newMessage(text) {
            return $("<div />").text(text).hide();
        };
    }

    var displayNextHiddenMessagesIn = function(parent) {
        var $parent = $(parent);

        $parent.find('div:hidden:last').slideDown(function() {
            displayNextHiddenMessagesIn($parent);
        });
    };

    $("#input1").click(function()
    {
        createMessageFor('.parent', [1,2,3]);
        displayNextHiddenMessagesIn('.parent');
    });

})(jQuery);

see it in action: http://jsfiddle.net/natedavisolds/TP66b/6/
Update
Here's what it looks like as a plugin:
(function($) {
    $.fn.fanDownHiddens = function() {
        return this.each(function() {
            var $parent = $(this);

            $parent.find('div:hidden:last').slideDown(function() {
                $parent.fanDownHiddens();
            });
        });
    }
})(jQuery);

And then the onload with the test data.
(function($) {

    $(function() {
        $("#input1").click(function() {
           setupTestMessages('.parent');
           $('.parent').fanDownHiddens();
        });
    });

     function setupTestMessages(parent) {
        var $parent = $(parent),
            testData = [1,2,3];

        for(var content in testData) {
            var $message = newMessage(testData[content]);

            $parent.prepend($message);
        }

        function newMessage(text) {
            return $("<div />").text(text).hide();
        };
    }
})(jQuery);

Here's the plugin working: http://jsfiddle.net/natedavisolds/TP66b/12/


Simple quadtree template implementation I'm returning to C++ after more than 10 years.  The code I wrote for a simple quadtree style class has a ton of new elements I haven't used.  I'm looking for comments on the design and not necessarily the quadtree implementation.

The code in its current state will work for my needs, with the exception of error catching.  Can I improve it? Am I making any design mistakes?

#include <array>

template <typename T>
class Quadtree
{
public:
    typedef std::array<Quadtree, 4> Quadrants; // class scope typedef

    T data; // the template data
    Quadtree* parent;

    Quadtree(T _data) // constructor
    {
        data = _data;
    }

    void Split(Quadrants nodes)
    {
        quadrants = &nodes;

    }

    Quadtree operator[](int _index) // keeps syntax consistent with std::array
    {
        return (*quadrants)[_index];
    }
private:
    Quadrants* quadrants;
};

Usage looks like this:

Quadtree<int> tree(5);
Quadtree<int> node(3);
Quadtree<int>::Quadrants quadrant = { *new Quadtree<int>(1), *new Quadtree<int>(2), *new Quadtree<int>(4), node };
tree.Split(quadrant);

int value = tree[3].data;
// value now equals 89

It actually works...

After thinking a bit I believe I figured out why this thing works.

This paragraph is inteded purely for people who thinks that the question is invalid due to broken code.

It is invalid. Yes. In terms of standard C++. But undefined behavior doesn't mean that the behavior can be defined on some platform when using some compiler. By defined I don't mean accident. By defined I mean reproducible and consistent behavior. Compiler can do copy elision from call site to function site, so that the function will receive the same thing in terms of memory location as on the call site!

But it is non standard conformant compiler. Or is it? The thing is that taking the address of the local variable and using it somewhere else is undefined behavior, so why miss such a great opportunity to optimize code? I believe these are the thoughts that came to mind of the compiler implementer.

May be it is possible to verify that using disassembly.

Code Review:

As I mentioned in the comment, the design is strange mix of C and Java/C#. Class is used, but parent and data are left public. Data is taken by value (or maybe by reference was intended?). So there are lots of strange things happening here aside from the undefined behavior.

Ambiguous model

After a few minutes thinking I couldn't find out what is meant by the Quadtree.

Obsolete design

C++ now has smart pointers. I've seen on cppcon 2016 unique_ptr<> compiling down to the version with raw pointers. It was simple case, but I believe more complex cases will behave the same. It might complicate design a bit, but it will make it leak free by default (well, the program can crash though, due to double free, but it is better than memory leak in my opinion).

using declarations are now preferred to typedefs.

using Quadrants = std::array<Quadtree, 4>;

No clear preference of value/reference

The array is stored by pointer. I know that may be copy elision is intended, but the code leaves lifetime management of the nodes to the caller, which potentially makes the usage very hard and error prone. Famous Scott Meyers said: "Make interfaces easy to use correctly and hard to use incorrectly". Very sound advice and should be kept in mind when designing public interfaces.

There is a thing called move constructor in C++11, but on std::array<> it will behave the same as if it is by element copy, so rip it. Some wrapper over std::vector<> might be preferred.

Non idiomatic C++

In general, I would say that the code doesn't look like C++ in the form that I see here or on the web by acknowledged people. Though I would say that it is also important to know when idiomatic C++ can't do the job wanted, but idiomatic C++ should be preferred if it possible.


Generating a 64 bit unique integer in Java I need to generate a 64 bit unique integer in Java. I need to make sure that there is very few or no collisions if possible.

I came up with the below code which works fine:

public class TestUniqueness {

    private static final AtomicLong TS = new AtomicLong();

    public static void main(String[] args) {
        // for testing, just added the for loop
        for (int i = 1; i <= 100000; i++) {
            System.out.println(getUniqueTimestamp());
        }
    }

    public static long getUniqueTimestamp() {
        long micros = System.currentTimeMillis() * 1000;
        for (;;) {
            long value = TS.get();
            if (micros <= value)
                micros = value + 1;
            if (TS.compareAndSet(value, micros))
                return micros;
        }
    }
}

I will be running the above code in production.

My specification is only to generate the 64 bit unique integer, that's it.

In this case, there's no need for anything more complicated than atomically incrementing a counter:

public class Counter {

    private static final AtomicLong counter = new AtomicLong(0);

    public static long getNextNumber(){
        return counter.incrementAndGet();
    }
}

To offer a more specific critique of your code, it's unnecessarily complicated and inefficient.  I don't see anything incorrect about it (i.e. it seems like it meets your criteria), but there's the old saying about obviously no bugs vs no obvious bugs (paraphrased).


Programming Puzzle - N Teams K Wins each I found a problem here. I am sharing the problem and my attempt at solution. I would like to find out if this is the best approach possible.

  One day, at the "Russian Code Cup" event it was decided to play
  football as an out of competition event. All participants was divided
  into n teams and played several matches, two teams could not play
  against each other more than once.

  The appointed Judge was the most experienced member - Pavel. But since
  he was the wisest of all, he soon got bored of the game and fell
  asleep. Waking up, he discovered that the tournament is over and the
  teams want to know the results of all the matches.

  Pavel didn't want anyone to discover about him sleeping and not
  keeping an eye on the results, so he decided to recover the results of
  all games. To do this, he asked all the teams and learned that the
  real winner was friendship, that is, each team beat the other teams
  exactly k times. Help Pavel come up with chronology of the tournir
  that meets all the conditions, or otherwise report that there is no
  such table.

  Input

  The first line contains two integers:  \$n\$ and \$k\$ (\$1 ≤ n, k ≤ 1000\$).

  Output

  In the first line print an integer \$m\$ - number of the played
  games. The following \$m\$ lines should contain the information about all
  the matches, one match per line. The \$i\$-th line should contain two
  integers \$ai\$ and \$bi\$ (\$1 ≤ ai\$, \$bi ≤ n\$; \$ai ≠ bi\$). The numbers \$ai\$ and \$bi\$
  mean, that in the \$i\$-th match the team with number \$ai\$ won against the
  team with number \$bi\$. You can assume, that the teams are numbered from
  1 to \$n\$.

  If a tournir that meets the conditions of the problem does not exist, then print -1.

My solution:

<?php
//$n = $argv[1];
//$k = $argv[2];
$s = fopen("php://stdin", 'r');
$n = stream_get_contents($s);
$dum= explode(" ",trim($n));
$n=$dum[0];
$k=$dum[1];
if ($n * $k > ($n * ($n - 1)) / 2) {
    echo "-1\n";exit;
}
$output = $n*$k."\n";
$winMap = array();
for ($i = 1; $i <= $n; $i++) {
    $winMap[$i] = 0;
}
for ($i = 1; $i <= $n; $i++) {
    for ($j = $i + 1; $j <= $n; $j++) {
        if ($winMap[$i] < $k) {
            $output .= "$i $j\n";
            $winMap[$i] += 1;
        } else if ($winMap[$j] < $k) {
            $winMap[$j] += 1;
            $output .=  "$j $i\n";
        }
    }
}
echo $output;
?>

Style

You are lacking consistency in the spacing in assignments.

Organisation

It might be worth extracting the algorithm in a function. It makes code easier to read and to test.

<?php
function guessChronology($n, $k)
{
    if ($n * $k > ($n * ($n - 1)) / 2) {
        return "-1\n";
    }
    $output = $n*$k."\n";
    $winMap = array();
    for ($i = 1; $i <= $n; $i++) {
        $winMap[$i] = 0;
    }
    for ($i = 1; $i <= $n; $i++) {
        for ($j = $i + 1; $j <= $n; $j++) {
            if ($winMap[$i] < $k) {
                $output .= "$i $j\n";
                $winMap[$i] += 1;
            } else if ($winMap[$j] < $k) {
                $winMap[$j] += 1;
                $output .=  "$j $i\n";
            }
        }
    }
    return $output;
}

$s = fopen("php://stdin", 'r');
$n = stream_get_contents($s);
$dum = explode(" ",trim($n));
$n = $dum[0];
$k = $dum[1];
echo guessChronology($n, $k);
?>

Re-using functions

You can use array_fill to write $winMap = array_fill(1, $n, 0);


Generating a random directed graph I wrote a function that returns a directed graph as an adjacency matrix. The function requires two arguments: the amount of nodes and the amount of edges. At first, nodes are placed and instantly connected to the graph. When all nodes are added, random edges are created until all edges are placed.
Code:
public int[,] GenerateMatrix(int Nodes, int Edges)
    {
        if (Edges < Nodes - 1) throw new Exception("Too few edges");
        if (Edges > Nodes * (Nodes - 1)) throw new Exception("Too many edges");

        int[,] adjacencyMatrix = new int[Nodes, Nodes];

        // Gives every cell a value of zero
        for (int y = 0; y < Nodes; y++)
        {
            for (int x = 0; x < Nodes; x++)
            {
                adjacencyMatrix[x, y] = 0;
            }
        }

        int placedEdges = 0;

        for (int i = 1; i < Nodes; i++)
        {
            // produce edge between rnd(0, amountofnodes) to new node
            int fromVertex = random.Next(0, i);
            int weight = random.Next(1, 10);

            adjacencyMatrix[i, fromVertex] = weight;
            placedEdges++;
        }

        while (placedEdges < Edges)
        {
            int fromVertex = random.Next(0, Nodes);
            int weight = random.Next(1, 10);

            int targetVertex = random.Next(0, Nodes);
            while (targetVertex == fromVertex || adjacencyMatrix[targetVertex, fromVertex] != 0) //|| adjacencyMatrix[fromVertex, targetVertex] != 0)// tredje condition tar bort parallella kanter
            {
                fromVertex = random.Next(0, Nodes);
                targetVertex = random.Next(0, Nodes);
            }

            adjacencyMatrix[targetVertex, fromVertex] = weight;
            placedEdges++;
        }

        return adjacencyMatrix;
    }

Here are my suggestions:

Nodes, Edges: most of the time the function's parameters don't start with big letter. nodes and edges are more preferable.

Also consider to restraint the possible range of these variables. uint or even smaller like ushort might be a better fit for your use case.

throw new Exception: It is good that you do preliminary checks. Usually the preferred Exception type is ArgumentException (1) to indicate that you have a problem with the calling parameters.

adjacencyMatrix: You don't need to zero all of the entries inside this matrix because by default their value will be zero. (int is a value type with a default value 0).

If you wish to clean the array, because you have received that for example via a parameter then you can use the built-in Array.Clear (1):
Array.Clear(adjacencyMatrix, 0, adjacencyMatrix.Length);

random.Next(0, i): Alternative: random.Next() % i

random.Next(1, 10): Alternative: new Random().Next() * (10 - 1) + 1 (1)

placedEdges++: instead of increasing it every time you could initialize it with the proper value: int placedEdges = Nodes -1;

fromVertex and targetVertex:

fromVertex + toVertex
OR sourceVertex and targetVertex would

give more convenient naming.


Perform a Caesar Cipher Shift on a given string I'd like feedback on my solution to the outlined programming challenge (medium level). I've tried it fast in any way I know how to, but, what might be a more efficient and/or pythonic solution?

  [Caesar Cipher]
  Using the Python language, have the function CaesarCipher(str,num) take the
  str parameter and perform a Caesar Cipher shift on it using the num parameter
  as the shifting number. A Caesar Cipher works by shifting each letter in the
  string N places down in the alphabet (in this case N will be num).
  Punctuation, spaces, and capitalization should remain intact. For example if
  the string is "Caesar Cipher" and num is 2 the output should be
  "Ecguct Ekrjgt".

import doctest
import logging
import timeit

from string import ascii_lowercase, ascii_uppercase

logging.basicConfig(
    level=logging.DEBUG,
    format="%(asctime)s - %(levelname)s - %(message)s")
# logging.disable(logging.CRITICAL)

ALPHABET_LENGTH = 26
# Wrap alphabet letters
LOWERCASE = ascii_lowercase + ascii_lowercase
UPPERCASE = ascii_uppercase + ascii_uppercase

def encode_string(s: str, n: int) -> str:
    """Perform a Caesar Cipher shift on s leaving punctuation, spaces intact

    :param s: the str on which to perform a Cipher shift
    :type s: str
    :param n: the number of places in the alphabet to shift the letter.
              Positive for a forward shift, negative for a backwards shift
    :type n: int
    >>> encode_string(s="Caesar Cipher", n=2)
    'Ecguct Ekrjgt'
    >>> encode_string(s="Caesar Cipher", n=54)
    'Ecguct Ekrjgt'
    >>> encode_string(s="Aa Bb", n=0)
    'Aa Bb'
    >>> encode_string(s="Cc Dd", n=-28)
    'Aa Bb'
    """
    if n ==0:
        return s

    # Incase of very large n, find lowest equivalent offset
    n = n % ALPHABET_LENGTH

    # value of n used to index constants, equivalent to -n steps in backwards direction
    if n < 0:
        n = ALPHABET_LENGTH - n

    new_str = ""

    for c in s:
        if c in LOWERCASE:
            i = LOWERCASE.index(c)
            encoded_char = LOWERCASE[i + n]
            new_str = new_str + encoded_char
        elif c in UPPERCASE:
            i = UPPERCASE.index(c)
            encoded_char = UPPERCASE[i + n]
            new_str = new_str + encoded_char
        else:
            new_str = new_str + c

    return new_str

if __name__ == "__main__":
    doctest.testmod()
    print(timeit.timeit(
        "encode_string(s='Caesar Cipher', n=2)",
        setup="from __main__ import encode_string, ALPHABET_LENGTH, LOWERCASE, UPPERCASE",
        number=100000)
    )

Generally speaking your code looks quite good from my point of view. It's nicely structured, readable and documented. Well done!

Using LOWERCASE = ascii_lowercase + ascii_lowercase in conjunction with n = n % ALPHABET_LENGTH also seems to be a very clever way to implement the shift.

You don't need

if n < 0:
    n = ALPHABET_LENGTH - n

because modulo will handle negative numbers in the same way naturally.

Using LOWERCASE.index(...) to find the position of the letter in the alphabet could be optimized by using some kind of look-up table like so:

LOWERCASE_LUT = {letter: i for i, letter in enumerate(ascii_lowercase)}
UPPERCASE_LUT = {letter: i for i, letter in enumerate(ascii_uppercase)}

Also appending to a string with + in Python is wasteful, because strings are immutable, so each time this is done, a new string will be created. What you'll often find instead is to collect the string parts in a list which is then joined at the end.

All of this leads to the following code:

def encode_string_cr(s: str, n: int) -> str:
    """Perform a Caesar Cipher shift on s leaving punctuation, spaces intact

    :param s: the str on which to perform a Cipher shift
    :type s: str
    :param n: the number of places in the alphabet to shift the letter.
              Positive for a forward shift, negative for a backwards shift
    :type n: int
    >>> encode_string(s="Caesar Cipher", n=2)
    'Ecguct Ekrjgt'
    >>> encode_string(s="Caesar Cipher", n=54)
    'Ecguct Ekrjgt'
    >>> encode_string(s="Aa Bb", n=0)
    'Aa Bb'
    >>> encode_string(s="Cc Dd", n=-28)
    'Aa Bb'
    """
    if n == 0:
        return s

    n %= ALPHABET_LENGTH

    chars = []
    for c in s:
        if c in ascii_lowercase:
            i = LOWERCASE_LUT[c]
            encoded_char = LOWERCASE[i + n]
        elif c in ascii_uppercase:
            i = UPPERCASE_LUT[c]
            encoded_char = UPPERCASE[i + n]
        else:
            encoded_char = c
        chars.append(encoded_char)

    return "".join(chars)

This is roughly 1,5x faster than the original code, at the cost of a little bit more memory for those look-up tables.


Finding top most N common character grams in English I suspect there might be a much more efficient way to accomplish this task.  I would appreciate a more clever hint for performance increase.  I am also looking for feedback on style and simplification where appropriate.

I assume that the set of what counts as English words is a given and that I don't need to do something clever like an actual dictionary or use Google n-grams.  That being said, the results could differ depending on the domain and also what is accepted as a word.

The original problem set M=3 (3-gram characters), which makes the results not so interesting (ans=[(u'the', 191279), (u'and', 92292), (u'ing', 57323), (u'her', 44627), (u'hat', 38609)]).

#gets the top N most common substrings of M characters in English words
from nltk import FreqDist
from nltk.corpus import gutenberg

def get_words():
    for fileid in gutenberg.fileids():
        for word in gutenberg.words(fileid):
            yield word

def ngrams(N, which_list, strict=False):
    list_size = len(which_list)
    stop = list_size
    if strict:
        stop -= (N - 1)
    for i in xrange(0, stop):
        element = which_list[i]
        ngram = [element]
        j = 1
        index = j + i
        while j < N and index < list_size:
            ngram.append(which_list[index])
            j += 1
            index += 1
        yield ''.join(ngram)

def m_most_common_ngram_chars(M=5, N=3):
    n_grams = []
    for word in get_words():
        for ngram in ngrams(N, word, strict=True):
            n_grams.append(ngram)
    f = FreqDist(n_grams)
    return f.most_common(M)

l = m_most_common_ngram_chars(M=5, N=3)
print l

Building up on Caleth's answer:

Beware that:

def ngrams(N, word):
    for i in xrange(len(word) - N):
        yield word[i:i+N]

will not account for the last character:

for g in ngrams(3, "small"):
    print(g)

will output

sma
mal

If it was the purpose of the strict parameter to allow to include/skip the last character (and be able to print the missing all), you can use it that way:

def ngrams(N, words, strict=True):
    last = int(strict)
    for i in xrange(len(word) - N + last):
        yield word[i:i+N]

If, however, you wanted to allow to generate n_grams whose length is lower than N:

def ngrams(N, words, strict=True):
    last = N - 1 if strict else 0
    for i in xrange(len(word) - last):
        yield word[i:i+N]

As per the documentation, FreqDist accepts generators in its constructor. It is thus more memory efficient to turn the n_grams list into a generator:

n_grams = (ngram for ngram in ngrams(N, word) for word in get_words())

Your top comment would be most suited as a docstring for the m_most_common_ngram_chars function. In fact, each function might need its own docstring.
Since almost all your code is functions, you may want to put your last two lines into the module check if __name__ == "__main__":. It will allow you to run your code from the command line as usual and it will do exactly the same thing; but also to import it and call the function with other parameters without running anything at import time.

from nltk import FreqDist
from nltk.corpus import gutenberg

def get_words():
    """generate english words from the whole gutemberg corpus"""
    for fileid in gutenberg.fileids():
        for word in gutenberg.words(fileid):
            yield word

def ngrams(N, word, strict=True):
    """generate a sequence of N-sized substrings of word.
    if strict is False, also account for P-sized substrings
    at the end of the word where P < N"""

    last = N - 1 if strict else 0
    for i in xrange(len(word) - last):
        yield word[i:i+N]

def m_most_common_ngram_chars(M=5, N=3):
    """gets the top M most common substrings of N characters in English words"""
    f = FreqDist(ngram for ngram in ngrams(N, word) for word in get_words())
    return f.most_common(M)

if __name__ == "__main__":
    # Uses the default values M=5, N=3
    print m_most_common_ngram_chars()


Macro that interchanges 2 arguments Define a macro swap(t, x, y) that interchanges two arguments of type t.(Block structure will help.)

The ideea is that a variable defined in a block structure exists only inside the block structure. So, I can create a temporary variable without affecting the code.

Here is my solution:

#include <stdio.h>

#define swap(t, x, y) {t tmp = x; x = y; y = tmp;}

int main() {
    int x = 10, y = 2;
    swap(int, x, y);

    printf("%d %d", x, y);
    return 0;
}

If you are using GCC, we can use the typeof()(C99) keyword to get rid of one of the arguments.  Also, add a do-while so the macro to be used in contexts where it would otherwise be problematic.

#define SWAP(a, b) do { typeof(a) t; t = a; a = b; b = t; } while(0)

You could also use an exclusive-or (^=) to get rid of that temporary variable, but that only works for integers.

#define SWAP(a, b) do { a ^= b; b ^= a; a ^= b; } while(0)


Vectorize Matlab sum I have the following Matlab function:

function [res] = a3_funct(x)
    res = 0;
    for i = 1:size(x,1)
        res = res + abs(x(i))^(i+1);
    end
end

It's emulating this equation:

\$f(x) = \sum_{i=1}^{n}|x_i|^{i+1}\$

Here is an example use:

>> a3_funct([1,2,3]')

ans =

    90

I know I should be able to use the sum() function to make this faster, but how do I get the exponents in there?

Vectorized approach:

res = sum(abs(x(:).'.^(2:numel(x)+1)))

Read more about vectorization techniques here.

Thus, your function would look like this:

function res = a3_funct(x)
    res = sum(abs(x(:).'.^(2:numel(x)+1)));
return


Weather App - is my code efficient? I have been building a weather app that changes the background colour depending on the weather temperature and description.
This is the code I have to get it to work.
const body = document.querySelector('body');

const colorList = [
  {weather: 'Clear', color1: '#7AE7C7', color2: '#72C1E1'},
  {weather: 'Clouds', color1: '#F981BB', color2: '#7F7E84'},
  {weather: 'Drizzle', color1: '#b2c9c8', color2: '#698b8b'},
  {weather: 'Fog', color1: '#C5B2A6', color2: '#7F7E84'},
  {weather: 'Rain', color1: '#504AC4', color2: '#59AED1'},
  {weather: 'Snow', color1: '#bfc9cf', color2: '#77BDE0'},
  {weather: 'Thunderstorm', color1: '#314F71', color2: '#4A4176'},
  {weather: 'Tornado', color1: '#939393', color2: '#e47977c5'}

 ]

const colorList2 = [
      {color1: '#C94926', color2: '#BB9F34'},
      {color1: '#89a1dd', color2: '#E4E5E7' }
   ]

if(weather.list[0].main.temp < 5 && weather.list[0].weather[0].main == 'Clear') {
      body.style.backgroundImage = `linear-gradient(to bottom right,
      ${colorList2[1].color1}, ${colorList2[1].color2})`;
   } else if(weather.list[0].main.temp > 15 && weather.list[0].weather[0].main == 'Clear'){
        body.style.backgroundImage = `linear-gradient(to bottom right,
        ${colorList2[0].color1}, ${colorList2[0].color2})`;
     } else {
          colorList.forEach(color => {
             if(color.weather == weather.list[0].weather[0].main) {
                body.style.backgroundImage = `linear-gradient(to bottom right,
                ${color.color1}, ${color.color2})`;
   }

Is there a more efficient way of doing the same thing? Are forEach loops overkills? Should I use if else statements? Should I be doing it at all in JavaScript?
Many thanks in advance.

First, your weather color list is unique by weather key, and you're querying the color based on it. I'd suggest turning this into an object of colors keyed by weather condition. This way, it's easier to locate the colors by condition than using a loop. You also gain one extra advantage: you can add your special cases of Clear in the same object, just keyed with a unique key value.
Next, you'll want to assign those properties into variables. Long property accesses are hard to read.
Now you'll want to split your logic. In your code, you're mixing color selection logic with background setting logic in a conditional. It's always important to figure out what thing is actually conditional from what can be extracted off of it. In this case, only color1 and color2 are conditional, not the setting of the background.
Here's how I'd do it.

const colorsByCondition = {
  Clear: {
    color1: '#7AE7C7',
    color2: '#72C1E1'
  },
  // Special forms of Clear
  Clear5: {
    color1: '#89a1dd',
    color2: '#E4E5E7'
  },
  Clear15: {
    color1: '#C94926',
    color2: '#BB9F34'
  },
  // Other conditions
  Clouds: {
    color1: '#F981BB',
    color2: '#7F7E84'
  },
  // and so on...
}

// Assign to variables
// const weather = weather.list[0]
// const temp = weather.main.temp
// const conditions = weather.weather[0].main

// Assuming these values represent our weather
const temp = 6
const conditions = 'Clear'

// Find the right colors
const { color1, color2 } = temp < 5 && conditions == 'Clear'
  ? colorsByCondition.Clear5
  : temp > 15 && conditions == 'Clear'
    ? colorsByCondition.Clear15
    : colorsByCondition[conditions]

// Apply to the body
document.body.style.backgroundImage = `linear-gradient(to bottom right, ${color1}, ${color2})`


IEnumerable to typed Dictionary Scenario

A portable controls/widgets library for Xamarin.Forms (PCL Profile 111) contains a combobox/dropdown/picker control where the ItemsSource property is an IEnumerable.

The ItemsSource can be populated with any IEnumerable of objects.
The combobox displays value by

DisplayMemberPath: string

The combobox syncs selection:

SelectedItem : object
SelectedIndex: int

To do all this the IEnumerable must be parsed so we can

Select index
Select item from source collection

The tricky part

Parse IEnumerable to Dictionary<object, object> in a Portable Class Library.
The sourceDictionary field is used to sync the "selected" properties as described above.

    private Dictionary<object, object> sourceDictionary;

    /// <summary>
    ///     Parse type of ItemsSource as Dictionary and populate <see cref="Picker.Items" /> and
    ///     <see cref="sourceDictionary" />.
    /// </summary>
    private void PopulateItemsAndSelectionValuesFromDictionary()
    {
        // TODO: Better way to get Dictionary from IEnumerable
        if (this.ItemsSource is Dictionary<object, object>)
        {
            foreach (var newItem in (Dictionary<object, object>)this.ItemsSource)
            {
                this.AddToItemsAndSelectionValues(newItem.Key, newItem.Value);
            }
        }
        else if (this.ItemsSource is Dictionary<object, string>)
        {
            foreach (var newItem in (Dictionary<object, string>)this.ItemsSource)
            {
                this.AddToItemsAndSelectionValues(newItem.Key, newItem.Value);
            }
        }
        else if (this.ItemsSource is Dictionary<object, int>)
        {
            foreach (var newItem in (Dictionary<object, int>)this.ItemsSource)
            {
                this.AddToItemsAndSelectionValues(newItem.Key, newItem.Value);
            }
        }
        else if (this.ItemsSource is Dictionary<int, object>)
        {
            foreach (var newItem in (Dictionary<int, object>)this.ItemsSource)
            {
                this.AddToItemsAndSelectionValues(newItem.Key, newItem.Value);
            }
        }
        else if (this.ItemsSource is Dictionary<string, object>)
        {
            foreach (var newItem in (Dictionary<string, object>)this.ItemsSource)
            {
                this.AddToItemsAndSelectionValues(newItem.Key, newItem.Value);
            }
        }
        else if (this.ItemsSource is Dictionary<int, int>)
        {
            foreach (var newItem in (Dictionary<int, int>)this.ItemsSource)
            {
                this.AddToItemsAndSelectionValues(newItem.Key, newItem.Value);
            }
        }
        else if (this.ItemsSource is Dictionary<string, string>)
        {
            foreach (var newItem in (Dictionary<string, string>)this.ItemsSource)
            {
                this.AddToItemsAndSelectionValues(newItem.Key, newItem.Value);
            }
        }
    }

    /// <summary>
    ///     Add the provided KeyValuePair to <see cref="Picker.Items" /> and <see cref="sourceDictionary" />
    /// </summary>
    private void AddToItemsAndSelectionValues(object key, object value)
    {
        this.Items.Add(this.ValueIsDisplayMember() ? value.ToString() : key.ToString());
        this.sourceDictionary.Add(this.ValueIsDisplayMember() ? key : value, new KeyValuePair<object, object>(key, value));
    }

If you know that ItemsSource is a dictionary then you can simply cast it into IDictionary and iterate it because you just need objects and it gives you exacly that:

var items = (IDictionary)ItemsSource;
foreach (var key in items.Keys)
{
    AddToItemsAndSelectionValues(key, items[key]);
}

You cast it into a concrete dictionary but you never use the types anyway so go directly with objects instead.

That's all. You don't need any of those ifs.