Brightness adjustment control for multiple UI elements I have an application that basically exists as a map and some controls. The controls change the look and feel of the map. I have a slider which is used to change the brightness, for example. At the moment there is only one map but more might be added in future. This seems to me like a good situation for observer pattern. However, the instance Controller of the slider exists far away from an instance of the map component so I'm not sure how exactly to implement it. To propagate a reference to the map through lots of function calls seems like poor design. (Perhaps it's due to poor design that they are referenced so far apart but I don't have control of that)

To solve this, I've written some glue code which sits in between. I've called them services for lack of a better name. They both work, but I'm not sure it's as elegant as it could be. I'm also not sure as to whether the Java Observer/Observable are adding anything at all in the more complicated service. I wonder whether there's another design pattern that may be better suited here, too.

Any help would be great.

Map:

public class Map
{
    public void setBrightness(int i)
    {
        System.out.println("Brightness set to: " + i);
    }
}

Controller:

public class Controller
{
    public void brightnessChanged()
    {
        // Create an "event" and fire it to all listeners
        BrightnessService.update(11111);
        OtherBrightnessService.update(55555);
    }
}

A simple brightness service:

import java.util.LinkedList;
import java.util.List;

public abstract class BrightnessService
{
    private static final List<Map> OBSERVERS;

    static
    {
        OBSERVERS = new LinkedList<>();
    }

    public static void register(Map map)
    {
        OBSERVERS.add(map);
    }
    public static void update(int i)
    {
        for (Map obs : OBSERVERS)
        {
            obs.setBrightness(i);
        }
    }
}

A possibly too complicated brightness service

import java.util.Observable;
import java.util.Observer;

public abstract class OtherBrightnessService
{
    static class MyObserver implements Observer
    {
        private Map map;

        public MyObserver(Map m)
        {
            map = m;
        }

        @Override
        public void update(Observable o, Object o1)
        {
            map.setBrightness(((Brightness) o).get());
        }
    }
    static class Brightness extends Observable
    {
        private int brightness = 0;

        public void set(int i)
        {
            brightness = i;

            setChanged();
            notifyObservers();
        }

        public int get()
        {
            return brightness;
        }
    }

    static Brightness brightness;

    static
    {
        brightness = new Brightness();
    }

    public static void register(Map map)
    {
        MyObserver observer = new MyObserver(map);
        brightness.addObserver(observer);
    }
    public static void update(int i)
    {
        brightness.set(i);
    }
}

Main

public class ObserverTest
{
    public static void main(String[] args)
    {
        // Set up map
        Map m = new Map();
        BrightnessService.register(m);
        OtherBrightnessService.register(m);

        // Change the brightness, this would fire from a user event
        // and be implemented in a different file
        Controller c = new Controller();
        c.brightnessChanged();
    }
}

Why did you assume that only Map will ever want to observe the brightness value? As your code hints, you will possibly have not only multiple maps at one point, but also multiple points from which the brightness could be controlled. Which in return would also want to register with that service to update their displayed state in case of external control.

What you did forget to implement in that specific service, is an oldBrigthness != newBrightness test. If the control is set to the same value again, you don't want to propagate. Even less so if the scenario mentioned above occurs, and the self-adjustment of a brightness control could possibly broadcast another update.

I probably wouldn't use the old Observable and Observer classes, their interface predates the introduction of generics in 1.5, and as you noticed isn't even type safe yet.

You should revise on the API design on your service. You now have the odd situation where the BrightnessService needs to be aware of Map, while on the other side the controlling sites need to be aware of BrightnessService.

This is a rather unfortunate situation, since you now created a dependency chain from BrightnessSlider via BrightnessService to Map. This means a BrightnessSlider can't exist, without Map existing in the same application.

The solution to break this chain, is to make Map instead aware of BrightnessService, respectively define a generic interface in the package of BrightnessService which allows other components to register with this service. Now both the BrightnessSlider and the Map can exist and access the BrightnessService without the other component being part of the same application. As a side effect, this also solves the case where BrightnessSlider wants to observes the currently set brightness as well.

private static final List<Map> OBSERVERS;

Caps lock is for constants, not for mere static class variables. At least not if they can be considered "variable", and not constant.

Well, actually something as a static class variable  shouldn't exist in your application to begin with. You should try to keep your classes always instantiable and free from side effects. This also applies to this service.

If you actually need to ensure that only a single instance of this service can exist, use the Singleton pattern. This is about the only exception to the rule where static final may be used to declare something else than a constant.

There are quite a few reasons for that, among others that the BrightnessSlider and Map classes usually would also want to hold a reference onto the BrightnessService they have been registered with (and be it only to provide a clean shutdown by unsuscribing themselves!).

It also allows you to move freely between Singleton and other techniques, such as dependency injection, should your code base require that later on. As the call sites already work on a specific instance of BrightnessService (with disregard as where they obtained it from!), there is little friction in changing that later on.

Furthermore, static methods should also be free of side effects, by convention.
Performing constant calculations, or returning a constant result/reference? Good.
Mutating some global state directly? Not good.


Project Euler 25 - 1000-digit Fibonacci Number I have recently completed problem 25 from the project Euler site.

  The Fibonacci sequence is defined by the recurrence relation:

  \$\qquad F_n = F_{n−1} + F_{n−2}\$, where \$F_1 = 1\$ and \$F_2 = 1\$.

  Hence the first 12 terms will be:

  \$\qquad F_1 = 1\$

  \$\qquad F_2 = 1\$

  \$\qquad F_3 = 2\$

  \$\qquad F_4 = 3\$

  \$\qquad F_5 = 5\$

  \$\qquad F_6 = 8\$

  \$\qquad F_7 = 13\$

  \$\qquad F_8 = 21\$

  \$\qquad F_9 = 34\$

  \$\qquad F_{10} = 55\$

  \$\qquad F_{11} = 89\$

  \$\qquad F_{12} = 144\$

  The 12th term, \$F_{12}\$, is the first term to contain three digits.

  What is the first term in the Fibonacci sequence to contain 1000 digits?

I have solved this problem in Java, however, the execution time is fairly slow, and, I am looking for ways to improve it; below is my code, as well as the output given:

import java.util.ArrayList;
import java.math.BigInteger;
public class ProjectEuler25 {
  public static void main(String [] args) {
    long start = System.nanoTime();
    ArrayList<BigInteger> fibonacciNumbers = new ArrayList<BigInteger>();
    boolean validNo = true;
    int x = 2;
    BigInteger tempAns = null;
    fibonacciNumbers.add(BigInteger.valueOf(1));
    fibonacciNumbers.add(BigInteger.valueOf(1));
    do {
        tempAns = fibonacciNumbers.get(x-1).add(fibonacciNumbers.get(x-2));
        fibonacciNumbers.add(tempAns);
        x++;
        if(tempAns.toString().length() >= 1000) {
            validNo = false;
        }
    } while (validNo == true);
    Long stop = System.nanoTime();
    System.out.println("The first term in the Fibonacci sequence to contain 1,000 digits is term: " + fibonacciNumbers.size());
    System.out.println("Execution time: " + ((stop - start) / 1e+6) + " ms");
  }
}

Output:

  The first term in the Fibonacci sequence to contain 1,000 digits is term: 4782

  Execution time: 220.858659 ms

I know that it is possible to solve this problem in C# in 2ms. I have provided the code for this solution below:

int i = 0;
int cnt = 2;
BigInteger limit = BigInteger.Pow(10, 999);
BigInteger[] fib = new BigInteger[3];

fib[0] = 1;
fib[2] = 1;

while (fib[i] <= limit) {
    i = (i + 1) % 3;
    cnt++;
    fib[i] = fib[(i + 1) % 3] + fib[(i + 2) % 3];
}

C# code sourced from here.

From looking at the C# code, the one thing which stands out to me, is maybe there is a better way to store the previous numbers than using an ArrayList?

I translated the given C# code to Java, and timed it on my machine using the Unix time command. It ran in 0.161 seconds. Your original code ran in 1.023 seconds. Here's the Java translation of the C# code:

import java.math.BigInteger;

public class Euler25 {
  public static void main(String[] args) {
    int i = 0;
    int cnt = 2;
    BigInteger limit = (new BigInteger("10")).pow(999);
    BigInteger[] fib = new BigInteger[3];

    fib[0] = BigInteger.ONE;
    fib[2] = BigInteger.ONE;

    while ((fib[i]).compareTo(limit) < 0) {
        i = (i + 1) % 3;
        cnt++;
        fib[i] = fib[(i + 1) % 3].add(fib[(i + 2) % 3]);
    }
    System.out.printf("Fibonacci %d has 1000 digits\n", cnt);
  }
}

Not to be excessively negative, but the solution in the C# code is much cleaner than the one you implemented. No shame in that; it happens to all of us. But I assume you wanted some advice on making the code you have run faster, because if you just wanted to copy that C#, you easily could have. So that's the direction I'll go with the rest of the answer.

It honestly puzzles me that this code is so much faster than yours. I would have expected the BigInteger math to be the really slow part, but this code does the same amount of BigInteger math as your original code.

Your code does take quite a bit more memory, since it stores all the Fibonacci numbers it calculates, whereas this code only stores what it needs to calculate the next one, and uses an integer to count how many Fibonacci numbers it's seen so far. An array list is backed by an array, so actually accessing the items shouldn't be much slower than with a plain array, but there might be some kind of cache or memory allocation effect. With this code, the compiler can block out a single, static chunk of memory. In the original code, the ArrayList that stores Fibonacci numbers keeps on expanding, so the backing array might have to be reallocated several times. Every time the array is reallocated, everything stored in it has to be moved over to the new storage space, which is pretty slow if you have a big list. If that turns out to be the problem, you can try passing the ArrayList constructor a guess for big you think the list might get (the default size is ten).

If I were you, I would profile the code and look for some issue like that. Dig into the ArrayList implementation and see if it's spending a lot of time reallocating. Just see where the code is spending its time, and try to figure out why that's where it's spending its time. If you don't already have a profiler you like, Netbeans has a pretty good one built in, and I'm sure Eclipse and other Java IDEs also have them. If you profile and post some of your numbers, we can probably give you better help with diagnosing performance issues.

