Implement a fold-like Function in Haskell Let's say I need to write a function that applies a function to each item in a List, and then appends it to an accumulator. When the List is empty, return the accumulator.

go :: [a] -> (a -> [b]) -> [b] -> [b]
go [] f acc = acc
go (x:xs) f acc = go xs f (acc ++ f x)

myFold :: [a] -> (a -> [b]) -> [b]
myFold as f = go as f []

I used the go function so that I wouldn't force the myFold caller to have to provide an empty [b] type. Also, I used the go function to achieve tail recursion.

In Scala, I would've put @annotation.tailrec to make the compilation fail if the compiler could not perform tail-call optimization.

Is the above code idiomatic in Haskell?

First, your function is concatMap from Prelude.

More idiomatic ways to write it:

myFold :: (a -> [b]) -> [a] -> [b]
myFold = concat . map
-- or
myFold f = foldr (\x acc -> (f x) ++ acc) []

If efficiency is a concern, it's a completely different question. Recommended steps in priority order:

Always compile at least with -O2
Lists are slow. ++ operation especially. Consider Data.Sequence or even vectors.
Avoid lazyness in your types
Provide all small functions with INLINE pragma.
And more...


Implementation of `ulp` and `nextAfter` for platforms where these are missing I need the ulp method in Codename ONE, and since it isn't available there, I tried to come up with my own version. The code is completely written from scratch, based solely on the documentation of the java.lang.Math class.

Please ignore the @Override annotations. This is from a utility class I wrote to be able to use the same code for Java SE and Codename ONE development (this obviously is the Codename ONE version).

private static final double MAX_ULP = com.codename1.util.MathUtil.pow(2.0, 971);

@Override
public double ulp(double d) {
    if (Double.isNaN(d)) {
        // If the argument is NaN, then the result is NaN.
        return Double.NaN;
    } else if (Double.isInfinite(d)) {
        // If the argument is positive or negative infinity, then the
        // result is positive infinity.
        return Double.POSITIVE_INFINITY;
    } else if (d == 0.0) {
        // If the argument is positive or negative zero, then the result is Double.MIN_VALUE.
        return Double.MIN_VALUE;
    } else if (d == Double.MAX_VALUE || d == -Double.MAX_VALUE) {
        //If the argument is ±Double.MAX_VALUE, then the result is equal to 2^971.
        return MAX_ULP;
    } else {
        d = Math.abs(d);
        return nextAfter(d, Double.MAX_VALUE) - d;
    }
}

@Override
public double copySign(double x, double y) {
    return com.codename1.util.MathUtil.copysign(x, y);
}

private boolean isSameSign(double x, double y) {
    return copySign(x, y) == x;
}

@Override
public double nextAfter(double start, double direction) {
    if (Double.isNaN(start) || Double.isNaN(direction)) {
        // If either argument is a NaN, then NaN is returned.
        return Double.NaN;
    } else if (start == direction) {
        // If both arguments compare as equal the second argument is returned.
        return direction;
    } else if ((start == Double.MIN_VALUE || start == -Double.MIN_VALUE)
            && (!isSameSign(start, direction) || Math.abs(direction) < Math.abs(start))) {
        // If start is ±Double.MIN_VALUE and direction has a value such that
        // the result should have a smaller magnitude, then a zero with
        // the same sign as start is returned.
        return copySign(0.0, start);
    } else if (Double.isInfinite(start) && !Double.isInfinite(direction)) {
        // If start is infinite and direction has a value such that the
        // result should have a smaller magnitude, Double.MAX_VALUE with
        // the same sign as start is returned.
        return copySign(Double.MAX_VALUE, start);
    } else if ((start == Double.MAX_VALUE || start == -Double.MAX_VALUE)
            && (Double.isInfinite(direction) && isSameSign(start, direction))) {
        // If start is equal to ± Double.MAX_VALUE and direction has a
        // value such that the result should have a larger magnitude, an
        // infinity with same sign as start is returned.
        return start > 0.0 ? Double.POSITIVE_INFINITY : Double.NEGATIVE_INFINITY;
    } else if (start==0.0) {
        return copySign(Double.MIN_VALUE, direction);
    } else {
        long d = ((direction > start) == (start>=0)) ? 1L : -1L;
        long x = Double.doubleToLongBits(start) + d;
        return Double.longBitsToDouble(x);
    }

}

PS: I wrote this code from scratch (for licence reasons), so if anybody has a use for this, feel free to use and modify it in your own projects.

EDIT: The code I originally posted still had an error I found after checking all corner cases against the JDK version. I had to introduce another check for start==0.0 in nextAfter to get all unit tests pass. Also, the condition for setting d was flawed.

The code looks neat enough, well formatted, etc. That's great. There is one common issue you have in your code, and that is the else-if for early-return blocks.

The code:

if (conditionA) {
    ... do something
    return something;
} else if (conditionB) {
    ... do something
    return something;
} else if .....

has redundant if blocks. The logic is right, but, it would be better as:

if (conditionA) {
    ... do something
    return something;
}
if (conditionB) {
    ... do something
    return something;
}
if .....

You have that pattern in a lot of places.

A second general concern I have is that you do not specify 'final' for your parameters in places where it can make sense. The 'final' concept is debatable for performance, but I have found that it makes a difference in both the disciple, and occasionally the performance. For example, the function:

private boolean isSameSign(double x, double y) {
    return copySign(x, y) == x;
}

would be better as:

private boolean isSameSign(final double x, final double y) {
    return copySign(x, y) == x;
}

ulp()

It would be convenient in your ULP to do the d = Math.abs(d) earlier in the function as it simplifies some logic. Consider the following:

public double ulp(final double input) {
    final double d = Math.abs(input);
    if (Double.isNaN(d)) {
        // If the argument is NaN, then the result is NaN.
        return Double.NaN;
    }
    if (Double.isInfinite(d)) {
        // If the argument is positive or negative infinity, then the
        // result is positive infinity.
        return Double.POSITIVE_INFINITY;
    }
    if (d == 0.0) {
        // If the argument is positive or negative zero, then the result is Double.MIN_VALUE.
        return Double.MIN_VALUE;
    }
    if (d == Double.MAX_VALUE) {
        //If the argument is ±Double.MAX_VALUE, then the result is equal to 2^971.
        return MAX_ULP;
    }
    return nextAfter(d, Double.MAX_VALUE) - d;
}

I assume that your research on \$2^{971}\$ is accurate for Double.MAX_VALUE.

copySign()

Edit: This method confused me, and lead me to introduce a bug in my code (now fixed).

It is my opinion that the arguments are the wrong way around. I would expect copySign(x, y) to copy the sign from x to y, but your code implements it the other way around. Instead of arguments x and y, they should have better names, and the method should be named differently as well:

public double copySignToFrom(final double target, final double sign) {...}

My preference would be to just reverse the arguments, but that method is overridden, and the implementation is not actually shown here (Note: Show all your code next time...).

nextAfter()

Again, like ulp(), this function would benefit from removing the 'else if' construct. The early return makes it unnecessary.

The logic in here looks good. I have tried to see if there's a better way of doing things, and, I think there may be... by recording the signs of the start & direction, you can do all the math in absolute terms, and re-conevrt the signs later.... It's a toss up as to whether the performance will be better to do the conversion each time, and then have simpler conditions on the logic.

So, the only real criticism I have here is the else if, but, for interest, consider the following:

Edit: revised my code example because of copySign usage bug...

public class DoublePlay {

    private static final long MASK = Long.MIN_VALUE;

//    @Override
    public double copySign(double target, double source) {
        if ((Double.doubleToLongBits(target) & MASK) == (Double.doubleToLongBits(source) & MASK)) {
            return target;
        }
        return Double.longBitsToDouble(Double.doubleToLongBits(target) ^ MASK);
    }

    private boolean isSameSign(double x, double y) {
        return copySign(x, y) == x;
    }

    public double nextAfter(final double start, final double direction) {
        if (Double.isNaN(start) || Double.isNaN(direction)) {
            // If either argument is a NaN, then NaN is returned.
            return Double.NaN;
        }
        if (start == direction) {
            // If both arguments compare as equal the second argument is returned.
            return direction;
        }

        final double absstart = Math.abs(start);
        final double absdir = Math.abs(direction);
        final boolean tozero = !isSameSign(start, direction) || absdir < absstart;

        if (tozero) {
            // we are reducing the magnitude, going toward zero.
            if (absstart == Double.MIN_VALUE) {
                return copySign(0.0, start);
            }
            if (Double.isInfinite(absstart)) {
                return copySign(Double.MAX_VALUE, start);
            }
            return copySign(Double.longBitsToDouble(Double.doubleToLongBits(absstart) - 1L), start);
        }

        // we are increasing the magnitude, toward +-Infinity
        if (absstart == Double.MAX_VALUE) {
            return copySign(Double.POSITIVE_INFINITY, start);
        }
        return copySign(Double.longBitsToDouble(Double.doubleToLongBits(absstart) +1L), start);

    }

    private static final void testValue(DoublePlay dp, final double base, final double direction) {
        double result = dp.nextAfter(base, direction);
        System.out.printf("From: %016x %s%nStep: %016x %s%nTo  : %016x %s%n%n",
                Double.doubleToLongBits(base), String.valueOf(base),
                Double.doubleToLongBits(direction), String.valueOf(direction),
                Double.doubleToLongBits(result), String.valueOf(result)
                );
    }

    public static void main(String[] args) {
        DoublePlay dp = new DoublePlay();
        testValue(dp, Double.MIN_VALUE, Double.POSITIVE_INFINITY);
        testValue(dp, Double.MIN_VALUE, Double.NEGATIVE_INFINITY);
        testValue(dp, -Double.MIN_VALUE, Double.POSITIVE_INFINITY);
        testValue(dp, -Double.MIN_VALUE, Double.NEGATIVE_INFINITY);
    }

}

The above code makes the decision 'early' about whether the magnitude of the nextAfter() is increasing (to infinity), or decreasing (to zero). It simplifies the rest of the conditions.

Added an Ideone with the code as well


Scraping names from multipages without clicking on the next button I've written a script in python in association with selenium to harvest some coffee shop names from yellowpages. Although, the webpage is not javascript injected one, I used selenium for the purpose of experiment, specially how to handle multipage without clicking on the next button.

Upon execution I could notice that my script runs flawlessly and parses the names from that page. However, it does all this very slowly. Is there any way I can make it's performance faster than how it is now being limited within selenium?

Here is the code:

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

driver = webdriver.Chrome()
wait = WebDriverWait(driver, 10)

for page_num in range(1,3):
    driver.get('https://www.yellowpages.com/search?search_terms=pizza&geo_location_terms=San%20Francisco%2C%20CA&page={0}'.format(page_num))
    for item in wait.until(EC.presence_of_all_elements_located((By.CSS_SELECTOR, "div.info"))):
        try:
            name = item.find_element_by_css_selector('a.business-name span[itemprop=name]').text
        except:
            name = ''
        print(name)

driver.quit()

The code is pretty much straightforward and understandable, but I would still work on the following stylistic and readability issues:

extract constants. It might be a good idea to extract the URL template and maximum page number into separate constants, or as arguments of a function
put your main execution logic into the if __name__ == '__main__' to avoid it being executed if the module is imported
avoid using bare exception clauses. Be specific about the exceptions you are handling. In this case, NoSuchElementException is a good exception to handle if an item name is not found
use try/finally to safely close the driver if it fails before quitting - this way you would eliminate a situation when you have opened "ghost" browser windows left from failed scraping tries

All things applied:

from selenium import webdriver
from selenium.common.exceptions import NoSuchElementException
from selenium.webdriver.common.by import By
from selenium.webdriver.support.ui import WebDriverWait
from selenium.webdriver.support import expected_conditions as EC

def scrape(url, max_page_number):
    driver = webdriver.Chrome()
    wait = WebDriverWait(driver, 10)

    try:
        for page_number in range(1, max_page_number + 1):
            driver.get(url.format(page_number))

            for item in wait.until(EC.presence_of_all_elements_located((By.CSS_SELECTOR, ".info"))):
                try:
                    name = item.find_element_by_css_selector('a.business-name span[itemprop=name]').text
                except NoSuchElementException:
                    name = ''

                print(name)
    finally:
        driver.quit()

if __name__ == '__main__':
    url_template = 'https://www.yellowpages.com/search?search_terms=pizza&geo_location_terms=San%20Francisco%2C%20CA&page={0}'
    max_page_number = 2

    scrape(url_template, max_page_number)


Coin sum dynamic programming in Python I am preparing for an interview, and here's a popular problem on the dynamic programming. And I want to see if there's any feedback or code review for my solution at dynamic programming question.

#  Problem: Coin Sum
#
#  Given an array of coins and a target total, return how many
#  unique ways there are to use the coins to make up that total.
#
#  Input:  coins {Integer Array}, total {Integer}
#  Output: {Integer}
#
#
#  Example:
#  Input:  [1,2,3], 4
#  Output: 4
#
#  1+1+1+1
#  1+3
#  1+1+2
#  2+2
#
#
#  Input:  [2,5,3,6], 10
#  Output: 5
#
#          2+3+5
#          5+5
#          2+3+2+3
#          2+2+6
#          2+2+3+3
#

#  Note: You have an unlimited number of each coin type. All coins in the
#        coin array will be unique
#        Order does not matter. Ex: One penny and one nickel to create six
#        cents is equivalent to one nickel and one penny
#
#

def coin_sum(coins, total):
    # tabulation way
    arr = [1] + [0] * total
    for coin in coins:
        for i in range(coin, total + 1):
            arr[i] += arr[i - coin]
    return 0 if total == 0 else arr[total]

# Time Complexity: O(N*M), let the number of coins be m.
# We iterate from arr[coin] -> arr[n], or ~ n operations on each coin, hence n*m.
# Auxiliary Space Complexity: O(N)

It passes all of the following 5 tests for my Coin sum tests:

def expect(count, name, test):
    if (count == None or not isinstance(count, list) or len(count) != 2):
        count = [0, 0]
    else:
        count[1] += 1

    result = 'false'
    errMsg = None
    try:
        if test():
            result = ' true'
            count[0] += 1
    except Exception as err:
        errMsg = str(err)

    print('  ' + (str(count[1]) + ')   ') + result + ' : ' + name)
    if errMsg != None:
        print('       ' + errMsg + '\n')

def lists_equal(lst1, lst2):
    if len(lst1) != len(lst2):
        return False
    for i in range(0, len(lst1)):
        if lst1[i] != lst2[i]:
            return False
    return True

print("\nCoin Sum Tests")
test_count = [0, 0]

def test():
    example = coin_sum([1, 2, 3], 4)
    return example == 4

expect(test_count, 'should work for first example case', test)

def test():
    example = coin_sum([2, 5, 3, 6], 10)
    return example == 5

expect(test_count, 'should work for second example case', test)

def test():
    example = coin_sum([2], 10)
    return example == 1

expect(test_count, 'should work for a single coin', test)

def test():
    example = coin_sum([7, 15], 20)
    return example == 0

expect(test_count, 'should work when there is no solution', test)

def test():
    example = coin_sum(
        [78, 10, 4, 22, 44, 31, 60, 62, 95, 37, 28, 11, 17, 67, 33, 3, 65, 9, 26, 52, 25, 69, 41, 57, 93, 70, 96, 5,
         97, 48, 50, 27, 6, 77, 1, 55, 45, 14, 72, 87, 8, 71, 15, 59], 100)
    return example == 3850949

expect(test_count, 'should work for variety of coins and large total', test)

print(('\nPASSED: ' + str(test_count[0]) + ' / ' + str(test_count[1])))

Some tips:

Know your PEP8, format your code appropriately.
Learn string formatting - and know the differences between 3.6 and earlier.
Your errMsg and Exception process seem like you expect an error but what error are you expecting? Might want to specify which error you're expecting from your edge cases. ValueError? AttributeError? Also, you shouldn't perform equality statements against None.
if (count == None or not isinstance(count, list) or len(count) != 2) has redundant brackets

And use a proper test harness instead of writing your own. Here is your code with your expect function removed and using pytest:

import pytest

def coin_sum(coins, total):
    # tabulation way
    arr = [1] + [0] * total
    for coin in coins:
        for i in range(coin, total + 1):
            arr[i] += arr[i - coin]
    return 0 if total == 0 else arr[total]

def test01():
    example = coin_sum([1, 2, 3], 4)
    assert example == 4

def test02():
    example = coin_sum([2, 5, 3, 6], 10)
    assert example == 5

def test03():
    example = coin_sum([2], 10)
    assert example == 1

def test04():
    example = coin_sum([7, 15], 20)
    assert example == 0

def test05():
    example = coin_sum(
            [78, 10, 4, 22, 44, 31, 60, 62, 95, 37, 28, 11, 17, 67, 33, 3, 65, 9, 26, 52, 25, 69, 41, 57, 93, 70, 96, 5,
             97, 48, 50, 27, 6, 77, 1, 55, 45, 14, 72, 87, 8, 71, 15, 59], 100)
    assert example == 3850949

And here are the results:

C:\PycharmProjects\codereview\tests>pytest test_tt.py
======================== test session starts ========================
platform win32 -- Python 3.7.0, pytest-3.6.2, py-1.5.4, pluggy-0.6.0
rootdir: C:\PycharmProjects\codereview\tests, inifile:
plugins: cov-2.5.1, celery-4.2.0
collected 5 items

test_tt.py .....                                               [100%]

===================== 5 passed in 0.13 seconds ======================

Hope this helps!


Load drinks and their ingredients into dictionaries I'm working on a web application using Bottle, and it is, at this point, functional.

The gist of it is that I have a database of drinks, with associated IDs, and associated sets of ingredients. the name and ID of the drink are passed to the client, which displays them.

Currently, when the program loads up, all drinks and their associated ingredient amounts are loaded into a list of dictionaries, like so:

SampleDrinkDict = {'name': 'Screwdriver', 'id': 4, 'vodka':2, 'orange juice':6}
DrinkDictList = []
DrinkDictList.append(SampleDrinkDict)

Once the DrinkDictList is fully populated, I pass the names and IDs of every one to the client, which populates those in a list. When a drink is selected, it makes a callback to scan the DrinkDictList and find the matching 'id' field. Once it finds it in the list, the ingredient amounts and details are populated on the page.

Would it be faster/better/more proper for me to only query ingredients from the DB once a drink is selected?

For example, would it be faster/more pythonic/better to do this?

SampleDrinkDict = {'name': 'Screwdriver', 'id': 4,}
DrinkDictList = []
DrinkDictList.append(SampleDrinkDict)

@bottle.route('/drinkSelected/:id'):
def getIng(id,db):
    sqlString = '''SELECT ingredients WHERE Drink_id = ?'''
    argString = (id,)
    ingredientList = db.execute(SqlString, argString).fetchall()

This is all running on a Raspberry Pi, and I'm worried about taxing the RAM once the drink list becomes huge. Is there any reason to go with one approach over the other?

Whether you need to store data in the DB is really gonna depend on your testing, but here are a few thoughts about how to reduce memory usage while still building the entire drink table at startup:

While they're convenient, you don't necessarily need to use dictionaries for this.  You could replace each dictionary with a tuple where the first item is the drink name, the second item is the drink id, and the rest of the items are tuples of size 2 with the ingredient name and the ratio for that ingredient.  This would change your example to ('Screwdriver', '4', ('vodka', 2), ('orange juice', 6)).  sys.getsizeof gives 280 for the dictionary and only 88 for the tuple, a significant reduction.
If you're still pressed for space, you can take advantage of the fact that you probably have a lot of overlap in terms of your ingredients.  Build a list with a string for each ingredient that one of your drinks uses.  Then each tuple can simply have an int referring to the index of the ingredient in your ingredient list.  This prevents you from storing a string like 'vodka' 20 different times.  Once again using sys.getsizeof gives a value of 42 for the string 'vodka' and a value of 24 for the number 100.  This will of course only work if you have enough overlap in your ingredients.

If those ideas aren't enough to reduce your memory usage to an acceptable amount, you're probably going to need to go with your second solution(unless some other folks have some better ideas!).  I don't really see one of your solutions as more pythonic than the other, so I wouldn't worry about that.

And enjoy the raspberry pi!


