Shuffling many collections by folding over a numeric range Often, I find myself folding over a range object if I need to repeatedly transform something. This, to me though, feels like an abuse of fold since I never even use the numbers that I'm folding with.

Almost perfect example:

(ns shuffle-example
    (:import [java.util Collection Collections]))

(defn shuffle [^Collection coll rand-gen]
  (let [j-coll (java.util.ArrayList. coll)]
    (Collections/shuffle j-coll rand-gen)
    (into (empty coll) j-coll)))

(defn shuffle-many [^Collection coll n-shuffles rand-gen]
  (reduce (fn [c _] (shuffle c rand-gen))
          coll
          (range n-shuffles)))

Is this considered bad form? Is there a more idiomatic way of achieving this? I know in this case it's inefficient since it requires constant conversions between list types, but it shows my question well.
Is there a better way to deterministically shuffle a list in Clojure? Clojure comes with a shuffle function, but it doesn't accept a Random, so it's not very useful. Short of writing my own shuffle from scratch, this is the best I've been able to come up with.

If you want to transform a value v by applying a function f n times, it may be clearer to use a combination of iterate and nth:

(nth (iterate f v) n)

If you want to write a 'good' general-purpose implementation of shuffle that accepts a random generator, I suggest you simply adapt the implementation in clojure.core. You'll see the implementation you proposed is actually pretty close!

Regarding performance, keep in mind that if you want to shuffle a coll n times, nothing will beat the performance of repeatedly shuffling a mutable collection in place.


Insert 271 million records from text file to mongodb I have 271 million records, line by line in a text file that I need to add to MongoDB, and I'm using Python and Pymongo in order to do this.

I first split the single file containing the 271 million records into multiple files containing each 1 million lines, and have written the current code to add it to the database:

import os
import threading
from pymongo import MongoClient

class UserDb:
    def __init__(self):
        self.client = MongoClient('localhost', 27017)
        self.data = self.client.large_data.data

threadlist = []

def start_add(listname):
    db = UserDb()
    with open(listname, "r") as r:
        for line in r:
            if line is None:
                return
            a = dict()
            a['no'] = line.strip()
            db.data.insert_one(a)
    print(listname, "Done!")

for x in os.listdir(os.getcwd()):
    if x.startswith('li'):
        t = threading.Thread(target=start_add, args=(x,))
        t.start()
        threadlist.append(t)

print("All threads started!")

for thread in threadlist:
    thread.join()

This starts as many threads as there are files, and adds every line to the db as it goes through it. The bad thing is that after 3 hours it had only added 8.630.623.

What can I do to add the records faster?

There are 261 threads currently running.

Example of one row of data: 12345678 (8 digits)

First, you need to consider that adding 271 million records is going to be long, there's no changing that. You should also consider your processing power. You could have the best code in the world, if it runs on an average computer it's going to be average.

Now, what can be done better?

What you're doing is called bulk insertion. There are mechanisms with every database engine that enable doing this faster than one insert at a time. For example, you should consider using the Bulk object to insert many lines at the same time. Basically, you want most of your processing to be done by the database engine, simply because it was written by experts on how to do things fast and without problems. Looking at the Bulk object maximum size, your bulk insert cannot be bigger than 16 megabytes. Considering you have 8 digits number, you can figure out what's the biggest bulk insert you can do!

Running 261 threads probably hurts your performance. After all, the 261 threads probably don't run in parallel (because.. I doubt you have 261 threads available, but who knows). Do more research of multi-threading, maybe using a simple parallel library with way less threads would even be faster. I've got to admit I'm not an expert in threading myself, but I'm pretty confident running 261 threads will simply hurt your performance, these threads need to be managed!

Now, there are a couple things that bother me with your situation :

How many files do you have? If you have like... a million files with 271 lines each, you should consider pre-processing the whole thing to reduce the number of file access necessary. I'd even consider keeping only 1 file.
If you have \$2.71*10^8\$ 8 digit numbers and, I didn't have my morning coffee yet, there are \$9*10^7\$ possible combinations of 8 digits, you would have a lot of duplicates. Are you sure there isn't a simpler way to insert your data than reading 271000000 8 digits numbers from files?

Anyways, I think the main thing you need to consider is using bulk insertion, which brings me to my final tip : Read the documentation of whatever database engine you're using. They are big pieces of code and it's pretty sure you will find something interesting to help you.


when we need to add some javascript code to html, where we have to put it? when we need to add some javascript code to html, where we have to put it?

What is the best practice:

in line:

<script type="text/javascript">document.write(new Date().getFullYear())</script>

or centralized:

<header><script type=text/javascript> document.getelementbyid('someid').innerhtml='date'.....</script></header>

Optimally, it's loaded just before the body tag closes

    <!-- all html above here -->
    <script src="..."></script>
</body>

for the following common reasons:

The DOM elements that you are operating are safely parsed by then.
Script tags block execution. If they delay in loading and/or parsing, the things that follow it will have to wait. Thus, to prevent delay and from seeing a blank screen while scripts load, we load the page first, scripts last.

I have a similar answer, along with other answers, at StackOverflow which should explain more.


Rotate a linked list to the right by k places The task

  Given a linked list and a positive integer k, rotate the list to the
  right by k places.

  For example, given the linked list 7 -> 7 -> 3 -> 5 and k = 2, it
  should become 3 -> 5 -> 7 -> 7.

  Given the linked list 1 -> 2 -> 3 -> 4 -> 5 and k = 3, it should
  become 3 -> 4 -> 5 -> 1 -> 2.

My solution

class LinkedNode {
  constructor(value, next = null) {
    this.value = value;
    this.next = next;
  }
}

const n4 = new LinkedNode(5);
const n3 = new LinkedNode(3, n4);
const n2 = new LinkedNode(7, n3);
const n1 = new LinkedNode(7, n2);

let n = n1;

const rotate = (l, k) => {
  let p = l;
  const first = l;
  let last;
  let res;
  let i = 0;
  while(p) {
    i++;
    if (!p.next) { last = p; }
    p = p.next;
  }
  const size = i;
  if (!(k % size)) { return l; }
  const aim = size - (k % size) - 1;
  p = l;
  i = 0;
  while (p) {
    if (i++ === aim) {
      res = p.next;
      p.next = null;
      last.next = first;
      return res;
    }
    p = p.next;
  }
};

n = rotate(n, 0);
while (n) {
  console.log(n.value);
  n = n.next;
}

You started to write a class, but then stopped. You should continue. There are three things that are potentially \$O(n)\$ in this problem:

You need to know the length of the list.
You need to know the last node of the list.
You need to know the k-1th node of the list.

Two of those obviously go together, so you could justify writing a helper method that returns them both. The third one is arbitrary enough that there's not really any way to speed it up.

If you are in some kind of timed environment, you'll definitely want to wrap the list in a class and cache the last pointer and the size of the list.
If you do that, you can quickly compute k % size and then find #3, which will be \$O(n)\$.

If I were trying to write challenges for a timed competition, I would include a large list and k = 2 * n - 1.

I see several obvious methods in your code, including a more sophisticated constructor that could take an array (perhaps an outer LinkedList class), the length operation, the get_last operation, the get_item operation, the print or perhaps map operation, and obviously missing is the compare operation.

You should probably check for k == 0 before finding the last item. I can easily see a time-limit-exceeded trap being to construct a huge list, then do a rotate with k=0.


jQuery solution for a client's site I'm in the process of working on a client's site. I have a section containing a ul with list items and a div containing the slides that are triggered when clicking on an li item.

For instance,
Item One
 triggers
...
.

Here is the jQuery portion:

var slidesNav = $('#slides-nav > li'),
    slidesContent = $('#slides-content').find('div'),
    id = '',
    slideId = '';

// Set initial state of slides to hidden, except the first one.
slidesContent.hide().first().slideDown();

var setClickEvents = function (id) {

    $('#nav-' + id).on('click', function () {
        slideId = $('#slide-' + id);

        if (slideId.css('display') !== 'block') {
            slidesContent.slideUp();
            slideId.slideDown();
        }
    });
};

for (var i = 0; i < slidesNav.length; i++) {
    id = $(slidesNav[i]).attr('id').substr('4');

    // assign click events
    setClickEvents(id);
}

If my solution can be improved, please explain as my ultimate goal is to become a better programmer.

Here is a jsbin with the full code. It's a variation of the original for the client.

You could turn this into a general purpose plugin, but I'm not gonna go into that. There are also many good plugins that provide this functionality.

I'd ditch the IDs and just use the index of the current element as the reference.

You'll obviously still need a parent element with an ID, for nav and slides, so your code knows where they are.

Create class for active slide/nav, when a nav item is clicked, you get the index, multiply it by slide width/height, and animate. You get the corresponding slide with the index, so no need for IDs. Give the clicked element an active class. First step as soon as click happens would be to remove the active class so you can reassign it.

Then just put together some css using your parent elements class/ID.

Your code will be more generic, and your html will be cleaner.


Unit Test to verify number of times lazy initialization logic is executed for a data member I'll write a small simulation of my actual class first to request insights on unit testing one of the behaviors of this class. Here is my class A:

public class A {
    private final String a;
    private final String b;
    private String c;

    public A(final String a, final String b) {
       this.a = a;
       this.b = b;
       this.c = null;
    }

    public getC() {
       if (this.c==null) {
          // Logic to compute value of this.c using this.a and this.b
          // set this.c
       }

       return this.c;
    }
}

In the above class I'm doing lazy initialization of a data member c. Responsibility of class A is to  compute this.c from this.a and this.b and cache it so that there is no need to compute it again on future invocation of getC. Class A has only this responsibility.

I want to write a unit test to verify that on Multiple invocations of getC() method on the same instance of class A, logic for computing this.c should be executed only once. Could anyone please help in figuring out, how can I write this unit test ? One way I can see is delegating the logic of computing this.c to another class B and then making class A dependent on class B so that a mock of class B can be injected during testing. After injection of mock of B, Mockito's feature to verify number of times method invocation happens on a mock can be used. But Class A has only one responsibility of computing this.c and delegating that responsibility to another object is looking like an overkill to me.

Delegate the computation of a and b to a method, say, .compute(). Since we want to spy this method's execution, we will suppose that we have Guava and its useful @VisibleForTesting annotation, and make the method protected:

@VisibleForTesting
protected String compute(String a, String b) { // code here }

Now, code written using Mockito and TestNG (with test file in same package than class A, so that the compute() method is visible):

@Test
public void computationOfCHappensOnlyOnce()
{
    final String a = "whatever";
    final String b = "you want";

    final A spy = spy(new A(a, b));

    for (int i = 0; i < 10; i++)
        spy.getC();

    verify(spy, times(1)).compute(eq(a), eq(b));
}

Note: the way your class is currently written, it is NOT thread safe. The quick, coarse way to fix it is to make .getC() synchronized.

EDIT Discussion on a different design to get the value of c

This requires a creation of a new factory class for instances of class A; it would have the responsibility to create new instances and compute c:

// if the class is a singleton...
final FactoryForA factory = FactoryForA.getInstance();

final A a = factory.newA(a, b);

// This calls back to the factory to compute c:
a.getC();

The A class needs to implement .equals() and .hashCode() (on fields a and b but obviously not c!). Also, it has a constructure which is now package private (so that the factory can access it) and whose prototype is:

final String a, b;
final FactoryForA factory;
// No `c` member, see below

A(String a, String b, FactoryForA factory)
{
    this.a = a; this.b = b; this.factory = factory;
}

public getC()
{
    return factory.computeC(this);
}

Now for the factory itself: it has an ExecutorService and a Map<A, FutureTask<String>>. Here is the code of computeC():

// instance variables of the factory
private final ExecutorService service
    = Executors.newFixedThreadPool(5); // for example
private final Map<A, FutureTask<String>> map = new HashMap<>();

// package private
String computeC(final A a)
{
    FutureTask<String> task;
    // Synchronized map access
    synchronized(map) {
        // If there is no task created for that instance of A, create it
        // and submit it for execution
        task = map.get(a);
        if (task == null) {
            task = createFutureTask(a); // TODO, but nothing too complicated
            // IMPORTANT! If you don't do this, the task is not executed
            // and .get() below will block indefinitely
            service.execute(task);
            map.put(a, task);
        }
    }
    // A FutureTask will keep its status! We can therefore call .get()
    // as much as we like; the computation will only have happened once
    return task.get();
}

One advantage is that if two instances created for A are equal (ie, the same a and b parameters), computation for c happens only once!


Dijktra's Algorithm Visualizer A couple months ago I made an interactive dijktra visualizer that allowed the user to setup the map nodes and run the algorithm. I thought it was a good way to solidify my algorithm and html/css knowledge. I'd like for you guys to test my application out and give any advice in any way I could make it better.
One particular element that i'd like to improve is the map rendering, which I did by making a table and updating it's rows and columns. This solution worked out good but I found it to be slow with maps with larger dimensions.
function render () {
    let html = ''

    for (let row = 0; row < mapHeight; row++) {
        html += '<tr>'

        for (let column = 0; column < mapWidth; column++) {
            // Ignore "nodeIndex", "colorIndex" and such
            nodeIndex = column + ( row * mapWidth )
            let colorIndex = myNodes[nodeIndex].colorIndex

            html += `<td style="background-color:${nodeColors[colorIndex]};"></td>`
        }

        html += '</tr>'
    }

    //"mapTable" being the element in which this table is rendered
    mapTable.innerHTML = html
}

Hope you guys like it and please feel welcome to give any criticism, have fun!
My page: https://github.com/Tlomoloko/Dijkstra_Visualizer

I don't know if there is a way to avoid the nested loop in this case because you need to do a calc that involves every inner loop index with every outer loop index.
But I think that is not necessary to calculate row * mapWidth in every iteration in the inner loop, once that both variables are available outside it
for (let row = 0; row < mapHeight; row++) {
    //...
    let cached = row * mapWidth;

    for (let column = 0; column < mapWidth; column++) {
        nodeIndex = column + cached;
        let colorIndex = myNodes[nodeIndex].colorIndex;

        //...
    }
//...
}


Creating a questionnaire Im making a Python program. It is basically a questionnaire and I was just wondering if there is a shorter way of doing this as at the moment I'm writing out all of the questions in separate functions. I was wondering if there is an alternative way of doing this in almost a single function where the function just repeats but the question and right answers change every time. If I cannot do this is there an alternate way to do this without having to do separate functions for each question? is there a faster/more efficient way of completing this?

All advice would be much appreciated and I have pasted the first 4 questions and the rest of the code below. I will be having many more questions but i just did 4 so that it wouldn't take up too much space :)

import time
questionNumber = 0
right = 0
wrong = 0
name = str(input("Please Enter your name"))
print()
def questions():
    def questionOne():
        global right, wrong, questionNumber
        print("What is the population of New Zealand?")
        print("Is it A:6.7m B:3.2m C:5.1m or D:4.5m")
        ans = str(input())
        if ans == "D" or ans == "d" or ans == '4.5' or ans == '4':
            print("You got it right!")
            right = right + 1
        else:
            print("You got it wrong!")
            wrong = wrong+1
        questionNumber = questionNumber + 1
    questionOne()
    time.sleep(2)
    print()
    print("So far",name,"You have got",right,"Answers right,",wrong,"Answers wrong and you have completed",questionNumber,"Questions")
    print()
    time.sleep(4)
    def questionTwo():
        global right, wrong, questionNumber
        print("What year did the first european set foot on New Zealand (Abel Tasman)")
        print("Is it A:1830 B:1543 C:1642 or D:1765")
        ans = str(input())
        if ans == "C" or ans == "c" or ans == '1642'or ans == '3':
            print("You got it right!")
            right = right + 1
        else:
            print("You got it wrong!")
            wrong = wrong+1
        questionNumber = questionNumber + 1
    questionTwo()
    time.sleep(2)
    print()
    print("So far ",name,"You have got",right,"Answers right,",wrong,"Answers wrong and you have completed",questionNumber,"Questions")
    print()
    time.sleep(4)
    def questionThree():
        global right, wrong, questionNumber
        print("How many Kiwi are there left in New Zealand (Approx)")
        print("Is it A:2000 B:600 C:70,000 or D:100000")
        ans = str(input())
        if ans == "D" or ans == "d" or ans == '100000'or ans == '4':
            print("You got it right!")
            right = right + 1
        else:
            print("You got it wrong!")
            wrong = wrong+1
        questionNumber = questionNumber + 1
    questionThree()
    time.sleep(2)
    print()
    print("So far ",name,"You have got",right,"Answers right,",wrong,"Answers wrong and you have completed",questionNumber,"Questions")
    print()
    time.sleep(4)
    def questionFour():
        global right, wrong, questionNumber
        print("How many new babys where born in New Zealand in 2015")
        print("Is it A:61,000 B:208,000 C:98,000 or D:18,000")
        ans = str(input())
        if ans == "D" or ans == "d" or ans == '100000'or ans == '4':S
            print("You got it right!")
            right = right + 1
        else:
            print("You got it wrong!")
            wrong = wrong+1
        questionNumber = questionNumber + 1
    questionFour()
questions()

Some PEP8(which is a Style Guide for Python Code) comments regarding your coding style and formatting:

1. Spacing

Between methods, you should have two blank lines.

def questionOne():
    ....

def questionTwo():
    ....

2. Naming conventions

In PEP8 it's recommended to:

  Use the function naming rules: lowercase with words separated by
  underscores as necessary to improve readability.

That said, questionNumber would become question_number

I've seen many people using also camelCase for variables, so that's rather a matter of preference.

The same rule applies for your methods:

questionOne() -> question_one()

Put a space after each comma (,). It really helps when going through your code.

So this:

print("\nSo far ",name,"You have got",right,"Answers right,",wrong,"Answers wrong and you have completed",questionNumber,"Questions\n")

Might be rewritten as:

print("\nSo far ", name, "You have got", right, "Answers right,", wrong, "Answers wrong and you have completed",
      questionNumber, "Questions\n")

3. Other comments

Instead of right = right + 1 you can use an augmented assignment: right += 1.

More, instead of doing this:

print()
print("So far",name,"You have got",right,"Answers right,",wrong,"Answers wrong and you have completed",questionNumber,"Questions")
print()

You can do:

print("\nSo far",name,"You have got",right,"Answers right,",wrong,"Answers wrong and you have completed",questionNumber,"Questions\n")

I don't like how you formatted the above print() but I'll come back to it later on.

Let's see what we've got so far using the modifications I mentioned so far:

import time

wrong = 0
right = 0
question_number = 0
name = str(input("Please Enter your name: "))

def questions():
    def question_one():
        global right, wrong, question_number
        print("What is the population of New Zealand?")
        print("Is it A:6.7m B:3.2m C:5.1m or D:4.5m")
        ans = str(input())
        if ans == "D" or ans == "d" or ans == '4.5' or ans == '4':
            print("You got it right!")
            right += 1
        else:
            print("You got it wrong!")
            wrong += 1
        question_number += 1

    question_one()
    time.sleep(2)
    print("\nSo far", name, "You have got", right, "Answers right,", wrong, "Answers wrong and you have completed",
          question_number, "Questions\n")
    time.sleep(4)

    def question_two():
        global right, wrong, question_number
        print("What year did the first european set foot on New Zealand (Abel Tasman)")
        print("Is it A:1830 B:1543 C:1642 or D:1765")
        ans = str(input())
        if ans == "C" or ans == "c" or ans == '1642' or ans == '3':
            print("You got it right!")
            right += 1
        else:
            print("You got it wrong!")
            wrong += 1
        question_number += 1

    question_two()
    time.sleep(2)
    print("\nSo far ", name, "You have got", right, "Answers right,", wrong, "Answers wrong and you have completed",
          question_number, "Questions\n")
    time.sleep(4)

    def question_three():
        global right, wrong, question_number
        print("How many Kiwi are there left in New Zealand (Approx)")
        print("Is it A:2000 B:600 C:70,000 or D:100000")
        ans = str(input())
        if ans == "D" or ans == "d" or ans == '100000' or ans == '4':
            print("You got it right!")
            right += 1
        else:
            print("You got it wrong!")
            wrong += 1
        question_number += 1

    question_three()
    time.sleep(2)
    print("\nSo far ", name, "You have got", right, "Answers right,", wrong, "Answers wrong and you have completed",
          question_number, "Questions\n")
    time.sleep(4)

    def question_four():
        global right, wrong, question_number
        print("How many new babys where born in New Zealand in 2015")
        print("Is it A:61,000 B:208,000 C:98,000 or D:18,000")
        ans = str(input())
        if ans == "D" or ans == "d" or ans == '100000' or ans == '4':
            print("You got it right!")
            right += 1
        else:
            print("You got it wrong!")
            wrong += 1
        question_number += 1

    question_four()

questions()

Digging deeper:

Globals

Try to avoid using globals as much as possible. Global variables should be avoided because they inhibit code reuse. The reason they are bad is that they allow functions to have hidden (as in "non-obvious" and "undeclared") and thus hard to understand side effects. Also, this can lead to Spaghetti code.

Duplicate code

As you can see, your functions are pretty much the same, so you can use this and try making a single method. More, we can get rid of that ugly usage of inner methods. It's just not necessary in this case.

What I'd do is:

Create three lists: one containing questions, one answers and another one which contains the correct answers.
Then create a simple method, and basically do what you've done, but in a shorter / nicer way.

Let's see what we have:

list_of_questions = ['What is the population of New Zealand ?',
                     'What year did the first european set foot on New Zealand (Abel Tasman) ?',
                     'How many Kiwi are there left in New Zealand (Approx) ?',
                     'How many new babys where born in New Zealand in 2015 ?']

lists_of_answers = [['A:1830', 'B:1543', 'C:1642', 'D:1765'],
                    ['A:1830', 'B:1543', 'C:1642', 'D:1765'],
                    ['A:2000', 'B:600',  'C:70,000', 'D:100000'],
                    ['A:61,000', 'B:208,000', 'C:98,000', 'D:18,000']]

list_of_correct_answers = [['D', 'd', '4.5', '4'],
                           ['C', 'c', '1642', '3'],
                           ['D', 'd', '100000', '4'],
                           ['D', 'd', '100000', '4']]

def questions():
    wrong = 0
    right = 0

    for each_question, each_answer, each_correct_answer in zip(list_of_questions, lists_of_answers, list_of_correct_answers):
        print(each_question + '\n' + ' '.join(each_answer))
        get_answer = raw_input()

        if get_answer in each_correct_answer:
            print('Your answer is correct!\n')
            right += 1
        else:
            print('That is not the answer I had in mind!\n')
            wrong += 1
        print('So far, you answered correctly to {0} questions and incorrectly to {1}. Good luck!'.format(right, wrong))

if __name__ == '__main__':
    questions()

You can observe that I also added if __name__ == '__main__':. The reason I did this is that you can also import your code as a module in another script and then run the main function if and when your program decides.

Next steps:

Please try to be consistent when writing code.
Avoid printing to many things. And if you do so, try to review your grammar.


Simple calculator (going H.A.M with defining functions) I am toying around and playing a lot with defining a function. I'm trying to get extremely comfortable with it. The code I have wrote works fairly well, but definitely is not perfect. Also, the function I made called playagain is having some errors. When I type in "Yes" for the input I'm getting a strange TypeError. If you can fix that, please explain to me how, in the simplest way possible.

import sys

OPTIONS = ["Divide", "divide", "Multiply", "multiply", "Add", "add", "Subtract", "subtract"]

def userinput():
    while True:
        try:
            number = int(input("Number: "))
            break
        except ValueError:
            print("NOPE...")
    return number

def operation():
    while True:
        operation = input("Multiply/Divide/Add: ")
        if operation in OPTIONS:
            break
        else:
            print("Not an option.")
    return operation

def playagain():
    while True:
        again = input("Again? Yes/No: ")
        if again == "Yes" or again == "yes":
            break
        elif again == "No" or again == "no":
            sys.exit(0)
        else:
            print("Nope..")

def multiply(x,y):
    z = x * y
    print(z)

def divide(x,y):
    z = x / y
    print(z)

def add(x,y):
    z = x + y
    print(z)

def subtract(x,y):
    z = x - y
    print(z)

while True:
    operation = operation()
    x = userinput()
    y = userinput()
    if operation == "add" or operation == "Add":
        add(x,y)
    elif operation == "divide" or operation == "Divide":
        divide(x,y)
    elif operation == "multiply" or operation == "Multiply":
        multiply(x,y)
    elif operation == "subtract" or operation == "Subtract":
        subtract(x,y)

    playagain()

I tried defining as much functions as possible to make it simpler and easier to fix and debug. Please let me know what you think.

NOTE: I am not making this program to actually be a beneficial program. I am simply using it to help myself with the defining a function portion of Python.

Of the feedback on your earlier version of this programme, the most obvious thing that you haven't done yet is to use lower() on text inputs so that you only have to compare against lower case options. e.g.

    again = input("Again? Yes/No: ").lower()
    if again == "yes":
        break
    elif again == "no":
        sys.exit(0)

It might be useful to know that the operations you define already exist as methods of numeric variables in Python, called __mul__, __add__, etc., and are also available within the operator module. e.g. operator.add(3, 4) is equivalent to 3 + 4. So you could, for example, do this

import operator

OPTIONS = {
    'divide': operator.truediv,
    'multiply': operator.mul,
    'add': operator.add,
    'subtract': operator.sub
}

def user_input():
    while True:
        try:
            return int(input("Number: "))
        except ValueError:
            print("NOPE...")

def get_operation():
    while True:
        operation = input("Multiply/Divide/Add/Subtract: ").lower()
        try:
            return OPTIONS[operation]
        except KeyError:
            print("Not an option.")

def play_again():
    while True:
        again = input("Again? Yes/No: ").lower()
        if again == "yes":
            return True
        elif again == "no":
            return False
        else:
            print("Nope..")

while True:
    operation = get_operation()
    x = user_input()
    y = user_input()
    print(operation(x, y))
    if not play_again():
        break


Binary permissions system with bitwise operators + properties I've written a simple permissions system that I need to be rather fast, yet useable by other developers who primarily use Java. Is a bitwise permissions system with properties a good compromise. Any suggested speed improvements would be greatly appreciated.

class AppliancePerms(object):
    """Permissions for the Appliance class."""
    _cmd_right = 2 ** 0
    _rest_right = 2 ** 1
    _ssh_right = 2 ** 2
    _num_of_rights = 3

    _all_rights = _cmd_right + _rest_right + _ssh_right

    def __init__(self, perm_digits=None):
        # default to all rights
        if perm_digits is None:
            self._perms = AppliancePerms._all_rights
        else:
            self._perms = perm_digits

    @property
    def permissions(self):
        # zfill to show the total number of permissions even when left most right isn't True
        return bin(self._perms)[2:].zfill(AppliancePerms._num_of_rights)

    @permissions.setter
    def permissions(self, perm_digits):
        """
        Set all permissions at once with a number.
        Suggested syntax is AppliancePermOInst.permissions(int('111', 2))
        """
        self._perms = perm_digits

    @permissions.deleter
    def permissions(self):
        self._perms = 0

    @property
    def rest(self):
        return self._perms & AppliancePerms._rest_right

    @rest.setter
    def rest(self, boolean):
        if boolean:
            self._perms |= AppliancePerms._rest_right
        else:
            del self.rest

    @rest.deleter
    def rest(self):
        if self._perms & AppliancePerms._rest_right:
            self._perms -= AppliancePerms._rest_right

    @property
    def ssh(self):
        return self._perms & AppliancePerms._ssh_right

    @ssh.setter
    def ssh(self, boolean):
        if boolean:
            self._perms |= AppliancePerms._ssh_right
        else:
            del self.ssh

    @ssh.deleter
    def ssh(self):
        if self._perms & AppliancePerms._ssh_right:
            self.perms -= AppliancePerms._ssh_right

    @property
    def cmd(self):
        return self._perms & AppliancePerms._cmd_right

    @cmd.setter
    def cmd(self, boolean):
        if boolean:
            self._perms |= AppliancePerms._cmd_right
        else:
            del self.cmd

    @cmd.deleter
    def cmd(self):
        if self._perms & AppliancePerms._cmd_right:
            self._perms -= AppliancePerms._cmd_right

I don't like how static and WET your class is.
You don't really need all the _rights, and you should probably remove the repetition in all your properties, except permissions.
I'd make a function to build properties for you.
Take the cmd property:

@property
def cmd(self):
    return self._perms & AppliancePerms._cmd_right

@cmd.setter
def cmd(self, boolean):
    if boolean:
        self._perms |= AppliancePerms._cmd_right
    else:
        del self.cmd

@cmd.deleter
def cmd(self):
    if self._perms & AppliancePerms._cmd_right:
        self._perms -= AppliancePerms._cmd_right

This is almost identical to the other two functions, the only difference is AppliancePerms._cmd_right.
And so if we were to want to write a function to build this then we'd start by replacing that, with a say variable bit.
Since you're using @property, rather than property I'm not sure how the del fn_name would work,
and so I'd change it to use the property function.
This means we can get:

def build_permission_property(bit):
    def get(self):
        return self._perms & bit

    def set(self, boolean):
        if boolean:
            self._perms |= bit
        else:
            delete(self)

    def delete(self):
        if self._perms & bit:
            self._perms -= bit

    return property(get, set, delete)

This works as you'd want, but I'd change the delete to use a bit mask.
Since it'll be a mask we can then use &=, and remove the branch.
To make the mask we need it actually really easy.
It's just ~bit and since Python's int's are 'infinite' it'll work on all numbers.
I'd also change get to return 1 or 0 as it stands it can return 0, 1, 2, 4, ....
Changing these can result in:

def build_permission_property(bit_index):
    bit = 1 << bit_index
    def get(self):
        return (self._perms & bit) >> bit_index

    def set(self, boolean):
        if boolean:
            self._perms |= bit
        else:
            delete(self)

    def delete(self):
        self._perms &= ~bit

    return property(get, set, delete)

And to use it is quite simple.
We just set the class' variable to the property.
I'd keep _num_of_rights, and would change the permissions getter to use str.format
And so could result in:

class AppliancePerms(object):
    def __init__(self, perm_digits=None):
        # Default to all permissions
        if perm_digits is None:
            perm_digits = (1 << type(self)._num_of_rights) - 1
        self._perms = perm_digits

    cmd = build_permission_property(0)
    rest = build_permission_property(1)
    ssh = build_permission_property(2)
    _num_of_rights = 3

    @property
    def permissions(self):
        return '{:0>{}b}'.format(self._perms, type(self)._num_of_rights)

    @permissions.setter
    def permissions(self, perm_digits):
        self._perms = perm_digits

    @permissions.deleter
    def permissions(self):
        self._perms = 0

This is quite nice, all you need to do is manually set the properties, and _num_of_rights to the amount.
But it's still too WET for me.
And so I'd use the more advanced Python feature of metaclasses.
This may not be good for developers that don't know Python too well.
I'd base this metaclass off one of martineau's answers on SO.

You can program this in two classes,
but I'd prefer three as then if you need more permission classes it's easy to implement.
These classes are:

Permission metaclass.
This will take a list from the class, and build the properties.
Base permissions class.
This will inherit the metaclass in a Python 2 and Python 3 friendly way. (the cross-Python metaclass inheritance is odd.)
It will also define the permissions property, and default _perms to zero, so subclasses don't have to.
Subclass.
This will be a domain specific class that provides the list of properties.
It will also define custom functionality, such as defaulting to all permissions.

This could become:

def build_permission_property(bit_index):
    bit = 1 << bit_index
    def get(self):
        return (self._perms & bit) >> bit_index

    def set(self, boolean):
        if boolean:
            self._perms |= bit
        else:
            delete(self)

    def delete(self):
        self._perms &= ~bit

    return property(get, set, delete)

class PermissionsMetaClass(type):
    def __new__(meta, classname, bases, class_dict):
        dict_ = class_dict.copy()
        dict_.pop('__module__', None)
        dict_.pop('__qualname__', None)
        for i, perm in enumerate(dict_.get('_permissions', [])):
            dict_[perm] = build_permission_property(i)
        return type.__new__(meta, classname, bases, dict_)

class Permissions(PermissionsMetaClass("Permissions", (object,), {})):
    _perms = 0
    @property
    def permissions(self):
        return '{:0>{}b}'.format(self._perms, len(type(self)._permissions))

    @permissions.setter
    def permissions(self, perm_digits):
        self._perms = perm_digits

    @permissions.deleter
    def permissions(self):
        self._perms = 0

class AppliancePerms(Permissions):
    _permissions = ['cmd', 'rest', 'ssh']
    def __init__(self, perm_digits=None):
        # Default to all permissions
        if perm_digits is None:
            perm_digits = (1 << len(type(self)._permissions)) - 1
        self._perms = perm_digits

I've provided both with and without a metaclass, as metaclasses are a bit harder to understand. And so it's up to you which design you'd rather use.
I mostly just recommend adding the function build_permission_property.


Circular Array Rotation C++ HackerRank I solved the Circular-Array-Rotation problem on HackerRank.  I conceded defeat and looked at the answers given on HackerRank itself but the ones I saw used things like vectors, which are concepts I'm a noob at as my knowledge of C++ thus far extends only as far as virtual functions and classes and such (junior COE student). So here's the description of the problem:

  John Watson performs an operation called a right circular rotation on an array of integers, \$a[0], a[1], \dotsc, a[n-1]\$. After performing one right circular rotation operation, the array is transformed from \$a[0], a[1], \dotsc, a[n-1]\$ to \$a[n-1], a[0], \dotsc, a[n-2]\$.

  Watson performs this operation \$k\$ times. To test Sherlock's ability to identify the current element at a particular position in the rotated array, Watson asks \$q\$ queries, where each query consists of a single integer, \$m\$, for which you must print the element at index \$m\$ in the rotated array (i.e., the value of \$a[m]\$).

  Input Format

  The first line contains 3 space-separated integers, \$n\$, \$k\$, and \$q\$, respectively.

  The second line contains \$n\$ space-separated integers, where each integer \$i\$ describes array element \$a[i]\$ (where \$0 \le i < n\$).
  Each of the \$q\$ subsequent lines contains a single integer denoting \$m\$.

  Constraints

  \begin{align*}
  1 &\le n &&\le 10^5 \\
  1 &\le a[i] &&\le 10^5 \\
  1 &\le k &&\le 10^5 \\
  1 &\le q &&\le 500 \\
  0 &\le m &&\le n - 1
\end{align*}

  Output Format

  For each query, print the value of the element at index \$m\$ of the rotated array on a new line.

  Sample Input

3 2 3
1 2 3
0
1
2

  Sample Output

2
3
1

  Explanation

  After the first rotation, the array becomes \$[3,1,2]\$.
  After the second (and final) rotation, the array becomes \$[2,3,1]\$.

  Let's refer to the array's final state as array \$b\$. For each query, we just have to print the value of \$b[m]\$ on a new line:

  \$m = 0\$, so we print 0 on a new line.
  \$m = 1\$, so we print 1 on a new line.
  \$m = 2\$, so we print 2 on a new line.

This is my code, sadly it only works for a few of the test cases, and times out for the others. Any feedback welcome, including suggestions on learning new methods or anything.

int n;
int k;
int q;
cin >> n >> k >> q;
int *arr = new int[n];
int *query = new int[q];
for (int i = 0; i < n; i++)
{
    cin >> arr[i];
}
for (int j = 0; j < q; j++)
{
    cin >> query[j];
}

int *arr2 = new int[n];
int count = 0;
while (count < k)
{
    int temp = arr[n - 1];
    for (int j = 1; j < n; j++)
    {
        arr2[j] = arr[j - 1];
    }
    arr2[0] = temp;
    for (int j = 0; j < n; j++)
    {
        arr[j] = arr2[j];
    }
    count++;
}

for (int m = 0; m < q; m++)
{
    cout << arr2[query[m]] << endl;
}

Rather than performing a rotation k times, perform one rotation of k places.

So, you have an array [5, 4, 3, 2, 1] and want to rotate it 3 places:

Create a new array containing the results
Start iterating through the array, calculating the index in the new array and assigning the values
Starts at i = 0 (value 5). Wants a rotation of 3, so newArray[i + k] = array[i]
Continues this up to i = 2, where we find a hitch: newArray[i + k] = array[i], a.k.a. newArray[5] = array[2], so out of bounds. Uh oh!
We must make the assignment cycle around, so we must use mod with the size of the array to produce a single iteration over the initial array with this: newArray[(i + k) % n] = array[i]


Accumulating inner exception messages Say, there is a class with some methods, that have try catch blocks. I need to accumulate messages from all inner exceptions of generated exception and use throw new Exception(exceptionMessages).
I came up with two possible solutions:

Create a private method, concatenate inner exception messages, return a string and raise an exception in a caller method. (SomeMethod1 below);
Create a private method, concatenate inner exception messages and raise exception inside it. (SomeMethod2 below);

Code snippet:

class SomeClass
{
    private void _raiseException(Exception ex)
    {
        var exceptionMessages = new StringBuilder();
        do
        {
            exceptionMessages.Append(ex.Message);
            ex = ex.InnerException;
        }
        while (ex != null);
        throw new Exception(exceptionMessages.ToString());
    }

    private string _getInnerExceptions(Exception ex)
    {
        var exceptionMessages = new StringBuilder();
        do
        {
            exceptionMessages.Append(ex.Message);
            ex = ex.InnerException;
        }
        while (ex != null);
        return exceptionMessages.ToString();
    }

    public void SomeMethod1()
    {
        try
        {
            //some code to try
        }
        catch (Exception e)
        {
            throw new Exception(this._getInnerExceptions(e));
        }
    }

    public void SomeMethod2()
    {
        try
        {
            //some code to try
        }
        catch (Exception e)
        {
            this._raiseException(e);
        }
    }
}

Which one of them would be better, or is there another way of doing it, or maybe this approach is not good at all?

Svick is right

Use the AggregateException class to collect all exceptions you need. With AggregateException everyone can prepare it's own code to handle the multiple exception situation but with your original solution they have to handle a huge string in a stock exception class. How can be this usefull?

Storing exception.Message

It's bad. If you store only the exceptions' messages you will loose a lot of information and the types of the exceptions. How would you handle an exception if only the Message is what you have? Parsing it? And we havent talked about the specific properties of specific exceptions like some validation exception which can contain the unvalid property name and value for an object.


Configuring limits in a usercontrol I am trying to solve the warning, which avoids the call of a virtual method, I have seen different answers, but none of them seems to solve my doubt, I understand why this warning appears and is one of the ones you should not suppress. My code gets data from Matlab and uses a method to configure the limits in a usercontrol. Should I build another class to call from there the method?
abstract public class MatlabData:eSoundMaster
{
    //[sdon] changed to override from derived class
    public bool EQAMAvail = false;
    public readonly int numTonspuren = 32;
    public readonly int numSubgroups = 8;
    public readonly int numStandardTS = 22;
    public readonly int numPremiumTS = 10;
    public readonly int numOutputs = 2;
    public readonly int numSoundtrackLUTs = 124;

    public MatlabData(int nTonspuren, int nIntermediateMixerChannels, int nStandardTS, int nPremiumTS, int LimiterNumSettings)

    {
        //Override predefined vars with values from child class
        this.numTonspuren = nTonspuren;
        this.numSubgroups = nIntermediateMixerChannels;
        this.numStandardTS = nStandardTS;
        this.numPremiumTS = nPremiumTS;
        //this.ProjectType = ProjectType;
        this.LimiterNumSettings = LimiterNumSettings;
        ConfigLimiter();
    }

    public bool isEQAMAvailable()
    {
        return EQAMAvail;
    }

    public virtual void ConfigLimiter()
    {
        //Overwrite from derived class
    }
}

class MatlabData_MLBevo : MatlabData
{
    public new const int numTonspuren = 32;
    public new const int numSubgroups = 8;
    public new const int numStandardTS = 22;
    public new const int numPremiumTS = 10;
    public new const int LimiterNumSettings = 7;

    public MatlabData_MLBevo()
        :base(numTonspuren, numSubgroups, numStandardTS, numPremiumTS, LimiterNumSettings)
    {
        EQAMAvail = false;
    }

    //ConfigLimiter method in this class override method in base class automatically
    //parts of the label already configured in matlabconstants region of this class
    public override void ConfigLimiter()
    {
        LabelText = new string[LimiterNumSettings];
        LabelText[0] = "Threshold (dB)";
        LabelText[1] = "Out Gain (dB)";
        LabelText[2] = "Knee Depth (dB)";
        LabelText[3] = "Ratio (dB)";
        LabelText[4] = "Attack Time (ms)";
        LabelText[5] = "Decay Time (ms)";
        LabelText[6] = "Post Limiter Gain";

        LabelName = new string[LimiterNumSettings + 2];
        LabelName[0] = "LimiterCore1.threshold";
        LabelName[1] = "LimiterCore1.outputGain";
        LabelName[2] = "LimiterCore1.kneeDepth";
        LabelName[3] = "LimiterCore1.ratio";
        LabelName[4] = "LimiterCore1.attackTime";
        LabelName[5] = "LimiterCore1.decayTime";
        LabelName[6] = "PostLimGain.gainDB";
        LabelName[7] = "gsys.SYS.";
        LabelName[8] = "Limiter_Stereo.Limiter_CH";

        LabelMinimum = new double[LimiterNumSettings];
        LabelMinimum[0] = -60;
        LabelMinimum[1] = 0;
        LabelMinimum[2] = 0.1;
        LabelMinimum[3] = 1;
        LabelMinimum[4] = 0.009;
        LabelMinimum[5] = 0.01;
        LabelMinimum[6] = -100;

        LabelMaximum = new double[LimiterNumSettings];
        LabelMaximum[0] = 0;
        LabelMaximum[1] = 100;
        LabelMaximum[2] = 60;
        LabelMaximum[3] = 100;
        LabelMaximum[4] = 1000;
        LabelMaximum[5] = 1000;
        LabelMaximum[6] = 20;
    }
}

There are quite a few issues in this code to worry about on top of the virtual call in the constructor...
abstract public class MatlabData:eSoundMaster

The accessibility modifier should always come first and classes in C# are always PascalCase.
public abstract class MatlabData : ESoundMaster

You are using public readonly fields in the base class:
 public readonly int numTonspuren = 32;

It's usual to have public readonly fields use PascalCase
You then hide them with constants in the derived class and pass them to the base class's constructor. Don't hide the fields!

Don't abbreviate in your code. If you are going to, be consistent about it. E.g. nTonspuren vs numTonspuren.
Methods in C# should be PascalCase. isEQAMAvailable should be IsEQAMAvailable. That said, EQAMAvail should be a readonly property and the method should be removed.
In general, and without a good reason not to*, one should prefer properties to fields in C#. It's idiomatic to do that and it's an extra level of abstraction and leaves you free to change how that property is implemented without breaking consumers.
From the example, it's not clear why you have a class hierarchy at all here. It looks like you need one class with a factory:
// Typed directly on CodeReview, apologies for errors.
public class LabelDefinition
{
    public string Text { get; }
    public string Name { get; }
    // etc.
}

public class MatlabData : eSoundMaster
{
    public bool EQAMAvail { get; }
    public int NumTonspuren { get; }
    public int NumSubgroups { get; }
    public int NumStandardTS { get; }
    public int NumPremiumTS { get; }
    public int NumOutputs { get; }
    public int NumSoundtrackLUTs { get; }

    public MatlabData(
        int numTonspuren,
        int numIntermediateMixerChannels,
        int numStandardTS,
        int numPremiumTS,
        LabelDefinition[] labels,
        string[] additionalLabelNames)

    {
        NumTonspuren = numTonspuren;
        NumSubgroups = numIntermediateMixerChannels;
        NumStandardTS = numStandardTS;
        NumPremiumTS = numPremiumTS;
        LabelText = labels.Select(l => l.Text).ToArray();
        LabelName = labels.Select(l => l.Name).Concat(additionalLabelNames).ToArray();
        // etc.
    }
}

public class MatlabDataFactory
{
    public MatlabData CreateDefault => new MatlabData(...);
}

*There are vanishingly few reasons to use a public field over a property.


Find the repeated elements in a list You are given an array of n+2 elements. All elements of the array are in range 1 to n. All elements occur once except two numbers, which occur twice. Your task is to find the two repeating numbers.

My solution is:

t=int(input()) #number of test cases
for _ in range(t):
    n=int(input()) # no. of array elements
    l=[int(x) for x in input().split()]  #array elements
    for i in range(1,n-1):
        if l.count(i)==2:  # using count function to count the elements
            print(i,end=' ')
    print()

How can I improve it?  Currently, it shows time limit exceeded on hackerrank.

Minor

Consider using the unittest module instead of using input to test
Use a if __name__ == "__main__" guard
Make it a function
Don't print but return variables

Timing improvements

  l.count(i) == 2

It will search the entire list from the start.

Let's take a sample input (worst case) of [2, 3, 4, 5, 6, 7, 8, 1, 1, 2, 2], it will search the entire list 10 times!

This can be done in \$ O(n) \$ (worst case), because we know

  All elements of the array are in range 1 to n

You can create a list temp with length n filling it with None or 0
For every number in the array, check if the value in temp[i] is empty (0 or None)

If it is empty put the value on that position
If the number is the same as the value of the temp list, we have seen it before

Revised Code

def find_repeating(lst):
    counts = [None] * len(lst)
    for i in lst:
        if counts[i] is None:
            counts[i] = i
        elif i == counts[i]:
            yield i

Note

This has the disadvantage of continuing to loop after the numbers are found, to avoid this you could add the elements to another array and return when the length of the array is 2.

It will not make the most "beautiful" code, but it will be faster than @Peilonrayz's answer for anything but worst case, since that requires to loop through the entire array.

def find_repeating(lst, count=2):
    ret = []
    counts = [None] * len(lst)
    for i in lst:
        if counts[i] is None:
            counts[i] = i
        elif i == counts[i]:
            ret += [i]
            if len(ret) == count:
                return ret

Timing all solutions


A class that uses a few other classes to generate an email I seem to find my self writing a lot of classes like the below where there seems to be some kind of send method that just calls a bunch of other methods.  Is this bad practice?  Is there a more object oriented way to do this?

The below class uses User, UserDetail (preference object), and a custom Mailchimp api client.  It basically goes through creates a campaign, adds the html body to that campaign and then sends it.

# Generate and send mailers
class Mail
  def initialize(user_id)
    @user = User.find(user_id)
    @detail = @user.details
    @client = @user.mailchimp
    @campaign_id
  end

  def send
    create_campaign
    create_html_content
    send_campaign
  end

  private

  def create_campaign
    params = @detail.create_campaign_params
    @campaign_id = @client.create_campaign(params)
  end

  def create_html_content
    params = @detail.create_content_params
    @client.create_campaign_content(@campaign_id, params)
  end

  def send_campaign
    @client.send_campaign(@campaign_id)
  end
end

I find it weird that you wrote this as a module rather than as a class.

How is this code going to be called?  Mail.new(user_id).send, I assume?  It doesn't seem to make much sense to call #send twice on the same object.  So, really, there isn't any point to make an object at all.  You could just as well define a function.

def send_mail_to_user(user_id)
  user = User.find(user_id)
  client = user.mailchimp
  campaign_id = client.create_campaign(user.detail.create_campaign_params)
  client.create_campaign_content(campaign_id, user.detail.create_content_params)
  client.send_campaign(campaign_id)
end


HackerNews news fetcher The following code is part of a big application. I am new to Go, and appreciate feedback on the best practices and improvements. Please consider styling and comments as well.

Every 30 minutes, this will fetch news from HackerNews and store it in a database:

What fetches the news:

package services

import (
    "fmt"
    "net/http"
    "log"
    "io/ioutil"
    "github.com/demas/cowl-go/pkg/quzx-crawler"
    "encoding/json"
    "github.com/demas/cowl-go/pkg/postgres"
    "time"
    "strconv"
)

// represent an implementation of quzx_crawler.HackerNewsService
type HackerNewsService struct {
}

const idsUrl = "https://hacker-news.firebaseio.com/v0/topstories.json?print=pretty"
const newsUrl = "https://hacker-news.firebaseio.com/v0/item/%d.json?print=pretty"
const syncInterval = 60 * 30 // 30 minutes

func (s *HackerNewsService) Fetch() {

    var lastSyncTime int64
    var err error

    lastSyncTimeStr := (&postgres.SettingsRepository{}).GetSettings("lastHackerNewsSyncTime")
    if lastSyncTimeStr == "" {
        lastSyncTime = time.Now().Unix() - syncInterval - 1
    } else {
        lastSyncTime, err = strconv.ParseInt(lastSyncTimeStr, 10, 64)
        if err != nil {
            log.Fatal(err)
        }
    }

    currentTime := time.Now().Unix()

    if lastSyncTime + syncInterval > currentTime {
        return
    }

    // get list of messages
    res, err := http.Get(idsUrl)
    if err != nil {
        log.Fatal(err)
    }

    jsn, err := ioutil.ReadAll(res.Body)
    res.Body.Close()
    if err != nil {
        log.Fatal(err)
    }

    // decode
    var ids []int64
    err = json.Unmarshal(jsn, &ids)

    if err != nil {
        log.Fatal(err)
    } else {

        for _, id := range ids {

            if !(&postgres.HackerNewsRepository{}).NewsExists(id) {

                // fetch each news
                log.Println("hacker news: " + fmt.Sprintf(newsUrl, id))
                res, err := http.Get(fmt.Sprintf(newsUrl, id))
                if err != nil {
                    log.Fatal(err)
                }

                jsn, err := ioutil.ReadAll(res.Body)
                res.Body.Close()
                if err != nil {
                    log.Fatal(err)
                }

                // decode
                var news quzx_crawler.HackerNews
                err = json.Unmarshal(jsn, &news)

                if err != nil {
                    log.Fatal(err)
                } else {
                    (&postgres.HackerNewsRepository{}).InsertNews(news)
                }
            }
        }
    }

    (&postgres.SettingsRepository{}).SetSettings("lastHackerNewsSyncTime",  strconv.FormatInt(currentTime, 10))
}

What stores the data:

package postgres

import (
    "log"
    "github.com/demas/cowl-go/pkg/quzx-crawler"
)

// represent a PostgreSQL implementation of quzx_crawler.HackerNewsService
type HackerNewsRepository struct {
}

func (r *HackerNewsRepository) NewsExists(id int64) bool {

    var cnt int
    err := db.Get(&cnt, "SELECT count(*) FROM HackerNews WHERE Id = $1", id)

    if err != nil {
        log.Fatal(err)
    }

    return cnt != 0
}

func (r *HackerNewsRepository) InsertNews(n quzx_crawler.HackerNews) {

    tx := db.MustBegin()

    insertQuery := `INSERT INTO HackerNews(Id, By, Score, Time, Title, Type, Url, Readed)
                VALUES($1, $2, $3, $4, $5, $6, $7, $8)`

    _, err := tx.Exec(insertQuery,
              n.Id, n.By, n.Score, n.Time, n.Title, n.Type, n.Url, 0)

    if err != nil {
        log.Fatal(err)
    }

    tx.Commit()
}

newsUrl → newsURL, same for idsUrl.
const syncInterval = 30 * time.Minute is more readable. You can also convert everything that represents a time into a time.Duration or a time.Time, rather than deal with Unix timestamps. There are functions in the time package that do what you want to do (time.Date, time.Since, etc.).
About that, why does Fetch need to do complicated things with times, checking when was the last time it was called, etc.? Shouldn't the caller take care of this with a simple time.Ticker instead? It feels like this complexity shouldn't be there.
HackerNewsService is a weird name for an implementation. HackerNewsFetcher maybe?
Your postgres package isn't very idiomatic, which makes the callers do weird things to use it. For example, why do you have a SettingsRepository struct that doesn't contain anything? Why not simply have GetSettings as a package-level function? Same for the other empty structs of this package (HackerNewsRepository…).
It's more idiomatic to test for len(myString) == 0 rather than myString == "".
I'm not exactly sure what you're doing with lastSyncTime (this is pretty complex). Maybe add a comment for the line lastSyncTime = time.Now().Unix() - syncInterval - 1 or refactor this part?
Talking about refactor, Fetch() is way too long. Split it in sub-functions.
No need for an else after log.Fatal.
Rather than:

// fetch each news
log.Println("hacker news: " + fmt.Sprintf(newsUrl, id))

why not:
    log.Println("fetching from hacker news", fmt.Sprintf(newsUrl, i))
Using log.Fatal everywhere makes your code not very resilient. Maybe it's worth looking at "what can fail for transient reasons" (typically, database transactions…), and re-try the thing that failed a few times with exponential backoff.
There's lots of whitespace in the "what stores the data" part, which isn't very idiomatic. Empty lines in functions aren't really frequent.


Promise-based HTTP abstraction layer I am currently working in a project based on a microservices architecture pattern.
Services are wired up by HTTP calls. They eventually call each others for fetching or putting some data.

I am working on the web page which envelops the whole microservices. I mean, the web page actually needs to send and retrieve data from all the services (except one).
There are several times where the web page behaves as a broker, i.e.:

  Fetch some data from service 1,
  fetch some data from service 2.
  Send something to service 3 based on previous responses.

That's why I decided to implement some functions to avoid lots of duplicated code chunks. This is what I call the promise-based HTTP abstraction layer.

I am developing the web page in Typescript, by the way.

I want to mention what my request function is. The request function performs HTTP requests, unsurprisingly.
It is an axios call wrapper. However, for the sake of simplicity, since it is not that important I'll show only the header.

type HttpMethod = 'GET' | 'POST' | 'PUT' | 'PATCH' | 'DELETE';
declare function request<TResult>(method: HttpMethod, url: string, body?: any): Promise<TResult>;

And, for comfort, I declared its prefixed argument different calls

const get = request.bind(this, 'GET');
const post = request.bind(this, 'POST');
// And so on ...

But I refactored this to take profit of the request type argument

const get = <TResult>(url: string, body: any) => request<TResult>('GET', url, body);
const post = <TResult>(url: string, body: any) => request<TResult>('POST', url, body);
// And so on ...

And, a waitAll function which requests to call the URLs passed in and returns a single Promise which will be resolved whenever all the requests are done.

interface HttpRequest<TResult> {
    url: string;
    request: (url: string, body?: any) => Promise<TResult>;
    body?: any;
};
const waitAll = (...reqs: HttpRequest<any>[]): Promise<any[]> =>
    Promise.all(reqs.map(req => req.request(req.url, req.body)));

With this, I can do something like

(async () => {
    const req1: HttpRequest<FirstResponse> = {
        url: `http://example.com/one`,
        request: post,
        body: {
            id: 9,
            data: [`hi`, `there`]
        }
    };
    const req2: HttpRequest<SecondResponse> = {
        url: `http://example.com/one`,
        request: get
    };

    const results = await waitAll(req1, req2);
    const [r1, r2] = results;

    const body = { ...r1, ...r2 };
    const finalResponse = await post<ThirdResponse>(`http://example.com/three`, body);

    // do something w/ final response
})();

This is working but I notice some wrong things.
It feels weird the HttpResponse<> object, that wraps the url and the body that, in fact, will be called inside its own request property function.
And when I call waitAll, I'd expect my r1 and r2 to be of the matching type (I mean, r1 must be
 of type FirstResponse, and r2 must be of type SecondResponse) just like finalResponse is of type ThirdResponse.

