Is this a proper way to cancel an onMouseOver which uses setTimeout? (Vanilla JS) I've got a working solution for an onMouseOver event which rolls over a set of images and replaces them in the element. Due to my lack of knowledge in JS I would like to know if this is a proper solution. It works in Chrome and Firefox.

The solution sets attributes (i.e. isRunning="true", isCanceled="true") , reads them depending on the user action (onMouseOver, onMouseOut) and determines if the action should be continued.

Contraint: The solution must stay in Vanilla JS. No libraries may be used.

Thanks for any suggestion!

var thumbNails = {
  123: [
    "https://image.flaticon.com/icons/svg/188/188234.svg",
    "https://image.flaticon.com/icons/svg/188/188235.svg",
    "https://image.flaticon.com/icons/svg/188/188236.svg",
    "https://image.flaticon.com/icons/svg/188/188237.svg",
    "https://image.flaticon.com/icons/svg/188/188238.svg",
    "https://image.flaticon.com/icons/svg/188/188239.svg",
    "https://image.flaticon.com/icons/svg/188/188240.svg",
    "https://image.flaticon.com/icons/svg/188/188241.svg",
    "https://image.flaticon.com/icons/svg/188/188242.svg"
  ]
};

function rollover(element) {
  var id = element.getAttribute("id");
  if (!isRunning(element)) {
    setRunning(element, "true");
    setImageSrc(element, thumbNails[id][0]);
    start(1, thumbNails[id], element);
  }
}

function start(startIndex, thumbNails, element) {
  if (!isCanceled(element) && startIndex < thumbNails.length) {
    setTimeout(function() {
      if (!isCanceled(element)) {
        setImageSrc(element, thumbNails[startIndex]);
      }
      startIndex++;
      console.log("currentIndex: " + startIndex + " - Running? " + isRunning(element) + " - Cancel? " + isCanceled(element));
      start(startIndex, thumbNails, element);
    }, 700);
  } else {
    setRunning(element, "false");
    setCanceled(element, "false");
  }
}

function cancel(element) {
  console.log("cancel");
  if (isRunning(element)) {
    setCanceled(element, "true");
  }
}

// HELPERS

function isRunning(element) {
  return element.getAttribute("isrunning") === "true";
}

function setRunning(element, state) {
  element.setAttribute("isrunning", state);
}

function isCanceled(element) {
  return element.getAttribute("iscanceled") === "true";
}

function setCanceled(element, state) {
  element.setAttribute("iscanceled", state);
}

function setImageSrc(element, url) {
  element.setAttribute("src", url);
}
<html>
<head>
</head>
<body>
  <img id="123" onmouseover="rollover(this)" onmouseout="cancel(this)" src="https://image.flaticon.com/icons/svg/188/188238.svg" width="100" height="100">
</body>
</html>

Response to your prompt

  Due to my lack of knowledge in JS I would like to know if this is a proper solution.

This code sets attributes on the elements to store properties. While this works, it technically leads to invalid HTML1. Alternatives include using data attributes and setting properties on the variable representing the element. See responses to Custom attributes - Yea or nay? and  Associate Data With HTML Element (without jQuery)

Other review feedback

Many developers believe it is good to move all JavaScript code out of the markup. So instead of the onmouseover=... and onmouseout=... attributes, you could register those event handlers in the JavaScript using EventTarget.addEventListener()

document.addEventListener('DOMContentLoaded', function() {
  var image = document.getElementById('123');
  image.addEventListener('mouseover', rollover);
  image.addEventListener('mouseout', cancel);
});

With an approach like this, the this keyword inside the functions rollover and cancel refers to the element, so there isn't a need to accept the element as a parameter. If you did want to pass parameters to those functions, it could be achieved using partially applied-functions with Function.bind():

  //set context of 'this' to the image and pass string literal '123' as the first parameter
  image.addEventListener('mouseover', rollover.bind(image, '123'));

The id attribute can be used instead of .getAttribute('id').

So instead of this line:

var id = element.getAttribute("id");

the same can easily be achieved with:

var id = element.id;

This may make assigning that value to a new variable superfluous.

1https://stackoverflow.com/a/13041243/1575353


find_index for first occurrence of pattern in text This is an implementation of find_index for a pattern in text. I got asked this coding question in a coding interview.

This Python function returns the starting index of the first occurrence of pattern in text and returns None if no index is found. For example, find_index('ababc', 'abc') returns 2 and find_index('abc', 'z') returns None.

I have the following implementation that handles most of the cases and I think the worst case running time is \$O(m*n)\$.

def find_index(text, pattern):
    assert isinstance(text, str), 'text is not a string: {}'.format(text)
    assert isinstance(pattern, str), 'pattern is not a string: {}'.format(text)
    #if it's empty string, return 0
    if pattern == '':
        return 0
    text_index = 0
    while text_index != len(text):
        for i in range(len(pattern)):
            if text_index + i < len(text):
                if text[text_index + i] != pattern[i]:
                    break
                if i == len(pattern) - 1:
                    return text_index
        text_index += 1

I am not sure if I exhausted all of the edge cases, but I ran the following unit test, and it passed all tests.

import unittest

class StringsTest(unittest.TestCase):
   def test_find_index_with_matching_patterns(self):
        assert find_index('abc', '') == 0  # all strings contain empty string
        assert find_index('abc', 'a') == 0  # single letters are easy
        assert find_index('abc', 'b') == 1
        assert find_index('abc', 'c') == 2
        assert find_index('abc', 'ab') == 0  # multiple letters are harder
        assert find_index('abc', 'bc') == 1
        assert find_index('abc', 'abc') == 0  # all strings contain themselves
        assert find_index('aaa', 'a') == 0  # multiple occurrences
        assert find_index('aaa', 'aa') == 0  # overlapping pattern
        assert find_index('thisisabsolutelycrazy', 't') == 0

    def test_find_index_with_non_matching_patterns(self):
        # Negative test cases (counterexamples) with non-matching patterns
        assert find_index('abc', 'z') is None  # remember to test other letters
        assert find_index('abc', 'ac') is None  # important to test close cases
        assert find_index('abc', 'az') is None  # first letter, but not last
        assert find_index('abc', 'abz') is None  # first 2 letters, but not last

if __name__ == '__main__':
    unittest.main()

Python is a duck typed language, so it's not considered normal to assert instance types. Instead simply assume that the user passed a string, and let the program fail if the input can't be processed sensibly.

You calculate the length of text and pattern potentially many times during a run. Unless you plan on modifying them during the run you can save the results to a variable and reuse that.

You can get rid of if text_index + i < len(text) by changing your while statement to take into account the length of the pattern.

Apropos runtime: I'm purposefully not commenting on more advanced algorithms for text searching, mainly because I don't memorize things I have literally never had to use.


Read and reformat a list of global variables output by a compiler I'm currently going through a large Python program, with an eye to making it more readable and maintainable. Currently using Pylint and other code review tools throws a lot of unused variable warnings because of the way I've been using .format(). Since this program manipulates a lot of text, I use .format() quite a lot, which results in many spurious warnings. I could turn these warnings off, but I'd rather not because so far they've been useful in finding genuinely unused variables in my code.

Here's an example of the existing code style. (The program is very large so this is just the tip of the iceberg, selected to show the difference in style that I'm considering. Its purpose is to read and process compiler output, which will then be fed to another set of tools. This particular function is from a module which reads and reformats a list of global variables output by a compiler. The names of some other internal functions have been modified a bit to make it easier to read out of context.)

def parse_global_variable(line, length):
    last_element = length-1
    template = ''
    if length == 0:
        template = ' Record   GLB  {logical_address} {physical_address} {module:34}{var_name}' \
                   '\n                0    {width:7}{var_type}'
    else:
        template = ' Record   GLB  {logical_address} {physical_address} {module:34}{var_name}' \
                   '(0..{last_element})\n                0    {width:7}{var_type}'
    module = line.partition('.')[0].rpartition('\t')[2]
    logical_address = line.partition('#')[2].partition('#')[0]
    physical_address = get_physical_address(logical_address)
    var_name = get_var_name(line))
    width = line.rpartition('\t')[2]
    if length != 0:
        width = str(int(width)/length)
    return template.format(**locals())

This is a different style I'm considering, which inlines most of the variables, shortens variable names in the template string, and uses explicit arguments to .format() instead of using **locals():

def parse_global_variable(line, length):
    template = ''
    if length == 0:
        template = ' Record   GLB  {la} {pa} {m:34}{vn}' \
                   '\n                0    {w:7}{vt}'
    else:
        template = ' Record   GLB  {la} {pa} {m:34}{vn}(0..{le})' \
                   '\n                0    {w:7}{vt}'
    logical_address = line.partition('#')[2].partition('#')[0]
    width = line.rpartition('\t')[2]
    if length != 0:
        width = str(int(width)/length)
    return template.format(la=logical_address, pa=get_physical_address(logical_address),
        m=line.partition('.')[0].rpartition('\t')[2], vn=get_var_name(line), le=length-1, w=width)

Is it stylistically preferable, more readable, more Pythonic, etc. to use **locals() or explicit arguments? Is using abbreviated names such as vn or pa in the format string frowned upon when there are explicit arguments? And should variables which are only used in the format string be inlined?

Edit: Here is a sample input line:

Record      imsd_ini.ini            16#3200#        0       1424

The output that this module produces corresponding to that line is:

Record   GLB  3200 133200 imsd_ini                          ini
              0    1424   ini_type

The docs for locals() imply that there's some overhead involved with using it:

  Update and return a dictionary representing the current local symbol table. Free variables are returned by locals() when it is called in function blocks, but not in class blocks.

