Parse a file containing key-value pairs followed by coordinates of nodes in a graph I've a file which is divided into two parts:

The header
The data part

The header has of course a different format than the data part. The header part contains lines that look like this KEY: VALUE or KEY : VALUE or KEY:VALUE, in theory, clearly.

The data part starts with a delimiter, which in my specific case should always be NODE_COORD_SECTION, and ends with another delimiter, which should always be EOF.

In the header, I only need to keep track of two key-value pairs: the DIMENSION and the BEST_KNOWN.

Each line or entry in the data section should be formatted as

  INDEX X_COORDINATE Y_COORDINATE

There could be more than one space between the values. INDEX should be an int, X_COORDINATE and Y_COORDINATE should be floats or doubles.

I'm parsing the header and the data section differently. The first one is with getline with the help of the find method of the string class, and the second with the overloaded operator >> of the ifstream object. I've read on a Stack Overflow's post that mixing these two ways of parsing lines could be dangerous and thus should be avoided, but I thought that for my case it would be the best way to do it.

I want my code first to be correct, but I would like it to also be as efficient and fast as possible, because this parsing of the file, structured as I've been describing above, is part of a program which must be as fast as possible.

Note, I'm throwing exceptions whenever a line is not formatted as I was expecting. I think this a good way of handling these kinds of errors in general, but I don't know if in C++ this is actually a good idea. Notice that I've already read something about exception handling in C++.

I think this is the relevant code for you:

int dimension = -1;
int best_known = -1;
vector <unique_ptr<Node>> nodes;

void read_file(const char *file_name) {

    ifstream file(file_name);

    if (file.fail()) {
        throw runtime_error("file failed to open");
    }

    string line;
    size_t found_at;

    while (getline(file, line)) {

        if (line.find("BEST_KNOWN") != string::npos) {
            found_at = line.find(':');

            if (found_at == string::npos) {
                throw runtime_error("line containing 'BEST_KNOWN' formatted not as expected");
            }

            best_known = stoi(line.substr(found_at + 1));

        } else if (line.find("DIMENSION") != string::npos) {
            found_at = line.find(':');

            if (found_at == string::npos) {
                throw runtime_error("line containing 'DIMENSION' formatted not as expected");
            }

            dimension = stoi(line.substr(found_at + 1));

        } else if (line.find("NODE_COORD_SECTION") != string::npos) {

            if (dimension == -1 || best_known == -1) {
                throw runtime_error("dimension and best known result should have already been read");
            }

            unsigned index;
            double x, y;

            while (file >> index >> x >> y) {
                unique_ptr <Node> p(new Node(index, x, y));
                nodes.push_back(move(p));
            }

            break;
        }
    }

    file.close();
}

Node is a class that represents a node in a graph. I'm using unique_ptrs here to handle memory release automatically, but I don't know if this has a bad impact on the overall performance. Honestly I would be using C if it wasn't for the classes that C++ provides, because I feel that with C++ I will be losing performance.

One problem we all know regarding speed is caching. A std::vector<T> has a nice property, namely that &vec[i] + 1 == &vec[i+1], e.g. the memory is continuous. So whenever you have a loop like

for(size_t i = 0; i < values.size(); ++i) {
    values[i] += 2;
}

the values values[i+1], values[i+2] … will already reside in your cache, and you don't have to access your main memory for those. However if you store only pointers in your vector<T>, ie.

std::vector <std::unique_ptr<Node>> nodes;

you lose that nice property, since we know have to access *vec[i], and &(*vec[i]) != &(*vec[i+1]). The above code becomes

for(size_t i = 0; i < values.size(); ++i) {
    *(values[i]) += 2;
}

and we really want to keep the number of * low. Luckily, unless you want to allow nodes[i] == nullptr for some i, there is no need to use a layer of indirection. Just use

std::vector <Node> nodes;

and create the Node at the end of your vector with std::vector<T>::emplace_back:

        while (file >> index >> x >> y) {
            nodes.emplace_back(index, x, y);
        }

Next, you usually don't want to use the whole namespace std. It's considered bad practice. And last but not least, you want to use variables as local as possible, to get rid of errors that are hard to debug (e.g. found_at should be declared/initialized in your if).

If we follow all of these points, we end up with

int dimension  = -1;
int best_known = -1;
std::vector <Node> nodes;

void read_file(const char *file_name) {

    std::ifstream file(file_name);

    if (file.fail()) {
        throw std::runtime_error("file failed to open");
    }

    std::string line;

    while (std::getline(file, line)) {

        if (line.find("BEST_KNOWN") != std::string::npos) {
            const auto found_at = line.find(':');

            if (found_at == std::string::npos) {
                throw std::runtime_error("line containing 'BEST_KNOWN' formatted not as expected");
            }

            best_known = std::stoi(line.substr(found_at + 1));

        } else if (line.find("DIMENSION") != std::string::npos) {
            const auto found_at = line.find(':');

            if (found_at == std::string::npos) {
                throw std::runtime_error("line containing 'DIMENSION' formatted not as expected");
            }

            dimension = std::stoi(line.substr(found_at + 1));

        } else if (line.find("NODE_COORD_SECTION") != std::string::npos) {
            break;
        }
    }

    if (dimension == -1 || best_known == -1) {
        throw std::runtime_error("dimension and best known result should have already been read");
    }

    unsigned index;
    double x, y;

    while (file >> index >> x >> y) {
        nodes.emplace_back(index, x, y);
    }

    file.close();
}

Note that I've moved the parse of the single node coordinates after the initial getline approach.

Concerning your additional questions: yes, exceptions provide a way to control your flow, but so would error return codes, e.g.

enum READ_ERROR { BEST_KNOWN_FORMAT, DIMENSION_FORMAT, BEST_DIM_NOT_READ, READ_OK };

....

    if (dimension == -1 || best_known == -1) {
        return BEST_DIM_NOT_READ;
    }

The choice is yours, but exception handling and stack unwinding is often a hassle. The compiler can use some optimizations if it knows that a certain part of your code will never throw. However, since we're using nodes.emplace_back, we're already using at least one function that can possibly throw.

Also note that you slightly violate the DRY principle in your line.find("DIMENSION") and line.find("BEST_KNOWN") lines, but another layer of indirection would make things slightly more confusing (for the reader).

Other than that, well done. Your code doesn't fall into other pitfalls that usually happen, e.g.

while(file >> index >> x >> y)

is exactly how you should handle formatted extraction with iostream.


How are my javascript module pattern and promise pattern? I'm working on an app that downloads some SVG paths from a server and draws them.

Anything in here is open to critique, but specifically I'd like to review my use of a module pattern and my implementation and use of promise objects.

The "modules" are each a file, and the aim is to limit and explicitly declare dependencies.  My use of promises is to simplify the use of ajax.

My 3 files are:

init.js - the main application.
promise.js - my implementation of promises
ajax.js - ajax functionality

init.js:

(function (window, document, bigmap, ajax) {
    "use strict";

    var attr = {
        fill: "#fafafa",
        stroke: "#505050",
        "stroke-width": 1,
        "stroke-linejoin": "round"
    };

    var drawings, svg_canvas;

    svg_canvas = bigmap.initialize("map", window.innerWidth - 21, window.innerHeight - 21);

    ajax.post("php/paths.php", null)
        .then(function (value) { drawings = JSON.parse(value); })
        .then(function () { bigmap.draw(svg_canvas, drawings.walls, null); })
        .then(function () { bigmap.draw(svg_canvas, drawings.depts, attr); });

}(this, this.document, parent.bigmap, parent.ajax));

promise.js:

var promise = function () {
    "use strict";

    var pending = [],
        ready = false,
        result;

    var then = function (callback) {
        if (typeof callback === "function") {
            if (ready === false) {
                pending.push(callback);
            } else {
                callback(result);
            }
        }
        return this;
    };

    var keep = function (response) {
        if (ready === false) {
            ready = true;
            pending.forEach(function (value, index, ar) {
                result = response;
                value(result);
            });
            pending = undefined;
        }
    };

    var isReady = function () {
        return ready;
    };

    return {
        keep: keep,
        share: {
            then: then,
            isReady: isReady
        }
    };

};

ajax.js:

var ajax = (function (XMLHttpRequest, promise) {
    "use strict";

    var done = 4, ok = 200;

    function post(url, parameters) {

        var XHR = new XMLHttpRequest();
        var p = promise();

        if (parameters === false || parameters === null || parameters === undefined) {
            parameters = "";
        }

        XHR.open("post", url, true);
        XHR.setRequestHeader("content-type", "application/x-www-form-urlencoded");
        XHR.setRequestHeader('X-Requested-With', 'XMLHttpRequest');
        XHR.onreadystatechange = function () {
            if (XHR.readyState === done && XHR.status === ok) {
                p.keep(XHR.responseText);
            }
        };
        XHR.send(parameters);

        return p.share;
    }

    return {
        post: post
    };

}(parent.XMLHttpRequest, parent.promise));

Thanks.

I assume that you are not going to use readily available third-party libraries for this one. If I had the option to, I'd be using jQuery as it already supports promises, and AJAX.

Anyways, let's start reviewing. I pretty much explain in the comments so watch out for them. Any explanations are usually out of the comments though.

//actually, there is no point doing the localization thing since they
//are objects (and passed by reference). Anything that changes the
//original object, these references you have here also change
//
//However, it's still useful for assigning aliases so I'll let this one pass
(function (window, document, bigmap, ajax) {
    "use strict";

    //personal preference though
    //I prefer chaining the vars with commas
    //declarations on top, those with values last
    //also, you can merge declarations and assignments, like in svg_canvas
    var drawings,
        svg_canvas = bigmap.initialize("map", window.innerWidth - 21, window.innerHeight - 21),
        attr = {
            fill: "#fafafa",
            stroke: "#505050",
            "stroke-width": 1,
            "stroke-linejoin": "round"
        };

    //promises are used for async to maintain a "sync" feeling in the code
    //however, if operations are not async, avoid using a promise
    //if bigmap.draw is not async, then I'd avoid chaining them in .then()'s
    //because it's overhead
    //
    //but I assume they are async since they are SVG drawing operations
    ajax.post("php/paths.php", null)
        .then(function (value) {
            drawings = JSON.parse(value);
        })
        .then(function () {
            bigmap.draw(svg_canvas, drawings.walls, null);
        })
        .then(function () {
            bigmap.draw(svg_canvas, drawings.depts, attr);
        });

}(this, this.document, parent.bigmap, parent.ajax));

On the promises, I'd avoid the module pattern and use prototypes. Imagine this: Every call to promise() returns an object with it's own properties as well as functions. When using prototypes, each promise will have it's own properties, but will share the functions. Forget privates, this one saves memory. However, just like me, if you hate the new Constructor() approach, you can mask the operation in a function call.

