Depleting certain data from a webpage asynchronously I've written a scraper in python using asyncio library to exhaust the name, address and phone number of all category from a webpage. Few days back, when I created a scraper to parse a webpage recursively, I got a suggestion to use asynchronous process to crawl a webpage with enormous data for the sake of optimum performance and avoiding blocking nature. However, this time I tried to follow that process so that my crawler can scrape the webpage asynchronously. It is running specklessly now. I used css selector to make the parser more readable along with acceleration of speed. This is my first time to work with this library, so I suppose there are scopes to take it to the next level whetting it's performance.

Here is what I've written:

import requests
from lxml import html
import asyncio

main_url = "https://www.someweblink/letter/{0}"

async def alphabetical_links(mainurl):

        # scraping all the links with alphbetical index from start page
        for link in [mainurl.format(chr(page)) for page in range(97,123)]:
            response = requests.get(link).text
            tree = html.fromstring(response)

            for container in tree.cssselect(".proSitemapLink a"):
                await sub_links(container.attrib['href'])
                # going for the contents of the sublinks lies within each page with alphabetical index

async def sub_links(process_links):

        response = requests.get(process_links).text
        root = html.fromstring(response)

        for container in root.cssselect(".proListing"):
            name = container.cssselect("h2 a")[0].text if container.cssselect("h2 a") else ""
            # Used text_content() to get the full address separated by "br" tag
            address = container.cssselect(".proListingLocation")[0].text_content() if container.cssselect(".proListingLocation") else ""
            phone = container.cssselect(".proListingPhone")[0].text if container.cssselect(".proListingPhone") else ""
            profilelink = container.cssselect("h2 a")[0].attrib['href'] if container.cssselect("h2 a") else ""
            await adding_weblink(name, address, phone, profilelink)
            # finally it will go to the profile page to extract website address if available

async def adding_weblink(name, address, phone, profilelink):

        response = requests.get(profilelink).text
        root = html.fromstring(response)
        weblink = root.cssselect(".proWebsiteLink")[0].attrib['href'] if root.cssselect(".proWebsiteLink") else ""
        if weblink:
            print(name, address, phone, weblink)
        else:
            print(name, address, phone)

loop = asyncio.get_event_loop()
loop.run_until_complete(alphabetical_links(main_url))
loop.close()

magic numbers

ASCII numbers are pretty obvious, but I would still rather see you express it in this way:

letters = range(ord('a'), ord('z') + 1)
for link in [mainurl.format(chr(page)) for page in letters:

Alternatively, import string and iterate through string.ascii_lowercase.

naming

I'm not fond of these identifiers, as method name is noun, and argument is verb:

async def sub_links(process_links):

Consider making it:

async def process_links(links):

(or process_links(sub_links) if you prefer). Some folks would use "url" for "link", but that's not a substantive criticism. "Process" is a bit vague - I feel download_links() is better.

DRY

Consider breaking out a temp variable:

    anchor = container.cssselect("h2 a")
    name = anchor[0].text if anchor else ""

Better, break that out as a trivial helper function so address and others can concisely use it. I never see such idioms when using BeautifulSoup - perhaps you'd like to try that library in your next project?

Again, I'm not happy with the adding_weblink() identifier, it is much too vague. Your comment suggests the appropriate verb would be download_profile() (or download_profile_page()).
Also, PEP8 asks that you name the parameter profile_link - no biggie.

Settle on tree vs. root, just pick one.

Consider writing a brief helper function that returns html.fromstring(requests.get(link).text).

In this context weblink is on the redundant side, probably enough to just call it a link.

The if weblink: test probably isn't needed, as printing blank + empty_string before newline is pretty harmless.

output format

I can't imagine that name, address, etc. are easy to unambiguously parse from your output. It seems like you want a csv.writer, which would let you writerow(name, address, phone, weblink).

Imagine that 1% of all GET requests, or 10%, yielded temporary fail that you should retry. Would the current code grab 99% (or 90%) of all pages? Could a cleanup module conveniently identify links that should be retried?

Overall the code is nice and clear. I am not left with doubts about whether it would do what it is intended to do. (If Houzz changes details like .proListingLocation there will be some fragility issues, but hey, that's the life of a scraper. Definitely store a sample web page in source control, along with corresponding unit tests, so you can debug things in future when the inevitable website update happens. Have Jenkins run a daily test against the website so you notice bit-rot within 24 hours before it drifts "too" far.)


List the combinations that add up to 0 from a vector of numbers I'm writing a function that, given a Vec<i32> of numbers, lists the combinations that add up to 0. The code works, but it feels amateurish.

Specifically, the inside of the loop looks really ugly -- there must be a simpler way of doing this that avoids unwrap() and the repetition. Maybe with match?

fn calc_zer(input: Vec<i32>) -> Vec<(i32, i32, i32)> {
    let mut output: Vec<(i32, i32, i32)> = Vec::new();
    for a in (0..input.len() - 2) {
        for b in (a+1..input.len() -1) {
            for c in (b+1..input.len()) {
                if (input.get(a).unwrap() + input.get(b).unwrap() + input.get(c).unwrap() == 0) {
                    output.push((*input.get(a).unwrap(), *input.get(b).unwrap(), *input.get(c).unwrap()));
                }
            }
        }
    }
    output
}

Rust is a statically-compiled language, which means you get warnings and errors at compile time. Don't ignore those:

warning: unnecessary parentheses around `for` head expression
 --> src/main.rs:3:14
  |
3 |     for a in (0..input.len() - 2) {
  |              ^^^^^^^^^^^^^^^^^^^^
  |
  = note: #[warn(unused_parens)] on by default

warning: unnecessary parentheses around `for` head expression
 --> src/main.rs:4:18
  |
4 |         for b in (a+1..input.len() -1) {
  |                  ^^^^^^^^^^^^^^^^^^^^^
  |
  = note: #[warn(unused_parens)] on by default

warning: unnecessary parentheses around `for` head expression
 --> src/main.rs:5:22
  |
5 |             for c in (b+1..input.len()) {
  |                      ^^^^^^^^^^^^^^^^^^
  |
  = note: #[warn(unused_parens)] on by default

warning: unnecessary parentheses around `if` condition
 --> src/main.rs:6:20
  |
6 |                 if (input.get(a).unwrap() + input.get(b).unwrap() + input.get(c).unwrap() == 0) {
  |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: #[warn(unused_parens)] on by default

Next, run Clippy to get even more automated suggestions. Note that this contains the answer to your question:

warning: this argument is passed by value, but not consumed in the function body
 --> src/main.rs:1:24
  |
1 | pub fn calc_zer(input: Vec<i32>) -> Vec<(i32, i32, i32)> {
  |                        ^^^^^^^^ help: consider changing the type to `&[i32]`
  |
  = note: #[warn(needless_pass_by_value)] on by default
  = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#needless_pass_by_value

warning: called `.get().unwrap()` on a Vec. Using `[]` is more clear and more concise
 --> src/main.rs:6:20
  |
6 |                 if input.get(a).unwrap() + input.get(b).unwrap() + input.get(c).unwrap() == 0 {
  |                    ^^^^^^^^^^^^^^^^^^^^^ help: try this `&input[a]`
  |
  = note: #[warn(get_unwrap)] on by default
  = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#get_unwrap

Extract local variables so you don't even have the overhead of accessing the array multiple times.
There's no reason to provide an explicit type for output - it will be inferred from the return value.
Extract a variable for input.len() since that never changes during the iteration.
It's almost always better to iterate over the collection directly instead of indexing it. We can also use Iterator::enumerate to get the index as we go along.

This yields

fn calc_zer(input: &[i32]) -> Vec<(i32, i32, i32)> {
    let mut output = Vec::new();

    let len = input.len();

    for (a_i, &a) in input[0..len - 2].iter().enumerate() {
        for (b_i, &b) in input[a_i + 1..len - 1].iter().enumerate() {
            for &c in &input[a_i + b_i + 2..len] {
                if a + b + c == 0 {
                    output.push((a, b, c));
                }
            }
        }
    }

    output
}

This is a bit ugly, and even worse — I got it wrong originally! See the revision history for details. This was caused because enumerate starts counting from zero, even though we are iterating through an offset into the original vector.

However, Rust has a strong emphasis on reusing code via crates. In this example, I turn to one of my favorites — itertools:

extern crate itertools;

use itertools::Itertools;

fn calc_zer(input: &[i32]) -> Vec<(i32, i32, i32)> {
    input.iter()
        .tuple_combinations()
        .filter(|&(a, b, c)| a + b + c == 0)
        .map(|(&a, &b, &c)| (a, b, c))
        .collect()
}

Much more readable!


Fisher-Yates shuffle Java implementation This is my attempt at an implementation of the modern Fisher-Yates shuffle in Java. I'm not sure if it can be made more efficient, but I did my best to make it as simple as possible, and I learned how to use generics specifically for this, so If I did something wrong in that regard, let me know.

package com.kestrel.test;

import java.util.ArrayList;
import java.util.Random;

public class ShuffleTest
{
    public static <T> ArrayList<T> shuffle(ArrayList<T> a)
    {
        Random rand = new Random();

        for(int n = a.size() - 1; n > 0; n--)
        {
            int index = rand.nextInt(n + 1);
            T temp = a.get(index);

            a.set(index, a.get(n));
            a.set(n, temp);
        }
        return a;
    }

    public static <T> T[] shuffle(T[] a)
    {
        Random rand = new Random();

        for(int n = a.length - 1; n > 0; n--)
        {
            int index = rand.nextInt(n + 1);
            T temp = a[index];

            a[index] = a[n];
            a[n] = temp;
        }
        return a;
    }
}

I know that I would have to implement methods for all of the primitive array types separately, but I'm short on time, and I don't think autoboxing will slow down my code all that much.

You are both mutating & returning the value you are passing to the function.  Pick one.   Either return void or return a new array/ArrayList without mutating the input.

Your first shuffle() method requires an ArrayList, yet any Collection which implements the List interface would work, and will work well if it has \$O(1)\$ get & set complexity.  So consider loosening the type from a concrete type to the List interface.  (It will even work for LinkedList, albeit with horrible performance, but working slowly is arguably better than not being able to work at all.)

The value returned by ArrayList<T>::set(int idx, T obj) is the previous contents of that location.  Therefor, the temporary is not necessary.

        T temp = a.get(index);
        a.set(index, a.get(n));
        a.set(n, temp);

can become:

        a.set(n, a.set(index, a.get(n)));

or more clearly, just Collections.swap(a, n, index);.

Similarly,

        T temp = a[index];
        a[index] = a[n];
        a[n] = temp;

can also become Collections.swap(a, n, index);, a similar function which takes a T[] instead of a List<?> as the first argument.

Here is an implementation of your "something like K<T> using generics" from the comments.  And no longer coding from the hip, so the Java syntax is actually correct.

MacBook-Pro:~ aneufeld$ jshell
|  Welcome to JShell -- Version 10.0.1
|  For an introduction type: /help intro

jshell> public class ShuffleTest {
   ...>   public static <T,K extends List<T>> K shuffle(Collection<T> a, Supplier<K> supplier) {
   ...>     K dup = supplier.get();
   ...>     dup.addAll(a);
   ...>     Collections.shuffle(dup);  // Or use your shuffle implementation
   ...>     return dup;
   ...>   }
   ...> }
|  created class ShuffleTest

jshell> var orig = List.of("Hello", "world");
orig ==> [Hello, world]

jshell> ArrayList<String> shuffled = ShuffleTest.shuffle(orig, ArrayList<String>::new);
shuffled ==> [world, Hello]

jshell>


Refactoring a loop with many boolean conditions I usually spent more time on refactoring than in developing itself. But this little piece of code is really hard to refactor. I'd like to keep it as short as possible, but it's quite undarstandable:

foreach($all_headers as $uid => $h) // Intera gli headers
{

    // Is the header valid?
    $isInbox   = $h->to == $mailbox->address;
    $hasPin    = ($pin = substr($h->subject, 0, 5)) !== false && is_numeric($pin);
    $hasTitle  = strlen($title = trim(substr($h->subject, 5))) > 0;
    $isAllowed = $hasPin && call_user_func($pincall, $mailbox->address, $h->from, $pin);

    $log->info("messaggio $uid: in:$isInbox pin:$hasPin alw:$isAllowed");

    // Skip this header if not vlaid
    if (!$isInbox || !$hasPin || !$isAllowed) continue;

    // Add some properties to current header
    $h->title = $hasTitle ? $title : null;
    $h->pin   = $pin;
    $valid_headers[$uid] = $h;

}

Since i'm filtering $all_headers array, one cool idea would be using array_filter with some kind of mechanism to add/remove conditions "on the fly".

Thank you people, as always.

This won't make it shorter, but you might consider changing the condition checking to only do the actual check if needed instead of doing all of them at once, making you loose the advantage of the short-circuiting || operator.

E.g.

if (!isInbox()) continue;
if (!hasPing()) continue;
...

Another possibility, that is even less short, is to create a filtering iterator, e.g.:

class ValidHeaderIterator extends FilterIterator
{
    private $mailbox;
    private $pincall;

    public function __construct($headers, $mailbox, $pincall)
    {
        $this->mailbox = $mailbox;
        $this->pincall = $pincall;
        parent::__construct(new ArrayIterator($headers));
    }

    public function accept()
    {
        $header = $this->getInnerIterator()->current();
        return isInbox($header) && hasPin($header) && isAllowed($header);
    }

    public function isInbox($header)
    {
        return $header->to == $this->mailbox->address;
    }

    public function hasPin($header)
    {
        $pin = $this->getPin($header);
        return $pin !== false && is_numeric($pin);
    }

    public function getPin($header)
    {
        return substr($header->subject, 0, 5));
    }

    public function isAllowed($header)
    {
        return call_user_func($this->pincall,
                              $this->mailbox->address,
                              $header->from,
                              $this->getPin());
    }
}

The calling code would then look like:

foreach(new ValidHeaderIterator($headers, $mailbox, $pincall) as $header) {
    // do stuff
}


Two CSS grids side by side, one is scrollable, the other isn't I've made a section of my site to have two grids side by side. On the left-hand side is one big static (sticky in my case) image, on the right-hand side is a (grid) long column of several other images, that is scrollable. So as the user scrolls the grid on the right-hand side of the page, the grid on the left remains 'stuck' until the user has reached the end of the grid on the right.

My code below works, but I was wondering if there a more elegant way of doing this?

html,
body,
p,
ol,
ul,
li {
  background-color: #1A1A1A;
  margin: 0;
  padding: 0;
}

.container {
  height: 100vh;
  width: 95%;
  margin: auto;
  position: relative;
}

#tattoos {
  width: 95%;
  display: flex;
  justify-content: space-between;
  height: 2000px;
  position: relative;
}

#sectionOne {
  display: grid;
  height: 100vh;
  grid-template-rows: repeat(1, 1fr);
  grid-template-columns: repeat(1, 1fr);
  grid-gap: 15px;
  width: 60%;
  position: sticky;
  top: 0;
}

.gallery-item {
  display: flex;
  justify-content: center;
  align-items: center;
  opacity: 1;
  text-transform: uppercase;
  font-family: 'Lora';
  font-weight: 700;
  color: white;
  font-size: 9vw;
}

#image-one {
  background-image: url("https://i.ibb.co/HXfG735/tattoo.jpg");
  background-size: cover;
  background-position: 0% 80%;
  box-shadow: inset 0 0 0 2000px rgba(0, 0, 0, 0.8);
  margin: 4% 0;
}

.sectionTwo {
  width: 37%;
  display: grid;
  height: 1940px;
  grid-template-rows: repeat(7, 1fr);
  grid-template-columns: repeat(1, 1fr);
  grid-gap: 30px;
  margin: 2.5% 0;
}

#image-two {
  background-image: url("https://i.ibb.co/kBJrryt/7ab34470-9318-4fc1-a6da-656ca31399c5.jpg");
  background-size: cover;
  background-position: 0% 80%;
}

#image-three {
  background-image: url("https://i.ibb.co/SKCnT4d/b19aaf2a-84cf-42b4-9ccd-6205ca1be395.jpg");
  background-size: cover;
  background-position: 0% 50%;
}

#image-four {
  background-image: url("https://i.ibb.co/xF4Qv50/b5d6695b-2142-42cc-8eef-2963311edfd6.jpg");
  background-size: cover;
  background-position: 0% 60%;
}

#image-eight {
  background-image: url("https://i.ibb.co/0nY3VDG/d1ecf205-6b1b-4a1a-b94b-7d75aa177464.jpg");
  background-size: cover;
  background-position: 0% 60%;
}