So it might be best to avoid it from that perspective alone.  (Though it's probably fine in most cases, and avoiding it might be a case of premature optimization.)

I generally prefer verbose format strings when using them this way, using the philosophy of "If the string was separated from the code that uses it, could someone still figure out what each variable in the string is referring to?".  So I frequently end up doing somewhat of a hybrid approach -- using a dictionary for the format arguments rather than locals() and rather than having to repeat every single argument as a keyword argument:

def parse_global_variable(line, length):
    last_element = length-1
    template = ''  # This is actually redundant

    f = {
        'module': line.partition('.')[0].rpartition('\t')[2],
        'logical_address': line.partition('#')[2].partition('#')[0],
        'var_name': get_var_name(line),
        'width': line.rpartition('\t')[2]
    }
    f['physical_address'] = get_physical_address(f['logical_address'])

    if length == 0:
        template = ' Record   GLB  {logical_address} {physical_address} {module:34}{var_name}' \
                   '\n                0    {width:7}{var_type}'
    else:
        template = ' Record   GLB  {logical_address} {physical_address} {module:34}{var_name}' \
                   '(0..{last_element})\n                0    {width:7}{var_type}'
        f['width'] = str(int(f['width'])/length)
    return template.format(**f)


Search for names in a hard-coded list of contacts I am currently taking a class at my local community college for java programming as well as trying to learn as much as I can on my own time. I would like your opinion on a program I wrote that would display any results matching a user inputted search term. It works, but I feel like it could be written better or even be made more effective, I'm not sure as I am still kind of new to Java. Could you please let me know if I am doing anything wrong like bad coding habits, doing things that are more easily done with better code, or suggest anything you think I should improve upon.

package controller;
import java.util.Scanner;

public class PhoneListDemo {

    public static void main(String[] args) {

        promptUser();

    }
    public static void promptUser() {

        Scanner keyboard = new Scanner(System.in);
        System.out.println("Enter a name or characters to search for: ");
        String userSearch = keyboard.nextLine();
        keyboard.close();
        searchAndDisplay(userSearch);
    }
    public static void searchAndDisplay(String userSearch) {

        String[] names = {"Harrison, Rose", "James, Jean", "Smith, William", "Smith, Brad"};
        String[] phone = {"555-2234", "555-9098", "555-1785", "555-9224"};

        for(int i = 0; i < names.length; i++) {

        if(names[i].toLowerCase().contains(userSearch.toLowerCase())) {

            System.out.println(names[i] + ":  " + phone[i]);

            }
        }
    }
}

Structure
It surprises me that promptUser() also performs the search.  Amongst other things, this makes it harder for you to test the prompting, and it makes it harder to re-use the function for something else.
I would expect main() to look like this:
public static void main(String[] args)
{
    String search_string
         = promptUser("Enter a name or characters to search for: ");
    searchAndDisplay(search_string);
}

We need to modify promptUser() to accept an argument and return a value:
public static String promptUser(String prompt) {
    Scanner keyboard = new Scanner(System.in);
    System.out.println(prompt);
    String userSearch = keyboard.nextLine();
    keyboard.close();
    return userSearch;
}

I hope you see how it has become more reusable.
Data structure
We would like to be able to separate the search and display parts of searchAndDisplay() (because one day, we'll want to search and update, or search and remove).  That's hampered at the moment because each record is scattered across two lists (and we have a weakness that the two lists need to be kept aligned with each other).
Instead, it will be easier to work with a single list of records.  Let's define a Record structure that looks something like:
public class Record
{
    String name;
    String phone;
}

Now, if we have a list of Record, we can have a search() function that returns a Record and a display() function that accepts a record and prints it.  I'll let you fill in the rest of Record - you'll want at least a constructor, and probably accessors and a bool matches(String substring).
String comparison
Comparing strings by converting both to lower case usually works for English names, but less well in other languages.  For example, it will prevent AMELIE from matching Amélie in French (where accents are not usually kept on upper-case words), and it will miss Groß when searching for GROSS.  There are a couple of Stack Overflow answers that address this (and probably better than my very rusty Java knowledge):

How to check if a String contains another String in a case insensitive manner in Java?
Case-insensitive storage and unicode compatibility

Beware that there are quite a few bad answers on those questions, mostly recommending toLower() or toUpper() as you have now.
You probably want something like
    Pattern search_re = Pattern.compile(userSearch, Pattern.CASE_INSENSITIVE | Pattern.LITERAL | Pattern.UNICODE_CASE);
    for (int i = 0;  i < names.length;  i++) {
        if (p.matcher(name).find()) {
            System.out.println(names[i] + ":  " + phone[i]);
        }
    }

(where Pattern is java.util.regex.Pattern - you'll want to import that).
Obviously you'll need to adapt that a little once you're storing Record objects rather than separated names and phones.


Multiplying 2 numbers without using * operator in Java I saw this interview question and decided to solve using recursion in Java.

  Write a multiply function that multiples 2 integers without using *

public class Main {

  public static void main(String[] args) {

    Scanner in = new Scanner(System.in);
    System.out.println("Enter first num: ");
    double num = in.nextDouble();
    System.out.println("Enter second num: ");
    double numTwo = in.nextDouble();

    System.out.println(multiply(num, numTwo));
  }

  private static double multiply(double x, double y) {

    if (x == 0 || y == 0) {
      return 0;
    } else if (y > 0) {
      return x + multiply(x, y - 1);
    } else if (y < 0) {
      return -multiply(x, -y);
    } else {
      return -1;
    }
  }
}

What should I return instead of -1 to make this clear?

The challenge only requires you to work with integers. This technique would not actually be able to multiply two floating-point numbers, so don't use double. A proper solution for integers would not have a failure node that requires you to return -1.

A better algorithm to use would be the Russian peasant method which can be done by bitwise operations and addition.


Castling move in Chess I have just finished building a Chess game in Java as an exercise. Whilst I am quite happy with the rest of the code, my castling move check code is lengthy and ugly and I want to see if there is a way to improve it.

There is a board class, which has a list of all pieces which are all unique classes extended from a single chessman class. This code is contained within the king class.

getOwner returns an enum which has a value of 0 or 1 depending on the player
checkPosition returns a boolean if there is a unit at the position or not
doMove is the normal form of moving pieces
getUnit retrieves a unit at a specified position
getPossibleMoves is every legal move a unit can make in the form of a point list

@Override
public boolean move(Point target){
    int owner = super.getOwner().getValue() * 2;
    List<Point> castlingMove = new ArrayList<>(Arrays.asList(new Point(3,1),new Point(7,1), new Point(3,8), new Point(7,8)));
    List<Point> rooks = new ArrayList<>(Arrays.asList(new Point(1,1), new Point(8,1), new Point(1,8), new Point(8,8)));
    Board b = super.getBoard();
    if (castlingMove.contains(target) && !this.moved) {
        boolean success = false;
        if (target.equals(castlingMove.get(owner + 1)) && b.checkPosition(rooks.get(owner + 1))){ // Castling Short
            success = checkRook(b, rooks, castlingMove, owner, 1);
        } else if(target.equals(castlingMove.get(owner)) && b.checkPosition(rooks.get(owner))){ // Castling Long
            success = checkRook(b, rooks, castlingMove, owner, 0);
        }
        if (success) this.moved = true;
        return success;
    } else {
        boolean success = this.doMove(target);
        if (success) this.moved = true;
        return success;
    }
}

private boolean checkRook(Board b, List<Point> rooks, List<Point> castlingMove, int owner, int moveType) {
    Chessman u = b.getUnit(rooks.get(owner + moveType));
    if (u instanceof Rook){
        Rook r = (Rook) u;
        if (!r.getMovedStatus()) {
            boolean noCheck = true;
            boolean freeSpace = true;
            for (Chessman unit : b.getAllUnits()){
                if (unit.getOwner() == super.getOwner()) continue;
                Point[] t = {(Point) super.getPosition().clone(), (Point) r.getPosition().clone()};
                int[] s = {5, 9};
                Point m = t[moveType];
                while (t[moveType].getX() < s[moveType]) {
                    if (unit.getPossibleMoves().contains(m)) noCheck = false;
                    m.translate(1, 0);
                    if (b.checkPosition(m) && m.x < s[moveType]) freeSpace = false;
                }
            }

            if (noCheck && freeSpace) {
                int[] p = {4,6};
                super.setPosition(castlingMove.get(owner + moveType));
                r.setPosition(new Point(p[moveType],super.getPosition().y));
                return true;
            }
        }
    }
    return false;
}

If anyone needs more code, just say so. I didn't include the whole project because it is a large amount of code.

I see three main issues: the improper usage of primitive constants, the bad naming of everything, and too long and complex methods. Case study: private boolean checkRook(Board b, List<Point> rooks, List<Point> castlingMove, int owner, int moveType).

The method name checkRook. If I call checkRook(...) and get "true" or "false", I have no idea what that actually means. Name the method so that "true" and "false" can be answers to the question. "checkRook"? "true." doesn't sound like a conversation. "rookCanMove?" "true." works much better (if that's what the method is doing, I'm still not sure...). In the context of a chess game the word "check" also has a special meaning, so you should be really, really careful to only use it correctly. "checkRook" makes no sense in that respect, either (unless it means "a rook giving a check to the opposing king").
The variable should be Board board, not Board b. It's not 1970s, you don't need to save bytes. "board" is much more readable than "b", and readability should be the king.
int owner and int moveType. These should be enums: Owner owner and MoveType moveType. For an integer owner, one has no idea what value is white and what value is black, but Owner.WHITE and Owner.BLACK are self-explanatory. Similarly, "moveType 5" says nothing to anyone -- compare with MoveType.CHECK and MoveType.EN_PASSANT.
Rather than giving the method both the Board and a list of rooks, consider implementing a new method into the Board class, something like Point[] getPiecesByType(PieceType.WHITE_ROOK). The board should hold references to its pieces, so you should be able to get their positions from it. This helps you remove one of those method parameters. Anything more than three is usually too much anyway.

Point[] t = {(Point) super.getPosition().clone(), (Point) r.getPosition().clone()};
int[] s = {5, 9};
// ...
if (b.checkPosition(m) && m.x < s[moveType])

Again, horrible naming. These cannot be read out of context, nor without scrolling back and forth, looking for the variable initializations. Actually, many of those are incomprehensible even when you see the definition. s, seriously? The t is a list of two points, so apparently a move from Point A to Point B: this should be encapsulated into a new class, called, for example, Move.

Furthermore, assuming the method checkRook checks something about the rooks in relation to the castling, I don't know how you managed to make the method so long. The only thing you need to check about rooks is that they haven't moved. Then you should have other methods for checking that your king is not in check and that it's not going to move over or onto a square that is in check.

Overly long methods like checkRook violate the well known principle that is formalized in Clean Code as follows:

  FUNCTIONS SHOULD DO ONE THING. THEY SHOULD DO IT WELL. THEY SHOULD DO IT ONLY.

Uncle Bob continues:

  We should also be able to write a brief description of the class in about 25 words, without using the words “if,” “and,” “or,” or “but.”

and that's obviously true for methods too. So break your methods into smaller ones that have descriptive names.


Bulk slack emoji downloader NOTE
The script relies on ruamel-yaml and requests packages
available on pypi.

I wrote a script which goes over the pack file details as specified by the emojipacks project, and downloads (and saves) to the local drive. It downloads the files as threads, and saves to disk with the shutil's copyfileobj method.
Feel free to nitpick each and everything which could be improved.
from argparse import ArgumentParser
from pathlib import Path
from shutil import copyfileobj
from threading import Thread

import requests
import ruamel.yaml as YAML

yaml = YAML.YAML(typ="safe")
DOWNLOAD_DIR: str = "emojis"

def download_file(url: str, filename: str, parent: Path):
    """
    Downloads the file from `url` and saves to `filename` inside the `parent` directory.
    """
    suffix = url.split(".")[-1]
    if suffix in ("png", "jpg", "gif", "jpeg"):
        filename = f"{filename}.{suffix}"
    path = Path(parent / filename)
    with requests.get(url, stream=True) as response:
        with path.open(mode="wb") as f:
            copyfileobj(response.raw, f)

def create_download_thread(url: str, name: str, path: Path) -> Thread:
    thread = Thread(target=download_file, args=(url, name, path))
    thread.start()
    return thread

def main(dir_path: str, download_dir: str):
    path = Path(dir_path)
    threads = []
    if not path.is_dir():
        return
    for yaml_file in path.glob("*.yaml"):
        data = yaml.load(yaml_file)
        emoji_dir = Path(download_dir) / data["title"]
        emoji_dir.mkdir(parents=True, exist_ok=True)
        for emoji in data["emojis"]:
            names = [emoji.get("name"), *emoji.get("aliases", [])]
            src = emoji.get("src")
            threads += [create_download_thread(src, name, emoji_dir) for name in names]
    for thread in threads:
        thread.join()

if __name__ == "__main__":
    parser = ArgumentParser(description="Download emojipacks to specified directory!")
    parser.add_argument(
        "directory",
        nargs="+",
        help="Directory(ies) with yaml for the emojipack specifications.",
    )
    parser.add_argument(
        "-p",
        "--download-path",
        help="Directory where emojis will be downloaded.",
        default=DOWNLOAD_DIR,
    )
    args = parser.parse_args()
    for path in args.directory:
        main(path, args.download_path)

The script was created because the project emojipacks itself is no longer maintained and the authentication system has been updated from slack. The downloaded emojis are then bulk uploaded to slack using another utility slack-emojinator.

EDIT
Since this point was raised in the answer(s), the yaml specification is:

Also note that the yaml file must be indented properly and formatted
as such:
title: food emojis:
  - name: apple
    src: http://i.imgur.com/Rw0Vlda.png
  - name: applepie
    src: http://i.imgur.com/g4RU1fM.png

..with the src pointing to an image file. According to Slack:
[...]
It is possible to give multiple names to a single emoji using yaml
such as:
title: octicons emojis:
  - name: pr
    aliases:
      - pullrequest
      - mergerequest
    src: https://i.imgur.com/rhwNxfc.png

emphasis mine

Parametrization
DOWNLOAD_DIR is being used as a default for a command-line argument. This is surprising, since at first glance one would assume that this global constant is simply being used as the download directory value itself. It's less clutter and more obvious if you simply remove this global and write the string literal beside the default kwarg.
More pathlib
You're already using it, but you can benefit from it more. Rather than making a Path() here:
path = Path(parent / filename)

Use urllib.parse.urlparse to get you a ParseResult with a path component, since it's a more thorough and safe parsing method than bare split and you only care about path

Call path.rsplit('/', 1)[-1] - not split - on the path component to get the filename-like

Make a Path from the result

If you deeply care about image-likes, then check mypath.suffix for .png, etc.

Use / as you are now.

Set membership
if suffix in ("png", "jpg", "gif", "jpeg"):

should use a set for speed, and also because it more closely matches your meaning. Also, pathlib will need leading dots:
if suffix in {'.png', '.jpg', '.gif', '.jpeg'}:

Noisy failure
if not path.is_dir():
    return

does not help the user understand why the script quit with no message. Raise some kind of error here.
Fail-safe list construction
The first part of this expression:
[emoji.get("name"), *emoji.get("aliases", [])]

will not do what you want. If there is no 'name', a None will be inserted rather than nothing. A more "naive" approach that will actually work is:
names = emoji.get('aliases', [])
name = emoji.get('name')
if name is not None:
    names.append(name)

Order probably doesn't matter to you since you had been kicking off threads all at once.
Somewhat-related: is this -
src = emoji.get("src")

actually optional? I doubt it. Currently you require that it be an actual URL. Either use [] if you want this to keep being fatal, or:
src = emoji.get("src")
if src is not None:
    create_download_thread ...

List append of a comprehension
        threads += [create_download_thread(src, name, emoji_dir) for name in names]

creates a temporary list only to throw it away again once it's been appended. The following will use the generator but not materialize it to a temporary list:
threads.extend(create_download_thread(src, name, emoji_dir) for name in names)


Regex to match several slightly different strings I work with a charity that does a number of jobs, more than 100, less than 1000 a year. Jobs are identified with a year, and an up to 3 digit serial number, so 2021#123 for instance.
I'm writing a Python report program to pull jobs out of our database, and I'd like be be able to specify ranges of jobs rather like a print dialogue can specify ranges of pages, so a dash separated range 2021#123-45, or a comma separated list 2021#003,5,07,89,131 as well. I want to be able to parse all of these representations out of text strings.
I'm only interested in the syntax at the moment, the semantics of whether a range is valid is handled later. Even though I have very limited experience of regexes, I felt a one-liner would beat a whole bunch of python ifs.
The valid strings I want to match are therefore of the form

20y#n          OR
20y#n,q,p,r    OR
20y#n-m

where y is any two digits, and m,n,p,q,r are 1 to 3 digits.
This is a question about the regex. The only python consideration is the use of r'…' for a raw string.
I struggled and failed to write a regex that captures any of these forms by trying to match the year#ddd, and then OR the different endings. I think I fell foul of the single job form not having an ending at all. I've therefore ended up with this rather agricultural effort, that works, but looks ugly. I didn't put the comments in just for this post; I put them in knowing that if I needed to change things next month, I'd need them.
job_patt = r'((?:20\d\d#\d{1,3}-\d{1,3})|(?:20\d\d#\d{1,3}(?:,\d{1,3})+)|(?:20\d\d#\d{1,3}))'
# cap group  (                                                                             )
# non-caps    (?:                      ) (?:                           ) (?:              )
# OR the non-caps, in this L->R order   |                               |
# get year#      20\d\d#                    20\d\d#                         20\d\d#
# get 1 to 3 digits     \d{1,3}                    \d{1,3}                         \d{1,3}
# get one instance of range    -\d{1,3}
# get one or more repeats of list         ',ddd'          (?:,\d{1,3})+

What could be done to shorten or tidy up this regex?

This isn't so much a Python question as it is a RegEx question.  The Python tag is almost irrelevant.
'+' -vs- '*'
You regex includes two alternates:

(?:20\d\d#\d{1,3}(?:,\d{1,3})+)
(?:20\d\d#\d{1,3})

The difference in these expressions is the first one include one or more repeats of ,\d{1,3}, where as the second does not include any repeats of that ...
... in other words, you have 20\d\d#\d{1,3} followed by 0 or more repeats of ,\d{1,3}.
So we can replace the + modifier with the * modifier, and the pair of alternates becomes simply
(?:20\d\d#\d{1,3}(?:,\d{1,3})*)
Common prefix
All three (or rather, both now that the last two have been combined into one) regex's start with 20\d\d#.  You can factor this out of both patterns.
20\d\d# (?: (?: \d{1,3}-\d{1,3} | \d{1,3}(?:,\d{1,3})* ) )
Capture Groups
There is no point creating a capture group for the entire regex expression.  The entire match is always captured, as group 0.
Reworked Regex & Example
The regex can be rewritten as:
r"20\d\d#(?:(?:\d{1,3}-\d{1,3}|\d{1,3}(?:,\d{1,3})*))"
which is shorter, but is still quite unreadable.  It is better for future programmers at the charity (as well as for yourself) to document the regex using the re.VERBOSE option.  I find it clearer than comments below the regular expression that try to document the parts of the regular expression through vertically lined up notes.
Note: Since comments follow the octothorpe (#) character in re.VERBOSE mode, we need to escape it (\#) to indicate that it is actually a character we want matched, and not the start of a comment.
import re

JOB_PATTERN = re.compile(r"""
                          20\d\d                     # A 4-digit year
                          \#                         # followed by a hash mark
                          (?:                        # followed by either ...
                             (?:\d{1,3}-\d{1,3})        # a range of serial #'s
                             |                          # or
                             (?:\d{1,3}(?:,\d{1,3})*)   # a list of serial #'s
                          )""", re.VERBOSE)

test_cases = ["junk2021#123-45bar",
              "baz2021#003,5,07,89,131foo"]

for test_case in test_cases:
    m = JOB_PATTERN.search(test_case)
    if m:
        print("Found:", m[0])

Output:
Found: 2021#123-45
Found: 2021#003,5,07,89,131

Update
Based on the OP's response to RootTwo's comment, named capturing groups are actually desired.  Here is an updated example with RootTwo's enhancement:
import re

JOB_PATTERN = re.compile(
    r"""(?P<year>              # Capturedas "year" ...
           20\d\d                 # a 4-digit year
        )
        \#                     # followed by a hash mark
        (?:                    # followed by either ...
           (?P<range>             # captured as "range" ...
              \d{1,3}-\d{1,3}        # a range of serial #'s
           )
           |                      # or
           (?P<list>              # captured as "list" ...
               \d{1,3}               # a serial number
               (?:,\d{1,3})*         # or list of comma-seperated serial #'s
           )
        )
        """, re.VERBOSE)

test_cases = ["junk2021#123-45bar",
              "baz2021#003,5,07,89,131foo"]

for test_case in test_cases:
    m = JOB_PATTERN.search(test_case)
    if m:
        if m.lastgroup == "range":
            start, end = m['range'].split('-')
            print("Year:", m['year'], " from", start, "to", end)
        else:
            serial_numbers = m['list'].split(',')
            print("Year:", m['year'], " serial #':", serial_numbers)

Output:
Year: 2021  from 123 to 45
Year: 2021  serial #': ['003', '5', '07', '89', '131']


Placing a Google map in Colorbox I need to place a Google map in Colorbox overlay and am wondering if there's a better way to do this.  I've just started out with Google's tutorial so there's nothing fancy going on.

Right now I'm hiding the div that Google loads the map into:

Here's the call to Colorbox:

var latlng = new google.maps.LatLng(-34.397, 150.644);
var myOptions = { zoom: 8, center: latlng, mapTypeId: google.maps.MapTypeId.ROADMAP };
var map = new google.maps.Map(document.getElementById("map_canvas"), myOptions);

$(document).ready(function(){

    $("#farm_on_google_maps").colorbox({
        inline: true,
        width:700,
        height:450,
        href : '#map_canvas'
        });
});

Even though it works, I wonder if the map could be loaded directly into Colorbox.

If you want to dynamically insert the Google Map, instead of simply linking to an href, you can do as follows:

In your HTML, have the basic outline for the colorbox ready:

Then, when you invoke your colorbox, invoke it with an html option:

$("#farm_on_google_maps").colorbox({
            inline: true,
            width:700,
            height:450,
            html : $("#box").html()
            });

This will setup your colorbox with the basic HTML structure. We then want to insert the google map dynamically into the colorbox as follows:

var googleMap = $("#cboxLoadedContent").find("div.google-map")[0];
var latlng = new google.maps.LatLng(-34.397, 150.644);
var myOptions = { zoom: 8, center: latlng, mapTypeId: google.maps.MapTypeId.ROADMAP };
var map = new google.maps.Map(googleMap, myOptions);

The cboxLoadedContent div is created by the colorbox when it's invoked, so it's simply a matter of grabbing that, and using it to setup the Google Map.


Validate fields in Python dictionary using dictionary of lambdas I would love some feedback on this.  I use it to validate web-form objects before persisting them in the dangerously-lazy MongoDb.  I have a class and many validators for each of my collections.  Is there a better way to write or go about this?

class ValidationError(Exception):
    pass

class User(object):

    @staticmethod
    def validate(data):

        def _id(val):
            if not isinstance(val, ObjectId):
                raise ValidationError('not ObjectId')
            else:
                return val

        map_keys = dict(
            _id=lambda x: _id(x)
        )

        def count(val):
            try:
                return int(val)
            except TypeError:
                raise ValidationError('%s not integer' % val)

        new_data = {}

        for key in data:
            try:
                new_data[key] = map_keys[key](data[key])
            except KeyError:
                raise ValidationError('Key not in User: "%s"' % key)
            except ValidationError, error:
                print key, error
                raise
        return new_data

lambda is redundant here as you can use _id directly as well:

    map_keys = dict(
        _id=_id
    )

Other than that, your approach seems ok. You are reinventing the wheel though and it might be a good idea to research existing libraries.


Upper section of a vertical single-page website This is my very first attempt to write HTML and CSS. I designed this website for a friend. I'm not sure if I used the best practices or maybe I did something wrong but I just started my learning process so I would love a review to point bad practices and things I could have done better or I did wrong.

This is meant to be the first section (above the fold) of a vertical single-page website.

* {
  font-family: Open Sans;
  margin: 0px
}

#PageHeader {
  background-color: rgb(236, 100, 62);
  height: 615px;
}
/* logo and nav look aligned on the original file :c */
#logo {
  position: relative;
  margin-left: 40px;
  top: 30px;
}

a.nav:link {
  text-decoration: none;
  font-size: 12px;
  font-weight: bold;
  color: white;
  text-transform: uppercase;
  margin-right: 40px;
  float: right;
}

a.nav:hover{
  border-width: 0px 0px 3px 0px;
  border-style: solid;
}

#heading{
  color: white;
  text-align: center;
  line-height: 1.2;
  margin-top: 150px;
}

h1{
  font-size: 60px;
  margin-bottom: 30px;
}

    <title>Empreendi na Rede</title>
    <meta charset="utf-8">
    <meta name="author" content="Luciano Infanti">

    <link href="https://fonts.googleapis.com/css?family=Open+Sans:400,400i,800" rel="stylesheet">

        Solucoes
        Metodologia
        Sobre nos
        Depoimentos
        Clientes
        Contato

A consultoria empresarial
 acaba de ser redefinida

Aliquam et placerat lacus. Donec pharetra nibh a placerat tincidunt.

  Integer viverra nisl dolor, in viverra enim sollicitudin nec.

HTML validation

Always include a doctype declaration.  These days, HTML 5 is recommended, so the document should start with <!DOCTYPE html>.

The
 is unclosed.  That would likely explain your vertical alignment issues.

The <meta charset="utf-8"> declaration should occur as early as possible, preferably immediately after the opening  tag, because the charset affects how all text in the document is interpreted.  It would be better not to make the browser re-parse the <title>.

The  tag must have an alt attribute.  The omission of the alt attribute is unacceptable here, especially since the image contains text.  It is odd that the logo has a height attribute but no width.

HTML semantics

The
 inside the
 is not appropriate, since it is there for styling.  I would go with a width rule in CSS, using em as the measurement unit so that it scales with the font size.

Writing class="nav" in every navigation link, though harmless, seems excessive.  These elements could be selected in CSS using nav > a instead.

It's unclear which of the  links, if any, takes you back to the "home" page.

CSS

The * selector is excessive.  If you want to set a font for the page, a body rule should suffice.

In #PageHeader, you set a background-color.  It would be a good idea to also set the color at the same time.  A comment saying /* white text on orange */ would be nice.  You should specify color: white on the links too, in such a way that applies even to visited links.

When you float each nav link to the right, the visual order ends up backwards relative to the DOM order.  I would rather float the entire .  It would be a good idea to specify a margin-left on the nav so that it will not overlap the logo if the page is too narrow.

body {
  font-family: Open Sans;
  margin: 0px
}

#PageHeader {
  background-color: rgb(236, 100, 62);
  color: white;
  height: 615px;
}
/* logo and nav look aligned on the original file :c */
#logo {
  position: relative;
  margin-left: 40px;
  top: 30px;
}

nav {
  float: right;
  margin-left: 200px;
}

nav > a {
  text-decoration: none;
  font-size: 12px;
  font-weight: bold;
  text-transform: uppercase;
  margin-right: 40px;
  color: white;
}

nav > a:hover {
  border-width: 0px 0px 3px 0px;
  border-style: solid;
}

#heading {
  color: white;
  text-align: center;
  line-height: 1.2;
  margin-top: 150px;
}

#heading h1 {
  font-size: 60px;
  margin-bottom: 30px;
  margin-left: auto;
  margin-right: auto;
  width: 15em;
}

    <title>Empreendi na Rede</title>
    <meta charset="utf-8">
    <meta name="author" content="Luciano Infanti">

    <link href="https://fonts.googleapis.com/css?family=Open+Sans:400,400i,800" rel="stylesheet">

        Solucoes
        Metodologia
        Sobre nos
        Depoimentos
        Clientes
        Contato

