TranslateIt class using the Yandex translation service I wrote a class TranslateIt for translating text files with Yandex.Translate service. I feel the code is rather bulky. Is it better to decompose it to several methods? I'm eager to know opinions of experienced people.

import requests
import os
import chardet

class TranslateIt(object):
    """
    methods
    translateit (source, **kwargs)
    translateit - translates source into output
    source - txt file to be translated
    froml - source language, if not specified, yandex tries to autodetect
    tol - translation language, default is ru
    output - translayed txt file

    attributes
    params - parameters dict for yandex api
    encoding - encoding of source file detected by _checkEncoding method
    source_name, output_name - absolute paths of source and output files respectively
    code - result of translation
    message - error message if translation failed
    translated = translated text if succeeded

    constants
    MAX_SIZE - maximum size of a source file according to yandex api
    """

    key = 'trnsl.1.1.20170331T021116Z.53a353fef9d94e08.9617ecfdaf7fbe170972c4107aac8780e53dd21f'
    api_url = 'https://translate.yandex.net/api/v1.5/tr.json/translate'
    MAX_SIZE = 10000
    yandex_string = "\nTranslated by Yandex.Translate service, 'http://translate.yandex.ru/'" #added to translation result according to licence requirements

    #check the encoding of a source file
    def _checkEncoding(self, file_name):
        result = chardet.detect(open(file_name, "rb").read())
        open(file_name).close()
        return result['encoding']

    #translates source to output
    def translateIt(self, source, **kwargs):
        self.params = {'key': TranslateIt.key}
        #check if kwargs are valid
        kwargs_tuple = ('output', 'froml', 'tol')
        if not all([key in kwargs_tuple for key in kwargs.keys()]):
            raise NameError ('Invalid named parameter')
        # check if source file exists
        if os.path.exists(source) and source.endswith('.txt'):
            self.encoding = TranslateIt._checkEncoding(self, source)
            self.source_name = os.path.abspath (source)
            # if source exists, assign it's content to text parameter
            with open(self.source_name, 'r', encoding=self.encoding) as f:
                text = f.read()
                if len (text) < TranslateIt.MAX_SIZE:
                    self.params['text'] = text
                else:
                    raise ValueError('File size can not be more than {} symbols'.format(TranslateIt.MAX_SIZE))
        else:
            raise FileNotFoundError('No data source file found')

        #create translation pair
        if 'tol' not in kwargs.keys():
            kwargs['tol'] = 'ru'
        if 'from' not in kwargs.keys():
            self.params['options'] = 1
            self.params['lang'] = kwargs['tol']
        else:
            self.params['lang'] = kwargs['froml'] + '-' + kwargs['tol']

        #create output file:
        if 'output' in kwargs.keys():
            self.output_name = os.path.abspath(kwargs['output']) if kwargs['output'].endswith('.txt') else os.path.abspath(kwargs['output'] +'.txt')
        else:
            self.output_name = os.path.abspath('translated_' + self.params['lang'] + '_' + os.path.split(self.source_name)[-1])

        #translate, save result in output file if succeeded
        response_result = requests.get(TranslateIt.api_url, params=self.params).json()
        self.code = response_result['code']
        if self.code != 200:
            self.message = response_result['message']
            print ('Translation failed\n Error code: {0}, error message: {1}'.format(self.code, self.message))
        else:
            self.translated = response_result['text'][0] + TranslateIt.yandex_string
            with open(self.output_name, 'w', encoding='utf8') as f:
                f.write(self.translated)
            print ('Translation successful, result saved in {0}'.format(self.output_name))

Some ideas:

If the arguments are a fixed list, don't use **kwargs. I find **kwargs is only really useful if you have some middleware code which needs to forward an arbitrary set of arguments to another piece of code. That way people don't need to rely on your documentation (or when the documentation inevitably gets out of date, the code) to understand which parameters the method takes. Using named parameters you can also get rid of loops over **kwargs.
IMO attributes and constants shouldn't be documented at all. Any IDE will be able to auto-complete them, and their names should be sufficiently self-explanatory that you don't need extra documentation.
Starting a method name with check implies an assertion is happening. Use get when that is what you're doing.
This is a personal style item, but I prefer that the interesting/important code is at the top of the file. _checkEncoding is not that interesting.
If key is a secret it should not be hard-coded.
Don't second guess TranslateIt's API. For example, don't verify the max size on the client side. Instead you should rely on the API to tell you when the size is too large and forward the information to the user. This way your implementation is simpler and more importantly doesn't get out of sync with the API.
You should use the with open() pattern in _checkEncoding.
Use pep8 to fix style issues like method names.
Your script is not usable from a command line.
Why make yandex_string a field? It is only used once, and is never changed, so you might as well treat it as a constant.
The code is not testable as it stands. This is too big a subject to expand much on here, but suffice it to say that translateIt does much more than one thing.


Refactor three state button I have a button (or icon) which can have three states.

On hover, it changes state and on click, the new state gets transferred to another state. This state is kept until you click again for which it gets back to normal.

The code is working, but it does not look or feel good. Is there any way I can re-factor this?

Fiddle
More real example

HTML

    1

        2

        3

CSS

.hidden { display: none; }

JQuery

$(function() {
    $(".rendering-expert.request-list-supported").hover(function() {
        if (!$(this).hasClass("has-request")) {
            $(this).find(".fa-cog").hide();
            $(this).find("[rel='request-list-add']").removeClass("hidden");
        }
    }, function() {
        if (!$(this).hasClass("has-request")) {
            $(this).find(".fa-cog").show();
            $(this).find("[rel='request-list-add']").addClass("hidden");
        }
    }).on("click", "[rel='request-list-add']", function(e) {
        e.preventDefault();
        var toggler = $(this);
        var href = toggler.attr("href");
        toggler.parent().find(".fa-cog").hide();
        toggler.next("[rel='request-list-remove']").removeClass("hidden");
        toggler.toggleClass("hidden");
        toggler.parent().toggleClass("has-request");
    }).on("click", "[rel='request-list-remove']", function(e) {
        e.preventDefault();
        var toggler = $(this);
        var href = toggler.attr("href");
         toggler.parent().find(".fa-cog").show();
         toggler.prev("[rel='request-list-remove']").addClass("hidden");
         toggler.addClass("hidden");
         toggler.parent().removeClass("has-request");
    });
});

Please note that some parts are stripped in this example. It will only change state in my production if an AJAX request succeeds.

Since jquery event handler is a memory and processing "eater" because has to listen for the events then I think is better to delegate the hover responsibility to css. The change is just remove class="hidden" from your elements and then set the right selectors to user display:none to hide or display:inline to show - I use inline instead of block since it is applied to anchor tag and the default behavior for this tag is inline.

With this change then javascript can be simplified and the hover handlers can be removed.

BTW you still have to implement the click handler to add the has-request class.

The code should look likes the following,

In your css:

.rendering.rendering-expert.request-list-supported:hover [rel=request-list-add],
.rendering.rendering-expert.request-list-supported.has-request [rel=request-list-remove]
{
    display: inline
}
.rendering.rendering-expert.request-list-supported [rel=request-list-add],
.rendering.rendering-expert.request-list-supported [rel=request-list-remove],
.rendering.rendering-expert.request-list-supported:hover .fa-cog,
.rendering.rendering-expert.request-list-supported.has-request .fa-cog,
.rendering.rendering-expert.request-list-supported.has-request [rel=request-list-add]
{
    display: none
}

And then replace your js:

$(function() {
    $(".rendering-expert.request-list-supported").on("click", function(e) {
        e.preventDefault();
        var toggler = $(this);
        toggler.toggleClass("has-request");
    });
});


Get data from external API app.js

var employees = require('../models/employees');

employees.read(req.params.id, function(body) {
    console.log(body.firstName);
});

models/employees

var request = require('request');

var employees = {

    read: function(id, callback) {
        request
            .get('http://api.mysite.com/employees/' + id, function(error, response, body) {
                body = JSON.parse(body);
                return callback(body);
            })
    },
};

module.exports = employees;

This works (returns the employee name correctly), but I'm not sure if this is the correct (async) way of getting data from an API and displaying it.

You didn't implement any error handling. Before doing something with the api response body, you should check if an error occured. Also the callback function of your read method should use the error first pattern. Here is a very short explanation: http://www.codingdefined.com/2015/10/what-are-error-first-callbacks-in-nodejs.html

Further, JSON.parse can throw an error if your api doesn't respond with a valid json string. So you might want to handle this error, too. Just add a try-catch block.


Potential quicksort optimizations I implemented qsort() like this. I was wondering what improvements I could do to make it better.

public static void qsort(int[] a, int start, int end) {
    if (start >= end) return;
    int pivot = start;
    int lt = start + 1;
    int gt = end;
    int temp;
    while (lt <= gt) {
        if (a[pivot] > a[lt]) {
            temp = a[pivot];
            a[pivot] = a[lt];
            a[lt] = temp;
            pivot = lt++;
        } else {
            while (gt >= lt)  {
                if (a[gt] < a[lt]) {
                    temp = a[lt];
                    a[lt] = a[gt];
                    a[gt] = temp;
                    break;
                }
                gt--;
            }
            gt--;
        }
    }
    qsort(a, start, pivot - 1);
    qsort(a, pivot + 1, end);
}

One thing I quickly spot is this:
If your second while loop completes naturally instead of via a break, you will also exit the  first while loop. Thus the last instruction executed, gt--, is unnecessary.

Let's look at the execution paths:

//BEGINLOOPS
while (lt <= gt) {
    if (a[pivot] > a[lt]) {
        temp = a[pivot];
        a[pivot] = a[lt];
        a[lt] = temp;
        pivot = lt++;
    } else {
        while (gt >= lt)  {//If false, does gt--;, then jumps to ENDLOOPS
            if (a[gt] < a[lt]) {
                temp = a[lt];
                a[lt] = a[gt];
                a[gt] = temp;
                break;//does gt--;, then jumps to BEGINLOOPS
            }
            gt--;
        }
        gt--;
    }
}
//ENDLOOPS
qsort(a, start, pivot - 1);
qsort(a, pivot + 1, end);

There's only 2 ways the second while loop can end. Either via a break or via gt decrementing until it equalizes with lt.

So we should alter the one execution path where the gt--; is unnecessary.

