Increase performance creating Mandelbrot set in python I created a program in python that generates an image of the mandelbrot set. The only problem I have is that the program is quite slow, it takes about a quarter of an hour to generate the following image of 2000 by 3000 pixels:

I first created a matrix of complex numbers using numpy according to amount of pixels. I also created an array for the image generation.

import numpy as np
from PIL import Image

z = 0

real_axis = np.linspace(-2,1,num=3000)
imaginary_axis = np.linspace(1,-1,num=2000)

complex_grid = [[complex(np.float64(a),np.float64(b)) for a in real_axis] for b in imaginary_axis]

pixel_grid = np.zeros((2000,3000,3),dtype=np.uint8)

Then I check whether each complex number is in the mandelbrot set or not and give it an RGB colour code accordingly.

for complex_list in complex_grid:
    for complex_number in complex_list:
        for iteration in range(255):
            z = z**2 + complex_number
            if (z.real**2+z.imag**2)**0.5 > 2:
                pixel_grid[complex_grid.index(complex_list),complex_list.index(complex_number)]=[iteration,iteration,iteration]
                break
            else:
                continue
        z = 0

Finally I generate the image using the PIL library.

mandelbrot = Image.fromarray(pixel_grid)
mandelbrot.save("mandelbrot.png")

I am using jupyter notebook and python 3. Hopefully some of you can help me improve the performance of this program or other aspects of it.

I'm going to reuse some parts of the answer I recently posted here on Code Review.

Losing your Loops

  (Most) loops are damn slow in Python. Especially multiple nested loops.

  NumPy can help to vectorize your code, i.e. in this case that more
  of the looping is done in the C backend instead of in the Python
  interpreter. I  would highly recommend to have a listen to the talk
  Losing your Loops: Fast Numerical Computing with NumPy by Jake
  VanderPlas.

All those loops used to generate the complex grid followed by the nested loops used to iterate over the grid and the image are slow when left to the Python interpreter. Fortunately, NumPy can take quite a lot of this burden off of you.

For example

real_axis = np.linspace(-2, 1, num=3000)
imaginary_axis = np.linspace(1, -1, num=2000)
complex_grid = [[complex(np.float64(a),np.float64(b)) for a in real_axis] for b in imaginary_axis]

could become

n_rows, n_cols = 2000, 3000
complex_grid_np = np.zeros((n_rows, n_cols), dtype=np.complex)
real, imag = np.meshgrid(real_axis, imaginary_axis)
complex_grid_np.real = real
complex_grid_np.imag = imag

No loops, just plain simple NumPy.

Same goes for

for complex_list in complex_grid:
    for complex_number in complex_list:
        for iteration in range(255):
            z = z**2 + complex_number
            if (z.real**2+z.imag**2)**0.5 > 2:
                pixel_grid[complex_grid.index(complex_list),complex_list.index(complex_number)]=[iteration,iteration,iteration]
                break
            else:
                continue
        z = 0

can be transformed to

z_grid_np = np.zeros_like(complex_grid_np)
elements_todo = np.ones((n_rows, n_cols), dtype=bool)
for iteration in range(255):
    z_grid_np[elements_todo] = \
        z_grid_np[elements_todo]**2 + complex_grid_np[elements_todo]
    mask = np.logical_and(np.absolute(z_grid_np) > 2, elements_todo)
    pixel_grid_np[mask, :] = (iteration, iteration, iteration)
    elements_todo = np.logical_and(elements_todo, np.logical_not(mask))

which is just a single loop instead of three nested ones. Here, a little more trickery was needed to treat the break case the same way as you did. elements_todo is used to only compute updates on the z value if it has not been marked as done. There might also be a better solution without this.

I added the following lines

complex_grid_close = np.allclose(np.array(complex_grid), complex_grid_np)
pixel_grid_close = np.allclose(pixel_grid, pixel_grid_np)
print("Results were similar: {}".format(all((complex_grid_close, pixel_grid_close))))

to validate my results against your reference implementation.

The vectorized code is about 9-10x faster on my machine for several n_rows/n_cols combinations I tested. E.g. for n_rows, n_cols = 1000, 1500:

Looped generation took 61.989842s
Vectorized generation took 6.656926s
Results were similar: True

Lose a dimension

An aspect I somehow slightly ignored while looking at your code was that you're essentially creating a grayscale image since all of your color channel values are the same. Accounting for this, you can easily reduce the size of data the program has handle to from, in your case, 3000x2000x3 to 3000x2000. This will likely help your program to be more cache efficient, although I'm not an expert in this field.

Edit/Appendix: Further timings

Including the "no square root" optimization as suggested by trichoplax in his answer and Peter Cordes in the comments like so

mask = np.logical_and((z_grid_np.real**2+z_grid_np.imag**2) > 4, elements_todo)

will give you about another second and a half for n_rows, n_cols = 1000, 1500, i.e. about 12x the speed of the original solution

10 loops, best of 5: 4.98 s per loop
10 loops, best of 5: 4.28 s per loop (in grayscale, 14x)

A quick implementation of Reinderien's hint towards the symmetry of the Mandelbrot set will again add a factor of about two to that.

10 loops, best of 5: 2.54 s per loop (~24x)
10 loops, best of 5: 2.07 s per loop (in grayscale, ~30x)

However, my quick hacking approach did not lead to an output that was completely within the tolerance of np.allclose compared to the original one. Funnily, it seems to be off by one at a single pixel, but visually still the same. Since this post is already quite long, I will leave the reimplementation as an exercise to the reader.

Depending on your needs, you might also go down with the floating point precision from 64bit to 32bit on addition to all the previously mentioned optimizations.

10 loops, best of 5: 1.49 s per loop (~41x)
10 loops, best of 5: 1.14 s per loop (in grayscale, ~54x)


An imitation of reading a C Struct from binary file in python 2.7 Preliminary

I need to read a binary file constructed by writing a Struct to it with Python. So I decided to write a simple wrapper using struct module.

Code

import struct

class structure:
    def __init__ ( self, little_endian = True ):
        self._little_endian = little_endian

        self._table  = dict() #stores field name, its format and value
        self._pack  = list()  #stores field names in order fields must be read

    def add_field( self, field_name, field_format ):
        if field_name in self._table.keys():
            print "WARNING :: Field with name '%s' already exists in this structure" % field_name
        elif field_format == "":
            print "WARNING :: Format cannot be an empty string"
        else:
            self._table[ field_name ] = { "format" : field_format, "value" : None }
            self._pack.append( field_name )
        #maybe error handling is required here

    def get_value( self, field_name ):
        try:
            return self._table[ field_name ][ "value" ]
        except KeyError:
            print "WARNING :: There is no such field named '%s' in this structure" % field_name
            return None

    def get_format_string( self ):
        f_str = ""

        for key in self._pack:
            f_str += self._table[ key ][ "format" ]

        if (self._little_endian == True):
            f_str = "<" + f_str
        else:
            f_str = ">" + f_str

        return f_str

    def get_size( self ):
        return struct.calcsize( self.get_format_string() )

    def fill( self, file_ptr ):
        result = struct.Struct( self.get_format_string() ).unpack( file_ptr.read( self.get_size() ) )
        for (i,key) in enumerate(self._pack):
            self._table[ key ][ "value" ] = result[ i ]

    def show( self ):
        print "General info:"
        print "%20s = %s"  % ( "Format string", self.get_format_string() )
        print "%20s = %d" % ( "Size (bytes)", self.get_size() )
        print "Value:"
        for key in self._pack:
            print "%20s = " % key, self._table[ key ][ "value" ]

Usage

Usage is straightforward. I used the real binary produced by OWON oscilloscope here so some of the values might look strange (consist non-printable characters).

import structure

s = structure.structure( little_endian=True )
s.add_field( "header", "10s" )
s.add_field( "serial", "14s" )
s.add_field( "string", "18s" )
s.add_field( "trig_time", "f" )
s.add_field( "float1", "f" )
s.add_field( "float2", "f" )

with open( "./data/trigger.bin" ) as f:
    s.fill( f )

s.show()

Output

General info:
       Format string = <10s14s18sfff
        Size (bytes) = 54
Value:
              header =  SPBS01e�
              serial =  SDS60621715234
              string =
           trig_time =  99.0
              float1 =  3.41239206136e-06
              float2 =  0.001953125

As always any suggestions, ideas and critics are appreciated.

