Beginner Python OOP web scraper I started learn OOP in Python. I have OOP basics in Java and apparently I have a problem with thinking in OOP in Python (and using the best functionalities and syntax). I have doubts about my OOP design in web scraping for a local book review page. So the basic idea is scraping information about book with the user reviews for that books.

The structure of the Python package:

/bookscraper
    __init__.py
    book.py
    comment.py
    parsers.py

parsers.py:

from urllib.request import urlopen
from bs4 import BeautifulSoup
from bookscraper.book import Book
from bookscraper.comment import Comment

class Parser:
    def __init__(self, url, name, search_url):
        self.url = url
        self.name = name
        self.search_url = search_url

class DatabazeKnih(Parser):
    PAGE_URL = 'http://www.databazeknih.cz/'
    NAME = 'Databáze knih'
    SEARCH_URL = 'search?q={}&hledat=&stranka=search'

    def __init__(self):
        super().__init__(DatabazeKnih.PAGE_URL, DatabazeKnih.NAME, DatabazeKnih.SEARCH_URL)

    def search(self, name):
        self.search_html = urlopen(DatabazeKnih.PAGE_URL + DatabazeKnih.SEARCH_URL.format(name))
        self.book_link = DatabazeKnih.PAGE_URL + self.get_book_link() + '?show=alldesc'
        return self.get_book_info()

    def get_book_page(self):
        return urlopen(self.book_link)

    def get_book_link(self):
        soup = BeautifulSoup(self.search_html, 'html.parser')
        return soup.find(type='book').attrs['href']

    @staticmethod
    def parse_comment(comment):
        user = comment.find('a').string
        text = comment.find('p', {'class': 'odtopm new2 justify'}).text
        date = comment.find('span', {'class': 'pozn_lightest odleft_pet'}).string
        return user, text, date

    @staticmethod
    def get_comments(soup):
        comments_objects = []
        comments = soup.find_all('div', {'class': 'komholdu'})
        for comment in comments:
            user, text, date = DatabazeKnih.parse_comment(comment)
            comments_objects.append(Comment(user, text, date))
        return comments_objects

    @staticmethod
    def get_authors(soup):
        return [author.string for author in soup.find(itemprop='author').find_all('a')]

    def get_book_info(self):
        soup = BeautifulSoup(self.get_book_page(), 'html.parser')
        title = soup.find(itemprop='name').string
        rating = soup.findAll('a', {'class': 'bpoints'})[0].string
        text = soup.find(itemprop='description').string
        authors = DatabazeKnih.get_authors(soup)
        year = soup.find(itemprop='datePublished').string
        comments = DatabazeKnih.get_comments(soup)
        return Book(title, rating, text, authors, year, comments)

Then there are the Book and Comment classes. I will add some functionality to these classes. Now I prepared only constructors:

class Book:
    def __init__(self, title, rating, text, authors, year, comments):
        self.title = title
        self.rating = rating
        self.text = text
        self.authors = authors      # this is a list
        self.year = year
        self.comments = comments    # this is a list

class Comment:
    def __init__(self, user, text, date):
        self.user = user
        self.text = text
        self.date = date

Then I can't get info about book via creating parser object:

from bookscraper.parsers import DatabazeKnih

parser = DatabazeKnih()
book = parser.search('ydris')  # Name of the book

My code is working, but is it a good OOP design? I have some doubts about the design of methods in the DatabazeKnih class. In search method I set two attributes I found it is not a good practice, but how should I rewrite? Should I create setters for this two attributes?

Use list comprehensions instead of creating empty array and appending to it in a for loop. It's both faster and more readable.

@staticmethod
def get_comments(soup):
    return [
        Comment(*DatabazeKnih.parse_comment(comment)
        for comment in soup.find_all('div', {'class': 'komholdu'})
    ]

But If would argue that you don't need classes here at all.

from bs4 import BeautifulSoup
from collections import namedtuple
import requests as r

Book = namedtuple('Book', 'title rating text authors year comments')
Comment = namedtuple('Comment', 'user text date')
PAGE_URL = 'http://www.databazeknih.cz/'

def search(name):
    page = r.get(PAGE_URL + 'search', params=dict(q=name, stranka='search')).text
    link = BeautifulSoup(page, 'html.parser').find(type='book').attrs['href']
    book_page = r.get(PAGE_URL + link, params=dict(show='alldesc')).text
    return book_info(book_page)

def book_info(book_page):
    soup = BeautifulSoup(book_page, 'html.parser')
    title = soup.find(itemprop='name').string
    rating = soup.findAll('a', {'class': 'bpoints'})[0].string
    text = soup.find(itemprop='description').string
    authors = [elem.string for elem in soup.find(itemprop='author').find_all('a')]
    year = soup.find(itemprop='datePublished').string
    comments = [
        Comment(*parse_comment(elem))
        for elem in soup.find_all('div', {'class': 'komholdu'})
    ]
    return Book(title, rating, text, authors, year, comments)

def parse_comment(comment):
    user = comment.find('a').string
    text = comment.find('p', {'class': 'odtopm new2 justify'}).text
    date = comment.find('span', {'class': 'pozn_lightest odleft_pet'}).string
    return user, text, date

My suggestions are.

Use requests library, It's more pythonic than urllib.

Use namedtuples instead of classes where possible. Easier to read and to write. If you need to add functionality you can inherit from namedtuple.

Don't write one liner get_this get_that functions. It's useful when you're hiding some implementation, but hiding one or two lines - it's useless and it's even a bit less readable (when you would need this implementation).

What you're doing with function name get_authors you can do just as easy with giving a variable self-explanatory name like authors = .... Everybody would understand that next one or two lines (if it's a comprehension) is getting authors in some way and they can read this lines if they want details.

Also get_book_info means suggesting it's loading or getting something from somewhere. In java it just means that this is some property, but in python we have @property decorator and don't need getters and setters so get is used when you're loading something. Instead it's more accurate to think of it as pure function that takes html and outputs Book. You can call it make_book_info, def book_info_from(html): or just book_info.


User input function keep looping until valid filename entered The following function is basically asking the user to input a file name for a file that the calling program is going to do something with.  If the user enters an invalid file name, I want the program to loop back and give them another chance.  The code works, but is this the proper way to accomplish this?  Also, are there other errors that could get triggered that I should deal with?

def get_filename(file_type):
    while True:
        print('enter ' + file_type + ' filename: ')
        filename = input()
        print(filename)
        try:
            with open(filename, 'r') as f:
                my_file = f.read()
            return my_file
        except FileNotFoundError:
            print('No such file.  Check file name and path and try again.')

x = get_filename('TEMPLATE')
print(x)

is this the proper way to accomplish this?

Yes; looping is the best approach for this type of functionality (see e.g. Asking the user for input until they give a valid response).

are there other errors that could get triggered that I should deal with?

There are reasons other than not finding the file that opening and reading the file could fail; in this case, I would probably catch the more general OSError to handle all of these. I would also restructure slightly to minimise the code in the try block:
try:
    with open(filename) as f:  # note that 'r' is the default mode
        my_file = f.read()
except OSError:
    print('Error accessing file. Check file name and path and try again.')
else:
    return my_file

A few other minor notes:

You should consider adding a docstring, explaining what the function does and how to use it;
Currently, the file_type is only used for display purposes - consider adding checks that the user has selected an appropriate file;
Returning the string content of the file means you have to read the whole thing into memory up-front, which may not be ideal with larger files; and
I would use str.format rather than + concatenation, and just pass the text straight to input as the prompt argument rather than having a separate print call.


Thread spawning child threads and waiting for children to finish Scenario: I have a main thread which spawns few child threads to do some independent tasks. After each child thread finishes their tasks, the main thread proceeds. The code I have written is working in most of the time. However, in case one of child thread gets blocked, the main thread has to wait for ever. As it is not happening frequently, I am kind of OK as of now. But I am looking for a better solution

I am not trying to interrupt the child threads by waiting in main thread and then interrupting the child threads, because I don't know upfront how much time I need to wait for all the children to finish their tasks.However, it should not take more than 10 mins(worst case) for the children to finish their tasks. But if I go with thread interrupt way, I will have to wait for 10 mins always even if all children finishes their tasks in 2 mins itself. Lots of time (8 min here) will be wasted for nothing.

So, is it possible to review my code and suggest if any enhancement is possible?

Thread Class

public class MyThread implements Runnable{

@Override
public void run() {
    /*
     * Actual method do the following:
     * 1. Say we have 10 person. So, do the following for each person
     *   1.1 Fetch social network(FB,LN,TW etc )profiles
     *   1.2 Process the profiles and store in database
     * 2. Finished
     */
    System.out.println("hello world by thread "+ Thread.currentThread().getName());
  }
}

Invoker Class

public class ThreadInvoker {

public static void main(String[] args){

    List<Thread> threadList = getMyThreadList();

    for (Thread thread : threadList) {
        thread.start();
    }

    System.out.println("Waiting for Child Threads to die");

    for (Thread thread : threadList) {
        try {
            thread.join();
            System.out.println(thread.getName() + " Finished its job");
        } catch (InterruptedException e) {
            System.out.println("Interrupted Exception thrown by : "
                    + thread.getName());
        }
    }

    System.out.println("All Child Threads Finished their Job");
}

/**
 * Dummy method to get a list of Threads. Content of this method may not be reviewed.
 * Actual method do some other way to return a list of threads
 * @return
 */
private static List<Thread> getMyThreadList() {
    List<Thread> threadList = new ArrayList<>();
    MyThread myThread;
    Thread thread;
    for(int i=0; i<10; i++){
        myThread = new MyThread();
        thread = new Thread(myThread);
        thread.setName("Thread "+i);
        threadList.add(thread);
    }
    return threadList;
 }
}

Instead of creating threads use an ExecutorService and use invokeAll(Collection<Callable>, long, timeUnit). This will return when all calls are done or the timeout expires whichever happens first. Tasks that aren't completed will be canceled (which can be checked by the isCanceled() property of the matching Future in the return value).

This will mean you will need to change the Runnables to Callables but that just means returning null in the call method.


Determining if someone has read and seen all the questions of a book I am trying to evaluate if someone has read and seen all the questions of a book.

I am using PHP to average everything out after I get the data. I know MySQL can avg, but I can't seem to wrap my head around how I can average the box number of the questions (similar to spaced repetition learning) if they haven't seen every question.

How can this function be better? I feel there might be a more elegant way of doing it.

SELECT COUNT(s.id),
       (SELECT COUNT(u.id)
          FROM user_sections u
         WHERE u.chpt_id = ? && u.user_id = ?
       ),
       (SELECT COUNT(q.id)
          FROM question q
         WHERE q.chapter_id = ? && (q.module = 1 || q.module = ?)
       ),
       (SELECT COUNT(v.id)
          FROM user_questions v
         WHERE v.chpt_id = ? && u.user_id = ?
       ),
       (SELECT AVG(v.box)
          FROM user_questions v
         WHERE v.chapter_id = ? && u.user_id=?
       ),
       (SELECT AVG(m.$mkts_col)
          FROM mkts m
         WHERE m.chapter_id = ? && (m.module = 1 || m.module = ?)
       )
  FROM sections s
 WHERE s.chapter_id = ? && (s.module = 1 || s.module = ?)