A consultoria empresarial acaba de ser redefinida

Aliquam et placerat lacus. Donec pharetra nibh a placerat tincidunt.

  Integer viverra nisl dolor, in viverra enim sollicitudin nec.


How to efficiently (fast) calculate the transpose of a *permutation matrix* in Python3? The permutation matrix is represented as a list of positive integers, plus zero. The number indicates the position of the 1 in that row, e.g. a number zero would mean that the 1 is in the right-most position².

My first attempt is as follows, together with a printing function to help assess the result.
It seems too convoluted to me.
Please note that a permutation matrix multiplication is simpler than an ordinary matrix multiplication.

def transpose(m):
    """Transpose a permutation matrix.
    m: list of positive integers and zero."""
    c = {}
    idx = 0
    for row in reversed(m):
        c[-row] = idx
        idx += 1

    return list(map(
        itemgetter(1), sorted(c.items(), reverse=False, key=itemgetter(0))
    ))

def print_binmatrix(m):
    """Print a permutation matrix 7x7.
    m: list of positive integers and zero."""

    for row in m:
        print(format(2 ** row, '07b'), row)

# Usage example with a matrix 35x35. Ideally, it should scale up without sacrificing speed at smaller matrices like this.
transpose([8, 4, 21, 17, 30, 28, 1, 27, 5, 3, 16, 12, 11, 14, 20, 6, 33, 19, 22, 25, 31, 15, 13, 18, 10, 0, 7, 2, 9, 23, 24, 26, 29, 32, 34])
# Result
[25, 6, 27, 9, 1, 8, 15, 26, 0, 28, 24, 12, 11, 22, 13, 21, 10, 3, 23, 17, 14, 2, 18, 29, 30, 19, 31, 7, 5, 32, 4, 20, 33, 16, 34]

