Concise way of creating a IntRange out of an Int I want to create a collection (in this case a map due to the extended function 'groupBy') that maps an IntRange to a list of a data class, which members are determined whether an int inside the data class is within the IntRange.
The IntRanges are 0..9, 10..19, 20..29, etc.

Concrete example:

data class Trip(
    val (...)
    val distance: Double
)

[...]

val mappedTrips = trips.groupBy {
    ((it.duration-(it.duration)%10))..((it.duration-(it.duration)%10)+9) }

Explicitly the

((it.duration-(it.duration)%10))..((it.duration-(it.duration)%10)+9)

part seems to be not very concise, hard to read and therefore I was wondering if there is a way to make it more efficient, meaning more easy to read, concise.

Thank you in advance.

Edit:

TaxiPark Class

package taxipark

data class TaxiPark(
        val allDrivers: Set<Driver>,
        val allPassengers: Set<Passenger>,
        val trips: List<Trip>)

data class Driver(val name: String)
data class Passenger(val name: String)

data class Trip(
        val driver: Driver,
        val passengers: Set<Passenger>,
        // the trip duration in minutes
        val duration: Int,
        // the trip distance in km
        val distance: Double,
        // the percentage of discount (in 0.0..1.0 if not null)
        val discount: Double? = null
) {
    // the total cost of the trip
    val cost: Double
        get() = (1 - (discount ?: 0.0)) * (duration + distance)
}

Task

Find the most frequent trip duration among minute periods 0..9, 10..19, 20..29, and so on.
Return any period if many are the most frequent, return null if there're no trips.

Test sample

class TestTask5TheMostFrequentTripDurationPeriod {

    private fun testDurationPeriod(expected: Set<IntRange?>, tp: TaxiPark) {
        val actual = tp.findTheMostFrequentTripDurationPeriod()
        val message = "Wrong result for 'findTheMostFrequentTripDurationPeriod()': $actual."
        if (expected.size <= 1) {
            Assert.assertEquals(
                    message + tp.display(),
                    expected.firstOrNull(), actual)
        } else {
            Assert.assertTrue(message +
                    tp.display() +
                    "\nPossible results: $expected" +
                    "\nActual: $actual\n",
                    actual?.let { it in expected } ?: expected.isEmpty())
        }
    }

    @Test
    fun test00() = testDurationPeriod(setOf(null), taxiPark(0..1, 0..10))

    @Test
    fun test01() {
        // The period 30..39 is the most frequent since there are two trips (duration 30 and 35)
        testDurationPeriod(setOf(30..39), taxiPark(1..3, 1..5,
                trip(1, 1, duration = 10),
                trip(3, 4, duration = 30),
                trip(1, 2, duration = 20),
                trip(2, 3, duration = 35)))
    }

    @Test
    fun test02() = testDurationPeriod(setOf(30..39), taxiPark(0..5, 0..9,
            trip(0, listOf(2, 9), duration = 14, distance = 25.0),
            trip(1, listOf(8), duration = 39, distance = 37.0, discount = 0.2),
            trip(5, listOf(0, 5), duration = 27, distance = 28.0, discount = 0.3),
            trip(4, listOf(0, 6), duration = 33, distance = 14.0),
            trip(2, listOf(5, 1, 4, 3), duration = 2, distance = 15.0),
            trip(4, listOf(7), duration = 27, distance = 2.0),
            trip(4, listOf(4, 6), duration = 31, distance = 31.0),
            trip(3, listOf(9, 0), duration = 34, distance = 7.0),
            trip(5, listOf(3), duration = 25, distance = 33.0),
            trip(1, listOf(0, 7, 2, 3), duration = 13, distance = 17.0))
    )
}

Since it.duration is quite long, you could use the alternative expression:

(it.duration / 10 * 10) .. (it.duration / 10 * 10 + 9)

You still have common subexpressions, though. To avoid these, you can define a cluster:

fun cluster(x: Double, size: Double) = (x - x % size) .. (x - x % size + size - 1)

or, to avoid the common subexpression:

fun cluster(x: Double, size: Double) = (x - x % size).let { it .. (it + size - 1) }

(I wrote the above without a compiler, so it may or may not work, but you get the idea.)

Then, you can group the trips by their cluster:

trips.groupBy { cluster(it.duration, 10.0) }

By the way, the group "30-39" also contains 39.999, so you might get contacted by a nitpicker that your range should better be called "[30,40)". ;)


Model classes for a quiz app in Django I am creating simple quiz app in Django. It works, but I think that way I've done it is not the best. I have Exam detail view, which display questions related to that exam and answers for these questions. What are your suggests about it? I pasted only models, because I think that this is the problem here.

from django.db import models

class Answer(models.Model):
    """
    Answer's Model, which is used as the answer in Question Model
    """
    text = models.CharField(max_length=128, verbose_name=u'Answer\'s text')
    is_valid = models.BooleanField(default=False)

    def __str__(self):
        return self.text

class Question(models.Model):
    question_text = models.CharField(max_length=256, verbose_name=u'Question\'s text')
    answers = models.ManyToManyField(Answer)
    is_published = models.BooleanField(default=False)

    def __str__(self):
        return "{content} - {published}".format(content=self.question_text, published=self.is_published)

class Exam(models.Model):
    """
    Exam's model, works as a wrapper for the questions
    """
    name = models.CharField(max_length=64, verbose_name=u'Exam name', )
    slug = models.SlugField()
    questions = models.ManyToManyField(Question)

    def __str__(self):
        return self.name

As mentioned in the comments, it would be useful if you included more than just the models here. As a result, I can only really do a design review of what appears to be a fairly generic quiz application.

Right now your models are set up this way

So exams can share questions, and questions can share answers. This is useful in cases where exams typically share questions, and questions share common answers, but it can (and often will) cause issues down the road. I'll get to those later.

Typically the next step is to build out the models for making selections on an exam. You might consider a adding a new many-to-many relationship on the exam that holds the selections (assuming a single exam per person). Here's a rough diagram of what that would look like, where the "selection" is the many-to-many relationship. This can either be implemented as a single property, or as a separate model with two foreign keys.

But then there is a problem: How do you handle questions/answers duplicated on exams? There's no easy way to determine what selection tied to which question, because the answer that was selected is shared across multiple questions in the same exam. This means that you can't properly associate selections with questions, and you can't share answers across an exam, because there are multiple ways traverse the tree.

My recommendation would be to remove the flexibility of sharing questions and answers in favor of a more rigid, single-directional tree structure.

So a question has a defined set of answers, and those answers are unique to the question and cannot be shared. And those questions are specific to the exam that are specified for, and cannot appear across multiple exams. This means that a selection (the "glue" between the answer and the exam) cannot be duplicated across a single exam, so there will always be a clear path from an exam to an answer, and the reverse.

So the revised models would look like

from django.db import models

class Exam(models.Model):
    """
    Exam's model, works as a wrapper for the questions
    """
    name = models.CharField(max_length=64, verbose_name=u'Exam name', )
    slug = models.SlugField()

    def __str__(self):
        return self.name

class Question(models.Model):
    question_text = models.CharField(max_length=256, verbose_name=u'Question\'s text')
    is_published = models.BooleanField(default=False)
    exam = models.ForeignKey(Exam, related_name='questions')

    def __str__(self):
        return "{content} - {published}".format(content=self.question_text, published=self.is_published)

class Answer(models.Model):
    """
    Answer's Model, which is used as the answer in Question Model
    """
    text = models.CharField(max_length=128, verbose_name=u'Answer\'s text')
    is_valid = models.BooleanField(default=False)
    question = models.ForeignKey(Question, related_name='answers')

    def __str__(self):
        return self.text

All diagrams were created using nomnoml.


RandomCount class: Randomising numbers in an array and generating them in a random order public class SimpleRandomCount extends RandomCount
{
 /**
 * Generate an array containing elements in a random order
 *
 * @param size the size of the array to be generated
 */
public SimpleRandomCount(int size) {
super(size);
}

/**
* Randomise the array
*/
protected void randomise() {
int[] copy = new int[array().length];
int randomIndex;

// used to indicate if elements have been used
boolean[] used = new boolean[array().length];
Arrays.fill(used,false);
for (int index = 0; index < array().length; index++) {
    do {
        randomIndex = randomIndex();
    } while (used[randomIndex]);

    copy[index] = array()[randomIndex];
    used[randomIndex] = true;
}

for (int index = 0; index < array().length; index++) {
    array()[index] = copy[index];
   }
 }
      public static void main(String [] args){
      RandomCount count = new SimpleRandomCount(5);
      System.out.println("Array is " + Arrays.toString(count.array()));
     }
}

The SimpleRandomCount class is part of another class called RandomCount. I was wondering if there was a cleverer, shorter way of randomising the numbers without using a boolean value to help do it?

Thanks for any help it is very much appreciated.

Here's a simple algorithm lifted directly from the Collections source code. It is modified to shuffle an int[] in-place. That is, the array passed in will be modified. You probably want to give clients the opportunity to specify their own Random instance in a constructor. If you keep concrete inheritance, it might also make sense for the Random to be stored in RandomCount.

public final class SimpleRandomCount extends RandomCount {
    private final Random random = new Random();

    public SimpleRandomCount(int size) {
        super(size);
    }

    protected void randomise() {
        this.shuffleInPlace(array(), this.random);
    }

    private void shuffleInPlace(final int[] values, final Random random) {
        for (int i = values.length; i > 1; i--) {
            final int swapIndex = random.nextInt(i);
            final int temp = values[i - 1];
            values[i - 1] = values[swapIndex];
            values[swapIndex] = temp;
        }
    }

    public static void main(String [] args){
        RandomCount count = new SimpleRandomCount(5);
        System.out.println("Array is " + Arrays.toString(count.array()));
    }
}

Unrelatedly, it's probably a Bad Idea to expose your raw array in a public method like array(). If you must use extension instead of a RandomCount interface, either make the array member variable protected or provide a protected direct accessor. Clients using the public accessor should only have access to a copy of the array. Otherwise they're going to do Bad Things to it, and that will affect the internals of your class.


Manage Case Statement with (Int, long, short, double.....) I have the following case statements that has to be modified to fit these cases:
Dec, Double, Int, Long, Short, Date, and String.

Please let me know if there is an easier way to handle this rather than repeating the following (slightly modified for each case):