What you have written is essentially six independent queries.  Let's drop the pretense that these are correlated subqueries.  Let's also reorder the output columns.  We can also simplify col = A || col = B as col in (A, B).

SELECT (SELECT COUNT(s.id)
          FROM sections s
         WHERE s.chapter_id = ? AND s.module IN (1, ?)
       ),
       (SELECT COUNT(q.id)
          FROM question q
         WHERE q.chapter_id = ? AND q.module IN (1, ?)
       ),
       (SELECT AVG(m.$mkts_col)
          FROM mkts m
         WHERE m.chapter_id = ? AND m.module IN (1, ?)
       ),
       (SELECT COUNT(u.id)
          FROM user_sections u
         WHERE u.chpt_id = ? AND u.user_id = ?
       ),
       (SELECT COUNT(v.id)
          FROM user_questions v
         WHERE v.chpt_id = ? AND u.user_id = ?
       ),
       (SELECT AVG(v.box)
          FROM user_questions v
         WHERE v.chapter_id = ? AND u.user_id = ?
       );

That's about all you can do to rewrite this particular query.  Depending on how your application works, it might make sense to obtain all of these statistics globally, instead of for particular user + chapter + module combinations.  In that case, you would write two queries, each with JOIN and GROUP BY clauses.  One query be joined by chapter and module conditions, and the other would be joined by chapter and user.

After rearranging the columns, it becomes more obvious that you used both user_questions.chpt_id and user_questions.chapter_id.  One of them is probably an error.


Function that returns another function, for a programming task This is a task I was given for an online course. This is my solution to the problem.

Task:
Write a function called general_poly, that meets the specifications below.

For example, general_poly([1, 2, 3, 4])(10) should evaluate to 1234 because

1*10^3 + 2*10^2 + 3*10^1 + 4*10^0

So in the example the function only takes one argument with general_poly([1, 2, 3, 4]) and it returns a function that you can apply to a value, in this case x = 10 with general_poly([1, 2, 3, 4])(10).

def general_poly (L):
    """ L, a list of numbers (n0, n1, n2, ... nk)
    Returns a function, which when applied to a value x, returns the value
    n0 * x^k + n1 * x^(k-1) + ... nk * x^0 """
    #YOUR CODE HERE

My Code:

def general_poly (L):
    """ L, a list of numbers (n0, n1, n2, ... nk)
    Returns a function, which when applied to a value x, returns the value
    n0 * x^k + n1 * x^(k-1) + ... nk * x^0 """

    def function_generator(L, x):
        k = len(L) - 1
        sum = 0
        for number in L:
            sum += number * x ** k
            k -= 1
        return sum

    def function(x, l=L):
        return function_generator(l, x)

    return function

The name function_generator could be improved. What this function does is to evaluate the polynomial at the given value x. So a name like evaluate would be better.
The body of function_generator can be simplified. First, it's simpler to iterate over L in reversed order, because then you can start with k = 0 rather than k = len(L) - 1:

def evaluate(L, x):
    k = 0
    sum = 0
    for number in reversed(L):
        sum += number * x ** k
        k += 1
    return sum

Now that k goes upwards, you can use enumerate to generate the values for k:

def evaluate(L, x):
    sum = 0
    for k, a in enumerate(reversed(L)):
        sum += a * x ** k
    return sum

And now that the body of the loop is a single expression, you can use the built-in sum function to do the addition:

def evaluate(L, x):
    return sum(a * x ** k for k, a in enumerate(reversed(L)))

Another approach to polynomial evaluation is to generate the power series in \$x\$ separately:

def power_series(x):
    "Generate power series 1, x, x**2, ..."
    y = 1
    while True:
        yield y
        y *= x

and then combine it with the coefficients using zip:

def evaluate(L, x):
    "Evaluate the polynomial with coefficients in L at the point x."
    return sum(a * y for a, y in zip(reversed(L), power_series(x)))

This is more efficient for large polynomials because each step in power_series is a multiplication by x, which is cheaper to compute than the exponentiation x ** k.

Finally, you could take advantage of this transformation: $$a_k x^k + a_{k-1} x^{k-1} + \dots + a_1x + a_0 = (\cdots ((0 + a_k)x + a_{k-1})x + \cdots + a_1)x + a_0$$ and write:

def evaluate(L, x):
    "Evaluate the polynomial with coefficients in L at the point x."
    r = 0
    for a in L:
        r = r * x + a
    return r

(This is Horner's method.)
There's no need for function, because it's fine for the body of function_generator to refer to nonlocal variables like L. So you can write:

def general_poly(L):
    "Return function evaluating the polynomial with coefficients in L."
    def evaluate(x):
        r = 0
        for a in L:
            r = r * x + a
        return r
    return evaluate

An alternative approach is to use functools.partial:

from functools import partial

def evaluate(L, x):
    "Evaluate the polynomial with coefficients in L at the point x."
    r = 0
    for a in L:
        r = r * x + a
    return r

def general_poly(L):
    "Return function evaluating the polynomial with coefficients in L."
    return partial(evaluate, L)

This would be useful if you sometimes wanted to call evaluate directly (instead of via general_poly).


Tic-Tac-Toe in Python 2.x This is a class called Board which handles the tracking and checking of the state of the game. self.board is a dict of coordinate: state pairs, for e.g. (0,1): 'x'.

I could have hardcoded the checking lists in but decided to be adventurous and learn a bit of itertools.

The complete program is about 130 lines of code and I somehow managed to cram an ASCII board using a coordinate printer thingamajig in there (not included in this sample).

This is the code of a Pythoner 4 months into learning his first language. I seem to be very concerned about documentation, though I may just be in love with doctests.

class Board(object):
    """
    A tictactoe board using coordinates.

    Coordinates start at (0,0) in the top left.

    >>> myboard = Board()

    >>> myboard.set((0,1), 'x')
    >>> myboard.set((0,1), 'foo')
    Traceback (most recent call last):
        ...
    ValueError: state must be one of 'x', 'o', or ' '
    >>> myboard.set((7,1), 'o')
    Traceback (most recent call last):
        ...
    ValueError: coord does not exist

    >>> myboard.get((0,1))
    'x'
    >>> myboard.get('banana')
    Traceback (most recent call last):
        ...
    ValueError: coord does not exist

    >>> myboard.set((0,0), 'x')
    >>> myboard.has_row()
    []
    >>> myboard.set((0,2), 'x')
    >>> myboard.has_row()
    ['x']
    """

    def __init__(self):
        self.board = {i: " " for i in itertools.product((0,1,2), repeat=2)}

    def set(self, coord, state):
        """
        Set a space on the board to `state`.

        For e.g.: set((0,1), 'x')
        """
        if state not in ('x', 'o', ' '):
            raise ValueError("state must be one of 'x', 'o', or ' '")
        elif coord not in self.board.keys():
            raise ValueError("coord does not exist")
        else:
            self.board[coord] = state

    def get(self, coord):
        """
        Get the state of the space at `coord`.
        """
        try:
            return self.board[coord]
        except KeyError:
            raise ValueError("coord does not exist")

    def filled(self):
        """
        Return amount of spaces filled.
        """
        f = 0

        for v in self.board.itervalues():
            if v != ' ':
                f += 1

        return f

    def has_row(self):
        """
        Return a list of players who have a row of three.
        """
        across = [[i for i in itertools.product([0,1,2], [n])] for n in xrange(3)]
        down = [[(b, a) for a, b in i] for i in across]
        diag = [
            [(0,0), (1,1), (2,2)],
            [(0,2), (1,1), (2,0)],
        ]

        results = []

        for row in itertools.chain(across, down, diag):
            values = [self.board[c] for c in row]

            if values == ['x', 'x', 'x']:
                results.append('x')
            if values == ['o', 'o', 'o']:
                results.append('o')

        return results

First of all: great work on the doctests.

Your spacing could be improved, you might want to run a tool like pep8 over your code and fix the issues it reports: (0, 1, 2) instead of (0,1,2) and such.

Your set method is quite defensive, which is a good thing. However, do you really want to allow setting a field to ' ' when it already contains a 'x' or 'o'? Likewise: Do you want to allow placing something on a field that already contains an 'x' or an 'o'? Either way is fine, depending on if you want the board to know about the rules of the game or not.

One thing I would recommend changing is replacing the elif. Prefer

elif coord not in self.board:

over

elif coord not in self.board.keys():

It's a bit more idiomatic.

The method filled could also be a bit more simple. You're looping over all the values. Using a list comprehension, you could write

return len([x for x in self.board.itervalues() if x != ' '])

There are probably many more ways to rewrite it, though.

Your definition of across:

across = [[i for i in itertools.product([0,1,2], [n])] for n in xrange(3)]

That looks quite complicated to me.

[i for i in itertools.product([0, 1, 2], [n])]

can be replaced by

list(itertools.product([0, 1, 2], [n]))

or even better:

[(i, n) for i in (0, 1, 2)]

This gives

across = [[(i, n) for i in (0, 1, 2)] for n in xrange(3)]

It seems a bit silly to use both (0, 1, 2) and xrange(3). I'd suggest replacing both by range(3) or (0, 1, 2). Using xrange is a bit superfluous for ranges of length 3.

across = [[(i, n) for i in range(3)] for n in xrange(3)]

Ideally, you'd want to just mirror the definition for down, instead of re-using across, as it more cleanly symbolises the symmetry

down = [[(n, i) for i in range(3)] for n in range(3)]

Beyond that, your rows is quite clean, except the definition of values could be replaced by

values = map(self.board.get, row)

But that doesn't matter that much.


Selecting a prize category based on two parameters I get an object with categories, first category has 2 types. Both types are in the first and second position but sorting is not constant.

I have to return category 1 with playLevel === 2 when bonusPlus is true and category 1 with playLevel === 1 otherwise.

Here is the code:

const getFirstCategory = (bonusPlus, prizeCategories) => {
  if (bonusPlus) {
    if (prizeCategories[0].playLevel === 2) {
      return [prizeCategories[0]];
    }
    return [prizeCategories[1]];
  }
  if (prizeCategories[0].playLevel === 1) {
    return [prizeCategories[0]];
  }
  return [prizeCategories[1]];
};

How can I reduce / beautify this code?

To make @kfx's answer more readable:

const getFirstCategory = (bonusPlus, prizeCategories) => {
  const bonusPlusLevel = bonusPlus ? 2 : 1;
  if (prizeCategories[0].playLevel === bonusPlusLevel) {
    return [prizeCategories[0]];
  }
  return [prizeCategories[1]];
};


JWT Authentication for REST API Here is my code to implement authentication in a Node/Express/Sequelize project. This is my first time using JWT and I would appreciate any help!

// Load required packages
const jwt = require('jsonwebtoken');
const expressJwt = require('express-jwt');
const config = require('../config');
const User = require('../models').User;

exports.authenticate = function(req, res) {
  const username = req.body.username;
  const password = req.body.password;
  User.findOne({
    where: { username: username }
  }).then((user) => {
    // Make sure the password is correct
    if (user.verifyPassword(password)) {
      const token = jwt.sign({
        username: user.username
      }, config.jwtSecret);
      res.json({
        success: true,
        token: token,
        username: user.username
      });
    }
  }).catch((error) => {
    console.error(error);
    res.sendStatus(404);
  });
}

exports.isAuthenticated = expressJwt({ secret: config.jwtSecret });

Though not in Javascript, my uses of JWT have always used the password as the JWT secret.  The JWT payload is whatever you need to identify the user -- username in your case.  You can add in a CSRF token of some sort as well.  I have an example on my GitHub that is a very simple application using JWT as the authentication mechanism - https://github.com/dave-shawley/readings/blob/7d2504587daa6a174fc3cbc0a5478fa817412eea/readings/static/js/login.js#L10-L26 is the javascript login code.  Mind you that I am most certainly not a javascript programmer so don't read too much into my example for style advice ;)

My login code builds a JWT payload that looks something like:

{
  "exp": 1488027170,
  "iss": "https://whatever.example.com/login",
  "csrf": "123456ABCDEF",
  "nbf": 1488026870
}

where "nbf" is the current time, "exp" is the expiration time, and "iss" is the referring web site.  "csrf" is a one-time token that is embedded in the HTML form.

I encode this structure using the entered password as the secret and pass the resulting token to my login endpoint.  I have the user name in a secured cookie but it could be passed in the JWT payload as well.  On the receiving side, I look up the user information in my data store by the user name from the cookie.  Then I verify that the JWT payload was signed using the password as the secret and that it is still valid.  If everything checks out, then the user is authenticated.


Unix shell function for adding directories to PATH Adding a directory to the PATH in Unix/Linux is a very common task.  However, what if the directory is already in the path?  My goal is to write a shell function (preferably portable) that will add a directory to the front or back of the PATH variable, but only if it's not already there.

Here's what I have (syntax is in zsh / bash):

#-------
# DESC: Adds a directory to the PATH if it's not already in the PATH
# ARGS:
#  1 - The directory to add
#  2 - Which end of PATH to add to.  Use "front" to prepend.
#-------
add2path() {
  if ! echo $PATH | egrep "(^|:)$1(:|\$)" > /dev/null ; then
    if [[ $2 = "front" ]]; then
      PATH="$1:$PATH"
    else
      PATH="$PATH:$1"
    fi
    export PATH
  fi
}

This function works.  I have tested it on Ubuntu, Solaris, and FreeBSD.  And I have tested it in bash, zsh, and ksh.  But I want to make sure that it is as portable (first and foremost), readable, and as efficient as possible.

Remarks

I know that the =~ operator would be more readable, but I had trouble with it not working on certain OSes, particularly Solaris.
I know I could (and arguably should) use grep -Fq instead of egrep routing to /dev/null, but again this didn't work on certain OSes (Solaris)

Ironically, the way to fix this on Solaris is to add /usr/xpg4/bin to the PATH.  :)