[2]: This way, the "zero matrix" is the one with 1's in the secondary diagonal, since only one 1 is allowed per column.

Perhaps only marginally, but the readability of your code can be improved by using enumerate and the reverse=True flag in sorted.

def transpose_r(ls):
    return [len(ls)-1-k
            for k, Pk in sorted(enumerate(ls),
                                reverse=True,
                                key=itemgetter(1))]

If we index starting from the left, then this is further simplified.

def transpose_l(ls):
    return [x for x, Px in sorted(enumerate(ls), key=itemgetter(1))]

The transpose has the funny property that transpose(ls)[ls[j]] == j. We can use this to build the transpose without sorting.

def transpose_l(ls):
    tr_ls = [0]*len(ls)

    for l in ls:
        tr_ls[ls[l]] = l

    return tr_ls

def transpose_r(ls):
    n = len(ls)
    tr_ls = [0]*n

    for l in ls:
        tr_ls[n - 1 - ls[l]] = n - 1 - l

    return tr_ls

Alternatively, we can use enumerate again.

def transpose_l(ls):
    tr_ls = [0]*len(ls)

    for ix, l in enumerate(ls):
        tr_ls[l] = ix

    return tr_ls

def transpose_r(ls):
    n = len(ls)
    tr_ls = [0]*n

    for ix, l in enumerate(ls):
        tr_ls[n - 1 - l] = n - 1 - ix

    return tr_ls


SQL query where id=… or id=… or id=… etc I haven't had much experience with postgresql (none) and I am wondering/hoping that there is a better way for me to do this query.

SELECT * FROM member_copy WHERE id = 17579 OR id = 17580 OR id = 17582 ect.

There are about 800 where clauses in total so this will take a while and I need to run it on a fairly regular basis.

The way to write that query is:

SELECT * FROM member_copy WHERE id IN (17579, 17580, 17582);

However, the real question is, where did that list of ids come from?  If the list of ids is the result of another database query, then you should be doing either a subselect or a join instead.


Checking if any competitors have a non-empty name Take this:

has_competitors:function(){
  var c = false;
  _.each(this.competitors, function(obj, index){ // lodash here
    if(obj.name && obj.name.trim() !== ''){
      c = true;
      return false; // to break the loop
    }
  });
  return c;
}

I don't like the form of this, but till now I didn't find a more concise way. Could you suggest a better way? :) I mean, less lines.

Since ES5 (which all browsers support nowadays) is array.some(). It works in the same way as Lodash's/Underscore's _.some() but without the library.
If you target ES6, you can further trim your code using shorthand object methods and arrow functions.
Your object.trim() can blow up if object.name is truthy but not a string. Do a typeof obj.name === 'string' instead of a truthy check. If it is always guaranteed to be a string, the truthy check is redundant since a trimmed empty string will always be empty.

So your code could be as simple as:

has_competitors(){
  return this.competitors.some(obj => obj.name.trim() !== '')
}


Removeclass addclass multiple selectors optimization I've written a function that works fine, but being new to jQuery, I'd like some reviews on writing this more cleanly. Anything advice would help; just trying to learn!

function displayContent() {
  var $link1 = $('.row.nav li a.bio');
  var $link2 = $('.row.nav li a.stylist');
  var $link3 = $('.row.nav li a.contact');
  var $content = $('#text-content')
  var $bio = $("#bio");
  var $stylist = $("#stylist");
  var $contact = $("#contact");
  var $overlay = $('.content-overlay');

  //link1
  $link1.click(function (e) {
    e.stopPropagation();
    $link2.removeClass('active');
    $link3.removeClass('active');
    $link1.addClass('active');
    $contact.hide();
    $stylist.hide();
    $bio.fadeIn(700);
    $overlay.show();
  });
  //link2
  $link2.click(function (e) {
   //same code here
  });
  //link3
  $link3.click(function (e) {
    //same code here
  });

  //close overlay/hide content
  $('html').click(function (e) {
//hide code
  });

}

Using $(this) is very helpful in situations where you have similar/same code.  The way it works, is if say you have a class .general , which 50 different div's on the page all have that class. But lets say, if a user clicks on any one of those 50 div's , you want ONLY that div that was clicked on to have a border of 5px solid red. So instead of writing

$('.general').click(function() {
    $('.general').css({border: '5px solid red'});
});

which would give all 50 div's a border, you can write this

$('.general').click(function() {
    $(this).css({border: '5px solid red'});
});

and now only the div that was clicked on will have the border. Also, it will work more than once, so if you then clicked on 10 other div's with class general, they would all get the border red as well.

This can be applied to your code like so : (only included the relevant info)

function displayContent() {

  var $links = $('.row.nav li a.bio, .row.nav li a.stylist, .row.nav li a.contact');

}

  $links.click(function (e) {  // so you just need one click function instead of 3
    e.stopPropagation();
    $links.removeClass('active');  // removes the class active from all $links
    $(this).addClass('active');  // gives the class active to the one clicked on

  });


Format 2 Excel sheets then save both sheets as a new workbook I'm trying to optimize some VBA that I put together. I know that using  Select a lot slows down performance quite a bit so I've been trying to cut that out where I can. Is there anything else in my code that I can change or do differently to improve performance? Thank you in advance!

Sub PP_Formatting_Saving()

Application.ScreenUpdating = False

'Ungroups sheets by selecting Wksht 2
Worksheets(2).Select

'Input 1 into Cell AK1 then Multiply columns to convert to number
Range("AK1").Value = "1"
Range("AK1").Copy

Range("N" & Rows.Count).End(xlUp).Select
Range(Selection, Selection.End(xlUp)).Select
Selection.PasteSpecial Paste:=xlPasteAll, Operation:=xlMultiply, _
SkipBlanks:=False, Transpose:=False

Range("AA" & Rows.Count).End(xlUp).Select
Range(Selection, Selection.End(xlUp)).Select
Selection.PasteSpecial Paste:=xlPasteAll, Operation:=xlMultiply, _
SkipBlanks:=False, Transpose:=False

Range("G" & Rows.Count).End(xlUp).Select
Range(Selection, Selection.End(xlUp)).Select
Selection.PasteSpecial Paste:=xlPasteAll, Operation:=xlMultiply, _
SkipBlanks:=False, Transpose:=False

Range("AK1").ClearContents

'Format Amount as Accounting
Range("J" & Rows.Count).End(xlUp).Select
Range(Selection, Selection.End(xlUp)).Select
Selection.Style = "Comma"

