"Does an array contain the same elements as another array?" Write a static method named contains that accepts two arrays of
  integers a1 and a2 as parameters and that returns a boolean value
  indicating whether or not a2's sequence of elements appears in a1
  (true for yes, false for no). The sequence of elements in a2 may
  appear anywhere in a1 but must appear consecutively and in the same
  order. For example, if variables called list1 and list2 store the
  following values:

  int[] list1 = {1, 6, 2, 1, 4, 1, 2, 1, 8}; int[] list2 = {1, 2, 1};

  Then the call of contains(list1, list2) should return true because
  list2's sequence of values {1, 2, 1} is contained in list1 starting at
  index 5. If list2 had stored the values {2, 1, 2}, the call of
  contains(list1, list2) would return false because list1 does not
  contain that sequence of values. Any two lists with identical elements
  are considered to contain each other, so a call such as
  contains(list1, list1) should return true.

  You may assume that both arrays passed to your method will have
  lengths of at least 1. You may not use any Strings to help you solve
  this problem, nor methods that produce Strings such as
  Arrays.toString.

I'm looking for general feedback to improve my future code. I'm intent on learning Java, so any style tips would be well appreciated. Other ways to accomplish this task?

public static boolean contains(int[] list1, int[] list2) {
    for (int i = 0; i <= (list1.length - list2.length); i++) {
        if (list1[i] == list2[0]) {
            for (int j = 1; j < list2.length; j++) {
                if (list1[i + j] == list2[j]) {
                    if (j == (list2.length - 1)) {
                        return true;
                    }
                } else {
                    break;
                }
            }
        }
    }
    return false;
}