grep is most likely an overkill. After setting IFS=":", the $PATH is conveniently split into words. Then the presence of the directory can be determined in a simple loop

    IFS=":"
    for pathdir in $PATH; do
        if [ $pathdir == $1]; then return; fi
    done

    # Now restore IFS and modify path as needed.


Webscraper for www.coinmarketcap.com I built a webscraper to take all the historical data from CoinMarketCap.com. A site which records historical data for cryptocurrencies which unfortunately does not have a feature to download all of their data in one convenient file.

It works but it's somewhat slow and probably could be designed better. I'm self taught so I'd love some advice on how to improve the code's functionality, speed, and readability. Thanks a lot in advance.

def scrape_coin_marketcap():
    print('Loading Packages...')
    from bs4 import BeautifulSoup
    import requests
    import pandas as pd

    print('Scraping links...')
    #Download HTML of links to historical CoinMarketCap data
    url = 'https://coinmarketcap.com/historical/'
    r  = requests.get(url)
    data = r.text
    soup = BeautifulSoup(data,'html.parser')

    #scrape a list of links to historical data
    raw_links = []
    for link in soup.find_all('a'):
        raw_links.append(link.get('href'))

    #Remove non-historical links
    historical_links = []
    for link in raw_links:
        if "201" in str(link):
            historical_links.append('https://coinmarketcap.com' + link)

    print('Scraping Data....')

    #Scrape historical data from each time period
    master_df = pd.DataFrame()
    num_links = len(historical_links)
    print(str(num_links) + " dates to be scraped...")
    for link in historical_links:
        try:
            res = requests.get(link)
            soup = BeautifulSoup(res.content, 'lxml')
            table = soup.find_all('table')[0]
            df = pd.read_html(str(table))[0]
            date = str(soup.find_all('h1')[0])[51:-5]
            df['date'] = date
            master_df = master_df.append(df)
            print("    Scraping: " + str(date))
        except:
            print("    ERROR Scraping: " + str(link))

    print('Saving to disk...')
    master_df.to_csv('CoinMarketCap.csv', index = False)
    print("Completed.")

if __name__ == "__main__":
    scrape_coin_marketcap()

Welcome to Code Review SE, nice first question! Here are some of my comments, not pertaining to performance:

There's no reason to import modules in the body of scrape_coin_marketcap(). Just do it at the top of the source code file. If you need to be sure the modules are present at all, you can catch ImportError:

import sys

try:
    from bs4 import BeautifulSoup
    import requests
    import pandas as pd
except ImportError:
    sys.exit("This script requires Beautiful Soup 4, Requests and Pandas.")

Note: I'm using sys.exit() here instead of print(), because I'm assuming this is a standalone script and an ImportError should therefore be fatal.
The URL for the webpage is unlikely to change in the future, and isn't supposed to be modified at runtime, so you should mark it as a constant variable. Constants use the UPPERCASE_WITH_UNDERSCORES naming convention:

HISTORICAL_DATA_URL = "https://coinmarketcap.com/historical/"

This comment:

#Download HTML of links to historical CoinMarketCap data

... is pretty pointless. Anyone familiar with Python doesn't need that comment to understand what the snippet of code below it does. Only add comments to difficult to comprehend code, or to snippets that need extra attention for whatever reason.
The following pattern:

my_list = []
for x in some_iterable:
    my_list.append(some_function(x))

... is a good candidate for a list comprehension:

my_list = [some_function(x) for x in some_iterable]

... so in your case:

raw_links = [link.get("href") for link in soup.find_all("a")]

In fact, the part where you remove non-historical links could benefit from a list comprehension too:

historical_links = [
    "https://coinmarketcap.com/" + link for link in raw_links
    if "201" in str(link)
]

Don't mix single and double quotes. Either choice is valid, but mixing them is a bad idea. As an exception, you are encouraged to use single quotes if you need to output literal double quotes, and vice versa.
Unless you need access to the result of r.text later, don't bind it to a variable.
Debug / error messages should not be sent to stdout, but to stderr. You can do that by explicitly passing sys.stderr as a parameter to print():

import sys

...

print("Loading Packages...", file=sys.stderr)


Printing some metrics from a Windows machine New to Ruby here. I just made a script that print some metrics from a Windows machine about its disk. The script works fine and as I wanted it to. Now, since I'm new to the ruby and programming world, I'd like to ask you if anyone could help me to make to code look better or if there's other more efficient way I could have wrote it.

require 'rubygems' if RUBY_VERSION < '1.9.0'
require 'sensu-plugin/metric/cli'
require 'socket'

class LogicalDiskMetric < Sensu::Plugin::Metric::CLI::Graphite

  option :scheme,
    :description => "Metric naming scheme, text to prepend to .$parent.$child",
    :long => "--scheme SCHEME",
    :default => "#{Socket.gethostname}"

  #Run a wmic command to get some stats about the disks usage
  def get_logical_disk_metrics
    tmp = `wmic logicaldisk get caption, drivetype, freespace, size`.split("\n")
    logical_disk = []
    bytes_to_gbytes = 1073741824

    #Convert data from bytes to gigabytes and find the used space (size-free)
    tmp.each do |disk|
      disk = disk.split(" ")
      if disk[1].to_i == 3
        disk[2] = (disk[2].to_f/bytes_to_gbytes).round(2)
        disk[3] = (disk[3].to_f/bytes_to_gbytes).round(2)
        disk.push((disk[3].to_f - disk[2].to_f).round(2))
        logical_disk.push(disk)
      end
    end
    return logical_disk
  end

  #Add the disks usage in %
  def get_logical_disk_usage_percentage(logical_disk)
    logical_disk.each do |disk|
      disk.push(((disk[2].to_f/disk[3].to_f)*100).round(2))
      disk.push(100.00-disk[5])
    end
    return logical_disk
  end

  def run
    timestamp = Time.now.utc.to_i
    logical_disk = get_logical_disk_metrics()
    logical_disk = get_logical_disk_usage_percentage(logical_disk)

    logical_disk.each do |disk|
      metrics = {
        :"logical_disk_#{disk[0]}" => {
          :"size(Gb)" => disk[3],
          :"free_space(Gb)" => disk[2],
          :"free_space(%)" => disk[5],
          :"used_space(Gb)" =>  disk[4],
          :"used_space(%)" => disk[6]
        }
      }
      metrics.each do |parent, children|
        children.each do |child, value|
          output [config[:scheme], parent, child].join("."), value, timestamp
        end
      end
    end
    ok
  end
end

Some general pointers:

Refrain from using tmp as variable names. raw_output will do the job better.

Working with arrays as object replacement (x[0] is the caption, x[1] is the type...) is cumbersome, unreadable, and difficult to maintain. I suggest moving to Hash:

tmp.map(&:split).select { |_, drivetype, _, _| drivetype.to_i == 3 }.map do |caption, drivetype, freespace, size|
  {
    caption: caption,
    drivetype: drivetype,
    freespace: (freespace.to_f/bytes_to_gbytes).round(2),
    size: (size.to_f/bytes_to_gbytes).round(2),
    usedspace: ((size.to_f - freespace.to_f)/bytes_to_gbytes).round(2)
  }
end

This code returns an array of hashes, one for each disk with drivetype '3'. The variables (caption, drivetype, etc...) are created when ruby translates the array to the list of variables declared, so actually the tmp.map(&:split).map { |caption, drivetype, freespace, size| ... } is like saying:

tmp.map do |disk|
  disk = disk.split
  caption = data[0]
  drivetype = data[1]
  freespace = data[2]
  size = data[3]
  ...
end


Drawing a grid on canvas I have created canvas (800x400) - and filled it with a grid. It works but the rendering of the grid (lines) takes around 3 seconds, which seems excessively long. Am I doing something wrong?

var drawGrid = function(w, h, id) {
    var canvas = document.getElementById(id);
    var ctx = canvas.getContext('2d');
    ctx.canvas.width  = w;
    ctx.canvas.height = h;

    for (x=0;x<=w;x+=20) {
        for (y=0;y<=h;y+=20) {
            ctx.moveTo(x, 0);
            ctx.lineTo(x, h);
            ctx.stroke();
            ctx.moveTo(0, y);
            ctx.lineTo(w, y);
            ctx.stroke();
        }
    }

    };

    drawGrid(800, 400, "grid");