'Format header color & bold
Range("A1").Select
Range(Selection, Selection.End(xlToRight)).Select
With Selection.Interior
    .Pattern = xlSolid
    .PatternColorIndex = xlAutomatic
    .Color = 12632256
    .TintAndShade = 0
    .PatternTintAndShade = 0
End With
Selection.Font.Bold = True

'Autofit all columns
Cells.Select
Cells.EntireColumn.AutoFit
Columns("O:O").ColumnWidth = 40

'Filter all headers
Selection.AutoFilter

'Unwrap Text in sheet 1
Worksheets(1).Select
Cells.Select
With Selection
    .WrapText = False
End With

'Format Amount as Accounting
Range("C" & Rows.Count).End(xlUp).Select
Range(Selection, Selection.End(xlUp)).Select
Selection.Style = "Comma"

'Input 1 into Cell AK then Multiply columns to convert to number
Range("AK1").Value = "1"
Range("AK1").Copy
Range("B" & Rows.Count).End(xlUp).Select
Range(Selection, Selection.End(xlUp)).Select
Selection.PasteSpecial Paste:=xlPasteAll, Operation:=xlMultiply, _
SkipBlanks:=False, Transpose:=False

'Autofit all columns
Cells.Select
Cells.EntireColumn.AutoFit

'Format header color & bold
Range("A1").Select
Range(Selection, Selection.End(xlToRight)).Select
With Selection.Interior
    .Pattern = xlSolid
    .PatternColorIndex = xlAutomatic
    .Color = 12632256
    .TintAndShade = 0
    .PatternTintAndShade = 0
End With
Selection.Font.Bold = True

'Filter all headers
Selection.AutoFilter

Dim ws As Worksheet
Dim wbNew As Workbook

'Select PP sheet, filter all cells on Variable1 in Column 31.          Copy all of that then paste in new sheet, rename sheet as PP1.      UsedRange selects all cells with value in it
Sheets("PP").Select
ActiveSheet.UsedRange.AutoFilter Field:=31, Criteria1:= _
    "Variable1"
Cells.Select
Range("Y1005").Activate
Selection.Copy
Sheets.Add After:=ActiveSheet
ActiveSheet.Paste
ActiveSheet.Name = "PP1"

'Select BK sheet, filter all cells on Variable2 in Column 16.  Copy all of that then paste to new sheet, rename as BK1.  UsedRange selects all values in worksheet.
Sheets("Bk").Select
ActiveSheet.UsedRange.AutoFilter Field:=16, Criteria1:= _
    "Variable2"
Cells.Select
Range("O15").Activate
Application.CutCopyMode = False
Selection.Copy
Sheets.Add After:=ActiveSheet
ActiveSheet.Paste
ActiveSheet.Name = "BK1"

'Copy BK & PP in Array then save as new workbook
Worksheets(Array("BK", "PP")).Copy
Set wbNew = ActiveWorkbook

'Naming the workbook w/ date format
wbNam = "BK_PP_"
dt = Format(DateAdd("m", -1, Now), "yyyy_mm")

'Save location
ActiveWorkbook.SaveAs "\\user\location\" & wbNam & dt & ".xlsx"

'Close new workbook that just saved
ActiveWorkbook.Close

'Delete sheets created in original workbook
Sheets("PP1").Delete
Sheets("BK1").Delete

End sub

Most times you have Range.Select followed by Selection.Something you can remove the Select and Selection to leave it Range.Something. This helps clarify what the code is doing. The same may be true of Worksheet.Select (or Activate). But doing this with a worksheet will alter any unqualified Range variables in your code that follows.

Range without VariableName.Range refer to the ActiveSheet and can lead to issues if you're not aware of this. My first step was to go through and everything after Worksheets(2).Select to fully qualify it with

Dim secondSheet As Worksheet
Set secondSheet = Worksheets(2)

Range("AK1").Value = "1" then became secondSheet.Range("AK1").Value = "1" giving me a lot more confidence that removing the .Select shouldn't alter the code. I did the same with Worksheets(1).Select as well. After that I went after the Range.Selects.

Range("N" & Rows.Count).End(xlUp).Select
Range(Selection, Selection.End(xlUp)).Select
Selection.PasteSpecial Paste:=xlPasteAll, Operation:=xlMultiply, _
SkipBlanks:=False, Transpose:=False

I took the above Select and Selection to be finding where the last cell on a specific column. Range("N" & Rows.Count).End(xlUp).Select became what follows. My choice of variable names are self describing, helping you immediately understand what the code is there for.

Dim lastCell as range
set lastCell = secondSheet.Range("N" & Rows.Count).End(xlUp)

Going up from this last cell to get a contiguous range Range(Selection, Selection.End(xlUp)).Select became

Dim modifyRange as Range
Set modifyRange = secondSheet.Range(lastCell, lastCell.End(xlUp))

Lastly replace the Selection in Selection.PasteSpecial Paste:=xlPasteAll, Operation:=xlMultiply, SkipBlanks:=False, Transpose:=False with modifyRange and it's looking a lot better. But this is being done in 3 places on the secondSheet and once on firstSheet. Rather than having duplicated code, create a Sub procedure ConvertToNumeric that takes in 2 parameters ByVal sheetToUse As Worksheet and ByVal columnToFindLastCellOn As String. The names let you know when typing the name ConvertToNumeric<space> and getting intellisense to show you the names of the arguments you'll supply. Any future changes are made in this single location now be reflected wherever it's called at ensuring the update is correct.

Private Sub ConvertToNumeric(ByVal sheetToUse As Worksheet, ByVal columnToFindLastCellOn As String)
    Dim helperCell As Range
    Set helperCell = sheetToUse.Range("AK1")
    helperCell.value = "1"
    helperCell.Copy

    Dim lastCell As Range
    Set lastCell = sheetToUse.Cells(Rows.Count, columnToFindLastCellOn).End(xlUp)

    Dim modifyRange As Range
    Set modifyRange = sheetToUse.Range(lastCell, lastCell.End(xlUp))
    modifyRange.PasteSpecial Paste:=xlPasteAll, Operation:=xlMultiply, SkipBlanks:=False, Transpose:=False

    helperCell.ClearContents
End Sub

You have a few places with comments like 'Format Amount as Accounting that are describing what you're doing. When this occurs it's pretty safe to make that chunk of code be it's own Sub or Function. Sub if it doesn't return a value or Function if it does.

Using the same idea as above Range("J" & Rows.Count).End(xlUp).Select became a Dim lastCell As Range variable and the Range(Selection, Selection.End(xlUp)).Select became Dim modifyRange As Range. You get

Private Sub FormatAsAccounting(ByVal sheetToUse As Worksheet, ByVal columnToApplyFormatTo As String)
    Dim lastCell As Range
    Set lastCell = sheetToUse.Cells(Rows.Count, columnToApplyFormatTo).End(xlUp)

    Dim modifyRange As Range
    Set modifyRange = sheetToUse.Range(lastCell, lastCell.End(xlUp))
    modifyRange.Style = "Comma"
End Sub

It's called in code FormatAsAccounting secondSheet, "J" where secondSheet is a worksheet variable and columnToApplyFormatTo is a string that tells you what column you'll be using. Admittedly the modifyRange variable isn't actually needed and sheetToUse.Range(lastCell, lastCell.End(xlUp)).Style ... could be used but serves to illustrate how as code becomes simpler many times it helps clean itself.

The same thing occurred with headers as well.

When you get to the saving a workbook that has copies of your worksheets you have variables Dim ws As Worksheet that's never used and Dim wbNew As Workbook that's assigned but then never used. There is also dt and wbNam, neither of which are declared. In the VBE coding window go to Tools>Options>Require Variable Declaration. This will put Option Explicit into any new module and requires you to Dim every single variable. It takes more thought and effort to code, but doing something well generally takes effort. Future-you will thank current-you for doing so because it'll save you from many needless issues in your code. BTW Debug>Compile VBAProject (Alt+D+L for keyboard shortcut) is your friend. That'll tell the compiler to compile the code and if something isn't correct it'll alert you by taking you straight there. Keep doing that until there's no more compile-time problems.

Back to the code.

You're selecting a sheet, filtering the sheet, selecting all cells on the sheet to copy that information to a sheet that was just added. This is done for the sheet "PP" and "BK". Those copied sheets are then copied to a new workbook only to be deleted in your current workbook. You can avoid this by creating a new workbook and using that variable.

Dim saveBook As Workbook
Set saveBook = Workbooks.Add

Putting that code into a Sub and having your cursor on the word Add if you press Ctrl+i or Edit>Quick Info you should see Add([Template]) As Workbook appear. This indicates the Add function on the Workbooks collection returns a Workbook object, which coincidentally is what saveBook has been dimensioned as. Right clicking on Add and choosing Definition (3rd from bottom) will take you to the object browser where you can see at the bottom pane what I just described. This gives you a new workbook to use that you'll eventually save where you want. The Object Browser is another topic, but learning to use it will help tremendously.

You can directly copy a sheet by using the Copy procedure on a worksheet object and tell it where to be copied to with either the Before or After argument.

Dim ppSheet As Worksheet
Set ppSheet = Sheets("pp")
ppSheet.Copy Before:=saveBook.Worksheets(1)

As part of your eventual file name dt isn't descriptive. I used monthPriorDate since that's what you're doing with DateAdd. Putting all the pieces together you get SaveExternalCopy.

Private Sub SaveExternalCopy()
    Dim saveBook As Workbook
    Set saveBook = Workbooks.Add

    Dim ppSheet As Worksheet
    Set ppSheet = Sheets("pp")
    ppSheet.UsedRange.AutoFilter field:=31, Criteria1:="Variable1"

    ppSheet.Copy Before:=saveBook.Worksheets(1)
    Dim ppCopy As Worksheet
    Set ppCopy = saveBook.Worksheets(1)
    ppCopy.Name = "PP1"

    Dim bkSheet As Worksheet
    Set bkSheet = Sheets("Bk")
    bkSheet.UsedRange.AutoFilter field:=16, Criteria1:="Variable2"

    bkSheet.Copy After:=ppCopy
    Dim bkCopy As Worksheet
    Set bkCopy = Sheets.Add(After:=bkSheet)
    bkCopy.Name = "BK1"

    Dim monthPriorDate As String
    monthPriorDate = Format(DateAdd("m", -1, Now), "yyyy_mm")

    'Dont forget to delete any default sheets that are created
    'when the workbook is first created.

    saveBook.SaveAs "\\user\location\" & "BK_PP_" & monthPriorDate & ".xlsx"
    saveBook.Close
End Sub

My final code ended up as follows. Save a copy before running the code as I've not 100% fully tested everything and there might be an unforseen issue that wasn't caught.

PP_Formatting_Saving now is shorter and everything it's doing is self-descriptive. Each Sub that's called is doing one thing, which makes it easier to fix a problem when looking at 10-20 lines of code instead of a single block of around 90 lines of code. Comments are mostly gone. Comments should describe the why of something being done not the what since what is happening should be apparent. Why is for when one choice was made over another, something like 'Doing it this way over <OtherWaysName> is 5 times faster.

