C Implementation of atof I'm a beginner at C.
I'm currently implementing atof to build a raytracer, however I'm still learning how to efficiently write programs.
Assignement
Instructions
The program takes a scene description file as argument to generate objects. Some of these paramaters are float. Example of the file.

I'm parsing through the file. As I'm restricted in terms of lines allowed per function and I'm currently learning how double pointers work, I'm using a double char pointer. Example of one such function using lc_atof.
int    a_parsing(char *str, t_pars *data)
{
    if (*(str++) == 'A')
    {
        if (((data->a_ratio = lc_atof(&str)) >= 0.0) && data->a_ratio <= 1.0 && errno == 0)
        //
            if (((data->a_R = lc_atoi(&str)) >= 0) && data->a_R <= 255 && errno == 0)
                if (*(str++) = ',' && ((data->a_G = lc_atoi(&str)) >= 0) && data->a_G <= 255 && errno == 0)
                    if (*(str++) = ',' && ((data->a_B = lc_atoi(&str)) >= 0) && data->a_B <= 255 && errno == 0)
                        return (skip_space(&str));
    }
    return (0);
}

Current code to review:
#include <stdlib.h>
#include <unistd.h>
#include <fcntl.h>
#include <stdio.h>
#include <string.h>
#include <limits.h>
#include <errno.h>
#include <float.h>

static float    conversion(char **str)
{
    double    d_nbr;
    double    power;

    d_nbr = 0.0;
    power = 10.0;
    while (isdigit(**str))
    {
        d_nbr = d_nbr * 10.0 + (**str - 48);
        if (d_nbr > FLT_MAX)
        {
            errno = EIO;
            return (-1);
        }
        (*str)++;
    }
    if (**str == '.')
    {
      (*str)++;
      if (isdigit(**str))
      {
        d_nbr = d_nbr * 10.0 + (**str - 48);
        if (d_nbr > FLT_MAX)
        {
            errno = EIO;
            return (-1);
        }
        (*str)++;
        return ((float)(d_nbr / power));
      }
    }
    errno = EIO;
    return (-1);
}

float            lc_atof(char **str)
{
    float    n;
    int        sign;

    n = 0.0;
    sign = 1.0;
    if (!str || !*str)
    {
        errno = EIO;
        return (-1);
    }
    while (isspace(**str))
        (*str)++;
    if (**str == '+' || **str == '-')
    {
        if (**str == '-')
            sign = -1.0;
        (*str)++;
    }
    if (!isdigit(**str))
    {
        errno = EIO;
        return (-1);
    }
    if ((n = conversion(str)) == 0 && errno != 0)
       return (-1);
    return (sign * n);
}

The only tweaks to the actual atof I've made is having a double char pointer as argument and returning -1 in case of errors.
Every input much appreciated.

Portability
There is no guarantee that this code will be using ASCII so it would be better to use '0' rather than 48 which is something of a magic number. Using '0' makes it more readable and easier to understand.
lc_atof Doesn't Handle String Termination or End Of Line Correctly
This code doesn't handle a NULL terminated string or an end of line character. The function isspace() returns true for end of line so the code will walk right past it.
    while (isspace(**str))
        (*str)++;
    if (**str == '+' || **str == '-')
    {
        if (**str == '-')
            sign = -1.0;
        (*str)++;
    }
    if (!isdigit(**str))
    {
        errno = EIO;
        return (-1);
    }

Complexity
I reaize that you didn't ask for this to be review, but the complexity of each if statement in the example call function is too much and caused me to make an error in my review previously:
int a_parsing(char* str, t_pars* data)
{
    if (*(str++) == 'A')
    {
        if (((data->a_ratio = lc_atof(&str)) >= 0.0) && data->a_ratio <= 1.0 && errno == 0)
            //
            if (((data->a_R = lc_atoi(&str)) >= 0) && data->a_R <= 255 && errno == 0)
                if (*(str++) = ',' && ((data->a_G = lc_atoi(&str)) >= 0) && data->a_G <= 255 && errno == 0)
                    if (*(str++) = ',' && ((data->a_B = lc_atoi(&str)) >= 0) && data->a_B <= 255 && errno == 0)
                        return (skip_space(&str));
    }
    return (0);
}

I would rewrite the code as :
#define MAX_COLOR   0xFF
int a_parsing_prime(char* str, t_pars* data)
{
    if (*(str++) == 'A')
    {
        data->a_ratio = lc_atof(&str);
        if (!errno && data->a_R <= MAX_COLOR)
        {
            if (*(str++) = ',')
            {
                data->a_G = lc_atoi(&str);
                if (!errno && data->a_G <= MAX_COLOR)
                {
                    if (*(str++) = ',')
                    {
                        data->a_B = lc_atoi(&str);
                        if (!errno && data->a_B <= MAX_COLOR)
                        {
                            return (skip_space(&str));
                        }
                    }
                }
            }
        }
    }
    return (0);
}

which truely shows the complexity of the function.


Trying to implement cross-class functionality that needs a field to operate in C# Crosspost from here, it was suggested that this is better place to pose my question.

I'm implementing a series of classes in a game that all derive from one base class that references the scripting of the object that it pertains. Most of these are objects that are generated by certain spells/abilities, and very frequently there is a lot of overlap between functionality each script needs.

Unfortunately, the functionality requires the use of some kind of field to use, whether it be a Dictionary or a couple of primitives. Normally I'd just apply an interface to each one, but because each "piece of functionality" requires a field to operate correctly that doesn't seem optimal. And adding every kind of multi-use functionality into the base class seems like it would make each object unnecessarily bloated.

Here's an example:

BlazingAura_Server

...
public BlazingAuraC_Server conditionInstance;
public Queue<KeyValuePair<int, float>> reapplyDelay = new Queue<KeyValuePair<int, float>>();

...
void Update()
{
    KeyValuePair<int, float> id;
    while (reapplyDelay.Count > 0)
    {
        if (reapplyDelay.Peek().Value - BlazingAuraC_Server.BURNING_REAPPLY_DELAY > conditionInstance.duration)
        {
            id = reapplyDelay.Dequeue();
        }
        else
        {
            break;
        }
    }
}

void OnTriggerEnter(Collider collider)
{
    if (collider.tag != "Spell")
    {
        EntityInfoServer collidedInfo = collider.transform.GetComponentInChildren<EntityInfoServer>();
        collidedInfo.QueueDamageRaw(conditionInstance, BlazingAuraC_Server.TICK_FREQ * BlazingAuraC_Server.BASE_DPS, ValueModDamageSubtype.Elemental);

        if (!reapplyDelay.Any(id => id.Key == collidedInfo.entityId))
        {
            reapplyDelay.Enqueue(new KeyValuePair<int, float>(collidedInfo.entityId, conditionInstance.duration));
            collider.transform.GetComponentInChildren<EntityInfoServer>().AddConditionServer(Condition.Burning, conditionInstance);
        }
    }
}

And here's another example:

Storm_Server

...
public float duration;
public Queue<KeyValuePair<int, float>> reapplyDelay = new Queue<KeyValuePair<int, float>>();

...
void Update()
{
    duration += Time.deltaTime;

    if (Mathf.CeilToInt((duration + Time.deltaTime) / TICK_FREQ) > Mathf.CeilToInt(duration / TICK_FREQ))
    {
        Activate(stormCollider);
    }

    KeyValuePair<int, float> id;
    while (reapplyDelay.Count > 0)
    {
        if (reapplyDelay.Peek().Value - STORM_DRENCH_DELAY > duration)
        {
            id = reapplyDelay.Dequeue();
        }
        else
        {
            break;
        }
    }
}

public void OnTriggerEnter(Collider collider)
{
    if (collider.tag == "Player")
    {
        //Debug.Log("IN STORM");
        EntityInfoServer collidedInfo = collider.transform.GetComponentInChildren<EntityInfoServer>();

        if (!reapplyDelay.Any(id => id.Key == collidedInfo.entityId))
        {
            reapplyDelay.Enqueue(new KeyValuePair<int, float>(collidedInfo.entityId, duration));
            collider.transform.GetComponentInChildren<EntityInfoServer>().AddConditionServer(Condition.Drenched, stormInfo);
        }
    }
}

Basically, each class keeps track of entities that have collided with it and store it in a queue, and make sure that a condition doesn't get reapplied unless the entity has been dequeued after a certain duration. However, it requires the Queue to work correctly, but declaring a Queue and utilizing an interface in each class doesn't seem like very good abstraction and feels sloppy in general.

What are my best options for including functionality like this (i.e. having a reapply delay) across multiple classes?

I think the rule you should follow here is "favor composition over inheritance".

What that means in your case is that you create a separate class, something like ReapplyDelay and make both classes own an instance of that class.

The code could look something like this:

class ReapplyDelay
{
    private Queue<KeyValuePair<int, float>> reapplyDelay = new Queue<KeyValuePair<int, float>>();

    public void RemoveOld(float delay, float duration)
    {
        while (reapplyDelay.Count > 0)
        {
            if (reapplyDelay.Peek().Value - delay > duration)
            {
                reapplyDelay.Dequeue();
            }
            else
            {
                break;
            }
        }
    }

    public bool AddNew(EntityInfoServer collidedInfo, float duration)
    {
        if (!reapplyDelay.Any(id => id.Key == collidedInfo.entityId))
        {
            reapplyDelay.Enqueue(new KeyValuePair<int, float>(collidedInfo.entityId, duration));
            return true;
        }

        return false;
    }
}

And your two original classes would then look like this:

BlazingAura_Server:

private BlazingAuraC_Server conditionInstance;
private ReapplyDelay reapplyDelay = new ReapplyDelay();

void Update()
{
    reapplyDelay.RemoveOld(BlazingAuraC_Server.BURNING_REAPPLY_DELAY, conditionInstance.duration);
}

void OnTriggerEnter(Collider collider)
{
    if (collider.tag != "Spell")
    {
        EntityInfoServer collidedInfo = collider.transform.GetComponentInChildren<EntityInfoServer>();
        collidedInfo.QueueDamageRaw(conditionInstance, BlazingAuraC_Server.TICK_FREQ * BlazingAuraC_Server.BASE_DPS, ValueModDamageSubtype.Elemental);

        if (reapplyDelay.AddNew(collidedInfo, conditionInstance.duration))
        {
            collider.transform.GetComponentInChildren<EntityInfoServer>().AddConditionServer(Condition.Burning, conditionInstance);
        }
    }
}

Storm_Server:

private float duration;
private ReapplyDelay reapplyDelay = new ReapplyDelay();

void Update()
{
    duration += Time.deltaTime;

    if (Mathf.CeilToInt((duration + Time.deltaTime) / TICK_FREQ) > Mathf.CeilToInt(duration / TICK_FREQ))
    {
        Activate(stormCollider);
    }

    reapplyDelay.RemoveOld(STORM_DRENCH_DELAY, duration);
}

public void OnTriggerEnter(Collider collider)
{
    if (collider.tag == "Player")
    {
        //Debug.Log("IN STORM");
        EntityInfoServer collidedInfo = collider.transform.GetComponentInChildren<EntityInfoServer>();

        if (reapplyDelay.AddNew(collidedInfo, duration))
        {
            collider.transform.GetComponentInChildren<EntityInfoServer>().AddConditionServer(Condition.Drenched, stormInfo);
        }
    }
}

You know your code better than I do, so the details might be different, but the general idea should be clear: extract common independent pieces of functionality into a separate class and then use that.

I have also changed your fields from public to private: if you really need to expose them, you should use public properties, not fields.


Chart of answered CR posts percentages over time - Part 2 I created a graph to show the change in the percentage of questions answered over time and the first version fetched data from chat transcripts using PHP. The sub-domain I have registered for my webspace doesn't offer HTTPS so the calls to file_get_contents() would fail when run there. Because of this, I started looking at alternatives.