//constructor
function PromiseBuilder(){
    this.pending = [];
    this.ready = false;
    this.result;
}

//our shared functions

//this one stores the callback
PromiseBuilder.prototype.then = function (callback) {
    //personal preference, but I use single quotes
    if (typeof callback === 'function') {
        //ready is boolean. instead of checking for false,
        //you can use loose checking and ! which means *if not* ready
        if (!ready) {
            this.pending.push(callback);
        } else {
            //so you won't lose the instance, use call and pass this
            //as "this" in your function
            callback.call(this,result);
        }
    }
    //according to the PromisesA spec [http://wiki.commonjs.org/wiki/Promises/A]
    //each then should return a new promise
    //I haven't read the full spec or try implementing it
    //but you should, if you want it to become a proper PromiseA type promise
    return this;
};

//I guess this one acts as the "resolve" - you should name it that way
PromiseBuilder.prototype.resolve = function () {

    //saving the instance and all arguments
    var instance = this,
        resolveArgs = arguments;

    if (!ready) {

        ready = true;

        //ES5 foreach? or are you using a shim/fix?
        this.pending.forEach(function (value, index, ar) {

            //instead of being restrained in one argument,
            //why not pass the entire arguments array to the callback
            //by using .apply()
            value.apply(instance,resolveArgs);
        });

        this.pending = undefined;
    }
};

PromiseBuilder.prototype.isReady = function () {
    return this.ready;
};

//our masking function
function promise() {
    return new PromiseBuilder();
};

Same as well for AJAX, we use literal namespacing instead. since the ajax response is now bound to the promise there is no need to have instances for our ajax.

var ajax = {
    post : function(url,parameters){
        var XHR = new XMLHttpRequest(),
            p = promise(),
            parameters = params || ''; //parameters is either itself or blank if none

        XHR.open("post", url, true);
        XHR.setRequestHeader("content-type", "application/x-www-form-urlencoded");
        XHR.setRequestHeader('X-Requested-With', 'XMLHttpRequest');
        XHR.onreadystatechange = function () {
            if (XHR.readyState === 4 && XHR.status === 200) {
                promise.keep(XHR.responseText);
            }
        };
        XHR.send(parameters);

        return promise;
    }
}


Choosing a color based on exam score How can I simplify this code?

<?php
class smth {
    protected static $MARK_UNDEFINED = 'Undefined';
    protected static $MARK_F  = 'ECTS-F';
    protected static $MARK_FX = 'ECTS-FX';
    protected static $MARK_E  = 'ECTS-E';
    protected static $MARK_D  = 'ECTS-D';
    protected static $MARK_C  = 'ECTS-C';
    protected static $MARK_B  = 'ECTS-B';
    protected static $MARK_A  = 'ECTS-A';

    protected function getECTSMark($rate, $current, $examRate)
    {
        $color = self::$MARK_UNDEFINED;
        if ($current > 0) {
            $percent = $rate / $current;
            if ($examRate !== NULL AND $examRate < 22)
                $color = self::$MARK_FX;
            elseif ($percent < 0.31)
                $color = self::$MARK_F;
            elseif ($percent < 0.60)
                $color = self::$MARK_FX;
            elseif ($percent < 0.65)
                $color = self::$MARK_E;
            elseif ($percent < 0.71)
                $color = self::$MARK_D;
            elseif ($percent < 0.85)
                $color = self::$MARK_C;
            elseif ($percent < 0.95)
                $color = self::$MARK_B;
            else
                $color = self::$MARK_A;
        }
        return $color;
    }
}

Note: I do not advocate this to be used, it is definitely wrong, untested, horrible and confusing.

But it should give you a start in using the associative array method that Alexander Kludt noted in his comment.

<?php
class Smth
{
    protected $marks = array(
        'Undefined' => 0, 'ECTS-F' => 0.31, 'ECTS-FX' => 0.60, 'ECTS-E' => 0.65,
        'ECTS-D' 0.71, 'ECTS-C' => 0.85, 'ECTS-B' => 0.95, 'ECTS-A'
    );

    public function getEctsMark($rate, $current, $examRate)
    {
        $color = reset($this->marks);
        $percent = $rate / $current;

        if($current > 0 && $examRate < 22) {
            return "ECTS-FX";
        }

        /**
         * This is where the explanation is required. This *should* *maybe*
         * compare the percent to the current start band and the next band. So
         * say for example the percent is 0.78(78%) this would foreach through
         * the  marks array and eventually return true on >= $percentStart(0.71)
         * and <= the next band start up which is 0.85.
         */
        $i = 0;
        foreach($this->marks as $key => $percentStart) {
            if($percent >= $percentStart && $percentStart <= $this->marks[$i+1]) {
                $color = $key;
                break;
            }
            $i++;
        }

        return $color;
    }
}

If you are wanting feedback on your original code, I made a few changes which might satisfy your criteria of simplification over at this 3v4l.


Script to check zero crossings to check if microphones phases are aligned I have 'n' microphones placed in square fashion and I want to make sure that all the channels must be aligned in time exactly the same if the signal is equidistant from all the 'n' microphones i.e. in the center of the square.

I have written this script to do the difference in zero crossing timings for only two signals (.wav file) and if the difference is above some precision then print that and bail out.

from scipy.io import wavfile
import numpy as np
import argparse

parser = argparse.ArgumentParser(description='diff the zero crossing of two files')
parser.add_argument('-f1', '--file_name_1', help='provide first file name')
parser.add_argument('-f2', '--file_name_2', help='provide second file name')
parser.add_argument('-p', '--precision', help='precision to compare against', type=float, default=0.0001)

args = parser.parse_args()
print(args)
files = []
files.append(args.file_name_1)
files.append(args.file_name_2)

sampling_rates = []
signals = []
for file in files:
  fs, signal = wavfile.read(file)
  signal = signal / max(abs(signal))                        # scale signal
  sampling_rates.append(fs)
  signals.append(signal)
  assert min(signal) >= -1 and max(signal) <= 1
  print 'fs           ==> ', fs, 'Hz'                       # sampling rate
  print 'len(signal)  ==> ', len(signal), 'samples'

sampsPerMilli = 0
#files should have same sampling rates and length
prev_rate = sampling_rates[0]
for rate in sampling_rates[1:]:
  cur_rate =  rate
  sampsPerMilli = int(rate / 1000)
  if prev_rate != cur_rate:
    print("rates doesn't match %d %d"% (prev_rate, cur_rate))
    exit(0)
  cur_rate = rate

#signal length also should be same
prev_length = len(signals[0])
for signal in signals[1:]:
  cur_length = len(signal)
  if prev_length != cur_length:
    print("length of signals doesn't match for %d %d"% (prev_length, cur_length))
    exit(0)
  cur_length = prev_length

zccs = []
for signal in signals:
  zcc = []
  DC = np.mean(signal)
  newSignal = signal - DC
  for i in range(1, int(len(newSignal))):
    if((newSignal[i] * newSignal[i-1]) < 0):
      #print("crossing at %f seconds"% ((i/sampsPerMilli) * 0.001))
      zcc.append((i/sampsPerMilli) * 0.001)
  zccs.append(zcc)

for a, b in zip(zccs, zccs[1:]):
  if len(a) != len(b):
    print("length doesn't match %d %d"% (len(a), len(b)))
    for c, d in zip(a, b):
      if c-d > args.precision:
        print("precision %f c %f d %f exceeded"% (args.precision, c, d))
        exit(0)

Is there any better approach or can this script be improved?

This is more of a usability thing than a code thing, but I'd suggest making the non-optional arguments positional so the user doesn't need to type the flags out.
A simple way to put two items into a list is files = [file1, file2].
You can also build sampling_rates and signals via list comprehension.
Since you're explicitly only ever handling two files, building a loop to handle the second thru last files is unnecessary and confusing.
Don't assign a value to a variable if you're going to overwrite it with something else before using it -- again, it's just adding lines of code that do nothing but make it harder to read the script (and use up an infinitesimal amount of processing power).  Combined with (4) above, you have two loops that take seven lines of code to do what's really a single simple equality comparison.
The rest of the script (everything starting at zccs) is hard to understand, but I attempted to condense some of the more magical logic into a function and use list comprehension instead of long loops; better variable names and/or type declarations (normally I'd add those as part of code review suggestions, but I'm not familiar with any of these libraries) would help someone who's not familiar with this particular domain be able to read the script and figure out what it's doing.  Here's what I ended up with after making the above edits:

from scipy.io import wavfile
import numpy as np
from argparse import ArgumentParser

parser = ArgumentParser(description='diff the zero crossing of two files')
parser.add_argument('files', nargs=2, metavar='file',
                    help='one of the files to diff')
parser.add_argument('-p', '--precision', type=float, default=0.0001,
                    help='precision to compare against')

args = parser.parse_args()

# Read data from wav files, scale signals by the max in each file.
sampling_rates, data = zip(*[wavfile.read(file) for file in args.files])
signals = [signal / max(abs(signal)) for signal in data]

# check we didn't somehow bungle the signal normalization math
assert all([min(signal) >= -1 and max(signal) <= 1 for signal in signals])

# files should have same sampling rates and signal length
assert sampling_rates[0] == sampling_rates[1], "rates don't match"
assert len(signals[0]) == len(signals[1]), "signal lengths don't match"

samples_per_ms = int(sampling_rates[1] / 1000)

def zcc(signal):
    """magic!!! (todo: write better docstring)"""
    norm_signal = signal - np.mean(signal)
    return [
      i * 0.001 / samples_per_ms  # is this right?  i is the index!
      if norm_signal[i] * norm_signal[i-1] < 0
      for i in range(1, len(norm_signal))
    ]

for a, b in zip(*[zcc(signal) for signal in signals]):
  if len(a) != len(b):
    print("length doesn't match %d %d"% (len(a), len(b)))
    for c, d in zip(a, b):
      if c - d > args.precision:
        print("precision %f c %f d %f exceeded"% (args.precision, c, d))
        exit(0)


A simple function that removes empty or tags containing just ' ' I've written a function that should get rid of empty p, span, etc tags and those with just ' ' and am looking for ways to improve it. My original solution was very 'wet', but I've managed to come up with a drier solution.

The Original HTML:

text

&nbsp;

text

text

text

text

&nbsp;

text

&nbsp;

text

&nbsp;

text

&nbsp;

text

My Original Solution:

/*
    How to make this drier?
    ORIGINAL UNCLEAN SOLUTION
*/
    var ps = document.getElementsByTagName('p'),
    spans = document.getElementsByTagName('span'),
    strongs = document.getElementsByTagName('strong');

    for (let el of ps) {
        if (el.innerHTML == '&nbsp;') { // can't also include if '' at this stage
            el.parentNode.removeChild(el);
        }
    }
    for (let el of spans) {
        if (el.innerHTML == '&nbsp;' || el.innerHTML == '') {
            el.parentNode.removeChild(el);
        }
    }
    for (let el of strongs) {
        if (el.innerHTML == '&nbsp;' || el.innerHTML == '') {
            el.parentNode.removeChild(el);
        }
    }
    for (let el of ps) {
        if (el.innerHTML == '') {
            el.parentNode.removeChild(el);
        }
    }

My 'drier' solution:

/*
    MY CLEANER SOLUTION
*/
    var ps = document.getElementsByTagName('p'),
    spans = document.getElementsByTagName('span'),
    strongs = document.getElementsByTagName('strong');

    for (let el of ps) {
        cleaner(el);
    }
    for (let el of spans) {
        cleaner(el);
    }
    for (let el of strongs) {
        cleaner(el);
    }

    function cleaner(el) {
        if (el.innerHTML == '&nbsp;' || el.innerHTML == '') {
            el.parentNode.removeChild(el);
        }
    }

Would someone mind quickly running over both solutions and verifying that my 2nd solution is best? Also, I wonder whether that could be improved, or whether anyone has any better ideas for a solution? Thanks for the help here - for brevity, I'm looking at writing concise but also clear code.

There is a bug

You need to run the script several times to remove all empty elements.

Two points

You say remove empty tags that contain "" or a single space "&nbsp;". Does that include " " or "  " two or more spaces. What about other white space characters?
Your element removal is order dependent because you use getElementsByTagName which returns a live list.

Consider the html

  You first check all the p tags which fail the empty test, then you test the span tags which passes and you get

 which is, by your definition, empty and should have been removed.

On the other hand the html

 will first remove the p then remove the span.

The removal process is order dependent. Not what your question indicates.

Changes

For the first point you could use element.textContent to check for empty elements. It will ignore the HTML and convert the &nbsp; to a space for you. You could even use element.textContent.trim() and thus get all blank elements (like the pseudo-class :blank (Which has very limited support FF only))

This also covers the second point.

Example Mark and remove

To reduce the DOM calls you can mark and remove deleting the marked elements only.

const isNotMarked = el => {
    while (el && el.parentNode && !el.parentNode.marked) {
       el = el.parentNode;
       if (el.marked) { return false }
    }
    return true;
}
[...document.querySelectorAll("span, p, strong")]
    .filter(el => el.textContent.trim() === "" && isNotMarked(el) ? el.marked = true : false)
    .forEach(el => el.parentNode.removeChild(el));

Example simple brute force

Mark and remove saves you attempting to delete already deleted elements but you may not care, as the shorter form, is a two liner, and thus could be argued to be the better solution.

document.querySelectorAll("span, p, strong")
    .forEach(el => el.textContent.trim() === "" && el.parentNode.removeChild(el))

The following snippet shows the HTML after using your function and then the two example functions

   /*=================================================================================
OP ver modified for example
 =================================================================================*/

var ps = cleaned.getElementsByTagName('p'),
    spans = cleaned.getElementsByTagName('span'),
    strongs = cleaned.getElementsByTagName('strong');

    for (let el of ps) { cleaner(el); }
    for (let el of spans) {  cleaner(el); }
    for (let el of strongs) { cleaner(el);  }

    function cleaner(el) {
        if (el.innerHTML == '&nbsp;' || el.innerHTML == '') {
            el.parentNode.removeChild(el);
        }
    }
    content.textContent = cleaned.innerHTML;

/*=================================================================================
 Mark and remove
 =================================================================================*/
const isNotMarked = el => {
    while (el && el.parentNode && !el.parentNode.marked) {
       el = el.parentNode;
       if (el.marked) { return false }
    }
    return true;
}
[...cleanerClean.querySelectorAll("span, p, strong")]
    .filter(el =>  el.textContent.trim() === "" && isNotMarked(el) ? el.marked = true : false)
    .forEach(el => el.parentNode.removeChild(el));

contentA.textContent = cleanerClean.innerHTML;

/*=================================================================================
 Brute force remove
 =================================================================================*/
simplerClean.querySelectorAll("span, p, strong")
    .forEach(el => el.textContent.trim() === "" && el.parentNode.removeChild(el))

contentB.textContent = simplerClean.innerHTML;
#content {
   display: block;
}

text

&nbsp;

text

text

text

text

&nbsp;

text

&nbsp;

text

&nbsp;

text

&nbsp;

text

<fieldset>
<legend>Original OPs script & Resulting HTML</legend>

</fieldset>

text

&nbsp;

text

text

text

text

&nbsp;

text

&nbsp;

text

&nbsp;

text

&nbsp;

text

<fieldset>
<legend>Mark and remove</legend>

</fieldset>

text

&nbsp;

text

text

text

text

&nbsp;

text

&nbsp;

text

&nbsp;

text

&nbsp;

text

<fieldset>
<legend>Brute force remove</legend>

</fieldset>


Logging without Code Bloat I was wondering if any of you know any best practices or design patterns so that I can have good logging in my programs without them looking messy or bloated.

I am currently using C# and NLog, however I guess any advice here would be language and tool agnostic.

The problem we have is that we want to have good logging, however we can't seem to find a way around having many lines which just log operations, which can turn a simple method like this:

void Foo()
{
    Bar bar = dbContext().Bars.First();
    bool someCondition = bar.DoSomething();
    if (someContition)
    {
        dbContext().FooBars.Add(new FooBar());
    }
}

Into something which looks like this:

void Foo()
{
    _logger.Info("Getting first bar from database...");

    Bar bar = dbContext().Bars.First();

    _logger.Info("First bar returned from database, id = {0}", bar.Id);

    _logger.Info("Doing something on bar with id = {0}", bar.Id);
    bool someCondition = bar.DoSomething();

    _logger.Info("Something done on bar with id = {0}, response = {1}", bar.Id, someCondition);

    if (someContition)
    {
        _logger.Warn("Adding new FooBar to database");
        dbContext().FooBars.Add(new FooBar());
        _logger.Warn("Added new FooBar to database successfully");
    }
}

Here we now have more logging lines than code lines.

However I am being told by operations that this level of logging is required, and at the moment is still too coarse.

Is there any way around this ugly manual logging, or am I stuck with it?

This kind of trace logging could conceivably be solved with an Aspect.

Aspect oriented programming allows you to add boilerplate-style code to your program without actually writing the boilerplate inline. You could, for example, add an aspect than logged a method name and parameters every time a program entered a method, and then logged the method name and return value every time a method returned.

Of course, you don't necessarily want to log every method. Instead, most aspect frameworks allow you to indicate which code you want the boilerplate added to.

I don't know enough about C# to recommend a specific aspect-oriented framework to use, or appropriate syntax for you. Sorry.


How to Improve This Program, Add Comments, etc.? Do you have any suggestions to improve or extend this script? Any ideas for removing or adding comments/docstrings?

import dbm
import subprocess
import time
subprocess.call("clear")
try:
    # Tries to import cPickle, a faster implementation of pickle.
    import cPickle as pickle
except ImportError:
    import pickle

class PickleDB:
    def open_db(self):
        """Gets and opens the database"""
        self.db_name = input("Enter a database name\n")
        # Makes sure the database name is compatible
        db_parts = self.db_name.split(".")
        self.db_name = db_parts[0]
        self.db = dbm.open(self.db_name, "n")

    def get_data(self):
        """Gets data to write into the database"""
        subprocess.call("clear")
        self.raw_data = input("Enter your raw data\n")
        # Pickles data
        self.pickled_data = pickle.dumps(self.raw_data)

    def write_to_db(self):
        """Writes data into database"""
        # Logs time
        self.start = time.time()
        # Creates keys
        self.db["raw"] = self.raw_data
        self.db["pickled"] = self.pickled_data

    def close_db(self):
        """Closes database"""
        self.db.close()
        # Logs end time
        self.end = time.time()

    def info(self):
        """Prints out info about the database"""
        subprocess.call("clear")
        print("Load data from database using dbm")
        print("Keys: raw\tpickled")
        # Prints total time
        print("Speed:", self.end - self.start, "seconds")

# Function calls
pickled_db = PickleDB()
pickled_db.open_db()
pickled_db.get_data()
pickled_db.write_to_db()
pickled_db.close_db()
pickled_db.info()

import dbm
import subprocess
import time
subprocess.call("clear")

This isn't cross-platform. Sadly, there isn't really a cross-platform alternative

try:
    # Tries to import cPickle, a faster implementation of pickle.
    import cPickle as pickle
except ImportError:
    import pickle

class PickleDB:
    def open_db(self):
        """Gets and opens the database"""
        self.db_name = input("Enter a database name\n")

You really shouldn't call input inside of a work class. The code responsible for User I/O should be separate from that doing any actual work.

        # Makes sure the database name is compatible
        db_parts = self.db_name.split(".")
        self.db_name = db_parts[0]

Ok, why are you ignoring part of the name if its not good? Shouldn't you complain to the user and ask for a new name? Also, I'd avoid assigning to self.db_name only the replace it again a bit later. Since you don't use it outside this function, I'd probably just have a local variable

        self.db = dbm.open(self.db_name, "n")

    def get_data(self):
        """Gets data to write into the database"""
        subprocess.call("clear")
        self.raw_data = input("Enter your raw data\n")
        # Pickles data

This comment is a little obvious, I'd delete it

        self.pickled_data = pickle.dumps(self.raw_data)

I'd expect a function that gets data to return it to the caller. Generally, functions that drop state onto the class is a sign of poor design. Methods should usually communicate by parameters and return values not object storage.

    def write_to_db(self):
        """Writes data into database"""
        # Logs time
        self.start = time.time()

Collecting timing information isn't something that's usually part of the actual class doing the work. Usually you'd time the class from the outside.

        # Creates keys
        self.db["raw"] = self.raw_data
        self.db["pickled"] = self.pickled_data

    def close_db(self):
        """Closes database"""
        self.db.close()
        # Logs end time
        self.end = time.time()

    def info(self):
        """Prints out info about the database"""
        subprocess.call("clear")
        print("Load data from database using dbm")
        print("Keys: raw\tpickled")
        # Prints total time
        print("Speed:", self.end - self.start, "seconds")

As noted, output and timing is usually something you want to do outside of the work class itself.

# Function calls
pickled_db = PickleDB()
pickled_db.open_db()
pickled_db.get_data()
pickled_db.write_to_db()
pickled_db.close_db()
pickled_db.info()

Here's the overall thing about your code. You are using a class as a group of functions and treating the object storage like a place to put global variables. You aren't making effective use of the class. In this case, you'd be better off to use a collection of functions than a class.