Java Summation Program I'm new to Java so I have given myself small projects to help myself learn the ropes of java. My project was to create a function that would summate an expression no matter how high the degree or value of n.
I ended up being able to complete it but I wanted someone who knew what they were doing to look over it and see if I was doing things the right way.
A goal was to use no pre-built packages but I ended up using the java.lang.Math.pow() function (I didn't import it).
Also, what recommendations do you have for learning for beginners? I do have some experience programming but I feel like I never learned the correct way...
Code:
package TestSpace;
public class MathClass {

/*
    Steps:
    1. Get array of any length and n
    2. Reverse list so highest degree coefficients are last
    3. Run for loop for each time n increases
    4.Run for loop inside of that for each entry in array
*/

    //args consists of the coefficients of x; must include all degrees of x counting down from the highest degree
    public static float sigma (int[] args, int n) {
        float value = 0;
        int length = args.length;
        //reverses list
        int[] reverselist = reverselist(args);

        for(int i = 1;i<n+1;i++) {

            for(int v = 0; v < length;v++) {
                // Multiplies coefficient by "x" raised to the power of its degree
                value += reverselist[v] * java.lang.Math.pow(i, v);
            }
        }
        return value;
    }

//Reverses the order of the given array so that highest degree is at the end
    public static int[] reverselist(int[] args) {
        int length = args.length;
        int[] newint = new int[length];
        for(int i = 0;i<length;i++) {
            newint[i] = args[length - (i+1)];
        }
        return newint;
    }

//Makes printing simple
    public static void print(Object obj) {
        try {
            System.out.println(obj);
        }catch(Exception e){
            System.out.println(e);
        }
    }

    //main function
    public static void main(String[] args) {
        print(sigma(new int[] {5,2,7,4},4));
    }
}

Follow Java naming conventions.  Your package name should be all
lowercase - 'testspace'. 'reverselist' should be 'reverseList'.
Don't use fully qualified 'java.lang.Math', just 'Math'. You don't need to import classes from the java.lang package.
Indent your comments to match the code.
Use descriptive variable names, e.g. 'v' could be 'exponent' or
'power'.  Why use 'i' and then describe it as 'x' in the comment? Should 'reverselist' be 'reverseArray'?

In general your algorithms are reasonable.


Scrape a reddit post submission for comments and save comments to JSON file (revised) This is the revised code after incorporating the suggestions from user Roman Susi.
Link to the previous post here.

Notable edits: I have factored out the bigger functions into smaller tasks. The functions and variables have(hopefully) less ambiguous names, and the JSON object is now deepcop[ied].

I am still interested in making the code more Pythonic, factoring out the functions if need be, and wondering if my use of deepcopy is appropriate. I am open to any other suggestions.

Updated Code:

#!/usr/bin/python3
from copy import deepcopy
import json
import os
import praw

"""
Scrapes posts for comments and saves the comment ID and comment text to the
accompanying JSON file.
"""

JSON_DEFAULT_LOC = os.environ.get('JSON_LOCATION') or os.path.join(
    os.path.dirname(os.path.dirname(os.path.abspath(__file__))),
    "survey_response.json")

def login():
    """ Create a reddit instance according to praw.ini configuration.
    Call reddit.user.me() as a check.
    Return reddit instance.
    """
    print("Authenticating")
    reddit = praw.Reddit("appname", user_agent="Chrome:com.example.bot-"
                                                "name:v1 (by /u/)")
    print("Authenticated as {}".format(reddit.user.me()))
    return reddit

def load_survey_responses(fp=None):
    """ Load the current survey response file as a JSON object.
    """
    if fp is None:
        fp = JSON_DEFAULT_LOC
    with open(fp, "r") as file:
        survey_results = json.load(file)
    return survey_results

def scrape_submissions(reddit):
    """ For each post specified in survey_responses.json, scrape the comments
    within the post and write the new or updated responses to the JSON dict.
    """
    survey_responses = load_survey_responses()
    shortlink_id_list = extract_shortlink_id(survey_responses)

    for sl_id in shortlink_id_list:
        submission = reddit.submission(id=sl_id)
        try:
            updated_responses = prepare_survey_update(submission,
                                                      sl_id,
                                                      survey_responses)
            write_comments_to_file(updated_responses)
            print("Comments from '{}' "
                  "saved to file".format(submission.subreddit))
        except OSError as e:
            print("Could not write to file: {}".format(e))

def extract_shortlink_id(survey_responses):
    return [survey_responses[sub][0]["shortlink"][-6:]
            for sub in survey_responses]

def prepare_survey_update(submission, sl_id, survey_responses):
    """ Combs through a submission's comments and replaces the "more comments",
    extracts the comments from the submission, and prepares file to be written
    with an updated JSON dictionary.
    """
    submission_replaced = replace_more_comments(submission)
    submission_comment = extract_submission_comment(submission_replaced)
    updated = update_response(survey_responses, submission_comment, sl_id)
    return updated

def replace_more_comments(submission):
    submission.comments.replace_more(limit=0)
    return submission

def extract_submission_comment(submission_replaced):
    submission_comment = [{comment.id: comment.body}
                          for comment in submission_replaced.comments.list()]
    return submission_comment

def update_response(old_responses, comment_to_add, sl_id):
    new_responses = deepcopy(old_responses)
    for i in new_responses:
        if sl_id in new_responses[i][0]["shortlink"]:
            new_responses[i][1]["responses"] = comment_to_add
    return new_responses

def write_comments_to_file(updated_json, fp=None):
    if fp is None:
        fp = JSON_DEFAULT_LOC
    with open(fp, "w") as fp:
        json.dump(updated_json, fp)

def main():
    reddit = login()
    scrape_submissions(reddit)

if __name__ == "__main__":
    main()

This is much clearer now (hopefully, you also agree).

Some more ideas:

It is sometimes considered cleaner to always define functions before their use.

Some old comments still apply (magic number -6 and some namings), but there may be reasons for not changing that.

Code now almost reached the top of possibilities for non-object-oriented methodology, so I do not think it's worth crossing the line unless desired.

Using module-level or global data is usually frowned upon, because it makes functions to work magically (there are some exceptions of course). In case of module level configuration, one usually need to override the variable (eg, via monkeypatching). It's much easier for the function to accept the context, which may contain configuration. In the code above, load_survey_responses is the only violation I see, again, maybe it is justified. To see specifically what I mean, imaging that you will need to provide fp parameter as a script parameter: it will require changes to the code in several places instead of just providing fp upfront and propagating to further function calls as part of a context.

(Some readers may notice that prepare_survey_update now reminds
Transaction Script pattern, see https://stackoverflow.com/questions/16139941/transaction-script-is-antipattern discussion on it's pros and cons)

Now that code became cleaner, I've noticed the script reads and writes to the same file (now it overwrites the file with each call). It's ok as far as you do not do things concurrently. But it's another story.


Probability that Three Pieces Form a Triangle I wrote some code to estimate the solution to the following problem:

If you break a line segment at two random points, what is the probability that the new line segments can form a triangle?

The code is relatively simple, and it works. However, I can't shake the feeling that each of the three functions I've created could be written more "Pythonically." For example, the first function (can_form_triangle) explicitly writes out the combinations of pairs from (a, b, c) and then compares them to the element of (a, b, c) which is not in the pair. There must be a way to express this more succinctly, more elegantly — more Pythonically.
To be clear, I'd just like advice on refactoring the code to be more elegant/Pythonic. I am not looking for advice on how to change the actual functionality to solve the problem more efficiently.
Here's the code:
#!/usr/bin/python3

from random import random

def can_form_triangle(a, b, c):
    '''Determines if lengths a, b, and c can form a triangle.

    Args:
        a, b, c: Number representing the length of a side of a (potential) triangle

    Returns:
        True if all pairs from (a, b, c) sum to greater than the third element
        False otherwise
    '''
    ab = a + b
    ac = a + c
    bc = b + c

    if (ab > c and ac > b and bc > a):
        return True

    return False

def try_one_triangle():
    '''Simulates breaking a line segment at two random points and checks if the segments can form a triangle.

    Returns:
        True if the line segments formed by breaking a bigger line segment at two points can form a triangle
        False otherwise
    '''
    first_point = random()
    second_point = random()
    sorted_points = sorted((first_point, second_point))

    return can_form_triangle(sorted_points[0], sorted_points[1] - sorted_points[0], 1 - sorted_points[1])

def estimate_triangle_probability():
    num_success = num_attempts = 0
    for _ in range(10000000):
        num_success += 1 if try_one_triangle() else 0
        num_attempts += 1

    print('Success:', num_success)
    print('Attempts:', num_attempts)
    print('Ratio:', num_success / (num_attempts))

if __name__ == '__main__':
    estimate_triangle_probability()

Direct boolean return

As @Grajdeanu Alex says, this:

if (ab > c and ac > b and bc > a):
    return True

return False

can simply be

return ab > c and ac > b and bc > a

Type hints

def can_form_triangle(a, b, c):

can be

def can_form_triangle(a: float, b: float, c: float) -> bool:

Sort unpack

first_point = random()
second_point = random()
sorted_points = sorted((first_point, second_point))

return can_form_triangle(sorted_points[0], sorted_points[1] - sorted_points[0], 1 - sorted_points[1])

can be

    first_point, second_point = sorted((random(), random()))

    return can_form_triangle(first_point, second_point - first_point, 1 - second_point)

Digit triples

10000000 is more easily read as 10_000_000.

Attempt looping

num_attempts will evaluate to 10_000_000 so it's not worth tracking unless you add an early-exit mechanism.

The whole loop can be replaced with

num_success = sum(
    1
    for _ in range(10_000_000)
    if try_one_triangle()
)

Redundant parentheses

print('Ratio:', num_success / (num_attempts))

does not need the inner parens.


Using Choose and Apply Templates with Mode, vs Complexless Template Matches So I have a little XML (Sub)language,
it has 3 elements of interest: para, point, and code

If a code occurs inside a para, point element I want to handle it for inline use,
if not, i want to set it up as it's own display section

Currently I'm doing it like so:

  <xsl:template match="code">
    <xsl:choose>
      <xsl:when test="exists(ancestor::para) or exists(ancestor::point)">
        <xsl:apply-templates select="." mode="inline"/>
      </xsl:when>
      <xsl:otherwise>
        <xsl:apply-templates select="." mode="display"/>
      </xsl:otherwise>
    </xsl:choose>
  </xsl:template>

  <xsl:template match="code" mode="inline">
    <xsl:value-of select="."/>
  </xsl:template>

  <xsl:template match="code" mode="display">

<xsl:value-of select="."/>

  </xsl:template>

Instead of doing it like that i could have just two templates,
one for display and one for inline, and set up the match expressions to work out which it was.

Which is better?

It may not matter a lot for this case, but usually when dealing with XSLT our rule of thumb has been, always to handle different cases with different stylesheets rather than one huge complex XSLT.


In-place quicksort algorithm in Python 2 I have tried to implement an in-place quicksort Algorithm in Python 2. Please suggest how I can

Make the code more efficient
Increase readability
Improve writing style
Make it more Pythonic

def swap(x,y):
    return y,x

def sort(ar, start, end, l):

    if start<end-1:

        # pivot element is taken as the last element
        p = ar[end-1]

        ind=start-1

        # when a smaller no. than pivot is found, swap it with first bigger-tahn-pivot element.
        # keep index as the recently swapped smaller integer, if no smaller number found, than index is start-1 because pivot is out as first element

        for i in xrange(start,end-1):
            if ar[i]<p:
                ind=i
                for j in xrange(0,i):
                    if ar[j]>p:
                        ind=j
                        ar[i],ar[j] = swap(ar[i],ar[j])
                        break

        #Swap pivot with the first bigger-than-pivot number
        ar[ind+1], ar[end-1] = swap(ar[ind+1], p)

        ar = sort(ar, start, ind+1, l)
        ar = sort(ar, ind+2, end, l)

        return ar

    else:
        return ar

Your swap function is pointless. It would be far more clear just to switch the order directly in the right hand side of every assignation.

Also, in the second for loop. I'm not sure you're supposed to iterate from 0. At best it should be from start. Remember, the idea of quicksort is that you only modify the part of the array that corresponds to the function, [start, end) in this case. However, with that loop, you're now making O(n^2) iterations, which can be improved like this:

p = ar[end-1]
idx_le = start
idx_gt = end-2
while idx_le <= idx_gt:
    if ar[idx_le] <= p:
        idx_le += 1
    else:
        ar[idx_gt], ar[idx_le] = ar[idx_le], ar[idx_gt]
        idx_gt -= 1

ar[idx_le], ar[end-1] = ar[end-1], ar[idx_le]
return ar

The main idea is to keep track where the elements less than p should be placed, same with the elements greater than p. The new recursion should be [start, idx_le) [idx_le+1, end).


Positive and negative FizzBuzz I want to eliminate code duplication in my Fizz-Buzz like program. Any ideas?

#include <stdio.h>
int myseries(int n) {
  int i, cpt = 0;
  if (n < 0) {
    for (i = 0; i >= n; i--) {
      // if the number is multiple of both three and five
      if (i % 15 == 0) {
        printf("lancelot\n");
      }
      // if the number is multiple of 3
      else if(i % 3 == 0) {
        printf("Fizz\n");
      }
      // if the number is multiple of 5
      else if(i % 5 == 0) {
        printf("Buzz\n");
        cpt++;
      }
      else {
        printf("%d\n", i);
      }
    }
    return cpt;
  }
  else {
    for (i = 0; i <= n; i++) {
      // if the number is multiple of both three and five
      if (i % 15 == 0) {
        printf("lancelot\n");
      }
      // if the number is multiple of 3
      else if(i % 3 == 0) {
        printf("Fizz\n");
      }
      //if the number is multiple of 5
      else if(i % 5 == 0) {
        printf("Buzz\n");
        cpt++;
      }
      else {
        printf("%d\n",i);
      }
    }
    return cpt;
  }
}
//example
main() {
  printf("the number of buzz is : %d", myseries(-16));
}

Move common code to function

The first thing you could do to eliminate duplication is to take the common code and move it to a function.  In your case, the result would look like this:

// Returns 1 for buzz, 0 for any other.
int handleNum(int num)
{
    // if the number is multiple of both three and five
    if (num % 15 == 0) {
        printf("lancelot\n");
    }
    // if the number is multiple of 3
    else if(num % 3 == 0) {
        printf("Fizz\n");
    }
    // if the number is multiple of 5
    else if(num % 5 == 0) {
        printf("Buzz\n");
        return 1;
    }
    else {
        printf("%d\n", num);
    }
    return 0;
}

int myseries(int n) {
    int i, cpt = 0;
    if (n < 0) {
        for (i = 0; i >= n; i--) {
            cpt += handleNum(i);
        }
    }
    else {
        for (i = 0; i <= n; i++) {
            cpt += handleNum(i);
        }
    }
    return cpt;
}

Handle negative and positive alike

The next thing you could do is to combine the negative and positive cases into the same loop.  Both loops iterate the same number of times, just in different directions.  So you could just remember whether the original number was negative, and then iterate only in the positive direction, like this:

int myseries(int n) {
    int i, cpt = 0;
    int numberSign = (n < 0) ? -1 : 1;

    n *= numberSign;
    for (i = 0; i <= n; i++) {
        cpt += handleNum(i * numberSign);
    }
    return cpt;
}

Other things

Your indentation level of 2 is too small for my eyes.  I had to increase the indentation to 4 to properly distinguish the different levels.

When you print the result in main(), you should print a newline as well.  When I ran your program, my shell prompt was put at the end of your output.


Particle simulator using Python - Why is the NumPy version slower than pure Python? This is an interesting example from G.Lanaro's book, Python High Performance. The program is a simple simulator which describes movement of particles based on their positions and angular velocities (x, y, and ang_v attributes of a Particle class). The bottleneck of the program is the evolve method which is initially implemented using pure Python, and then the author tries to show how we can improve the performance of this specific method using NumPy operations.

My problem is that when I compare the run-time of NumPy version (evolve_numpy) versus the pure Python version (evolve), I can hardly see any improvement in terms of runtime; in fact, it seems like pure Python is a bit faster here.

I wrote all of this code myself, based off of what I learned in the book.

The initial version of evolve is as follows

    def evolve(self, dt):
        timestep = .00001
        nstep = int(dt / timestep)
        for i in range(nstep):
            for p in self.particles:
                t_x_ang = timestep * p.ang_v
                norm = (p.x ** 2 + p.y ** 2) ** .5
                p.x, p.y = (p.x - t_x_ang * p.y / norm, p.y + t_x_ang * p.x/norm)

            # repeat for all particles
        # repeat for all time-steps

Here is the NumPy version of the same method,

    def evolve_numpy(self, dt):
        timestep = 0.00001
        nsteps = int(dt / timestep)
        r_i = np.array([[p.x, p.y] for p in self.particles])
        ang_vel_i = np.array([p.ang_v for p in self.particles])
        for i in range(nsteps):
            norm_i = np.sqrt((r_i ** 2).sum(axis=1))
            v_i = r_i[:, [1, 0]]
            v_i[:, 0] *= -1
            v_i /= norm_i[:, np.newaxis]
            d_i = timestep * ang_vel_i[:, np.newaxis] * v_i
            r_i += d_i
            for i, p in enumerate(self.particles):
                p.x, p.y = r_i[i]

To test the runtime of both versions, a benchmark2 function is defined as follows:

def benchmark2(nparts, m):
    particles = [Particle(uniform(-1, 1), uniform(-1, 1), uniform(-1, 1))
                 for i in range(nparts)]
    sim = ParticleSimualtor(particles)
    if m == 1:
        sim.evolve(.1)
    elif m == 2:
        sim.evolve_numpy(.1)
    else:
        raise Exception('not a valid method')

You can access the complete code here: https://gist.github.com/OmidMiresmaeili/2891c4f1ce95f65cfab8ed0469e427d4

My results are as follows:

1min 40s for NumPy version and 1min 29s for Python version

But the author claim (and I understand the reasoning) that the NumPy version runs much faster than the initial version of evolve without any NumPy operations:

This is a figure from page 71 of the book. It shows run-time for different particle size (nparts argument in the benchmark2 function). The author then concludes:

  The plot shows that both the implementations scale linearly with particle size, but the runtime in the pure Python version grows much faster than the NumPy version; at greater sizes, we have a greater NumPy advantage. In general, when using NumPy, you should try to pack things into large arrays and group the calculations using the broadcasting feature.

Now I'm not sure if it is a typo in the book or maybe I am not understanding this correctly. I would appreciate any input you can give me.

Community wiki - update as posted by the OP in the question based on review by Graipher and AlexV

As Graipher and AlexV mentioned the problem with NumPy version was that the inner loop which updates particle positions (x and y) should be performed independently (not repeated in each timestep). The updated version is as follows,

def evolve_numpy(self, dt):
    timestep = 0.00001
    nsteps = int(dt / timestep)
    r_i = np.array([[p.x, p.y] for p in self.particles])
    ang_vel_i = np.array([p.ang_v for p in self.particles])
    for i in range(nsteps):
        norm_i = np.sqrt((r_i ** 2).sum(axis=1))
        v_i = r_i[:, [1, 0]]
        v_i[:, 0] *= -1
        v_i /= norm_i[:, np.newaxis]
        d_i = timestep * ang_vel_i[:, np.newaxis] * v_i
        r_i += d_i
    for i, p in enumerate(self.particles):
        p.x, p.y = r_i[i]

Doing this improves the results considerably (see the figure below) which shows that the results stated in the book were accurate and this was my mistake (as expected:)),

(The runtime of numpy method is now less than 4 seconds.)


Enabling or disabling a login view based on authentication state Is it good to have this:

if(DataHolder.i().getLoginParamInfo().equals(LoginState.stateLoggingIn)) {
    getView().findViewById(R.id.login_button).setEnabled(true);
}
else {
    getView().findViewById(R.id.login_button).setEnabled(false);
}

or this:

getView().findViewById(R.id.login_button).setEnabled(DataHolder.i().getLoginParamInfo().equals(LoginState.stateLoggingIn));

Short code vs longer code:

Writing shorter code can in certain situations be a good thing. But in your case I would not recommend this as it makes your code hard to read and/or maintain. Try something like this:

//I don't know the return-type of getLoginParamInfo(), hence the "LoginState".
LoginState parameterInfo = DataHolder.i().getLoginParamInfo();
boolean isLoggingIn = parameterInfo.equals(LoginState.stateLoggingIn);
getView().findViewById(R.id.login_button).setEnabled(isLoggingIn);

If you want, you can combine the first two lines into one. This gives following code:

boolean isEqual = DataHolder.i().getLoginParamInfo().equals(LoginState.stateLoggingIn);
getView().findViewById(R.id.login_button).setEnabled(isEqual);

But again, pumping al code in one line is certainly not a good practice.

Method naming:

Giving your functions/methods names like i() is definitly not good practice. It doesn't tell anything about what it does and what it eventually returns.


Converting an array-returning function to a lazy evaluator Take a look at the following function:

std::vector<double> get_data(
    std::vector<double> const &data,
    double const t,
    double const x)
{
    std::vector<double> result(data.size(), 0.0);

    double const
        A = func(t, x),
        B = func2(t, x);

    std::transform(std::begin(data) + 1, std::end(data), std::begin(result) + 1,
        [A, B](double const val){
            return A * val + B;
        });

    return result;
}

I am attempting to convert the function above into some sort of structure that computes the values of the result array lazily. First, I will present the code, then describe some rationale.

class data_obj {
    std::vector<double>
        m_data;

    double
        m_A , m_B;

public:
    data_obj(
        std::vector<double> data,
        double const t,
        double const x)
    : m_data(data)
    {
        set_values(t, x);
    }

    void set_values(double const t, double const x)
    {
        m_A = func(t, x);
        m_B = func2(t, x);
    }

    friend class obj_iterator;
};

class obj_iterator {
    data_obj const
        &m_obj;

    std::vector<double>::const_iterator
        m_data_iter, m_data_end;

    double
        m_val;

public:
    obj_iterator(data_obj const &obj)
    : m_obj(obj)
    , m_data_iter(m_obj.m_data.begin())
    , m_data_end(m_obj.m_data.end())
    , m_val(0) { }

    bool has_next() const {
        return m_data_iter != m_data_end;
    }

    void next() {
        ++m_data_begin;

        m_val = *m_data_iter * m_obj.m_A + m_obj.m_B;
    }

    double val() const {
        return m_val;
    }
};

Here are some points to explain the rationale/requirements:

t and x are independent parameters in an optimizer, hence the obj_iterator::set_values() method. On the other hand, data_obj::m_data is constant throughout.
I chose a reference member in obj_iterator::m_data because I want the lifetime of the iterator to be dependent on the lifetime of the object holding the data, as mentioned here.

Finally, here's an example usage of the classes created:

data_obj res_obj(src_data, t, x);

for(obj_iterator res_iter(res_obj); res_iter.has_next(); res_iter.next()) {
    std::cout << res_iter.val() << ", ";
}

Is my rationale flawed? How would you improve this implementation? As someone who programs in python, this could be easily achieved with generator functions.

You've done a lot of things well. Going the route of an iterator was a good choice, and you got the small details of const correctness and references and whatnot correct. There are, however, a few things I think can improved.

Naming

Your names are very undescriptive. In fact, it took me far longer than it should have to realize it's just modelling a linear function over a certain domain. Something like apply_linear for the returning version might be good, and something like linear_iterator might be good for the other one. Also, data_obj is very vague name. Pretty much all objects contain data of some kind. I might call it something like linear_function or something.

Structure

Something about your structure is a little off. I think instead of having obj_iterator take a data_obj for construction, I would have data_obj have begin() and end() methods that return a beginning iterator and an ending one. That way, you don't have to get into any friend stuff (right now data_obj is pretty useless to anything except the iterator since its members are private).

I'm also a bit uncomfortable with func and func2 being hardcoded. What if you want to change them? What if you want them to not be applied? It has essentially tightly coupled data_obj to func and func2, and worse yet, there's not really any reason for it. You can just have A and B be accepted for the values, and then you can apply func and func2 if you want.

That's essentially tightly coupling the linear function to those two functions. Further more, the coupling is super easy to get rid of: instead of taking t and x and applying func and func2, just accept a value that won't be further transformed.

In essence, I'm essentially suggesting that you generalize your thinking a bit farther. Currently you're modeling y(x) = m(t, z) * x + b(t, z). The problem is that m and b are constants, so there's no point in modeling them as part of the function. Instead, you can just model a typical linear function y(x) = m*x + b and take in m and b as constants (m = m(t, z), b = b(t, z)).

Idiomatic Iterator

Like I said earlier, it's good that you went with iterators for this. It's the best (built in) way in C++ to achieve a generator. It would have been a bit nicer if you'd done it more idiomatically though. In C++, iterators are used as pairs of iterators with a currently iterated iterator and an ending iterator. Also, operator++, operator* and operator== (or operator!=) are typically used instead of methods like next(), has_next(), etc.

auto fn = linear_function(2.5, 37.2, {...});
for (auto it = fn.begin(), end = fn.end(); it != end; ++it) {
    std::cout << *it << "\n";
}

A nice side effect of this is that you actually get rewarded for being idiomatic. The standard library becomes a lot more compatible with your stuff, and you can get range based for loops as a happy side effect:

// Wooo standard library!
auto fn = linear_function(2.5, 37.2, {...});
auto sum = std::accumulate(std::begin(fn), std::end(fn), 0.0);

// Wooo pretty loops!
for (auto y : linear_function(2.5, 37.2, {...})) {
    std::cout << *it << "\n";
}

Minor things

These are minor enough that I don't think any of them deserve their own section.

I would consider having data_obj work on a pair of iterators instead of being coupled to vector (what if you want to use it on an array, std::array, std::set, etc?).
I would considering make data_obj immutable.
data should be std::move'd m_data to ensure an extra copy is avoided.

Style

This is all subjective, of course, but:

I'm not a fan of the m_ prefix. I prefer either plain member names (e.g. data), or member names with a trailing underscore if method/member name conflicts are expected (e.g. data_). The m_ prefix is still relatively common, but it seems to be dying off, and personally, I can't wait for it to finish dying.
When function parameters can reasonably fit on one line, they should be on one line.
Indented variables per type is very non-standard. Standard is to have each variable on its own line with its own type declaration.

Putting it together

An example can probably illustrate much more effectively than my rambling words:

// Models a linear function
class linear_function {
private:
    const double m;
    const double b;

public:
    // Construct the function from slop intercept representation
    linear_function(double m, double b) : m(m), b(b)
    { }

    // Evaluate the linear function for a specific value.
    double operator()(double x) const { return m * x + b; }

    bool operator==(const linear_function& other) const {
        return m == other.m && b == other.b;
    }
};

// Iterates over a given domain for a linear function.
template<typename Iter>
class linear_function_iterator { // You'd want to extend or mimic std::iterator in a library-esque implementation
private:
    const linear_function fn_;
    Iter iter_;
    const Iter end_;

public:
    // Whether you want to take a single iterator or a pair is going to depend on typical usecase. Taking
    // a pair makes use a bit safer though since you can do checking to ensure proper use.
    linear_function_iterator(linear_function fn, Iter iter, Iter end)
      : fn_(std::move(fn)), iter_(iter), end_(end)
    { }

    double operator*() const {
        assert(iter_ != end_); // Example of using two iterators instead of one to provide additional error checking.
        return fn_(*iter_);
    }

    linear_function_iterator& operator++() {
        assert(iter_ != end_);
        ++iter_;
        return *this;
    }

    bool operator==(const linear_function_iterator& other) const {
        return fn_ == other.fn_ && iter_ == other.iter_;
    }

    bool operator!=(const linear_function_iterator& other) const {
        return !operator==(other);
    }
};

// Convenience class for a linear function over a given domain.
// Provides both an upfront approach or generator-like.
template<class Iter>
class fixed_linear_function { // This seems like a terrible name, but I'm blanking on anything better.
private:
    linear_function fn_;
    const Iter begin_;
    const Iter end_;
public:

    using iterator = linear_function_iterator<Iter>;

    fixed_linear_function(linear_function fn, Iter domain_begin, Iter domain_end)
      : fn_(std::move(fn)), begin_(domain_begin), end_(domain_end)
    {}

    std::vector<double> evaluate() const {
        std::vector<double> results;
        std::copy(begin(), end(), std::back_inserter(results));
        return results;
    }

    iterator begin() {
        return iterator(fn_, begin_, end_);
    }

    iterator end() {
        return iterator(fn_, end_, end_);
    }

};

// Because I'm lazy
template<typename Iter>
fixed_linear_function<Iter> make_fixed_linear_function(double m, double b, Iter beg, Iter end) {
    return fixed_linear_function<Iter>(linear_function(m, b), beg, end);
}

// Soooooo lazy
template<typename Container>
auto make_fixed_linear_function(double m, double b, const Container& c) -> fixed_linear_function<decltype(std::begin(c))> {
    return make_fixed_linear_function(m, b, std::begin(c), std::end(c));
}

This brings up an interesting note: generality and idiomaticness in C++ often come at a high cost, but also give high reward with regards to reusability and interoperability. This is significantly longer than your code, but it can be used with the standard library, and it's more flexible. In fact, if you were actually going to implement the idiomatic C++ approach like this, you could actually very easily take it one step farther and arrive at a generic generator. All that would be required would be renaming linear_function_iterator to generator, templating out linear_function to be any function (and also likely removing the end iterator since generators typically support infinite iteration), and deducing the return type of operator* instead of assuming its double.

Note: This is strictly an example implementation. Operators are only partially defined, template traits aren't present, and overall, this is missing a lot of the corner cases/optimizations/etc that would make it applicable for real use.

Example usage:

int main() {
    std::vector<double> values{1.0, 3.5, 27.2};
    for (auto y : make_fixed_linear_function(3, 2.5, values)) {
        std::cout << y << '\n';
    }

    auto fn = make_fixed_linear_function(3, 2.5, values);
    std::cout << std::accumulate(std::begin(fn), std::end(fn), 0) << '\n';
}

Generic generator

For toy, or small applications, it's not really going to matter, but if you find yourself doing this a lot in a real application and not some kind of school assignment or throwaway program, you'd likely want to find a well known and highly regarded generator implementation (I think Boost has a generator). It would save you time, it'd be more likely to be correct, and it'd likely be better designed than anything you or I can think of.


Web page that calculates results of four arithmetic operations for two numbers This is the code that I have written for a simple mathematical application using javascript. In accordance with the task, I broke the program into functions that perform the computations.
But I guess my code looks very large:

function addTwoN(a, b){
    return (a + b);
}
function subtractNumb(a,b){
    return (a - b);
}
function multiplNumb(a,b){
    return (a * b);
}
function divisNumb(a,b){
    return (a / b);
}
function myFunction(){
    const firstInput = document.getElementById("first-number").value;
    const secondInput = document.getElementById("second-number").value;
    const result = document.getElementById("result");
    var firstNumb = Number(firstInput);
    var secondNumb = Number(secondInput);
    var sum = addTwoN(firstNumb, secondNumb);
    var sbtr = subtractNumb(firstNumb, secondNumb);
    var multi = multiplNumb(firstNumb, secondNumb);
    var divis = divisNumb(firstNumb, secondNumb);
    var str =
    firstNumb + " + " + secondNumb + " = " + sum + "
" +
    firstNumb + " - " + secondNumb + " = " + sbtr + "
" +
    firstNumb + " * " + secondNumb + " = " + multi + "
" +
    firstNumb + " / " + secondNumb + " = " + divis;
    result.innerHTML = str;
}
<link href="https://fonts.googleapis.com/css?family=Open+Sans:400,700" rel="stylesheet">
<link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/4.0.0-alpha.6/css/bootstrap.min.css" integrity="sha384-rwoIResjU2yc3z8GV/NPeZWAv56rSmLldC3R/AZzGRnGxQQKnKkoFVhFQhNUwEyJ" crossorigin="anonymous">

<form class="ml-5 mb-5 mt-5">

     What is the first number?
     <input type="number" class="form-control" name="" id="first-number" oninput="validity.valid||(value=''); myFunction()" min="0" style="width: 90px">

     What is the second number?
     <input type="number" class="form-control" name="" oninput="validity.valid||(value=''); myFunction()" id="second-number" min="0" style="width: 90px">

</form>

One obvious simple change is to avoid making unnecessary variable assignments.  For example, just write

const a = Number(document.getElementById("first-number").value);

instead of

const firstInput = document.getElementById("first-number").value;
…
var firstNumb = Number(firstInput);

You should also be more consistent with naming (e.g. addTwoN vs. multiplNumb).  myFunction needs a better name; I suggest onInputUpdated.

The code could be simplified by treating each of the four operations as data.  Then, you apply the same transformation on each element of the operations list.

const operations = [
    {symbol: '+', callback: function(a, b) { return a + b }},
    {symbol: '-', callback: function(a, b) { return a - b }},
    {symbol: '*', callback: function(a, b) { return a * b }},
    {symbol: '/', callback: function(a, b) { return a / b }},
];

function onInputUpdated() {
    const a = Number(document.getElementById("first-number").value);
    const b = Number(document.getElementById("second-number").value);
    const result = operations.map(function(op) {
        return a + " " + op.symbol + " " + b + " = " + op.callback(a, b);
    }).join('
');
    document.getElementById("result").innerHTML = result;
}
<link href="https://fonts.googleapis.com/css?family=Open+Sans:400,700" rel="stylesheet">
<link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/4.0.0-alpha.6/css/bootstrap.min.css" integrity="sha384-rwoIResjU2yc3z8GV/NPeZWAv56rSmLldC3R/AZzGRnGxQQKnKkoFVhFQhNUwEyJ" crossorigin="anonymous">

<form class="ml-5 mb-5 mt-5">

     What is the first number?
     <input type="number" class="form-control" name="" id="first-number" oninput="validity.valid||(value=''); onInputUpdated()" min="0" style="width: 90px">

     What is the second number?
     <input type="number" class="form-control" name="" oninput="validity.valid||(value=''); onInputUpdated()" id="second-number" min="0" style="width: 90px">

</form>


Recursive component JSON configuration builder My goal here is to make data driven nested forms in React. I've created a function that points to a form configuration JSON, which can contain both components and references to other configuration files, with their own components. I'm splitting it to separate files to keep things DRY and to get an easier overview (hierarchy can be about 5 levels deep, one JSON file would be rather hard to overview).

I've introduced $ref to be able to point to another JSON configuration and $extends for inheritance (configurations can differ dependings on context but have common properties). args will be spread out as props to the component.

config-builder.js

function createConfiguration(configuration) {
  return _.reduce(configuration.components, (result, data) => {
    // if it's a reference to another configuration - fetch it - otherwise use as is
    const component = data.$ref ? getComponentByReference(data) : data
    return addResult(result, component.key, component.components ? { ...component, components: createConfiguration(component) } : component)
  }, {})
}

function getComponentByReference(data) {
  const component = _.merge({}, configMap[data.$ref], data)
  const result = component.$extends ? _.merge({}, configMap[component.$extends], component) : component
  // clear internal values like $ref, $extends
  return _.omitBy(result, (value, key) => _.startsWith(key, '$'))
}

// use an array to wrap results to handle multiple keys of the same type on same level
function addResult(result, key, data) {
  if(!result[key]) {
    result[key] = [data]
  } else {
    result[key].push(data)
  }
  return result
}

foo.json

{
  "key": "foo",
  "type": "DefaultContainer",
  "components": [
    { "key": "name", "type": "Text" },
    { "$ref": "Bar" }
  ]
}

bar.json

{
  "key": "bar",
  "type": "DefaultContainer",
  "components": [
    { "key": "id", "type": "DropDown", "args": { options: [] } }
  ]
}

https://jsfiddle.net/wkmrp4gc/

Good things

This code uses const for variables that don't get re-assigned.

The functions are concise, though some of the lines are a bit long due to ternary operators.

Suggestions

Bear in mind that functional programming is typically slower than imperative because each iteration leads to a function being added to the call stack. This would be noticeable with large data sets. Having the function addResult() seems like an excess step since it is only called in one spot. The lines to ensure the object at the given key is an array and push an element to that array could simply exist in the callback to the _.reduce() call.

Also, instead of either creating a new array with the data item or pushing it into an existing array, it could create an empty array when appropriate and then always push the data. This may be slightly less performant but requires fewer lines of code than storing the data in a temporary variable.

function createConfiguration(configuration) {
  return _.reduce(configuration.components, (result, data) => {
    const component = data.$ref ? getComponentByReference(data) : data;
    if(!result[component.key]) {
      result[component.key] = [];
    }
    result[component.key].push(component.components ? { ...component, components: createConfiguration(component) } : component)
    return result;
  }, {});
}

Unless you fully understand the ways Automatic semicolon insertion can be broken by certain statements, add semi-colons to terminate the lines.


forever.sh - should keep a process alive forever This script's only responsibility is to keep a process alive. Forever.
It's sole dependencies shall be bash and coreutils. I'm not sure if it would be compatible with sh, that would be even better.

#!/bin/bash
#
# (c) Netcetera AG
# Author:  Christian Mäder
date="10.07.2015"
version="1.0"
#

respawn() {
    while true; do
        $myprocess
        exit_code=$?

        now=`date +"%Y-%m-%dT%H:%M:%S%z"`

        if [ $exit_code -ne 0 ] || [ $force -eq 1 ]; then
            if [ $quite -eq 0 ]; then
                echo "$now | service $myprocess crashed with code $?. Restarting after ${timeout}s." >&2
            fi
            sleep $timeout
        else
            break
        fi
    done
}

help() {
    echo "Usage: $1 [-f] [-q] [-t #] \"command\""
    echo "       $1 -v"
    echo "       $1 -h"
    echo
    echo "  -v             : Shows the version of this tool."
    echo "  -h             : Shows this help text."
    echo "  -f             : Restart the program even if it exited with an exit"
    echo "                   status of 0."
    echo "  -q             : If set, this script won't write anything to stdout or stderr."
    echo "  -t #           : Seconds for how long the script should wait until"
    echo "                   the restart happend."
    echo "  command        : The command to execute continuously until it exits"
    echo "                   gracefully or forever if -f is set."
    echo
    echo "Example: $1 -f -t 2 \"echo hello, see you again in two.\""
}

about() {
    echo "Respawns a process which dies. Hence it keeps it runnig forever."
    echo "Version $version from $date."
}

while true; do
    case "$1" in
        "--help")
            help $0
            break
            ;;
        "-h")
            help $0
            break
            ;;
        "-v")
            about
            break
            ;;
        "-q")
            quite=1
            shift
            ;;
        "-f")
            force=1
            shift
            ;;
        "-t")
            if [[ $2 =~ '^[0-9]+$' ]]; then
                timeout=$2
                shift
            fi
            shift
            ;;
        *)
            if [ $# -eq 0 ]; then
                about
                echo
                help $0
            else
                quite=${quite-0}
                force=${force-0}
                timeout=${timeout-"1"}
                myprocess=$1
                respawn
            fi
            break
            ;;
    esac
done

For everyone's reference have I created a gist with an updated version which incorporated the feedback.

By quite, you mean quiet, I think.  You also have a typo with "runnig".

The script would be better self-documented if you put the about and help functions first.

In help, instead of echoing $1, which may be a path, it would be more conventional to echo just the basename.

It would be more elegant for the respawn function to accept the command as a parameter rather than using the $myprocess global variable.  "Process", to me, implies a particular instance, i.e. a running program with a PID, so I'd rename myprocess to command.  You only need to execute date if you're actually going to print a log message.  I'd rearrange the conditions to emphasize how to exit the loop and to reduce nesting.

respawn() {
    command="$1"
    while true; do
        $command
        exit_code=$?

        if [ $exit_code -eq 0 -a $force -ne 1 ]; then
            break
        elif [ $quiet -ne 1 ]; then
            echo "`date +"%Y-%m-%dT%H:%M:%S%z"` | service $command exited with code $?. Restarting after ${timeout}s." >&2
        fi
        sleep $timeout
    done
}

In the argument parser, the --help and the -h conditions are identical.  You should write

case "$1" in
    "--help"|"-h")
        help "$0"
        break
        …