//BEGINLOOPS
while (lt <= gt) {
    if (a[pivot] > a[lt]) {
        temp = a[pivot];
        a[pivot] = a[lt];
        a[lt] = temp;
        pivot = lt++;
    } else {
        while (gt >= lt)  {//If false, jumps to ENDLOOPS
            if (a[gt] < a[lt]) {
                temp = a[lt];
                a[lt] = a[gt];
                a[gt] = temp;
                gt--;//does gt--;,
                break;//then jumps to BEGINLOOPS
            }
            gt--;
        }
    }
}
//ENDLOOPS
qsort(a, start, pivot - 1);
qsort(a, pivot + 1, end);

That saves you one instruction for each time you end via that one execution path.

Additionally, you know what pivot is already, so I'd split this in a helper function and a recursive function.

public static void quickSort(int[] a, int start, int end) {
    if (start >= end) return;
    qsort(a, start, end);
}

private static void qsort(int[] a, int start, int end) {
    int pivot = start;
    int lt = start + 1;
    int gt = end;
    int temp;
    while (lt <= gt) {
        if (a[pivot] > a[lt]) {
            temp = a[pivot];
            a[pivot] = a[lt];
            a[lt] = temp;
            pivot = lt++;
        } else {
            while (gt >= lt)  {
                if (a[gt] < a[lt]) {
                    temp = a[lt];
                    a[lt] = a[gt];
                    a[gt] = temp;
                    gt--;
                    break;
                }
                gt--;
            }
        }
    }
    pivot--;
    if(start < pivot){
        qsort(a, start, pivot);
    }
    if(lt < end){
        qsort(a, lt, end);
    }
}

Whoa, that's different near the end. Yeah, I rushed ahead. Let me explain.

pivot starts at start. lt starts at start + 1, which is pivot + 1. Furthermore, there's only one place where pivot is changed (pivot = lt++) and there's only one place where lt is changed (also pivot = lt++). Looking at that line tells us it does this:
Set pivot to the value of lt (which by definition is pivot + 1), then up lt by one, making lt to be pivot + 1 again. One wonders whether you even need the different variable - if you wanted to conserve memory, I would strip the unneeded variable.

So with that logic, excepting the moment of which that one line is executed in which they are altered, pivot and lt are always 1 apart. Thus I can use lt as a shorthand for pivot + 1.

Examples, with pivot = 12:

qsort(a, start, pivot - 1);//12 - 1 = 11
qsort(a, pivot + 1, end);//12 + 1 = 13

    pivot--;//lt is still 13, 12-- = 11
    if(start < pivot){//only run if start >= end is false, so only run if start < end is true.
        qsort(a, start, pivot);//11
    }
    if(lt < end){//only run if start < end is true.
        qsort(a, lt, end);//lt is still 13, I changed nothing to it.
    }

You should also consider removing the pivot variable and seeing if that makes a difference.

That said, all performance optimizations should be profiled and benchmarked properly. Who knows what optimizations the JVM will make for you. But if you were a human who executed the code by hand, these optimizations would speed the code up for you.

Looking deeper at it (why not?) shows me one more problem: if you made no swaps, you will still split around your pivot, reiterating the entire array except one element. That's really, really bad. You'll basically have \$T_{end-start}\$ iterations, where \$T\$ is a triangular number. In other words: 100 elements sorted gives you 5050 iterations or so. That's bad.
Let's add a boolean for that.

public static void quickSort(int[] a, int start, int end) {
    if (start >= end) return;
    qsort(a, start, end);
}

private static void qsort(int[] a, int start, int end) {
    int pivot = start;
    int lt = start + 1;
    int gt = end;
    int temp;
    boolean swapped = false;
    while (lt <= gt) {
        if (a[pivot] > a[lt]) {
            temp = a[pivot];
            a[pivot] = a[lt];
            a[lt] = temp;
            pivot = lt++;
            swapped = true;
        } else {
            while (gt >= lt)  {
                if (a[gt] < a[lt]) {
                    temp = a[lt];
                    a[lt] = a[gt];
                    a[gt] = temp;
                    swapped = true;
                    gt--;
                    break;
                }
                gt--;
            }
        }
    }
    if(swapped){
        pivot--;
        if(start < pivot){
            qsort(a, start, pivot);
        }
        if(lt < end){
            qsort(a, lt, end);
        }
    }
}


GtkInfobars for a GTK+ application I instantiate GtkInfobars a lot in my GTK+ application in order to communicate with the user. There are various types of infobars, depending on the message. Basically, any infobar could be a combination of the 4 different infobar message types and 5 different icons (which are painted on the left side of the infobar).

Initially, I would call my custom infobar function like this:

infobar("Message. Woo.", type=gtk.MESSAGE_INFO, icon=gtk.STOCK_DIALOG_WARNING, timeout=5)

After a while I decided I wanted to simplify all the creation calls... so I modified my infobar function so that I could do this:

infobar("Message. Woo.", type=(1,3), timeout=3)

I feel like the second way is better and that it's worth the code obfuscation, but I suspect not everyone will agree with me.

def infobar(self, msg=None, type=(1,1), timeout=3, vbox=None):
    """Popup a new auto-hiding InfoBar."""

    # List of possible infobar message types
    msgtypes = [0,
                gtk.MESSAGE_INFO,      # 1
                gtk.MESSAGE_QUESTION,  # 2
                gtk.MESSAGE_WARNING,   # 3
                gtk.MESSAGE_ERROR]     # 4

    # List of possible images to show in infobar
    imgtypes = [gtk.STOCK_APPLY,            # 0
                gtk.STOCK_DIALOG_INFO,      # 1
                gtk.STOCK_DIALOG_QUESTION,  # 2
                gtk.STOCK_DIALOG_WARNING,   # 3
                gtk.STOCK_DIALOG_ERROR]     # 4

    ibar                    = gtk.InfoBar()
    ibar.set_message_type   (msgtypes[type[0]])
    if vbox:
        # If specific vbox requested: assume ibar for filemode, add cancel button
        ibar.add_button     (gtk.STOCK_CANCEL, gtk.RESPONSE_CANCEL)
        ibar.connect        ('response', self.cleanup_filemode)
    else:
        # If no specific vbox requested: do normal ibar at the top of message area
        vbox = self.vbox_ibar
        ibar.add_button     (gtk.STOCK_OK, gtk.RESPONSE_OK)
        ibar.connect        ('response', lambda *args: ibar.destroy())
    vbox.pack_end           (ibar, False, False)
    content                 = ibar.get_content_area()
    img                     = gtk.Image()
    img.set_from_stock      (imgtypes[type[1]], gtk.ICON_SIZE_LARGE_TOOLBAR)
    content.pack_start      (img, False, False)
    img.show                ()
    if msg:
        # If msg was specified, show it, but change the default color
        label               = gtk.Label()
        label.set_markup    ("{}".format(msg))
        content.pack_start  (label, False, False)
        label.show          ()
    # FIXME: Why doesn't Esc trigger this close signal?
    ibar.connect            ('close', lambda *args: ibar.destroy())
    ibar.show()
    if timeout:
        glib.timeout_add_seconds(timeout, ibar.destroy)
    return ibar

I'd move all of those message into an external resource file something like:

<message id="file.open.fail" type="warning" icon="error" delay="5">
    Error. Could not open file:<tt>{filename}</tt>
</message>

Then I'd simply pop the message using:

self.infobar('file.open.fail', filename = filename)

That should cleanup the calls to infobar, and make it easier to do things like provide strings for a different language.

EDIT:

Quick hack of an implementation

from xml.etree.ElementTree import XML, tostring

# of course, you'd want to load this from an external file
message_file = XML("""
<messages>
    <message id="file.open.fail" type="warning" icon="error" delay="5">
        Error. Could not open file:<tt>{filename}</tt>
    </message>
    <message id="file.open.good" type="warning" icon="error" delay="5">
        File opened!
    </message>
</messages>
""")

messages = {}
for message in message_file.findall("message"):
    messages[message.attrib['id']] = message

def message(id, **kwargs):
    message_detail = messages[id]
    text = ''.join( tostring(node) for node in message_detail)
    return text.strip().format(**kwargs)

print message('file.open.fail', filename = 'file.txt')
print message('file.open.good', filename = 'file.txt')

Doesn't try to handle type/icon/delay. You'll probably also want to consider how you want to do whitespace.


Placing navigation items into an array I am looking for better ways to write this code. At the moment it grabs a list of all navigation items and places them into an array (using PDO).

However, as you can see, it is using two foreach loops to grab the subnavigation items under each parent. Is there a better way that this code can be written?

    <?php
    if($grabNav = $db->prepare("SELECT caption,url,visibility,id,class,parent_id FROM navigation ORDER BY parent_id ASC, order_id ASC"))
    {
        $grabNav->execute();
        $grabNav = $grabNav->fetchAll();
        foreach($grabNav as $nav)
        {
            $visibility = true;
            switch($nav['visibility'])
            {
                default:
                case 0: $visibility = false; break;
                case 1: $visibility = true; break;
                case 2: if(LOGGED_IN && isset($cUser)) { $visibility = true; } else { $visibility = false; } break;
                case 3: if(LOGGED_IN && isset($cUser)) { $visibility = false; } else { $visibility = true; } break;
            }
            if(!$visibility) { continue; }
            if($nav['parent_id'] != -1) { continue; }

            $class = $core->output($nav['class']);
            if($nav['id'] == PAGE_ID) { $class .= ' selected'; }
            echo '
output($nav['url']).'">'.$core->output($nav['caption'],true).'
';
            foreach($grabNav as $sub)
            {
                if($sub['parent_id'] == $nav['id'])
                {
                    $visibility = true;
                    switch($sub['visibility'])
                    {
                        default:
                        case 0: $visibility = false; break;
                        case 1: $visibility = true; break;
                        case 2: if(LOGGED_IN && isset($cUser)) { $visibility = true; } else { $visibility = false; } break;
                        case 3: if(LOGGED_IN && isset($cUser)) { $visibility = false; } else { $visibility = true; } break;
                    }
                    if(!$visibility) { continue; }
                    $subClass = $core->output($sub['class']);
                    echo'
output($sub['url']).'">'.$core->output($sub['caption'],true).'
';
                }
            }
            echo'

';
        }
    }
    ?>

Return Early

If you extract the PHP code to a function, you can return early, and thus save a level of nesting:

<ul id="nav">
<?php printNavigation(); ?>
</ul>