<canvas id="grid"></canvas>

Try using a SVG object on the canvas:

<svg width="100%" height="100%" xmlns="http://www.w3.org/2000/svg">
    <defs>
        <pattern id="smallGrid" width="8" height="8" patternUnits="userSpaceOnUse">
            <path d="M 8 0 L 0 0 0 8" fill="none" stroke="gray" stroke-width="0.5" />
        </pattern>
        <pattern id="grid" width="80" height="80" patternUnits="userSpaceOnUse">
            <rect width="80" height="80" fill="url(#smallGrid)" />
            <path d="M 80 0 L 0 0 0 80" fill="none" stroke="gray" stroke-width="1" />
        </pattern>
    </defs>

    <rect width="100%" height="100%" fill="url(#smallGrid)" />
</svg>

produces a grid like this:

whereas changing fill="url(#smallGrid)" to fill="url(#grid)" produces the following:

Which then you can feed into the canvas like the following:

var drawGrid = function(w, h, id) {
    var canvas = document.getElementById(id);
    var ctx = canvas.getContext('2d');
    ctx.canvas.width  = w;
    ctx.canvas.height = h;

    var data = '<svg width="100%" height="100%" xmlns="http://www.w3.org/2000/svg"> \
        <defs> \
            <pattern id="smallGrid" width="8" height="8" patternUnits="userSpaceOnUse"> \
                <path d="M 8 0 L 0 0 0 8" fill="none" stroke="gray" stroke-width="0.5" /> \
            </pattern> \
            <pattern id="grid" width="80" height="80" patternUnits="userSpaceOnUse"> \
                <rect width="80" height="80" fill="url(#smallGrid)" /> \
                <path d="M 80 0 L 0 0 0 80" fill="none" stroke="gray" stroke-width="1" /> \
            </pattern> \
        </defs> \
        <rect width="100%" height="100%" fill="url(#smallGrid)" /> \
    </svg>';

    var DOMURL = window.URL || window.webkitURL || window;

    var img = new Image();
    var svg = new Blob([data], {type: 'image/svg+xml;charset=utf-8'});
    var url = DOMURL.createObjectURL(svg);

    img.onload = function () {
      ctx.drawImage(img, 0, 0);
      DOMURL.revokeObjectURL(url);
    }
    img.src = url;
}
drawGrid(800, 400, "grid");
<canvas id="grid"></canvas>

Of course, you'll need to play around with the sizes of the grid to match what you want. However you'll find that this method is much faster.


Finding matching strings in a column I have col D (in sheet1 called Students), col A (in Sheet2 called Students too) and col B (in sheet2 called Age).  In colD there are a lot of similar Students names that are sorted from A to Z.

I have this code that finds matched students name and put his age in col D (sheet1):

Set rngSearch = Sheets("Sheet1").Range("D:D")
For Each rngSearch In Drng
    Set rngFound = Arng.Find(What:=rngSearch, LookIn:=xlValues, LookAt:=xlWhole,   SearchOrder:=xlByRows, SearchDirection:=xlNext, MatchCase:=False, SearchFormat:=False)
    'if studentname is found:
     If Not rngFound Is Nothing Then
         rngSearch.Offset(0, 0) = rngFound.Offset(0, 1)
     Else
         rngSearch.Offset(0, 0) = "-"
     End If
Next

I want to loop col D once and if exists a match students name in colA (sheet2), I want to copy in colD (sheet1) the B column (sheet2) that matches to colA (sheet2).

I sorted colD because I don't want to loop every similar students name.  I mean: if a student name (for example: Andrew) in colD matches to a student name ('Andrew') in colA, then copy colB (matches to col A- for example Andrew has 15 years old) in colD (sheet1).  And if the string 'Andrew' is repeated in colD (I found it in another cells), don't loop for it again colB (sheet2), but copy the value from the first string.

For example:

(Sheet1)

colD: Students:
row1: Andrew
row2: Andrew
row3: Andrew
row4: Andrew
row5: Andrew
row6: Andrew
row7: Ben
row8: Ben
row9: Edoardo
row10: Helen
row11: Leonardo
row12: Leonardo
row13: Robert
rowN: ..

(Sheet2)

      Students:  colB: Age:
row1: Michael          16
row2: Timoth           20
row3: Andrew           15
row4: Edoardo          19
row5: Ben              13
row6: Robert           24
row7: Helen            17

What I want:

(Sheet1)

colD: Students:
row1: 15
row2: 15
row3: 15
row4: 15
row5: 15
row6: 15
row7: 13
row8: 13
row9: 19
row10: 17
row11: 0
row12: 0
row13: 24
rowN: ..

Can anyone help me to optimize this code, please?

I solved the problem. Here is the code:

Set rngSearch = Sheets("Sheet1").Range("D:D")
For Each rngSearch In Drng
    'all activities are searched in Arng:
    Set rFnd = Sheets("Sheet1").Range("D2:D" & lastRowD).Find(What:=sText, LookAt:=xlPart)
    Set rngFound = Arng.Find(What:=rngSearch, LookIn:=xlValues, LookAt:=xlWhole, SearchOrder:=xlByRows, SearchDirection:=xlNext, MatchCase:=False, SearchFormat:=False)

    If Not rFnd Is Nothing Then
        rngSearch.Offset(0, 0) = rngSearch.Offset(-1, 0) 'if found multiple strings, don't loop column A but copy the row above
    Else
        'if activity is found:
        If Not rngFound Is Nothing Then
            rngSearch.Offset(0, 0) = rngFound.Offset(0, 1)
        Else
            rngSearch.Offset(0, 0) = "0"
        End If
    End If
Next


Pandas Groupby operation on a large dataset I am working with a large dataset expanding > 50 years. Each year has ~10 million lines of records with multiple variables/columns. I need to perform groupby operations by location and time. My code runs extremely slow - it takes 2-5 hours to process 1 year's data depending on the number of stations in the year. I looked at a few posts on multiprocessing, but since I have no experiences with it, I am not sure if that method applies to my problem. I'd appreciate it if someone can point out how I can make the code more efficient.

#!/usr/bin/env python
# encoding: utf-8
import numpy as np
import pandas as pd
import datetime
import argparse
from scipy.stats.mstats import hmean

def Nstat(df):
    duMW = [6,7,8,9,30,31,32,33,34,35,98]
    d = {}
    d['NMW']  = df['MW'].count()
    d['NPW']  = df['PW'].count()
    d['NDU']  = df.loc[ isd['RH']<=90,'MW'].isin(duMW).sum()
    d['NDU6']  = (df.loc[ df['RH']<=90,'MW']==6 ).sum()
    d['NDU7']  = (df.loc[ df['RH']<=90,'MW']==7 ).sum()
    d['NDU8']  = (df.loc[ df['RH']<=90,'MW']==8 ).sum()
    d['NDU9']  = (df.loc[ df['RH']<=90,'MW']==9 ).sum()
    d['NDU30'] = (df.loc[ df['RH']<=90,'MW']==30).sum()
    d['NDU31'] = (df.loc[ df['RH']<=90,'MW']==31).sum()
    d['NDU32'] = (df.loc[ df['RH']<=90,'MW']==32).sum()
    d['NDU33'] = (df.loc[ df['RH']<=90,'MW']==33).sum()
    d['NDU34'] = (df.loc[ df['RH']<=90,'MW']==34).sum()
    d['NDU35'] = (df.loc[ df['RH']<=90,'MW']==35).sum()
    d['NDU98'] = (df.loc[ df['RH']<=90,'MW']==98).sum()
    d['NDUpw'] = (df.loc[ df['RH']<=90,'PW']==3).sum()
    d['VIS_Hvg'] = hmean(df.loc[df['VIS']>0,'VIS'])
    d['Vi_Avg'] = df['Vi'].mean()

    return pd.Series(d,index=['NMW','NPW',\
'NDU','NDU6','NDU7','NDU8','NDU9','NDU30','NDU31','NDU32',\
'NDU33','NDU34','NDU35','NDU98','NDUpw','VIS_Hvg','Vi_Avg'])

if __name__ =='__main__':
    parser = argparse.ArgumentParser()
    parser.add_argument("start_year",type=int,help='4-digit start year')
    parser.add_argument("end_year",type=int,help='4-digit end year')
    args = parser.parse_args()
    years = np.arange(args.start_year,args.end_year)

    dTypes = {
        'NMW':'Int32',\
        'NPW':'Int32',\
        'NDU':'Int32',\
        'NDU6':'Int32',\
        'NDU7':'Int32',\
        'NDU8':'Int32',\
        'NDU9':'Int32',\
        'NDU30':'Int32',\
        'NDU31':'Int32',\
        'NDU32':'Int32',\
        'NDU33':'Int32',\
        'NDU34':'Int32',\
        'NDU35':'Int32',\
        'NDU98':'Int32',\
        'NDUpw':'Int32'\
    }

    for iyr,yr in enumerate(years):
        print('process year {:d} at {:s}'.format(yr,datetime.datetime.now().strftime('%m-%d %H:%M:%S')))
        isd = pd.read_hdf('isd_lite_'+str(yr)+'.h5',dtype={'STATION':'str'})

        isd['YYYYMM'] = pd.to_datetime(isd['YYYYMMDDHH'],format='%Y%m%d%H').dt.strftime('%Y%m')
        isd['VIS'] = isd['VIS']/1000.
        isd['Vi'] = isd['VIS'].apply(lambda x: 1/x if x>0 else np.nan)

        print('>> groupby and output at {:s}'.format(datetime.datetime.now().strftime('%m-%d %H:%M:%S')))

        stn_month = isd.groupby(['STATION','YYYYMM']).apply(Nstat).reset_index().astype(dTypes)
        stn_month.to_csv('stn_month_'+str(yr)+'.csv',index=False,float_format='%.3f')

The last groupby (by STATION and YYYYMM) operation is most time consuming.
I have a fairly good work station (256 cores) and want to maximize the use of it.

A sample file is provided here. It takes 7 min to process this file. Not horribly long due to a small number of stations.

Consider the following points:

Pandas has a good datetime functionality; you shouldn't cast into strings and then later on group by those. It's unnatural and slow. Instead, just do:

isd['YYYYMM'] = pd.to_datetime(isd['YYYYMMDDHH'],format='%Y%m%d%H')

And then in the groupby, you can simply do

stn_month = isd.groupby(['STATION', isd['YYYYMM'].dt.to_period('M')]) ...

In general, using apply is usually not great for performance. First, notice that you are doing a lot of things inside Nstat that are not necessary: all the lines like d['NDU6']  = (df.loc[ df['RH']<=90,'MW']==6 ).sum() are unnecessary in a sense that you can just precompute this outside of the function. As a side note, the way that you write is unnatural to me and I would more simply do:

df[(df['RH'] <= 90) & (df['MW'] == 6)]

Second, the agg function also takes a dictionary so that you can just do:

isd.groupby(['STATION', isd['YYYYMM'].dt.to_period('M')]).agg({'MW' : 'count', 'PW' : 'count', 'Vi': 'mean'})

