Reduce database touches for one to many data pull I have the following function which works properly and returns all of the rows from a specific table, then loops through each row to add data related to it as a subarray.
public function fetch_products() {
    $return = $this->db->all("SELECT * FROM consumables_products");

    foreach($return as &$r) {
        $r['consumables'] = $this->db->all("SELECT A.*, B.description FROM consumables_product_consumables as A, consumables_consumables as B WHERE product_id=? AND B.uid=A.consum_id", [$r['uid']]);
    }

    return $return;
}

The result looks like this:
Array
(
    [0] => Array
        (
            [uid] => 1
            [description] => Test
            [consumables] => Array
                (
                    [0] => Array
                        (
                            [uid] => 1
                            [consum_id] => 186
                            [product_id] => 1
                            [amount] => 5
                            [description] => 3 X 1" GREEN Direct Thermal
                        )

                    [1] => Array
                        (
                            [uid] => 2
                            [consum_id] => 185
                            [product_id] => 1
                            [amount] => 1
                            [description] => 3 X 1" ORANGE Direct Thermal
                        )

                )

        )

)

This could get very inefficient with hundreds or thousands of rows in my consumables_products table, and an undefined number of rows in the consumables_product_consumables table.
I'm wondering if there is a way that I can do this in a single query, so that I don't have to do a separate call for every row in my consumables_products table.

A single query could be used to get all results. This would require formatting the results differently...
The table consumables_products could be added as a (n INNER) JOIN.
JOIN consumables_products P on P.uid = A.product_id --or is it B. product_id instead of A.uid??