<?php
function printNavigation() {
    if(!($grabNav = $db->prepare("SELECT caption,url,visibility,id,class,parent_id FROM navigation ORDER BY parent_id ASC, order_id ASC"))) {
        return;
    }
    [...]
    ?>
}

Remove Duplication and further simplifications

Currently, your visibility check and your one long echo line are duplicates. You can extract these to functions:

function isVisible($visibilityLevel, $cUser) {
    switch($visibilityLevel, $cUser)
    {
        case 0: return false;
        case 1: return true;
        case 2: return LOGGED_IN && isset($cUser);
        case 3: return !LOGGED_IN || !isset($cUser);
        default: throw new Exception('invalid visibility level.');
    }
}

And then use it as

    foreach($grabNav as $nav)
    {
        if(!isVisible($nav['visibility'], $cUser) || $nav['parent_id'] != -1) { continue; }
        [...]

Note also that I removed the useless default from the beginning of the switch, added it again at the end, and threw an exception for invalid values.

I also simplified cases 2 and 3 (if (cond) true else false can be phrased as cond), and I combined the two continue conditions.


Image carousel with jQuery 3.3.1 I'm trying to make a carousel with jQuery and I want to know, how to simplify the code further? so... this is my code:

$(document).ready(function () {
    $(".cont-btn button").click(function (event) {
        var detectId = event.target.id;

        $(".cont-btn button").removeClass("active");
        $(this).addClass("active");

        for (i = 1; i <= 4; i++) {
            if (detectId === ("btn" + i) && i === 1) {
                $(".cont-img div").css("left", "0%");
            } else if (detectId === ("btn" + i)) {
                $(".cont-img div").css("left", "-" + (i - 1) + "00%");
            }
        }
    });
});
body {
    width: 100%;
    margin: 0;
    padding: 0;
}

.carousel {
    width: 100%;
    height: 150px;
    position: relative;
    overflow: hidden;
}

.carousel .cont-img {
    width: 100%;
    height: 100%;
}

.carousel .cont-img div {
    width: 100%;
    height: 100%;
    position: absolute;
    left: 0;
    background-color: aqua;
    transition: left .75s ease-in-out;
}

.carousel .cont-img div:nth-of-type(2) {
    background-color: aquamarine;
    margin-left: 100%;
}

.carousel .cont-img div:nth-of-type(3) {
    background-color: blueviolet;
    margin-left: 200%;
}

.carousel .cont-img div:nth-of-type(4) {
    background-color: cornflowerblue;
    margin-left: 300%;
}

.carousel .cont-btn {
    position: absolute;
    bottom: 18px;
    left: 50%;
    transform: translateX(-50%);
    cursor: default;
}

.carousel .cont-btn button {
    width: 18px;
    height: 18px;
    background-color: transparent;
    border: 2px solid #000;
    border-radius: 100%;
    margin: 0 5px;
    padding: 0;
    outline: none;
    transition: background-color .25s ease-in-out;
    cursor: pointer;
}

.carousel .cont-btn .active {
    background-color: #000;
}

        <button type="button" id="btn1" class="active"></button>
        <button type="button" id="btn2"></button>
        <button type="button" id="btn3"></button>
        <button type="button" id="btn4"></button>

Thank you very much in advance to anyone who can help me.

I like it.

There are only a few things I would reconsider:

detectedId is a clunky name, clickedId or selectedId seems better
You hardcode 4, at least use a constant, ideally detect how many buttons there dynamically
The treatment for the first button is different enough that it can be placed outside of the loop
It seems you only need to know which button was clicked, which can be found by analyzing the selected id

if(selectedId === "btn1"){
  $(".cont-img div").css("left", "0%");
}else{
  let id = selectedId.split('btn')[1]*1;
  $(".cont-img div").css("left", "-" + (id - 1) + "00%");
}

This way you dont even need to hardcode 4


join method inside axios call I am doing a join in the axios call.
since doing two different api calls.
for performance is this the good way.
or do I need to use async await and then do join.
providing code snippet below

export function getSearch(value) {
    console.log('-- getSearch value', value);
    return dispatch => {
        if (value.length) {
            if (typeof this._source !== 'undefined') {
                this._source.cancel('Operation canceled due to new request');
            }
            this._source = axios.CancelToken.source();
            return axios
                .get(
                    `http://localhost:787878/hold/search?mode=Smart&value=${value}`,
                    {
                        cancelToken: this._source.token,
                    }
                )
                .then(response => {

                    console.log('search response.data-->', response.data);
                    let values = response.data.map(filterSearch => {
                        if (filterSearch.playerIDs) {
                            console.log(
                                'search response.data playerIDs',
                                filterSearch.playerIDs[0]
                            );
                            return filterSearch.playerIDs[0].number + ',';
                        }

                    });
                    console.log(
                        'search response.data join--->',
                        values.join('')
                    );
                    axios
                        .get(
                            `http://localhost:787878/hold/animalworldCardList/?playerID=${values.join(
                                ''
                            )}&isHistory=false`,
                            //localhost:787878/hold/ce-affiliation/?animalPimsId=${animalPimsId}&isHistory=${isHistory}`,
                            {
                                cancelToken: this._source.token,
                            }
                        )
                        .then(responseplayerIDs => {
                            console.log(
                                'search response.data -->',
                                responseplayerIDs
                            );
                            dispatch({
                                type: ANIMAL_SEARCH_DATA,
                                payload: {
                                    animalSearch: response.data,
                                    playerIDs:
                                        responseplayerIDs.data.animalsInfo,
                                },
                            });
                            //  callBack(response);
                        })
                        .catch(error => {
                            if (axios.isCancel(error)) {
                                console.log(
                                    '-- Request canceled',
                                    error.message
                                );
                            } else {
                                console.log('-- Error', error.message);
                            }
                        });
                })
                .catch(error => {
                    if (axios.isCancel(error)) {
                        console.log('-- Request canceled', error.message);
                    } else {
                        console.log('-- Error', error.message);
                    }
                });
        } else {
            dispatch({
                type: ANIMAL_SEARCH_DATA,
                payload: [],
            });
        }
    };
}

Using Array.join is unrelated to usage of async and await. Also note that the time it takes to call Array.join is insignificant in comparison to the time it takes to make the two API calls.

You can consider call1().then(data => call2(data)) roughly equivalent to const data = await call1(); call2(data). There's no (obvious) performance gain to be made here by using async/await over then. You would have a slight readability gain though. Note that your second call is dependent on your first call. When this is not the case you can get a performance improvement by making both calls simultaneously like so Promise.All([call1(), call2()]). That is not applicable here, but maybe this was what you had in mind?

The vast majority of the time here is spent waiting for the API calls. If you have ownership of the back end and want to improve performance, then I would suggest you consider creating a new endpoint that better fits your need so you only need to make one API call.

Also I prefer this._source !== undefined over typeof this._source !== 'undefined', but that's only if you're sure that this._source is declared.

In general I think this code looks fine.


Gravitational Brute Force N-body Algorithm I've just started dabbling with code, and as a learning exercise I've written a simple algorithm for solving gravitational n-body problems numerically in JavaScript.

I would be very grateful for general feedback as far as the quality of the code goes (first time I'm asking somebody to review my code; I'm very shy about it!), and more specifically advise on how I could optimize it as when I implement the algorithm in THREE.js in the shape of a simulation of the solar system the simulation starts lagging when I include more than around 70 bodies in the simulation. I know n-body algorithms are computationally intensive by nature, especially those of the brute force variety, but I'm pretty sure I've missed out on some useful tricks that would push it a bit further.

Update: to see a working example of the code, check out the jsfiddle I have thrown together https://jsfiddle.net/e9t88dwh

function nBodyProblem(parameters) {

    this.g = parameters.g; //Gravitational constant
    this.law = parameters.law; //Force law, inverse-square in our universe
this.dt = parameters.dt; //Time step to be used
    this.masses = parameters.masses; //Object array with initial conditions for the bodiess to be simulated

}

nBodyProblem.prototype = {

constructor: nBodyProblem,

getDistance: function (m1, m2) {

    return Math.pow((m1.x - m2.x) * (m1.x - m2.x) + (m1.y - m2.y) * (m1.y - m2.y) + (m1.z - m2.z) * (m1.z - m2.z), this.law);

    },

    updatePositionVectors: function () {

        for (var i = 0, len = this.masses.length; i < len; i++) {

            this.masses[i].x += this.masses[i].vx * this.dt;
            this.masses[i].y += this.masses[i].vy * this.dt;
            this.masses[i].z += this.masses[i].vz * this.dt;
        }

        return this;

    },

    updateVelocityVectors: function () {

        var forceVectorX = 0;
        var forceVectorY = 0;
        var forceVectorZ = 0;

        for (var i = 0, ilen = this.masses.length; i < ilen; i++) {

            for (var j = 0, jlen = this.masses.length; j < jlen; j++) {

                //We don't want to calculate self gravity!!!!!

                if (i !== j) {

                    forceVectorX += (this.g * this.masses[j].m) * (this.masses[j].x - this.masses[i].x) / this.getDistance(this.masses[i], this.masses[j]);
                    forceVectorY += (this.g * this.masses[j].m) * (this.masses[j].y - this.masses[i].y) / this.getDistance(this.masses[i], this.masses[j]);
                    forceVectorZ += (this.g * this.masses[j].m) * (this.masses[j].z - this.masses[i].z) / this.getDistance(this.masses[i], this.masses[j]);

                }

            }

            this.masses[i].vx += forceVectorX * this.dt;
            this.masses[i].vy += forceVectorY * this.dt;
            this.masses[i].vz += forceVectorZ * this.dt;

            forceVectorX = 0;
            forceVectorY = 0;
            forceVectorZ = 0;

        }

        return this;

    }

};

you did something in updateVelocityVectors that I couldn't quite figure out at first

updateVelocityVectors: function () {

    var forceVectorX = 0;
    var forceVectorY = 0;
    var forceVectorZ = 0;

    for (var i = 0, ilen = this.masses.length; i < ilen; i++) {

        for (var j = 0, jlen = this.masses.length; j < jlen; j++) {

            //We don't want to calculate self gravity!!!!!

            if (i !== j) {

                forceVectorX += (this.g * this.masses[j].m) * (this.masses[j].x - this.masses[i].x) / this.getDistance(this.masses[i], this.masses[j]);
                forceVectorY += (this.g * this.masses[j].m) * (this.masses[j].y - this.masses[i].y) / this.getDistance(this.masses[i], this.masses[j]);
                forceVectorZ += (this.g * this.masses[j].m) * (this.masses[j].z - this.masses[i].z) / this.getDistance(this.masses[i], this.masses[j]);

            }

        }

        this.masses[i].vx += forceVectorX * this.dt;
        this.masses[i].vy += forceVectorY * this.dt;
        this.masses[i].vz += forceVectorZ * this.dt;

        forceVectorX = 0;
        forceVectorY = 0;
        forceVectorZ = 0;

    }

    return this;

}

in both of the for loops you added an extra variable to hold the length of  this.masses, but never use it anywhere else in the loop. I think that you shouldn't declare the variable at all and just use this.masses.length in the declaration of both for loops like this

for (var j = 0, j < this.masses.length; j++) {

or an even better idea would be to assign the value to a variable outside of both loops so that you only call the length property of this.masses once, like this.

var massesLength = this.masses.length;
for (var i = 0; i < massesLength; i++) {
    for (var j = 0; j < massesLength; j++) {
    ...

Same thing here as well

updatePositionVectors: function () {

    for (var i = 0, len = this.masses.length; i < len; i++) {

        this.masses[i].x += this.masses[i].vx * this.dt;
        this.masses[i].y += this.masses[i].vy * this.dt;
        this.masses[i].z += this.masses[i].vz * this.dt;
    }

    return this;

},

and I would remove some of the vertical white space as well.

updatePositionVectors: function () {
    var massesLength = this.masses.length;
    for (var i = 0; i < massesLength; i++) {
        this.masses[i].x += this.masses[i].vx * this.dt;
        this.masses[i].y += this.masses[i].vy * this.dt;
        this.masses[i].z += this.masses[i].vz * this.dt;
    }
    return this;
},

@Ismael Miguel  is correct, by using the length property inside the for declaration you would access that property every loop, which would make this loop very inefficient, so create the variable outside of the for loop.


Shell Script (Bash) to sum multiples of different arguments below a number I'm interested in learning how to create shell scripts with bash so here's one of my first exercises, taken from here. It is an extension of the problems Elementary - 4 and 5.

The goal of the program is to sum all multiples or ARG1 or ARG2 below the user's input number.

#!/bin/bash
echo "This program will find all the multiples of ARG1 or ARG2 below or equal to your number and sum them up. Please input your limit:"
read limit

commonMultiple=`expr $1 \* $2`

sumArg1=0
sumArg2=0
sumCommonMultiple=0

for (( i = 0; i <= $limit; i+=$1 )); do
    sumArg1=`expr $sumArg1 + $i`
done
for (( i = 0; i <= $limit; i+=$2 )); do
    sumArg2=`expr $sumArg2 + $i`
done
for (( i = 0; i <= $limit; i+=$commonMultiple )); do
    sumCommonMultiple=`expr $sumCommonMultiple + $i`
done

echo "The result is `expr $sumArg1 + $sumArg2 - $sumCommonMultiple`."

This works perfectly but I'd like the input of more experienced programmers. I don't think my logic is efficient. There must be something obvious I'm missing that could have made my code a lot simpler.

Disclaimer: apparently some of the stuff I used is outdated. I'll take it into account for my future code.*

Arithmetic operations

If you look closely, something is a bit odd here:

for (( i = 0; i <= $limit; i+=$1 )); do

Ideas? How about: i is written without a dollar, but limit is written with a dollar... In fact you can drop the dollar from limit too, and the expression will be slightly simpler. From $1 you cannot drop it, as the meaning of 1 would be taken literally, because it's numeric, unlike the others.

The same is true for commonMultiple here, you can drop the dollar:

for (( i = 0; i <= $limit; i+=$commonMultiple )); do

The general rule is, in arithmetic operations within ((...)) you don't need to use the dollars in variable names.

Don't use backticks

Instead of this:

commonMultiple=`expr $1 \* $2`

This is the recommended writing style for sub shells:

commonMultiple=$(expr $1 \* $2)

Don't use expr

It's better to avoid expr, and you can do the same thing easier using Bash arithmetics, for example:

((commonMultiple = $1 * $2))

The for loops can be simplified similarly, instead of:

for (( i = 0; i <= $limit; i+=$1 )); do
    sumArg1=`expr $sumArg1 + $i`
done

You can write:

for (( i = 0; i <= limit; i+=$1 )); do
    ((sumArg1 += i))
done

Apply the same thing everywhere.


Error handling for Stripe credit card processing I have a rails application that uses Stripe for credit card processing. Currently several exceptions are being handled anywhere a transaction takes place. The code below is being used currently in 11 places. In all places it is essentially the same with the exception of the message being passed to stripe_error. Is it possible to create a reusable piece of code that can be used in all these places?

begin
  charge = Stripe::Charge.retrieve(@purchase.stripe_charge_id)
  resp = charge.capture(amount: fee.cancellation_fee,
                        receipt_email: @customer.email)
  @purchase.update_attributes(
    accepted_charge: true,
    stripe_transaction_id: resp['balance_transaction']
  )
  logger.info("Charged customer for cancellation #{@customer.id} #{@purchase.total_fee}")
  event('cancellation_charge', 'purchase_id', @purchase.id,
        fee: @purchase.total_fee, customer_id: @purchase.customer_id,
        provider_id: @purchase.provider_id)
  purchase_set_md5(@purchase.id)
rescue Stripe::CardError => e
  @err = stripe_error(e, 'cancellation', fee.cancellation_fee)
  @purchase.update_attributes(
    accepted_charge: false,
    charge_errors: message,
    stripe_transaction_id: '',
    status: "Cancellation fee charge did not go through #{@err['message']}"
  )
  purchase_set_md5(@purchase.id)
  render :err_show, status: :payment_required, json: @err
  return
rescue Stripe::InvalidRequestError => e
  @err = stripe_error(e, 'cancellation_invalid_request', fee.cancellation_fee)
  render :err_show, status: :payment_required, json: @err
  return
rescue Stripe::AuthenticationError => e
  @err = stripe_error(e, 'cancellation_authentication', fee.cancellation_fee)
  render :err_show, status: :payment_required, json: @err
  return
rescue Stripe::APIConnectionError => e
  @err = stripe_error(e, 'cancellation_api_connect', fee.cancellation_fee)
  render :err_show, status: :payment_required, json: @err
  return
rescue Stripe::StripeError => e
  @err = stripe_error(e, 'cancellation_generic error', fee.cancellation_fee)
  render :err_show, status: :payment_required, json: @err
  return
rescue => e
  @err = stripe_error(e, 'cancellation_something_else', fee.cancellation_fee)
  render :err_show, status: :payment_required, json: @err
  return
end

stripe_error

  def stripe_error(e, charge_type, fee = 0)
    logger.error("e #{e}")
    err = {
      charge_type: charge_type,
      errcode: ERR_STRIPE_ERR,
      message: "#{e}"
    }
    event(charge_type, 'error', "#{e}")
    logger.error("Charging error on #{charge_type} purchase for #{fee} ")
    logger.error("type #{e} ")
    err
  end

Your first step would be to separate out actually rendering the error message from rescueing the exceptions:

def render_errors(errors, template: :err_show,  status: :payment_required)
  render template, status: status, json: errors
end

Then can you group the majority of the exceptions:

rescue Stripe::InvalidRequestError,
       Stripe::AuthenticationError
       Stripe::APIConnectionError,
       Stripe::StripeError => e

render_errors( stripe_error(e) )

A good idea would be then to separate generating the errors and logging them.


Sorting 10,000 unique randomly-generated numbers I have to write this program in java.

  Write a program name sorting.java that will use an array to store 10,000 randomly generated numbers (ranging from 1 to 10,000 no repeat number).

Here is what I have so far:

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

        Random rgen = new Random();  // Random number generator
        int[] nums = new int[10,000];  //array to store 10000 random integers (1-10,000)

        //--- Initialize the array to the ints 1-10,000
        for (int i=0; i<nums.length; i++) {
            nums[i] = i;
        }

        //--- Shuffle by exchanging each element randomly
        for (int i=0; i<nums.length; i++) {
            int randomPosition = rgen.nextInt(nums.length);
            int temp = nums[i];
            nums[i] = nums[randomPosition];
            nums[randomPosition] = temp;
        }

        //Print results
        for (int i = 0; i < nums.length; i++){
        System.out.println(nums[i]);
        System.out.println("\n");
    }
}

Apart from the compiler errors, implementation issues and code style suggestions others have pointed out, your shuffling algorithm is fundamentally flawed.

Wikipedia explains it nicely:

  Similarly, always selecting i from the entire range of valid array
  indices on every iteration also produces a result which is biased,
  albeit less obviously so. This can be seen from the fact that doing so
  yields nn distinct possible sequences of swaps, whereas there are only
  n! possible permutations of an n-element array. Since nn can never be
  evenly divisible by n! when n > 2 (as the latter is divisible by n−1,
  which shares no prime factors with n), some permutations must be
  produced by more of the nn sequences of swaps than others. As a
  concrete example of this bias, observe the distribution of possible
  outcomes of shuffling a three-element array [1, 2, 3]. There are 6
  possible permutations of this array (3! = 6), but the algorithm
  produces 27 possible shuffles (33 = 27). In this case, [1, 2, 3], [3,
  1, 2], and [3, 2, 1] each result from 4 of the 27 shuffles, while each
  of the remaining 3 permutations occurs in 5 of the 27 shuffles.

To demonstrate this bias, I changed your code to only create three elements per array and ran it sixty million times. Here are the results:

Permutation    Occurences
[1, 2, 3]:     8884128
[2, 3, 1]:     11111352
[3, 1, 2]:     8895318
[3, 2, 1]:     8891062
[2, 1, 3]:     11107744
[1, 3, 2]:     11110396

If your shuffling algorithm were correct, one would expect a relatively uniform distribution. However, the standard deviation is huge at about 1215764 (or ~2%) which should ring alarm bells. For comparison, here are the results of using the proven Fisher–Yates shuffle:

Permutation    Occurences
[1, 2, 3]:     10000566
[2, 3, 1]:     9998971
[3, 1, 2]:     10000640
[3, 2, 1]:     10000873
[2, 1, 3]:     9998249
[1, 3, 2]:     10000701

As one would expect from a correct implementation, the standard deviation is low at about 1105 (or ~0.002%).

Here's the correct implementation for reference:

for (int i = numbers.length - 1; i > 0; i--)
{
    int swapIndex = random.nextInt(i + 1);
    int temp = numbers[i];
    numbers[i] = numbers[swapIndex];
    numbers[swapIndex] = temp;
}

However, another problem presents itself even with a correct shuffling algorithm:

A pseudo-random number generator is limited by its period, i.e. it can only produce a certain number of unique shuffles:

  [...] a shuffle driven by such a generator cannot possibly produce more distinct permutations than the generator has distinct possible states.

java.util.Random has a period no larger than 248 which is unable to produce an overwhelming majority of the 10000! (approximately 2.85 × 1035659) possible permutations of your array. The default implementation of SecureRandom isn't much better at no more than 2160.

In the case of such a long array, the Mersenne Twister is a more adequate choice with a period of 219937-1 and excellently uniform distribution (although still not enough to produce all the possible permutations. At some point, it makes more sense to look into true random number generators that are based on physical phenomena).

So, in my opinion, the real moral of the story is this:

  Take special care when working with randomness or pseudorandomness as the consequences of tiny mistakes can be hard to detect but devastating. Use Collections.shuffle instead of reinventing the wheel. For your (presumably) casual use, you might not need to worry about these inadequacies at all. On the other hand, it doesn't hurt to be aware of them.


Making a reservation using Selenium/PhantomJS I have this small script that needs to make a reservation at a DMV office.

The problem is that the reservation window opens only every few hours for a few minutes (it might show up in one of these months: December, January, February or March of next year). My code is too slow to even "catch" the "open day".

Any ideas on how I can speed up the whole process? Maybe a different approach?

from selenium import webdriver
import time

firstName = "John"
lastName = "Smith"
yob = "1929"
email = "john.smith29@yolol.com"
phone = "555-123-4567"

browser = webdriver.PhantomJS()
browser.get("https://page/page2/ACFCustom/Se.aspx")

def check_date_avaialbility():
    # Choose a service
    browser.find_element_by_name("ct$cphBy$rS$ctl$btn1").click()
    # Choose an office
    browser.find_element_by_name("cphBy_rpt_btn1").click()

    # Loop through the next 4 months and search for an open window (The day will be highlighted in green)
    for i in range(1, 5):
        try:
            print("Searching for available day... ")
            browser.find_element_by_xpath("//*[contains(@style, 'background-color:#4A0889;')]").click()
            browser.find_element_by_xpath("//*[@id='cphBy_rblAT']").click()
            browser.find_element_by_id("cphBy_Nt").click()
            form_data()
        except:
            print("There are no available dates. Next page... \n")
            browser.find_element_by_name("ctl_cphBy_rdcAD_NN").click()
            time.sleep(1)

    # If you couldn't find anything three. Go back to the "home" page
    browser.find_element_by_name("cphBy_lnkbnK").click()
    time.sleep(1)
    # Start going through all the next 4 months all over again
    check_date_avaialbility()

def form_data():
    print("Filling up the form...")
    browser.find_element_by_id("cphBy_txbFN").send_keys(firstName)
    browser.find_element_by_id("cphBy_txbLN").send_keys(lastName)
    browser.find_element_by_id("cphBy_txbYB").send_keys(yob)
    browser.find_element_by_id("cphBy_txbE").send_keys(email)
    browser.find_element_by_id("cphBy_txbPN").send_keys(phone)
    browser.find_element_by_id("cphBy_S").click()

check_date_avaialbility()

You can try approaching the problem without using an actual browser and sending HTTP requests using requests and parsing HTML using, say, BeautifulSoup. This though would involve diving into how the page is formed, is more error-prone and would be far more suspicious and easier to detect in general. Of course, with no browser overhead, this would be much faster.

There are some other relevant tools like the Scrapy web-scraping framework. Which, if needed, can be combined with a JavaScript engine - see scrapy-splash.

There are though some things you can try to speed things up in your current approach:

remove time.sleep() hardcoded delays and switch to using Explicit Waits based on WebDriverWait and Expected Conditions
avoid using XPaths as it is generally the slowest element location technique (of course, that's not a bottleneck here)
you can pass certain flags to PhantomJS to help it load pages faster - like --load-images
you can try switching to headless Chrome instead of PhantomJS - I've seen benchmarks showing that the headless Chrome beats PhantomJS in terms of both speed and memory usage


How many chores can you accomplish in a set time? I am learning java and doing some practice problems. My code works, but I feel like I used a workaround sort of way to accomplish it. Is there another way to express what I want the program to do?

Question: Problem J4: Time on task

  Input specification:

    First line is the total number of minutes to accomplish the chores.

    Second line is number of chores to choose from

    The next 'n' lines are how long each chore takes to complete.

  Output specification:

    The total amount of chores one can do given the amount of time

My code:

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

public class Time {

public static void main(String args[]) {
    try {
        BufferedReader input = new BufferedReader(new InputStreamReader(
                System.in));

        //get the amount of time
        int maxMins = Integer.parseInt(input.readLine());
        //get the total amount of chores
        int totalChores = Integer.parseInt(input.readLine());
        //create an array to store each chore and how long it takes to complete it
        int[] list = new int[totalChores];

        for (int i = 0; i < totalChores; i++) {
            list[i] = Integer.parseInt(input.readLine());
        }
        Arrays.sort(list);

        //initialize counters, one for minutes and the other for chores done
        int totalMins = 0;
        int chores = 0;

        //I don't like the below part, not quite sure how to change it
        for (int j = 0; totalMins < maxMins; j++) {
            totalMins = totalMins + list[j];
            if (totalMins <= maxMins) {
                chores++;
            }
        }

        System.out.println(chores);

    } catch (IOException e) {
        e.printStackTrace();
    }
}

}

Formatting: Indent after opening { and unindent after closing }.

Variable naming: A better name for list would be choreList.

When iterating through an array use an enhanced-for loop:

for (int chore:list) {
        chore = Integer.parseInt(input.readLine());
}

Your algorithm for computing the maximum amount of chores that can be accomplished could be simplified as follows:

for (int chore:list){
    maxMins -= chore;
    if (maxMins > 0){
        chores++;
    }
    else {
        break;
    }
}


Beginner's Implementation of Guess The Word As the title suggests, I'm a complete beginner in Haskell. I've just made my way through the "Input and Output" section of Learn You A Haskell For Greater Good and I thought it would be good to try my hand at a simple project like a "Guess The Word" game in Haskell.

The idea behind the game is to guess all the letters of a randomly selected word. If you guess all the letters, you win.

Without further ado, the complete program is below. I'm looking for feedback regarding program structure (particularly in separating I/O from pure functions), any inefficiencies I could look out for, any potential bugs, and any Haskell idioms and general best practices I could incorporate.

import Data.Char
import Data.Maybe
import System.Random

data Difficulty = Easy | Medium | Hard
    deriving (Read, Show, Eq)

createDifficulty :: String -> Maybe Difficulty
createDifficulty s = lookup s [
    ("e", Easy),
    ("m", Medium),
    ("h", Hard),
    ("", Easy)
    ]

isDifficulty :: String -> Bool
isDifficulty = isJust . createDifficulty

randomNumber :: Int -> Int -> IO (Int)
randomNumber begin end = do
    gen <- getStdGen
    return $ head $ randomRs (begin, end) gen

-- Please don't provide review about these tables. They're just meant to be demonstrative of the general idea I'm going for.
fetchWords :: Difficulty -> [String]
fetchWords Easy   = ["foo", "bar"]
fetchWords Medium = ["abc", "def"]
fetchWords Hard   = ["foobar", "whoami"]

generateSecret :: Difficulty -> IO (String)
generateSecret difficulty = do
    let table = fetchWords difficulty

    num <- randomNumber 0 (length table)

    return (table !! num)

replaceChar :: Int -> String -> Char -> String
replaceChar n s c = pre ++ [c] ++ tail post
    where (pre, post) = splitAt n s

compareSecret :: String -> String -> Int -> Char -> String
compareSecret secret guess pos c =
    if secret !! pos == c then
        replaceChar pos guess c
    else
        guess

isInt :: String -> Bool
isInt = all isDigit

isChar :: String -> Bool
isChar []     = False
isChar (c:cs) = isAlpha c && null cs

promptUntil :: (String -> Bool) -> String -> (String -> IO ()) -> IO (String)
promptUntil pred prompt failure = do
    putStr (prompt ++ "\n> ")
    input <- getLine
    if pred input then
        return input
    else do
        failure input
        promptUntil pred prompt failure

loop :: String -> String -> IO ()
loop secret guess = do

    putStrLn guess

    input1 <- promptUntil isInt "Enter position:" (\input -> putStrLn $ input ++ " is not a number.")
    input2 <- promptUntil isChar "Enter character:" (\input -> putStrLn $ input ++ " is not a character.")

    let pos = read input1 :: Int
    let c   = head input2

    let result = compareSecret secret guess pos c
    if result /= secret then
        loop secret result
    else
        return ()

main = do
    putStrLn "\n Welcome to Guess The Word!\n"

    difficultyString <- promptUntil isDifficulty "Enter difficulty level [e/m/h] or leave empty for easy difficulty: "
        (\input -> putStrLn $ input ++ " is not a valid difficulty.")

    let difficulty = fromMaybe (Easy) (createDifficulty difficultyString)

    putStrLn $ show difficulty ++ " difficulty selected."

    secret <- generateSecret difficulty

    loop secret $ replicate (length secret) '-'

    putStrLn secret
    putStrLn "You win!"

Overall the code looks really nice. I like the separation with a separate Difficulty type instead of doing the parsing directly in fetchWords.
Bugs
I have found three bugs:
Crash on invalid user input
If a user enters a number that is larger than the number of characters in the string, the program will crash. This is because you use the partial1 function (!!) without verifying its precondition that for xs !! i, i >= 0 and i < length xs. This could also be solved if you used a non-partial version of the function which returns a Maybe value so you can display a better error to the user and ask them to retry.
Random crash every 3:rd execution
There is also an off-by-one error in generateSecret, since randomRs generates an inclusive range and length xs is not a valid index for xs. This will cause the program to crash randomly on average once every length (fetchWords difficulty) + 1 times you play.
Only one random number
Speaking of randomness: the randomNumber function calls getStdGen, which
doesn't update the RNG state, it just fetches it. This means that all random numbers will be the same. To make it update the state, you can either use randomRIO which will update the global RNG state automatically, or use newStdGen which will split the RNG state to allow you to generate random numbers from pure code. In this code you don't notice it though, since you only generate a single number.
The randomNumber function can be fixed by simplifying it to
randomNumber :: Int -> Int -> IO Int
randomNumber begin end = randomRIO (begin, end)

Idiomatic code
Partial functions
You use partial functions in a couple of places. Many of them can be avoided by using the principle "Parse, don't validate". In this case, all the "isX" functions can be replaced with getX functions that try to parse the data and Maybe returns a parsed value. The createDifficulty function is already such a function. This guarantees that the validation actually matches the code that uses it.
Here's an example of how it could be done. I have only included the relevant functions here. The index is validated, but there is no easy way to tell which list it is a valid index for unless you use some advanced trickery2.
import Text.Read (readMaybe)

newtype ValidIndex = ValidIndex Int

createDifficulty :: String -> Maybe Difficulty
createDifficulty s = lookup s [
    ("e", Easy),
    ("m", Medium),
    ("h", Hard),
    ("", Easy)
    ]

getInt :: String -> Maybe Int
getInt = readMaybe

isValidIndex :: [a] -> Int -> Bool
isValidIndex list idx = idx >= 0 && idx < length list

getValidIndex :: [a] -> String -> Maybe ValidIndex
getValidIndex list str = do
  idx <- getInt
  guard $ isValidIndex list idx
  return $ ValidIndex idx

getChar :: String -> Maybe Char
getChar [c] | isAlpha c = Just c
getChar []  = Nothing

promptUntil :: (String -> Maybe a) -> String -> String -> IO a
promptUntil parser prompt failureMessage = do
    putStr (prompt ++ "\n> ")
    input <- getLine
    case parser input of
      Just result -> return result
      Nothing -> do
        putStrLn $ input ++ failureMessage
        promptUntil parser prompt failure

loop :: String -> String -> IO ()
loop secret guess = do

    putStrLn guess

    pos <- promptUntil (getValidIndex secret) "Enter position:" " is not a valid index."
    c <- promptUntil getChar "Enter character:" " is not a character."

    let result = compareSecret secret guess pos c
    if result /= secret then
        loop secret result
    else
        return ()

main = do
    putStrLn "\n Welcome to Guess The Word!\n"

    difficulty <- promptUntil createDifficulty "Enter difficulty level [e/m/h] or leave empty for easy difficulty: "
        " is not a valid difficulty."

    putStrLn $ show difficulty ++ " difficulty selected."

    secret <- generateSecret difficulty

    loop secret $ replicate (length secret) '-'

    putStrLn secret
    putStrLn "You win!"

-- These functions are still partial, but we have checked their preconditions.
replaceChar :: ValidIndex -> String -> Char -> String
replaceChar (ValidIndex n) s c = pre ++ [c] ++ tail post
    where (pre, post) = splitAt n s

compareSecret :: String -> String -> ValidIndex -> Char -> String
compareSecret secret guess pos@(ValidIndex i) c =
    if secret !! i == c then
        replaceChar pos guess c
    else
        guess

Separating business logic from generic combinators
There are some cases where you can separate the code that is specific
to your application from generic code that could be used anywhere.
For example, the generateSecret function can be split into a pickOne function:
pickOne :: [a] -> IO a
pickOne [] = error "pickOne: Empty list"
pickOne xs = do
    num <- randomRIO (0, length xs - 1)
    return $ xs !! num

generateSecret :: Difficulty -> IO (String)
generateSecret = pickOne . fetchWords

As you can see, the function is still partial, but since the word list is hard coded, we can probably assume that there is at least one word for each difficulty.
Explicit import lists
The recommended way of handling imports is to either use a qualified import like this:
import qualified Data.Char as Char

example = Char.isDigit '2'

or use an explicit import list like this
import Data.Char (isDigit)

example = isDigit '2'

This makes it easier for readers to see where functions come from.
Performance
Since all strings and lists are really short in this example, performance will not really be an issue. However, if the strings or lists were a lot longer, there are a few things to note. All functions that work with an index into a list are O(n) (instead of the O(1) they would be if we were working with a mutable array).
Haskell has support for mutable vectors, but then you lose the purity that is so nice about Haskell. If you want to keep purity, you can use a Data.Map, which has O(n*log n) complexity, but at the cost of a O(n*log n) memory cost. You could also use Data.Text, which still has O(n) complexity, but has a much more compact representation and is faster overall.
Anyways, as I said, this is not really relevant for this program since n (the length of the lists/strings) is really small and will probably never be large.

A partial function is any function that can crash
For example using Liquid Haskell or by using some fancy type-level trickery where the length of the list is in its type


Hash Table: adding and finding functions I made this function to teach myself about hash tables. Any ideas? I commented out free(addition) because it was giving me a compiler error, but I need to figure out how to deallocate the memory.

#define hashSize 1000

typedef struct hashElem{
    char* date;
    char* holiday;
}hashElem;

typedef struct hash{
    struct hashElem element;
    struct hash* next;
}hash;

hash* hashTable[hashSize];

// Hash function
size_t murmurHash(const void *keyPtr, size_t keySize)
{
     /*  DESCRIPTION:    Very fast string hashing algorithm with excelleng distribution and
     *                  collision resistance. This source code has been released into the public
     *                  domain.
     *
     *  NOTES:          http://murmurhash.googlepages.com
     *
     *  RETURNS:        Hashtable index for the key
     *
     */

    /* 'm' and 'r' are mixing constants generated offline.         */
    /* They're not really 'magic', they just happen to work well.  */

    const unsigned int m = 0x5bd1e995;
    const int r = 24;

    size_t hashValue = 0;

    /* Mix 4 bytes at a time into the hash */

    const unsigned char * data = (const unsigned char *)keyPtr;

    while(keySize >= 4)
    {
        unsigned int k = *(unsigned int *) data;

        k *= m;
        k ^= k >> r;
        k *= m;

        hashValue *= m;
        hashValue ^= k;

        data += 4;
        keySize -= 4;
    }

    /* Handle the last few bytes of the input array */

    switch(keySize)
    {
        case 3: hashValue ^= data[2] << 16;
        case 2: hashValue ^= data[1] << 8;
        case 1: hashValue ^= data[0];
                hashValue *= m;
    };

    /* Do a few final mixes of the hash to ensure the last few  */
    /* bytes are well-incorporated.                             */

    hashValue ^= hashValue >> 13;
    hashValue *= m;
    hashValue ^= hashValue >> 15;

    return (hashValue % hashSize);
}

// strlen: return length of string
int strlen(char *s)
{
    char* p = s;

    while (*p != '\0')
        ++p;
    return p - s;
}

// Add Holiday into hash table
void addHoliday(char* date, char* holiday)
{
   char* key;
   int keyLen;
   size_t keyHash;
   hash* addition = malloc(sizeof(*addition));

   key = date;
   addition->element.date = key;
   addition->element.holiday = holiday;

   // Add holiday to Hash Table
   if (hashTable[murmurHash(key, strlen(key))] == NULL)
   {
       addition->next = NULL;
       hashTable[murmurHash(key, strlen(key))] = addition;
   }
   else  //If there is already an element at the index, add current holiday to beginning of list
   {
       addition->next = hashTable[murmurHash(key, strlen(key))];
       hashTable[murmurHash(key, strlen(key))] = addition;
   }

   //free(addition);
}

// If a date is entered, retrieve holiday from hash function index
char* getHoliday(char* date)
{
    return(hashTable[murmurHash(date, strlen(date))]->element.holiday);
}

void main(void)
{
   addHoliday("01/01/13", "New Years Day");
   printf("%s\n", getHoliday("01/01/13"));

   getchar();
}

For one thing, this:

   // Add holiday to Hash Table
   if (hashTable[murmurHash(key, strlen(key))] == NULL)
   {
       addition->next = NULL;
       hashTable[murmurHash(key, strlen(key))] = addition;
   }
   else  //If there is already an element at the index, add current holiday to beginning of list
   {
       addition->next = hashTable[murmurHash(key, strlen(key))];
       hashTable[murmurHash(key, strlen(key))] = addition;
   }

can simply be this:

   // Add holiday to Hash Table
   size_t idx = = murmurHash(key, strlen(key));
   addition->next = hashTable[idx];
   hashTable[idx] = addition;

Which cuts at least one invoke of the hash function, and in the process a similar number of strlen invokes. There is little sense is "checking" for a NULL in the hash table slot, because that is precisely what you want the next pointer of your new node to be anyway.

Not entirely clear either is whether you intended on your keys being unique. Both code snippets above (yours and mine) do not enforce unique keys. It is worth a thought to determine whether that feature is needed or not.

And (not really) finally, try and make a habit of creating hash functions that are independent of the hash table size. Though that may sound odd, it makes sense from the perspective of reuse. A hash function ideally returns a "value". That value is then massaged (usually via a modulo with the hash table size) to drive the final key into a slot within the table constraints. But that operation is separate from the hash function itself, which should be restricted only by the domain of the return type. In other words, this:

size_t murmurHash(const void *key, size_t len)

should simply return a value between 0 and the maximum value of a size_t on the platform. Period. No table size should be involved in the has function itself. Just chew on data and return some size_t. The caller of the function (the inserter, the lookup, etc), then takes this value and restricts it down to the table size (usually via a modulo of some kind). This makes the invocation look something like this:

   // Add holiday to Hash Table
   size_t idx = = murmurHash(key, strlen(key)) % hashSize;
   addition->next = hashTable[idx];
   hashTable[idx] = addition;

Though it may seem a little odd at first, it makes your hash functions more robust. They are not restricted by some table size. The user of the hash function does that step; not the function itself. This is the approach used by the standard C++ library std::hash<> template, and it is a good design to emulate.

Best of luck.


Creating restrictions for a sprite's possible position on the screen The great feedback I got on my first post on Code Review made me wary of code repetition. This code does what it is supposed to, which is to create restrictions for a sprite's possible position on the screen.

Is this a neat looking solution, or can it be considered code repetition?

public void move(int x, int y) {
    int maxX = screenWidth - image.getWidth()/2;
    int minX = image.getWidth() / 2;
    int maxY = screenHeight - image.getHeight()/2;
    int minY = image.getHeight() / 2;

    if (x > maxX)
        this.x = screenWidth - image.getWidth();
    else if (x < minX)
        this.x = 0;
    else
        this.x = x - image.getWidth() / 2;

    if (y > maxY)
        this.y = screenHeight - image.getHeight();
    else if (y < minY)
        this.y = 0;
    else
        this.y = y - image.getHeight() / 2;
}

It is possible to refactor your code starting from the definition of your variables:

int maxX = screenWidth - image.getWidth()/2;
int minX = image.getWidth() / 2;
int maxY = screenHeight - image.getHeight()/2;
int minY = image.getHeight() / 2;

You are repeating function calls image.getWidth(), image.getHeight() and divisions by 2 in more than one definition, while you could call store functions calls results and  division results in other variables like below:

int width = image.getWidth();
int height = image.getHeight();
int minX = width / 2;
int minY = height / 2;
int maxX = screenWidth - minX;
int maxY = screenHeight - minY;

The other code you can simplify is the following:

if (x > maxX)
        this.x = screenWidth - image.getWidth();
    else if (x < minX)
        this.x = 0;
    else
        this.x = x - image.getWidth() / 2;
    // same behaviour of the above code
    if (y > maxY)
        this.y = screenHeight - image.getHeight();
    else if (y < minY)
        this.y = 0;
    else
        this.y = y - image.getHeight() / 2;

It is the same code applied one time to width and other time to height : you could define a helper function to be applied two times like below:

private int getNewCoordinate(int c, int l, int max, int min, int maxl) {
        if (c > max) { return maxl - l; }
        if (c < min) { return 0; }
        return c - l / 2;
}

Now if I haven't made confusion with some variable your original code can be written like this:

public void move(int x, int y) {
    int width = image.getWidth();
    int height = image.getHeight();
    int minX = width / 2;
    int minY = height / 2;
    int maxX = screenWidth - minX;
    int maxY = screenHeight - minY;

    this.x = getNewCoordinate(x, width , maxX, minX, screenWidth);
    this.y = getNewCoordinate(y, height, maxY, minY, screenHeight);
}

private int getNewCoordinate(int c, int l, int max, int min, int maxl) {
    if (c > max) { return maxl - l; }
    if (c < min) { return 0; }
    return c - l / 2;
}


Android View Controller: Precondition vs ClassCastException I am working on an Android app that has an Abstract view controller class. I am using that Abstraction and implementing it for an Android Spinner view. It has a method that assigns the view to be controlled. I implemented it as:
@Override
public void setView(@Nonnull View view) {
    super.setView(view);
    mView = (Spinner) Preconditions.checkNotNull(view);
}

I am getting feedback in a code review that would have me revise the method to:
@Override
public void setView(@Nonnull View view) {
    super.setView(view);

    Preconditions.checkNotNull(view, "The view provided is null");
    Preconditions.checkArgument(view instanceof Spinner,
            "view provided was not a Spinner view");
    mView = Spinner.class.cast(view);
}

I am unclear as to why the second is a better implementation.  From what I understand, using Class.cast() is only really useful for generics (1, 2).
Other than that, it looks like the second implementation does the same thing. It will crash the app if view is null, or crash the app if view is not an instance of Spinner.
Is there a benefit to using the Precondition instanceOf check to crash the app in testing rather than a standard ClassCastException?

This is a relatively new area in Java, and best practice is not as well established....

... but, as I think about it, I think your reviewer has a point.

In your code base, consistency counts for a lot. If you check all your preconditions in a consistent way, and are disciplined about it, it will make a number of things easier to manage and maintain:

logs will be consistent
Exceptions will have more meaningful messages
Programmer's expectations are clearly shown

So, if the suggestions from your reviewer are consistent with the common-practice at your location, then, absolutely, do them.

Even if they are not common practice, still do them.

But, I have some comments to come after a small diversion.....

  You are correct that Spinner.class.cast(...) was introduced with Generics. The main purpose of this method appears to be related to the return value of methods with a Generic return type or where generic erasure makes internal manipulation of the data impossible without some help from the class (i.e. it counteracts the effects of erasure).

  These methods (or the class they are in) are supplied with an instance of the generic class, and they then use that class instance to cast the value.... for example, in Collections.checkedList(....). It is unusual to see this method (cast()) referenced from a static context Spinner.class.cast(value) when a simple (Spinner)value will suffice.

  On the other hand, it is interesting that, if you have multiple values being cast on a single line of code, the ClassCastException stack trace for (Spinner)value will be different to the trace for Spinner.class.cast(value). I need to verify this, but it may make traces slightly more useful.

  So, Spinner.class.cast(value) is not exactly the same as (Spinner)value.

Now, having said that, I can understand why Spinner.class.cast(...) may be recommended, but then I would have to question why Preconditions.checkArgument(view instanceof Spinner, ...) was recommended....

The view instanceof Spinner is logical to me, but, given the context of using Spinner.class.cast() I would also suggest that you should use Spinner.class.isInstance(view) as well.

If you are going to use these methods then I would suggest consistency.

Finally, I think that, unless there is the intention to turn this class in to a more generic-based one, that using the cast(...) and isInstance(...) is redundant. I would prefer the traditional (Spinner)view and view instanceof Spinner.

But, I fully agree that if you are going to check pre-conditions, that separating out the precondition check for instanceof is a good idea (although I would add the word 'The' to the beginning of the message).

So, if it were me, I would (given the context), suggest a compromise:

@Override
public void setView(@Nonnull View view) {
    super.setView(view);

    Preconditions.checkNotNull(view, "The view provided is null");
    Preconditions.checkArgument(view instanceof Spinner,
            "The view provided was not a Spinner view");
    mView = (Spinner)view;
}


Reversing the elements in the list Q1. Define a function reverse_list that takes a list as an argument
  and returns None, but reverses the elements in the list as a side
  effect. Do not use any built-in list methods (especially not reverse),
  but assignment statements are fine.

Below is the iterative solution:

def reverse_list_iter(s):
    """Reverse the contents of the list s and return None.

    >>> digits = [6, 2, 9, 5, 1, 4, 1, 3]
    >>> reverse_list_iter(digits)
    >>> digits
    [3, 1, 4, 1, 5, 9, 2, 6]
    >>> d = digits
    >>> reverse_list_iter(d)
    >>> digits
    [6, 2, 9, 5, 1, 4, 1, 3]
    """
    median = len(s)//2
    length = len(s)
    for index in range(median):
        temp = s[index]
        s[index] = s[(length - 1) - index]
        s[(length - 1) - index] = temp

Below is the recursive solution:

def reverse_list_recur(s):
    """Reverse the contents of the list s and return None.

    >>> digits = [6, 2, 9, 5, 1, 4, 1, 3]
    >>> reverse_list_recur(digits)
    >>> digits
    [3, 1, 4, 1, 5, 9, 2, 6]
    >>> d = digits
    >>> reverse_list_recur(d)
    >>> digits
    [6, 2, 9, 5, 1, 4, 1, 3]
    """
    median = len(s)//2
    length = len(s)
    def reverse(index):
        if index == median:
            return
        else:
            temp = s[index]
            s[index] = s[(length - 1) - index]
            s[(length - 1) - index] = temp
            reverse(index + 1)
    reverse(0)

How can I improve the solution?

In general, Python's tuple assignment and unpacking allows you to rewrite:

temp = foo
foo = bar
bar = temp

as:

foo, bar = bar, foo

which saves you two lines in both versions! This gets you to:

s[index], s[(length - 1) - index] = s[(length - 1) - index], s[index]

Rather than indexing by (length - 1) - index, note that e.g. s[-1] is the last item in the list, s[-2] the penultimate item, and so on. So you can rewrite:

s[(length - 1) - index]

as:

s[-index - 1]

and we're now down to:

s[index], s[-index - 1] = s[-index - 1], s[index]

and there's no need to calculate length.

For the iterative version I would factor out median, too, going from:

median = len(s)//2
for index in range(median):
    ...

to simply:

for index in range(len(s) // 2):  # note whitespace
    ...

Although it's now factored out, note that:

median = len(s)//2
length = len(s)

could be rearranged to:

length = len(s)
median = length // 2  # and here

len(s) is O(1), but why duplicate it? DRY!

As I've previously shown, adding an extra parameter with a default value (def reverse_list_recur(s, index=0):) can simplify recursive approaches, removing the need for a nested function.

I would also remove the closure on median and explicit return/else and just test if index < len(s) // 2: before the swap:

def reverse_list_recur(s, index=0):
    """Reverse the contents of the list s and return None.

    >>> digits = [6, 2, 9, 5, 1, 4, 1, 3]
    >>> reverse_list_recur(digits)
    >>> digits
    [3, 1, 4, 1, 5, 9, 2, 6]
    >>> d = digits
    >>> reverse_list_recur(d)
    >>> digits
    [6, 2, 9, 5, 1, 4, 1, 3]
    """
    if index < len(s) // 2:
        s[index], s[-index - 1] = s[-index - 1], s[index]
        reverse_list_recur(s, index+1)

Depending on how you interpret "built-in list methods", the following may be OK:

def reverse_list(s):
    s[:] = s[::-1]


Small coroutine class What do you think about this?

#include <alloca.h>

#include <setjmp.h>

#include <cassert>

#include <functional>

template <::std::size_t N = 4096>
class coroutine
{
  jmp_buf env_in_;
  jmp_buf env_out_;

  bool running_{};

  char stack_[N];

public:
  coroutine() = default;

  auto running() const noexcept
  {
    return running_;
  }

  template <typename F, typename ...A>
  void run(F&& f, A&& ...a)
  {
    if (setjmp(env_in_))
    {
      return;
    }
    // else do nothing

    auto top(reinterpret_cast<char*>(&top));
    alloca(top - (stack_ + N));

    running_ = true;

    [this, f = ::std::forward<F>(f)](A&& ...a) __attribute__ ((noinline))
    {
      f(::std::ref(*this), ::std::forward<A>(a)...);

      running_ = false;

      yield();
    }(::std::forward<A>(a)...);
  }

  void yield() noexcept
  {
    if (setjmp(env_out_))
    {
      return;
    }
    else
    {
      longjmp(env_in_, 1);
    }
  }

  void resume()
  {
    assert(running_);
    if (setjmp(env_in_))
    {
      return;
    }
    else
    {
      longjmp(env_out_, 1);
    }
  }
};

Usage:

#include <iostream>

#include "coroutine.hpp"

int main()
{
  coroutine<> c;

  c.run([](decltype(c)& c)
    {
      for (int i{}; i != 3; ++i)
      {
        ::std::cout << i << ::std::endl;

        c.yield();
      }
    }
  );

  while (c.running())
  {
    c.resume();
  }

  return 0;
}

EDIT

Change

auto top(reinterpret_cast<char*>(&top));

to

char* top;
top = reinterpret_cast<char*>(&top);

Also check out an updated version of the code.

I think you need to document your assumptions and/or intended use cases a little better. Right now, I don't see how anything based on setjmp/longjmp could actually implement a coroutine, since longjmp can only ever jump upward in the call-stack, never downward. (Really, it can't jump "downward" because there is no "downward": the only things below us on the call-stack are things that have already finished executing, and things which will execute in the future.)

Notice that "things which will execute in the future" includes "interrupt handlers". You might want to check whether your implementation plays well with Unix signal handlers — I suspect it does not.

alloca(top - (stack_ + N));

This is a red flag: you're calling alloca without assigning the result anywhere. A sufficiently smart compiler will just optimize away this useless call. (Yeah, I kinda get what you're trying to do... but the compiler is under no obligation to get it.) What you really need to do here is heap-allocate a buffer for your stack, and then somehow get the saved stack pointer in the jmp_buf to point to it.

And I only kinda see what you're doing here, anyway. Are you assuming that the coroutine object itself must be allocated on the stack? What happens if I make a global or static coroutine object?

[this, f = ::std::forward<F>(f)](A&& ...a) __attribute__ ((noinline))

The __attribute__((noinline)) here is another red flag. This time I don't get what you're trying to do. What would break if this lambda's operator() got inlined?

Also, style nit: I've never seen anyone put a space after the word __attribute__ before.

Other style nits:

auto running() const noexcept

auto is a strange way to spell bool. It's not saving you any bytes.

// else do nothing

The above comment is highly misleading, in that it's followed by 13 lines of really dense code — hardly "do nothing"! I'd delete that comment.

auto top(reinterpret_cast<char*>(&top));

I'm actually amazed that this works, given that the decltype of &top is not known at the time the RHS is parsed. ...Oh wait, yeah, it doesn't work. Both GCC and Clang reject this code.

for (int i{}; i != 3; ++i)

Kudos for using prefix increment. Everything else about this for-loop is pretty unusual, though. I strongly recommend looping over the range [0,3) like this instead:

for (int i = 0; i < 3; ++i)

API design suggestions:

Empty angle brackets suck. I'd recommend something more like

template <size_t N> class coroutine_with_size { ... }

using coroutine = coroutine_with_size<4096>;

It's unfortunate that your coroutine has to do all its own I/O; a better design would be like the one found in Javascript (and Python), where coroutines can yield values. Unfortunately, the ability to yield values causes a cascade of API changes. Look:

c.run([](auto&& c) {
    for (int i = 0; i < 3; ++i) {
        c.yield(i);
    }
});

while (c.running())
{
    int ii = c.resume();
    std::cout << ii << std::endl;
}

Well, unless c is somehow "looking ahead" to determine the value it's going to return next time, c.running() doesn't really make sense anymore. We've got to have c.resume() do all the hard work, at which point we need a way to say "okay, no more data, please stop iterating". That is, we need a way for c.resume() to terminate execution but not return a value. This smells like an exception. So:

c.run([](auto&& c) {
    for (int i = 0; i < 3; ++i) {
        c.yield(i);
    }
});

while (true)
{
    try {
        int ii = c.resume();
        std::cout << ii << std::endl;
    } catch (const coroutine::done&) {
        break;
    }
}

But now what is c.run() actually doing? It can't be actually starting to run the provided lambda, because there's nowhere for us to stash the yielded result yet (we haven't gotten to ii's declaration). So all it's really doing now is constructing the coroutine object and then returning immediately. That smells like a constructor. So:

auto c = coroutine([](auto&& c) {
    for (int i = 0; i < 3; ++i) {
        c.yield(i);
    }
});

while (true)
{
    try {
        int ii = c.resume();
        std::cout << ii << std::endl;
    } catch (const coroutine::done&) {
        break;
    }
}

And unfortunately now we've got move-constructible coroutine objects, and your entire implementation strategy falls apart even more than it had been. Crap!

...Speaking of which, your current implementation provides defaulted copy and move constructors! You should definitely delete those:

coroutine(const coroutine&) = delete;
coroutine& operator=(const coroutine&) = delete;
// move operations get deleted implicitly due to the above;
// but I wouldn't object to deleting them explicitly as well

Getting back to the API-design example: the next step in a Javascript-like design would be allowing c.resume() to take input values, as well as yielding output values. Mind you, I'm not sure what the implementation for this would look like in C++. It might not be 100% possible. :)

auto c = coroutine([](auto&& c, int step) {
    for (int i = 0; i < 3; i += step) {
        step = c.yield(i);
    }
});

while (true)
{
    try {
        int ii = c.resume(1);
        std::cout << ii << std::endl;
    } catch (const coroutine::done&) {
        break;
    }
}

EDIT: Okay, I think it's not 100% possible — not without templating the coroutine class on the "inputs" and "outputs" (args-to-resume and args-to-yield) that it expects. I've done my own toy implementation here for comparison. Muddling through that implementation also helped me understand the undefined-behavior magic about which I complained at the top of this review.

At least in the trivial case you started with — a coroutine used only as a generator, and whose c.resume() takes no input values — it would make sense to provide a sort of coroutine_generator_iterator so that one could write simply

for (int ii : coroutine_generator_iterator(c)) {
    std::cout << ii << std::endl;
}

This coroutine_generator_iterator concept wouldn't be very useful for coroutines that were actually used as coroutines, e.g. passing control back and forth. But it's definitely a good idea if you're just trying to do Python-style generator functions in C++.


Interrupt lengthy for loops I have this piece of code I would like some help with. I'm running a lengthy process that I want to interrupt at any time. The thing is, as you can see in the code, I have if(stop) everywhere, also in the methods inside the for (since they have for loops also).

stop is a member variable of the class, and the Stop() method is called from the UI from a different thread.

I don't think making it multithreading would be a good option since would be more complex to keep state and to communicate between threads.

Can you think of a better way to solve this, making the code cleaner?

public void Start()
{
    for(int i = 0; i < Length; i++)
    {
        if(stop)
            return;

        LengthyMethodA();

        if(stop)
            return;

        LengthyMethodB();

        if(stop)
            return;

        LengthyMethodC();
    }
}

public void Stop()
{
    stop = true;
}

You could always set up a list of Actions that need to be worked on, and then loop through them checking for Stop each time.

Something like :

List<Action> actions = new List<Action> {
  () => LengthyMethodA(),
  () => LengthyMethodB(),
  () => LengthyMethodC() };

  for(int i = 0; i < Length; i++)
  {
    foreach (Action action in actions)
    {
       action();
       if(stop)
            return;
    }
  }

This may or may not be better. It depends on the actual code. The actions list would represent an ordered set of transactional units of code - each unit must be completed, but the whole set can be broken apart and cancelled if necessary.


1D Peak Finder Algorithm I'm reviewing MIT Introduction to Algorithm lectures/exercises and am trying to implement a one dimensional peak finder algorithm.

I've got a working copy but it's a bit messy and I've had to put some array size constraints to get it working properly. How can I improve this implementation?

def peakfinder(arr):
    if len(arr) == 0: # If list is 0, there is no peak.
        return None

    if len(arr) == 1: # If list has been reduced to 1 element, it's a peak.
        return arr

    mid = len(arr) / 2

    left  = mid - 1
    right = mid + 1

    if arr[left] <= arr[mid] >= arr[right]:
        return arr[mid] # Base case. This is a peak.

    if arr[mid] < arr[left]: # Look to left side of array for peak.
        return peakfinder(arr[:mid])

    if arr[mid] < arr[right]: # Look to right side of array for peak.
        return peakfinder(arr[mid+1:])

Crashing bug

If I call peakfinder([0, 0]), I get an IndexError:

Traceback (most recent call last):
  File "peaks.py", line 32, in <module>
    peakfinder([0, 0])
  File "peaks.py", line 22, in peakfinder
    if arr[left] <= arr[mid] >= arr[right]:
IndexError: list index out of range

(I’m not sure if these are the “size constraints” you allude to in the question, you didn’t provide details.)

If we inspect these, we discover that we have (L, M, R) = (0, 1, 2), but trying to get the element at index 2 will fail. This will fail whenever you have an array of length 2.

One way to handle this would be to add an extra base case for when you have an array of length 2. Alternatively, you could tweak the bounds checking so that it only looks for elements within the bounds of the array.

Let’s suppose we never pass in an array of size 2. But it turns out other arrays will hit the same, if they reduce to an length-2 array at an intermediate step. For example:

>>> peakfinder([0, 1, 0, 0])
Traceback (most recent call last):
  File "peaks.py", line 35, in <module>
    peakfinder([0, 1, 0, 0])
  File "peaks.py", line 29, in peakfinder
    return peakfinder(arr[:mid])
  File "peaks.py", line 31, in peakfinder
    if arr[mid] < arr[right]: # Look to right side of array for peak.
IndexError: list index out of range

Here’s a small test I wrote with Hypothesis to find some counterexamples:

from hypothesis import given, strategies as st

@given(st.lists(st.integers()))
def test_peakfinder_doesnt_crash(xs):
    """Finding the peak of a list doesn't crash."""
    peakfinder(xs)

The peak of an array must be an element of the array

Sounds reasonable enough, right? But your code will return a list of length 1 if any of the intermediate steps reduce to such a list, an element otherwise. For example:

>>> from peaks import peakfinder
>>> peakfinder([1, 0])
[1]
>>> [1] in [1, 0]
False
>>> peakfinder([1, 2, 3])
2
>>> 2 in [1, 2, 3]
True

You should tweak the case when len(arr) == 1 to return the single element:

if len(arr) == 1:
    return arr[0]

Here’s another small test I wrote to find a counterexample:

from hypothesis import assume, given, strategies as st

@given(st.lists(st.integers()))
def test_peak_is_in_original_list(xs):
    """The peak of a list is in the list."""
    assume(len(xs) > 0)
    assert peakfinder(xs) in xs