A major drawback of this design is that you cannot reuse a single instance of structure.structure to read multiple records.  Instead of s.fill(file_ptr), consider making a method s.read(f) that returns an OrderedDict of the deserialized data.  (By the way, mentioning "pointer" in a parameter name isn't very Pythonic.)  Note that that would also replace your .get_value() method with a dictionary lookup, which can be dealt with more idiomatically. (Or, at least implement dict-compatible iteration and subscripting in your structure class.)

Switching to a reusable .read() method would also eliminate a point of awkwardness in your code, which is that ._table is a dict of dicts, each having a "format" and a "value".

In your implementation of .fill(), why not call .unpack_from(file_ptr) instead of .unpack( file_ptr.read( self.get_size() ) )?

A more natural default endianness would be either big-endian (because that is the standard Network Byte Order) or host byte order (because that is a non-opinionated choice that also happens to be the path of least resistance).

Whatever you design, it should accommodate the opposite operation — serializing data to a file.

Consider allowing a structure to contain nested structures as fields.

The .show() method should probably be .__str__() instead.

I'm not convinced of the benefit of requiring multiple calls to .add_field() to set up the structure.  I'd prefer to have the constructor accept the field specification as a list of 2-tuples.  Not only would the calling code be cleaner, but the implementation of the structure class would be simpler as well if it is immutable.

I'm not a fan of your printed warnings.  Utility code, such as this, has absolutely no business contaminating sys.stdout, and ideally shouldn't print to sys.stderr either.  An end user would be completely clueless about what to do about these "warnings".  A programmer would also be puzzled about where these warnings came from (if it's a large program) and what actually happened as a consequence.  Rather, you should decide to either tolerate these situations gracefully or raise exceptions.  In my opinion…

"Field with name … already exists" should raise an ArgumentError.
"Format cannot be an empty string" should be tolerated (or perhaps an assertion failure).
"There is no such field" would be eliminated altogether based on my suggestion to return an OrderedDict.  If you had to offer a .get_value() method, then you should just let the KeyError propagate.


JavaScript (ES7) LOC counter I've been writing JavaScript for a couple years but nobody's ever really looked at my code.

This is a small library I wrote to check code modularity, w/ input like avg-lines . -name '*.js' (the arguments are passed straight to find) you might get:

Checked 20 files (1615 LOC):
  80% of files contain > 137 LOC
  20% of files contain 67.68% of the total LOC
  The largest file (./dist/redux.js) contains 638 LOC

index.js

import {exec} from 'child_process'

const i80 = arr => Math.ceil(arr.length * 0.8)
const sum = arr => arr.reduce((pv, [v]) => pv + v, 0)
const insertTypeOption = args =>
  /('?[-\|])/.test(args) ? args.replace(/('?[-\|])/, '-type f $&') : `${args} -type f`
const generateCommand = args => `find ${insertTypeOption(args)} | xargs wc -l`

export default args => new Promise((resolve, reject) => {
  const process = exec(generateCommand(args))
  const files = []
  const interval = setInterval(() => console.log(`checked ${files.length} files`), 3000)
  process.stderr.on('data', ::console.error)
  process.stdout.on('data', input =>
    files.push(
      ...input.split('\n').map(l =>
        l && l.match(/^ *(\d+) +(.+)$/)).filter(v => v).
        map(([, val, file]) => [parseInt(val, 10), file]).
        filter(([, filename]) => filename !== 'total')))
  process.on('close', exit => {
    clearInterval(interval)
    if (exit) reject(exit)
    files.sort(([v0], [v1]) => v0 - v1)
    if (!files.length) reject(`No files matched \`${generateCommand(args)}\``)
    else {
      resolve({
        numFiles: files.length,
        numLines: sum(files),
        avgLines: files[i80(files)][0],
        linesInequality: sum(files.slice(i80(files))) / sum(files),
        largestFileName: files.slice(-1)[0][1],
        largestFile: files.slice(-1)[0][0]})}})})

logger.js

import avgLines from './index'

const args = process.argv.slice(/node/.test(process.argv[0]) ? 2 : 1).map(v => `'${v}'`).join(' ')
const log = str => {
  let bold = false
  const newStr = str.split('**').reduce((pv, v) => (bold = !bold, `${pv}\x1b[${bold ? 1 : 0}m${v}`))
  console.log(newStr)}

export default avgLines(args).then(
  ({numFiles, numLines, avgLines: avgLines$, linesInequality, largestFileName, largestFile}) => (
    log(`Checked **${numFiles}** files (**${numLines}** LOC):`),
    log(`  **80%** of files contain > **${avgLines$}** LOC`),
    log(`  **20%** of files contain **${(linesInequality * 100).toFixed(2)}%** of the total LOC`),
    log(`  The largest file (**${largestFileName}**) contains **${largestFile}** LOC`))).
  catch(::console.error)

Ok, first, you've gone overboard. :D

You have a lot of regular expressions, but on first glance, I don't even know what they're for. What are they matching? It's best if you name them by putting them in const.

I see you have some personal touch on the code, like the ending parens being on the very last line. Makes the code look like CoffeeScript at first look. Anyways, code is for humans. Put some whitespace, break code into logically grouped lines, add some comments why you do things this way. One cannot really comprehend without tracing through (which is a sign of bad things to come).

Also, you're running this through Node, size doesn't matter there. If you run JS on a browser, there's what they call a minifier.

Also, variable names. Some make sense, some don't. l, v1, v0, pv, i80, they don't really tell me what they are. One terrible mistake devs often do is they assume the reader knows the context. Sure, pv means "previous value", but I only knew that because I know how reduce works. For someone that doesn't, that's a different story.

80% and 20% are hardcoded.


Ruby method that returns an age from a date of birth I have a method on my user model to calculate the user's age and return a human readable string.  My user's can be between 1 month old and above so the returned string is different depending on if the person is "2 months old" or "1 year old" or "2 years and 3 months old".

I have reviewed a few SO posts to come to this solution.  Is there anything I am missing?  Leap years?

def age
    dob = self.date_of_birth

    # if a date of birth is not nil
    if dob != nil

      # get current date
      now = Date.current

      # has person had their birthday yet this year
      had_birthday = ((now.month > dob.month || (now.month == dob.month && now.day >= dob.day)) ? true : false)

      # if yes then subtract this year from birthday year, if not then also subtract 1 to get how many full years old they are
      years = now.year - dob.year - (had_birthday ? 0 : 1)

      # get the calendar month difference from birthdya calendar month and today's calendar month.  if they have not had their birthdya yet then subtract the difference from 12
      months = had_birthday ? now.month - dob.month : 12 - (now.month - dob.month)

      # for under 1 year olds
      if years == 0
        return months > 1 ? months.to_s + " months old" : months.to_s + " month old"

      # for 1 year olds
      elsif years == 1
        return months > 1 ? years.to_s + " year and " + months.to_s + " months old" : years.to_s + " year and " + months.to_s + " month old"

      # for older than 1
      else
        return months > 1 ? years.to_s + " years and " + months.to_s + " months old" : years.to_s + " years and " + months.to_s + " month old"
      end

    # No date of birth saved so can not calculate age
    else
      return "No Date of Birth"
    end
  end

Make an additional function  to handle plural

In-lining the extra logic to handle the plural is messy, I suggest a small separate method:

def plural(word, value)
  "#{value.to_s} #{word}#{value > 1 ? 's' : ''}"
end

Reduce nesting by returning early

If you write

if dob == nil
  return "No Date of Birth"
end

You can reduce nesting in the rest of the function and highlight the handling of the special case.


Makefile which only forwards to subdirectories I have a Makefile, which should when calling make just call make in a list of subdirectories. The subdirectories should not be automatically detected but are fine to be hard coded (Tool1, Package2, Submodule3 below). Furthermore, if I call make Tool1, Tool1 should be built (i.e. the directory names should be recognised as targets).
At this top level I have no use for dependency checking, the Makefiles in the subdirectories take care of that. My current version looks like this:

subdirs := Tool1 Package2 Submodule3

.PHONY: all clean $(subdirs)

all:

clean:

define makesubdir
$(1):
  make -C $(1)

all: $(1)
endef

define makesubdirclean
$(1)-clean:
  make -C $(1) clean

clean: $(1)-clean
endef

$(foreach SUBDIR,$(subdirs),$(eval $(call makesubdir,$(SUBDIR))))
$(foreach SUBDIR,$(subdirs),$(eval $(call makesubdirclean,$(SUBDIR))))

What I already considered:

I don't want a for loop in the all target, to allow parallel building with make -j. An older version read like:

all:
   for DIR in $(subdirs); do make -C $$DIR ; done
The subdirectories should be defined only in one place, adding a new subdirectory should happen only on one place.
The subdirectories are .PHONY such that make gets called in them, regardless of the directory's timestamp.
I thought all: $(subdirs) would be easier to read than all: $(1) but that way only Tool1 got built.

It could be a matter of personal taste, but I use functions as the last resort. Consider

.PHONY: $(subdirs)

all: $(subdirs)
clean: $(subdirs)

$(subdirs):
    make -C $@ $(MAKECMDGOALS)


Better way to return data via AJAX? Let's say I have three files: a page that is used for searching data called search, a file that handles requests called controller, and a file that has functions which the controller calls based on what POST parameter is passed. From the search page, I post some data and specify my action (search) via AJAX to the controller. The controller then takes that and calls a function that returns the data (which is inserted into the appropriate place in the DOM) which looks something like this:

<?php
function fetchStudentData($search, $roster) {

    //If a user is not logged in or the query is empty, don't do anything
    if(!isLoggedIn() || empty($search)) {
        return;
    }
    global $db;

    //Placement roster search
    if($roster == "placement") {
        $query = $db->prepare("SELECT t1.id,
                t2.placement_date,
                t2.placement_room,
                t1.student_id,
                t1.student_type,
                t1.last_name,
                t1.first_name,
                t1.satr,
                t1.ptma,
                t1.ptwr,
                t1.ptre,
                t1.ptlg,
                t1.waiver,
                t2.tests_needed,
                t2.blackboard_id
                FROM student_data AS t1
                JOIN additional_data AS t2
                ON t1.id = t2.student_data_id
                WHERE t2.placement_date = :search
                ORDER BY t1.last_name ASC");
        $query->bindParam(":search", $search);
        $query->execute();

        if($query->rowCount() == 0) {
            return;
        }

        echo "

Placement Roster For $search

            Placement Room
            Student ID
            Student Type
            Last Name
            First Name
            SATR
            PTMA
            PTWR
            PTRE
            PTLG
            Waiver
            Tests Needed
            Blackboard ID

";
        while($row = $query->fetch()) {
            echo "

                {$row['placement_room']}
                {$row['student_id']}
                {$row['student_type']}
                {$row['last_name']}
                {$row['first_name']}
                {$row['satr']}
                {$row['ptma']}
                {$row['ptwr']}
                {$row['ptre']}
                {$row['ptlg']}
                {$row['waiver']}
                {$row['tests_needed']}
                {$row['blackboard_id']}

";
        }
    }
}
?>

This is obviously working very well for me but I wanted to know if there's a better way I should be doing this? Should I be returning the raw data from the database and encoded as JSON and then have my JavaScript take care of creating elements/structure? I see that a lot with various API's but this is something internal to my company and not something that other developers will be using. Is there anything wrong with my approach?

First of all, you should read about MVC.

There are a lot of tutorials out there, as well as frameworks (like CodeIgniter) that implement it. Basically your code should be divided into 3 main categories:

Controllers - where all logic should be placed like validations, checks. This is also where Views and Models are used, hence "controller"
Models - basically the source of data. Usually this is the abstraction of the data storage (databases, sessions, external requests) in the form of functions.
Views - the presentation, or in this case, the HTML.

Reasons to use MVC or MV* patterns is

Portability.

The files, especially the views and models, can be plucked out of the architecture, and be used elsewhere in the system without modification.

Models are simply function declarations one can just plug in the controller and call their functions. Views are simply layouts that, when plugged with the right values, display your data. They can be used and reused with little or no modification unlike with mixed code.
Purpose-written code and Readability

You would not want your HTML mixed with the logic, nor the data gathering mechanism. This goes for all other parts of your code. For example, when you edit your layout, all you want to see on your screen is your layout code, and nothing more.

Never, and I mean NEVER return marked-up data using AJAX (use JSON).

The main reason AJAX is used is because it's asynchronous. This means no page-loading, no more request-wait-respond, and you could do stuff in parallel. It's pseudo-multitasking for the human.

On the same train of thought, the reason why JSON was created was because XML was too heavy for data transfer and we needed a lightweight, data transport format. XML is NOT a data-transport format but a document and mark-up language (Though by "X" for extensibility, it has served a lot of purposes beyond document).

Use client-side templating

JavaScript engines have become fast these days that even developers create parsers/compilers on JavaScript. And with that comes client-side templating, like Mustache.

Coupled with raw data from JSON, which is very light, you can integrate that data into a template to form your view and have JavaScript display it, like you already do.

Have a rendered version and a data version

Taking into account the previous answer, if you are making a website rather than a web app, where JS is optional rather than required, one should create a website that works without JS.

However, for the same data but different format, the URL should not change (much). You should then take into account in the request what data type is requested by the client. And so you must accept a parameter which indicates the data type.

An example url would look like this:

//for a website page format (default: HTML):
http://example.com/my/shop/available/items.php

//for the JSON version
http://example.com/my/shop/available/items.php?format=JSON

//for the XML version
http://example.com/my/shop/available/items.php?format=XML

This means, that for the same page (available items), you have 3 formats you can use. This can be seen in the Joomla Framework (the framework that powers the Joomla CMS)

With these 3 tips, your code will be cleanly separated and purposely written (MVC), fast on transport and rendering (JSON + client-side templating) and extensible (multi-format support).


Creating users with different roles using .Net Identity 2.0 within Entity Framework I am using .Net Identity 2.0 with Entity Framework 6.0. I have a Person class inheriting IdentityUser. I have a Teacher (has additional Title property) and Student (has additional StudentNumber property) class inheriting from the Person class. I also have their corresponding roles in the database: Student and Instructor (or teacher).

I wonder if the code below is the most efficient way of creating users with specific roles, it seems to be a redundant but I could not figure a shorter way:

 gEchoLuDBContext _db = new gEchoLuDBContext();

        var userStore = new UserStore<IdentityUser>(_db);
        var userManager = new UserManager<IdentityUser>(userStore);

        foreach (ListItem role in rb_Roles.Items)
        {
            if (role.Selected)
            {
                var user = new Person()
                {
                    UserName = txt_Username.Text,
                    FirstName = txt_Firstname.Text,
                    LastName = txt_Lastname.Text,
                    Email = txt_Email.Text
                };

                if (role.Text == "Student")
                {
                    user = new Student()
                    {
                        StudentNumber = " ",
                        UserName = txt_Username.Text,
                        FirstName = txt_Firstname.Text,
                        LastName = txt_Lastname.Text,
                        Email = txt_Email.Text
                    };
                }
                else if (role.Text == "Instructor")
                {
                    user = new Teacher()
                    {
                        Title = "",
                        UserName = txt_Username.Text,
                        FirstName = txt_Firstname.Text,
                        LastName = txt_Lastname.Text,
                        Email = txt_Email.Text
                    };
                }

                IdentityResult result = userManager.Create(user, txt_Password.Text);
                if (result.Succeeded)
                {
                    IdentityResult result2 = userManager.AddToRole(user.Id, role.Text);
                    if (!result2.Succeeded)
                    {
                        lbl_Result.Text = "The user created successfully. But, the selected roles are not assigned!";
                    }
                }
            }
        }

Well, you say that both Student and Teacher inherit from the base class Person. Considering that, and the fact that you never actually use either the Student or Teacher specifically in this code, why bother checking the Role at all? Your UserManager<IdentityUser> is taking in a person, not a Teacher or a Student.

Now, if I would happen to be wrong about that, I would suggest looking into the Factory Pattern. You could create a factory that takes in all of the fields from your GUI as arguments, and decides which type of Person to create based on the arguments it's recieved. It won't do away with all of the if...else if logic, but it will abstract it away and hide it from your GUI and make your code more testable.


Solution to LeetCode Next Greater Element Ⅰ in Python This is my solution to LeetCode's "Next Greater Element Ⅰ":

You are given two arrays (without duplicates) nums1 and nums2 where nums1’s elements are subset of nums2. Find all the next greater numbers for nums1's elements in the corresponding places of nums2.
The Next Greater Number of a number x in nums1 is the first greater number to its right in nums2. If it does not exist, output -1 for this number.

class Solution:

    def nextGreaterElement(self, nums1: List[int], nums2: List[int]) -> List[int]:
        need, ready, next_greater = set(nums1), set(), {}
        for n in nums2:
            for k in ready:
                if k < n:
                    next_greater[k] = n
            ready = {k for k in ready if k not in next_greater}
            if n in need and n not in next_greater and n not in ready:
                ready.add(n)
        return [next_greater[k] if k in next_greater else -1 for k in nums1]

The nextGreaterElement method accepts 2 lists. Neither list contains duplicates, and nums1 is a subset of nums2. For every num in nums1, it will output the first number greater than num positioned to the right of num in nums2, or -1 is none is found. e.g.:
Input: nums1 = [4,1,2], nums2 = [1,3,4,2].
Output: [-1,3,-1]

I loop through the keys in ready, and the body of the loop does nothing unless k < n. len(ready) can be very large relative to the number of values that satisfy k < n. This seems like a very common thing so I'm wondering if there's a more explicit/pythonic way to write this?
Or should I be using a different data structure entirely?

Since list one is a subset of list two, iterating over two lists can be avoided. This can be achieved by finding the next greater element of each element for the bigger list and storing the result in a hash table.

To calculate the next greater element of each element in a list we can iterate the list. For each element, we can find the next greater element by again iterating the list. This approach is simple but will have O(n^2) complexity.

Another approach is by using stack.

First, push the first element of the list int the stack.
Now iterate the list and compare the top element of the stack with the current element of the list.
If the current element is greater than the top element then that means the current element is the next greater element of the top element of the stack. Store this result in a dictionary. Pop the top element out of the stack.
Repeat above two steps until the stack top element is greater than the current element or stack is empty.
At the end of each iteration push the current element in the stack.

The code will make things clearer:

def calculate_nge_dict(arr):

    nge_dict = {}
    stack_ = []
    stack_.append(arr[0])
    nge_dict[arr[0]] = -1
    for i in arr[1:]:
        next_ = i
        nge_dict[next_] = -1
        while stack_:
            top_ = stack_.pop()
            if next_ <= top_:
                stack_.append(top_)
                break
            else:
                nge_dict[top_] = next_
        stack_.append(next_)
    return nge_dict

def next_greater_element(nums1, nums2):
    nge_dict = calculate_nge_dict(nums2)
    return [nge_dict[i] for i in nums1]

You can get a better understanding of the algorithm from this answer.


Refactor Linq grouping All of the class here share these same property

Country, Total and Date.

Total property are integer in some classes and decimal in some classed.

The extension method below has a lot of duplication. i have tried to refactor it using interface but i can't find a way to refactor it at the last step on selection since it need different class

    public static class TotalApprovedPOAmountStatisticExtension
{
    public static void MergeNullAndEmplyCountry(this IEnumerable<TotalApprovedPOAmountStatistic> totalUserStatistic)
    {
        foreach (var item in totalUserStatistic)
        {
            if (string.IsNullOrWhiteSpace(item.Country))
            {
                item.Country = "";
            }
        }

        totalUserStatistic = totalUserStatistic
                            .GroupBy(item => new { item.Country, item.Date })
                            .Select(groupedUser => new TotalApprovedPOAmountStatistic()
                            { Country = groupedUser.Key.Country, Total = groupedUser.Sum(item => item.Total), Date = groupedUser.Key.Date }).ToList();

    }
}

    public static class TotalPOStatisticListExtension
{
    public static void MergeNullAndEmplyCountry(this IEnumerable<TotalPOStatistic> totalUserStatistic)
    {
        foreach (var item in totalUserStatistic)
        {
            if (string.IsNullOrWhiteSpace(item.Country))
            {
                item.Country = "";
            }
        }

        totalUserStatistic = totalUserStatistic
                            .GroupBy(item => new { item.Country, item.Date })
                            .Select(groupedUser => new TotalPOStatistic() { Country = groupedUser.Key.Country, Total = groupedUser.Sum(item => item.Total), Date = groupedUser.Key.Date }).ToList();

    }
}

    public static class TotalProjectAmountStatisticListExtension
{
    public static void MergeNullAndEmplyCountry(this IEnumerable<TotalProjectAmountStatistic> totalUserStatistic)
    {
        foreach (var item in totalUserStatistic)
        {
            if (string.IsNullOrWhiteSpace(item.Country))
            {
                item.Country = "";
            }
        }

        totalUserStatistic = totalUserStatistic
                            .GroupBy(item => new { item.Country, item.Date })
                            .Select(groupedUser => new TotalProjectAmountStatistic() { Country = groupedUser.Key.Country, Total = groupedUser.Sum(item => item.Total), Date = groupedUser.Key.Date }).ToList();

    }
}

    public static class TotalProjectStatisticListExtension
{
    public static void MergeNullAndEmplyCountry(this IEnumerable<TotalProjectStatistic> totalUserStatistic)
    {
        foreach (var item in totalUserStatistic)
        {
            if (string.IsNullOrWhiteSpace(item.Country))
            {
                item.Country = "";
            }
        }

        totalUserStatistic = totalUserStatistic
                            .GroupBy(item => new { item.Country, item.Date })
                            .Select(groupedUser => new TotalProjectStatistic() { Country = groupedUser.Key.Country, Total = groupedUser.Sum(item => item.Total), Date = groupedUser.Key.Date }).ToList();

    }
}

    public static class TotalUserStatisticListExtension
{
    public static void MergeNullAndEmplyCountry(this IEnumerable<TotalUserStatistic> totalUserStatistic)
    {
        foreach (var item in totalUserStatistic)
        {
            if (string.IsNullOrWhiteSpace(item.Country))
            {
                item.Country = "";
            }
        }

        totalUserStatistic = totalUserStatistic
                            .GroupBy(item => new { item.Country, item.Date })
                            .Select(groupedUser => new TotalUserStatistic() { Country = groupedUser.Key.Country, Total = groupedUser.Sum(item => item.Total), Date = groupedUser.Key.Date }).ToList();

    }
}

You can use an interface and a generic method to remove the code duplication:

public interface IStatistic
{
    string Country {get; set;}
    DateTime Date {get; set;}
    Decimal Total {get; set;}
}

public static class StatisticExtension
{
    public static void MergeNullAndEmplyCountry<T>(this IEnumerable<T> totalUserStatistic) where T : IStatistic, new()
    {
        foreach (var item in totalUserStatistic)
            if (string.IsNullOrWhiteSpace(item.Country))
                item.Country = "";

        totalUserStatistic = totalUserStatistic.GroupBy(item => new { item.Country, item.Date })
                                                .Select(groupedUser => new T()
                                                {
                                                    Country = groupedUser.Key.Country,
                                                    Total = groupedUser.Sum(item => item.Total),
                                                    Date = groupedUser.Key.Date
                                                }).ToList();
    }
}

Important here is the new() constraint, which allows us to create an instance of T.

If the Total property of a class is int instead of decimal, we can make use of an explicit interface implementation:

public class BlaStatistic : IStatistic
{
    public string Country {get; set;}
    public DateTime Date {get; set;}
    public Int32 Total {get; set;}
    Decimal IStatistic.Total
    {
        get { return Convert.ToDecimal(Total); }
        set { Total = Convert.ToInt32(value); }
    }
}

Also, your extension method should actually return something, because it won't change the IEnumerable<...> you pass in:

public static IEnumerable<T> MergeNullAndEmplyCountry<T>(this IEnumerable<T> totalUserStatistic) where T : IStatistic, new()
{
    ...
    return totalUserStatistic.GroupBy...
}


Find the next 7 o'clock NSDate In my very specific scenario, I need to get an NSDate holding the next time the hour will be 7 (either AM or PM).

On the case that this is the first time to do this, I need to jump extra 12 hours (e.g., if I got Apr 1st 7AM - to jump it to 7PM).

After playing with playgrounds I was able to achieve what I believe solves this, but looking at my code it feels too hard-coded and full of magic numbers. I am wondering if there is a cleaner way to do this.

//constants
NSInteger secInHour = 60 * 60;
NSInteger secInMin = 60;
NSInteger NO_VALUE = NSIntegerMax;

//Break "now" to components
NSCalendar *cal = [[NSCalendar alloc] initWithCalendarIdentifier:NSCalendarIdentifierGregorian];
cal.locale = [NSLocale localeWithLocaleIdentifier:@"he_IL"]; //force a 24hr locale - easier to calculate
NSInteger nowHr = [cal component:NSCalendarUnitHour fromDate:date];
NSInteger nowMin = [cal component:NSCalendarUnitMinute fromDate:date];
NSInteger nowSec = [cal component:NSCalendarUnitSecond fromDate:date];
NSTimeInterval nowMS = date.timeIntervalSince1970 - (double)((int)date.timeIntervalSince1970);

//Get closest "7" hour
BOOL isFirstTime = YES;
NSInteger firstTimeDelta = isFirstTime ? 12 : 0;

//find hour distance
NSInteger addHr = NO_VALUE;
BOOL alarmToConcludeDay = true;
if (nowHr >= 7 && nowHr < 19) {
    //now is day
    alarmToConcludeDay = true;
    addHr = 19 - nowHr - 1;
} else {
    //now is night
    alarmToConcludeDay = false;
    if (nowHr < 7) {
        addHr = 7 - nowHr - 1;
    } else {
        addHr = 7 - nowHr - 1 + 24;
    }
}

//sum everything
if (addHr != NO_VALUE) {
    //how much to add to now?
    NSInteger addMin = 60 - nowMin - 1;
    NSInteger addSec = 60 - nowSec - 1;
    NSInteger addMS = 1 - nowMS;

    NSTimeInterval addTotal = (double)((addHr + firstTimeDelta) * secInHour + addMin * secInMin + addSec) + addMS;

    NSDate *alarmDate = [date dateByAddingTimeInterval:addTotal];
}

There's a much easier approach to this problem.

We can use NSCalendar's nextDateAfterDate:matchingComponents:options: method to find both our next 7am and 7pm.  Then it's simply a matter of comparing these values and returning the lesser of the two.

NSDate * nextSevenOClock() {
    NSCalendar *calendar = [NSCalendar currentCalendar];
    NSDateComponents *components = [[NSDateComponents alloc] init];

    components.hour = 7;
    NSDate *nextSevenAm = [calendar nextDateAfterDate:[NSDate date]
                                   matchingComponents:components
                                              options:NSCalendarMatchNextTimePreservingSmallerUnits];

    components.hour = 19;
    NSDate *nextSevenPm = [calendar nextDateAfterDate:[NSDate date]
                                   matchingComponents:components
                                              options:NSCalendarMatchNextTimePreservingSmallerUnits];

    NSDate *nextSeven = nextSevenAm;
    if ([nextSeven compare:nextSevenPm] == NSOrderedDescending) {
        nextSeven = nextSevenPm;
    }

    return nextSeven;
}

Importantly, this approach with account correctly for things like daylight savings time and leap seconds, which the original solution does not.

This method is very specific however.  It would be good if we could get a bit more utility out of it.  And rather than a simple C-style function, I've expanded it properly into an NSDate category.

@implementation NSDate (NextTimeStamp)

+ (instancetype)nextTimeStampMatchingHour:(NSUInteger)hour {
    return [self nextTimeStampMatchingHour:hour minute:0 second:0];
}

+ (instancetype)nextTimeStampMatchingHour:(NSUInteger)hour minute:(NSUInteger)minute {
    return [self nextTimeStampMatchingHour:hour minute:minute second:0];
}

+ (instancetype)nextTimeStampMatchingHour:(NSUInteger)hour minute:(NSUInteger)minute second:(NSUInteger)second {
    NSUInteger morningHour = hour % 12;
    NSUInteger eveningHour = morningHour + 12;

    NSCalendar *calendar = [NSCalendar currentCalendar];
    NSDateComponents *components = [[NSDateComponents alloc] init];

    components.minute = minute;
    components.second = second;

    components.hour = morningHour;
    NSDate *nextAM = [calendar nextDateAfterDate:[NSDate date]
                              matchingComponents:components
                                         options:NSCalendarMatchNextTimePreservingSmallerUnits];

    components.hour = eveningHour;
    NSDate *nextPM = [calendar nextDateAfterDate:[NSDate date]
                              matchingComponents:components
                                         options:NSCalendarMatchNextTimePreservingSmallerUnits];

    return [nextAM compare:nextPM] == NSOrderedAscending ? nextAM : nextPM;
}

@end

For your original exxample of looking for the next 7 o'Clock, we can just do simply:

NSDate *nextSeven = [NSDate nextTimeStampMatchingHour:7];

But with these extra methods, we can look for more specific times:

NSDate *nextSevenFifteen = [NSDate nextTimeStampMatchingHour:7
                                                      minute:15];


Turkish word filtering I need to filter Turkish words according to vovel alternation rules. For example -ler is plural but can have form of -lar thus I use metaphonemes for these transitive allomorphs as vovel_dict in my algorithm.

There is only one condition not to alternate vovel it's 'ken' which excluded. I wrote this but it takes too much runtime do you have any suggestions?

def char_filter(morph):
    char_dict = {'d': 'D', 't': 'D', 'a': 'A', 'e': 'A', 'ı': 'H', 'i': 'H', 'u': 'H', 'C': 'C', 'g': 'G', 'k': 'G'}
    res = []
    res1 = []
    flag = []

    if 'ken' in morph:
        flag.append([morph.rindex('ken'), ['k','e','n']])

    for i in morph:
        res2 = i
        if i in char_dict:
            res2 = i.upper()
        res1.append(res2)

    if len(flag) > 0:
        for z in flag:
            res1[z[0]:z[0]+3] = z[1]
    res.append(string.join(res1, sep=''))
    return res[0]

Naming

res, res1, res2? These names convey absolutely no meaning. flag? Sounds like some sort of boolean value to me; but you assign it a list. i, z? one-letter variable names are essentially used for integer indices, not characters or… what is z already… oh, a list.

Even morph sounds badly chosen, I’m laking a bit of context here to fully understand it, I guess, but I would have used origin_string or something alike.

Control flow

if len(flag) > 0:
    for z in flag:
        # do something

is twice as much complicate than it should be. Once because if len(flag) > 0 can be written if flag: empty container are considered False in a boolean context. And once again because the if is not needed: for z in flag will be a no-op if flag is an empty container.

Also, since you’re not modifying morph and the only thing you store in flag (if any) is [morph.rindex('ken'), ['k','e','n']], why not get rid of flag and directly use that list in the last part of your computation?

def char_filter(morph):
    char_dict = {'d': 'D', 't': 'D', 'a': 'A', 'e': 'A', 'ı': 'H', 'i': 'H', 'u': 'H', 'C': 'C', 'g': 'G', 'k': 'G'}
    res = []
    res1 = []

    for i in morph:
        res2 = i
        if i in char_dict:
            res2 = i.upper()
        res1.append(res2)

    if 'ken' in morph:
        i = morph.rindex('ken')
        res1[i:i+3] = ['k','e','n']
    res.append(string.join(res1, sep=''))
    return res[0]

Data structures

Most of the data structures you use are not suited to your needs. Why storing only one string as the first item of res if you extract it right after having assigned it. Store the string directly in res intead of res[0]. Better: do not use res and return the string right after building it, you’re not making any other use of res anyway:

return ''.join(res1)

Also note the prefered syntax for join which is an operator of the separator string.

You’re also building a dictionary in char_dict but never using the values stored into it, only checking for the existence of keys. Two possibilities:

either you have a bug and need to use res2 = char_dict[i] instead of res2 = i.upper();
or you just need to simplify the data structure and only store the characters that you want to test against. A list, a string or a set are better fit for this task.

I will consider the second option.

The last thing to note is that this collection will not be changed between calls so we can safely define it outside of the function to avoid building a new one each time we call char_filter.

A little bit on pythonic constructs

some_list = []
for variable in some_collection:
    value = some_function(variable)
    some_list.append(value)

Is better written with a list-comprehension: more readable and faster.

some_list = [some_function(variable) for variable in some_collection]

Proposed improvements

SPECIAL_CHARS = 'dtaeıiuCgk'

def char_filter(origin_string):
    filtered_letters = [char.upper() if char in SPECIAL_CHARS else char
                        for char in origin_string]

    if 'ken' in origin_string:
        i = origin_string.rindex('ken')
        filtered_letters[i:i+3] = ['k','e','n']

    return ''.join(filtered_letters)


Context managers that also deal with sys.stdin and sys.stdout It's pretty typical for Linux command-line utilities that deal with files to accept either a file as input, or stdin. It's also pretty common for them to be able to output to a file or to stdout.

These should be supported:

python myprogram.py input output
cat input | python myprogram.py > output
python myprogram.py input > output

Additionally, I don't like editing files in-place. I'd rather make a tmpfile and then only copy over the tmpfile if the operation is a success, but I'd also rather not deal with tmpfiles every time I write something that has an output file. So both

python myprogram --in-place input
python myprogram filename filename (ie, input is the same as output)

should use a tmpfile.

I wrote this pair of context managers to make this type of interface a little easier to write. They're meant to be used something like this:

with infile(infile_filename) as f:
     for line in f: #do some stuff
with outfile(outfile_filename, infile_name=infile_filename, inplace=inplace) as f:
#get values for these arguments from argparse.
     f.write('blah')

I would love to know what y'all think of it.

#!/usr/bin/python3
import os
import shutil
import sys
import tempfile

class infile(object):
    def __init__(self, file_name=None):
        self.file_name = file_name
    def __enter__(self):
        if self.file_name is None:
             self.f = sys.stdin
        else:
             self.f = open(self.file_name)
        return self.f
    def __exit__(self, etype, value, traceback):
        if self.f is not sys.stdin:
            self.f.close()
    def __getattr__(self, val):
        return getattr(self.f, val) # pass on other attributes to the underlying filelike object

class outfile(object):
    def __init__(self, file_name=None, *, infile_name=None, inplace=False):
        self.file_name = file_name
        self.infile_name = infile_name
        self.inplace = inplace
    def __enter__(self):
        if self.inplace or (self.file_name and self.file_name == self.infile_name):
            self.f = tempfile.NamedTemporaryFile(mode='w+t', delete=False)
            self.tmppath = self.f.name
        elif self.file_name is None:
            self.f = sys.stdout
            self.tmppath = None
        else:
            self.f = open(self.file_name, 'w')
            self.tmppath = None
        return self.f
    def __exit__(self, etype, value, traceback):
        # If got no errors...
        if etype is None and self.tmppath:
            self.f.flush()
            shutil.copy(self.tmppath, self.infile_name)
        if self.f is not sys.stdout:
            self.f.close()
        if self.tmppath:
            os.remove(self.tmppath)
    def __getattr__(self, val):
        return getattr(self.f, val)

All the files you're dealing with are already context managers, including sys.stdin and sys.stdout. Delegating to them instead of reinventing the wheel, your infile class becomes a trivial function:

def infile(filename=None):
    if filename is None:
        return sys.stdin
    return open(filename)

outfile isn't quite as easy as that, but we can still simplify it a little. First, the inplace flag completely changes the behaviour of the function (causes it to choose between mostly disjoint codepaths), and would usually be specified as a literal in the source. It makes more sense to have it as a separate function instead. If you do need to decide between them based on user input, you can always write a short wrapper function.

I'll reuse the name outfile for the clobbering version, and use atomic_update for the non-clobbering version. The clobbering version is then really just as simple as infile:

def outfile(filename=None):
    if filename is None:
        return sys.stdout
    return open(filename, 'w')

The updating version really does need to do some extra stuff after it the files get closed. So it does need to be it's own context manager. But instead of writing it as a class, it's a little easier to use the stdlib contextlib to write it as a coroutine:

from contextlib import contextmanager

@contextmanager
def atomic_update(filename):
    if filename is None:
        f = sys.stdout
    else:
        f = tempfile.NamedTemporaryFile(mode='w+t', delete=False)
        tmppath = f.name

    with f:
        yield f

    if filename is not None:
        shutil.copy(tmppath, filename)
        os.remove(tmppath)

nb. It would be nice if we just return sys.stdout like in the other cases, but the contextmanger decorator considers it an error if the generator doesn't yield exactly once.

As it is, this is a little bit cumbersome to use effectively - you need to open the file for input separately (the tempfile is returned empty), and then it's probably a good idea to be careful to close them in the right order. It also loses, eg, file permissions, which isn't ideal. You might want to copy the original file (and metadata) into the temp file, and then shutil.move it back when you're done.


Extract numbers from a string-Javascript Consider the following string in javascript:

var string="border-radius:90px 20px 30px 40px";

I want to extract the 4 numbers from that string and store them in an array called numbers.To do that I developed the following code:

var numbers=string.split("border-radius:");
numbers=numbers[1].split("px");

This code is working fine but I was wondering if there is a better approach/solution.Any ideas are welcomed

I used the following regexp to extract numbers from a string:

var string = "border-radius:10px 20px 30px 40px";
var numbers = string.match(/\d+/g).map(Number);


Who are these lurkers? Last night in The 2nd Monitor there was a discussion about postless users - I wanted to see how many there are, whether they're one-timers that showed up once, registered and never came back, and whether they vote at all.

So I whipped up this query on SEDE, showing voters and their post count, as well as other information:

with sub as (

  select distinct
    Users.Id
   ,Users.DisplayName
   ,Users.CreationDate
   ,Users.LastAccessDate
   ,datediff(day, cast(Users.CreationDate as date), cast(Users.LastAccessDate as date)) MembershipDays
   ,case when cast(Users.CreationDate as date) = cast(Users.LastAccessDate as date) then 1 else 0 end IsOneTimer
   ,sum(case when Posts.Id is null then 0 else 1 end) over (partition by Users.Id) PostCount
   ,Users.Reputation
   ,Users.UpVotes + Users.DownVotes TotalVotes
   ,Users.UpVotes
   ,Users.DownVotes
  from Users
  left join Posts on Users.Id = Posts.OwnerUserId
  where Users.Id > 0

), agg as (

  select
     Id
    ,DisplayName
    ,CreationDate
    ,LastAccessDate
    ,IsOneTimer
    ,MembershipDays
    ,case when MembershipDays = 0 then TotalVotes else TotalVotes / cast(MembershipDays as decimal) end VotesPerDay
    ,PostCount
    ,Reputation
    ,TotalVotes
    ,UpVotes
    ,DownVotes
  from sub
  where TotalVotes > 0)

select
     Id
    ,DisplayName
    ,CreationDate
    ,LastAccessDate
    ,IsOneTimer
    ,MembershipDays
    ,round(VotesPerDay, 3) VotesPerDay
    ,PostCount
    ,Reputation
    ,TotalVotes
    ,UpVotes
    ,DownVotes
from agg
order by
  PostCount
 ,cast(VotesPerDay as int) desc
 ,cast(LastAccessDate as date) desc
 ,TotalVotes desc
 ,Reputation
 ,IsOneTimer
 ,CreationDate

Returns 26740 rows returned in 211 ms (cached). Is there any way it could be improved, performance or otherwise?

Postless Users query on SEDE.

Output

It would be nice if the usernames were clickable.  You can achieve that using the magic [User Link] column.  It might make the query fail to complete on a large site like Stack Overflow, though.

It's weird that you cast(VotesPerDay as int) and cast(LastAccessDate as date) for sorting.  Why not sort by VotesPerDay (without rounding) and LastAccessDate directly?

Implementation

The names of your CTEs, sub and agg, don't mean anything to me.

select distinct should be replaced with a smarter join wherever possible.  If you look at the execution plan, you'll see that it estimates that there are 3 million rows, when there are actually just 26740 Code Review users.  The estimated subtree cost is 1048.  In comparison, my query below has an estimated subtree cost of 5.7 and 27580 rows for Code Review, and those numbers scale depending on the size of the site.

Your agg CTE has a TotalVotes > 0 as a WHERE condition.  It would probably be more efficient to test for (UpVotes > 0 OR DownVotes > 0), because that has a possibility to take advantage of an index (though we don't know whether the Users table actually has such an index in SEDE).

Suggested solution

WITH FilteredUsers AS (
    SELECT *
         , DATEDIFF(day, CAST(CreationDate AS DATE), CAST(LastAccessDate AS DATE)) AS MembershipDays
        FROM Users
        WHERE
            Id > 0 AND
            (UpVotes > 0 OR DownVotes > 0)
), PostCounts AS (
    SELECT OwnerUserId
         , COUNT(*) AS PostCount
        FROM Posts
        GROUP BY OwnerUserId
)
SELECT Id
     , DisplayName
     , CreationDate
     , LastAccessDate
     , CASE WHEN MembershipDays = 0 THEN 1 ELSE 0 END AS IsOneTimer
     , MembershipDays
     , ROUND((UpVotes + DownVotes) / CASE WHEN MembershipDays = 0 THEN 1 ELSE CAST(MembershipDays AS DECIMAL) END, 3) AS VotesPerDay
     , COALESCE(PostCount, 0) AS PostCount
     , Reputation
     , UpVotes + DownVotes AS TotalVotes
     , UpVotes
     , DownVotes
    FROM FilteredUsers
        LEFT OUTER JOIN PostCounts
            ON FilteredUsers.Id = PostCounts.OwnerUserId
    ORDER BY PostCount
           , VotesPerDay DESC
           , LastAccessDate DESC
           , TotalVotes DESC
           , Reputation
           , IsOneTimer
           , CreationDate;


Beginner programmer on sending error messages I am looking to optimize one function that returns error messages to the user.

    /// <summary>
    /// Handles the Ouverture event of the PrescriptionVue control.
    /// </summary>
    /// <param name="sender">The source of the event.</param>
    /// <param name="e">The <see cref="System.EventArgs"/> instance containing the event data.</param>
    private void PrescriptionVueOuverture(object sender, EventArgs e)
    {
        Message message = ServiceCouchePolygoneFacade.Instance.RecalculerSuperficiePolygone();
        Message message2 = ServicePrescriptionFacade.Instance.MettreAJourTablePrescription();
        if (message != null || message2 != null)
        {
            if (message != null)
            {
                this.prescriptionVue.AfficherMessage(message);
            }
            else
            {
                this.prescriptionVue.AfficherMessage(message2);
            }

            this.prescriptionVue.Fermer();
        }

        this.InitialiserVue();
    }

I have 2 service that conduct different manipulation and both of them returns an Object "Message" that are either gonna bu null or contain the error.

What im looking to optimize is the condition block in wich i look if either of those are null and if one isnt, wich one, so that i can send the one that contains the error.

one way to do it is to use coalesce :

/// <summary>
/// Handles the Ouverture event of the PrescriptionVue control.
/// </summary>
/// <param name="sender">The source of the event.</param>
/// <param name="e">The <see cref="System.EventArgs"/> instance containing the event data.</param>
private void PrescriptionVueOuverture(object sender, EventArgs e)
{
    var message = ServiceCouchePolygoneFacade.Instance.RecalculerSuperficiePolygone() ?? ServicePrescriptionFacade.Instance.MettreAJourTablePrescription();

    if(message != null)
    {
        this.prescriptionVue.AfficherMessage(message);

        this.prescriptionVue.Fermer();
    }

    this.InitialiserVue();
}

the coalesce would evaluate the left-hand operand, if it's null, then it'll get the right-hand operand.


Java Swing Exercise: BMI Calculator This is my first question here and this is my working Body Mass Index Swing Calculator.
Any improvement or recommendation is welcome.

// Metric units
// Weight in kg
// Height in meters

import javax.swing.*;
import java.awt.*;

public class MainProgram {

    public static void calculateIMC(DataPanel in){
        String salida = in.weight.getText();
        System.out.print(salida);
    }

    public static void main(String[] args) {

        JFrame frame = new JFrame("BMI");

        DataPanel in = new DataPanel();

        frame.setVisible(true);
        frame.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
        frame.setBounds(150,150,500,400);
        frame.setLayout(new GridLayout(0,2));
//        frame.pack();
        frame.add(in);
        System.out.println(Grade.getGrade(33.4));
    }
}

This is an enum example

public enum Grade {
    INS("Insuficient Weight",18.5),
    NRM("Normal Wight",24.9),
    SBR1("Overweight Grade 1",26.9),
    SBR2("Overweight Grade 2: Probesity",29.9),
    OBS1("Obesity Grade 1",34.9),
    OBS2("Obesity Grade 2",3.9),
    OBS3("Obesity Grade 3: Morbid",49.9),
    OBS4("Obesity Grade 4: Extreme",50);

    private final String title;
    private final double lim;

    public String getTitle() {
        return title;
    }

    public double getLim() {
        return lim;
    }

    Grade(String s, double i) {
        title = s;
        lim = i;
    }

    public static String getGrade(double value){
        for (int i=0; i < Grade.values().length;i++){
            if(value > Grade.OBS4.getLim()){
                return Grade.OBS4.getTitle();
            } else if(value < Grade.INS.getLim()){
                return Grade.INS.getTitle();
            } else if(value > Grade.values()[i].getLim() && value < Grade.values()[i+1].getLim()){
                return Grade.values()[i+1].getTitle();
            }
        }
        return null;
    }
}

And the Panel

import javax.swing.*;
import java.awt.*;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;

public class DataPanel extends JPanel {
    JTextField weight = new JTextField(10);
    JTextField height = new JTextField(10);
    JButton calculate = new JButton("Calculate");

    DataPanel(){
        initialize();
    }

    private void initialize() {
        add(new JLabel("Weight: "));
        add(weight);
        add(new JLabel("Height: "));
        add(height);
        add(calculate);

        calculate.addActionListener(new ActionListener() {
            @Override
            public void actionPerformed(ActionEvent e) {
                double w=Double.parseDouble(weight.getText());
                double h=Double.parseDouble(height.getText());
                double bmi=w/(h*h);
                JOptionPane.showMessageDialog(null,"Your BMI is: "+String.format("%.2f",bmi));
                JOptionPane.showMessageDialog(null,"Your Grade is: "+Grade.getGrade(bmi));

            }
        });

        setVisible(true);
        setBorder(BorderFactory.createTitledBorder("BMI"));
//        setBounds();
//        setLayout(new GridLayout(0,2));
        setMaximumSize(new Dimension(30,100));
    }

}

for (int i=0; i < Grade.values().length;i++){
            if(value > Grade.OBS4.getLim()){
                return Grade.OBS4.getTitle();
            } else if(value < Grade.INS.getLim()){
                return Grade.INS.getTitle();
            } else if(value > Grade.values()[i].getLim() && value < Grade.values()[i+1].getLim()){
                return Grade.values()[i+1].getTitle();
            }
        }

You don't need the first two conditions in the for loop.  You can instead say

        if (value > Grade.OBS4.getLim()) {
            return Grade.OBS4.getTitle();
        }

        if (value < Grade.INS.getLim()) {
            return Grade.INS.getTitle();
        }

        for (int i = 1; i < Grade.values().length; i++) {
            if (value > Grade.values()[i - 1].getLim() && value < Grade.values()[i].getLim()) {
                return Grade.values()[i].getTitle();
            }
        }

This is because those two conditions are invariant relative to the loop.  They'll either be true the first time or they will never be true.  Since you are returning, an else is unnecessary (although harmless if you prefer it).

We don't need to check i equal to 0, as there is no -1 grade.  We instead just check the insufficient grade before the loop.

If the BMI grade is exactly equal to the border marker, you'll fall through to the null case.  You should make one of those inequalities "or equal to", i.e. >= or <=.  Which one depends on the exact rules, which you don't include in your question.  I'm going to choose >=.

        for (Grade grade : Grade.values()) {
            if (value < grade.getLim()) {
                return grade.getTitle();
            }
        }

        return Grade.OBS4.getTitle();

You may need to change your limits to match.

Now we don't do any checks outside the loop.  And we don't need to compare across grades.  We know it's greater than or equal to previous grades, as if it were less than, we would already have returned.

If we fall through the loop, we know that the value represents extreme obesity.

I've added some additional whitespace for readability.

I would prefer to replace Lim with Limit for readability.

You can also use a NavigableMap here.  See here for an example on a similar problem.


Neighbours from point connections I am working with a mesh of triangles (in 3D, although I doubt it makes a difference). The mesh is given as list of lists, each list containing the indices of the three vertices of a triangle in said mesh.

For example:

0 2 6
0 12 15
2 66 10234

I want to find the neighbours of each point, i.e., to have an array where the first row contains the neighbours of point 0, the second the neighbours of point 1 and so on.

How do I make it more efficient?

points is a list of lists containing the point coordinates, while triangles is a list of lists containing the triangles in the mesh:

from numpy import ma

pointsNb = points.shape[0]

trianglesNb = triangles.shape[0]

neighbours = [[] for i in range(pointsNb)] # for each point in points, make a list of its neighbours

# find neighbours
for i in range(0, trianglesNb):
    triangle = ma.array(triangles[i][:], mask=False)
    for j, point in enumerate(triangles[i]):
        triangle[j] = ma.masked
        for possible_neighbour in triangle[~triangle.mask]:
            if possible_neighbour not in neighbours[point]:
                neighbours[point].append(possible_neighbour)
        triangle[j] = point

There's no documentation. What is this code supposed to do? How am I supposed to call it?
There's no functional decomposition. Code is easier to understand and test if it's organized into functions with well-defined inputs and outputs.
There's no inherent order to the neighbours of a point, so the natural representation of the neighbours is a set, not a list. Also, if you used a set, then you'd have a more efficient membership test, and you could update it with new points without having to check whether they are already present.

So I would write:

from collections import defaultdict

def find_neighbours(triangles):
    """Given an array of triangles (triples of points), return a graph in
    adjacency representation: that is, a map from point to the set of
    its neighbours.

    """
    graph = defaultdict(set)
    for x, y, z in triangles:
        graph[x].update((y, z))
        graph[y].update((x, z))
        graph[z].update((x, y))
    return graph


NFA and DFA implementation Can you review my F# code and point out some insights about it?

What I want to know, in order of relevance:

Avoid so much common code between DFA and NFA. I want to make something more generic, there is much code in common there.
Make it more F# idiomatic

Performance is not a concern. Readability is.

module DFA =
    type DeterministicFiniteAutomaton = {
        InitialState: string
        FinalStates: Set<string>
        Transitions: Map<string * char, string>
    }

    let private nextState (symbol:char) (state:string) (transitions:Map<string * char, string>) =
        transitions |> Map.tryFind (state, symbol)

    let rec private haltState (input:string) (index:int) (state:string) (transitions:Map<string * char, string>) =
        match index with
        | i when i = input.Length -> state
        | _ ->
            match nextState input.[index] state transitions with
            | None -> null
            | Some state -> haltState input (index+1) state transitions

    let accepts (input:string) (dfa:DeterministicFiniteAutomaton) =
        dfa.FinalStates |> Set.contains (haltState input 0 dfa.InitialState dfa.Transitions)

module NFA =
    type NondeterministicFiniteAutomaton = {
        InitialState: string
        FinalStates: Set<string>
        Transitions: Map<string * char, string List>
    }

    let private nextState (symbol:char) (state:string) (transitions:Map<string * char, string List>) =
        transitions |> Map.tryFind (state, symbol)

    let rec private haltStates (input:string) (index:int) (state:string) (transitions:Map<string * char, string List>) =
        match index with
        | i when i = input.Length -> Seq.singleton state
        | _ ->
            match nextState input.[index] state transitions with
            | None -> Seq.empty
            | Some states ->
                states |> Seq.collect (fun state ->
                    haltStates input (index+1) state transitions)

    let accepts (input:string) (nfa:NondeterministicFiniteAutomaton) =
        haltStates input 0 nfa.InitialState nfa.Transitions
        |> Set.ofSeq
        |> Set.intersect nfa.FinalStates
        |> Set.count > 0

A quick look shows the only difference with your FiniteAutomaton types is that the transition either takes a string or string List.  So genericize it!

type FiniteAutomaton<'a> = {
    InitialState: string
    FinalStates: Set<string>
    Transitions: Map<string * char, 'a>
}
type DeterministicFiniteAutomaton = FiniteAutomaton<string>
type NondeterministicFiniteAutomaton = FiniteAutomaton<string List>

The rest pretty much falls in place:

let private nextState symbol state fa =
    fa.Transitions |> Map.tryFind (state, symbol)

let rec private haltState (input:string) index state fa =
    match index with
    | i when i = input.Length -> state
    | _ ->
        match nextState input.[index] state fa with
        | None -> null
        | Some state -> haltState input (index+1) state fa

let accepts input fa =
    fa.FinalStates |> Set.contains (haltState input 0 fa.InitialState fa)

I try to remove as much type annotations as possible.  Just let F#'s type inference do its magic. Also, it was convenient to pass around the fa instead of fa.Transitions.  Finally, the code compiles, but I have no idea if it works.

Edit:

If you want to be totaly generic you can do this:

type FiniteAutomaton<'STATE, 'TOKEN when 'STATE:comparison and 'TOKEN:comparison> = {
    InitialState: 'STATE
    FinalStates: Set<'STATE>
    Transitions: Map<'STATE * 'TOKEN, 'STATE List>
}

Also 'STATE should be a 'TOKEN List.


Consume rest endpoint removing payload's unwanted properties and falsy values Let's suppose that there's a third part web service and I want to make a GET
request to it and use the response to feed my project's service.

Next code will allow me to create a dict object only with non-falsy keys' values:

import requests
from abc import ABC, abstractmethod

class AbstractPayload(dict, ABC):
    def __new__(cls, *args, **kwargs):
        """Validate: values are not empty and that each key in kwargs exists
        in cls.fields iterable, then it creates and returns the final dict
        instance using the field and the value as key-value pairs."""
        return {
            field: v for field, v in kwargs.items() if v and field in cls.fields
        }

    @property
    @abstractmethod
    def fields(cls):
        """Valid fields' iterable. Override this as you need."""
        pass

class UpdatePayload(AbstractPayload):
    fields = ('location', 'capacity', 'maxlatency', 'tags')

# Usage example:
response = requests.get('/thirdpart/device/189')
if response.ok:
    requests.patch('/mydevices/33', json=UpdatePayload(**response.json()))

Summary:

Avoiding unwanted fields being added to the endpoint patch payload.
Having a unique block of code (__new__ method) where the properties are
gonna be evaluated to discard falsy values.
Making it easily adaptable to all the api resources (just inheriting from
a class and defining a fields iterable)

Questions:

What do you think about this approach?
Do you have any suggestions to improve it?
Or even better, Do you have another cleaner approach?

field: v for field, v in kwargs.items() if v and field in cls.fields

Feel free to break that into a multi-line comprehension.

Note that the in has linear cost w.r.t. len(cls.fields).

    """Valid fields' iterable. Override this as you need."""
    pass

Consider specifying that it shall be a set rather than just an iterable. (Also, I suppose return None would more clearly show that by default we don't return an iterable. Or consider doing an explicit raise.)


Mean of the squares of differences for a large matrix I have tried to obtain speed gains by transferring Matlab calculations to C and call it using .mex. My goal is to operate on a matrix \$B\$  which has roughly the dimension 10000x1000

$$
\frac{1}{\text{size(B,1)}}\sum_{i=1}^{2+l*t} \text{mean}( (B(:,i)-B(:,i+1))^2)$$

for \$ l*t<\text{size(B,2) and chosen } t \in \mathbb{N}\$.

My matlab code is as follows:

A=randn(10000,1000);

tic
RVV_Da_path=ones(size(A,2)-1,1);
i=2;
j=1;
while i <= size(A,2)+1-t_step
   RVV_Da_path(j)=    sum(mean(diff((A(:,1:i)),1,2).^2,1));
   i=i+t_step;
   j=j+1;
end
RVV_Da_path(j)=    sum(mean(diff((A(:,1:end)),1,2).^2,1));
toc

My C code goes as follows:

#include <math.h>
#include "mex.h"

void mexFunction(int nlhs, double *plhs[], int nrhs, const mxArray *prhs[])
{

  // calculate 1/size(A,1)*sum_{i=1}^{2+l*t} mean(A(:,i)-A(:,i+1))^2, for l*t < size(A,2), t \in N

#define RV_Out  plhs[0]
#define A_In    prhs[0]
#define t_In    prhs[1]

  double *A,*RV,p,Num_Of_Rows;
  int M, N,Mt,t,i,j,m,k;

  A=mxGetPr(A_In);

  t=mxGetScalar(t_In);
  M=mxGetM(A_In);

  Num_Of_Rows=mxGetM(A_In);
  N=mxGetN(A_In);

  RV_Out=mxCreateDoubleMatrix(1, N, mxREAL); // init vektor to save the sum of squared first differences
  RV = mxGetPr(RV_Out);
  memset(RV, 1, N);

  i=0;
  k=0;
  while(i < N-t)
  {
    for(j=0;j<=i;j++)
    {
      for(m=0;m<M;m++)
      {
        RV[k]+=1/Num_Of_Rows*(pow(A[m + M * j]  -  A[m + M * (j+1)],2.0)) ; // calculate the sum of squared first differences until last possible step
      }
    }

    i+=t;
    k++;
  }

  for(j=0;j<N-1;j++)
  {
    for(m=0;m<M;m++)
    {
      RV[k]+=1/Num_Of_Rows*(pow(A[m + M * j]  -  A[m + M * (j+1)],2.0)) ; // calculate the sum of squared first differences until end
    }
  }

  for(i=k+1;i<N;i++) RV[i]=1;  // set the remaining entries equal to 1
  return;

}

Now when I translate the C code to a .mex file and compare the speed of the Matlab and the C code the C code is roughly 4-8 times slower for big \$ B\$'s. Why is this the case? Where is the bottleneck in my C-code and how can I work around it?

The overall Matlab algorithm can be simplified.  The key is to realize that:

Inter-column differences never change
You can use cumulative sums on the means of each column

First let us remove the variable step factor.  So tstep = 1.  I will reincorporate the variable step factor later.

Let B be a \$m \times n\$ matrix.  B(:,1:i) yields a \$m \times i\$ submatrix of B.
Let D = diff(B(:,1:i),1,2).  This yields the column differences of B(:,1:i).

The first column of D, aka D(:,1), is equal to the second column of B minus the first column of B, aka B(:,2) - B(:,1)
The second column of D, aka D(:,2) is equal to the third column of B minus the second column of B, aka B(:,3) - B(:,2)
...
The last column of D, aka D(:,i-1) is equal to the \$i^{th}\$ column of B minus the \$(i-1)^{th}\$ column of B, aka B(:,i) - B(:,i-1)

You'll notice that if i increases by 1, these column differences remain the same.  The only thing that changes is that we obtain one additional column difference in D.  Hence you do not need to perform the diff() function multiple times.  Simply perform it once on the entire \$m \times n\$ matrix to yield a \$m \times (n-1)\$ matrix of column differences and take submatrices of this difference matrix.

So the original code you posted:

B=(20*randn(10000,1000));
RV_B=ones(size(B,2),1);
t_step = 1;

i=2;
j=1;

while i <= size(B,2)+1-t_step
    RV_B(j)=    sum(mean(diff((B(:,1:i)),1,2).^2,1));
    i=i+t_step;
    j=j+1;
end

RV_B(j)=    sum(mean(diff((B(:,1:end)),1,2).^2,1));
M_1=RV_B';

Can be simplified to:

RV_B=ones(size(B,2),1);
t_step = 1;
D = diff(B,1,2);

i=2;
j=1;

while i <= size(B,2)+1-t_step
    RV_B(j)=    sum(mean(D(:,1:i-1).^2,1));
    i=i+t_step;
    j=j+1;
end

RV_B(j)=    sum(mean(D(:,1:end).^2,1));
M_1=RV_B';

The next step is to realize that if D is our \$m \times (n-1)\$ difference matrix, mean(D) yields a \$1 \times (n-1)\$ matrix of column means.  If we truncate columns of this matrix as we do when we take submatrix D(:,1:i-1) it does not affect the means of the columns that are still there.  So instead of performing mean() over and over on submatrices, we can again pull it out the loop, perform mean() once on the entire matrix D, and take submatrices of this mean matrix.

So now the code could look like this:

RV_B=ones(size(B,2),1);

t_step = 1;
D = diff(B,1,2);
MD = mean(D.^2);

i=2;
j=1;

while i <= size(B,2)+1-t_step
    RV_B(j)=    sum(MD(:, 1:i-1));
    i=i+t_step;
    j=j+1;
end

RV_B(j)=    sum(MD(:, 1:end));
M_1 = RV_B';

Similarly, instead of performing a sum on the rows of the submatrices, we can perform a cumulative sum along the rows (or rather row): $$\begin{align}CSMD[i][j] &= \sum_{k=1}^{j} MD[i][k] \\
&= CSMD[i][j-1] + MD[i][j]\end{align}\\$$

Incorporating Matlab's built-in cumsum() along with changing some intermediary variable names to better illustrate their purpose yields the following code:

diffB = diff(B,1,2);
meanSquaredDiff = mean(diffB.^2);
M_1 = cumsum(meanSquaredDiff, 2);

Now we no longer have any explicit loops (in the matlab code at least).  This algorithm is much faster and simpler.  Consequently, it is much easier to port to C.

You will find that for tstep=1, M_1 for your algorithm has dimension size(B,2) and for the simplified code I provided it has dimension size(B,2)-1.  However, in your original algorithm, M_1(end) and M_1(end-1) are always the same if tstep=1, hence you were performing one calculation too many.

Now let us reincorporate the variable step factor.  Realize that if we increase tstep it is equivalent to taking every \$\textrm{tstep}^{th}\$ output after the cumulative sum.  So if you want tstep=5 you perform the same three-line code above but at the end sample it via M_1 = M_1(1:5:end) or in general M_1 = M_1(1:step:end).  You can use isequal to compare the outputs of your original approach to this approach.  Remember that you one-padded your output so if you use \$\textrm{tstep} \gt 1\$, you need to remove the extra padding when comparing the matrices.

Finally the last index of the non-padded output will differ as well (if \$\textrm{tstep} \gt 1\$) since your computation computes the last index differently than the rest of the indices.  You can fix this by performing tmp =  M_1(end); M_1 = M_1(1:step:end); M_1(end) = tmp; instead of simply M_1 = M_1(1:step:end) however the latter approach is more consistent in my opinion.


Supporting Enum in EF 5 with .Net 4.0 I'm currently working through Getting Started with EF5 using MVC 4 in Visual Studio 2010 (.NET 4.0) to learn about how to use ASP.NET and Entity Framework. During the beginning of the tutorial I ran into an issue with enum support when creating a database with Code First Migrations. For reasons unknown I have to work in this environment and create my own support for enums so I decided to make the property int instead then I would convert it to Enum when I want to display it like so:

using System;
using System.Web;
using System.Web.Mvc;
using System.Web.Mvc.Html;
using System.Linq;
using System.Linq.Expressions;

namespace ContosoUniversity.Extensions
{
    public static class Extensions
    {
        public static String DisplayAsEnum<TModel, TValue>(this HtmlHelper<TModel> cHtml,
                                                           Expression<Func<TModel, TValue>> cExpression,
                                                           Type cType,
                                                           String strDisplay = "Unknown")
        {
            try
            {
                if (cType.IsEnum)
                {
                    object cResult = cExpression.Compile().DynamicInvoke(cHtml.ViewData.Model);

                    if (cResult != null)
                    {
                        strDisplay = Enum.ToObject(cType, cResult).ToString();
                    }
                }
            }
            catch (Exception cExc)
            {
                strDisplay = cExc.ToString();
            }

            return (strDisplay);
        }
    }
}

In my razor cshtml file I display it to the webpage with this call:

@Html.DisplayAsEnum(modelItem => item.Grade, typeof(Grade), "No Grade")

I have a feeling that this is not the "professional" or "right" way of handling the lack of enum support so I was wondering if there is anyway to improve this function or is there a better way of handling enumerations in EF 5 with .Net 4.0 support?

I was shown a Stack Overflow question that provides me with a better solution to handling the enum support. It is very helpful and I am using it at the moment, but since I do like the function that I made, is it possible to improve it?

Ideally you won't want to change the strDisplay, so we should probably take it out of the parameter list and make it a constant inside the method and then just return in each of the 3 circumstances.

var display = "Unknown";

try
{
    if (cType.IsEnum)
    {
        object cResult = cExpression.Compile().DynamicInvoke(cHtml.ViewData.Model);

        if (cResult != null)
        {
            return Enum.ToObject(cType, cResult).ToString();
        }
    }
}
catch (Exception cExc)
{
    return cExc.ToString();
}

return display;

this seems weird to me that we are using the variable (whatever you choose to name it)  only once, I would probably in this circumstance (since we aren't allowing it to be changed)  just return the string Unknown like this

try
{
    if (cType.IsEnum)
    {
        object cResult = cExpression.Compile().DynamicInvoke(cHtml.ViewData.Model);

        if (cResult != null)
        {
            return Enum.ToObject(cType, cResult).ToString();
        }
    }
}
catch (Exception cExc)
{
    return cExc.ToString();
}

return "Unknown";


Listing the first 100 prime numbers I thought of this implementation, and I want to get feedback from you.. what design would you use for printing first 100 prime numbers?

I used the fact that, if the number is not divisible by any prime less than itself, it will be a new prime.

    ArrayList<Integer> list = new ArrayList<Integer>();
    list.add(2); // first prime 2 goes to our collection
    int count = list.size(); // count is our size it will be max 100
    int number = 3;  // first prime number after 2 is 3

    while ( count != 100 ) // we want first 100 prime
    {
        boolean isPrime = true; // we assume that the number is prime
        for ( int i = 0; i < list.size(); i++ )
        {
            if ( number % list.get(i) == 0 ) // we check for every less prime
                isPrime = false; // however, if it is divided by any other less prime, isPrime will be false

        }

        if ( isPrime ) // if it stays true, we will add it to our collection
        {
            list.add(number);
        }

        number++; // we try every number
        count = list.size(); // count equals size of collection at every turn

    }

    System.out.println(list);

Some notes:

Your bracing does not follow standard Java conventions. This is more of a matter of preference, but this is how I would format your code:

ArrayList<Integer> list = new ArrayList<Integer>();
list.add(2);
int count = list.size();
int number = 3;

while (count != 100) {
    boolean isPrime = true;
    for (int i = 0; i < list.size(); i++) {
        if (number % list.get(i) == 0) {
            isPrime = false;
        }
    }
    if (isPrime) {
        list.add(number);
    }
    number++;
    count = list.size();
}
System.out.println(list);

Some other points about formatting:

I have removed some excess blank spaces and lines.
I put braces around all the statements inside the if statement without braces.

Now, to the loop:

while (count != 100) {
    boolean isPrime = true;
    for (int i = 0; i < list.size(); i++) {
        if (number % list.get(i) == 0) {
            isPrime = false;
        }
    }
    if (isPrime) {
        list.add(number);
    }
    number++;
    count = list.size();
}

This could easily be a for loop:

for (int count = 1, number = 3; count < 100; number++) {
    boolean isPrime = true;
    for (int i = 0; i < list.size(); i++) {
        if (number % list.get(i) == 0) {
            isPrime = false;
        }
    }
    if (isPrime) {
        list.add(number);
    }
    count = list.size();
}

Also, all your list.size(). You can remove many of the calls:

for (int count = 1, number = 3; count < 100; number++) {
    boolean isPrime = true;
    for (int i = 0; i < list.size(); i++) {
        if (number % list.get(i) == 0) {
            isPrime = false;
        }
    }
    if (isPrime) {
        list.add(number);
        count++;
    }
}

You can also remove much of the iterations of the inner loop by breaking when isPrime is true, or just:

for (int count = 1, number = 3; count < 100; number++) {
    for (int i = 0; i < list.size(); i++) {
        if (number % list.get(i) == 0) {
            list.add(number);
            count++;
            break;
        }
    }
}

But the main thing is, your code is not as efficient as it could be. Try a Sieve:

List<Integer> result = new LinkedList<Integer>();
int n = 1.4 * 100 * Math.log(100);
boolean[] isPrimeArray = new boolean[n + 1];
for (int i = 2; i <= n; i++) {
    isPrimeArray[i] = true;
}
for (int i = 2, primesLeft = 100; i * i <= n && primesLeft > 0; i++) {
    if (isPrimeArray[i]) {
        result.add(i);
        primesLeft--;
        for (int j = i; i * j <= n; j++) {
            isPrimeArray[i * j] = false;
        }
    }
}
System.out.println(result);

The sieve does:

Sets all numbers to true (as in, is a prime).
Starts at 2, and works its way through the primes. While doing that, marks all the multiples of a prime to false.
If it has 100 primes, the loop will terminate.
Print the result.

Also you have 100 as a magic number. Set it as a field:

private static final int MAX = 100;

// Code here

Use:

List<Integer> result = new LinkedList<Integer>();
int n = 1.4 * MAX * Math.log(MAX); // Overestimate by 40%
boolean[] isPrimeArray = new boolean[n + 1];
for (int i = 2; i <= n; i++) {
    isPrimeArray[i] = true;
}
for (int i = 2, primesLeft = MAX; i * i <= n && primesLeft > 0; i++) {
    if (isPrimeArray[i]) {
        result.add(i);
        primesLeft--;
        for (int j = i; i * j <= n; j++) {
            isPrimeArray[i * j] = false;
        }
    }
}
System.out.println(result);

This will result in a much faster result.


Caesar cipher in Python I wrote this looping Caesar cipher with a default key. I'd love for some input on how I could make this operate better/more effectively.

#Et Tu, Brute? [Caesar's Cipher]
#loops?
#Determine type of cipher...
doLoop=("y")
while doLoop==("y"):
#yep. loops.
    det=input("Enter N for number or C for character; 'exit' to quit program ").lower() #lower case for simplicity! it don't care!
#Now the fun part!
#Numbers...
    if (det)==("n"):
        print("The default cipher key is 5")
        numRaw=input("Enter number to be decoded: ")
        if ord(numRaw)<ord("5"):
            numDec=ord(numRaw)+5
        else:
            numDec=ord(numRaw)-5
        print("The decoded number is: ",(chr(numDec)))

#Letters...
    if (det)==("c"):
        print("The default cipher key is 13")
        charRaw=input("Enter character to be decoded: ").lower() #lower case for simplicity!
        if ord(charRaw)<ord("m"):
            charDec=ord(charRaw)+13
        else:
            charDec=ord(charRaw)-13
        print("The decoded character is: " , (chr(charDec)))
    if det==("exit"):
        break

Python strings have a translate method that applies a substitution cipher. There is also a function str.maketrans (string.maketrans in Python 2) that helps with building the translation table:

>>> from string import ascii_lowercase as alphabet
>>> shift = 13
>>> cipher = str.maketrans(alphabet, alphabet[shift:] + alphabet[:shift])
>>> print("caeser salad is tasty".translate(cipher))
pnrfre fnynq vf gnfgl

of coarse in python2(with a shift of 13) one can simply do "My Message".encode("rot13")


Simple recursive Summing program Any tips on how to improve this simple recursion program?

#include <iostream>
#include <cstdlib>

using namespace std;

int findSum(int*, const int, int, int);

int main()
{
    int length = 5, sum = 0, n = 0;
    int list[length];

    for (n; n < length ; n++ )
    {
     list[n] = n + 1; //initializes array elements to 1,2,3,4,5
    }

    n = 0;

    cout << findSum(list, length, n, sum) << endl;

    system("pause");
    return 0;
}

int findSum(int list[], const int length, int n, int sum)
{
    if (n == length) //if end of list has been reached, stop recursion
    {
        return sum;
    }
    else
    {
        sum += list[n]; //adds list elements
        n++;            //increments list
        return findSum(list, length, n, sum);
    }
}

I think I'd prefer something like this:

int findsum(int const *list, int length) {
    if (0 == length)
       return 0;
    return list[0] + findsum(list+1, length-1);
}

This follows one of the typical recursive formulations -- basically a function that's almost like an inductive proof. Start by establishing a correct value for the simplest possible base case (or some obvious base case anyway), and then establish a pattern that's correct for some some sort of "Base+1" type of case.


Collating users and their scores Is anything what could I improve on my code?

import java.math.BigDecimal;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Map;

public class Helper {

    public static Map<User, BigDecimal> createPlayersScoreMap(List<User> users, List<Score> scores) {
        List<User> players = getPlayers(users);
        Map<User, BigDecimal> playersScoreMap = new HashMap<User, BigDecimal>();

        for (User player: players) {
            BigDecimal sumScore = new BigDecimal(0);
            for (Score score: scores) {
                if (player.equals(score.getPlayer())) {
                    sumScore = sumScore.add(score.getResult());
                }
            }
            playersScoreMap.put(player, sumScore);
        }
        return playersScoreMap;
    }

    private static List<User> getPlayers(List<User> users) {
        List<User> filteredUsers = new ArrayList<User>(users);
        for (Iterator<User> it = filteredUsers.iterator(); it.hasNext();) {
            if (!it.next().isPlayer())
                it.remove();
        }
        return filteredUsers;
    }

}

Helper is not a good class name. You should be able to come up with something that provides more context to what the functions are doing.

In getPlayers(), I would loop and add instead of adding all and then removing.

private static List<User> getPlayers(List<User> users) {
    List<User> filteredUsers = new ArrayList<User>();
    for (User user : users) {
        if (user.isPlayer()) {
            filteredUsers.add(user);
        }
    }
    return filteredUsers;
}

I also added curly braces around the if block. It's not necessary, but it will prevent errors if you need to add an extra line later. You also included them in createPlayersScoreMap(), so it's best to be consistent.

If you have a large number of players and scores, you can make createPlayersScoreMap() more efficient (in terms of Big O processing). If n is the number of users and m is the number of scores, the method is currently O(n*m). However, if you sum all the scores first and then filter the players, you can get O(n+m).

public static Map<User, BigDecimal> createPlayerScores(List<User> users, List<Score> scores) {
    List<User> players = getPlayers(users);
    Map<User, BigDecimal> playerScores = new HashMap<User, BigDecimal>();

    for (Score score: scores) {
        User player = score.getPlayer();
        BigDecimal sumScore = score.getResult();
        if (playerScores.containsKey(player) {
            sumScore = sumScore.add(playerScores.get(player));
        }
        playerScores.put(player, sumScore);
    }
    Map<User, BigDecimal> filteredScores = new HashMap<User, BigDecimal>();
    for (User player : players) {
        filteredScores.put(player, playerScores.get(player));
    }
    return filteredScores;
}

I also change the name of the function/variable. They type system makes it obvious that you are dealing with a Map, you don't need to include it in the variable name. If the calling method has Lists and Maps that deal with players and scores, then it might be acceptable to include the type as part of the variable name.


Wordpress downloader and unpacker Wrote a small bash-script today that downloads the latest swedish wordpress archive , extracts it and writes wp-config.php

Would appreciate any feedback that could improve it / prettify it.

The script assumes that the database name & the username is always the same.

#!/bin/bash

#Download wordpress tar.gz
echo "Laddar ner senaste wordpress på svenska"
tarfile=$(curl -s https://sv.wordpress.org/ | grep -ioE "https:\/\/sv.wordpress.org\/wordpress-[0-9].[0-9].[0-9]-sv_SE.tar.gz")
wget $tarfile -O senaste_wp.tar.gz
tar xvfz senaste_wp.tar.gz -C ./
rm senaste_wp.tar.gz

# Move the wordpress files to this dir
mv wordpress/* .
rm -r wordpress

# Get wordpress salts
salts=$(curl https://api.wordpress.org/secret-key/1.1/salt/)

# Read userinput
read -p "Ange databasanvändare : " dbuser
read -p "Ange databaslösenord : " dbpass

# Write wp-config.php
cat > wp-config.php << EOF
define('DB_NAME', '$dbuser');
define('DB_USER', '$dbuser');
define('DB_PASSWORD', '$dbpass');
define('DB_HOST', 'localhost');
define('DB_CHARSET', 'utf8');
define('DB_COLLATE', '');

$salts

\$table_prefix  = 'wp_';

define('WP_DEBUG', false);

if ( !defined('ABSPATH') )
        define('ABSPATH', dirname(__FILE__) . '/');

require_once(ABSPATH . 'wp-settings.php');
EOF

You don't need to escape slashes for grep, you can simplify to:

tarfile=$(curl -s https://sv.wordpress.org/ | grep -ioE "https://sv.wordpress.org/wordpress-[0-9].[0-9].[0-9]-sv_SE.tar.gz")

curl and wget are too similar tools. Consider using only one of them, to reduce the number of dependencies of your script.

Instead of this:

wget $tarfile -O senaste_wp.tar.gz
tar xvfz senaste_wp.tar.gz -C ./
rm senaste_wp.tar.gz

You can pipe directly to tar, without needing a temporary file, like this:

curl $tarfile | tar xvz

Lastly, your script is not using anything Bash specific, so you could change the shebang line to !#/bin/sh.


A generic logger in modern C++ I've created a simple logging system that I'm planning to use and I've tried out some modern C++ features to make it generic. First I've made a log class that represents a single log message:

struct log : std::stringstream
{
private:
    template <typename Level, size_t I = 0, typename... Stamps>
    static inline typename std::enable_if<I == sizeof...(Stamps), void>::type
        __stamp(log& l)
    {
    }

    template <typename Level, size_t I = 0, typename... Stamps>
    static inline typename std::enable_if<I < sizeof...(Stamps), void>::type
        __stamp(log& l)
    {
        l << std::tuple_element_t<I, std::tuple<Stamps...>>::Get<Level>();
        __stamp<Level, I + 1, Stamps...>(l);
    }

public:
    template <typename Level, typename... Stamps>
    static log& emit()
    {
        static log l;
        __stamp<Level, 0, Stamps...>(l);
        l << ": ";
        return l;
    }

    template <typename T>
    log& operator<<(T el)
    {
        *((std::stringstream*)this) << el;
        return *this;
    }

    template <typename Logger>
    void Flush()
    {
        Logger::Log(str().c_str());
        clear();
    }
};

template <typename Logger>
std::ostream& endlog(std::ostream& out)
{
    ((log*)&out)->Flush<Logger>();
    return out;
}

typedef Logger<ConsoleSink> DefaultLogger;
std::ostream& endlog(std::ostream& out)
{
    ((log*)&out)->Flush<DefaultLogger>();
    return out;
}

And I've created the main logger:

template <typename... Sinks>
class Logger
{
private:
    static std::tuple<Sinks...> s_Sinks;

private:
    template <size_t I = 0, typename... Tp>
    static inline typename std::enable_if<I == sizeof...(Tp), void>::type
        __Log(const char* msg)
    {
    }

    template <size_t I = 0, typename... Tp>
    static inline typename std::enable_if<I < sizeof...(Tp), void>::type
        __Log(const char* msg)
    {
        std::get<I>(s_Sinks).Write(msg);
        __Log<I + 1, Tp...>(msg);
    }

public:
    static void Log(const char* msg)
    {
        __Log<0, Sinks...>(msg);
    }
};

template <typename... Sinks>
std::tuple<Sinks...> Logger<Sinks...>::s_Sinks;

An example usage would be:

log::emit<Info, LevelStamp, SeparatorStamp, TimeStamp>() << "Hello, Logging " << 3 << endlog;

Now I can see a lot of problems with my own code, for example because of my emit function is defined with variadic stamps, there's no way to define a default overload for it so if I always want the same stamps, I have to type them out. Another is that my DefaultLogger is not configurable from outside the logger code which I think should be a feature.

Is my approach any good? How could I improve it?

Stop using double underscore.
The double underscore is reserved for the implementation it definitely should not be used in "user code".

See: What are the rules about using an underscore in a C++ identifier?

Recursive template Call.

You use a recursive technique to call the Write() method for all your sinks. I prefer to use the loop technique (I would say this is a more modern approach but I have no reference for that). But I find it clearer as you don't need the empty terminating function call.

private:
    using Tuple=std::tuple<Sinks...>;

    template<std::size_t... index>
    static void writeToLogs(char const* m, std::integer_sequence<std::size_t, index...> const&)
    {
        auto ignore = {(std::get<index>(s_Sinks).Write(m),true)...};
    }

public:
    static void Log(const char* msg)
    {
        writeToLogs(msg, std::make_index_sequence<std::tuple_size<Tuple>::value>());
    }

Design

Everybody and their grandson tends to want to write a logger at some point in time. The trouble is we already have a good logger built into the *nix world (syslog). If you are not using syslog to do your logging you are doing it wrong.

It (syslog) already does everything you need to do and more and there is a whole ecosystem built around it for monitoring. Now there is nothing wrong with writing a wrapper around syslog but you have to be smart about it (the current ones are very smart).

Log(debug, getSomeMessage());

This should not call the function getMessage() if the debugging level is set to production. That would be a complete wast of time and resources as debug messages are currently being ignored.