The implicit join on consumables_consumables could also be written in a similar fashion.
$results = $this->db->all("SELECT P.uid as productUid, P.description as productDesc, A.*, B.description
    FROM consumables_product_consumables as A
    JOIN consumables_consumables as B ON B.uid = A.consum_id
    JOIN consumables_products P on P.uid = A.product_id
");

From there array_reduce() (or else a foreach loop) could be used to return the data in the specified format:
Note this is untested code...
return array_values(array_reduce(function($carry, $row) {
    $key = $row['productUid'];
    if (!isset($carry[$key])) {
        $carry[$key] = [
            'uid' => $row['productUid'],
            'description' => $row['productDesc'],
            'consumables' => []
        ];
    }
    $carry[$key]['consumables'][] = array_diff_key($row, array_flip(['productUid', 'productDesc']));
    return $carry;
}, []));

That array_flip(['productUid', 'productDesc']) could be stored in a variable outside the loop though it would need to be brought in using a use statement or if PHP 7.4+ is used then an arrow function could be used, which would allow use of that variable without the need for a use statement.
e.g.
$keysToUnset = array_flip(['productUid', 'productDesc']);
return array_values(array_reduce(function($carry, $row)  {
    ...
    $carry[$key]['consumables'][] = array_diff_key($row, $keysToUnset);
    return $carry;
}, []));

Also consider if all fields from consumables_product_consumables are needed. If not, select only the fields needed in order to minimized the returned data set.


Coin Estimator by Weight I've created a mini project where the user will enter the total weight of the coins and the program will compute how many coins they have and how many wrappers they'll need. I initially found this challenge in a subreddit named r/beginnerprojects

def coinEstimator():

    ##########################################
    # Intro text                        ######
    ##########################################

    print("Welcome to the coin estimator.")
    print("Please enter the type of coins: ")
    print("1. Cent")
    print("2. Nickel")
    print("3. Dime")
    print("4. Quarter")

    ##############################################
    # Setting up variables for the user's input ##
    ##############################################

    userChoose = int(input("Enter: "))

    ##############################################################
    # Conditionals for different types of coin the user wants.  ##
    ##############################################################

    if userChoose == 1:             # Conditionals for Cents
        totalWeight = float(input("What is the total weight of your cents in grams?\n"))

        (cointValue, dimeWrapper) = cointEstSol(totalWeight, "Cent")

        print("The total cents you have is {0}. You need {1} wrapper(s)".format(cointValue, dimeWrapper))

    elif userChoose == 2:           # Conditionals for Nickel
        totalWeight = float(input("What is the total weight of your nickels in grams?\n"))

        (cointValue, dimeWrapper) = cointEstSol(totalWeight, "Nickel")

        print("The total nickels you have is {0}. You need {1} wrapper(s)".format(cointValue, dimeWrapper))

    elif userChoose == 3:           # Conditionals for Dime
        totalWeight = float(input("What is the total weight of your dimes in grams?\n"))

        (cointValue, dimeWrapper) = cointEstSol(totalWeight, "Dime")

        print("The total dimes you have is {0}. You need {1} wrapper(s)".format(cointValue, dimeWrapper))

    elif userChoose == 4:           # Conditionals for Quarter
        totalWeight = float(input("What is the total weight of your quarters in grams?\n"))

        (cointValue, dimeWrapper) = cointEstSol(totalWeight, "Quarter")

        print("The total quarters you have is {0}. You need {1} wrapper(s)".format(cointValue, dimeWrapper))

def cointEstSol(weight, coins):

    ########################################################
    ## Setting up variables for the solution of coins    ###
    ########################################################

    coinWeight = {"Cent": 126, "Nickel": 199, "Dime": 113, "Quarter": 226}     # Dictionary for the different weights of coins
    coinCountWrap = {"Cent": 50, "Nickel": 40, "Dime": 50, "Quarter": 40}      # Dictionary for the amount of coins that can be put in a wrapper

    cointValue = weight // coinWeight[coins]                                   # Solution to compute how many coins we have

    if cointValue < coinCountWrap[coins]:                                      # Conditionals for how many wrappers would be needed.
        coinWrapper = 1
    else:
        coinWrapper = cointValue // 50
        coinWrapper += 1

    return (cointValue, coinWrapper)                                           # Returning the total amount of coins and wrapper

coinEstimator()

Please tell me what areas i need to improve. As you can see my code is crude and was not beautifully written.

Honest review would be needed. Any shortcuts, codes that need amendments or just a way to improve this code would be appreciated.

Extra inquiry:
Although i don't think this would be the appropriate place to ask this questions but i want this to be made as object oriented programming approach. I don't know how, so if you guys can convert this code to OOP, that would be great. I wanted to know how python experts would approach this in an OOP style. It's okay if you don't answer this.

You got the right approach by using dictionaries and helper function (coinEstSol). You should go further and provide the same kind of approach to coinEstimator.

Furthermore, having informations split in several collections, such as your coinWeight and coinCountWrap is not ideal as it gets harder to extend them latter. Instead, you could work with triplets reprensenting a coin: name, weight and wrap. You could also use collections.namedtuple to make for better understanding of what these values are.

I would use a list to store these coin informations as it allows for automatic building of the menu:

import collections

Coin = collections.namedtuple('Coin', 'name weight wrap')

COINS = [
    Coin('Cent', 126, 50),
    Coin('Nickel', 199, 40),
    Coin('Dime', 113, 50),
    Coin('Quarter', 226, 40),
]

def estimate(coin):
     question = 'What is the total weight of your {}s in grams?\n'.format(coin.name)
     weight = float(input(question))
     value = weight // coin.weight
     wrapper = value // coin.wrap
     if value % coin.wrap:
         wrapper += 1
     return coin.name, value, wrapper

def main():
    print("Welcome to the coin estimator.")
    print("Please enter the type of coins: ")
    for index, coin in enumerate(COINS, 1):
        print(index, coin.name)

    user_choice = int(input('Enter: '))
    name, value, wrapper = estimate(COINS[user_choice - 1])
    print('The total', name, 'you have is', value, 'You need', wrapper, 'wrapper(s)')

Be aware, also, that your float and int calls can raise ValueError if the user doesn't enter a numeric value. You can also get an IndexError if the choice from the user is over the number of coins defined. Depending of your needs, you may want to take care of that.


Guess the Number(Python) I know this is kind of stuff is somewhat done to death but please still try to review it fairly.

My code will only go within the interval of [1,9]

def guessing_game():
    no_of_guess = 0
    c = "y"
    import random
    a = random.randint(1,9)
    while c == "y":
        b = input("Enter a guess: ")
        if b.isdigit():
            if int(b) == a:
                c = input("You got it! Play again? Enter \"Y\" if yes, and anything else to exit. ").lower()
                no_of_guess +=1
                a = random.randint(1,9)
            elif int(b) > a:
                c = input("Too high. Try again? Enter \"Y\" if yes, and anything else to exit. ").lower()
                no_of_guess +=1
            else:
                c = input("Too small. Try again? Enter \"Y\" if yes, and anything else to exit. ").lower()
                no_of_guess +=1
        else:
            c = input("Haha. Try again? Enter \"Y\" if yes, and anything else to exit. ").lower()
    if c != "y":
        print("How unfortunate. Well, you made %s guess(es)."%no_of_guess)

guessing_game()

A few minor things:

Imports should generally be at the module level, not inside functions.
Use more intention-revealing variable names. no_of_guess is borderline OK, but a, b and c don't tell me anything.
Think about control flow more carefully. For example:

you increment the guess counter for any numerical input, so you don't necessarily need to wait for the conditional checks; and
outside the loop do you really need to recheck whether the input wasn't "y"?

You can use single-quoted strings to avoid escaping double quote within them.
You can also reduce duplication by extracting functions and constants

With these suggestions addressed:

import random

RULES = 'Enter "Y" if yes, and anything else to exit. '

def win_prompt():
    return input("You got it! Play again? " + RULES).lower()

def lose_prompt(message):
    return input(message + " Try again? " + RULES).lower()

def guessing_game():
    number_of_guesses = 0
    target = random.randint(1,9)
    continuing = "y"
    while continuing == "y":
        guess = input("Enter a guess: ")
        if guess.isdigit():
            number_of_guesses += 1
            if int(guess) == target:
                continuing = win_prompt()
                target = random.randint(1,9)
            elif int(guess) > target:
                continuing = lose_prompt("Too high.")
            else:
                continuing = lose_prompt("Too small.")
        else:
            continuing = lose_prompt("Haha.")
    print("How unfortunate. Well, you made %s guess(es)." % number_of_guesses)

guessing_game()


Simulation Framework I started creating a small abstract simulation framework and would be happy if someone could take a look at the code and give me feedback on it.

The code can be found here, and it is currently not in a 100% finished and tested state.

It should be (almost) multi threading ready.

This is some sample code on how you can use the framework:

class Program
    {
        static void Main(string[] args)
        {
            Debug.Listeners.Add(new ConsoleTraceListener());

            var system = new SimulationSystem();

            // Add a thread and a simulation engine to the simulation framework
            system.AddSimulationThread(new SimulationThread(new SimulationEngine[] {new TestSimulationEngine() }.ToList()));

            // Add a new simulation group
            system.World.Objects.Add(new SimulationGroup());

            // Add a new object to the group
            ((SimulationGroup)system.World.Objects[0]).Objects.Add(new SimulationObject());
            system.World.Objects.Add(new SimulationObject());

            DateTime loopStart = DateTime.Now;
            DateTime loopEnd = DateTime.Now;

            // Simulates the update loop
            while (true)
            {
                loopStart = DateTime.Now;
                system.Update(loopStart - loopEnd);
                loopEnd = DateTime.Now;

                // Wait for user to press enter to start the next loop
                //Console.ReadLine();
            }
        }
    }

with this TestSimulationEngine class:

public class TestSimulationEngine : SimulationEngine
{
    public override void UpdateWorld(SimulationContext context, TimeSpan step)
    {
        // Do some stuff to the simulation world

        Console.WriteLine("UpdatingWorld with step {0}", step);

        // Add a new object each loop
        context.World.Objects.Add(new SimulationObject());
    }
}

A short introduction to the architecture behind the framework:

There is the class SimulationSystem which you can use to initialise a new simulated world. You can assign SimulationEngines to defined threads. For example a physicsengine to thread 1, a heatsystem to thread 2, ...

Also it holds the SimulationWorld which is a hierarchical system. Inside the World you can add SimulationObjects and SimulationGroups. A SimulationGroup is the same as a SimulationObject but can also have sub-objects.

For example, inside the world you can add a car as a simulation group. Inside the car you have the engine, driver, etc as simulation sub-objects.

You can also set any property you want on the objects using the code object["Property"].

First, a general consideration about your project. I cannot see any unit test. I'd strongly encourage you to write some, as I think that unit tests can help you coming up with a better design. I haven't looked at the code in details so unfortunately I don't have other comments on that.

Looking at the example main you posted I see you should try to rewrite your code so that you do dependency injection and in a way that you don't have to violate the law of Demeter to setup the simulation.

What about changing the constructor of SimulationSystem to SimulationSystem(IEnumerable<SimulationThread> simulationThreads, IEnumerable<SimulationGroup> simulationGroups)? If you do that you make it clear what you need to run your simulation and you can avoid doing the very ugly operations involving system.World.Objects.

I am not sure what you want to do with all that calls to DateTime.Now, but they don't look quite right to me. I think that the idea of having system.Update take a TimeSpan as a parameter is good, but I would not expect it to be called in that way. What about this:

var currentDateTime = /* compute it somehow */
var endDateTime = /* compute it somehow */
var simulationTimespan = /* compute it somehow */
while(currentDateTime < endDateTime)
{
    system.Update(simulationTimespan);
    currentDateTime = currentDateTime.Add(simulationTimespan);
}


Translation function - one word set to another I have this Excel VBA function that takes three parameters. It is essentially a translation mechanism with the following parameters:

A sentence which has to be translated.
A range of words to look for and what to translate them to.
Shows in which column the translation is in the range (usually it's 2).

This function works perfect with small 'vocabulary', but when the range gets big and many translations occur, my file can become so difficult it loads in 15 minutes. I have no ideas how to make this code simpler because I'm new to VBA. Could anyone give me advice?

Function FindSubStringAndReplaceRange(sentence As String, range As range, selected As Integer)
    Dim position As Integer
    Dim searchableLength As Integer
    Dim newSentence As String
    Dim intASCII As Integer

    Dim searchable As String
    Dim replaceWith As String

    FindSubStringAndReplaceRange = sentence

    For Each i In range.Rows
        If IsEmpty(i.Cells(1, 1)) = False Then

            searchable = i.Cells(1, 1)

            If IsEmpty(i.Cells(1, selected)) = False Then
                replaceWith = i.Cells(1, selected)
            Else
                replaceWith = vbNullString
            End If

            searchableLength = Len(searchable)
            position = InStr(UCase(FindSubStringAndReplaceRange), UCase(searchable))
            If position <> 0 Then
                intASCII = Asc(Right$(Left$(FindSubStringAndReplaceRange, position), 1))

                If intASCII >= 65 And intASCII <= 90 Then
                    replaceWith = UCase(Left$(replaceWith, 1)) + Right$(replaceWith, Len(replaceWith) - 1)
                End If

                FindSubStringAndReplaceRange = Left$(FindSubStringAndReplaceRange, position - 1) + replaceWith + Right$(FindSubStringAndReplaceRange, Len(FindSubStringAndReplaceRange) - (position + searchableLength - 1))

            End If
         End If
    Next

End Function

Best approach
If your vocabulary library is very large you need to implement another data structure and sorting algorithm for your vocabulary. I would go with Bucket Sort. You need to partition your strings into separate buckets of characters
to optimize. Comparing entire strings is not efficient.
Alternative approach
If you are lazy or prefer a different approach, utilize Microsoft Query (ACE.OLEDB - SQL) which is much more efficient than any VBA code. Dump your vocabulary to a single worksheet and execute an SQL to find your matches like so (for each sentence separately):

Split string to tokens and dump it to another worksheet DumpedTokens

Execute a query like this:

Find all matches in the Vocab worksheet. Keep tokens that don't have translations:
SELECT Iif(Vocab.Word = NULL, Dumped.Token, Vocab.TranslatedWord) FROM [DumpedTokens$] as Dumped
LEFT OUTER JOIN [VocabularySheet$] as Vocab
ON UCase(Vocab.Word) = UCase(Dumped.Token)

Join the strings back into sentences

This will probably run a couple of seconds.
If you want you can use my SQL AddIn to Excel which makes it easier to write SQL queries in Excel: http://www.analystcave.com/excel-tools/excel-sql-add-in-free/


All-Python implementation of n-dimensional convolution To better understand how discrete finite convolution works (read educational purposes) I wrote an all-python implementation of the convolution function. The idea was for it to give the same output as numpy.convolve, including the mode options. I generalized the code so that it functions for n-dimensional convolutions rather than just for 1-dimensional convolutions. This means it more closely matches the behavior of scipy.signal.convolve.

def py_nd_convolve(s, k, mode='full'):
    # All python implementation of n-dimensional scipy.signal.convolve for educational purposes

    # First ensure that one of the arrays fits inside of the other and by convention call this one
    # the kernel "k" and call the larger array the signal "s".
    if all(np.less_equal(k.shape, s.shape)):
        pass
    elif all(np.greater(k.shape, s.shape)):
        s, k = k.astype(float), s.astype(float)
    else:
        raise ValueError(f'Arrays have mismatched shapes: {k.shape} and {s.shape}')

    dim_list = np.arange(s.ndim)
    n_s = np.array(s.shape, dtype=int)
    n_k = np.array(k.shape, dtype=int)
    n_full = n_s + n_k - 1
    out = np.zeros(n_full, dtype=float)
    zero_arr = np.zeros(s.ndim, dtype=int)
    for ind, val in np.ndenumerate(out):
        ind_arr = np.array(ind)  # convert index tuple to numpy array so it can be used for arithmetic

        # i_min and i_max specify the allowed range which indices can take on in each dimension
        # to perform the convolution summation. The values for these are non-trivial near the
        # boundaries of the output array.
        i_min = np.maximum(zero_arr, ind_arr - n_k + 1)
        i_max = np.minimum(ind_arr, n_s - 1)

        # create a d-dimensional tuple of slices to slice both the kernel and signal to perform the convolution.
        s_slicer = tuple([slice(i_min[d], i_max[d] + 1) for d in dim_list])
        k_slicer = tuple([slice(ind[d]-i_max[d], ind[d]-i_min[d]+1) for d in dim_list])

        # element-wise product and summation of subset of signal and reversed kernel
        out[tuple(ind)] = np.multiply(s[s_slicer], np.flip(k[k_slicer])).sum()

    # This section of code clips the output data as per user specification to manage boundary effects
    n_min = np.zeros_like(n_full)
    n_max = n_full
    if mode == 'full':  # take all possible output points, maximal boundary effects
        pass
    elif mode == 'valid':  # eliminate any part of the output which is effect by boundary effects
        n_min = n_k-1
        n_max = n_s
    elif mode == 'same':  # clip the output so that it has the same shape as s
        n_min = (n_k-1)/2
        n_max = n_s + (n_k-1)/2
    slicer = [slice(int(n_min[d]), int(n_max[d])) for d in dim_list]  # d-dimensional slice tuple for clipping
    return out[tuple(slicer)]

Since this code was for educational purposes I did not desire it to run fast, I namely desired for it to be very readable. As written the code is very very slow. While scipy.signal.convolve can convolve a (200, 200) array with a (50, 50) array in a few milliseconds this code takes a few seconds.

I am not interested in making this code as fast as possible because I know that for that I could simply use the built in functions. I also know that I could possibly perform this operation faster by using an fft approach.

Question

Rather, I am curious if there are any glaring performance issues with the code that can be easily modified so that I can maintain a similar level of readability and logical flow but a higher level of performance.

For example, am I making an egregious numpy mistakes such as copying arrays unnecessarily or misusing memory in a very bad way?

I've noticed that I could flip the k array outside of the loop but it doesn't look like this will save much time.
It looks like a big chunk of time is spent on defining s_slicer and k_slicer.

Specific suggestions:

All t abbr mks th c hard 2 rd. Plz repl abbrs w/full wrds. Think of variable names as comments – they don't matter at all to the computer, but they do matter to anybody reading the code. A good rule of thumb is that if any of your comments explain what is coming up rather than why it was done like that, you can probably replace the comment with more elegant code. I've been working over a year with some colleagues on a medium-size new system, and we've taken this approach to heart: about 0.5% of the lines in Python files have comments on them.
Conversions often take a lot of time, especially on a lot of items, and even more especially in a loop. Working with only lists or only tuples might speed up things a bit. In terms of readability, tuples are generally used when there is a fixed number of items, often of different types. Neither of these seem to apply to this code.
The contents of the function can probably be split into at least three more for readability:

validation,
mode handling, and
the loop contents.

The modes could be related as Enum values:

class Mode(Enum):
    FULL = "full"
    VALID = "valid"
    SAME = "same"

def …(…, mode: Mode = Mode.FULL):

General suggestions:

black can automatically format your code to be more idiomatic.
isort can group and sort your imports automatically.
flake8 with a strict complexity limit will give you more hints to write idiomatic Python:

[flake8]
max-complexity = 4
ignore = W503,E203

I would then recommend adding type hints everywhere and validating them using a strict mypy configuration:

[mypy]
check_untyped_defs = true
disallow_untyped_defs = true
ignore_missing_imports = true
no_implicit_optional = true
warn_redundant_casts = true
warn_return_any = true
warn_unused_ignores = true


Removing all delegates from an event dispatcher Given a class which is an event dispatcher whose underlying store is a dictionary of events whose key is the event type private readonly Dictionary<Type, Delegate> _applicationEventHandlers;, is this the correct way to remove all remaining delegates when the class needs to be disposed?

    private void RemoveAllListeners()
    {
        foreach (Type handlerType in _applicationEventHandlers.Keys)
        {
            Delegate handlers = _applicationEventHandlers[handlerType];
            Delegate.RemoveAll(handlers, handlers);
        }
    }

Or is there a better way? I have included the whole class below for clarity.

using System;
using System.Collections.Generic;

internal class ApplicationEventDispatcher : IApplicationEventDispatcher, IDisposable
{
    private bool _disposed;
    private readonly Dictionary<Type, Delegate> _applicationEventHandlers;

    public ApplicationEventDispatcher()
    {
        _applicationEventHandlers = new Dictionary<Type, Delegate>();
    }

    ~ApplicationEventDispatcher()
    {
        Dispose(false);
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    private void Dispose(bool disposing)
    {
        if (_disposed) return;

        if (disposing)
        {
            // free other managed objects that implement IDisposable only
            RemoveAllListeners();
        }

        // release any unmanaged objects
        // set the object references to null

        _disposed = true;
    }

    public void AddListener<T>(ApplicationEventHandlerDelegate<T> handler) where T : IApplicationEvent
    {
        if (_applicationEventHandlers.ContainsKey(typeof(T)))
        {
            Delegate handlersForType = _applicationEventHandlers[typeof(T)];
            _applicationEventHandlers[typeof(T)] = Delegate.Combine(handlersForType, handler);
        }
        else
        {
            _applicationEventHandlers[typeof(T)] = handler;
        }
    }

    public void RemoveListener<T>(ApplicationEventHandlerDelegate<T> handler) where T : IApplicationEvent
    {
        if (_applicationEventHandlers.ContainsKey(typeof(T)))
        {
            var handlerToRemove = Delegate.Remove(_applicationEventHandlers[typeof(T)], handler);
            if (handlerToRemove == null)
            {
                _applicationEventHandlers.Remove(typeof(T));
            }
            else
            {
                _applicationEventHandlers[typeof(T)] = handlerToRemove;
            }
        }
    }

    private void RemoveAllListeners()
    {
        foreach (Type handlerType in _applicationEventHandlers.Keys)
        {
            Delegate handlers = _applicationEventHandlers[handlerType];
            Delegate.RemoveAll(handlers, handlers);
        }
    }

    public void Dispatch(IApplicationEvent @event)
    {
        if (@event == null)
        {
            return;
        }

        if (_applicationEventHandlers.ContainsKey(@event.GetType()))
        {
            _applicationEventHandlers[@event.GetType()].DynamicInvoke(@event);
        }
    }
}

Delegate.RemoveAll has no effect on existing delegates. It actually constructs a new one. Delegates are immutable and cannot be changed after construction.

You can remove this line. I would not implement IDisposable or any cleanup code here at all. Just release reference on event dispatcher (it makes sense to do it anyway after calling Dispose().


Prime reversion algorithm - JavaScript I solved the following problem:

Consider the range 0 to 10. The primes in this range are: 2, 3, 5, 7, and thus the prime pairs are: (2,2), (2,3), (2,5), (2,7), (3,3), (3,5), (3,7),(5,5), (5,7), (7,7).

Let's take one pair (2,7) as an example and get the product, then sum the digits of the result as follows: 2 * 7 = 14, and 1 + 4 = 5. We see that 5 is a prime number. Similarly, for the pair (7,7), we get: 7 * 7 = 49, and 4 + 9 = 13, which is a prime number.

You will be given a range and your task is to return the number of pairs that revert to prime as shown above. In the range (0,10), there are only 4 prime pairs that end up being primes in a similar way: (2,7), (3,7), (5,5), (7,7). Therefore, solve(0,10) = 4)

Note that the upperbound of the range will not exceed 10000. A range of (0,10) means that: 0 <= n < 10.

I was wondering if my solution could be improved or if I should use a completely different approach.

function isPrime(n) {
    if(n < 2){
        return false;
    }
    for (var i = 2; i <= parseInt(Math.sqrt(n)); i++) {
        if (n % i === 0) {
            return false;
        }
    }
    return true;
}

function getPrimes(s, e) {
    var primes = [];
    for (var p = s; p <= e; p++) {
        if(isPrime(p)){
            primes.push(p);
        }
    }
    return primes;
}

function generatePairs(primes){
    var pairs = [];
    for(var i = 0; i < primes.length; i++){
        for(var j = i; j < primes.length; j++){
            pairs.push([primes[i], primes[j]]);
        }
    }
    return pairs;
}

function sumDigits(n){
    var sum = 0;
    while(n > 0){
        sum += n % 10;
        n = parseInt(n/10);
    }
    return sum;
}

function solve(a, b) {
    var pairs = generatePairs(getPrimes(a, b - 1));
    var res = 0;
    for(pair of pairs){
        var tmp = sumDigits(pair[0] * pair[1]);
        if(isPrime(tmp)){
            res++;
        }

    }
    return res;
}

Your code looks good, just a few small things I would change. First, I didn't know where to start reading your code, since the function you would start with is at the bottom, and your example doesn't actually call it.

parseInt is used to convert a string to a number. If you just want to floor a number, use Math.floor instead. It is much faster.

In the isPrime function, every iteration of the loop calculates the same square root, which is not a simple calculation. Store the square root in a variable before the loop, and compare to that.

The getPrimes function works fine, but has potential for optimization, based on what we know about primes. For instance, with exception of 2, every primes is odd, so you only need to check every second number. You could also look into other methods, like a prime sieve.

There is no reason to still is var. Use let and const.


Dynamically adding controls to a form in a WinForms project I have created this function in my DetailsScreen.cs, but I have no idea if this is the correct file to put this kind of code in, or if coding it this way is the correct approach. I know that it works because I have tested it.

My application generates a questionnaire based on selected (cyber security) standards. I have a form DetailsScreen, which requires the user to fill in some details such as name, company, etc. They also need to select from a checkedlist (questionStandardInput), the question standard they wish to use in this interview. For each selected standard, two answer standards need to be selected from two drop down lists (maturity answer standard and compliance answers standard).

Therefore, I need to be able to dynamically add labels and comboxes for each question standard. I asked because most code concerning label font etc. is placed in .Designer.cs files instead of the .cs files. But I need certain logic from the .cs file in order to properly format the controls, which is why I put it in here.

// Load answer input according to the number of questions standards selected.
private void questionStandardInput_MouseUp(object sender, MouseEventArgs e)
{
    questionLabels.Clear();
    maturityInput.Clear();
    complianceInput.Clear();
    selectionPanel.Controls.Clear();

    foreach (var item in questionStandardInput.CheckedItems)
    {
        Label qs_label = new Label();
        qs_label.Location = new Point(0, 0 + (questionLabels.Count * 115));
        qs_label.Font = new Font("Arial", 12F, FontStyle.Regular);
        qs_label.Size = new Size(150, 18);
        qs_label.Text = item.ToString();
        questionLabels.Add(qs_label);
        this.selectionPanel.Controls.Add(qs_label);

        Label m_label = new Label();
        m_label.Location = new Point(0, 0 + (questionLabels.Count * 115 + 35));
        m_label.Font = new Font("Arial", 12F, FontStyle.Regular);
        m_label.Size = new Size(150, 18);
        m_label.Text = "Maturity standard";
        this.selectionPanel.Controls.Add(m_label);

        Label c_label = new Label();
        c_label.Location = new Point(0, 0 + (questionLabels.Count * 115 + 70));
        c_label.Font = new Font("Arial", 12F, FontStyle.Regular);
        c_label.Size = new Size(170, 18);
        c_label.Text = "Compliance standard";
        this.selectionPanel.Controls.Add(c_label);

        ComboBox m_input = new ComboBox();
        m_input.Location = new Point(170, 0 + (questionLabels.Count * 115 + 35));
        m_input.Font = new Font("Arial", 12F, FontStyle.Regular);
        m_input.Size = new Size(200, 22);
        m_input.DropDownStyle = ComboBoxStyle.DropDownList;
        maturityInput.Add(m_input);
        this.selectionPanel.Controls.Add(m_input);

        ComboBox c_input = new ComboBox();
        c_input.Location = new Point(170, 0 + (questionLabels.Count * 115 + 70));
        c_input.Font = new Font("Arial", 12F, FontStyle.Regular);
        c_input.Size = new Size(200, 22);
        c_input.DropDownStyle = ComboBoxStyle.DropDownList;
        complianceInput.Add(c_input);
        this.selectionPanel.Controls.Add(c_input);

        InitializeMaturityStandardInput(m_input);
        InitializeComplianceStandardInput(c_input);
    }
    this.saveAssessmentButton.Location = new Point(235, 0 + (questionLabels.Count * 115));
    this.selectionPanel.Controls.Add(saveAssessmentButton);
}

In my eyes it is an appropriated way to build up your form that way. The *.Designer file is for designer generated code - but if you need to add custom logic to modify your GUI the *.cs file is the correct location.

Just a few points:

There are some numbers (115, 70, 35, ...) that seems to have a special meening (e.g. row height, margin, whatever). IMHO it makes sense to store that numbers as constants with a meaningful name and use that constants instead.
I am not sure if the MouseUp event is the right place to trigger that kind of logic. If there is another event (CheckBoxChanged, SelectedItemChanged, ...) I would suggest to use that.
There is some repeating logic for creating the ComboBox / Label and so on. That logic could be extracted to a separted method. For instance:

private static ComboBox CreateComboBox()
{
    return new ComboBox()
    {
        Font = new Font("Arial", 12F, FontStyle.Regular)
        Size = new Size(200, 22)
        DropDownStyle = ComboBoxStyle.DropDownList
    }
}


Logging and getting the response in a controller I have an action that does two things:

It sends a chain of commands, and gets the response from an external API that we call wrapper.
It logs the chain of commands and the tunnel (look at tunnels as different machines) the command was sent to.

My problem is the contents of the if statement. The two things (sending the command/getting the response, and logging the exchange) are happening in that if block, and they both need the chain and tunnel variables. I'd ideally want to have two functions, getResponseFromWrapper() and logCommandAndResponse(), effectively reducing the number of statements inside my if block to just two.

public function indexAction(Request $request)
{
    $response = '';
    $form = $this->createForm(new TerminalType());
    $form->handleRequest($request);

    if ($form->isValid()) {
        $data = $form->getData();
        $tunnel = $data['tunnel'];
        $chain = $data['chain'];

        $response = $this->retrieveResponseFromWrapper($chain, $tunnel);
        $this->get('command_logger')->log($chain, $tunnel, $response);
    }

    return $this->render('terminal/index.html.twig', [
        'title' => 'Terminal',
        'form' => $form->createView(),
        'response' => $response
    ]);
}

In other words, I feel like the three first lines in the if block are making it a bit more difficult for the reader to know what's happening.

Better code might be:

public function indexAction(Request $request)
{
    $response = '';
    $form = $this->createForm(new TerminalType());
    $form->handleRequest($request);

    if ($form->isValid()) {
        list($chain, $tunnel) = $this->extractChainAndTunnelFromForm($form);
        $response = $this->retrieveResponseFromWrapper($chain, $tunnel);
        $this->get('command_logger')->log($chain, $tunnel, $response);
    }

    return $this->render('terminal/index.html.twig', [
        'title' => 'Terminal',
        'form' => $form->createView(),
        'response' => $response
    ]);
}

This is much shorter. However, returning two values from a function leaves a bad taste in my mouth.

How can I improve the first code block and reduce the number of statements in the if block? Do I need to create something like a DTO (i.e. have my extractCommandAndTunnelFromForm() create a plain old PHP object and set the tunnel and chain as its properties)? Do I need to inject the commandLogger service into the service that sends commands to the wrapper, and remove logging from the controller entirely?

First of all, the first code is perfectly fine in matter of conciseness and readability. You're being a lil bit obsessive, but it's not perfect in matter of  design.

Second, if I really had to do so to just improve the readability, I would refactor retrieveResponseFromWrapper() to accept a, let's say, WrapperRequest instance. On the other hand, I would refactor extractChainAndTunnelFromForm() to a createWrapperRequestFromForm() which obviously returns a WrapperRequest instance after its own internal processing of form data:

if ($form->isValid()) {
    $request  = $this->createWrapperRequestFromForm($form);
    $response = $this->retrieveResponseFromWrapper($request);

    // If you wanna go shorter for the sake of sanity:
    //
    // $response = $this->retrieveResponseFromWrapper(
    //     $request = $this->createWrapperRequestFromForm($form)
    // );

    // I also recommend to move this out, controllers just log access things
    $this->get('command_logger')
         ->log($request->getChain(), $request->getTunnel(), $response);
}

Now that I wrote it, you see, it's shorter and more unreadable! IMO shorter code and minimal number of methods are not necessarily an indication of more concise and readable code. The number of statements in a if block is not an effective factor to measure readability.

I remember when I was trying to find one-liners for every damn single thing I wrote. I went insane and even swapped ifs with equivalent AND/OR statements. It was of course a pleasure but I've learnt to embrace simplicity and verbosity in order to be more readable for others.

Back to the design thing; The above code is a better code, not because it's shorter or it has just one return value, but because it separates your domain logic apart from the controller and it avoids implementation leaks. This controller does not know about $tunnel and $chain, it just transports them as blackbox data. That's why we often need Data Validation components separated from the controller and the Form Validation component.

I think that would be a better idea to refactor our controllers as just a Transport Layer around HTTP instead of such minor improvements. To pursue this I always think that my app needs be fully functional when the data is fed through terminal instead of HTTP, without regular forms.

To conclude; A reader must always tend to read the code in Service/Repo/Model/... classes in order to understand the business logic. Reading the controller code of a well-designed application must be boring cause it's just the means not the thing!


Most efficient JavaScript countdown timer implementation I have this timer working already(jsbin):

HTML:

  <input id="m" type="number" value="0" min="0" max="59"/>
  <input id="s" type="number" value="2" min="0" max="59"/>
  <input id="start" type="button" value="Start!" />
  <output>00:00</output>

JavaScript:

var o = document.querySelector('output'),
m = document.querySelector('#m')
s = document.querySelector('#s'),
start = document.querySelector('#start');

start.addEventListener('click', function(){
    var mm = parseInt(m.value),
        ss = parseInt(s.value);
    timer = function(){
        if(mm>0){
            if(ss>0){
                ss--;
            }
            else{
                mm--;
                ss = 59;
            }
            setTimeout("timer()", 1000);
            o.value = (mm<10 ? '0' + mm : mm) + ":" + (ss<10 ? '0' + ss : ss);
        }
        else{
            if(ss>0){
                ss--;
                o.value = "00:" + (ss<10 ? '0' + ss : ss);
                setTimeout("timer()", 1000);
            }
        }
    }

    timer();
}, false);

It seems a lot of code for just a timer. Is it possible to implement this with less code? Maybe with loops, Date object or something else?

This code have problem and that is number of items in call stack when the time is big enough. I tried to write same code with while or for but I couldn't get a good result.

MY QUESTION is how to write this countdown with for or while?

document.querySelector("#id") is just a costly version of document.getElementById("id"). document.getElementById is the fastest way to select an element in your page (to the smartasses of you: besides document.head, document.body etc)

document.querySelector("tagname") is also just a costly version of document.getElementsByTagName("tagname")[0]. Coincidentally, that's the second fastest way to get an element.

document.querySelector and its array-like version are only useful for compound statements that are either too long or too difficult to do with "regular" DOM methods. One might argue that if your html is compound and complex, you're doing it wrong, and they'd most likely be right.

If I were you, I'd also do future-you a favor and name your variables in a way that makes sense.

So, what do we have so far?

var output = document.getElementsByTagName("output")[0],
    minutes = document.getElementById("m"),
    seconds = document.getElementsById("m"),
    start = document.getElementById("start");

Moving on. The event listener is fine, now about parsing integers...

Try this piece of code: parseInt("08"). The result is 0. Why, you ask? Well, if you don't pass a radix as a second parameter, parseInt will be nice enough to guess what you mean. parseInt sees that your string begins in a 0 and, of course, assumes the number is in octal! Why wouldn't it be?

Point: either pass a radix like this: parseInt(string, 10) or use parseFloat, which will always assume a decimal number. Whichever you like best, though I once heard that parseFloat is faster, dunno if that's true. I personally prefer parseFloat, for no obvious reason

So we get:

var mm = parseFloat(minutes.value),
    ss = parseFloat(seconds.value);

Magnificent.

This line:

timer =

Contains one of the most widely made errors in javascript. It's called implied global.

Basically, when you omit the var statement, the variable definition list gets appended to the global object (which window refers to). So:

function foo() { answer = 42; }
foo();
window.answer === 42;

Never omit the var statement. Even if you do wish to append a variable to the global object, do it explicitly (window.answer = 42;). Better that then let someone scratch their head trying to figure out how magic variables appeared from seemingly nowhere.

Besides one more thing, your code is fine. What's that other thing, you ask? setTimeout

When you pass a string to setTimeout, it's the same as calling eval. It evaluates the string. That's slow and bad. Instead, pass a function to it:

setTimeout(timer, 1000);

Since timer is a function, you can pass it around like any other value.

Now, one might ask: "How do I call func with parameters?"

setTimeout(function() {
    func(something, somethingElse);
}, 1000);
//or
setTimeout(func, 1000, something, somethingElse);

About your actual code? it's fine. It's not the best piece of code I've ever, but it's not the worse as well. It's not too long for a timer, since all that's done is decrementing and calling itself again. I'd move the < 10 checks to their own function, but maybe that's because I'm insane and not that "your way" is worse.

tl;dr Your code contains errors, but other than that, it's fine.


Any recommendations to improve this logging framework? Or is it overkill? So I've been kind of irritated with my logging recently as I find myself copying and pasting the same kinds of generic messages around to lots of different methods or retyping them ever so slightly differently (when they ought to be identical).

Hence, I created this kind of framework:

public class Log {

// other logging utility methods truncated

    public enum Message {

        //Main
        PROGRAM_EXIT ("Exiting program, return code = {0}"),
        THREAD_INTERRUPTED ("Thread interrupted while {0}"),
        FATAL_TERMINATING_ERROR ("Fatal exception thrown through entire stack: {0}"),
        SHUTDOWN_HOOK_EXCEPTION ("Exception while attempting to gracefully exit: {0}"),
        IO_READ_ATTEMPT ("Attempting to read file: {0}"),
        IO_READ_FAILURE ("Unable to read from file: {0}"),
        IO_WRITE_ATTEMPT ("Attempting to write to file: {0}"),
        IO_WRITE_FAILURE ("Unable to write to file: {0}"),
        IOEXCEPTION ("IOException while {0} to {1} : {2}"),

        private final String text;

        private Message(String text) {
            this.text = text;
        }

            @Override
            public String toString() {
                return text;
            }

        public String format(Object ... args) {
            String returnValue = toString();
            for(int i = 0; i < args.length; i++) {
                String placeholder = "{" + i + "}";
                if(!returnValue.contains(placeholder)) {
                    LOGGER.error("Too many arguments supplied to generateMessage() from " + getCallingMethodName());
                    LOGGER.error(" -- " + name());
                    break;
                }
                returnValue = returnValue.replace("{" + i + "}", args[i] == null ? "null" : args[i].toString());
            }
            return returnValue;
        }
    }
}

It's then invoked like the following:

private static final String FILE_PATH = "resources/something.txt";
//...
LOGGER.error(Log.Message.IO_READ_FAILURE.format(FILE_PATH));

... which would log ...

  Unable to read from file: resources/something.txt

It all works fine, but it feels a little too verbose at times for what I'm trying to accomplish, like I've tacked on a bit too many nuts and bolts.  Like I've gone deer hunting with a .50 cal or something.

Any suggestions to improve this?  The same messages are used redundantly in several places, so I am seeing some value out of it, but I'm not sure if there's a good way to make this still more clean or otherwise accomplish what I'm trying to do in a more succinct way.

If it's relevant, I'm using log4j.

You don't need to re-implement MessageFormat. It is much more powerful than what your implementation provides.

You don't show us the class of LOGGER, but based on the fact that you have a Log class, I suspect you are not using a logging library. I highly recommend you look into using one instead of implementing your on custom implementation.

If you are using a logging library, you are abstracting to much away from the library. All of the frameworks I've used support message formating and detailed exception logging (with stack traces).

The problem you are really trying to solve here is to allow yourself to reuse a number of common log message formats. This can be done with a simple set of Strings set to public static final.


Finding person with most medals in Olympics database I have an Olympics database from each Olympic year and I want to find the person that has won the most medals.  The main problem is that I'm basically querying the same sub-query twice in SUBSET1 and SUBSET2. How would I go about making this more efficient?

Select athlete  FROM ( Select athlete, Sum(total_medals) as total_medals
from Olympics Group by athlete) as SUBSET1 Where total_medals =
( Select Max( total_medals ) FROM ( Select Sum(total_medals) as total_medals
from Olympics    Group by athlete ) as SUBSET2);

This alternative to @rolfl's answer is more readable, in my opinion.  It also has a more efficient execution plan.

WITH medal_count AS (
    SELECT athlete
         , SUM(total_medals) AS grand_total_medals
         , RANK() OVER (ORDER BY SUM(total_medals) DESC) AS rank
        FROM Olympics
        GROUP BY athlete
)
SELECT athlete
     , grand_total_medals
    FROM medal_count
    WHERE rank = 1
    ORDER BY athlete;

SQLFiddle


Rust Book: The Twelve Days of Christmas In order to learn Rust I'm going through The Rust Book and did the implemented a program for the following exercise:

Print the lyrics to the Christmas carol “The Twelve Days of Christmas,” taking advantage of the repetition in the song.

I have decided to use lookup tables for the repeating lines of the song and ordinals because I believe those to be good for performance.
I'd appreciate any comments on my code, especially any suggestions on how to avoid the recurring magic number 12.
type Day = u8;

const LYRICS: [&str; 12] = [
    "a partridge in a pear tree",
    "two turtle doves",
    "three French hens",
    "four calling birds",
    "five gold rings",
    "six geese a laying",
    "seven swans a swimming",
    "eight maids a milking",
    "nine ladies dancing",
    "ten lords a leaping",
    "eleven pipers piping",
    "twelve drummers drumming",
];

const ORDINALS: [&str; 12] = [
        "first",
        "second",
        "third",
        "fourth",
        "fifth",
        "sixth",
        "seventh",
        "eigth",
        "ninth",
        "tenth",
        "eleventh",
        "twelfth",
];

fn main() {
    for day in 0..12 {
        if day != 0 {
            println!();
        }

        print_first_line(day);
        print_lyrics(day);
    }
}

fn print_first_line(day: Day) {
    println!(
        "On the {} day of Christmas my true love gave to me",
        ORDINALS[day as usize],
    )
}

fn print_lyrics(day: Day) {
    for line in (0..=day).rev() {
        if line == 0 && day != 0 {
            println!("And {}", LYRICS[line as usize]);
        } else {
            println!("{}", capitalize(LYRICS[line as usize]));
        }
    }
}

fn capitalize(text: &str) -> String {
    let mut chars = text.chars();

    let mut new_text = String::with_capacity(text.len());

    if let Some(c) = chars.next() {
        new_text.extend(c.to_uppercase());
        new_text.extend(chars);
    }

    new_text
}

Nicely done, I have very little negative to say!
Most of this answer will be an exploration into some more advanced ways of doing what you are doing, overkill for this case, but very useful in larger projects.

The day as usize everywhere is a bit of a distraction. I definitely understand the desire to store a small int when your data is known to be small (i.e. at most 11), but in this case you are only using Day as an index, so using type Day = usize directly seems like better design.

However, if you want to be fancy (and/or learn a bit about implementing your own types in Rust), you have an alternative which I will describe below: the basic idea is to implement your own types (use struct A(B) instead of type A = B), then implement the Index trait to index using Day directly.
For a small piece of code like this, it doesn't make a huge difference, but for large projects it's really helpful to avoid accidental mistakes like indexing an array with the wrong variable (i.e. in your case, indexing with some random usize in context instead of a Day between 0 and 11).
If you do that you get a type error, and this also allows you to abstract better: treating Day as an opaque type that is used to get the lyrics from your lyrics objects, rather than as an integer with arbitrary functionality. As an added bonus, this also helps avoid the magic number 12 problem that you mention.
Diving in, we start by defining new types (struct A(B)) for Day and for a list indexed by Day.
The #[derive(...)] is telling Rust that we want Days to be able to be copied, checked for equality, and printed out, copying over that functionality from u8.
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
struct Day(u8);

#[derive(Debug)]
struct DayList<T>([T; 12]);

const FIRST_DAY: Day = Day(0);
const LAST_DAY: Day = Day(11);

const LYRICS: DayList<&str> = DayList([
    ...

Then, we implement the ability to index DayList objects with Day.
For a variable of type A where we defined struct A(B), we use x.0 to get the underlying B data.
(This explicitness is part of the point of doing this, we make sure to keep A variables and B variables separate.)
impl<T> Index<Day> for DayList<T> {
    type Output = T;
    fn index(&self, day: Day) -> &T {
        self.0.index(day.0 as usize)
    }
}

Finally, we want to be able to iterate over days, something like this utility function seems most useful for your code:
fn days_upto(day: Day) -> impl DoubleEndedIterator<Item = Day> {
    (0..=day.0).map(Day)
}

This takes a Day and iterates over all days up to and including it. Perhaps this seems like overkill! But we get a lot  of benefit out of it: now we can modify the rest of your code to not have indices at all, and only iterate over Days directly and index DayList with Day. Type safety! Here's how it looks:
fn main() {
    for day in days_upto(LAST_DAY) {
        if day != FIRST_DAY {
            println!();
        }

        print_first_line(day);
        print_lyrics(day);
    }
}

fn print_first_line(day: Day) {
    println!(
        "On the {} day of Christmas my true love gave to me",
        ORDINALS[day],
    )
}

fn print_lyrics(day: Day) {
    for line in days_upto(day).rev() {
        if line == FIRST_DAY && day != FIRST_DAY {
            println!("And {}", LYRICS[line]);
        } else {
            println!("{}", capitalize(LYRICS[line]));
        }
    }
}

Finally: the if block
if day != FIRST_DAY {
    println!();
}

can be avoided if you print a title with the lyrics as well, which seems realistic. E.g.:
println!("The Twelve Days of Christmas");
println!("----------------------------");
for day in days_upto(LAST_DAY) {
    println!();
    print_first_line(day);
    print_lyrics(day);
}


Asynchronously load and unload a queue This code is supposed to asynchronously load a queue from a generating function.

#include <queue>
#include <functional>
#include <thread>
#include <mutex>
#include <condition_variable>
#include <iostream>
#include <stdexcept>

using namespace std;

template <typename Container>
struct LoadAsync {

    LoadAsync( size_t queue_size, function<Container()> f )
        : find_next( f ), size( queue_size ),
          worker_thread( &LoadAsync<Container>::worker_thread_function, this )
    {
        // first, build the basics
        unique_lock<mutex> lk( m );
        cv_init.wait( lk, [this] { return this->qued.size() >= this->size; } );
    }

    LoadAsync( const LoadAsync& ) = delete;

    void worker_thread_function()
    {
        while ( true ) {
            unique_lock<mutex> lk( m );
            cv.wait( lk, [this] { return this->qued.size() < this->size; } );
            while ( this->qued.size() < this->size ) {
                try {
                    qued.emplace( find_next() );
                } catch (domain_error) {
                    this->size--;
                }
            }
            lk.unlock();
            cv_init.notify_one();
        }
    }

    Container pop_front()
    {
        unique_lock<mutex> lk( m );
        cv_init.wait( lk, [this] { return this->qued.size() >= this->size; } );
        if (this->qued.size() == 0)
            throw out_of_range("out of range");
        auto front = move( qued.front() );
        qued.pop();
        lk.unlock();
        cv.notify_one();
        return front;
    }

  private:
    size_t size;
    queue<Container> qued;
    function<Container()> find_next;
    thread worker_thread;
    mutex m;
    condition_variable cv;
    condition_variable cv_init;
};

int main()
{
    auto func = []() {
        static int which = 1;
        if ( which < 10 )
            return which++;
        else
            throw domain_error("out of range");
    };

    LoadAsync<int> la( 3, func );

    for ( int i = 0; i < 11; ++i ) cout << la.pop_front() << endl;

}

Is this a good approach?  Are there any gotchas here in using condition_variables?  Are there any long bottlenecks here?  Is waiting on a condition variable an immediate thing?

I see one instance of undefined behavior in your code.

LoadAsync( size_t queue_size, function<Container()> f )
        : find_next( f ), size( queue_size ),
          worker_thread( &LoadAsync<Container>::worker_thread_function, this )
...

void worker_thread_function()
{
    while ( true ) {
        unique_lock<mutex> lk( m );

...

thread worker_thread;
mutex m;

The LoadAsync constructor constructs each member variable in the order in which they were declared — i.e., it constructs this->worker_thread before constructing this->m. And the way you construct worker_thread causes it to start running immediately, and the first thing it does is attempt to lock m — whose constructor may not have finished yet!

The simplest solution is to reorder your member variables so that worker_thread will be constructed last.

Style notes:

using namespace std; is a bad idea, and you shouldn't do it (especially in a header file like this). Explicitly qualify std::mutex, std::domain_error, and so on. It's a good habit to get into.
&LoadAsync<Container>::worker_thread_function is a verbose way of saying &LoadAsync::worker_thread_function: the <Container> part is implied in this context, just as it is in your copy-constructor declaration.
You don't need to explicitly = delete your copy-constructor: since one of your non-static member variables is a non-movable std::mutex, your class itself becomes non-movable by default.
cv and cv_init are bad names. Consider cv_consume and cv_produce, or cv_queue_empty and cv_queue_full. (Also, it's strange that your pop_front will block waiting for the producer to finish running find_next() even when the queue isn't empty. Doesn't that basically defeat the purpose of having two threads?)
qued is a weird name; perhaps you meant q or queue?
Container is a bad name for the object type being contained by your queue. I suggest renaming it T.

You don't provide any way to clean up after your worker_thread. By default, std::thread's destructor simply blocks until the thread finishes, which (since your worker thread is just an infinite loop) means that destroying a LoadAsync object will hang forever.

Calling worker_thread.detach() is one solution, but will still "leak resources" in a sense. It would be better to change the worker_thread's condition from while (true) to something like while (!this->finished), change your predicate from

[this] { return this->qued.size() < this->size; }

to

[this] { return finished || (qued.size() < size); }

and then change the body of the destructor to

{
    unique_lock<mutex> lk(m);
    this->finished = true;
}
this->cv.notify_one();
this->worker_thread.join();

It's bad practice to call the user-provided function find_next() while holding the lock on m. Consider what would happen if find_next tried to take the same lock:

bool first = true;
LoadAsync<int> la(3, [&]() {
    return first ? (first = false, 42) : la.pop_front() + 1;
});

The easiest solution is to drop the lock, call find_next(), and then reacquire the lock to move the result into the queue. Of course, the move-constructor of Container might take the lock...

Also, notice that if find_next throws anything other than std::domain_error, your program will die in std::terminate. That's not so good.

Lastly, notice that you're relying on the notion of a "producer thread" and a "consumer thread" being able to run concurrently with each other. If your program is running in a context so resource-constrained that it can only get hold of a single thread, then your code will deadlock. IMHO that's extremely unlikely, and even if it did happen I would hope that thread's constructor would throw an exception instead of blithely continuing on its way. But I don't know.


Function for timing executions (inspired by MATLAB tic toc) Inspired by MATLAB's tic/toc I've written a short helper function for calculating execution time.

from time import time

def tic():
    """Simple helper function for timing executions.

    Returns a closure that holds current time when calling tic().

    Usage:
    toc = tic()
    //some code
    print(toc())
    """
    t = time()
    return (lambda: (time() - t))

I'm well aware that there are python modules (e.g. timeit) that handle this kind of problem. However I do not want to time the execution of a whole script. I'm using this function in an interavtive shell for querying a mssql server.

What do you think? Is this good practice?

I have a minor stylistic point, but you don't need (and shouldn't use) brackets around return values. Neither the lambda itself nor the lambda's return value.

return lambda: time() - t

It is minor but it actually confused me and made me think there was more of an expression there because of how rarely brackets are used like this.


Remove a query string parameter from a URL I have a URL in wstring format in C++, e.g.:

http://stackoverflow.com?Country=US&City=Seattle&State=WA

I want to get rid of Country query param from the above string. Country query param can be the first query param or could be the last too.

 wstring ManagementUrl = L"http://stackoverflow.com?Country=US&City=Seattle&State=WA"
    wstring queryParam = L"&Country=US";
    wstring queryParam1 = L"Country=US";

    string::size_type urlLength = ManagementUrl.length();
    string::size_type queryLength = queryParam.length();
    string::size_type queryLength1 = queryParam1.length();

    size_t index = ManagementUrl.find(queryParam);
            if (index != std::wstring::npos)
            {
                wstrLink.erase(index, queryLength);
            }
        size_t index1 = ManagementUrl.find(queryParam1);
            if (index != std::wstring::npos)
            {
                wstrLink.erase(index, queryLength1);
            }

Is there a better way to do this?

The problem with your design is that you assume the URL is actually well formed. In my experience (writing web crawlers) we found that greater than 80% of URL's on the web are badly formatted the biggest offender is the first ? that separates the attributes from the target (this was often a &).

Now if you URL is badly formatted like this:

http://stackoverflow.com&Country=US&City=Seattle&State=WA
                   //   ^ Notice the bad format

Your code will break that URL and turn it into:

http://stackoverflow.comUS&City=Seattle&State=WA

Probably not what you want.

Also URL as they pass through systems tend to be normalized (so they can be sorted and all sorts of other things). So your URL may have started of as the above but because it was normalized your query parameters have been re-ordered.

 http://stackoverflow.com?City=Seattle&Country=US&State=WA
                  // Query parameters ordered lexicographically.

Now if you just go and remove the attribute names they may not be in the order you expect.

Now imagine a well formatted URL but with another domain.

http://No.Country=US.For.Old.Men.com?Country=US&City=Seattle&State=WA

Which is a perfectly valid domain. But your code is going to strip out a segment for the domain (and thus make the URL invalid).

http://No..For.Old.Men.com?Country=US&City=Seattle&State=WA
     //  ^^ two dotes consecutively is not valid domain.

Your best bet is to get some already written software for handling URL's there are just too many corner cases for you to write and expect to handle the URL correctly.


Pascal's triangle in mit-scheme As suggested by the name of source file (ex1.12.scm), I just started learning mit-scheme by reading SICP. In Exercise 1.12, I'm asked to "compute elements of Pascal's triangle by means of a recursive process." For fun and extra practice, I wrote a program to print the whole triangle. (It took me several minutes to print a Pascal's triangle, and several hours to beautify it :/)

(define (pascal x)
  (define (pascal-iter m n max-len)
    (cond ((and (> m 0) (> n 0))
             (pascal-iter m (- n 1) max-len)
             (beauti-print (pascal-item m n) max-len))
          ((= n 0) (pascal-iter (- m 1) (- m 1) max-len)
                   (display "\n")
                   (print-space (* (- x m) max-len)))
          ((= m 0) "Done")))
  (define (pascal-item m n)
    (cond ((= n 1) 1)
          ((= n m) 1)
          (else (+ (pascal-item (- m 1) (- n 1))
                   (pascal-item (- m 1) n)))))
  (define (beauti-print item max-len)
    (print-space (floor (- max-len
                       (/ (num-of-digit item) 2))))
    (display item)
    (print-space (ceiling (- max-len
                         (/ (num-of-digit item) 2)))))
  (define (num-of-digit n)
    (+ (floor (/ (log n) (log 10))) 1) )
  ;; (print-space 1)   -> " "
  ;; (print-space 1.5) -> "  "
  (define (print-space n)
    (cond ((> n 0) (display " ")
                (print-space (- n 1)))
          (else (display ""))))
  (pascal-iter x x (num-of-digit (pascal-item x (/ x 2)))))

(pascal 12)

Here is the output:

;Loading "ex1.12.scm"...

                                   1
                                1     1
                             1     2     1
                          1     3     3     1
                       1     4     6     4     1
                    1     5     10    10    5     1
                 1     6     15    20    15    6     1
              1     7     21    35    35    21    7     1
           1     8     28    56    70    56    28    8     1
        1     9     36    84   126   126    84    36    9     1
     1     10    45   120   210   252   210   120    45    10    1
  1     11    55   165   330   462   462   330   165    55    11    1
;... done

One of its drawbacks is obvious: recursion is very expensive. In fact, (pascal 15) would produce ;Aborting!: maximum recursion depth exceeded on my machine.

However, please don't put too much emphasis on performance-related issues when answering this question, because I think for me, for this moment, the form of good programming habits takes higher priority than the optimization of algorithm. As a result, please focus on readability, code style and so on.

Keep your concerns separated

One design principle that is vital to engineering good software is that of "separation of concerns".  In other words, each function should be concerned with doing one, and only one, thing.

If you step back and take a look at the pascal function, you will see it's trying to do two things at once:

Compute Pascal's triangle
Pretty-print the triangle

You can separate these concerns.

Let's start with the pascal function.  It should only compute the values in the triangle.  It should return a list of "rows", where a row is represented a list of numbers.

So we can use pascal to compute a list of lists of numbers.  Now we want to pretty-print this list.  We could do this all in one function (and there's nothing really wrong with that).  But if we're careful, we can make things more generic (and thus reusable).  We can compose pretty-printing the triangle into the following:

Formatting each row.  This involves taking a list of numbers and returning a string representation for that row.
Centering the rows.  Basically, padding each string with spaces.
Printing the rows.  Taking the strings and print them each on separate lines.

We can define functions format-row, center-lines, and print-lines to handle these tasks.  Now we can define a function pretty-print-pascal that combines these functions:

(define pretty-print-pascal (x)
  (print-lines (center-lines (map format-row (pascal x)))))


Running multiple games concurrently with node.js I am currently creating multiple games by adding objects of type Game as properties of an object called games. The names of these properties are set programmatically - for example, the first game will be a property of games called game1. Sockets joining this game will join the room room1.

Is this the right approach? It feels like there might be a cleaner way.

var express = require('express');
var app = express();
var http = require('http').Server(app);
var io = require('socket.io')(http);
var Game = require('./Game.js');
app.use(express.static(__dirname + '/public'));
app.get('/', function(req, res) {
    res.sendFile('/index.html');
});

var games = {
    //Games get put here
};

var gameCount = 0;

io.on('connection', function(socket) {

    var i = gameCount;
    var arrayOfGames = [];

    while(i){
        var gameInfo = {};
        gameInfo.name = games["game"+i]["getName"]();
        gameInfo.no = games["game"+i]["getNo"]();
        arrayOfGames.push(gameInfo);
        i--;
    }

    socket.emit('populate game list',arrayOfGames);

    socket.on('create game',function(options){
        gameCount++;
        options.no = gameCount;
        games["game"+ gameCount] = new Game(options);
        socket.join('room' + gameCount);
        socket.room = 'room'+gameCount;
    });

    socket.on('join game',function(no){
        socket.join('room'+no);
        socket.room = 'room'+no;
    });

});

var port = Number(process.env.PORT || 3000);
http.listen(port, function() {
    console.log('listening on *:3000');
});

I can show the client side code and class code if you request, but I don't feel it's relevant here.

First I would suggest separating your game server from your static server. You wouldn't want your static server clogging up your game server process. The advantage is that you can easily spawn more processes if you want, across different ports, or even across different machines. And if one goes down, it's just that one, not everything.

So that means the following should be written separately:

Your static server (express)
Your room list server
Game server (which stores the game state)

Static server will be a standalone thing. Room list will share data with the game server. All your room listing server does is just get the active rooms from some DB. The game server will hold the game states across rooms. Creating a room would mean storing it's id to the DB for the server listing.

I also consider using UUIDs instead of counting. Create the id when a game object is created. This ensures that a game object will have a unique id upon creation, without relying on outer logic to supply it one.


Change-Making Problem Solution Please take a moment and review my code and tell me how I did. This is the first time in my class (CSC 111) I've been given an assignment in which I had to use a lot of my own resources and develop most of it on my own.

  Write an application that prompts for and reads a double value
  representing a monetary amount. Then determine the least
  number of each bill and coin needed to represent that amount,
  starting with the highest. Assume that ten dollars is the
  maximum size needed.
  For example, if the value 47.63 is entered the program should
  display

  4 ten dollar bills
  1 five dollar bills
  2 one dollar bills
  2 quarters
  1 dimes
  0 nickels
  3 pennies

My code works mostly flawlessly (I believe). I would like to hear other people's opinion on my solution. I understand it probably isn't the most efficient out there. How can it be improved? Are there things to watch out for?

import java.math.BigDecimal;
import java.text.NumberFormat;
import java.util.Scanner;
import java.util.Locale;

public class Assignment01
{
    public static void main(String[] args)
    {

    /* Define Constants */

    BigDecimal tenDollar = BigDecimal.valueOf(10);
    BigDecimal fiveDollar = BigDecimal.valueOf(5);
    BigDecimal oneDollar = BigDecimal.valueOf(1);
    BigDecimal quarter = BigDecimal.valueOf(0.25);
    BigDecimal dime = BigDecimal.valueOf(0.10);
    BigDecimal nickel = BigDecimal.valueOf(0.05);
    BigDecimal penny = BigDecimal.valueOf(0.01);

             /* Get Input From User */

    System.out.print("Please enter the amount to be converted: ");
   Scanner scan = new Scanner(System.in);
    BigDecimal money = scan.nextBigDecimal();

    NumberFormat usdFormat = NumberFormat.getCurrencyInstance(Locale.US);
    usdFormat.setMinimumFractionDigits(2);
    usdFormat.setMaximumFractionDigits(2);

    System.out.println("Amount you entered: " + usdFormat.format(money));

            /* Begin Processing and Displaying Information */

    BigDecimal tenDollarNext = money.divideToIntegralValue(tenDollar);
    System.out.println(tenDollarNext.intValue() + " Ten Dollar Bills");

    BigDecimal fiveDollarNext = money.subtract(tenDollar.multiply(tenDollarNext));
    BigDecimal i = fiveDollarNext.divide(fiveDollar);
    System.out.println(i.intValue() + " Five Dollar Bills");

    BigDecimal oneDollarNext = fiveDollarNext.subtract(fiveDollar.multiply(BigDecimal.valueOf(i.intValue())));
    BigDecimal f = oneDollarNext.divide(oneDollar);
    System.out.println(f.intValue() + " One Dollar Bills");

    BigDecimal quarterNext = oneDollarNext.subtract(oneDollar.multiply(BigDecimal.valueOf(f.intValue())));
    BigDecimal q = quarterNext.divide(quarter);
    System.out.println(q.intValue() + " Quarters");

    BigDecimal dimeNext = quarterNext.subtract(quarter.multiply(BigDecimal.valueOf(q.intValue())));
    BigDecimal d = dimeNext.divide(dime);
    System.out.println(d.intValue() + " Dimes");

    BigDecimal nickelNext = dimeNext.subtract(dime.multiply(BigDecimal.valueOf(d.intValue())));
    BigDecimal n = nickelNext.divide(nickel);
    System.out.println(n.intValue() + " Nickels");

    BigDecimal pennyNext = nickelNext.subtract(nickel.multiply(BigDecimal.valueOf(n.intValue())));
    BigDecimal p = pennyNext.divide(penny);
    System.out.println(p.intValue() + " Pennies");

    }
}

BigDecimal money = scan.nextBigDecimal();

The specification said

  reads a double value representing a monetary amount.

However, BigDecimal is the correct type to represent a monetary amount. There's a case to be made for using scan.nextDouble() to comply with the spec and then converting it to BigDecimal. There's also a case to be made that you should talk to the person who set the task and suggest improving the spec.

    BigDecimal tenDollarNext = money.divideToIntegralValue(tenDollar);
    System.out.println(tenDollarNext.intValue() + " Ten Dollar Bills");

    BigDecimal fiveDollarNext = money.subtract(tenDollar.multiply(tenDollarNext));
    BigDecimal i = fiveDollarNext.divide(fiveDollar);

Why the inconsistencies? tenDollarNext and fiveDollarNext aren't analogues of each other, and one uses divideToIntegralValue(...) where the other uses divide(...).intValue().

There are seven chunks of code dealing with seven values of token which are virtually identical. DRY: use a loop.


Avoid static initializer for lists of image and video formats? I wrote the following code to read constants from annotations:

public class ContentUtils {
    static {
        String[] videoFormats = null;
        String[] imageFormats = null;
        try {
            Field multipartFileField = MultipartFileWrapper.class.getField("multipartFile");
            Extensions annotation = multipartFileField.getAnnotation(Extensions.class);
            imageFormats = annotation.imageFormats();
            videoFormats = annotation.videoFormats();
        } catch (NoSuchFieldException e) {
            if (imageFormats == null) {
                imageFormats = new String[]{};
            }
            if (videoFormats == null) {
                videoFormats = new String[]{};
            }
        }
        ACCEPTED_IMAGE_FORMATS = imageFormats;
        ACCEPTED_VIDEO_FORMATS = videoFormats;
    }

    public static final String[] ACCEPTED_IMAGE_FORMATS;
    public static final String[] ACCEPTED_VIDEO_FORMATS;
}

Is there a way to rewrite it better?

Or something else improve at this code?

According to your comment,
if it's only the MultipartFileWrapper.class.getField("multipartFile") statement that might throw a NoSuchFieldException,
then when that happens, the lines that set imageFormats and videoFormats will not be reached, so these values will remain null, and the if conditions in the catch block are pointless.

Furthermore, it's good to minimize the scope of try-catch blocks.
Consider this reworked version:

static {
    Field multipartFileField = null;
    try {
        multipartFileField = MultipartFileWrapper.class.getField("multipartFile");
    } catch (NoSuchFieldException e) {
        // maybe log that something bad happened?
    }
    if (multipartFileField != null) {
        Extensions annotation = multipartFileField.getAnnotation(Extensions.class);
        ACCEPTED_IMAGE_FORMATS = annotation.imageFormats();
        ACCEPTED_VIDEO_FORMATS = annotation.videoFormats();
    } else {
        ACCEPTED_IMAGE_FORMATS = new String[]{};
        ACCEPTED_VIDEO_FORMATS = new String[]{};
    }
}

Here, I don't need to ask what might throw the NoSuchFieldException,
it's obvious, which is good.

As for avoiding the static initializer...
if you want to have static fields with non-trivial constants,
then there's just no other way,
you have no choice but to initialize them in a static initialization block like this.
Other option would be to give up the static fields,
make them non-static and initialize in the constructor.
But this probably won't make much sense,
since the field values come from static data (MultipartFileWrapper.class).


Assigned a System resource to an image based on variable I have a mapping application and the marker I'm using is based on the field rep assigned to that visit.  My code is just a Select Case based on the rep's name and then it sets the bitmap for that person.

My code works fine, but there are 19 different field reps and I feel there must be a better way.

The code loops through a CSV file and grabs the Field Rep's name.  Based on who it is, a System Resource image for that person is assigned to the image.

Select Case CurrentRecord(iAssignedToCol)
  Case "Al B. Connor"
    If CurrentRecord(iStatusCol) = "Scheduled" Then
      MapIcon = New Bitmap(My.Resources.Scheduled_ABC)
    ElseIf CurrentRecord(iStatusCol) = "Tentatively Scheduled" Then
      MapIcon = New Bitmap(My.Resources.Tentative_ABC)
    End If
  Case "Donald E. Firestone"
    If CurrentRecord(iStatusCol) = "Scheduled" Then
      MapIcon = New Bitmap(My.Resources.Scheduled_DEF)
    ElseIf CurrentRecord(iStatusCol) = "Tentatively Scheduled" Then
      MapIcon = New Bitmap(My.Resources.Tentative_DEF)
    End If
  Case "3rd name"
    If CurrentRecord(iStatusCol) = "Scheduled" Then
      MapIcon = New Bitmap(My.Resources.Scheduled_GHI)
    ElseIf CurrentRecord(iStatusCol) = "Tentatively Scheduled" Then
      MapIcon = New Bitmap(My.Resources.Tentative_GHI)
    End If
  Case "4th name"
    If CurrentRecord(iStatusCol) = "Scheduled" Then
      MapIcon = New Bitmap(My.Resources.Scheduled_JKL)
    ElseIf CurrentRecord(iStatusCol) = "Tentatively Scheduled" Then
      MapIcon = New Bitmap(My.Resources.Tentative_JKL)
    End If
  Case "5th name"
    If CurrentRecord(iStatusCol) = "Scheduled" Then
      MapIcon = New Bitmap(My.Resources.Scheduled_MNO)
    ElseIf CurrentRecord(iStatusCol) = "Tentatively Scheduled" Then
      MapIcon = New Bitmap(My.Resources.Tentative_MNO)
    End If
  Case "6th name"
    If CurrentRecord(iStatusCol) = "Scheduled" Then
      MapIcon = New Bitmap(My.Resources.Scheduled_PQR)
    ElseIf CurrentRecord(iStatusCol) = "Tentatively Scheduled" Then
      MapIcon = New Bitmap(My.Resources.Tentative_PQR)
    End If
  Case "7th name"
    If CurrentRecord(iStatusCol) = "Scheduled" Then
      MapIcon = New Bitmap(My.Resources.Scheduled_STU)
    ElseIf CurrentRecord(iStatusCol) = "Tentatively Scheduled" Then
      MapIcon = New Bitmap(My.Resources.Tentative_STU)
    End If
  Case "8th name"
    If CurrentRecord(iStatusCol) = "Scheduled" Then
      MapIcon = New Bitmap(My.Resources.Scheduled_VWX)
    ElseIf CurrentRecord(iStatusCol) = "Tentatively Scheduled" Then
      MapIcon = New Bitmap(My.Resources.Tentative_VWX)
    End If
  Case "9th name"
    If CurrentRecord(iStatusCol) = "Scheduled" Then
      MapIcon = New Bitmap(My.Resources.Scheduled_YZA)
    ElseIf CurrentRecord(iStatusCol) = "Tentatively Scheduled" Then
      MapIcon = New Bitmap(My.Resources.Tentative_YZA)
    End If
End Select

marker = New GMarkerGoogle(New PointLatLng(sSearchLat, sSearchLong), MapIcon)

Thanks!

this should be written once like this:

private sub whatever(byref res As Bitmap)
  If CurrentRecord(iStatusCol) = "Scheduled" Then
      MapIcon = New Bitmap(res)
  ElseIf CurrentRecord(iStatusCol) = "Tentatively Scheduled" Then
      MapIcon = New Bitmap(res)
  End If
end sub

and

Select Case CurrentRecord(iAssignedToCol)
    Case "Al B. Connor"
         whatever(My.Resources.Scheduled_ABC)
    Case "Donald E. Firestone"
         whatever(My.Resources.Scheduled_DEF)
    Case....

Hope you get my point..


Library for generating passwords I am going to make this code below into the second release of my library, but I want to make sure I haven't missed anything. If there are improvements that can be made, I'd like to implement them.
This code is also on GitHub.
import random
import secrets
import string
import urllib.request

class Simple():
    def __init__(self, length: int, characters = None):
        '''A simple password'''
        self.length = length
        self.characters = characters
        self.output = []

    def generate(self, num_of_passwords: int):
        '''
        Generates a password depending on the num_of_passwords
        and the arugments provided in the simple class
        '''
        characters = ''
        if self.characters is None:
            characters = string.ascii_letters + string.digits
        else:
            characters = self.characters

        for i in range(num_of_passwords):
            password = ''
            for c in range(self.length):
                password += secrets.choice(characters)
            self.output.append(password)
        return self.output

    def return_result(self, index: int):
        '''
        Returns the password which is at the specified index
        in the output list.
        '''
        try:
            return self.output[index]
        except IndexError:
            print(f'Incorrect index specified. Please provide an index relevant to the number of passwords generated')

    def clear_results(self):
        '''Clears the output list if you want to make way for new passwords'''
        self.output.clear()

class Complex(Simple):
    def __init__(self, length, string_method, numbers=True, special_chars=False):
        '''
        Creates a customisable password depending on length,
        string_method, numbers and special_chars
        '''
        characters = ''
        self.output = []

        methods: dict = {
            "upper": string.ascii_uppercase,
            "lower": string.ascii_lowercase,
            "both": string.ascii_letters,
        }

        characters += methods[string_method]

        if numbers:
            characters += string.digits
        if special_chars:
            characters += string.punctuation

        super().__init__(length=length, characters=characters)

class Memorable(Simple):
    def __init__(self, numbers=True):
        '''A memorable password'''
        self.numbers = numbers
        self.output = []

    def generate(self, num_of_passwords: int):
        '''Gets some random words'''
        word_url = "http://svnweb.freebsd.org/csrg/share/dict/words?view=co&content-type=text/plain"
        headers = {'User-Agent': 'Mozilla/5.0 (Windows NT 6.1; Win64; x64)'}
        req = urllib.request.Request(word_url, headers=headers)
        response = response = urllib.request.urlopen(req)
        long_txt = response.read().decode()
        words = long_txt.splitlines()

        '''
        Generates the password containing 2 words
        and numbers if self.numbers == True
        '''
        for i in range(num_of_passwords):
            password = ''
            two_words = ''
            for i in range(2):
                two_words += secrets.choice(words).title()
            password = two_words
            if self.numbers == True:
                for i in range(random.randint(3, 4)):
                    password += secrets.choice(string.digits)
            self.output.append(password)
        return self.output

# Test Scenarios

if __name__ == "__main__":
    var = Memorable()
    print(var.generate(3))

1. Add even more typing
It's good that you use some basic type annotations but you should use them everywhere, not just on a few select parameters. Have a look at the typing module, especially if you want to support Python versions before 3.9. For example, your function signatures could be
def __init__(self, length: int, characters: Optional[str] = None):
def generate(self, num_of_passwords: int) -> None:
def __init__(self, length: int, string_method: Literal["upper", "lower", "both"], numbers: bool = True, special_chars: bool = False):

2. Keyword-only parameters
This is a personal suggestion and I am sure you will find people who disagree with me on it. In your current code, Complex(5, "lower", True, False) is a valid call; yet, it is completely meaningless to a reader. By making the last two parameters keyword-only, you force the user to be more expressive: Complex(5, "lower", numbers=True, special_chars=False).
On that note, I suggest to give the parameters more expressive names such as include_numbers and include_special_characters.
3. Add better docstrings
You should ask yourself: who do you write your code documentation for? Right now, comments like "A simple password" don't provide much helpful information to anyone except for maybe yourself. What makes a password "simple"? Does it only contain numbers and letters? Is it short? Is it easy to remember? The same holds true for "complex", "memorable" and other terms. I'll do a shameless plug of my own recent blog post on that topic for more information.
4. Security
In the context of "passwords", "security" should always be mentioned, even if you feel that for your particular purpose you don't actually need it. Toby Speight already commented that getting words from a dictionary in the web with a hardcoded URL gives a pretty good target for attackers. An easy and platform-independent workaround would be to include a complete word dictionary in your package itself so there is no need to get it via HTTP.
Another issue might come up due to the fact that random, which you use to build your passwords, is not cryptographically secure. The secrets module, which you also used, is already a better alternative.
5. Enums
This point, again, might be a bit controversial. Instead of using "method" strings, "upper", "lower", and "both", I'd suggest to use an enum class instead, which makes it easier for your IDE to help you and gives you errors more quickly should you try to use a value that does not exist.
class PasswordGenerationStrategy(enum.Enum):
  Upper = "upper"
  Lower = "lower"
  Both = "both"


SQL code reuse with C# I have some code that allows for SQL reuse for C#. I'm pretty happy with it, but the fact that I can't find anything similar on the web makes me think I'm going about it the wrong way. I have a GitHub repo with a Visual Studio solution, and the code is repeated down below.

Here's the use case:

I'm working on a fairly standard C# and SQL Server project where we're generating reports. Each report is essentially the results of SQL query, displayed in HTML in the browser. I'd rather not use an ORM like Entity Framework, since we need to exactly control the SQL for performance reasons (e.g. use SQL server windowing functions, table hints, etc).

However, many of the reports are similar, just slicing and dicing by different attributes, so there's a desire for code reuse. SQL Server has stored procedures, views, and functions, but the consensus seems to be that you'll get into trouble trying to use them to create abstractions. To get around this I'm trying to use C# to reuse SQL code.

I'm essentially using Visual Studio's T4 text templates to generate SQL files at development time. I'm then passing the SQL to Stack Exchange's Dapper.NET project to handle variable substitution, issuing the query, and POCO [de]serialization.

Given a query like this, in the template file CuteAnimalsByLocation.tt:

<#@ output extension=".sql" #>
select * from animals a
where a.IsCute = 1 and a.IsFuzzy = 1 and a.Location = @location

And this file, DeadlyMachinesByLocation.tt:

<#@ output extension=".sql" #>
select * from DeadlyMachines m
where m.IsLethal = 1 and m.HasExplosives = 1 and m.Location = @location

I can reuse the above queries in a more complex query by writing the following, AnimalsInPeril.tt:

<#@ output extension=".sql" #>

WITH
CuteAnimalsInLocation as
(
    <#@ include file="CuteAnimalsByLocation.tt" #>
),
DeadlyMachinesInLocation as
(
    <#@ include file="DeadlyMachinesByLocation.tt" #>
)
select a.* from CuteAnimalsInLocation a
inner join DeadlyMachinesInLocation m on a.Location = m.Location

I can run all 3 of these queries like this: (assuming the POCOs Animal and DeadlyMachine exist; they're just POCOs that map to the table schemas):

using (var connection = new SqlConnection(ConnectionString))
{
    connection.Open();
    var queryParams = new { Location = "NorthAmerica" };

    IEnumerable<Animal> animalsNeedingHelp =
        connection.QueryFromFile<Animal>("AnimalsInPeril", queryParams);
    IEnumerable<Animal> cuteAnimals =
        connection.QueryFromFile<Animal>("CuteAnimalsByLocation", queryParams);
    IEnumerable<DeadlyMachine> deadlyMachines =
        connection.QueryFromFile<DeadlyMachine>("DeadlyMachinesByLocation", queryParams);
}

connection.QueryFromFile is defined as this, and depends on Dapper:

public static class DapperFileExtensions
{
    public static IEnumerable<TReturn> QueryFromFile<TReturn>(this IDbConnection cnn, string file, object params = null)
    {
        var sql = File.ReadAllText(file + ".sql");
        return cnn.Query<TReturn>(sql, params);
    }
}

Can anyone see problems with this approach? I guess the only thing I can think of is that I could add caching around the File.ReadAllText call.

You say performance is a concern, but you're doing this.

select a.* from

That's poor form whether or not you already have a table scan in the underlying query plan for the CTE.

I think you misunderstood the article you linked to. It is warning against doing exactly what you've done here. SQL just doesn't lend itself to code reuse in the way a "regular" programmer is accustomed to. It's a different beast that way, it takes a different mindset.

SQL is a set based query language. Code reuse comes in the form of (well tuned) views and stored procedures. If you need this kind of fine grained control over the SQL, then keep it where it belongs, in the database.

Depending on just how dynamic your queries are, you may not always be getting the benefit of query plan caching. Each time a query that the analyzer hasn't seen before is processed, it has to generate a new plan, so you'll take a performance hit.

Lastly, I left a comment, but I should mention it here too. Be careful using query hints. You may be able to outwit the analyzer now, based on the current data, but the data will change over time. When the data has changed and your hint is no longer the most efficient query plan, the analyzer won't be able to choose the most efficient plan because you've told it not to. Performance gained this way may degrade over time.


Implementation of counting sort I'm trying to learn more about counting sort and I just implemented the example given in CLRS,  my question is: How can I improve this code?

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

void counting_sort(int *, int, int);
int find_max(int *, int);

int main(void)
{
    int l_size;
    int i;

    scanf("%d", &l_size);

    int *num_list = calloc(l_size, sizeof(int));

    for (i = 0; i < l_size; i++)
        scanf("%d", &num_list[i]);

    int max = find_max(num_list, l_size);
    counting_sort(num_list, l_size, max);

    puts("Sorted:");
    for (i = 0; i < l_size; i++)
        printf("%d ", num_list[i]);

    printf("\n");

    return 0;

}

void counting_sort(int *num_list, int l_size, int max)
{
    int i, j;

    int *count_list = calloc(max + 1, sizeof(int)); // auxiliary array C
    int *sorted_list = calloc(l_size, sizeof(int));

    for (i = 0; i < l_size; i++)
        count_list[num_list[i]]++;

    for (i = 1; i < max + 1; i++)
        count_list[i] += count_list[i - 1];

   for (i = l_size - 1; i >= 0; i--)
   {
        sorted_list[count_list[num_list[i]] - 1] = num_list[i];
        count_list[num_list[i]]--;
   }

   memcpy(num_list, sorted_list, l_size * sizeof(int));
}

int find_max(int *num_list, int l_size)
{
    int i;
    int max = num_list[0];

    for (i = 1; i < l_size; i++)
    {
        if (num_list[i] > max)
            max = num_list[i];
    }

    return max;
}

You can avoid modifying the input parameter num_list and the memcopy by returning the sorted list pointer. All relevant changes to your code are marked with // (1) in the code below.

You also have a memory leak in counting_sort since you don't release count_list. The fix is marked // (2).

Same for the lists in main // (3)

Note : The program also seg faults when negative data is entered. You either have to guard against that, or also find the minimum data value,  allocate count_list big enough and fix the indices when accessing it.

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

int* counting_sort(int *, int, int); // (1)
int find_max(int *, int);

int main(void)
{
    int l_size;
    int i;

    scanf("%d", &l_size);

    int *num_list = calloc(l_size, sizeof(int));

    for (i = 0; i < l_size; i++)
        scanf("%d", &num_list[i]);

    int max = find_max(num_list, l_size);
    int* sorted = counting_sort(num_list, l_size, max); // (1)

    puts("Sorted:");
    for (i = 0; i < l_size; i++)
        printf("%d ", sorted[i]); // (1)

    printf("\n");

    free(sorted); // (3)
    free(num_list); // (3)

   return 0;
}

int* counting_sort(int *num_list, int l_size, int max) // (1)
{
  int i, j;

  int *count_list = calloc(max + 1, sizeof(int)); // auxiliary array C
  int *sorted_list = calloc(l_size, sizeof(int));

  for (i = 0; i < l_size; i++)
    count_list[num_list[i]]++;

  for (i = 1; i < max + 1; i++)
    count_list[i] += count_list[i - 1];

  for (i = l_size - 1; i >= 0; i--)
    {
      sorted_list[count_list[num_list[i]] - 1] = num_list[i];
      count_list[num_list[i]]--;
    }

  free(count_list);  // (2)

  return sorted_list;  // (1)
}

int find_max(int *num_list, int l_size)
{
    int i;
    int max = num_list[0];

    for (i = 1; i < l_size; i++)
    {
        if (num_list[i] > max)
            max = num_list[i];
    }

    return max;
}