I hope this will get you started.


Can I improve my jquery dialog code, critisicm welcome Fairly new to jquery, can i improve this code please?

HTML

login

Javascript

function Dialog() {

    // Event handler for a dialog click.
    $('a.dialog').click(function () {

        var dialog = $("#" + this.id + "_dialog");

        // Bind the hide functions here, as the elements have now been loaded into the DOM.
        $("#overlay, #close").click(function () {
            HideDialog(dialog)
        });
        window.onresize = function () {
            CentreBoxInViewport(dialog);
        };

        SetupDialog(dialog);
        return false;
    });

    // Event handler for an image click.
    $('div.status > a').click(function () {

        // Get the object for the lightbox.
        var dialog = $("#lightbox");

        // Get the path to the image.
        var path = this.getAttribute("href");

        // Bind the hide functions here, as the elements have now been loaded into the DOM.
        $("#overlay, #close").click(function () {
            HideDialog(dialog)
        });
        window.onresize = function () {
            CentreBoxInViewport(dialog);
        };

        SetupLightBox(path, dialog);
        return false;
    });
}

function SetupLightBox(path, dialog) {

    // Create a new image.
    var image = new Image();

    // The onload function must come before we set the image's src, see link for explanation.
    // http://www.thefutureoftheweb.com/blog/image-onload-isnt-being-called.

    // Anonymous function set to onload event, to make sure we only proceed if the image has been loaded.
    image.onload = function () {
        $('#image').attr('src', path)
        CentreBoxInViewport(dialog);
        ShowOverlay(dialog);
    };

    // Set the src, and show the image.
    image.src = path;
}

function SetupDialog(dialog) {
    CentreBoxInViewport(dialog);
    ShowOverlay(dialog);
}

function ShowDialog(dialog) {
    dialog.fadeTo(200, 1);
}

function HideDialog(dialog) {
    dialog.fadeTo(200, 0, function () {
        dialog.hide();
        HideOverlay();
    });
}

function ShowOverlay(dialog) {
    $('#overlay').fadeTo(200, 0.5, function () {
        ShowDialog(dialog)
    });
}

function HideOverlay() {
    $('#overlay').fadeTo(200, 0, function () {
        $('#overlay').hide();
    });
}

function CentreBoxInViewport(dialog) {

    // Get the dimensions of the viewport.
    var height = $(window).height()
    var width = $(window).width();

    // Calculate the position of the lightbox.
    var boxtop = (height / 2) - dialog.height() / 2;
    var boxleft = (width / 2) - dialog.width() / 2;

    // Position the box.
    dialog.css({
        top: boxtop,
        left: boxleft
    });
}

In JavaScript it is custom to give functions names that start with a lowercase letter so that they are not confused with objects.
Personally I'd avoid "hardcoding" the connection of the id of the link with the id of the dialog. (login -> login_dialog). Instead you could use the href of the link to refer to dialog. Example:

login