Function to find a certificate in either the user or the machine store I wanted a function that would return a certificate, given its serial number, regardless of which store it is in. It has to do almost the same exact thing but twice (once for each store). To keep it encapsulated I wrote the "same thing" bit as a lambda, sort of like a Javascript programmer might.

public X509Certificate FindClientCertificate(string serialNumber)
{
    Func<StoreLocation, X509Certificate> find = (location) =>
    {
        X509Store store = new X509Store(location);
        store.Open(OpenFlags.OpenExistingOnly);
        IEnumerable certs = store.Certificates.Find(X509FindType.FindBySerialNumber, serialNumber, true);
        return certs.OfType<X509Certificate>().FirstOrDefault();
    };
    return find(StoreLocation.CurrentUser) ?? find(StoreLocation.LocalMachine);
}

Questions:

Is the purpose and implementation of this function easier to read with the lambda expression? Or is this utterly confusing?
Is there any performance loss or other sort of hidden cost of using a lambda expression as a nested function?

I'm looking forward to C#7 local functions :)

I agree with @JanDotNet that by using a separate method you will gain readability but that's just my subective view.

As you are asking about C#7 stuff I assume that you will use .NET >= 4.6. Starting with .NET 4.6 the X509Store implements IDisposable hence you should enclose the using of the X509Store inside a using block.

Instead of using a concrete type you could use the var type  to declare your variables.

This would result in

public X509Certificate FindClientCertificate(string serialNumber)
{
    return Find(serialNumber, StoreLocation.CurrentUser) ?? Find(serialNumber, StoreLocation.LocalMachine);
}
private static X509Certificate Find(string serialNumber, StoreLocation location)
{
    using (var store = new X509Store(location))
    {
        store.Open(OpenFlags.OpenExistingOnly);
        var certs = store.Certificates.Find(X509FindType.FindBySerialNumber, serialNumber, true);
        return certs.OfType<X509Certificate>().FirstOrDefault();
    }
}


"ACM ICPC Team" challenge I'm attempting this question on HackerRank, which states the follows:

  You are given a list of \$N\$ people who are attending ACM-ICPC World
  Finals. Each of them are either well versed in a topic or they are
  not. Find out the maximum number of topics a 2-person team can know.
  And also find out how many teams can know that maximum number of
  topics.

  Note Suppose \$a\$, \$b\$, and \$c\$ are three different people, then \$(a,b)\$ and
  \$(b,c)\$ are counted as two different teams.

  Input Format

  The first line contains two integers, \$N\$ and \$M\$, separated by a single
  space, where \$N\$ represents the number of people, and M represents the
  number of topics. \$N\$ lines follow. Each line contains a binary string
  of length \$M\$. If the ith line's jth character is \$1\$, then the ith person
  knows the jth topic; otherwise, he doesn't know the topic.

In my code below, I replaced \$N\$ with ppl and \$M\$ with topics.

ppl, topics = map(int, input().split())
knowledge = []

max_group = 0
max_topic = 0

for x in range(ppl):
    inp = input()
    x = [int(i) for i in inp]
    knowledge.append(x)

for x in range(ppl):
    for y in range(x+1, ppl):
        current_topic = 0

        for z in range(topics):
            if knowledge[x][z] | knowledge[y][z] == 1:
                current_topic += 1

        if current_topic == max_topic:
            max_group += 1
        elif current_topic > max_topic:
            max_topic = current_topic
            max_group = 1

print(max_topic)
print(max_group)

The solution above should return the correct outputs (passed three test cases), but there are simply way too many loops (5, if I am correct). What are some ways that I can reduce the amount of loops and bring the run-time down?

I don't see a better than \$(O(N^2M))\$ (that is, brute force) solution; however an asymptotic constant could be improved significantly.

First, I recommend to represent an individual knowledge as an integer rather than a list. This way computing a team knowledge is a matter of a single bitwise or, and a number of topics covered is the amount of bits set.

To obtain an integer representation of knowledge, recall that int() takes base as a second argument:

    for _ in range(ppl):
        knowledge.append(int(input(), 2))

Then analyze a combined team of x, y knowledge as

    team_topics = bit_count(knowledge[x] | knowledge[y])

There are many ways to implement bit_count. Here is a good place to start.

Observations not related to performance:

Naming is not particularly good. I expect current_topic to be a loop index running through topics, not the count of topics covered by team. Similarly, max_topic is named as if it represents the maximal index in topics table.
Dummy loop variable (x in the input loop) is by convention denoted by _. Seeing _ the reader knows it is dummy, and doesn't look for how it might be used in the code.


Simpler way of showing, hiding & disabling elements I'm currently having this code & looking for a simpler and shorter way of showing, hiding & disabling my elements...

$("#chReportData").click(function(){
            if($(this)[0].checked){
                $("#reportDataOptions").show();
            } else {
                $("#ReportDataStatusOptions").hide();
                $("#reportDataOptions").hide();
                $('#chkReportPermission').attr('checked', false);
                $('#chReportDataStatus').attr('checked', false);
                $('#chReportDataCummulative').attr('checked', false);
                $('.allowedUpload').attr('checked', false);
                $('.allowedDelete').attr('checked', false);
            }
        });

        $("#chReportDataStatus").click(function() {
            if ($(this)[0].checked) {
                $("#ReportDataStatusOptions").show();
            } else if ($('#chReportDataCummulative').is('checked')) {
                $("#ReportDataStatusOptions").hide();
                $('.allowedUpload').attr('checked', false);
                $('.allowedDelete').attr('checked', false);
            } else {
                $("#ReportDataStatusOptions").hide();
                $('.allowedUpload').attr('checked', false);
                $('.allowedDelete').attr('checked', false);
            }
        });

It works fine, I'm just looking for a simpler way... If you know of a shorter & simpler way, please share...

Added HTML...

Report Data:<asp:CheckBox ID="chReportData" runat="server" CssClass="floatL shad1 trans1" ClientIDMode="Static"/>

                    Report Management:<asp:CheckBox ID="chkReportPermission" runat="server" CssClass="floatL shad1 trans1" ClientIDMode="Static"/>

                    <asp:RadioButton ID="chReportDataStatus" runat="server" ClientIDMode="Static" CssClass="floatL shad1 trans1" GroupName="reportData" Text="Report Type: Status" TextAlign="Left"/>

                    <asp:RadioButton ID="chReportDataCummulative" runat="server" ClientIDMode="Static" CssClass="floatL shad1 trans1" GroupName="reportData" Text="Report Type: Cummulative" TextAlign="Left"/>

                    Upload Files <asp:CheckBox ID="chkAllowedUpload" runat="server" CssClass="floatL shad1 trans1" class="allowUpload"/>

                    Delete Files <asp:CheckBox ID="chkAllowedDelete" runat="server" CssClass="floatL shad1 trans1" class ="allowDelete"/>

Edit: now that you have shared your html I could make a better response ...     http://jsfiddle.net/EjBW7/2/

There is a lot of repeated code, you could write some functions to avoid repeating the code, the branches in the second block seem redundant. You could use jquery's ability to target multiple elements in one selector.

setCheckboxValue = function (selector, enable)
{
    return $(selector).attr('checked', enable);
}

$("#chReportData").click(function(){

 if($("#chReportData").is(':checked') ){
      console.log('#chReportData show');
     $("#reportDataOptions").show();
 } else {
     console.log('#chReportData hide');

     $("#ReportDataStatusOptions, #reportDataOptions").hide();
     setCheckboxValue('#chkReportPermission, #chReportDataStatus, #chReportDataCummulative, .allowedUpload, .allowedDelete', false);
 }
});

$("#chReportDataStatus,#chReportDataCummulative").click(function() {
  if ($("#chReportDataStatus").is(':checked') ) {
       console.log('#ReportDataStatusOptions show');
    $("#ReportDataStatusOptions").show();
 } else {
     console.log('#ReportDataStatusOptions hide');
    $("#ReportDataStatusOptions").hide();
    setCheckboxValue('.allowedUpload, .allowedDelete', false);
 }
});


Reverse an array without affecting special characters Given a string, that contains special character together with alphabets (‘a’ to ‘z’ and ‘A’ to ‘Z’), reverse the string in a way that special characters are not affected.

Example:
Input:   str = "a:b!c"
Output:  str = "c:b!a"

I am using two indexes one to start from the beginning and the other from the end of the array.Swap if they are valid chars  and then increment indexes,increment indexes if invalid chars.Any feedback appreciated!

P.S: I am using static methods for convenience.

The Code:

public class ReverseArrayWithoutSpecialChars {

    static HashSet<Character> invalidChars = new HashSet<Character>(Arrays.asList(',', '?', ':','!','$'));

    public static char[] reverseSkippingSpecialChars(char[] charArray) {

        int l = 0;
        int r = charArray.length - 1;
        for (int i = 0; i < charArray.length; i++) {

            if (r <= l) {
                break;
            }
            if (!isInvalidChar(charArray[l]) && !isInvalidChar(charArray[r])) {
                charArray = swapChars(charArray, l, r);
                l++;
                r--;
            } else if (isInvalidChar(charArray[l])) {
                l++;
            } else if (isInvalidChar(charArray[r])) {
                r--;
            }
        }
        return charArray;
    }

    public static char[] swapChars(char[] charArray, int index1, int index2) {
        char temp = charArray[index1];
        charArray[index1] = charArray[index2];
        charArray[index2] = temp;
        return charArray;
    }

    public static boolean isInvalidChar(char character) {
        return invalidChars.contains(character);
    }

    public static void main(String args[]) {
        char[] charArray = new char[]{'a', 'b', 'c', 'd', '?', 'f', 's', ':', 'w'};

        System.out.println(reverseSkippingSpecialChars(charArray));
    }
}

Class fields

    static HashSet<Character> invalidChars = new HashSet<Character>(Arrays.asList(',', '?', ':','!','$'));

You might as well go ahead and make this final as well.

    public static final Set<Character> invalidChars = new HashSet<>(Arrays.asList(',', '?', ':', '!', '$'));

As a final variable, it's clear that it won't change.

We should probably specify a visibility.  Unless it's a secret, we should go ahead and make it public.  Since it's final, no one can change it.

It is better to type to the interface than the implementation.  This allows us to change the implementation without changing the type.  And it forces us to limit ourselves to the operations available in the interface.

In newer versions of Java, you don't need to say Character twice.  The compiler will figure out the type on the second one.