Note that double-quoting variables is a good habit.

I don't believe that the [[ $2 =~ '^[0-9]+$' ]] test is available in traditional Bourne shell.  If portability is a concern, you could write the not-quite-equivalent test

case "$2" in
    [0-9]*) timeout=$2 ; shift
    ;;
esac
shift


Singly-Linked List In-Place Reversal I've created a function in C to reverse a singly-linked list in place:

typedef struct n {
    int key;
    struct n* next;
} node;

node* inplaceReverse(node* head, int index) {
    if(!head || !head->next) {
        /* Indication of last node (or empty list). This will be recursively returned to become the new head pointer of the reveresed list. */
        return head;
    }else {
        /* Reverse the list after head and store its return in tmp. */
        node* newhead = inplaceReverse(head->next,index+1);
        /* Make the next node's next point to head. */
        head->next->next = head;
        /* If head was the first node, make its next NULL. */
        if(!index) head->next = NULL;
        return newhead;
    }
}

It works but one thing that bothered me is that I needed to create an additional argument for the actual function (int index). This is to indicate the original head of the list so I know which one to set to NULL after the reverse. It feels like there's an obvious and better way to do this but I'm not sure.

Other suggestions for optimization and improvements are also welcomed.

My primary concern with this implementation is the fact that it is recursive and thus the amount of stack that you are consuming is directly proportional to the length of the linked list.  If the linked list is something like 10 nodes long, you are likely going to be fine.  However, if it is 1,000,000 nodes long then that is something that you can expect to blow the stack.