Case GetType(Integer)
    If String.IsNullOrEmpty(c.Text) Then
        list.Add(CInt(0))
    Else
        Try
            list.Add(CInt(c.Text))
        Catch ex As Exception
            WriteAudit(ex, True)
            Dim intValue As Integer
            If Not Integer.TryParse(c.Text, intValue) Then
                c.Text = c.Text.Substring(0, c.Text.Length - 1)
                MessageBox.Show("Please Enter Only Integer Values Here")
            End If
        End Try
    End If

For value types (i.e. all your types except String) you can use the Convert.ChangeType method:

list.Add(Convert.ChangeType(c.Text, theType))


Earliest time frog can jump to the other side of a river in C# I'm new to C#, I programmed a little in Java in the past. I had the two previous exercises reviewed here (permCheck, cyclicRotation, with a 100% score, just like this one), and I'm applying what the accepted answers contributed to me.
Even though the site expects names like public int solution(int X, int[] A), I refactored the code after it was accepted, and applied those contributions I mentioned.
Is this how professional code should look like in terms of quality, or are there still things to improve in solutions of easy exercises like this written by me?

Task description

A small frog wants to get to the other side of a river. The frog is
initially located on one bank of the river (position 0) and wants to
get to the opposite bank (position X+1). Leaves fall from a tree onto
the surface of the river.
You are given an array A consisting of N integers representing the
falling leaves. A[K] represents the position where one leaf falls at
time K, measured in seconds.
The goal is to find the earliest time when the frog can jump to the
other side of the river. The frog can cross only when leaves appear at
every position across the river from 1 to X (that is, we want to find
the earliest moment when all the positions from 1 to X are covered by
leaves). You may assume that the speed of the current in the river is
negligibly small, i.e. the leaves do not change their positions once
they fall in the river.
For example, you are given integer X = 5 and array A such that:
A[0] = 1
A[1] = 3
A[2] = 1
A[3] = 4
A[4] = 2
A[5] = 3
A[6] = 5
A[7] = 4
In second 6, a leaf falls into position 5. This is
the earliest time when leaves appear in every position across the
river.
Write a function:
class Solution { public int solution(int X, int[] A); }
that, given a non-empty array A consisting of N integers and integer
X, returns the earliest time when the frog can jump to the other side
of the river.
If the frog is never able to jump to the other side of the river, the
function should return −1.
For example, given X = 5 and array A such that:
A[0] = 1
A[1] = 3
A[2] = 1
A[3] = 4
A[4] = 2
A[5] = 3
A[6] = 5
A[7] = 4
the function should return 6, as explained above.
Write an efficient algorithm for the following assumptions:
N and X are integers within the range [1..100,000]; each element of
array A is an integer within the range [1..X].

/// <summary>
/// Check if a given array contains the integers 1..N
/// </summary>
/// <returns>
/// -1 if <paramref name="fallenLeaves"/> does not contain all integers
///     1..N, where N = <paramref name="requiredAmountOfLeaves"/>
/// a possitive int i, if the required amount of ints were present at
///     <paramref name="fallenLeaves"/>
/// </returns>

const int FROG_CANT_JUMP_TO_THE_OTHER_SIDE = -1;

public static int GetSecondsRequired
    (int requiredAmountOfLeaves, int[] fallenLeaves)
{
    bool[] leavesAsSteps = new bool[requiredAmountOfLeaves + 1];
    int espectedSum = 0, correctSum = 0;
    for (int i = 1; i <= fallenLeaves.Length; i++)
    {
        if (i <= requiredAmountOfLeaves)
        //get summatory of 1..N
            correctSum += i;
        if (fallenLeaves[i - 1] <= requiredAmountOfLeaves &&
            !leavesAsSteps[fallenLeaves[i - 1]])
        {
        //accumulate where the expected leaf fell and set its location to true
            espectedSum += fallenLeaves[i - 1];
            leavesAsSteps[fallenLeaves[i - 1]] = true;
        }
        if (espectedSum == correctSum && i >= requiredAmountOfLeaves)
        //if all the espected leaves fell, then return the array's
        //index where the last expected leaf was found
            return i - 1;
    }
    return FROG_CANT_JUMP_TO_THE_OTHER_SIDE;
}

Readability

The code needs more spacing. Considering the comments, the indentation and the ifs, it's hard to read without some good old empty lines.

Use brackets when using conditions, especially if you have comments above your single line, it gets really confusing. Also, the readability is increased and, the biggest factor, you'll avoid weird bugs.

If you really don't want to use brackets, at least indent your comments so it's clear the line under it is still into the if.

Considering that we have your explanation of the problem, we can understand pretty clearly what your variable names mean. But still... it could be improved or at least well documented. fallenLeaves doesn't represent fallen leaves, it represent when leaves are falling at which position. expectedSum and correctSum don't mean much, to a point I'm wondering if they are well initialized.

Result for now :

public static int GetSecondsRequired(int requiredAmountOfLeaves, int[] fallenLeaves)
{
    bool[] leavesAsSteps = new bool[requiredAmountOfLeaves + 1];
    int espectedSum = 0, correctSum = 0;

    for (int i = 1; i <= fallenLeaves.Length; i++)
    {
        if (i <= requiredAmountOfLeaves)
        {
            //get summatory of 1..N
            correctSum += i;
        }

        if (fallenLeaves[i - 1] <= requiredAmountOfLeaves &&
            !leavesAsSteps[fallenLeaves[i - 1]])
        {
            //accumulate where the expected leaf fell and set its location to true
            espectedSum += fallenLeaves[i - 1];
            leavesAsSteps[fallenLeaves[i - 1]] = true;
        }

        if (espectedSum == correctSum && i >= requiredAmountOfLeaves)
        {
            //if all the espected leaves fell, then return the array's
            //index where the last expected leaf was found
            return i - 1;
        }
    }

    return FROG_CANT_JUMP_TO_THE_OTHER_SIDE;
}

Algorithm

You use i - 1 everywhere but one place, it would be smarter to reverse this. Use i everywhere and i + 1 at one place.
You use the accessor fallenLeaves[i] often, you should consider storing it in a variable. It will make the code more readable. So : int currentFallenLeaf = fallenLeaves[i];
The usage of expectedSum and correctSum is too complicated for the problem at hand. You don't need to check the sum, only that the number of true elements in leavesAsSteps (minus one because of the zero index) equals requiredAmountOfLeaves.
You could check right at the beginning of the loop if you already marked the "leaf step" as true and that the leaf is valid, so that you don't do useless work.

public static int GetSecondsRequired(int requiredAmountOfLeaves, int[] fallenLeaves)
{
    // You should comment why there's a + 1 here.
    bool[] leavesAsSteps = new bool[requiredAmountOfLeaves + 1];
    int numberOfFallenLeaves = 0;

    for (int i = 0; i < fallenLeaves.Length; i++)
    {
        int currentFallenLeaf = fallenLeaves[i];

        // Have we already checked this number?
        // Is the leaf number out of range?
        // If so, let's just stop right there for this leaf.
        if (currentFallenLeaf > requiredAmountOfLeaves
            || leavesAsSteps[currentFallenLeaf])
        {
            continue;
        }

        numberOfFallenLeaves++;
        leavesAsSteps[currentFallenLeaf] = true;

        // Have we marked all our leaves? We're done.
        if (numberOfFallenLeaves == requiredAmountOfLeaves)
        {
            return i;
        }
    }

    return FROG_CANT_JUMP_TO_THE_OTHER_SIDE;
}


Printing out JSON data from Twitter as a CSV I'm extremely new to Python, and the Twitter API, but I found an example online that walked me through the process. Now that I've been playing around with it for awhile, I've begun to push the limits of what my laptop can handle in terms of processing power with my current code. I was hoping someone here could check over it and make recommendations on optimizing it.

Goal: Take JSON output from the Twitter streaming API and print out specific fields to a CSV file. (The sysarg is used to pass the input and output filenames).

# import necessary modules
import json
import sys

# define a new variable for tweets
tweets=[]

# import tweets from JSON
for line in open(sys.argv[1]):
    try:
        tweets.append(json.loads(line))
    except:
        pass

# print the name of the file and number of tweets imported
print "File Imported:", str(sys.argv[1])
print "# Tweets Imported:", len(tweets)

# create a new variable for a single tweets
tweet=tweets[0]

# pull out various data from the tweets
tweet_id = [tweet['id'] for tweet in tweets]
tweet_text = [tweet['text'] for tweet in tweets]
tweet_time = [tweet['created_at'] for tweet in tweets]
tweet_author = [tweet['user']['screen_name'] for tweet in tweets]
tweet_author_id = [tweet['user']['id_str'] for tweet in tweets]
tweet_language = [tweet['lang'] for tweet in tweets]
tweet_geo = [tweet['geo'] for tweet in tweets]

#outputting to CSV
out = open(sys.argv[2], 'w')
print >> out, 'tweet_id, tweet_time, tweet_author, tweet_author_id,    tweet_language, tweet_geo, tweet_text'

rows = zip(tweet_id, tweet_time, tweet_author, tweet_author_id,    tweet_language, tweet_geo, tweet_text)

from csv import writer
csv = writer(out)

for row in rows:
    values = [(value.encode('utf8') if hasattr(value, 'encode') else value) for value in row]
    csv.writerow(values)

out.close()

# print name of exported file
print "File Exported:", str(sys.argv[2])

Put all import statements at the top.
You might leak a file descriptor, since you call open(sys.argv[1]) without closing it.  (Whether a leak actually occurs depends on the garbage collector of your Python implementation.)  Best practice is to use a with block, which automatically closes the resources when it terminates.

with open(sys.argv[1]) as in_file, \
     open(sys.argv[2]) as out_file:
    …

It would be better to define your variables in the same order as they will appear in the CSV output.
Rather than creating an empty array and appending to it in a loop, use a list comprehension.

tweets = [json.loads(line) for line in in_file]

You read all the tweets into an array of JSON objects, then slice the data "vertically" by attribute, then re-aggregate the data "horizontally".  That's inefficient in terms of memory usage as well as cache locality.
Unless you have a good reason, just transform one line of input at a time.  (A good reason might be that you want to produce no output file at all if an error occurs while processing any line.)

import json
import sys
from csv import writer

with open(sys.argv[1]) as in_file, \
     open(sys.argv[2], 'w') as out_file:
    print >> out_file, 'tweet_id, tweet_time, tweet_author, tweet_author_id,    tweet_language, tweet_geo, tweet_text'
    csv = writer(out_file)
    tweet_count = 0

    for line in in_file:
        tweet_count += 1
        tweet = json.loads(line)

        # Pull out various data from the tweets
        row = (
            tweet['id'],                    # tweet_id
            tweet['created_at'],            # tweet_time
            tweet['user']['screen_name'],   # tweet_author
            tweet['user']['id_str'],        # tweet_authod_id
            tweet['lang'],                  # tweet_language
            tweet['geo'],                   # tweet_geo
            tweet['text']                   # tweet_text
        )
        values = [(value.encode('utf8') if hasattr(value, 'encode') else value) for value in row]
        csv.writerow(values)