Eliminate unnecessary variables

        int l = 0;
        int r = charArray.length - 1;
        for (int i = 0; i < charArray.length; i++) {

            if (r <= l) {
                break;
            }

You can write this either

        for (int l = 0, r = charArray.length - 1; l < r; ) {

or

        int l = 0;
        int r = charArray.length - 1;
        while (l < r) {

You don't need the extra i variable.  Nor do you need to do the l < r check separately.

It's up to you if you use a for or a while.  Either will work.  The for is a bit more compact, but you have to leave the update section blank.  Either will be shorter.

Not really invalid

            if (!isInvalidChar(charArray[l]) && !isInvalidChar(charArray[r])) {
                charArray = swapChars(charArray, l, r);
                l++;
                r--;
            } else if (isInvalidChar(charArray[l])) {
                l++;
            } else if (isInvalidChar(charArray[r])) {
                r--;
            }

It's not that the characters are invalid.  They are expected and you want to preserve them.  It's that the character are special and treated different.  Consider

            if (isSpecialChar(charArray[l])) {
                l++;
            } else if (isSpecialChar(charArray[r])) {
                r--;
            } else {
                // neither is a special character
                swapChars(charArray, l, r);
                l++;
                r--;
            }

You use the terminology special elsewhere, so why not here?

I also like that this emphasizes the positive.

Since we already know that neither character is a special character, we don't have to check again.

The swapChars method modifies the charArray in place.  We don't have to return it and assign.

Generating a character array

        char[] charArray = new char[]{'a', 'b', 'c', 'd', '?', 'f', 's', ':', 'w'};

If you want to type less, consider

        char[] charArray = {'a', 'b', 'c', 'd', '?', 'f', 's', ':', 'w'};

or

        char[] charArray = "abcd?fs:w".toCharArray();

No big deal, but why type more than necessary?


Clean up PHP & MYSQL code (Picked up by Swift as blog reader iOS app) New to PHP and was just wondering if any improvements can be made, maybe increase efficiency or just clean up code?

I know I'm using an old MYSQL maybe we can update it. Just want to make sure the connection can be as best to its ability. Also, if you can, can you explain on how to improve security?

The JSON gets picked up by Swift iOS app.

<?php

$dbhost = "website";
$dbuser = "user";
$dbpass = "password";
$dbname = "database";

$connection = mysql_connect($dbhost,$dbuser,$dbpass);

// Checking if connection can be established
if(!$connection){

    die("Connection Failed");

} else {

// Selecting Database
$dbconnect = mysql_select_db($dbname, $connection);

// Checking if it can connect to Database
if(!$dbconnect){

    die("Unable to connect to Database");

} else {

    $query = "SELECT * FROM blogs";
    $resultset = mysql_query($query, $connection);
    $records= array();

    // Loop and check all records and add to array
    while($r = mysql_fetch_assoc($resultset)) {

        $records[] = $r;
    }

    // Output data as JSON
    echo json_encode($records);
 }
}

?>

Consider this:

<?php
    $dbhost     = "website";
    $dbuser     = "user";
    $dbpass     = "password";
    $dbname     = "database";
    $connection = mysql_connect($dbhost, $dbuser, $dbpass);
    //Kill page if connection could not be established
    //I also added mysql_error() for it to display an error if there was an issue, so you can troubleshoot.
    if(!$connection) {
        die("Connection Failed:" . mysql_error());
    }

    $dbconnect = mysql_select_db($dbname, $connection);
    //Same as before, kill page if there was an issue, and display an error.
    //No need for the IF-ELSE statements, if the `die()` function is hit, the page stops loading anyways.
    if(!$dbconnect) {
        die("Unable to connect to Database: " . mysql_error());
    }

    $query     = "SELECT * FROM blogs";
    $resultset = mysql_query($query, $connection);
    $records   = array();
    // Loop and check all records and add to array
    while($r = mysql_fetch_assoc($resultset)) {
        $records[] = $r;
    }
    // Output data as JSON
    echo json_encode($records);
?>

But I highly recommend switching to mysqli_* or PDO statements:

Please, don't use mysql_* functions for new code. They are no longer maintained and the community has begun the deprecation process, and mysql_* functions have been officially removed in PHP 7. Instead you should learn about prepared statements and use either PDO or mysqli_*. If you can't decide, this article will help to choose your best option.

Here is a refactor to mysqli_*

<?php
    $dbhost     = "website";
    $dbuser     = "user";
    $dbpass     = "password";
    $dbname     = "database";

    $records   = array();

    $connection = new mysqli($dbhost, $dbuser, $dbpass, $dbname);
    if($connection->connect_errno > 0){
        die('Unable to connect to database [' . $connection->connect_error . ']');
    }
    $query     = "SELECT * FROM blogs";
    if(!$result = $connection->query($query)){
        die('There was an error running the query [' . $connection->error . ']');
    }
    while($row = $result->fetch_assoc()){
        $records[] = $row;
    }
    echo json_encode($records);
?>

There are a couple of differences here, the first connection checks for an actual connection to the database AND to the table in 1 statement.

Another difference is it shows an error specifically for the query if there was a problem, and not for a table connection error in the 2nd error check.

Here is a PDO refactor:

  NOTE: I don't actively develop with PDO, I may have missed something here.

<?php
    $dbhost     = "website";
    $dbuser     = "user";
    $dbpass     = "password";
    $dbname     = "database";
    $charset = 'utf8';

    $records   = array();

    $dsn = "mysql:host=$dbhost;dbname=$dbname;charset=$charset";
    $opt = [
        PDO::ATTR_ERRMODE            => PDO::ERRMODE_EXCEPTION,
        PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
        PDO::ATTR_EMULATE_PREPARES   => false,
    ];
    $pdo = new PDO($dsn, $dbuser, $dbpass, $opt);

    $query     = "SELECT * FROM blogs";
    $stmt = $pdo->query($query);
    while ($row = $stmt->fetch()) {
        $records[] = $row;
    }
?>


simple "if"-less algebraic computation using CLOS For educational purposes I've tried to implement a simple algebraic OOP example using CLOS. The functionality is as far as I can say as it is supposed to be. The intended approach was to implement a simple example of ideomatic OOP polymorphism. For a greater learning experience I've wanted to show the code to get suggestions, critic and so on.

;; main node class, most general
(defclass node ()
  ;; abstract class for nodes
  ((value :initarg :value :reader value)))

(defgeneric evaluate (node)
  (:documentation "the standard evaluation"))

;; node containing an operation, more specific
(defclass op-node (node)
  ;; abstract class for a node containing an operation
  ((left-node  :initform nil :initarg :left-node  :reader left-node)
   (right-node :initform nil :initarg :right-node :reader right-node)))

(defmethod evaluate ((node op-node))
  (apply (value node)
         (list (evaluate (left-node node))
               (evaluate (right-node node)))))

;; concrete nodes
(defclass add-node (op-node)
  ;; class for addition
  ((value :initform #'+ :reader value)))

(defclass minus-node (op-node)
  ;; class for subtraction
  ((value :initform #'- :reader value)))

(defclass multi-node (op-node)
  ;; class for multiplication
  ((value :initform #'* :reader value)))

(defclass value-node (node)
  ;; class for a node containing a numerical value
  ((value :initarg :value :reader value)))

(defmethod evaluate (value-node)
  (value value-node))

;; 1 + 3
(setf *simple-add* (make-instance 'add-node
                                  :left-node  (make-instance 'value-node :value 1)
                                  :right-node (make-instance 'value-node :value 3)))

;; 1 * 10
(setf *simple-multi* (make-instance 'multi-node
                                    :left-node  (make-instance 'value-node :value 1)
                                    :right-node (make-instance 'value-node :value 10)))

;; (1 + 3) + (1 * 10)
(setf *advanced-add* (make-instance 'add-node
                                    :left-node *simple-add*
                                    :right-node *simple-multi*))

(evaluate *simple-add*)   ;; => 4
(evaluate *simple-multi*  ;; => 10
(evaluate *advanced-add*) ;; => 14

(defmethod evaluate ((node op-node))
  (apply (value node)
         (list (evaluate (left-node node))
               (evaluate (right-node node)))))

can be written:

(defmethod evaluate ((node op-node))
  (funcall (value node)
           (evaluate (left-node node))
           (evaluate (right-node node))))

For this example, instead of different subclasses for every binary operation, you could just use a binop class and instantiate it with the respective functions, since they share the same evaluate method, but that depends on what else you want do do with the nodes. (Maybe, for example, you want to have different print-object methods for each of them.)

(Also, using toplevel setfs on unestablished symbols is considered bad style, but I suppose this was just for testing and demonstration purposes. In real code, defparameter or defvar should be used.)


Creating an in-memory ViewCounter that commits to database every 10 minutes This singleton is responsible for counting every pageview (called Routes in our case) we get by id, until the timer runs out and commits the results to the database in one query. Then the counter gets reset and the timer starts again.

I made this to reduce the amount of connections happening on our database at a given time.

Our pages are HttpServlet subclasses, so they get called concurrently. Our viewcounter should support this.

This is the code I think would be right for this:

@Singleton
@ConcurrencyManagement(BEAN)
public class RouteViewcounterSingleton {

    private ConcurrentHashMap<Integer, Integer> viewCounter;

    @EJB
    private RouteViewcountDAO routeViewcountDAO;

    @Inject
    private RouteDAO routeDao;

    @Resource
    TimerService timerService;

    @PostConstruct
    public void initialize() {
        viewCounter = new ConcurrentHashMap<>();
    }

    public void incrementViewCount(Integer routeId) {
        if (viewCounter.containsKey(routeId)) {
            Integer counter = viewCounter.get(routeId);
            counter++;
            viewCounter.put(routeId, counter);
        } else {
            viewCounter.put(routeId, 1);
        }
    }

    @Timeout
    public void programmaticTimeout(Timer timer) {
        Logger.getLogger(RouteViewcounterSingleton.class.getName()).log(Level.SEVERE, "ViewCounter timer timeout: " + DateTime.now(), "");
    }

    @Schedule(second = "0", minute = "*/10", hour = "*", info = "ViewCounterTimer")
    public void scheduleTimer() {
        if (viewCounter != null) {
            int i = 0;
            for (Map.Entry<Integer, Integer> entrySet : viewCounter.entrySet()) {
                try {
                    Integer routeId = entrySet.getKey();
                    Integer viewCount = entrySet.getValue();
                    Route route = routeDao.find(routeId);
                    RouteViewcount routeViewcount = routeViewcountDAO.find(route);
                    if (routeViewcount != null) {
                        routeViewcount.setViews(routeViewcount.getViews() + viewCount);
                        routeViewcountDAO.edit(routeViewcount);
                    } else {
                        routeViewcountDAO.create(new RouteViewcount(route, viewCount));
                    }
                    i++;
                } catch (MultipleResultsException | NoResultException | AttributeNotUniqueException | VersionConflictException ex) {
                    Logger.getLogger(RouteViewcounterSingleton.class.getName()).log(Level.SEVERE, null, ex);
                }
            }
            Logger.getLogger(RouteViewcounterSingleton.class.getName()).log(Level.INFO, "Viewcounter persisted for " + i + " routes on :" + DateTime.now(), "");
            viewCounter.clear();
        }
    }
}

This method is broken in two ways. First, multiple threads can access the counter at the same time, and based on ordering, you may lose some updates. Second, if the counter is being created for the first time by multiple threads, you may create the counter multiple times.

public void incrementViewCount(Integer routeId) {
    if (viewCounter.containsKey(routeId)) {
        Integer counter = viewCounter.get(routeId);
        counter++;
        viewCounter.put(routeId, counter);
    } else {
        viewCounter.put(routeId, 1);
    }
}

In addition, you are not clear()ing the map in a thread-safe way. You should strongly consider an existing counter class, like Guava's AtomicLongMap. Writing a thread-safe counter is surprisingly non-trivial.


Excel's SUMIFS implemented using PANDAS, the Python Data Analysis Library I've implemented Excel's SUMIFS function in Pandas using the following code. Is there a better — more Pythonic — implementation?

from pandas import Series, DataFrame
import pandas as pd
df = pd.read_csv('data.csv')
# pandas equivalent of Excel's SUMIFS function
df.groupby('PROJECT').sum().ix['A001']

One concern I have with this implementation is that I'm not explicitly specifying the column to be summed.

Data File

Here's an example CSV data file (data.csv), although I'm displaying | instead of commas to improve the visual appearance.

DATE     | EMPLOYEE     | PROJECT | HOURS
02/01/14 | Smith, John  | A001    | 4.0
02/01/14 | Smith, John  | B002    | 4.0
02/01/14 | Doe, Jane    | A001    | 3.0
02/01/14 | Doe, Jane    | C003    | 5.0
02/02/14 | Smith, John  | B002    | 2.0
02/02/14 | Smith, John  | C003    | 6.0
02/02/14 | Doe, Jane    | A001    | 8.0

Equivalent Excel SUMIFS Function

If I were to open data.csv in Excel and wanted to determine how many hours were worked on project A001, I would use the SUMIFS formula as follows:

=SUMIFS($D2:$D8, $C2:$C8, "A001")

Where the SUMIFS function syntax is:

=SUMIFS(sum_range, criteria_range1, criteria1, [criteria_range2,
        criteria2], …)

The usual approach -- if you want all the projects -- would be

>>> df.groupby("PROJECT")["HOURS"].sum()
PROJECT
A001       15
B002        6
C003       11
Name: HOURS, dtype: float64

This only applies the sum on the desired column, as this constructs an intermediate SeriesGroupBy object:

>>> df.groupby("PROJECT")["HOURS"]
<pandas.core.groupby.SeriesGroupBy object at 0xa94f8cc>

If you're only interested in the total hours of a particular project, then I suppose you could do

>>> df.loc[df.PROJECT == "A001", "HOURS"].sum()
15.0

or if you dislike the repetition of df:

>>> df.query("PROJECT == 'A001'")["HOURS"].sum()
15.0

but I find that I almost always want to be able to access more than one sum, so these are pretty rare patterns in my code.

Aside: .ix has fallen out of favour as it has some confusing behaviour.  These days it's recommended to use .loc or .iloc to be explicit.


Concurrent bidirectional dictionary I've been agonizing over this for an hour or two. I require a bidirectional dictionary that was thread safe. From what I understand about dictionaries, they're thread safe when being read from, but no so much when being written to. I've tried to write this code snippet to my understanding of correct, but a review would be excellent if possible.

class ConcurrentBidirectionalDictionary<TKey,UKey>
{
    private Dictionary<TKey, UKey> BackingDictionary;

    public ConcurrentBidirectionalDictionary()
    {
        BackingDictionary = new Dictionary<TKey, UKey>();
    }

    //Adding to dictionaries is not thread-safe ergo, lock when adding
    private readonly object _SyncLock = new object();
    public void Add(TKey item1, UKey item2)
    {
        if (!BackingDictionary.ContainsKey(item1) && !BackingDictionary.ContainsValue(item2))
        {
            lock (_SyncLock)
            {
                if (!BackingDictionary.ContainsKey(item1) && !BackingDictionary.ContainsValue(item2))
                {
                    BackingDictionary.Add(item1, item2);
                }
                else
                {
                    ThrowNewDuplicateKeyException(item1, item2);
                }
            }
        }
        else
        {
            ThrowNewDuplicateKeyException(item1, item2);
        }
    }

    private void ThrowNewDuplicateKeyException(TKey item1, UKey item2)
    {
        if (BackingDictionary.ContainsKey(item1) && BackingDictionary.ContainsValue(item2))
        {
            throw new ArgumentException("Duplicate Keys not allowed", "item1, item 2", null);
        }
        else if (BackingDictionary.ContainsKey(item1))
        {
            throw new ArgumentException("Duplicate Keys not allowed", "item1", null);
        }
        else if (BackingDictionary.ContainsValue(item2))
        {
            throw new ArgumentException("Duplicate Keys not allowed", "item 2", null);
        }
    }

    //Reading is thread safe, no lock needed
    public TKey GetFromUKey(UKey UKey)
    {
        var Record = BackingDictionary.FirstOrDefault(x => EqualityComparer<UKey>.Default.Equals(x.Value, UKey));
        if (Record.Equals(default(KeyValuePair<TKey,UKey>)))
        {
            throw new IndexOutOfRangeException($"Unable to find UKey {UKey}");
        }
        return Record.Key;
    }

    public UKey GetFromTKey(TKey TKey)
    {
        return BackingDictionary[TKey];
    }
}

It is better to use CouncurrentDictionary to reduce lock contention:

class Map<X, Y>
{
    public X this[Y y]
    {
        get { return YX[XY[YX[y]]]; } // Ensure Set completion
        set { Set(value, y); }
    }

    public Y this[X x]
    {
        get { return XY[x]; }
        set { Set(x, value); }
    }

    void Set(X x, Y y)
    {
        if (!YX.GetOrAdd(y, x).Equals(x))
            throw new InvalidOperationException();

        if (!XY.GetOrAdd(x, y).Equals(y))
            throw new InvalidOperationException();
    }

    ConcurrentDictionary<X, Y> XY { get; } = new ConcurrentDictionary<X, Y>();
    ConcurrentDictionary<Y, X> YX { get; } = new ConcurrentDictionary<Y, X>();
}


Flash Memory block write counting & thresholding I am trying to solve this question:

  In a block storage system, new data is written in blocks. We are going
  to represent the flash memory as one sequential array. We have a list
  of block writes coming in the form of arrays of size 2: writes[i] =
  [first_block_written, last_block_written].

  Each block has a rewrite limit. If rewrites on a block reach a certain
  specified threshold we should run a special diagnostic.

  Given blockCount (an integer representing the total number of blocks),
  writes (the list of block-write arrays of size 2), and threshold, your
  task is to return the list of disjoint block segments, each consisting
  of blocks that have reached the rewrite threshold. The list of block
  segments should be sorted in increasing order by their left ends.

  Example

  For blockCount = 10, writes = [[0, 4], [3, 5], [2, 6]], and threshold
  = 2, the output should be blockStorageRewrites(blockCount, writes, threshold) = [[2, 5]].

  After the first write, the blocks 0, 1, 2, 3 and 4 were written in
  once
  After the second write, the blocks 0, 1, 2 and 5 were written in
  once, and the blocks 3 and 4 reached the rewrite threshold;
  After the final write, the blocks 2 and 5 reached the rewrite threshold as well,
  so the blocks that should be diagnosed are 2, 3, 4 and 5. Blocks 2, 3,
  4 and 5 form one consequent segment [2, 5]. For blockCount = 10,
  writes = [[0, 4], [3, 5], [2, 6]], and threshold = 3, the output
  should be blockStorageRewrites(blockCount, writes, threshold) = [[3,
  4]];

  For blockCount = 10, writes = [[3, 4], [0, 1], [6, 6]], and threshold
  = 1, the output should be blockStorageRewrites(blockCount, writes, threshold) = [[0, 1], [3, 4], [6, 6]].

I have come up with a correct naive solution here but it's too slow to pass the test cases:

def blockStorageRewrites(blockCount, writes, threshold):
    a = defaultdict(int)
    for (l, r) in writes:
        for i in range(l, r+1):
            a[i] += 1

    start = None
    res = []
    for i in range(0, blockCount+1):
        if a[i] >=threshold:
            if start is None:
                start = i
        elif a[i] < threshold and start is not None:
            res.append([start, i-1])
            start = None
    return res

How do I optimize it to run faster?

You can get an immediate speed-up by ditching the defaultdict(int), and using a bytearray(blockCount+1) instead.  Both have roughly \$O(1)\$ lookup time, but the latter has a much smaller constant factor.  In the former, each key must be hashed, then binned, then a linear search through the bin is required to find the correct key entry, if it exists, and creating it if it does not.  In the latter, the key is a direct index to the required memory.

This demonstrates a better than 3x improvement of bytearray over defaultdict:

>>> from collections import defaultdict
>>> from timeit import timeit
>>> def original():
    d = defaultdict(int)
    for i in range(1000000):
        d[i] += 1

>>> def using_bytearray():
    b = bytearray(1000000)
    for i in range(1000000):
        b[i] += 1

>>> timeit(original, number=100)
25.083420443999984
>>> timeit(using_bytearray, number=100)
7.779039941000008

Not only is the bytearray faster, it is also more memory efficient.  The bytearray uses just 57 bytes of overhead to store an array of 1 million bytes.  In contrast, the defaultdict weighs in at over 40x more memory ...

>>> sys.getsizeof(b)
1000057
>>> sys.getsizeof(d)
41943144

... but the real kicker is that is just the size of the dictionary's binning structures.  It excludes the size of all of the keys and values stored in the dictionary; those are harder to total as many small integers are interned and don't use additional storage allocation, and duplicate values may point to the same object.

Note: If threshold can exceed 255, you’d need to use array.array instead, with the appropriate type code for the largest rewrite count you’ll experience.

Of course, this is still the wrong approach.  With writes = [[0, 999999], [1, 1000000]], it should be obvious that block 0 is written once, blocks 1 through 999999 are written twice, and block 1000000 is written once.  There is no need to loop through the million indices; we just need to consider the end-points of each range.

So just store the starting & ending points, and the number of times each has been seen.  With a sorted array of indices of where the transitions occur, you should be able to iterate over these transition points, and construct your diagnostic list.

Implementation left to student.

Bonus: it is possible use one simple Dict[int,int] structure to store both starting and ending counts, if you think about it the right way.


Simple yet threadsafe file cache in java I'm attempting to cache contents of a file to avoid frequent reads, using a very simple implementation in a single class. The file contains a list of emailids, one per line. The file changes rarely, and the cache should be updated immediately when it does. So I've checked the file's timestamp in each read operation. The class will be used in servlets and rest apis - as member variable. Although I've synchronized the only public method of the class, I'm not sure whether the code is threadsafe. Do I need to add/change something here to ensure thread safety ?

public class CachedFile {

    private File file ;
    private long lastModified ;
    private List<String> fileLines ;
    private boolean fileCached ;

    public CachedFile( final String inputFileName )
    {
            file = new File( inputFileName ) ;
            fileCached = false ;
    }

    public synchronized List<String> getLines() throws IOException
    {
            //if file is not cached or if its modified after the last read, read & cache it
            if( fileCached == false || fileModifiedAfterLastRead() )
            {
                    readFile() ;
            }

            return fileLines ;
    }

    private void readFile() throws IOException
    {
            //read file to cache
            fileLines = FileUtils.readLines( file ) ;

            //cache the last modified time
            lastModified = file.lastModified() ;

            //set the cached flag to true
            fileCached = true ;
    }

    private boolean fileModifiedAfterLastRead()
    {
            return( file.lastModified() > lastModified ) ;
    }
}

Since you mentioned that the file could be written by another process (assuming in another jvm), there is no way you can introduce the Object monitor based thread safety. So you can depend upon the last modified time of the file object. You can also remove the synchronization from your getLines() if you dont return the instance variable. But you do return that so synchronization should be there in getLines().

Basically you want to read the file without the data corruption. That could happen if another process is writing to the file when your process reads from it. If that is the case, you want to re-read the file. Here is how I would implement it.

 private void readFile() throws IOException {
     int retryCount = 5;

     while (retryCount > 0) {

       long beforeRead = file.lastModified();
        //read file to cache
       fileLines = FileUtils.readLines(file);

       if (beforeRead == file.lastModified())
           break;

       retryCount--;
     }
  //cache the last modified time
    lastModified = file.lastModified() ;

  //set the cached flag to true
    fileCached = true ;
}

The while loop will retry the read operation if there is a mismatch. Also the retry has to be finite number to avoid looping forever in case of any file system failures.


Selective autorotation in a UINavigationController In a current project I needed to enable/disable autorotation of the UI to specific UIView's. I looked around and did not find a working solution that fitted my case and returning NO in

shouldAutorotateToInterfaceOrientation:(UIInterfaceOrientation)interfaceOrientation

method did not work as it seems to get overridden by the value returned by the UIView in which the UINavigationController resides in.

So I now keep track of which view is currently being pushed into the stack by using the delegate method:

- (void)navigationController:(UINavigationController *)navigationController didShowViewController:(UIViewController *)viewController animated:(BOOL)animated

and then check if the current view is my "don't rotate view" in

shouldAutorotateToInterfaceOrientation:(UIInterfaceOrientation)interfaceOrientation

and return NO if that's the case, works perfectly.

In my code it looks like this:

- (BOOL)shouldAutorotateToInterfaceOrientation:(UIInterfaceOrientation)interfaceOrientation
{
    // Return YES for supported orientations
    if (currentView == [DimensionConversionViewController class]) {
        return NO;
    }
    return YES;
}

- (void)navigationController:(UINavigationController *)navigationController didShowViewController:(UIViewController *)viewController animated:(BOOL)animated {
    currentView = [viewController class];
}

This looks more like a hack to me. Has anybody come across the same problem and solved it in another/more elegant way? As stated above this seems to be the only way to achieve this.

First of all you could skip storing the current view controller in a variable. And instead of checking for the correct class you could delegate your shouldAutorotateToInterfaceOrientation: to the current visible one in the navigation controller.

- (BOOL) shouldAutorotateToInterfaceOrientation:(UIInterfaceOrientation)interfaceOrientation {
    // assumes you've got access to the navigation controller here
    UIViewController *visibleController = navigationController.visibleViewController;

    return [visibleController shouldAutorotateToInterfaceOrientation:interfaceOrientation];
}


Header file for accessing components globally in library In my project I have a header file which contains only functions which are put into a namespace, the purpose of these functions is to be used from another class of the framework I'm working on, or from client code.

Does this code follow best practices? How can I efficiently just keep such data?

#pragma once

#include <iomanip>
#include <sstream>
#include <chrono>
#include <ctime>

namespace lwlog::datetime
{
    std::string get_chrono(std::string format)
    {
        auto now = std::chrono::system_clock::now();
        auto in_time_t = std::chrono::system_clock::to_time_t(now);

        std::stringstream ss;
        ss << std::put_time(std::localtime(&in_time_t), format.c_str());
        return ss.str();
    }

    std::string get_time()                      { return get_chrono("%H:%M:%S"); }
    std::string get_date()                      { return get_chrono("%Y-%m-%d"); }
    std::string get_date_short()                { return get_chrono("%m/%d/%y"); }
    std::string get_second()                    { return get_chrono("%S"); }
    std::string get_minute()                    { return get_chrono("%M"); }
    std::string get_hour_24()                   { return get_chrono("%H"); }
    std::string get_hour_12()                   { return get_chrono("%I"); }
    std::string get_weekday()                   { return get_chrono("%A"); }
    std::string get_weekday_abbreviated()       { return get_chrono("%a"); }
    std::string get_day()                       { return get_chrono("%d"); }
    std::string get_month()                     { return get_chrono("%m"); }
    std::string get_month_name()                { return get_chrono("%B"); }
    std::string get_month_name_abbreviated()    { return get_chrono("%b"); }
    std::string get_year()                      { return get_chrono("%Y"); }
    std::string get_year_short()                { return get_chrono("%y"); }
}

As noted by commenters, you will get multiple definition errors if you try to include this header in multiple translation units. The functions must be marked inline, or the header file reduced to the declarations, with the definitions placed in a separate source file.
The std::string format argument is copied unnecessarily as it is passed by value. It should be passed by const& instead. (Or replaced with std::string_view).
The difference between all these functions is the format string. As such, we don't really need the functions at all. We can define the format strings as constants, and the user can pass the one they need to get_chrono.
get_chrono is probably not the best name for the function. Maybe format_time_now, or format_time.
The function would be much less restrictive if the time were passed as an argument. It can then be used for formatting other times than now.
Note also that in the original code, if we wanted to call get_second and get_minute for one point in time (say for some custom output format), we can't! Because now will have changed between the calls, and we might end up with incorrect output.


JavaScript - Given a list of integers, return the first two values that adds up to form sum. sum = x + y The following function sum_pairs takes two arguments one array and another number, It should return the first two values (parse from the left please) in order of appearance that adds up to form the sum. Negative numbers and duplicate numbers can and appear.

For example

sum_pairs([10, 5, 2, 3, 7, 5],         10)
#              ^-----------^   5 + 5 = 10, indices: 1, 5
#                    ^--^      3 + 7 = 10, indices: 3, 4 *
#  * entire pair is earlier, and therefore is the correct answer
== [3, 7]

My following code works but takes time. I know it can be optimized but can't think how can I replace any of the loops.

var sum_pairs=function(ints, s){
  var result = [];
  var finalResult = undefined

  for(let i = 0; i < ints.length; i++){
    var anotherArray = ints.slice()
    anotherArray[i] = 0
    for(let j = 0; j < anotherArray.length; j++){
      var sum = ints[i] + anotherArray[j]
      if(sum === s){
        var tempResult = []
        tempResult.push(i)
        tempResult.push(j)
        result.push(tempResult)
      }
    }
  }

  var resultArray = []
  result.map((array, index) => {
    array.map((number, idx) => {
      array[idx] === array[idx+1] ? result.splice(index, 1) : false
      array.every((num, i) => i === array.length - 1 || num < array[i + 1]) ? resultArray = array : false
    })
  })

  s === 0 ?
  finalResult = [0, 0] : resultArray.length === 0 ?
  finalResult = undefined : finalResult = [ints[resultArray[0]], ints[resultArray[1]]]

  return finalResult
}

Any suggestion to optimize further?

Bug

The implementation doesn't work well with duplicate numbers:

console.log(sum_pairs([10, 5, 5, 3, 7, 4], 10));
[ 3, 7 ]

That output should be [ 5, 5 ].

Also,
for sum_pairs([10, 5, 2, 3, 7, 5], 8) I would have expected [ 5, 3 ] instead of [ 3, 5 ] as the output.
You didn't mention that results should be sorted.

The slowness

I see several reasons for the slowness:

The implementation is quadratic, using a nested loop
The implementation uses several expensive operations, such as slice and splice of arrays

Also, the implementation is extremely complicated and hard to follow,
so there might be other causes too.

A simpler solution is possible if you track the elements seen so far in an object, and check if target - current was seen or not.

var sum_pairs = function(ints, s) {
  var seen = {};

  for (let i = 0; i < ints.length; i++) {
    let num = ints[i];
    if (seen[s - num]) {
      return [s - num, num];
    }
    seen[num] = true;
  }
}


List component using plain JS beginner web dev here.
I created a custom list component (with edit feature) in plain JS/HTML.

I have few questions on which I would like to get an answer (besides the normal feedback that you folks do here on the site):

is this a standard/right way to create and use components when using plain JS?
if I can create and use objects like this, what is the benefit of creating such component using say react.js?

ListComponent.js

///////
//
// ListComponent is a class which lets you create a list component
// dynamically using JS and DOM. The list component also has some features
// out of the box - e.g. editing items when clicked.
//
function ListComponent(type) {

    // Model data of the list.
    this.data = [{ name: "mona", id: 0 }, { name: "dona", id: 1 }, { name: "jona", id: 2 }],

    // Create a list component.
    this.create = function () {
        let list = document.createElement(!type ? "ul" : type);
        list.id = "customList";
        document.body.appendChild(list);
        this.draw();
    },

    this.remove = function () {
        // Remove our list component from DOM
        var elem = document.getElementById("customList");
        return elem.parentNode.removeChild(elem);

    }

    ///
    // draw
    //  Appends items to the list component.
    //  Deletes any child items first if there are any.
    //
    this.draw = function () {
        let that = this;

        // First delete all items of the list.
        let list = document.getElementById("customList");
        while (list.firstChild) {
            list.removeChild(list.firstChild);
        }

        // Now, append new items.
        that.data.forEach(function (item) {
            let listItem = document.createElement("li");

            // Listen to click event on this list item.
            listItem.addEventListener("click", function () {
                let newName = prompt("Enter item name", item.name);
                if (newName == null || newName.length == 0)
                    return;

                // Make change in the list model data.
                for (let i = 0; i < that.data.length; i++) {
                    if (that.data[i].id == item.id) {
                        that.data[i].name = newName;
                        break;
                    }
                }

                // Redraw the list.
                that.draw();
            }, false);

            listItem.innerHTML = item.name;
            listItem.id = item.id;
            list.appendChild(listItem);

        });
    }
}

index.html

<!doctype html>

<html lang="en">

<head>
    <meta charset="utf-8">
    <title>Demo site</title>
    <!-- Import our list component -->
    <script src="ListComponent.js"></script>
</head>

<script>

    document.addEventListener("DOMContentLoaded", function (event) {
        // Run this code when DOM is loaded.
        let list = new ListComponent("ol");
        list.create();
    });

</script>

</body>

</html>

is this a standard/right way to create and use components when using plain JS?

It's not technically wrong, but when you're writing re-usable components it's better to use prototype than to create methods in the constructor itself.

For example,

function listComponent(){}
listComponent.prototype.create = function() { .. };
listComponent.prototype.remove = function(){ ... };
listComponent.prototype.draw = function(){ ... };

When you create the methods inside the constructor then those methods are duplicated in memory for every object created. When you use the prototype those methods only need to be defined once.

If you want to write code that feels like a class then use an actual class. (JS has those now).

Aside from that, don't store you data in the function that creates the list. Pass the data into the constructor or create method.

  if I can create and use objects like this, what is the benefit of
  creating such component using say react.js?

If all you need is to generate a simple list then there is no reason to use react. React has a lot of components that your code doesn't have, like interactive features, etc.

If you take a few minutes to read about exactly what react is and does the answer to this question will be clear.


Weighted Random Choice I have a Cython function that takes a list of weights/probabilities (double) and returns a random index into the list.

For example:

choose_one(np.array([0.1, 0.4, 0.2, 0.3]), 4)

Returns 0 with probability 0.1, 1 with probability 0.4 etc.

The Cython code is:

cdef extern from "stdlib.h":
    double drand48()
    void srand48(long int seedval)

@cython.boundscheck(False)
@cython.wraparound(False)
@cython.nonecheck(False)
def choose_one(np.ndarray[double, ndim=1] weights, Py_ssize_t length):
    cdef Py_ssize_t idx, i
    cdef double cs
    cdef double random
    random = drand48()
    cs = 0.0
    i = 0
    while cs < random and i < length:
        cs += weights[i]
        i += 1
    return i - 1

I profiled some complex simulation code and found that about 50% of the execution time is spent in this function which is called approximately 3.5 million times. Because it is such a time hog, and such an important utility, I'd like to speed it up if possible.

One of the problems is that the input arrays are often very large with thousands of entries.

I tried various methods from Numpy in Cython and none of them come close to as fast as what I have written in Cython. I am a Cython newbie and am curious if there is something else I need to do to speed this up.

The performance can be improved a bit more by declaring the function using cpdef (or cdef) instead of def (when the calling code is using Cython types too, check out http://docs.cython.org/src/userguide/language_basics.html#python-functions-vs-c-functions for more info).

Another idea that might help is to check whether the random number is greater than 0.5 and, if it is, count backwards from the end of the array rather than the front (assuming that the values in the array always add up to one).

Also, if this function takes up half of the program's running time then it seems likely that the program could be restructured to use a different algorithm which calls this function less often, but this would probably require much more effort than the other suggestions.


Dynamic web scraper in Python The other week I faced the struggle of scraping dynamically generated content. So I used the selenium library with combination of requests & bs4, the thing is I am unsure of the quality of the implementation as I just learned how to use those tools. I want to have a general feedback on the way I used the libraries, the quality of my code and the logic behind it.

Link to GitHub README.

from selenium import webdriver
from selenium.webdriver.chrome.options import Options
from selenium.webdriver.common.by import By
from selenium.webdriver.support.ui import WebDriverWait
from selenium.webdriver.support import expected_conditions as ec
import selenium.common.exceptions
import requests
from bs4 import BeautifulSoup
from time import sleep

def scraper():
    opts = Options()
    opts.add_argument('--headless')
    driver = webdriver.Chrome(r'C:\Users\leagu\chromedriver.exe', options=opts)

    pos = input('Enter your desired position: ')
    URL = 'https://remote.co/remote-jobs/search/?search_keywords='+pos.replace(' ', '+')
    driver.get(URL)

    # Scroll to the bottom of the page
    while True:
        try:
            WebDriverWait(driver, 5).until(
                ec.text_to_be_present_in_element(
                    (By.CLASS_NAME, 'load_more_jobs'),
                    'Load more listings')
                )
            loadMore = driver.find_element_by_class_name('load_more_jobs')
            loadMore.click()
        except:
            try: # can't locate element - click the close on the popup add
                WebDriverWait(driver, 5).until(
                    ec.presence_of_element_located((By.CLASS_NAME, 'portstlucie-CloseButton'))
                    )
                addClose = driver.find_element_by_xpath('//*[@id="om-oqulaezshgjig4mgnmcn-optin"]/div/button')
                addClose.click()
            except: # Timeout / can't locate add - break
                break

    # Find all the job listings
    listings = driver.find_elements_by_class_name('job_listing')

    if len(listings) == 0:
        print(f'There are 0 jobs found by {pos} criteria. Please use different wording.')
        sleep(5)
        scraper()
    else:
        scrapeN = input(f"There are {len(listings)} number of jobs for the {pos} position. If u wish to view a portion of them enter the number of the jobs to view else write 'max': ")
        if scrapeN.lower() == 'max':
            scrapeN = len(listings)
        scrapeN = input(f"There are {len(listings)} number of jobs for the {pos} position. If u wish to view a portion of them enter the number of the jobs to view else write 'max': " )
        print('\n')

    for i in range(int(scrapeN)): # Iterate trough all the job listings
        URL = listings[i].find_element_by_tag_name('a').get_attribute('href')
        html = requests.get(URL)
        soup = BeautifulSoup(html.content, 'html.parser')

        jobT = soup.find('h1', class_='font-weight-bold').text
        jobPD = soup.find('time').text
        link = soup.find('a', class_='application_button')['href']

        print(f'Job - {jobT}. This job was {jobPD}.\nMore information about the job at {URL}. \nLink for application - {link}', end='\n\n')

if __name__ == '__main__':
    scraper()

What I like:

usage of f-strings, but you are not using them everywhere eg: URL = 'https://remote.co/remote-jobs/search/?search_keywords='+pos.replace(' ', '+')
usage of if __name__ == '__main__':
code is fairly easy to grasp
effort to provide a decent working script with a minimum of code (70 lines)

What I like less:

usage of exception handling: I would be more specific and not catch any type of exception, only handle those that are relevant to the operation being performed in the try/catch block (selenium.common.exceptions)
lack of global exception handling in the rest of the code
also think about logging full exception details to a file so you don't have to guess what has gone wrong
I would also avoid nesting of try/catch blocks, try to separate each operation from each other (see below)
mixing lower case and upper case in variable names: remember that variable names are case-sensitive in Python. jobPD and jobpd are two different variables that can get assigned different values so this could be a nasty source of bugs
variable names should be more descriptive and meaningful: jobPD does not give a clear hint about what it represents. More descriptive names would be job_title, job_posted_time etc

Regarding the scraping process, make sure that the DOM elements your are expecting are really present: websites change their layout more or less often and you must spot changes that you could break your application. You can either check with Selenium or BS4 if you have already retrieved the HTML. But it seems logical to use Selenium. If you use BS, note the behavior of the different functions:

From the BS4 doc (emphasis is mine):

  If find_all() can’t find anything, it returns an empty list. If find()
  can’t find anything, it returns None

You have this block (the nested try/catch block):

    try: # can't locate element - click the close on the popup add
        WebDriverWait(driver, 5).until(
            ec.presence_of_element_located((By.CLASS_NAME, 'portstlucie-CloseButton'))
            )
        addClose = driver.find_element_by_xpath('//*[@id="om-oqulaezshgjig4mgnmcn-optin"]/div/button')
        addClose.click()
    except: # Timeout / can't locate add - break
        break

Is is better to anticipate and avoid exceptions, rather than handling them. So it seems to me that you should verify the existence of the element if possible, rather than trigger an exception.

Instead, you could use the more generic findElements function. Note that findElement is different than findElements. The difference:

  findElements will return an empty list if no matching elements are
  found instead of an exception (NoSuchElementException)

Reference: Find Element and FindElements in Selenium WebDriver

However, if you stick to the current approach, you should not blindly catch all exceptions: in this context the one relevant exception that may occur is: NoSuchElementException.

There is one thing that is not okay: you are using the requests module in parallel to Selenium. That is unnecessary since you already have an instance of Selenium that you can use. To fetch the whole HTML just use:

html = driver.page_source

then feed it to BS

Final thought: have you thought about logging the results to a file, a table or CSV perhaps ? The console buffer may be too small if you retrieve lots of results.


Monoid Instance for `a -> b` I attempted to implement a Monoid' instance for a function a -> b.

class Monoid' a where
    mempty  :: a -> a
    mappend :: a -> a -> a

instance Monoid' (a -> b) where
    mempty  f   = f
    mappend f g = g . f

Please critique it for correctness and style.

In addition, I'm curious how mappend should be implemented. Should f or g be applied first?

Something nice about hackage help/documentation pages like the one you linked: you can go straight to the source code by clicking on the links on the right side of the page.  Looking at the source, there is an instance for Monoid b => Monoid (a -> b), which is not quite exactly what you're trying to do, since it restricts b to types with a Monoid implementation.  It uses a nice trick where it implements this monoid in terms of the other monoid.

The Data.Monoid code:

instance Monoid b => Monoid (a -> b) where
    mempty _ = mempty
    mappend f g x = f x `mappend` g x

If you have any difficulty understanding it, keep in mind that the mempty and mappend functions are for different monoids on the left hand side than they are for the right hand side.

I can think of 2 ways of getting 2 a -> bs to combine: throw away the first one, or throw away the second one.  Neither seems similar to what you're trying to do (or particularly useful).

Besides the (already noted) problem that . as an implementation of mappend won't typecheck, there's another problem: mempty.  As written, it'll typecheck, but the type signature of your mempty is a -> a, while it should be just a.  mempty is a value in the monoid.

I'd be tempted to change the definition to mempty = id, but id :: a -> a not a -> b.  You could try to make a constant function that just returns some particular b, but since you have no idea what type b will be, that's not possible.

In general, haskell's type system is very nice -- not only can it catch type errors, but it can serve as documentation, provide a bit of heuristic guidance and help you organize your thoughts.  When something doesn't typecheck, it means what you're trying to do doesn't quite make sense.