I would recommend rewriting the algorithm to not use recursion.  This can be done in the following manner.

node = list->head;
if (node is NULL) then there is nothing to do

for (next = node->next; next != NULL; next = node->next) {
    remove 'next' from the list

    /*
     * the act of removing 'next' from the list will automatically
     * update node->next.  Note that 'node' never changes.
     */

    add 'next' to the head of the list
}

Hope this helps.


Making a list of customers who buy the top N items from a supplier I have a method that works perfectly well and does what it's supposed to, albeit a little slow (30 - 40 seconds).  I would like to be able to speed it up or at least make it more efficient, any ideas please?

The purpose is to generate a list of which customers are buying the top requested (topN) lines from a supplier. The first part gets a list of products from the requested supplier, the second part works out which products are the best selling by distribution and then sales and then adds those products to an array and they form the column names for the datatable.  The last part loops all of the customer accounts and puts yes or no in the column if they have bought it in the last 3 months.  I hope that makes sense.

private void CustomersBuyingRange(string supplier, int topN)
{
    // Get a list of products from a supplier.
    string[] products = productDetails
                        .Where(x => x.SupplierID == supplier.ToUpper())
                        .Select(x => x.ProductCode).ToArray();

    // Rank the products by distribution and then by sales,
    // then add the required amount to an array.
    string[] topSellers = detailedOrderLines
                            .Where(x => products.Contains(x.ProductCode))
                            .GroupBy(x => x.ProductCode)
                            .Select(x => new
                            {
                                x.FirstOrDefault().ProductCode,
                                DeliveredQty = x.Sum(p => p.DeliveredQty),
                                Distribution = x.Select(c => c.CustomerID).Distinct().Count()
                            })
                            .OrderByDescending(x => x.Distribution)
                            .ThenByDescending(x => x.DeliveredQty)
                            .Take(topN)
                            .Select(a => a.ProductCode).ToArray();

    DataTable table = new DataTable("Customers");
    table.Columns.Add("CustomerID", typeof(string));
    table.Columns.Add("Customer", typeof(string));
    table.Columns.Add("AccountManager", typeof(string));
    // Add the columns with the products.
    foreach (string product in topSellers)
    {
        table.Columns.Add(product, typeof(string));
    }

    // We only want customers that have had an order in the last 3 months.
    var customers = customerDetails
        .Where(x => x.LastInvoiceDate > DateTime.Now.AddMonths(-3).Date)
        .OrderBy(x => x.CustomerName)
        .ToList();

    foreach (var customer in customers)
    {
        DataRow row = table.NewRow();
        row["CustomerID"] = customer.CustomerID;
        row["Customer"] = customer.CustomerName;
        row["AccountManager"] = customer.AccountManager;
        for (int i = 0; i < topN; i++)
        {
            row[topSellers[i]] = detailedOrderLines
                .Any(x => x.CustomerID == customer.CustomerID &&
                     x.ProductCode == topSellers[i] &&
                     x.InvoiceDate > DateTime.Now.AddMonths(-3).Date
                     ) ? "Yes" : "No";
        }

        table.Rows.Add(row);
    }
    Export.ExportToExcel(table, true);
}