# print the name of the file and number of tweets imported
print "File Imported:", str(sys.argv[1])
print "# Tweets Imported:", tweet_count
print "File Exported:", str(sys.argv[2])


Classic Hundred Doors Simulation I am doing the 100 Doors problem as an exercise to practice user input and loops. I do understand the problem can be solved by outputting every square number up to the maximum number, but it defeats the purpose of the exercise. The 100 Doors problem goes like this:

  \$N\$ doors are closed. In the first pass, I open all of them. In the second pass, I toggle every second door. In the third pass, I toggle every third door. I continue this until I have completed the \$N\$th pass. Find all the doors that will remain open after \$N\$ passes.

Here is the code:

import java.io.IOException;
import java.util.Scanner;

public class hundredDoors {
    static boolean hasNum = false;

    public static void main(String[] args) throws IOException {
        while (!hasNum) {
            Scanner input = new Scanner(System.in);
            int numOfDoors = 0;

            System.out.print("Please enter the number of doors: ");
            if (input.hasNextInt()) {
                hasNum = true;
                numOfDoors = input.nextInt();
                if (numOfDoors < 1) {
                    hasNum = false;
                    System.out.println("Please enter an integer.");
                    continue;
                }
                boolean[] doors = new boolean[numOfDoors];

                for (boolean door : doors) {
                    door = false;
                }

                for (int index = 1; index <= numOfDoors; index++) {
                    for (int door = index - 1; door <= numOfDoors - 1; door += index) {
                        doors[door] = !doors[door];
                    }
                }

                String output = "Doors still opened: \n";
                int loops = 0;
                for (int door = 0; door <= numOfDoors - 1; door++) {
                    if (loops == 10) {
                        output += "\n";
                        loops = 0;
                    }
                    if (doors[door]) {
                        output += (door + 1 + " ");
                        loops += 1;
                    }
                }
                input.close();
                System.out.println(output);
            }

            else {
                System.out.println("Please enter an integer.");
            }
        }
    }
}

One note for this program is that \$N\$ is supplied by the user. The other is that for formatting sakes, each line will have a maximum of 10 numbers (# of door).

My question here isn't how to make it more efficient (the most efficient method is to output every square since they have an odd number of factors as explained here) but rather if the code can be more clean/clear (ie, names of indentifiers, loops can be shortened, input validation check can be better, etc).

The program is difficult to read because all code is in the main()
function. As an example, one has to jump to the end of the program in
order to understand how the “get a positive integer from the user” loop
works.

It is generally better to separate the I/O from the computation, and
have dedicated functions for a single purpose. In your example, the overall
structure could for example look like this:

static int readPositiveInteger(String prompt) throws IOException {
    // ...
}

static boolean[] openDoors(int numOfDoors) {
    // ...
}

static void printOpenDoors(boolean[] doors) {
    // ...
}

public static void main(String[] args) throws IOException {

    int numOfDoors = readPositiveInteger("Enter the number of doors: ");
    boolean[] doors = openDoors(numOfDoors);
    printOpenDoors(doors);
}

More remarks:

The naming convention for Java classes is upper camelcase, in your case
HundredDoors.
The boolean hasNum should be a local variable in main().
boolean variables are by default initialized to false, the explicit
initialization in a loop is not needed.
You create a new Scanner instance for each attempt to read a positive
integer. A single instance is sufficient if you read lines from standard
input with nextLine().
The message “Please enter an integer” should be “Please enter a positive
integer" since that is what you need and check for.
Array indices start at zero whereas the door numbers start at one.
If you allocate one additional boolean element for the doors array then
the door number can be used as array index directly, instead of adding/subtracting
one at various places. That makes the code better understandable.
The output can be written directly to standard output instead of
concatenating it to a string first.

Putting it all together, the program could look like this:

import java.io.IOException;
import java.util.Scanner;

public class HundredDoors {

    static int readPositiveInteger(String prompt) throws IOException {
        Scanner scanner = new Scanner(System.in);
        int i = 0;
        do {
            System.out.print(prompt);
            try {
                i = Integer.parseInt(scanner.nextLine());
            } catch (NumberFormatException e){
                i = 0;
            }
            if (i <= 0) {
                System.out.println("Please enter a positive integer!");
            }
        } while (i <= 0);
        return i;
    }

    static boolean[] openDoors(int numOfDoors) {
        boolean[] doors = new boolean[numOfDoors + 1];
        for (int index = 1; index <= numOfDoors; index++) {
            for (int door = index; door <= numOfDoors; door += index) {
                doors[door] = !doors[door];
            }
        }
        return doors;
    }

    static void printOpenDoors(boolean[] doors) {
        System.out.println("Doors still opened:");
        int count = 0;
        for (int door = 1; door < doors.length; door++) {
            if (doors[door]) {
                if (count == 10) {
                    System.out.println();
                    count = 0;
                }
                count += 1;
                System.out.print(door + " ");
            }
        }
        System.out.println();
    }

    public static void main(String[] args) throws IOException {

        int numOfDoors = readPositiveInteger("Please enter the number of doors: ");
        boolean[] doors = openDoors(numOfDoors);
        printOpenDoors(doors);
    }
}


Script to unpack archives of various kinds Just looking for some feedback and constructive criticism. This is the second script I ever made. Just recently started learning the basics. Anyway just wanted to post the script and see if maybe I can get some input on where I can maybe do things better.
The script is for extracting the compressed file types you run into the most on Linux. I added the option to extract to a directory of the user's choosing and not just keep it limited to only the working directory.
I feel like I have too many if/then statements and they can somehow be better but I'm not sure. And not sure if I Implemented the exits the right way. Some of you are probably gonna cringe when you see this lol! Anyway if you can help thanks.
#!/bin/bash

error() {
    echo "For help, type: nzip -h"
}

usage() {
    echo "Usage: nzip [FILE]..."
    echo "By default file(s) will be extracted to the working directory."
    echo
    echo "OPTIONS.."
    echo
    echo "-d, --directory, eg. nzip [file] [-d] [directory]"
    echo
    echo
}

if [ $# -eq 0 ]
then
    error >&2
    exit 1
fi

if [ $# -eq 1 ]
then
    case $1 in
        -h) usage
            exit 0
            ;;
        *.7z) 7z e "$1"
            ;;
        *.tar.bz2) tar xjvf "$1"
            ;;
        *.tar.gz) tar xzvf "$1"
            ;;
        *.rar) unrar e "$1"
            ;;
        *.tar) tar xvf "$1"
            ;;
        *.bz2) bunzip2 "$1"
            ;;
        *.tbz) tar xjvf "$1"
            ;;
        *.gz) gunzip "$1"
            ;;
        *.tgz) tar xzvf "$1"
            ;;
        *.jar) unzip "$1"
            ;;
        *.xz) tar xvf "$1"
            ;;
        *.zip) unzip "$1"
            ;;
        *.Z) uncompress "$1"
            ;;
        *) echo "No extraction option for $1" >&2
            exit 1
    esac
    exit 0
fi

if [ $# -eq 3 ] && [ "$2" = "-d" ]
then
    case $1 in
        *.7z) 7z e "$1" -o"$3"
            ;;
        *.tar.bz2) tar xjvf "$1" -C "$3"
            ;;
        *.tar.gz) tar xzvf "$1" -C "$3"
            ;;
        *.rar) unrar e "$1" "$3"
            ;;
        *.tar) tar xvf "$1" -C "$3"
            ;;
        *.bz2) bunzip2 "$1" -c > "$3"
            ;;
        *.tbz) tar xjvf "$1" -C "$3"
            ;;
        *.gz) gunzip "$1" -c > "$3"
            ;;
        *.tgz) tar xzvf "$1" -C "$3"
            ;;
        *.jar) unzip "$1" -d "$3"
            ;;
        *.xz) tar xvf "$1" -C "$3"
            ;;
        *.zip) unzip "$1" -d "$3"
            ;;
        *.Z) uncompress "$1" -c > "$3"
            ;;
        *) echo "No extraction option for $1" >&2
            exit 1
    esac
else
    error >&2
    exit 1