if (list1[i] == list2[0]) {
            for (int j = 1; j < list2.length; j++) {
                if (list1[i + j] == list2[j]) {

We can drop this from three lines to one.

        for (int j = 0; j < list2.length && list1[i + j] == list2[j]; j++) {

A side benefit is that this handles the case where list2 is empty (zero length).  The original code would crash in that case.

At the expense of an extra comparison (which could be compiled out), we get rid of two levels of indent and

                } else {
                    break;

Because this will leave the loop automatically.  It won't be necessary to break out manually.

It might be better to replace

        if (list1[i] == list2[0]) {
            for (int j = 1; j < list2.length; j++) {
                if (list1[i + j] == list2[j]) {
                    if (j == (list2.length - 1)) {
                        return true;
                    }
                } else {
                    break;
                }
            }
        }

with

        int j = 0;
        while (j < list2.length && list1[i + j] == list2[j]) {
            j++;
        }

        if (j == list2.length) {
            return true;
        }

Moving the scope of j outside the inner loop allows us to move the test out of the loop as well.  So rather than testing on every iteration, we can test just once.

Alternately

        for (int j = 0; list1[i + j] == list2[j]; j++) {
            if (j + 1 >= list2.length) {
                return true;
            }
        }

Does the same thing as the original code.  It may not be obvious, but this does the same thing with fewer comparisons.

But we're back to throwing an exception if list2 is zero length.  This does exactly the same thing as the original code with fewer comparisons and less code.


Euler Project 14 - Longest Collatz Sequence in PHP This is my solution to Project Euler 14:

  The following iterative sequence is defined for the set of positive integers:

n → n/2 (n is even)
n → 3n + 1 (n is odd)

  Using the rule above and starting with 13, we generate the following sequence:

13 → 40 → 20 → 10 → 5 → 16 → 8 → 4 → 2 → 1

  It can be seen that this sequence (starting at 13 and finishing at 1) contains 10 terms. Although it has not been proved yet (Collatz Problem), it is thought that all starting numbers finish at 1.

  Which starting number, under one million, produces the longest chain?

  NOTE: Once the chain starts the terms are allowed to go above one million.

<?php
$count = 0 ;
$max = 0;

for($n = 2 ; $n < 1000000 ; $n++){
    while ($n > 1)
    {
        if ($n % 2 == 0 )
        {
            $n = $n/2;

        }
        else
        {
            $n = 3*$n + 1 ;

        }
        $count += 1;

       if($count > $max )
       {
            $max = $count;
            $final = $n;
       }
    }
 }
 echo $final;
?>

It took so long to run. I looked at some other solutions and they were very similar to my code logically, but they were running way much faster than mine. My question is, what is it that makes my code inefficient?

Redoing tons of work

Consider a number like 100. By the time $n gets to 100, we will have already done the work for $n == 50 (which has a length of 25). So rather than simply seeing that the result for $n was 25, and adding 1 (since we're one step away from that) -- we have to recalculate all 26 steps from scratch.

For that matter, we'd've already done $n as 33, so just by having gotten to 100, we should already know that it's at 26 without doing any work.

Save your state as you go along - this is known as memoization. Given that the resulting longest chain is of length 525, that's a lot of work that we'll end up saving. Just that one chain will end up with 525 operations instead of 138,075...


Scraping and analyzing recent articles on bitcoin This script scrapes recent articles on bitcoin, does sentiment analysis, and does some mock trading based on the sentiment of the articles.  I'm looking for advice on code style, and I would love to learn how to write beautiful Ruby.

require 'coinbase'
require 'sanitize'
require 'cgi'
require 'htmlentities'
require 'sentimental'
require 'simple-rss'
require 'open-uri'
require 'sqlite3'
require 'date'

@db = SQLite3::Database.open('instructions.db')
@db.execute("CREATE TABLE IF NOT EXISTS instructions2(Id INTEGER PRIMARY KEY AUTOINCREMENT, Date TEXT, Do_now TEXT, Do_later TEXT, Buy_price TEXT, Sell_price TEXT)")
@rss = SimpleRSS.parse(open('http://fulltextrssfeed.com/news.google.com/news?pz=1&cf=all&ned=us&hl=en&q=Bitcoin&output=rss'))
@coinbase = Coinbase::Client.new(ENV['COINBASEKEY'])
@coder = HTMLEntities.new
Sentimental.load_defaults
@analyzer = Sentimental.new
@ran_today = false
@scores_today = []
@log = File.open('log.txt', 'w')

def write_log(me)
  @log << DateTime.now.to_s + " : " + me.to_s
end

def check_sentiment
  @ran_today = true
  @rss.items.each do |item|
    @scores_today.push(@analyzer.get_score(clean(item.description)))
  end
  react_sentiment
end

def react_sentiment
  avg = @scores_today.reduce(:+).to_f / @scores_today.size
  do_now = :nothing
  do_later = :nothing
  #if negative, sell now, buy when low
  if avg > 3.0
    do_now = :buy
    do_later = :sell
  elsif avg < -2.0
    do_now = :sell
    do_later = :buy
  end
  write_log("day's sentiment score : " + avg.to_s)
  method(do_now).call
  write_log(@db.execute("INSERT INTO instructions2(Date, Do_now, Do_later, Buy_price, Sell_price) VALUES (?, ?, ?, ?, ?)", (Date.today+2).to_s, do_now.to_s, do_later.to_s, check_buy.to_s, check_sell.to_s))
end

def read_instructions
  @db.execute("SELECT Id, Date, Do_now, Do_later, Buy_price, Sell_price FROM instructions2;").each do |id, date, do_now, do_later, buy_price, sell_price|
    if Date.today.to_s == date
      if do_now == "buy"
        if check_buy < sell_price.to_f
          buy
        else
          puts "want to buy, buy " + buy_price + "  > sell " + sell_price
        end
      elsif do_now == "sell"
        if check_sell > buy_price.to_f
          sell
        else
         puts "want to sell, sell " + sell_price + " > buy " + buy_price
        end
      end
      @db.execute("DELETE * FROM instructions2 WHERE Date=?", Date.today.to_s)
    end
  end
end

def buy
  write_log("would buy at " + check_buy.to_s + " at " + Date.today.to_s)
end

def sell
  write_log("would sell at " + check_sell.to_s + " at " + Date.today.to_s)
end

def nothing
  write_log("would do nothing at " + Date.today.to_s)
end

def check_sell
  @coinbase.sell_price(1)
end

def check_buy
  @coinbase.buy_price(1)
end

def clean(stuff)
  Sanitize.clean(CGI.unescapeHTML(stuff).to_s.force_encoding('UTF-8'))
end

check_sentiment
read_instructions
@ran_today = true
while true
  if Time.now.hour == 23 and not @ran_today
    check_sentiment
  elsif Time.now.hour == 1
    read_instructions
  elsif Time.now.hour == 24
    @ran_today = false
    @scores_today = []
  end
  sleep(60000)
end

The @ variable prefix should only be used inside a class to denote instance variables. That said, for a script like this, you might like to wrap all the logic in a class (or rather a module since you don't need multiple instances).

Use string interpolation. So instead of

DateTime.now.to_s + " : " + me.to_s

you can write

"#{DateTime.now} : #{me}"


Prime number dictionary I attempted to write a read-only dictionary, containing primes:

#inspired by http://ceasarjames.wordpress.com/2011/07/10/the-quadratic-sieve/
class PrimeDict(dict):
    def __init__(self):
        super(PrimeDict,self).__setitem__(2,2)
        self.previous_max = 2

    def __setitem__(self, key, value):
        return self.__getitem__(key) #it's a read-only dict

    def __delitem__(self, key):
        return self.__getitem__(key) #it's a read-only dict

    def __find_primes(self,n):
        if n < self.previous_max: return
        sieve = range(self.previous_max,n)
        for x in self.keys():
            i = x * ((self.previous_max - 1)//x + 1)
            while i < n:
                sieve[i - self.previous_max] = 0
                i += x
        index = self.previous_max
        while index <= n ** 0.5:
            if sieve[index-self.previous_max]:
                i = index ** 2
                while i < n:
                    sieve[i-self.previous_max] = 0
                    i += index
            index += 1
        self.previous_max = n
        for x in sieve:
            if x:
                super(PrimeDict,self).__setitem__(x,x)

    def __getitem__(self,key):
        key = int(key)
        if key>= self.previous_max:
            self.__find_primes((key+1))
        try:
            return super(PrimeDict,self).__getitem__(key)
        except:
            return False

https://gist.github.com/aausch/6709819

Is there any way for me to make this class definition cleaner and/or shorter?

1. Comments on your code

No docstrings! What does your class do and how am I supposed to use it? Also, no doctests.
Wouldn't it be more natural for this class to be a set of primes, rather than a dictionary mapping primes to True and other keys to False?
There's only one set of primes, so this is a rare opportunity to make use of the Singleton pattern. That is, each call to PrimeDict() ought to return the same object.
By implementing __setitem__ like this:

def __setitem__(self, key, value):
    return self.__getitem__(key) #it's a read-only dict

you give callers the false impression that this operation succeeded:

>>> p = PrimeDict()
>>> p[1] = True
>>> # looks good, but:
>>> p[1]
False

Instead, you should raise an exception:

def __setitem__(self, key, value):
    raise TypeError("'PrimeDict' object doesn't support item assignment")

Similarly for __delitem__:

def __delitem__(self, key):
    raise TypeError("'PrimeDict' object doesn't support item deletion")

By deriving your class from Python's dict, you make all of the dict methods available. This means that a caller might accidentally bypass your attempt to make the dictionary read-only, by calling some other method that you haven't overridden. For example:

>>> p = PrimeDict()
>>> p.update({1: True})
>>> p[1]
True

The thing to do is not to subclass dict, but to subclass collections.abc.Mapping and have the actual dictionary be a member variable. Or, if you take my suggestion in point 2 above, subclass  collections.abc.Set and have the actual set as a member variable. I'll show how to do this in my revised code in section 2 below.
The method __find_primes has a private name. Why do you do that? The intended use of private names in Python is to "avoid name clashes of names with names defined by subclasses" but that's obviously not necessary here. All that the private name achieves here is to make your code a bit harder to debug:

>>> p = PrimeDict()
>>> p.__find_primes(10)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'PrimeDict' object has no attribute '__find_primes'
>>> p._PrimeDict__find_primes(10)
>>> p
{2: 2, 3: 3, 5: 5, 7: 7}

You start out by adding 2 to the set of primes and then set self.previous_max = 2. But it would be simpler to start out with the empty set of primes and self.previous_max = 1. The sieve of Eratosthenes is self-starting.
Instead of:

i = index ** 2
while i < n:
    sieve[i-self.previous_max] = 0
    i += index

make use of Python's range function, and write:

for i in range(index ** 2, n, index):
    sieve[i - self.previous_max] = 0

The code in __find_primes has to subtract self.previous_max from the array index every time it looks up something in sieve. It would be more efficient to do this subtraction once when setting up the loop bounds. For example, instead of the code that I suggested above:

for i in range(index ** 2, n, index):
    sieve[i - self.previous_max] = 0

you could write:

for i in range(index ** 2 - self.previous_max, n - self.previous_max, index):
    sieve[i] = 0

The line:

key = int(key)

can raise ValueError. Wouldn't it be better to return False if key can't be converted to an integer?

2. Revised code

try:
    from collections.abc import Set
except ImportError:
    from collections import Set # Python 3.2 or earlier

class PrimeSet(Set):
    """A PrimeSet object acts like the set of prime numbers:

        >>> p = PrimeSet()
        >>> 2 in p
        True
        >>> 4 in p
        False

    The object sieves for primes as needed. When iterated over, it
    yields the primes found so far:

        >>> 30 in p
        False
        >>> list(p)
        [2, 3, 5, 7, 11, 13, 17, 19, 23, 29]

    """
    # Singleton pattern: there is only one set of primes.
    instance = None
    def __new__(cls):
        if cls.instance is None:
            cls.instance = super(PrimeSet, cls).__new__(cls)
        return cls.instance

    def __init__(self):
        super(PrimeSet, self).__init__()
        self.max = 1            # Highest number sieved to so far.
        self.set = set()        # Set of primes up to self.max.

    def __iter__(self):
        return iter(self.set)

    def __len__(self):
        return len(self.set)

    def __contains__(self, key):
        try:
            key = int(key)
        except ValueError:
            return False
        if key > self.max:
            self.find_primes(key)
        return key in self.set

    def find_primes(self, n):
        """Sieve for primes from self.max + 1 up to and including n."""
        if n <= self.max:
            return
        sieve = [True] * (n - self.max)
        for p in self:
            for i in range(p - 1 - self.max % p, len(sieve), p):
                sieve[i] = False
        for p, isprime in enumerate(sieve):
            if isprime:
                p += self.max + 1
                self.set.add(p)
                for i in range(p * p - self.max - 1, len(sieve), p):
                    sieve[i] = False
        self.max = n


Farmer crossing as first exercise As a first step into the world of Alloy, I've done my own version of the Fox/Chicken/Grain problem. I'd be grateful for any comments (also, is there a better place to do this?).
I thought it worth separating out the Farmer from the other items so I don't have to keep removing it from the set of items.
open util/ordering[Time]

sig Time {}

enum Place {Near, Far}

abstract sig Locatable { location: Place one -> Time }
abstract sig Edible extends Locatable {}
one sig Fox, Chicken, Grain extends Edible {}
one sig Farmer extends Locatable {}

pred init(t: Time) { Locatable.location.t = Near }

pred done(t: Time) { Locatable.location.t = Far }

pred stayPut(t, t': Time, edibles: set Edible) {
    all e : edibles | e.location.t = e.location.t'
}

pred carryAcross(t, t' : Time) {
    one e: Edible {
        e.location.t = Farmer.location.t
        e.location.t' = Farmer.location.t'
        stayPut[t, t', Edible - e]
    }
}

pred crossRiver(t, t' : Time) {
    stayPut[t, t', Edible]
}

pred nextCrossing(t, t' : Time) {
    Farmer.location.t' != Farmer.location.t
    carryAcross[t, t'] or crossRiver[t, t']
}

pred eats [a, b: Edible] { a->b in Fox->Chicken + Chicken->Grain }

fact ProtectFromEating {
    all p, q: Edible, t: Time |
    p.eats[q] and p.location.t = q.location.t => q.location.t = Farmer.location.t
}

fact Traces {
    first.init
    all t: Time - last | let t' = t.next |
        done[t] or nextCrossing [t, t']
}

fact Done {
    some t: Time | done[t]
}

run {} for 8 Time

I am personally not a fan of the Time pattern, it gets messy quickly. There is an extension in the works, called electrum that allows you to use variables.
Overall your solution seems too detailed. The magic of alloy is that you only have to say what you want to achieve. I.e. the animals should always be safe and and somehow they need to end up on the other side. I.e. you don't care about the crossing itself that much as long as you ensure there is never a lunch happening.
My personal favorite solution for this problem is:
open util/ordering[Crossing]
enum Object { Farmer, Chicken, Grain, Fox }

let eats = Chicken->Grain + Fox->Chicken
let safe[place] = Farmer in place or (no place.eats & place)

sig Crossing {
    near, far : set Object,
    carry :  Object
} {
    near = Object - far
    safe[near]
    safe[far]
}

run {
    first.near = Object
    no first.far
    all c : Crossing - last, c': c.next {
        Farmer in c.near implies {
            c'.far = c.far + c.carry + Farmer
        } else {
            c'.near = c.near + c.carry + Farmer
        }
    }
    some c : Crossing | c.far = Object
} for 8

Peter Kriens @pkriens


Function for most frequent level of string or factor categorical variable in R I have a function that finds the most frequent level of string and factor variables. This is used in a data.table aggregation on Big Data, for non-numeric columns.

Right now the overall aggregation runs extremely slow. I'm trying to make improvements in other aspects as well (reducing observations and columns, etc) and I would like to see if there's a want to improve this function:

MaxTable <- function(InVec, mult = FALSE) {
  if (!is.factor(InVec)) InVec <- factor(InVec)
  A <- tabulate(InVec)
  if (isTRUE(mult)) {
    levels(InVec)[A == max(A)]
  }
  else levels(InVec)[which.max(A)]
}

Your function isn't very complicated so I don't know how much more efficient you can get.  However, maybe removing the if and else states may help.  How about something like this?

Code

# Sample Data
dat <- c("a", "a", "b", "c", "d","a", "a", "b", "c", "d","a", "a", "b", "c", "d","a", "a", "b", "c", "d","a", "a", "b", "c", "d")

# Mode function
MaxTable <- function(x){
     dd <- unique(x)
     dd[which.max(tabulate(match(x,dd)))]
}

Output

> MaxTable(dat)

[1] "a"

How many variables are you trying to process? You may want to think about writing a function that splits up the number of obs. for parallel processing, tabulate the mode for each section, merge each section, and then do which.max() .  Although, I'm not sure if you are reaching the limits of R.  Also, maybe a database such as mysql would be another option.


Reporting progress to main form's controls from within a threaded heavy task I'm trying to figure out how to nicely write a Winforms app that reports the progress of completing a long task onto the main form's controls without causing Cross-thread op exceptions and such. So far I've come up with this scheme which at least works, but doesn't look very nice:

Progress<MyProgress> progress = new Progress<MyProgress>();
private void Form1_Load(object sender, EventArgs e)
{
    progress.ProgressChanged += (x, y) =>
        Invoke(new Action(() => { toolStripStatusLabel1.Text = y.text;
                                  toolStripProgressBar1.Value = y.percentage; }));
}

bool DoVeryHeavyStuff(IProgress<MyProgress> progress)
{
    progress.Report(new MyProgress() { percentage = 0, text = "Doing heavy stuff…" });
}

async void button_doHeavyStuff_Click(object sender, EventArgs e)
{
    bool success = await Task.Run(() => DoVeryHeavyStuff(progress));
}

As you can see, the right part of the progress.ProgressChanged += looks overly complex. Isn't there a way it can be written more simply?

One thing I could think of is creating a separate method, but that wouldn't help much, as it would still have to be referred to in that line.

progress.ProgressChanged += (x, y) =>
    Invoke(new Action(() => progress_ProgressChanged(x, y)));

void progress_ProgressChanged(object sender, MyProgress e)
    {
        toolStripStatusLabel1.Text = e.text;
        toolStripProgressBar1.Value = e.percentage;
    }

I managed to break it apart like this, but it looks even worse:

progress.ProgressChanged += (x, y) => ReportProgressA(x, y);
void ReportProgressA(object sender, MyProgress e)
{
    Invoke(ReportProgressB(sender, e));
}
Action ReportProgressB(object sender, MyProgress e)
{
    return () => progress_ProgressChanged(sender, e);
}

Invoke seems to be essential to avoiding cross-thread exceptions, but it requires a delegate as a parameter. I don't have much experience with delegates, so I don't know how to simplify the Action initialization code.

It looks like I can't just drop the explicit cast to Action for some reason:

What you've provided is a lambda expression.

The Invoke method takes a delegate as an argument. Before I explain what a delegate is, the simple answer to your question is provide one of the predefined delegate types Action or Func. These may seem like magic, but trust me, they are super simple concepts once you wrap your head around them.

Invoke(new Action(() => {...})) Action takes no parameters and returns void

Invoke(new Func<bool>(() => { return false; })); Func<T> returns T and has no parameters.

Action and Func<T,...> are simply predefined delegates. Action is defined like so:

public delegate void Action();

What is a delegate?

A delegate plain and simple is a way to describe a method signature - that is, what does the method return (if anything) and what arguments (if any) does it take.

Another way to think about this is when I'm done doing my work I always notify someone to let them know the status of goals achieved. To make this process flexible I allow you to specify that unit of work. I will invoke the custom method and send it the status of my work. For this to happen obviously I need to set some general ground rules, the rules being that the method I invoke needs to take a string (the status I send you) and return a response to me (maybe that response is "ok go home" or etc).

The beauty of specifying only the method signature is that you can make your own custom method to be invoked (as you want to above) or you could specify any method that matches the signature. In our case string in, string out would match String.Reverse (just the name). Obviously in our scenario I may be confused by the response, but this way of programming allows you to plug in different implementations. One implementation could be that I notify you via phone call, or email, or text message etc.

Depending on your use case you might return void from the method because you just want an email to be kicked off once I internally invoke the method you've given to me.

You see a lot of this programming in thing like JavaScript, it is asynchronous, it returns control back to you right away and allows you to specify a callback function, essentially a delegate that should be invoked once it's done.


Class method with begin transaction and return errors I've created a method which is useful for executing multiple queries, and it works perfectly.  Is this a good way to handle this?

public function foo($rQuery)
{
    try {
        $this->done->beginTransaction();
        foreach($rQuery as $val) {
            $rSql = $this->done->prepare($val);
            $rSql->execute();
        }

        $commitCode = $this->done->commit();
    }
    catch (PDOException $ex) {
        $errorInfo = "Ooops: ".$ex->getMessage();

        $roalBackCode = $this->done->rollBack();
    }

    return array('errorInfo' => $errorInfo, 'commitCode' => $commitCode, 'roalBackCode' => $roalBackCode);
}

I think this is going to be a first, but yes. If I had to work on a project, and come across this code, I'd be happy. The only thing I'd change, perhaps, is the name of the PDO instance: $this->done doesn't say I am a DB connection, IMO. The catching of the exception is required here, but the fact that you're not re-throwing it, or throwing a new, more generic exception is debatable. I'll briefly explain why, but that's entirely up to you to decide

Other than that, I'd say you're pretty much on the money.
A couple of suggestions, though:

Type-hinting
You're assuming the user of your code will be so kind as to pass an array of strings to your function. He/she would have to look at your method to determine what arguments you're expecting. An easier way of doing this would be:

public function foo(array $queries)
{//this method clearly expects an array, pass something else, and it will crash
}

Strings
You're also assuming the array argument will be an array of strings, whereas there is no guarantee that'll be the case. If I were you, I'd do some checking/filtering:

$queries = array_filter($queries, 'is_string');

That will filter out any non-string elements from the argument array.

Notice: undefined variables
The array you're returning uses both the $commitCode and $errorInfo variables. Why? If the commitCode variable was set, then there will be no errorInfo. Include a return statement in the try block, or return true. If there was an error, then return the errorInfo.

return $this->db->commit();
//in catch:
return array('errorInfo' => $e->getMessage(), 'rollback' => $this->db->rollBack());

Or better still, rethrow the exception possibly contained within a more generic/custom exception object. The caller should be allowed to handle exceptions. The caller should know what was passed to your method, therefore any resulting exception is his responsibility:

catch (PDOException $e)
{
    throw new RuntimeException(
        'Not commited: '. $this->db->rollBack(),
        0,
        $e
    );
}

Prepared statements, no binds
Of course, this isn't your actual code, I take it, but you're creating prepared statements and then execute them with no (room for) binds.

If the query strings come from a trusted source, there's no need to create an instance of PDOStatement, IMO:

foreach($queries as $query)
{
    $this->db->exec($query);
}

will do, and it'll increase performance, too

Allow binds
You'll soon find this method lacking in usability, though. If I wanted to execute a number of SELECT * FROM tbl WHERE ... or UPDATE or INSERT or any query with parameters for that matter, I can't use your method.
Don't worry, though, for that's an easy fix:

public function foo(array $queries)
{
    try
    {
        foreach($queries as $query)
        {
            if (is_array($query))
            {
                $stmt = $this->db->prepare($query['prepare']);
                $stmt->execute($query['bind']);
            }
            else
            {
                if (is_string($query)) $this->db->exec($query);
            }
        }
        return $this->db->commit();
    }
    catch (PDOException $e)
    {
         return array(
             'errorInfo' => $e->getMessage(),
             'rollbackCode' => $this->db->rollBack()
         );
    }
}

That way, I'm able to call this method like so:

$instance->foo(
    array(
        'SELECT * FROM someTable',
        array(
            'prepare' => 'SELECT * FROM anotherTable WHERE someField = :field',
            'bind'    => array(':field' => $val)
    )
);

Of course, this implementation still depends on the user being kind enough to pass the correct arrays to your method. If I were you, I'd create an "Argument class":

class QueryArgument
{
    private $query = null;
    private $bind = null;
    public function __construct($query = null, array $bind = null)
    {
        if (is_array($query))
        {//allow user to pass array('prepare' => '', 'bind')
            $bind = isset($query['bind']) && is_array($query['bind']) ? $query['bind'] : null;
            $query  = isset($query['prepare']) ? $query['prepare'] : null;
        }
        $this->query = is_string($query) ? $query : null;
        $this->bind = null;
    }
    //this is just to make life easier
    public function getTransactionFormat()
    {
        if ($this->bind)
        {
            return array(
                'prepare' => $this->query,
                'bind'    => $this->bind
            );
        }
        return $this->query;
    }
    //implement getters and setters
    public function setBind(array $bind)
    {
        $this->bind = $bind;
        return $this;
    }
    public function getBind()
    {
        return $this->bind;
    }
}

Now you can use this like so:

public function foo(array $queryArgs)
{
    $queries = array_filter($queryArgs, array($this, 'filterQueries'));
    try
    {
        foreach($queries as $query)
        {//same as before
            if (is_array($query)) {/* ... */}
            else {/* ... */}
        }
    }
    catch (PDOException $e) {}
}
//filter out only those elements that are instances of QueryArgument
//convert them to array or string
private function filterQueries($instance)
{

    if ($instance instanceof QueryArgument) return $instance->getTransactionFormat();
    return false;
}


A background service that will wait for items to process without busy-waiting Problem space: I have the concept of a Job that needs to be processed. Jobs need to be processed sequentially, as one may depend on another. It's possible for a Job to be paused, in which case the next Job should not be started - the Job should either be explicitly continued, or cancelled, in order to get the next one to start.
If the background service doing these Jobs finishes, it should wait for an update to a RESTful resource before trying to start processing again. I don't want the service to busy-wait.
public class ExecutionBackgroundService : BackgroundService
{
    private readonly IJob _job;
    private readonly AutoResetEvent _isRunning = new AutoResetEvent(true);

    public ExecutionBackgroundService(IJob job)
    {
        _job = job;
    }

    protected override async Task ExecuteAsync(CancellationToken stoppingToken)
    {
        // Required for the method to be executed asynchronously, allowing startup to continue.
        await Task.Yield();
        while (!stoppingToken.IsCancellationRequested)
        {
            _isRunning.WaitOne(); // blocks the thread, waiting for the event to be set by something else calling the Resume method
            await _job.Process();
        }
    }

    /// <summary>
    /// Notifies the background service to resume processing.
    /// </summary>
    public void Resume()
    {
        _isRunning.Set();
    }
}

Jobs are represented by a DB entity. The IJob implementation looks something like this:
    public async Task Process()
    {
        using var scope = _serviceScopeFactory.CreateScope();
        var database = scope.ServiceProvider.GetService<MyDbContext>();
        var iterator = new DatabaseJobIterator(database);
        var worker = scope.ServiceProvider.GetService<IWorker>();
        while (iterator.HasNext())
        {
            await worker.DoLongRunningWork(iterator.Next);
        }
    }

Where the implementing class is singleton-scoped, and the DB and IWorker are both request-scoped (hence why I'm getting them using the scope factory).
DatabaseJobIterator is basically a simplified IEnumerator that just exposes two methods HasNext and Next, where HasNext will query the database for the oldest job, and assign it to Next. If the job is ready to be processed, it returns true. If the job is paused, it returns false.
An API controller that handles POSTs and PUTs to Jobs will then just call Resume on the background service, indicating that it might have new items to process if it isn't already running.
My question is whether I've used the AutoResetEvent correctly. Is there a more standard way to get the background service to wait without e.g. busy-waiting in a while (true) loop?

I suggest SemaphoreSlim instead of AutoResetEvent here because it has asynchronous API and will not block the current thread, and it accepts CancellationToken. Task.Yield is good hack but it can be useless because await in case of not null SynchronizationContext will restore the context and WaitOne() can anyway block the current thread.
public class ExecutionBackgroundService : BackgroundService
{
    private readonly IJob _job;
    private readonly SemaphoreSlim _semaphore = new SemaphoreSlim(1); // 1 is concurrency degree

    public ExecutionBackgroundService(IJob job)
    {
        _job = job;
    }

    protected override async Task ExecuteAsync(CancellationToken stoppingToken)
    {
        try
        {
            while (true)
            {
                await _semaphore.WaitAsync(stoppingToken);
                await _job.Process();
            }
        }
        catch (OperationCanceledException ex)
        { } // occurs when WaitAsync receives canceling callback from the Token, you may log it if needed.
    }

    /// <summary>
    /// Notifies the background service to resume processing.
    /// </summary>
    public void Resume()
    {
        lock (_semaphore)
        {
            if (_semaphore.CurrentCount == 0) // is it waiting now?
                _semaphore.Release();
        }
    }
}

This way is better because now you may cancel the Token without calling the Resume().
Btw it's a programming pattern - Producer/Consumer. The best way for moving forward is using the specially designed for this purpose almost brand-new classes - System.Threading.Channels. Refer to this link.


Get real time geojson on Javascript I frequently need to obtain some info about coordinates, because I have dynamic elements on a map (in Openlayers). So I use SetInterval and XMLHttpRequest Object to do it.

window.onload = function () {
    var requestURL = "data.geojson";
    var request = new XMLHttpRequest();
    request.onreadystatechange = function () {
        if (request.readyState == 4) {
            console.clear();
            console.log(request.response);
        }
    }
    var prueba = setInterval(function () {
        request.open('GET', requestURL, true);
        request.setRequestHeader('authenticated_user', 'terrain');
        request.send();
    }, 2000);
}

Is It the correct way?

First of all, readyState 4 simply means XHR is done. The response, however, can still either be a success (2xx) or a failure (4xx, 5xx). In order to distinguish a success from an error, you should also check status.

Next would be the reuse of the XHR instance. Although it's perfectly valid (open reinitializes the instance), you'll have to be absolutely sure subsequent calls aren't affected by the previous calls. It's probably much safer to just create a new instance of XHR for each request.

Then instead of window.onload, consider using addEventListener. The issue is that there might be other scripts assigning a callback to window.onload, overwriting your handler. On the other hand, multiple calls to addEventListener will just append handlers.

A simpler way to do it, assuming your target audience doesn't use legacy browsers, is fetch. It's essentially jQuery's $.ajax but natively implemented.

window.addEventListener('load', event => {
  setInterval(() => {
    fetch('data.geojson', {
      method: 'GET',
      headers: { authenticated_user: 'terrain' }
    }).then(() => {
      console.clear();
      console.log(request.response);
    })
  }, 2000)
})


Route structure for multiple associations in Rails I'm doing a site at the moment (first Rails site, learning as I code) and I'm concerned that I'm over complicating the routes.

My associations boil down to:

class Recipe < ActiveRecord::Base
    has_and_belongs_to_many :categories
    belongs_to :book
end

class Category < ActiveRecord::Base
    belongs_to :category_type
    has_and_belongs_to_many :recipes
end

class Book < ActiveRecord::Base
  has_many :recipes
end

I want to end up with URLs like this:

/books/3/recipes  #to show all recipes from book 3
/category/12/recipes #to show all categories from category 12
/recipes/3  #to show recipe 3

The good news is I have this all working, but in the course of doing so I feel like I may have strayed a little from "the Rails way" and I was wondering if someone could take a look and let me know if I've gone wrong. The bits I'm particularly concerned about are my routes.rb file (because I appear to be repeating myself):

resources :books do
  resources :recipes
end

resources :categories do
  resources :recipes
end

In particular, the recipes#index action, which seems a little verbose:

def index

  if(params[:category_id])
    @recipes = Recipe.find_by_category(params[:category_id])
  elsif(params[:book_id])
    @recipes = Recipe.where(:book_id => params[:book_id])
  else
    @recipes = Recipe.find(:all)
  end
end

Now, it might be that this is all absolutely fine, but I just want to check before I get too much further in!

This is probably fine, though there are other ways to do it like passing filters to /recipes (?category_id=x or ?book_id=x). Your controller action is probably fine too, though I may have considered making multiple controllers.

Though i'm not sure why you're doing find_by_category instead of the a find_all_by_category or the same .where as you're doing for book_id. Also, Recipe.all is more rails 3.


Retrieving values from UITableViewCell I have a UITableview with custom cells, every cell has a button called cellButton. When the user taps the button I present an action sheet which has two buttons, buttonOne and buttonTwo. When buttonOne has been tapped I call a method with some values from the actual cell, when buttonTwo I perform a segue and prepareForSegue: method to pass data from the table view cell to another VC. I have a working solution, but I'm not sure that this is the best way because I'm passing the PFUser object from the index path to an instance variable and only use it when the user choose something in the action sheet. I never needed passing values like this, I always used directly the NSIndexPath *indexPath = [self.tableView indexPathForSelectedRow];.. way in prepareForSegue:, therefore I'm a bit confused now. Is it a correct way or can it go wrong in any case?

@property (nonatomic, strong) PFUser *userObj;

-(UITableViewCell *) tableView:(UITableView *)tableView cellForRowAtIndexPath:(NSIndexPath *)indexPath {

    CustomTableViewCell *cell = [self.tableView dequeueReusableCellWithIdentifier:@"cell"];

    PFUser *user = [self.dataSourceArray objectAtIndex:indexPath.row];
    cell.usernm.text = user.username;
    cell.userId.text = user.objectId;

    cell.cellButton.tag = indexPath.row;
    [cell.cellButton addTarget:self action:@selector(buttonTapped:) forControlEvents:UIControlEventTouchUpInside];
    return cell;
}

- (void)buttonTapped:(id)sender {

    UIButton *button = (UIButton *)sender;
    self.userObj = [self.dataSourceArray objectAtIndex:button.tag];
   [self setupActionSheet];

}

- (void) setupActionSheet {

    AHKActionSheet *actionSheet = [[AHKActionSheet alloc] initWithTitle:NSLocalizedString(nil, nil)];

    [actionSheet addButtonWithTitle:NSLocalizedString(@"Button 1", nil)
                              image:[UIImage imageNamed:@"1"]
                               type:AHKActionSheetButtonTypeDefault
                            handler:^(AHKActionSheet *as) {

                                [Utilz checkUser:self.userObj.objectId with:self.userObj.username];

                            }];

    [actionSheet addButtonWithTitle:NSLocalizedString(@"Button 2", nil)
                              image:[UIImage imageNamed:@"2"]
                               type:AHKActionSheetButtonTypeDefault
                            handler:^(AHKActionSheet *as) {

                                [self performSegueWithIdentifier:@"presentNextView" sender:self];

                            }];

    [actionSheet show];

}

- (void)prepareForSegue:(UIStoryboardSegue *)segue sender:(id)sender {
    if ([[segue identifier] isEqualToString:@"presentNextView"]) {
        if ([[segue destinationViewController] isKindOfClass:[ViewControllerTwo class]]) {

            ViewControllerTwo *dataToPass = [segue destinationViewController];

            dataToPass.usrStr = self.usrObj.username;
            dataToPass.objStr = self.usrObj.objectId;

        }
    }
}

Let's look at tableView:cellForRowAtIndexPath:.  This needs to be abstracted quite a bit more.
First of all, CustomTableViewCell is a bad name for a class.  Every table view cell is custom.  We need to spend more time working on our cell subclass, because the name is just the start.
Let's change our cell.  Instead of exposing our UI elements, we should create a property for a PFUser and a delegate.  We also need to create an enum in our cell's header to send to our delegate, and a protocol for our delegate to conform to.
Enum:
typedef NS_ENUM(NSInteger, MyUserCellAction) {
    MyUserCellActionCheckUser,
    MyUserCellActionPerformSegue
};

Of course, this needs more accurate, more descriptive names, but only you know what your app does, so be sure to pick some really good names.  It shouldn't be "PerformSegue", but should be whatever action is the reason we're performing the segue... are we logging in?
Protocol:
@protocol MyUserCellDelegate <NSObject>

@optional - (void)myUser:(PFUser *)user didPerformAction:(MyUserCellAction)action;

@end

Again, this needs slightly better naming, because the class shouldn't be called "MyUserCell".  We need a name that is more descriptive, but the protocol method should be something about like this.  An optional method which passes back the user for which this action is being performed, and the action out of the enum we just defined.
@interface:
@interface MyUserCell : UITableViewCell

@property (weak) id<MyUserCellDelegate> delegate;
@property PFUser *user;

@end

And we might have other properties in here, but importantly, we should be exposing the UI elements like the buttons or various text boxes.  The cell should take care of setting up its own view.
Whenever the user property is set, we should set up the dynamic parts of our UI (the labels).
- (void)setUser:(PFUser *)user {
    _user = user;
    // setup UI
}

Our cell's button should already be tied to an IBAction within the cell.  Either we set the cell up in an interface builder and link it from there, or we create it entirely programmatically and add the action in init.
- (IBAction)buttonTapped:(id)sender {
    // cell handles its button being tapped
}

Here, we can preset our action sheet.
In one handler block, we'll put the following snippet:
if ([self.delegate respondsToSelector:@selector(myUser:didPerformAction:)]) {
    [self.delegate myUser:self.user didPerformAction:MyUserCellActionCheckUser];
}

And in the other, this snippet:
if ([self.delegate respondsToSelector:@selector(myUser:didPerformAction:)]) {
    [self.delegate myUser:self.user didPerformAction:MyUserCellActionPerformSegue];
}

Now, all that's left is setting something up as the delegate.
In the .h file with the @interface section, we need to declare the object as conforming to the protocol:
@interface MyDelegateClassViewController : UIViewControll <MyUserCellDelegate>

And do all of our normal stuff, but then in the .m file, we need to include the following method if we wish to handle the action of the button being pressed:
- (void)myUser:(PFUser *)user didPerformAction:(MyUserCellAction)action {
    if (action == MyUserCellActionCheckUser) {
        // do what we want
    } else if (action == MyUserCellActionPerformSegue) {
        // do the other thing we want
    }
}

And now, our cell and table and view controller aren't so tightly coupled.  If we want to add more actions, we can do that.  If we want to add some actions that might just update the PFUser object, the cell can handle that without calling to the delegate.  If we want to use a different delegate for our even number cells than for our odd number cells, we can do that and easily get different actions despite the same set of buttons.


Parse a contrived message protocol with first byte data length followed by data I think this code to parse a contrived message protocol with first byte data length followed by data is a little ugly.  Has anyone got any suggestions on how to make it more elegant and more robust?

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

#define LENGTHBYTES 1

void splitmessages(unsigned char* bstream, int length);
void printmsg(unsigned char* msg);

int main() {

   unsigned char stream[] = {2, 'A', 'A', 1, 'B', 3, 'C', 'C', 'C', 1, 'D' };
   int len = sizeof(stream) / sizeof(stream[0]);

   printf("\nTest 1 - four messages together\n");
   splitmessages(stream, len); //works

   printf("\nTest 2 - two complete msgs, then 2 complete msgs\n");
   splitmessages(stream, len-2);  //works
   splitmessages(stream+9, 2);

   //split across a message - in data
   printf("\nTest 3 - messages split across data part\n");
   splitmessages(stream, len-4);  //works
   splitmessages(stream+7, 4);

   //split across a message - just after size
   printf("\nTest 4 - messages split just after size part\n");
   splitmessages(stream, len-5);
   splitmessages(stream+6, 5);

   //split across a message - just before size
   printf("\nTest 5 - messages split just before size part\n");
   splitmessages(stream, len-6);
   splitmessages(stream+5, 6);

   return 0;
}

void splitmessages(unsigned char* bstream, int length) {
   //Create msg as soon as you have size and save as static
   static unsigned char* msg = 0;
   static int tempbufposition = 0;
   static int tocompletemsg = 0;

   enum STAGE { SIZE, DATA };
   STAGE stage = SIZE;
   int size = 0;

   //adjust stage to DATA if already have start of msg
   if (tocompletemsg != 0)
      stage = DATA;

   //get cached size
   if(tempbufposition != 0 && msg)
      size = msg[0];

   for(int i=0; i<length; ++i){
     switch(stage) {
     case SIZE:
       size = bstream[i];
       if(msg) {
            free(msg);
            msg = 0;
            tempbufposition = 0;
            tocompletemsg = 0;
        }
        //re-create complete msg so add 1 byte for size
        msg = (unsigned char*)malloc(size+LENGTHBYTES);
        msg[0] = size;
        tempbufposition = 1;
        tocompletemsg = size;

        stage = DATA;
      break;
      case DATA:
         //do we have enough data for a complete message
         if(i + tocompletemsg <= length) {
            memcpy(msg+tempbufposition, &bstream[i], tocompletemsg);
            printmsg(msg);
            stage = SIZE;
            i += tocompletemsg-1;
            tocompletemsg = 0;
            tempbufposition = 0;
         } else {
            //add available data
            memcpy(msg+tempbufposition, &bstream[i], length-i);
            tempbufposition +=(length-i);
            tocompletemsg -= (length-i);
            i += (length-i)-1;
         }

           break;
         }
   }
}

void printmsg(unsigned char* msg) {
   if(msg && *msg) {
       int size = msg[0];
       printf("%d ", msg[0]);
       for(int i = 1; i <= size; ++i)
          printf("%c ", msg[i]);

       printf("\n");
   }
}

Below is an alternative example of how messages could be extracted.  Instead of a state machine with saved-state inside your splitmessages function, I have moved the saved-state out of the function into a structure that is passed-in.   I have also checked for unexpected data at the start of the function.  The return value is 0 on failure or the number of bytes consumed from the input stream on success.  I didn't check for size bytes occurring in the wrong place - replace the memcpy with a function that checks the bytes copied for that.

Note that your protocol has to distinguish between data bytes and size bytes. I have arbitrarily made the boundary 32, the first printable char.

#include <string.h>
#include <assert.h>

#define DATA_MIN_VALUE 32
#define DATA_MAX_SIZE  (DATA_MIN_VALUE)

struct message {
    size_t size;
    size_t remaining;
    char data[DATA_MAX_SIZE];
};
typedef struct message Message;

static inline size_t min(size_t a, size_t b)
{
    return a < b ? a : b;
}

static inline int is_data(unsigned char ch)
{
    return  ch >= DATA_MIN_VALUE;
}

static size_t
get_message(Message *msg, const unsigned char *bstream, size_t length)
{
    size_t len;
    assert(length);

    if ((msg->remaining == 0) && is_data(bstream[0])) {
        /* Data instead of message; caller must handle - eg. discard data up
         * to next message */
        return 0;
    }

    if (is_data(bstream[0])) {
        msg->size = 0;
        msg->remaining = bstream[0];
    }

    len = min(length-1, msg->remaining);

    memcpy(&msg->data[msg->size], &bstream[1], len);
    msg->size += len;
    msg->remaining -= len;

    return len + 1;
}

The code should compile but is untested.  Hope it helps to have another perspective.


Distribute an array of numbers equally The user will input two numbers, say 19 and 5. We need to equally divide 19 equally into 5 groups and output the start index and the end index of each group (using zero-based indexes). Example:

  Grp 1: 0   1   2   3

  Grp 2: 4   5   6   7

  Grp 3: 8   9   10  11

  Grp 4: 12  13  14  15

  Grp 5: 16  17  18

The output should be

  Grp 1: 0   3

  Grp 2: 4   7

  Grp 3: 8   11

  Grp 4: 12  15

  Grp 5: 16  18

I have written the program. But I feel it is not good. Can you please tell me if there is any other way.

#include<stdio.h>
#define A 19
#define B 5
#define PER_ENTRY A/B
struct divide {
    int start;
    int end;
};
void calc_even(struct divide *div, int no)
{
    int remain = A % B ;
    int extra_add = 1;
    if (no > remain) {
        extra_add = 0;
    }
    static int priv_end = 0;
    if (no == 1) {
        div->start = 0;
        div->end = (PER_ENTRY - 1) + extra_add ;
    } else {
        div->start = priv_end + 1;
        if (no == B) {
            div->end = A - 1;
        } else {
            div->end = div->start + (PER_ENTRY - 1) +  extra_add ;
        }
    }
    priv_end = div->end;
}
int main()
{
    int i = 0;
    struct divide num_diff[B];

    for (i = 1 ; i <= B ; i++ ){
        num_diff[i-1].start = 0;
        num_diff[i-1].end = 0;
        calc_even(&num_diff[i-1], i);
    }
    for (i = 1 ; i <= B ; i++ ) {
        printf("Grp%d: %d %d \n", i, num_diff[i-1].start, num_diff[i-1].end);
    }
}

Can anyone tell me how the solution could be optimized?

There are a number of things that I see that may help you improve your program.

Avoid needless recalculation

There is a lot of calculation within the calc_even routine that doesn't need to be repeated for every step through the loop.  Also, there are two separate loops in main -- one the calculates and stores the results and another that prints them.  There is no reason that the results couldn't simply be generated and printed within a single loop.  In addition to being more efficient in terms of computation, it would also use less memory since there would be no need to save the results.  They can simply be calculated and discarded.

Prefer const to #define

The use of #define A 19 is better expressed as const int A = 19; because the latter has a defined type, while a #define does not enforce any notion of type.

Think of the user

The program as currently coded needs to be recompiled if we want to try any other pair of numbers.  It would be better to accept those parameters from the command line rather than making them fixed.

Use library functions

The program calculates both a quotient and remainder for a given pair of numbers.  There is already a library function div that does that in a single step.

Understand integer conversion rules

This code can be greatly simplified:

int extra_add = 1;
if (no > remain) {
    extra_add = 0;
}

A boolean value, when converted to an int in C is defined as 0 if it is false and 1 if it is true, so that bit of code can be expressed instead as:

int extra_add = (no <= remain);

Use more descriptive names

The names you have chosen are generally not too bad, but A, B and no are not very good names.

Reduce complex logic

The calc_even calculations are entirely too complex for the requirements of the program.  Think first about how you'd do this with pencil and paper.  If we have 5 bins and 19 numbers, we divide to get a quotient (3) and remainder (4).  So that means that all bins will have at least 3 numbers and 4 of the bins will have one extra.

Putting it all together

Applying all of these suggestions, an alternative might look like this:

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

int main(int argc, char *argv[])
{
    if (argc < 3) {
        puts("Usage: divide number bins");
        return 0;
    }
    int number = atoi(argv[1]);
    if (number <= 0) {
        puts("Error: number must be >= 0");
        return 1;
    }
    int bins = atoi(argv[2]);
    if (bins <= 0) {
        puts("Error: bins must be >= 0");
        return 1;
    }
    div_t qr = div(number, bins);
    int begin = 0;
    for (int i=0; i < bins; ++i) {
        int end = begin + qr.quot + (i < qr.rem);
        printf("Grp %d: %d %d\n", i+1, begin, end-1);
        begin = end;
    }
}


Simple Python job vacancies downloader I have created a BeautifulSoup vacancies parser, which works, but I do not like how it looks. Therefore, I'd be very happy if somebody could give me any improvements.

from urllib.request import urlopen
from bs4 import BeautifulSoup

page = urlopen('http://rabota.ua/zapros/python/%D0%BA%D0%B8%D0%B5%D0%B2')
soup = BeautifulSoup(page, "lxml")
vacancies = {}

a = soup.find_all("a", class_="t")
inx = 0
for x in a:
    inx += 1
    vacancies[inx] = {'position': x.contents[0].strip(),
                      'link': 'http://rabota.ua{}'.format(x.get('href'))}

a = soup.find_all("a", class_="rua-p-c-default")[2:]
inx = 0
for x in a:
    inx += 1
    vacancies[inx].update({'company': x.get_text()})

for x in vacancies:
    print('{}\n{}\n{}\n'.format(vacancies[x]['company'], vacancies[x]['position'], vacancies[x]['link']))

Here is the HTML code:

A couple of suggestions:

Use requests instead of urllib. This is mostly a matter of preference but I think you'll find it easier to work with.

An example on how this would look is:

import requests

page = requests.get('http://rabota.ua/zapros/python/%D0%BA%D0%B8%D0%B5%D0%B2').content

Fix the bug

In some cases you'll get:

  Traceback (most recent call last):
  http://rabota.ua/company209907/vacancy6312501
      vacancies[x]['link'])) UnicodeEncodeError: 'ascii' codec can't encode characters in position 26-33: ordinal not in range(128)

Which means you're trying to parse characters which cannot be decoded as they are. I'd recommend using the encode function to get rid of it:

for x in vacancies:
    print('{}\n{}\n{}\n'.format(
        vacancies[x]['company'].encode('ascii', 'ignore'),
        vacancies[x]['position'].encode('ascii', 'ignore'),
        vacancies[x]['link'].encode('ascii', 'ignore')))

Instead of using a counter, you could use the builtin that Python already has: enumerate().

This:

inx = 0
for x in a:
    inx += 1
    vacancies[inx].update({'company': x.get_text()})

Will become:

for counter, x in enumerate(a, 1):
    vacancies[counter].update({'company': x.get_text()})

So far, we have this:

from bs4 import BeautifulSoup
import requests

page = requests.get('http://rabota.ua/zapros/python/%D0%BA%D0%B8%D0%B5%D0%B2').content
soup = BeautifulSoup(page, "lxml")

a = soup.find_all("a", class_="t")

vacancies = {}
for counter, x in enumerate(a, 1):
    vacancies[counter] = {
        'position': x.contents[0].strip(),
        'link': 'http://rabota.ua{}'.format(x.get('href'))
    }

a = soup.find_all("a", class_="rua-p-c-default")[2:]

for counter, x in enumerate(a, 1):
    vacancies[counter].update({'company': x.get_text()})

for x in vacancies:
    print('{}\n{}\n{}\n'.format(
        vacancies[x]['company'].encode('ascii', 'ignore'),
        vacancies[x]['position'].encode('ascii', 'ignore'),
        vacancies[x]['link'].encode('ascii', 'ignore')))

Other suggestions:

I'm not a fan of your naming conventions:

a might become href_tag
x might become element

More, I'd split everything into functions as this will make your code readable and easier to maintain:

from bs4 import BeautifulSoup
import requests

URL = 'http://rabota.ua/zapros/python/%D0%BA%D0%B8%D0%B5%D0%B2'

def get_html():
    return BeautifulSoup(requests.get(URL).content, 'lxml')

def parse_html():
    content = get_html()
    href_tag = content.find_all('a', class_='t')

    vacancies = {}
    for counter, element in enumerate(href_tag, 1):
        vacancies[counter] = {
            'position': element.contents[0].strip(),
            'link': 'http://rabota.ua{}'.format(element.get('href'))
        }

    href_tag = content.find_all("a", class_="rua-p-c-default")[2:]

    for counter, element in enumerate(href_tag, 1):
        vacancies[counter].update({'company': element.get_text()})

    for element in vacancies:
        print('{}\n{}\n{}\n'.format(
            vacancies[element]['company'].encode('ascii', 'ignore'),
            vacancies[element]['position'].encode('ascii', 'ignore'),
            vacancies[element]['link'].encode('ascii', 'ignore'))
        )

if __name__ == '__main__':
    parse_html()

Yet another way of splitting the logic of your program:

from bs4 import BeautifulSoup
import requests

URL = 'http://rabota.ua/zapros/python/%D0%BA%D0%B8%D0%B5%D0%B2'

def get_html():
    return BeautifulSoup(requests.get(URL).content, 'lxml')

def get_position_and_link(content):
    href_tag = content.find_all('a', class_='t')

    vacancies = {}
    for counter, element in enumerate(href_tag, 1):
        vacancies[counter] = {
            'position': element.contents[0].strip(),
            'link': 'http://rabota.ua{}'.format(element.get('href'))
        }

    return vacancies

def update_info(content):
    company_href_tag = content.find_all("a", class_="rua-p-c-default")[2:]
    vacancies = get_position_and_link(content)

    for counter, element in enumerate(company_href_tag, 1):
        vacancies[counter].update({'company': element.get_text()})
    return vacancies

def main():
    content = get_html()
    vacancies = update_info(content)

    for element in vacancies:
        print('{}\n{}\n{}\n'.format(
            vacancies[element]['company'].encode('ascii', 'ignore'),
            vacancies[element]['position'].encode('ascii', 'ignore'),
            vacancies[element]['link'].encode('ascii', 'ignore'))
        )

if __name__ == '__main__':
    main()

You can see that I also added if __name__ == '__main__'. By doing the main check, you can have that code only execute when you want to run the module as a program and not have it execute when someone just wants to import your module and call your functions themselves.

As @Mathias suggested in comments, instead of using the encode() function, you might as well do:

def main():
    content = get_html()
    vacancies = update_info(content)

    for element in vacancies:
        for info in ('company', 'position', 'link'):
            print(vacancies[element][info])
        print('\n')

The advantage of this method might appear when you have a lot of unrecognised ascii characters which, with the first version, may lead to something like: .,..


Prepared PHP statement to fetch posts for some category I am slowly learning PHP and have been using prepared statements.

I have a simple category.php page, which takes a category tag to display posts from that category, such as localhost/php/category.php?tag=sysadmin. I want to select the Category Title from the "j_oracle_cat" table based on the value of the incoming tag in the querystring.

My code seems to be extremely cumbersome and long-winded (I'm using MySQLi instead of PDO):

<?php
/* connection string up here */

/* get category tag */
$foo = $_GET['tag'];

/* if it contains an apostrophe go to homepage */
$foo2 = strpos($foo, '\'');
if ($foo2 > 0) { Header("Location: default.php"); }

/* sql */
$cat_title_sql = "SELECT fld_title FROM j_oracle_cat WHERE fld_tag = ? LIMIT 1";

/* category variable */
$foo_cat = $foo;

/* initialise and prepare */
$stmt = $conn->stmt_init();
$result_cat_title = $stmt->prepare($cat_title_sql);
if(!$result_cat_title) { trigger_error("Error preparing statement: $stmt->error
SQL query: $cat_title_sql", E_USER_ERROR); }

/* bind */
$result = $stmt->bind_param('s',$foo_cat);
if(!$result) { trigger_error("Error binding parameter: $stmt->error
", E_USER_ERROR); }

/* execute */
$stmt->execute();

/* store */
$stmt->store_result();
$count_cat = $stmt->num_rows;

/* if no results go to homepage */
if ($count_cat == 0) {
    Header("Location: ../default.php");
    } elseif ($result) {
        $stmt->bind_result($fld_title);
        while ($stmt->fetch()) {
            $cat_ttl = $fld_title;
        }
}

/* ... header include down here */
?>
<title><?php echo $cat_ttl?></title>

I just wondered if I am making a meal of this, and if the code is over the top, or if I am on the right track (both with the prepared statement and handling the incoming querystring variable).

$foo2 = strpos($foo, '\'');
if ($foo2 > 0) { Header("Location: default.php"); }

Creating another variable for this, especially with a generic name like "foo2", is superfluous and should be avoided. if (strpos(..) > 0) will do just fine.
Just redirecting with a header does not stop the script execution. Your script will continue to run, making this rather pointless. You need to exit explicitly.
This check will fail if the quote is the first character, since strpos returns 0 in that case.
Why are you worrying about quotes in particular at all? There's really no point to this.

What you may want to check for instead is whether the value is defined at all.

$foo_cat = $foo;

There's no point in reassigning this variable yet again.

$result_cat_title = $stmt->prepare($cat_title_sql);
if(!$result_cat_title) ...

Using yet another rather mis-named variable is pretty superfluous, again.

trigger_error(..)

If you're using a (poorly) user-defined error handler (or somebody who may be using this code in the future adds one), your script may not actually exit when you trigger an error. You should either still explicitly exit after this, or perhaps switch to exceptions.

$count_cat = $stmt->num_rows;

Again a rather superfluous variable assignment.

Header("Location: ../default.php");

Again a header which does not terminate script execution.
You should also consider indicating a 404 Not Found error instead of a silent redirect.

else if ($result)

Why are you checking $result here? That was a variable used in completely different context. Also, since the previous header statement was supposed to terminate the script, you can skip this check entirely.

while ($stmt->fetch())

You're only expecting a single result, no need for a loop.

$cat_ttl = $fld_title;

Yet another superfluous variable assignment.

<title><?php echo $cat_ttl?></title>

You should take care of HTML injection/syntax issues and HTML-encode your output appropriately.

Beyond this indentation and variables names are often sub-optimal.

In summary:

<?php

if (!isset($_GET['tag'])) {
    header('HTTP/1.0 400 Bad Request');
    exit;
}

/* connection string up here */

$stmt = $conn->stmt_init();

$query = 'SELECT ld_title FROM j_oracle_cat WHERE ld_tag = ? LIMIT 1';
if (!$stmt->prepare($query)) {
    throw new Exception("Error preparing statement: $stmt->error, SQL query: $query");
}

if (!$stmt->bind_param('s', $_GET['tag'])) {
    throw new Exception("Error binding parameter: $stmt->error");
}

$stmt->execute();
$stmt->store_result();

if ($stmt->num_rows == 0) {
    header('HTTP/1.0 404 Not Found');
    exit;
}

$stmt->bind_result($title);
$stmt->fetch();

?>
<title><?php echo htmlspecialchars($title); ?></title>


Event driven timed I/O The following code is used to access a shield which converts settings written over I2C to Servo output.

I make use of the MRAA library since that's the default supported by my hardware.

To prevent my I2C commands from being written all at the same time, I use setTimeout to wait a given amount of time before executing the next.

For those unfamiliar with MRAA and I2C:

writeReg accepts two arguments. The first is the target register, the second is the data. As long as two different registers are being written, it's no problem if one is written earlier than the other. However, I want to make this code more generic so it can be used with something different than Servos as well. A generic solution should give me the possibility to burst multiple parts of data to the same address and keep the order intact.

Save to say my current structure is not easily expandable. How should I make it more expandable the Node.JS way? Expandability is more important than performance, but since I'm looking for an idiomatic solution this shouldn't be relevant.

Also note the data written seems redundant, but there are out-of-scope reasons for this.

var m = require('mraa'); // I/O library

Servo = new m.I2c(0)
Servo.address(0x74)

timer();

function timer() {
  firstI2C();
  setTimeout(timer, 5000)
}

function firstI2C() {
  Servo.writeReg(1, 250);
  Servo.writeReg(2, 128+16+0x0);
  Servo.writeReg(37, 0);
  console.log("1");
  setTimeout(secondI2C, 1000)
}

function secondI2C() {
  Servo.writeReg(1, 5);
  Servo.writeReg(2, 128+16+0x0);
  Servo.writeReg(37, 0);
  console.log("2");
  setTimeout(thirdI2C, 1000)
}

function thirdI2C() {
  Servo.writeReg(1, 250);
  Servo.writeReg(2, 128+16+2);
  Servo.writeReg(37, 0);
  console.log("3");
  setTimeout(fourthI2C, 1000)
}

function fourthI2C() {
  Servo.writeReg(1, 5);
  Servo.writeReg(2, 128+16+2);
  Servo.writeReg(37, 0);
  console.log("4");
}

I've been thinking about a function which would accept data in JSON and maps it to such functions and which keeps execution order by the position in the JSON (list). However, such a function is way beyond my current skill level.

A very simple solution is to use a Map that (as its name implies) maps a string to a function so that you can pass an array of sorted strings and ask for the application to get each function and run it in the specified order.

Something like this:

var myFuncs={ 'firstI2C':firstI2C,
              'secondI2C':secondI2C,
              //so on
}

Then you can have a function that tells the program to run next function. For example:

var nextMethodIndex=0;

var methodOrder=['firstI2C','secondI2C','thirdI2C',....] // this array can be fed as JSON.

function runNextMethod()
{
    if(nextMethodIndex<methodOrder.length)
    {
         var func= myFuncs[methodOrder[nextMethodIndex]];
         nextMethodIndex++;
         setTimeout(func, 1000)
    }

}

Finally you should modify your functions to call runNextMethod when they finish their job.

function timer() {
  runNextMethod();
  setTimeout(timer, 5000)
}

function firstI2C() {
  Servo.writeReg(1, 250);
  Servo.writeReg(2, 128+16+0x0);
  Servo.writeReg(37, 0);
  console.log("1");
  runNextMethod();
}


Call multiple api endpoints in NodeJS I am new in NodeJS and I try to write code based on tutorials I find online. I have a NodeJS app (based on Strapi) and an external API from a Cloud Servers provider. The process I follow is as:

Call the cloud servers provider to authorise and get an access token

Call the Strapi to get the list of servers I have added as content type

For each server, call the cloud server provider to get a list of apps and their disk usage in the server

Iterate in the results and call the Strapi again to update the data fields in another content type called subscriptions.
async function asyncForEach(array, callback) {
  for (let index = 0; index < array.length; index++) {
    await callback(array[index], index, array);
  }
}

const waitFor = (ms) => new Promise(r => setTimeout(r, ms));

await axios.post('https://api.cloudways.com/api/v1/oauth/access_token?email=' + process.env.CLOUDWAYS_EMAIL + '&api_key=' + process.env.CLOUDWAYS_API)
.then((res) => {
  //get access token for cloudways authorization
  const accessToken = res.data.access_token;
  //get all servers from strapi
  strapi.query('server').find({})
    .then((res2) => {
      //call cloudways for each server
      asyncForEach(res2, async (sub) => {
          await axios.get('https://api.cloudways.com/api/v1/server/monitor/summary?server_id=' + sub.ServerID + '&type=db', {
            headers: {
              Authorization: 'Bearer ' + accessToken,
            },
          }).then((res3) => {
            //update subscription disk usage in strapi
            asyncForEach(res3.data.content, async (app) => {
              if ( app.datapoint ) {
                strapi.query('subscription').update({ ApplicationSlug: app.name }, { DiskUsage: Math.ceil(app.datapoint[0]) });
              }
            });
          }).catch((res3) => {
            console.log(res3.response.data);
          });
      });

    });
});

The waitFor is a helper function to wait for X seconds, since Cloud Server provider has a limitation in how many calls you can do per minute. But I wasn't able to integrate it well in my code.

Some thoughts:

You are using async but not "using" it.  You can simplify this code for the reader by doing const accessToken = await axios.post(...); and similar to de-nest and make the code more understandable. This also helps your avoid bad variable names like res1, res2, res3, etc.
Promise.all() is your friend when it comes to making batch calls like this.  Combined with a tool like your waitFor function, you can introduce any kind of request pattern you want.  Right now you are making calls in series which is kind of harder to throttle when it comes to meeting rate limits, as now you have to account for response times in determining when to make next call vs. an up-front staggered triggering of the requests that can be possible with Promise.all() (see example below).
If you are going to use a tool like axios or Fetch API that require a res.data or res.json() type of step, you might consider putting a simple wrapper around the response handling to clean up code a bit. This is also a good way to standardize error handling at the client level. In this case, I think such a wrapper could also be used to manage the access token state (though not shown that way in my example below).
Break down your code along logical functional lines (another option available to you after using async more conventionally).  For example, the status request and strapi update are one logical operational unit.  Getting the server list is another logical piece of functionality.
If you control your strapi endpoint/database you should consider adding/leveraging bulk query support for the datapoint updates. Querying in loops is kind of an anti-pattern and pretty much any modern data store supports bulk updates.
I would recommend defining key constants/variables for aiding in code understanding and maintenance. For example, if I wanted to migrate to v2 of this API at some point, I don't want to have to go replace v1 with v2 in a bunch of places.  Centralize how you hold such module "configuration" logic. This makes your code's dependencies much more visible. For example, defining apiKey and email from process.env in my example below makes it really clear to the reader that this dependency exists, without having to find the variable mixed into a string definition deep down in the code somewhere.
Consider implementing more appropriate error handling. My guess is that this server monitoring process shouldn't be designed to just silently swallow errors.

Putting it together might yield something like:
import axios from 'axios';
import strapi from 'your-strapi-module';

const apiBase = `https://api.cloudways.com/api/v1`;
const apiKey = process.env.CLOUDWAYS_API;
const email = process.env.CLOUDWAYS_EMAIL;
const maxRps = 10; // max requests per second for throttling
const throttleInterval = 1000 / maxRps;

// a small wrapper around axios for this particular API
// this might best live in a separate module
const apiRequest = async ({ headers = {}, method = 'get', url }) => {
  const opts = {
    baseUrl: apiBase,
    headers,
    method,
    url
  };
  return axios(opts).then( (res) => res.data );
};

// method to obtain OAuth token, again perhaps best in separate module with API axios wrapper.
// this could be made to lazily provision the token on any API request if not already received
const getAccessToken = async () => {
  const method = 'post';
  const url = `/oauth/access_token?email=${ email }&api_key=${ apiKey }`;

  const data = await apiRequest({ method, url});
  return data.access_token;
};

const getServerList = async () => strapi.query('server').find({});

const wait = (ms) => new Promise( (resolve) => setTimeout(resolve, ms) );

const updateServerStatus = async (serverId, accessToken) => {
  const url = `/server/monitor/summary?server_id=${ serverId }&type=db`;
  const headers = {
    Authorization: 'Bearer ' + accessToken
  };

  return apiRequest({ headers, url })
    .then( ({ content }) => {
      // I am not throttling here as no need to throttle this data store query was noted
      // Ideally, this could be made into single update query though
      const promises = content.map( (app) => {
        return strapi.query('subscription').update(
          { ApplicationSlug: app.name },
          { DiskUsage: Math.ceil(app.datapoint[0]) }
        );
      });

      return Promise.all(promises);
    });
};

const updateAll = async () = {
  // we can parallelize getting access token and getting server list
  const initPromises = [
    getServerList(),
    getAccessToken()
  ];

  const [ serverList, accessToken ] = await Promise.all(initPromises);

  // map your server list to an array of update promises
  const updatePromises = serverList.map((server, i) => {
    // stagger the requests to meet rate limits
    return wait(i * throttleInterval)
      .then( () => updateServerStatus(server.ServerId, accessToken) );
  });

  return Promise.all(updatePromises);
};

// call the main function somehow (could be exported from module for example)
export {
  updateAll as default,
  updateAll,
  // again, following probably make sense in separate module
  apiRequest,
  getAccessToken
}

Note that since we are now "using" async in a more appropriate fashion you now begin to make your logic available in specific functions.  For example, if I just wanted to allow one to update the status of a single server, I could expose the updateServerStatus function in exports. There would have been no way to offer that sort of code flexibility with your current approach of using nested thenables.


Generating row and column of a given nonogram-like table What are nonogram tables?

  Nonograms, also known as Hanjie, Picross or Griddlers, are picture logic puzzles in which cells in a grid must be colored or left blank according to numbers at the side of the grid to reveal a hidden picture.

There are two kind of nonogram tables, ones with only one row and column, and kinds with multiple rows and columns. For simplicity's sake, I have assumed that nonogram table given has only one row and one column, and 5x5 only. That is why my table is only nonogram-like, a true nonogram table gives out two columns if a row is as if colored-blank-blank-blank-colored. But in my code if the nonogram table is like that, the column will be 2 instead of 1 1.

Anyways, the code, in which I have assumed that blank spaces are 0 and painted spaces are asterisk. Keep in mind that blank space can be any character.

def nono_descriptor(given_string):
    number_of_lines = given_string.count('\n') #number of line breaks
    length_of_string = len(given_string) #length of the given string

    if number_of_lines != 4 and length_of_string != 29:
        raise Exception("You need exactly four line breaks and 25 characters!") #throw exception if conditions don't match

    rows_split = [] #list that contains each line split
    columns_list = [] #list that contains columns
    rows_list = [] #list that contains rows

    list_of_lines = given_string.split('\n') #split the string by line break

    for each_line in list_of_lines:
        rows_split.append(list(each_line)) #populate rows list with split up characters
        columns_list.append(each_line.count('*')) #populate columns list with the number of asterisks in each column

    for i in range(5):
        temp_list = [row[i] for row in rows_split]
        rows_list.append(temp_list.count('*'))

    print (columns_list, rows_list)

nono_descriptor('0*0*0\n***00\n****0\n*0*0*\n****0')

Would love to hear your opinions.

Again, don't mistake this code as a nonogram table generator. It only generates a table similar to that of a nonogram.

Size of the grid

    raise Exception("You need exactly four line breaks and 25 characters!") #throw exception if conditions don't match

Using a generic Exception is bad practice. What should the calling code look like if it want to handle such failure? Something like

with open('da_file.txt') as picture:
    pic = picture.read()
try:
    descr = nono_descriptor(pic)
except Exception:
    print('Not the right size')
else:
    print(descr)

probably. Meaning that if anything goes wrong, it would be treated as if the input was not as expected.

You should at least use a more specific kind of exception (such as ValueError) or roll your own:

class BadGridSizeError(Exception):
    pass

def nono_descriptor(...):
    ...
    raise BadGridSizeException("...")

But why limit at 5×5 grids in the first place, why not let the user provide a grid with the size they like? It is easy to check that the grid is square and keep the size for further operations instead of hardcoding the value 5:

def nono_descriptor(given_string):
    list_of_lines = given_string.splitlines()
    grid_size = len(list_of_lines)

    assert all(len(row) == grid_size for row in list_of_lines), "Square grid expected"

    rows_split = [] #list that contains each line split
    columns_list = [] #list that contains columns
    rows_list = [] #list that contains rows

    for each_line in list_of_lines:
        rows_split.append(list(each_line)) #populate rows list with split up characters
        columns_list.append(each_line.count('*')) #populate columns list with the number of asterisks in each column

    for i in range(grid_size):
        temp_list = [row[i] for row in rows_split]
        rows_list.append(temp_list.count('*'))

    print (columns_list, rows_list)

Using an assert here is also more friendly to the user that can deactivate such checks (using the -O or -OO switch on the command line) when they know that their input is good.

You don't even need to restrict the grid to being square, you can have m×n grids if you check that each row is the length of the first one:

def nono_descriptor(given_string):
    list_of_lines = given_string.splitlines()

    assert list_of_lines, "At least one row expected"

    grid_size = len(list_of_lines[0])

    assert all(len(row) == grid_size for row in list_of_lines), "Rectangular grid expected"

    rows_split = [] #list that contains each line split
    columns_list = [] #list that contains columns
    rows_list = [] #list that contains rows

    for each_line in list_of_lines:
        rows_split.append(list(each_line)) #populate rows list with split up characters
        columns_list.append(each_line.count('*')) #populate columns list with the number of asterisks in each column

    for i in range(grid_size):
        temp_list = [row[i] for row in rows_split]
        rows_list.append(temp_list.count('*'))

    print (columns_list, rows_list)

Counting by rows and columns

You can easily get the count of '*' using a list-comprehension rather than using append():

columns = [row.count('*') for row in list_of_lines]

You can argue that it's better to build row_split in the meantime while iterating over list_of_lines… But you don't need that list. The only thing you use it for is to be able to index the ith column in each row. But you can do that exactly the same way using plain strings because list(my_str)[i] is exactly the same as my_str[i]. Meaning you can write:

columns = [row.count('*') for row in list_of_lines]

rows = []
for i in range(grid_size):
    tmp = [row[i] for row in list_of_lines]
    rows.append(tmp.count('*'))

However there are two improvements possible. The first one being using a list-comprehension:

rows = [[row[i] for row in list_for_lines].count('*') for i in range(grid_size)]

The second one being to use zip instead of trying to manually do its exact job:

rows = [colum.count('*') for column in zip(*list_of_lines)]

One more advantage using zip is that it will stop producing columns when the shortest row has been exhausted. Meaning you don't even need to check that the grid is rectangular anymore:

def nono_descriptor(given_string):
    lines = given_string.splitlines()

    columns = [row.count('*') for row in lines]
    rows = [column.count('*') for column in zip(*lines)]

    print(columns, rows)

print vs return

Printing a result within a function that perform some computations is bad practice. Your function can't be reused in a different context. What if I wanted to iterate over some files and use your function to append the various counts at the end of said file. Well I can't, because you print the result, so I can't do anything with it:

def nono_descriptor(given_string):
    lines = given_string.splitlines()

    columns = [row.count('*') for row in lines]
    rows = [column.count('*') for column in zip(*lines)]

    return columns, rows

Your use case should now be

print(nono_descriptor('0*0*0\n***00\n****0\n*0*0*\n****0'))

which might seem more work here, but it lets you reuse you function way more easily.


Project Euler 10 - Summation of primes I'm currently attempting to learn OCaml, and I'm working thought the Project Euler problems to do so.  Here's some code I knocked together for problem 10.

I am looking for idiomatic feedback rather than algorithmic

(*
The sum of the primes below 10 is 2 + 3 + 5 + 7 = 17.

Find the sum of all the primes below two million.

MODIFYING CODE FROM PROBLEM 7

*)

(* first thing we are going to do is write a bit of code that checks to see if a number is prime   *)

let rec isPrimeRec number start = if (start*start)>number then 1 else  if number mod start = 0 then -1 else isPrimeRec number (start+1);;

let i = ref 2;;
let sum = ref 0 in
while !i <2000000 do
if (isPrimeRec !i 2) = 1 then
begin
sum:= !sum + !i; i:= !i + 1;
Printf.printf "%d is prime, it is prime number %d\n" !i !sum;
end
else
i:= !i + 1
done;;

let temp = ref 0;;
temp:= isPrimeRec 19 3;
Printf.printf "The value is %d\n" !temp

Now - I almost purposely didn't write this to be efficient algorithmically (for example I am aware that prime numbers can't be even)  and there are a few other things that I would change for efficiency - but I'm interested in style feedback - so I'd like the code critiqued much more on the level of "In OCaml, one would normally bracket expression X for readability" or "OCaml let's you use this, clearly syntax instead" - rather than "it's a property of prime numbers that"

Here are some comments and an example rewrite of your code.

Indentation and line breaks are not idiomatic to OCaml, it's good practice in any language.

A more intuitive type for is_prime would be to have only one argument, so let's encapsulate is_prime_rec:

let is_prime =
  let rec is_prime_rec number start =
    if start * start > number then true (* OCaml provides a type `bool`, distinct from `int`, so it's better to return `true` instead of `1`. *)
    else if number mod start = 0 then false
    else is_prime_rec number (start+1) in
  fun n -> is_prime_rec n 2;;

which yields val is_prime : int -> bool = <fun>.

You know the number of iterations in the loop, so better use for rather than while.
This also avoid manually incrementing i.

let sum = ref 0 in
for i = 2 to 2_000_000 do (* detail: OCaml parses `2_000_000` as `2000000`, which is more readable. *)
  if is_prime i then (* no need for parentheses around the if clause *)
    begin
      sum:= !sum + i;
      Printf.printf "%d is prime, it is prime number %d\n" i !sum;
    end
done;;

I can't help but add an algorithmic remark: consider using a sieve.


Use of containers for storing Students I am currently working in a project (I have simplified it for the sake of question) that requires a container to store students and retrieve them as fast as possible or access them with \$O(1)\$ if possible. I will also be deleting students from any place but always adding them at the end of the queue.

I have three structs: Student, Class and University.

Student: has name and age
Class: has name of course and array of shared pointers of size 2
University: has deque of Students and deque of class

#include <memory>
#include <deque>
#include <string>
#include <iostream>

constexpr size_t SIZE = 150;

struct Student{
    std::string name;
    size_t age;

    Student(std::string n, size_t a){
        name = n;
        age = a;
    }
    ~Student(){
        std::cout << __func__ << std::endl;
    }
};

struct Class{
    // Lets assume that each class has 2 student for the sake of argument
    // and exactly 2.
    std::shared_ptr<Student> my_class[2];
    std::string course;

    Class(std::string c, std::shared_ptr<Student> n0, std::shared_ptr<Student> n1){

        course = c;
        my_class[0] = n0;
        my_class[1] = n1;
    }
};

struct University{

    std::deque<std::shared_ptr<Student>> student_deq;
    std::deque<std::shared_ptr<Class>> class_deq;

    void add_student(std::string n, size_t a){
        student_deq.push_back(std::make_shared<Student>(n, a));
    }

    void add_class(std::string c, size_t i, size_t j){
        class_deq.push_back(std::make_shared<Class>(c, student_deq[i], student_deq[j]));
    }
};

int main(){

    University u;

    for(size_t i = 0; i < SIZE; i++){
        u.add_student("Xyz", i);
    }

    for(size_t j = 0; j < SIZE; j++){
        if(j+1 == SIZE)
            u.add_class("abc", j, 0);
        else u.add_class("abc", j, j+1);
    }
    std::cout << u.student_deq[9]->name << std::endl;
return 0;
}

I have some questions:

Do you think my choice of deque as a data structure (container) is appropriate?

I want the retrieve (from anywhere) / add (to end)/ remove (from anywhere) to be \$O(1)\$. I use deque because unlike vector it does not have to copy all the data from beginning of the array in case the memory grows and have to allocate new slab of contiguous memory. Since deque is made of small chunks of contiguous memory not all chunks need to be copied (at least that is what I think of). Also the retrieve and add to end is \$O(1)\$ in case of deque. I know remove is \$O(n)\$: \$n\$ being size of one of the chunk of contiguous memory the item reside in.

I think I could also use HashMap (unordere_map) but I read somewhere that hash_map does not necessarily give \$O(1)\$ because of collision when data gets huge and also my student number will definitely grow hugely (e.g. 100,000 and up also for sake of argument). I also heard that implementing hash function to avoid collision is complex.

I also think I can't use std::map since retrieval is \$O(ln)\$.
Do you think the use of shread_ptr inside the deque for memory management is a good design choice?

I used sharad_ptr so that I don't have to do the manual delete of the allocated memory. I also make the class store the shared_ptr rather than a copy of the student because the Student class may contain other information that might be costly to copy. Also a student can be in more then one class ( This way instead of creating multiple copy shared_ptr will only increase the count - Here also I am not sure is it really necessary to increase count because since there will only be one copy I was also thinking about passing by const ref so that we dont have the overhead of increasing the count itself)
Is there a container which gives \$O(1)\$ for retrieval, adding to the end and removal from anywhere in the container (of-course except hash map as I discussed about it above) or anything that is close enough that provide these features?

Do you think my choice of deque as a data structure (container) is appropriate?

Seems good.

  I want the retrieve (from anywhere) / add (to end)/ remove (from anywhere) to be O(1)O(1).

See: What are the complexity guarantees of the standard containers?

So std:: deque does not provide a O(1) erase (remove).

  I use deque because unlike vector it does not have to copy all the data from beginning of the array in case the memory grows and have to allocate new slab of contiguous memory. Since deque is made of small chunks of contiguous memory not all chunks need to be copied (at least that is what I think of). Also the retrieve and add to end is O(1)O(1) in case of deque. I know remove is O(n)O(n): nn being size of one of the chunk of contiguous memory the item reside in.

All that is correct. Just does not make removal O(1). Because the chunks can be large m. The removal of an item is O(m) where m is the size of the chunk.

  I think I could also use HashMap (unordere_map)

That does support all the requirements you need.

  but I read somewhere that hash_map does not necessarily give O(1)O(1) because of collision when data gets huge and also my student number will definitely grow hugely (e.g. 100,000 and up also for sake of argument).

Yes collisions will add time. But they do not change the complexity. It still remains O(1).

  I also heard that implementing hash function to avoid collision is complex.

It is exceedingly complicated to do correctly and you should never do this. Use the standard one. Only implement a hash function if that is your actual job (or you are some kind of maths genius). This is too easy to get wrong so use a good well known hash (the one provided by the standard is fine).

  I also think I can't use std::map since retrieval is O(ln)O(ln).

Correct.

  Do you think the use of shread_ptr inside the deque for memory management is a good design choice?

No. Given the choice between dynamic/automatic variables you should always prefer automatic variables.

But you don't provide enough details to provide a good answer either. Personally I would have one entity that owns all the students (say the University). Then you classes can contain references to the object owned by the university (ie they could iterators as a reference).

  I used sharad_ptr so that I don't have to do the manual delete of the allocated memory.

That's a good thing and correct usage if you are using dynamic memory. But as stated above I don't think dynamic memory is the correct solution.

  I also make the class store the shared_ptr rather than a copy of the student because the Student class may contain other information that might be costly to copy.

Sure. You should not have two copies of the same data. That will just introduce consistency issues. But that still does not require dynamic memory allocation. The class store can store so other type of reference to the object (Note: I use the term reference at the conceptual level not the C++ language level).

  Also a student can be in more then one class ( This way instead of creating multiple copy shared_ptr will only increase the count - Here also I am not sure is it really necessary to increase count because since there will only be one copy I was also thinking about passing by const ref so that we dont have the overhead of increasing the count itself).

Don't do that. The whole point of using the shared pointer is to make sure it correctly exists when you access it. If you do this it could be destroyed and then you try and access it via a class and get undefined behavior. A better solution here would be to use a std::weak_ptr. But I still prefer my technique of using a reference (even if that means more work cleaning up).

  Is there a container which gives O(1)O(1) for retrieval, adding to the end and removal from anywhere in the container (of-course except hash map as I discussed about it above) or anything that is close enough that provide these features?

See the provided link above.


Optimising single-delimiter string tokenisation I am trying to optimise my tokenizing of tab delimited strings:

static void split_line(string &line, input_record &rec)
{
    int col = 0;
    char *row = &line[0];
    char *token = strtok(row, "\t");
    while (token)
    {
        switch(++col)
        {
            case 2 : rec.sequence = token; break;
            case 4 : rec.content = token; break;
            case 9 : rec.position = atoi(token); return;
        }
        token = strtok(NULL, "\t");
    }
}

where

struct typedef
{
    string sequence;
    string content;
    unsigned int position;
} input_record

This function is called on the result from each getline from a parent, causing split_line to be called over 100 million times.

Each line has at least 15 columns (of which 2, 4 and 9 are useful), and I assign the relevant tokens to variables in a struct. The first 10 columns of a string contain roughly 150 characters (variable length) and I am handling millions of records. Currently, it takes ~0.8µs to process a string, and is a bottleneck in my code.

Does anyone have advice for squeezing more speed out of this?

I tried the following change to your function. It does not use strtok. It does the tokenizing in the function itself. I got a little bit of speed up.

void split_line2(string &line, input_record &rec)
{
    int col = 0;
    char *start = &line[0];
    char sep = ','; // Using ',' instead of '\t' for my testing.
    for ( char* iter = start; *iter != '\0'; ++iter )
    {
       if ( *iter == sep )
       {
          *iter = '\0';
          switch(++col)
          {
             case 2 : rec.sequence = start; break;
             case 4 : rec.content = start; break;
             case 9 : rec.position = atoi(start); return;
          }
          start = iter+1;
       }
    }
}

Here's the test program and results from my testing:

#include <iostream>
#include <string>
#include <ctime>
#include <cstring>

using std::string;

struct input_record
{
    string sequence;
    string content;
    unsigned int position;
};

void split_line1(string &line, input_record &rec)
{
    int col = 0;
    char *row = &line[0];
    char *token = strtok(row, ",");
    while (token)
    {
        switch(++col)
        {
            case 2 : rec.sequence = token; break;
            case 4 : rec.content = token; break;
            case 9 : rec.position = atoi(token); return;
        }
        token = strtok(NULL, ",");
    }
}

void split_line2(string &line, input_record &rec)
{
    int col = 0;
    char *start = &line[0];
    char sep = ',';
    for ( char* iter = start; *iter != '\0'; ++iter )
    {
       if ( *iter == sep )
       {
          *iter = '\0';
          switch(++col)
          {
             case 2 : rec.sequence = start; break;
             case 4 : rec.content = start; break;
             case 9 : rec.position = atoi(start); return;
          }
          start = iter+1;
       }
    }
}

void test1(std::string &line, int n)
{
   clock_t start = clock();

   for ( int i = 0; i < n; ++i )
   {
      input_record rec;
      split_line1(line, rec);
   }

   clock_t end = clock();
   std::cout << "Time: " << (end-start)/CLOCKS_PER_SEC << std::endl;
}

void test2(std::string &line, int n)
{
   clock_t start = clock();

   for ( int i = 0; i < n; ++i )
   {
      input_record rec;
      split_line2(line, rec);
   }

   clock_t end = clock();
   std::cout << "Time: " << (end-start)/CLOCKS_PER_SEC << std::endl;
}

int main(int argc, char** argv)
{
   std::string line(argv[1]);
   int n = atoi(argv[2]);
   test1(line, n);
   test2(line, n);
}

Results:

~/Stack-Overflow/cpp>>./test-507 "1,abcd,3,xyz,4,5,6,7,8,9,10,11" 50000000
Time: 3
Time: 2

~/Stack-Overflow/cpp>>./test-507 "1,abcd,3,xyz,4,5,6,7,8,9,10,11" 100000000
Time: 6
Time: 4

~/Stack-Overflow/cpp>>./test-507 "1,abcd,3,xyz,4,5,6,7,8,9,10,11" 200000000
Time: 13
Time: 9

~/Stack-Overflow/cpp>>./test-507 "1,abcd,3,xyz,4,5,6,7,8,9,10,11" 400000000
Time: 26
Time: 18

Update

If I change the struct from containing std::string to char*, there is substantial savings.

struct input_record
{
    char* sequence;
    char* content;
    unsigned int position;
};

~/Stack-Overflow/cpp>>./test-507 "1,abcd,3,xyz,4,5,6,7,8,9,10,11" 400000000
Time: 16
Time: 8

If that is an option, that will be save you a bunch of time.