Vehicle inventory program First time posting here, let me apologize ahead of time if this is not the correct format.. Anyway, I am new to programming and I am working on a school assignment to design a vehicle inventory system in Python (assignment details below). The code seems to be working as it should. I am questioning the 'update vehicle' function I defined outside of the class, whether it would make more sense to change to a class method. Or any other feedback anyone has. Thanks.
Create a final program that meets the requirements outlined below.

Assignment:
Create an automobile class that will be used by a dealership as a vehicle inventory program.  The following attributes should be present in your automobile class:

private string make
private string model
private string color
private int year
private int mileage

Your program should have appropriate methods such as:

constructor
add a new vehicle
remove a vehicle
update vehicle attributes

At the end of your program, it should allow the user to output all vehicle inventory to a text file.

class Automobile:

    def __init__(self):
        self.make = " "
        self.model = " "
        self.color = " "
        self.year = 0
        self.mileage = 0

    def add_vehicle(self):
        self.year = int(input("Enter year: "))
        self.make = input("Enter make: ")
        self.model = input("Enter model: ")
        self.color = input("Enter color: ")
        self.mileage = int(input("Enter mileage: "))

    def __str__(self):
        return('%d %s %s Color: %s Mileage: %d' %
              (self.year, self.make, self.model, self. color,
               self.mileage))

vehicle_list = []

def edit(vehicle_list):
    pos = int(input('Enter the position of the vehicle to edit: '))
    new_vehicle = car.add_vehicle()
    new_vehicle = car.__str__()
    vehicle_list[pos-1] = new_vehicle
    print('Vehicle was updated')

user=True
while user:
    print ("""
    1.Add a Vehicle
    2.Delete a Vehicle
    3.View Inventory
    4.Update Inventory
    5.Export Inventory
    6.Quit
    """)
    ans=input("What would you like to do? ")
    if ans=="1":
        car = Automobile()
        car.add_vehicle()
        vehicle_list.append(car.__str__())

    elif ans=="2":
        for i in vehicle_list:
            vehicle_list.pop(int(input('Enter position of vehicle to remove: ')))
            print('Successfully removed vehicle')
    elif ans=="3":
        print(vehicle_list)
    elif ans=="4":
        edit(vehicle_list)
    elif ans=='5':
        f = open('vehicle_inv.txt', 'w')
        f.write(str(vehicle_list))
        f.close()
    else:
        print('try again')

The assignment mentions private fields. In Python those are created (faked, really) by prefixing the field with two underscores, as in __make.
Interactive command line programs are a scourge, because they are a pain to automate (including for testing purposes). Have a look at argparse to create a non-interactive program which would be called for example as ./automobile.py add "My car". In general there are no input calls in 99.998% of production code.
I'm not sure whether this recommendation is Pythonic, but I prefer creating a pretty-print method rather than overriding __str__ for printing the object in a user readable format.
The edit method takes a parameter which shadows an existing name. This is a common source of confusion and bugs.
add_vehicle should be inlined into __init__. It doesn't actually add a vehicle (which would mean adding it to something, such as vehicle_list), but rather initializes the vehicle fields, making it effectively a constructor.
Your code does not print the existing fields of a vehicle before replacing it. This may or may not be outside the scope of the assignment, but it is a usability issue.
The user variable serves no purpose and can be inlined.
edit asks for a one-offset index but when deleting a vehicle you use a zero-offset. You can use one or the other (ideally zero-offset for libraries and one-offset for user facing stuff) but it should be consistent.
Use with open(...) to guarantee closing the file cleanly.


Multiple SQL queries, one per ID ids contains 100 ids that are comma delimited.  Is it fine or does it need improvement?

 def getFriendPoints(ids: String) = {
    DB.withConnection { implicit c =>
      try {
        var pointMap:scala.collection.immutable.Map[String,String]=Map()
        var i=ids.split(",")
        for(pos<-0 to i.size){
        val result = SQL("SELECT  point,privacy FROM `tableName` WHERE id={user}").on("user" -> i(pos)).apply().head
        val point = if (result[Boolean]("privacy")) { result[Double]("point").toString } else { "0" }
        pointMap+=("id"+pos -> i(pos), "point" -> point)
        }
        pointMap
      } catch {
        case e: Exception =>
          Map("result" -> "error")
      }
    }
  }

Preamble: I don't know scala. All code you see here is improvised from the question and a few bits of scala that I've seen somewhen. Improvements and comments are welcome
Naming:
I am going to be blunt here... Your names are bad. Not horrible, but seriously bad.

var i = ids.split(",");

first thing jumping me here is, that you have a single thing (the id-string) named with a plural. next thing that jumps me, is that you have a collection with multiple elements and it's name is a single letter.
Rename that stuff:

ids -> idString
i -> idCollection (or even ids, anything is better...)

on we go:

for(pos<-0 to i.size){

usually the iterator in a for loop has your single letter identifier, extremely often, that is our friend i. Also you don't really have to cramop that all together. Give your variables and statements some fresh air:
for (i <- 0 to idCollection.size) {

looks much more readable IMO ;)

val result = SQL([...]

result is a dumb name. result of what? yes, I know it's the SQL statment, but if you just skim it, you could think it's what you want to return in the end.
instead I'd use dbRecord or similar.
Alright, so much for the names, now we get to the real wtf
Storing numbers:
WHY DO YOU STORE NUMBERS AS STRINGS?
AND WHY DO YOU STORE POINTS IN A SEPARATE ENTRY? pardon caps
Don't shoehorn everything into Strings:
var pointMap:scala.collection.immutable.Map[String,Double]=null
[...]
val point:Double = if (result[Boolean]("privacy")) {
    result[Double]("point")
} else {
    0.0
}
pointMap+=("id" + idCollection(i) -> point)


back_inserter for several container arguments Recently I wrote a back_inserter with the same interface as the std::back_inserter just for supporting several containers in the constructor and doing push_back for each. The solution needs C++17. Please give me some feedback! Thanks.

#include <iterator>
#include <tuple>

namespace stx {

template <class Container, class... Containers>
class back_insert_iterator
{
public:
    using value_type        = void;
    using difference_type   = void;
    using pointer           = void;
    using reference         = void;
    using iterator_category = std::output_iterator_tag;

    explicit back_insert_iterator(Container& container, Containers&... containers) : containers(container, containers...) {
    }

    back_insert_iterator& operator=(const typename Container::value_type& value) {
        std::apply([&](auto&... container) { ((container.push_back(value)), ...); }, containers);
        return *this;
    }

    back_insert_iterator& operator=(typename Container::value_type&& value) {
        std::apply([&](auto&... container) { ((container.push_back(std::move(value))), ...); }, containers);
        return *this;
    }

    back_insert_iterator& operator*() {
        return *this;
    }

    back_insert_iterator& operator++() {
        return *this;
    }

    back_insert_iterator& operator++(int) {
        return *this;
    }

private:
    std::tuple<Container&, Containers&...> containers;
};

template <class Container, class... Containers>
auto back_inserter(Container& container, Containers&... containers) {
    return back_insert_iterator<Container, Containers...>(container, containers...);
}

}

And you can use it like this:

#include <iostream>
#include <vector>

auto main() -> int {

    const auto numbers = std::vector<int>{ 1, 2, 3, 4, 5 };

    std::vector<int> copy1, copy2;

    std::copy(numbers.begin(), numbers.end(), stx::back_inserter(copy1, copy2));

    std::cout << "(numbers == copy1) = " << std::boolalpha << (numbers == copy1) << '\n';
    std::cout << "(numbers == copy2) = " << std::boolalpha << (numbers == copy2) << '\n';

};

A front_inserter would of course look similar, just calling push_front instead. Some specific additional questions:

does the operator= method with the r-value work as intended or do I need to pass the value differently to the lambda?
do I need an extra static_assert to check that each Container supports the same value_type or at least some that are convertible to avoid overloading the user with template errors in case they are different?
is there another solution where I could let the user provide and optional argument for execution policy, for example std::execution::par
the original std::back_inserter saves the container as pointer - is there any advantage versus my inserter which saves them per reference?

The general structure seems fine to me. However, some things can still be improved!
Iterator requirements
Iterators are required to satisfy the Swappable and CopyAssignable named requirements, but the current implementation doesn't. (This is also why the std::back_inserter implementation you looked at used pointers instead of references, the latter cannot be reseated.)
And from a design perspective, this provides an interesting dilemma: Do you want to support assignment from different underlying container types? For example, like this:
std::deque<int> copy1{};
std::list<int> copy2{};

auto out = stx::back_inserter(copy1, copy2);
// out is of type stx::back_inserter_iterator<std::deque<int>, std::list<int>>

std::vector<int> copy3{};

out = stx::back_inserter(copy2, copy3, copy1);
// assignment from stx::back_inserter_iterator<std::list<int>, std::vector<int>, std::deque<int>>

If no, the solution is easy: Change the container references to pointers (to make assignment possible), add assignment operator overloads, and either use std::swaps default implementation or write your own swap function for stx::back_inserter_iterator.

This is the easy way, but in reality likely not very useful, since assignment only allows for the exact same container types.

If yes, the solution becomes more complicated, as the actual container types need to be abstracted away. One idea for this is using inheritance in an external storage class (rough idea):
template<typename ValueType>
class container_storage_base {
public:
    virtual ~container_storage_base() {}
    virtual void push_back(const ValueType&) = 0;
    virtual void push_back(ValueType&&) = 0;
};

template<typename Container, typename... Containers>
class container_storage : container_storage_base<typename Container::value_type> {
    std::tuple<Container&, Containers&...> containers; // can use references here

public:
    void push_back(const typename Container::value_type& value) override { ... }
    void push_back(typename Container::value_type&& value) override { ... }
};

Inside of stx::back_inserter_iterator, you could then simply store a std::shared_ptr<container_storage_base<ValueType>>, thus freely allowing assignment between different stx::back_inserter_iterator<ValueType> instances (as long as ValueType remains the same).

Why std::shared_ptr?
We need some indirection in order to allow for virtual dispatch to work, and using a pointer seems to be the easiest solution for that. However, we also need to release the container_storage object once we no longer need it, and for that, smart pointer come in very handy.
But: Iterators need to be copyable, and that isn't something supported by std::unique_ptr (only allowing for a unique owner), thus std::shared_ptr was chosen.

Other Nitpicks

Most functions could be marked noexcept (even if only conditionally).

Q & A

does the operator= method with the r-value work as intended or do I need to pass the value differently to the lambda?

Works as intended.
Well, mostly. With the expansion, value will be moved from for every container, i.e. every container but the first will move from an already moved-from value. To get the initial value to all containers, copies will need to be made.

do I need an extra static_assert to check that each Container supports the same value_type or at least some that are convertible to avoid overloading the user with template errors in case they are different?

Do you need to? Not necessarily.
Would it be nice for users of the implementation? Absolutely!

is there another solution where I could let the user provide and optional argument for execution policy, for example std::execution::par

I'm confused: Execution policies are for algorithms, not iterators themselves.
Of course, you can always add a data member to store a value if you want to, but I fail to see how storing an execution policy would be helpful here, as there aren't any calls to algorithms internally (and just adding an execution policy to data doesn't accomplish anything).

the original std::back_inserter saves the container as pointer - is there any advantage versus my inserter which saves them per reference?

See above, you cannot reassign references.


Simple HTML5 registration form I'm creating a simple registration form for a Linux installation party I'm organizing.

Although I'm a decent programmer, I rarely create HTML files. I want to know if this form is safe for my users and if it will work on most devices. Am I following all the good web development practices? Is my code pure HTML5?

<!DOCTYPE html>

  <meta name="viewport" content="width=device-width, initial-scale=1.0">
  <meta charset="UTF-8">
  <meta name="description" content="Installation fest inscription form">
  <meta name="author" content="name - email@example.com">
  <title>Installfest Inscription</title>

Installfest Inscription

  <form action="/action_page" method="post">
        First name:

        <input type="text" name="firstname">

        Last name:

        <input type="text" name="lastname">
        Laptop model:

        <input type="text" name="model">
  </form>

Thanks!

I wanted to know if this form is safe for my users and if it will work on most devices.

Your HTML form will definitely work on all devices. But as for being 'safe for users', that would probably depend on your backend code you are using to submit your form eg PHP. You would use PHP (or some other backend language eg Python) to handle the submitted data. The browser would decide if the form is safe enough based on how safely and securely you encrypt and submit the data.

  Am I following all the good web development practices?

You have followed most of the good programming practices for web development. Though there aren't many things I can improve on for such a simple webpage, there are some minor problems:

It is more conventional to use
 not
 because it is a self-closing tag. Adding the / in the tag tells the browser not to waste time looking for the (non-existent) closing
 tag.
And it is not a good idea to place your input labels as 'naked' untagged text. Instead, use label tags:

Firstname:
<input type="text" name="firstname" />

Web developers usually write the doctype in lowercase, like this:

<!doctype html>

not this:

<!DOCTYPE HTML>

But there is no major difference.

  Is my code pure HTML5?

This is all pure HTML5, I see no other programming language (eg JS) or markup language (eg CSS) in your sample code.


Speed up weighted average for Leela Chess Zero Problem: Given a vector of about 40 values m with normal error sd compute the weighted average of the values weighted by the chance that they are the maximum.

I have come up with 2 different approaches to solve this. The first is numerical integration, the second is monte-carlo sampling.

import numpy as np
from scipy import integrate
from scipy.stats import norm

m = np.arange(-1,1,.03)
sd = np.ones(len(m))*.12

def integ():
    n = len(m)
    dx = .05
    ts = np.arange(-2, 2, dx)
    cdfs = np.ones(len(ts))
    for i in range(n):
        cdfs *= norm.cdf(ts, m[i], sd[i])

    def func(i, x):
        return norm.pdf(x, m[i], sd[i]) / norm.cdf(x, m[i] , sd[i]) * cdfs

    ans = np.zeros(n)
    for i in range(n):
        ys = func(i, ts)
        ans[i] = integrate.simps(ys, dx=dx)
    ans *= m
    return ans

def monte():
    nn=10000
    a = np.random.normal(size=(len(m),nn))*sd[:,None]+m[:,None]
    vals = (a==a.max(axis=0)).sum(axis=1)/nn
    vals *= m
    return vals

Both of these solutions work, but they are in the range of 1/1_000 of a second rather than the 1/1_000_000 of a second I am looking for.

This code is a prototype that I will eventually be writing in c++, but I want to make sure that this function is possible to calculate quickly enough for the change to be worth it, so I really don't care about formatting or readability, just performance.

Possible improvements:

Calculate for less than 40 variables. Take top 5 and calculate the probabilities for them. The Monte-Carlo shows that the probabilities go down fast. And reducing the number of variables will significantly reduce complexity.
Only take variables that are 2 standard deviations from the maximum
In Monte-Carlo you spend 83% time in generating normally distributed random variables. Check lagged Fibonacci generator. Link
Monte-Carlo numpy calculations could all be combined into one loop in a lower level language (numba, C++, C).
Use pairwise probabilities of all variables with maximum variable to approximate the probabilities.

Code for profiling Monte-Carlo:

%load_ext line_profiler
%lprun -f monte monte()

Output:

Timer unit: 3.52617e-07 s

Total time: 0.0399899 s
File: <ipython-input-2-5df66cc29248>
Function: monte at line 1

Line #      Hits         Time  Per Hit   % Time  Line Contents
==============================================================
     1                                           def monte():
     2         1         20.0     20.0      0.0      nn=10000
     3         1      94115.0  94115.0     83.0      a = np.random.normal(size=(len(m),nn))*sd[:,None]+m[:,None]
     4         1      19254.0  19254.0     17.0      vals = (a==a.max(axis=0)).sum(axis=1)/nn
     5         1         19.0     19.0      0.0      vals *= m
     6         1          1.0      1.0      0.0      return vals


Retry a Promise resolution N times, with a delay between the attempts I want some JavaScript code to take 3 things as parameters:

A function returning a Promise.
The maximum number of attempts.
The delay between each attempt.

What I ended up doing is using a for loop. I did not want to use a recursive function : this way, even if there are 50 attempts the call stack isn't 50 lines longer.

Here is the typescript version of the code:

/**
 * @async
 * @function tryNTimes<T> Tries to resolve a {@link Promise<T>} N times, with a delay between each attempt.
 * @param {Object} options Options for the attempts.
 * @param {() => Promise<T>} options.toTry The {@link Promise<T>} to try to resolve.
 * @param {number} [options.times=5] The maximum number of attempts (must be greater than 0).
 * @param {number} [options.interval=1] The interval of time between each attempt in seconds.
 * @returns {Promise<T>} The resolution of the {@link Promise<T>}.
 */
export async function tryNTimes<T>(
    {
        toTry,
        times = 5,
        interval = 1,
    }:
        {
            toTry: () => Promise<T>,
            times?: number,
            interval?: number,
        }
): Promise<T> {
    if (times < 1) throw new Error(`Bad argument: 'times' must be greater than 0, but ${times} was received.`);
    let attemptCount: number;
    for (attemptCount = 1; attemptCount <= times; attemptCount++) {
        let error: boolean = false;
        const result = await toTry().catch((reason) => {
            error = true;
            return reason;
        });

        if (error) {
            if (attemptCount < times) await delay(interval);
            else return Promise.reject(result);
        }
        else return result;
    }
}

The delay function used above is a promisified timeout:

/**
 * @function delay Delays the execution of an action.
 * @param {number} time The time to wait in seconds.
 * @returns {Promise<void>}
 */
export function delay(time: number): Promise<void> {
    return new Promise<void>((resolve) => setTimeout(resolve, time * 1000));
}

Is this a "good" way of doing it? And if not, how could I improve it?

Call stack / Job queue / Event queue

Calling an async function does not use the call stack in the way you may be used to.

Rather the async call remains on the call stack only until an await is encountered. Then the call is removed from the call stack and the await (a micro task (I think it is called)) is placed in the job queue (new to ES6). Micro tasks are very similar to the events but have higher priority (micro tasks will be executed before events)

That means that async functions can not overflow the call stack if you call it recursively as long as there is an await before the recursion.

Awaiting the recursion call will not work as the code to the right of await is executed before the current context is removed from the call stack.

The following JS example shows a recursive count down from 1billion (it will take some time but will not throw a call stack overflow)

Eg

async function recursive(c = 1e9) { // 1 billion
    if (c > 0) {
        await Promise.resolve(); // removes call recursive from call stack
                                 // adds this micro task to the job queue
                                 // Execution is idle and thus the micro task
                                 // is removed from the queue and next line is
                                 // executed
        return recursive(c - 1);
    }
    return c;
}
recursive().then(console.log); // will in time display 0

Recursion

There is no danger in using a recursive and less complex style for your solution. The example below outlines the principle in JS (I am too lazy to type typescript)

const MAX_TRYS = 10; TRY_TIMEOUT = 500;
function toTry() {
    return new Promise((ok, fail) => {
        setTimeout(() => Math.random() < 0.05 ? ok("OK!") : fail("Error"), TRY_TIMEOUT);
    });
}
async function tryNTimes(toTry, count = MAX_TRYS) {
    if (count > 0) {
        const result = await toTry().catch(e => e);
        if (result === "Error") { return await tryNTimes(toTry, count - 1) }
        return result
    }
    return `Tried ${MAX_TRYS} times and failed`;
}

tryNTimes(toTry).then(console.log);

Promise resolve/reject are micro tasks

I will point out that even if you used just plain old promises rather than a async function the result is the same. A promise adds a micro task to the job queue and will not resolve or reject until the current call stack is empty.

It is therefor safe to recursively call the calling function from the resolved/rejected promise.


VueJS to do list - handling clear event on input I made a basic to-do list in VueJS, but it feels really clunky and obtuse to me. One of the key things is that if the text input value is empty and the user clicks the button labeled submit then it shows a red error. When you start typing, it goes away. I tried declaring the variables in a different scope but it breaks the application. See full sample below:

// components
Vue.component('todoitem', {
  template: "
Test Item
"
})

// app code
var app = new Vue({
  el: '#app',
  data: {
    todos: [],
    button: {
      text: 'Hide'
    },
    seen: true
  },
  methods: {
    addItem: function() {
      let item = document.getElementById("list-input").value;
      let error = document.getElementById("error");
      if (item == "") {
        error.style.display = "block";
      } else {
        app.todos.push({ text: item });
        error.style.display = "none";
      }
    },
    removeItem: function(index) {
      this.todos.splice(index, 1);
    },
    removeError: function() {
     let item = document.getElementById("list-input").value;
      let error = document.getElementById("error");
     if (item == "") {
        error.style.display = "block";
      } else {
        error.style.display = "none";
      }
    },
    toggleSeen: function() {
      app.seen = !app.seen;
      app.button.text = app.seen ? 'Hide' : 'Show';
    }
  }
});
#app {
  width: 900px;
  padding: 2em;
  margin: 0 auto;
}