There are roughly 3000 customers.
Any advise would be greatly appreciated.

As said in a comment this may be done better and faster with an SQL script depending on how the involved data sets are created, but below are some optimization suggestions.

You should maybe consider some input check:

private void CustomersBuyingRange(string supplier, int topN)
{
  if (string.IsNullOrWhiteSpace(supplier)) return;
  if (topN <= 0) throw new ArgumentOutOfRangeException(nameof(topN));

Optimizations:

1)

  // Get a list of products from a supplier.
  string[] products = productDetails
                      .Where(x => x.SupplierID == supplier.ToUpper())
                      .Select(x => x.ProductCode).ToArray();

supplier.ToUpper() is called repeatedly for every element in the vector. Consider to do it once:

  supplier = supplier.ToUpper();

  // Get a list of products from a supplier.
  var products = productDetails
                      .Where(x => x.SupplierID == supplier)
                      .Select(x => x.ProductCode);

As shown, there is also no need to populate an array with the result here.

2)

  var customers = customerDetails
      .Where(x => x.LastInvoiceDate > DateTime.Now.AddMonths(-3).Date)
      .OrderBy(x => x.CustomerName)
      .ToList();

Again you create a new DateTime object for each customer you evaluate in the Where() call. Consider to create that date once:

    DateTime minDate = DateTime.Now.AddMonths(-3).Date;
    // We only want customers that have had an order in the last 3 months.
    var customers = customerDetails
        .Where(x => x.LastInvoiceDate > minDate)
        .OrderBy(x => x.CustomerName);

You can then reuse it in the last loop too.