I looked at using other scripting languages and found that github offered github pages and noted that it was secure but didn't allow any server-side scripting. So tried using client-side JavaScript with the fetch API. Initially there were CORS issues and after a little research I found this post, which mentioned a Heroku page that would act as a proxy. I used the proxy to get the data, and I have been interested in the new ES-8 async functions and the await operator so I utilized those.

How does the code look? What would you change about it?

Can you help make the chart results go up by providing a good answer? And remember to vote on any answer that is helpful.

const url = 'https://chat.stackexchange.com/search?q=%22RELOAD!+There+are%22&user=125580&room=8595&pagesize=150';
const pUrl = 'https://cors-escape.herokuapp.com/' + url;
const chartOptions = {
  title: {
    display: true,
    text: 'Answered percentage in recent months'
  },
  scales: {
    xAxes: [{
      display: true,
      scaleLabel: {
        display: true,
        labelString: 'Date'
      }
    }],
    yAxes: [{
      display: true,
      scaleLabel: {
        display: true,
        labelString: 'Percentage'
      }
    }]
  }
};
const dataset = {
  label: 'Answered Percentage',
  backgroundColor: window.chartColors.red,
  borderColor: window.chartColors.red,
  fill: false
};

async function getGraphData() {
  const loadingImage = document.getElementById('loading');
  try {
    const response = await fetch(pUrl);
    const container = document.createElement('div');
    container.innerHTML = await response.text();
    const timestamps = container.getElementsByClassName('timestamp');
    const contents = container.getElementsByClassName('content');

    const labels = [],
      data = [];
    if (contents.length === timestamps.length) {
      let i = 0;
      for (contentElement of contents) {
        const matches = contentElement.innerHTML.match(/\d{2}.\d{4}/);
        if (matches.length) {
          data.unshift(matches[0]);
          labels.unshift(timestamps[i++].innerHTML);
        }
      }
      if (data.length) {
        loadingImage.style.display = 'none';
        drawChart(data, labels);
      }
    }
  } catch (err) {
    loadingImage.parentNode.innerHTML = 'Error loading data';
  }
}

document.addEventListener('DOMContentLoaded', getGraphData)
const drawChart = (data, labels) => {
  Chart.defaults.global.legend.display = false;
  const config = {
    type: 'line',
    data: {
      labels,
      datasets: [Object.assign(dataset, {data})]
    },
    options: chartOptions
  };
  const ctx = document.getElementById('canvas').getContext('2d');
  window.myLine = new Chart(ctx, config);
};
h1 {
  font-family: Arial, "Helvetica Neue", Helvetica, sans-serif;
}

  Graph of Answered Question Percentage on Code Review SE

  <canvas id="canvas"></canvas>

An error and a logic problem.
The code would fail if you ran it as a module or in strict mode due to undefined variable. There is also a problem when parsing the HTML, if there is missing data the timestamps and the percentages will go out of sync because you fail to increase i.
Async functions handle errors by assigning them to the returned promise catch callback. You have wrapped a try catch around code that may throw unrelated development error which you would see as "Error loading data" and not differentiate a legit error from source/logic error.
It is unclear what you intend to happen when fetched data is bad (eg 404 page) asd this will not throw error and you ignore such errors without giving any feedback.
A clearer separation of roles with fetch and parsing as functions rather than the single async function.
Data is also all over the place with the graph settings declared at the top and then more in the function that creates the graph.
General points

You use an undeclared variable contentElement in getGraphData

