Reverse a Linked List in place I am solving some programming challenges for learning Javascript. The challenge I chose is:

  Given the head of a singly linked list, reverse it in-place.

Here is my implementation in JS

// Given the head of a singly linked list, reverse it in-place.
const ReverseLinkedList = {
    reverseLinkedList: (/** @type{ListNode} */ head) => {
        if (head === undefined) {
            return undefined;
        }

        if (head.next === undefined) {
            return head;
        }

        let temp = head;
        let tail = head.next.next;

        head = temp.next;
        head.next = temp;
        head.next.next = undefined;

        while (tail !== undefined) {
            let tempNext = tail.next;

            temp = head;
            head = tail;
            head.next = temp;

            tail = tempNext;
        }

        return head;
    },
    ListNode: function ListNode(/** @type{ListNode} */ next, val) {
        return {
            next: next,
            val: val
        }
    }
};

module.exports = ReverseLinkedList;

and my test class

const expect = require('chai').expect;
const reverseLinkedList = require('../../src/reverseLinkedList');

let head;
let node_01;
let node_02;
let reversed;

node_02 = reverseLinkedList.ListNode(undefined, 2);
node_01 = reverseLinkedList.ListNode(node_02, 1);
head = reverseLinkedList.ListNode(node_01, 0);

reversed = reverseLinkedList.reverseLinkedList(head);
expect(reversed.val).to.equal(2);
expect(reversed.next.val).to.equal(1);
expect(reversed.next.next.val).to.equal(0);
expect(reversed.next.next.next).to.equal(undefined);

node_01 = reverseLinkedList.ListNode(undefined, 1);
head = reverseLinkedList.ListNode(node_01, 0);
head.next = node_01;

reversed = reverseLinkedList.reverseLinkedList(head);
expect(reversed.val).to.equal(1);
expect(reversed.next.val).to.equal(0);
expect(reversed.next.next).to.equal(undefined);

head = reverseLinkedList.ListNode(undefined, 0);
reversed = reverseLinkedList.reverseLinkedList(head);
expect(reversed.val).to.equal(0);
expect(reversed.next).to.equal(undefined);

Being a Javascript newbie, any comments on the styling and best practices? Also any comments on the algorithm itself is highly appreciated.

I'm not a big fan of your type-hinting comments.  Let JavaScript be JavaScript.  If you want typing, practice TypeScript instead.

I find it counterintuitive that the ListNode constructor takes its next parameter before its val parameter.  Putting val first is more natural (see my piDigits example below), and also allows next to be omitted for the last node in a list.

You can use the {val, next} syntactic shorthand.

As for the reverseLinkedList algorithm, there is too much code, with too many special cases.  The fact that you mention .next.next, reaching two elements ahead, is a sign that you aren't thinking about the loop invariant the right way.  You want the code to consider just the local situation at the head.

It's not really appropriate to explicitly set a variable to undefined; undefined is supposed to represent the fact that no assignment has taken place.  In any case, you should be more lenient than checking for … === undefined.

I find that it's nearly always unhelpful to use temp as a variable name or in a variable name.

const ListNode = (val, next) => { return {val, next}; };

const reverseLinkedList = (head) => {
    let newTail = null;
    while (head) {
        let next = head.next;
        head.next = newTail;
        newTail = head;
        head = next;
    }
    return newTail;
};

let piDigits = ListNode(3, ListNode(1, ListNode(4, ListNode(1, ListNode(5)))));
console.log(reverseLinkedList(piDigits));

let emptyList = null;
console.log(reverseLinkedList(emptyList));


My version of C++ quicksort I built my code according to the algorithm provided in this YouTube video. Please review it and help me find any problems. It definitely takes a lot longer than the qsort() function in C++. But the algorithm I use should be just as fast. Can anyone find problems?

#include <iostream>
#include <time.h>

using namespace std;

void randomize( int* array, int size );
void qsort( int* array, int leftbound , int rightbound );
void swap( int &a, int &b );
void output( int* array, int size );

int main()
{
    //declare array
    int array[100];

    //get size of the array
    int size = sizeof(array)/sizeof(int);

    //randomize value
    randomize(array, size);

    //start the quicksort algorithm
    qsort(array, 0, size-1 );

    //output the array
    output(array, size);

    return 0;
}

void randomize( int* array, int size )
{
    srand((int)(time(0)));
    for ( int i = 0 ; i < size ; i++ ) {
        array[i] = rand();
    }
}

void output( int* array, int size )
{
    for ( int i = 0 ; i < size ; i++ ) {
        cout << array[i] << endl;
    }
}

void qsort( int* array, int leftbound , int rightbound )
{
    if (rightbound > leftbound) {
        int pivot = array[leftbound + (rand() % (rightbound - leftbound + 1))];
        int leftposition = leftbound;
        int rightposition = rightbound;
        while ( leftposition != rightposition ) {
            while ( array[leftposition] < pivot ) {
                leftposition++;
            }
            while ( array[rightposition] > pivot ) {
                rightposition--;
            }
            swap ( array[leftposition], array[rightposition] );
        }
        qsort( array, 0, leftposition - 1 );
        qsort( array, leftposition + 1, rightbound );
    }
}

void swap(int &a, int &b)
{
    int tmp = a;
    a = b;
    b = tmp;
}

Please stop using:

using namespace std;

It may seem like a time saver. But for anything that isn't a toy program, it causes problems with collisions. So, it is a bad habit to get into. Stop before it becomes a habit.

You have alternatives.
You can selectively bring stuff into the current scope:

using std::cout;
using std::vector;

Alternatively (my preference) you prefix stuff in the the standard library with std::. The name std was chosen so it was short enough that people would not be hampered with typing it.

Prefer to use the C++ version of the headers:

#include <time.h>
// Prefer
#include <ctime>

Note: This also puts the functions in the standard namespace.

This is correct:

int size = sizeof(array)/sizeof(int);

But it makes the code brittle. If you change the type of the array. You also need to remember to change type here in the size expression. It is best to let the compiler deduce this so use:

int size = sizeof(array)/sizeof(array[0]);

Usage:

qsort(array, 0, size-1 );

Here you are making right bound point at the rightmost item. In C++ it is so common that the end points one past the end of the data it may make the code slightly harder to grok. I would stick with the standard convention of pointing one past the end. This means there will be some adjustment in your qsort but not that much and it makes it easier for C++ users to read.

Prefer pre-increment:

++leftposition;  // rather than leftposition++;

Though it makes no difference in this situation. There are situations where it can. So it one of those habits you should get into. Then when the types of objects are changed you do not need to search your code to make sure you are using the efficient version you know you used the efficient version automatically.

Your test for a sortable array is not aggressive enough:

if (rightbound > leftbound) {

You only don't sort if the array is zero length. Which may potentially lead to long recursion as you randomly choose a pivot point that always dives stuff to one side (luckily your non standard code below saved you by always leaving the pivot off subsequent sorts).

The real answer is you only need to sort arrays that have a size >= 2. (size zero and one are already sorted).

if ((rightbound - leftbound /* +1 depending on if you rightbound is one past or not*/ ) >= 2)

Small difference from a standard implementation here:
Which can potentially give you an infinite loop.

        while ( array[leftposition] < pivot ) {
            leftposition++;
        }
        while ( array[rightposition] > pivot ) {
            rightposition--;
        }

What happens if the value is equal to the pivot.
In that case it will go to a random side. Also if both left and right side hit a value that is equal to pivot then you will swap them and restart the loop which will do nothing swap them and restart the loop (etc)

        while ( array[leftposition] <= pivot ) {
            leftposition++;
        }
        while ( array[rightposition] > pivot ) {
            rightposition--;
        }

Your left bound of the first recursive call is wrong.

    qsort( array, 0, leftposition - 1 );
    qsort( array, leftposition + 1, rightbound );

Should be leftbound.

    qsort( array, leftbound, leftposition - 1 );
    qsort( array, leftposition + 1, rightbound );

Since there can potentially be more than one value equal to pivot. Maybe you can remove the all from the side you put them in.

You need to look at the standard libs.

We have a std::swap()

void swap(int &a, int &b)

// use
std::swap(val1, val2)

We have a way to print stuff:

void output( int* array, int size )

// use
std::copy(std::begin(array), std::end(array), std::ostream_iterator<int>(std::cout, "\n"));

We have a way to generate values:

void randomize( int* array, int size )

// use
generate(std::begin(array), std::end(array), ::rand);

This is what I got:

#include <iostream>
#include <iterator>
#include <algorithm>
#include <ctime>

void qsort( int* array, int leftbound , int rightbound );

int main()
{
    //declare array
    int array[100];

    //get size of the array
    int size = sizeof(array)/sizeof(array[0]);

    //randomize value
    std::srand(std::time(NULL));
    std::generate(std::begin(array), std::end(array), std::rand);

    //start the quicksort algorithm
    qsort(array, 0, size);

    //output the array
    std::copy(std::begin(array), std::end(array), std::ostream_iterator<int>(std::cout, "\n"));
}

void qsort( int* array, int leftbound , int rightbound )
{
    if ((rightbound - leftbound) >= 2)
    {
        int pivot = array[leftbound + (rand() % (rightbound - leftbound))];
        int leftposition    = leftbound;
        int rightposition   = rightbound - 1;

        while ( leftposition < rightposition )
        {
            while ((array[leftposition] <= pivot) && (leftposition < rightposition))
            {   ++leftposition;
            }
            while ((array[rightposition] > pivot ) && (leftposition < rightposition))
            {   --rightposition;
            }
            std::swap(array[leftposition], array[rightposition]);
        }
        // At least the pivot point will be on the left hand side.
        // This will also be the largest value. So move the leftposition back
        // to remove all the pivot points.
        while(((leftposition-1) > leftbound) && (array[leftposition-1] == pivot))
        {   --leftposition;
        }
        qsort(array, leftbound, leftposition-1);    // leftposition is one past the end of the left
        qsort(array, rightposition+1, rightbound);  // Thus at the start of the right.
    }
}


Admin and Client "is a" user First of all, I'd want to represent, in my application, two different users (admin and client) which can access to two different classes, through two different graphic interfaces...

My idea is to represent faithfully what is in the e/r and I'd want to generalize shared information (like nick, pw, email) using an interface and represent specific information into the two sub-classes

To store information, I use a database and a piece of the e/r is:

Now, in my code, I've implemented it in this way:

Utente entity  - represented using Interface:

package users;

/**
 * Interface which represent a general user (both admin either client)
 *
 * @author Federico Cuozzo
 *
 */
public interface User {
    public String getNick();
    public String getPw();
    public String getEmail();
}

Admin entity  - represented using Class with Override:

package users;

import global.Globals;
import java.util.ArrayList;

/**
 * Represents the admin user instance and his all information.
 *
 * @author Federico Cuozzo
 *
 */
public class Admin implements User {
    private String nick;
    private String pw;
    private String email;
    private ArrayList<Integer> authorizations = new ArrayList<Integer>();   // List of authorizations rwx in integer (r-- 4, rwx 7)

    public Admin(String aNick, String aPw, String anEmail) {
        this.nick = aNick;
        this.pw = aPw;
        this.email = anEmail;
        this.authorizations.add((int) Globals.r.nextInt((7-4) + 1) + 4);
        this.authorizations.add((int) Globals.r.nextInt((7-4) + 1) + 4);
        this.authorizations.add((int) Globals.r.nextInt((7-4) + 1) + 4);
    }

    // Accessor methods for private fields
    @Override
    public String getNick() { return this.nick; }
    @Override
    public String getPw() { return this.pw; }
    @Override
    public String getEmail() { return this.email; }
    public ArrayList<Integer> getAuthorizations() { return this.authorizations; }
}

Client entity  - represented using Class with Override:

package users;

/**
 * Represents the client user instance and his all information.
 *
 * @author Federico Cuozzo
 *
 */
public class Client implements User {
    private String nick;
    private String pw;
    private String name;
    private String surname;
    private String email;
    private String street;
    private String city;
    private String cap;
    private String phoneNumber;

    public Client(String aNick, String aPw, String anEmail, String aName, String aSurname, String aStreet, String aCity, String aCap, String aPhoneNumber) {
        this.nick = aNick;
        this.pw = aPw;
        this.name = aName;
        this.surname = aSurname;
        this.email = anEmail;
        this.street = aStreet;
        this.city = aCity;
        this.cap = aCap;
        this.phoneNumber = aPhoneNumber;
    }

    // Accessor methods for private fields
    @Override
    public String getNick() { return this.nick; }
    @Override
    public String getPw() { return this.pw; }
    @Override
    public String getEmail() { return this.email; }
    public String getName() { return this.name; }
    public String getSurname() { return this.surname; }
    public String getStreet() { return this.street; }
    public String getCity() { return this.city; }
    public String getCap() { return this.cap; }
    public String getPhoneNumber() { return this.phoneNumber; }
}

Is this correct? Using an interface to represent main class and two concrete classes to represent sub-classes...

While the implementation looks logically correct, some remarks about code style:

package name should include your organization, e.g. com.your.company.users, com.your.company.global
there is no need to prefix arguments with a, you can use the same name as the object fields, since through this, it can be determined which one is referred.

E.g:

public Client(String nick, ...) {
    this.nick = nick;
    // ...
}


Handling exceptions and executing code if one was found I have this code:

try
{
    Console.WriteLine("Input number {0}", i + 1);
    int number = ReadNumber(start, end);
}
catch (Exception ex)
{
    if (ex is ArgumentException)
    {
        Console.WriteLine("The input number wasn't in the alowed range. Please try again.");
    }
    else if (ex is FormatException)
    {
        Console.WriteLine("The input isn't an integer number. Please try again.");
    }
    else
    {
        Console.WriteLine("An unknown error occured. Please try again.");
    }

    i--;
}

I am wondering if there is a better way to do this - catch all exceptions and handle them and then, if any exception was caught, I want to reduce i by 1.

So I'm assuming that all of this code is in a for loop, and that for loop is incrementing i as it's loop statement.

What you can do here is not unconditionally increment i in each iteration of the loop, and instead only increment i if you're able to read in the number successfully:

for (int i = 0; i < 10; )
{
    try
    {
        Console.WriteLine("Input number {0}", i + 1);
        int number = ReadNumber(start, end);
        i++;
    }
    catch (ArgumentException)
    {
        Console.WriteLine("The input number wasn't in the alowed range. Please try again.");
    }
    catch (FormatException)
    {
        Console.WriteLine("The input isn't an integer number. Please try again.");
    }
    catch (Exception)
    {
        Console.WriteLine("An unknown error occured. Please try again.");
    }
}

However, for this case I'd say that there's an option that's better still.

What you should really do is, rather than having a method try to get another value from the user and possibly failing and possibly succeeding and needing to be called again, create a method that keeps asking the user for a value until it gets a valid one.  This logic is far easier to implement at that scope than it is to try to continually add more iterations to the loop.

Once you create a method that will always get a valid value, trying as hard as it needs to:

public static int ReadNumber(int start, int end, int iteration)
{
    while (true)
    {
        Console.WriteLine("Input number {0}", iteration + 1);
        int n;
        if (!int.TryParse(Console.ReadLine(), out n))
            Console.WriteLine("The input isn't an integer number. Please try again.");
        else if (n < start || n > end)
            Console.WriteLine("The input number wasn't in the alowed range. Please try again.");
        else
            return n;
    }
}

(Also note the use of conditional checks to determine if the user input is valid, rather than using exceptions for non-exceptional control flow.)

Now our loop is dead easy:

for (int i = 0; i < 10; i++)
{
    int number = ReadNumber(start, end, i);
}


Generic toString() for JPA base entity class In a Java/JPA project I usually define a base class (on which I put the id). The goal is to implement an generic toString() on this base class (named BaseEntity) from which all other entities inherit. We played around with apache commons ReflectionToStringBuilder, but we ran into problems because of cyclic references and the lack of different handling depending on the type of the fields.

My wish is that, if an Entity is printed/logged I get all field names and their values except for fields of type collection, in this case we just want the IDs of these objects to avoid recursion (PS: We can assume that collections on entities always contain other entities and they have an ID)

The desired output pattern is the following

classname[fieldname1=value1, fieldname2=value2,..., collectionname1=[id1,id2,..], collectionname2=[id1,id2,...],...]

a sample output for an entity "Project" which subclasses BaseEntity may look like this:

Project[nameLong=Testproject, nameShort=tpr, support=false, active=true, priority=0, blockers=[20], responsibilities=[13,14,15]]

We came up with the following implementation using reflection:

@Override
public String toString() {
    // init container for field names
    List<String> fieldToPrintout = new ArrayList<String>(0);

    // get all fields from this object (use declared fields to get private
    // and protected fields)
    Field[] fieldsOnTheObject = this.getClass().getDeclaredFields();

    // set the accessibility for field which are private
    AccessibleObject.setAccessible(fieldsOnTheObject, true);

    for (Field field : fieldsOnTheObject) {
        try {
            // ignore static fields
            if (!Modifier.isStatic(field.getModifiers())) {
                // get the object from the field
                Object fieldValue = field.get(this);
                // check if the field is a collection
                if (Collection.class.isAssignableFrom(field.getType())) {
                    // check if the collection generic type is BaseEntity
                    if (field.getGenericType() instanceof ParameterizedType) {
                        Class<?> genericType = (Class<?>) ((ParameterizedType) field.getGenericType()).getActualTypeArguments()[0];
                        if (BaseEntity.class.isAssignableFrom(genericType)) {
                            // iterate over all BaseEntities and get all id's
                            List<String> ids = ((Collection<BaseEntity>) fieldValue).stream().map(p -> p.getId().toString()).collect(Collectors.toList());
                            fieldToPrintout.add(field.getName() + "=[" + String.join(",", ids) + "]");
                        }
                    }
                } else { // --> no collection
                    // note: Field.get(Object) creates wrapper objects for primitive
                    fieldToPrintout.add(field.getName() + "=" + fieldValue);
                }
            }
        } catch (IllegalAccessException ex) {
            logger.catching(ex);
        }
    }
    return MessageFormat.format("{0}[{1}]", this.getClass().getSimpleName(), String.join(", ", fieldToPrintout));
}

I'm currently not really happy with the code, I'm sure it can be improved/simplified. Any suggestions?

So, there are a couple of things that can be done with this. First, use methods! Short, well-named methods can make it much easier to read the code. Second, use short-circuiting logic. continue is your friend. Most of your comments don't really contribute much, or can be obviated with good method names. You can also set the size of the ArrayList at creation time, because you know how many fields you're going to be dealing with. In my opinion, something like this looks a lot cleaner:

@Override
public String toString() {

    final Field[] fields = this.getClass().getFields();
    final List<String> fieldsToPrintOut = new ArrayList<String>(fields.length);

    AccessibleObject.setAccessible(fields, true);

    for (final Field field : fields) {
        if (Modifier.isStatic(field.getModifiers())) {
            continue;
        }

        final Object fieldValue;
        try {
            fieldValue = field.get(this);
        } catch (final IllegalAccessException ex) {
            logger.catching(ex);
            continue;
        }

        if (!isCollection(field)) {
            fieldsToPrintOut.add(field.getName() + "=" + fieldValue);
            continue;
        }

        if (!isParameterized(field) || !isBaseEntity(field)) {
            continue;
        }

        final List<String> ids =
                ((Collection<BaseEntity>) fieldValue).stream().map(p -> p.getId().toString()).collect(Collectors.toList());
        fieldsToPrintOut.add(field.getName() + ids);

    }
    return MessageFormat.format(this.getClass().getSimpleName() + fieldsToPrintOut);
}

private static boolean isCollection(final Field field) {
    return Collection.class.isAssignableFrom(field.getType());
}

private static boolean isParameterized(final Field field) {
    return field.getGenericType() instanceof ParameterizedType;
}

private static boolean isBaseEntity(final Field field) {
    final Class<?> genericType =
            (Class<?>) ((ParameterizedType) field.getGenericType()).getActualTypeArguments()[0];

    return BaseEntity.class.isAssignableFrom(genericType);
}


search for text across multiple tables I've got the following query.

SELECT xcli.ID_X_Table, cli.ID, cli.Presentation, COALESCE(cli.MobilePhone, cli.BusinessPhone, cli.HomePhone), EmailAddress, cli.Descr
FROM B_Client cli
INNER JOIN X_Table xcli ON xcli.TableName = 'B_Client' AND ISNULL(cli.Flg_Deleted, 0) = 0
WHERE cli.EmailAddress LIKE '%'+@SearchText+'%'
UNION
SELECT xcli.ID_X_Table
      , cli.ID
      , cli.Presentation
      , ISNULL(cli.MobilePhone, ISNULL(cli.BusinessPhone, cli.HomePhone))
      , cli.EmailAddress
      , cli.Descr
FROM B_Client cli
INNER JOIN X_Table xcli ON xcli.TableName = 'B_Client'
                        AND ISNULL(cli.Flg_Deleted, 0) = 0
WHERE RTRIM(ISNULL(cli.ClientName, '')) + ' ' + LTRIM(ISNULL(cli.FirstMiddleName, '')) LIKE '%'+@SearchText+'%'
    OR RTRIM(ISNULL(cli.FirstMiddleName, '')) + ' ' + LTRIM(ISNULL(cli.ClientName, '')) LIKE '%'+@SearchText+'%'
    OR SUBSTRING(RTRIM(ISNULL(cli.FirstMiddleName, '')), 0, PATINDEX('% %', RTRIM(ISNULL(cli.FirstMiddleName, '')))) + ' ' + LTRIM(ISNULL(cli.ClientName, '')) LIKE '%'+@SearchText+'%'
UNION
SELECT xcli.ID_X_Table
       , cli.ID
       , cli.Presentation
       , dbo.Get_B_Car_RegNum(car.ID, GETDATE()) as RegNum
       , car.Presentation
       , cli.Descr
FROM B_Car car
INNER JOIN X_Table xcli on xcli.TableName = 'B_Client'
INNER JOIN B_Client cli on cli.ID = [dbo].[Get_B_Car_ID_B_Client](car.ID, GETDATE())
WHERE car.CarPotential = 0
    AND (car.Descr LIKE '%'+@SearchText
        OR (SELECT TOP 1 RegNum FROM B_Car_RegNum
            WHERE ID_B_Car = car.ID
                AND Dte_Start <= GETDATE() ORDER BY Dte_Start DESC) LIKE '%'+@SearchText+'%')

This query searches for user-provided text across multiple tables (it's a sort of primitive search engine).
The unions overlap because they are added programmatically if the input is appropriate. For example if the input contains letters it won't search for telephone numbers and so on.

The query is quite slow in production. It takes minutes to retrieve the query. The tables contain hundreds of thousands entries. I'm not sure if this query performs poorly due to its design, or because of the magnitude of data.

Tips and critique are welcome.

You have a few different challenges that are affecting performance.

Expressions on the field used to filter, rather than converting the filtered result.
EX: RTRIM( ISNULL( cli.ClientName, '' )) + ' ' + LTRIM( ISNULL( cli.FirstMiddleName, '' )) LIKE '%' + @SearchText + '%'

AND  ISNULL( cli.Flg_Deleted, 0 ) = 0

Using a scalar valued function in your resultset

RegNum = dbo.Get_B_Car_RegNum( car.ID, GETDATE()),

Using a scalar valued function in a join

ON cli.ID = dbo.Get_B_Car_ID_B_Client( car.ID, GETDATE())

Using a wildcard at the start of the search string

car.Descr LIKE '%' + @SearchText

Possibly, the use of UNION rather than UNION ALL.

The expressions and wildcards negate the use of any indexes and force a table scan on the underlying table.
As an example changing AND  ISNULL( cli.Flg_Deleted, 0 ) = 0 to AND ( CLI.Flg_Deleted IS NULL OR cli.Flg_Deleted = 0 ) can allow the optimizer to use an index on Flg_Deleted because the desired value is not dependent on an expression

The scalar valued function turns your set based query into a procedural query, (one row at a time) for which SQL Server is not optimized.  See if you can convert this into an inline table-valued function.  You should also consider  dumping the result into a table variable and join on that.  That would at least prevent running the udf for every possible combination of your joined table.

Each UNION will check for duplicates in the top and bottom result sets.  If the data is guaranteed unique between the result sets, you'll gain some performance by switching to UNION ALL, removing the distinct sort across the combined results.


Emulating Python zip() Function I'm brushing up on Python, working my way through Python Workout. One of the exercises is to emulate the Python zip() function, assuming each iterable passed to the function is of the same length. Here's what I've come up with:
def myzip(*args):
    result = [[] for _ in range(len(args[0]))]
    for i, v in enumerate(args):
        for j, w in enumerate(v):
            result[j].append(w)

    for i, v in enumerate(result):
        result[i] = tuple(v)

    return result

While my function works, I have a hunch there must be a more succinct way of achieving the same thing (forgoing the nested loops). Is there? I'm running Python 3.9.

You should not apply len, because that requires that the first argument be at least as strong as a Collection, when zip should not need that guarantee and should only need Iterable which is a weaker type.
Gathering results in memory can be inefficient and lead to memory starvation. It's worth noting that an in-memory approach might actually execute more quickly than a generator but you would want to test this.
Attempt some type hinting. The amount of type-hinting you can do here is fairly limited, but you can do better than nothing.
It's also worth noting that the official documentation has a reference implementation.
Suggested
from typing import Any, Iterable, Tuple

def myzip(*args: Iterable[Any]) -> Iterable[Tuple[Any, ...]]:
    first_iter, *other_iters = (iter(a) for a in args)

    for first in first_iter:
        yield (first, *(
            next(other) for other in other_iters
        ))

for a, b in myzip([1, 2, 3], [4, 5, 6]):
    print(a, b)

Alternate
This version does not need to wrap the first argument in an iterator.
def myzip(first_iter: Iterable[Any], *args: Iterable[Any]) -> Iterable[Tuple[Any, ...]]:
    others = [iter(a) for a in args]

    for first in first_iter:
        yield (first, *(
            next(other) for other in others
        ))


Showing a unique result based on options selected I created a simple quiz in HTML/CSS and plain Javascript. There are three questions with two options each, so there are eight possible combinations of answers, and therefore results. You can only pick one option per question.

This is how I am currently assigning a result to each combination in Javascript. The input is done through HTML.

function GetResult(){
var result;
var inputs = document.getElementsByTagName('input');
for(var i = 0, i < inputs.length, i++){
if(inputs[i].checked){
result = result + inputs[i].value;
}}

if(result == 'NaturalHistoricalRelaxed'){
//result A
}
if(result == 'NaturalHistoricalFast'){
//result B
}
if(result == 'NaturalModernRelaxed'){
//result C
}
if(result == 'NaturalModernFast'){
//result D
}
if(result == 'UrbanHistoricalRelaxed'){
//result E
}
if(result == 'UrbanHistoricalFast'){
//result F
}
if(result == 'UrbanModernRelaxed'){
//result G
}
if(result == 'UrbanModernFast'){
//result H
}
}

My question is, is there a better way of assigning each result to each combination?

I know that many people would tell me to use a Javascript library, but I don't want to, so please do not write answers like that.

I am not familiar enough with JavaScript to write out the code while I am travelling, but I can at least describe a simple algorithm to do so generally in a faster and slightly simpler way. I here assume that you have access to the raw results and don't need to re-interpret them from the collected string.

For each answer we assign a 0 or 1 to the result, and then join them together as a bit mask, by shifting each answer by the 0-indexed numbering of the question it is an answer to. You can then construct an array of all the possible results,
and when you want to know which result a response corresponds to, you can just construct an integer from the above bit mask and use it as an index for the array's result you want. If you have unique results for every response, then this is about as good as you can do (remember you can put a function in the array and call it if you want to do the compilation lazily). If you can construct the results from some kind of pattern, then you can find the right indexes by filtering the indexes with and based bit masked (with not applied to the result if you want the ones which have 0's in those masks).


How to unit test this method? I have this method inside a class that is responsible for setting up and executing a model:

public FlowJob PrepareAndScheduleFlowModelJob(string coordinates)
{
    GeometryCoordinateString coordinateString =
        new GeometryCoordinateString(coordinates);
    DataSet intersectedWatershedsDataSet =
        FindIntersectingWatersheds(coordinateString);
    List<string> xCoords = new List<string>();
    List<string> yCoords = new List<string>();
    List<string> ascFiles = new List<string>();
    string submitJobID = "fromServa" + new Random().Next().ToString();
    ProcessWatershedResults(intersectedWatershedsDataSet, xCoords, yCoords, ascFiles);
    ScheduleFlowJob(xCoords, yCoords, ascFiles, submitJobID);
    return new FlowJob(submitJobID);
}

It is a long running process so the client caller uses the FlowJob to poll status and get results.  The client itself doesn't care about any of the intermediate data and only wants to specify the input location for the model to run.

Every method that is called inside this method have their own unit tests already to ensure those work properly.

I know I can write an integration test ensuring that the entire thing in fact works.

I don't see anything here to unit test except that various methods are called.  Any suggestions?

As far as testing the method itself:
In most testing frameworks, you would create parameterized tests that provide the inputs and expected outputs.

NUnit example below (omitting setup code to build ObjectUnderTest):

[TestCaseSource ("GetPrepTestData")]
public void TestPrepareAndScheduleFlowJob (string inputCoords, FlowJob expectedJob)
{
   var  outputJob = ObjectUnderTest.PrepareAndScheduleFlowModelJob (inputCoords);
   Assert.That (outputJob, Is.EqualTo (expectedJob));
}

private static IEnumerable<TestCaseData> GetPrepTestData ()
{
   yield return new TestCaseData ("some coord string", new FlowJob (expectedId)).SetName ("first test");
   yield break;
}

The private method calls can largely be ignored unless they have external dependencies, in which case you should add some injection and mocking with your parent object.  This will allow you to ensure your private method calls inside PrepareAndScheduleFlowJob execute quickly and return expected results.

edit:
The above solution requires that you override Object.Equals.  You could change the assert to a set of asserts for testing equality, but Object.Equals overrides are preferable, as it will cover FlowJob and any future/current subclasses in addition to providing equality check capability to anyone using the class.  Additionally, in a framework like NUnit, overriding Object.ToString is highly recommended, as many test frameworks will call ToString when printing assert failures, and having something more than just the type name is useful when assessing why your tests may have failed.


How do I make my Rock-Paper-Scissor program more JavaScript-like? I have implemented my first program in JavaScript, Rock-Paper-Scissors. I would like JavaScript developers to help me make this program more JavaScript-like by following the idioms of the language. I am also open to any suggestions related to simplification of logic and code readability.

The following is the complete listing of the program:

<html>
<head></head>
<body>
    <script>
        var choiceArray = ["paper", "rock", "scissors"];

        var getUserChoice = function()
        {
            var userChoice = undefined;
            var userChoiceIndex = undefined;

            while (true)
            {
                userChoice = prompt("What do you like - paper, scissor or rock");
                userChoiceIndex = choiceArray.indexOf(userChoice);

                if (userChoiceIndex >= 0 && userChoiceIndex <= 2)
                    return userChoice;

                alert("Invalid Choice!");
            }
        }

        var getComputerChoice = function()
        {
            var computerChoiceIndex = Math.floor(Math.random() * 3);
            return choiceArray[computerChoiceIndex];
        }

        var decideWinner = function(userChoice, computerChoice)
        {
            if (userChoice === computerChoice)
                return "There is a tie!";

            if (userChoice === "paper")
            {
                if (computerChoice === "rock")
                {
                    return "paper wins!";
                }
                else //scissors
                {
                    return "scissors win!";
                }
            }
            if (userChoice === "rock")
            {
                if (computerChoice === "paper")
                {
                    return "paper wins!";
                }
                else //scissors
                {
                    return "rock wins!";
                }
            }
            else //scissors
            {
                if (computerChoice === "paper")
                {
                    return "scissors wins!";
                }
                else //rock
                {
                    return "rock wins!";
                }
            }
        }

        var runGame = function()
        {
            var userChoice = getUserChoice();
            var computerChoice = getComputerChoice();
            var gameResult = decideWinner(userChoice, computerChoice);
            gameResult = "Computer Choice = " + computerChoice + ". " + gameResult;
            alert(gameResult);
        }

        runGame();
    </script>
</body>
</html>

Well, there might be a few things.

The most important thing that can be changed is removing all those magic numbers: What happens when we want to play “Rock, Paper, Scissors, Lizard, Spock” instead? You'd think you would only have to update the choiceArray – but unfortunately the length of that array is encoded in various places.

This means that if (userChoiceIndex >= 0 && userChoiceIndex <= 2) should be

if (userChoiceIndex >= 0 && userChoiceIndex <= choiceArray.length - 1)

and var computerChoiceIndex = Math.floor(Math.random() * 3) should be

var computerChoiceIndex = Math.floor(Math.random() * choiceArray.length);

Your decideWinner function isn't really extensible, because you encode the winning relationships as code, not as the data it actually is. I'd use an object literal that only encodes what choice wins over another, like:

var winningData = {
  paper:     { rock:     true },
  rock:      { scissors: true },
  scissors:  { paper:    true }
};

Also, I'd implement decideWinner as a comparator that returns one of -1, 0, 1 depending on whether the first or the second argument won. This decouples the decision logic from UI like text.

function(choiceA, choiceB) {
  if (choiceA === choiceB)           return  0;
  if (winningData[choiceA][choiceB]) return -1;
  if (winningData[choiceB][choiceA]) return  1;
  console.warn("No ordering between " + choiceA + " and " + choiceB + " was found");
  return 0;
}

The game result is then determined as:

var ordering = decideWinner(userChoice, computerChoice);
var winningMessage = ordering < 0 ? "You win!" :
                     ordering > 0 ? "The computer wins!" :
                                    "It's a tie!";
var gameResult = "You chose: "          + userChoice     + ".\n" +
                 "The computer chose: " + computerChoice + ".\n" +
                 winningMessage;

Now when we extend the game, we just have to update the winningData. Notice that I display who wins, not what choice wins, as I don't think that would be tremendously interesting to a player. Also, he would have to make an association from the choice to the player, which is unnecessary mental effort.


Disallow `rm` to remove `*` Note: If you googled this by the title of this question, don't use this script unless you know what it is supposed to do.

This is a script in bash 3+ that I have used for long for preventing rm * and rm -rf * from accidentally invoked and removing important files by mistake. I put it in my ~/.bash_aliases.

alias rm='set -f;rm'
rm(){
    if [[ "$-" == *i* ]]
    then
        if [ "$1" = "*" ] || [ "$2" = "*" ] || [ "$1" = "./*" ] || [ "$2" = "./*" ]
        then
            echo "Abort: refusing to remove *, please go to the parent folder and do rm <folder_name>/*" 1>&2
            set +f
            return 1
        fi
    fi
    set +f
    /bin/rm -i $@
}

set +f

I would like to know whether there are any vulnerabilities and whether it can be improved.

The vulnerability I see is that only the first two arguments are checked.  You could check all of them:

rm() {
    [[ $- == *i* ]] && for arg
    do
        if [[ $arg = "*" || $arg = "./*" ]]
        then
            # abort
       fi
   done
   # do the rm

This kind of checking will miss other dangerous wildcards like ** or ?*.  You can get safer checking by expanding * yourself, then see if the expanded arguments contain that same list of files:

# alias not needed here; we want globs to be expanded
rm() {
    declare -a star=(*)
    declare -a dotslashstar=(./*)
    if [[ "$@" == *"${star[@]}"*  ||  "$@" == *"${dotslashstar[@]}"* ]]
    then
         # abort

... but then you can't (for example) empty a directory full of temp files with rm *.tmp, if *.tmp and * match the same thing.


Double every 2nd item in a list Some exercise required me to write a piece of code to double every second item in a list. My first thought was just using cons and I came up with the following:

doubleSecond :: Num a => [a] -> [a]
doubleSecond [] = []
doubleSecond [x] = [x]
doubleSecond (x:y:xs) = x : (y * 2) : doubleSecond xs

While it's straightforward and does work, it just doesn't look that elegant and I'm curious how this could be rewritten in a more elegant fashion :)

Built-ins and generality

It is a good first try, but I discourage explicit recursion and suggest a larger use of built-ins, Haskell provides you with incredibly powerful and general functions. Also writing something a bit more general will make you familiar with first class functions (the core of the Haskell experience).

Decomposition

As always, I am going to divide the task in smaller, more manageable parts:

Find out how to apply a function to each second item of list.
Give multiply_by_two as argument to the function above.

mapSecond

mapSecond :: (a -> a) -> [a] -> [a]
mapSecond f = zipWith ($) (cycle [id, f])

Given a function and a list, apply the function to each second item of the list.

This function is quite advanced, let me explain it:

zipWith: given a function and two list applies the function to each pair. If you are familiar with Python, zip = zipWith (,)

Prelude> zipWith (++) ["Hello", "Foo"] ["World", "Bar"]
["HelloWorld","FooBar"]

$: given a function and two arguments, applies it to the arguments.

Prelude> ($) (*) 2 5
10

cycle: given a list repeats it forever.

Prelude> take 10 $ cycle [1,2,3]
[1,2,3,1,2,3,1,2,3,1]

The second argument is omitted, as it can be 'currried' (simplified) away.

The main function is now:

main = print $ ( mapSecond (* 2) ) [1,2,3,4,5,6]


Web scraping job postings from Facebook I built a web scraper to pull a list of jobs on Facebook and other websites but want to break up the code into functions that I can reuse for other websites. This structure is working but I think it can be more efficient with functions. I'm getting stuck on how to structure the functions. It's only pulling two pages for testing.

from time import time
from requests import get
from time import sleep
from random import randint
from IPython.core.display import clear_output
from warnings import warn
from bs4 import BeautifulSoup
import csv

# Range of only 2 pages
pages = [str(i) for i in range(1, 3)]
cities = ["Menlo%20Park%2C%20CA",
          "Fremont%2C%20CA",
          "Los%20Angeles%2C%20CA",
          "Mountain%20View%2C%20CA",
          "Northridge%2CCA",
          "Redmond%2C%20WA",
          "San%20Francisco%2C%20CA",
          "Santa%20Clara%2C%20CA",
          "Seattle%2C%20WA",
          "Woodland%20Hills%2C%20CA"]

# Preparing the monitoring of the loop
start_time = time()
requests = 0

with open('facebook_job_list.csv', 'w', newline='') as f:
    header = csv.writer(f)
    header.writerow(["Website", "Title", "Location", "Job URL"])

for page in pages:
    for c in cities:
        # Requests the html page
        response = get("https://www.facebook.com/careers/jobs/?page=" + page +
                       "&results_per_page=100&locations[0]=" + c)

        # Pauses the loop between 8 and 15 seconds
        sleep(randint(8, 15))

        # Monitor the frequency of requests
        requests += 1
        elapsed_time = time() - start_time
        print("Request:{}; Frequency: {} request/s".format(requests, requests/elapsed_time))
        clear_output(wait=True)

        # Throw a warning for non-200 status codes
        if response.status_code != 200:
            warn("Request: {}; Status code: {}".format(requests, response.status_code))

        # Break the loop if number of requests is greater than expected
        if requests > 2:
            warn("Number of requests was greater than expected.")
            break

        # Parse the content of the request with BeautifulSoup
        page_soup = BeautifulSoup(response.text, 'html.parser')
        job_containers = page_soup.find_all("a", "_69jm")

        # Select all 100 jobs containers from a single page
        for container in job_containers:
            site = page_soup.find("title").text
            title = container.find("div", "_69jo").text
            location = container.find("div", "_1n-z _6hy- _21-h").text
            link = container.get("href")
            job_link = "https://www.facebook.com" + link

            with open('facebook_job_list.csv', 'a', newline='') as f:
                rows = csv.writer(f)
                rows.writerow([site, title, location, job_link])

Some quick suggestions:

The requests module can urlencode strings for you if you use the params keyword:

import requests

cities = ["Menlo Park, CA"]
pages = range(1, 3)
url = "https://www.facebook.com/careers/jobs/"

for city in cities:
    for page in pages:
        params = {"page": page, "results_per_page": 100, "locations[0]": city}
        response = requests.get(url, params=params)

Organize your code using functions. This allows you to give them a readable name (and even add docstrings).

def get_job_infos(response):
    """Parse the content of the request to get all job postings"""
    page_soup = BeautifulSoup(response.text, 'lxml')
    job_containers = page_soup.find_all("a", "_69jm")

    # Select all 100 jobs containers from a single page
    for container in job_containers:
        site = page_soup.find("title").text
        title = container.find("div", "_69jo").text
        location = container.find("div", "_1n-z _6hy- _21-h").text
        job_link = "https://www.facebook.com" + container.get("href")
        yield site, title, location, job_link

This is a generator over which you can iterate. Using the lxml parser is usually faster.

Note that csv can write multiple rows at once using writer.writerows, which takes any iterable of rows:

with open('facebook_job_list.csv', 'a', newline='') as f:
    writer = csv.writer(f)
    writer.writerows(get_job_infos(response))

This way you only have to open the file once per page, instead of a hundred times. Even better would be to make the whole thing a generator, so you can write all rows while opening the file only once:

def get_all_jobs(url, cities, pages):
    for city in cities:
        for page in pages:
            params = {"page": page, "results_per_page": 100, "locations[0]": city}
            response = requests.get(url, params=params)
            # check status code

            yield from get_job_infos(response)

            # rate throttling, etc here
            ...

if __name__ == "__main__":
    cities = ["Menlo Park, CA", ...]
    pages = range(1, 3)
    url = "https://www.facebook.com/careers/jobs/"

    with open('facebook_job_list.csv', "w") as f:
        writer = csv.writer(f)
        writer.writerow(["Website", "Title", "Location", "Job URL"])
        writer.writerows(get_all_jobs(url, pages, cities))

This way the get_all_jobs generator will yield jobs as it is being iterated over, getting the next page when needed.


Regular expression for application version Only these formats are accepted.

1.1.1
1.1.1-r
1.1.1-b
1.1.1-r1
1.1.1-b1

I wrote this code. What don't I like in it? I used parentheses and now I have two groups. In fact, I don't need to do anything with groups. And there might be some bugs as well.

package com.sandbox;

import java.util.regex.Matcher;
import java.util.regex.Pattern;

public class Main {
/**
 * @param args
 */
public static void main(String[] args) {
    checkVersion("10.22.5-r");
}

private static void checkVersion(String current) {
    String regex = "([1-9]\\d*)\\.(\\d+)\\.(\\d+)-[b|r](?:[1-9]\\d*)*";
    Pattern pattern = Pattern.compile(regex);
    Matcher matcherCurrent = pattern.matcher(current);
    System.out.println("Matches: " + matcherCurrent.matches());
    System.out.println("The first digit: " + matcherCurrent.group(1));
    System.out.println("The second digit: " + matcherCurrent.group(2));
    System.out.println("The third digit: " + matcherCurrent.group(3));

    // Compare and decide whether an update is necessary
}

}

Thank you for the answers you give me. I'll vote up as soon as I have enough reputation.

It seems to me that you're using * when it looks like that's not the behaviour you want.

i.e. this bit -(b|r)* do you really mean a dash followed by either a 'b' or an 'r' zero or more times? I think it's more likely that you meant more along the lines of: there can be a dash followed by a 'b' or an 'r' but there doesn't have to be.

Do you want 1.1.1-brrrrrrrrrrr1 to match? What's the goal of the regex, is it just to see if it fits the pattern or do you want to use it to get the various numbers out?

Btw: If you don't care about groups you can use (?: ) instead of () to create non capturing groups.

Edit:

I think your regex needs to be more like: ^([1-9]\\d*)\\.(\\d+)\\.(\\d+)(?:-[br]{1}[0-9]?\\d*)?$ This makes everything following the first 3 numbers optional.

I do wonder whether a regex is the right option though... If you know the strings you are comparing are definitely well formed version numbers just do normal string manipulation.

// Pseudocode
String versionNumber = "1.1.1-r1";
int indexOfDash = versionNumber.indexOf("-");
if (indexOfDash != -1)
{
    versionNumber = versionNumber.substring(0, indexOfDash);
}
String[] numbers = versionNumber.split(".");

// Now compare your numbers.

I generally like to avoid regular expressions as they are a lot more difficult to get right than people give them credit for. However, if you also need to check that it is a well formed version number you probably want to use the regex option.

In fact, I got the regex wrong - edited now. I changed the matching for the final part of the version number and added in the anchors.


Was this shell script ready for open-source? (Random XKCD wallpaper) I wrote the following little shell script and published it on GitHub. In the repo there's also a README file that explains how to use it in more detail.

This is the first time I open-sourced/published something I wrote, so I'm interested whether you think that this code is ready for the public.

Some pointers, but feel free to add your own thoughts:

Does it look professional? If not, what would need to be done?
Are the comments too much? Was it ok to publish it, although I know that under some circumstances it doesn't work (I wrote about that in the README)?

#    random xkcd wallpaper: gets a random xkcd comic from xkcd.com and sets it as the desktop background.

#!/bin/bash

# /random/comic redirects to random comic. Wget gets the index.html of the comic.
wget http://dynamic.xkcd.com/random/comic/

echo $(pwd)

#Searches the line in the index.html file that points to the url where the actual comic is placed.
#The image urls are of the form: http://imgs.xkcd.com/comics/.'name of comic'.(png | jpg)
url=$(cat index.html | grep -o -m 1 http://imgs.xkcd.com/comics/.*\.png)

#Assuming picture format is .png. Gets the name of the image file by only matching what comes after the last forward slash.
name_pic=$(echo $url | grep -o [^/]*\.png)
is_png=1

#Sets url and name_pic in the case of the picture being in .jpg format.
if [ -z "$url" ]
then
    url=$(cat index.html | grep -o -m 1 http://imgs.xkcd.com/comics/.*\.jpg)
    name_pic=$(echo $url | grep -o [^/]*\.jpg)
    is_png=0
fi

#Downloads the image and saves it under its appropriate name.
wget --output-document="$name_pic"  "$url"

#Sets the desktop background
gconftool-2 --set --type=string /desktop/gnome/background/picture_filename $(pwd)/"$name_pic"

#Cleans up
rm index.html

#For some reason, if the image is moved to fast (e.g without a wait) the background does not get set.
sleep 1

#The current wallpaper can always be found under "current_xkcd_wallpaper.png" or "current_xkcd_wallpaper.jpg". Also prevents cluttering the directory with images.
#If you want to keep all the pictures you downloaded, uncomment the following lines.

#Cleans up. Makes sure that there is only one current_xkcd_wallpaper image file.
#rm current_xkcd_wallpaper.*

rm current_xkcd_wallpaper.*

if [ $is_png = 1 ] ; then
    mv $(pwd)/"$name_pic" $(pwd)/current_xkcd_wallpaper.png
else
    mv $(pwd)/"$name_pic" $(pwd)/current_xkcd_wallpaper.jpg
fi

#!/bin/bash

The shebang line has no effect unless it's on the very first line, so you should put it before the copyright notice.

echo $(pwd)

echo $(command) is almost always the same as just writing command (the exception being that echo adds a newline at the end if the command doesn't already produce one, but that's not the case here). So you can just change the above to pwd.

cat index.html | grep -o -m 1 http://imgs.xkcd.com/comics/.*\.png

That's useless use of cat. If you want to grep through a file, you can just pass the filename as the last argument - no need to cat and pipe. (The same goes for later places in the code where you do the same with .jpg).

gconftool-2 --set --type=string /desktop/gnome/background/picture_filename $(pwd)/"$name_pic"

I think it'd be a good idea to make the command to change the wallpaper configurable, so non-GNOME users can use your script too.

Also I'm not sure why you set the image to be the wallpaper before renaming it. I think it'd make more sense to do so afterwards (that might also solve your issue of having to sleep).

On a general note you might take care to gracefully handle already existing files. For example if the script is called in a directory where an index.html already exists (which is not that unlikely), your script will a) not work and b) delete that index.html, which its owner might not appreciate.


Longest word in dictionary that is a subsequence of a given string I am trying out the following former Google interview question.

  The Challenge Given a string S and a set of words D, find the longest
  word in D that is a subsequence of S.

  Word W is a subsequence of S if some number of characters, possibly
  zero, can be deleted from S to form W, without reordering the
  remaining characters.

  Note: D can appear in any format (list, hash table, prefix tree, etc.

  For example, given the input of S = "abppplee" and D = {"able", "ale",
  "apple", "bale", "kangaroo"} the correct output would be "apple"

  The words "able" and "ale" are both subsequences of S, but they are
  shorter than "apple". The word "bale" is not a subsequence of S
  because even though S has all the right letters, they are not in the
  right order. The word "kangaroo" is the longest word in D, but it
  isn't a subsequence of S. Learning objectives This question gives you
  the chance to practice with algorithms and data structures. It’s also
  a good example of why careful analysis for Big-O performance is often
  worthwhile, as is careful exploration of common and worst-case input
  conditions

My approach uses a greedy algorithm.

Sort D in descending order(longest word first)
Find the index of first character in a word
Scan S from indexOfFirstCharacter to find other characters in the word
If we reach to the end of string and there are still characters remaining to be seen in a word then the word is not found
Repeat 2-4 until we find a word

from collections import deque
D = ["able","ale", "apple", "bale", "kangaroo"]
"""
TC : DlogD
"""
if len(D) > 1:
  D.sort(key=len,reverse=True)

S = "abppplee"
s_dict_first = {}
"""
TC : O(S)
"""
for i,s in enumerate(S):
  if s not in s_dict_first: s_dict_first[s] = i

"""
TC : O(1)
"""
#return first index of char
def getindex(char):
  if char in s_dict_first:
    return s_dict_first[char]
  return -1

"""
TC : O(w + w +S) = O(w + s)
"""
def isSubstringPresent(startIndex,word):
  #word is too big to be found in S
  if len(word) - 2 > len(S) - 1 - startIndex:
    return False
  remaining_chars = list(word[1:])
  char_queue = deque()
  for char in remaining_chars:
    char_queue.append(char)
  for i in range(startIndex,len(S)):
    if len(char_queue) == 0:return True
    if S[i] == char_queue[0]:
      char_queue.popleft()
  return len(char_queue) == 0

"""
TC : O(DlogD + D(w + s))
"""
for word in D:
  first_char = word[0]
  indexStart = getindex(first_char) + 1
  if indexStart == 0:continue
  if isSubstringPresent(indexStart,word):
     print(word)
     break;

I am open to suggestion in improving the code / any bug in my time/space complexity analysis or  any other useful comments.

How about using a regex for S, where all characters are optional by appending a ? to each character:

import re

D = ["able","ale", "apple", "bale", "kangaroo"]
S = "abppplee"

regex = '^' + '?'.join(list(S)) + '?' + '$'

words = sorted(D, key=lambda word: len(word), reverse=True)

for word in words:
    if re.match(regex, word):
        print(word)
        break


Merge sort and selection sort comparison taking too long I have this python code that compares merge sort and selection sort, but it is taking "forever."

import time
import random

# Selection Sort Code #
def maxIndex(J):
    return J.index(max(J))

def swap(LCopy, i, j):
    temp = LCopy[i]
    LCopy[i] = LCopy[j]
    LCopy[j] = temp

# Implementation of selection sort
def selectionSort(L):
    for i in range(len(L)-1, 1, -1):
        j = maxIndex(L[0:i+1])
        swap(L, i, j)

# Merge Sort Code #
# Assumes that L[first:mid+1] is sorted and also
# that L[mid: last+1] is sorted. Returns L with L[first: last+1] sorted

def merge(L, first, mid, last):

    i = first # index into the first half
    j = mid + 1 # index into the second half

    tempList = []

    # This loops goes on as long as BOTH i and j stay within their
    # respective sorted blocks
    while (i <= mid) and (j <= last):
        if L[i] <= L[j]:
            tempList.append(L[i])
            #print L[i], "from the first block"
            i += 1
        else:
            tempList.append(L[j])
            #print L[j], "from the second block"
            j += 1

    # If i goes beyond the first block, there may be some elements
    # in the second block that need to be copied into tempList.
    # Similarly, if j goes beyond the second block, there may be some
    # elements in the first block that need to be copied into tempList
    if i == mid + 1:
        tempList.extend(L[j:last+1])
        #print L[j:last+1], "some elements in second block are left over"
    elif j == last+1:
        tempList.extend(L[i:mid+1])
        #print L[i:mid+1], "some elements from first block are left over"

    L[first:last+1] = tempList
    #print tempList

# The merge sort function; sorts the sublist L[first:last+1]
def generalMergeSort(L, first, last):
    # Base case: if first == last then it is already sorted

    # Recursive case: L[first:last+1] has size 2 or more
    if first < last:
        # divide step
        mid = (first + last)/2

        # conquer step
        generalMergeSort(L, first, mid)
        generalMergeSort(L, mid+1, last)

        # combine step
        merge(L, first, mid, last)

# Wrapper function
def mergeSort(L):
    generalMergeSort(L, 0, len(L)-1)

m = 10
n = 100000
n_increments = 9
baseList = [ random.randint(0,100) for r in range(n) ]

i = 0

while i < n_increments:
    j = 0
    sel_time = 0
    mer_time = 0

    while j < m:
        # Do a Selection Sort #
        x = time.clock()

        selectionSort( baseList)

        y = time.clock()

        sel_time += ( y - x )

        random.shuffle( baseList )

        # Do a Merge Sort #

        x = time.clock()

        mergeSort( baseList )

        y = time.clock()

        mer_time += ( y - x )

        random.shuffle( baseList )

        j += 1
    print "average select sort time for a list of", n, "size:", sel_time / m
    print "average merge sort time for a list of", n, "size:", mer_time / m

    j = 0
    i += 1
    n += 10000

The first reason that your code is slow is because you are implementing a selection sort which will be slow in any implementation. It has to scan through a large list looking for the values for each iteration. That's gonna be slow.

The second problem is that you freely create new lists. For example:

j = maxIndex(L[0:i+1])

The slice creates a new list and copies the elements in L into it. That works fine when you are dealing with small lists, but in the lists you are playing with here, thats gonna be a significant speed drain. For the most efficiency in cases like this you need to avoid creating new lists altogether and just move stuff between elements in the same list.

All in all, it isn't really much slower then I'd expect the algorithm you are implementing to be in Python. Nothing you are doing should produce pathologically bad performance. However, there is a reason why we use a sort implementation in C. If you are just interested in implementing algorithms for the learning experience, I suggest you use smaller lists. Thats a lot of data to sort with a bad algorithm.


Fizz Buzz interview question code Reading Jeff Atwood's "Why Can't Programmers.. Program?" led me to think that almost all of the solutions would not be something I would want in my own code-base.  The Fizz Buzz program specification from Imran is here which says:

  Write a program that prints the numbers from 1 to 100. But for
  multiples of three print “Fizz” instead of the number and for the
  multiples of five print “Buzz”. For numbers which are multiples of
  both three and five print “FizzBuzz”.

Implementing my solution differently than almost everyone else - I would like to put my solution up for review (as everyone doing it differently must think that my solution is not the best).

Could you please review my approach.  Is it overkill to suggest such a solution?  Under what conditions would you split it into a Model and Views?  Are there any issues with the code?

For a start, I don't think a language agnostic definition of the problem can fully describe the best solution.  In an interview I would find out by asking questions what was expected of the Fizz Bang code.  Probable questions and my assumed answers are:

Whether any changes were likely to be made in the future? (such as
new words, different rules other than 'Fizz' on multiples of 3 and 'Bang' on 5).    Yes
Whether the existing code-base was procedural or OO?    OO

I have decided to split the model and view components so that different output formats could be easily implemented.

Model:

class Model_Fizz_Buzz
{
   private $fizzBuzz;

   /** Construct the Fizz Buzz game model.
    *  @param fizzBuzz \Array The list of period => text for the game.  If the
    *  number is a multiple of the period then the text should be used.
    */
   public function __construct(Array $fizzBuzz=array())
   {
      $this->fizzBuzz = $fizzBuzz;
   }

   /** Get the Fizz_Buzz game numbers.
    *  @param start \int The number to start from (defaults to 1).
    *  @param end   \int The number to finish with (defaults to 100).
    */
   public function get($start=1, $end=100)
   {
      $data = array();

      for ($num = $start; $num <= $end; $num++)
      {
         $data[$num] = '';

         foreach ($this->fizzBuzz as $period => $text)
         {
            if ($num % $period === 0)
            {
               $data[$num] .= $text;
            }
         }

         if (empty($data[$num]))
         {
            $data[$num] = $num;
         }
      }

      return $data;
   }
}

View:

class View_Text_Lines
{
   /** Write the data values separated by newlines.
    *  @param data \array The data to be written.
    */
   public function write(Array $data)
   {
      foreach ($data as $val)
      {
         echo $val . PHP_EOL;
      }
   }
}

Usage would be:

$fizzView = new View_Text_Lines();

echo PHP_EOL . '-- Standard Fizz Buzz' . PHP_EOL;
$fizzBuzz = new Model_Fizz_Buzz(array(3 => 'Fizz',
                                      5 => 'Buzz'));
$fizzView->write($fizzBuzz->get());

echo PHP_EOL . '-- Three Buzz Bang' . PHP_EOL;
$threeBuzz = new Model_Fizz_Buzz(array(3 => 'Three',
                                       4 => 'Buzz',
                                       5 => 'Bang'));
$fizzView->write($threeBuzz->get(30, 61));

Is it overkill to suggest such a solution?

Yes, I think so :-)

  Under what conditions would you split it into a Model and Views?

If you need more than one view. For example, if the specification says that the program should be able to write the results to the screen, a CSV/PDF file, a network socket, a web service etc.

  Are there any issues with the code?

It's fine, just three issues:

Naming: I'd rename $data to $result.
Input checks:

What should happen when $start > $end? (Check and throw an exception.)
What should happen when $period is not a number? (Throw an exception in the constructor.)

Unit tests are missing.

  Whether any changes were likely to be made in the future?

I would not try predicting it. If there are some change requests refactor it. The tests will show whether there is a regression or not. On Programmers.SE there are same questions about it:

Design for future changes or solve the problem at hand
Future proofing code


Implementing Division of Streams (where it's a stream of polynomial coefficients) Per this homework, I'm trying to implement a Fractional (Stream Integer) where the stream represents coefficients of polynomials:

  where Q is defined as Q = (A'/b0) + x((1/b0)(A' − QB0)).

  where A/B = Q,

  That is, the first element of the result is A'/b0; the remainder is
  formed by computing A' − QB' and dividing each of its elements by b0

  Note that A' == tail of A and same for B'

Here's my attempt:

divStreams :: Stream Integer -> Stream Integer -> Stream Integer
divStreams a@(Cons x xs) b@(Cons y ys) =
    Cons (x `div` y) (streamMap ((1 `div` y) *) xs - (multStreams q ys) )
      where q = divStreams a b

with the following functions' definitions:

multStreams :: Stream Integer -> Stream Integer -> Stream Integer
multStreams (Cons x xs) b@(Cons y ys) =
    Cons (x * y) (streamMap (*x) ys + multStreams xs b)

data Stream a = Cons a (Stream a)

streamMap :: (a -> b) -> Stream a -> Stream b
streamMap f (Cons x xs) = Cons (f x) rest
   where rest = streamMap f xs

Note that I defined Num (Stream Integer) where (+) == combineStreams (+)

combineStreams :: (a -> b -> c) -> Stream a -> Stream b -> Stream c
combineStreams f (Cons x xs) (Cons y ys) = Cons (f x y) rest
   where rest = combineStreams f xs ys

Thanks to this answer that resolved an issue I had implementing multiplication of Streams.

There are a couple of cases where using where isn't gaining us anything. For instance, combineStreams can just be written as

combineStreams :: (a -> b -> c) -> Stream a -> Stream b -> Stream c
combineStreams f (Cons x xs) (Cons y ys) = Cons (f x y) (combineStreams f xs ys)

Same goes for streamMap.

If you're not using combineStreams anywhere else, we can just write

instance Num (Stream Integer) where
    (Cons x xs) + (Cons y ys) = Cons (x + y) (xs + ys)

I just want to repeat the definition given in the homework, for clarity:

  If \$A = a_0 + x A'\$ and \$B = b_0 + x B'\$, then \$A / B = Q\$,
  where \$Q\$ is defined as

  \begin{align} Q = (a_0 / b_0) + x((1 / b_0)(A' - QB')). \end{align}

Your definition of divStreams is very close to this, but we can use the following trick where q refers to itself, just as in the formula above:

divStreams :: Stream Integer -> Stream Integer -> Stream Integer
divStreams (Cons x xs) (Cons y ys) =
    let q = Cons (x `div` y) (streamMap ((1 `div` y) *) (xs - q * ys)) in q

assuming you have defined * to be multStreams.


Remove duplicates from string without additional buffer I completed an exercise from the Cracking the Coding Interview:

  Design an algorithm and write code to remove the duplicate characters in a string without using any additional buffer. NOTE: One or two additional variables are fine. An extra copy of the array is not.

  Follow-up: Write the test cases for this method.

I would like to ask whether my solution for the question is good enough, because the solution given in the book doesn't seem to work correctly.

import java.util.Arrays;

public class duplicateString{

    public static void rduplication(char [] word){
        if(word == null) return;
        int len = word.length;
        if(len<2) return;
        int count=0;
        for(int i =0;i<word.length;i++){
            for(int j=0;j<word.length;j++){
                if(word[i] == word[j]){
                    count = count + 1;
                    System.out.println(count);
                }
                if(word[i] == word[j] && count > 1){
                    word[j] =0;

                }

            }
            count = 0;
        }
        System.out.println(Arrays.toString(word));
    }

    public static void main(String []  args){
        String word = "ababab";
        rduplication(word.toCharArray());
    }
}

Method Signature: your method should return a result, instead of printing it to increase reusability and (automatic) testability.
Complexity: The complexity of your method is higher than it has to be. One simple improvement would be to start the second loop at int j = i to avoid duplicate checks.
Performance/Duplication: You check word[i] == word[j] twice. Just move the if (count > 1) check inside the first if to avoid this duplication.
Argument Checks: Just returning in case of an invalid argument (null in this case) is not a good idea; throw an exception instead to avoid possible bugs in the future.
Argument Checks: Returning nothing in case of a word length of 1 or 0 seems like a bug. I would expect an empty array or an array with the one entry to be returned (ie removeduplicate(['a']) should equal ['a'], not nothing).
Result Value: Your array still contains entries in the positions where duplicate characters were (eg aaaaab -> [a, , , , , b]. This isn't what I would expect (also, the question talks about accepting a string, and presumably returning a string as well; it then also talks about arrays, so using an array internally - instead of eg charAt - is presumably correct, but I would still expect the input and output to be strings).
count = count + 1 can be written as count++.
Naming: removeDuplication would be a lot clearer. removeDuplicateCharacters would be even more precise.
Formatting: Your spacing is inconsistent, leading to less readable code.
Declare variables as late as possible to increase readability. For example, count isn't needed until inside the first loop.

Taking this all together, you would get something like this:

public static String removeDuplicateCharacters(String word) {
    Objects.requireNonNull(word);

    char[] wordArray = word.toCharArray();

    int len = wordArray.length;
    for (int i = 0; i < wordArray.length; i++) {
        int count = 0;
        for (int j = i; j < wordArray.length; j++) {
            if (wordArray[i] == wordArray[j]) {
                count++;
                if (count > 1) {
                    wordArray[j] = 0;
                }
            }
        }
    }
    return String.valueOf(wordArray);
}

It's a bit too nested for my taste, but otherwise I think it's mostly ok.


More correct way to avoid using a list for dropdown This is my code

Any status &#9662;
        <form>

                    <input class="option" type="submit" name="status" value="Active" />

                    <input class="option" type="submit" name="status" value="Inactive" />

        </form>

Any date &#9662;
        <form>

                    <input class="option" type="submit" name="date" value="Today" />

                    <input class="option" type="submit" name="date" value="This Week" />

        </form>

In this example I have a dropdown with a list that I then make an effort to style not like a list (i.e removing bullet points, etc).

Is it possible to produce cleaner code to achieve the same result, without using a list structure, or is a list actually the right approach to this problem?

Lists are optional, but most people feel that you should use some sort of block level element to contain inline elements (some of the reasoning you'll hear is that the content still makes sense even if there's no styling or that you have a "list of options").  If you don't want to use a list, you'd be looking at elements like paragraph (which is wrong here since you're looking at text fragments) or the semantically void div to fill this role.

How does one look at inactive items from yesterday?  To me, this looks like a misuse of form elements.  The submit element isn't for changing state in a form, it is for submitting the contents of the form.

If you're only allowed to look at inactive or active items and never both, then a select element is what you should be using.  If the data is allowed to be filtered by any combination (some/all/none), then you should be looking at checkboxes grouped together by a fieldset:

<form>
    <fieldset>
        <legend>Status</legend>

<input type="checkbox" checked /> Active

<input type="checkbox" /> Inactive

    </fieldset>

    <fieldset>
        <legend>Date</legend>

<input type="checkbox" checked /> Today

<input type="checkbox" /> Yesterday

    </fieldset>
</form>

http://jsfiddle.net/kx4w7/1/


Dijkstra finding shortest path satisfying given constraints The question is originated from Hackerrank.

  Suppose there are 1 to N stores in a city which are connected by
  bidirectional roads associated with traveling times. Each store sells
  some types of fishes (0 <= type_of_fish_store_i < K), in total K
  types of fishes are selling in the city. A cat starts at store 1 and
  traveling along some path puchases fishes at each store on the path.

  Calculate minimum time that satisfies the constraints

  the cat has to end at a specified store X
  the cat has to end with specified types of fishes in the basket

  Note: a store including the final destination can be visited more than once

  Shop and fish types selling at that shop ({shop:fish_type}):

{1: {1}, 2: {2}, 3: {3}, 4: {4}, 5: {5}}

  Adjacency matrix with cells filled with time cost (referring to
  cost_matrix):

[[0, 10, 10, 0, 0],
 [10, 0, 0, 10, 0],
 [10, 0, 0, 0, 10],
 [0, 10, 0, 0, 10],
 [0, 0, 10, 10, 0]]

Approach I've implemented:

def dijkstra(cost_matrix, start, wanted, end):
    """

    :param cost_matrix: adjacency matrix filled with time costs
    :param start: the store where the cat starts at
    :param wanted: types of fishes the cat wants at the end of journey
    :param end: the store where the cat ends at
    :return:
    """
    fish_basket = shop_and_fish[start]
    accum = 0
    h =[]
    visited = [start]

    while not (wanted.issubset(fish_basket) and start == end):
        for i, dist in enumerate(cost_matrix[start - 1]):
            if dist > 0:
                    heapq.heappush(h, [ accum + dist, i + 1, fish_basket | shop_and_fish[i + 1], visited + [i + 1]])
        # print("heap: ", h)
        accum, start, fish_basket, visited = heapq.heappop(h)
    print("Total time: ", accum, ", End store:", start, ", Fish types collected: ", fish_basket, ", Path: ", visited)
    return accum

Execute:

dijkstra(cost_matrix, 1, {1,2,3,4,5}, 5)
# this is saying the final accepted state is the cat
# at 5th store with {1,2,3,4,5} types of fishes in hand

Returns

50
# Total time:  50 , End store: 5 , Fish types collected:  {1, 2, 3, 4, 5} , Path:  [1, 2, 4, 5, 3, 5]

Problem

Efficiency. Expanding unnecessary nodes such as [1,2,1,2,1,2], in some other cases [1,2,3,4,3,2,1,2,3,4..], maybe some other unforeseen patterns. Any thoughts on how to eliminate them? I've tried palindrome, but it seems to add a lot of complexity as it examines every path and every subpath of that path. Moreover, if you've identified any other improvements, feel free to add to answer.

This is an interesting problem. You hacked Dijsktra's algorithm to make it solve the problem, but this hack has a cost. In Dijkstra's classical algorithm:

you stop once all nodes have been visited;
you start an iteration with the best (shortest) candidate path.

In your version:

you stop once you reach your destination and you have a full basket of fishes;
same start for iterations.

Basically, you walk until your basket is full and then you rush to the end store. The BFS selection ensure you that you get the effective shortest path (good point: your algorithm is correct).

Of course, without the Dijkstra's stop condition, your algorithm is not O(n lg n) anymore.

Consider the following graph for instance:

A -- B --...-- C

The cost_matrix is:

cost_matrix =  [[0, 1, 0],
                [1, 0, 999],
                [0, 999, 0]]

If you look for a path from 1 to 3, your function will play ping-pong between 1 and 2 until the distance reaches 999 and then consider 3.

A bit of theory now. Take the following instance of your problem: every store (node) has only one type of fish, and two stores do not have the same type of fish. You want a basket filled with all existing types of fish. That means: you have to visit each store at least once. This is a variation of the Travelling Salesman problem because you are allowed to visit every store mutliple times, but the complexity is the same (see https://math.stackexchange.com/questions/1269983/traveling-salesman-problem-why-visit-each-city-only-once and https://en.wikipedia.org/wiki/Travelling_salesman_problem#Computational_complexity).

Hence, I think you won't find any easy answer. It seems to me that your best choice, if you want to improve efficiency, is to "branch and bound".


Advent of Code 2019 Day 15 in Python I did Advent of Code recently and enjoyed Day 15, which involves exploring a maze (my attempt in Kotlin). I've tried to recreate it partially in Python, minus the emulator:

https://repl.it/@MalcolmCrum/ReflectingFarNaturaldocs

It works but my code is largely 1:1 from Kotlin, and I'm not certain if I'm doing things in a Pythonic way. For example, I use this pattern regularly in Kotlin:

val direction = when (path.first()) {
            position + NORTH -> NORTH
            position + SOUTH -> SOUTH
            position + EAST -> EAST
            position + WEST -> WEST
            else -> error("Can't find first step from $position towards $destination in $path")
        }

The Python equivalent I found is this, which while nice and concise is maybe a little too "clever"? (ignore the 1 offset, I changed path logic slightly)

direction: Direction = {self.position + d: d for d in Direction}[path_to_destination[1]]

You have:

direction: Direction = {self.position + d: d for d in Direction}[path_to_destination[1]]

The dict is just mapping each key to key - self.position (I don't know your exact data models, but I'm going to assume that these are some kind of vector and that + and - behave in intuitive ways).  Hence:

direction = path_to_destination[1] - self.position

and maybe:

assert abs(direction) == 1

if you want to require that direction is a unit vector (again, I'm assuming your vector representation is reasonable and implements __abs__ to return the magnitude).


Python AES-CBC implementation using AES-ECB I am looking for ways to make this code more "pythonic" and any issues with my implementation. This was designed to solve Crytopal's Challenge 10. This challenge requires recreating the AES-CBC cipher using a library-provided AES-ECB function. PKCS7 padding is also used to align bytes to block boundaries.

from Cryptodome.Cipher import AES

def xor(in1, in2):
    ret = []
    for i in range(0, max(len(in1), len(in2))):
        ret.append(in1[i % len(in1)] ^ in2[i % len(in2)])
    return bytes(ret)

def decrypt_aes_ecb(data, key):
    cipher = AES.new(key, AES.MODE_ECB)
    return cipher.decrypt(data)

def encrypt_aes_ecb(data, key):
    cipher = AES.new(key, AES.MODE_ECB)
    return cipher.encrypt(data)

def pkcs7(val, block_size=16):
    remaining = block_size - len(val) % block_size
        if remaining == block_size:
            remaining = 16
    ret = val + chr(remaining).encode() * remaining

    return ret

def unpkcs7(val, block_size=16):
    pad_amount = val[-1]
    if pad_amount == 0:
        raise Exception
    for i in range(len(val) - 1, len(val) - (pad_amount + 1), -1):
        if val[i] != pad_amount:
            raise Exception
    return val[:-pad_amount]

def decrypt_aes_cbc(data, key, iv = b'\x00' * 16, pad=True):
    prev_chunk = iv

    decrypted = []

    for i in range(0, len(data), 16):
        chunk = data[i : i + 16]
        decrypted += xor(decrypt_aes_ecb(chunk, key), prev_chunk)
        prev_chunk = chunk

    if pad:
        return unpkcs7(bytes(decrypted))
    return bytes(decrypted)

def encrypt_aes_cbc(data, key, iv = b'\x00' * 16, pad=True):
    if pad:
        padded = pkcs7(data)
    else:
        padded = data

    prev_chunk = iv

    encrypted = []

    for i in range(0, len(padded), 16):
        chunk = padded[i : i + 16]
        encrypted_block = encrypt_aes_ecb(xor(chunk, prev_chunk), key)
        encrypted += encrypted_block
        prev_chunk = encrypted_block

    return bytes(encrypted)

Try to follow PEP8. Just a few little things like when assigning defaults to arguments don't put spaces around the =; two spaces between functions and imports; [i : i + 16] -> [i: i + 16]. Very minor stuff but adds up over a larger piece of code.
Avoid assigning variables to small bits of logic if they're only going to be used once. e.g.:

ret = val + chr(remaining).encode() * remaining
return ret

Could become:

return val + chr(remaining).encode() * remaining

Your names could be improved for clarity; avoid using vague names like key, var, data, etc.
I like ternary operators so I'd change encrypt_aes_cbc() to:

padded = pkcs7(data) if pad else data

You can do something similar in decrypt_aes_cbc() with your returns.
In fact, I'd extract the second part of the function to a generator and call it in encrpyt_aes_cbc(). By which I mean:

def gen_encrypted(key, iv):
    """Maybe give this function a better name!"""

    for i in range(0, len(padded), 16):
        ...
    yield encrypted

Then change the second half of encrypt_aes_cbc() to call that generator. This nicely compartmentalises your code, removes the need for the encrypted list and should be faster. I can explain this more thoroughly if you're confused about yield.
Your argument block_size=16 in unpkcs7 is not used.
Your indentation is off in pkcs7.
Try to be specific when raising exceptions. What error exactly are you catching? Exceptions also takes a string argument so you can explain the problem. e.g.:

raise IndexError('This is not a valid position in some_list')


Price rating approach I'm pretty new to web development and I don't know if my solution is OK according to the best practices, considering performance and clean code.

I have a Merchant model that belongs to a Price model.

Price table:

create_table "prices", force: :cascade do |t|
  t.string   "price_range"
  t.integer  "percentile"
  t.datetime "created_at",  null: false
  t.datetime "updated_at",  null: false
end

The current values inside the table:

**price_range**    **percentile **
Up to $20.00            20
$20.01 to $40.00        40
$40.01 to $60.00        60
$60.01 to $80.00        80
More than $80.00       100

To show this at my view I'm using:

  <div class="full-prices" style= width:"<%= "#{@merchant.price.percentile}%" %>

According to the width value the dollar signals will be colored (the CSS part of code is in the jsFiddle link).

jsFiddle

I'm not sure about that approach. Can anyone recommend a finer solution to achieve the same or is that OK?

Using a string column is not a very ideal solution. How would you for example query for rows where the price range is within or below 70.00$?

What if you want to do calculations in the database based on the values?

Instead you should use two columns:

create_table "prices", force: :cascade do |t|
  t.decimal   "price_min"
  t.decimal   "price_max"
  t.integer  "percentile"
  t.datetime "created_at",  null: false
  t.datetime "updated_at",  null: false
end

Note that when dealing with money you should use an integer in the lowest unit of currency (cents) or a decimal. See: why not use Double or Float to represent currency?

You can even write a methods in your model to get/set price_range with a range:

class Price
  def price_range
    (price_min..price_max)
  end

  def price_range=(range)
    self[:price_min], self[:price_max] = range.begin, range.end
  end
end

If you are using Postgres you can use the native range database type.

create_table :events do |t|
  t.numrange :price_range
  t.integer  "percentile"
  t.datetime "created_at",  null: false
  t.datetime "updated_at",  null: false
end

You can write a helper method to format it nicely

module PriceHelper
  def price_range_human( range )
    "$%s to $%s" % [range.begin, range.end].map(:number_to_currency)
  end
end

See ActionView::Helpers::NumberHelper


Recreating Google homepage for learning purposes Like other posters, I'm currently working on recreating Google's Homepage for The Odin Project.

I'm new to HTML and CSS but I'm eager to learn and have been looking around for an answer, testing different code, and pacing back & forth - I'm still stuck.

I'm having an issue with positioning things on my project. I've read through W3School's documentation on CSS but the method in which I used to position my buttons "Google Search" & "I'm feeling lucky" seems like a in-this-case-solution that may not work with all browsers or especially responsive design.

This is the area I'm trying to duplicate:

I was able to center the logo and the search bar with:

{margin-left: auto;
margin-right: auto;
display: block;}

I was able to center the buttons with:

#googleSearch {display: inline-block; margin-left: 520px;
margin-right: auto;}
#feelingLucky {display: inline-block;}

All of my code is below but my questions is: is there a better way to position the buttons and if so, what? Playing with the margin-left until it looks right seems to me like the equivalent of using a bunch of line breaks instead of changing the margin or padding.

li {font-family: arial,sans-serif;
font-size: 13px; list-style: none; display: inline-block;}

nav {text-align: right; padding-right: 160px; word-spacing: 10px;}

#userName {opacity: .55;}

a, a:visited {color: black; text-decoration: none;}

a:hover {text-decoration: underline;}

img, #searchBox {display: block;
margin-left: auto;
margin-right: auto;}

img {margin-top: 200px}

#googleSearch {display: inline-block; margin-left: 520px;
margin-right: auto;}
#feelingLucky {display: inline-block;}
<!DOCTYPE HTML>

<link rel="stylesheet" href="index.css">
<meta charset="UTF-8">
<title>Google</title>
<link rel="shortcut icon" type="image/x-icon" href="http://www.google.com/favicon.ico">

Jarod

Gmail

Images

    <form action="#" method="get" name="searchForm">
    <input id="searchBox" type="text" name="searchBox">

    <input id="two" type="submit" value="Google Search">
            <input id="feelingLucky" type="submit" value="I'm Feeling Lucky">

    </form>

Is there a better way to position my buttons?

Add both buttons in a single <div>, and center that div instead of the buttons individually.


Ping function in Python Below is a piece of code which takes a list of IPs, pings the hosts and returns (prints) the list for "Alive" and "Dead". Any feedback on what can be done better is welcomed, mainly, speed and reliability! Would you accept it as production code? What about try/except blocks and logging?

import subprocess
from multiprocessing import Pool, cpu_count

hosts = ('127.0.0.1', 'google.com', '127.0.0.2', '10.1.1.1')

def ping(host):
    ret = subprocess.call(['ping', '-c', '5', '-W', '3', host], stdout=open('/dev/null', 'w'), stderr=open('/dev/null', 'w'))
    return ret == 0

host_list = {"Alive":[], "Dead":[]}

host_stat = {True: host_list["Alive"], False: host_list["Dead"]}

p = Pool(cpu_count())
[host_stat[success].append(host) for host, success in zip(hosts, p.map(ping, hosts))]
print host_stat

I’ll start with a meta comment:

Python (and production code) emphasis readability. There are several parts of your code which are quick difficult to read, or work out what they’re for (more details below). In production code, we don’t just want speed and reliability – we also want code that somebody else can debug in a year, who’s never seen it before. Try to write something that’s easy to follow.

Then a few style comments:

I prefer to list module imports (after grouping them per PEP 8’s recommendations) in alphabetical order. If you have a long list of module imports, this makes it much easier to quickly see whether a module has been imported, or find duplicate imports.
Quoting PEP 8 on line length:

  Limit all lines to a maximum of 79 characters.

The line which defines ret is 126 characters long, which means I can't see it without scrolling off the screen. Ick.

More general comments:

Add comments and docstrings. I don’t know what any of this code does, and even if I work it out, I don’t know why you wrote it that way. For example, what does the ping function return, and why are you passing all those flags to the ping utility?
Use better variable names. A few examples:

I expect a variable named host_list to be a list, but it’s actually a dict.
In production code, I steer clear of single-character variable names like p. Fine in small scripts, but it’s a pain to grep for later.
I’d change the name of the ping function so it can be discussed unambiguously from the command-line ping utility.

I wouldn’t accept these variable names in “production code”.
Not everything is encapsulated in a function. The ping function provides a nice wrapper around the ping utility, but checking the list of hosts is handled at the main level. Wrap this in a function that takes a list of hosts, and returns a list of alive/dead. That makes it easier to reuse this code later.

Then wrap the testing of this particular list of hosts in a main() function, and use the if __name__ == '__main__': main() construction. More portable.
You assume the list of host names is correct. If I drop in something that clearly isn’t a hostname (for example, '127.0.0.1 notahostname' into the hosts tuple), I get a non-zero return code from the subprocess call, but it’s not because that host is dead – it’s because it’s nonsensical.

I would consider storing the return code from each host: this is a much more useful indicator of what’s happened. Alternatively, you could check that hostnames are valid before you pass them in – this Stack Overflow answer looks like it might be useful.
There seems to be duplication of data in host_list and host_stat. I’m really not sure what the point of having both of these dictionaries is. Why not just start with empty lists in host_stat?
That list comprehension is too complicated. Not only is it too long (86 chars), but it’s not immediately obvious what it does. Break it down across multiple lines (remember, Python emphasises readability).
Using /dev/null to throw away the output from subprocess is platform-specific. It’s better to use os.devnull, which should work on non-Unix systems. (from How to hide output of subprocess in Python 2.7)

Four woolier comments:

Are the keys and values of host_stat the right way round? Once you have a dict of host availability data, I’d imagine you’re going to go through the list of hosts one-at-a-time and check if they’re available. For that, surely having the keys be the hostnames makes more sense (but that’s just me).
Why is hosts a tuple? I'd make it a list instead. Also, break it across multiple lines (see below).
I don’t see the value of multiprocessing here. Perhaps if you’re doing it for lots of hostnames, but otherwise I wouldn’t bother.
What do you need logging for? The only thing that might be useful is the return code from ping, but since you’re throwing that away, I’m not sure what you think you’d like to log.

With those comments in mind, here's how I'd rewrite your script:

import os
import subprocess

def ping_return_code(hostname):
    """Use the ping utility to attempt to reach the host. We send 5 packets
    ('-c 5') and wait 3 milliseconds ('-W 3') for a response. The function
    returns the return code from the ping utility.
    """
    ret_code = subprocess.call(['ping', '-c', '5', '-W', '3', hostname],
                               stdout=open(os.devnull, 'w'),
                               stderr=open(os.devnull, 'w'))
    return ret_code

def verify_hosts(host_list):
    """For each hostname in the list, attempt to reach it using ping. Returns a
    dict in which the keys are the hostnames, and the values are the return
    codes from ping. Assumes that the hostnames are valid.
    """
    return_codes = dict()
    for hostname in host_list:
        return_codes[hostname] = ping_return_code(hostname)

    return return_codes

def main():
    hosts_to_test = [
        '127.0.0.1',
        'google.com',
        '127.0.0.2',
        '10.1.1.1'
    ]
    print verify_hosts(hosts_to_test)

if __name__ == '__main__':
    main()


Render multiple SSRS reports in parallel This service takes in a report request object that can contain multiple reports that need to be rendered. This code works but I'm not sure if I've implemented anything wrong. Note the exportedImage.Result makes this synchronous but when I tried adding async and await to the Parallel.ForEach it doesn't seem to work so this was essentially a workaround, that's why I'm looking to figure out if I've implemented this correctly or not. _exporter.Export is an asynchronous method.
Just FYI, when I tried to add async and await, I added it like so:
await Task.Run(() => Parallel.ForEach(request.Reports, async report =>
var exportedImage = await _exporter.Export(_reportServerUrl, reportPath, export, reportParameters);
var reportDictionary = new ConcurrentDictionary<int, string>();

await Task.Run(() => Parallel.ForEach(request.Reports, report =>
{
    var (reportPath, bindingOrder, reportParameters) = report;

    var exportedImage = _exporter.Export(_reportServerUrl, reportPath, export, reportParameters);

    if (exportedImage.Result == null)
    {
        throw new InvalidOperationException($"Could not render report {reportIdentifier} with binding order {bindingOrder}.");
    }

    reportDictionary.GetOrAdd(bindingOrder, exportedImage.Result);
}));

Here's pure async example without any workarounds.
private async Task ProcessReport(Report report, ConcurrentDictionary<int, string> reportDictionary)
{
    var (reportPath, bindingOrder, reportParameters) = report;

    var exportedImage = await _exporter.Export(_reportServerUrl, reportPath, export, reportParameters);

    if (exportedImage == null)
    {
        throw new InvalidOperationException($"Could not render report {reportIdentifier} with binding order {bindingOrder}.");
    }

    reportDictionary.GetOrAdd(bindingOrder, exportedImage);
}

var reportDictionary = new ConcurrentDictionary<int, string>();

List<Task> tasks = new List<Task>();
foreach (var report in request.Reports)
{
    tasks.Add(ProcessReport(report, reportDictionary));
}
await Task.WhenAll(tasks);