EDIT: @ChrisHayes discovered why the code is so much slower: it's the toString call. I took the original code and just replaced the toString call with a check against a limit of \$10^999\$ as the C# code does. Still used the array list, still stored all the Fibonacci numbers. As Chris Hayes observed, this reduced the runtime from about 900ms to about 13ms. The author of this blog post also found that BigInteger.toString() was quite slow. An answer on this page implies that BigIntegers are stored in a way that makes it easier to implement the BigInteger.toString(radix) method that lets you convert a BigInteger into other bases, like hexadecimal or ternary. One could apparently implement BigInteger in a more specific way that makes it quick to convert into a base-10 string, but difficult or impossible to convert into strings in other bases, but the standard library implementers chose a representation which was more general, but slower to convert into a string.

See the end of the answer for my final version of the code, including modifications I made to get rid of toString.

[/EDIT]

Aside from the performance issues, I had some readability issues—not that your code was unreadable, just that it was harder to read than it had to be, and that contributed a little to the confusion that we had over where the number 4872 was coming from. The biggest one is the variable x. Unless you're working with coordinate axes, please don't call variables x. The C# code gives the analogous variable the name cnt, which is better, since you can tell it's probably some kind of counter. In general, that C# code is quite clean, so it's a good model to learn from, though I'd probably just go all the way and call it count. n could also work, since it's traditional to index the Fibonacci numbers with n. (This is another reason why x is confusing; if I'm reading a program about Fibonacci numbers, I can process F_n or the like pretty easily, but F_x is just strange.)

I also found your use of the do-while loop with a boolean flag confusing. A for or while loop with a break statement would have been better, but I think the best would be to let the do-while work for you, and write something like this:

do {
    tempAns = fibonacciNumbers.get(n-1).add(fibonacciNumbers.get(n-2));
    fibonacciNumbers.add(tempAns);
    n++;
} while (tempAns.toString().length() < 1000);

which takes advantage of the fact that tempAns gets initialized before the loop condition is checked.

The BigInteger class has built-in constants for zero and one, since they're so common. So you can initialize your array list like this:

fibonacciNumbers.add(BigInteger.ONE);
fibonacciNumbers.add(BigInteger.ONE);

It's a bit shorter than BigInteger.valueOf(1). (I like Java well enough, but it sure can get long.)

Here's the entire code sample with my suggested changes, including the one that eradicates toString and boosts performance that Chris Hayes suggested:

import java.util.ArrayList;
import java.math.BigInteger;

public class ProjectEuler25 {
  public static void main(String [] args) {
    long start = System.nanoTime();
    ArrayList<BigInteger> fibonacciNumbers = new ArrayList<BigInteger>();
    int n = 2;  // Index of current Fibonacci number.
    BigInteger tempAns = null;
    BigInteger limit = new BigInteger("10").pow(999); // First 1000-digit number.

    fibonacciNumbers.add(BigInteger.ONE);
    fibonacciNumbers.add(BigInteger.ONE);
    do {
        tempAns = fibonacciNumbers.get(n-1).add(fibonacciNumbers.get(n-2));
        fibonacciNumbers.add(tempAns);
        n++;
    } while (tempAns.compareTo(limit) <= 0);

    Long stop = System.nanoTime();
    System.out.println("The first term in the Fibonacci sequence " +
                       " to contain 1,000 digits is term: " +
                        fibonacciNumbers.size());
    System.out.println("Execution time: " + ((stop - start) / 1e+6) +
                       " ms");
  }
}


Discover random web servers and print the index page title Github repo

I wrote this little toy today that probes random web servers and prints out the title of their index page. I'm curious if this code is any good since I'm a bit rusty.

get_titles.py

#!/usr/bin/env python

""" Probe random web server and print out the title of their index page. """

import os
import sys
import lxml.html

def get_title(url):
    '''Extract the title from a webpage.
    If anything goes wrong like the request
    times out or the page does not have a
    title get_title will return None'''
    try:
        title = lxml.html.parse(url)
        return title.find(".//title").text
    except KeyboardInterrupt:
        sys.exit(0)
    except Exception:
        return None

def shell(command):
    """Run a shell command.
    return it's output as a string"""
    return os.popen(command).read()

def scan(nhosts):
    """Run an nmap scan of n hosts.
    This will return IPv4 addresses of hosts
    that are up and have a service running on
    port 80. Simply supply the number of hosts
    you would like to check."""
    results = set(shell('./discover.sh {}'.format(nhosts)).split('\n'))
    if not results:
        print 'Nothing Found.'
        sys.exit(0)
    return ["http://{}".format(ip) for ip in results if ip]

def main():
    """ Run the scan."""
    if not len(sys.argv) >= 2:
        print 'You forgot to tell me how many hosts to check'
        sys.exit(0)
    for url in scan(sys.argv[1]):
        title = get_title(url)
        if title:
            print title, url

if __name__ == '__main__':
    main()

discover.sh

#!/bin/bash

nhosts=$1

nmap \
--host-timeout 10s \
--max-retries 0 \
–min-parallelism 100 \
-T5 \
-Pn \
-n \
-iR $1 \
-p80 \
-oG tmp.txt \
--open \
tmp.txt > /dev/null

cat tmp.txt | grep 'Host' | awk '{print $2}'
rm tmp.txt

Overall, the code is pretty clean. Some notes about the Python part:

be consistent about the documentation string style - enclose them with triple quotes, start with an upper-case letter, end with a dot (PEP8 reference). I also think that you can use less newline breaks there.
if we would follow PEP8 importing recommendations, there should be a newline between the "system" level imports and third-parties:

import os
import sys

import lxml.html

you should have 2 line breaks between the function definitions (reference)
use print() as a function for Python 3 compatibility
I would avoid unnecessary invert of the logic and replace:

if not len(sys.argv) >= 2:

with:

if len(sys.argv) < 2:

avoid using bare except clauses
you can use .findtext() method:

title.findtext(".//title")

which will return None if title element was not found.


Arithmetic sequence in array 3 elements forms arithmetic sequence, when difference between any two consecutives of them is the same.

In our task we are given array A. Any pair of integers in this array is called slice (eg. (B,C)), when 0 <= B < C< N, where N is an array length (0<=N<=60).

A slice (B,C) is called arithmetic, when the sequence:
A[B],A[B+1],...,A[C-1],A[C] is arithmetic and B+1 < C.

My method should return the number of arithmetic slices in array.
Time complexity should be linear. When result exceeds 1000000000 method should return -1.
Is my solution correct?

public int computeNumberOfArithmeticSlices(int[] A) {
    int front = 0, total = 0;
    int result = 0;
    List list;
    for (int back = 0; back < A.length && front < A.length; ) {
        list = new ArrayList();
        front = back + 2;
        total = A[front - 1] - A[back];
        list.add(back);
        list.add(front - 1);
        int i = 0;
        boolean ok = false;
        while (front < A.length && A[front] - A[front - 1] == total) {
            ok = true;
            i++;
            list.add(front);
            back = front;
            front++;
            result += i;
            if (result > 1000000000)
                return -1;
        }
        if (!ok)
            back++;
    }
    return result;
}

Using raw types (generic types without type parameter) is a major violation:

List list = new ArrayList();

Never ever do this. In any case, as @JS1 already pointed out in comments,
this list in your implementation is unused anyway. (another major violation)

Declare variables in the smallest scope possible.
For example,
you could declare front inside the for statement,
and total inside the body:

for (int back = 0, front = 0; back < A.length && front < A.length; ) {
    front = back + 2;
    int total = A[front - 1] - A[back];

You could also convert the inner while loop to a for loop,
and declare the variable i inside the statement.


Bash script to monitor Webserver php script Below is a script I wrote today to use CURL to monitor the length of various HTTP server outputs and output a response if this is different than 103 characters.
This is my first bash script, so I had some reading/experimenting to do and it is quite rough.
The idea is that if there are no errors, my php script will ouput exactly 103 characters. If there are errors, this will be increased.

IDScript

#!/bin/bash
# Check for missing IDs - greater output than 103 characters
cd "/media/SMALL/scripts"

if [[ -z "$1" ]] # check if script arguement exists
then
        a=$(./IDscript "server");
        a=$(./IDscript "server2")$a;  #concatenate with previous output
        a=$(./IDscript "server3")$a;

        lenA=$(expr length "$a");

        if [ $lenA -eq 0 ] #check length of A is empty
        then
                echo "No errors";
        else
                mail -s "TID error" myEmail@company.org <<< "There are errors:"$a; # send the e-mail
        fi
        exit;
fi

#create random number from 50 to 99
r=$(( $RANDOM % 50 + 50 ));
#bypass HTTP proxy with random number
testURL=$1"/checkError?which=IDError&rand="$r;

curlOut=$(curl -s "$testURL");
curlLength=$(expr length "$curlOut");

if [ $curlLength -ne 103 ] #103 char is response length when no missing IDs exist
then
echo "length is not 103:"$testURL;
echo -e "\n";
fi

Comments and suggestions are welcome!

#!/bin/bash

Minor, still good to know, this is not portable, f.e. BSDs will choke on it. This might not be an issue if you only intent to use it on Linux, but if you ever release script into the wild, you should consider using a portable shebang line.

#!/usr/bin/env bash

if [[ -z "$1" ]] # check if script arguement exists
then

I'm a friend of the one-line-style, but that is for sure a pure personal preference.

if [[ -z "$1" ]]; then # check if script arguement exists

a=$(./IDscript "server");

From the context of this I can say two things:

that variable needs a better name
that script needs a better name

It is not obvious what the script does and neither what the variable will hold.

a=$(./IDscript "server");

In a shell script, there's no need to close every line with a semicolon. You can do it, but it's not necessary.

lenA=$(expr length "$a");

If you're going to use Bash, you can also use Bash syntax:

lenA=${#a}

Or shortened to:

if [ ${#a} -eq 0 ] #check length of A is empty
then

if [[ -z "$1" ]]

What I made a habit out of in the last time is extracting arguments into named variables so that the script is easier to read.

webAddress=$1

if [[ -z "$webAddress" ]]
...
testURL=$webAddress"/checkError?which=IDError&rand="$r;

This is of course not always useful, but pretty neat if you're going to use $1 in multiple places, as the script becomes easier to read.

echo -e "\n";

This can be shortened.

echo


Iterating over the lines of a stream This structure is supposed to iterate over the lines of an input stream, using std::getline. Any remark is welcome!

#include <iostream> //istream
#include <cassert>

struct line_iterator
{
    explicit
    line_iterator( std::istream & is )
        : m_stream( &is )
        , m_line( extract_line() )
    {
    }

    line_iterator()
        : m_stream( nullptr )
        , m_line()
    {
    }

    line_iterator( line_iterator & ) = default;

    std::string operator*()
    {
        assert( m_stream );
        return m_line;
    }

    line_iterator operator++()
    {
        assert( m_stream );
        update_stream();
        return *this;
    }

    line_iterator operator++(int)
    {
        line_iterator copy( *this );
        ++(*this);
        return copy;
    }
private:
    void update_stream()
    {
        if ( m_stream->good() )
            m_line = extract_line();

        else
        {
            m_line.clear();
            m_stream = nullptr;
        }
    }

    std::string extract_line()
    {
        std::string line;
        std::getline( *m_stream, line );
        return line;
    }

private:
    std::istream *  m_stream;
    std::string     m_line;

    friend bool operator!=( const line_iterator&, const line_iterator& );
    friend bool operator==( const line_iterator&, const line_iterator& );
};

inline
bool operator!=( const line_iterator & a, const line_iterator & b )
{
    return !(a.m_stream == b.m_stream);
}

inline
bool operator==( const line_iterator & a, const line_iterator & b )
{
    return a.m_stream == b.m_stream;
}

You might consider providing typedefs for the types expected by std::iterator_traits.
Your type should meet the requirements of InputIterator.  This means that you should also provide an overload for operator ->.
The semantics of operator != are not really useful.  For example, this won't allow you to use your iterator in the idiomatic

line_iterator begin = …;
line_iterator end = …;
while (begin != end)
  {
    std::cout << *begin << std::endl;
    ++begin;
  }

because there is no value you can set end to such that it compares not non-equal to begin only after begin was advanced a certain number of times.  I suggest you implement it somehow like this:

friend bool
operator!=(const line_iterator& lhs, const line_iterator& rhs) noexcept
{
  if (lhs.m_stream == nullptr && rhs.m_stream == nullptr)
    return false;
  else if (lhs.m_stream == nullptr)
    return rhs.m_stream->good();
  else if (rhs.m_stream == nullptr)
    return lhs.m_stream->good();
  else
    return lhs.m_stream != rhs.m_stream;
}

This will allow you to use a default-constructed line_iterator as end and have it compare not non-equal to begin only after begin's stream is exhausted.

You should then implement lhs == rhs as !(lhs != rhs).
You are making many copies of the string which is not as efficient as it could be.  I also feel that it makes your code more complicated than necessary.  You should read directly into m_string by implementing the increment operator as

line_iterator&
operator++()
{
  assert(m_stream && m_stream->good());
  std::getline(*m_stream, m_string);
  return *this;
}

and the dereferencing operator as

const std::string&
operator*() const
{
  return m_string;
}

In your constructor, default-construct m_string and (if the pre-conditions are met) advance once.
Personally, I prefer to only mention those members in the constructor's initializer list that depend on the constructor arguments.  Those that get default values I prefer to initialize in the class definition.

class line_iterator
{

public:

  line_iterator()
  {
    // nothing to do
  }

  line_iterator(std::istream& istr) : m_stream {&istr}
  {
    if (m_istr->good())
      std::getline(*m_stream, m_string);
  }

  …

private:

  std::string m_string {};     // defaults to empty string
  std::istream * m_stream {};  // defaults to 'nullptr'
};

If you're going to make the friends inline anyway, I'd prefer to define them right in the class body.


Truncate text to some number of words when clicked I am currently using the code below to truncate text:

 function truncate($elem, wordNum){

var words = $elem.find('p').text().trim().split(/\s+/).slice(0,wordNum);
var replacement = $("<p/>").text(words.join(" ") + "...");
$elem.find('.read-more').click(function(){
   if($(this).text() == 'Read More'){
    replacement.hide();
    $elem.find('p:eq(1)').show();
   } else if($(this).text() == 'Read Less'){
       replacement.show();
    $elem.find('p:eq(1)').hide();
   }
    $(this).text(function(i,old){
    return old=='Read More' ?  'Read Less' : 'Read More';
});
return false
});

$elem.find('p:first').before(replacement);
$elem.find('p:eq(1)').hide();
}

truncate($("p"), 5);

Works, but it's not the most efficient solution. Is there a better way ?

A few thoughts:

/\s/ is probably not the best thing to be searching for.  I would consider using /\b/ to look for all word boundaries, not just spaces.
It seems the $elem here would be a jQuery collection, however you are treating it is a single element without using things such a $.each() to be able to apply to multiple elements.  Your function fails if more than one element is passed in $elem.  This is a bad way to think about writing code against jQuery.  You should be thinking about working against collections, not single elements.
Since you are using jQuery, I would strongly suggest writing this as a jQuery plug-in such that you can better attach the behavior to a collection of elements (i.e. $('.some_class').truncatable({count: ...});.
Is word count really meaningful as a way to determine how you are truncating? I would think better logic would be to look for first word boundary at a location less than X number of characters in string as your truncation point.  That way you get more consistent size of truncated text, not subject to vary greatly based on overly long or overly short words in the content.
In the second example, it seems odd to hard-code nested <p>, .read_more, etc. into the function. Why would $elem not already be whatever element you want to work with (the <p> in this case)?  What if in the future you wanted to apply this to a <div> that does not have a <p> nested inside of it? Your code will break. You are limiting the usefulness of your code in doing this.
You should get in the habit of using strict comparisons (===, !==) by default instead of loose comparisons (==, !=).  This will make your code less fragile to unexpected truthy/falsey behavior, and this easier to maintain/debug.  I would say the loose comparisons should only be used when you have a very specific use case for doing so and would ideally include a comment in such cases explaining why such loose comparison is appropriate for that case.
I question the approach of storing the full/truncated texts in an alternate DOM elements.  I would think that perhaps when this truncation behavior is applied to an element, that you might want to store the full text (and perhaps the truncated text as well, if you want to avoid recalculation) in a data attribute.  If you take this approach, I would suggest using $.text() or native javascript textContent as means to make this change on click vs. $.html() or innerHTML for performance reasons.  You definitely don't want to be inserting nodes in DOM using $.before() here.
I know this is code review and we are reviewers should focus on the code presented, but I would suggest that you think about whether you are re-inventing the wheel here.  There are several plug-ins for jQuery that do this already - https://plugins.jquery.com/tag/truncate/


WCF using TAP without worrying about timeouts I really like using the new TAP pattern in .Net 4.5. and I am updating some of my older projects to use it.

One of my old patterns was to use EAP with WCF so I could have functions that could take longer than 60 seconds (the default timeout for WCF) to complete without doing custom setup on the client side App.Conifg.

Here is a simple example of what I would do.

[ServiceContract(CallbackContract = typeof(ICallback))]
public interface IService1
{
    [OperationContract(IsOneWay = true)]
    void Test(int arg);
}

[ServiceContract]
interface ICallback
{
    [OperationContract(IsOneWay=true)]
    void Callback(int arg);
}

public class Service1 : IService1
{
    public void Test(int arg)
    {
        var callback = OperationContext.Current.GetCallbackChannel<ICallback>();
        Thread.Sleep(61000); //Sleep for 61 sec.
        callback.Callback(arg + 1);
    }
}

Now I want to put a wrapper around my EAP pattern and turn it in to a TAP pattern (as an aside, just returning a TAP pattern like public Task<int> Test(int arg) will still have the 60 second time limit but it does work)

Here is the solution I came up with, this code would be run on the client.

static class ProxyClient
{
    private delegate void CallbackDelegate(int arg);

    private class CallbackClass : IService1Callback
    {
        public event CallbackDelegate CallbackEvent;

        void IService1Callback.Callback(int arg)
        {
            var tmp = CallbackEvent;
            if (tmp != null)
                tmp(arg);
        }
    }

    public static async Task<int> Test(int arg)
    {
        var callback = new CallbackClass();
        var client = new Service1Client(new InstanceContext(callback));
        TaskCompletionSource<int> tcs = new TaskCompletionSource<int>();

        //start listening for the completion event
        callback.CallbackEvent += (resultArg) => tcs.TrySetResult(resultArg);

        client.Test(arg);

        //wait for the result
        var result = await tcs.Task;

        //close the connection
        client.Close();

        return result;
    }
}

The main concern I had was receiving the completion callback for another invocation if a connection was used more than once. So I create and breakdown the connection from inside the function entirely.

I would love to hear input if this is a good idea, or is there a better way to accomplish long running tasks in WCF without having to modify timeout values client-side at the time of proxy creation.

Design Trigger

I don't understand why you implement a duplex wcf operation to emulate a simplex operation, just to avoid configuring the timeout. Let's assume you have a really good reason why you don't want to change the default timeouts, then your solution can still time out on being idle (after 10 minutes), because you changed from timing out on SendTimeout to ReceiveTimeout.

As explained here the following timeouts have an impact on your code:

SendTimeout:
maximum threshold to expire an ongoing simplex operation. -> The timeout you don't like to configure (default: 1 minute)
ReceiveTimeout:
maximum idle time on a session. -> The timeout between the one-way message and the callback fired back. (default: 10 minutes)

Review

Your concern about using a single instance for multiple calls is valid. Using a single instance with a single callback is the simplest solution. Otherwise, you would need to implement a matching system with correlation ids in the request and callback contracts.
The lifetime management is not robust enough. Use a try-finally block to close the connection. Also, distinguish between Abort (when connection is Faulted) and Close (graceful disconnect) (Close Connection).

//wait for the result
var result = await tcs.Task;  // <- if this throws an exception

//close the connection
client.Close();       // <- this won't be called


Rolling median solution I was implementing a rolling median solution and was not sure why my Python implementation was around 40 times slower than my C++ implementation.

Here are the complete implementations:

C++

#include <iostream>
#include <vector>
#include <string.h>
using namespace std;

int tree[17][65536];

void insert(int x) { for (int i=0; i<17; i++) { tree[i][x]++; x/=2; } }
void erase(int x) { for (int i=0; i<17; i++) { tree[i][x]--; x/=2; } }
int kThElement(int k) {
    int a=0, b=16;
    while (b--) { a*=2; if (tree[b][a]<k) k-=tree[b][a++]; }
    return a;
}

long long sumOfMedians(int seed, int mul, int add, int N, int K) {
    long long result = 0;
    memset(tree, 0, sizeof(tree));
    vector<long long> temperatures;
    temperatures.push_back( seed );
    for (int i=1; i<N; i++)
      temperatures.push_back( ( temperatures.back()*mul+add ) % 65536 );
    for (int i=0; i<N; i++) {
      insert(temperatures[i]);
      if (i>=K) erase(temperatures[i-K]);
      if (i>=K-1) result += kThElement( (K+1)/2 );
    }
    return result;
}

// default input
// 47 5621 1 125000 1700
// output
// 4040137193

int main()
{
  int seed,mul,add,N,K;
  cin >> seed >> mul >> add >> N >> K;
  cout << sumOfMedians(seed,mul,add,N,K)  << endl;
  return 0;
}

Python

def insert(tree,levels,n):
        for i in xrange(levels):
                tree[i][n] += 1
                n /= 2
def delete(tree,levels,n):
        for i in xrange(levels):
                tree[i][n] -= 1
                n /= 2

def kthElem(tree,levels,k):
        a = 0
        for b in reversed(xrange(levels)):
                a *= 2
                if tree[b][a] < k:
                        k -= tree[b][a]
                        a += 1
        return a

def main():
        seed,mul,add,N,K = map(int,raw_input().split())
        levels = 17
        tree = [[0] * 65536 for _ in xrange(levels)]
        temps = [0] * N
        temps[0] = seed
        for i in xrange(1,N):
                temps[i] = (temps[i-1]*mul + add) % 65536
        result = 0
        for i in xrange(N):
                insert(tree,levels,temps[i])
                if (i >= K):
                        delete(tree,levels,temps[i-K])
                if (i >= K-1):
                        result += kthElem(tree,levels,((K+1)/2))

        print result

# default input
# 47 5621 1 125000 1700
# output
# 4040137193
main()

On the above mentioned input (in the comments of the code), the C++ code took around 0.06 seconds while Python took around 2.3 seconds.

Can some one suggest the possible problems with my Python code and how to improve to less than 10x performance hit?

I don't expect it to be anywhere near C++ implementation but to the order of 5-10x. I know I can optimize this by using libraries like NumPy (and/or SciPy). I am asking this question from the point of view of using Python for solving programming challenges. These libraries are usually not allowed in these challenges. I am just asking if it is even possible to beat the time limit for this algorithm in Python.

In case somebody is interested, the C++ code is borrowed from floating median problem here.

Integer math in tight loops is an area where C++ will definitely run circles around Python. I think your expectations are too high. However, I'd like to point out a couple of things for you, even if they are not enough to reach your goal.

Avoid indexing when you can use direct iteration instead. For example, insert can be written like this and Python won't have to deal with i:

def insert(tree,n):
     for level in tree:
         level[n] += 1
         n /= 2

Python does not optimize constant subexpressions out of loops, so you'll want to compute (K+1)/2 before the main loop and store it in a variable.


Predict mars robot position Description

  A robot lands on Mars, which happens to be a cartesian grid; assuming that we hand the robot these instructions, such as LFFFRFFFRRFFF, where "L" is a "turn 90 degrees left", "R" is a "turn 90 degrees right", and "F" is "go forward one space, please write control code for the robot such that it ends up at the appropriate-and-correct destination, and include unit tests.

Here is an example output with command "FF":

[0, 2]

Code

class Robot {
  private int x;
  private int y;
  private int currentDirection;

  Robot() {
    this(0, 0);
  }

  Robot(int x, int y) {
    this.x = x;
    this.y = y;
    currentDirection = 0;
  }

  public void move(String moves) {
    for (char ch : moves.toCharArray()) {
      if (ch == 'R') currentDirection += 1;
      if (ch == 'L') currentDirection -= 1;

      currentDirection = currentDirection % 4;

      if (ch != 'F') continue;

      System.out.println(currentDirection);
      if (currentDirection == 0) {
        y += 1;
      } if (currentDirection == 1) {
        x += 1;
      } if (currentDirection == 2) {
        y -= 1;
      } if (currentDirection == 3) {
        x -= 1;
      }
    }
  }

  public void reset() {
    x = 0;
    y = 0;
    currentDirection = 0;
  }

  public String position() {
    return x + ":" + y;
  }
}

class Main {
  public static void main(String[] args) {
    Robot robot = new Robot();
    robot.move("FF");
    System.out.println(robot.position()); // 0,2
    robot.reset();
    System.out.println(robot.position()); // 0,0
    robot.move("FFRF");
    System.out.println(robot.position()); // 1,2
    robot.reset();
    robot.move("FFRRRFF");
    System.out.println(robot.position()); // -2,2
  }
}

How can I make this code more object oriented?

I would consider refactoring direction into an enum, instead of a magic number. This would allow to encapsulate the related logic in this type.

It might look like that:

    enum Direction {
        UP(0, 1),
        RIGHT(1, 0),
        DOWN(0, -1),
        LEFT(-1, 0);

        final int xVector;
        final int yVector;

        private final Direction FIRST = Direction.values()[0];
        private final Direction LAST = Direction.values()[Direction.values().length];

        Direction(int xVector, int yVector) {
            this.xVector = xVector;
            this.yVector = yVector;
        }

        Direction rotatedLeft() {
            return this == FIRST
                    ? LAST // cycle complete
                    : Direction.values()[ordinal() - 1]; // previous value
        }

        Direction rotatedRight() {
            return this == LAST
                    ? FIRST // cycle complete
                    : Direction.values()[ordinal() + 1]; // next value
        }
    }

And then the Robot code becomes (including some subjective clean up, unrelated to the object-oriented aspect itself):

class Robot {
    private int x;
    private int y;
    private Direction currentDirection;

    Robot() {
        this(0, 0);
    }

    Robot(int x, int y) {
        this.x = x;
        this.y = y;
        currentDirection = Direction.UP;
    }

    public void move(String moves) {
        for (char code : moves.toCharArray()) {
            // a matter of taste, I like to extract logic
            // out of looping constructs for clarity
            move(code);
        }
    }

    private void move(char code) {
        switch (code) {
            case 'R': {
                currentDirection = currentDirection.rotatedRight();
                break;
            }
            case 'L': {
                currentDirection = currentDirection.rotatedLeft();
                break;
            }
            case 'F': {
                // you'd call the println thing here.
                // as it's not part of the requirements I assumed it to be a debugging artifact
                this.x += currentDirection.xVector;
                this.y += currentDirection.yVector;
                break;
            }
        }
    }

    public void reset() {
        x = 0;
        y = 0;
        currentDirection = Direction.UP;
    }

    public String position() {
        return x + ":" + y;
    }
}

I think this is more object-oriented, and also an improvement.

You could take it one step further and encapsulate the x and y coordinates in a little standalone class (Position). Like so:

    class Position {
        static final Position DEFAULT = new Position(0, 0);

        final int x;
        final int y;

        public Position(int x, int y) {
            this.x = x;
            this.y = y;
        }

        public Position movedInto(Direction direction) {
            return new Position(x + direction.xVector, y + direction.yVector);
        }

        @Override
        public String toString() {
            return x + ":" + y;
        }
    }

Then in the Robot class you replace x and y fields with Position position, and you no longer recalculate the position in the Robot class, as you can simply go:

case 'F': {
    position = position.movedInto(currentDirection);
    break;
}

in the move method.

Plus:

    public void reset() {
        position = Position.DEFAULT;
        currentDirection = Direction.UP;
    }

    public String position() {
        return position.toString();
    }

Yet another idea would be to encapsulate the move codes themselves into a class or classes. It's sometimes hard to say where to draw the line, as making code more and more OO comes at the cost of more boilerplate, and can be a form of overengineering.

I admit I haven't tested my refactored version, I only approached the question in terms of code design.


Bash: call make when directory changes I usually like to use entr when developing on my own hardware to run commands when a directory changes.
However, sometimes I'm working in an environment where I don't have much control over the availability of helpful programs so many of us take for granted.
I've been using Git Bash on Windows, and I've been getting tired of hunting down my build terminal, pressing up in my last history to get to the build command, and hitting enter to build my project. I want it to automatically run when I save a new change. But I don't have entr or any of the other usual suspects (like inotifywait) to assist me.
So to overcome this problem, with a bit of Internet searching to assist, I wrote a Bash function to watch the src/ directory and build the project when anything in the directory changes (including timestamps, because -l):
monitor_and_build () {
  local sauce=$(ls -l $1)
  while [ true ] ;
  do
    sleep 1
    local newsauce=$(ls -l $1)
    if [[ "$sauce" != "$newsauce" ]]; then
      make
    fi
    local sauce="$newsauce"
  done
}

to be invoked:
$ monitor_and_build src/

I have a working directory Makefile that calls make -C src/.
Since I did it in such an ad-hoc way, I'm sure I missed a trick or there's some edge cases this has a problem with. Please review!
Bonus question: aside from the obvious "writing your own programs in Bash is error-prone and foolish" (I admit it took me a few drafts to get it working) - suppose I had entr available, and then what are the benefits of using a program like entr instead?

You're going to get false positives by comparing the output of ls -l.  In many locales, this command gives date stamps in different formats for newer files (day+time) and older files (year+day).  When any file transitions from "new" to "old" in this scheme, you'll get a spurious notification.  If you have the GNU implementation of ls, you can make it use a specific timestamp format (e.g. --time-style=full-iso) which might alleviate that problem.  Or just accept the false positives (make should be reasonably quick if nothing actually changed).
The usual problem of failing to quote arguments ($1 where you should have written "$1") will mess up arguments that contain whitespace or shell pattern characters.  Although that might be unlikely for your source directories, it's trivial to fix, and a good habit to have.
Testing that the string true is non-empty ([ true ]) is an unusual approach to making an infinite loop - most shell programmers would simply use the true command.  Actually, it's probably better to test the result of sleep, which returns non-zero if interrupted.
Instead of polling, you should be able to block on filesystem notification events.  Perhaps inotifywait meets your needs here?
If we use plain [ rather than [[, the code becomes portable (POSIX) shell, without needing Bash extensions, so is useful to more people.
Is it right that you want to run make in the current directory?  For some users, it might make more sense to make -C "$1".  You could be more flexible by allowing a command to be specified (here I've kept make as default):
monitor_and_build()
{
    local d=${1:-.}; shift
    test -d "$d" || return 1
    local sauce=$(ls -l --time-style=full-iso "$d")
    while sleep 1
    do
        local newsauce=$(ls -l --time-style=full-iso "$d")
        if [ "$sauce" != "$newsauce" ]
        then "${@:-make}"
        fi
        local sauce="$newsauce"
    done
}


Managing employee photos in a database I'm looking for a better way to manage employee photos for employee records in our database, and a way to query for the employee photos while having them on disk I have used a FileTable to store all the EmployeePhotos.

Then in my Employee View I created a join using substring, which I am concerned about. I am wondering if there is a better way of doing this.

Here's the View:

CREATE VIEW [emp].[ViewEmployees]
    AS SELECT
        ue.Id,
        ue.Active,
        ue.Comments,
        ue.Created,
        ue.IsLockedOut,
        ue.LastLoggedOn,
        ue.NameIdentifier,
        e.FirstName,
        e.MiddleName,
        e.LastName,
        e.DisplayName,
        e.Duties,
        e.EmailAddress,
        e.FullPart,
        e.JobTitle,
        e.LegacyId,
        e.OtherPhones,
        e.Phone,
        e.Profile,
        e.StartDate,
        e.ShowOnDirectories,
        ep.name as PhotoName,
        ep.file_stream as PhotoBytes
        FROM [security].[UserEntity] as ue
Full Join [emp].[Employees] as e on ue.Id = e.Id Left Outer Join emp.EmployeePhotos as ep on SUBSTRING(e.EmailAddress, 0, CHARINDEX('@', e.EmailAddress, 0)) = SUBSTRING (ep.name, 0, CHARINDEX('.', ep.name, 0)) where ue.UserEntityType = 0

When you add files to the folder, it automatically creates a record in the file table with a new guid.  We want to allow administrators access to that folder as a network share so they can drop new employee photos in it, or edit them.  As such I can't really think of a better way to join it on my employee view.

Or maybe I shouldn't have it in the view at all, and instead do a separate query in the application to the file table for the photo.

The FULL JOIN looks a bit odd for this query.  An INNER JOIN would be more appropriate if all employees have an associated UserEntity row (trusted foreign key relationship) and your intent is to return only employee user entity types, which seems to be the purpose of EntityType = 0 in the WHERE clause.

It is best to avoid applying functions to columns in JOIN and WHERE clauses because that prevents indexes on those columns from being used efficiently.  Columns should stand alone on at least one side of the comparison operator when possible.  If all photos have the same file extension and you don't have subdirectories in your file table, you can use the refactored example below to build the file namespace path and use the GETPATHLOCATOR function to return the unique path_locator of the photo.  The path_locator column is the file table primary key so this will be efficient and ensure only one row is returned per employee.

CREATE VIEW [emp].[ViewEmployees]
    AS SELECT
        ue.Id,
        ue.Active,
        ue.Comments,
        ue.Created,
        ue.IsLockedOut,
        ue.LastLoggedOn,
        ue.NameIdentifier,
        e.FirstName,
        e.MiddleName,
        e.LastName,
        e.DisplayName,
        e.Duties,
        e.EmailAddress,
        e.FullPart,
        e.JobTitle,
        e.LegacyId,
        e.OtherPhones,
        e.Phone,
        e.Profile,
        e.StartDate,
        e.ShowOnDirectories,
        ep.name as PhotoName,
        ep.file_stream as PhotoBytes
FROM [security].[UserEntity] as ue
JOIN [emp].[Employees] as e on
    ue.Id = e.Id
LEFT JOIN emp.EmployeePhotos as ep on
    ep.path_locator =
        GETPATHLOCATOR (
            FILETABLEROOTPATH ( 'emp.EmployeePhotos')
            + '\'
            + SUBSTRING(e.EmailAddress, 0, CHARINDEX('@', e.EmailAddress, 0))
            + '.jpg')
WHERE
    ue.UserEntityType = 0;

Alternatively, you could create a unique index on the file table name column and change the file table LEFT JOIN clause to:

WHERE ep.name = SUBSTRING(e.EmailAddress, 0, CHARINDEX('@', e.EmailAddress, 0))
            + '.jpg'

Regarding you question about retrieving the photo via a separate query, it would be best to return the image in the same query, assuming the application actually uses it.  If you have cases where this view is used but the photo isn't needed, make sure the PhotoName and PhotoBytes columns are omitted from the SELECT query column list when querying this view.  That should allow SQL Server to remove the unneeded file table from the query plan.


Appropriate separation of concerns for this case? This is a method that calls a web service, it uploads an audio file and fetches metadata back.

While the advantage is that there is only one method to call there are a few concerns about it :

There is some UI-related code (upload progress)
Some audio-related code (format detection)
Web-service related code (first purpose)

In the end, while it tries to be smart, it's completely tied to a particular audio library, it will report progress anyway and well, it doesn't only upload ...

How would you apply separation of concerns to this current method ?

Would transforming the fileFormat to a parameter be acceptable ?
What about handler, how can it be used without being tied to the upload process ?

I'm not necessarily asking for code, directions to some documentation on the subject is fine too.

public class TrackUpload
{
    public async Task<string> Upload(string apiKey, string path, ProgressEventHandler handler)
    {
        if (apiKey == null) throw new ArgumentNullException("apiKey");
        if (path == null) throw new ArgumentNullException("path");
        using (var stream = new ReadOnlyFileStream(path))
        {
            if (handler != null)
            {
                stream.ProgressChanging += handler;
            }

            string fileFormat = TrackFormatDetector.Detect(path);
            if (fileFormat == null)
            {
                throw new InvalidAudioFormatException();
            }

            var dataContent = new MultipartFormDataContent();
            dataContent.Add(new StringContent(apiKey), "api_key");
            dataContent.Add(new StringContent(fileFormat), "filetype");
            dataContent.Add(new StreamContent(stream), "track");
            var client = new HttpClient();
            client.DefaultRequestHeaders.UserAgent.Add(
                new ProductInfoHeaderValue(new ProductHeaderValue("xxxx", "1.0")));
            HttpResponseMessage response =
                await client.PostAsync(@"http://developer.echonest.com/api/v4/track/upload", dataContent);
            response.EnsureSuccessStatusCode();
            HttpContent content = response.Content;

            if (handler != null)
            {
                stream.ProgressChanging -= handler;
            }

            using (var reader = new StreamReader(await content.ReadAsStreamAsync()))
            {
                string s = await reader.ReadToEndAsync();
                return s;
            }
        }
    }
}

You should move fileFormat to parameters, this method is doing too much
Use IProgress<T> instead of handler to properly report progress back to the UI thread (see Enabling Progress and Cancellation in Async APIs for examples).
Add CancellationToken parameter if necessary


Streamreader file.Readline properly closing the stream Is this OK? (.Net 4.5)

try
{
    StreamReader sr = = new StreamReader(ValidFilePathName);
    line = file.ReadLine();
    While (line != null)
    {
        line = file.ReadLine();
    }
    sr.Close();
}
catch
{
    // blah Do nothing
}

I feel like there is something wrong with my pattern here. My concern is that when there is a file exception, does the stream get closed properly? I was taught to use using to set the stream, but Code Analysis tool does not like it. Perhaps there is something else. It seems to read the file OK, but I remember needing to use File stream and stream reader in the past... Was there a good reason for combining the two?

You always need to dispose of objects in the finally close to ensure that they are disposed correctly.

StreamReader sr = null;
try
{
    sr = = new StreamReader(ValidFilePathName);
    line = file.ReadLine();
    While (line != null)
    {
        line = file.ReadLine();
    }
}
catch
{
    // blah Do nothing
}
finally
{
   if (sr != null)
   {
       sr.Close();
   }
}


Increase security of sign up form code Does my PHP look secure enough for a sign up form?

<?php
$con=mysqli_connect("host","user","password","db_name");
$sql="INSERT INTO users values (?, ?, ?)";
if ($stmt = mysqli_prepare($con,$sql)) {
  mysqli_stmt_bind_param($stmt, "sss",
    $_POST["username"], $_POST["pwd"], $_POST["email"]);
  if (mysqli_stmt_execute($stmt)) {
    echo "User added!";
  }
}

It looks like you have a good and solid Insert, probably pretty safe, but this still allows a user to enter data into the database that could be run at a later time,  you should still check the input coming in before inserting it into the Database even though it has been "prepared".

The prepare just means that the insert will not break anything, but if you run a SQL Query on the information in that column where someone has entered SQL injection Characters it could still crash the database.

Please make sure the input is safe all the way around.  Alert the user that they cannot enter certain characters but be aware you are asking for an e-mail address there too. The Password should be encrypted, not sure whether you should do that here or in the Database itself, probably in the PHP, but that is another review altogether.


Testing new C++11 features with Hangman I wrote a Hangman game to try out some of the new features in C++11. I'm pretty new to C++, and I would like some good advice on how I can improve this code (in terms of conventions, bad/good practices, ...):

#include <iostream>
#include <fstream>
#include <vector>
#include <string>
#include <stdexcept>
#include <random>
#include <algorithm>

const unsigned initial_number_of_lives = 5;

// Returns a random word from the given words file.
std::string pick_word(const std::string& words_filename) {
  std::vector<std::string> words;
  std::ifstream words_file(words_filename.c_str());
  if (!words_file) {
    throw std::runtime_error("Couldn't open file.");
  }
  std::string word;
  do {
    std::getline(words_file, word);
    words.push_back(word);
  } while (!words_file.eof());

  std::uniform_int_distribution<uintmax_t> random_distribution(0, words.size() - 1);
  std::mt19937 random_engine(static_cast<std::mt19937::result_type>(std::time(nullptr)));
  uintmax_t word_index = random_distribution(random_engine);
  return words[word_index];
}

int main(int argc, const char *argv[]) {
  if (argc != 2) {
    std::cerr << "usage: " << argv[0] << " <words-file>\n";
    return 1;
  }

  std::string word;
  try {
    word = pick_word(argv[1]);
  } catch(...) {
    std::cerr << argv[0] << ": couldn't open words file: " << argv[1] << '\n';
    return 1;
  }

#if DEBUG
  std::cout << "(debug) word: " << word << "\n\n";
#endif

  std::cout << "Welcome to hangman!"; // Newline in main loop below.

  unsigned lives = initial_number_of_lives;
  std::vector<char> guessed_letters;
  for (;;) {
    bool won = true;
    for (char &letter : word) {
      if (std::find(guessed_letters.begin(),
                    guessed_letters.end(),
                    letter) == guessed_letters.end()) {
        won = false;
      }
    }
    if (won) {
      std::cout << "You won the game!!1\n";
      return 0;
    }

    std::cout << "\n\nLives left: " << lives;

    std::cout << "\nAlready guessed: ";
    for (char &letter : guessed_letters) {
      std::cout << letter << ' ';
    }

    std::cout << '\n';
    for (char &letter : word) {
      if (std::find(guessed_letters.begin(),
                    guessed_letters.end(),
                    letter) != guessed_letters.end()) {
        std::cout << letter;
      } else {
        std::cout << '_';
      }
    }

    char guess;
    std::cout << "\nEnter a letter: ";
    std::cin >> guess;
    guess = std::tolower(guess);
    // Don't allow player to enter the same letter twice.
    if (std::find(guessed_letters.begin(),
                  guessed_letters.end(),
                  guess) != guessed_letters.end()) {
      std::cout << "You have already guessed that letter!\n";
      continue;
    }

    // Don't allow player to enter anything but letters.
    if (!std::isalpha(guess)) {
      std::cout << "That is not a letter!\n";
      continue;
    }

    guessed_letters.push_back(guess);

    bool word_contains_letter = false;
    for (char &letter : word) {
      if (letter == guess) {
        word_contains_letter = true;
        break;
      }
    }

    if (word_contains_letter) continue;
    --lives;
    if (lives == 0) {
      std::cout << "Game over! The right word was: " << word << '\n';
      break;
    }
  }

  return 0;
}

A few thoughts:

Instead of

do {
    std::getline(words_file, word);
    words.push_back(word);
} while (!words_file.eof());

You should do

while(std::getline(words_file, word))
    words.push_back(word);
}

Otherwise, the read which causes the stream to go into an invalid state will still be pushed.

Also, why not include the filename that cannot be opened in the exception text and then output the exception text instead of the hard-coded text you have now.  As things are, you may be ignoring other exceptions (although I don't see when one would happen).  Perhaps output the exception.what() in addition to what you have now.

I would also advise splitting some of the game logic into functions -- you don't need them in multiple places as things are, but it could be easier to follow.  Things like the piece setting word_contains_letter would look better as function calls, in my opinion.

Oh, another thing:  you may want to use #ifndef NDEBUG rather than #ifdef DEBUG, as that is the default way to check whether debugging is enabled or not.


Is this UnitTest for updating an object in data-access layer sensible? I'm still trying to learn how to write good tests (and write testable code).

This is the function I want to verify,

 void IAutoTisDal.UpdateRange(Range range)
  {
     using (var repo = _repositoryFactory.GetRepository(_baseline))
     {
        var rangeqry = from r in repo.Query<Range>()
                       where r.Id == range.Id
                       select r;
        var rangeToUpdate = rangeqry.SingleOrDefault();
        if (rangeToUpdate == null)
           throw new Exception(string.Format("Range ID {0} not found", range.Id));

        rangeToUpdate.Status = range.Status;
        rangeToUpdate.FirstPsn = range.FirstPsn;
        rangeToUpdate.LastPsn = range.LastPsn;
        rangeToUpdate.PageCount = (int)(range.LastPsn - range.FirstPsn);
        rangeToUpdate.CourierId = range.CourierId;
        rangeToUpdate.WorkflowServer = range.WorkflowServer;
        rangeToUpdate.DoubleFeedInd = range.DoubleFeedInd;

        repo.Commit();
     }
  }

And the unit test:

  [TestMethod]
  public void UpdateRange_RangesPropertiesAreCopied()
  {
     var rangeToUpdate = new Range()
     {
        Id = 1, CourierId = 0, Status = 20,
        WorkflowServer = null,
        FirstPsn = -1, LastPsn = -1, PageCount = 0
     };
     var rangeToSave = new Range()
     {
        Id = 1, CourierId = 1234, Status = 40,
        WorkflowServer = new PhysicalStation(){ Id = 1, Name = "fakeserver"},
        FirstPsn = 123, LastPsn = 456, PageCount = 333
     };
     var repoMock = new Mock<IRepository>();
     repoMock.Setup(x => x.Query<Range>()).Returns(new List<Range>(new[] {rangeToUpdate}).AsQueryable()).Verifiable();
     repoMock.Setup(x => x.Commit()).Verifiable();
     var factoryMock = new Mock<IRepositoryFactory>();
     factoryMock.Setup(x => x.GetRepository(It.IsAny<string>())).Returns(repoMock.Object);
     IAutoTisDal dal = new AutoTisDal(factoryMock.Object, "");

     dal.UpdateRange(rangeToSave);

     repoMock.VerifyAll();

     Assert.AreEqual(rangeToUpdate.Id, rangeToSave.Id);
     Assert.AreEqual(rangeToUpdate.CourierId, rangeToSave.CourierId);
     Assert.AreEqual(rangeToUpdate.LastPsn, rangeToSave.LastPsn);
     Assert.AreEqual(rangeToUpdate.FirstPsn, rangeToSave.FirstPsn);
     Assert.AreEqual(rangeToUpdate.PageCount, rangeToSave.LastPsn - rangeToSave.FirstPsn);
     Assert.AreSame(rangeToUpdate.WorkflowServer, rangeToSave.WorkflowServer);
     Assert.AreEqual(rangeToUpdate.Status, rangeToSave.Status);
  }

I know I need to write another test against the expected exception if no RangeId matches.

I also realize I'm writing test after the code.

First of all, your single method is doing multiple things:

Get Range by id
Copy properties from one instance to another
Commit

Let's try to separate responsibilities:

 Range GetRangeById(Repository repo, int id)
 {
    return repo.Query<Range>().SingleOrDefault(r=>r.Id == id);
 }

 void UpdateRange(Range rangeToUpdate, Range range)
 {
        // I'd even make this Range extension method
        rangeToUpdate.Status = range.Status;
        rangeToUpdate.FirstPsn = range.FirstPsn;
        rangeToUpdate.LastPsn = range.LastPsn;
        rangeToUpdate.PageCount = (int)(range.LastPsn - range.FirstPsn);
        rangeToUpdate.CourierId = range.CourierId;
        rangeToUpdate.WorkflowServer = range.WorkflowServer;
        rangeToUpdate.DoubleFeedInd = range.DoubleFeedInd;
 }

 void IAutoTisDal.UpdateRange(Range range)
  {
     using (var repo = _repositoryFactory.GetRepository(_baseline))
     {
        var rangeToUpdate = GetRangeById(range.Id);
        if (rangeToUpdate == null)
           throw new Exception(string.Format("Range ID {0} not found", range.Id));

        UpdateRange(rangeToUpdate, range);
        repo.Commit();
     }
  }

Two new methods are already easier to understand and test.
And, honestly, I don't think that writing test for IAutoTisDal.UpdateRange makes much sense. It won't provide much value.


Read and write in C# buffer in parallel I have a buffer - int array with defined length. I want to read and write values into this array in parallel. For instance, 5 readers and 3 writers with different intervals. Additionally, I want to block waiting readers and waiting writers until a specific operation (read or write) is available.

Buffer:

public class IntBuffer
{
    private readonly AutoResetEvent _readDone = new AutoResetEvent(false);
    private readonly AutoResetEvent _writeDone = new AutoResetEvent(false);

    private readonly int[] _array;
    private int _index;

    public IntBuffer(int bufferSize)
    {
        _array = new int[bufferSize];
        _index = -1;
    }

    public void Write(int insert)
    {
        while (true)
        {
            if (_index >= -1 && _index < _array.Length - 1)
            {
                try
                {
                    Monitor.Enter(this);

                    if (_index >= -1 && _index < _array.Length - 1)
                    {
                        _array[++_index] = insert;
                        return;
                    }
                }
                finally
                {
                    Monitor.Exit(this);
                    _readDone.Set();
                }
            }

            Console.WriteLine("waiting for add...");
            _writeDone.WaitOne();
        }

    }

    public int Read()
    {
        while (true)
        {
            if (_index >= 0)
            {
                try
                {
                    Monitor.Enter(this);
                    if (_index >= 0)
                    {
                        var val = _array[_index--];
                        return val;
                    }
                }
                finally
                {
                    Monitor.Exit(this);
                    _writeDone.Set();
                }
            }

            Console.WriteLine("waiting for read...");
            _readDone.WaitOne();
        }
    }
}

Reader:

public class Reader
{
    private readonly IntBuffer _buff;

    public Reader(IntBuffer buff)
    {
        _buff = buff;
    }

    public void Start(int sleepMs)
    {
        while (true)
        {
            Thread.Sleep(sleepMs);
            Console.WriteLine($"reader read {_buff.Read()}");
        }
    }
}

Writer:

public class Writer
{
    private readonly IntBuffer _buff;
    private readonly Random _random = new Random(Guid.NewGuid().GetHashCode());

    public Writer(IntBuffer buff)
    {
        _buff = buff;
    }

    public void Start(int sleepMs)
    {
        while (true)
        {
            Thread.Sleep(sleepMs);
            int val = _random.Next(1, 500);
            _buff.Write(val);
            Console.WriteLine($"writer write {val}");
        }
    }
}

All in one:

var buffer = new IntBuffer(8);

new Thread(() => new Writer(buffer).Start(1700)).Start();
new Thread(() => new Writer(buffer).Start(2900)).Start();
new Thread(() => new Writer(buffer).Start(3450)).Start();

new Thread(() => new Reader(buffer).Start(5000)).Start();
new Thread(() => new Reader(buffer).Start(3000)).Start();
new Thread(() => new Reader(buffer).Start(1400)).Start();

Please review and add remarks.

What you are trying to archive is very similar to the BlockingCollection. Note that you can look at the .net framework's source code to get an idea how to implement it professionally ;).

The overall implementation seems to work and I think the blocking mechanism does what it is supposed to.

However, some points to note:

Do not use the instance of the buffer as synchronization object (Monitor.Enter(this);) because the object can be locked by someone else from outside... Create a private sync object instead.
In method Read, the temporary variable val is not required - just return the value.
I would rename the class to make clear, that the call of Write may block the current thread (maybe BlockingBuffer, BlockingStack or something like that).
Why not making the class generic?


A bash script to submit an AWS EMR job and poll until the cluster terminates #!/bin/bash
set -ue

# use AWS CLI to launch a cluster
create_cluster_response=$(aws emr create-cluster \
  --release-label emr-4.3.0 \
  --region us-east-1 \
  --etc) # many more arguments; see http://docs.aws.amazon.com/cli/latest/reference/emr/create-cluster.html

# use jq to parse and display result
cluster_id=$(echo "${create_cluster_response}" | jq -r '.ClusterId')
echo "ClusterId=${cluster_id}"
echo "AWS Management Console URL: https://console.aws.amazon.com/elasticmapreduce/home?region=us-east-1#cluster-details:${cluster_id}"

# block until the EMR cluster terminates
previous_state=''
while true; do
  json=$(aws emr describe-cluster --cluster-id ${cluster_id}) # see http://docs.aws.amazon.com/cli/latest/reference/emr/describe-cluster.html
  state=$(echo "${json}" | jq -r '.Cluster.Status.State')
  # echo state changes
  if [[ ${state} != ${previous_state} ]]; then
    now=$(date -u +"%Y-%m-%dT%H:%M:%SZ") # format date as ISO 8601 UTC
    message=$(echo "${json}" | jq -r '.Cluster.Status.StateChangeReason.Message')
    echo "${now} ${state} ${message}"
    previous_state="${state}"
  fi
  # terminate this script when the cluster terminates
  if [[ ${state} == TERMINATED ]]; then
    echo "EMR JOB SUCCESS"
    exit 0
  fi
  if [[ ${state} == TERMINATED_WITH_ERRORS ]]; then
    echo "EMR JOB FAILURE"
    exit 1
  fi
  sleep 10
done

I am very new to programming bash and will appreciate any and all feedback.

you can use emr wait instead of describe-cluster

aws emr wait cluster-running --cluster-id <value> --region <value>

http://docs.aws.amazon.com/cli/latest/reference/emr/wait/cluster-running.html


Parser for gleaning data from twitter I've written a script in python to parse the name, tweets, following and follower of those people available in view all section in my profile page of twitter. My scraper is able to parse those aforesaid fields flawlessly. Any input on the improvement of my parser will be highly appreciated. Here is what I've written:

from selenium import webdriver
import time

def browsing_pages():

    driver = webdriver.Chrome()
    driver.get('https://twitter.com/?lang=en')

    driver.find_element_by_xpath('//input[@id="signin-email"]').send_keys('username')
    driver.find_element_by_xpath('//input[@id="signin-password"]').send_keys('password')
    driver.find_element_by_xpath('//button[@type="submit"]').click()
    time.sleep(5)

    #Clicking the viewall link
    driver.find_element_by_xpath("//small[@class='view-all']//a[contains(@class,'js-view-all-link')]").click()
    time.sleep(5)

    for links in driver.find_elements_by_xpath("//div[@class='stream-item-header']//a[contains(@class,'js-user-profile-link')]"):
        scraping_docs(links.get_attribute("href"))
        #tracking down each profile links under viewall section

def scraping_docs(item_link):

    driver = webdriver.Chrome()
    driver.get(item_link)

    # gleaning information of each profile holder
    for prof in driver.find_elements_by_xpath("//div[@class='route-profile']"):
        name = prof.find_elements_by_xpath(".//h1[@class='ProfileHeaderCard-name']//a[contains(@class,'ProfileHeaderCard-nameLink')]")[0]
        tweet = prof.find_elements_by_xpath(".//span[@class='ProfileNav-value']")[0]
        following = prof.find_elements_by_xpath(".//span[@class='ProfileNav-value']")[1]
        follower = prof.find_elements_by_xpath(".//span[@class='ProfileNav-value']")[2]
        print(name.text, tweet.text, following.text, follower.text)

        driver.quit()

browsing_pages()

I'd focus on 3 main things:

don't use time.sleep() to wait for elements on a page. With hardcoded time delays there is a tendency to wait more than actually needed most of the time and less than needed sometimes - not reliable at all. Instead, use Explicit Waits with WebDriverWait class and a set of Expected Conditions
remove the overhead of firing up a separate Chrome instance - collect the links into a list a reuse the same WebDriver instance - you should expect improvements in page load times as well
improve your locators - XPath locators are generally the slowest - use "by id" locators whenever possible; handling class attributes with CSS selectors is more reliable (raw contains() in XPath may generate false positives - it can be a bit better with concat())

In the end, you should have something like this:

from selenium import webdriver
from selenium.webdriver.common.by import By
from selenium.webdriver.support.wait import WebDriverWait
from selenium.webdriver.support import expected_conditions as EC

def browsing_pages(driver):
    driver.get('https://twitter.com/login')
    wait = WebDriverWait(driver, 10)

    driver.find_element_by_css_selector('form.signin input.email-input').send_keys('username')
    driver.find_element_by_css_selector('form.signin input.js-password-field').send_keys('password')
    driver.find_element_by_css_selector('form.signin button[type=submit]').click()

    view_all = wait.until(EC.element_to_be_clickable((By.CSS_SELECTOR, ".view-all a.js-view-all-link")))
    view_all.click()

    # wait for a profile link to become visible
    wait.until(EC.visibility_of_element_located((By.CSS_SELECTOR, ".stream-item-header a.js-user-profile-link")))

    links = [link.get_attribute("href") for link in driver.find_elements_by_css_selector(".stream-item-header a.js-user-profile-link")]
    for link in links:
        scraping_docs(driver, link)

def scraping_docs(driver, item_link):
    driver.get(item_link)

    # gleaning information of each profile holder
    for prof in driver.find_elements_by_css_selector(".route-profile"):
        name = prof.find_element_by_css_selector("h1.ProfileHeaderCard-name a.ProfileHeaderCard-nameLink")
        tweet = prof.find_element_by_css_selector(".ProfileNav-value")

        _, following, follower = prof.find_elements_by_css_selector(".ProfileNav-value")[:3]
        print(name.text, tweet.text, following.text, follower.text)

if __name__ == '__main__':
    driver = webdriver.Chrome()

    try:
        browsing_pages(driver)
    finally:
        driver.quit()

You may also go for a class with the driver instance kept at self.driver, but I think it's pretty much okay to do it this "functional" way considering that you have only two functions. Good read on the subject:

Stop Writing Classes
Start Writing More Classes


Mapping several URLs to one login page My htaccess has this block in it:

RewriteRule ^login$ _user/login.php
RewriteRule ^login\.php$ _user/login.php
RewriteRule ^_user/login$ _user/login.php
RewriteRule ^_user/login\.php$ _user/login.php

How do I consolidate this block? The objective is to point to _user/login.php regardless of where on the server it's called from.

The last rule is completely superfluous, but harmless: it just maps the desired URL to itself.

The four cases can be combined into one rule:

RewriteRule ^(_user/)?login(\.php)?$ _user/login.php

In a regular expression, (something)? makes the something optional.  That is, whether the URL starts with _user/ or not, and whether the URL ends with .php or not, it will get mapped to the desired login page.


Implementing lock within a Python script I've written a script in Python using multiprocessing to handle multiple process at the same time and make the scraping process faster. I've used locking within it to prevent two processes from changing its internal state. As I'm very new to implement locking within multiprocessing, I suppose there is room for improvement.

What the scraper does is collect the name, address and phone number of every coffee shop traversing multiple pages from Yellowpage listings.

import requests
from lxml.html import fromstring
from multiprocessing import Process, Lock

link = "https://www.yellowpages.com/search?search_terms=coffee&geo_location_terms=Los%20Angeles%2C%20CA&page={}"
itemstorage = []

def get_info(url,lock,itemstorage):
    response = requests.get(url).text
    tree = fromstring(response)
    for title in tree.cssselect("div.info"):
        name = title.cssselect("a.business-name span")[0].text
        try:
            street = title.cssselect("span.street-address")[0].text
        except IndexError: street = ""
        try:
            phone = title.cssselect("div[class^=phones]")[0].text
        except IndexError: phone = ""
        itemstorage.extend([name, street, phone])
    return printer(lock,itemstorage)

def printer(lock,data):
    lock.acquire()
    try:
        print(data)
    finally:
        lock.release()

if __name__ == '__main__':
    lock = Lock()
    for i in [link.format(page) for page in range(1,15)]:
        p = Process(target=get_info, args=(i,lock,itemstorage))
        p.start()

General Feedback

This code is fairly straight-forward and easy to read. There are only three functions and none of them extend beyond thirteen lines. If you really wanted more atomic functions you could abstract some functionality from get_info, for instance the code that parses each listing.

The name link doesn’t feel as appropriate for a string literal that represents a URL as something like url_template. Also see the related section below about naming constants.

The description was somewhat vague and didn’t specify whether each listing should correspond to an individual list within the returned list but if so, one could use itemstorage.append() instead of itemstorage.extend().

Suggestions

While it isn’t a requirement, it is recommended that each function have a docstring.

  All modules should normally have docstrings, and all functions and classes exported by a module should also have docstrings. Public methods (including the __init__ constructor) should also have docstrings.1

Additionally, while constants aren’t really different than variables in python, idiomatically uppercase letters are used for the naming constants in python as well as many other languages. As was mentioned above, url_template feels more appropriate for the string currently named link so it may improve readability to use uppercase letters to signify that is a constant: URL_TEMPLATE.

1https://www.python.org/dev/peps/pep-0257/


First solution to FizzBuzz I'm a first year college student, Computer Science major. This is my crack at the FizzBuzz interview question in Java. What kind of improvements could I make?

//Prints numbers 1 - 100, 25 values per line.
//Numbers that are multiples of 3 are replaced with Fizz
//Numbers that are mulitples of 5 are replaced with Buzz
//Numbers that are multiples of both 3 and 5 are replaced with FizzBuzz

public class FizzBuzz {

    public static void main(String[] args) {

        String fizz = "Fizz";
        String buzz = "Buzz";
        String fizzBuzz = "FizzBuzz";

        //Loop from 1 through 100
        for (int i = 1; i <= 100; i++) {

            if (i % 3 == 0 && i % 5 == 0) {          //Checks for numbers that are both multiples of 5 and 3
                if (i % 25 != 0) {                   //Control flow allows 25 values printer per line
                    System.out.printf("%4.8s ", fizzBuzz);
                }
                else {
                    System.out.printf("%4.8s\n ", fizzBuzz);
                    System.out.println();
                }

            }

            else if (i % 3 == 0) {                   //Checks for numbers that are multiples of 3
                if (i % 25 != 0) {                   //Control flow allows 25 values printer per line
                    System.out.printf("%4.8s ", fizz);
                }
                else {
                    System.out.printf("%4.8s\n ", fizz);
                    System.out.println();
                }

            }

            else if (i % 5 == 0) {                   //Checks for numbers that are multiples of 5
                if (i % 25 != 0) {                   //Control flow allows 25 values printer per line
                    System.out.printf("%4.8s ", buzz);
                }
                else {
                    System.out.printf("%4.8s\n ", buzz);
                    System.out.println();
                }

            }

            else {                                   //Prints numbers that are not multiples of 3 or 5
                if (i % 25 != 0) {                   //Control flow allows 25 values printer per line
                    System.out.print(i + " ");
                }
                else {
                    System.out.println(i + " ");
                    System.out.println();
                }
            }
        }
    }
}

System.out.printf("%4.8s ", ...); is actually not necessary here: this code means that you will format your String so that it has at minimum 4 characters, at most 8. Since "Fizz", "Buzz" and "FizzBuzz" have a length between 4 and 8, this will result in just outputing the String with no change. Therefore, it is simpler to have System.out.print(... + " ");.
You have one part of your code that is repeated 4 times. Code duplication is something you want to avoid at all costs. It is better to refactor this duplicated logic into a small reusable method that you can then call elsewhere.
In the same way, you can store inside two booleans the result of whether the number of divisible by 3 and 5.

In this case, you are repeating the check to print only 25 values per line, when you could do this only a single time after all the if/else logic.

You can then write your code a lot more simply:

for (int i = 1; i <= 100; i++) {
    boolean shouldFizz = i % 3 == 0;
    boolean shouldBuzz = i % 5 == 0;
    if (shouldFizz && shouldBuzz) { // Checks for numbers that are both multiples of 5 and 3
        System.out.print(fizzBuzz);
    } else if (shouldFizz) { // Checks for numbers that are multiples of 3
        System.out.print(fizz);
    } else if (shouldBuzz) { // Checks for numbers that are multiples of 5
        System.out.print(buzz);
    } else { // Prints numbers that are not multiples of 3 or 5
        System.out.print(i);
    }
    System.out.print(" ");
    if (i % 25 == 0) { // Control flow allows 25 values printer per line
        System.out.println();
        System.out.println();
    }
}


Python finding next word permutation with function generator Learning how to use function generators in Python I practiced with the following code that finds the next permutation of a word and then uses this new word as input for the next one until there are no further permutations. So for example 'dbac' is followed by 'dacb' followed by 'dabc' and so on.

Basically two questions: 1) the use of two times break to stop the two for-loops is there a better way to break out of the loops, and 2) the final part try: return finally: StopIteration is this a proper construction?

def nextperm(word):

    stoploop = True
    while stoploop:
        wordlist = [*word]
        length = len(wordlist)-1
        stoploop = False
        for index_1 in range(length-1, -1, -1):
            for index_2 in range(length, index_1-1, -1):
                if wordlist[index_2] < wordlist[index_1]:
                    wordlist[index_2], wordlist[index_1] = wordlist[index_1], wordlist[index_2]
                    _first = wordlist[0:index_1+1]
                    _second = wordlist[-1:index_1:-1]
                    wordlist = _first + _second
                    word = ''.join(wordlist)
                    stoploop = True
                    yield word
                    break
            if stoploop:
                break

    try:
        return print('no further permutation possible')
    finally:
        raise StopIteration

so if you do:

for i, word in enumerate(nextperm('dcba')):
    print(f'{i:2}, {word}')

output is:

 0, dcab
 1, dbca
 2, dbac
 3, dacb
 4, dabc
 5, cdba
 6, cdab
 7, cbda
 8, cbad
 9, cadb
10, cabd
11, bdca
12, bdac
13, bcda
14, bcad
15, badc
16, bacd
17, adcb
18, adbc
19, acdb
20, acbd
21, abdc
22, abcd
no further permutation possible

1. Review

The name nextperm is misleading as (i) it generates permutations that precede its argument; (ii) it generates multiple permutations, not just one. So a name like preceding_permutations would be clearer.
There is no need for try: ... finally: or raise StopIteration or return. If we try to get additional values from a generator that has finished, then Python automatically raises StopIteration. For example:

>>> def generator():
...     yield 1
>>> g = generator()
>>> next(g)
1
>>> next(g)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
StopIteration

It's not necessary to convert word to a list on each iteration of the while loop, because on subsequent iterations we already have this list in the variable wordlist.
All the permutations are the same length, so it is not necessary to reassign length on each loop.
This loop goes all the way down to index_1:

for index_2 in range(length, index_1-1, -1):

But when index_2 == index_1, the condition wordlist[index_2] < wordlist[index_1] can never be true. So this iteration is wasted. It would be better to write the second loop like this:

for index_2 in range(length, index_1, -1):

Reversed ranges are quite tricky to reason about, and this kind of mistake is easy to make, so we should consider using reversed instead to make things simpler:

for index_2 in reversed(range(index_1 + 1, length + 1)):

(And similarly for index_1.)
When I read the name "length", I expect this to be the length of the word, not the length of the word minus one. So it would be easier to follow the logic if we had length = len(wordlist), and adjusted the ranges accordingly.
The code reverses the last part of wordlist, leaving the first part unchanged:

_first = wordlist[0:index_1+1]
_second = wordlist[-1:index_1:-1]
wordlist = _first + _second

It would make sense to implement this using a slice assignment:

wordlist[index_1 + 1:] = reversed(wordlist[index_1 + 1:])

This has several advantages: we avoid having to take the slice for the first part of the list, we reuse the memory in wordlist, and the use of reversed makes it clear what is happenig and reduces the risk of getting the slice indexes wrong.
The names index_1 and index_2 are used a lot: it would shorten the code and make it easier to read if we had names like i and j.
The stoploop logic can be removed if we introduce a local function that generates pairs of indexes:

def indexes():
    for i in reversed(range(length - 1)):
        for j in reversed(range(i + 1, length)):
            yield i, j

Now the two loops over the indexes can be combined into one:

for i, j in indexes():

and since we now have a single loop, we can break out of it using break, with no need for stoploop.

2. Revised code

def preceding_permutations(word):
    """Generate the permutations of word that precede it, in reverse
    lexicographical order.

    >>> ' '.join(preceding_permutations('bdca'))
    'bdac bcda bcad badc bacd adcb adbc acdb acbd abdc abcd'

    """
    word = list(word)
    length = len(word)
    def indexes():
        for i in reversed(range(length - 1)):
            for j in reversed(range(i + 1, length)):
                yield i, j
    while True:
        for i, j in indexes():
            if word[j] < word[i]:
                word[j], word[i] = word[i], word[j]
                word[i + 1:] = reversed(word[i + 1:])
                yield ''.join(word)
                break
        else:
            break


How can I re-write the conditionals for this button I've been trying out React for a couple of days and I tried to build a button component with some conditionals. Everything works but I feel like I am not doing it the right way.
Could anyone review my code and give me some advice and tell me what I am doing wrong or what i could do better. I think I am using too many return statements.
I use Typescript for type checking.
Would appreciate any help!
import { Link } from "gatsby";

interface Props {
  content: string,
  color: string,
  path?: string,
  type?: string
}

export default function Button(props: Props): JSX.Element {
  let elementType;

  const checkProps = () => {
    const availableColors = ['blue', 'grey', 'dark-grey', 'white'];
    const borderColors = [''];
    let buttonStyle;

    // Check if props.color exists in availableColors
    if(!availableColors.includes(props.color)) {
      console.error(`"${props.color} color key not available for button style`);
      return;
    }

    buttonStyle = buttonStyles[`button--${props.color}`];

    // Check if props.content is valid
    if(!props.content) {
      console.error(`"${props.content} is not valid as content`);
      return;
    }

    // Check if props.type exists
    if(!props.type) {
      elementType = <button className={ buttonStyle }>{ props.content }</button>
      return;
    }

    if(props.type !== 'link') {
      console.error(`"${props.type} is not a valid type, use 'link' instead or use nothing for default button`);
      return;
    } else {
        elementType = <Link to={ props.path } className={ buttonStyle }>{ props.content }</Link>
    }
  }

  checkProps();

  return (
    <React.Fragment>
      { elementType }
    </React.Fragment>
  );
}```

Narrow types with TypeScript instead of erroring There are many places in the code where you check a condition based on how the Button is being called, and if it isn't fulfilled, you log an error and return nothing. This is a reasonable approach in JavaScript, but in TypeScript, you have a much better option, which is to use types to require only certain arguments. This way, rather than an error being thrown at runtime, an error will be thrown by TypeScript at compile-time when a user of Button attempts to pass props that aren't valid. Turning typos / accidental incorrect function calls into compile-time errors is one of the greatest advantages of TypeScript, and it's great that you're using TypeScript already, so go ahead and take advantage of it.
For example, rather than:
const availableColors = ['blue', 'grey', 'dark-grey', 'white'];
const borderColors = [''];
let buttonStyle;

// Check if props.color exists in availableColors
if(!availableColors.includes(props.color)) {
  console.error(`"${props.color} color key not available for button style`);
  return;
}

You can make availableColors as a type instead, and require that the color prop matches it:
'blue' | 'grey' | 'dark-grey' | 'white'
Similarly, rather than
if(!props.content) {
  console.error(`"${props.content} is not valid as content`);

you can require that the content property is of type:
T & (T extends '' ? 'Content must not be empty' : {})

and instead of
if(props.type !== 'link') {

require a type of 'link' | '' exactly.
Using the above methods will also let you declare buttonStyle and elementType with const - or you could even inline their values into the props.
Default export? You have:
export default function Button

This will mean that the imported button is not really tied to its Button name anymore. For example, it would be easy for someone to initially type:
import button from './Button';

only to run into problems since they forgot to capitalize it. Using named exports makes these sorts of typos more difficult, because one has to intentionally rename it to get something other than Button:
// Natural import
import { Button } from './Button';

or
// Deliberate renaming
import { Button as RenamedVariableName } from './Button';

Destructure immediately Rather than referencing props multiple times in the function body, you might consider extracting the properties from the props immediately in the function definition - it makes the logic later less noisy.
Let TS infer types when possible There's no need to note that a function returns a particular type if

TypeScript can already infer the type automatically, and
The type is obvious to a reader of a code at a glance

So, you may consider removing the JSX.Element from (props: Props): JSX.Element.
buttonStyles? It sounds like buttonStyles is an object that contains strings, where the strings are class names. So buttonStyles isn't a very precise variable name - it doesn't contain styles, it contains class names. Maybe instead call it buttonClassNamesByColor?
Fragments In newer versions of React, you may use <> and </> instead of <React.Fragment> </React.Fragment>. Or, even better, since you're returning only a single JSX element regardless, you can use the conditional operator to return either one or the other element, without wrapping everything in a fragment.
export const Button = <T extends string>({
    content,
    color,
    path,
    type,
}: {
    content: T & (T extends '' ? 'Content must not be empty' : {});
    color: 'blue' | 'grey' | 'dark-grey' | 'white';
    path?: string;
    type: '' | 'link';
}) => {
    const buttonClassName = buttonClassNamesByColor[`button--${color}`];
    return type === ''
        ? <button className={buttonClassName}>{content}</button>
        : <Link to={path} className={buttonClassName}>{content}</Link>;
}