I am pretty sure there are lots of improvements to achieve a better-looking API and, the most important thing, take profit of Typescript type safety.

Any thoughts are welcome.

First thing, you're right about HttpRequest and the oddness of having to pass manually body and url .

To improve the code, I would suggest to chose a programming paradigm, and to stick to it:

example in OOP:

class HttpRequest<TResult> {
    constructor(private method: HttpMethod, private url: string, private body?: any) {}
    request():Promise<TResult> {
        return request(this.method, this.url, this.body);
    }
};

const req1  = new HttpRequest<FirstResponse>('POST', `http://example.com/one`, {
    id: 9,
    data: [`hi`, `there`]
  });
const req2 = new HttpRequest<SecondResponse>('GET', `http://example.com/one`);

// you now just have to call req1.request() for example , without parameters

in more FP friendly way of doing thing:

function my_request<T>(httpReq: HttpRequest) {
    return  (): Promise<T> => request(httpReq.method, httpReq.ufl, httpReq.body)
}

const req1  = my_request<FirstResponse>({method:'POST', url:`http://example.com/one`, body:{
    id: 9,
    data: [`hi`, `there`]
});
const req2 = my_request<SecondResponse>({method:'GET', url:`http://example.com/one`});

// you now just have to call req1() for example , without parameters

Now, we can improve the typing of waitAll with mapped types, for example for the OOP version (it's equivalent for the FP one):

function waitAll<T extends any[]>(...reqs: {[K in keyof T]: HttpRequest<T[K]>}): Promise<T> {
  // the any here is mandatory as Promise.all doesn't support (yet) the above signature
  return Promise.all(reqs.map(req => req.request())) as any;
}

// result type will be a [FirstResponse, SecondResponse]
const result = waitAll(req1, req2)

Now, you should ask yourself if you're not "over-abstracting". A code like that:

const req1 = request('POST', `http://example.com/one`, {id: 9});
const req2 = request('GET', `http://example.com/one`);

const [r1, r2] = await Promise.all(req1, req2);

const finalResponse = await request('POST', `http://example.com/three`, body);

uses only standard TS stuffs (await, Promise.all), hence is often easier to read for a developper that didn't write the code.


Ad filtering in Angular factory I'm an 'old school' programmer (C, C++), and I have step-by-step, algorithmic thinking. I'm writing some code in Angular + Django and I know that this code is ugly but I don't have any idea how to refactor it.

First of all, I have the models:

class Product(models.Model):
    name = models.CharField(max_length=100)
    status = models.CharField(max_length=10, choices=STATUSES.items(), default='new')

class Ad(models.Model):
    image = models.ImageField(upload_to="ad")
    width = models.IntegerField(blank=True)
    height = models.IntegerField(blank=True)
    product = models.ForeignKey(Product, null=True, blank=True)

class AdStatus(models.Model):
    status = models.IntegerField(default=1)
    ad = models.ForeignKey(Ad)

Next I have simple classes for serializing:

class AdSerializer(serializers.HyperlinkedModelSerializer):
    class Meta:
        model = Ad
        fields = ["url", "id", "image", "width", "height". "product"]
#etc...

And every model has its own factory in Angular:

  .factory('AdStatus',["$resource", function ($resource){
    var Resource = $resource(
        "/api/ad_status/:ad_id/",
        {ad_id: '@id'},
        {
            query: {
                isArray: true,
                transformResponse: function (data) {
                    var items = angular.fromJson(data);
                    return items.results;
                }
            },
            update: {
                method: "PUT",
            }
        },
        {
            stripTrailingSlashes: false
        }
    );
    Resource.prototype.changeStatus = function(status){
        this.status = status;
        this.$update();
    };
    return Resource;
  }])
/etc

This is the main code:

//controller isn't bad

angular.module('controllers.statuses', [])
.controller('StatusCtr', ["$scope", "$stateParams", "StatusSetter",  function($scope, $stateParams, StatusSetter) {
    $scope.click = function(){
        $scope.t = StatusSetter.change($stateParams.id);// this is id of product
    };
}]);

But this makes me a little sick:

.factory('StatusSetter', ["AdStatus", "Product", "Ad", function(AdStatus, Product, Ad){
    var changeStatus = function(id, productStatus, adStatus){
        var product = Product.get({product_id: id});
        product.$promise.then(function(data) {
            product.changeStatus(productStatus);
            var ads = Ad.query();
            var ad_status = AdStatus.query();
            var tmp = [];
            ads.$promise.then(function(data){
                for (var i = 0; i < ads.length; i++) {
                    if (ads[i].product == product.url){
                        tmp.push(ads[i]);
                    }
                }
                ad_status.$promise.then(function(data){
                    for (var i = 0; i < ad_status.length; i++){
                        for (var j = 0; j < tmp.length; j++){
                            if (ad_status[i].ad == tmp[j].url){
                                ad_status[i].changeStatus(adStatus);
                            }
                        }
                    }

                })
            });
        });
        return product;
    };
    return {
        change: function(id){
            return changeStatus(id, "new_status", 30);
        }
    }
  }])

The main philosophy of this code is:

  change AdStatuses which points at Ads (which points at Products)

Any ideas? Maybe I can separate this 'filtering' to $resource or something.

I think what you want to do is:

Get the product URL
Find the Ads with that product URL
Use each Ad URL to find the relevant AdStatus object
Update each AdStatus object

This can be done by chaining the operations with the $resource.$promise

.factory('StatusSetter', ["AdStatus", "Product", "Ad", "$q"
                          function(AdStatus, Product, Ad, $q)
{
    var changeStatus = function(id, newStatus, newValue){
        var filteredAdsList = [];
        var product = Product.get({product_id: id});
        product.$promise.then(function(productObj) {
            //chain product status change
            return productObj.changeStatus(newStatus).$promise;
        }) .then (function (productObj) {
            //wait for product status to finish change
            //chain Ad query
            return  Ad.query().$promise;
        }) .then (function (adsObj) (
            //push filtered list
            angular.forEach(adsObj, function (iAd)) {
                 if (iAd.product == product.url){
                     filteredAdsList.push(iAd);
                 }
             };
            //chain adStatus query
            return AdStatus.query().$promise;
        }) .then (function (adStatusObj) {})
            //start with empty promise
            var qPromise = $q.when();
            angular.forEach(adStatusObj, function (iAdStatus) {
                angular.forEach(filteredAdsList, function(jAd) {
                     if (iAdStatus.ad == jAd.url){
                         //add to promise chain
                         qPromise = qPromise.then (function() {
                             var nextPromise =
                                iAdStatus.changeStatus(newValue).$promise;
                             return nextPromise;
                         });
                     };
                });
            });
            //return promise chain
            return qPromise;
        }) .then (function () {
            console.log("all status updated")
        })
        return product;
    };
    return {
        change: function(id){
            return changeStatus(id, "new_status", 30);
        }
    }
}])