Public Sub PP_Formatting_Saving()

    Application.ScreenUpdating = False

    Dim secondSheet As Worksheet
    Set secondSheet = Worksheets(2)
    secondSheet.Select 'This may possible be removable

    ConvertToNumeric secondSheet, "N"
    ConvertToNumeric secondSheet, "AA"
    ConvertToNumeric secondSheet, "G"

    FormatAsAccounting secondSheet, "J"
    FormatTheHeaderOf secondSheet

    secondSheet.Cells.EntireColumn.AutoFit
    secondSheet.Columns("O:O").ColumnWidth = 40
    secondSheet.Cells.AutoFilter '¿Does this turn it on or off? 'Filter all headers

    Dim firstSheet As Worksheet
    Set firstSheet = Worksheets(1)
    firstSheet.Activate 'This may possible be removable also
    firstSheet.Cells.WrapText = False

    FormatAsAccounting firstSheet, "C"
    ConvertToNumeric firstSheet, "B"
    firstSheet.Cells.EntireColumn.AutoFit
    FormatTheHeaderOf firstSheet
    firstSheet.Cells.AutoFilter

    SaveExternalCopy
End Sub

Private Sub ConvertToNumeric(ByVal sheetToUse As Worksheet, ByVal columnToFindLastCellOn As String)
    Dim helperCell As Range
    Set helperCell = sheetToUse.Range("AK1")
    helperCell.value = "1"
    helperCell.Copy

    Dim lastCell As Range
    Set lastCell = sheetToUse.Cells(Rows.Count, columnToFindLastCellOn).End(xlUp)

    Dim modifyRange As Range
    Set modifyRange = sheetToUse.Range(lastCell, lastCell.End(xlUp))
    modifyRange.PasteSpecial Paste:=xlPasteAll, Operation:=xlMultiply, SkipBlanks:=False, Transpose:=False

    helperCell.ClearContents
End Sub

Private Sub FormatTheHeaderOf(ByVal sheetToUse As Worksheet)
    Dim header As Range
    Set header = sheetToUse.Range(sheetToUse.Range("A1"), sheetToUse.Range("A1").End(xlToRight))
    With header.Interior
        .Pattern = xlSolid
        .PatternColorIndex = xlAutomatic
        .Color = RGB(192, 192, 192) 'RGB(192,192,192) = 12632256
        .TintAndShade = 0
        .PatternTintAndShade = 0
    End With
    header.Font.Bold = True
End Sub

Private Sub FormatAsAccounting(ByVal sheetToUse As Worksheet, ByVal columnToApplyFormatTo As String)
    Dim lastCell As Range
    Set lastCell = sheetToUse.Cells(Rows.Count, columnToApplyFormatTo).End(xlUp)

    Dim modifyRange As Range
    Set modifyRange = sheetToUse.Range(lastCell, lastCell.End(xlUp))
    'NumberFormat could possibly be used instead of Style
    'I'm not sure if you have altered the "Comma" Style though
    'modifyRange.NumberFormat = "_(* #,##0.00_);_(* (#,##0.00);_(* ""-""??_);_(@_)"
    modifyRange.Style = "Comma"
End Sub

Private Sub SaveExternalCopy()
    Dim saveBook As Workbook
    Set saveBook = Workbooks.Add

    Dim ppSheet As Worksheet
    Set ppSheet = Sheets("pp")
    ppSheet.UsedRange.AutoFilter field:=31, Criteria1:="Variable1"

    ppSheet.Copy Before:=saveBook.Worksheets(1)
    Dim ppCopy As Worksheet
    Set ppCopy = saveBook.Worksheets(1)
    ppCopy.Name = "PP1"

    Dim bkSheet As Worksheet
    Set bkSheet = Sheets("Bk")
    bkSheet.UsedRange.AutoFilter field:=16, Criteria1:="Variable2"

    bkSheet.Copy After:=ppCopy
    Dim bkCopy As Worksheet
    Set bkCopy = Sheets.Add(After:=bkSheet)
    bkCopy.Name = "BK1"

    Dim monthPriorDate As String
    monthPriorDate = Format(DateAdd("m", -1, Now), "yyyy_mm")

    'Dont forget to delete any default sheets that are created
    'when the workbook is first created.

    saveBook.SaveAs "\\user\location\" & "BK_PP_" & monthPriorDate & ".xlsx"
    saveBook.Close
End Sub

Lastly Rubberducks Code Inspections brought up a lot of what I addressed and has been very helpful to my VBA knowledge and improving my coding habits. I'm a contributor and openly biased in favor of it.


Javascript Object deep copy one-liner My goal was to create a single-line function that produces a deep copy of a javascript object.

Requirements:

Executable on a single line without the use of semicolons (exception: last character of the line)
Nested objects should not be references to the originals

Function:

let copiedObjct = (copy = (obj) => (Object.keys(obj).reduce((v, d) => Object.assign(v, {[d]: (obj[d].constructor === Object) ? copy(obj[d]) : obj[d]}), {})))(obj);

Usage (expanded):

let obj = { /*...*/ }
let copiedObjct = (copy = (obj) => (
  Object.keys(obj).reduce((v, d) => Object.assign(v, {
    [d]: (obj[d].constructor === Object) ? copy(obj[d]) : obj[d]
  }), {})
))(obj);

My questions are:

Is this code efficient?
Are there any reasons this shouldn't be used in the wild?
Is there a way I can make it shorter?

Is this code efficient?

Probably... not.

Efficiency can mean different things. Efficiency could mean "fast running code" or "use the least amount of memory" - this can only be determined by running the code through a profiler. Efficiency could also mean "I can use it easily" or "works reliably" or "handles edge cases well" - this can only be determined by having other people use it.

  Are there any reasons this shouldn't be used in the wild?

Yes.

Does the code really run on a variety of objects? What if I put things like Arrays, Dates, RegExps, Maps, Sets, etc. in my object? Does it handle circular references? This is a naive implementation of deep-copy which is fine in small apps and prototypes where you know what the data looks like. But in more general cases, this will probably not hold.

If you'd like to see how a general-purpose deep-copy function looks like, see how jQuery.extend or lodash.assign is implemented.

  Is there a way I can make it shorter?

Wrong question. Code size is irrelevant in JS, you have minifiers for that.


STL Stack Implementation I implemented std::stack from the STL for deeper understanding of the language and memory since I still am only a beginner. I implemented the stack using a singly linked list.

Header file:

/* Header file for abstract data type "STACK" implemented using a linked list  */

#ifndef STACK_H
#define STACK_H

template <class T>
class stack {

public:
void push(T);     //Function that inserts elements into the stack
bool empty();     //Function to test whether the stack is empty
T top();          //Returns top element of stack
void pop();       //Removes element at the top of the stack
int size();       //Returns size of stack
void print();     //Prints stack contents

struct node {     //Definition of node structure with constructor and destructor

T node_data;
node *next;

//default ctor
node() { next = nullptr;  }

//default dtor
~node() { delete root_node; }
};

private:
node *root_node = nullptr;
int elements = 0;
};

#endif //STACK_H

And here is the actual implementation file:

/* Definitons of the STACK data structure implemented using a linked list */

#include <iostream>
#include "stack.h"

using std::cout;
using std::endl;

/* FUNCTION: Test to check if the stack is empty or has at least one element */

template <class T>
bool stack<T>::empty() { return (root_node == nullptr); }

/* FUNCTION: Returns current size of the stack */
template <class T>
int stack<T>::size() { return elements; }

/* FUNCTION: Adds nodes to the stack with one argument which is the data to be inserted */

template <class T>
void stack<T>::push(T data) {

//Operation to preform if the stack is empty.
//Root element is popped off last (First in, Last out)
if ( empty() ) {
    root_node = new node;
    root_node->node_data = data;
    root_node->next = nullptr;
    elements++;
     }

    //Operation to preform if stack is not empty.
    //Elements inserted into stack with dynamic allocation.
    else  {
    node *new_node = new node;
*new_node = *root_node;
root_node->next = new_node;
root_node->node_data = data;

elements++;
 }
}

/* FUNCTION: Removes element at the top of the stack */
template <class T>
void stack<T>::pop() {

if (size() > 1) {
node *temp_node = new node;
temp_node = root_node->next;

root_node = temp_node;
elements--;
 }

else if (size() == 1) {
root_node = nullptr;
elements--;
}

else {cout << "\nOperation pop() failed: Stack is empty!" << endl;}
}

/* FUNCTION: Retrieves element at the top of the stack */
template <class T>
T stack<T>::top() {
if (!empty()) {return root_node->node_data;}
else {cout << "\nOperation top() failed: Stack is empty!" << endl; return -1;}
}

/* FUNCTION: Prints the stack contents */
template <class T>
void stack<T>::print() {
int index = size();
for (int i = 1; i < index; i++) {
        cout << "Element " << i << ": " << "  "  << top() << endl;
        pop();
        }
}

I would appreciate all criticism relevant to code, style, flow, camelCase vs underscore, and so forth.

I would appreciate all criticism relevant to code, style, flow, camelCase vs underscore, and so forth.