3)

    for (int i = 0; i < topN; i++)
    {
      row[topSellers[i]] = detailedOrderLines
          .Any(x => x.CustomerID == customer.CustomerID &&
               x.ProductCode == topSellers[i] &&
               x.InvoiceDate > DateTime.Now.AddMonths(-3).Date
               ) ? "Yes" : "No";
    }

Here you should be able to run a foreach loop instead:

      foreach (var topSeller in topSellers)
      {
        row[topSeller] = detailedOrderLines
            .Any(x => x.CustomerID == customer.CustomerID &&
                 x.ProductCode == topSeller &&
                 x.InvoiceDate > minDate) ? "Yes" : "No";
      }

4)

foreach (var customer in customers)
  {
    DataRow row = table.NewRow();
    row["CustomerID"] = customer.CustomerID;
    row["Customer"] = customer.CustomerName;
    row["AccountManager"] = customer.AccountManager;
    for (int i = 0; i < topN; i++)
    {
      row[topSellers[i]] = detailedOrderLines
          .Any(x => x.CustomerID == customer.CustomerID &&
               x.ProductCode == topSellers[i] &&
               x.InvoiceDate > DateTime.Now.AddMonths(-3).Date
               ) ? "Yes" : "No";
    }
    table.Rows.Add(row);
  }

Here you keep require the entire detailedOrderLines although you previously when creating the topSellers actually grouped those by ProductCode. So if you instead of selecting topSellers as strings select them as anonymous objects like:

  // Rank the products by distribution and then by sales,
  // then add the required amount to an array.
  var topProducts = detailedOrderLines
                          .Where(x => products.Contains(x.ProductCode))
                          .GroupBy(x => x.ProductCode)
                          .Select(x => new
                          {
                            ProductCode = x.FirstOrDefault().ProductCode,
                            DeliveredQty = x.Sum(p => p.DeliveredQty),
                            Distribution = x.Select(c => c.CustomerID).Distinct().Count(),
                            OrderLines = x.ToList()
                          })
                          .OrderByDescending(x => x.Distribution)
                          .ThenByDescending(x => x.DeliveredQty)
                          .Take(topN).ToList();

you only have to query the subset of orderLines that belongs to the current ProductCode:

    foreach (var customer in customers)
    {
      DataRow row = table.NewRow();
      row["CustomerID"] = customer.CustomerID;
      row["Customer"] = customer.CustomerName;
      row["AccountManager"] = customer.AccountManager;

      foreach (var topProduct in topProducts)
      {
        row[topProduct.ProductCode] = topProduct.OrderLines.Any(x => x.CustomerID == customer.CustomerID && x.InvoiceDate < minDate) ? "Yes" : "No";
      }

      table.Rows.Add(row);
    }

All in all the method could then be something like:

private void CustomersBuyingRange(string supplier, int topN)
{
  if (string.IsNullOrWhiteSpace(supplier)) return;
  if (topN <= 0) throw new ArgumentOutOfRangeException(nameof(topN));

  supplier = supplier.ToUpper();

  // Get a list of products from a supplier.
  var products = productDetails
                      .Where(x => x.SupplierID == supplier)
                      .Select(x => x.ProductCode);

  // Rank the products by distribution and then by sales,
  // then add the required amount to an array.
  var topProducts = detailedOrderLines
                          .Where(x => products.Contains(x.ProductCode))
                          .GroupBy(x => x.ProductCode)
                          .Select(x => new
                          {
                            ProductCode = x.FirstOrDefault().ProductCode,
                            DeliveredQty = x.Sum(p => p.DeliveredQty),
                            Distribution = x.Select(c => c.CustomerID).Distinct().Count(),
                            OrderLines = x.ToList()
                          })
                          .OrderByDescending(x => x.Distribution)
                          .ThenByDescending(x => x.DeliveredQty)
                          .Take(topN).ToList();

  using (DataTable table = new DataTable("Customers"))
  {
    table.Columns.Add("CustomerID", typeof(string));
    table.Columns.Add("Customer", typeof(string));
    table.Columns.Add("AccountManager", typeof(string));
    // Add the columns with the products.
    foreach (var topProduct in topProducts)
    {
      table.Columns.Add(topProduct.ProductCode, typeof(string));
    }

    DateTime minDate = DateTime.Now.AddMonths(-3).Date;
    // We only want customers that have had an order in the last 3 months.
    var customers = customerDetails
        .Where(x => x.LastInvoiceDate > minDate)
        .OrderBy(x => x.CustomerName);

    foreach (var customer in customers)
    {
      DataRow row = table.NewRow();
      row["CustomerID"] = customer.CustomerID;
      row["Customer"] = customer.CustomerName;
      row["AccountManager"] = customer.AccountManager;

      foreach (var topProduct in topProducts)
      {
        row[topProduct.ProductCode] = topProduct.OrderLines.Any(x => x.CustomerID == customer.CustomerID && x.InvoiceDate < minDate) ? "Yes" : "No";
      }

      table.Rows.Add(row);
    }

    Export.ExportToExcel(table, true);
  }
}

Disclaimer: I haven't had the opportunity to test the above, so don't hang me if it doesn't improve anything or if I have misunderstood something.


VBA macro which filters and splits dataset into new workbooks This VBA macro uses a worksheet (source_data_worksheet) to filter and split out the records based on about a dozen unique column (D) values in a dataset of about to separate workbooks which are saved to a folder.

There are over 10,000 records x 18 columns total with text and numbers (the overall file size of source worksheet is about 1.3MB). The number of records per unique value of filtering variable range from over 100 to over 5000. The source dataset is sorted by the variable used for filtering/splitting out.

In addition to creating the worksheet in the new workbook with the filtered subset of the data, the macro also copies two worksheets from the source workbook to the destination workbook next to the newly output subset worksheet.

One of the worksheets is very simple/small (SheetA), but another one (SheetB) has a table of about 100 rows by 9 columns which are using VLOOKUP and INDIRECT functions to cross-reference in data from the newly created subset worksheet based on some criteria.

The macros works and the data is populated as expected. It starts out fairly quick with the first subset (about 300 rows) populating in a few seconds.

However, things slow down substantially (10 minutes or longer for some of the larger subsets) and the number of output records itself does not seem proportionate to the longer time duration. The macro also seems to tax the system a lot as it nearly locks up Excel so that it's best to save and close all other worksheets before it runs. I am wondering if there is something in the code that reduces efficiency and causes undue system load during execution.

The output file formal is .xlsm (as opposed to .xlsx default) because some of the copied worksheets contain worksheet-level VBA that needs to be preserved in the new output files.

System: Office Pro 2010, 32-bit running on 64-bit Win i5 3Ghz 8GB.

    Option Explicit

Sub Create_by_keyrecord()

Dim r As Long, rng As Range, ws As Worksheet, wb As Workbook

Dim lastRow As Integer

Application.DisplayAlerts = False
Application.ScreenUpdating = False

With Sheets("Sheet1")
    Sheets.Add().Name = "temp"
    .Range("D7", .Range("D" & Rows.Count).End(xlUp)).AdvancedFilter Action:=xlFilterCopy, CopyToRange:=Sheets("temp").Range("D7"), Unique:=True
     For Each rng In Sheets("temp").Range("D7", Sheets("temp").Range("D7").End(xlDown))
        .AutoFilterMode = False
        .Range("D7").AutoFilter Field:=3, Criteria1:=rng
        Set ws = Sheets.Add

        lastRow = .Range("B7:I7").End(xlDown).Row
        .Range("B7:S" & lastRow).SpecialCells(xlCellTypeVisible).Copy
        ws.Range("B7").PasteSpecial xlPasteColumnWidths
        ws.Range("B7").PasteSpecial xlPasteAll
        .Range("B2:S6").Copy ws.Range("B2")
        Columns("A:A").ColumnWidth = 1

        For r = 1 To lastRow
            ws.Rows(r).RowHeight = .Rows(r).RowHeight
        Next r

        'ws.Name = rng
        ws.Name = "source_data_worksheet"
        ws.Move
        .AutoFilterMode = False
        Rows.Hidden = False
        Columns.Hidden = False
        ActiveWindow.DisplayGridlines = False
        Range("G7").Select
    ActiveWindow.FreezePanes = True

    Range("B6:S6").AutoFilter

'copy additional worksheets to new workbook
    Set wb = ActiveWorkbook
        Windows("sourseworksheet.xlsm").Activate
        Sheets("SheetA").Select
        Sheets("SheetA").Copy After:=wb.Sheets(1)
        wb.Sheets("SheetA").Visible = xlSheetHidden

        Windows("sourseworksheet.xlsm").Activate
        Sheets("SheetB").Select
        Sheets("SheetB").Copy After:=wb.Sheets(1)
'end new code'

        ActiveWorkbook.Protect Password:="password", Structure:=True, Windows:=True

        ActiveWorkbook.SaveAs _
        Filename:="C:\...\" & rng & "filename.xlsm ", _
        FileFormat:=xlOpenXMLWorkbookMacroEnabled

        ActiveWorkbook.Close 'SaveChanges:=True

    Next rng
      Sheets("temp").Delete

End With

Application.DisplayAlerts = True
Application.ScreenUpdating = True

End Sub

I agree with @RubberDuck about the RowHeight issue.  There are a couple ways to look at this.

Look at the Help in VBA for this:

  If you return the RowHeight property of several rows, you will either get the row height of each of the rows (if all the rows are the same height) or null (if they’re different heights).

So you can either write code to test if the RowHeight of your source = a number and then set the new sheet to the same value knowing that they are all the same. This is a bit fiddly maybe if you are new to VBA.

The other answer is to copy the EntireRow from your filtered data which will then copy the RowHeight for you. Is there anything beyond column S which you don't want copied? If not, then let's copy the entire row. It might even be quicker to copy the entire row and then Clear columns T:whatever.

Another point is that you are not touching Application.Calculation but you have a sheet that uses the INDIRECT function and you are doing a lot of filtering. I suspect this is the biggest issue. That function is Volatile which means Excel recalculates it even if it doesn't need to. This is because Excel cannot track which cells feed the function.

I suggest that you set Application.Calculation = xlCalculationManual at the top  of your code (like you do with ScreenUpdating) and then set it to Application.Calculation = xlCalculationAutomatic at the end. It is a good idea to store the original value before setting it to xlCalculationManual and then you can use that value to restore it. Here's the code:

Dim xlCalc As XlCalculation

' Save the original setting
xlCalc = Application.Calculation
' Put calcs to manual
Application.Calculation = xlCalculationManual
'
' ... do some intensive work
'

' Put calcs back to original setting
Application.Calculation = xlCalc

When you save a workbook with calculations set to xlCalculationManual the workbook remembers the setting which can then cause problems when you re-open the workbook because it doesn't re-calculate itself. So we should somehow keep track of your new workbooks, keep them open and save & close them after putting calculations back to their original setting. We can use the Collection object for this.

I've added some references to ThisWorkbook so that the code is referring to the correct workbook. Change these to Workbooks("book_name.xlsm") if it is not the workbook that is running the code.

This is what I think the changed code looks like. This includes copying the EntireRow so you'll need to revert back to your code if this doesn't fit your requirements. WARNING: I cannot be sure it will work correctly for your data, so use F8 to step through it and test it thoroughly.

