Simple stack and linked list I know that Python has a built-in list data structure, but I want to implement my own linked list and stack data structures for practice.

I have three classes Node, Stack and List.

The code works, but I feel there is a better way to do this.

I would like to have some general critique.

class Node:
    def __init__(self):
        self.item = None
        self.link = None

class Stack:
    def __init__(self):
        self.top = 0
        self.head = Node()

    def isEmpty(self):
        return top == 0

    def push(self, item):
        oldHead = self.head
        self.head = Node()
        self.head.item = item
        self.head.link = oldHead
        self.top += 1

    def pop(self):
        item = self.head.item
        self.head = self.head.link
        self.top -= 1
        return item

class List:
    def __init__(self):
        self.top = 0
        self.head = Node()

    def get(self, *args):
        if not args:
            return self.head.item
        elif type(args[0]) is int:
            pos = args[0]
            current = self.head
            count = 0
            while count < pos - 1 and current.link != None:
                current = current.link
                count += 1
           return current.item
       else:
            raise TypeError

    def pop(self, *args):
        if not args:
            item = self.head.item
            head = self.head.link
            return item

    def append(self, item):
        oldhead = self.head
        self.head = Node()
        self.head.item = item
        self.head.link = oldhead
        self.top += 1

    def insert(self, item, pos):
        current = self.head
        count = 0
        while count < pos - 1 and current.link != None:
            current = current.link
            count += 1
        node = Node()
        node.item = item

        node.link = current.link
        current.link = node
        self.top += 1

It's a little hard to review code like this in Python because we're pretending there's no built-in list class, but a Python without a list doesn't feel like Python at all, so the code ends up looking like some other language.

I mention this because there are named tuples in Python 2.7 and Python 3, and they pretty much do exactly what your Node class does:

import collections
Node = collections.namedtuple('Node', ['item', 'link'])

n = Node(item=22, link=self.head)  # Make a new node.

Then again, if we're pretending lists don't exist, maybe we should pretend tuples don't exist either.

There's a lot of duplication in your code. These two classes are very similar. The push and append methods appear to be exactly the same method, for example. The two pop methods are almost exactly the same. The constructors are the same. So this seems like a good place to use inheritance. Make a base class, maybe with a name like LinkedList, which has the constructor and any methods which are exactly the same. Make the List and Stack both inherit from the LinkedList.

In your current code, you have separate implementations for pop that seem to do exactly the same thing, except that the stack version also removes the item. I would add separate pop and top methods. pop always removes and returns the top item, while top just returns the item without removing it. Then, if you want to make it impossible to remove the top item of a list, override the pop method to just call top. This is more or less how C++'s STL works, although C++'s pop doesn't return the item it removed.

So the final code would look something like this:

class LinkedList:
    def __init__(self):
        self.top_index = 0
        self.head = Node()

    def push(self, item):
        # Code from push / append here

    def pop(self):
        # Code that removes and returns the top here.

    def top(self):
        # Code that just returns the head here.

    def is_empty(self):
        return self.top_index == 0

class List(LinkedList):
    def insert(self, item, pos=0):
        # Code for insert here.

class Stack(LinkedList):
    # Whatever functionality is unique to the stack.
    # If there isn't any, you can consider not having a stack
    # class and just using a LinkedList.

I'll leave the details up to you, but that's basically how I would do it.

Am I right in thinking the following function is incomplete?

def pop(self, *args):
    if not args:
        item = self.head.item
        head = self.head.link
        return item

Were you going to add an else clause the retrieves the item at a specified index, like what you have in get? If that was what you had in mind, I think default arguments would be a better solution than using *args. Using get as a model, something like this:

def get(self, pos=0):
    if pos == 0:
        return self.head.item
    elif pos > 0:
        current = self.head
        count = 0
        while count < pos - 1 and current.link != None:
            current = current.link
            count += 1
       return current.item
    else:
       raise IndexError()

I find this clearer than the version with *args. Also, in case you wanted to use *args in the future, you can now do so without having to parse it.

Your use of style conventions and code formatting looks good. It can be confusing to get this right in Python since the standard library sets such a horrible example :)


Returns values for a specific array and key This PHP (5.3) code is intended to take an array and a key as inputs, and return an array of the values paired with that key‡

function valuelist($array, $array_column) {
    $return = array();
    foreach($array AS $row){
        $return[]=$row[$array_column];
    };
    return $return;
};

Can this be improved in terms of processing speed?

‡-For example, given:

$get_role_action = array(
    array("ACTION_CD" => "RETURN_PETITION"),
    array("ACTION_CD" => "UNLOCK_RECORD"),
    array("ACTION_CD" => "ACKNOWLEDGE"),
    array("ACTION_CD" => "REQUEST_POST_ACTION"),
    array("ACTION_CD" => "CHANGE_REQUEST_TYPE")
);

$variables['role_action_list']=valuelist($get_role_action, 'ACTION_CD'); should yield:

["role_action_list"]=>
    array(17) {
    [0]=>
    string(15) "RETURN_PETITION"
    [1]=>
    string(13) "UNLOCK_RECORD"
    [2]=>
    string(11) "ACKNOWLEDGE"
    [3]=>
    string(19) "REQUEST_POST_ACTION"
    [4]=>
    string(19) "CHANGE_REQUEST_TYPE"
}

I was able to play around with your requirements, and I think I got a solution.

Here is what I did to profile your code.

I set up the data array with:

$new = array(
    array('a' => 1,
        'b' => 2),
    array('a' => 3,
        'b' => 4),
    array('a' => 5,
        'b' => 6),
);

Calling it like so:

valuelist($new, 'b');

to produce:

Array
(
    [0] => 2
    [1] => 4
    [2] => 6
)

And the average function time was about 2.1894 E-6 seconds.

However, PHP 5.5 introduced us to the array_column function! And it happens to do exactly what you want! Yipee!

We can do a big swap-out, and come up with this:

function valuelist($array, $array_column) {
    return array_column($array, $array_column);
}

Now when we do the 50k calls, we learn that now the average function time is about 2.6226 E-10 seconds.

However, there is a little overhead due to the user function applied over top of the built-in function. By just calling array_column without the function does reduce the processing time, but by an amount so small it's hard to even say it's an amount! Just for the sake of cleanliness, I'd remove the user function anyways.

Don't tell me that's not an improvement!

However, without this function, I was unable to get a faster execution. I tried array_filter, array_map, and array_walk. Using use for the closure, as that proved faster than array_map's third parameter.


Speed up a function that checks for a sequence I created a  function yes.seq that takes two arguments, a pattern pat and data dat, the function looks for the presence of a pattern in the data and in the same sequence.
For example:
dat <- letters[1:10]
dat
[1] "a" "b" "c" "d" "e" "f" "g" "h" "i" "j"
pat <- c('a',"c","g")

 yes.seq(pat = pat,dat = dat)
[1] TRUE

because this sequence is in the pattern and in the same order.
"a" "b" "c" "d" "e" "f" "g" "h" "i" "j"
If, for example, we reverse the pattern, then we get FALSE:
yes.seq(pat = pat,dat =  **rev(dat)**   )
[1] FALSE

Here is my function:
yes.seq <- function(pat , dat){
  lv <- rep(F,length(pat))
  k <- 1
  for(i in 1:length(dat)){
            if(dat[i] == pat[k])
              {
              lv[k] <- TRUE
              k <- k+1
              }
    if(k==length(pat)+1) break
  }
  return(  all(lv)   )
}

I am not satisfied with the speed of this function.  Can you help me with that?

Here is a vectorized version:
yes.seq <- function(dat, pat) {
  paste(dat[dat %in% pat], collapse = "") == paste(pat, collapse = "")
}

yes.seq(dat, pat)
# [1] TRUE
yes.seq(dat, rev(pat))
# [1] FALSE


Comparing two n×m matrices I have to do a pairwise compare of a couple of text files. They have the following format:

id_1|errorcode_1|m|data_1_1|data_1_2|...|data_1_m
id_2|errorcode_2|m|data_2_1|data_2_2|...|data_2_m
  .
  .
  .
id_n|errorcode_n|m|data_n_1|data_n_2|...|data_n_m

The columns are ordered, but the rows can be in an arbitrary order. And there are always a couple of data-fields that have to be ignored.

This is my method that I call twice (switching the target and source array). It takes around 20 minutes to execute with two files where n = 16000 and m = 100.

private static string[] GetDifferences( IEnumerable<string> source
                                      , IEnumerable<string> target
                                      , IEnumerable<int> ignoredIndices)
{
    var uniqueInSource = new List<string>();

    foreach (var sourceLine in source)
    {
        var found = false;
        var sourceSplit = sourceLine.Split('|').ToList();

        // filter out malformed lines
        if (sourceSplit.Count > 2 && !int.TryParse(sourceSplit[2], out _))
        {
            continue;
        }

        foreach (var index in ignoredIndices)
        {
            sourceSplit.RemoveAt(index + 3);
        }

        foreach (var targetLine in source)
        {
            var targetSplit = targetLine.Split('|').ToList();

            if (targetSplit.Count > 2 && !int.TryParse(targetSplit[2], out _))
            {
                continue;
            }

            foreach (var index in ignoredIndices)
            {
                targetSplit.RemoveAt(index + 3);
            }

            if (targetSplit.SequenceEqual(sourceSplit))
            {
                found = true;
                break;
            }
        }

        if (!found)
        {
            uniqueInSource.Add(string.Join("|", sourceLine));
        }
    }

    return uniqueInSource.ToArray();
}

I feel that my approach might be too naive since I think it's somewhere around O(m*n2).

Your method is currently doing three things at a time and repeating the same logic multiple times:

parsing logs-1 (parsing a line)
parsing logs-2 (parsing a line again with the exact same code)
comparing both logs

It would be much easier to optimize and use it if you refactored it into  specialized structures/methods.

You put the first two cases into the static factory method Parse. It'll encapsulate the parsing part so you have implemented it only once.

Then you need to encapsulate the equality. You do this by implementing the IEquatable<LogLine> interface. You should do this very carefuly because both the Equals and GetHashCode methods are important for the performance.

This new class can now be very easily tested for: equality, parsing and performance.

public class LogLine : IEquatable<LogLine>
{
    public string Errorcode { get; set; }

    public string[] Data { get; set; }

    // add other properties...

    public static LogLine Parse(string log, IEnumerable<int> ignoreColumns)
    {
        // implement parsing
    }

    public bool Equals(LogLine other)
    {
        // implement log equality
    }

    public override bool Equals(object obj)
    {
        return base.Equals(obj as LogLine);
    }

    public override int GetHashCode()
    {
        // implement log hash-code
    }
}

And that's it because you now have a very LINQ-friendly class that you can use like this where you can parse all lines with a simple Select:

// dummies
var logs1 = Enumerable.Empty<string>();
var logs2 = Enumerable.Empty<string>();
var ignoreColumns = Enumerable.Empty<int>();

var logLines1 = logs1.Select(l => LogLine.Parse(l, ignoreColumns));
var logLines2 = logs2.Select(l => LogLine.Parse(l, ignoreColumns));

And you can compare them however you like with Except or Intersect or use GroupBy or Distinct or whatever you want. If you use more then a single extension should call .ToList() after each parsing so that you don't parse them multiple times.


Number factorization in clojure I'm just a Clojure noob (started learning yesterday). Here is my helloworld program. It factorizes number into primes. Do you have any comments? How could I make this code cleaner and shorter? Maybe I can deduplicate some things?

(defn lazy-primes
  ([] (cons 2 (lazy-seq (lazy-primes 3  [ 2 ]))))
  ([current calculated-primes]
   (loop [ [first-prime & rest-primes] calculated-primes]
     (if (> (* first-prime first-prime) current)
       (cons current (lazy-seq (lazy-primes
                                (inc current)
                                (conj calculated-primes current))))
       (if (= 0 (mod current first-prime))
         (lazy-seq (lazy-primes (inc current) calculated-primes))
         (recur rest-primes))))))

(defn factorize
  ([num] (factorize num '(1) (lazy-primes)))
  ([num acc primes]
   (if (= num 1) acc
       (loop [ [head & rest] primes ]
         (if (= 0 (mod num head))
           (factorize (/ num head) (cons head acc) primes)
           (recur rest))))))

Wow! If this is you after one day, we hope for great things.

However, you can make your program simpler. Let's start with factorize. It has a couple of defects:

It uses proper recursion where tail recursion with recur would
suffice. So it would overflow the stack for a number having too many
factors. However, since it uses long arithmetic, no number is big
enough to do this.
Each recursive call retries all the primes that have failed to be
factors.

You can do it with a single loop if you exhaust each prime factor as you go:

(defn factorize [num]
  (loop [num num, acc [1], primes (lazy-primes)]
    (if (= num 1)
      acc
      (let [factor (first primes)]
        (if (= 0 (mod num factor))
          (recur (quot num factor) (conj acc factor) primes)
          (recur num acc (rest primes)))))))

Now let's look at lazy-primes. While sticking to your algorithm ...

We can simplify the base case - the one with no arguments.
You produce a lazy sequence level for every number tested. We can
use recur to short-circuit tail recursion for all the numbers that
fail to be prime.
We can use take-while, map, and not-any? to express the prime
testing more clearly (though it will run slower). Because these
are lazy, we don't do any redundant testing.

The result is

(defn lazy-primes
  ([] (lazy-primes 2 []))
  ([current known-primes]
    (let [factors (take-while #(<= (* % %) current) known-primes)
          remainders (map #(mod current %) factors)]
      (if (not-any? zero? remainders)
        (lazy-seq (cons
                    current
                    (lazy-primes (inc current) (conj known-primes current))))
        (recur (inc current) known-primes)))))

A couple of arbitrary changes:

I've used the abbreviated #(...) function syntax.
I renamed calculated-primes to known-primes.

Edited to correct an erroneous comment.


Convert Excel files to CSV I wrote the following script to run through several excel files and format them before saving as a CSV for upload to a Quickbase app I'm creating. Each excel file is roughly 100k lines and the code takes about 3-5 minutes per file. I'm currently using pandas to make my edits. If there is a more efficient way please let me know.

import pandas as pd
import os

# set working directories for files
starting_folder=('Purchasing/unformatted/')
save_folder = ('Purchasing/')

#create list of excel file names from folder
files = []
for file in os.listdir(starting_folder):
    filename = os.fsdecode(file)
    files.append(filename)

# create list for file names to be saved as csv
save_files = [w.replace('xlsx','csv') for w in files]

# create data frame of fiscal calendar
calendar = pd.read_excel('Purchasing/Fiscal Calendar 15-18.xlsx')
fiscal_calendar = pd.DataFrame([])
#create new data frame with only two columns
fiscal_calendar['InvoiceDate'] = calendar['Date']
fiscal_calendar['Week'] = calendar['Week.1']

#loop through csv files to format and save to new location
for i in range(len(files)):
    #reads unformatted excel file into dataframe
    df = pd.read_excel(starting_folder+files[i])
    #change dtype of dates in report to date time
    df['InvoiceDate'] = pd.to_datetime(df['InvoiceDate'])
    #drop unwanted rows
    df = df.drop(df[df.Brand == 'NONPROD'].index)
    #add in fiscal week based on invoice date
    Sygma = pd.merge(df,fiscal_calendar, how = 'left', on = 'InvoiceDate')
    #save to csv for db loading
    Sygma.to_csv(save_folder+save_files[i],index = False)

There are a few small fixes, but just to point out, pandas.read_excel is notoriously slow..

  Code.py

import pandas as pd
import os

# # NOTE: If there are multiple worksheets this will take awhile,
# # and you can speed it up by specifying just the worksheet you want
# create data frame of fiscal calendar
fiscal_calendar = pd.read_excel('Purchasing/Fiscal Calendar 15-18.xlsx')
fiscal_calendar = fiscal_calendar[["Date", "Week.1"]].rename(columns={"Date": "InvoiceDate", "Week": "Week.1"})

# set working directories for files
starting_folder = ('Purchasing/unformatted/')
save_folder = ('Purchasing/')

#create list of excel file names from folder
for file in os.listdir(starting_folder):
    input_name = starting_folder + os.fsdecode(file)
    df = pd.read_excel(input_name)

    # cleanup
    df["InvoiceDate"] = pd.to_datetime(df["InvoiceDate"])
    df = df.drop(df[df.Brand == 'NONPROD'].index)

    # create the output file and save it to .csv
    output_name = save_folder + filename + ".csv"
    Sygma = pd.merge(df, fiscal_calendar, how='left', on='InvoiceDate')
    Sygma.to_csv(output_name, index=False)


Sierpiński's carpet fractal animation for teaching Python 3 I am teaching programming (in this case - 1 on 1 tutoring of a teenager interested in programming) and this code will be a final stage of a progression toward program generating a nice image of Sierpiński's triangle.

Any comments how this code can be improved are welcomed! But problems with unclear code that break standard practices are especially welcomed, performance issues are less important here.

from PIL import Image
from PIL import ImageDraw

def save_animated_gif(filename, images, duration):
    # done using https://pillow.readthedocs.io/en/latest/handbook/image-file-formats.html#saving
    first_image = images[0]
    other_images = images[1:]
    first_image.save(filename, save_all=True, append_images=other_images, duration=duration, loop=0)

def make_pattern(draw, x, y, section_size, remaining_levels):
    if remaining_levels <= 0:
        return
    hole_color = (5, 205, 65)
    corner = (x + section_size / 3, y + section_size / 3)
    # -1 necessary due to https://github.com/python-pillow/Pillow/issues/3597
    opposite_corner = (x + section_size * 2/3 - 1, y + section_size * 2/3 - 1)
    draw.rectangle((corner, opposite_corner), fill=hole_color)
    parts = 3
    for x_index in range(parts):
        for y_index in range(parts):
            x_anchor = x + section_size * x_index / parts
            y_anchor = y + section_size * y_index / parts
            new_size = section_size / 3
            new_levels = remaining_levels - 1
            make_pattern(draw, x_anchor, y_anchor, new_size, new_levels)

def make_carpet(levels, size):
    carpet_color = (5, 60, 20)
    carpet = Image.new("RGBA", (size, size), carpet_color)
    draw = ImageDraw.Draw(carpet)
    make_pattern(draw, 0, 0, size, levels)
    return carpet

levels = 7
size = 3**levels
carpets = []
carpets.append(make_carpet(0, size))
standard_frame_time_in_ms = 1200
durations = [standard_frame_time_in_ms / 2] # first stage visible for a short time
for i in range(levels - 1):
    carpets.append(make_carpet(i + 1, size))
    durations.append(standard_frame_time_in_ms)
durations[-1] *= 4 # final stage of animation visible for a long time

save_animated_gif("Sierpiński's carpet.gif", carpets, durations)

output:

full sized image

Algorithm

You can simplify the code and make it run faster if you construct the next-level carpet by continuing to work on the previous image (punching more holes in it), rather than starting with a blank slate every time.  The code can look prettier and more Pythonic too, since the technique lets you get rid of the recursion.

Coding practices

It's a good habit to write docstrings, especially if you are using this code to teach a student!

Avoid free-floating code; all code should be in a function.  Follow the standard practice of writing if __name__ == '__main__': main() at the end of the code.

You can combine the two import statements into one, since Image and ImageDraw are both being imported from the same module.

It should be easier to tell that the color triples represent dark green and light green.  A comment would work.  In my solution below, I've opted to use explanatory variables.  Furthermore, the colors should be specified in a more obvious place, rather than buried in some obscure place in the code.

In Python, it is usually possible to find a more elegant way to building a list than by repeatedly .append()ing.  Below, I construct carpets using a generator, and durations using the * operator.

You know that all of the coordinates should be integers.  Use integer division (//) rather than floating-point division (/) wherever possible.

You can use itertools.product() to avoid nested for loops for x and y.

To split a list into the first element and subsequent elements, you can write first_image, *other_images = images.

Suggested solution

from itertools import product
from PIL import Image, ImageDraw

def save_animated_gif(filename, images, durations):
    """
    Save images as frames of an animated GIF.  Durations should specify the
    milliseconds to display each frame, and should be of the same length as
    images.
    """
    # https://pillow.readthedocs.io/en/latest/handbook/image-file-formats.html#saving
    first_image, *other_images = images
    first_image.save(filename, save_all=True, append_images=other_images, duration=durations, loop=0)

def punch_hole(draw, x, y, section_size, hole_color):
    """
    For a square with a corner at (x, y) and sides of length section_size,
    divide it into 9 tiles, and fill the center tile with hole_color.
    """
    corner = (x + section_size // 3, y + section_size // 3)
    # -1 necessary due to https://github.com/python-pillow/Pillow/issues/3597
    opposite_corner = (x + section_size * 2//3 - 1, y + section_size * 2//3 - 1)
    draw.rectangle((corner, opposite_corner), fill=hole_color)

def make_carpets(n, carpet_color, hole_color):
    """
    Generate n PIL Images, each of Sierpiński's carpet with increasing levels
    of detail.
    """
    image_size = 3**n
    carpet = Image.new("RGBA", (image_size, image_size), carpet_color)
    yield carpet
    for section_size in (3**i for i in range(n, 1, -1)):
        carpet = carpet.copy()
        draw = ImageDraw.Draw(carpet)
        for x, y in product(range(0, image_size, section_size), repeat=2):
            punch_hole(draw, x, y, section_size, hole_color)
        yield carpet

def main():
    N = 7
    DARK_GREEN = (5, 60, 20)
    LIGHT_GREEN = (5, 205, 65)

    carpets = make_carpets(N, carpet_color=DARK_GREEN, hole_color=LIGHT_GREEN)
    durations = [1200] * N              # 1200ms per frame, except...
    durations[0] //= 2                  # first frame is shorter
    durations[-1] *= 4                  # final frame is longer

    save_animated_gif("Sierpiński's carpet.gif", carpets, durations)

if __name__ == '__main__':
    main()

Alternative calculations

In the suggested solution above, I've written punch_hole() to be similar to your make_pattern(), in that they are both responsible for rendering a square of size section_size.  However, the arithmetic can be simplified by specifying the size of the center hole instead, so that no division is necessary.

def draw_square(draw, x, y, size, color):
    """
    Fill a square with one corner at (x, y) with the specified color.
    """
    # -1 necessary due to https://github.com/python-pillow/Pillow/issues/3597
    draw.rectangle((x, y), (x + size - 1, y + size - 1)), fill=color)

def make_carpets(n, carpet_color, hole_color):
    """
    Generate n PIL Images, each of Sierpiński's carpet with increasing levels
    of detail.
    """
    image_size = 3**n
    carpet = Image.new("RGBA", (image_size, image_size), carpet_color)
    for hole_size in (3**i for i in range(n, 0, -1)):
        draw = ImageDraw.Draw(carpet)
        for x, y in product(range(hole_size, image_size, 3 * hole_size), repeat=2):
            draw_square(draw, x, y, hole_size, hole_color)
        yield carpet
        carpet = carpet.copy()


Validation methods to ensure start and end date come after existing instances We have a "Committee" class and I need to ensure that any new committee has start and end dates (don't know why they're CharFields instead of Dates or even Ints, but that's not my call) that are after any existing instances.  This works, but I would appreciate any general review as well as specifically on my solution to my test problem:  When no Committees exist, my tests broke as noted in the comment.  My solution worked but feels hacky.

class Committee(models.Model):
    start_year = models.CharField(max_length=4)
    end_year = models.CharField(max_length=4)

    def save(self, *args, **kwargs):
        # new committees need to ensure start and end dates are in the future

        if not self.pk:
            committee_set = Committee.objects.all()
            latest_start_year = committee_set.aggregate(Max('start_year'))["start_year__max"]
            latest_end_year = committee_set.aggregate(Max('end_year'))["end_year__max"]

            # in the test database when there are no other committees yet, these evaluate to
            # None and break the tests. Doesn't really affect production code otherwise.
            try:
                latest_start_year = int(latest_start_year)
            except TypeError:
                latest_start_year = 0
            try:
                latest_end_year = int(latest_end_year)
            except TypeError:
                latest_end_year = 1

            if not (self.start_year > latest_start_year):
                raise ValidationError("Start year (%s) must be greater than most recent "
                                      "Committee start year (%s)" % (
                                          self.start_year, latest_start_year))

            if not (self.end_year > latest_end_year):
                raise ValidationError("End year (%s) must be greater than most recent "
                                      "Committee end year (%s)" % (self.end_year, latest_end_year))

        super(Committee, self).save(*args, **kwargs)

Instead of trying to convert to int and catching TypeError,
it would be better to check for None values.

Instead of committee_set.aggregate(Max('start_year'))["start_year__max"],
you could give a custom alias to the max value,
making the code slightly more compact.

Instead of getting the latest start and end years in the beginning,
it would be better to delay getting the latest end year until it's needed.
That way you may be able to avoid one unnecessary query.

Instead of the style formatting strings with '%s' % (...),
the modern way is to use '{}'.format(...).

Combining the above tips, I would write the main part like this:

query = Committee.objects

latest_start_year = query.aggregate(max=Max('start_year'))['max']
if latest_start_year and int(self.start_year) <= int(latest_start_year):
    raise ValidationError("Start year {} must be greater than most recent {}".format(self.start_year, latest_start_year))

latest_end_year = query.aggregate(max=Max('end_year'))['max']
if latest_end_year and int(self.end_year) <= int(latest_end_year):
    raise ValidationError("End year {} must be greater than most recent {}".format(self.end_year, latest_end_year))

If it's not very important to include in the error message the latest start and end years,
a more compact and more efficient solution is possible:

query = Committee.objects

if query.filter(start_year__gt=self.start_year).exists():
    raise ValidationError("Start year must be greater than most recent")

if query.filter(end_year__gt=self.end_year).exists():
    raise ValidationError("End year must be greater than most recent")

And let's not forget about test cases.
Django provides a rich framework for testing.
Here are some examples:

class CommitteeTests(TestCase):
    def test_create_first(self):
        self.assertEqual(0, Committee.objects.count())
        Committee(start_year='1991', end_year='1992').save()
        self.assertEqual(1, Committee.objects.count())

    def test_create_second_valid(self):
        self.assertEqual(0, Committee.objects.count())

        Committee(start_year='1991', end_year='1995').save()
        self.assertEqual(1, Committee.objects.count())

        Committee(start_year='1992', end_year='1996').save()
        self.assertEqual(2, Committee.objects.count())

    def test_should_fail_if_start_is_before_latest_start(self):
        Committee(start_year='1991', end_year='1995').save()
        with self.assertRaises(ValidationError):
            Committee(start_year='1990', end_year='1995').save()

    def test_should_fail_if_end_is_before_latest_end(self):
        Committee(start_year='1991', end_year='1995').save()
        with self.assertRaises(ValidationError):
            Committee(start_year='1992', end_year='1994').save()

Lastly, you didn't mention about it,
but the current code allows Committee instances having start date after end date.
Probably it would be a good idea to prevent that too,
not to mention valid numeric values.
These validations should come first before the others querying the database,
and there should be test cases for them too.


Simon Says using Arduino UNO I built a Simon Says game using an Arduino UNO board. This was my first "real" arduino project. I would like to know how to make it better, game mechanics and code efficiency.

#define RedTone 165
#define GreenTone 330
#define YelloTone 659
#define BlueTone 1319
#define FailTone 100

#define BuzzerPin 8

/* The led and button pins are set this way because I use their position as a hexadecimal.
 *  LED PINS:
      red:0
      green:1
      yello:2
      blue:3
    Button Pins:
      red:4
      green:5
      yello:6
      blue:7
*/

byte mem[20];
int sound[16] = {0, RedTone, GreenTone, 0, YelloTone, 0, 0, 0, BlueTone, 0, 0, 0, 0, 0, 0, FailTone};  //the tones are placed in the arrey in the hex spot for turning on the leds.

void setup() {
  DDRD = 0x0F;
  randomSeed(analogRead(0));
  pinMode(BuzzerPin, 1);
}

void blink(int i) { //Blinking and making sound
  PORTD = i;
  tone(BuzzerPin, sound[i]);
  delay(500);
  PORTD = 0x00;
  noTone(BuzzerPin);
  delay(500);
}

void NT(int i) { //Just blinking
  PORTD = i;
  delay(500);
  PORTD = 0x00;
  delay(500);
}

void set_mem() {  //sets the random order of the leds and transforms into the led's pin in hexadecimal
  int b;
  for (int i = 0; i < 20; i++) {
    b = random(4);
    if (b == 3)
      mem[i] = 0x08;
    else if (b == 2)
      mem[i] = 0x04;
    else if (b == 1)
      mem[i] = 0x02;
    else
      mem[i] = 0x01;
  }
}

bool check(int i) { //checks if the pressed button is correct
  byte look;
  for (int x = 0; x <= i; x++) {
    look = 0;
    do
    {
      look = PIND >> 4;  //makes the button press pin to the pin of led with the same color
    }
    while (look == 0);  //waits until a button is pressed
    blink(look); //lights up the button's led
    if (look != mem[x])
      return 0;
  }
  return 1;
}

void loop() {
  set_mem();
  for (int i = 0; i < 20; i++) {
    for (int x = 0; x <= i; x++)
      blink(mem[x]);
    NT(0x09); //signals when it finnished showing the order
    if (check(i) == 0) {
      blink(0x0F); //shows if the player failed
      break; //couses a restart
    }
    NT(0x06); //signals when the player finished pressing the leds
  }
}

How it looks and works:

(I'm having problems making the snippet work, so here is the link to the site itself and here is a video of it working (sorry for the low quality).)

<iframe frameborder='0' height='448' marginheight='0' marginwidth='0' scrolling='no' src='https://circuits.io/circuits/4145099-simon-says-leds-and-buttons/embed#breadboard' width='650'></iframe>

Here are a few suggestions, in my opinion, to improve your code:

Using Port Registers:
You use PORTD, PIND, and DDRD in the code. These can be replaced bydigitalWrite(),digitalRead() and pinMode(). Here is the Arduino reference page for Port Registers. Although they will quicken the speed of execution, in your case, that high speed is not required, especially in the setup. The suggested alternatives will fulfill the task equally well.
Electronic Advice:
Avoid using the pin 0 and pin 1. These are used for Serial debugging, and even though you are not using serial communication, it is good practice to not use them for I/O. Here is a reddit post discussing that, here is the official board layout reference.
Specific Code reductions:
in set_mem(), instead of the logical conditions, you can predefine an array map = {0x01, 0x02, 0x04, 0x08} and then set the mem[i] = map[random(4)].
Gameplay improvements
As it stands, it is very confusing to see what is going on, and how to play the game. Assuming that you are limited by these components and the difficulty level of the code is not to be scaled, you can consider the following:

Indicating correct pressing. Currently, it is difficult to make out which two LEDs signal the completion of showing the required sequence, and which two signal you having pressed the buttons correctly. You can create another function which does not light up any LED but just signals a correct press by means of a musical sound, the relative pleasantness indicates to any onlooker that the answer was correct. Consider a simple increasing and decreasing tone, or even better, look-up the frequencies somewhere like here of musical notes and create a pleasant tune! Just replace the NT(0X06)by the new function.
Indicating failure. You can include the Failtone for a larger duration with all the 4 LEDs to indicate failure. You might have to make a new function to replace NT(0X09).
I am not sure whether having to repeat the sequence with one new added press each time is a part of the design. But if it is not, you can create a new sequence each time, by keeping set_mem() in the loop, so that every time they complete a sequence, an entirely new sequence of presses (longer by 1) comes up next.

Provisions for faulty game play. Currently, the person can press the buttons while the sequence is being displayed. Although it does not affect the gameplay in any way, it is generally good practice to anticipate faulty play. adding, for example, the following in the code below blink(mem[x]) in the nested for loop, may achieve that, where failurefunction is what you use to indicate failure.

Code:

  look = PIND >> 4;
  if (look != 0){
  failurefunction();
  break;
  }

I hope this was helpful.


Handling null exception when having multiple variables I have a lot of variables that I need to check for exceptions and output the empty field in case of a null returned value (I am reading a calender list from Sharepoint and my program is supposed to send email notifications if some of the conditions are met).

I surrounded my variables with a try-catch with a generic output "variable not found". Here is an example;

try
{
var name = item["Name"].ToString();
var dueDate= item["Due Date"].ToString();
.
.//more variables
.
var Title= item["Title"].ToString();
}
catch (Exception ex)
                        {
                            Console.WriteLine();
                            Console.WriteLine(ex.Message); //generic message for now
                            Console.WriteLine();
                        }

Is there is a way to handle this better? I know that going to each individual line and adding an exception is an option but it would save me a lot of time if I can just output something like the variable name.

Refactor into a method:

var name = this.GetVariable("Name");
var dueDate= this.GetVariable("Due Date");
.
.//more variables
.
var Title= this.GetVariable("Title");

private string GetVariable(string name)
{
    try
    {
        return item[name].ToString();
    }
    catch (Exception ex)
                        {
                            Console.WriteLine();
                            Console.WriteLine(name + " not found.");
                            Console.WriteLine();
                            return null;
                        }
}


Bash script that can uncompress: tar, gzip, rar This code generally works exactly as it is supposed to. I'm learning bash I'm looking for some constructive help that could help me improve my work.

#!/bin/bash

usage()
{
    echo "Program for uncompressing  tar, gzip, rar archives"
    echo "Allowed parametres: --help, --file, --input, --directory"
    echo "Examples:"
    echo "./uncompress.sh --file firstFile secondFile"
    echo "./uncompress.sh --input inputFile.txt"
    echo "./uncompress.sh --directory /home/"

}

uncompress_file()
{
    if [ -f $1 ]
    then
        temp1=`file $1 | cut -d: -f2 | grep tar`

            if [ "$temp1" != "" ]
                then
                    echo    "$1 is a tar archive, uncompressing..."
                    tar xf $1 > /dev/null
            fi

        temp1=`file $1 | cut -d: -f2 | grep RAR`

            if [ "$temp1" != "" ]
                then
                    echo    "$1 is a rar archive, uncompressing..."
                    unrar e $1 > /dev/null
            fi

        temp1=`file $1 | cut -d: -f2 | grep gzip`

            if [ "$temp1" != "" ]
                then
                    echo    "$1 is a gzip archive, uncompressing..."
                    tar xzf $1 > /dev/null

            fi

    else
    echo    "$1 is not a valid file!"
    fi
}

uncompress_dir()
{
    for file in "$1"/*
        do
            if [ -f $file ]
            then
            uncompress_file $file
            fi

            if [ -d $file ]
            then
            uncompress_dir $file
            fi

        done

}

if [ "$1" = "--help" ]
    then
    usage
    exit

fi

if test $# -lt 2
    then echo "What about parametres?" 1>&2
    exit 1
fi

param=$1;
shift;

if [ "$param" = "--file" ]
    then

        while [ $# -ne 0 ]
        do
            uncompress_file $1
            shift
        done
    exit
fi

if [ "$param" = "--input" ]
    then
        if [ -f $1 ]
            then
                while read line
                do
                    uncompress_file $line

                done < $1
            else
                echo "$1 is not a valid file"
        fi
    exit
fi

if [ "$param" = "--directory" ]
    then
        if [ -d $1 ]
            then
                uncompress_dir $1
            else
            echo    "$1 is not a valid directory"
        fi
    exit
fi

echo "wrong parametres!";

Don't use cut and grep, use parameter substitution and a case statement over the file extension to decide the file type (or use the file command if you want to be really clever, but I recommend the former actually)
Don't use [ "${foo}" != "" ], use [ -n "${foo}" ]
Don't mix test and [, just stick to [ unless you want to write code that looks autoconf generated
Quote all your variables, especially the ones containing filenames to account for spaces, e.g. tar xzf "$1" > /dev/null
Don't parse command line options yourself, use getopts (or getopt, in case you really, really want GNU-style options)
Improve your indentation


The last standing gunman in a circle I don't know why in most of the interviews, interviewers are asking this question commonly.

The question is: There are n number of persons standing in a circle. The first person has a gun and he kills the very next person who is alive and hands over the gun to the next person. Who will remain?

Here is my code for the above scenario.
It works, if anyone needs to ask.

public static void void main(String ar[]) {

    int numberOfGuys = 10000;
    List<Integer> list = new ArrayList<>();
    for ( int i = 1; i <= numberOfGuys; i++ ) {
        list.add( i );
    }
    boolean isNeighbour = false;

    System.out.println( list );
    Iterator<Integer> i = list.iterator();
    list = new ArrayList<>();
    boolean isTrue = true;
    while ( isTrue ) {
        int k = i.next();
        if ( isNeighbour )
            i.remove();
        else {
            list.add( k );
        }
        if ( !i.hasNext() ) {
            System.out.println( list );
            i = list.iterator();
            if ( list.size() == 1 )
                isTrue = false;
            list = new ArrayList<>();
        }
        isNeighbour = !isNeighbour;
    }
}

Style review

I think numberOfGuys should be a parameter obtained in the main args, so that you respect the possibility to obtain n from an external source.

As pointed in the comments, don't do isTrue = false, that's... weird. True is always true, period. You could rename it hasRemainingGuys or something like that.

Data structure
Using an ArrayList probably isn't the best data structure for your problem. This data structure is fast when it comes to obtaining data, but removing elements form it, which you do a lot, is slower. You'd want a structure where Remove is an \$O(1)\$ operation and, obviously, where it's easy to navigate from one element to the next. The LinkedList sounds like a good idea, as I'll point out below.
Alternative algorithm
I'd recommend creating some sort of circular linked list, so you wouldn't need to recreate the iterator when you reach the end of the list and to simplify some conditions. The idea is to have a linked list, where the last node's "next node" is the first one. This way, your generator could run until there's no next values.
Iterator<Integer> iterator = circularLinkedList.iterator();
//To start with the "first guy"
int k = -1;
while (iterator.hasNext()) {

    //Move on to the next shooter
    k = iterator.next();

    if (!iterator.hasNext()) {
        break;
    }

    //Move to the next shoot...ee?
    iterator.next();
    iterator.remove();
}

And... yeah, that's all. k contains the last guy standing.
Now, you need to implement a circular linked list and create an iterator that works with it. There are many examples on internet on how to do this.
You could also explore an option where you don't actually remove the element from the list, but simply keep a boolean where an index is flagged as dead or not. This might also be faster.


Setting manager class for reading and managing Settings.ini file of my program Setting manager class for reading and managing Settings.ini file
I have started a project creating Software Installer application for auto download and auto install software, similar how Ninite works. I have created SettingsManager.java class for reading and managing Settings.ini file of the program so the rest of the program can use it. This is just a draft of the class an it is not complete, but it is functional. I would like your opinions on the overall design and implementation of this kind of class.
The SettingsManager.java class
package com.zoran_jankov.software_installer.app;

import java.io.File;
import java.io.IOException;
import java.util.HashMap;
import java.util.Map;

import org.ini4j.Ini;

public class SettingsManager
{
    private static SettingsManager instance;

    private static final String SETTINGS_FILE_PATH = "D:/Programing/Java/Software Installer/src/main/resources/Settings.ini";

    private Map<Software, String> localRepositories = new HashMap<Software, String>();
    private Map<Software, String> networkRepositories = new HashMap<Software, String>();
    private Map<Software, String> onlineRepositories = new HashMap<Software, String>();
    private Map<Software, String> arguments = new HashMap<Software, String>();

    private SettingsManager()
    {
        loadSettings();
    }

    public static SettingsManager getInstance()
    {
        if(instance == null)
        {
            instance = new SettingsManager();
        }
        return instance;
    }

    public void loadSettings()
    {
        Ini settings = null;

        try
        {
            settings = new Ini(new File(SETTINGS_FILE_PATH));
        }
        catch (IOException e)
        {
            e.printStackTrace();
        }

        localRepositories = getInstallerSettings(settings, "Local Repositories");

        networkRepositories = getInstallerSettings(settings, "Network Repositories");

        onlineRepositories = getInstallerSettings(settings, "Online Repositories");

        arguments = getInstallerSettings(settings, "Arguments");

    }

    private Map<Software, String> getInstallerSettings(Ini setttings, String sectionName)
    {
        Map<Software, String> list = new HashMap<Software, String>();

        for (Software installer : Software.values())
        {
            list.put(installer, setttings.get(sectionName, installer.name()));
        }

        return list;
    }

    public String getLocalRepository(Software software)
    {
        return localRepositories.get(software);
    }

    public String getNetworkRepository(Software software)
    {
        return networkRepositories.get(software);
    }

    public String getOnlineRepository(Software software)
    {
        return onlineRepositories.get(software);
    }

    public String getArguments(Software software)
    {
        return arguments.get(software);
    }
}

The Software.java enum
package com.zoran_jankov.software_installer.app;
public enum Software
{
    AIMP,
    CCleaner,
    DWGSee,
    Firefox,
    FoxitReader,
    ImgBurn,
    Java,
    LibreOffice,
    Notepad,
    Office365,
    OpenOffice,
    RocketChat,
    Skype,
    SpacePlanning,
    Speccy,
    Thunderbird,
    UltraVNC,
    VLC,
    ZabbixAgent,
    Zip;
}

The Settings.ini file
So you can better understand what is happening in the code.

Defualt Java style is to have the opening braces on the same line, but that's down to preferences. Just be consistent in your code.

private static final String SETTINGS_FILE_PATH = "D:/Programing/Java/Software Installer/src/main/resources/Settings.ini";

You want to pass that path, somehow, as it is not portable.

public class SettingsManager
{
    private static SettingsManager instance;

    public static SettingsManager getInstance()
    {
        if(instance == null)
        {
            instance = new SettingsManager();
        }
        return instance;
    }
}

This is not thread-safe, in case you care about it. An obligatory number of threads could end up with their own, not-shared instances of the SettingsManager. There are three ways to prevent that:
The first is to use a static initializer:
public class SettingsManager
{
    private static SettingsManager instance;

    static
    {
        instance = new SettingsManager();
    }
}

Static initializers are guaranteed to be synchronized and will only be executed once the class is accessed.
The second option is to synchronize the whole method:
public class SettingsManager
{
    private static SettingsManager instance;

    public static synchronized SettingsManager getInstance()
    {
        if(instance == null)
        {
            instance = new SettingsManager();
        }
        return instance;
    }
}

This guarantees a single instance for everyone, but it means that an expensive lock is acquired for each access. "Expensive" in this case means "compared to simply getting the value", in 99.99% of the cases you will not notice the performance difference to method #3.
The third method is to check twice whether the value is available, once "cheap", and then synchronized:
public class SettingsManager
{
    private static SettingsManager instance;

    public static SettingsManager getInstance()
    {
        if(instance == null)
        {
            synchronized(SettingsManager.class)
            {
                if(instance == null)
                {
                    instance = new SettingsManager();
                }
            }
        }
        return instance;
    }
}

That skips the locking in most instances, except when required, namely if two threads try to access the instance for the first time.

private static final String SETTINGS_FILE_PATH = "D:/Programing/Java/Software Installer/src/main/resources/Settings.ini";

You want to pass that path, somehow, as it is not portable in the slightest in this way.
Now, that leads to the problem that you only have one single, global instance, and if you pass the path you have to ask yourself the question what should be done when a second instance with a different path is requested. It might be better to have a single instance which you pass around in your application, or which you make globally accessible throughout your application, and not through the class level of this class.

        Ini settings = null;

        try
        {
            settings = new Ini(new File(SETTINGS_FILE_PATH));
        }
        catch (IOException e)
        {
            e.printStackTrace();
        }

You most likely want to re-throw that exception, or let it propagate freely by letting loadSettings throw IOException. Silently swallowing exceptions might lead to a really bad headache when debugging problems.

Map<Software, String> list = new HashMap<Software, String>();

That's not a list, that's a map. Naming like that is confusing. Also, in Java 8+ you can omit the types in the constructor, as they are derived from the declaration:
Map<Software, String> list = new HashMap<>();

public enum Software
{
    AIMP,
    CCleaner,

Java convention is that enum members are considered constants, therefor should be UPPER_SNAKE_CASE.

Now, what you don't want is that Software enum, that should be configuration that you read, too, because that allows easier maintenance and extension of the software. Overall, your ini format seems not suited for what you want, it looks more like you want to invert the format:
[AIMP]
Local Repository=...
Network Repository=...
Online Repository=...
Arguments=...

[CCleaner]
Local Repository=...
Network Repository=...
Online Repository=...
Arguments=...

Because with that in place, you can kill off Software completely and make your application driven by configuration. You simply read the ini file and display the list of software that is configured instead of having it hard-coded.


How to improve performace of this Map Reduce function, Python mrjob I'm trying to get the most out of this code, so I would understand what should I look for in the future. The code below, works fine, I just want to make it more efficient.

Any suggestions?

from mrjob.job import MRJob
import operator
import re

# append result from each reducer
output_words = []

class MRSudo(MRJob):

    def init_mapper(self):
        # move list of tuples across mapper
        self.words = []

    def mapper(self, _, line):
        command = line.split()[-1]
        self.words.append((command, 1))

    def final_mapper(self):
        for word_pair in self.words:
            yield word_pair

    def reducer(self, command, count):
        # append tuples to the list
        output_words.append((command, sum(count)))

    def final_reducer(self):
        # Sort tuples in the list by occurence
        map(operator.itemgetter(1), output_words)
        sorted_words = sorted(output_words, key=operator.itemgetter(1), reverse=True)
        for result in sorted_words:
            yield result

    def steps(self):
        return [self.mr(mapper_init=self.init_mapper,
                        mapper=self.mapper,
                        mapper_final=self.final_mapper,
                        reducer=self.reducer,
                        reducer_final=self.final_reducer)]

if __name__ == '__main__':
    MRSudo.run()

Since the reduce function in this case is commutative and associative you can use a combiner to pre-aggregate values.

def combiner_count_words(self, word, counts):
    # sum the words we've seen so far
    yield (word, sum(counts))

def steps(self):
    return [self.mr(mapper_init=self.init_mapper,
                    mapper=self.mapper,
                    mapper_final=self.final_mapper,
                    combiner= self.combiner_count_words,
                    reducer=self.reducer,
                    reducer_final=self.final_reducer)]


Inserting into a binary search tree in C I am in the process of refactoring this simple binary search tree code responsible for adding new values:

void add_internal(Node *root, int value) {
    // Decide whether to insert left or right
    if (root->value >= value) {
        // Decide whether to insert here or down the subtree
        if (root->left) {
            add_internal(root->left, value);
        } else {
            root->left = create_node(value);
        }
    } else {
        // Decide whether to insert here or down the subtree
        if (root->right) {
            add_internal(root->right, value);
        } else {
            root->right = create_node(value);
        }
    }
}

to this:

Node **relevant_node = (root->value >= value) ? &root->left : &root->right;

if (*relevant_node) {
    add_internal(*relevant_node, value);
} else {
    *relevant_node = create_node(value);
}

Both solutions work. My refactored one tho is pretty much the result of trial and error, will it be as robust as the first solution?

There is a third option:

Node *add_internal(Node *root, int value) {
    if(root == NULL) {
        return create_node(value);
    }

    // Decide whether to insert left or right
    if (root->value >= value) {
        root->left = add_internal(root->left, value);
    } else {
        root->right = add_internal(root->right, value);
    }
    return root;
}

Basically moves the null test into the recursive call.

However this version requires it to be called as root = add_internal(root, value);. But it has the advantage that it can deal with a null root in a transparent way.


Locating existing records In order to reduce redundancy in my app, I have added the method self.find_or_keep(object) to many of my models in order to find existing records with the same values, so I can reference the already existing ones instead of creating new records. Basically, this method checks most attributes for equality between two objects; id, created_at and updated_at are always excluded for obvious reasons.

class IPv4 < ActiveRecord::Base
  # returns its param or an already existing record
  def self.find_or_keep(ipv4)
    # create new object in memory, if param is a Hash
    ipv4 = IPv4.new(ipv4) if ipv4.is_a?(Hash)
    return (entry = IPv4.find_by(address: ipv4.address, subnetmask: ipv4.subnetmask)) ? entry : ipv4
  end
end

class Service < ActiveRecord::Base
  # returns its param or an already existing record
  def self.find_or_keep(service)
    # create new object in memory, if param is a Hash
    service = Service.new(service) if service.is_a?(Hash)
    return (entry = Service.find_by(protocol: service.protocol, port_from: service.port_from, port_to: service.port_to)) ? entry : service
  end
end

As you can see, it's not very DRY. The problem here is that the method each time has to check for different parameters, so I have to rewrite the find_by every time I want to use the method in another class. What I have in mind for DRYing it up is something like this:

class IPv4 < ActiveRecord::Base
  implement_find_or_keep_with(:address, :subnetmask)
end

class Service < ActiveRecord::Base
  implement_find_or_keep_with(:protocol, :port_from, :port_to)
end

class ActiveRecord::Base
  # whatever is need
end

Is something like this possible? If so, how would I implement it?

Rails already has a built-in mechanism for doing this with the first_or_initialize method within ActiveRecord::Relation.

As an example, you can perform the following

Service.where(protocol: params[:protocol], port_from: params[:port_from], port_to: params[:port_to]).first_or_initialize

This will try and find a service with the given attributes OR initialize a new object with those attributes set if it can't find it.

Hope this helps.


decreasing solver speed How can I decrease the time complexity and increase efficiency, without writing a new algorithm. My solution solves the majority of puzzles in a fast time, but for some difficult ones it can take over a minute! I have added some of the difficult puzzles below the code.
import numpy as np
from skimage.util import view_as_blocks  # pip install scikit-image

#input: 9x9 numpy array, empty cell = 0
#output: 9x9 numpy array: if not solution all array entries should be -1
#backtracking depth-first search with constraint propagation

#searches for 0 value in board
def zero_search(sudoku):
    for i in range(len(sudoku)):
        for j in range(len(sudoku[0])):
            if sudoku[i][j] == 0:
                return (i, j)  # row, col

    return False

#parameters: board, num = inserted value, pos = board position(vector/tuple)
def valid(sudoku, num, pos):
    # Check row
    for i in range(len(sudoku[0])):
        if sudoku[pos[0]][i] == num and pos[1] != i:
            return False

    # Check column
    for i in range(len(sudoku)):
        if sudoku[i][pos[1]] == num and pos[0] != i:
            return False

    # Check box
    box_x = pos[1] // 3
    box_y = pos[0] // 3

    #y-axis/columns
    for i in range(box_y*3, box_y*3 + 3):
        #x-axis/rows
        for j in range(box_x * 3, box_x*3 + 3):
            if sudoku[i][j] == num and (i,j) != pos:
                return False

    return True

def solver(sudoku):

    find = zero_search(sudoku)
    if not find:
        return True

    else:
        row, col = find

    #check insertion values 1-9
    for i in range(1,10):
        if valid(sudoku, i, (row, col)):
            sudoku[row][col] = i

            if solver(sudoku):
                return True

            sudoku[row][col] = 0

    return False

def initial_invalid(sudoku):

    # Check row
    for i in range(9):
        dup_lst=[]
        for j in range(9):
            if sudoku[j][i]!=0:
                if sudoku[j][i] in dup_lst:
                    return True

                else:
                    dup_lst.append(sudoku[j][i])

    #check column
    for i in range(9):
        dup_lst=[]
        for j in range(9):
            if sudoku[i][j]!=0:
                if sudoku[i][j] in dup_lst:
                    return True

                else:
                    dup_lst.append(sudoku[i][j])

#not needed
def final_valid (sudoku):

    subgrids = view_as_blocks(sudoku, (3, 3))

    sums = [np.sum(subgrids[i][j]) for j in range(3) for i in range(3)]
    if sum(sums) !=405:
        return False

    else:
        return True

    #row_sum = sudoku.sum(axis=1)
    #if sum(row_sum) != 405:
    #   return False

    #col_sum = sudoku.sum(axis=0)
    #if sum(col_sum) != 405:
    #    return False

def sudoku_solver(sudoku):

    if initial_invalid(sudoku):
        return np.full((9,9),-1)

    if solver(sudoku):
            return sudoku
    else:
        return np.full((9,9),-1)

[[0 8 0 4 3 0 0 0 0]
 [0 0 5 0 0 9 0 0 0]
 [6 0 0 0 8 0 0 7 0]
 [0 0 0 0 9 0 0 0 3]
 [0 0 0 8 0 7 0 0 0]
 [9 0 0 0 0 0 0 5 4]
 [0 6 0 0 0 0 0 0 5]
 [0 0 8 0 0 0 4 0 0]
 [0 4 0 0 0 6 0 1 0]]

[[0 0 2 0 0 0 0 0 4]
 [0 5 0 0 1 3 7 0 0]
 [7 9 0 0 0 0 0 5 0]
 [0 0 9 0 0 0 0 6 0]
 [0 0 0 0 3 0 5 0 8]
 [5 0 7 0 0 0 4 0 0]
 [0 0 0 0 6 0 8 0 0]
 [0 6 0 0 2 7 0 4 0]
 [8 0 0 0 0 0 0 2 0]]

[[0 0 0 6 0 0 2 0 0]
 [0 0 0 0 0 9 0 6 0]
 [0 8 0 0 0 5 0 0 3]
 [1 0 0 4 0 0 9 0 0]
 [8 3 0 0 0 0 0 0 0]
 [0 2 0 0 0 6 0 0 0]
 [0 0 0 0 6 0 0 0 0]
 [2 5 0 3 0 7 0 9 0]
 [0 0 1 0 0 0 0 8 4]]

Keep track of possible values
A comment in your code says:

#backtracking depth-first search with constraint propagation

...but you're not actually propagating constraints. You can reduce the number of move validations by keeping track of the possible moves, i.e. the values a cell can  actually have. Each inserted value (correct or not) constrains the possible values of cells in the same row, column and box, thus reducing the number of values you have to consider in subsequent recursive calls.
Choose empty cells with least number of possible values
You're choosing the first empty cell. If that happens to have very little constraints, you're unnecessarily exploring a lot of possibilities that may have been invalidated if you had chosen a different cell first. Choosing the empty cell with the least number of possible values drastically reduces the breadth of your depth-first search tree.
Example implementation
I tried to stay close to your original code and this code should only demonstrate how you can implement the mentioned concepts. There are still things that could be improved, but since you specifically asked about time complexity I concentrated purely on that.
import numpy as np
from skimage.util import view_as_blocks  # pip install scikit-image

# input: 9x9 numpy array, empty cell = 0
# output: 9x9 numpy array: if not solution all array entries should be -1
# backtracking depth-first search with constraint propagation

# Returns (row, col) of an empty cell with the least number of possible values.
# Returns False if no empty cell exists.
def get_emtpy_cell_with_least_possible_values(sudoku, possible_values):
    min_possible_values = 10
    min_row = -1
    min_col = -1
    for i in range(len(sudoku)):
        for j in range(len(sudoku[0])):
            if sudoku[i][j] == 0:
                num_possible_numbers = sum(possible_values[i][j])
                if num_possible_numbers < min_possible_values:
                    min_possible_values = num_possible_numbers
                    min_row = i
                    min_col = j
    if min_possible_values < 10:
        return (min_row, min_col)
    else:
        # No empty cell found
        return False

def remove_possible_values(possible_values, row, col, value):
    """ Remove 'value' from the possible values in row, col, and the box containing
    cell [row][col]. Returns a list of tuples (row, col) for each cell that has been changed.
    """
    cells_changed = []
    for c_index, c in enumerate(possible_values[row]):
        if c[value - 1] == 1:
            cells_changed.append((row, c_index))
            c[value - 1] = 0

    for r in range(9):
        if possible_values[r][col][value - 1] == 1:
            cells_changed.append((r, col))
            possible_values[r][col][value - 1] = 0

    # Check box
    box_x = col // 3
    box_y = row // 3

    # y-axis/columns
    for i in range(box_y*3, box_y*3 + 3):
        # x-axis/rows
        for j in range(box_x * 3, box_x*3 + 3):
            if possible_values[i][j][value - 1] == 1:
                cells_changed.append((i,j))
                possible_values[i][j][value - 1] = 0
    return cells_changed

def restore_possible_values(possible_values, value, changed_cells):
    """ Restore 'value' as possible value for each cell (row, col) in 'changed_cells'.
    """
    for row, col in changed_cells:
        possible_values[row][col][value - 1] = 1

def solver(sudoku, possible_values):
    next_cell = get_emtpy_cell_with_least_possible_values(sudoku, possible_values)
    if not next_cell:
        return True
    else:
        row, col = next_cell

    # check insertion values 1-9
    for guessed_value in range(1, 10):
        if possible_values[row][col][guessed_value - 1] != 1:
            continue
        # update possible values and sudoku board for recursive call
        changed_cells = remove_possible_values(possible_values, row, col, guessed_value)
        sudoku[row][col] = guessed_value

        if solver(sudoku, possible_values):
            return True
        # restore possible values and sudoku board for next iteration
        restore_possible_values(possible_values, guessed_value, changed_cells)
        sudoku[row][col] = 0

    return False

def initial_invalid(sudoku):

    # Check row
    for i in range(9):
        dup_lst = []
        for j in range(9):
            if sudoku[j][i] != 0:
                if sudoku[j][i] in dup_lst:
                    return True

                else:
                    dup_lst.append(sudoku[j][i])

    # check column
    for i in range(9):
        dup_lst = []
        for j in range(9):
            if sudoku[i][j] != 0:
                if sudoku[i][j] in dup_lst:
                    return True

                else:
                    dup_lst.append(sudoku[i][j])

def sudoku_solver(sudoku):

    if initial_invalid(sudoku):
        return np.full((9, 9), -1)

    possible_values = np.ones((9,9,9), dtype=int)
    # initialize possible values based on already existing values
    for row in range(9):
        for col in range(9):
            value = sudoku[row][col]
            if value != 0:
                remove_possible_values(possible_values, row, col, value)

    if solver(sudoku, possible_values):
        return sudoku
    else:
        return np.full((9, 9), -1)

Some measurements on my computer using your examples:

original code
improved code

Sudoku 1
21.836s
1.449s

Sudoku 2
2.753s
0.639s

Sudoku 3
50.993s
0.479s


Dynamic array sorting and storing program I have written a program which closely resembles the way contacts are added and dynamically sorted in ascending order in your phone or other device. Though I have used Integers instead of String, I was just wondering if this has any real world use and how I can improve it.

import java.util.*;

public class Ascending
{

    ArrayList<Integer> array = new ArrayList<Integer>();

    public Ascending()
    {
        array.add(2);
        array.add(4);
        array.add(6);
        array.add(8);
    }

    public static void main()
    {
        Ascending obj = new Ascending();
        obj.ascendingOrder();
    }

    public void ascendingOrder()
    {
        for(int loop=0;loop<=10;loop++)
        {
            Scanner sc = new Scanner(System.in);
            System.out.println("The array is 2,4,6,8 ");
            System.out.println("Enter a number you wish to insert in the array");
            int input = sc.nextInt();
            int i,size = array.size(),last=size-1;

            for(i=0;i<size;i++)
            {
                if(input<array.get(i))
                {
                    array.add(i,input);
                    System.out.println("\nSorted array -:");
                    for(int y:array)
                    {
                        System.out.println(y);

                    }
                    break;
                }
            }

            if(input>array.get(last))
                {
                    array.add(size,input);
                    System.out.println("\nSorted array -:");

                    for(int q:array)
                    {
                        System.out.println(q);
                    }
                }
        }
    }
}

import java.util.*;

It's often preferred to do separate imports rather than import an entire group.  Some disagree with this.  You can see both viewpoints argued at https://stackoverflow.com/questions/147454/why-is-using-a-wild-card-with-a-java-import-statement-bad

    ArrayList<Integer> array = new ArrayList<Integer>();

It's generally preferred to use the interface on the left side, as so

    List<Integer> numbers = new ArrayList<Integer>();

It won't make much difference in a program like this, but in a program where you pass your field as a method parameter, it can make it much easier to change implementations in the future.  It also encourages you to think about whether or not to use implementation specific functionality.

I prefer to use a more specific name than array.  Of course, in this case, you don't have much of one as the field is pretty generic.  I might also go with data.  Some would prefer array to these names.

    public Ascending()
    {
        array.add(2);
        array.add(4);
        array.add(6);
        array.add(8);
    }

Is that test code in your constructor?  That's not the place for it.  Consider

    public Ascending(List<Integer> initial)
    {
        numbers.addAll(initial);
    }

or better yet, something like

    public Ascending(List<Integer> initialData)
    {
        for (Integer number : initialData)
        {
            numbers.add(number);
        }
    }

As that makes sure that the order is correct (assuming add is implemented correctly).

Then you can change the values in the calling code rather than editing the class itself.  I.e. change

        Ascending obj = new Ascending();

to something like

        Ascending ascending = new Ascending(Arrays.asList(2, 4, 6, 8));

Alternately, you could define a new method add and call it in main.  This would also allow you to pull some code out of your ascendingOrder method.

    public void ascendingOrder()

Usually methods have verb names.  Since this takes input, I might call it something like addFromInput.

Also consider breaking this into three parts:  a for loop in main; an input process method; and an add method.

        for(int loop=0;loop<=10;loop++)
        {
            Scanner sc = new Scanner(System.in);

You don't need to make a new Scanner for every iteration.  Do it once prior to starting the loop:

        Scanner sc = new Scanner(System.in);
        for (int loop = 0; loop <= 10; loop++)
        {

I also added some whitespace in the for loop declaration.  It makes it easier for me to read (the computer won't care).

            System.out.println("The array is 2,4,6,8 ");

Consider writing a toString method so that you can just say

            System.out.println("The array is " + this);

Then it will output the current array on every iteration rather than outputting the original array.  This also allows you to change the initial values in only one place.

Alternately, you can write a print method that does the same thing.  The toString is more flexible though.

            int i,size = array.size(),last=size-1;

            for(i=0;i<size;i++)
            {
                if(input<array.get(i))
                {
                    array.add(i,input);
                    System.out.println("\nSorted array -:");
                    for(int y:array)
                    {
                        System.out.println(y);

                    }
                    break;
                }
            }

            if(input>array.get(last))
                {
                    array.add(size,input);
                    System.out.println("\nSorted array -:");

                    for(int q:array)
                    {
                        System.out.println(q);
                    }
                }

The indent looks wrong at the end there.  However, this whole block of code can be replaced with

            add(input);
            System.out.println("\nSorted array -: " + this);

if you define add and toString as previously discussed.

The add method can be defined as

    public void add(Integer number)
    {
        int i = 0;
        int size = numbers.size();

        while (i < size && number < numbers.get(i))
        {
            i++;
        }

        numbers.add(i, number);
    }

or

    public void add(Integer new)
    {
        int i = 0;

        for (Integer number : numbers)
        {
            if (new < number) {
                numbers.add(i, new);
                return;
            }

            i++;
        }

        numbers.add(new);
    }

among other implementations.

Note that I don't do your last check.  It's unnecessary in both versions.

In the first version, i always equals the correct place in the list when it exits the loop.

In the second version, it never reaches the code outside the loop unless it has already checked every previous location.  Note that when used with just one argument, add appends the element to the list.  This has the same effect as numbers.add(size, new); but is shorter to write.

Moving the printing of the array out of the add process simplifies the logic as well and saves duplicating code.


Do you view this ThrowingFunction interface as "interface abuse"? I am currently implementing a fun project involving lambdas and this project makes heavy use of the ability of Java 8 to have default methods in interfaces.

But when does such "heavy use" turn into an outright abuse? Here is one example of an interface which I have defined, and its 4 methods may extend to even more in the future; am I overdoing it?

@FunctionalInterface
public interface ThrowingFunction<T, R>
    extends Function<T, R>
{
    R doApply(T t)
        throws Throwable;

    @Override
    default R apply(T t)
    {
        try {
            return doApply(t);
        } catch (Error | RuntimeException e) {
            throw e;
        } catch (Throwable tooBad) {
            throw new ThrownByLambdaException(tooBad);
        }
    }

    default Function<T, R> orReturn(R defaultValue)
    {
        return t -> {
            try {
                return doApply(t);
            } catch (Error | RuntimeException e) {
                throw e;
            } catch (Throwable ignored) {
                return defaultValue;
            }
        };
    }

    default <E extends RuntimeException> Function<T, R> orThrow(
        Class<E> exceptionClass)
    {
        return t -> {
            try {
                return doApply(t);
            } catch (Error | RuntimeException e) {
                throw e;
            } catch (Throwable tooBad) {
                throw ThrowablesFactory.INSTANCE.get(exceptionClass, tooBad);
            }
        };
    }
}

(Not that this is relevant, but the source for ThrowablesFactory can be viewed here.)

When does such "heavy use" turn into outright abuse?

What it seems like you're asking is "Am I abusing Default Methods?".

When in doubt about how to use a new piece of functionality I typically turn to the Java tutorials. As is the case with other new pieces of functionality in Java 8, there is documentation about Default Methods here.

One specific snippet that caught my eye is this:

"Default methods enable you to add new functionality to the interfaces of your libraries and ensure binary compatibility with code written for older versions of those interfaces."

I would ask yourself several questions about the implementation in assessing whether or not the new functionality is being abused:

Am I exposing too much business logic in the interface through the default methods?
Is there sufficient justification to expose the methods through the interface as opposed to having them implemented in an abstract class?
Is the code going to be maintainable?
Am I using the default methods feature in the way which it was created to be used?

To address your question about overdoing it, in my opinion you are. The documentation seems to imply what you should be using default methods for and this does not fit that mold. Especially since you're starting with an empty slate as opposed to adding new functionality to existing libraries for which you have to maintain binary compatibility.

In addition, take a look at the Java 8 List interface. Default methods are provided which accept other functional arguments and utilize those arguments on the implemented instance. The same seems true for the Comparator interface.

From the snippet provided it seems like the default methods do not utilize functional arguments. I also don't see any advantage of doing things this way over using the template pattern (which it seems like you're doing anyway) and having an abstract base class declare  the doApply method and have the abstract class call that from behind the curtain of the interface.

I'm biased in that I favor hiding functionality behind interfaces as opposed to exposing it. I perceive the intention of interfaces to be encapsulating data and hiding business logic and design following those concepts.

Here is some reading pertaining to anti-patterns that I try to keep in mind that may be applicable to your question:

Golden Hammer
Silver Bullet

Also, Oracle almost HAD to add some kind of functionality to the language that would provide backwards compatibility with existing libraries while also injecting new functionality. I consider the usage that the developers of the language chose to be a good starting place for evaluating my own usage.

As an aside, there looks to be some really cool stuff in your project.


Simple Publisher Subscriber in Java I am new to Java and had some difficulty creating this simple Publisher Subscriber class. It finally seems to be working and I wanted to check if I have been following best practices and see if there are any improvements I can make.

The following Java class accepts any topics as a string (e.g. "rand_int") and forwards any messages to that topic to any subscribers.

import java.util.ArrayList;
import java.util.HashMap;
import java.util.Map;
import java.util.function.Consumer;

public class PubSub {
  private Map<String, ArrayList<Consumer<?>>> topicsSubscribers =
      new HashMap<String, ArrayList<Consumer<?>>>();
  private static PubSub pubSubSingleton = null;

  public static PubSub getInstance() {
    if (pubSubSingleton == null)
      pubSubSingleton = new PubSub();

    return pubSubSingleton;
  }

  public <T> void publish(String topic, T message) {
    ArrayList<Consumer<?>> subscribers = this.topicsSubscribers.get(topic);
    if (subscribers == null)
      return;

    for (Consumer subscriberConsumer : subscribers) {
      subscriberConsumer.accept(message);
    }
  }

  public synchronized <T> void subscribe(String topicName, Consumer<T> subscriberCallback) {
    ArrayList<Consumer<?>> subscribers = this.topicsSubscribers.get(topicName);
    if (subscribers == null) {
      subscribers = new ArrayList<Consumer<?>>();
      subscribers.add(subscriberCallback);
      this.topicsSubscribers.put(topicName, subscribers);
    } else {
      subscribers.add(subscriberCallback);
    }
  }
}

Example usage:

public class SomeClass {
  public SomeClass() {
    PubSub.getInstance().subscribe("rand_int", this::randIntCallback);
    PubSub.getInstance().publish("rand_int", 46);
  }

  public randIntCallback(int number) {
    System.out.println(number);
  }
}

The above code should output "46".

As I said, I am very new to Java and would like some feedback on my code to help improve. So any help on this is very appreciated.

Initializing object variables

  private Map<String, ArrayList<Consumer<?>>> topicsSubscribers =
      new HashMap<String, ArrayList<Consumer<?>>>();

The standard is to set the types as the interfaces, like Map and List.

    private final Map<String, List<Consumer<?>>> topicSubscribers = new HashMap<>();

Also, you don't need to specify the contents of <> in the implementation instantiation.  The compiler will work that out for you.

Initializing class variables

  private static PubSub pubSubSingleton = null;

  public static PubSub getInstance() {
    if (pubSubSingleton == null)
      pubSubSingleton = new PubSub();

    return pubSubSingleton;
  }

You don't have to do it this way.  You can simply say

    private static final PubSub pubSubSingleton = new PubSub();

    public static PubSub getInstance() {
        return pubSubSingleton;
    }

    private PubSub() {
    }

This saves the null check each time while still only initializing once.

However, Singleton is generally considered an anti-pattern now.  You may want to consider alternatives.  One possibility would be a separate instance for each message type.

Your original code does not make the constructor private.

The Java standard is four column indents rather than the two you are using.

Formatting

    if (subscribers == null)
      return;

If you are going to use the statement form, you should probably keep it one line:

    if (subscribers == null) return;

If it's too complicated to fit into one line, you should probably use the block form instead.

I actually use the block form consistently regardless.

    if (subscribers == null) {
        return;
    }

Factor out the common

    if (subscribers == null) {
      subscribers = new ArrayList<Consumer<?>>();
      subscribers.add(subscriberCallback);
      this.topicsSubscribers.put(topicName, subscribers);
    } else {
      subscribers.add(subscriberCallback);
    }

This could be

    if (subscribers == null) {
        subscribers = new ArrayList<Consumer<?>>();
        topicsSubscribers.put(topicName, subscribers);
    }

    subscribers.add(subscriberCallback);

You don't have to do the add before the put.  It's the same object either way.  Otherwise, you'd have to do the put after the add in the non-null case.


Aspect-oriented vs explicit style in Python Readability counts, but is very subjective. The following snippets are equivalent in functionality and turn a generator iterator into a chunked HTTP response in Flask. Which of those two patterns is more Pythonic, readable and convenient?

A) Aspect style with function decorators that change the return value:

@app.route('/')
@flask_response_decorator
@optional_streaming_decorator
@progress_log_decorator
def index() -> Iterator:
    """Index page, optionally HTTP chunk-streamed and formatted."""
    return Foo.get_instance().are_changes_locked()

or

B) explicit decorators within the function body

@app.route('/')
def index() -> flask.Response:
    """Index page, optionally HTTP chunk-streamed and formatted."""
    response = Foo.get_instance().are_changes_locked()
    return flask.Response(optional_iterator_to_stream(iterator_to_progress_log(response)))

Pythonic, readable and convenient

are all subjective and open to debate, even though "Pythonic" is less so than the others. That said, a simplified version may be illustrative. Comparing

@foo
@bar
def baz(value: T) -> T:
    return value

to

def baz(value: T) -> FooT:
    return foo(bar(value))

there are two issues with the former:

The final return type is now hidden in foo.
It is easy to confuse the layering - is baz(value) equivalent to foo(bar(baz(value))) or bar(foo(baz(value))) (or even baz(foo(bar(value))) in case of a novice)? Nested decorators come up so rarely that I would have to look at the documentation to be absolutely sure whether they were in the right sequence.

There seems to be two situations where multi-level decorators would be fine:

If they pass through the return value unchanged. A logger would be a typical example.
If the decorators are commutative, that is, their order doesn't matter for the outcome.


Replace for loop while with apply family function in R I have trouble with for loop, my code runs very slowly. The thing I want to do is to use function from apply family to make my codes run faster (instead of using for loopand while). Here is an example and my loop:

require(data.table)
require(zoo)

K<-seq(1,1000, by=1)
b<-c(rep(2,250), rep(3, 250), rep(4, 250), rep(5,250))
a<-c(rep(6,250), rep(7,250), rep(8,250), rep(9,250))
rf<-rep(0.05, 1000)
L<-rep(10,1000)
cap<-rep(20,1000)
df<-data.frame(K, rf, L, cap, a,b)
blackscholes <- function(S, X, rf, h, sigma) {
    d1 <- (log(S/X)+(rf+sigma^2/2)*h)/sigma*sqrt(h)
}
df$logiterK<-log(df$K)
df<-as.data.table(df)
df[,rollsd:=rollapply(logret, 250, sd, fill = NA, align='right')*sqrt(250), by=c("a", "b")]
df[,assetreturn:=c(NA,diff(logiterK)),by=c("a", "b")]
df[,rollsdasset:=rollapply(assetreturn, 249, sd, fill=NA, align='right')*sqrt(250), by=c("a", "b")]
df[,K1:=(cap+L*exp(-rf)*pnorm(blackscholes(K,L,rf, 1,rollsdasset[250]))-rollsdasset[250])/pnorm(blackscholes(K,L,rf, 1,rollsdasset[250])),by=c("a","b")]

errors<-ddply( df, .(a,b), function(x) sum((x$K-x$K1)^2))
df<-as.data.frame(df)
df<-join(df, errors, by=c("a", "b"))
for ( i in 1:nrow(errors)){
    while(errors$V1[i] >= 10^(-10)) {
        df<-as.data.table(df)
        df[,K:= K1,by=c("a", "b")]
        df[,assetreturn:=c(NA,diff(log(K))),by=c("a", "b")]
        df[,rollsdasset:=rollapply(assetreturn, 249, sd, fill=NA, align='right')*sqrt(250), by=c("a", "b")]
        df[,iterK1:=(cap+L*exp(-rf)*pnorm(blackscholes(K,L,rf, 1,rollsdasset[250]))-rollsdasset[250])/pnorm(blackscholes(K,L,rf, 1,rollsdasset[250])) ,by=c("a", "b")]
        df<-as.data.frame(df)
        errors$V1[i]<-sum((df[df$V1 %in% errors$V1[i],"K"]-df[df$V1 %in% errors$V1[i],"K1"])^2)
        }
    }

Any help would be appreciated.

You could replace the for loop with a function + sapply like this:

reduce.errors <- function(err) {
  while (err >= 10^(-10)) {
    df<-as.data.table(df)
    df[,K:= K1,by=c("a", "b")]
    df[,assetreturn:=c(NA,diff(log(K))),by=c("a", "b")]
    df[,rollsdasset:=rollapply(assetreturn, 249, sd, fill=NA, align='right')*sqrt(250), by=c("a", "b")]
    df[,iterK1:=(cap+L*exp(-rf)*pnorm(blackscholes(K,L,rf, 1,rollsdasset[250]))-rollsdasset[250])/pnorm(blackscholes(K,L,rf, 1,rollsdasset[250])) ,by=c("a", "b")]
    df<-as.data.frame(df)
    err <- sum((df[df$V1 %in% err,"K"]-df[df$V1 %in% err,"K1"])^2)
  }
}
sapply(errors$V1, reduce.errors)

But I don't think this will make it faster at all. If I understand correctly you need the while loop there to reduce the error below a threshold, and so you need the iteration and this cannot be replaced with the "apply" functions easily.

If you want to improve the speed, I think you'll need to rethink come up with a different approach, if it's even possible.


Creating some Loaded Dice to make our board game fairer Some friends and I were playing a board game and decided that the random dice rolls just weren't fair! So we came up with a scheme for making the rolls slightly more evenly distributed, and I implemented it in python. The class represents 2 six sided dice and makes the probability of a given combination coming up (there are 36 options for 2 dice) inversely proportional to the number of times it has come up previously - thereby ensuring the distribution tends to be more even than randomness allows:
from collections import Counter
from typing import Tuple, Counter as TCounter
import random

class LoadedDice:
    """
    Simulation of 2 Dice being rolled.
    Weights the random roll to try and tend to an even distribution
    """

    def __init__(self, sides: int = 6) -> None:
        self.sides = sides
        self.history: TCounter[int] = Counter(range(sides ** 2))

    def roll(self) -> Tuple[int, int]:
        result_index = self.get_weighted_random_roll_index()
        self.history[result_index] += 1
        return self.roll_value_from_index(result_index)

    def get_weighted_random_roll_index(self) -> int:
        roll_threshold = random.random()
        reciprocals_total = sum(self.reciprocals())
        running_total = 0
        for result_index, value in enumerate(self.reciprocals()):
            running_total += value / reciprocals_total
            if roll_threshold <= running_total:
                return result_index

    def roll_value_from_index(self, index: int) -> Tuple[int, int]:
        mod, remainder = divmod(index, self.sides)
        roll_1 = mod + 1
        roll_2 = remainder + 1
        return roll_1, roll_2

    def reciprocals(self):
        for v in self.history.values():
            yield 1 / v

    @property
    def roll_history(self) -> TCounter[Tuple[int, int]]:
        result: TCounter[Tuple[int, int]] = Counter()
        for roll_index, count in self.history.items():
            result[self.roll_value_from_index(roll_index)] += count
        return result

    @property
    def result_history(self) -> TCounter[int]:
        result: TCounter[int] = Counter()
        for roll_index, count in self.history.items():
            result[sum(self.roll_value_from_index(roll_index))] += count
        return result

    def __repr__(self):
        return repr(self.roll_history)

... which can be used like this:
especially_fair_dice = LoadedDice(6)  # simulate 2 6-sided dice
especially_fair_dice.roll()  # returns the dice values results like (1,5)

Feedback
I'd like some review on types, as I've never done them before, efficiency of my algorithm; I feel like it's a bit overly complicated, anything I've missed about pseudo-random numbers that make my approach undesirable etc.
Results
I plotted some of the results, for 100 rolls, the loaded die approach tends to perform better than random at keeping to the expected distribution, while still feeling random (that's the goal, interpret it as you wish, I don't know how to quantify it either:)

Performance
If you intend on using this for (e.g.) large-scale Monte-Carlo simulation, consider moving to vectorized Numpy rather than base Python.
Direct generators
There's no need for yield here:
    for v in self.history.values():
        yield 1 / v

Instead,
return (1/v for v in self.history.values())

Counter initialization
roll_history can be reduced, I think, to
return Counter(
    (
        sum(self.roll_value_from_index(roll_index)),
        count,
    )
    for roll_index, count in self.history.items()
)

In other words, Counter accepts an iterable of key-count tuples. If the above generator expression seems unwieldy to you, write a separate method to yield roll-count pairs:
    for roll_index, count in self.history.items():
        roll_1, roll_2 = self.roll_value_from_index(roll_index)
        yield roll_1 + roll_2, count


Pass service to view model or leave thin controller as is I have the following action:

    public PartialViewResult Analyze()
    {
        var viewModel = new WelcomeAnalyzeViewModel
        {
            IsDrugsPanelVisible = !Service.IsMDStandalone(),
            IsDevicesPanelVisible = Service.IsSubscribedToMedTech(),
            IsCompaniesPanelVisible = !Service.IsCTStandalone(),
            IsCountriesPanelVisible = !Service.IsCTStandalone() || (Service.IsCTStandalone() && Service.HasImsAddOn()),
            IsFinancePanelVisible = !Service.IsCTStandalone()
        };

        return PartialView("Analyze", viewModel);
    }

I'm not sure if I should just pass the Service property to the constructor of the WelcomeAnalyzeViewModel or leave this code as is but from the perspective of a thin controller this doesn't feel right.

The Service property contains an object to a permission service (IPermissionService) which can be used for other applications outside of the web environment.

Would it be acceptable for one service to call another service, so for instance I create an AnalyzeService which contains methods with calls to IPermissionService methods?

I would suggest having a separate class for doing this processing and mapping.
It will be a service or mapper class takes your Service object in the constructor and has a method which return WelcomeAnalyzeViewModel object.

Also you can do it all through the method, by passing your Service object to this method and return the WelcomeAnalyzeViewModel, this will make your controller very thin something like that:

public PartialViewResult Analyze()
{
        var viewModel = Mapper.GetWelcomeAnalyzeViewModel(Service);
        return PartialView("Analyze", viewModel);
}

public class Mapper
{
  public WelcomeAnalyzeViewModel(ServiceType service)
  {
      return new WelcomeAnalyzeViewModel
        {
            IsDrugsPanelVisible = !Service.IsMDStandalone(),
            IsDevicesPanelVisible = Service.IsSubscribedToMedTech(),
            IsCompaniesPanelVisible = !Service.IsCTStandalone(),
            IsCountriesPanelVisible = !Service.IsCTStandalone() || (Service.IsCTStandalone() && Service.HasImsAddOn()),
            IsFinancePanelVisible = !Service.IsCTStandalone()
        };
  }
}

And i will put this mapper class in the ViewModels project if you have a separate project for them.


Filtering a Vec of structs and creating new vector of Strings or strs in Rust I'm working through the Rust book, and it has the following example of how to use the filter method on Iterators (source):
struct Shoe {
    size: u32,
    style: String,
}

fn shoes_in_my_size(shoes: Vec<Shoe>, shoe_size: u32) -> Vec<Shoe> {
    shoes.into_iter().filter(|s| s.size == shoe_size).collect()
}

I started thinking of how I could improve this function with the following objectives:

Do not consume/take ownership the shoes vector, so the caller can still use it.
Return a vector of just the styles, rather than of the whole Shoe struct.

I came up with the following two possible solutions:
fn shoe_styles_in_my_size(shoes: &[Shoe], shoe_size: u32) -> Vec<&String> {
    shoes
        .iter()
        .filter(|s| s.size == shoe_size)
        .map(|s| &s.style)
        .collect()
}

and:
fn shoe_styles_in_my_size(shoes: &[Shoe], shoe_size: u32) -> Vec<&str> {
    shoes
        .iter()
        .filter(|s| s.size == shoe_size)
        .map(|s| &s.style[..])
        .collect()
}

The difference being that the first example returns Vec<&String> and the second returns Vec<&str>.
What criteria would I use to determine which of these two is "better?"  I've been under the impression that using str is preferable to using String when possible, since it is more flexible (hopefully this is accurate?) Is there any overhead associated with the [..] operation?  My understanding says no, but I'm not sure.

The [..] slicing operation isn't going to cost anything, but isn't very descriptive either. I'd want to call s.style.as_str() instead.
As for the return type, I'd agree that &str is more appropriate, since it is the more general type. The functionality of a String is a superset of what you can do with a str, so it's irrelevant when that extra functionality isn't useful to the caller


Code for printing list in React Below is an implementation of a React app printing list. Are there any areas of improvement/suggestions in below code base

  class ListElement extends React.PureComponent {
   render() {
       return
 this.props.handleClick(this.props.index)}>{this.props.item.text}
;
   }
}

class ListComponent extends React.Component {

       handleClick(index) {
           this.props.onClick(index);
       }

       render() {
           const listDetails = this.props.listDetails;
           let height,width;
           if (listDetails.hasOwnProperty("size")) {
               height = listDetails.size.height;
               width = listDetails.size.width;
           }
           return (

                   {this.props.items.map((item, i) => <ListElement item={item} key={i} index={i} handleClick={this.handleClick.bind(this)}/>)}

           );
       }
    }

    ListComponent.propTypes = {
       items: PropTypes.arrayOf(PropTypes.shape({
           text: PropTypes.string.isRequired
       })).isRequired,
       listDetails: PropTypes.object.isRequired
    };

Use function syntax/memo when possible
Use destructuring
Use object literal shorthand
Define defaults for width and height
handleClick is just onClick renamed
Use tooling for formatting your code (eslint, prettier)
Prefer ternary over if (not always true)
Have a well defined API for your components (ListElement needs text, not the entire item, and doesn't really need index)

const ListElement = React.Memo(({ handleClick, label }) => (

{label}

));

const ListComponent = React.Memo(
  ({
    items,
    listDetails: {
      size: { height, width }
    },
    onClick
  }) => (

      {items.map(({ text }, i) => (
        <ListElement label={text} key={i} handleClick={() => onClick(i)} />
      ))}

  )
);

ListComponent.propTypes = {
  items: PropTypes.arrayOf(
    PropTypes.shape({
      text: PropTypes.string.isRequired
    })
  ).isRequired,
  listDetails: PropTypes.object.isRequired
};

// Not quite sure I'm setting defaultProps correctly here, but you get the idea
ListComponent.defaultProps = {
  listDetails: { size: { height: DEFAULT_HEIGHT, width: DEFAULT_WIDTH } }
};

You can also update propTypes with the shape of listDetails or change so that ListComponent takes size as an attribute instead, depending on what makes sense in your scenario.