By returning data and promises and chaining those promises, you can create code that uses asynchronous APIs in a way that looks familiar.

For more information on the AngularJS $q service and chaining promises see  AngularJS $q Service API Reference - chaining promises

Also notice that I used angular.forEach for iteration. For more information on using angular.forEach see the AngularJS angular.forEach API Reference.


Prime number checker in Python 3 - follow-up I have updated my code from the last time it got reviewed. If anyone has some more suggestions on how I can improve this and/or make it faster it would be appreciated!

def is_prime(num):
    '''Calculates whether num is a prime number,
    if it is return True otherwise return False'''

    # SPECIALCASE: These are special cases that are not primes
    if num % 2 == 0 and num != 2 or num <= 1:
        return False

    # Look for a factor
    for i in range(3, int(num ** (0.5)) + 1, 2):
        # If there is a factor
        if num % i == 0:
            # Its not a prime number
            return False

    # If there is no factors and its not a special case
    return True

0. Overview

Your code is well written and what it does is clear. However I will argue that if you want a drastic increase in performance, you need to switch your algorithm. Testing primality can be done in polynomial time, but is generally a problem which requires sophisticated math. The problem can be solved in polynomal time as shown by the AKS_primality_test. However there are far faster algorithms than this one. As you can see, it is not the easiest thing to understand without a background in higher mathematics.