Why the if (matches.length) { Will that ever happen??? And if it does doesn't that mean that i as index to timeStamps no longer matches the the for loop position?

window is the default object and not needed for direct references. If you are defining globals, clearly define them in the global scope. If they are external use comments to indicate that they external globals.

Why assign to a variable that is never used? window.myLine = new Chart(

Keep it DRY. Axis object could be a function, also several other parts can be reduced using functions.

Reduce number of lines of source code by avoiding single use variable declarations.

Avoid using innerHTML when possible as it is much slower than textContent.

Use direct element id referencing when you are in control of content.

Only wrap the relevant code in try catch. ECMAScript 2018 lets you drop the error object after the catch token. Better yet separate the async part of the function and use promise catch function.

Use the shortest form when possible. Eg you used datasets: [Object.assign(dataset, {data})] that can be datasets: [{...dataset, data}]

Why the if (contents.length === timestamps.length) and no warning if the data is bad?.

Double/same line open [{, ({, or [[... etc should be followed by single indent and matching double close. See rewrite.

Redundant repeated X axis label content " 12:00 AM" can be removed

Graph heading and page heading are different. Maybe assign the graph heading from the page heading??

Possible rewrite
This rewrite reduces the number of lines and encapsulates most of the requirements within a single function reducing the global namespace pollution.
Data vetting was a little unclear so just added that to the graphing function to show a warning if no data found.

"use strict";
// uses chartColors defined in ???.js

document.addEventListener("DOMContentLoaded", displayAnswerRate);

function displayAnswerRate() {
    const CORS_PROXY = "https://cors-escape.herokuapp.com/";
    const url = "https://chat.stackexchange.com/search?q=%22RELOAD!+There+are%22&user=125580&room=8595&pagesize=150";

    const showError = message => {
        errorEl.style.display = "block";
        errorEl.textContent = message;
    }
    getDataURL(url)
        .then(parseDataAndChart)
        .catch(() => {
            loading.style.display = "none";
            showError("Error loading data");
        });

    async function getDataURL(url) { return (await fetch(CORS_PROXY + url)).text() }

    function parseDataAndChart(markup){
        const elsByClass = cName => [...html.querySelectorAll("." + cName)].reverse();
        const html = document.createElement('div');
        html.innerHTML = markup;
        drawChart(
            elsByClass("content").map(el => el.textContent.split("(")[1].split("%")[0]),
            elsByClass("timestamp").map(el => el.textContent.replace(" 12:00 AM",""))
        );
        loading.style.display = "none";
    }

    function drawChart(data, labels) {
        const axis = name => [{display: true, scaleLabel: {display: true, labelString: name}}];
        if (data.length > 0) {
            Chart.defaults.global.legend.display = false;
            new Chart(canvas.getContext('2d'), {
                type: 'line',
                data: {
                    labels,
                    datasets: [{
                        label: 'Answered Percentage',
                        backgroundColor: chartColors.red,
                        borderColor: chartColors.red,
                        data,
                        fill: false
                    }]
                },
                options: {
                    title: {display: true, text: heading.textContent},
                    scales: { xAxes: axis("Date"), yAxes: axis("Percentage") }
                }
            });
        } else {
            showError("No data found???");
        }
    }
}
body {
  font-family: Arial, "Helvetica Neue", Helvetica, sans-serif;
}
h2 {
  text-align : center;
}
#errorEl {
  display : none;
  color: red;
}

Answered percentage in recent months

  <canvas id="canvas"></canvas>


Processing large time-series data I am currently processing some large time-series data with pandas, and I have a function that is intolerably slow, and I'm sure it could be done faster.

The problem is: I'm studying a factory that produces things. It runs continuously throughout the week, but on weekends it shuts down. Before the end of the week, and at the start of a new one, the factory behaves differently, which interferes with the analysis that I'm doing, and so I want to filter out a time window around these weekends.

I have a large dataframe, call it df, whose rows are the articles produced and the columns are their various attributes, one of which is the time at which it was produced, df['timeProduced']. These articles are produced at irregularly-spaced points in time.I want to discard the rows in the table whose timeProduced entry was near one of these shutdown periods. The actual data is confidential, but it looks similar to this:

index   partId        colour   timeProduced  \ ...
1       '026531|352'  Red    2017-02-01 00:00:02
2       '026531|353'  Blue   2017-02-01 00:00:03
3       '026531|354'  Blue   2017-02-01 00:00:05
4       '026531|355'  Green  2017-02-01 00:00:09

This  takes tens of minutes to crunch through a million entries. I know it's slow because it isn't at all vectorized, but I'm not sure how to do a pure vectorized numpy/pandas implementation. Any ideas?

def dropIrregularGaps(series, gapLength, runIn):
    '''
    Designed for time-series data where there is points sampled at irregular time intervals.
    Detects adjacent points that are sampled too far apart, and then removes points on either
    side of the gap which are within a defined runIn period.
    Assumes timeseries data is already sorted. If not, will deliver garbage.
    series is a pandas series object, with values as pandas DateTime objects.
    gapLength is the amount of time that is considered to be a shutdown
    runIn is the length of time to remove on either side of the gap.
    returns a list of indices that are valid
    '''
    samples = list(series)
    indices = list(series.index)

    prev = samples[0]
    ind = indices[0]
    allGoodIndices = []
    currentGoodIndices = [ind]
    currentGoodTimes = [prev]
    skipPoint = None

    for new, ind in zip(samples[1:], indices[1:]):
        if skipPoint:
            if new - skipPoint >= runIn:
                # if a gap has been detected, skip over all points until the current
                # point is past the run-in period.
                skipPoint = None
                currentGoodIndices = [ind]
                currentGoodTimes = [new]

        elif new - prev > gapLength:
            # if a gap is detected. cut out the cooldown period from the list,
            # and add what remains to the list of goodIndices.
            endPoint = currentGoodTimes[-1]
            while currentGoodTimes and (endPoint - currentGoodTimes[-1] < runIn):
                del (currentGoodTimes[-1])
                del (currentGoodIndices[-1])

            allGoodIndices += currentGoodIndices
            currentGoodIndices = []
            currentGoodTimes = []
            skipPoint = new
        else:
            currentGoodIndices += [ind]
            currentGoodTimes += [new]

        prev = new

    allGoodIndices += currentGoodIndices

    return allGoodIndices

I operate this function by taking my dataframe, and running:

result = dropIrregularGaps(df['timeProduced'],pd.Timedelta('4 hours'), pd.Timedelta('8 hours 0 minutes'))

I then use the result to index into the dataframe, giving me the dataframe without the startup/cooldown periods.

df = df.loc[result]

Some general tips

functions

split the work in functions, so you can verify each part individually, each doing a specific job which can be individually tested

PEP-8

Try to follow the guidelines

My algorithm

If your pandas version is >= 0.20, you can use pandas.merge_asof If you have a series with the end and start of the work week

dummy data

np.random.seed(1)
gap_max, run_in = 3, 2
indices = [0, 1, 2, 3, 7, 8, 9, 13, 15, 16, 17, 18]
values = np.random.random(size = len(indices))
data = pd.DataFrame({'time': time, 'values': values})

     time  values
0     0   0.417022004702574
1     1   0.7203244934421581
2     2   0.00011437481734488664
3     3   0.30233257263183977
4     7   0.14675589081711304
5     8   0.0923385947687978
6     9   0.1862602113776709
7     13  0.34556072704304774
8     15  0.39676747423066994
9     16  0.538816734003357
10    17  0.4191945144032948
11    18  0.6852195003967595

so for this data, we expect the values of 1, 2, 3, 7, 8, 9, 13, 15 dropped

Finding the gaps

The gap can be found by using DataFrame.shift.

def find_weekend(times, gap_max):
    gap = times - times.shift(1) > gap_max
    week_start = times[gap]
    weekend_start = times[gap.shift(-1).fillna(False)]
    return weekend_start, week_start
find_weekend(data['time'], gap_max)

 3    3
 6    9
 Name: index, dtype: int64,
 4     7
 7    13
 Name: index, dtype: int64

Marking the start of the data as the beginning of a week can be done by adding gap.iloc[0] = True as 2nd line. Marking the end of the data as also an end of the week can be done by changing to .fillna(True)

merging with the data

Since merge_asof expects DataFrames, we first need to do some transformation

def drop_irregular_gaps(data, gap_max, run_in):
    weekend_start, week_start = find_weekend(data[time_label], gap_max)
    df_week_end = weekend_start.to_frame(name=time_label).assign(run_out=True)
    df_week_start = week_start.to_frame(name=time_label).assign(run_in=True)
    df_data = data[[time_label]]

Then we can use 2 merges, one forward to mark the end of the week, one backward to mark the beginning of the week

    before_weekend = pd.merge_asof(
        df_data, df_week_end,
        on=time_label, direction='forward', tolerance=run_in,
    ).set_index(time_label)['run_out'].fillna(False).values
    after_weekend = pd.merge_asof(
        df_data, df_week_start,
        on=time_label, direction='backward', tolerance=run_in,
    ).set_index(time_label)['run_in'].fillna(False).values

These are 2 array with True as value if they are in a run_in or run_out period

array([False,  True,  True,  True,  True,  True,  True, False, False,
        False, False, False], dtype=bool),
 array([False, False, False, False,  True,  True,  True,  True,  True,
        False, False, False], dtype=bool)

Next we just us an or and not for the boolean indexing

    to_drop = before_weekend | after_weekend
    return data[~to_drop]

drop_irregular_gaps(data, gap_max, run_in)

      time    values
0     0       0.417022004702574
9     16      0.538816734003357
10    17      0.4191945144032948
11    18      0.6852195003967595

This can be easily adapted to 2 separate values for the run_in

Datetime data

The algorithm should be agnostic about whether the time_label data is numeric or datetime. I verified this algorithm also works with this dummy data

data_start = pd.Timestamp('20180101')
time = data_start + pd.to_timedelta([0, 1, 2, 3, 7, 8, 9, 13, 15, 16, 17, 18], unit='day')
gap_max, run_in = pd.to_timedelta(3, unit='day'), pd.to_timedelta(2, unit='day')
values = np.random.random(size = len(indices))
data = pd.DataFrame({'time': time, 'values': values})
drop_irregular_gaps(data, gap_max, run_in)

      time        values
0     2018-01-01  0.417022004702574
9     2018-01-17  0.538816734003357
10    2018-01-18  0.4191945144032948
11    2018-01-19  0.6852195003967595

alternative without merge_asof

Since apparently merge_asof doesn't work as good with duplicate data, here a variant with a loop. If there are a lot of weekends, this might be slower, but I reckon it will still be faster than the original code

def mark_runin(time, week_endpoints, run_in, direction='backward'):
    mask = np.zeros_like(time, dtype=bool)
    for point in week_endpoints:
        interval = (point, point + run_in) if direction == 'forward' else (point - run_in, point)
        mask |= time.between(*interval).values
    return mask
mark_runin(time, weekend_start, run_in)

array([False,  True,  True,  True,  True,  True,  True, False, False, False, False, False], dtype=bool)

def drop_irregular_gaps2(data, gap_max, run_in, time_label = 'time'):
    times = data[time_label]
    weekend_start, week_start = find_weekend(times, gap_max)
    before_weekend = mark_runin(times, weekend_start, run_in, direction = 'backward')
    after_weekend = mark_runin(times, week_start, run_in, direction = 'forward')
    to_drop = before_weekend | after_weekend
    return data[~to_drop]
drop_irregular_gaps2(data, gap_max, run_in)

  time        values
0 2018-01-01  0.417022004702574
9 2018-01-17  0.538816734003357
10    2018-01-18  0.4191945144032948
11    2018-01-19  0.6852195003967595


BrainFuck Interpreter in C++ Generic Headers

#include <vector>
#include <string>
#include <iostream>
#include <fstream>

typedef std::string const           BFProgram;

BF Interpreter

class BFInterpretor
{
    static const std::size_t MemorySize = 10000;

    std::vector<char>           memory;
    std::vector<std::size_t>    openLoops;
    BFProgram&                  program;
    std::istream&               input;
    std::ostream&               output;
    std::size_t                 dataPointer;
    std::size_t                 progPointer;

    std::size_t scanToEndOfLoop()
    {
        std::size_t nestedLoops = 1;
        std::size_t scanPos     = progPointer;
        for(;nestedLoops != 0;++scanPos)
        {
            switch(program[scanPos])
            {
                case '[':   ++nestedLoops;  break;
                case ']':   --nestedLoops;  break;
                default:                    break;
            }
        }
        return scanPos;
    }

    public:
        BFInterpretor(BFProgram& program, std::istream& input = std::cin, std::ostream& output = std::cout)
            : memory(MemorySize)
            , program(program)
            , input(input)
            , output(output)
            , dataPointer(0)
            , progPointer(0)
        {}

        static bool bfComment(char c)
        {
            switch(c)
            {
                case '>': case '<': case '.': case ',': case '+': case '-': case '[': case ']':
                    return false;
                default:
                    break;
             }
             return true;
        }
        void run()
        {
            while(progPointer < program.size())
            {
                ++progPointer;
                switch(program[progPointer - 1])
                {
                    case '>':   dataPointer = (dataPointer + 1) % MemorySize;break;
                    case '<':   dataPointer = (dataPointer - 1) % MemorySize;break;
                    case '+':   ++(memory[dataPointer]);                     break;
                    case '-':   --(memory[dataPointer]);                     break;
                    case '.':   output.put(memory[dataPointer]);             break;
                    case ',':   memory[dataPointer] = input.get();           break;
                    case '[':   if (memory[dataPointer]) {
                                    openLoops.push_back(progPointer);
                                }
                                else {
                                    progPointer = scanToEndOfLoop();
                                }
                                break;
                    case ']':   if (memory[dataPointer]) {
                                    progPointer = openLoops.back();
                                }
                                else {
                                    openLoops.pop_back();
                                }
                                break;
                    default:    break; /* Ignore all comments */
                }
            }
        }
};

Program Loader

BFProgram loadProgram(std::istream& program)
{
    std::string  programText;

    std::string line;
    while(std::getline(program, line))
    {
        line.erase(std::remove_if(std::begin(line), std::end(line), [](char c){return BFInterpretor::bfComment(c);}), std::end(line));
        programText.append(line);
    }
    return programText;
}
BFProgram loadProgram(std::string const& file)
{
    if (file == "-") {
        return loadProgram(std::cin);
    }
    std::ifstream   program(file);
    return loadProgram(program);
}

Main

int main(int argc, char* argv[])
{
    std::vector<std::string>    args(argv+1 , argv+argc);
    if (args.empty()) {
        args.push_back("-");
    }

    for(std::string const& file: args) {
        std::string     program = loadProgram(file);
        BFInterpretor   interpretor(program);
        interpretor.run();
    }
}

An malformed program (with misbalanced brackets) may cause an UB (e.g. popping back from an empty vector; or running out of program space in scanToEndOfLoops).
I recommend implementing step method:

void run() {
    while (progPointer < program.size()) {
        step();
    }
}

with an intention to reuse it in a debugger.


Sort and source configuration files from one of two directories In a zsh I want to loop over all files in a configuration files' directory (/etc/myapp/) to source them. The files should be sourced in order and are named with two leading digits 10-early 40-middle 99-last. A user without write permission to /etc should be allowed to extend the list of files and override the global ones. This is done by creating a $HOME/.myapp directory. The user files should be sourced in order with the global files, i.e. /etc/myapp/10-early $HOME/.myapp/20-rightafter /etc/myapp/40-middle $HOME/.myapp/90-late /etc/myapp/99-last. A user should also be allowed to override global files, i.e. when $HOME/.myapp/99-last exists, then /etc/myapp/99-last should be skipped.

As answerded here and here, I create an array with all the config files and then use basename on the array (like here) to restrict it to the NN-lllll part and sort them. Eventually each file gets sourced from $HOME/.myapp if it exists there, and from /etc/myapp otherwise.

# https://stackoverflow.com/a/10981499
# https://unix.stackexchange.com/a/26825
thefiles=(/etc/myapp/* $HOME/.myapp/*(N))
# https://stackoverflow.com/a/9516801
uniquified=( $( for f in "${thefiles[@]}" ; do basename $f ; done | sort -V | uniq) )
for f in $uniquified
do
# -r checks if file exists and is readable
  if [[ -r $HOME/.myapp/$f ]]
  then
    source $HOME/.myapp/$f
  else
    source /etc/myapp/$f
  fi
done

Is there a more elegant solution and should I be aware of errors I need to catch?

You can combine sort -V | uniq into sort -uV.

Note that the -V flag of sort doesn't exist in BSD.
Consider replacing it with -n if that's good enough.

The if inside the loop can be written more compactly as:

[[ -r $HOME/.myapp/$f ]] && source $HOME/.myapp/$f || source /etc/myapp/$f

You could also replace source with . which is equivalent.


Find and select image files from webpage For some reason, I feel like this is a bit messy and could be cleaner. Any suggestions?

I'm selecting any image files ending in .png or .jpg, and removing any image source files that contains avatar (meaning it's a header image or an avatar).

def self.images(url)
  proxy_addr = 'http://localhost:'
  proxy_port = 8080
  doc = Nokogiri::HTML(open(url), proxy: "#{proxy_addr}#{proxy_port}")
  images = doc.css('img[src$="jpg"], img[src$="png"]').select do |uri|
    uri['src'] =~ %r{^http://(\d+|media)}
  end
  images.map { |uri| uri['src'] }.reject { |uri| uri =~ /avatar/ }
end

There is no point in hard-coding proxy_addr and proxy_port separately, then concatenating them using interpolation.  On the other hand, it would be useful to make the proxy configurable.  One way is to use a default parameter:

def self.images(url, proxy='http://localhost:8080')
  doc = …
end

(Personally, I'd choose to have proxy default to nil, but the formulation above preserves compatibility with your existing code.)

You appear to be using the open function that is provided by OpenURI.  You have a bug: the proxy needs to be specified as an option to open rather than to Nokogiri#HTML.  Furthermore, opening the stream like that makes it impossible to close it properly.  I suggest using a block instead:

def self.images(url, proxy=nil)
  open(url, proxy: proxy) do |io|
    doc = Nokogiri::HTML(io)
    …
  end
end

Your regular expression should be case-insensitive when it looks for http:, as recommended by RFC 3986 Sec 3.1:

  An implementation should accept uppercase letters as equivalent to lowercase in scheme names (e.g., allow "HTTP" as well as "http") for the sake of robustness but should only produce lowercase scheme names for consistency.

Furthermore, hostnames are case-insensitive, as stated in RFC 1035 Sec 2.3.3 (though clarified by RFC 4343 Sec 2):

  For all parts of the DNS that are part of the official protocol, all
  comparisons between character strings (e.g., labels, domain names, etc.)
  are done in a case-insensitive manner.

Also consider allowing URLs that start with https:.

The parameter to your two blocks should not be named uri, but rather img, after the <img> tags that your CSS-style selector requested.

The way you sequence select, map, and reject is awkward.  I suggest

doc.css('img[src$="jpg"], img[src$="png"]').map do |img|
  img['src']
end.select do |uri|
  uri =~ %r{^https?://(\d+|media)}i and uri !~ /avatar/
end


Exploring code objects The first version of my solution was wrong (the usage of compile() function), for details see Stack Overflow.

The solution has been fixed and now it works correctly. Another approach is used: my_add.__code__. The questions stayed the same.

I am reading the book: Obi Ike-Nwosu. Inside The Python Virtual Machine and have reached the "Code Objects" chapter. This chapter has all code object fields printed out:

co_argcount = 1
co_cellvars = ()
co_code = b'|\x00d\x01\x16\x00d\x02k\x02r\x1e|\x00d\x03\x16\x00d\x02k\x02r\x1ed\\
x04S\x00n,|\x00d\x01\x16\x00d\x02k\x02r0d\x05S\x00n\x1a|\x00d\x03\x16\x00d\x02k\x02r\
Bd\x06S\x00n\x08t\x00|\x00\x83\x01S\x00d\x00S\x00'
co_consts = (None, 3, 0, 5, 'FizzBuzz', 'Fizz', 'Buzz')
co_filename = /Users/c4obi/projects/python_source/cpython/fizzbuzz.py
co_firstlineno = 6
co_flags = 67
etc...

but there is no explanation how this can be done programatically, so I wrote my own code:

# The explored function
def my_add(a,b):
    first = a
    second = b

    return first + second

def print_co_obj_fields(code_obj):
    # Iterating through all instance attributes
    # and calling all having the 'co_' prefix
    for name in dir(code_obj):
        if name.startswith('co_'):
            co_field = getattr(code_obj, name)
            print(f'{name:<20} = {co_field}')

# The function code object (__code__) usage
print_co_obj_fields(my_add.__code__)

Output

co_argcount          = 2
co_cellvars          = ()
co_code              = b'|\x00}\x02|\x01}\x03|\x02|\x03\x17\x00S\x00'
co_consts            = (None,)
co_filename          = ./source.py
co_firstlineno       = 17
co_flags             = 67
co_freevars          = ()
co_kwonlyargcount    = 0
co_lnotab            = b'\x00\x01\x04\x01\x04\x02'
co_name              = my_add
co_names             = ()
co_nlocals           = 4
co_stacksize         = 2
co_varnames          = ('a', 'b', 'first', 'second')

Have I done this the optimal way? How would you solve this problem?

main guard

Put the parts of your code that are the ones calling for execution behind a if __name__ == "__main__": guard

if __name__ == "__main__":
    def my_add(a, b):
        first = a
        second = b

        return first + second
    print_co_obj_fields(my_add.__code__)

This way you can import this python module from other places if you ever want to

Documentation

There is a python convention (docstring) to document the behaviour of a method: PEP-257

def print_co_obj_fields(code_obj):
    """prints all the instance variables of the code object that start with 'co_'"""

Indentation

To decrease the levels of indentation, you can reverse the test of startswith, and continue when it fails:

def print_co_obj_fields(code_obj):
    # Iterating through all instance attributes
    # and calling all having the 'co_' prefix
    for name in dir(code_obj):
        if not name.startswith('co_'):
            continue
        co_field = getattr(code_obj, name)
        print(f'{name:<20} = {co_field}')

An alternative if using the builtin filter:

def print_co_obj_fields(code_obj):
    # Iterating through all instance attributes
    # and calling all having the 'co_' prefix
    for name in filter(lambda field: field.startswith("co_"), dir(code_obj)):
        co_field = getattr(code_obj, name)
        print(f'{name:<20} = {co_field}')

Presentation

Separate the generation of the results from the presentation. The easiest way to do this is to have print_co_obj_fields yield the name and co_field instead of printing them

def co_obj_fields(code_obj):
    """generator that yields the fields in a code_object and their value"""
    for name in filter(lambda field: field.startswith("co_"), dir(code_obj)):
        co_field = getattr(code_obj, name)
        yield name, co_field

def print_co_obj_fields(fields):
    for name, co_field in fields:
        print(f"{name:<20} = {co_field}")

The main part then changes to:

if __name__ == "__main__":

    def my_add(a, b):
        first = a
        second = b

        return first + second

    fields = co_obj_fields(my_add.__code__)
    print_co_obj_fields(fields)


Does this intersection algorithm work like it's supposed to? and can I make it more efficient? It's to test the collision between a ray and a sphere.

float BoundingSphere::Intersects(Ray ray)
{
D3DXVECTOR3 direction = ray.Direction();
D3DXVECTOR3 vec = D3DXVECTOR3();
D3DXVECTOR3 cToO = ray.Origin() - centre;
float rSquared = radius * radius;
float vSquared = 0;

if(D3DXVec3Dot(&cToO, &cToO) <= rSquared)
{
    return 0;
}
else if(D3DXVec3Dot(&cToO, &direction) <= 0)
{
    vec = cToO - direction * D3DXVec3Dot(&cToO, &direction);
    vSquared = D3DXVec3Dot(&vec, &vec);

    if(vSquared <= rSquared)
    {
        D3DXVECTOR3 normDir = *D3DXVec3Normalize(&direction, &direction);
        float mul = sqrt(rSquared - vSquared);

        return D3DXVec3Length(
            &(ray.Origin() -
            (centre + vec - D3DXVECTOR3(normDir.x *= mul, normDir.y *= mul, normDir.z *= mul))));
    }
    else
    {
        return -1;
    }
}
else
{
    return -1;
}
}

it should return -1 if there is no collision, 0 if the origin of the ray is inside the sphere. Otherwise it should return a float of the distance from the ray's origin to the first point of collision.

Since there's no answer yet, I'll take a crack at it.

My first impression is that the nesting is somewhat deep and some of the variables could be moved closer to its site of usage. Here's one possible refactor:

float BoundingSphere::Intersects(Ray ray)
{
    D3DXVECTOR3 cToO = ray.Origin() - centre;
    float rSquared = radius * radius;
    if(D3DXVec3Dot(&cToO, &cToO) <= rSquared) return 0;

    D3DXVECTOR3 direction = ray.Direction();
    if(D3DXVec3Dot(&cToO, &direction) > 0) return -1;

    D3DXVECTOR3 vec = cToO - direction * D3DXVec3Dot(&cToO, &direction);
    float vSquared = D3DXVec3Dot(&vec, &vec);
    if(vSquared > rSquared) return -1;
    // ...
}

There should be someway to multiple a D3DXVECTOR3 with a scalar value at once. Even if that's not directly supported it's trivial to overload the operator *= yourself. The return is also getting rather long. I broken that long statement into more digestible pieces:

    float mul = sqrt(rSquared - vSquared);
    D3DXVECTOR3 normDir = *D3DXVec3Normalize(&direction, &direction);
    normDir *= mul;

    return D3DXVec3Length(&(ray.Origin() - centre - vec + normDir));
}

I haven't looked at the collision in too much detail, it seems ok from a glance.

  it should return -1 if there is no collision, 0 if the origin of the ray is inside the sphere...

It sounds like you have a clear idea of how this method is suppose to behave. Why not grab a unit testing framework, like google test, and write a unit test for each of those cases? Testing BoundingSphere::Intersects can be as simple as this:

TEST(BoundingSphereTest, testRayInsideSphere)
{
    BoundingSphere testSphere(D3DXVECTOR3(0, 0, 0), 42);
    Ray testRay(0, 0);

    EXPECT_EQ(-1, testSphere.Intersects(testRay));
}

Note, I'm just guessing on how those objects are constructed but this should give you the main idea. Fix them up as appropriate.


Alerting for rapid disk growth I have a server that is pretty stable (disk capacity) until a certain event occurs and then the drive grows over the course of two days until it is full. I wrote a very simple bash script but I'm not too happy with parts of it. First, I want to be able to see the capacity as a percent down to the tenths place. Also, doing an evaluation I had to strip my value of any decimal places to compare it against my defined threshold. Lastly I feel that I made too much use of cut where I could've been more efficient. I'd love to get some tips for improvement.

#! /bin/bash

monDrive=/opt
prevReading=/root/diskcheckreading.txt
percentThreshold=1
emailAlert=alerts@domain.com

#Check if log file exists

if [ ! -f $prevReading ]
    then
        touch $prevReading
        prev=$(printf "%0.2f\n" $(bc -l <<< $(df /opt | grep opt | cut -d' ' -f24)/$(df /opt | grep opt | cut -d' ' -f23)*100))
        echo $prev > $prevReading

fi

#Get current percentage used of monitored drive
current=$(printf "%0.2f\n" $(bc -l <<< $(df /opt | grep opt | cut -d' ' -f24)/$(df /opt | grep opt | cut -d' ' -f23)*100))

#import previous reading
prev=$(cat $prevReading)

#Calculate the difference
results=$(bc -l <<< $current-$prev )

#Convert to percentage
results100=$(bc <<< $results*100)

#Remove any number after decimal place so that bash can evaluate
rescleanup=$(echo $results100 | cut -d'.' -f1)

#Convert to percentage
percentThreshold100=$(bc -l <<< $percentThreshold*100)

#Check if the difference exceeds the threshold
if  [ $rescleanup -gt $percentThreshold100 ]
     then printf "\"$monDrive\" has grown $results%% in the past hour.\n\n$(hostname):$(pwd)/$(basename $0)" | mail -s "Disk Alert - $(hostname):$monDrive" $emailAlert

fi

#log current reading for next check
emailecho $current > $prevReading

Proceed when prevReading file does not exist?

Your script proceeds to create the file first, and then almost immediately performs the same calculation. In this case, it might be better to just create the file first, exit from the script and then wait for its next invocation to do the comparison.
Performing the same calculation twice

You are repeating the same commands for setting prev and current, so you should put it in a function:

checkDiskUse() {
    df "$monDrive" | awk '/\'"$monDrive"'/{printf "%0.2f",$3/$2*100; exit}'
}

cut -d' ' -f23/24 can be substituted with awk

As illustrated above, awk can 'solve' your weird usage of cut-ting on whitespaces, and then calling bc on them. Your use of cut assumes that the relevant line has your desired values (used / total) exactly 23 whitespaces before them, which may be wrong in the future. Let awk do the whitespace parsing for you instead, and simply extract the third and second values respectively ($3, $2).
Validate file contents

It's good practice to sanity check that the file you are reading from has valid data. A simple way to do so is to check if printf is able to parse it correctly or not:

printf "%0.2f" "$prev" > /dev/null || exit

If the input value $prev is valid, printf returns with a successful exit code. Otherwise, the statement proceeds to the 'or' || part and exit the script as a fail-safe measure.
[ vs [[

As you have discovered, the basic test command [ does not deal with decimal values, but bash's built-in [[ conditional expression happens to handle that well. Compare:

$ [ 0.601 -gt 0.6 ] && echo Y
bash: [: 0.601: integer expression expected
$ [[ 0.601 > 0.6 ]] && echo Y
Y

To sum it up

monDrive=/opt
prevReading=/root/diskcheckreading.txt
percentThreshold=1
emailAlert=alerts@domain.com
checkDiskUse() {
    df "$monDrive" | awk '/\'"$monDrive"'/{printf "%0.2f",$3/$2*100; exit}'
}
[ -f $prevReading ] && prev=$(cat $prevReading) || { checkDiskUse > $prevReading; exit; }
printf "%0.2f" "$prev" > /dev/null || exit
current=$(checkDiskUse)
results=$(bc <<< "$current - $prev")
if [[ $results > $percentThreshold ]]
    then printf "\"$monDrive\" has grown $results%% in the past hour.\n\n$(hostname):$(pwd)/$(basename $0)" | \
        mail -s "Disk Alert - $(hostname):$monDrive" $emailAlert
fi
emailecho $current > $prevReading

Note: $monDrive is assumed to only contain / at the start, since only the first is escaped in the awk statement. Refer to comments for a somewhat hack-y workaround in vim's syntax highlighting (YMMV). Alternatively, I think using END instead of the regex might work too:

df "$monDrive" | awk 'END{printf "%0.2f",$3/$2*100; exit}'


String sequence in ES6 Given an unique string that represents a sequence of characters, the class should implement three methods (getNextChar, getNextString and the genSequence generator).

How it works

We give a sequence of characters. For example: "abc"
We give a starting string. For example: "a"
The generator yields the following strings infinitely.

The next string of an existing string works like a dictionary, only the length doesn't change as long as there are still characters to fulfill.

a
b
c
aa
ab
ac
ba
..
cc
aaa
aab

This is not homework, but a user in Stack Overflow chat presented the challenge and I considered taking it to a higher level.

class SeqString {

    constructor (sequence) {

        this._sequence = sequence;
        this._first    = sequence[0];
        this._last     = sequence.slice(-1);

        return this;
    }

    getNextChar (char) {

        let seq   = this._sequence,
            index = (seq.indexOf(char) + 1) % seq.length;

        return seq[index];
    }

    getNextString (str) {

        let last = str.slice(-1),
            init = str.slice(0, str.length - 1),

            lastOfSeq  = this._last,
            firstOfSeq = this._first;

        // If it is overflowing
        if (last === lastOfSeq) {

            let trail = 1,
                i     = init.length;

            // Look for an index that won't overflow
            while (i-- && init[i] === lastOfSeq) ++trail;

            // If there is no such index, then the result is a new sequence
            // with an increased length
            if (i === -1) return firstOfSeq.repeat(trail + 1);

            // If there is, change the matching character at the index
            // and reset every character after that
            str = init.slice(0, i) + this.getNextChar(init[i]) +
                  firstOfSeq.repeat(trail);

            return str;

        }

        return init + this.getNextChar(last);
    }

    *genSequence (str) {

        let newStr = this.getNextString(str);

        yield newStr;
        yield* this.genSequence(newStr);
    }

}

Specific questions

How can I improve my class functions, while maintaining a balance between performance and readability?
Tips on how I could make the code more readable for others.

Test cases

I have made a function test, to help debugging.

function test (sequence, startStr, times = 10) {

    let it = new SeqString(sequence).genSequence(startStr);

    while (times--)
        console.log(it.next().value);
}

Examples of output

Alphaset tests

const lowerAlpha = "abcdefghijklmnopqrstuvwxyz";
const alpha      = lowerAlpha + lowerAlpha.toUpperCase();

test(alpha, 'abc', 1); // abd
test(alpha, 'BZZ', 1); // Caa
test(alpha, 'Z', 1); // aa
test(alpha, 'ZZ', 1); // aaa

A long sequence for test(alpha, "Stack"): http://lpaste.net/137336

Other tests

test("pen", "p", 10);

  e  n  pp  ..  np

test("0123456789", "5", 10);

  6  7  8  9 00 01  02 ..

It should

Use ES6 features
Expose a generator
Handle every kind of unique sequence
Be readable

Well, to me it looks pretty good. I've made a few minor tweaks, mainly renaming variables, removing unneeded temp variables and renaming the class and methods, to make the code a little more self documenting.

There was one thing that struct me as odd. Your class only works with charSets which contain unique chars. "ppen" will output "p" ten times.

I would definitely add a pruning feature to remove any duplicates, that would make it a more robust class. Something like this first snippet below.

const CharStitcher = (function(){

    class CharStitcher {

        constructor (string) {
            string = CharStitcher.pruneChars(string);
            this._str = string;
            this._1st = string[0];
            this._last = string[ string.length -1 ];
            return this;
        }

        next (char) {
            return this._str[ ( this._str.indexOf(char) + 1 ) % this._str.length ];
        }

        genString (str) {

            let lastChar = str.slice(-1);
            let otherChars = str.slice(0, str.length - 1);

            if (lastChar === this._last) {
                let charAt = 1;
                let upTo = otherChars.length;
                while ( otherChars[upTo] === this._last && --upTo ) ++charAt;
                if ( upTo === -1 ) return this._1st.repeat( charAt + 1 );
                return otherChars.slice( 0, upTo ) + this.next( otherChars[upTo] ) + this._1st.repeat( charAt );
            }

            return otherChars + this.next(lastChar);
        }

        *stitch (str, newStr) {
            newStr = this.genString(str);
            yield newStr, yield* this.stitch(newStr);
        }

    }

    CharStitcher.pruneChars = function (source, pruned){
      pruned = {}
      source.split('').forEach(function(char){
        if(!Object.prototype.hasOwnProperty.call(pruned, char)){
          pruned[char] = undefined;
        }
      })
      return Object.keys(pruned).join('');
    }

    return CharStitcher;

})();

Otherwise, I've removed all the comments, I feel it remains just as readable as your initial version.

class CharStitcher {

    constructor (string) {
        this._str = string;
        this._1st = string[0];
        this._last = string[ string.length -1 ];
        return this;
    }

    next (char) {
        return this._str[ ( this._str.indexOf(char) + 1 ) % this._str.length ];
    }

    genString (str) {

        let lastChar = str.slice(-1);
        let otherChars = str.slice(0, str.length - 1);

        if (lastChar === this._last) {
            let charAt = 1;
            let upTo = otherChars.length;
            while ( otherChars[upTo] === this._last && --upTo ) ++charAt;
            if ( upTo === -1 ) return this._1st.repeat( charAt + 1 );
            return otherChars.slice( 0, upTo ) + this.next( otherChars[upTo] ) + this._1st.repeat( charAt );
        }

        return otherChars + this.next(lastChar);
    }

    *stitch (str, newStr) {
        newStr = this.genString(str);
        yield newStr;
        yield* this.stitch(newStr);
    }

}

I've also rewritten your test a little bit, also just minor tweaks.

function test (charSet, startStr, times = 10) {
    let progress = new CharStitcher(charSet).stitch(startStr);
    while ( times-- ) console.log(progress.next().value);
}

test("pen", "p", 10);

class CharStitcher {

    constructor (string) {
        this._str = string;
        this._1st = string[0];
        this._last = string[ string.length -1 ];
        return this;
    }

    next (char) {
        return this._str[ ( this._str.indexOf(char) + 1 ) % this._str.length ];
    }

    genString (str) {

        let lastChar = str.slice(-1);
        let otherChars = str.slice(0, str.length - 1);

        if (lastChar === this._last) {

            let charAt = 1;
            let upTo = otherChars.length;

            while ( otherChars[upTo] === this._last && upTo-- && ++charAt);

            if ( upTo === -1 ) return this._1st.repeat( charAt + 1 );

            return otherChars.slice( 0, upTo ) + this.next( otherChars[upTo] ) + this._1st.repeat( charAt );

        }

        return otherChars + this.next(lastChar);

    }

    *stitch (str, newStr) {

        newStr = this.genString(str);

        yield newStr;

        yield* this.stitch(newStr);

    }

}

function test (charSet, startStr, times = 10) {
    let progress = new CharStitcher(charSet).stitch(startStr);
    while ( times-- ) console.log(progress.next().value);
}

test("pen", "p", 10);

Again, nothing major, hope my comments are helpful.


Multiply all elements in an array except one The problem is: loop through a list of numbers and for each one of them get the product of all the others and return a new array.

For example in [1,2,3,4,5] the third element should be 40 because 1 × 2 × 4 × 5 = 40

I'm looking for feedback on efficiency and good practices.

function main(numbers) {
    var result = [];
    for (var i in numbers) {
        var n = 1;
        for (var j in numbers) {
            if (i != j) {
                n *= numbers[j];
            }
        }
        result.push(n);
    }
    return result;
}

var a = [2, 6, 9, 31, 55];

console.log(main(a).toString());

What you have shown is an \$O(n^2)\$ solution in that you use nested loops to determine the results.

I would suggest an \$O(2n)\$ approach:

function main(numbers) {
    // one iteration to get product of whole array
    var arrayProduct = numbers.reduce(function(product, value) {
        return product * value;
    }, 1);

    // one iteration to map new array values by dividing
    // full array product by current value
    return numbers.map(function(value) {
        return arrayProduct / value;
    });
}

You could probably use for loops instead of Array functions to micro-optimize this, but if I put myself into the position of someone who might grade such an algorithmic challenge  as this. I would probably be happier to see code like this, as it still meets the requirement to optimize the performance to \$O(2n)\$, while providing code that is more readable.  In other words, I would rather see the reduce() and map() calls in a production code base unless I had a use case where I really needed to squeeze every drop of performance out of this function.

ES6 version:

const main = (numbers) => {
    const arrayProduct = numbers.reduce( (product, value) => product * value, 1);
    return numbers.map( (value) => arrayProduct / value );
}

One could also easily foresee a case where larger arrays could easily hit up against the Number.MAX_SAFE_INTEGER value in javascript (\$2exp53\$), so one might consider throwing an error at some point to abort the operation.

const main = (numbers) => {
    const arrayProduct = numbers.reduce(
        (product, value) => {
            const product = product * value;
            if (product >= Number.MAX_SAFE_INTEGER) {
                throw new RangeError('Product has reached Number.MAX_SAFE_INTEGER');
            }
            return product;
        },
        1
    );
    return numbers.map( (value) => arrayProduct / value );
}


Get file name with extension and get only extension I have again 2 functions but this time they return either the full file name with extension or just the extension with a dot.

Both actions are so similar that I just copy pasted the code from one and changed 1 character... but at the inside I am cringing because it looks terrible to just copy paste. In addition I am not sure if my way of doing it is good and efficient?

-- Return everything until first forward slash starting at the end. http://my.image.com/bla.jpg would return bla.jpg
function GetFileName(url)
  local str = url
  local temp = ""
  local result = ""

  -- Get file name + extension until first forward slash (/) and then break
  for i = str:len(), 1, -1 do
    if str:sub(i,i) ~= "/" then
      temp = temp..str:sub(i,i)
    else
      break
    end
  end

  -- Reverse order of full file name
  for j = temp:len(), 1, -1 do
    result = result..temp:sub(j,j)
  end

  return result
end

-- Get the extension of a string determined by a dot . at the end of the string.
function GetFileExtension(url)
    local str = url
  local temp = ""
  local result = "." -- ! Remove the dot here to ONLY get the extension, eg. jpg without a dot. The dot is added because Download() expects a file type with a dot.

  for i = str:len(), 1, -1 do
    if str:sub(i,i) ~= "." then
      temp = temp..str:sub(i,i)
    else
      break
    end
  end

  -- Reverse order of full file name
  for j = temp:len(), 1, -1 do
    result = result..temp:sub(j,j)
  end

  return result
end

Usage:

print(GetfileName("http://mypath.to.my.image/image.jpg"))
print(GetFileExtension("http://mypath.to.my.image/image.jpg"))

Result:

image.jpg
.jpg

What would be a better approach or is this as good as it goes? (I highly doubt that. :))

You are doing too much work manually. It's so much easier and more powerful to use pattern matching.

The two functions can be written as one-liners:

function GetFileName(url)
  return url:match("^.+/(.+)$")
end

function GetFileExtension(url)
  return url:match("^.+(%..+)$")
end

In the patterns, .+ matches one or more any characters as much as possible (greedy). / matches itself, %. matches the dot .. ^ and $ are the anchors for beginning and ending of a string respectively.


Sum of proper divisors Given a natural number \$n\$ (\$1 \le n \le 500000\$), please output the
  summation of all its proper divisors.

  Definition: A proper divisor of a natural number is the divisor that is strictly less than the number.

  e.g. number 20 has 5 proper divisors: 1, 2, 4, 5, 10, and the divisor
  summation is: 1 + 2 + 4 + 5 + 10 = 22.

  Input

  An integer stating the number of test cases (equal to about 200000),
  and that many lines follow, each containing one integer between 1 and
  500000 inclusive.

  Output

  One integer each line: the divisor summation of the integer given
  respectively.

My solution:

It solved the problem in 1.63 seconds, which seems suboptimal considering its 3-second limit.

import java.util.ArrayList;
import java.util.Scanner;

public class Main
{
    public static void main(String[] args) {
        Scanner reader = new Scanner(System.in);
        int testcases = reader.nextInt();
        reader.nextLine();
        ArrayList<Integer> values = new ArrayList<Integer>();
        for(int i = 0; i<testcases; i++){
            values.add(reader.nextInt());
            reader.nextLine();
        }
        reader.close();
        for(int i = 0; i<testcases; i++){
            int sum = 1;
            int testcase = values.get(i);
            double lim = Math.sqrt(testcase);
            for(int k = 2; k<=lim; k++){
                if(testcase%k==0){
                    sum+=k;
                    int z = testcase/k;
                    if(k != z)
                        sum+=z;
                }
            }
            if(testcase==1)
                sum=0;
            if(i<testcases-1)
                System.out.println(sum);
            else
                System.out.print(sum);
        }
    }
}

EDIT: Updated code, which ran in a more respectable 0.22 seconds.

Taking out reader.nextLine(); improved the runtime by around 0.2 seconds.

Adding a special treatment to perfect squares seemed to have no noticable impact on speed (might depend on test cases?).

It turned out using BufferedReader and BufferedWriter had an immense impact on IO speed, reducing runtime to 0.22 seconds.

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

public class Main
{
    public static void main(String[] args) throws NumberFormatException, IOException {
        BufferedReader in = new BufferedReader(new InputStreamReader(System.in));
        BufferedWriter out = new BufferedWriter(new OutputStreamWriter(System.out));
        int testcases = Integer.parseInt(in.readLine());
        int[] values = new int[testcases];
        for(int i = 0; i<testcases; i++){
            values[i]=Integer.parseInt(in.readLine());
            }
        for(int i = 0; i<testcases; i++){
            int sum = 1;
            int testcase = values[i];
            double lim = Math.sqrt(testcase);
            for(int k = 2; k<lim; k++){
                if(testcase % k == 0){
                    sum += k + testcase / k;
                }
            }
            int intLim = (int) lim;
            if (intLim * intLim == testcase) {
                sum += lim;
            }
            if(testcase == 1)
                sum=0;
            out.write(sum + "\n");
        }
        out.flush();
    }
}

In the innermost for loop,
you have a condition to check in every cycle if k is the square root.
But most of the time it's not equal to the square root.
It would be more optimal to rework that,
and add a special treatment for square numbers once, after the loop:

for (int k = 2; k < lim; k++) {
    if (testcase % k == 0) {
        sum += k + testcase / k;
    }
}
int intLim = (int) lim;
if (intLim * intLim == testcase) {
    sum += lim;
}

You don't need the reader.nextLine() calls in this code,
you can safely remove them.
You can also remove the reader.close(),
no need to close System.in.

int testcases = reader.nextInt();
//reader.nextLine();
List<Integer> values = new ArrayList<>();
for (int i = 0; i < testcases; i++) {
    values.add(reader.nextInt());
    //reader.nextLine();
}
//reader.close();

Is it really important to not print a newline at the end?
If it's not important, then you can rewrite the loop as a for-each loop:

for (int testcase : values) {
    // ...
    System.out.println(sum);
}

Minor things:

It's better to declare variables with interface types when possible, so instead of ArrayList<Integer> values = ..., use List<Integer> values = ...
As of Java 7, use the diamond operator <> when creating parameterized types, for example List<Integer> values = new ArrayList<>()
Always use braces even with single-statement if/else/for
Put spaces around operators and parentheses, for example instead of if(testcase%k==0){ write as if (testcase % k == 0) {


Python 3.6 Dice Histogram: Using random.seed effectively I am not 100% sure that I am using the random.seed correctly for this assignment, or if there is a more efficient way to write the code. Any feedback is appreciated.

Assignment Description:
Simulate rolling 2 die one hundred times. Keep track of the total of the two die. Print a histogram in which the total number of times the dice rolls equals each possible value is displayed by printing a character, such as *, that number of times

import random
random.seed(2)
# declaring an array to count the occurrences
a= [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]

# looping for 100 times and saving the count in array
for i in range(100):
    x = random.randint(1,6)
    y = random.randint(1,6)
    a[x+y] = a[x+y] + 1

# printing histogram
for i in range(2, 13):
    print(str(i).rjust(2)+"s: ","*"*a[i])

Various small comments:

you can write a[x+y] += 1
you always call random.seed with the same input so you'll always have the same output. You could call it with no value so that it uses the time (which is different from one run to another).
instead of having an explicit 13 at the end of your code, you could use len(a)
you could use a constant to store 100 to avoid having a magic number in the middle of your code
you could use a constant to store the number of faces per dice
you could initialise a by using list multiplication. In particular, you could reuse the constant introduced just before.

At this stage, you have:

NB_ITER = 100
DICE_MAX_NUM = 6

random.seed()

# declaring an array to count the occurrences
a= [0] * (2 * DICE_MAX_NUM + 1)

# looping and saving the count in array
for i in range(NB_ITER):
    val = random.randint(1, DICE_MAX_NUM) + random.randint(1, DICE_MAX_NUM)
    a[val] += 1

# printing histogram
for i in range(2, len(a)):
    print(str(i).rjust(2) + "s: ", "*" * a[i])

To be continued but you could use collections.Counter...


Simulate Random Risk Battle For Prof. Yorgey's 2013 Haskell course, I'm working on a homework to simulate a Risk battle.

type Army = Int

data Battlefield = Battlefield { attackers :: Army, defenders :: Army }
                    deriving (Show, Eq)

In addition, Prof. Yorgey implement the following code to simulate the random throwing of dice:

newtype DieValue = DV { unDV :: Int }
  deriving (Eq, Ord, Show, Num)

first :: (a -> b) -> (a, c) -> (b, c)
first f (a, c) = (f a, c)

instance Random DieValue where
  random           = first DV . randomR (1,6)
  randomR (low,hi) = first DV . randomR (max 1 (unDV low), min 6 (unDV hi))

die :: Rand StdGen DieValue
die = getRandom

The second homework question asks to implement the battle function:

  which simulates a single battle (as explained above) between two
  opposing armies. That is, it should simulate randomly rolling the
  appropriate number of dice, interpreting the results, and updating the
  two armies to reflect casualties. You may assume that each player will
  attack or defend with the maximum number of units they are allowed.

And then for my implementation:

battle :: Battlefield -> Rand StdGen Battlefield
battle bf = return $ battleOneRound bf

battleOneRound :: Battlefield -> Battlefield
battleOneRound bf = updateArmy bf (compete a_dice d_dice)
  where
    a_dice = rollDieN . getLegalAttackers $ (attackers bf)
    d_dice = rollDieN . getLegalDefenders $ (defenders bf)

rollDieN :: Army -> [DieValue]
rollDieN n
  | n <= 0     = []
  | otherwise = evalRand die (mkStdGen (1000*n)) : rollDieN (n-1)

type AttackersDice = [DieValue]
type DefendersDice = [DieValue]

-- defender must have at least 2 left at base
getLegalDefenders :: Army -> Army
getLegalDefenders n
  | n >= 4    = 2
  | n == 3    = 1
  | otherwise = 0

-- attackers must have at least 1 left at base
getLegalAttackers :: Army -> Army
getLegalAttackers n
  | n >= 4    = 3
  | n == 3    = 2
  | n == 2    = 1
  | otherwise = 0

Testing

ghci> let bf = battle $ Battlefield 10 20

ghci> (evalRand bf) $ mkStdGen 10
Battlefield {attackers = 8, defenders = 20}

ghci> (evalRand bf) $ mkStdGen 5
Battlefield {attackers = 8, defenders = 20}

ghci> (evalRand bf) $ mkStdGen 444444
Battlefield {attackers = 8, defenders = 20}

As you can see, the actual StdGen argument does not appear to have any impact on the battle results. That's because rollDieN uses mkStdGen (1000*n) to simulate random die throws.

Please critique this implementation. I'm not sure if I've achieved randomness in the battle function.

rollDieN doesn't look very random to me

*Risk> map unDV $ rollDieN 100
[5,5,5,5,5,5,5,5,5,5,5,5,5,5,5,5,5,5,5,5,5,5,5,5,5,5,5,5,
 5,5,5,5,5,5,5,5,5,5,5,5,5,5,5,5,5,5,5,6,6,6,6,6,6,6,6,6,
 6,6,6,6,6,6,6,6,6,6,6,6,6,6,6,6,6,6,6,6,6,6,6,6,6,6,6,6,
 6,6,6,6,6,6,6,6,6,6,6,6,6,6,6,6]

This looks better:

*Risk> map unDV $ evalRand (replicateM 100 die) (mkStdGen 123456)
[4,5,2,3,5,6,2,3,2,5,4,5,5,5,1,1,6,6,2,3,2,5,5,3,4,1,3,1,
 3,2,4,4,3,2,2,6,2,2,3,1,6,6,4,5,3,6,6,6,5,2,2,1,6,3,6,3,
 6,3,6,5,6,5,1,1,1,2,1,5,4,4,3,3,3,3,3,3,4,5,5,2,4,1,5,4,
 4,2,1,1,5,1,4,2,6,5,3,5,2,3,6,1]

Now we can write a function that will return the two army's dice rolls

rollDice n = replicateM n die

battleTest :: Battlefield -> Rand StdGen ([Int], [Int])
battleTest battlefield = do
    attackerRolls <- rollDice $ getLegalAttackers (attackers battlefield)
    defenderRolls <- rollDice $ getLegalDefenders (defenders battlefield)
    return (map unDV attackerRolls, map unDV defenderRolls)

And check that our output looks sane

*Risk> evalRand (battleTest (Battlefield 10 20)) (mkStdGen 12)
([6,3,2],[5,1])
*Risk> evalRand (battleTest (Battlefield 10 20)) (mkStdGen 123456)
([4,5,2],[3,5])

Now assuming a function to update the battlefield based on dice rolls, we just change it to

battle :: Battlefield -> Rand StdGen Battlefield
battle battlefield = do
    attackerRolls <- rollDice $ getLegalAttackers (attackers battlefield)
    defenderRolls <- rollDice $ getLegalDefenders (defenders battlefield)
    return $ nextBattlefield attackerRolls defenderRolls battlefield

There's also a problem with getLegalDefenders. The homework says

  The defending player may defend with up to two units (or only one if
  that is all they have).

So instead of

getLegalDefenders :: Army -> Army
getLegalDefenders n
  | n >= 4    = 2
  | n == 3    = 1
  | otherwise = 0

It should look like

getLegalDefenders :: Army -> Army
getLegalDefenders n
  | n >= 2    = 2
  | n == 1    = 1
  | otherwise = 0

Or

getLegalDefenders n = max 0 (min 2 n)


Optimising iteration and substitution over large dataset I have the following code:

import pandas as pd
import numpy as np
import itertools
from pprint import pprint

# Importing the data
df=pd.read_csv('./GPr.csv', sep=',',header=None)
data=df.values
res = np.array([[i for i in row if i == i] for row in data.tolist()], dtype=object)

# This function will make the subsets of a list
def subsets(m,n):
    z = []
    for i in m:
        z.append(list(itertools.combinations(i, n)))
    return(z)

# Make the subsets of size 2
l=subsets(res,2)
l=[val for sublist in l for val in sublist]
Pairs=list(dict.fromkeys(l))

# Modify the pairs:
mod=[':'.join(x) for x in Pairs]

# Define new lists
t0=res.tolist()
t0=map(tuple,t0)
t1=Pairs
t2=mod

# Make substitions
result = []
for v1, v2 in zip(t1, t2):
    out = []
    for i in t0:
        common = set(v1).intersection(i)
        if set(v1) == common:
            out.append(tuple(list(set(i) - common) + [v2]))
        else:
            out.append(tuple(i))
    result.append(out)

pprint(result, width=200)

# Delete duplicates
d = {tuple(x): x for x in result}
remain= list(d.values())

What it does is as follows: First, we import the csv file we want to work with in here. You can see that it is a list of elements, for each element we find the subsets of size two. We then write a modification to the subsets and call it mod. What it does is to take say ('a','b') and convert it to 'a:b'. We then, for each pair, go through the original data and where ever we find the pairs we substitute them. Finally we delete all the duplicates as it is given.

The code works fine for small set of data. Yet the problem is that the file I have, has 30082 pairs where for each the list of ~49000 list should be scanned and pairs being replaced. I run this in Jupyter and after some time the Kernel dies. I wonder how one can optimise this?

How to resolve the output size issue (many GB in memory and console) is highly specific to your overall setting, i.e. how you want to use the data further, so I restrict my answer to look into possible performance gains.

Performance baseline

I determine the baseline by restricting you original loop logic to the first 20 pairs:

t0=res.tolist()
t0=[tuple(x) for x in t0]
t1=Pairs
t2=mod

def run_loop_original(limit=20):
    # Make substitions
    result = []
    for i_pair, (v1, v2) in enumerate(zip(t1, t2)):
        if i_pair >= limit:
            break
        out = []
        for i in t0:
            common = set(v1).intersection(i)
            if set(v1) == common:
                out.append(tuple(list(set(i) - common) + [v2]))
            else:
                out.append(tuple(i))
        result.append(out)
    return result

    >>> %timeit run_loop_original(20)
    1 loops, best of 3: 1.33 s per loop

Performance Sinks

As far as I can tell, those are:

type conversion of string arrays
repeated string comparisons to identify the pairs within tuples
large nested list construction by double for-loop

Container Type Conversions

Here I simply leave out the tuple conversion in the nested list construction. Your code does not indicate that order of ingredients matters, so the information is equivalent, whether it is a list, tuple, or set.

t0=res.tolist()
t0=[tuple(x) for x in t0]
t1=Pairs
t2=mod

def run_loop_non_tuple(limit=20):
    # Make substitions
    result = []
    for i_pair, (v1, v2) in enumerate(zip(t1, t2)):
        if i_pair >= limit:
            break
        out = []
        for i in t0:
            common = set(v1).intersection(i)
            if set(v1) == common:
                out.append(set(i) - common)
                out[-1].add(v2)
            else:
                out.append(i)
        result.append(out)
    return result

    >>> %timeit run_loop_non_tuple(20)
    1 loops, best of 3: 1.09 s per loop

This is not all, as the conversion of the individual tuples/lists to sets for the pair finding can be done outside the loop:

t0=[set(i) for i in res.tolist()]
t1=Pairs
t2=mod

def run_loop_sets(limit=20):
    # Make substitions
    result = []
    for i_pair, (v1, v2) in enumerate(zip(t1, t2)):
        if i_pair >= limit:
            break
        v1 = set(v1)
        out = []
        for i in t0:
            common = v1.intersection(i)
            if v1 == common:
                out.append(i - common)
                out[-1].add(v2)
            else:
                out.append(i)
        result.append(out)
    return result

    >>> %timeit run_loop_sets(20)
    1 loops, best of 3: 460 ms per loop

Repeated String Comparisons

Two things might help speeding up:

instead of comparing strings individually, an index can be built to find the relevant entries
there is a very limited amount of words overall, so they can be mapped to integers for faster matching

Let's start with a word mapping:

from itertools import chain
all_words = np.array(list(set(chain(*t0))))
word_mapping = {word: i for i, word in enumerate(all_words)}

    >>> len(all_words)
    381

There is no need to convert everything beforehand. Just go directly for creating an index. This can be realized as a numpy 2D-array, where we have a row per  element of t0 and a column per all_words. The values are just True/False values as uint8 (1 Byte per value).

word_index = np.zeros(shape=(len(t0), len(all_words)), dtype=np.uint8)
for i_row, row in enumerate(t0):
    for word in row:
        word_index[i_row, word_mapping[word]] = 1

    >>> word_index.shape, word_index.sum()
    ((48983, 381), 416041)

You can now lookup which rows have a pair of words by using the & operator:

    >>> word_index[:, word_mapping[Pairs[0][0]]] & word_index[:, word_mapping[Pairs[0][1]]]
    array([0, 0, 0, ..., 0, 0, 0], dtype=uint8)

So we rebuild the loop with this advantage:

t0=[set(i) for i in res.tolist()]
t1=[set(x) for x in Pairs]
t2=mod

def run_loop_indexed(limit=20):
    # Make substitions
    result = []
    for i_pair, (v1, v2) in enumerate(zip(t1, t2)):
        if i_pair >= limit:
            break
        i1 = [word_mapping[x] for x in v1]
        ix_contain_pair = word_index[:, i1[0]] & word_index[:, i1[1]]
        out = []
        for row, contains_pair in zip(t0, ix_contain_pair):
            if contains_pair:
                out.append(row - v1)
                out[-1].add(v2)
            else:
                out.append(row)
        result.append(out)
    return result

    >>> %timeit run_loop_indexed(20)
    1 loops, best of 3: 216 ms per loop

Double for Loop

It would still help to avoid the nested list construction via the double loop. What helps there is, that the nested lists are identical to t0 if the pair is missing, and it is missing in the majority of cases. So we first construct a copy of t0 and then replace the rows that contain the pair.

To get the indices in t0, flatten and use np.nonzero:

    >>> np.ravel(np.nonzero(word_index[:, word_mapping[Pairs[0][0]]] & word_index[:, word_mapping[Pairs[0][1]]]))
    array([ 1915,  1987,  8062, 10593, 10614, 10663, 10879, 11235, 12021,
   12096, 13445, 13805, 14630, 17658, 17701, 18865, 20712, 22560,
   23573, 23840, 25379, 25487, 27338, 27690, 28630, 32266, 33259,
   33884, 34309, 34412, 35430, 35463, 35968, 36326, 36977, 39477,
   40292, 42138], dtype=int64)

t0=[set(i) for i in res.tolist()]
t1=[set(x) for x in Pairs]
t2=mod

def run_loop_indexed_2(limit=20):
    # Make substitions
    result = []
    for i_pair, (v1, v2) in enumerate(zip(t1, t2)):
        if i_pair >= limit:
            break
        i1 = [word_mapping[x] for x in v1]
        ix_contain_pair = np.ravel(np.nonzero(word_index[:, i1[0]] & word_index[:, i1[1]]))
        out = t0[:]
        for i in ix_contain_pair:
            out[i] = out[i] - v1
            out[i].add(v2)
        result.append(out)
    return result

    >>> %timeit run_loop_indexed_2(20)
    10 loops, best of 3: 43.1 ms per loop

I still expect you to run into memory limits on the full set and/or to crash your notebook connection trying to print the results.


Setting the value of properties via reflection I am working on an application in which I have to parse an XML file. Nodes of the XML file corresponds to an object. I have to populate the properties of the objects at run time. The code below queries the metadata of property and sets its value. Is there a better way to do it or is there a better way to map the elements of an XML to an object?

public static void SetPropertyValue(object obj, string propertyName, object propertyValue)
    {
        if (obj == null || string.IsNullOrWhiteSpace(propertyName))
        {
            return;
        }

        Type objectType = obj.GetType();

        PropertyInfo propertyDetail = GetPropertyInfo(objectType, propertyName);

        if (propertyDetail != null && propertyDetail.CanWrite)
        {
            Type propertyType = propertyDetail.PropertyType;

            Type dataType = propertyType;

            // Check for nullable types
            if (propertyType.IsGenericType && propertyType.GetGenericTypeDefinition() == typeof(Nullable<>))
            {
                // Check for null or empty string value.
                if (propertyValue == null || string.IsNullOrWhiteSpace(propertyValue.ToString()))
                {
                    propertyDetail.SetValue(obj, null);
                    return;
                }
                else
                {
                    dataType = propertyType.GetGenericArguments()[0];
                }
            }

            if (dataType.Equals(typeof(int)))
            {
                propertyValue = Convert.ToInt32(propertyValue);
            }

            if (dataType.Equals(typeof(long)))
            {
                propertyValue = Convert.ToInt64(propertyValue);
            }

            if (dataType.Equals(typeof(short)))
            {
                propertyValue = Convert.ToInt16(propertyValue);
            }

            if (dataType.Equals(typeof(bool)))
            {
                string stringPropertyValue = propertyValue.ToString();
                if (stringPropertyValue.Equals(Boolean.FalseString, StringComparison.OrdinalIgnoreCase) || stringPropertyValue.Equals("0"))
                {
                    propertyValue = false;
                }
                else
                {
                    if (stringPropertyValue.Equals(Boolean.TrueString, StringComparison.OrdinalIgnoreCase) || stringPropertyValue.Equals("1"))
                    {
                        propertyValue = true;
                    }
                }
            }

            if (dataType.Equals(typeof(float)))
            {
                propertyValue = Convert.ToSingle(propertyValue);
            }

            if (dataType.Equals(typeof(double)))
            {
                propertyValue = Convert.ToDouble(propertyValue);
            }

            if (dataType.Equals(typeof(decimal)))
            {
                propertyValue = Convert.ToDecimal(propertyValue);
            }

            if (dataType.Equals(typeof(byte)))
            {
                propertyValue = Convert.ToByte(propertyValue);
            }

            if (dataType.Equals(typeof(string)))
            {
                propertyValue = Convert.ToString(propertyValue);
            }

            if (dataType.Equals(typeof(char)))
            {
                propertyValue = Convert.ToChar(propertyValue);
            }

            if (dataType.Equals(typeof(DateTime)))
            {
                propertyValue = DateTime.ParseExact(propertyValue.ToString(), "yyyy-MM-dd", CultureInfo.InvariantCulture);
            }

            propertyDetail.SetValue(obj, propertyValue);

        }
    }

You can use reflection to parse some common .Net types:

public static object Parse(Type type, string str)
{
    try
    {
        var parse = type.GetMethod("Parse", new[] {typeof(string)});
        if (parse == null) throw new NotSupportedException();
        return parse.Invoke(null, new object[] { str });
    }
    //or don't catch
    catch (Exception)
    {
        return null;
    }
}

It will basically allow you to replace this code:

        if (dataType.Equals(typeof(float)))
        {
            propertyValue = Convert.ToSingle(propertyValue);
        }

        if (dataType.Equals(typeof(double)))
        {
            propertyValue = Convert.ToDouble(propertyValue);
        }

        if (dataType.Equals(typeof(decimal)))
        {
            propertyValue = Convert.ToDecimal(propertyValue);
        }

        if (dataType.Equals(typeof(byte)))
        {
            propertyValue = Convert.ToByte(propertyValue);
        }

        if (dataType.Equals(typeof(string)))
        {
            propertyValue = Convert.ToString(propertyValue);
        }

        if (dataType.Equals(typeof(char)))
        {
            propertyValue = Convert.ToChar(propertyValue);
        }

        if (dataType.Equals(typeof(int)))
        {
            propertyValue = Convert.ToInt32(propertyValue);
        }

        if (dataType.Equals(typeof(long)))
        {
            propertyValue = Convert.ToInt64(propertyValue);
        }

        if (dataType.Equals(typeof(short)))
        {
            propertyValue = Convert.ToInt16(propertyValue);
        }

with this line:

propertyValue = Parse(dataType, propertyValue.ToString());

It will work for every other class, that has public static void Parse(string) method declared (most standart value types do). You can add Parse(string) method to your own classes, to make them support this kind of deserialization. You can also modify the above code to use Parse(string, IFormatProvider) instead, to avoid parsing issues related to different decimal separators in different cultures.


Scala code to print a Sudoku board and any possible unsolved values I'm working through a Sudoku solver problem that I found at UPenn (no it's not my homework ;)). I'm a beginner at Scala, and I'm trying to write good functional code (not that this particular method really demonstrates that...).

A problem that keeps occurring for me involves trying to use Lists but not using vars, as per good Scala practice. In fact, vars are marked down in the assignment.

In the below method that prints a Scala board plus any unsolved values, I maintain a List of unsolved values. However I have to set this List as a var because I need to reassign it for each iteration of my loop. I don't think that using ImmutableList is a better solution.

What's a better approach here? Should I be re-writing this entire method to be tail recursive? I think that could be difficult and the resulting code might not be as readable. Also am I using too many ifs? Should every if be replaced with pattern matching in Scala?

/**
  * Displays a Sudoku board in a clear readable format. For any unsolved cells, possible answers are printed below
  * alongside their x and y coordinates.
  * @param message a string to display before the board is printed
  * @param board the Sudoku board itself
  */
def printBoard(board: Board, message: String = "Printing a board", displayPossibleValues: Boolean = true) = {
  val rowSeparator = "+ - - - + - - - + - - - +"
  var possibleValues = List[(Int, Int, List[Int])]()
  println(message)
  println(rowSeparator)
  for (i <- 0 until rows) {
    if (i == 3 || i == 6) {
      println(rowSeparator)
    }
    print("| ")
    for (j <- 0 until cols) {
      if (j == 3 || j == 6)
        print("| ")
      val currentCell = board(i)(j)
      if (currentCell.length == 1 && currentCell(0) != 0)
        print(currentCell.mkString + " ")
      else {
        print("x ")
        possibleValues = possibleValues :+ new Tuple3(i + 1, j + 1, currentCell)
      }
    }
    print("|")
    println
  }
  println(rowSeparator)
  if (displayPossibleValues) {
    println("Remaining values:")
    println(possibleValues.mkString("\n"))
  }
  println
}

Sample output from this method:

+ - - - + - - - + - - - +
| 2 8 4 | x 9 3 | 1 6 7 |
| 3 6 9 | 4 7 1 | x x 5 |
| 1 5 7 | 8 2 6 | 4 9 3 |
+ - - - + - - - + - - - +
| 5 7 6 | x 4 x | x 3 1 |
| 8 9 2 | 1 3 5 | 6 7 4 |
| 4 1 3 | x 6 x | 9 x x |
+ - - - + - - - + - - - +
| 7 2 x | x 8 9 | x 4 6 |
| 6 4 x | x 1 2 | x x x |
| 9 3 8 | 6 5 4 | 7 1 2 |
+ - - - + - - - + - - - +
Remaining values:
(1,4,List(3, 5, 9))
(2,7,List(2, 8))
(2,8,List(2, 8))
(4,4,List(2, 9))
(4,6,List(8, 9))
(4,7,List(2, 8))
(6,4,List(2, 7))
(6,6,List(7, 8))
(6,8,List(2, 5, 8))
(6,9,List(2, 8))
(7,3,List(1, 5))
(7,4,List(3, 9))
(7,7,List(3, 5))
(8,3,List(5, 8))
(8,4,List(3, 7))
(8,7,List(3, 5, 8))
(8,8,List(5, 8))
(8,9,List(8, 9))

First some minor points :

You create a tuple with new Tuple3(x, y, z), more ideomatic would be (x, y, z).
You were having troubles with using a var and immutable.List, it is perfectly ok to use mutable.ListBuffer inside your function. You just need to turn it into a List before returning, but since you are just printing out possibleValues I would just use ListBuffer.

Then my major point: I would split up printBoard in multiple functions :

reprBoard : a function which returns a textual representation (a String) of a Board.
possibleValuesOpenCells : a function which returns all the possible values of the empty cells of a Board.

You could then use these two functions in printBoard to print out the Board and the possible numbers for every open cell.

Below you find a possible implementation of reprBoard which is a little more functional (I have simplified Board to a matrix of numbers, but it should be simple to adjust this code to use your actual Board) :

// an open cell = 0
type Board = Vector[Vector[Int]]

def reprBoard(board: Board): String = {
  // I have made it independent of the size of the board
  // so you could pass a 9x9, a 4x4, ... sudoku board
  val rowLength = board.headOption.map(_.size).getOrElse(0)
  val size = Math.sqrt(rowLength.toDouble).toInt
  val rowSeparator =
    intersperse(size, "+ ", Vector.fill(size * size)("- ")).mkString.trim
  val colSeparator = "| "

  // it depends of the implementation of Board if you can map over the rows
  // and columns, but you should be able to do something similar
  val rows = board.map { row =>
    val cells = row.map ( cell =>
      if (cell != 0) s"$cell " else "x "
    )
    intersperse(size, colSeparator, cells).mkString.trim
  }
  intersperse(size, rowSeparator, rows).mkString("\n")
}

// a help function which adds an extra element every `n` elements and at
// the start and the end
// eg. intersperse(1, 0, Vector(1, 2))  ->  Vector(0, 1, 0, 2, 0)
def intersperse[A](n: Int, value: A, vector: Vector[A]) =
  vector.grouped(n).foldLeft(Vector.empty[A])((acc, elem) =>
    acc ++ Vector(value) ++ elem
  ) ++ Vector(value)

Which you could use as :

import scala.util.Random

// generate a random row with two open cells for testing
def randomRow =
  Random.shuffle(Random.shuffle(Vector.range(1, 10)).drop(2) ++ Vector(0, 0))

val board = Vector.fill(9)(randomRow)

reprBoard(board)
// + - - - + - - - + - - - +
// | 6 3 9 | x 8 1 | 5 7 x |
// | x 6 7 | x 2 5 | 4 8 3 |
// | 3 9 8 | x 2 7 | x 6 1 |
// + - - - + - - - + - - - +
// | 9 1 5 | 2 3 7 | 8 x x |
// | x 1 3 | x 7 8 | 2 6 5 |
// | 8 1 4 | 9 5 7 | x 6 x |
// + - - - + - - - + - - - +
// | 3 7 4 | x 6 1 | x 2 9 |
// | 4 6 3 | 7 x 8 | 2 5 x |
// | 7 2 5 | 8 9 4 | x x 6 |
// + - - - + - - - + - - - +

In possibleValuesOpenCells you could :

use your nested for loop if the cells themselves don't have their coordinates
or use a nested map like I did in reprBoard (when you don't need the i and j to create the tuple, you could also look if zipWithIndex can solve this)

printBoard then could look like :

def printBoard(board: Board, message: String = "Printing a board",
               displayPossibleValues: Boolean = true) = {
  println(message)
  println(reprBoard(board))
  println("Remaining values:")
  possibleValuesOpenCells(board).foreach(println)
}

You need to loop the board twice this way, but I think this is a (very) minor price to pay in comparision with the improved clarity.


Is there a better way of defining generic entity classes? I have some entities in use in my project, and to make things easier, I would like to have the type of the key for that entity defined via a generic. E.g.:

public abstract class Entity<T>
{
    public virtual T Id { get; set; }
}

This way, I can pull the Id field into the base entity and not have to define it every time (There's a few other fields on Entity in the real system, so there is other utility to doing this)

Then, since I'm using Fluent NHibernate, I make a generic mapping class to go along with this generic entity:

internal abstract class EntityMapping<T,TK> : ClassMap<T> where T : Entity<TK>
{
    protected EntityMapping()
    {
        Id(m => m.Id);
    }
}

And starting here is when I wonder if there's a better way of handling this. C# doesn't seem to infer the TK type parameter from the Entity definition, and having to add to all of my mapping classes the key parameter for the entity they map seems redundant, all the more so since it also extends to some of the higher  generic data access classes, e.g.:

public abstract class EntityRepository<T,TK> where T : Entity<TK> { }

Is there a tidier way of handling this, or is this what I have to do if I want to have the key type of my entities passed in?

There is no way to simplify generic parameters if you want to keep Entity class generic, as  noted by @svick.

The only thing I can suggest as alternative here is to remove generic parameter from Entity. I don't think you need 10 different types for Id field, most likely you'll have int Id or Guid Id, so you can create

public interface IEntity<T>
{
    T Id { get; set; }
}

public abstract class IntEntity: IEntity<int>
{
    public virtual int Id { get; set; }
}

public abstract class GuidEntity: IEntity<Guid>
{
    public virtual Guid Id { get; set; }
}

Not sure if you need IEntity<T> interface, added it so that you don't loose the possibility to reference both types of entities in generic manner.

Yes, you'll have to declare separate IntEntityRepository and GuidEntityRepository, but I don't think it's that hard given that you can extract common code to base class, and it's a one-time job.

As to mappings - I don't use class hierarchy for them (declaring all the fields on the actual entity mapper), but you can do the same trick as with repositories.


Review: Finished router class for custom mini framework I have finished my router and would like your thoughts on anything that may be inefficient of could be done better!:

class Router {

public $start_page = 'Dashboard' ; // Change to your home/default page.
public $staticRoutes = array(
    'alias' => 'ActualController',
) ;

function __construct()
{
    $url = substr(rtrim($_SERVER['REQUEST_URI'], '/'), 6) ;
    if($url)
    {
        $components = explode('/', $url, 3) ;
        if(array_key_exists($class, $this->staticRoutes))
        {
            $class = $this->staticRoutes[$class] ;
        } else {
            $class = preg_replace('/[^a-z-]/', '', $components[0]) ;
        }

        if(class_exists($class))
        {
            if(isset($components[1])) // has method...
            {
                $method = preg_replace('/[^a-z-]/', '', $components[1]) ;
                if(method_exists($class, $method))
                {
                    if(isset($components[2])) // has params...
                    {
                        $params = explode('/', $components[2]) ;
                        call_user_func_array(array($class, $method), $params) ;
                    } else {
                        $controller = new $class ;
                        $controller->$method() ;
                    }
                } else {
                    $e = 'Method '.$components[1].' does not exist.' ;
                    new Error(array('msg' => $e), 'Method Error') ;
                }
            } else { // No method so just go to default index if exists...
                $controller = new $class ;
                if(method_exists($controller, 'index'))
                {
                    $controller->index() ;
                } else {
                    $e = 'Class '.$class.' has no default index method.' ;
                    new Error(array('msg' => $e), 'Class Index Error') ;
                }
            }
        } else {
            $e = 'Class '.$components[0].' does not exist.' ;
            new Error(array('msg' => $e), 'Class Error') ;
        }
    } else {
        $controller = new $this->start_page ;
        $controller->index() ;
    }
}

}

Note that I have not implements the static routes functionality yet.

Your constructor is doing too much. Just because the only thing you are going to do with your router is initialize it, that doesn't mean you can't have more methods. And as Sven pointed out, it probably shouldn't have access to the global state. Inject whatever you need.

You have two public properties, one uses underscores the other camelCase. I would suggest picking a particular naming scheme and sticking to it. Consistency is important.

Your code seriously violates the arrow anti-pattern. You might consider refactoring to remove some of the indentation and if/else statements. For instance, reversing the first if/else and returning early will allow you to then drop the else, thus removing one level of indentation from your entire method.

if( ! $url ) {
    $controller = new $this->start_page;
    $controller->index();
    return;
}

//etc...

I've never been one for magic numbers and these vague array keys bug me. It shouldn't be necessary to provide comments to understand what each array key does. I would consider padding an array to ensure the size then creating a list. At the same time you can run each component through a loop to clean it. I'm wanting to say you can use array_map() or array_walk() instead of the loop, but I don't feel like trying to figure that out right now. I'm not quite sure what your REGEX is doing so I don't know if there is a better way, but check out filter_var(), that may do what you need.

$components = explode( '/', $url, 3 );
foreach( $components as $key => $component ) {
    $components[ $key ] = preg_replace( '/[^a-z-]/', '', $component );
}
$components = array_pad( $components, 3, FALSE );

list( $class, $method, $params ) = $components;

By the way, where did $class originally come from? The first time I see it is when you check to see if it exists in the $staticRoutes property. If that is a global you should know that globals are evil and you should be ashamed.