#image-nine {
  background-image: url("https://i.ibb.co/QYf6vDY/48a63fe2-066b-42d1-9211-040c6977ceff.jpg");
  background-size: cover;
  background-position: 0% 40%;
}

#image-ten {
  background-image: url("https://i.ibb.co/7RdJqgP/ce30fff2-1679-403f-8431-c6be4f8b1466.jpg");
  background-size: cover;
  background-position: 0% 80%;
}

#image-twelve {
  background-image: url("https://i.ibb.co/BtJxDc7/24b54530-710f-4173-a32a-36bd24a496e0.jpg");
  background-size: cover;
  background-position: 0% 50%;
}

      tatujes

Although this is not a full answer It will shorten your code,
You could remove 'background-size:cover;' & replace it with a class of bGCov,
(As your declaring this multiple times & if your application grows larger With many more Images, this will save many, many lines of code)
Aswell as Use Bitly URL shortener, to shorten all of your Image URL links, make sure to name them something farely short but related to the actual image as it makes keeping track of them easier later on.
Also if you started using scss background-position and a few other things could be made into variables.
this would shorten you code for a few lines...


Model class representing a calculator I have a simple Model class that represents a calculator.

Header file:

@class MMCalculator;

@protocol MMCalculatorDelegate <NSObject>

@required - (void)mmCalculator:(MMCalculator *)mmCalulatorFinishedCalculatingValues;

@end

@interface MMCalculator : NSObject

@property (nonatomic, weak) id <MMCalculatorDelegate> delegate;
@property float calculatedPay;
@property float calculatedSavingsForStuff;
@property float calculatedSavingsForProfitFormula;
@property float calculatedSavingsForTaxes;

- (void)calculateValuesWithMonthlyRevenue:(float)monthlyRevenue;
- (float)calculateYourPay:(float)monthlyRevenue;
- (float)calculateSavingsForStuff:(float)monthlyRevenue;
- (float)calculateSavingsForProfitFormula:(float)monthlyRevenue;
- (float)calculateSavingsForTaxes:(float)monthlyRevenue;

@end

Implementation file:

#import "MMCalculator.h"

#pragma mark - Static Variables
static float const percentageToPayYourself = 0.50f;
static float const percentageToSaveForStuff = 0.20f;
static float const percentageToSaveForProfitFormula = 0.20f;
static float const percentageToSaveForTaxes = 0.10f;

@implementation MMCalculator

#pragma mark - Calculator Methods
- (void)calculateValuesWithMonthlyRevenue:(float)monthlyRevenue
{
    //Call all 4 calculation methods and set calculated properties
    self.calculatedPay = [self calculateYourPay:monthlyRevenue];
    self.calculatedSavingsForStuff = [self calculateSavingsForStuff:monthlyRevenue];
    self.calculatedSavingsForProfitFormula = [self calculateSavingsForProfitFormula:monthlyRevenue];
    self.calculatedSavingsForTaxes = [self calculateSavingsForTaxes:monthlyRevenue];

    //Call delegate method
    [self.delegate mmCalculator:self];
}
- (float)calculateYourPay:(float)monthlyRevenue
{
    return percentageToPayYourself * monthlyRevenue;
}

- (float)calculateSavingsForStuff:(float)monthlyRevenue
{
    return percentageToSaveForStuff * monthlyRevenue;;
}

- (float)calculateSavingsForProfitFormula:(float)monthlyRevenue
{
    return percentageToSaveForProfitFormula * monthlyRevenue;
}

- (float)calculateSavingsForTaxes:(float)monthlyRevenue
{
    return percentageToSaveForTaxes * monthlyRevenue;
}

@end

Here are the key things I am looking for feedback on:

Right now I store the class's calculated values in public properties so they can be accessed by other classes. Should it be the class's responsibility to store these calculated values in it's properties, or should it be the responsibility of the class that is calling this class's methods?.
Did I define my static variables correctly? Are they following best practices?
What do you think of the class's delegate method?
Am I following best practices by using floats to represent money? I have a feeling that I should be using something else.

Let's address your questions:

I don't think that storing the calculated values in public properties is a good idea, mainly because If I call calculateValuesWithMonthlyRevenue with say, 1000, and then afterwards call calculateYourPay with 400, the other properties don't change to reflect my different monthly revenue. Therefore, I'm not sure why these should be stored as properties at all. I would eliminate calculateValuesWithMonthlyRevenue, and make this class a singleton with only static methods.
In most cases, static constants are written in ALLCAPS or ALL_CAP, depending on your prefered style. I would define these in your header file (if you want these values to be public) or in your implementation file (if you want the values to be private) with #define instead, but that's a matter of personally preference.
You don't show us what you do with this delegate method, so I can't exactly be sure what the point of it is. I would omit it for now.
Using floats is a bit dangerous as it lends itself to annoying floating-point errors such as .3 turning into .299999999. This is really a question of desired precision. If you're fine with being off by a bit, then floats are pretty decent. If not, you will have to define your own type to represent money.


2D model simulating water breaking a floodbank For a homework assignment we have to write a class that simulates water breaking a floodbank (very simplistic 2D simulation).