Sub Create_by_keyrecord()

Dim r As Long, rng As Range, ws As Worksheet, wb As Workbook
Dim xlCalc As XlCalculation
Dim newBookNames As Collection
Dim bookName As Variant
Dim filePath As String
Dim newFileName As String
Dim lastRow As Integer

    Application.DisplayAlerts = False
    Application.ScreenUpdating = False

    ' Save the original setting
    xlCalc = Application.Calculation
    ' Put calcs to manual
    Application.Calculation = xlCalculationManual

    Set newBookNames = New Collection

    With ThisWorkbook.Sheets("Sheet1")
        ThisWorkbook.Sheets.Add().Name = "temp"
        .Range("D7", .Range("D" & Rows.Count).End(xlUp)).AdvancedFilter Action:=xlFilterCopy, CopyToRange:=Sheets("temp").Range("D7"), Unique:=True
         For Each rng In Sheets("temp").Range("D7", Sheets("temp").Range("D7").End(xlDown))
            .AutoFilterMode = False
            .Range("D7").AutoFilter Field:=3, Criteria1:=rng
            Set ws = ThisWorkbook.Sheets.Add

            lastRow = .Range("B7:I7").End(xlDown).Row
            .Range("B7:S" & lastRow).EntireRow.SpecialCells(xlCellTypeVisible).Copy
            ws.Range("A7").PasteSpecial xlPasteColumnWidths
            ws.Range("A7").PasteSpecial xlPasteAll
            .Range("B2:S6").EntireRow.Copy ws.Range("A2")

            ' Maybe empty column A?
            ws.Columns("A:A").ClearContents

            ws.Columns("A:A").ColumnWidth = 1

            ws.Name = "source_data_worksheet"
            ' Create the new workbook using a Workbook object variable
            ' It means we already have a reference to the new workbook later
            ' in the code
            Set wb = Workbooks.Add
            ws.Move Before:=wb.Worksheets(1)
            Set ws = wb.Worksheets(1)
            .AutoFilterMode = False
            ' Which workbook/sheet are these lines of code for?
            Rows.Hidden = False
            Columns.Hidden = False

            ' Make sure the new book & sheet are active
            wb.Activate
            ws.Activate
            ActiveWindow.DisplayGridlines = False
            ws.Range("G7").Select
            ActiveWindow.FreezePanes = True

            ws.Range("B6:S6").AutoFilter

        'copy additional worksheets to new workbook
            ThisWorkbook.Sheets("SheetA").Copy After:=wb.Sheets(1)
            wb.Sheets("SheetA").Visible = xlSheetHidden

            ThisWorkbook.Sheets("SheetB").Copy After:=wb.Sheets(1)
        'end new code'

            wb.Protect Password:="password", Structure:=True, Windows:=True

            filePath = "C:\...\"
            newFileName = rng & "filename.xlsm"
            ActiveWorkbook.SaveAs _
                Filename:=filePath & newFileName, _
                FileFormat:=xlOpenXMLWorkbookMacroEnabled

            newBookNames.Add newFileName
        Next rng

        ThisWorkbook.Sheets("temp").Delete

    End With

    ' This might take a while because everything will re-calculate
    Application.Calculation = xlCalculationAutomatic

    ' Now loop through the new book names
    ' and save each workbook
    For Each bookName In newBookNames
        Workbooks(CStr(bookName)).Save
        Workbooks(CStr(bookName)).Close False
    Next bookName

    Application.DisplayAlerts = True
    Application.ScreenUpdating = True

End Sub


Simple ActiveRecord attributes encryption in Rails 5+ The task at hand was to ensure that certain columns in the database are encrypted in case the database data is stolen. I have a feeling that a lot can be improved here (naming, DSL, encapsulations) while keeping the functionality as simple as it is. Let me know what you think, I'd appreciate any suggestions and ideas!

application_record.rb

class ApplicationRecord < ActiveRecord::Base
  extend Encryptable
  #...
end

encryptable.rb

module Encryptable
  def encrypt_columns(*columns)
    columns.each do |column|
      if self.column_names.include?(column.to_s)
        define_method "#{column}=" do |value|
          self[column] = self.class.crypt.encrypt_and_sign(value)
        end

        define_method "#{column}" do
          self.class.crypt.decrypt_and_verify(self[column])
        end
      end
    end
  end

  def crypt
    salt = '"\xCA)P\x9Dbc\xF7\x85B\xF3v}*p\xA6~\xBAR\xC1K\xE3\x88\xB8\x18\xF7\xD1\xB6e0\x98\xEF \xB9%\xB3\x12\x9C\xFE,\x89\xF4\xDE\xED!:7\xAE<q\xB1.<~\xA3m\e\x8C\t\xCF&/3\xCE\xAE"'
    key = ActiveSupport::KeyGenerator.new('password').generate_key(salt)

    @crypt ||= ActiveSupport::MessageEncryptor.new(key)
  end
end

user.rb

class User < ApplicationRecord
  encrypt_columns :email, :social_security_number
end

Some notes:

The recommended way to overwrite accessors is to use super.

As @Flambino says, it's better not to hardcode secrets directly in the code. It can be a file not committed to the repository.

I'd write the crypt method this way:
  def crypt
    @crypt ||= begin
      key = ActiveSupport::KeyGenerator.new('password').generate_key('salt')
      ActiveSupport::MessageEncryptor.new(key)
    end
  end


Minimum Area Rectangle in Swift This is my solution to LeetCode – Minimum Area Rectangle in Swift

939. Minimum Area Rectangle
Given a set of points in the xy-plane, determine the minimum area of a rectangle formed from these points, with sides parallel to the x and y axes.
If there isn't any rectangle, return 0.

Example 1:

Input: [[1,1],[1,3],[3,1],[3,3],[2,2]]
Output: 4

Example 2:

Input: [[1,1],[1,3],[3,1],[3,3],[4,1],[4,3]]
Output: 2

class Solution {
    struct Point_Dng: Hashable{
        var x: Int
        var y: Int

        init(_ x: Int, _ y: Int) {
            self.x = x
            self.y = y
        }

        var hashValue: Int{
            return x * 100000 + y
        }

        static func == (_ lhs: Point_Dng, _ rhs: Point_Dng) -> Bool{
            return lhs.x == rhs.x && lhs.y == rhs.y
        }
    }

    func minAreaRect(_ points: [[Int]]) -> Int {
        let points_new = points.map({ (point: [Int]) -> Point_Dng in
            return Point_Dng(point[0], point[1])
        })
        let set = Set(points_new)
        var ans = Int.max
        for point in points{

            for point_piece in points{

                if point[0] != point_piece[0] , point[1] != point_piece[1] , set.contains(Point_Dng(point[0], point_piece[1])) ,set.contains(Point_Dng(point_piece[0], point[1])) {

                    ans = min(ans, abs((point_piece[1] - point[1] ) * (point_piece[0] - point[0])))
                }

            }
        }

        if ans == Int.max {
            return 0
        }
        else{
            return ans
        }

    }
}

Note:

1 <= points.length <= 500

0 <= points[i][0] <= 40000

0 <= points[i][1] <= 40000

All points are distinct.

According to LeetCode's note, I improved the hash performance.
I turned
var hashValue: Int{
     return "\(x)\(y)".hashValue
}

into
var hashValue: Int{
     return x * 100000 + y
}

because Swift's tuple is not hashable.
The prior one will lead to “Time Limit Exceeded”
How can I improve it further?
In fact I want to know is there something I missed in Swift.
Something out of my knowledge.
Because I did it simple.

Naming

The meaning of some identifier names is hard to grasp:

What does Point_Dng stand for? Why not simply Point?
What is point_piece in the inner loop, and how is it different from
piece from the outer loop?
set is too generic, what does it contain?
ans stands for “answer,” but actually contains the “minimal area” found so far.

Simplifications

As of Swift 4.2, the compiler automatically creates the required methods
for Equatable and Hashable conformance for a struct if all its member
are Equatable/Hashable.

A struct also has a default memberwise initializer if you don't define your
own.

The properties of a point are never mutated, so they can be declared as constants  (with let).

This makes the struct Point as simple as

struct Point: Hashable {
    let x: Int
    let y: Int
}

The closure in

let points_new = points.map({ (point: [Int]) -> Point_Dng in
    return Point_Dng(point[0], point[1])
})

can be simplified because the compiler can infer the argument type and the
return type automatically. Since the array is only needed for creating the
set, the assignments can be combined into one:

let pointSet = Set(points.map { point in Point(x: point[0], y: point[1]) })

Performance improvements

In the nested loop it suffices to consider only those pairs where one point
is the “lower left” and the other the “upper right” corner of a potential
rectangle. That reduces the number of tests, and makes the abs() call
redundant.

Putting it together

The following version was roughly twice as fast in my tests with
random arrays of 500 points (on a 3.5 GHz Intel Core i5 iMac, compiled
in Release mode, i.e. with optimizations):

class Solution {
    struct Point: Hashable {
        let x: Int
        let y: Int
    }

    func minAreaRect(_ points: [[Int]]) -> Int {
        let pointSet = Set(points.map { point in Point(x: point[0], y: point[1]) })

        var minArea = Int.max
        for lowerLeft in points {
            for upperRight in points {
                if upperRight[0] > lowerLeft[0]
                    && upperRight[1] > lowerLeft[1]
                    && pointSet.contains(Point(x: lowerLeft[0], y: upperRight[1]))
                    && pointSet.contains(Point(x: upperRight[0], y: lowerLeft[1])) {

                    let area = (upperRight[0] - lowerLeft[0]) * (upperRight[1] - lowerLeft[1])
                    minArea = min(minArea, area)
                }
            }
        }

        return minArea == Int.max ? 0 : minArea
    }
}

Further suggestions

Sorting the point array in increasing order of x-coordinates would allow to
find “lower left/upper right” pairs faster, potentially increasing the
performance.


Plotting from a Pandas dataframe I want to improve my code. Is it possible to get the plot without repeating the same instructions multiple lines?

The data comes from a Pandas' dataframe, but I am only plotting the last column (Total Acc.)

e=df['Total Acc'].round(4)*100

#The repetitions start:
row_awa = e.loc['AWA']
row_rem = e.loc['REM']
row_s1 = e.loc['S1']
row_s2 = e.loc['S2']
row_sws = e.loc['SWS']
row_stades = e.loc['stades']

#More repetitions
row_awa.plot()
row_rem.plot()
row_s1.plot()
row_s2.plot()
row_sws.plot()
row_stades.plot()

myLegend=plt.legend(bbox_to_anchor=(0., 1.2, 1., .102), prop ={'size':10}, loc=10, ncol=4,  #left, bottom, width, height
            title=r'TOTAL ACCURACY FOR MANY K-FOLD')                    #loc='center'
myLegend.get_title().set_fontsize('20')

To reduce the repetitions you could make use of lists by doing something like:

labels = ['AWA', 'REM', 'S1', 'S2', 'SWS', 'stades']

rows = []
for label in labels:
    rows.append(e.loc[label])

# or even shorter with list comprehension
rows = [e.loc[label] for label in labels]

for row in rows:
    row.plot()

Edit: The labels list can also be used as first argument of the legend function to show the correct labels.