var dialog = $(this.href.replace(/.*(?=#[^\s]*$)/, ''));

(The replace is necessary due to bugs in IE < 8)

You are repeatedly assigning the click and resize event handler on each click, so that each time a dialog is shown a additional handler is added, but never removed.

(There is more, however I don't have time right now. Maybe I'll come back later)


Autofiltering and deleting rows two times too slow For Each MyIndex In Worksheets
        If MyIndex.Name = "Inv_IB" Or MyIndex.Name = "Inv_MP"
            With Sheets(MyIndex.Name)
                nFilasActual = .Cells(Rows.Count, 4).End(xlUp).Row
                Set dataRng = .Range("A2", .Range("N" & nFilasActual))
                dataRng.Sort key1:=Range("E2"), order1:=xlAscending
                dataRng.AutoFilter Field:=5, Criteria1:="<>ME"
                Application.DisplayAlerts = False
                dataRng.Offset(1, 0).Resize(dataRng.Rows.Count - 1).SpecialCells(xlCellTypeVisible).Rows.Delete
                Application.DisplayAlerts = True
                .ShowAllData
                nFilasActual = .Cells(Rows.Count, 4).End(xlUp).Row
                Set dataRng = .Range("A2", .Range("N" & nFilasActual))
                dataRng.Sort key1:=Range("C2"), order1:=xlAscending
                dataRng.AutoFilter Field:=3, Criteria1:="<>BODEGA IND"
                Application.DisplayAlerts = False
                dataRng.Offset(1, 0).Resize(dataRng.Rows.Count - 1).SpecialCells(xlCellTypeVisible).Rows.Delete
                Application.DisplayAlerts = True
                .ShowAllData
                nFilasActual = .Cells(Rows.Count, 4).End(xlUp).Row
                dataRng.Sort key1:=Range("H2"), order1:=xlAscending
                .Range("A2:A" & nFilasActual).FormulaR1C1 = "=IF(RC[2]<>R[-1]C[2],1,IF(RC[6]=R[-1]C[6],R[-1]C,R[-1]C+1))"
                .Range("O2").FormulaR1C1 = "=1"
                .Range("O3:O" & nFilasActual).FormulaR1C1 = "=R[-1]C + 1"

            End With
        End If
Next MyIndex

I'm saving info on these 2 sheets from another WorkBook. Then I want to filter this and delete the rest of the info. This is taking too long for what I want. Any ideas for how I can speed this up?

The code as is suffers from a few problems, the most obvious at first glance ones being:

Lack of Vertical Spacing

All the code is compressed into very little vertical space. It's not easy to mentally follow along with an infodump. Instead a maintainer would have it easier if they could see logical units in the code.

This can be as easy as adding a single empty line before each section starting with `nFilasActual´.

Lack of Abstraction

This code is indented four levels deep. After the second level of indentation, you should consider extracting the code into a method, just to maintain a cohesive level off semantic abstraction.

Intermingling "high-level" method calls and "low-level" iterations is very much less than ideal. Instead you want code that's in the same procedure to have a roughly equal level of abstraction. This reduces the strain on the reader, because reading requires less context-switching.

Here are a few simplifications I'd make:

Extract Simple Helper Procedures

Since you repeat the following code twice, it's a good candidate for a separate procedure:

Sub SilentDeleteVisibleRows(filteredRange As Range)
    Application.DisplayAlerts = False
    filteredRange.Offset(1, 0).Resize(filteredRange.Rows.Count - 1) _
      .SpecialCells(xlCellTypeVisible).Rows.Delete
    ' This assumes DisplayAlerts was True before calling this method.
    ' Alternatively the state could be stored into a variable.
    Application.DisplayAlerts = True
End Sub

Searching the number of lines is also something you do thrice:

Function NumberOfLines(sheet As Worksheet) As Long
    NumberOfLines = sheet.Cells(Rows.Count, 4).End(xlUp).Row
End Function

You might also want to extract the whole With block into some procedure with a good name to help the readability of the code overall (and reduce the level of indentation, freeing horizontal space).


Converting from base-10 to word The following code is used to convert a base-10 number to a base-N number, where N is a length of a given alphabet that contains characters of which a base-N number can consist.

The number to convert is always increasing, like from 1 to 45789, not from 536 to 13. The resulting number may not contain zero as first digit, so I need to carry the one.

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

typedef unsigned long long ull;

void conv(ull num, const char *alpha, char *word, int base){
    while (num) {
       *(word++)=alpha[(num-1)%base];
       num=(num-1)/base;
    }
}

int main(){
   ull nu;
   const char alpha[]="abcdefghijklmnopqrstuvwxyzOSR34";

   /* "OSR43" was added to show that the letters of alpha
      are NOT in alphabetical order */

   char *word=calloc(30,sizeof(char));
   // word always contains null-terminator
   int base=(int)strlen(alpha);
   for (nu=1;nu<=1e8;++nu) {
       conv(nu,alpha,word,base);
       printf("%s\n",word);
   }
   return 0;
}

This code's working fine but I need to speed it up as much as possible. How do I do it?

Improving conv

To convert a base-10 number to base-N, I don't think it gets faster than the conv function your wrote. I would write it a bit differently though:

void toBaseN(ull num, const char *alpha, int base, char *word) {
    while (num) {
       *(word++) = alpha[(num - 1) % base];
       num = (num - 1) / base;
    }
}

What I changed:

Renamed the function: conv doesn't describe what it does
Rearranged the parameters: base is tightly related to alpha (the length), and I think it's good to have out-parameters as last
Add spaces around the operators to improve readability

For the record, this function doesn't return what I would expect.
It returns the digits in reverse order, which is a bit odd.
For example with the given alphabet,
it returns ba for 33 and ca for 34, when I would expect ab and ac, respectively.

The function has a number expectations with regards to the input:

word is expected to be big enough to contain the digits
word is expected to be filled with nulls
base is expected to be the length of alpha

The first one is reasonable and quite natural,
the others are not, and should be documented in a comment above the function.

Improving main

const char alpha[]="abcdefghijklmnopqrstuvwxyzOSR34";
char *word=calloc(30,sizeof(char));
// word always contains null-terminator
int base=(int)strlen(alpha);
for (nu=1;nu<=1e8;++nu) {

There are a couple of things I don't like about this bit:

Too packed code: add more spaces around operators like I did in the previous point
The comment // word always contains null-terminator seems misplaced. It seems you intended it for the line above it. It's more intuitive and readable to have comments above the line they refer to. The make it even more clear, it would be good to leave a blank line before the comment, and perhaps even after the statement
What is magic number 30? It would be better to make this a global constant
base is derived from alpha, it's tightly related, so I'd move these closer to each other

I suggest this writing style:

const char alpha[] = "abcdefghijklmnopqrstuvwxyzOSR34";
int base = (int) strlen(alpha);

// word always contains null-terminator
char *word = calloc(30, sizeof(char));

for (nu = 1; nu <= 1e8; ++nu) {

Improving speed

As I mentioned above, the general functionality of conv without a context is as fast as it can be. (And incorrect: normally I'd expect digits to be reversed.)

In the context of printing base-N numbers within the range [start : end],
you can do better.
You could convert start and end to base-N,
to do the counting in base-N.
This will be significantly faster than counting in base-10 and converting in every step.


Generating 'Decent' numbers While solving a problem from HackerRank, though it's an easy one, I wrote this big program. Not sure if recursion is applicable and hold good but to me it looked like it isn't. However when I run the program I see my code executed fairly well.
Here is the problem statement ("Sherlock and The Beast"):

Print the largest decent number.
A 'Decent' Number can have:

Only 3 and 5 as its digits.
Number of times 3 appears is divisible by 5.
Number of times 5 appears is divisible by 3.
Input should be the number of digits (N) where 1 <= N <= 100000.

For 10 such test cases if I have the inputs:
3647
8884
1233
99999
130
11111
3455
23454
123211
345

My code ran in 0.008688 seconds.
Is there any room for improvement?
int filldigits(num) {
    int max_div, num_of_5s, num_of_3s;
    if (num/3 == 0) {
        num_of_5s = num;
        print(num_of_5s, 0);
        return 0;
    }
    max_div = num/3;
    for (max_div; max_div>0; max_div--) {
        num_of_5s = max_div * 3;
        num_of_3s = num - num_of_5s;
        if (num_of_3s % 5 == 0 ) {
            print(num_of_5s, num_of_3s);
            return 0;
        }
        else
            continue;
    }

    max_div = num/5;
    for (max_div; max_div>=1; max_div--) {
        num_of_3s = max_div * 5;
        num_of_5s = num - num_of_3s;
        if (num_of_5s % 3 == 0)  {
            print(num_of_5s, num_of_3s);
            return 0;
        }
        else
            continue;
    }
    return -1;
}

Is there any room for improvement?

Yes there is.

You have a Diophantine equation \$3x + 5y = N\$, (\$3x\$ and \$5y\$ being the number of occurrences of 5 and 3 respectively. A base solution is \$x = 2N, y = -N\$. All other solutions are in form of \$x = 2N - 5k, y = -N + 3k\$. That is, you just need to find \$k\$, which makes both numbers positive. Such as k = (N-1)/3 + 1 (if x becomes negative, there are no solutions).

Bottom line is that neither the loop nor the special cases are needed.

See Wikipedia article for details.

Edit: Paper exercise as requested:

Let N = 8. Base solution: x = 16, y = -8. (16*3 - 8*5 = 48 - 40 = 8).

k = (8 - 1)/3 + 1 = 3. x' = 16 - 3*5 = 1; y' = -8 + 3*3 = 1.

3*x' + 5*x' = 3 + 5 = 8.

Edit: another exercise.

Let N = 30. Base solution: x = 60, y = -30.

k = 29/3 + 1 = 10.

x' = 60 - 5*10 = 10; y' = -30 + 3*10 = 0, yielding the decent number of 30 occurrences of 5 and 0 of 3; just as requested.


Fetching daylerees colour schemes for PHPStorm from Github I wanted to download all colour schemes for phpstorm and was too lazy to do it manually so I wrote a little script for it in python, because why not.

This is my very first Python script and I'm still learning through tutorials and what not.

Things it does:

Asks to input the path where to save the files
If the destination folder does not exist, creates it
Makes sure the the path ends with '/'
Instantiates a crawler instance
Crawler fetches the initial JSON payload
It loops over the items to check whether an item is a file or a directory
If it is a file, downloads it. If a folder, instantiates a new crawler instance with that directory path
Before downloading the files, it checks if the file exists already and prints the messages accordingly.

Things I can already see that I will go back and improve:

Make passing the link dynamic, instead of hard coding.
Implement a better path checking / validation.
use try / except methods instead where applicable.

And the code:

import urllib
import json
import time
import os
import re

class Crawler:
    def __init__(self, url, path):
        self.url = url
        self.path = path
        self.counter = 0
        self.total_files = 0

    def crawl(self):
        self.get_contents()

    def get_contents(self):
        response = urllib.urlopen(self.url).read()
        collection = json.loads(response)
        self.total_files = len(collection)
        for data in collection:
            if data.get('type') == 'file':
                self.download(data.get('name'), data.get('download_url'))
            elif data.get('type') == 'dir':
                dir_crawler = Crawler(data.get('url'), self.path)
                print('\nFound a directory. Pausing the main download and crawling the directory - ' + data.get(
                    'name') + '\n')
                dir_crawler.crawl()
                print('\n' + data.get('name') + ' directory is done. Continuing from where I left off.\n')
            time.sleep(1)

    def download(self, name, link):
        if name and not os.path.exists(self.path + name):
            download = urllib.URLopener()
            download.retrieve(link, self.path + name)
            self.counter += 1
            print('Downloading ' + name + ' (' + str(self.counter) + ' out of ' + str(self.total_files) + ')')
        else:
            print('File: ' + name + ' already exists in ' + self.path)

download_location = raw_input(
    "Where would you like me to save the files? (e.g. ./ or ../ or ./themes/ or /path/to/folder): ")
if download_location and re.match(r'.+/$', download_location):
    if not os.path.exists(download_location):
        os.mkdir(download_location)
    crawler = Crawler('https://api.github.com/repos/daylerees/colour-schemes/contents/jetbrains',
                      download_location)
    crawler.crawl()
    print('\n\nAll done. Themes have been saved into ' + download_location +
          '. Thank you for your cooperation. Have a nice day.')
else:
    print('The path must end with a forward slash.')

Instead of os.path.exists you may want to specifically use os.path.isdir and isfile. isdir checks if the path matches an existing folder, and isfile checks for files. It is possible for the path you pass to be valid but not of the correct type for your purposes, which would be a significant error for your folders as you then have invalid paths that you'd attempt to download to.

Also to make it easier on your user, I would wrap the main code in a while loop so that if they enter an invalid path the program doesn't exist the program still doesn't exit:

while True:
    download_location = raw_input(
        "Where would you like me to save the files? (e.g. ./ or ../ or ./themes/ or /path/to/folder): ")
    if download_location and re.match(r'.+/$', download_location):
        if not os.path.isdir(download_location):
            os.mkdir(download_location)
        crawler = Crawler('https://api.github.com/repos/daylerees/colour-schemes/contents/jetbrains',
                          download_location)
        crawler.crawl()
        print('\n\nAll done. Themes have been saved into ' + download_location +
              '. Thank you for your cooperation. Have a nice day.')
        break
    else:
        print('The path must end with a forward slash.')

Putting break after your print call will end the infinite loop once the code has  successfully run but otherwise it will keep asking for input.

Also no need for regex. Python has a str.endswith function that detects if a string ends with a certain string. So you could just use:

    if download_location.endswith('/'):

raw_input will at least return a string, so you can just call this function on download_location. If it's an empty string it will return False since an empty string doesn't end with '/'.


strlen and strcmp implementation in x86 FASM assembly I wrote my own implementation of strlen and strcmp from C in x86 FASM and I would like to know is there anything that should be changed or improved.

strlen needs string in eax and is returning length of that string into ebx.

strlen:
    mov ebx,0
    strlen_loop:
        cmp byte [eax+ebx],0
        je strlen_end
        inc ebx
        jmp strlen_loop
    strlen_end:
        inc ebx
        ret

strcmp needs two strings (one in eax, other in ebx) and returns into ecx 0 if strings are equal or 1/-1 if they are not.

strcmp:
    mov ecx,0
    strcmp_loop:
        mov byte dl,[eax+ecx]
        mov byte dh,[ebx+ecx]
        inc ecx
        cmp dl,0
        je strcmp_end_0
        cmp byte dl,dh
        je strcmp_loop
        jl strcmp_end_1
        jg strcmp_end_2
    strcmp_end_0:
        cmp dh,0
        jne strcmp_end_1
        xor ecx,ecx
        ret
    strcmp_end_1:
        mov ecx,1
        ret
    strcmp_end_2:
        mov ecx,-1
        ret

strlen

Correctness (Return Value): You are violating the convention for the strlen function, which is documented as returning the number of characters between the beginning of the string and the terminating null character without including the terminating NUL character. Your code includes the terminating NUL, given the position of the inc ebx instruction.

This may be fine if you control both the function's implementation and its usage, but it is confusing because it defies programmers' expectations and will be a recurring source of bugs. If you're going to return a length that includes the terminating NUL, you should consider calling your function something different than strlen.
Interface (ABI): All x86 calling conventions return a function's result in the eax register. Although you have documented your function as returning the result in ebx, this is utterly bizarre and is guaranteed to trip up every programmer who ever uses your code. When writing everything in assembly, you are of course free to define your own custom calling conventions, but you should only do so when there is a good reason (like an optimization possibility). I can't see a good reason here. It would be just as easy for you to arrange for your code to produce the result in eax, right where programmers will expect it to be.

It is also somewhat unusual to pass an argument in the eax register, but calling conventions vary in which registers they use to pass arguments, so this isn't flying in the face of every convention ever and is therefore more excusable. However, when you're writing in assembly and you have the opportunity to make these types of decisions, you should consider your choices carefully: what makes the most sense? what will be the easiest to use? what will be the most flexible? what will be the most efficient? Have a good reason for your choice! In this case, passing a pointer in eax makes little sense, since eax is almost universally used for return values, and pointers are almost never going to be the return value of a function. By choosing eax as the input register, you've virtually guaranteed that every caller will need an extra mov instruction to shuffle the input parameter into the appropriate register. Why create this situation when you don't have to?
Style (Indentation): The way you've indented the code, with the labels at the same level as the instructions, makes it difficult to read because all of the instructions aren't lined up. Instead, consider outdenting the internal labels (branch targets) so that they match the function name (external symbols). That will allow all instructions in the function to be lined up at the same vertical column, and thus allow anyone reading the code to skim it easily.

(The only drawback of this is that it makes it a bit harder to determine what is a function label and what is an internal label. Judicious use of whitespace is the most effective way to combat this. I also use a naming convention that allows me to recognize the difference at a glance.)

Also, use variable numbers of spaces between the opcode and the operands to ensure that all operands line up in vertical columns.
Style (Documentation): Maybe you have extensive external documentation that accompanies these functions, but probably not. Even if you do, documentation right in the code is extremely useful, easier to maintain, and should not be forsaken without a very good reason.

When writing functions in assembly, I use kind of a standard header containing a description, the inputs, the outputs, and the clobbers. If there are any assumptions made by the function, I will also call those out explicitly. So, in this case, following what you had documented in the question (even though I just called it into question), I'd write something like this:

; Determines the length of a C-style NUL-terminated string.
;
; Inputs:   EAX = address of beginning of string buffer
; Outputs:  EBX = length of the string, including the NUL terminator
; Clobbers: <none>

You should feel free to develop your own style. The important thing is having this information at the ready. Especially when programming in assembly where you can (and should) invent your own per-function custom calling conventions.
Optimization (Register Clearing): There is only one circumstance in which you should use mov reg, 0, and that's when you're trying to avoid clobbering the flags (e.g., in a sequence that contains a CMOVxx or a SETxx). In all other cases, you should clear a register by XORing it with itself: xor reg, reg. Peter Cordes has written a comprehensive explanation of the reasons why here, but it's enough to just remember the rule. Basically, it's shorter (and thus faster) and it breaks dependencies (which is sometimes important, and never hurts).
Optimization (Reduce Branching): When you want performance, avoid branching. That means that you should minimize branching as much as possible inside tight inner loops. strlen is the canonical example of a tight loop. You've got two branches inside of the loop, a conditional je and an unconditional jmp. Consider rearranging the code so that you only have one. This is almost always possible. Ideally, you want the common case to fall through and the unusual case to branch, but even if that's not possible, it's still better to have a single branch than several of 'em.
Optimization (Elide Repeated Instructions): You've repeated the inc ebx instruction. Consider how the code can be rearranged to avoid the need to do this. This isn't just the standard DRY advice, but a potential optimization opportunity. However, sometimes this trades off with the previous advice about reducing branching. Repeating a simple instruction in order to avoid a branch is virtually always worth it. As is arranging code so that you, say, speculatively increment inside of the loop, only to undo it by decrementing outside of the loop.
Optimization (Memory Access): In C terms, your code is maintaining an "index" into the string. That means you have to do a complex load-and-compare (cmp byte [eax+ebx], 0). You could instead maintain a "pointer" into the string, which would allow you to just do cmp byte [eax], 0. This is not only 1 byte shorter, but may execute more quickly.

Consider:

; Determines the length of a C-style NUL-terminated string.
;
; Inputs:   EBX = address of beginning of string buffer
; Outputs:  EAX = length of the string, including the NUL terminator
; Clobbers: CL, flags
strlen:
    lea    eax, [ebx + 1]

strlen_loop:
    mov    cl, byte [eax]
    inc    eax
    test   cl, cl
    jnz    strlen_loop

    sub    eax, ebx
    ret

This is about the most efficient way of implementing a standard strlen function:

The lea is used as a super-mov, allowing the value in ebx to be copied into eax, while simultaneously incrementing it by 1. (Although mov+inc would have been the same length in terms of bytes, lea might be slightly more efficient on certain processors.) ebx will remain a pointer to the beginning of the string, while eax will be a pointer to the current location in the string.
Inside of the loop, the CISC-style cmp instruction that took a memory operand has been split up into separate RISC-style mov+test instructions for improved scheduling. In particular, this means we can do an inc of the pointer after loading the value that it pointed to. We couldn't do the inc after the cmp, because the inc would clobber the flags (in particular, the zero flag) that we were going to try and read. In this arrangement, the test can also macro-fuse with the jnz instruction.
I've written jnz instead of jne even though they result in identical opcodes because I think it's a better mnemonic in this case. We are testing the value, and then jumping if it is not zero (i.e., if the current character is not the NUL character).
Outside of the loop, we do a final subtraction of the current pointer from the starting pointer, which gives us the length of the string, not including the terminating null.

The biggest drawback is that we've introduced the use of an additional register (the 8-bit cl), which expands our "Clobbers" list in the documentation. See why documenting this is important? The only reason why this would be a drawback, of course, is if the caller/consumer of this function needed to preserve the value in cl, as that would require an extra instruction or two. However, those instructions would be outside of the loop, and thus off the critical path, so it's almost certainly worth it for the optimizations gained.

Interestingly, this is basically the same code that Microsoft's C compiler will emit for a strlen operation when you have intrinsics enabled (otherwise, it'll generate a call to the strlen standard library function, which has essentially this same code inside of it).

This is better than Sep Roland's optimized implementation because it avoids the AGI stall introduced by this sequence of instructions in Sep's code:

inc     ebx
cmp     byte [eax+ebx-1], 0

Sep is being similarly clever here, doing the inc first and then undoing it inside of the cmp to avoid clobbering flags, but modifying ebx immediately before you use it in the addressing operands list for cmp results in a potential stall. Reordering the instructions avoids the stall; even though we had to add one extra instruction, the performance improvement more than makes up for it (unless you're optimizing for size, which you aren't, unless you're code-golfing—it's only a 1-byte difference).

So, this looks pretty good, right? Indeed, it is. As I said, it's about the best you're going to do for a straightforward, literal translation of strlen into assembly. But if you stretch your mind a bit, you can do better.

The slowest part of this code is the mov  cl, byte [eax] instruction that accesses memory at the beginning of every iteration of the loop. There's nothing we can do about the fact that we have to access memory, but notice that we're only reading 1 byte at a time here. Under the hood, the processor only does DWORD-sized reads, so it's actually reading 4 bytes and then throwing away all but the lowest-order byte. Why don't we just read 4 bytes at a time? Then, we can parallelize the code to actually deal with 4 bytes at a time, effectively unrolling the loop by a factor of 4.

The trick to parallelizing the code is figuring out an efficient way of checking all 4 of the bytes that we load in each iteration of the loop for NUL characters.

One possible attempt is the following (MASM syntax, sorry):

; Determines the length of a C-style NUL-terminated string.
;
; Inputs:   EBX = address of beginning of string buffer
; Outputs:  EAX = length of the string, including the NUL terminator
; Clobbers: ECX, flags
strlen:
  mov   ecx, ebx

ALIGN 16
CountChars:
  mov   eax, DWORD PTR [ecx]

  test  al,  al
  jz    SHORT ReturnLength
  inc   ecx

  test  ah,  ah
  jz    SHORT ReturnLength
  inc   ecx

  test  eax, 0x00FF0000
  jz    SHORT ReturnLength
  inc   ecx

  test  eax, 0xFF000000
  jnz   SHORT CountChars

ReturnLength:
  sub   ecx, ebx
  mov   eax, ecx
  ret

I've introduced an alignment of the branch target (CountChars) because aligned branch targets are more efficient, and it makes a measurable difference here because we're branching to that location a lot (each time we loop). When optimizing for code speed over size, this is a no-brainer.

There are obvious variations on this theme, especially with regard to register assignment. You might ask, why not use eax for the pointer to the current location in the string buffer, as that would eliminate the need for the penultimate mov instruction. There is a good reason I didn't, though: using eax to hold the character being tested inside of the loop makes the latter two test instructions (the ones that use immediate operands) 1 byte shorter than if any other register had been used. (Certain x86 instructions have optimized encodings when eax is the destination register for legacy reasons.) This doesn't normally matter too much, but it can help to ensure that a loop fits entirely within the cache. A 2-byte mov outside of the loop is a small price to pay for a 2-byte size reduction of the code inside the loop.

Details aside, this does get the job done with respect to testing 4 bytes at a time. But is it efficient? It feels like it might not be, because there's so much branching. Indeed, our "avoid branches in inner loops" intuition is right on, as this version is not significantly faster than the original. On modern architectures like Haswell, it's slightly faster; on older architectures, like Pentium 4 (where branch mispredictions are extremely expensive due to extremely long pipelines), it is slightly slower. In either case, the time delta is not even enough to matter. So this appears to be a dead-end. We bought ourselves a little bit of performance by reading a DWORD at a time, instead of a single byte, but any advantage gained is lost to all of the conditional branches that need to be executed each iteration of the loop.

In order to gain a real performance improvement, we need to take a different tack. And it needs to be one that reduces the number of branches. If we can find a branchless way to test all 4 bytes of the DWORD at once to see if any of them are 0 (a NUL byte), then we can reduce the number of branches in all but one iteration of the loop by a factor of 4. As it turns out, there is a solution for this, as described on the Bit Twiddling Hacks page.

Consider the following implementation (again in MASM syntax). It retrieves a DWORD and performs a series of bit manipulations on that value to determine if any of its byte are zero (i.e., contain NUL characters). If not, it branches back to the top and starts over with another DWORD-sized chunk of the string. If so, then it falls through and tests each of the four individual bytes (or, more accurately, three of the individual bytes, since if the low-order three bytes are non-zero, then we know the highest-order byte must be zero).

; Determines the length of a C-style NUL-terminated string.
;
; Inputs:   EBX = address of beginning of string buffer
; Outputs:  EAX = length of the string, including the NUL terminator
; Clobbers: ECX, flags
strlen:
  mov  ecx, ebx               ; save pointer to beginning of string for later

ALIGN 16
CountChars:
  mov  eax, DWORD PTR [ebx]   ; load DWORD value from current offset
  add  ebx, 4                 ; increment offset by number of bytes processed
  and  eax, 0x7F7F7F7F        ; mask out highest bit of each byte
  sub  eax, 0x01010101
  test eax, 0x80808080        ; TEST to ensure macro-op fusion with JZ
  jz   SHORT CountChars       ; if none of these 4 bytes were 0, start again with 4 more

  mov  eax, DWORD PTR [ebx-4] ; load previous 4 bytes (the ones we were just testing),
                              ;   using an immediate offset to compensate for eager
                              ;   increment in the loop while avoiding an AGI stall

  test al, al                 ; test low byte for NUL
  jz   SHORT ReturnLength
  inc  ebx                    ; wasn't the first byte, so increment byte count

  test ah, ah                 ; test the second byte for NUL
  jz   SHORT ReturnLength
  inc  ebx                    ; wasn't the second byte, so increment byte count

  test eax, 0x00FF0000        ; test the third byte for NUL
  jz   SHORT ReturnLength
  inc  ebx                    ; wasn't the third byte, so has to be the fourth (high) byte

ReturnLength:
  lea  eax, [ebx - 4]         ; undo the eager 4-byte increment; put result in EAX
  sub  eax, ecx               ; subtract starting pointer from current pointer
  ret

As it turns out, this is precisely the parallelization strategy taken by GLIBC (the C standard library implementation used by the GNU project, including the GCC compiler). The relevant code, including extensive descriptive comments, is available here; see also http://stackoverflow.com/q/20021066 and http://stackoverflow.com/q/11787810. This parallelized implementation is significantly faster on all x86 architectures than our previous "best" code—you're looking at around a 2× speedup.

I was pretty proud of this, until I thought some more and realized that there's no need to retest each of the 4 bytes. They were already tested in the initial bit-twiddling, and our answer about which of those 4 bytes contains the 0 is already available in the result of that bit-twiddling—we just need to extract it! This is great, because it eliminates three more branches, the three least likely to be correctly predicted (which of the 4 bytes actually contains the NUL is essentially random), and also the three most likely to enact a performance cost (since they are all back-to-back). It also saves us from having to reload the same 4 bytes again from memory.

; Determines the length of a C-style NUL-terminated string.
;
; Inputs:   EBX = address of beginning of string buffer
; Outputs:  EAX = length of the string, including the NUL terminator
; Clobbers: ECX, flags
strlen:
  mov  ebx, DWORD PTR [psz]
  xor  ecx, ecx

ALIGN 16
CountChars:
  mov  eax, DWORD PTR [ebx+ecx]
  add  ecx, 4
  and  eax, 0x7F7F7F7F
  sub  eax, 0x01010101
  and  eax, 0x80808080 ; must be AND here b/c we need result (but may not fuse)
  jz   SHORT CountChars

  ; At this point, EAX will contain one of the following values:
  ;  - 0x80808080 (if the low byte---byte 0---contained the 0)
  ;  - 0x80808000 (if byte 1 contained the 0)
  ;  - 0x80800000 (if byte 2 contained the 0)
  ;  - 0x80000000 (if the high byte, byte 3, contained the 0)
  bsf  eax, eax        ; find the first set bit, which gives either 7, 15, 23, or 31
  sub  ecx, 4          ; undo the eager increment by 4 from the loop
  shr  eax, 3          ; shift right by 3 (7 => 0, 15 => 1, 23 => 2, 31 => 3),
                       ;   thus giving the index of the NUL byte
  add  eax, ecx        ; add that byte index to the length, thus giving total length
  ret

This doesn't actually save a whole lot of time compared to the branching version, since the compare-and-branch sequence only gets executed once for each string. But, it does generally provide a minor performance boost, and it is never slower. This is true for all string lengths; there's no hidden overhead. Most of the cost comes from the relatively-expensive BSF operation. And, note that BSF is slower on AMD processors than Intel, so this code may be slightly less optimal on AMD, especially since AMD tends to use shorter pipelines and thus have less expensive branch mispredictions.

Now, I won't say that this is the fastest possible implementation, because there ain't no such thing as the fastest code, but it's pretty respectable. I'd even say it's about as good as you're going to get with bog-standard x86 instructions.

There is another frontier, and that is to use SIMD instructions (whether MMX, SSE 2, or SSE 4.2). This makes possible further substantial speed improvements, but my answer is much too long already, so I'll have to leave this stone unturned, since it is even more complicated to explain.

[NOTE: There's a fair amount of hand-waving in this answer with regards to which optimizations are sensible and what the performance differences are. I've actually done extensive benchmarking of this exact code before, on a variety of different x86 family architectures (mostly Intel), but I've left those details out of this answer for brevity (hah!). You'll just have to take my word for it. Or ask a question about it on Stack Overflow.]

strcmp

Aren't you tired of reading yet?


Summation of primes for large inputs I'm doing a problem from Project Euler to find the sum of all primes under two million.

My code manages to get a pretty good result for small inputs, but when trying inputs like million or two it just won't finish.

import time
import math

def primes_sum(limit):
    num_list = range(2, limit)
    i, counter = 2, 1
    while i <= math.sqrt(limit):
        num_list = num_list[:counter] + filter(lambda x: x%i!=0, num_list[counter:])
        i = num_list[counter]
        counter +=1
    print sum(num_list)

def main():
    start_time = time.time()
    primes_sum(2000000)
    print 'Rum time: '+str(time.time()-start_time)

if __name__ == "__main__":
    main()

The error for 2 million:

Internal error: TypeError: FUNCTION_TABLE[HEAP[(HEAP[(c + 4)] + 28)]] is not a function

Your primes_sum function seems to be inspired by the sieve of Erathostenes, but there are a number of things in your implementation that don't work out too well:

Do not remove items from you num_list. That's an expensive operation which basically requires rewriting the whole list. Plus, it makes the following point impossible.
If you have a list of consecutive integers, and you know the position i of a number n, you do not need to do modulo operations to figure out its multiples. Instead, repeatedly add n to i. Addition is much cheaper than modulo.
You don't actually need to have a list of numbers: instead keep a list of boolean values, with their position in the list indicating the number they represent. Ideally this would be a bit array, for maximal memory saving. But a list of Python booleans will save you 4 or 8 bytes per entry already.

A very straightforward implementation of a sieve would look like:

def sum_primes(n):
    sieve = [False, False] + [True] * (n - 1)  # 0 and 1 not prime
    for number, is_prime in enumerate(sieve):
        if not is_prime:
            continue
        if number * number > n:
            break
        for not_a_prime in range(number * number, n+1, number): #If you use range(__, n, __), the function leaves the last Boolean in the sieve list as True, whether or not it is a prime. If you change it to range(__, n+1, __), this problem is taken care of.
            sieve[not_a_prime] = False
    return sum(num for num, is_prime in enumerate(sieve) if is_prime)

On my system this spits the answer for 2 * 10^6 in under a second, manages to get 2 * 10^7 in a few, and can even produce 2 * 10^8 in well under a minute.


Jquery field add code - Improvements in readability, compactness or efficiency I've worked hard on this code and it functions exactly as I wanted, but I've been told it is a little bit unreadable. My focus is to get the code as clean as possible, and have things as efficient as possible.

This is the code I am working with:
http://jsfiddle.net/xT5X5/6/

//latest
var maxFields = 10;

$('.form').on('click', '.add', function () {
      var value_src = $(this).prev();
       var container = $(this).parent().prev();
    if ($.trim(value_src.val()) !== '') {
        if (container.children().length < maxFields) {
            var value = value_src.val();
            var html = '
' +
                '<input class="accepted" type="text" value="' + value + '" />' +
                '<input type="button" value="X" class="remove" />' +
                '
';

            $(html).appendTo(container);
            value_src.val('');
        } else {
            alert("You tried to add a field when there are already " + maxFields);
        }
    } else {
        alert("You didn't enter anything");
    }

})
    .on('click', '.remove', function () {
       $(this).parents('.line').remove();
});

$(".current").keyup(function(e) {
    if (e.which == 13) {
        $(this).next("input").trigger("click");
    }
});

$(document).on("keyup",".accepted",function(e) {
    if (e.which == 13) {
        $(this).closest('.copies').next().find(".current").focus();
    }
});

Can anyone give me any pointers on how I've done, and if there are any improvements that you can spot. Any feedback would be much appreciated!

Thank you.

First of all there is a great article on refactoring jQuery by Jack Franklin.

Second is my solution that i came up with in last 2 hours(ocd kicked in). My solution is probably not right, because i still have a ton to learn and would appreciate any feedback !!

Here fiddle with implementation. Here is a short version of what is going on inside, as you can see i tried to refactor each action in it's own method:

// detail object that handles our form
var details = {
    maxFields: 10,
    form: "",
    init: function(el) {},
    bindEvents: function() {},
    appendCopy: function(event) {},
    removeCopy: function(event) {},
    createCopy: function(value) {},
    focusOnEnter: function(event) {},
    addCopyOnEnter: function(event) {},
    _isValueEmpty: function(val) {},
    _isMaxReached: function(copyContainer) {},
    _getParent: function(event){},
    _getCurrent: function(event){}
    _getCopyContainer: function(event){},
    _getValue: function(event){},
};

// initialize our details object
    details.init(".form");

I think this could also be rewritten using $.deferred where each deferred would be responsible for tracking it's own progress (requirements,benefits,qualifications).


Reservoir Sampling of an enumerable collection of unknown size I have an enumerable source for which I do not know how big it is. I want to select a random element from it, without having to hold the whole collection in memory and with uniform distribution. I read about the reservoir sampling algorithm, and created a simple implementation for the very basic case where the reservoir size is 1, which is what I want.
Basically the idea is that if an item appears in position n starting by 1, if a random roll returns less than 1/n I pick that value as the current selected value.
static T Random<T>(IEnumerable<T> source, Random random)
{
    using var e = source.GetEnumerator();
    if(!e.MoveNext())
        return default;
    var result = e.Current;
    var count = 1;
    while(e.MoveNext())
    {
        if(random.NextDouble() < 1.0 / ++count)
            result = e.Current;
    }

    return result;
}

I created a demo project and the results seems to add up.
https://dotnetfiddle.net/0iXEck
public static void Main()
{
    var size = 1000;
    var tries = size * 100;
    var random = new Random();
    var array = Enumerable.Range(0, size).ToArray();
    var histogram = array.ToDictionary(k => k, v => 0);
    for(var i=0; i<tries; i++)
        histogram[Random(array, random)]++;
    foreach(var kv in histogram.OrderBy(kv => kv.Key))
        Console.WriteLine($"{kv.Key,5}: {kv.Value}");
}

I wonder if am I missing anything because probability knowledge is not my strongest point. Is this still "reservoir sampling algorithm" or does it have another name?

Let me try another answer.
Background info: Wikipedia Reservoir Sampling.
The intro header to the Wikipedia link states:

The size of the population n is not known to the algorithm and is
typically too large for all n items to fit into main memory.

But consider you have a dictionary to hold the results for all n items in your main memory.  And each example they provide has it returning a complete array, so it all has to fit into memory somehow.
You could make your life easier if your method accepted an IList<T> since that would return a known Count. Consider that each individual call to your method will enumerate over EACH element in the collection.  Since you pass in a 1000 element array, all 1000 elements are enumerated for one call, and you make 100 calls. I am just saying after the first call has been made, the size of the collection is no longer unknown.  Rather it is not remembered between invocations.
But that is not what you asked for. My answer continues to with IEnumerable<T> of unknown length.
Typically, one does not pass in the Random instance, but rather relies upon the method to have one handy.
The method name Random<T> is a poor name producing much confusion with the Random class. I suggest it be GetReservoirSample<T>.
For C#, the CodeReview community strongly supports uses braces to avoid one-liners.
There is no reason to OrderBy Key on histogram since it was created in Key order.
A quick reworking of your method, which includes Random as an optional parameter, would become:
private static Random _random = new Random();

public static T GetReservoirSample1<T>(IEnumerable<T> source, Random random = null)
{
    T result = default;
    int count = 0;
    if (random == null)
    {
        random = _random;
    }
    IEnumerator<T> e = source.GetEnumerator();
    // This enumerates over the entire collection!!!
    while (e.MoveNext())
    {
        if (random.NextDouble() < (1.0 / ++count))
        {
            result = e.Current;
        }
    }
    return result;
}

But that can be simplified by getting rid of the Random parameter.  And since you are iterating over all elements, there is no need for the enumerator.  You may use an foreach instead yielding smaller code.
public static T GetReservoirSample2<T>(IEnumerable<T> source)
{
    T result = default;
    int count = 0;
    // This enumerates over the entire collection!!!
    foreach (var element in source)
    {
        if (_random.NextDouble() < (1.0 / ++count))
        {
            result = element;
        }
    }
    return result;
}

IList versus IEnumerable
Finally, if you ever decide to change source to be an IList<T>, there is this version:
static T GetReservoirSample3<T>(IList<T> source)
{
    T result = source.Count > 0 ? source[0] : default;
    // This enumerates over the entire collection MINUS ONE!!!
    for (int i = 1;  i < source.Count; i++)
    {
        if (_random.NextDouble() < (1.0 / (i + 1.0)))
        {
            result = source[i];
        }

    }
    return result;
}

UPDATED
In my original example using IList<T>, there is no performance benefit.  Each example must enumerate over the full collection.  However, with a IList you can get a performance boost.  Essentially, your method remembers the last element that satisfies the condition but it must continue to enumerate over the remainder of the collection.
With a list, you can process the collection backwards and immediately return on the first item that satisfies the condition:
static T GetReservoirSample3B<T>(IList<T> source)
{
    for (int i = source.Count - 1; i > 0; i--)
    {
        if (_random.NextDouble() < (1.0 / (i + 1.0)))
        {
            return source[i];
        }

    }
    return source[0];
}

Also, I wanted to see the min and max values, so I altered Main for my own purposes.  I share it here:
public static void Main()
{
    var size = 1000;
    var tries = size * 100;
    var array = Enumerable.Range(0, size).ToArray();
    var histogram = array.ToDictionary(k => k, v => 0);
    var random = new Random();
    for (var i = 0; i < tries; i++)
    {
        histogram[GetReservoirSample3B(array)]++;
    }
    var min = int.MaxValue;
    var max = int.MinValue;
    foreach (var kv in histogram)
    {
        if (kv.Value < min)
        {
            min = kv.Value;
        }
        if (kv.Value > max)
        {
            max = kv.Value;
        }
    }
    foreach (var kv in histogram)
    {
        var extra = (kv.Value == min) ? "\t** MININUM ** " : (kv.Value == max) ? "\t** MAXIMUM **" : "";
        Console.WriteLine($"{kv.Key,5}: {kv.Value}{extra}");
    }
}

All that said, I do not know if you are correctly implementing a reservoir sampling.  From what I read, it expects you to return a sample subset of k elements where k is less than or equal to the source collection size of n.  Since you are returning a single element, that is for k == 1, then this produces the same distribution as:
static T GetReservoirSample3C<T>(IList<T> source) => source[_random.Next(source.Count)];