First, (contrary to Loki Astari's answer) I think your style is correct (i.e. please do not capitalize the first letter of your classes - keep them matching the std:: style).

Regarding the APIs of your code:

Your code doesn't enforce const correctness
For argument types, consider the following convention:

Observed parameter (no modification of the value in the function)

void function(const argument& a);

I/O parameter (function modifies the parameter, not owner):

void function(argument& a); // a is modified in the function (I/O parameter)

Owned parameter:

void function(argument a); // function "owns" a (gets it's own exclusive copy)

Following this, push() and top() should be written like this:

template<typename T>
void stack<T>::push(T data) // pass by value here
{
    root = new node{ std::move(data), root }; // and move the value here
    ++elements;
}

template<typename T>
const T&                // return a const T&
stack<T>::top() const   // and function is const
{
    if (!root)
        throw std::runtime_error("stack<T>::top: empty stack");
    return root->data;
}

Taking a parameter by value in push has these advantages:

specifies ownership in the interface
enables you to use all constructors of type T here
is exception safe (if the instance of the arg cannot be created, code will fail before entering the function body).
The size API:

int size();

Should be:

std::size_t size() const;

The print API has more problems:

It clears the list (either rename to "destructive_print", or ensure the iteration is not destructive). If you implement it in a non-destructive way, mark it const.
It introduces a dependency on std::cout that has nothing to do with the functionality of objects. Ideally, it shouldn't be a member. If you still want to have it as a member, pass the output stream instance as a parameter.

The constructor of the node class should construct a fully valid instance in one step. To decide on it's parameters, you should look at how you use it:

Usage example (your old code):

root_node = new node;
root_node->node_data = data;
root_node->next = nullptr;

Optimal example (new client code):

root_node = new node{ std::move(data), root_node }; // use std::move on the data

implies the node should be implemented like this:

template <class T>
class stack {
    // ...
    struct node { // struct, because it is internal and only
                  // access will be in stack
                  // (i.e. we can guarantee correct use in client code)

         T *node_data;
         node* next;
    };
}

The size should also be of std::size_t type.

Your stack doesn't have a destructor. The destructor should call delete on each node, in an iteration).

Your print element skips the last element in the list, as you begin indexing from 1 instead of 0, to less than number of elements. This means you iterate [elements - 1] times.

The pop() implementation can be written with no special case for one element:

template <class T>
void stack<T>::pop() {
    if(empty())
        throw std::runtime_error{"stack<T>::pop(): empty stack"};

    node* new_root = root_node->next;
    delete root_node;
    root_node = new_root;
    --elements;
}

Regarding the implementation of your class:

Do not print error messages, throw exceptions (this allows client code to decide what to do in case of an error, instead of forcing client code to get messages printed into it's application's output stream).


Using promises to GET and process data I'm working on a Meteor application which integrates a user's contacts from external sources (Google in the case of this example). I'm currently writing the server side code to retrieve this data and send it to the client.

I figured using promises to do this made sense due to the asynchronous manner of requests. So I have getContacts which sends the request to the Google API, and processContacts which processes and formats the response data:

getContacts = function (accessToken) {
  return new Promise(function (resolve) {
    httpRequest.get({
      url: 'https://www.google.com/m8/feeds/contacts/default/full?alt=json',
      auth: {
        'bearer': accessToken
      },
      headers: {
        'GData-Version': 3.0
      },
    }, function (err, res, body) {
      resolve(body);
    });
  });
}

processContacts = function(googleContacts) {
  return new Promise(function(resolve) {
    const contacts    = JSON.parse(googleContacts).feed.entry;
    const allContacts = [];
    const groupedContacts = { conflicts: [], new: [] };
    ...
    ...
    resolve(groupedContacts);
  });
}

I have a Meteor method google.contacts.import which is called synchronously from the client, and because all of this is asynchronous I'm using a future to force the client to wait for the call to finish:

Meteor.methods({
  'google.integration.import'(orgId) {
    check(orgId, String);

    const user = Users.getOne(Meteor.userId());
    let fut = new Future();

    getContacts(user.services.google.accessToken)
    .then(processContacts)
    .then(function(contacts){
      fut.return(contacts);
    })
    .catch(function(err){
      console.log('error! :' + err);
    });

    return fut.wait();
  },
})

This all works fine, but it seems...messy or somewhat convoluted. I'm relatively new to Javascript and ES6 in particular so I feel I could definitely improve this. Am I correct to be using promises here? I guess they probably aren't necessary in the case of processContacts. I also realize this is lacking in terms of error checking/reporting.

Any help or guidance is appreciated!

I think the preferred way to handle async on the server in Meteor is to use fiber aware async functions, e.g. use the http Meteor package which gives you fiber aware "sync" style calls on the server (you write it like a sync call but it does not block the thread) and standard async callback signature on the client.

Also, I am not sure about using futures on the client, is there even a fibers environment running on the client?  Instead, I think you just pass a standard callback to Meteor.call to handle the results of the method.


Query: user can see all posts by him and his friends This query is serving me a home page for users where a user can see all posts by him/her and his/her friends. This is an excellent query without any errors but I only wish to make it shorter if possible!

select U.fname, U.lname, P.postid, P.status, P.date, U.usernick, P.visible
from posts as P natural join users as U where
  U.visible=1 and P.visible=1
  and
  (P.userid='{$_SESSION['user_id']}' or P.userid in
    (select distinct U.userid from users as U inner join friends as F where
      (U.userid=F.userid and F.friend='{$_SESSION['user_id']}' and F.status=1) or
      (U.userid=F.friend and F.userid='{$_SESSION['user_id']}' and F.status=1)))
  or
  (P.streamid='{$_SESSION['user_id']}' or P.streamid in
    (select distinct U.userid from users as U inner join friends as F where
      (U.userid=F.userid and F.friend='{$_SESSION['user_id']}' and F.status=1) or
      (U.userid=F.friend and F.userid='{$_SESSION['user_id']}' and F.status=1)))
ORDER BY P.date DESC

CREATE  TABLE IF NOT EXISTS `users` (
  `userid` INT NOT NULL AUTO_INCREMENT ,
  `fname` VARCHAR(45) NOT NULL ,
  `lname` VARCHAR(45) NOT NULL ,
  `usernick` VARCHAR(45) NOT NULL ,
  `useremail` VARCHAR(45) NOT NULL ,
  `visible` INT(1) NOT NULL DEFAULT 1     //visible shows id is disables or not
 )

CREATE  TABLE IF NOT EXISTS `posts` (
  `postid` INT NOT NULL AUTO_INCREMENT ,   //Simple postid index
  `userid` INT NOT NULL ,                  //userid of poster
  `streamid` INT NOT NULL ,                // post is posted on his wall
  `status` VARCHAR(5000) NOT NULL ,        //the post that has been posted
  `date` TIMESTAMP NOT NULL DEFAULT now(), //post was posted on
  `visible` INT(1) NOT NULL DEFAULT 1 )    //post is deleted or not

CREATE  TABLE IF NOT EXISTS `network`.`friends` (
  `userid` INT NOT NULL ,                   //user who sent friend req
  `friend` INT NOT NULL ,                   //to whom friend req has been sent
  `status` INT(1) NOT NULL ,                //0 -> above both are not friends : 1->
                                            //both above are friends
  `date` TIMESTAMP NOT NULL DEFAULT now() , //
  `visible` INT(1) NOT NULL DEFAULT 1 )     //both are friends still

It looks like you are performing the same query against the Friends table twice.  If you need that query, I would split it off and run that query first, passing the result to your main query as a parameter.  But as you'll see at the end, I don't think that is necessary.

Also, you are using surrogate keys (yay!).  When you say "natural join" I think you are joining on userid AND visible (because they have common names between the two tables).  This is a waste, and also an error if you want posts that are visible and invisible.  Be specific about your join:

 from posts P join users U on P.userid = U.userid

Why do you have a status column in Friends table and what is the difference between status and visible?  Couldn't you just delete the Friend record if a user no longer wants that friend?  The reason I'm asking is that you could limit the size of this table and at the same time add a unique constraint on (userid, friendid) so that you test for uniqueness on insert and don't have to in the query.  Ensuring uniqueness takes time.  If you aren't already getting unique results, then your first choice should be to restructure your table and your query so that you do.

UNIQUE KEY `friendship_unique` (`userid`,`friend`),

You may have to use InnoDB instead of MyISAM in order for that to work.  MyISAM will accept it and silently ignore it because MyISAM does not support foreign keys or most constraints.  For this reason, InnoDB may be faster for joins.  With big tables I think it is.  The one down side of using InnoDB vs. MyISAM is that the minimum time for inserting into an InnoDB table is about 5ms where MyISAM inserts are less than 1ms.  In practice, this is only an issue if you are storing log files or session information in the database that needs to be updated with every request.  You can always turn a single table from InnoDB to MyISAM later if you need the insert speed more than you need query speed, but that should be the exception rather than the rule.

You are making a lot of non-standard type choices for your column definitions.  Why are you using int instead of bigint for your ID column?  Why are you using int instead of bit for your boolean columns?  Why aren't you using UTF-8?  Why no keys?  Are you using MyISAM tables because you don't want to pay to distribute this database, or is it just on a web server where it's free to use InnoDB?  I think something like the following would be much more standard:

CREATE TABLE IF NOT EXISTS `user` (
  `id` bigint(20) unsigned NOT NULL AUTO_INCREMENT
  `fname` VARCHAR(45) NOT NULL ,
  `lname` VARCHAR(45) NOT NULL ,
  `usernick` VARCHAR(45) NOT NULL ,
  `useremail` VARCHAR(45) NOT NULL ,
  `is_deleted` bit(1) NOT NULL DEFAULT b'0' COMMENT 'True if record is logically deleted.',
  PRIMARY KEY (`id`),
  UNIQUE KEY `id` (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8

CREATE TABLE IF NOT EXISTS `friend` (
  `id` bigint(20) unsigned NOT NULL
  `friend_id` bigint(20) unsigned NOT NULL
  `date` TIMESTAMP NOT NULL DEFAULT now() , //
  `is_deleted` bit(1) NOT NULL DEFAULT b'0' COMMENT 'True if record is logically deleted.',
  PRIMARY KEY (`id`),
  UNIQUE KEY `id` (`id`)
  UNIQUE KEY `user_friend_unique` (`id`,`friend_id`),
  KEY `friend_fk_user` (`id`),
  KEY `friend_fk_friend` (`friend_id`),
  CONSTRAINT `friend_fk_user` FOREIGN KEY (`id`) REFERENCES `user` (`id`) ON DELETE CASCADE,
  CONSTRAINT `friend_fk_friend` FOREIGN KEY (`friend_id`) REFERENCES `user` (`id`) ON DELETE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8

OK, now you have clearly told MySQL to use InnoDB, to make your surrogate key unique, index on it, use it as a foreign key, and to delete records from the friend table when either of the corresponding user records are deleted.  This ensures a much cleaner database - fewer records means faster queries.

Hmm... I also think you can restructure your query.  Here are your friends posts:

select F.fname, F.lname, P.postid, P.status, P.date, U.usernick, P.is_deleted
  from friend F join post P on p.user_id = F.id
  where F.id = '{$_SESSION['user_id']}'

Here are your posts:

select U.fname, U.lname, P.postid, P.status, P.date, U.usernick, P.is_deleted
  from post P join user U on P.user_id = U.id
  where P.user_id = '{$_SESSION['user_id']}'

You should be able to do something like that without making any of the other changes I suggested.


Performing multiple validation checks I have a validation which checks that a string is a valid asset number.  There are currently three formats it could be in, like: 001-123456, WSH002M52B, or CTR0025MLD.  The way my code is set up now is like:

public bool IsValidAssetNum(string assetNum)
{
    if (this.IsNorthAmericanAssetFormat(assetNum)) return true;
    if (this.IsGermanAssetFormat(assetNum)) return true;
    if (this.IsAustralianAssetFormat(assetNum)) return true;

    return false;
}

With each of those checks being like:

private bool IsNorthAmericanAssetFormat(string assetNum)
{
    var regex = new Regex(ComponentsResources.NorthAmericaAssetNumValidationRegex);
    var match = regex.Match(assetNum);

    return match.Success;
}

In the future, if more formats come up, we'll have to add a new specific format check as well as add it to the IsValidAssetNum function.  Is there a pattern out there to solve this issue?  I know a lot of if statement refactorings come from the command pattern or strategy pattern, but this seems like a different type of issue.

Regular expressions, when compiled down, are very high performing. I would suggest that you build a composite expression, let the Regex engine optimize it, and have just a single check... something like:

string[] validators = {ComponentsResources.NorthAmericaAssetNumValidationRegex, ....};
var compoundValidator = new Regex("(" + string.join(")|(", validators) + ")");

Then, in your code, you can reuse that single regex just once:

public bool IsValidAssetNum(string assetNum)
{
    return compoundValidator.Match(assetNum).Success;
}

There are any number of ways you can add validators to the the system, config files come to mind as being an easy way to manage them without code changes.


JavaScript BB-Code transformation I am doing a lot of stuff with JavaScript and jQuery lately, but am not heading for clean code too much, as I am usually writing userscripts or small applications for myself.

Recently, a community I am part of announced that it will change their supported BB-Codes, as the tabmenu BB-Code had a possible safety issue. By now, hundreds of posts have been done with tabmenus and a lot of users were complaining that editing all these posts will take forever.

As I am a guy who likes to automate as much stuff as possible, I took the task and developed a small application that changes posts with the tabmenu BB-Code into a format that is supported by the forum.

Here are two examples of what the application does:

Original BB-Code:

[tabmenu]
[tab=Tab1]Tab1 Text
[tab=Tab2]Tab2 Text
[/tabmenu]

Note: tabmenu has a closing tag, tab does not and will contain all text until the next tab or until the closing [/tabmenu].

Result:

[h]Tab1[/h]Tab1 Text
[h]Tab2[/h]Tab2 Text

h turn into headlines, just as in HTML and force a linebreak.

As the tabmenu does not only support tabs, but also subtabs, here is the second example:

Original BB-Code:

[tabmenu]
[tab=Tab1]
[subtab=Subtab11]Subtab11 Text
[subtab=Subtab12]Subtab12 Text
[tab=Tab2]
[subtab=Subtab21]Subtab21 Text
[/tabmenu]

Note: If you have subtabs in you tabmenu, you can't write text into tab areas. Apart from that subtabs behave like tabs when getting their content.

Result:

[h]Tab1[/h]
[spoiler=Subtab11]Subtab11 Text
[/spoiler][spoiler=Subtab12]Subtab12 Text
[/spoiler][h]Tab2[/h]
[spoiler=Subtab21]Subtab21 Text
[/spoiler]

tabs are still being replaced with h. The behavior for subtabs is a bit different though. Here my application is taking the title from the subtab, turning it into a spoiler-Tag (not comparable with the spoiler-codes of Stack Exchange, the spoiler-BB-Code is more like an accordion) and placing the content of the subtab between the opening spoiler-Tag and the closing one.

$(function () {
    $('#buttonGenerate').on('click', function () {
        // The format of titles can be like this: [tab=Title], [tab="Title"] & [tab='Title']
        // the user can choose the format with this checkbox
        var bbFormat = $('input[name=bbCodeFormat]:checked').val();
        var titleTemplate;
        var regexTab;
        var regexSubTab;

        switch (bbFormat) {
            // Depending on which format was chosen, different regex patterns are used
            case '1':
            // Format: [tab=Title], allows certain special chars in the title
                regexTab = /\[tab=((\w|[ äöüÄÖÜ,.\-;:_#'+*!"§$%&/()=?ß^°])*)\]/g;
                regexSubTab = /\[subtab=((\w|[ äöüÄÖÜ,.\-;:_#'+*!"§$%&/()=?ß^°])*)\]((.|\s)+?((?=\[subtab)|(?=\[\/tabmenu)|(?=\[tab)))/g;
                titleTemplate = '###';
                break;
            // Format: [tab="Title"] allows any special chars in the title
            case '2':
                regexTab = /\[tab="((.| )*)"\]/g;
                regexSubTab = /\[subtab="((.| )*)"\]((.|\s)+?((?=\[subtab)|(?=\[\/tabmenu)|(?=\[tab)))/g;
                titleTemplate = '"###"';
                break;
            // Format: [tab='Title'] allows any special chars in the title
            case '3':
                regexTab = /\[tab='((.| )*)'\]/g;
                regexSubTab = /\[subtab='((.| )*)'\]((.|\s)+?((?=\[subtab)|(?=\[\/tabmenu)|(?=\[tab)))/g;
                titleTemplate = "'###'";
                break;
        }

        // Gather original BB-Code from textarea
        var input = $('#inputArea').val();

        // Replace the tabmenu BB-Code via Regex
        input = input.replace(regexSubTab, '[spoiler=' + titleTemplate.replace('###', '$1') + ']$3[/spoiler]');
        input = input.replace(regexTab, '[h]$1[/h]');
        input = input.replace('[tabmenu]', '').replace('[/tabmenu]', '');
        $('#outputArea').val(input.trim());
    });
});

Your code is good, but it could be improved:

Using an enum

If you use an enum, you can avoid Magic Numbers in your switch statement:

    switch (bbFormat) {
        // Depending on which format was chosen, different regex patterns are used
        case '1':

Whereas, doing:

var BB_FORMAT_TYPES = {
    NoQuotes: 1,
    DoubleQuotes: 2,
    SingleQuotes: 3
};

with:

switch (bbFormat) {
    case BB_FORMAT_TYPES.NoQuotes:
        break;
    case BB_FORMAT_TYPES.SingleQuotes:
        break;
    case BB_FORMAT_TYPES.DoubleQuotes:
        break;
}

However, using an enum is usually suited more for when the contents of each have different execution styles. In your case, it'd be better to move the code to an object, and have a single execution path referencing said object.

    var object = {
        1: {
            regexTab: /\[tab=((\w|[ äöüÄÖÜ,.\-;:_#'+*!"§$%&/()=?ß^°])*)\]/g,
            regexSubTab: /\[subtab=((\w|[ äöüÄÖÜ,.\-;:_#'+*!"§$%&/()=?ß^°])*)\]((.|\s)+?((?=\[subtab)|(?=\[\/tabmenu)|(?=\[tab)))/g,
            titleTemplate: '###'
        },
        2: {
            regexTab: /\[tab="((.| )*)"\]/g,
            regexSubTab: /\[subtab="((.| )*)"\]((.|\s)+?((?=\[subtab)|(?=\[\/tabmenu)|(?=\[tab)))/g,
            titleTemplate: '"###"'
        },
        3: {
            regexTab: /\[tab='((.| )*)'\]/g,
            regexSubTab: /\[subtab='((.| )*)'\]((.|\s)+?((?=\[subtab)|(?=\[\/tabmenu)|(?=\[tab)))/g,
            titleTemplate: "'###'"
        }
    };

Chaining:

Instead of having the replace on each line, try using chaining:

    input = input.replace(regexSubTab, '[spoiler=' + titleTemplate.replace('###', '$1') + ']$3[/spoiler]');
                 .replace(regexTab, '[h]$1[/h]');
                 .replace('[tabmenu]', '')
                 .replace('[/tabmenu]', '');

that way you can avoid looking up and writing to the input variable each time.


Hough transform algorithm - Idiomatic c++ I've got some working code to compute the Hough transform for circles:

#include <stdio.h>
#include <opencv/cv.hpp>
#include <math.h>
#include <string>
#include <iostream>

int *** allocate3DArray(int y, int x, int r)  {
    int ***array = (int***)malloc(sizeof(int**)*y);
    for (int i = 0; i < y; i++)  {
        array[i] = (int**)malloc(sizeof(int*)*x);
        for (int j = 0; j < x; j++)  {
            array[i][j] = (int*)malloc(sizeof(int)*r);
            for(int k = 0; k < r; k++) {
                array[i][j][k] = 0;
            }
        }
    }
    return array;

}

void free3d(int ***arr, int y, int x, int r)  {
    for (int i = 0; i < y; i++)  {
        for (int j = 0; j < x; j++)  {
            free(arr[i][j]);
        }
        free(arr[i]);

    }
    free(arr);

}

vector<Circle> HoughTransformCircles(Mat image, Mat &gradient_mag, Mat &gradient_dir)  {
    //Maximum circle radius is rLen = image.width / 2
    int rLen = image.rows/2;
    int ***houghSpace = allocate3DArray(image.rows, image.cols, rLen);
    for (int y = 0; y < image.rows; y++) {
        for (int x = 0; x < image.cols; x++) {
            for (int r = 0; r < rLen; r++)     {
                int x0 = x - (int)(r*cos(gradient_dir.at<double>(y, x)));
                int y0 = y - (int)(r*sin(gradient_dir.at<double>(y, x)));
                if(x0 >= 0 && x0 < image.cols && y0 >= 0 && y0 < image.rows )  {
                    houghSpace[y0][x0][r]++;
                }
                x0 = x + (int)(r*cos(gradient_dir.at<double>(y, x)));
                y0 = y + (int)(r*sin(gradient_dir.at<double>(y, x)));
                if(x0 >= 0 && x0 < image.cols && y0 >= 0 && y0 < image.rows )  {
                    houghSpace[y0][x0][r]++;
                }

            }
        }
    }
    vector<Circle> circles;
    for (int y = 0; y < image.rows; y++)    {
        for (int x = 0; x < image.cols; x++)  {
            for (int r = 0; r < rLen; r++)  {
                if (houghSpace[y][x][r] > 30)  {
                    Circle temp  = Circle(x, y, r);
                    circles.push_back(temp);
                }
            }
        }
    }

    free3d(houghSpace, image.rows, image.cols, rLen);

   return circles;
}

How do I make this as idiomatic as possible? I am not allowed to use C++ 11  unfortunately.

Here are some suggestions for improving the code.

Use all required #includes

The code uses vector but doesn't include the corresponding header.  The code should have

#include <vector>

Use <cmath> instead of <math.h>

The difference between the two forms is that the former defines things within the std:: namespace versus into the global namespace.  Language lawyers have lots of fun with this, but for daily use I'd recommend using <cmath> (and also <cstdio>).  See this SO question for details.

Avoid malloc and free

The old C-style calls still work, of course, but are not recommended.  They are error prone and tend to require more code to do correctly (i.e. checking the return value of malloc for NULL).  Instead, the modern C++ idiom is to use RAII which stand for "Resource Acquisition Is Initialization).  See R.10 and R.1 for details.

Use objects

The 3D array would be much better implemented as an object.  While you could, of course, write one (and I'd encourage you to try that as a learning exercise), there's an even smarter way to do that in the next suggestion.

Know your libraries

The code is already using the OpenCV Mat class, so what would make sense would be to also use that for the Hough space as well.  So this line:

int ***houghSpace = allocate3DArray(image.rows, image.cols, rLen);

would be written instead like this:

int dims[] = { image.rows, image.cols, rLen };
Mat houghSpace(3, dims, CV_8UC(1), Scalar::all(0));

Since that's a properly designed C++ class, it will automatically allocate memory as needed and also de-allocate it when the houghSpace object's destructor is called.  Using it is almost a simple:

houghSpace.at<cv::Vec3i>(y0,x0)[r]++;

Eliminate unused variables

Unused variables are a sign of poor code quality, so eliminating them should be a priority.  In this code, gradient_mag in HoughTransformCircles and r in free3d are unused. Your compiler is probably also smart enough to tell you that, if you ask it to do so.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.  It also appears you've got using namespace cv; somewhere as well and it's also not a good idea for similar reasons.

Use variables to clarify your code

The code currently has this nested set of loops:

for (int y = 0; y < image.rows; y++) {
    for (int x = 0; x < image.cols; x++) {
        for (int r = 0; r < rLen; r++)     {
            int x0 = x - (int)(r*cos(gradient_dir.at<double>(y, x)));
            int y0 = y - (int)(r*sin(gradient_dir.at<double>(y, x)));
            if(x0 >= 0 && x0 < image.cols && y0 >= 0 && y0 < image.rows )  {
                houghSpace[y0][x0][r]++;
            }
            x0 = x + (int)(r*cos(gradient_dir.at<double>(y, x)));
            y0 = y + (int)(r*sin(gradient_dir.at<double>(y, x)));
            if(x0 >= 0 && x0 < image.cols && y0 >= 0 && y0 < image.rows )  {
                houghSpace[y0][x0][r]++;
            }
        }
    }
}

Instead, I'd write it like this:

for (int y = 0; y < image.rows; y++) {
    for (int x = 0; x < image.cols; x++) {
        for (int r = 0; r < rLen; r++)     {
            int a = r * std::cos(gradient_dir.at<double>(y, x));
            int b = r * std::sin(gradient_dir.at<double>(y, x));
            int x0 = x - a;
            int y0 = y - b;
            if(x0 >= 0 && x0 < image.cols && y0 >= 0 && y0 < image.rows )  {
                houghSpace.at<cv::Vec3i>(y0,x0)[r]++;
            }
            x0 = x + a;
            y0 = y + b;
            if(x0 >= 0 && x0 < image.cols && y0 >= 0 && y0 < image.rows )  {
                houghSpace.at<cv::Vec3i>(y0,x0)[r]++;
            }
        }
    }
}

In C++11, I'd use a range-for.

Provide complete code to reviewers

This is not so much a change to the code as a change in how you present it to other people.  Without the full context of the code and an example of how to use it, it takes more effort for other people to understand your code.  This affects not only code reviews, but also maintenance of the code in the future, by you or by others.  One good way to address that is by the use of comments.  Another good technique is to include test code showing how your code is intended to be used.

Use const where possible

The HoughTransformCircles routine does not  alter the image passed to it, and so it should be declared as taking const Mat& image as an argument.