A review of your algorithm
Show a probabilistic approach
Compare your algorithm, the probabilistic, and the isprime function from the primefac package.

1. Review

As your code is fairly short, there is not much to comment on. I really like that you use a docstring in the beginning of your code to document it. The code looks good.

Use the if __name__ == "__main__": module. It makes it easier to test and easier to import.
Some of the comments are unnecessary and there is too much space in the last return.
The num <= 1 is a bit strange. It catches everything from inputing \$0\$ and \$1\$ to negative and decimal numbers. In my opinion it is better to treat negative numbers and non-integers with an error instead. Then we can explicitly remove \$0\$ and \$1\$ as special cases.

One way to handle this could be

try:
    int(num)
except ValueError:
    raise ValueError("Ops. Input must be a positive integer")
if int(num) != num or num < 0:
    raise ValueError("Ops. Input must be a positive integer")

if num in [0, 1]: return False

The last line could also have been written as

if num <= 1: return False

Since we have already tested that num => 0. However I prefer the first version as it is much clearer what it actually checks. You should always try to make it as clear as possible what you want to catch.

1.1 Some improvements on the algorithm

Say you check some number with your algorithm. If the number is not divisible by \$3\$ you do not need to ever check any multiples of \$3\$ again. Yet your code checks \$6\$, \$9\$ and so forth. A better approach would be to iterate over the numbers not divisible by \$2\$ or \$3\$. This removes the checking of \$2/3\$ of all numbers in comparison to only removing \$1/2\$. So, what numbers are not divisible by \$3\$ or \$2\$? Well all numbers are on the form \$6k+1\$, \$6k+2\$, \$6k+3\$, \$6k+4\$ or \$6k+5\$. Since they are either \$0\$, \$1\$, \$2\$, \$3\$, \$4\$ or \$5\$ numbers away from being a multiple of \$6\$. This means that only numbers on the form \$6k+1\$ or \$6k+5\$ are not divisible by \$2\$ or \$3\$.