fi
```

I'm not sure why we have #!/bin/bash - it looks like plain, portable #!/bin/sh would be fine here.
Things I immediately like include the good error handling, using &2 for error messages and exiting non-zero.
All the tar commands can be combined, as tar is able (with -a) to automatically identify any compression scheme it handles.
As there's no need to continue once the archiver is run, we can replace the shell process using exec.  E.g.
case $1 in
    *.7z) exec 7z e "$1"
        ;;

Don't do that if we extend the program to process multiple input files, though (see below).
If we take the usual convention of flags first, then it makes it easier when we decide we want to handle more options (e.g. -v, so we can default to quiet operation).  And we can combine the two big case constructs by always passing a directory, but defaulting to the current working directory:
destination=.

die() {
    printf '%s\n' "$@" >&2
    exit 1
}

do_extract() {
    case $1 in
        *.tar.*|*.t[bg]z) tar xaf "$1" -C "$destination"
            ;;
        *.jar|*.zip) unzip "$1" -d "$destination"
            ;;
        # ...
    esac
}

while [ $# -ge 1 ]
do
    case "$1" in
        -d)
            [ $# -gt 1 ] || die "Usage: $0 [-d directory] file..."
            destination=$2
            shift 2
            ;;
        -*)
            die "Unrecognised option: $1"
        *)
            do_extract "$1"
            shift
    esac
done

Be careful with echo:

usage() {
    echo "Usage: nzip [FILE]..."
    echo "By default file(s) will be extracted to the working directory."
    echo
    echo "OPTIONS.."
    echo
    echo "-d, --directory, eg. nzip [file] [-d] [directory]"
    echo
    echo
}

Other (e.g. future) versions of echo may treat that -d as indicating a flag.
I'd be inclined to use a here-file for this:
usage() {
    cat <<'END'
Usage: nzip [FILE] [-d DESTINATION]
By default file(s) will be extracted to the working directory.

OPTIONS:
    -d destination
END
}

Note that I've changed the usage line, because we support only one file, and -d and the destination argument go together (and it's misleading to call it "directory" when it may be a non-directory name if we're unpacking a single file).


Controller for handling user registration I spent a lot of time doing Go Horse Extreme programming, but now I want to be a better person.

How can I make this method smaller, better, more OOP?

/**
 * @return \Illuminate\Http\RedirectResponse
 */
public function store()
{
    if (!User::is_valid(Input::all()))
    {
        Flash::error(User::$errors->first());

        return Redirect::back()->withInput();
    }

    $existing_user = User::where('email', Input::get('email'))->first();

    if (is_null($existing_user)) // The user does not exists.
    {
        // Prepare user data
        $user_input = Input::only(
            'firstName',
            'lastName',
            'email',
            'password');

        $confirmation_token = str_random(60);
        $user_data = array_merge($user_input, ['confirmation_token' => $confirmation_token]);

        // Store user
        $new_user = User::create($user_data);

        // Prepare email
        $data['name'] = Input::get('firstName') . ' ' . Input::get('lastName');
        $data['confirmation_token'] = $confirmation_token;
        $mail_subject = 'Confirmação de e-mail';

        // Send email
        Mail::send('emails.auth.email_confirmation', $data, function ($message) use ($mail_subject)
        {
            $message->to(Input::get('email'))->subject($mail_subject);
        });

        // Log the new user in
        Auth::login($new_user);

        $flash_message = sprintf(
            'Enviamos um email para %s. Confirme sua identidade',
            Input::get('email')
        );

        Flash::warning($flash_message);

        return Redirect::route('admin.dashboard');
    } else
    {
        if ($existing_user->is_active())
        {
            //Try to validate
            $stored = $existing_user->password;
            $typed = Input::get('password');
            $match = Hash::check($typed, $stored);

            if (!$match)
            {
                Flash::warning(
                    'Você já possui uma conta.
                    Faça login para continuar.'
                );

                return Redirect::route('login');
            } else
            {
                Auth::login($existing_user);
                Flash::message(
                    'Você já possui uma conta.
                    Bem vindo de volta!'
                );

                return Redirect::route('admin.dashboard');
            }
        } else
        {
            Auth::login($existing_user);

            $flash_message = sprintf(
                'Enviamos um email para %s. Confirme sua identidade [Reenviar]',
                $existing_user->email
            );

            Flash::warning($flash_message);

            return Redirect::route('admin.dashboard');
        }
    }
}

It just handles user registration. Everything seems to be working when I run my Behat tests, but I feel (deep in my soul) that things could be a lot better.

There are many ways you can refactor this method but since you are using Laravel things are pretty easy. Here is my attempt on refactoring (I'd rather call it re-writing) this method.

First of all, this controller is overloaded with so many responsibilities, a controllers sole job should only be to accept an HTTP Request, defer the execution to responsible object and deliver an HTTP Response, that is all a controller need to do. so lets refactor it first.

Defer execution to service class

I'd create a service class with name AccountService and inject it into AccountController (or whatever you call it in your case), then on AccountService I'd create a method named

createNewAcocunt($firstName, $lastName, $email, $password)

which will validate the data and create new account.

Take care of responsibilities

The else part of controller method store() defeats the purpose of REST thingy, I'd just issue a redirect to login page with an appropriate message instead of trying to log them in. all of login stuff and account activation status checking should be performed over login route so it clearly does not belongs to the account creator (or signup) route. with these 2 things in place the controller method store() would look like this

public function store()
{
    $details = Input::only('firstName', 'lastName', 'email', 'password');
    try {
        $user = $this->accountService->createNewAccount($details['firstName'], $details['lastName'], $details['email'], $details['password']);
        Auth::login($user->id);
        return Redirect::route('admin.dashboard');
    } catch (ValidationException $validationEx) {
        return Redirect::back()->withError($validationEx->getErrors()); // assuming you set errors on exception object before throwing it
    }
}

Now over to the AccountService class.
The method createNewAcocunt($firstName, $lastName, $email, $password), by its name, should create an account with data fed to it. In case there is an error it should throw an exception.

Take advantage of events as they happen

Now, you need to take care of responsibilities once again, do not overload this method with mail sending, user login or stuff like that, the only responsibility of this method is to create an account. To handle the mail sending and other tasks you need to perform after creating an account you can use laravel's EventDispatcher class or Event facade (Documentation here).
My method would look like this

public function createNewAcocunt($firstName, $lastName, $email, $password)
{
    $validation = $this->accountValidator->validate(
            compact('firstName', 'lastName', 'email', 'password')
    );

    if($validation->fails())
    {
        $up = new ValidationException('Invalid data for new account');

        // set error messages on exception to communicate them to upper layer
        $up->setErrors($validation->errors);
        throw $up;
    }

    $token = str_random(60)

    // if you use a repository then
    $user = $this->repository->createNewAccount($firstName, $lastName, $email, $password, $token);
    // or do it in whatever way you prefer

    // Now that the user is created fire suitable event
    Event::fire('account.created', [$user]);

    return $user;
}

Most of it has already sorted out, now you can catch the event account.created and perform whatever action you want.
My advice for sending out emails would be to queue emails instead of sending them in real time, this will decrease the response time since the codes will no longer poll for email to be dispatched

Mail::queue('emails.auth.email_confirmation', $data, function ($message) use ($mail_subject)
{
    $message->to(Input::get('email'))->subject($mail_subject);
});

read more about email queue here

and at last, you can customize the validation error message for email:unique rule and make that include a link to login page. check out the custom error message section over here. basically you can just pass in an associative array to Validator like this:

$messages = [
    'email.unique' => 'Looks like you already have an account, please Log in here'
];

$validation = Validator::make($data, $rules, $messages);
$validation->errors->get('email'); // should display the message as defined above

I hope this will give you at least an idea to get started with. I, for one, am really happy about your decision to go pro.


Class/model hierarchy for a school-management application Please find how I would model a system that had teachers, students, classes and parents. Instead of creating a model for each user type, I created only two models, User and Profile. Each user can have multiple profiles and each profile can have a unique profile_type.

I would love to get some feedback on my proposed architecture.

class User < ActiveRecord::Base
  has_many :profiles
  has_many :user_klasses
  has_many :klasses, through: :user_klasses

  def get_student_classes
    student_profile = profiles.select { |p| p.profile_type == 'student' }.first
    return [] if student_profile.nil?

    return klasses.select { |k| k.profile_id == student_profile.id }
  end

  def get_teacher_classes
    teacher_profile = profiles.select { |p| p.profile_type == 'teacher'}.first
    return [] if teacher_profile.nil?

    return klasses.select { |k| k.profile_id == teacher_profile.id }
  end

end

class Klass < ActiveRecord::Base
  has_many :user_klasses
  has_many :users, through: :user_klasses
  belongs_to :teacher, class_name: 'Profile', -> { where profile_type: 'teacher' },
                        dependent: :destroy
end

class UserKlasses < ActiveRecord::Base
  belongs_to :user
  belongs_to :klass
end

class Profile < ActiveRecord::Base
  belongs_to :user
end

You mention that you would rather not create extra models but I would consider creating a model for Teacher, Student and for Parent. It is not really that much more work but will probably keep the separation between the concepts much cleaner in the long run (as per Damien's comments). I personally would also use names like TeacherRole and ParentRole or TeacherFacet.

I would also consider keeping the cardinality as has_one unless you need to deal with the possibility that a user needs more than one teacher profile (possibly because they teach in more than one places) or more than one parent profile (maybe they have multiple children).

You obviously haven't shown your complete data model but it looks wrong that each user is linked to classes through the user_klasses relation but then you query classes using a profile_id field on the Klass. Firstly you probably want to query the profile_id on the UserKlass model (the profile_id on Klass looks like it refer to the teacher's profile) but you should probably put the klasses relation on the Profile model (or whatever you call it).

One other thing I noticed here:

  belongs_to :teacher, class_name: 'Profile', -> { where profile_type: 'teacher' },
                    dependent: :destroy

You probably don't want to remove the teacher record if a class is deleted, they may be teaching other classes after all. However you probably want to delete UserKlass if the Klass (or User for that matter) is deleted. Likewise it would make sense to delete the Profile record when a User is deleted.


Find a better logic to remove duplicated code from two different methods I have to update a class property with the method name and then call a method that executes some code based on this property. The code is the same in both methods, but I am not seeing a way to remove this code duplication. I am trying to avoid using a parameter.

public function register(){
    $this->operation = __FUNCTION__;
    $this->handleOperation();
}

public function unregister(){
    $this->operation = __FUNCTION__;
    $this->handleOperation();
}

Update 1

As Yoda suggested, here is a more complete code. It is a class to manipulate multiple functions for PHP autoloading. Now I am using parameters in handleOperation(). Example of use:

$Autoloader = new Autoloader();
$Autoloader->setFunctions(array('controllers', 'commons'));
$Autoloader->setModule('default');
$Autoloader->register();

Class code:

class Autoloader {

    private $functions_to_handle = array();
    private $module;
    private $function;
    private $paths = array();

    public function setFunctions($functions){
        $this->functions_to_handle = is_array($functions) ? $functions : array($functions);
    }

    public function setModule($module){
        $this->module = $module;
    }

    public function register(){
        $this->handleOperation(__FUNCTION__);
    }

    public function unregister(){
        $this->handleOperation(__FUNCTION__);
    }

    private function handleOperation($operation){
        $built_in_function = $this->mountBuiltInFunctionName($operation);

        foreach($this->functions_to_handle as $function){
            $built_in_function(array($this, $function));
        }
    }

    private function mountBuiltInFunctionName($operation){
        return 'spl_autoload_' . $operation;
    }

    private function controllers($classe) {
        $this->function = __FUNCTION__;

        $this->refreshPaths($classe);
        $this->loadClass();
    }

    private function commons($classe) {
        $this->function = __FUNCTION__;

        $this->refreshPaths($classe);
        $this->loadClass();
    }

    private function refreshPaths($classe){
        switch ($this->function) {

            case 'commons' :
                $this->paths = array(
                    '/usr/share/evokernel/core/common/' . $classe . '.php',
                );
            break;

            case 'controllers':
                $this->paths = array(
                    'core/controllers/' . $classe . '.php',
                    'modules/' . $this->module . '/controllers/' . $classe . '.php',
                    'shared/modules/' . $this->module . '/controllers/' . $classe . '.php'
                );
            break;

            default:
                throw new Exception('Invalid function: ' . $this->function);
        }
    }

    private function loadClass(){
        foreach ($this->paths as $path) {
            if (is_file($path)) {
                require_once($path);
                break;
            }
        }
    }
}

Short version: Use a parameter!

public function handleOperation($action) {}
public function register() { $this->handleOperation("register"); }
public function unregister() { $this->handleOperation("unregister"); }

Slightly longer version: Don't be a smartass

You don't need to dynamically call spl_autoload_$function, it's pointless and unreadable. Just call spl_autoload_registerand spl_autoload_unregisterwhen you need it. It's unlikely for the function names to change.


Ordered dictionary in swift 2 Loosely basing myself on this blog post, I implemented an ordered dictionary. In essence, it's a wrapper struct for a list of tuples with some initialisers and some functions (map, filter, toArray, ...) added to it.

struct OrderedDictionary<KeyType: Hashable, ValueType>: SequenceType
{
    typealias KeyValueType = (key:KeyType, value:ValueType)

    private var tuples: [KeyValueType] = []

    init() {}

    init(_ tupleArgs: KeyValueType...)
    {
        self.init(tupleArgs)
    }

    init(_ tupleArgs: [KeyValueType])
    {
        tuples = tupleArgs
    }

    subscript(key:KeyType) -> ValueType?
    {
        get
        {
            return tuples.filter({ $0.key == key })[0].value
        }

        set(newValue)
        {
            if let val = newValue
            {
                if tuples.contains({ $0.key == key })
                {
                    tuples = tuples.map({ $0.key == key ? ($0.key, val) : $0 })
                }
                else
                {
                    tuples.append((key, val))
                }
            }
            else
            {
                tuples = tuples.filter({ $0.key != key })
            }
        }
    }

    func generate() -> AnyGenerator<KeyValueType>
    {
        var nextIndex = 0
        return anyGenerator({
            if nextIndex >= self.tuples.count
            {
                return nil
            }
            else
            {
                return self.tuples[nextIndex++]
            }
        })
    }

    mutating func insert(tuple: KeyValueType, atIndex index: Int)
    {
        tuples = tuples.filter({ $0.key != tuple.key })
        tuples.insert(tuple, atIndex: index)
    }

    func map<NewKeyType:Hashable, NewValueType>(transform: (KeyValueType) -> (key:NewKeyType, value:NewValueType)) -> OrderedDictionary<NewKeyType, NewValueType>
    {
        return OrderedDictionary<NewKeyType, NewValueType>(tuples.map(transform))
    }

    func filter(includeElement: (KeyValueType) -> Bool) -> OrderedDictionary<KeyType, ValueType>
    {
        return OrderedDictionary(tuples.filter(includeElement))
    }

    func sort(isOrderedBefore: (KeyValueType, KeyValueType) -> Bool) -> OrderedDictionary<KeyType, ValueType>
    {
        return OrderedDictionary(tuples.sort(isOrderedBefore))
    }

    func reverse() -> OrderedDictionary<KeyType, ValueType>
    {
        return OrderedDictionary(tuples.reverse())
    }

    func toArray<T>(transform:((KeyValueType) -> T)) -> [T]
    {
        return tuples.map(transform)
    }

    func toArray() -> [KeyValueType]
    {
        return tuples
    }

    func toDict() -> [KeyType:ValueType]
    {
        var dict: [KeyType:ValueType] = [:]
        for tuple in tuples
        {
            dict[tuple.key] = tuple.value
        }
        return dict
    }
}

Does this look okay? Is it Swift-y enough and are there no major performance issues? I'm a bit worried about the filter, map, reverse and sort functions, but I'm not sure why. I'm also not confident about my use of map in the subscript setter.

Your code looks generally good (with two real problems explained below).

Instead of an empty default array

private var tuples: [KeyValueType] = []

which is overwritten in two of the three init methods, I would initialize the
array explicitly:

private var tuples: [KeyValueType]

init() {
    tuples = []
}

but that is perhaps a matter of taste.

The problem with the other two init methods is that they don't check
for duplicate keys, e.g. if you call

let od = OrderedDictionary(("foo", 1), ("bar", 2), ("foo", 3))

You either have to add an explicit check (and throw an error or fail
if duplicate keys are given), or add the key/value pairs sequentially
so that duplicate keys are silently overwritten with the new value:

init(_ tupleArgs: [KeyValueType]) {
    self.init()
    for (key, value) in tupleArgs {
        self[key] = value
    }
}

There is a problem in your subscript getter method:

return tuples.filter({ $0.key == key })[0].value

will crash if there is no key/value tuple for the given key, instead of returning
nil, because accessing [0] requires that there is at least one element.
This can be fixed as

return tuples.filter({ $0.key == key }).first?.value

because first returns an optional. But this is not very effective. The
filter method always traverses the entire array, but only the first (and only)
matching tuple is needed. This can be achieved with indexOf:

get {
    if let index = tuples.indexOf({ $0.key == key }) {
        return tuples[index].value
    } else {
        return nil
    }
}

Similarly, in the subscript setter method, it is not effective to call
contains first and then map the entire tuple array to a new array.
I would implement that as

set(newValue) {
    if let index = tuples.indexOf({ $0.key == key }) {
        if let val = newValue {
            // Update existing value
            tuples[index].value = val
        } else {
            // Remove existing value
            tuples.removeAtIndex(index)
        }
    } else if let val = newValue {
        // Add new value
        tuples.append((key, val))
    }
}

The generate method can be simplified considerably, since you are only
forwarding the enumeration to the tuples array:

func generate() -> AnyGenerator<KeyValueType> {
    return anyGenerator(tuples.generate())
}

The

func toArray<T>(transform:((KeyValueType) -> T)) -> [T]

is not really needed, you'll get the same result already with your
map() and toArray() method.

You should make your type conform to CustomStringConvertible so that
printing an ordered dictionary gives a nice looking result. A simple
implementation would be

extension OrderedDictionary : CustomStringConvertible {
    var description: String {
        return "[ "
            + tuples.map({ (key, value) in "\(key) : \(value)"}).joinWithSeparator(", ")
            + "]"
    }
}

Creating a new (ordered) dictionary by mapping both keys and values
can lead to unexpected results if two keys are mapped to the same
new key. E.g. (with the fixed init method from above):

let od1 = OrderedDictionary(("foo", 1), ("bar", 2), ("FOO", 3))
print(od1) // [ foo : 1, bar : 2, FOO : 3]

let od2 = od1.map { (key, value) in (key.uppercaseString, value) }
print(od2) // [ FOO : 3, BAR : 2]

You have to decide if that is wanted. Alternatively, define a
map() method which only maps the values but preserves the keys.

If you plan to add the code to a framework then you have to annotate the struct
itself and all its public methods as public.

Some remarks concerning the performance: Each key lookup is done by a linear
traversal of the tuples array. For large dictionaries this can become a problem.

The project from the referenced blog post solves this by maintaining both
a dictionary (for the key/value pairs) and an array (for the ordered keys).
 The advantage is that a
lookup is fast. But adding a new key or removing a key still requires a
linear traversal of the keys array. Whether this is acceptable or not
depends on how often keys are added/removed compared to key/value lookups.

One could store the key/value pairs in a tree structure to improve the
performance (e.g. AVL tree or a red-black tree). Of course that increases
the complexity of the code.


Wrapper for boolean type to reduce the use of if statements I want to create wrapper for boolean type to not use more if statements. Can you tell me that this class make sense?

public final class Bool {

    private boolean value;

    private Bool(boolean value) {
        this.value = value;
    }

    public static final Bool of(boolean value) {
        return new Bool(value);
    }

    public static final Bool ofFalse() {
        return new Bool(false);
    }

    public static final Bool ofTrue() {
        return new Bool(true);
    }

    public OrElse ifTrue(Procedure procedure) {
        if (value)
            procedure.invoke();
        return new OrElse(!value);
    }

    public OrElse ifFalse(Procedure procedure) {
        if (!value)
            procedure.invoke();
       return new OrElse(value);
    }

    public class OrElse {
        private boolean value;

        private OrElse(boolean value) {
            this.value = value;
        }

        public void orElse(Procedure procedure) {
            if (value)
                procedure.invoke();
        }
    }
}

Example usage:

public Bool findElement(String name) {
    for(Element el : elements) {
       if(el.getName().equals(name)) return Bool.ofTrue();
    }
    return Bool.ofFalse();
}

// And now in other place of code I may use:
findElement("el").ifTrue(() -> doSomething).orElse(() -> doSomethingElse)

Approach

The approach is interesting, but doesn't seem practical or actually useful.

I think that you need more justification for this than simply stating that you want to use less if-statements.

Especially since you essentially still have the if-structure, just with a different syntax:

// your code:
findElement("el").ifTrue(() -> doSomething).orElse(() -> doSomethingElse)

// regular if, one line (which I would not recommend, it's more difficult to read than multi-line):
if (findElement("el")) doSomething; else doSomethingElse;

// standard if:
if (findElement("el")) {
    doSomething;
} else {
    doSomethingElse;
}

If there are other reasons for your approach, you need to make them clear. But this is definitely not a good replacement for general-purpose if statements.

And if you use this throughout a project, you have dependencies to this custom wrapper everywhere. Removing it later will be a lot of work. You will also need to wrap each return value from libraries and the standard APIs (which will be ugly in itself), or your code will consist of a mixture of standard ifs and your approach, which will not help readability.

Readability

Your approach is already more difficult to read than the standard approach, and with anything except a simple function-call, your approach would be even less readable:

// your code:
findElement("el").ifTrue(() -> {doSomething; doSomething2; doSomething3;}).orElse(() -> doSomethingElse; doSomethingElse2;)

// standard:
if (findElement("el")) {
    doSomething;
    doSomething2;
    doSomething3;
} else {
    doSomethingElse;
    doSomethingElse2;
}

You can of course also make your version multi-line, but that doesn't make matters much better:

findElement("el").ifTrue(() -> {
    doSomething;
    doSomething2;
    doSomething3;
}).orElse(() -> {
    doSomethingElse;
    doSomethingElse2;
})

You are still recreating the if-structure, just with a worse syntax.

Performance

I did not profile your approach, but in many situations, the extra cost of the additional function calls for the simplest of basic expressions will be noticeable.

Misc

I would recommend curly brackets even for one-line statements. They do make code easier to read, and can prevent bugs.
I would always use this when accessing fields, it makes it clearer where values come from.
Do you really need the value field and if in OrElse? I would assume that an else should always be executed.


Is there currently anything wrong with my custom authentication and authorization? I have my reasons not to use the ASP.NET membership. Though this causes me some issues. I read about a thousand articles on ASP.NET MVC custom authentication and I've found that almost all of them are just too simple to be of any use.

What I need is a good way to login a user, keep some info about him securely in a cookie (info such as id, username, email and role) and later use this info to assure that he's authorized to visit and edit certain pages. The way I have things right now is a bit convoluted and possibly insecure.

Here's the relevant code in my login method:

// Set session cookie
var cookieName = CookieHelper.MergeIntoString(user.Id, user.Username, user.IsAdmin, user.IsStaff);
// Code for the CookieHelper class is to follow below!
var timeout = model.RememberMe ? 525600 : 2880; // 365 days or 2 days
var ticket = new FormsAuthenticationTicket(cookieName, model.RememberMe, timeout);
var encrypted = FormsAuthentication.Encrypt(ticket);
var cookie = new HttpCookie(FormsAuthentication.FormsCookieName, encrypted)
{
    Expires = DateTime.Now.AddMinutes(timeout),
    HttpOnly = true
};

Response.SetCookie(cookie);

Code for the CookieHelper class:

public static class CookieHelper
{
    public static string MergeIntoString(int id, string username, bool isAdmin, bool isStaff)
    {
        return id + "/" + username + "/" + isAdmin + "/" + isStaff;
    }

    public static Dictionary<string, object> DecodeFromString(string cookieName)
    {
        var cookieDictionary = new Dictionary<string, object>();
        var cookieArray = cookieName.Split('/');

        cookieDictionary.Add("Id", cookieArray[0]);
        cookieDictionary.Add("Username", cookieArray[1]);
        cookieDictionary.Add("IsAdmin", cookieArray[2]);
        cookieDictionary.Add("IsStaff", cookieArray[3]);

        return cookieDictionary;
    }
}

Why am I using the CookieHelper class? Because I have no idea how to add more information that will be encrypted in my cookie. So I decided to put the info into the cookie name string and later split it to get my info.

And this is how I later reach into the cookie to get the user id:

var authCookie = HttpContext.Request.Cookies[FormsAuthentication.FormsCookieName].Value;
if (authCookie == null)
    return RedirectToAction("Index", "Home");

var cookieName = FormsAuthentication.Decrypt(authCookie).Name;
var cookie = CookieHelper.DecodeFromString(cookieName);
var userId = Convert.ToInt32(cookie["Id"]);

Now I'm sure there's thousands of issues with this. Can you tell me of a better way of doing what I'm trying to do?

I can't help with the security aspect of it because I don't know enough about it. So just same general code review things:

It seems odd that your CookieHelper class contains two methods which seem like they should be complementary operations however they are named inconsistently and their parameter/results don't really match up:

The MergeIntoString accepts a bunch of parameters which are all taken from some kind of user object.
The DecodeFromString splits up a string which was generated by MergeIntoString but returns a dictionary instead which you then seemingly have to parse back into a user.

I would suggest to rename the first method to UserToCookie and pass the user as single parameter returning a string and rename the second one to CookieToUser which accepts a string and returns a user object.

Alternatively keep the dictionary for the second method but then also pass in a dictionary in the first. You could then have helper methods which convert users to/from dictionaries.
You have some magic constants without a unit and therefore had to write a comment of what it means.

   var timeout = model.RememberMe ? 525600 : 2880; // 365 days or 2 days

.NET has a TimeSpan type which is ideal to store this kind of information and it become obvious what unit it is and works nice with DateTime as well. So something along these lines:

var timeout = model.RememberMe ? TimeSpan.FromDays(365) : TimeSpan.FromDays(2);

....

    Expires = DateTime.Now + timeout,

Doesn't require a comment and it's immediately visible what the timeout could be.


Given n numbers, find out the least k numbers from them I have been solving the following problem:

  Given n numbers find out the least k numbers from them. For example, if there are 8 numbers like 5,6,3,4,7,8,9,-1. For k = 3 the result will be -1,3,4.

I implemented it in the following way:

make heap with first k numbers
traverse the rest k-n numbers

if heapMax > any of the k numbers then
swap

return heap containing k numbers

Can you please review my code and provide me with feedback?

#include <iostream>
#include <set>
#include <vector>

std::vector<int> getKLeastNumbers(std::vector<int> &nums, int k)
{
    std::vector<int> tmpNums;

    if(k<=0 || k > nums.size())
    {
        return tmpNums;
    }

    for(int i=0; i<k; i++)
    {
        tmpNums.push_back(nums.at(i));
    }
    std::make_heap(tmpNums.begin(), tmpNums.end());

    for(int i=k; i<nums.size(); i++)
    {
        if (tmpNums.front() > nums.at(i))
        {
            int tmp = nums.at(i);

            nums.at(i) = tmpNums.front();

            std::pop_heap(tmpNums.begin(),tmpNums.end());
            tmpNums.pop_back();

            tmpNums.push_back(tmp);
            std::push_heap(tmpNums.begin(), tmpNums.end());
        }
    }

    return tmpNums;
}

int main()
{
    // test codes
    std::vector<int>nums;
    nums.push_back(56);
    nums.push_back(5);
    nums.push_back(6);
    nums.push_back(60);
    nums.push_back(-6);
    nums.push_back(3);
    nums.push_back(600);

    nums = getKLeastNumbers(nums,1);

    for(int i=0; i<nums.size(); i++)
    {
        std::cout<<nums.at(i)<<std::endl;
    }

    std::vector<int>nums2;
    nums2.push_back(56);

    nums2 = getKLeastNumbers(nums2,-1);

    std::cout<<std::endl<<"Test set 2 "<<std::endl;
    for(int i=0; i<nums2.size(); i++)
    {
        std::cout<<nums2.at(i)<<std::endl;
    }
}

I assume you can use C++11.

Use initializer lists:

std::vector<int>nums;
nums.push_back(56);
nums.push_back(5);
nums.push_back(6);
nums.push_back(60);
nums.push_back(-6);
nums.push_back(3);
nums.push_back(600);

becomes

std::vector<int> nums = {56, 5, 6, 60, -6, 3, 600};

Use range based for:

nums = getKLeastNumbers(nums, 1);

for(int i=0; i<nums.size(); i++)
{
    std::cout<<nums.at(i)<<std::endl;
}

becomes

for (auto &i : getKLeastNumbers(nums, 1))
    std::cout << i << '\n';

Do not hide bugs. If you detect that someone used your function wrong tell them instead of letting them wonder why the return value doesn't make sense.

if(k<=0 || k > nums.size())
{
    return tmpNums;
}

becomes

assert(k <= 0 && k > nums.size());

or

if (k <=0 || k > nums.size())
    throw std::range_error("k must be bigger than 0 and less than num's size");

You seem to be using the heap functions correctly, however, there are functions more suitable for that. I would pick std::partial_sort.

std::vector<int> getKLeastNumbers(std::vector<int> &nums, int k)
{
    std::partial_sort(std::begin(nums), std::begin(nums) + k, std::end(nums));
    return { std::begin(nums), std::begin(nums) + k };
}

The declaration for getKLeastNumbers is a bit strange. nums is not const and gets modified which feels unexpected. Why can I not use getKLeastNumbers on a const std::vector?. I would prefer a const & to preserve the original value, allow const vector and to be able to initialize nums with an initializer list directly.


Rotating an Image using own algorithm in python For an uni assignment I have been giving the task of making my own rotating algorithm in python. This is my code so far.

import cv2
import math
import numpy as np

class rotator:

    angle = 20.0
    x = 330
    y = 330

    radians = float(angle*(math.pi/180))
    img = cv2.imread('FNAF.png',0)
    width,height = img.shape

    def showImg(name, self):
        cv2.imshow(name, self.img)
        self.img = np.pad(self.img, (self.height) ,'constant', constant_values=0)
        self.width,self.height = self.img.shape

    def printWH(self):
        print(self.width)
        print(self.height)

    def rotate(self):
        emptyF = np.zeros((self.width,self.height),dtype="uint8")
        emptyB = np.zeros((self.width,self.height),dtype="uint8")
        emptyBB = np.zeros((self.width,self.height),dtype="uint8")

        for i in range(self.width):
            for j in range(self.height):
                temp = self.img[i,j]
                #forward mapping
                xf = (i-self.x)*math.cos(self.radians)-(j-self.y)*math.sin(self.radians)+self.x
                yf = (i-self.x)*math.sin(self.radians)+(j-self.y)*math.cos(self.radians)+self.x
                #backward mapping should change the forward mapping to the original image
                xbb = (i-self.x)*math.cos(self.radians)+(j-self.y)*math.sin(self.radians)+self.x
                ybb = -(i-self.x)*math.sin(self.radians)+(j-self.y)*math.cos(self.radians)+self.x
                xbb = int(xbb)
                ybb = int(ybb)
                if xf < 660 and yf < 660 and xf>0 and yf > 0:
                    emptyF[int(xf),int(yf)] = temp
                else:
                    pass
                if xbb < 660 and ybb < 660 and xbb>0 and ybb > 0:
                    emptyBB[(xbb),(ybb)] = temp
                else:
                    pass
        cv2.imshow('Forward', emptyF)
        cv2.imshow('Backward', emptyBB)
def main():
    rotator.showImg('normal', rotator)
    rotator.printWH(rotator)
    rotator.rotate(rotator)
    cv2.waitKey(0)
    cv2.destroyAllWindows

if __name__ == '__main__':
    main()

Is there any way to improve this code or my algorithm? Any advise would be awesome.

NB. I also hav a problem with my backwards mapping. The output image has a lot of small black dots. Any suggestions to what that might be?

Please let me know.

The most common method for getting rid of the black dots is to paint over each pixel of the entire destination image with pixels copied from computed positions in the source image, instead of painting the source image pixels into computed positions in the destination image.

for i in range(660):
    for j in range(660):
        xb = int(...)
        yb = int(...)
        if xb in range(self.width) and yb in range(self.height):
            emptyF[i, j] = self.img[xb, yb]

Computing destination pixels positions from the source pixel positions may result in tiny areas unpainted in the destination due to numeric errors/limits.

Computing source pixels positions from the destination pixel position may result in a pixel value being copied to two adjacent locations, and other pixels never being copied from, but this is usually/often unnoticeable.


Parsing GSM 7-Bit messages in Java I wrote a function to decode GSM 7-Bit encoded messages as defined in the 3GPP standard. Example available here. I found a tool to help me check my answers here. I used the charset from here.

public String decode(final byte[] data) {
    if (data == null) {
        return null;
    }

    final ByteArrayOutputStream bos = new ByteArrayOutputStream();
    for (int i = 0, counter = 0; i < data.length; i++, counter++) {
        final byte current = data[i];
        final byte previous = i == 0 ? 0 : data[i - 1];

        if (counter == 7) {
            counter = 0;
        }

        final int shiftedCurrent = (current << counter) & 0x7F;
        final int shiftedPrevious = (previous & 0xFF) >> (8 - counter);
        bos.write(shiftedCurrent | shiftedPrevious);

        if (counter == 6) {
            bos.write((current & 0xFF) >> 1);
        }
    }

    try {
        final String result = bos.toString("X-Gsm7Bit");
        return result.endsWith("\r") ? result.substring(0, result.length() - 1) : result;
    } catch (final UnsupportedEncodingException e) {
        throw new AsnDecodeException("Cannot get charset: X-Gsm7Bit");
    }
}

Tests:

@Test
public void decodeTests() throws DecoderException {
    assertThat(decode(Hex.decodeHex("f4f29c9e769f1b".toCharArray()))).isEqualTo("testing");
    assertThat(decode(Hex.decodeHex("31D98C56B3DD70".toCharArray()))).isEqualTo("12345678");
    assertThat(decode(Hex.decodeHex("6176D94DAFCB1B".toCharArray()))).isEqualTo("alentur");
    assertThat(decode(Hex.decodeHex("61F79B8E2ECB5B657CB80D679701E77638CD768DDF6D".toCharArray()))).isEqualTo("another-example@gmail.com");
}

I feel like I could have done this with fewer shifts or & checks, but the thing I really don't like is my counter variable that I reset at 7. Any advice on how to improve this code?

thing I really don't like is my counter variable that I reset at 7

may be just keep incrementing the counter as counter = (counter + 1) % 8 instead of keeping it in the for loop.

  Any advice on how to improve this code?

this may be just me, but bit operations are very mysterious so I'd like to have a variable for 0x7F & 0xFF to understand what's to special

Also, I'm not really sure why counter == 6 leads to the additional bit. It would help if you could take it out into a function that gets called.

result.endsWith("\r") seems a bit odd. Is that a specific requirement. If so, do you also need to handle \n ?

throw new AsnDecodeException("Cannot get charset: X-Gsm7Bit"); when throwing exceptions, I'd recommend adding some context on the  input data leading to the error message. It helps a lot when

Don't forget to test for bad input data like

""
null
"zz"
"123456789ø"

I'd also just recommend creating a helper class and using hamcrest matchers to make the testing more readable i.e. assertThat(hex("f4f29c9e769f1b"), is("testing"))


File-copying Manager I have the following java class which has many private variables and methods:

package gui;
import java.awt.Color;
import java.io.File;
import java.util.*;
import java.util.stream.Stream;
import javax.swing.*;
import messages.Message;
import net.miginfocom.swing.MigLayout;
import utils.*;
public class FileCopyManager extends JFrame {
    public static String appName  = "File Copy Manager v1.6.4.0";
    private PreferencesManager pManager             = new PreferencesManager(this);
    private JCheckBoxMenuItem allowDuplicatesOption = new JCheckBoxMenuItem("Allow dupliactes in list");
    private Controller controller = new Controller();
    private StatusFrame status    = new StatusFrame();
    private FileHandler fHandler  = new FileHandler(status);
    private Message      msg      = new Message();
    private JMenuBar menuBar      = new JMenuBar();
    private JMenu   fileMenu      = new JMenu("File"),
                    editMenu      = new JMenu("Edit");
    private JMenuItem saveList    = new JMenuItem("Save queue"),
            exit                  = new JMenuItem("Exit"),
            loadList              = new JMenuItem("Load queue"),
            openAppDirectory      = new JMenuItem("Open app folder"),
            showPreferences       = new JMenuItem("Preferences"),
            exportPreferences     = new JMenuItem("Export Preferences"),
            deleteApp             = new JMenuItem("Delete app settings"),
            restartApp            = new JMenuItem("Restart Application");
        private File      selectedFile = null;
    private String destinationPath = null;
    private ArrayList<File> files  = new ArrayList<>();
    private JPanel panel = new JPanel();
    private JFileChooser chooser = new JFileChooser();
    private JButton addFiles, selectDestination, copyFile, copyFiles,
            deleteFile, deleteAll, openDestinationFolder,stopCopy;
    private JComboBox<String> fileNames;
    private DefaultComboBoxModel<String> model;
    private int selectedFileIndex = -1;
    private JTextField dragPanel = new JTextField(20);
    private JLabel dragLabel;
    private File listFile = new File("app"+PreferencesManager.sep+"userList.dat");
    private boolean allowDuplicates = false;
    private Thread[] copyThreads = new Thread[2];
    private boolean isNull(Object...t){
        /*
         * Check if any of the arguments is null
         */
    }
    private void allowCopy(){
        /*
         * Enables certain buttons based on certain criteria
         */
    }
    private void allowDelete(){
        /*
         * Enables certain buttons based on certain criteria
         */
    }
    private void allowEdits(){
        /*
         * Enables certain buttons based on certain criteria
         */
    }
    private void createList(){
        //Create a list with some random names
    }
    private void initDragAreas(){
        /*
        * Create some drag and drop ares in the GUI
        */
    }
    public void updateList(){
        // Update a lst in the CUI with some names
    }
    public void showFiles() {
        //Show some file names in the GUI
    }
    public void restart(){
        //Code that restarts the GUI
    }
    @SuppressWarnings("deprecation")
    private void initUIElements() {
        /***
        * Create some UI elements
        */
        initDragAreas();
    }
    public JLabel[] getLabels() {
        // return some labels
    }
    public JButton[] getButtons() {
        // return some buttons
    }
    public void preload() {
        /*
        * See if we need to change the main UI(change colors or font size)
        * and if we need do it first and then show the app
        */
    }
    public FileCopyManager) {
        initUIElements();
        preload();
    }
}

This application is a small project I made for fun during summer.
I was wondering: Is it a bad thing to use too many private members and methods?

Would it be bad for a production code?

You are putting all in one class. That makes structuring and rereading hard. Hence your question, and maybe you felt the urge to make sections in the class by something like // ---------------------.

Then it is time to look at the Model-View-Controller paradigm. An internet search will give better material on this subject than I can.

Basically you have one instance of a Controller class, that is more or less accessible from everywhere. There are View classes (the JFrame) and the Model classes, the data model, directory lists.

The Models can be developed independently, and used in a library.

The Controller is listener of views and models, and controls views.
It holds the data model and the view (JFrame).

The Views signal events to the controller. And provide hooks for the controller. They get passed data, the model.

So you can have several classes, in several subpackages.

The approach is:

Start a new project
Use a version control system, like GIT (has the repository with the project)
An empty controller that starts the JFrame with itself
The JFrame, the original code

And then move the data to the outside, handle in the Controller.
Data changes via events over the controller.
Business logic like enabling/disabling in the controller.

Then you might be able to split up the GUI in specific JPanels or such.

Instead of doing the same actionPerformed for a menu item and button, you might use an Action extending AbstractAction, and add that to the constructors of JMenuItem and JButton. That action might then be disabled/enabled instead of doing it to the components. It also means that you need not make all components available as fields, for instance all those JMenuItems.

For the rest, production quality, for me means, using a build infrastructure like maven, that handles for you the usage of third party libraries, versioning and so on.


PHP session fixation I'm trying to implement a system of restricted access. Right now I'm focusing on the "session fixation". I am concerned about the following aspects:

Control of a "fingerprint" of the user created by mixing UserAgent, IPAddress and a salt key. If the fingerprint were to change, destroy the user's session.
Using the same session id for a certain number of times, after which regeneration session id.
Regenerate the session id every time change the level of user authentication

What I want to know is whether my approach is right and if it is safe enough.
I apologize if the code is long, I tried to remove all the unnecessary parts.

I do not want a code review, but the opinions about it ... particularly if the mine is a safe approach.

<?php
class oLogonUser
{
    const SESSION_LIMIT = 10;
    const SESSION_SALT  = 'AS86F(,sa)8as7d+/234N&&"$£%';

    function __construct()
    {
        // init session
        $this->InitSession();

        // check if user logged
        if( $this->isUserLogged() )
        {
            // Check if finger print is valid
            if( !$this->isValidFingerPrint() )
                $this->LogOutUser(); // log out
        }
    }

    private function InitSession()
    {
        // check if session already start
        if( session_id() == '' )
            session_start();

        // Set user's fingerprint
        $this->setUserFingerPrint();

        // Session counter
        if( isset($_SESSION['UserSessionCounter']) )
            $_SESSION['UserSessionCounter'] = (int)$_SESSION['UserSessionCounter'] + 1;
        else
            $_SESSION['UserSessionCounter'] = 0;

        // if session counter exeed limit, regenerate session id
        if( $_SESSION['UserSessionCounter'] > self::SESSION_LIMIT )
        {
            $_SESSION['UserSessionCounter'] = 0;
            session_regenerate_id();
        }
    }

    private function isValidFingerPrint()
    {
        // checking if the user fingerprint is the same as that stored in session
        if( isset($_SESSION['UserFingerPrint']) )
            return ( $_SESSION['UserFingerPrint'] === $this->getUserFingerPrint() );

        return false;
    }

    private function getUserFingerPrint()
    {
        // return user fingerprint
        return md5($_SERVER['REMOTE_ADDR'] . $_SERVER['HTTP_USER_AGENT'] . self::SESSION_SALT );
    }

    private function setUserFingerPrint()
    {
        // store in session user fingerprint
        if( !isset($_SESSION['UserFingerPrint']) )
            $_SESSION['UserFingerPrint'] = $this->getUserFingerPrint();
    }

    public function LogOnUser($UserName, $Password)
    {
        //NB: in fact control the credentials in the database
        if( $UserName == 'test' && $Password == 'test' )
            $this->setUserLogged();
    }

    public function LogOutUser($UserName, $Password)
    {
        // destroy user session
        session_destroy();
        session_regenerate_id();
    }

    private function setUserLogged()
    {
        session_regenerate_id();

        // set user logged
        $_SESSION['UserLogged'] = TRUE;
    }

    public function isUserLogged()
    {
        // check if user is logged
        return (isset($_SESSION['UserLogged']) && $_SESSION['UserLogged'] == TRUE );
    }
}

// Test it
$objLogonUser = new oLogonUser();

if( !$objLogonUser->isUserLogged() )
    $objLogonUser->LogOnUser('test', 'test');

echo session_id() . '<BR />';

print_r($_SESSION);

Preferably, you would generate a random salt. Add a function to generate random salt, then be sure to call it in your constructor. If a user discovers you salt, that is a major security hole right now.

In my opinion, it would be best to separate this class into a separate file for easier use in multiple situations. Also, a separate file with useful functions like generating salt could be useful.

Your code is relatively secure, but you need a random salt generator. Otherwise, your code looks good.


Better multithreading with network IO and database queries I'm writing a script that:

fetch a list of urls from a db (about 10000 urls)
download all the pages and insert them into the db
parse the code
if(some condition) do other inserts into the db

I have a Xeon quad-core with hyper-threading, so a total of 8 thread available and I'm under Linux (64 bit).

I'm using cStringIO as buffer, pycurl to fetch the pages, BeautifulSoup to parse them and MySQLdb to interact with the database.

I tried to simplify the code below (removing all the try/except, parsing operation, ...).

import cStringIO, threading, MySQLdb.cursors, pycurl

NUM_THREADS = 100
lock_list = threading.Lock()
lock_query = threading.Lock()

db = MySQLdb.connect(host = "...", user = "...", passwd = "...", db = "...", cursorclass=MySQLdb.cursors.DictCursor)
cur = db.cursor()
cur.execute("SELECT...")
rows = cur.fetchall()
rows = [x for x in rows]  # convert to a list so it's editable

class MyThread(threading.Thread):
    def run(self):
        """ initialize a StringIO object and a pycurl object """

        while True:
            lock_list.acquire()  # acquire the lock to extract a url
            if not rows:  # list is empty, no more url to process
                lock_list.release()
                break
            row = rows.pop()
            lock_list.release()

            """ download the page with pycurl and do some check """

            """ WARNING: possible bottleneck if all the pycurl
                connections are waiting for the timeout """

            lock_query.acquire()
            cur.execute("INSERT INTO ...")  # insert the full page into the database
            db.commit()
            lock_query.release()

            """do some parse with BeautifulSoup using the StringIO object"""

            if something is not None:
                lock_query.acquire()
                cur.execute("INSERT INTO ...")  # insert the result of parsing into the database
                db.commit()
                lock_query.release()

# create and start all the threads
threads = []
for i in range(NUM_THREADS):
    t = MyThread()
    t.start()
    threads.append(t)

# wait for threads to finish
for t in threads:
    t.join()

I use multithreading so I don't need to wait if some requests are going to fail for timeout. That specific thread will wait but the others are free to continue with the other urls.

Here is a screenshot while doing nothing but the script:

It seems that 5 cores are busy while the other are not. So the questions are:

should I create as many cursors as the number of threads?
do I really need to lock the execution of a query? What happened if a thread execute a cur.execute() but not the db.commit() and another thread come in doing the execution + commit with another query?
I read about the Queue class, but I'm not sure if I understood correctly: can I use it instead of doing lock + extract a url + release?
using multithreading can I suffer from I/O (network) bottleneck? With 100 threads my speed doesn't exceed ~500Kb/s while my connection can go faster. If I move to multiprocess will I see some improvement on this side?
the same question but with MySQL: using my code, there could be a bottleneck on this side? All those lock + insert query + release can be improved in some way?
if the way to go is multithreading, is 100 an high number of threads? I mean, too many threads doing I/O requests (or DB queries) are useless because of the mutual exclusion of these operations? Or more threads means more network speed?

1:

Take a look at the eventlet library. It'll let you write code that fetches all the web pages in parallel without ever explicitly implementing threads or locking.

import cStringIO, threading, MySQLdb.cursors, pycurl

NUM_THREADS = 100
lock_list = threading.Lock()
lock_query = threading.Lock()

The purist that I am, I wouldn't make this locks globals.

db = MySQLdb.connect(host = "...", user = "...", passwd = "...", db = "...", cursorclass=MySQLdb.cursors.DictCursor)
cur = db.cursor()
cur.execute("SELECT...")
rows = cur.fetchall()
rows = [x for x in rows]  # convert to a list so it's editable

It would make more sense to do this sort of thing after you've define your classes. At least that would be python's convention.

class MyThread(threading.Thread):
    def run(self):
        """ initialize a StringIO object and a pycurl object """

that's pretty much the most terrible description of this function I could have come up with. (You seem to be thinking of that as a comment, but by convention this should be a docstring, and describe the function)

        while True:
            lock_list.acquire()  # acquire the lock to extract a url
            if not rows:  # list is empty, no more url to process
                lock_list.release()
                break
            row = rows.pop()
            lock_list.release()

It'd be a lot simpler to use a queue. It'd basically do all of that part for you.

            """ download the page with pycurl and do some check """

            """ WARNING: possible bottleneck if all the pycurl
                connections are waiting for the timeout """

            lock_query.acquire()
            cur.execute("INSERT INTO ...")  # insert the full page into the database
            db.commit()
            lock_query.release()

It'd be better to put this data in another queue and have a database thread take care of it. This works, but I think the multi-queue approach would be cleaner.

            """do some parse with BeautifulSoup using the StringIO object"""

            if something is not None:
                lock_query.acquire()
                cur.execute("INSERT INTO ...")  # insert the result of parsing into the database
                db.commit()
                lock_query.release()

Same here. Note that there is no point in python of trying to split up processing using threads. The GIL means you'll get no advatange.

# create and start all the threads
threads = []
for i in range(NUM_THREADS):
    t = MyThread()
    t.start()
    threads.append(t)

# wait for threads to finish
for t in threads:
    t.join()


Outputting Fibonacci numbers My program gets some input values and outputs Fibonacci values.

Generally, I'm adding single numbers from the right to left, and I'm saving it in the sum value and get rest of it and add to string. (The value of "second" will always be larger than "first").

Is there some way to speed up my code?

#include <iostream>
#include <string>
using namespace std;

string addition(string &first, string &second);

int main()
{
      unsigned int value;
      cin>>value;
      string first="1";
      string second="1";
      string helper;

      if(value==1)
          cout<<"1"<<endl;
      else if(value==2)
          cout<<"1"<<endl;

      for(int i=2; i<value;i++)
      {
            helper=addition(first,second);
            first=second;
            second=helper;
      }
      cout<<helper<<endl;
      return 0;
 }

string addition(string &first, string &second)
{
      string finalValue;
      short moveValue=0;
      unsigned int maximum=0, minimum=0;
      short sum;
      char app;

      maximum=second.length();
      minimum=first.length();

      for(int i=maximum-1;i>=0;i--)
      {
             if(minimum)
             {
                  minimum--;
                  sum=first[minimum]-48+second[i]-48+moveValue;
                  moveValue=sum/10;
                  app=sum%10+48;
                  finalValue=app+finalValue;
             }
             else
             {
                  sum=second[i]-48+moveValue;
                  moveValue=sum/10;
                  app=sum%10+48;
                  finalValue=app+finalValue;
             }
      }

      if(moveValue==1)
      {
           app=moveValue+48;
           finalValue=app+finalValue;
      }
      return finalValue;
}

I see a number of things that you could use to improve your code.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.  Know when to use it and when not to (as when writing include headers).

Avoid introducing unneeded variables

The helper string is not necessary.  Instead, you could rework your addition routine so that it calculates first+=second and then swaps first and second.  In that way, second would always contain the final value.  This would also help with the next suggestion.

Avoid introducing unneeded tests

With a little judicious rewrite, your main function can eliminate the special case testing to see if the terms value is 1 or 2.  I'd rewrite it like this:

int main()
{
    unsigned int maxterm;
    std::cin >> maxterm;
    std::string first="1";
    std::string second="1";

    for(unsigned i=2; i < maxterm; ++i) {
        fibnext(first,second);
    }
    std::cout << second << std::endl;
}

Note that I've also renamed a variable from value (which is not very descriptive) to maxterm and the routine from addition (which is descriptive but very general) to fibnext which more accurately suggests that it's calculating the next Fibonacci term.

Simplify your addition

There are a few ways to simplify the addition, but this is perhaps the most straightforward:

std::string &fibnext(std::string &first, std::string &second)
{
    char carry = 0;

    // make strings the same length by 0-padding first
    first.insert(0, second.length() - first.length(), '0');
    for(int i=first.length()-1; i >= 0; --i)
    {
        first[i] += second[i] - '0' + carry;
        if (first[i] > '9') {
            first[i] -= 10;
            carry = 1;
        } else {
            carry = 0;
        }
    }
    if (carry)
        first.insert(0, 1, '1');

    std::swap(first,second);
    return second;
}

Results

The effects of these changes are to reduce the number of times that strings are copied and constructed.  In fact, only two std::string objects are created and then expanded as needed.  Each time a digit is inserted at the beginning of a string, it (conceptually, anyway) needs to allocate a new space that's large enough, copy the string into that new memory, and then free the old space.  However, std::string implementations are often smart enough to allocate larger-than-immediately-needed chunks to help with exactly this type of operation.

I measure it as being almost 30x faster than the original code (0.10 seconds vs 2.88 seconds on my 64-bit Linux box) when calculating the 10,000th Fibonacci number.


Return boolean for navigation (nav) links before and after clicking them I want to write a function to check if some nav links exist or not. However, those nav links are under one of the listbox icons so I need to click the icon
before and after to check if the element exists. The following code works. Nonetheless, is there a better way for writing this?

def func(locator, name):
    if self.driver.find_element_by_xpath('{}{}/i'.format(locator, '[@aria-expand="false"]')):
        self.driver.find_element_by_xpath(locator).click()
    try:
        est = True
        self.driver.find_element_by_xpath('{}//a[text()="{}"]'.format(locator, name)).
    except NoSuchElementException:
        est = False
    if self.driver.find_element_by_xpath('{}{}/i'.format(locator, '[@aria-expand="true"]')):
        self.driver.find_element_by_xpath(locator).click()

    return est

You don't have to construct XPath expressions via string formatting at all. You can locate the element and then find an inner child element by calling the find_element_by_*() methods on a parent element, making the search "context-specific".

And, there is a better locator to find the links by text - "by link text".

Also, to check if the nav listbox is expanded or not, you can use .get_attribute() method to extract and check the value of the aria-expand attribute.

Improved code:

def func(locator, name):
    navigation_box = self.driver.find_element_by_xpath(locator)

    # expand the listbox if not expanded
    is_expanded = navigation_box.get_attribute("aria-expand") == 'true'
    if not is_expanded:
       navigation_box.click()
       is_expanded = True

    link_exists = len(navigation_box.find_elements_by_link_text(name)) > 0

    # collapse the listbox if expanded
    if is_expanded:
        navigation_box.click()

    return link_exists

You should additionally think of a better name for the function and add a docstring explaining what the function does.