.todo-list {
  list-style-type: square;
}

.todo-list__delete {
  display: none;
}

li:hover .todo-list__delete {
  display: inline-block;
}

Add an Item to the list

      {{ todo.text }}
      Delete

  <input v-on:keyup="removeError" type="text" id="list-input">
  <input type="submit" id="list-submit" v-on:click="addItem">
  Please Enter Text

    <todoitem></todoitem>

SEEN

  <button id="hide-seen" v-on:click="toggleSeen">{{ button.text }}</button>

How can I make this better?

DOM references

If you are using functions to lookup DOM elements (e.g. document.getElementById()) for elements contained within the root element (or component), it might likely be the wrong way of doing things, as far as VueJS code goes. With VueJS, you can use v-model to bind the value of the input to a data property - for example, the <input> for the new list item can have a v-model attribute added, like:

<input v-on:keyup="removeError" type="text" id="list-input" v-model="newItem">

And it would be wise to add an entry in the data object for that model value:

data: {
    todos: [],
    //...
    newItem: ''

Then when creating an item, use this.newItem for the value.

Conditional Rendering

The error message can be conditionally rendered using v-if (or v-show, depending on how the DOM should be laid out in various cases). Read more about conditional rendering here.

That way there is no need to lookup the DOM element and set the style to display: block, display: none, etc.

Method context

In the method toggleSeen() the variable app is used, but realize that the method is called in the context of app so this can be used instead (of app).

Computed properties

Use computed properties to control values that depend on other values. For example, the button text can be added to a computed property. Remove button from data and then add a computed property:

computed: {
    button: function() {
      return {
        text: this.seen ? 'Hide' : 'Show'
      };
    }
  }

That way the logic for the text property can be removed from the method toggleSeen(). Then that method call could be replaced by an inline expression, or else a checkbox could be used with the v-model attribute bound to seen.

// components
Vue.component('todoitem', {
  template: "
Test Item
"
})

// app code
var app = new Vue({
  el: '#app',
  data: {
    todos: [],
    seen: true,
    showError: false,
    newItem: '',
  },
  computed: {
    button: function() {
      return {
        text: this.seen ? 'Hide' : 'Show'
      };
    }
  },
  methods: {
    addItem: function() {
      this.showError = (this.newItem == "");
      if (!this.showError) {
        app.todos.push({
          text: this.newItem
        });
        this.newItem = '';
      }
    },
    removeItem: function(index) {
      this.todos.splice(index, 1);
    },
    removeError: function() {
      this.showError = false;
    },
    toggleSeen: function() {
      this.seen = !this.seen;
    }
  }
});
#app {
  width: 900px;
  padding: 2em;
  margin: 0 auto;
}

.todo-list {
  list-style-type: square;
}

.todo-list__delete {
  display: none;
}

li:hover .todo-list__delete {
  display: inline-block;
}

Add an Item to the list

      {{ todo.text }}
      Delete

  <input v-on:keyup="removeError" type="text" id="list-input" v-model="newItem">
  <input type="submit" id="list-submit" v-on:click="addItem">
  Please Enter Text

    <todoitem></todoitem>

SEEN

  <button id="hide-seen" v-on:click="toggleSeen">{{ button.text }}</button>


Change random list of numbers into all 5's using simple evolution simulation This is supposed to be an evolutionary algorithmic program, and I'm not sure if it does what it's supposed to do.

It is supposed to take a random list of numbers (containing at least one 5), and over many generations, increase the probability of 5's reproducing, thus having the 5's eventually taking over the output.

Is this an accurate representation of an evolutionary algorithm? If so, how can I make it better, and if not, how can I make it one?

import random as r
import os
import sys
import time
from subprocess import call
#call('color a', shell=True)
class Entity(object):
    def __init__(self, number):
        self.number = number
        self.chance = 10
        self.age = 0
class Fitness(object):
    def __init__(self, population):
        self.population = population

    def StartPopulation(self, pop, entities):
        for x in range(pop):
            entity = Entity(r.randint(1, 10))
            entities.append(entity)

    def FitnessMethod(self, entities):

        for x in entities:
            if x.number == 5:
                x.chance += 10
            else:
                x.chance += 0

            if x.chance > 100:
                x.chance = 100

            else:
                x.chance += 0

    def Reproduce(self,entities):
        x = entities[r.randint(0, (len(entities) - 1))]
        if r.randint(0, 100) < x.chance:
            random_children = r.randint(1, 3)
            for a in range(random_children):
                entity = Entity(x.number)
                entity.chance = x.chance
                entities.append(entity)
            x.age += 1

    def Aging(self, entities):
        for x in entities:
            if x.age >= 1:
                entities.remove(x)

pop = r.randint(5, 10)
entities = []
Fitness = Fitness(pop)
Fitness.StartPopulation(pop, entities)
for x in entities:
    print x.number, x.chance
print "~~~~~~~~~~~~~\n"
raw_input()

for x in range(200):
    Fitness.FitnessMethod(entities)
    Fitness.Reproduce(entities)
    Fitness.Aging(entities)
    for x in entities:
        print x.number, x.age, x.chance
    print "Generation_Mutation_Complete"
raw_input()

Picking up where Josay left off:
Simulating evolution
The current algorithm has some flaws as an evolution simulation:

Entities inherit their parents' fitness. Fitness should be determined by the entity's traits; it isn't a trait in its own right.
Entities only age when they reproduce, and the number of offspring doesn't depend on the fitness. This means all entities reproduce eventually; fitter ones just do it faster! Furthermore, the number of offspring doesn't depend on the fitness, so all entities have the same reproductive success! In an evolution simulation, fitter organisms should have more reproductive success.

Some other oddities:

Only one organism per step can reproduce. Why not let them all reproduce at once?
Randomness is introduced in too many places: in particular, the fitness is random, but it's also compared to a random number. You only need one of these. It's better to keep randomness out of the fitness function entirely, so its results are more meaningful and it's easier to debug.
The population grows without bound. It should probably be limited, perhaps with random.sample.

It's not necessary to model aging at all in a simple evolution simulator. You can simply have every organism die in every generation. Or you can have them die randomly, without keeping track of age.
There are two important parts of a genetic algorithm:

The fitness function: given an organism, how successful is it?

The breeding function: Given a population and a fitness function, produce the next generation. There are a lot of options here: you can choose parents randomly weighted by fitness, or let everything reproduce a few times and keep the fittest children, or let everything reproduce according to its fitness and keep random children.

Design
Fitness does not make much sense as a class, because its instances don't represent anything. If each instance represents a population, it could be a Population. But it's simpler to represent a population as a list of organisms, with no separate class. (Not everything needs to be a class!)
Some of the methods of Fitness should be operations on a single Entity, not a whole population.
Several functions modify objects unnecessarily. In particular, fitness_method should simply return a fitness rather than modifying the Entity. (This causes a bug: the fitness of 5 increases over time!)
Minor style issues
Some names could be improved:

fitness_method is fitness. Method names should say what they do, not the fact that they're methods.
aging should be removeOldOrganisms.
Entity is vague. Organism is more specific. Populations can be called population, not entities.

The loop to print a population is repeated and should be factored out.
raw_input can print a prompt, so the separate print before it is unnecessary. But there's no need to wait for user input at all.
It's not necessary to rename random.
How I'd do it
Here's a version with most of the above changes:
import random

class Organism:
    def __init__(self, number):
        self.number = number
    def fitness(self):
        "How many children can this organism have?"
        return 3 if self.number == 5 else 2
    def reproduce(self):
        "Make a child. Currently children are just copies of the parent."
        return Organism(self.number)

max_population = 100

def nextGeneration(population):
    children = []
    for x in population:
        n_children = random.randint(0, x.fitness())
        children += [x.reproduce() for _ in range(n_children)]
    return random.sample(children, min(len(children), max_population))

def printPopulation(population):
    for x in population:
        print x.number, x.fitness()

population = [Organism(random.randint(1, 10)) for _ in range(10)]
printPopulation(population)

for gen in range(10):
    print "Generation", gen, "population", len(population)
    population = nextGeneration(population)
    printPopulation(population)

This uses fitness to determine the number of children rather than which children survive; either is reasonable.


Application that processes XML based on configuration I used CodeClimate to evaluate my code and marked that the following piece of code appeared twice in my source:

    except IOError as e:
        log.error("I/O error({0}): {1}".format(e.errno, e.strerror))
        app_exit(1)

The justification is that the amount of duplication mass is 33, which is above default limit of 32.

How would you solve this problem? Would you disable the check? Would you wrap the 2 lines in a new method? Something else?

The duplicate part in question is in main:

#!/usr/bin/env python

import logging as log

def get_cfg(xmlfile):
    # gets configuration from file which
    # has the same name as XML
    # but with different extension.
    pass

def get_root(xmlfile):
    pass

def app_exit(status):
    log.info('Exiting...')
    exit(status)

def setup_logging(args):
    if args.verbose == 2:
        log.basicConfig(level=log.DEBUG)
    elif args.verbose == 1:
        log.basicConfig(level=log.INFO)
    else:
        log.basicConfig(level=log.WARN)

def main():
    import argparse

    parser = argparse.ArgumentParser()
    parser.add_argument('-f', '--file', help='XML file to be parsed')
    parser.add_argument("-v", "--verbose", action="count",
                        help="increase output verbosity")

    args = parser.parse_args()

    setup_logging(args)

    xmlfile = args.file
    log.info('Getting root element from XML {}.'.format(xmlfile))
    try:
        root = get_root(xmlfile)
    except ET.ParseError as e:
        log.error("Error while parsing {0}: {1}".format(xmlfile, e))
        app_exit(1)
    except IOError as e:
        log.error("I/O error({0}): {1}".format(e.errno, e.strerror))
        app_exit(1)

    log.info('Getting configuration of XML {}.'.format(xmlfile))
    try:
        cfg = get_cfg(xmlfile)
    except IOError as e:
        log.error("I/O error({0}): {1}".format(e.errno, e.strerror))
        app_exit(1)

    # processing of XML using configuration

if __name__ == '__main__':
    main()