Implementation is simple increase counter by \$6\$, test \$+1\$ and \$+5\$.

primes_to_remove = [2, 3, 5]
for prime in primes_to_remove:
    if num == prime: return True
    elif num % prime == 0: return False

# Look for a factor
limit = int(num**(0.5))+1
for multiple in range(6, int(num**(0.5))+1, 6):
    for k in [1, 5]:
        # If there is a factor
        if num % (multiple+k) == 0:
            # Its not a prime number
            return False

This can further be generalized by removing [2, 3, 5] as shown below

primes_to_remove = [2, 3, 5, 7, 11, 13, 19, 23, 29]
for prime in primes_to_remove:
    if num == prime: return True
    elif num % prime == 0: return False

# Look for a factor
limit = int(num**(0.5))+1
for multiple in range(30, int(num**(0.5))+1, 30):
    for k in [1, 7, 11, 13, 19, 23, 29]:
        # If there is a factor
        if num % (multiple+k) == 0:
            # Its not a prime number
            return False
return True

This idea of removing the first primes can again be modified into removing [2,3,5,7] and then check the remaining numbers. However the advantages of this method quickly diminish for larger primes as we are removing fewer and fewer numbers. As a last example, it is possible to only iterate over the primes. From the primesieve library we can import a prime generator.