My implementation works fine but I found that there were many downcasts (in my opinion, a flaw in Greenfoot's API, which should be using generic functions rather than functions that take classes as arguments, but I'm not sure if this is possible in Java since this is the first time I'm writing Java code).

Sandstone and Sand both derive from Actor, which is from Greenfoot.

import greenfoot.*;

public class Water extends Actor {
    public Water() {
        getImage().scale(64, 64);
    }

    public void act() {
        if (((DoorbraakWorld)getWorld()).stopped) return;

        if (Math.random() < 0.9) return;

        int dx = 0, dy = 0;
        switch ((int)(3.0 * Math.random())) {
            case 0: dx = 0; dy = 1; break;
            case 1: dx = 1; dy = 0; break;
            case 2: dx = -1; dy = 0; break;
        }

        // The next line looks so incredibly ugly…
        Sandstone sandstone = (Sandstone)getOneObjectAtOffset(dx, dy, Sandstone.class);
        if (sandstone != null) {
            getWorld().removeObject(sandstone);
            getWorld().addObject(new Water(), getX() + dx, getY() + dy);
        }

        Sand sand = (Sand)getOneObjectAtOffset(0, 1, Sand.class);
        if (sand != null) {
            ((DoorbraakWorld)getWorld()).stopped = true;
            return;
        }
    }
}

Could I reduce the number of downcasts or is it normal for Java code to contain many downcasts? What else could I improve?

Downcasting can usually be avoided by using common methods in abstract types.

    if (((DoorbraakWorld)getWorld()).stopped) return;

Is this the same as getWorld().stopped()?

    // The next line looks so incredibly ugly…
    Sandstone sandstone = (Sandstone)getOneObjectAtOffset(dx, dy, Sandstone.class);
    if (sandstone != null) {
        getWorld().removeObject(sandstone);
        getWorld().addObject(new Water(), getX() + dx, getY() + dy);
    }

You can use the Actor type: Actor sandstone = getOneObjectAtOffset(dx, dy, Sandstone.class)

    Sand sand = (Sand)getOneObjectAtOffset(0, 1, Sand.class);

Here too: Actor sand = getOneObjectAtOffset(0, 1, Sand.class);

    ((DoorbraakWorld)getWorld()).stopped = true;

How about greenfoot.stop()?


Filtering an array of objects The code does the following:

Gets an array of objects and filters them according to some properties
Returns the array of objects with the filtered data

I'm using the following code which is working and I wonder if there is a way to write it better.

By the way, I can use LodashJS if it's helpful here.

filterNullUndefinedOrRejected: function(oData) {
    if (oData && oData.length > 0) {
        var aNewExtendedObjects = [];
        for (var i = 0; i < oData.length; i++) {
            if (oData[i].va === null || oData[i].val === undefined || oData[i].state === "rejected") {
                if (oData[i].state === "rejected") {
                      console.log(oData[i].error);
                }
                oData.splice(i, 1);
                i--;
            }
            else{
                aNewExtendedObjects.push(oData[i].val);
            }
        }
        return aNewExtendedObjects[0];
    }
    return oData;
},

splice mutates the existing array. You wouldn't want to do that as you might not be sure where else the array is being used. You might be expecting values in the array after calling this function, yet this function wiped them off.

Now if you're really sure you want to use splice to remove items in an array, loop backwards. The ending i-- isn't very visible and missing it would mean skipped items.

Lodash isn't necessary as you have the native array.filter. This is unless you still plan to support browsers that don't do ES5 (like IE8 and older).

Your function isn't consistent. The ideal return is an array. But if I put in a falsy value, it returns that value, not an array.

The length check is meaningless. An empty array has the length of 0 which means the loop will never run at all.

Your function is essentially just including all values that aren't null, undefined or have a state of rejected. However, the name is misleading. You never explicitly mention that you're only getting the val properties as a result. You should name your function explicitly, or split off the operations separately.

As noted in the comments (and it wasn't apparent in the code), you're collecting the val property of the filtered items. This can easily be done with an array.map. So after filtering down to the items we do need, we transform the values to the ones we need.

filterNullUndefinedOrRejected: function(oData){
  return !oData ? [] : oData.filter(function(oDatum){
    if(oDatum.state === 'rejected') console.log(oDatum.error);
    return oDatum.val !== null && oDatum.val !== undefined && oDatum.state !== "rejected";
  }).map(function(oData){
    return oData.val;
  });
}


Client's method for channel subscribing I am writing an open-source application for real-time messaging in web. In my application clients from browser can subscribe on channels. But every channel belongs to a certain namespace which determines channel's settings:

client.subscribe('/football/news', function(message) {
    // message from channel received
});

where football is namespace name and news is channel name. football can be a default namespace for project and in this case we can write in this way:

client.subscribe('/news', function(message) {
    // message from channel received
});

i.e. without namespace name. This is how it works right now.

But my question is about / path separator. Is it ok? We need the way do separate namespace name and channel name. / usage was influenced by Bayeux protocol spec. But maybe it would be more simple and correct to write in such manner:

client.subscribe('football', 'news', function(message) {
    // message from channel received
});

or with default namespace:

client.subscribe(null, 'news', function(message) {
    // message from channel received
});

or even:

client.subscribe('news', function(message) {
    // message from channel received
});

I personally feel that the second way is better. But before refactoring I decided to ask for your opinion.

An opinion question, always tough to answer.

I like the first way better, 1 less parameter and it is easier to grok for me.

Incidentally, ABAP uses forward slashes for namespaces as well.


Creating a sorted hash counting the occurrences of a number in an array I figured something like this:

a= [1, 2, 3, 2, 2, 2, 3, 1, 1, 1, 2, 4, 4, 4, 4, 4, 4, 4, 4, 4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
a.inject({}) { |h, el| h[el] = a.count(el) if h[el].nil? ; h }.sort_by {|k,v| v}.reverse.inject({}) { |h, el| h[el[0]] = el[1] ; h}

This outputs:

=> {0=>10, 4=>9, 2=>5, 1=>4, 3=>2}

But it seems like a clunky solution that does to many passes over the "same" thing.

What would be a good way to do this from different viewpoints?

I.e:

an "impress your Java friends" (small one-liner not used in production)
Performance
Easily read

Not sure which viewpoint this would be, but this is what I came up with:

Hash[a.group_by {|e| e}.map {|k,v| [k, v.length]}]

Update: I'm not thrilled with this, but this one sorts by the occurrence count, descending.

Hash[a.group_by {|e| e}.map {|k,v| [k, v.length]}.sort_by {|a| a[1]}.reverse]


Infinite list of strings containing 'a' I was assigned to write a piece of Haskell code that makes an infinite list containing increasing numbers of 'a's.

My first thought was to write it as a list comprehension like this:

aStar = [replicate n 'a' | n <- [0..]]

But when I asked my professor he said I should use explicit recursion so I came up with this:

aStar :: [String]
--repeats a string from 0
aStar = repeat' 0 'a'

repeat' :: Int -> Char -> [String]
repeat' n x = [cycle' x n] ++ repeat' (n + 1) x
    where cycle' _ 0 = ""
          cycle' y z = [y] ++ cycle' y (z - 1)

Is there a shorter way to do it?

Both of your solutions have efficiency issues, but at least your original list comprehension has the advantage of being short and readable. Your revised solution is arguably worse.

The problem with replicate is that each string is rebuilt from scratch.

The problem with the second solution is that you are using ++, which appends to the end of a list. That requires traversing to the end of the list, which gets more and more time-consuming as the lists get longer. Anytime you write ++, you should try hard to find a better approach.

This is the solution that your professor probably had in mind (note the use of : rather than ++):

aStar :: [String]
aStar = aStar' 'a' ""
  where
    aStar' c s = s : aStar' c (c : s)


Selecting and copying event log entries for some machine I have a PL/pgSQL function with erratic performance:

DECLARE
l RECORD;
events_for_machine integer;
before_event "PRD".events_log;
machines_ids integer[];
island_controller RECORD;
before_order "PRD".events_log;
before_detail "PRD".events_log;
before_pallete "PRD".events_log;
before_operation "PRD".events_log;
timer timestamp;
timer2 timestamp;

BEGIN
machines_ids = string_to_array(machines_ids_g,',')::integer[];
for l in
select m.*
from
    "PRD".machines m
    inner join
    unnest(machines_ids) n(id) on n.id = m.id
where
    m.start_work_date < begin_date_g
order by m.id
LOOP

SELECT * INTO island_controller FROM "STRUCT".island_machines WHERE machine_id=l.id;

RAISE NOTICE 'pobieram zdarzenie before dla maszyny %',l.id;

    SELECT * INTO before_event FROM "PRD".events_log WHERE plc_time < begin_date_g AND (((event_type_id IN (1,51) AND machine_id = island_controller.controller_id AND island_id = island_controller.island_id))
    OR (event_type_id IN (2000,2001) AND machine_id=l.id)) ORDER BY plc_time DESC LIMIT 1;

IF before_event.plc_time IS NOT NULL THEN

RAISE NOTICE 'Getting info about first machine work time struct element';
RETURN QUERY SELECT * FROM "PRD".events_log WHERE event_type_id = 113 AND machine_id=l.id AND plc_time < before_event.plc_time ORDER BY plc_time DESC LIMIT 1;
RETURN QUERY SELECT * FROM "PRD".events_log WHERE event_type_id = 102 AND machine_id=l.id AND plc_time < before_event.plc_time ORDER BY plc_time DESC LIMIT 1;
RETURN QUERY SELECT * FROM "PRD".events_log WHERE event_type_id = 111 AND machine_id=l.id AND plc_time < before_event.plc_time ORDER BY plc_time DESC LIMIT 1;
RETURN QUERY SELECT * FROM "PRD".events_log WHERE event_type_id = 1010 AND machine_id=l.id AND plc_time < before_event.plc_time ORDER BY plc_time DESC LIMIT 1;
RETURN NEXT before_event;

END IF;
RAISE NOTICE 'generuje zdarzenia wlasciwe dla maszyny %',l.id;
    RETURN QUERY SELECT * FROM "PRD".events_log WHERE
            (event_type_id = ANY ('{1,51}'::integer[]) AND (machine_id=island_controller.controller_id AND island_id = island_controller.island_id) AND (plc_time BETWEEN begin_date_g AND end_date_g))
            OR (event_type_id = ANY ('{2000,2001,107}'::integer[]) AND machine_id=l.id AND (plc_time >= begin_date_g AND plc_time <= end_date_g))
            OR ((event_type_id = ANY ('{101,102,103,301,1010}'::integer[]) OR ((event_type_id >= 5000) AND (event_type_id <= 5999))) AND machine_id=l.id AND plc_time >= begin_date_g AND plc_time <= end_date_g) ORDER BY plc_time;

RAISE NOTICE 'koniec dla maszyny %',l.id;
END LOOP;

END;

Sometimes the function execution time is ~9 seconds and sometimes ~40 seconds for the same arguments.

What does it depend on?  What could be so inefficient?

I find this query difficult to read to begin with. Here are my comments:

When you DECLARE a bunch of variables it is useful to name them such that they will be easy for another programmer to identify throughout the script. For example adding v_ in front of them is a good way. Also calling a variable just l is pretty cryptic.
select * is sometimes not a good idea; that is, unless you actually need all of the columns. I see you use that a lot and it could be part of your performance issues. Part of why it may be problematic is also that your result set is unpredictable, if a column is added or dropped for instance, or renamed, your query would not be repeatable. It's better to get a "column does not exist" error than get unpredictable result set. Be explicit when you can.
The lack of proper indenting in your script, as well as the lack of a proper explanation in your question (or as comments within your script), in addition to some of it being in Polish it seems, makes it very difficult to decrypt what you are doing here unless you provide more information.
You have lots of variables, most of them holding lots of data (select * into etc.).  This likely is affecting performance. Multiple levels of nested subqueries can also slow it down.


Simon game / Four tiles game When I read the community challange, I couldn't wait. So here I go with the very first Four tiles game:

import pygame as pg
import sys
import random
import time
import tkMessageBox

if sys.version_info.major == 3:
    import tkinter as tk
else:
    import Tkinter as tk

from pygame.locals import *

def game(LENGHT):
    pg.init()
    pg.display.set_caption("Four tiles game")

    WIDTH = 600
    HEIGHT = 400
    DISPLAY = pg.display.set_mode((WIDTH, HEIGHT), 0, 32)

    WHITE = (255, 255, 255)
    RED = (255, 0, 0)
    green = (0, 255, 0)
    blue = (0, 0, 255)
    yellow = (255, 255, 0)
    COLORS = [RED, blue, green, yellow]

    WAITING_TIME = 500

    def generate_correct_sequence(length):
        return [random.randint(1, 4) for _ in range(length)]

    def draw_button(n, color):
        if n == 1:
            pg.draw.rect(DISPLAY, color, (0, 0, WIDTH / 2, HEIGHT / 2))
        if n == 2:
            pg.draw.rect(DISPLAY,color,
            (WIDTH / 2, 0, WIDTH / 2, HEIGHT / 2))
        if n == 3:
            pg.draw.rect(DISPLAY,color,
            (0, HEIGHT / 2, WIDTH / 2, HEIGHT / 2))
        if n == 4:
            pg.draw.rect(DISPLAY,color,
            (WIDTH / 2, HEIGHT / 2, WIDTH / 2, HEIGHT / 2))

    def draw_buttons():

        for i in range(1, 5):
            draw_button(i, COLORS[i - 1])

    def what_button_is_clicked(mouse_pos):
        """
        The buttons are numbered in the following way
            1 | 2
            _____

            3 | 4
        """
        if mouse_pos[0] < WIDTH / 2 and mouse_pos[1] < HEIGHT / 2:
            return 1

        if mouse_pos[0] > WIDTH / 2 and mouse_pos[1] < HEIGHT / 2:
            return 2

        if mouse_pos[0] < WIDTH / 2 and mouse_pos[1] > HEIGHT / 2:
            return 3

        elif mouse_pos[0] > WIDTH / 2 and mouse_pos[1] > HEIGHT / 2:
            return 4

    def flash(button_number, waiting_time):
        draw_button(button_number, WHITE)
        pg.display.update()
        pg.time.wait(waiting_time)
        draw_button(button_number, COLORS[button_number - 1])
        pg.display.update()

    def inform_user(correct_sequence, delay):
        for button in correct_sequence:
            flash(button, delay)

    draw_buttons()
    correct_sequence = generate_correct_sequence(LENGHT)
    inform_user(correct_sequence, WAITING_TIME)

    while True:
        for event in pg.event.get():
            if event.type == QUIT:
                pg.quit()
            if event.type == pg.MOUSEBUTTONUP:
                mouse_pos = pg.mouse.get_pos()
                if what_button_is_clicked(mouse_pos) == correct_sequence[0]:
                    correct_sequence.pop(0)
                else:
                    tkMessageBox.showinfo("LOSE", "You are the loser...")
                    pg.quit()

        if correct_sequence == []:
            tkMessageBox.showinfo("WIN", "You are the winner!!!")
            pg.quit()

if __name__ == "__main__":
    root = tk.Tk()
    root.wm_title("Four tiles game")

    def play():
        game(w.get())

    INTRODUCTION = tk.StringVar()
    INTRODUCTION.set("""Welcome to the four tiles game.
You will see some tiles light up one after the other.
Afterwards, you must click them in the same order they lit up before.""")
    tk.Message(root, textvariable=INTRODUCTION, width=500, font=30).pack()

    tk.Label(root, text="Select the lenght down here", font=30).pack()
    w = tk.Scale(root, from_=1, to=15, orient=tk.HORIZONTAL, length=400)
    w.pack()

EDIT: If you want to look at the game with added functionalaties and features and with nicer style go to this GitHub repo.

Missing the specs

You're missing a very important rule of the game:

  The pattern gets longer each time the player completes the pattern. If the player presses a wrong button, the game ends.

You implemented only a one-shot round, which is not so interesting.

Mutually exclusive conditions

These conditions are mutually exclusive:

if n == 1:
    # ...
if n == 2:
    # ...
if n == 3:
    # ...

So you should use elif for the 2nd and later conditions.

Use consistent names

It's odd to name some constants with all caps, and others with all lowercase like this:

WHITE = (255, 255, 255)
RED = (255, 0, 0)
green = (0, 255, 0)
blue = (0, 0, 255)
yellow = (255, 255, 0)

Be consistent, name all of them all caps!

Avoid hard-coding

This function relies on having 4 buttons:

def generate_correct_sequence(length):
    return [random.randint(1, 4) for _ in range(length)]

It would be better to avoid hardcoding the number 4.
You could for example use len(COLORS) instead.
Even better would be to use a dedicated constant,
and derive both the elements of colors from it,
and use it everywhere else where it's relevant, as in this function.

Use local variables to explain logic

In this code:

if mouse_pos[0] < WIDTH / 2 and mouse_pos[1] < HEIGHT / 2:
    return 1

if mouse_pos[0] > WIDTH / 2 and mouse_pos[1] < HEIGHT / 2:
    return 2
# ...

mouse_pos[0] and mouse_pos[1] are repeated many times,
which is tedious, and not as clear as it can be.
I don't actually have tkinter now,
but I'm guessing these stand for x and y coordinates.

I would either change the method to take parameters named x and y,
or introduce the local variables x and y early in the method:

x = mouse_pos[0]
y = mouse_pos[1]

The rest of the code would become shorter, and clearer.

Why not use 0-base indexing everywhere?

This method is slightly more complex than it needs to be due to the 1-based indexing:

def draw_buttons():
    for i in range(1, 5):
        draw_button(i, COLORS[i - 1])

I bet you could change to this simpler implementation,
and adjust the rest of the code to work with it without sacrificing anything:

def draw_buttons():
    for i in range(4):
        draw_button(i, COLORS[i])

Notice the hardcoded number 4 again. This highlights again the importance to avoid hardcoding. The range(1, 5) came from the fact of the hard limit 4,
and if you decided to change 4 at some point, searching in the code for "4" you would never find range(1, 5).

So, really, replace the number 4 with a constant. len(COLORS) may look tempting, but as I explained above, I suggest to use something else as the single authoritative point of control.


Sorting the array of strings on their lengths in Java , Using Maps, ascending order.... I wrote the java method to answer this question :
Write a Program to sort String on their length in Java? Your method should accept  an array of String and return a sorted array based upon the length of String

So my questions:
1. Is my method efficient enough?
2. Can it be improved somehow?

 private static String[] sort(String [] string) {
    /*Local variables*/
    Map<String, Integer> map=new LinkedHashMap<String, Integer>();
    Map<String, Integer> mapCopy=new LinkedHashMap<String, Integer>();
    int [] lengthsArray=new int[string.length];
    String [] sortedStrings=new String[string.length];
    int counter1=0;
    int counter2=0;

    /* Store all the pairs <key,value>
     * i.e <string[i],string[i].length>
     */
    for(String s:string){
        map.put(s, s.length());
        lengthsArray[counter1]=s.length();//store all the lengths
        counter1++;

    }
    mapCopy=new LinkedHashMap<String, Integer>(map);//make a copy of map
    Arrays.sort(lengthsArray);//sort the array of lengths

    /*
     * Sort array according to the array of lengths
     * by finding the matching value from the map
     * then add it to the final string array,and then remove it from the map
     */
    for(int item:lengthsArray){

        for(Map.Entry<String, Integer> e:map.entrySet()){
            if(item==e.getValue()){
                sortedStrings[counter2]=e.getKey();
                counter2++;
                map.remove(e.getKey());
                break;
            }
        }
    }

    map=mapCopy;
    System.out.println(map);//print map
    return sortedStrings;
}

I think you can use the Arrays.sort with Comparator to make this simple. just Override the comparator to compare the length of the array like this

public static String[] sortStrArray(String[] array){
   //sort arrays by length before returning
    Arrays.sort(array, new Comparator<String>() {
        @Override
        public int compare(String a, String b) {
            return Integer.compare(a.length(),b.length());//specifying compare type that is compare with length
        }
    });
     return array;
}

///Main Method here

String[] str = {"James","Ana","Michael","George","rose"};
    System.out.println(Arrays.toString(sortStrArray(str)));

Output:

  [Ana, rose, James, George, Michael]

with Java 8

 public static String[] sortStrWithLam(String[] array){
    //sort arrays by length before returning
    Arrays.sort(array,(a,b) -> Integer.compare(a.length(),b.length()));
    return array;
}


Further Optimizing a Segmented Sieve of Eratosthenes I've implemented a Segmented Sieve of Eratosthenes that assumes each segment is about length \$ \sqrt{N}\$ for some upper limit \$N\$.

I further optimized the function by skipping all the evens when sieving.

I'd like to know if anyone has any views or advice on further optimizing my implementation.

Note that there are three functions: the first, "SieveEratList" is a standard Sieve of Eratosthenes (to calculate primes in range \$ [1, \sqrt{N}] \$); the second, "find_first_multiple_odds", checks to find the initial index value in an interval of odd numbers (since we're skipping evens); and the third, "OptiSegment," is the optimized Segmented Sieve that calls the previous two when needed.

I'm open to any suggestions, whether it's space optimization or runtime optimization.

def SieveEratList(n):
    numbers = [False]*2+[True]*(n-2)
    result = []
    for index, prime_candidate in enumerate(numbers):
        if prime_candidate:
            result.append(index)
            for x in range(index*index, n, index):
                numbers[x] = False
    return result

def find_first_multiple_odds(p, low):
    # O(1)
    x = p*((low/p)+(low%p != 0))
    x += p*(x%2 == 0)
    return ((x-low)/2)

def OptiSegment(high):
    delta = int(high**0.5)+1*(int(high**0.5)%2)

    seed_primes = SieveEratList(delta+1)
    #boolean operator checks see if "high" is square
    segment_count = int(high/delta) - 1*((high**0.5)-int(high**0.5) == 0.0)

    for seg in range(1, segment_count+1):
        low_val = seg*delta+1
        top_val = delta*(seg+1)-1

        candidates = list(range(low_val, top_val+1, 2))
        prime_limit = top_val**0.5+1

        Q = iter(seed_primes)
        p = next(Q)
        p = next(Q)

        while p < prime_limit:
            q = find_first_multiple_odds(p, low_val)

            for i in range(q, delta/2, p):
                candidates[i] = False
            p = next(Q)

        seed_primes += [x for x in candidates if (x and x < high)]

    return seed_primes

Python 3.8 adds math.isqrt().  Using this, you can make a much less cryptic perfect square check:
high == math.isqrt(high) ** 2

Memory Optimization (speed too!)
Using a list of Boolean flags:
numbers = [False]*2 + [True]*(n-2)

is very space inefficient.  You are using 28 bytes per flag!  By packing 8 flags into one byte, you’d achieve a 99.5% memory usage reduction.  Doing this packing yourself would be slow and awkward.  Fortunately, there is a package that does this for you: bitarray.
numbers = bitarray.bitarray(n)
numbers.setall(True)
numbers[0:2] = False

Notice that slice assignment from a scalar Boolean?  It gets better when you use it to cross off the multiples of a prime candidate:
if prime_candidate:
    result.append(index)
    numbers[index*index::index] = False

and done!

Integer division
You should really use integer division // in find_first_multiple_odds():
x = p * ((low // p) + (low % p != 0))
...
return (x - low) // 2

PEP-8

use snake_case function names.  UpperCase is for class names
use a space around binary operators


Design pattern for adapter I wrote a framework and I want to hear the thoughts of other on the design patterns it uses. Every method contains three design patterns - adapters(strategy), intercepting filters, and observers.

A normal class/method in a class looks like this:

class Run extends PVStaticObject{

    public static function goForARun($miles) {

        $return ='I ran '.$miles. ' miles today';

        return $return;
    }
}

With the design patterns the class/method looks like this:

class Run extends PVStaticObject{

    public static function goForARun($miles) {

        if (self::_hasAdapter(get_class(), __FUNCTION__))
            return self::_callAdapter(get_class(), __FUNCTION__, $miles);

        $miles = self::_applyFilter(get_class(), __FUNCTION__, $miles, array('event' => 'args'));

        $return ='I ran '.$miles. ' miles today';

        self::_notify(get_class() . '::' . __FUNCTION__, $miles, $return);
        $return = self::_applyFilter(get_class(), __FUNCTION__, $return, array('event' => 'return'));

        return $return;
    }
}

A brieft explanation, the adapter will completely change the method by calling another method in its place. Its a way of changing the functionality of a class without modifying the core functionality.

Filters modify variables in a method by passing them out to another class's method or anonymous function where they are modified and return. Normal execution of the method continues.

Observers do not have a return and are purely call another class or anonymous function. They are for event drivin programming.

Examples of adding adapters, filters and observers are below.

Run::addObserver('Run::goForARun', 'run_observer', function($miles, $return){
    echo PVHtml::div('Running '. $miles. ' has caused you to lose 2 pounds', array('style' => 'margin-top:10px;'));
}, array('type' => 'closure'));

Run::addAdapter('Run','goForARun', function($miles){
    echo PVHtml::p('Because of the weather, you were not able to run '.$miles. ' today');
}, array('type' => 'closure'));

Run::addFilter('Run', 'goForARun', 'run_filter', function($data, $options) {

    $data = PVHtml::strong($data);
    $data = PVHtml::p($data);

    return $data;

}, array('type'=> 'closure', 'event' => 'return'));

So what is thoughts and feedback on the design patterns? They are meant to faciliate aspect oriented and event driven design. They also to replace design patterns like dependency injection easily.

PS: More examples of AOP here.

You should relearn design patterns.  Until you do this, your framework is bound to be messed up and poorly architected, if only because of your flawed axioms.

The Adapter Pattern is for taking class A implements InterfaceA and wrapping it inside a class Adapter so that you can use it with the same API as class B implements InterfaceB. It has nothing to do with executing different code or whatever inside another class.

The Strategy Pattern is meant to reduce cyclomatic complexity (basically, lots of nested if statements). It works like this:

interface BasicLogicI {
    public function execute();
}

class GreetingLogic implements BasicLogicI {
    public function execute() { echo "Hi!\n"; }
}

class DismissLogic implements BasicLogicI {
    public function execute() { echo "Bye!\n"; }
}

class Speaker {
    protected $strategy;

    public function __construct($strategy) {
        $this->changeContext($strategy);
    }
    public function changeContext($strategy) {
        unset($this->strategy);
        $this->strategy = $strategy;
    }
    public function speak() {
        $this->strategy->execute();
    }
}

$speaker = new Speaker(new GreetingLogic);
$speaker->speak();
$speaker->changeContext(new DismissLogic);
$speaker->speak();
// Output: Hi!
//         Bye!


Returning the last segment of a split string I recently reviewed a PR and saw the following.
const parts = someString.split('.');
return parts[parts.length - 1];

was changed to
const [last] = someString.split('.').reverse();
return last;

I commented that I am against the change for the reasons that it is harder to read and not performant.
The answer was that it is only hard to read, because I am not used to it and that performance does not matter if you use big frameworks like React for example.
Who is right and why?

I'm not entirely enthusiastic about either. The parts[parts.length - 1] using manual index lookup and subtraction may well take a moment to recognize, "Oh, this is getting the last element of the array". The second, using .reverse() followed by destructuring of the first item also could take a moment to think about before you understand what exactly it's doing.
As an alternative, consider using .pop() instead, I think it's more intuitive than both:
return someString
  .split('.')
  .pop();

The only (tiny) downside is that the above isn't functional, since it mutates the split array - but that's OK, since the array isn't being used anywhere else.
It's true that performance for something like this isn't really something to worry about if this is used in a larger project - what matters more is if it's easier to understand what the code is doing.
Remember that if something isn't immediately intuitive, that's what comments are for, though all of these cases are self-documenting enough that it's not needed.
Another way to make it easier to recognize what's going on would be to put this into a function with a descriptive name, such as getLastProperty (if the input is meant to show a path to a nested property lookup, for example - name it as appropriate for how it's meant to be used).


Splitting uppercase letters based on a dictionary A few days ago I learned about the dynamic programming approach to problem-solving; the thing is, I'm still not sure I am fully grasping the concept.

I decided to write an algorithm that takes a single String of uppercase letters, s, and split it into substrings based on a dictionary I have provided. My algorithm works as expected, although I'm not sure if i'm using the dynamic programming approach correctly. From what I understand, DP uses an array which holds solutions to the subproblems of a larger problem. This was my goal with the result array. Here is my simple program to demonstrate a solution to the problem:

import java.util.Scanner;

class WordBreak
{

    static String[] result;
    static String[] dictionary = {"I", "THE", "ABORT", "PLAN", "MEET", "CONFIRM", "AT",
                                  "DARK", "CABIN", "PROCEED", "MISSION", "MISS",
                                  "CHOCOLATE", "ALIENS", "CANNIBALS", "SAUCE"};

    public static void main(String[] args)
    {
        Scanner in = new Scanner(System.in);
        String str = in.next();
        int size = str.length();
        result = new String[size];
        print(str, 0);
    }
    //method which recursively breaks input string into substrings
    //which are stored in an array and, if they are English words, then printed
    static void print(String str, int ind)
    {
        //each recursive call shortens the input string until length is 0
        int length = str.length();
        //if length is 0, then print the resulting substrings to decode
        //the message
        if(length == 0)
        {
            for(int i = 0; i < ind; i++)
            {
                System.out.print(result[i] + " ");
            }
            System.out.println();
            return;
        }
        //if a valid substring of the dictionary, add it to result array and recur
        for(int i = 1; i <= length; i++)
        {
            if(valid(str.substring(0, i)) == true)
            {
                result[ind] = str.substring(0, i);
                print(str.substring(i), ind + 1);
            }
        }
    }
    //valid function returns true iff string s is an English word (in the dictionary)
    static boolean valid(String s)
    {
        boolean result = false;

        for(int i = 0; i < dictionary.length; i++)
        {
            if(s.equals(dictionary[i]))
            {
                result = true;
                break;
            }
        }
        return result;
    }
}

Given the input string of "ABORTTHEPLANMEETATTHEDARKCABIN" the algorithm does accurately print out "ABORT THE PLAN MEET AT THE DARK CABIN".

My question is, is this a proper dynamic programming approach? If not, what potential changes could I make to implement DP? My main goal here is to make sure I am effectively grasping the concept.

Lastly (kind of off topic), what would the big O of this algorithm be? My analysis tells me either \$O(n^2)\$ or \$O(n^3)\$ depending on how you look at the substring(i, j) method call. This is for my own curiosity to see if I'm analyzing it correctly.

Your big O is roughly O(N^2) if you consider your dictionary to be static.

I don't see anything in your solution that I would classify as dynamic programming.  Maybe you could make a DP solution by pre-parsing your dictionary so you don't check substrings multiple times but instead check each letter against each still-matching word.  Some more test cases might turn up some interesting bugs, too.


Binary search on real space I want to perform a binary search on a continuous unimodal function ƒ(x)=y, where x and y are real numbers. I'm not looking up values in an array, and so I don't have clean integer inputs that I'm stepping along.

My original attempt at a function (written in JavaScript) has the problem that when the input value you are looking for happens to sit on a boundary chosen by the algorithm, the algorithm continues to run until the numeric precision is exhausted:

/**
 * y: the target output value to find a matching input
 * minX: the smallest input value
 * maxX: the largest input value
 * ƒ:    a function that returns a value given an X
 * ε:    the threshold to compare outputs versus the target (default:0)
 */
function binarySearch(y, minX, maxX, ƒ, ε) {
    if (ε===undefined) ε=0;
    let m=minX, n=maxX, k, v, Δ;
    while (m<=n) {
        k = (n+m)/2;
        v = ƒ(k);
        Δ = y-v;
        if (Math.abs(Δ)<=ε) return k;
        if (Δ>0) m = k;
        else     n = k;
    }
    if (Math.abs(y-ƒ(m))<=ε) return m;
    if (Math.abs(y-ƒ(n))<=ε) return n;
}

With the above, the call binarySearch( 0, 0, 10, n=>n ) will run 1077 iterations until m=0 and n=5e-324 before (m+n)/2 is finally so close to 0 that even with ε=0 the JavaScript interpreter cannot tell the difference.

A hack I was going to use is to provide a minimum step function that modifies each boundary by a fixed amount (similar to array index +/- 1). This forces the boundary to move faster, but also requires an ε>0 in case the boundary overshoots the value. It feels gross:

// step: a minimum amount to move each boundary each time
function binarySearch(y, minX, maxX, ƒ, ε, step) {
    if (ε===undefined) ε=0;
    if (step===undefined) step=0;
    let m=minX, n=maxX, k, v, Δ;
    while (m<=n) {
        k = (n+m)/2;
        v = ƒ(k);
        Δ = y-v;
        if (Math.abs(Δ)<=ε) return k;
        if (Δ>0) m = k+step;
                 n = k-step;
    }
    if (Math.abs(y-ƒ(m))<=ε) return m;
    if (Math.abs(y-ƒ(n))<=ε) return n;
}

A ~clean fix is to check the boundaries on every pass by moving the final two if statements into the while loop. This calls ƒ() three times as often each pass, and so seems inelegant.

/**
 * y: the target output value to find a matching input
 * minX: the smallest input value
 * maxX: the largest input value
 * ƒ:    a function that returns a value given an X
 * ε:    the threshold to compare outputs versus the target (default:0)
 */
function binarySearch(y, minX, maxX, ƒ, ε) {
    if (ε===undefined) ε=0;
    let m=minX, n=maxX, k, v, Δ;
    while (m<=n) {
        k = (n+m)/2;
        v = ƒ(k);
        Δ = y-v;
        if (Math.abs(y-ƒ(m))<=ε) return m;
        if (Math.abs(y-ƒ(n))<=ε) return n;
        if (Math.abs(Δ)<=ε) return k;
        if (Δ>0) m = k;
        else     n = k;
    }
}

It smells to me like there ought to be an elegant solution, some sort of fencepost I'm not thinking of, that fixes this efficiently and elegantly.

I realize now that the only time this is a problem is when the value I'm looking for is either of the starting boundaries. In all other cases, a boundary is only moved after it is checked.

So, all I had to do was check both boundaries once, before the loop starts. A fixed, slightly terser version of the original function:

/**
 * y: the target output value to find a matching input
 * minX: the smallest input value
 * maxX: the largest input value
 * ƒ:    a function that returns a value `y` given an `x`
 * ε:    the threshold to compare outputs versus the target (default:0)
 */
function binarySearch(y, minX, maxX, ƒ, ε) {
    if (ε===undefined) ε=0;
    let m=minX, n=maxX, k, Δ;
    if (Math.abs(y-ƒ(m))<=ε) return m;
    if (Math.abs(y-ƒ(n))<=ε) return n;
    while (m<n) {
        Δ = y - ƒ( k = (n+m)/2 );
        if (Math.abs(Δ)<=ε) return k;
        if (Δ>0) m=k;
        else     n=k;
    }
}

Note that this function also fixes a ~bug in the while loop by changing the condition to m<n instead of m<=n.


jQuery score card form to determine pass or fail I have made a simple scorecard form which uses jQuery to automatically calculate the score of submissions depending on field values. Some fields are "critical" which means if they are a fail they automatically fail.

You can see a jsFiddle example to get a better understanding. I have tried to make the code as efficient as possible but would welcome peoples opinions on what improvements can be made.

//disable individual radios by#
$("#edit-submitted-calc-verification-completed-idv-correctly-1, #edit-submitted-calc-summary--action-2-notes-1").attr('disabled',true);

$("#webform-component-calc select, #webform-component-calc :radio").change(function(){

   var $score = 0;
   var $fails = 0;
   var $critFails = 0;

    $('select option:selected, :radio:checked').each(function(){

        var $thisVal = $(this).val();
        var $split = $thisVal.split('_');
        var $string = $split[0];
        var $points = parseInt($split[1],10);

      $(this).parents('tr').find('.webform-grid-option').removeClass('pass fail');

      if ($string == 'fail' || $string == 'critical') {

          $(this).parents('.webform-grid-option').addClass('fail');

          if($string == 'fail') {
              $fails += 1;
          }
          if($string == 'critical') {
              $critFails += 1;
          }
      }

      else {

          $(this).parents('.webform-grid-option').addClass('pass');
           if($string == 'pass' || $string == 'na') {
              $score += $points;
           }
        }
    });

    var $finalScore = $score;
    if($critFails > 0){
         var $finalScore = 0;
       }

    var $summary = 'Final Call score: '+$finalScore+'% with '+$fails+' Fails / '+$critFails+' Critical Fails';

    //alert($summary);
    $('#edit-submitted-calc-number-of-fails').val($fails);
    $('#edit-submitted-calc-call-score').val($score);
    $('#edit-submitted-calc-critical-failures-number-of-critial-fails').val($critFails);
    $('#edit-submitted-calc-critical-failures-final-score').val($finalScore);
    $('#edit-submitted-calc-summary').val($summary);

});

Personally I wouldn't change anything about the calculation itself. It all quite reasonable the way it is.

There are just two things could be optimized concerning jQuery:

You should be using .closet() instead of .parents() since you only need the first (closest) matching parent and not all ancestors.
Also you can optimize assigning the event handler to the radio buttons by using .delegate() (or .on with jQUery 1.7) instead.

There are also a few things which personally I'd do differently:

In the HTML I'd use th elements for the question cells and td just for the options, allowing you to get rid of the classes webform-grid-question and webform-grid-option.
I'm not a big fan of using $ in variable names, especially if they don't point to a jQuery object.
I don't like using input elements for output. I'd use a "normal" element such as span or div (or if HTML5 is a option the new output element).


BFS shortest path program The task was to find the shortest path between P and K in a maze. You can move horizontally and vertically, where # is a wall and . is free space. I've had a lot of trouble passing on the int k, which is the path length. I had to make a pair which contains a pair of x and y coordinates and int k.

Could I have done this any better without too much change in the code, especially with the pairs and the while loop? And how good is my code overall?

#include <iostream>
#include <string>
#include <vector>

using namespace std;

int dfs(string g[30][30], pair<int, int> p)
{
    vector <pair<pair<int, int>, int> > v;
    int visited[30][30] = {0};
    int x, y, k = 0;
    pair <pair<int, int>, int> mj;
    pair <pair<int, int>, int> z;
    pair <int, int> l;
    z = make_pair(make_pair(p.first, p.second), 0);
    v.push_back(z);
    while(v.size() != 0)
    {
        mj = v.front();
        l = mj.first;
        x = l.first;
        y = l.second;
        k = mj.second;
        v.erase(v.begin());
        if(g[x][y] == "K")
            return k;
        if((g[x+1][y] == "." or g[x+1][y] == "K") and not(visited[x+1][y]))
        {
            v.push_back(make_pair(make_pair(x+1, y), k+1));
            visited[x+1][y] = 1;
        }
        if((g[x][y+1] == "." or g[x][y+1] == "K") and not(visited[x][y+1]))
        {
            v.push_back(make_pair(make_pair(x, y+1), k+1));
            visited[x][y+1] = 1;
        }
        if((g[x-1][y] == "." or g[x-1][y] == "K") and not(visited[x-1][y]))
        {
            v.push_back(make_pair(make_pair(x-1, y), k+1));
            visited[x-1][y] = 1;
        }
        if((g[x][y-1] == "." or g[x][y-1] == "K") and not(visited[x][y-1]))
        {
            v.push_back(make_pair(make_pair(x, y-1), k+1));
            visited[x][y-1] = 1;
        }
    }
}

int main()
{
    int n, m;
    pair <int, int> p;
    string s, g[30][30];
    cin >> n;
    cin >> m;
    for(int i = 0; i < n; i++)
    {
        cin >> s;
        for(int j = 0; j < s.size(); j++)
        {
            g[i][j] = s.substr(j, 1);
            if(g[i][j] == "P")
                p = make_pair(i, j);
        }
    }
    cout << dfs(g, p);
    return 0;
}

Sample input:

6
10
##########
#.P#.....#
##.#.K#..#
#..####..#
#........#
##########

Sample output:

14

Please don't use using namespace std (read this).
The variable names are really not helping. Why not call a maze a maze instead of g? What are mj, z, l, k? It's hard to follow the logic with names like these.
Instead of working with strings like "K", "." and "P" it would be more efficient to use chars, and to give them meaningful names.
Instead of int visited[30][30], bool visited[30][30] would be more natural

Here's an alternative version of your code, with some variables renamed, constants used, using char instead of string, not using namespace std:

#include <iostream>
#include <vector>

using std::cin;
using std::cout;
using std::pair;
using std::make_pair;
using std::vector;

const char start = 'P';
const char target = 'K';
const char empty = '.';

int dfs(char maze[30][30], pair<int, int> p)
{
    vector <pair<pair<int, int>, int> > v;
    bool visited[30][30] = {0};
    int x, y, k = 0;
    pair <pair<int, int>, int> mj;
    pair <pair<int, int>, int> z;
    pair <int, int> l;
    z = make_pair(make_pair(p.first, p.second), 0);
    v.push_back(z);
    while(v.size() != 0)
    {
        mj = v.front();
        l = mj.first;
        x = l.first;
        y = l.second;
        k = mj.second;
        v.erase(v.begin());
        if (maze[x][y] == target) {
            return k;
        }
        if((maze[x+1][y] == empty or maze[x+1][y] == target) and not(visited[x+1][y]))
        {
            v.push_back(make_pair(make_pair(x+1, y), k+1));
            visited[x+1][y] = true;
        }
        if((maze[x][y+1] == empty or maze[x][y+1] == target) and not(visited[x][y+1]))
        {
            v.push_back(make_pair(make_pair(x, y+1), k+1));
            visited[x][y+1] = true;
        }
        if((maze[x-1][y] == empty or maze[x-1][y] == target) and not(visited[x-1][y]))
        {
            v.push_back(make_pair(make_pair(x-1, y), k+1));
            visited[x-1][y] = true;
        }
        if((maze[x][y-1] == empty or maze[x][y-1] == target) and not(visited[x][y-1]))
        {
            v.push_back(make_pair(make_pair(x, y-1), k+1));
            visited[x][y-1] = true;
        }
    }
}

int main()
{
    int n, m;
    pair <int, int> p;
    char s, maze[30][30];
    cin >> n;
    cin >> m;
    for(int i = 0; i < n; i++)
    {
        for(int j = 0; j < m; j++)
        {
            cin >> s;
            maze[i][j] = s;
            if(maze[i][j] == start)
                p = make_pair(i, j);
        }
    }
    cout << dfs(maze, p) << std::endl;
    return 0;
}

Next, notice the duplication in this code:

if((maze[x+1][y] == empty or maze[x+1][y] == target) and not(visited[x+1][y]))
{
    v.push_back(make_pair(make_pair(x+1, y), k+1));
    visited[x+1][y] = true;
}
if((maze[x][y+1] == empty or maze[x][y+1] == target) and not(visited[x][y+1]))
{
    v.push_back(make_pair(make_pair(x, y+1), k+1));
    visited[x][y+1] = true;
}

The treatment for (x+1, y) is the same as for (x, y+1), and (x-1, y) and (x, y-1). Extract the duplicated parts to a helper method:

void check_maze_pos(char maze[30][30], int x, int y, int k, vector<pair<pair<int, int>, int> > &v, bool visited[30][30])
{
    if ((maze[x][y] == empty or maze[x][y] == target) and !visited[x][y])
    {
        v.push_back(make_pair(make_pair(x, y), k + 1));
        visited[x][y] = true;
    }
}

And then you can replace the 4 duplicated cases with these 4 simpler lines:

check_maze_pos(maze, x+1, y, k, v, visited);
check_maze_pos(maze, x, y+1, k, v, visited);
check_maze_pos(maze, x-1, y, k, v, visited);
check_maze_pos(maze, x, y-1, k, v, visited);

You can still improve further:

Eliminate the duplication of the magic number 30 in many places
Simplify the duplicated type definitions using typedef


Improving performance of VBA batch scripts? I am using a script to open a bunch of Excel files, copying two or more sheets into a new file and saving this new file. It sure beats doing it manually, but I think it could be faster.

Here's my script:

Sub Flujo()

Const FilePath = "A path..."
Const Destination = "Another path..."

Dim app As New Excel.Application
app.Visible = False
app.ScreenUpdating = False
app.DisplayAlerts = False

Dim str As String
str = Dir(FilePath & "\*.xlsx")
Do Until str = ""

    Dim WrkBook As Workbook
    Set WrkBook = app.Workbooks.Open(Filename:=FilePath & "\" & str, UpdateLinks:=0)
    str = Dir()

    Dim NewBook As Workbook
    Set NewBook = app.Workbooks.Add

    WrkBook.Sheets("SheetName").Copy After:=NewBook.Sheets(3)
    WrkBook.Sheets("AnotherSheetName").Copy After:=NewBook.Sheets(3)

    NewBook.Sheets(1).Delete
    NewBook.Sheets(1).Delete
    NewBook.Sheets(1).Delete

    NewBook.SaveAs Filename:=Destination & "flujo_" & WrkBook.Name
    NewBook.Close False
    WrkBook.Close False
Loop

app.DisplayAlerts = True

app.Quit
Set app = Nothing
End Sub

Is there a way I could make it work faster?

-Edit- The script takes more or less 3 - 6 seconds per file. That's not too much but I feel it could be much faster. Or is this the fastest I can expect to get with VBA?

Rather than copying sheet by sheet - plus deleting three excess sheets one by one, either

Delete the sheets you don't need then use SaveAs on the reduced file.
Copy both sheets you need to a new file in a single hit.

(Also, don't Dim variables inside a loop)

(2) below

Sub Flujo()

Const FilePath = "A path..."
Const Destination = "Another path..."
Dim xlApp As New Excel.Application

With xlApp
    .Visible = False
    .ScreenUpdating = False
    .DisplayAlerts = False
End With

Dim strDir As String
Dim WrkBook As Workbook

strDir = Dir(FilePath & "\*.xlsx")

Do While Len(strDir) > 0
Set WrkBook = xlApp.Workbooks.Open(Filename:=FilePath & "\" & strDir, UpdateLinks:=0)
  WrkBook.Sheets(Array("SheetName", "AnotherSheetName")).Copy
  ActiveWorkbook.SaveAs Filename:=Destination & "flujo_" & WrkBook.Name
  ActiveWorkbook.Close False
  WrkBook.Close False
strDir = Dir
Loop

xlApp.DisplayAlerts = True

xlApp.Quit
Set xlApp = Nothing
End Sub


How to remove code duplication that difference only a few lines? I have 2 methods that do almost the same things except a few lines of code. The sample is like below.

public void Method1()
{
    DoX();

    if (value)
    {
        var input = GetA();
        DoA1();
        DoA2(input);
    }

    DoY();
}

public void Method2()
{
    DoX();

    if (value)
    {
        var input = GetB();
        DoB(input);
    }

    DoY();
}

I have two questions for this code.
1. Is it a good idea to remove code duplication is this case?
2. How to remove the duplication without make it harder to read the code

Based on your code as that is what we are reviewing a couple of suggestions.

Pass in a delegate that handles the changeable part
Use Template method pattern to implement the changing parts
Use composition and DI to inject the different elements

Example: Using delegates

private void DoMethod12(Action doMyAction)
{
    DoX();
    doMyAction();
    DoY();
}

public void Method1()
{
    DoMethod12(_ =>
    {
        var input = GetA();
        DoA1();
        DoA2(input);
    });
}

public void Method2()
{
    DoMethod12(_ =>
    {
       var input = GetB();
       DoB(input);
    });
}

Example 2: Template pattern

public abstract class MyDo
{
    protected abstract DoMyAction1();
    protected abstract DoMyAction2();

    public void Method1()
    {
        DoMethod12(doMyAction);
    }

    public void Method2()
    {
        DoMethod12(doMyAction2);
    }

    private void DoMethod12(Action doMyAction)
    {
        DoX();
        doMyAction();
        DoY();
    }
    private void DoX()
    {
    }
    private void DoY()
    {
    }
}

public class MyDoer : MyDo
{
    protected override DoMyAction1()
    {
           var input = GetA();
           DoA1();
           DoA2(input);
    }

    protected override DoMyAction2()
    {
       var input = GetB();
           DoB(input);
    }
}

Example 3: DI and composition

public interface IMyDoer
{
    void MyMethod1();
    void MyMethod2();
}

public class MyDoer
{
   private MyDoer _doer;

   public MyDoer(IMyDoer doer)
   {
      _doer = doer;
   }

    public void Method1()
    {
        DoMethod12(_doer.MyMethod1);
    }

    public void Method2()
    {
        DoMethod12(_doer.MyMethod2);
    }

    private void DoMethod12(Action doMyAction)
    {
        DoX();
        doMyAction();
        DoY();
    }

    private void DoX()
    {
    }
    private void DoY()
    {
    }
}

These written in notepad so no guareentee on exact syntax although hopefully they can give some ideas.


Single line FizzBuzz solution in LINQ Now FizzBuzz itself isn't a big challenge but I agree that it can be a good tool to see if someone can code or not.  I wanted to practice my LINQ a little bit so here's my single line FizzBuzz solution.

Enumerable.Range(1,100).Select(
                        n =>
                        (n % 15 == 0) ? "FizzBuzz" :
                        (n % 3 == 0) ? "Fizz" :
                        (n % 5 == 0) ? "Buzz" :
                        n.ToString())
                        .ToList()
                        .ForEach(Console.WriteLine);

Overall, I like this. It is concise, and neat, and all. My only beef is the nested ternaries.
In general, ternary operator precedence is complicated by the lack of blocks.... The need for the parenthesis on the modulo precedence makes the conditions clear ( (n % 15 == 0) ) and the actual indenting you have, makes the logic a little clearer, but precedence is not that simple, and I have seen bugs where subtle precedence problems have caused the wrong execution-path through the nested ternaries....
.... bottom line, is for simple statements, ternaries are great, but the readability is compromised when there are multi-levels of ternaries. Nice for small things, ugly for big.
I would have preferred if you made the coditionals more obvious ... by extracting to a function maybe... but that would ruin your 1-liner.....
How about ... (Shown in this ideone here):
Enumerable.Range(1,100).Select(
                    n =>
                    (n % 15 == 0) ? "FizzBuzz" :
                    (
                        (n % 3 == 0) ? "Fizz" :
                        (
                            (n % 5 == 0) ? "Buzz" :  n.ToString())
                        )
                    )
                    .ToList()
                    .ForEach(Console.WriteLine);

EDIT: The above answer is not as good as I wanted it to be.....
The exact problem with ternary precedence is that it is right-associative. This makes the logic potentially buggy, and hard to understand.....
Consider the ternaries:
expa = boola ? expc : expd;
expb = boolb ? expe : expf;

result = boolc ? expa : expb;

You can modify this to be:
expa = boola ? expc : expd;

result = boolc ? expa : boolb ? expe : expf;

Or, rewriting it in 'your' style, as:
expa = boola ? expc : expd;

result = boolc ? expa :
         boolb ? expe :
         expf;

But, you cannot 'inline' expa in the above as easily:
expb = boolb ? expe : expf;

result = boolc ? (boola ? expc : expd) : expb;

You need the parenthesis.
Inlining ternaries is just plain ugly.

Better solution
I regret suggesting to use the parenthesis to block the code in a more readable format. There is no more readable format, other than to extract the function, which is the 'right' thing to do:
    static private string FizzBuzz(int value)
    {
        if (value % 15 == 0)
        {
            return "FizzBuzz";
        }
        if (value % 3 == 0)
        {
            return "Fizz";
        }
        if (value % 5 == 0)
        {
            return "Buzz";
        }
        return value.ToString();
    }

    public static void Main()
    {
        foreach(var val in Enumerable.Range(1, 100).Select(n => FizzBuzz(n)))
            Console.WriteLine(val);
    }

Note, I have also undone the slow, and unnecessary conversion of the Enumerable to the ToList().ForEach() operation. Converting it to the list before re-converting it to an Enumeration is a waste of effort.
Bottom line, is that the 1-liner is not the best, or most readable format.
I have put this in an ideone, to show it working.


Find the kth prime and all the primes below it It works by finding 2n+1 with n from 1 to k, but not where 2n+1 would be divisible by one of the already known primes. I'd like to know if the program can find every prime, and if so, how it compares to the efficiency of other algorithms.

n=1
k=input()
primes=[2]
def f(n):
   b, a = 1, 1
   for i in primes:
      a=((2*n+1)%i)
      b=b*a
   return b
while k > n:
   if f(n) >= 1:
      primes.append((2*n)+1)
      n=n+1
   else:
      n=n+1
      k=k+1
print primes

Before we talk about efficiency, let's talk about something very important: readability. It took me an unreasonably long time to understand what it was your code was doing, due to lots of single-letter variable names, confusing flow control, and odd choices for functions.

Checking for primality

Let's start with f. f is apparently determining the primality of n, so let's call it is_prime. Furthermore, it's not actually determining the primality of n, it's determining the primality of 2n+1. That's too much for one function - let's change it's responsibility to take the number it tests. The way you're doing it (taking the % against every prime) is a good algorithm, but then you're multiplying the mods together. That doesn't make sense as an operation. You don't care about the product, you care if any of them are zero! And as soon as one of them is zero, you can return False.

Here's a clearer function:

def is_prime(n):
    for p in primes:
        if n%p == 0:
            return False
    return True

Or if you're generator inclined:

def is_prime(n):
    return not any(n%p == 0 for p in primes)

Main Loop

We need to find k primes. You are keeping track of this by comparing k to n and occasionally incrementing k. That is confusing. Since we're already keeping track of all the primes, the termination condition should be:

len(primes) >= k

And then we can handle iteration by the odd numbers with itertools.count():

num_primes = input()
primes = [2]

def is_prime(n): ...

for n in itertools.count(start=3, step=2):
    if is_prime(n):
        primes.append(n)

        if len(primes) >= num_primes:
            break

This program is far easier to understand.

Efficiency

We could do better in terms of efficiency. At a first approach, we're checking all the odd numbers but we can drop the multiples of 3 earlier by simply writing our own generator that spits out 3, 5, and then alternates adding 2 and 4. That's a quick optimization that let's us keep the same algorithm.

Better would be to use an entirely different, and very well-known algorithm: the Sieve of Eratosthenes. It is a much better algorithm for this problem.


Processing file inclusion directives using mutual recursion I am preprocessing a set of files where one file is included in another file with an include tag like shown below:

file A

include file B
include file C
 contents of file A
include file D
 contents of file A

Here I need to replace the include tags with contents of each of the respective files, just like what a compiler does. I have two functions for that:

def parseContent(self, rdbFilePath, content):
    finalContent = self.removeComments(content)
    includeFileSearch = re.compile(r'(?P<tag>(\s)*include+(\s*)\"(\s*)(?P<file>[a-zA-Z0-9\.\_/]*)\")')
    for includes in includeFileSearch.finditer(finalContent):
        finalContent = re.sub(includes.group('tag'),self.parseIncludes(rdbFilePath, includes.group('file')), finalContent)
    return finalContent

def parseIncludes(self, rdbFilePath, file):
    path = rdbFilePath + "/" + file
    f = open(path)
    pathDir = os.path.dirname(path)
    includedFileContent = self.parseContent(pathDir, f.read())
    return includedFileContent

As you can see, functions parseContent and parseIncludes call each other recursively to replace all the include tags in every file. The logic works fine. But it takes a bit long time to execute. Is there any better way to do the same with lesser execution time?

parseContent() calls both re.finditer() and re.sub(), which is inefficient, because the re.sub() has to find the insertion point that re.finditer() had already found. What you want is just re.sub():

  repl can be a string or a function… If repl is a function, it is called for every non-overlapping occurrence of pattern. The function takes a single match object argument, and returns the replacement string.

Going further, you should make just one substitution pass that handles both comments and includes. (You didn't show us the removeComments() code, so you'll have to adapt the regex yourself.)

The double-quotes in the regex have no special significance, and do not need to be escaped with backslashes. Also, the dot and the underscore characters do not need to be escaped within a character class.

DIRECTIVE_RE = re.compile(r'(?P<comment>#.*)|(?P<include>(\s)*include+(\s*)"(\s*)(?P<file>[a-zA-Z0-9._/]*)")')

def process_content(self, rdb_file_path, content):
    def directive_handler(match):
        if match.group('comment'):
            return ''
        elif match.group('include'):
            path = os.path.join(rdb_file_path, match.group('file'))
            with open(path) as f:
                return self.process_content(rdb_file_path, f.read())
    return DIRECTIVE_RE.sub(directive_handler, content)

open() should almost always be called using a with block to ensure that the filehandle gets closed automatically.

Note that I have used os.path.join() as a portable alternative to + "/" +.


Iterative Collatz with memoization I'm trying to write efficient code for calculating the chain-length of each number.

For example, 13 -> 40 -> 20 -> 10 -> 5 -> 16 -> 8 -> 4 -> 2 -> 1.
It took 13 iterations to get 13 down to 1, so collatz(13) = 10.

Here is my implementation:

def collatz(n, d={1:1}):
    if not n in d:
        d[n] = collatz(n * 3 + 1 if n & 1 else n/2) + 1
    return d[n]

It's quite efficient, but I'd like to know whether the same efficiency (or better) can be achieved by an iterative one.

Using the Python call stack to remember your state is very convenient and often results in shorter code, but it has a disadvantage:

>>> collatz(5**38)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "cr24195.py", line 3, in collatz
    d[n] = collatz(n * 3 + 1 if n & 1 else n/2) + 1
  [... many lines omitted ...]
  File "cr24195.py", line 3, in collatz
    d[n] = collatz(n * 3 + 1 if n & 1 else n/2) + 1
RuntimeError: maximum recursion depth exceeded

You need to remember all the numbers you've visited along the way to your cache hit somehow. In your recursive implementation you remember them on the call stack. In an iterative implementation one would have to remember this stack of numbers in a list, perhaps like this:

def collatz2(n, d = {1: 1}):
    """Return one more than the number of steps that it takes to reach 1
    starting from `n` by following the Collatz procedure.

    """
    stack = []
    while n not in d:
        stack.append(n)
        n = n * 3 + 1 if n & 1 else n // 2
    c = d[n]
    while stack:
        c += 1
        d[stack.pop()] = c
    return c

This is a bit verbose, and it's slower than your recursive version:

>>> from timeit import timeit
>>> timeit(lambda:map(collatz, xrange(1, 10**6)), number=1)
2.7360708713531494
>>> timeit(lambda:map(collatz2, xrange(1, 10**6)), number=1)
3.7696099281311035

But it's more robust:

>>> collatz2(5**38)
1002


Creating Mongoose model I'm creating a Mongoose model for a web service. I have a secondary index on facultyId because majority of the reads would want to search by facultyId. I've included some statics on the model in order to make it simple for people to query.

const mongoose = require("mongoose");
const Schema = mongoose.Schema;

mongoose.Promise = global.Promise;

const GAPFApplicationSchema = new Schema({
  facultyId: { type: Number, index: true },
  created: Number, // use UNIX time stamps for all dates
  lastModified: Number,
  status: { type: String, enum: ["SUBMITTED", "BUDGET_ALLOCATED"] },
  attachedDocuments: [
    {
      name: String,
      link: String,
      attachedDate: Number
    }
  ]
});

GAPFApplicationSchema.static("findByFacultyId", function(facultyId, callback) {
  return this.findOne()
    .where("facultyId")
    .equals(facultyId);
});

GAPFApplicationSchema.static("submit", function(gapf, callback) {
  return this.findOneAndUpdate(
    { facultyId: gapf.facultyId },
    gapf,
    { upsert: true }, // create it if doesn't exist
    callback
  );
});

const GAPFApplication = mongoose.model(
  "GAPFApplication",
  GAPFApplicationSchema
);

export { GAPFApplication };

I haven't used Mongoose before, so I was hoping to get some review on the approach taken here. The code is pretty short, so I didn't think there would be much to address.

You have created a good model overall and I can only suggest you some consideration.

Instead of using, created & lastModified.
I would recommend you to use timestamp option in mongoose.

const GAPFApplicationSchema = new Schema({
  facultyId: { type: Number, index: true },
  status: { type: String, enum: ["SUBMITTED", "BUDGET_ALLOCATED"] },
  attachedDocuments: [
    {
      name: String,
      link: String,
      attachedDate: Number
    }
  ]
}, {timestamp: true});

With this you won't need lastModified & created you will get 2 attributes createdAt & updatedAt and they would be handled by mongoose, so you wont have to worry about updating them.

status: { type: String, enum: ["SUBMITTED", "BUDGET_ALLOCATED"] },

you can also improve this piece of code, by changing it to number and create an internal mapping, this would allow mongoDB to search faster, as comparing string is much heavier task, than comparing numbers.


Decoding indeterminate types using JSON Decoder I'm working with a REST service that responds with indeterminate types. In particular, it can respond with a String, a correct payload (T: Decodable) or an error payload (PayloadError).
In order to cope with this type of requirement I've ended up with the following solution:
do {
    let decoder = JSONDecoder()
    let either = try decoder.decode(Either<T, PayloadError>.self, from: data)
    switch either {
        case .left(let value):
            completion(.success(value))
        case .right(let payloadError):
            completion(.failure(.generic)))
    }
} catch {
    if let value = String(data: data, encoding: .utf8) as? T {
        completion(.success(value))
    } else {
        completion(.failure(.generic))
    }
}

where Either is defined as:
enum Either<T, U> {
    case left(T)
    case right(U)
}

extension Either: Decodable where T: Decodable, U: Decodable {
    init(from decoder: Decoder) throws {
        if let value = try? T(from: decoder) {
            self = .left(value)
        } else if let value = try? U(from: decoder) {
            self = .right(value)
        } else {
            let context = DecodingError.Context(
                codingPath: decoder.codingPath,
                debugDescription: "Cannot decode \(T.self) or \(U.self)"
            )
            throw DecodingError.dataCorrupted(context)
        }
    }
}

Do you think the code looks good or should I use a more elegant solution?
Update 01.11.2020
These are the possible use cases along side with concrete responses.
Use Case 1
The client calls API method /authenticate. The server can responds with an authentication token of type String or a payload error of type PayloadError (if the server responds with unauthorized, for example). The JSON used to decode PayloadError is something like:
{
    code: 1,
    message: "Error message here"
}

PayloadError is defined as:
struct PayloadError: Decodable {
    let code: Int
    let message: String
}

Use Case 2
The client calls any other method. For example, it can call /user that gives back the info for the current user (the auth token is passed along). A struct User: Decodable is decoded from the returned JSON. If there is an error, the server replies with a PayloadError.
Based on the above use cases, the server can respond with:

String
T: Decodable (like User)
PayloadError

What I don't like is the code in catch since its intent is not clear.

I've finally end up splitting the two use cases. This in order to make the code readable and more maintainable.
PayloadError is mapped to a ServiceError: Error in order to align the output.
Use case 1
guard let data = response.data else {
    completion(.failure(.invalidData))
    return
}

let jsonDecoder = JSONDecoder()
if let payloadError = try? jsonDecoder.decode(PayloadError.self, from: data) {
    completion(.failure(ServiceError.from(payloadError)))
} else if let token = String(data: data, encoding: .utf8) {
    completion(.success(token))
} else {
    completion(.failure(.decodingError))
}

Use case 2
guard let data = response.data else {
    completion(.failure(.invalidData))
    return
}
let decoder = JSONDecoder()
guard let either = try? decoder.decode(Either<T, PayloadError>.self, from: data) else {
    completion(.failure(.decodingError))
    return
}
switch either {
case .left(let value):
    completion(.success(value))
case .right(let payloadError):
    completion(.failure(ServiceError.from(payloadError)))
}


MongoDB - Find records in one collection that match string in 2nd collection I have a MongoDB collection of product IDs/unique products attributes and a second collection of codes that relate to attributes common for products whose IDs are prefixed with those codes.

For example, two product IDs might be aaa17859 and aaa98542 so their code prefix is aaa.

I have several thousand codes, and the codes rarely change, so the 'codes' collection is more of a data repo. The products change frequently throughout the day, and imports can run from several hundred to a few thousand.

Currently, I am running the below to join the two sets together when the data is first loaded in.

import re

code_collection= list(db.codes.find())
for x in code_collection:
    p = re.compile(x['CODE-PREFIX'] + '.*')
    db.productdataset.update({'productID': {'$regex': p}}, { '$set': {'codeData': x['data_property']}}, multi=True )

Pro to this method is that it reduces the number of calls I would otherwise have to make once the data is initially loaded.

Major con is that when the code collection is updated (say, a new common attribute), something similar has to be rerun against the entire product collection. Plus, depending on this list, it can take a little longer than I'd like to load in the data. Also, this means I can end up with a good deal of duplication. In general, this approach just seems a little kludgy.

What I would like to optimize is a method to join these two sets together in one call, thus preventing the rerun. I have been looking at a lot of mapreduce examples, but have had trouble figuring out how I would be able to use the prefix from the code collection to use regex and  look up it's related productID.

You're turning the result of db.codes.find() into a list, but you're just iterating over it anyway. Why not just leave it in whatever form it arrives? If it can be converted to a list it can probably be iterated over. This would save you the time of conversion and mean you might not need to have a full list stored in memory.

Also x isn't a great name for the temporary value when you could just use code and take up only 3 more letters.


jQuery to target each #hdid I'm trying to clean up this function to target individual #hdid. I'm also using $this, but with no success.

<script>
        $(function() {
            $(".delete").click(function() {
                $('#load').fadeIn();
                var commentContainer = $(this).parent();
               // var id = $this.(".delete").val();
               // var string = id ;
               //  console.log(id);

                $.ajax({
                    type: "POST",
                    url: "delete-class.php",
                    data: {hdid: $('#hdid').val()},
                    cache: false,
                    success: function(){
                        commentContainer.slideUp('slow', function() {$(this).remove();});
                        $('#load').fadeOut();
                        //console.log(string);
                    }
                });
                return false;
            });
        });
    </script>

If you refactor your code a bit you can do it this way. I'm also making the assumption that you mean .hdid instead of #hdid there should only be one hdid per page if it's really an ID and not a CLASS. Here's a quick sample if you're interested in seeing it in action.

<script>
            var DeleteClass = function(hdid){
                $.ajax{
                    type: "POST",
                    url: "delete-class.php",
                    data: {hdid: hdid.val()},
                    cache:false,
                    success:function(){
                       hdid.parent().slideUp('slow',function(){$(this).remove();});
                    }
            }

    $(function() {
        $(".delete").click(function() {
            $('#load').fadeIn();
            $(this).children(".hdid").each(function(){
                DeleteClass($(this));
            });
            $('#load').fadeOut();
            return false;
        });
    });
</script>


Thread-safe database updates I have the following method that will return VariantID for given VariantName ("Name" in db).
To avoid calling my db few thousand times I have dumped all records (~30) to the simple dictionary.

I'm almost certain that this is the correct way of implementing this method but I would like to hear some feedback and any possible tips.

TestManagementEntities is an EntityFramework DbContext

Calling Method

private void UpdateVariants(WorkItem workItem, string variantValue, TestManagementEntities db)
    {

        string[] variants = variantValue.Split(',').Where(x => !String.IsNullOrEmpty(x)).ToArray();

        int[] variantIds = new int[variants.Count()];

        Dictionary<string, int> variantDictionary =
            db.Variants.ToDictionary(x => x.Name, y => y.VariantID);
        Object lockObject = new Object();

        System.Threading.Tasks.Parallel.For(0, variants.Count(), i =>
        {
            variants[i] = variants[i].Trim();
            //Method in question call
            variantIds[i] = GetVariantId(variants[i], variantDictionary, db, lockObject);
        });

        //Some not relevant to question code here
    }

private int GetVariantId(string variantName, Dictionary<string, int> variantDictionary, TestManagementEntities db, Object lockObject)
    {
        if (variantDictionary.ContainsKey(variantName))
        {
            return variantDictionary[variantName];
        }
        else
        {
            lock (lockObject)
            {
                if (variantDictionary.ContainsKey(variantName))
                {
                    return variantDictionary[variantName];
                }
                else
                {
                    Variant newVariant = new Variant();
                    newVariant.Name = variantName;
                    db.Variants.Add(newVariant);
                    db.SaveChanges();
                    variantDictionary.Add(variantName, newVariant.VariantID);
                    return newVariant.VariantID;
                }
            }
        }
    }

Why is your parameter variantValue a string? Why not an IEnumerable<string> or an ICollection<string> or something like that?

Why is variants an array? If it had been an ICollection<string> it would have had a Count property that was far less costly than .Count(). Don't use the (extension) method .Count(), use the Length property. Also: why is it called variants when it really contains names?

Why do you do this inside that loop: variants[i] = variants[i].Trim();? Why not do this when you do variantValue.Split(',').Where(x => !String.IsNullOrEmpty(x)).ToArray();

The whole logic seems really contrived to me: storing the contents of a table in a dictionary, just to check whether you need to add a value to the DB and to retrieve the IDs? Why not make it much more simple:

Query the DB for the entries where the name corresponds with the ones you've received as a parameter (a WHERE IN query).
Remove the results from the list of names.
Loop through the filtered names, create a Variant for each and only execute db.SaveChanges(); after you've added ALL items.
After you've executed db.SaveChanges(); all those newly created variants will have an ID which you can retrieve.

By the way, don't do ContainsKey if you need the value associated with said key, instead use TryGetValue.

I suppose that you need variantIds for that //Some not relevant to question code here, and that bothers me a bit. GetVariantId for instance does far more than the method name says it does, and thus I suspect UpdateVariants contains more logic that really shouldn't be there according to the Single Responsibility Principle.