Pardon my lack of Python knowledge, but I think a function something like this might work:

def fatal_error(message):
    log.error(message)
    app_exit(1)

And now your catch cases look more like this:

try:
    root = get_root(xmlfile)
except ET.ParseError as e:
    fatal_error("Error while parsing {0}: {1}".format(xmlfile, e))
except IOError as e:
    fatal_error("I/O error({0}): {1}".format(e.errno, e.strerror))

log.info('Getting configuration of XML {}.'.format(xmlfile))
try:
    cfg = get_cfg(xmlfile)
except IOError as e:
    fatal_error("I/O error({0}): {1}".format(e.errno, e.strerror))


Vitaly & Pie Programming challenge I've been trying to solve this problem on Codeforces:

  Challenge description:

  After a hard day Vitaly got very hungry and he wants to eat his
  favorite potato pie. But it's not that simple. Vitaly is in the first
  room of the house with n room located in a line and numbered starting
  from one from left to right. You can go from the first room to the
  second room, from the second room to the third room and so on — you
  can go from the (n - 1)-th room to the n-th room. Thus, you can go to
  room x only from room x - 1.

  The potato pie is located in the n-th room and Vitaly needs to go
  there.

  Each pair of consecutive rooms has a door between them. In order to go
  to room x from room x - 1, you need to open the door between the rooms
  with the corresponding key.

  In total the house has several types of doors (represented by
  uppercase Latin letters) and several types of keys (represented by
  lowercase Latin letters). The key of type t can open the door of type
  T if and only if t and T are the same letter, written in different
  cases. For example, key f can open door F.

  Each of the first n - 1 rooms contains exactly one key of some type
  that Vitaly can use to get to next rooms. Once the door is open with
  some key, Vitaly won't get the key from the keyhole but he will
  immediately run into the next room. In other words, each key can open
  no more than one door.

  Vitaly realizes that he may end up in some room without the key that
  opens the door to the next room. Before the start his run for the
  potato pie Vitaly can buy any number of keys of any type that is
  guaranteed to get to room n.

  Given the plan of the house, Vitaly wants to know what is the minimum
  number of keys he needs to buy to surely get to the room n, which has
  a delicious potato pie. Write a program that will help Vitaly find out
  this number.

  Input

  The first line of the input contains a positive integer n
  (2 ≤ n ≤ 105) — the number of rooms in the house.

  The second line of the input contains string s of length 2·n - 2.
  Let's number the elements of the string from left to right, starting
  from one.

  The odd positions in the given string s contain lowercase Latin
  letters — the types of the keys that lie in the corresponding rooms.
  Thus, each odd position i of the given string s contains a lowercase
  Latin letter — the type of the key that lies in room number
  (i + 1) / 2.

  The even positions in the given string contain uppercase Latin letters
  — the types of doors between the rooms. Thus, each even position i of
  the given string s contains an uppercase letter — the type of the door
  that leads from room i / 2 to room i / 2 + 1.

  Output

  Print the only integer — the minimum number of keys that Vitaly needs
  to buy to surely get from room one to room n.

I keep exceeding time limit on one of the test cases. I made several changes (i.e. using try/except instead of 'if a in b') to my program but it still can't pass that test.

My last version is this:

x = input()
pos = input()
keys = []
keys_short = []
toBuy = 0
for i in pos:

    if i.islower():
        keys.append(i)
        keys_short = list(set(keys))
    else:
        try:
            b = keys_short.index(i.lower())
        except ValueError:
            toBuy += 1
        else:
            keys.remove(i.lower())
            keys_short = list(set(keys))
print(toBuy)

Can you suggest any changes to make it faster?

Correctness. It is possible that Vitaly on his way picks up multiple keys of the same kind. Your code implies that there is no more than one of each kind.
Performance. As suggested in comments, conversions back and forth from lists to sets are expensive. A dictionary key_type: key_count is most likely what you need.


Updating multiple collections I'm currently building a MongoDB which has collections of 2 types:

A series of "dialog" collections
A collection storing the version numbers of each "dialog"

I'm looking to build an API, where-in I increment the version of the dialogs whenever that particular "dialog" is amended.

Basically I'll be updating multiple collections and I want to ensure that my code is appropriate.

On a given POST route I have something like:

router.post('/:dialogName', function(req,res){

    const dialogName = req.params.dialogName;
    const subId = req.body.subID;
    const newArray = req.body.newArray;
    const collection = db.get().collection(dialogName);

    collection.update({
        "synonyms._id": new ObjectId(subId)
    },
        {
            $set: {"synonyms.$.text": newArray}
        }
    ).then(function(doc){
        return updateVersions(dialogName).then(function(){
            console.log('version updated')
        })
        .catch(function(err){
            console.log('version not saved')
        })
    }).then(function(doc){
        res.status(200).end()
    }).catch(function(err){
        res.status(400).send("Couldnt update question")
    })
})

So once I successfully amend a doc, I invoke a promisified function that does this:

const updateVersions = function(dialog){
    return new Promise(function(resolve,reject){
        const versionCollection = db.get().collection('Version')
        versionCollection.update({
            "dialogs.name": dialog
        },{
            $inc: { "dialogs.$.__v": 1 , "__v": 1}
        }).then(function(response){
            resolve('ok')
        }).catch(function(err){
            console.log('errr : ',err)
            reject('err')
        })
    })
}

Is my approach correct?

I really think your code is having at least two issues. The first one is something that may confuse your service consumers a lot. The second one is probably a bug.

Careful With Status Codes

.catch(function(err){
  res.status(400).send("Couldnt update question")
})

This is not following RESTful approach. This .catch will be invoked if your code could not update a question. Meaning, the service was not able to fulfill a request. The correct HTTP response code for such situation is 500 Internal Server Error.

You should only return 400 Bad Request if the request was validated and there were issues with the payload (request body received). In such case, HTTP 400 is truly the best option, but in order to improve user experience, you should also provide the information about what exactly was wrong with the request. E.g. HTTP 404 + response payload

{
  "error": {
    "referenceId": "TOO_LONG_QUESTION",
    "description": "Question payload should not exceed 1024kb, however received 2048kb."
  }
}

The response format is actually arbitrary. Just think about what would be useful for a developer to know so that they can start troubleshooting in a right place.

Possible Bug

I numbered the code lines to make it easy to refer to them.

( 1)  collection.update({
( 2)    "synonyms._id": new ObjectId(subId)
( 3)  },
( 4)    {
( 5)        $set: {"synonyms.$.text": newArray}
( 6)    }
( 7)  ).then(function(doc){
( 8)    return updateVersions(dialogName).then(function(){
( 9)        console.log('version updated')
(10)    })
(11)    .catch(function(err){
(12)        console.log('version not saved')
(13)    })
(14)  }).then(function(doc){
(15)    res.status(200).end()
(16)  }).catch(function(err){
(17)    res.status(400).send("Couldnt update question")
(18)  })

If versionCollection.update(...) in updateVersions(...) fails, the promise will be rejected and we will fall into lines 11-13 which is good. There is one problem though... The promise created in line 1 (collection.update(...)) will NOT fail even though we failed to update the versionCollection. Try debugging it and you will see what I mean.

I think, what you need to do there is to re-throw the error like this:

(11)    .catch(function(err){
(12)        console.log('version not saved');
(12')       throw err;  <<-------------------------
(13)    })

Asynchronosity Is NOT Easy

Yes, it's not easy at all. Even with promises. I really recommend you to read about await/async feature of ES2017 which simplifies things a lot.

With await/async your code would look much more "regular" imperative code. It reads as synchronous but acts asynchronously. Here's a sketch (not a 100% working code):

router
  .post('/:dialogName', async (req, res) => {
    const dialogName = req.params.dialogName;
    const collection = db.get().collection(dialogName);

    try {
      const doc =  await collection.update(
        { "synonyms._id": new ObjectId(req.body.subID) },
        { $set: {"synonyms.$.text": req.body.newArray} }
      );

      try {
        await updateVersions(dialogName);
        console.log('version updated');
      } catch (err) {
        console.log('version not saved');
        throw err;
      }

      res.status(200).end();
    } catch {
      res.status(400).send("Couldnt update question");
    }
  })


First text game with Python - Higher or Lower Recently I have been learning Python programming, for about a week. I have covered all basic stuff - loops, strings, input masks - and have challenged myself by making a little game. It isn't much, that I know, but I was just wandering whether or not I have done everything correctly.

Any feedback regarding functions and methods used would be greatly appreciated.

import random
cards = ["2","3","4","5","6","7","8","9","10","Jack","Queen","King","Ace"]
suits = ["Spades","Hearts","Clubs","Diamonds"]
x = 1
while x < 20:
    for i in range(1,9):
        cardVal = cards[random.randint(0,12)]
        cardNum = cards.index(cardVal)
        suitVal = suits[random.randint(0,3)]
        if i == 1:
            print("Your starting card is the",cardVal,"of",suitVal)
        HoL = str(input("Higher (H), Lower (L) or Same (S): "))
        if HoL == "H":
            newCval = cards[random.randint(0,12)]
            newCnum = cards.index(newCval)
            if newCnum < cardNum + 1:
                print("Sorry, you fail. Card is",newCval,"of",suitVal)
                break
            elif newCnum > cardNum:
                print("Good job! Card is",newCval,"of",suitVal)
            else:
                print("Sorry, you fail. Card is",newCval,"of",suitVal)
                break
        if HoL == "L":
            newCval = cards[random.randint(0,12)]
            newCnum = cards.index(newCval)
            if newCnum > cardNum - 1:
                print("Sorry, you fail. Card is",newCval,"of",suitVal)
                break
            elif newCnum < cardNum:
                print("Good job! Card is",newCval,"of",suitVal)
            else:
                print("Sorry, you fail. Card is",newCval,"of",suitVal)
                break
        if HoL == "S":
            newCval = cards[random.randint(0,12)]
            newCnum = cards.index(newCval)
            if newCnum != cardNum:
                print("Sorry, you fail. Card is",newCval,"of",suitVal)
                break
            else:
                print("Good job! Card is",newCval,"of",suitVal)
    print("Congratulations!")
    repeat = str(input("Enter 'Yes' to run again: "))
    if repeat != "Yes":
        break

Here is an alternative implementation, with explanatory comments below. This introduces quite a few additional Python concepts:

from operator import ge, le, ne
import random

def get_valid_input(choices=set("hls")):
    while True:
        ui = input("Higher (H), Lower (L) or the Same (S)? ").lower()
        if ui in choices:
            return ui
        print("Not a valid choice, please try again.")

def game(rounds=8):

    cards = range(2, 15)
    faces = {11: 'Jack', 12: 'Queen', 13: 'King', 14: 'Ace'}
    suits = ["Spades", "Hearts", "Clubs", "Diamonds"]

    comparisons = {'h': le, 'l': ge, 's': ne}

    pass_template = "Good job! The card is the {0} of {1}."
    fail_template = "Sorry, you fail. The card is the {0} of {1}."

    while True:
        card = random.choice(cards)
        suit = random.choice(suits)
        print("The first card is the {0} of {1}.".format(faces.get(card,
                                                                   card),
                                                         suit))
        for round_ in range(rounds):
            next_card = random.choice(cards)
            next_suit = random.choice(suits)
            if comparisons[get_valid_input()](next_card, card):
                print(fail_template.format(faces.get(next_card,
                                                     next_card),
                                           next_suit))
                break
            print(pass_template.format(faces.get(next_card,
                                                 next_card),
                                           next_suit))
            card, suit = next_card, next_suit
        else:
            print("Congratulations!")
        repeat = input("Play again? ").lower()
        if repeat not in "yes":
            print("Goodbye.")
            break

if __name__ == "__main__":
    game()

Comments:

Note adherence to the Python Style Guide, PEP-0008, e.g. using lowercase_with_underscores for variable names;
random.choice is used rather than hard-coding bounds for range (if you stick with choosing the index, at least assign the index first rather than picking a card based on a random number then using index to get that same number back);
The card is now just an integer, making comparison easy without needing to find the index. The "face" (e.g. 'Jack') is only looked up when displaying to the user (using dict.get to return either the face string if it's in the dictionary or the card value if not);
A dictionary, with the operator module, is also used to reduce duplication in the main loop. The trick is recognising the duplication - in all cases, you are comparing two cards with a particular rule. The following now only happen in one place, reducing chance of error:

Picking the next card;
Comparing the cards; and
Telling the user they've won or lost;

A function forces the user to provide valid input. This will loop until they pick one of 'h', 'l' or 's' (case-insensitive);
while True replaces the odd behaviour with x that never gets incremented; and
The whole thing is wrapped in a function, which makes it easier to import elsewhere later and has also allowed the "magic number" 8 to be variable when calling game.

One additional thing you might want to consider is actually simulating a deck of cards. As it stands, the same card could appear twice, i.e. the game is currently being played as if the card were being put back into the deck each time. This may or may not be what you're aiming for - I would expect to play as if the (up to) eight cards were being laid out on the table one after another.


Minimal entity component system, take 2 Here's a followup to Minimal entity component system. Please see that question for background.

-- ecs.lua

local unpack = table.unpack or _G.unpack

local function addComponent(entity, component, instance)
  entity[component] = setmetatable(instance or {}, { __index = component })
  return entity
end

local function getComponentsTable(entity, components)
  local values = {}
  for i, component in ipairs(components) do
    local value = entity[component]
    if value == nil then return end
    table.insert(values, value)
  end
  return values
end

local function getComponents(...)
  return unpack(getComponentsTable(...) or {})
end

local entityPrototype = {
  addComponent = addComponent,
  getComponents = getComponents,
  getComponentsTable = getComponentsTable,
}

local function createEntity(instance)
  return setmetatable(instance or {}, { __index = entityPrototype })
end

local function createSystem(components, process)
  local function handle(entity)
    local values = getComponentsTable(entity, components)
    if values then
      process(entity, unpack(values))
    end
  end

  return function(entities)
    for i, entity in ipairs(entities) do
      handle(entity)
    end
  end
end

local function each(entities, components)
  local i = 0
  return function()
    while true do
      i = i + 1
      local entity = entities[i]
      if not entity then return end
      local values = getComponentsTable(entity, components or {})
      if values then return entity, unpack(values) end
    end
  end
end

return {
  createEntity = createEntity,
  addComponent = addComponent,
  getComponents = getComponents,
  getComponentsTable = getComponentsTable,
  createSystem = createSystem,
  each = each,
}

This version adds the following features:

System callbacks now receive not only the entity, but also any components listed in the first argument to createSystem. In other words you can do this now:

local rectangleRenderingSystem = ecs.createSystem(
  { 'position', 'rectangle' },
  function (entity, pos, rect)
    ui.drawRect(pos, rect)
  end)

This is nice, because if you switch from string-keyed components to table-keyed components (using addComponent), nothing in the function body needs to change.
There is a new function getComponents which does something similar.

local foo, bar = ecs:getComponents(someEntity, { 'foo', 'bar' })
-- or
local foo, bar = someEntity:getComponents({ FooComponent, BarComponent })

There is a new function each which is pretty useful. It takes an array of entities as its first argument, and an array of components as the second argument. It returns a function which iterates over all of the entities having matching components, returning the entity followed by each component in the list, just like the argument lists the systems callbacks receive. In other words you can do something like this:

     -- stop all the monsters
     -- we don't care about the value of the monster component, only that it exists
     for entity, vel in ecs.each(entities, { 'velocity', 'monster' }) do
       vel.vx = 0
       vel.vy = 0
     end

For a usage example, see Pong game built on a minimal entity component system.

I think your code is very well written, and understandable (even for me, although I'm currently only beginning to learn Lua, and I heard about ECS for the first time now :) )

I would suggest some minor improvements:

Document your methods, including parameters and return types.
In getComponentsTable add a comment explaining, why you break the loop if you hit a nil value (that was not clear for me, how we know that there are for sure no interesting values after this point)
Some parameters are ensured against nil (using or {}), but this is not the case for entity/entities. I suggest, either doing the same, or adding an explicit check for nil at the beginning of the method (and raising an error in case of nil).
The each function iterates indefinitely, until it finds a nil value in the array (I suppose, this means that it reads past the last element). Instead of this, why not just iterate over all the elements of the array, with a normal for loop? I don't see any good reason, but if there is one, I'd document it in a comment.
In some of the for loops, the variable i is not used. The Lua Style Guide recommends using underscores (_) for such cases.

Finally, one more thing: it is really useful that you have a working example for your system. However, I suggest to consider also adding unit tests, for more reasons:
- they ensure correctness if you need to do changes
- they help other to understand the code better
- it is possible them to run/verify them in an automated way (which is not the case for the example)


Implementing checksum add without carry in C I need to write a function in C which performs a checksum using the rule "add without carry". As I understand it, the "add without carry" concept is equivalent to the bitxor operator and given that the bitxor operation is associative and commutative, I am relying on that to perform the calculation. Below is my function. Note that the input is in decimal format and the checksum is of the binary digits with final output being in decimal format.

UCHAR checksumAddWoCarry(int num, ...)
{
    va_list valist;  /* List of variable arguments */
    UINT arg1, arg2; /* The first two arguments from the va_list */
    int i;           /* Iterator */
    UINT result;     /* Storage for returned result */

    va_start(valist, num); /* Start the usage of the list */

    arg1 = va_arg(valist,UINT); /* Pull out the first two variable arguments */
    arg2 = va_arg(valist,UINT);
    result = arg1 ^ arg2;
    if (num > 2) {
        for (i = 2; i < num; i++) {
            result ^= va_arg(valist,UINT);
        }
    }

    va_end(valist);

    return (UCHAR)result;
}

An example call would be

result = checksumAddWoCarry(3, 4,5,6);

where I expect that result is 7.

I would like an answer to two related questions.

Is my logic for the implementation correct? Specifically in
regards to my use of the bitxor operator to perform the checksum?
I want to know, aside from the logic of the implementation, if my
method is a good one. Are there any gotchas I could run into? Is
there a better/faster way?

It appears code is simply exclusive or-ing the arguments.  Sounds like a "add without carry".

"Is my logic for the implementation correct?"
Appears correct except the range of acceptable values is not clearly stated.
Gotcha 1.  What is UINT?  If UINT is narrower than int/unsigned, the arguments will be widened and va_arg(valist,UINT); is problematic.  Recommend unsigned instead.
Gotcha 2. arg1 = va_arg(valist,UINT); arg2 = va_arg(valist,UINT); Only makes sense if n it at least 1 or 2.  Should test for that first.
if (num > 2) {
    for (i = 2; i < num; i++) { looks dodgy.
Simplify

va_list valist;
va_start(valist, num);
unsigned result = 0;
while (num-- > 0) {
  result ^= va_arg(valist, unsigned);
}
va_end(valist);
return result;


Unit tests for a function that calculates demerit points for speeding I'm currently working on a unit testing course (NUnit 3.x). I've been tasked with a simple class to test all edge cases.

using System;

namespace TestNinja.Fundamentals
{
    public class DemeritPointsCalculator
    {
        private const int SpeedLimit = 65;
        private const int MaxSpeed = 300;

        public int CalculateDemeritPoints(int speed)
        {
            if (speed < 0 || speed > MaxSpeed)
                throw new ArgumentOutOfRangeException();

            if (speed <= SpeedLimit) return 0;

            const int kmPerDemeritPoint = 5;
            var demeritPoints = (speed - SpeedLimit)/kmPerDemeritPoint;

            return demeritPoints;
        }
    }
}

And -

using System;
using NUnit.Framework;
using TestNinja.Fundamentals;

namespace TestNinja.UnitTests
{
    [TestFixture]
    public class CalculateDemeritPointsTests
    {
        private DemeritPointsCalculator _demeritPointsCalculator;

        [SetUp]
        public void SetUp()
        {
            _demeritPointsCalculator = new DemeritPointsCalculator();
        }

        // my solutions
        [Test]
        [TestCase(65,0)]
        [TestCase(70,1)]
        [TestCase(75,2)]
        [TestCase(80,3)]
        [TestCase(0,0)]
        [TestCase(60,0)]
        [TestCase(66,0)]
        public void CalculateDemeritPoints_WhenCalled_ReturnsExpectedInt(int speed, int expectedResult)
        {
            var result = _demeritPointsCalculator.CalculateDemeritPoints(speed);
            Assert.That(result, Is.EqualTo(expectedResult));
        }

        [Test]
        public void CalculateDemeritPoints_SpeedLessThan0_ThrowsException()
        {
            Assert.That(() => _demeritPointsCalculator.CalculateDemeritPoints(-1), Throws.InstanceOf<ArgumentOutOfRangeException>());
        }

        [Test]
        public void CalculateDemeritPoints_SpeedGreaterThan300_ThrowsException()
        {
            Assert.That(() => _demeritPointsCalculator.CalculateDemeritPoints(301), Throws.InstanceOf<ArgumentOutOfRangeException>());
        }
    }
}

Am I using to many test cases for CalculateDemeritPoints_WhenCalled_ReturnsExpectedInt?

I was thinking about merging the final two tests into one as they would also work with test cases, but I wasn't sure if I was doing to many.

You probably want to

add a constant for speed 0
create a test for MaxSpeed
create a test case for 64 (it's one edge of the partition class  0 <= x < speedlimit and it could replace 60 which is in the same partition class)

I would keep the 66 test case since it is and edge case.

Even though the edges are cut pretty clear with ints (compared to using float values) you might still want to test them explicitly.


Optimization of a basic Sieve of Eratosthenes algorithm I have the following Sieve of Eratosthenes algorithm:

public static ArrayList<Integer> sieveOfEra(int limit) {
    int crosslimit = (int) Math.sqrt(limit);
    boolean[] sieve = new boolean[limit+1];
    ArrayList<Integer> primes = new ArrayList<Integer>(Arrays.asList(2));

    for (int n = 4; n <= limit; n += 2) {   // mark even mumbers > 2
        sieve[n] = true;
    }

    for (int n = 3; n <= crosslimit; n += 2) {
        if (!sieve[n]) {
            for (int m = n*n; m <= limit; m += 2*n) {
                sieve[m] = true;
            }
        }
    }

    for (int i = 3; i <= limit; i += 2) {
        if (!sieve[i]) {
            primes.add(i);
        }
    }

    return primes;
}

It returns an ArrayList of all the primes up to n. It first marks the composite numbers in a boolean array, then loops through the boolean array to add unmarked indexes as prime numbers in the primes ArrayList.

I'm looking for any feedback in general, but specially for ways to optimize the algorithm.

I understand that I can get rid of marking even numbers, and even including them in the sieve, but I don't understand how it would be implemented.

After adding 2 to the list of primes you are wasting half of the array space for even numbers, even time for crossing them out. Disconnect the number from it's index in the sieve, like in testprime = sieve[2*i + 3]. Now you can step with a stepsize of 1 through the sieve.

If you take that one step further and add 3 to the list of primes, you will notice that all remaining primes are of the form 6i+1 or 6i+5. The cases 6i, 6i+2, 6i+4 are even; 6i+3 is divisible by 3. So you can loop in steps of 6 and check for elements 6i+1 and 6i+5 in one pass.
Thus, there are only 6/6 - 3/6 - 2/6 = 1/6 numbers left to check for, meaning a sixfold speedup.


Supervisors and employees query, with a subselect and inner join I want to replace or improve the SELECT command of tblVisor to make it faster.

Is there any way to improve this SQL command?

SELECT tblVisor.supervisor_id,
       tblVisor.last_name,
       tblVisor.first_names,
       tblVisor.employee_job_profile_id,
       org_employees.last_name,
       org_employees.first_name,
       org_employees.job_code
FROM (SELECT cp_supervisor_properties.supervisor_id,
             persons.last_name,
             persons.first_names,
             cp_supervisor_properties.employee_job_profile_id
      FROM cp_supervisor_properties
      INNER JOIN persons ON persons.person_id = cp_supervisor_properties.supervisor_id) as tblVisor
INNER JOIN org_employees ON org_employees.employee_number = tblVisor.employee_job_profile_id
LIMIT 100

Unless I'm missing something there doesn't seem to be a reason for the
subselect, just join all three tables directly.  Also, INNER is the
default, so you could drop that prefix as well.

The query would then become something like the following:

SELECT cp_supervisor_properties.supervisor_id,
       persons.last_name,
       persons.first_names,
       cp_supervisor_properties.employee_job_profile_id,
       org_employees.last_name,
       org_employees.first_name,
       org_employees.job_code
FROM cp_supervisor_properties
INNER JOIN persons
  ON persons.person_id = cp_supervisor_properties.supervisor_id
INNER JOIN org_employees
  ON org_employees.employee_number = cp_supervisor_properties.employee_job_profile_id
LIMIT 100;

Without more information (about how the data looks and so) I don't see possible performance improvements.  If you have problems with the query time you should probably check for missing indexes on the join columns and in general look into query optimisation.


Exception, errors handling, best practice in WebApi Core I'm making my WebApi project.

My logic is, the controller didn't see data. It only triggers a service that returns objects to it. My simple method from TaskService looks like this:

public Task TakeTaskByUser(int taskId, string userId)
{
    var task = this.GetItem(taskId);

    if (task != null)
    {
        if (task.ApplicationUserId != null)
        {
            throw new TaskTakenByAnotherUserException();
        }

        task.ApplicationUserId = userId;
        task.StartTime = DateTime.Now;

        this.context.Entry(task).State = EntityState.Modified;
        this.context.SaveChanges();

        return task;
    }

    return null;
}

As you can see, in this method I try to find Task and attach Foreign Key to User by his UserId. If Task is already taken by another user, I throw a custom exception.

In my controller, Action looks like this:

[HttpPut("{taskId}/{userId}")]
[Authorize(Roles = "Developer, Manager")]
public IActionResult TakeTaskByUser([FromRoute] int taskId, [FromRoute] string userId)
{
    try
    {
        var task = this.taskService.TakeTaskByUser(taskId, userId.ToString());
        if (task != null)
        {
            return this.Ok(task);
        }

        return this.NotFound();
    }
    catch (TaskTakenByAnotherUserException)
    {
        return this.ValidationProblem();
    }
}

Is using a try-catch block in a controller good practice? Is using any exceptions in WebApi good, or I should use the method posted in this article (I like Level 1, seems easy and fun)?

How do I deal with errors that I know that can appear when someone sends a request?

Exceptions are pretty costly so as a general rule of thumb they should be avoided. Especially when we talk about REST endpoint where response time is crucial.

Also, to my taste you're messing up command-query separation principle in your code. I would have rewritten it roughly like this

[HttpPut("{taskId}/{userId}")]
[Authorize(Roles = "Developer, Manager")]
public IActionResult TakeTaskByUser([FromRoute] int taskId, [FromRoute] string userId)
{
    var task = this.taskService.GetTask(taskId);
    if (task == null)
        return this.NotFound();
    var taskValidationResult = this.taskValidator.Validate(task);
    if (!taskValidationResult.IsSuccess)
    {
        //handle validation failure
    }

    task = this.taskService.TakeTaskByUser(task, userId);

    return this.Ok(task);
}
```


Improving Fibonacci recursion with BigIntegers I've been tasked with making a fast Fibonacci recursive method that uses BigInteger to calculate REALLY big numbers. However, to calculate numbers past 30 it takes almost a MINIMUM of 60 seconds each time. Unfortunately, I have to use some type of recursion in my method even though the iteration way is WAY faster.

public static BigInteger theBigFib(int b) {
        BigInteger[] a = new BigInteger[100000];
        if(b < 2) {
            return BigInteger.ONE;
        }

        if(a[b] != null) {
            return a[b];
        }

        a[b] = theBigFib(b - 1).add(theBigFib(b - 2));
        return a[b];
    }

I have a loop in my main that runs the the method from values 20 to 30.

public static void main(String[] args) {
        final long start = System.nanoTime();
        for (int i = 20; i <= 30; i++) {
            System.out.println(theBigFib(i));
        }
    //  System.out.println(theBigFib(35));  Takes way too long
        final long end = System.nanoTime();
        System.out.println("This program took " + ((end - start) / 1000000000) + " second(s) to run.");
    }

Here are the results from the console. This is from the 2nd time running it which the first time came out to be 443 seconds.

10946
17711
28657
46368
75025
121393
196418
317811
514229
832040
1346269
This program took 227 second(s) to run.

I find this topic of recursion and program efficiency to be super interesting so I'm open to all suggestions. Only thing I cannot use is external libraries. :)

Every time you call theBigFib, you allocate a new BigInteger[] a object.  As a result, you're not caching any previous results.


Recursively walk directories with a callback function This code works, but I want to refine it:

# =======================
# Dir Walk
#
# This will recursively walk a directory
# ========================

function dirwalk(path::AbstractString, fn::Function)
  content = readdir(path)
  for c in content
    p = joinpath(path, c)
    if isdir(p)
      dirwalk(p, fn)
    elseif isfile(p)
      fn(p)
    end
  end
end

First, use a docstring instead of a comment:

"""Recursively walk a directory."""
function [ . . . ]

Using a docstring allows Julia's built-in help features to work with your function.

Next, I would recommend putting the function argument first:

function dirwalk(fn::Function, path::AbstractString)

Why? This allows dirwalk to be used with Julia's do block syntax:

dirwalk("path/to/file") do p
    # do stuff with p, the path
end

which is often nicer than using anonymous function syntax.

Now for the function itself, you don't need a temporary variable to store content. This:

for c in readdir(path)

is shorter and reads just as well. Inside the loop, things generally look fine (except the order of dirwalk could be reversed). Note that your implementation ignores symlinks, which are neither directories nor regular files.

As for style, the Julia style guide recommends 4-space indenting. Some projects use 2-space indenting, however, and that is a personal decision.

Finally, you might be interested in the walkdir function.


Using POST data to call a function from a specified class I've inherited a class in a project which is purposed to execute a function that exists in POST data from a specified class. I've tried to clean this up as much as possible as well as secure it against SQL injection but I'm wondering if I could have done a better job or written this better. Any assistance would be much appreciated!

class RegistrationProceduresController
{
    public function __construct($username, $password, $host, $port, $dbname)
    {
        $SP = new RegistrationProceduresModel($username, $password, $host, $port, $dbname);

        // Sanitize all the incoming POST data and move into an array of its own
        $sanitized = array_map(array($this, 'sanitize'), $_POST);

        // Check whether the method passed via our POST data is a public function within the model, or even exists
        // at all. If not then skip to logging.
        if(method_exists($SP, $sanitized['function']))
        {
            try
            {
                // Execute the function we passed via our post data against the model class post-sanitization.
                $SP->$sanitized['function']();
            }
            catch(exception $ex)
            {
                json_encode("Exception when trying to execute a sanitized function in registration controller: " . $ex->getMessage());
            }
        }
        else
        {
            try
            {
                $val = isset($sanitized['function']) ? $sanitized['function'] . " does not exist in model."  : "was not specified";

                json_encode("Function " . $val);
            }
            catch(exception $ex)
            {
                json_encode("Threw an error after failing to evaluate whether a $_POST method exists. Review registration controller. " . $ex->getMessage());
            }
        }
    }

    /// <summary>
    /// Takes an input and returns it with all HTML characters stripped.
    /// Protects against SQL injection.
    /// </summary>
    /// <param name="$input">Any string based input</param>
    public function sanitize($input)
    {
        try
        {
            return htmlspecialchars(trim($input));
        }
        catch(exception $ex)
        {
            json_encode("Failed to sanitize POST data array in registration controller: " . $ex->getMessage());
        }

        return null;
    }
}

This project is using POST data to call a function since it's being initialized by JavaScript. If there's a better way to call a PHP function from within JavaScript I would definitely, definitely value feedback on it!

Well, I'm going to post it as an answer, simply because comments aren't the right place for this:

  See edit for justification

You can't justify using $_POST data to call functions or methods in a script. never trust the network, POST data is coming from the network, never trust it. Full stop.
It's a piece of cake to send something like __toString, __invoke, __sleep and other magic-methods. They might exist, so they might pass the method_exist check you do. However, what they do is something different entirely.

You're basically calling a method on an object. That implies a finite list of methods that exist and are callable (there's a function called is_callable to chekc just that, BTW).
A safer way might be to have a property (array) containing a white-list of methods that can be invoked through JS requests:

$psMethods = array(
    'ajaxMethod1',
    'ajaxMethod2'
);

As an added bonus, you could use the method names as keys, and assign those keys a couple of arguments that the method might require.
The downside of this (a static array) is that you'll have to add and remove items from this array as you write your code. Don't worry however, help is at hand thanks to get_class_methods:

private $SP = null;
private $spMethods = array();//empty
//in constructor, or wherever you assign $this->SP
public function __construct(SPClass $sp)
{
    $this->SP = $sp;
    $methods = get_class_methods($sp);
    foreach($methods as $method)
    {
        if (substr($method, 0, 5) === 'ajax_')
        {
            $this->spMethods[] = substr($method,5);
        }
    }
}

Here, any method that has to be callable through an ajax request has to be given an arbitrary prefix (ajax_ in this case). That's how you can determine what methods are valid.
Then, when you receive your data:

if (in_array($_POST['function'], $this->spMethods)) //valid post data
    return $this->SP->{'ajax_'.$_POST['function']}();
else

What to do in the else cases? the choice is yours, really: either provide a default, fallback method in $this->SP:

public function ajax_fallback(array $post = null)
{
    return array('invalid/unknown request action', $post);
}

throw an exception or return a valid json encoded string (json_encode should be passed at least an array, strings needn't be encoded)

Resist the temptation to guess what $_POST['function'] should've been. If its contents is invalid, that can only mean one of the following things:

there's an error in either your PHP or JS code, in which case: fix it
clients trying to exploit your ajax call, in which case, don't try to help them by guessing, throw an exception. Amateur hackers aren't worth the code that'll only end up making it easier for them to actually achieve something
connection problems (unlikely), trying to work your way around those is pointless, so don't

Other considerations:
If you are going to post (part of) a function name to your server, don't call that parameter function. Everybody knows what a function is, so it doesn't take long for some spotty teen to realize which post param is worth trying to exploit!
Sending data over ajax isn't secure anyway, I'd not send a function name, but given the info you've provided, I can't tell which is the better way to go. It might be worth while to hash the function names, and compare the hash value to an array of hashes. The chances someone guesses a hash of (part of) a function name, especially when you add salt are very low indeed:

    $this->SP = $sp;
    $methods = get_class_methods($sp);
    foreach($methods as $method)
    {
        if (substr($method, 0, 5) === 'ajax_')
        {
            $this->spMethods[] = sha1(
                'YOUR_sup3rh3r03-0F=çH01ce'.substr($method,5);
            );
        }
    }

and just hard-code, or send these hashes over through a predefined ajax request.
Those teen-would-be-hackers are easily discouraged when they realize they have to use strings like "ff45204b9bbf938f7c10a734ed4e05434203790d", without knowing what they are :-)


Find min of 3 numbers hardcoded public static int min(int a, int b, int c)
{
    int result = 0 ;
    if( a < b && a < c && b < c) result = a ;
    else if( a < b && a < c && b > c) result = a ;

    else if( a > b && a < c && b < c) result = b ;
    else if( a < b && b  > c &&  c < a) result = c ;

    else if( a > b && b < c && a > c) result = b ;
    else if( a > b && a > c && c < b) result = c ;
    return result ;
}

Is it better than nested if statements?  Is there a more readable solution than this? To me it looks pretty readable, but I'm not sure whether it can be improved.

For these things we have java.lang.Math:

public static int min(final int a, final int b, final int c){
    return Math.min(a, Math.min(b, c));
}

Wow, look how short it is!

But it's 3 numbers today, it's 10 tomorrow.
As an alternative, how about an array?

public static int min(int... numbers){
    if (numbers.length == 0){
        throw new IllegalArgumentException("Can't determine smallest element in an empty set");
    }
    int smallest = numbers[0];
    for (int i = 1; i < numbers.length; i++){
        smallest = Math.min(smallest, numbers[i]);
    }
    return smallest;
}

I'd use the java.lang.Math solution, it's very short, and very readable.


Run length encoding of vectors in MATLAB: looped version and vectorized version A project I'm working on requires something akin to run length encoding on a vector in matlab. The data is in the form of a numeric vector, and the output is in the form of two vectors, elems and lens that hold the elements and their respective run lengths. For example data = [20 13 12 12 12 18 17 16 16 11] would give elems = [20 13 12 18 17 16 11] and lens = [1 1 3 1 1 2 1]. There is no minimum run length.

My first attempt uses loops:

clear all

data = [2 1 2 3 3 1 2 2 3 3 1 1 1 1 1 2 2 1 1 3 3 3 3 2 1 3 3 2 2 1 1 1 2 2 2 2 3 1 1 1];
n = length(data);
elems = zeros(n, 1);
lens = zeros(n, 1);

len = 1;
elem = data(1);
c = 1;
for k = 2:n
    if data(k) == elem
        len = len + 1;
    else
        elems(c) = elem;
        lens(c) = len;
        c = c + 1;
        elem = data(k);
        len = 1;
    end
end

elems(c) = elem;
lens(c) = len;
lens = lens(lens ~= 0);
elems = elems(lens ~= 0);

It's pretty basic although I couldn't think of a way to efficiently preallocate the elems and lens vectors to something smaller than n. I figured I could always start them small, e.g. n/2 or something, keep track of their size, then reallocate them to something larger, maybe 1.5 times the previous size, if they were running out of room, or something like that.

My second attempt is completely vectorized:

clear all

data = [2 1 2 3 3 1 2 2 3 3 1 1 1 1 1 2 2 1 1 3 3 3 3 2 1 3 3 2 2 1 1 1 2 2 2 2 3 1 1 1];
breaks = [true data(2:end) ~= data(1:end-1)];
run_starts = find([breaks true]);
elems_vec = data(breaks);
lens_vec = diff(run_starts);

Can either of these methods be improved? The algorithms are pretty simple, and Im really just looking for general feedback that can help me learn matlab a bit more.

I'm not sure about the performance of my following code however I'm using tabulate() to do the job. As you can see, the loops for filtering elements not to compute the frequency of elements. In other words, you might want to look at tabulate() or hist() to compute the frequent occurrence of elements.

x = [20 13 12 12 12 18 17 16 16 11];
j = 1;
elems = []; % store repeated elements
for i = 1:length(x)
    if isempty(find(elems == x(i)))
        elems(j) = x(i);
        j = j + 1;
    end
end
elems
t = tabulate(x); % get frequency table
for i = 1:length(elems)
    lens(i) = t(elems(i),2); % get how many each element in elems vector occurs
end
lens

The result is

elems =
    20    13    12    18    17    16    11
lens =
     1     1     3     1     1     2     1


Truncating text with jQuery but keep the HTML formatting I repeat here this answer on Stack Overflow.

I first posted an answer with not finalized code, as a simple description of the solution I could think, without any test. But later I remained interested, so I worked to make it (hopefully) perfectly functional.

To precisely define what it is meant to do, let me cite my previous answer:

  This is a classic dilemma for any CMS or blog, where the teaser should present the begin of an article: often the solution is either stripping text from its tags and cut at a precise count OR keep tags but cut approximately because the tags are counted too...

So here the intent is to take an HTML element with any number of children, and any nesting level and return:

the "same" element (i.e. keeping its tag and attributes)
where resulting text content (i.e. visible as characters in the resulting page) is limited to a given count
where resulting text is built from successive text nodes in their natural order
where encountered tags are keeped intact and at their natural place

Here is my actual solution:

function cutKeepingTags(elem, reqCount) {
  var grabText = '',
      missCount = reqCount;
  $(elem).contents().each(function() {
    switch (this.nodeType) {
      case Node.TEXT_NODE:
        // Get node text, limited to missCount.
        grabText += this.data.substr(0,missCount);
        missCount -= Math.min(this.data.length, missCount);
        break;
      case Node.ELEMENT_NODE:
        // Explore current child:
        var childPart = cutKeepingTags(this, missCount);
        grabText += childPart.text;
        missCount -= childPart.count;
        break;
    }
    if (missCount == 0) {
      // We got text enough, stop looping.
      return false;
    }
  });
  return {
    text:
      // Wrap text using current elem tag.
      elem.outerHTML.match(/^<[^>]+>/m)[0]
      + grabText
      + '</' + elem.localName + '>',
    count: reqCount - missCount
  };
}

And here is a working example. (I kept the HTML example posted by the previous question OP)

First of all, I would just like to say that this is a really good and useful function. From a Code Review standpoint, there are almost no errors in it that I know of. Here are a few things I found from examining it:

Keep spacing uniform
Line 8 doesn't have a space between parameters, where your other function calls do. This is most likely due to quick typing and not any major issue other than a cleanliness nitpick.

An invalid input for if
Lines 19-21 where you have an if like so:

if (missCount == 0) {
  // We got text enough, stop looping.
  return false;
}

You should never use == over === due to it being possible that something like "0" would match the same as 0. This is because the double equals signs finds if it matches an exact value, where triple equals signs tests for exact value and type. So your final code should look like this for the if statement:

if (missCount === 0) {
  // We got text enough, stop looping.
  return false;
}

Optional - JSHint
If you use JSHint in JSFiddle, then you'll run into errors when trying to run this:

elem.outerHTML.match(/^<[^>]+>/m)[0]
  + grabText
  + '</' + elem.localName + '>',

If you're worried about that then you just have to write it all on one line. But for being short and concise, that might not be what you want.


Trapping Rain Water in Python This is from Leetcode

My solution is based on Python 3. It was faster than 90% of solutions on Leetcode, but used too much memory. Please what makes this memory hungry? How can I make this more efficient?

def trap(height, bound=None):
    storage = 0
    temp_storage = 0
    left_bound = None
    max_left_bound = 0

    for elevation in height:
        if bound and elevation == bound:
            storage += temp_storage
            return storage

        max_left_bound = max(elevation, max_left_bound)

        if not left_bound:
            left_bound = elevation
            continue

        block_capacity = left_bound - elevation
        if block_capacity > 0:
            temp_storage += block_capacity
            continue
        storage += temp_storage
        temp_storage, left_bound = 0, elevation

    if not temp_storage:
        return storage

    height.reverse()

    r_capacity = trap(height, max_left_bound)

    storage += r_capacity

    return storage

I'm not sure what you mean by "used too much memory" — if I try submitting a copy of your code now, then LeetCode says:

Memory Usage: 12.3 MB, less than 57.53% of Python online submissions for Trapping Rain Water.

It is impossible to know whether the 12.3 MB is a lot or a little without knowing what this includes and how big the test cases were, neither of which LeetCode tells us.
1. Review

I found it difficult to follow the algorithm because of the proliferation of similarly named variables. There are four variables that contain heights: elevation, bound, left_bound and max_left_bound. How do these variables relate to each other? Is left_bound actually a right bound when iterating over heights in reverse? There are three variables that contain amounts of water: storage, temp_storage and block_capacity. Again, how do these relate to each other?
Finding good names for variables and writing comments to clarify any remaining difficulties, is crucial in writing code that will be maintainable when you come back to it after having forgotten how it works.

The trap function modifies the list heights (by reversing it). It's best if functions don't modify their arguments (unless modifying the arguments is the point). Otherwise callers will be surprised by the side-effects. In this case you could easily use reversed(heights) instead of heights.reverse().

The code considers each vertical slice through the landscape in turn, adding left_bound - elevation (that is, the current water height minus the current land height) to the total trapped water. But in practice left_bound (the current water height) stays the same for several iterations of the loop, so we could wait until it changes and then add the block of water all at once. See §2 below for how to do this.

2. Alternative solution
Consider a typical landscape filled with water:

If we ignore the difference between water and land, and just look at the overall surface level, it should be clear that the filled landscape is made up of rectangles arranged like this:

There is a central "plateau" whose height is the maximum height of the landscape, flanked by staircases that start low at the outside and rise up to the plateau.
If we find the height of the plateau, which is just max(landscape), then we can work inwards from the left and right ends, finding the rectangles in the rising staircases until we reach the plateau height. By summing the rectangles we get the overall area of the filled landscape, and by subtracting the land part, which is just sum(landscape), we get the water part.
def trap2(landscape):
    """Return area of water trapped by landscape (a list of heights)."""
    if not landscape:
        return 0
    plateau_height = max(landscape)
    plateau_width = len(landscape)
    total_area = 0
    for heights in (landscape, reversed(landscape)):
        height_max = i_max = 0  # maximum height so far and its index
        for i, height in enumerate(heights):
            if height > height_max:
                total_area += (i - i_max) * height_max
                if height == plateau_height:
                    plateau_width -= i
                    break
                height_max = height
                i_max = i
    return total_area + plateau_height * plateau_width - sum(landscape)

It is a bit clearer, I hope, how this works, and it is several times faster than the code in the post. The reasons why this is faster are (i) the accumulation total_area += ... is done once per rectangle, not once per vertical slice; and (ii) the subtraction of the land part is done once by calling the built-in function sum, which runs in fast compiled code.
The exact speedup depends on the shape of the landscape, but here's a test case where the revised code was about 8 times faster:
>>> from random import choices
>>> from timeit import timeit
>>> HEIGHTS = choices(range(100), weights=(100,) + (1,) * 99, k=1000000)
>>> timeit(lambda:trap(HEIGHTS), number=1) # code in the post
0.31537632799881976
>>> timeit(lambda:trap2(HEIGHTS), number=1) # revised code
0.04002986599880387

But on LeetCode it runs in much the same time as the code in the post — clearly they are not try big enough test cases to detect the difference.


Sanity Checking an unordered_set of unique_ptrs to objects I have a use case for a data store containing uniquely identified large objects where I can get partial updates over time. Each object can be identified by a 'key' consisting of a trio of values embedded in different parts of the structure of the object. Yes, this is terrible, and no, I can't change it.

I decided I wanted to use an unordered_set to store my objects because I did not want to have to extract the key to search in a map, and I have a distaste for multiple copies of data, in this case a separate key and the key data embedded in the object.

I have constructed a test case for my needs and it works exactly as I expected. But I am an old dog learning new tricks with respect to smart pointers and wanted some experienced eyes and minds to review it or point out pitfalls my basic test case won't trigger. So without further verbage:

#include <memory>
#include <string>
#include <unordered_set>
#include <list>
#include <iostream>

struct TestRecord
{
   std::string strVal;
   int intVal;
   int intVal2;
   std::string nonKeyStr;
};

using TestPtr = std::unique_ptr<TestRecord>;
using TestList = std::list<TestPtr>;

struct TestPtrHash
{
   std::size_t operator()(TestPtr const& s) const
   {
      if (!s)
      {
         return 0;
      }

      std::size_t h1 = std::hash<std::string>{}(s->strVal);
      std::size_t h2 = std::hash<int>{}(s->intVal);
      std::size_t h3 = std::hash<int>{}(s->intVal2);
      return h1 ^ (h2 << 1) ^ (h3 << 2);
   }
};

struct TestPtrEquals
{
   bool operator()(const TestPtr& lhs, const TestPtr& rhs) const
   {
      if (!lhs || !rhs)
      {
         return false;
      }

      if ((lhs->strVal == rhs->strVal) && (lhs->intVal == rhs->intVal) && (lhs->intVal2 == rhs->intVal2))
      {
         return true;
      }

      return false;
   }
};

using TestSet = std::unordered_set<TestPtr, TestPtrHash, TestPtrEquals>;

bool DoThings(const std::string context, const TestPtr& item)
{

   std::cout << context << ":" << item->strVal << " " << item->intVal << " " << item->intVal2 << " " << item->nonKeyStr << "\n";

   return true;
}

int main(int argc, char* argv[])
{
   TestList thisList;
   TestSet thisSet;

   thisList.emplace_back(new TestRecord{ "name", 1, 2, "Other" });
   thisList.emplace_back(new TestRecord{ "name2", 1, 2, "Less" });
   thisList.emplace_back(new TestRecord{ "name", 3, 2, "Is" });
   thisList.emplace_back(new TestRecord{ "name", 1, 5, "More" });
   thisList.emplace_back(new TestRecord{ "name2", 1, 2, "More" });

   while (!thisList.empty())
   {
      TestPtr finder(std::move(thisList.front()));
      thisList.pop_front();

      auto look = thisSet.find(finder);

      if (look != thisSet.end())
      {
         (*look)->nonKeyStr = finder->nonKeyStr;
         DoThings("replace",*look);
      }
      else
      {
         DoThings("new node",finder);
         thisSet.emplace(std::move(finder));
      }
   }

    return 0;
}

Consider specializing standard templates

If the key fields of your structure are general, wherever it is used (rather than being a subset chosen for this particular set), then you could specialize std::hash and std::equal_to instead of declaring your own functors:

#include <functional>
namespace std
{
    template<>
    std::size_t hash<TestPtr>::operator()(TestPtr const& s) const
    {
        if (!s) return 0;
        auto h1 = std::hash<std::string>{}(s->strVal);
        auto h2 = std::hash<int>{}(s->intVal);
        auto h3 = std::hash<int>{}(s->intVal2);
        return h1 ^ (h2 << 1) ^ (h3 << 2);
    }

    template<>
    bool equal_to<TestPtr>::operator()(TestPtr const& lhs, TestPtr const& rhs) const
    {
        return lhs
            && rhs
            && lhs->strVal == rhs->strVal
            && lhs->intVal == rhs->intVal
            && lhs->intVal2 == rhs->intVal2;
    }
}

using TestSet = std::unordered_set<TestPtr>;

Notes on the test program

I prefer to separate the test code (in Real Life, you'd put it in a separate file):

#include <functional>
#include <memory>
#include <string>
#include <unordered_set>

struct TestRecord
{
   // and the rest of the production code
};

// --- test code follows

#include <iostream>
#include <list>
bool DoThings(const std::string context, const TestPtr& item)
{
   //
   return true;
}

int main()
{
   // Note: omitted arguments, which weren't used
}

When you move the production code to a header, you don't have to strip out the unnecessary includes.

The return value of DoThings() is ignored; either use it, or return void.

When inserting, it's more idiomatic to call insert() rather than a separate find() step.  The return value tells you whether your value was inserted - if it wasn't, you also get an iterator to assign to.  However, as you've observed, you aren't able to perform the assignment if you passed your value by rvalue reference.  Still, I'd try to encapsulate this in a method:

auto insert_or_assign(TestSet& set, TestPtr&& value)
    -> std::pair<TestSet::iterator, bool>
{
    auto overwrite = set.erase(value);
    auto r = set.insert(std::move(value));
    r.second = !overwrite;
    return r;
}

You can then use it like this:

int main()
{
    std::vector<TestPtr> thisList;

    thisList.emplace_back(new TestRecord{ "name", 1, 2, "Other" });
    thisList.emplace_back(new TestRecord{ "name2", 1, 2, "Less" });
    thisList.emplace_back(new TestRecord{ "name", 3, 2, "Is" });
    thisList.emplace_back(new TestRecord{ "name", 1, 5, "More" });
    thisList.emplace_back(new TestRecord{ "name2", 1, 2, "More" });

    TestSet thisSet;
    for (auto& t: thisList) {
        auto inserted = insert_or_assign(thisSet, std::move(t));
        DoThings(inserted.second ? "new node" : "replace", *inserted.first);
    }
}