from primesieve import Iterator

def is_prime_primegenerator(num):
    it = Iterator()
    prime = it.next_prime()

    # Iterate over the primes below sqrt(n)
    limit = int(num**(0.5))+1
    while prime < limit:
        prime = it.next_prime()
        if num % prime == 0:
            print num, prime
            return False
    return True

This is the best we can do following down your path of checking primes from the smallest to the largest. It assumes however that the next_prime() is extremely fast. Which it luckily is.

Using the 235 version of the sieve above, it gave me a \$2.5\$ times speed increase of your code. Putting it all together gives

def is_prime(num):
    '''Calculates whether num is a prime number,
    if it is return True otherwise return Flase'''

    try:
        int(num)
    except ValueError:
        raise ValueError("Ops. Input must be a positive integer")
    if int(num) != num or num < 0:
        raise ValueError("Ops. Input must be a positive integer")

    # SPECIALCASE: These are special cases that are not primes
    if num in [0, 1]: return False

    primes_to_remove = [2, 3, 5, 7, 11, 13, 17, 19, 23, 29]
    for prime in primes_to_remove:
        if num == prime: return True
        elif num % prime == 0: return False

    # Look for a factor
    limit = int(num**(0.5))+1
    for multiple in range(30, int(num**(0.5))+1, 30):
        for k in [1, 7, 11, 13, 19, 23, 29]:
            # If there is a factor
            if num % (multiple+k) == 0:
                # Its not a prime number
                return False
    return True

if __name__ == '__main__':
    print is_prime(600851)

Using some quick comparisons it seems that the 235 version is the fastest for solving larger primes while 23 and the naive are pretty similar in running times.

               naive   23    235    primes
600851475151 | 117ms   71ms  40ms   19ms

2. Probabilistic primality test

A short explanation can be found on Wikipedia.

  Probabilistic tests

  Probabilistic tests are more rigorous than heuristics in that they
  provide provable bounds on the probability of being fooled by a
  composite number. Many popular primality tests are probabilistic
  tests. These tests use, apart from the tested number \$n\$, some other
  numbers a which are chosen at random from some sample space; the usual
  randomized primality tests never report a prime number as composite,
  but it is possible for a composite number to be reported as prime. The
  probability of error can be reduced by repeating the test with several
  independently chosen values of a; for two commonly used tests, for any
  composite \$n\$ at least half the a's detect n's compositeness, so \$k\$
  repetitions reduce the error probability to at most \$2^{−k}\$, which can be
  made arbitrarily small by increasing \$k\$.

  The basic structure of randomized primality tests is as follows:

  Randomly pick a number \$a\$.
  Check some equality (corresponding to the chosen test) involving \$a\$ and the given number \$n\$. If the equality fails to hold true, then \$n\$
  is a composite number, a is known as a witness for the
  compositeness, and the test stops.
  Repeat from step 1 until the required accuracy is achieved.

  After one or more iterations, if \$n\$ is not found to be a composite
  number, then it can be declared probably prime.

The important bit is the three steps at the bottom. We put our number through some test and if it passes there is a \$1/2\$ chance of it being composite. After passing \$k\$ tests, the chances of being composite is \$2^{-k}\$. When big banks check for primality they often use a probabilistic prime test where \$k\$ is around \$80\$. The chances of a composite passing \$80\$ tests is just

$$
P_{80}(\text{composite}) = 2^{-80} = \frac{1}{1208925819614629174706176}
$$

So if you test \$1208925819614629174706176\$ numbers, on average one of them will be a composite passing all the tests. Below I will show a simple implementation of the Miller-Rabin primality test. It is one of the fastest and simplest ways to implement a primetest. This algorithm can be divided into three steps:

Generate a small list of primes below some limit (I used 1 million)
Check if the prime number is even, and some other small tests. Then check if it is in the primeset.
If the prime is bigger than the limit apply the Miller-Rabin primality test \$25\$ times. The number \$25\$ is arbitrary, however it does not need to be particularly big as the chances of a composite slipping through is \$2^{-k}\$.

The reason for step 1 is both because it is faster, but also because the Miller-Rabin test have a harder time detecting small primes. I imported the fastest prime_generator I could find through google. However if you allow for external libaries primesieve is faster. However generating primes is just a small part of the excecution time, and not worth optimizing further.
\${}{}\$

import numpy as np

def primesbelow(n):
    # https://stackoverflow.com/questions/2068372/fastest-way-to-list-all-primes-below-n-in-python/3035188#3035188
    """ Input n>=6, Returns a array of primes, 2 <= p < n """
    sieve = np.ones(n/3 + (n%6==2), dtype=np.bool)
    sieve[0] = False
    for i in xrange(int(n**0.5)/3+1):
        if sieve[i]:
            k=3*i+1|1
            sieve[      ((k*k)/3)      ::2*k] = False
            sieve[(k*k+4*k-2*k*(i&1))/3::2*k] = False
    return np.r_[2,3,((3*np.nonzero(sieve)[0]+1)|1)]

smallprimeset = set(primesbelow(100000))
_smallprimeset = 100000

def isprime(n, precision=25):
    # http://en.wikipedia.org/wiki/Miller-Rabin_primality_test#Algorithm_and_running_time
    try:
        n = abs(int(n))
    except ValueError:
        print("Input must be a non-negative integer")
    if n in [0, 1]:
        return False
    elif n in [2, 3, 5]:
        return True
    elif n % 2 == 0:
        return False
    elif n < _smallprimeset:
        return n in smallprimeset

# Miller-Rabin primality test

d = n - 1
s = 0
while d % 2 == 0:
    d //= 2
    s += 1

for repeat in range(precision):
    a = random.randrange(2, n - 2)
    x = pow(a, d, n)

    if x == 1 or x == n - 1: continue

    for r in range(s - 1):
        x = pow(x, 2, n)
        if x == 1: return False
        if x == n - 1: break
    else: return False
return True

3. Comparison

I used

pip install primefac

To be able to use the isprime function from the primefac package. Then I did some simple primality testing on a few semi-large numbers.

All the tests where done a \$100\$ times and then i took the average. I did a \$1000\$ averages for Miller-Rabin and primefac for better accuracy. There is no need to test composite numbers, because all the implementations stop once a prime has been found. All the tests are therefore done on primes.

    number              naive             23       235    primegen     miller    primefac
    -----------------------------------------------------------------------------------------
             60085159     0.69 ms    1.09 ms   0.47 ms     1.02 ms    0.25 ms    0.24 ms
           6008514763       22 ms      61 ms      7 ms      3.8 ms   0.104 ms    0.070 ms
          60085147517       56 ms      30 ms     12 ms      6.5 ms   0.072 ms    0.090 ms
         600851475151      283 ms     281 ms    155 ms     56 ms     0.364 ms    0.331ms
    99194853094755497   117328 ms  113053 ms  58010 ms  19117 ms     0.638 ms    0.239180 ms
    -----------------------------------------------------------------------------------------

As you can see you do fairly well for small values. For \$60085159\$ your code is about \$2.75\$ times as slow as the Miller-Rabin. However as the values increase so does the difference. For \$600851475151\$ your code is about \$777\$ times slower. Finally for \$99194853094755497\$ the Miller-Rabin is \$183899\$ times faster.

There might be something wrong with my testing since the 23-version is about as fast as the naive implementation (yours). However the 235 is a simple speed increase. If you want a drastic improvement however you need to switch to another implementation. Either use a library which has a builtin isprime function, or use a probabilistic approach: Fermat primality test, Miller-Rabin, Solovay-Strassen primality test, Frobenius primality are just some of the many fast implementations.


My python hook for mercurial In our hg workflow we use default as testing branch and stable as the one that contains stable code. All changes are made in feature-branches that are started from latest stable. This hook checks if there is a "direct" commit to default or stable (they are denied, only merges may change contents of default and stable) or that new feature branch has been started from default (which is denied by conventions too).

Any proposal to make it more "pythonic" (or just better)?

from mercurial import context

def check(ui, repo, hooktype, node=None, source=None, **kwargs):
    for rev in xrange(repo[node].rev(), len(repo)):
        ctx = context.changectx(repo, rev)
        parents = ctx.parents()

        if len(parents) == 1:
            if ctx.branch() in ['default', 'stable']:
                ui.warn('!!! You cannot commit directly to %s at %s !!!' % (ctx.branch(), ctx.hex()))
                return True

            if parents[0].branch() == 'default':
                ui.warn('!!! You cannot start your private branch from default at %s !!!' % (ctx.hex()))
                return True

    return False

for rev in xrange(repo[node].rev(), len(repo)):
        ctx = context.changectx(repo, rev)

In Python, I generally try to avoid iterating using xrange. I prefer to iterate over what I'm interested in.

def revisions(repo, start, end):
    for revision_number in xrange(start, end):
        yield context.changectx(repo, revision_number)

for rev in revisions(repo, repo[node].rev(), len(repo)):
    ...

Although I'm not sure its worthwhile in this case.

The only other issue is your use of abbreviations like repo and rev. They aren't that bad because its pretty clear from the context what they stand for. But I'd write them fully out.


Largest prime factor for a given number I created a program to find the largest prime factor for a given number. The program works but unfortunately takes very long time to compute a huge number like 600851475143.

How can I optimise this program to run faster?

I don't know what does this multicore thing is, but I think it is a possible way to improve this program.

// A program to find the largest prime factor for a given number

#include <iostream>
#include <math.h>

using namespace std;

bool IsPrime (int n) {
   if (n < 2)      return false;
   if (n < 4)      return true;
   if (n % 2 == 0) return false;

   for (int i = 3; i <= int(sqrt(n)) + 1; i += 2)
       if (n % i == 0)
               return false;
   return true;
}

int main () {

    long long x;
    int lpf = 0; //Largest Prime Factor

    cin >> x;

    for (int n = 0; n <= x; n++)
        if (IsPrime(n) && x % n == 0)
            lpf = n;

    cout << lpf;
}

You have done the optimization of removing multiples of 2 (by incrementing by 2).

There is another slight improvement that removes multiples of 2 and 3 from the test.

bool IsPrime (int n) {
   if (n == 2)     return true;
   if (n == 3)     return true;
   if (n < 5)      return false;
   if (n % 2 == 0) return false;
   if (n % 3 == 0) return false;

   int inc = 4;
   int dec = -2;
   for (int i = 5; i <= int(sqrt(n)) + 1; i += inc)
   {
       if (n % i == 0)
               return false;
       inc = inc + dec;  // makes inc go 2/4/2/4/2/4/.....
       dec = dec * -1;
   }
   return true;
}

Another optimization for IsPrime() is that you only need to test using primes. ie no pointing in testing with 6 (its not prime) as all values that are divisible by 6 are also divisible by 3 (is prime). So if your loop only tested by using other primes:

// Assumes you are calling this function with monotimically inreassing
// values of n so that the `foundPrimes` vector will be constructed in order.
bool IsPrime (int n) {

   static std::vector<int>   foundPrimes = { 2, 3, 5, 7, 11 };

   for(std::size_t loop = 0; loop <  foundPrimes.size(); ++loop)
   {
       if (n == foundPrimes[loop])     { return true;}
       if (n % foundPrimes[loop] == 0) { return false;}
   }
   start = foundPrimes.last() + 2;

   for (int i = start; i <= int(sqrt(n)) + 1; i += 2)
   {
       if (n % i == 0)
               return false;
   }
   foundPrimes.push_back(n);
   return true;
}

The next optimization is called the Sieve of Eratosthenes. This needs a minor re-write but is very efficient. You would use the sieve to calculate all the primes you need first. Then just look up the values be checking if they are in the sieve.


Oriented-object approach on how to control different classes cleanly I'm currently creating a client/server application (using Java) and I want to make it fully controllable by an input stream (currently the console). At the moment only one class have to be controlled, but I want my code to stay easy to develop.

I found a solution but I think I can do better, so that's why I post this message: I want your advices on my method now and how I could improve it or how should it better be.

Here's my solution:

I created an interface CommandControllable like:

import java.util.ArrayList;

public interface CommandControllable
{
    public void execute(Command command);
    public void addCommandManager(CommandManager commandManager);
    public ArrayList<CommandManager> getCommandManagers();
}

A class Command like

public class Command
{
    protected String request;

    public Command(String request)
    {
        this.request = request;
    }

    public String ToString()
    {
        return this.request;
    }
}

A class CommandManager

import java.io.InputStream;
import java.util.Scanner;

public class CommandManager implements Runnable
{
    protected Scanner reader;
    protected CommandControllable toControl;
    protected boolean running = false;

    public CommandManager(InputStream stream)
    {
        this.reader = new Scanner(stream);
    }

    @Override
    public void run()
    {
        this.running = true;
        while(this.running)
            {
                if(this.reader.hasNextLine())
                    {
                        Command c = new Command(this.reader.nextLine());
                        this.toControl.execute(c);
                    }
            }
    }

    public void setControllable(CommandControllable toControl)
    {
        this.toControl = toControl;
    }

    public void stop()
    {
        this.running = false;
    }
}

If I want a class to be controllable I write something like:

public class Server implements CommandControllable
{
    protected ArrayList<CommandManager> commandManagers = new ArrayList<CommandManager>();

    public Server()
    {

    }

    public void stop()
    {
        System.out.println("Server stopped");
    }

    @Override
    public synchronized void execute(Command command)
    {
        switch(command.ToString())
        {
            case "quit":
                stop();
                break;
            default:
                System.out.println("Unrecognized command");
                break;
        }
    }

    @Override
    public void addCommandManager(CommandManager commandManager)
    {
        commandManager.setControllable(this);
        this.commandManagers.add(commandManager);
    }

    @Override
    public ArrayList<CommandManager> getCommandManagers()
    {
        return this.commandManagers;
    }
}

And

public static void main(String[] args)
{
    Server s = new Server();
    CommandManager manager = new CommandManager(System.in);
    s.addCommandManager(manager)

    Thread t_manager = new Thread(manager);
    t_manager.start();
}

Any advices on the current solution and how to improve it?

CommandManager referes to a CommandControllable, and CommandControllable refers to a set of CommandManagers. Who controls who? Such loops in dependency graph should be avoided.

Then, think if the Actor model can be applied in your case.


Uploading multiple images using PHP I literally spend all day working on PHP and I think I figured it out, but I need some reviews in order to make my code secure & okay. I am asking you about functionality here. The code below works.

index.php

<form action="upload.php" method="post" enctype="multipart/form-data">

        <input name="images[]" multiple type="file" id="file[]"/>

        <input type="submit">

</form>

upload.php

$target = "upload/";

$uploadit = 1; // I will change it with extension checker.
$checksize = // will check file size

foreach($_FILES['images']['name'] as $key => $value){
    $path = $_FILES['images']['name'][$key];
    $ext = pathinfo($path, PATHINFO_EXTENSION);

    $name = md5($name);
    $generate1 = md5(date('Y-m-d H:i:s:u'));
    $randomizer = uniqid($name);
    $name = $name . $generate1 . $randomizer;
        if ($uploadit == 1) {
               if (move_uploaded_file($_FILES['images']['tmp_name'][$key], $target.$name.".".$ext)){
                echo "".$value ."Your files have been upload successfully
\n";
            }
        } else {
            echo "Failed";
        }

If $target is accessible from a web browser, you need to verify that it does not have a dangerous extension.  For example, that it's not a PHP file.  Note that there are probably other dangerous extensions on your web server other than just '.php'.  For example, '.html' is a dangerous extension because of cross-site scripting type vulnerabilities.

You include a flag variable for this, but you aren't doing the checking.

$uploadit = 1; // I will change it with extension checker.

Why not use a Boolean here?

$uploadit = false; // I will change it with extension checker.

Also, it probably makes more sense to default to no and change to yes than to default to yes.

foreach($_FILES['images']['name'] as $key => $value){
    $path = $_FILES['images']['name'][$key];
    $ext = pathinfo($path, PATHINFO_EXTENSION);

Could just be

foreach ( $_FILES['images']['name'] as $key => $path ) {
    $ext = pathinfo($path, PATHINFO_EXTENSION);

In general, prefer more descriptive names than $key => $value.  In this case, I believe that $key is a numeric index, so $i or $index might be more appropriate.

$name = md5($name);

What's $name?

    if ($uploadit == 1) {

Given PHP's quirkiness, this should probably be

    if ( true === $uploadit ) {

According to the PHP date documentation,

$generate1 = md5(date('Y-m-d H:i:s:u'));

The date() function will always return '000000' for microseconds (u).

$date = new DateTime();
$generate1 = md5($date->format('Y-m-d H:i:s:u'));

Use DateTime::format() instead.

$randomizer = uniqid($name);

This does nothing for security.  Its purpose is to avoid clashes.  Note that this will put $name in the name twice.

$randomizer = $name . uniqid();

This does the same thing as the previous statement.

$name = $name . $generate1 . $randomizer;

You can write

$name .= $generate1 . $randomizer;

Not important now, but the next time you hit this situation, you might want to go with the shorter version.

Note that it is safer to put $target outside the web accessible area.  If you do that, then it is less of a concern about the extension.  If you do not do that, then you should care about the extension even if the filename is hard to guess.  It's not clear to me why you need it in a web accessible area if you are going to obfuscate the name.

Obfuscating the name does not provide security.  There have been exploits that have allowed attackers to view directory contents.  Also, there are any number of existing users that can do that.  Finally, you can't use the image without sharing the obfuscated name--possibly with an attacker.  So you might as well use the original name and secure that.

If you are trying to avoid name clashes, you might be better off with a database solution.  An autoincrement identifier with a unique index will be consistently unique and kept that way by the database.

It is preferable to whitelist extensions rather than blacklist them.  That way a change in the web server configuration can't open up a new dangerous extension when you aren't looking.

There's things you can do in either .htaccess or httpd configuration to prevent files in $target being evaluated in scripts.  For example, disable CGI.  You should probably do something like this in addition to an extension whitelist.