Generate Buildings in City I am developing a game, its map is based on a 2D city. The city is being generated as string, # are streets, O are buildings.

Here is an example:

#######
#OOOOO#
#OOOOO#
#######
#OOOOO#
#OOOOO#
#######

Now I am replacing the Os with other buildings, by replacing the character with an other. The other buildings are defined in a dictionary, where the identifying character is the key and the value is a number between 1 and 100 which is the propability of the building to replace an O.

Currently I am doing it like this:

// Turn city into charArray
char[] cityChars = renderedCity.ToCharArray();

// Cycle through all KeyValuePairs, ordered by Value ascending
foreach (KeyValuePair<char, byte> kvp in specialBuildings.OrderBy(key => key.Value))
{
    Random r = new Random(seed);
    // Do the following for all normal buildings in the city
    // If a random seeded number is smaller or equal to the propability of the spawn, change the building into the new one
    cityChars = cityChars.Select(c => c == 'O' ? (r.Next(0, 100) <= kvp.Value ? kvp.Key : c) : c).ToArray();
}

Is there a more elegant way to do this? In the best case, this would be achieved by using no loops at all, but I am struggling with LINQ a bit.

I think even the simplest solutions can and should be made modular and SOLID. Let's try to refactor your game to give you an idea how to begin...

We start with a city. A city can have different squares where a square is just a simple character/symbol.

class City : IEnumerable<Square>
{
    private readonly List<Square> _squares = new List<Square>();
    public void Add(Square square)
    {
        _squares.Add(square);
    }
    public IEnumerator<Square> GetEnumerator() => _squares.GetEnumerator();
    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
}

Each square has a position and a symbol. It's abstract so we can derive more concrete types from it.

abstract class Square
{
    protected Square(string symbol, int x, int y)
    {
        Symbol = symbol;
        X = x;
        Y = y;
    }
    public string Symbol { get; }
    public int X { get; }
    public int Y { get; }
    public override string ToString() => Symbol;
}

You seem to have two types currently. A building and a street so here they are:

abstract class Square
{
    protected Square(string symbol, int x, int y)
    {
        Symbol = symbol;
        X = x;
        Y = y;
    }
    public string Symbol { get; }
    public int X { get; }
    public int Y { get; }
    public override string ToString() => Symbol;
}

interface IUpgradable
{
    void Upgrade();
}

abstract class Building : Square, IUpgradable
{
    protected Building(string symbol, int x, int y, int level)
    : base(symbol, x, y)
    {
        Level = level;
    }
    public int Level { get; private set; }
    public void Upgrade()
    {
        Level++;
    }
    public override string ToString() => "O";
}

class School : Building
{
    public School(int x, int y, int level) : base("H", x, y, level) {}
}

class Hospital : Building
{
    public Hospital(int x, int y, int level) : base("S", x, y, level) {}
}

class Street : Square
{
    public Street(int x, int y) : base("#", x, y) { }
    public override string ToString() => "#";
}

Buildings can be upgraded thus the Upgrade method which increases its level. Each upgradable type implements the IUpgradable interface then so you can even filer on that.

I don't know how you generate your map but for the sake of this example I did it by hand:

Here's a simple city:

// ###
// #H#
// ###
var city = new City
{
    new Street(0, 0), new Street(1, 0),new Street(2, 0),
    new Street(0, 1), new Hospital(1, 1),new Street(2, 1),
    new Street(0, 2), new Street(1, 2),new Street(2, 2),
}

Next we want to upgrade some buildings (I might have the condition for upgrading wrong but it should just demostrate the general idea)

Random r = new Random(seed);

// helper encapsulating the can-upgrade logic
var canUpgradeBuilding = new Func<Building, bool>(b => b.Level <= r.Next(0, 100));

// upgrading buildings
foreach (var b in city.OfType<Building>().Where(canUpgradeBuilding))
{
    b.Upgrade();
}


Dr. Nim game in Java This video should explain how the game works, but in simple form, it's a game where you enter a number between 1 and 3, This number is than removed from 12, in doing so the computer will then pick a number between 1-3 based on what number you have picked, if you choose to remove 1 marble it should say that it has removed 3 marbles. This works because there are 3 lots of 4 in 12. And the maths tells it to remove what number you have chosen from 4. Because there are 3 lots of 4, you can never win, which is the whole point of the game.

Is there any way I could improve on the code?

 import java.util.Scanner;

    public class Main {

        public static int input;
        public static int marble = 12;
        public static int comFirst = (int) (Math.random() * 3 + 1);

        @SuppressWarnings("resource")
        public static void main(String[] args) throws InterruptedException {

            System.out.println("There Are 12 Marbles, You Can Only Remove Up To 3 Marbles At A Time");
            System.out.println("Your Goal Is To Remove The Last Marble, Marble 1");
            System.out.println("To Remove A Marble Either Enter 1, 2 or 3");
            System.out.println("");

            while (true) {

                if (marble == 1) {
                    System.out.println("There Is " + marble + " Marble Remaining");
                } else {
                    System.out.println("There Are " + marble + " Marbles Remaining");
                }

                Scanner scanInput = new Scanner(System.in);
                input = scanInput.nextInt();

                if (input > 3 || input < 1) {
                    System.out.println("You Can Only Remove 3 Marbles At A Time");
                } else {

                    marble = marble - input;

                    Thread.sleep(250);
                    int cpuAnswer = 4 - input;
                    if (cpuAnswer == 1) {
                        System.out.println("The Computer Has Removed " + cpuAnswer + " Marble");
                    } else {
                        System.out.println("The Computer Has Removed " + cpuAnswer + " Marbles");
                    }
                    if (marble - cpuAnswer == 0) {
                        System.out.println("There Are No Marbles Remaining, The Computer Has Won!!");
                        System.out.println("The Computer Has Won");
                        return;
                    }
                    if (marble - input == 0) {
                        System.out.println("There Are No Marbles Remaining, You Have Won!!");
                        return;
                    }
                    marble = marble - cpuAnswer;

                }
            }
        }

    }

Break early instead of nested if

           if (input > 3 || input < 1) {
               System.out.println("You Can Only Remove 3 Marbles At A Time");
           } else {
               ....
           }

The above can be rewritten as a simple if statement with continue, this prevent the indention from creeper up at the side of the screen.

           if (input > 3 || input < 1) {
               System.out.println("You Can Only Remove 3 Marbles At A Time");
               continue;
           }
           ....

Placing the scanner outside the loop

Scanners use internal buffering to buffer its inputstream, this mean they may consume large amounts, instead of only the part they need for the the concurrent operation. When placing the scanner outside the loop, this also allows for the player to put in the moves much quicker after each other, an still having the program pick up every move.

Place Scanner scanInput = new Scanner(System.in); before while(true){, and wrap the scanner in a try with resources like this:

try(Scanner scanInput = new Scanner(System.in)){
    while(true) {
        ....
    }
}

This also allows you to get rid of @SuppressWarnings("resource").

Proper english doesn't have all words title case

(all println statements)

SOme people have problems with reading sentences with every word uppercase, use the english grammar rules, make only names and the first word of a sentence starting with a uppercase.

Check the state of scanInput.hasNextInt()

You should check the state of scanInput.hasNextInt() before every number is get, this method can return false if the program is accessed by the command line and the command line return EOF, this can be triggered with ctrl + D on linux. The best way to do this is replacing the true in your while loop with the check.

Unused variables

public static int comFirst = (int) (Math.random() * 3 + 1);

At the moment, you have 1 unused variable. Either you will be using this variable for a future expansion, or this variable is left while you were working on the application.

You have won case triggers after the computer move

This case triggers after the computer has done its move. If you really planning on the player to win sometime, you should move this case before the computer makes a move.

Win cases too strict

if (marble - cpuAnswer == 0) {
if (marble - input == 0) {

While with the current implementation of your application, all wins are perfect and ending with exactly 0. If you update your implementation of the computer or player in the future, this may pose a problem in the future.

if (marble - cpuAnswer =< 0) {
if (marble - input =< 0) {

The final result would look like this:

import java.util.Scanner;

public class Main {

    public static int input;
    public static int marble = 12;

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

        System.out.println("There Are 12 Marbles, You Can Only Remove Up To 3 Marbles At A Time");
        System.out.println("Your Goal Is To Remove The Last Marble, Marble 1");
        System.out.println("To Remove A Marble Either Enter 1, 2 or 3");
        System.out.println("");

        try (Scanner scanInput = new Scanner(System.in)) {
            while (scanInput.hasNextInput()) {

                if (marble == 1) {
                    System.out.println("There Is " + marble + " Marble Remaining");
                } else {
                    System.out.println("There Are " + marble + " Marbles Remaining");
                }

                input = scanInput.nextInt();

                if (input > 3 || input < 1) {
                    System.out.println("You Can Only Remove 3 Marbles At A Time");
                    continue;
                }
                marble = marble - input;
                if (marble - input =< 0) {
                    System.out.println("There Are No Marbles Remaining, You Have Won!!");
                    return;
                }
                Thread.sleep(250);
                int cpuAnswer = 4 - input;
                if (cpuAnswer == 1) {
                    System.out.println("The Computer Has Removed " + cpuAnswer + " Marble");
                } else {
                    System.out.println("The Computer Has Removed " + cpuAnswer + " Marbles");
                }
                if (marble - cpuAnswer =< 0) {
                    System.out.println("There Are No Marbles Remaining, The Computer Has Won!!");
                    System.out.println("The Computer Has Won");
                    return;
                }

                marble = marble - cpuAnswer;
            }
        }
    }
}

Splitting the main method

By splitting the main method you archieve better code reuse.

@TheCoffeeCup has written an excellent answer for this part of the code.


Class creating dropdown options from MySQL data for multiple scripts I created this class to handle some dropdowns that I'd been creating using a squirrelly combination of HTML, jQuery, and AJAX. This works much better in the one script that I've deployed it in:

namespace appwpclass;

class PopulateDropdown
{
    public $skillArray;
    public $grpArray;
    public $facArray;
    /**
     * PopulateDropdown constructor.
     */
    public function __construct() {
        $skillArray[] = $this->skillArray;
        $grpArray[] = $this->grpArray;
        $facArray[] = $this->facArray;
    }

    public function skillSelect($category){
        //query to db to get skills
        return $skillArray;
    } //end function

    public function groupSelect($category, $mem_type, $selected_group=NULL){

// query to get list of clients -- it is allowed to be NULL in which case ALL active clients will be returned
        return $grpArray;
    } //end function

    public function facilitySelect($selected_group){
        //query to get list of facilities associated with a $selected_group, which is a required field.
            return $facArray;

    } //end function

} //end class

Now, in my admittedly "procedural" PHP script (which I'm trying to refactor), I have "required" this Class:

require '../appwpclass/PopulateDropdown.php';

And in that script, I have these statements in order to populate the various dropdowns:

$dropdown = new appwpclass\PopulateDropdown();
$skillsArr = $dropdown->skillSelect($category);
$groupArr = $dropdown->groupSelect($category, $memType, $selGroup);
$facilityArr = $dropdown->facilitySelect($selGroup);

And then I use those arrays in filter dropdown HTML, as in this example:

<select name="selGroup" id="selGroup" size="1" class="textfield" onchange="submit()">
 <option value="0">--Any--</option>
<?php
foreach( $groupArr as $group_id=>$group_name ) {
    $selected = ( $group_id == $selGroup ) ? ' selected' : '';
    ?>
    <option value="<?php echo $group_id;?>"<?php echo $selected;?>><?php echo $group_name;?></option>
<?php } ?>

This all works fine in this one script, but I have a lot of scripts that also use one, two, or all of these dropdowns.
So I thought about just using them in a "require" somewhere in each script, but I'm thinking there has to be a better OOP way.
Maybe I should create another class and instantiate PopulateDropdown in that class's constructor? Or in another method in that other class? And then have a separate method for each of those statements, e.g. a method for

 $skillsArr = $dropdown->skillSelect($category)

, etc.. I guess if I did that, I'd need to "pass" the 3 properties that are required by one or more of these existing methods.

Would anyone please provide me some direction on this? I'm pretty new to OOP and I'm starting to use Laravel but I'm nowhere near ready to refactor this enterprise system yet.

If I used a new class, as I suggested, then obviously I'd require that class or use that namespace in my other scripts. Just seems like that would be more concise and less prone to errors.

For such a use case a class would be rather useless. A class is used to encapsulate the stuff that belongs to just a single matter. Here, there is nothing in common between these functions.

If you want to use a class, consider creating a class related to Skills, with one of its methods responsible for returning a list of skills.

but if you want to just refactor your code, a set of functions will do as well. Make your PopulateDropdown.php as

function skillSelect($category){
    //query to db to get skills
    return $skillArray;
} //end function

function groupSelect($category, $mem_type, $selected_group=NULL){

// query to get list of clients
    return $grpArray;
} //end function

function facilitySelect($selected_group){
    //query to get list of facilities associated with a $selected_group
        return $facArray;

} //end function

and then

require '../appwpclass/PopulateDropdown.php';
$skillsArr = skillSelect($category);
$groupArr = groupSelect($category, $memType, $selGroup);
$facilityArr = facilitySelect($selGroup);

So you will have the desired functionality without a single class.

You may also want to create another helper function, to reduce the amount of duplicated code,

function show_option($array, $selected) {
    foreach( $array as $id => $name ) {
        $selected = ( $id == $selected ) ? ' selected' : '';
        ?>
        <option value="<?= $id ?>"<?= $selected ?>><?= $name ?></option>
        <?php
    }
}

which will make your HTML much cleaner

<select name="selGroup" id="selGroup" size="1" class="textfield" onchange="submit()">
    <option value="0">--Any--</option>
    <?php show_option($groupArr, $selGroup); ?>

  So I thought about just using them in a "require" somewhere in each script, but I'm thinking there has to be a better OOP way

Well, to use some function or a class you must require them, this way or another. Although classes could be autoloaded, you have to require the autoloader definition anyway. So, although there are other ways, your current approach is good enough.


Recursive function that reverse the words in a string I have been working on a problem that asks me to a write recursive function that can reverse the words in a string. For example if the string is "cat is running", then the result should be "running is cat".

I already completed the problem, but I feel like my implementation is really bad. Does anyone know any other way to solve this problem? Are there other ways to improve my code?

#include <stdio.h>
#include <string.h>
void reverse(char* string, int count);

int main()
{
   char string[] = "cat is running";

   //result will be running is cat

   reverse(string, -1);
   printf("%s\n", string);
}

void reverse(char* string, int count) {
   if (count == 0) {
       return;
   }

   int first = 0;
   int second = 0;
   int length = 0;

   for (int i = 0; i < strlen(string); i++) {
       if(string[i] == ' ' && first == 0) {
           first = i;
           length++;
           if (length == count) {
              break;
           }
       } else if(string[i] == ' ') {
           second = first+1;
           first = i;
           length++;
           if (length == count) {
               break;
           }
       }
   }

   if(first == 0 && second == 0) {
       return;
   }
   int flag = 0;
    for(int i = first; i >= 0; i--) {
       char temp = string[i];
       for(int k = i; k < strlen(string) - 1; k++) {
           if(flag > 2 && k == (strlen(string) - flag)) {
               k = strlen(string);
           } else {
               string[k] = string[k+1];
           }
        }
    if(i == second) {
        i = 0;
    }
    if (flag == 0) {
        string[(strlen(string) - 1) - flag] = temp;
        flag++;
    } else {
        string[(strlen(string) - flag)] = temp;
        flag++;
    }
}

count = length;
reverse(string, count - 1);
}

The program below is just for fun.

Since the program asks for recursion, then in principle, recursion should be the sole iteration technique. Assuming that the requirement is to only implement a single function, and that string is '\0' terminated, the following program illustrates how this is possible.

Basically, the technique is to use the value of count control the mode of the reverse function. There are three modes: initial call, reverse each word, and reverse string.

A positive count signals the initial call, and count represents the length of the string. On initial call, the program follows the algorithm recipe of: first reverse the input string, then reverse each word in the input string.

A count value of zero causes the function to find the beginning and end of the first word in the string, reverse it, and recursively repeat the process beginning after the just reversed word.

A negative count causes the function to reverse the string assuming its length is -(count + 1). The off by one length is used to make sure the string reversal case does not run into the word reversal case. Thus, a -1 count represents an empty string.

void reverse (char *string, int count) {
    if (string == NULL || *string == '\0') return;
    // positive count := initial call
    if (count > 0) {
        reverse(string, -count-1); // reverse the whole string
        reverse(string, 0);        // reverse each word in whole string
        return;
    }
    // zero count := reverse each word in the string
    // skip over whitespace, then find end of word, reverse word, repeat
    if (count == 0) {
        char *p = string;
        int sl = strspn(p, " \t");
        int wl = strcspn(p + sl, " \t");
        reverse(p + sl, -wl-1);    // reverse the found word
        reverse(p + sl + wl, 0);   // reverse each word in rest of string
        return;
    }
    // negative count := reverse string of length -(count + 1)
    // reverse the string by swapping first and last, then shrink
    count = -(count + 1);          // fix-up count value
    if (count < 2) return;
    char t = string[0];
    string[0] = string[count-1];
    string[count-1] = t;
    reverse(++string, ---count);
}

Note the lack of looping structures within the function. All loops are accomplished with recursion or hidden by standard library functions strspn and strcspn.


Performing add/delete/count operations on a list Problem Statement (HackerRank)

  You have a list of integers, initially the list is empty.

  You have to process Q operations of three kinds:

  add s: Add integer s to your list, note that an integer can exist more
  than one time in the list
  del s: Delete one copy of integer s from the list, it's guaranteed
  that at least one copy of s will exist in the list.
  cnt s: Count how many integers a are there in the list such that a AND
  s = a, where AND is bitwise AND operator

  Input Format

  First line contains an integer Q.  Each of the following Q lines
  contains an operation type string T and an integer s.

  Constraints

  \$1 \le Q \le 200000\$
  \$0 \le s \lt 2^{16}\$

  Output Format

  For each cnt s operation, output the answer in a new line.

  Sample Input

7
add 11
cnt 15
add 4
add 0
cnt 6
del 4
cnt 15

  Sample Output

1
2
2

  Explanation

  For first line, we have 15 AND 11 = 11 so the answer is 1

  For second line, 6 AND 0 = 0 and 6 AND 4 = 4 so the answer is 2

  For third line, 4 has been deleted and we have 15 AND 11 = 11 and 15
  AND 0 = 0 so the answer is 2

My working code:

operations = int(raw_input())
current = 1
lst = []
while current <= operations:
    count = 0
    input_ = raw_input().split()
    operation = input_[0]
    num = int(input_[1])
    if operation == 'add':
        lst.append(num)
    elif operation == 'cnt':
        for number in lst:
            if number & num == number:
                count += 1
        print(count)
    elif operation == 'del':
        lst.remove(num)
    current += 1

You don't exploit the nature of the question.

  Constraints

  \$1 \le Q \le 200000\$
  \$0 \le s \lt 2 ^ {16}\$

Why are these important? It's so you can achieve greater performance, for a price in memory.

You know that there is \$2 ^ {16}\$ possible numerical inputs,
and there is a maximum of 200000 inputs.

If the last input is count, and all the rest are add,
you will be recounting numbers a lot. Leading to a dramatic performance loss.

First you would want to prevent recounting if possible.
One way to do this is to make a dictionary to count the occurrences of numbers.
However due to constant type changes you can just use a list instead.

lst = [0] * (2 ** 16)

This takes more time than using lst = [], but it will pay off.

If we look at the performance of your 3 functions.
I use the information from Python's time complexity page.

Add

lst.append(num)

This is average case \$O(1)\$, worst case \$O(n)\$, (not amortized worst case).

lst is internally stored as an array,
and so if you grow past the bounds all the data must move.

And so I will say this is \$O(n)\$.

Del

lst.remove(num)

This is average case \$O(n)\$, worst case \$O(n)\$.

And so this is \$O(n)\$.

cnt

for number in lst:
    if number & num == number:
        count += 1
print(count)

And again this is \$O(n)\$.

Overall

You can have 200000 calls to functions that are \$O(n)\$.
If you ask me that's not good on performance.

You can easly make add and del \$O(1)\$.

If you use lst = [0] * (2 ** 16).

if operation == 'add':
    lst[num] += 1
elif operation == 'del':
    lst[num] -= 1

It's like using a default-dictionary where the index is only numbers.
However this currently will have the drawback of cnt always being \$O(2 ^ {16})\$.

If you wish to fix that you can use a set, to store the numbers, so it is \$O(n)\$.

Here is a solution that passes a few more of the tests. It does not pass them all.

operations = int(raw_input())
nums = set()
storage = [0] * (2 ** 16)

for _ in xrange(operations):
    input_ = raw_input().split()
    operation = input_[0]
    num = int(input_[1])

    if operation == 'add':
        nums |= {num}
        storage[num] += 1
    elif operation == 'cnt':
        print(sum(
            storage[number]
            for number in nums
            if (number & num) == number
        ))
    elif operation == 'del':
        storage[num] -= 1

This uses a similar solution as yours, my lst is nums.
However, I aimed at obtaining \$O(1)\$ in both add and del.

I can't complete the problem.
The way Python programmers that solved this use [0] * (2 ** 16) and binary logic I don't know. Which make all the functions \$O(256)\$.


Refactoring my problem-solution to reduce complexity I need some help refactoring my solution to the problem below:
Problem Statement

An Association of Computer Scientists meets on a weekly basis. There is a certain number of seats in the meeting room and all members know their seats. For example, if there are 10 seats and only 5 members attending, each member will sit on his own seat, and there may be empty seats left between the members, because all have to sit on their 'reserved' seats.
If a member, sitting on seat Xi, starts or joins the discussion, exactly Ti people (after him) will take part of the discussion. For every member, we know his seat and his voice. Only those members who are within the reach of the previous member's voice can hear him and participate in the discussion. For example, if we are given the seat and the voice of a member, and they are 1, 7 (respectively), only people seated on seats within range of 1 to 8 (inclusive) can hear him and participate in the discussion.

Example Data

Input:
N - number of members attending
Xi - seat of member i (for all i=1,2,...N), Li - his voice
Output:
The number of people participating in the discussion after member
i.
Example Input:
5

1 7

2 3

8 7

10 5

100 2

Output:
4

1

2

1

1

And now, here's my solution:
#include <iostream>
#include <vector>
using namespace std;

int main()
{
        int n;
        cin >> n;
        vector<pair<int, int> > c;
        for (int i=0; i<n; i++) {
                int a, b;
                cin >> a >> b;
                c.push_back(make_pair(a,b));
        }
        for (int i=0; i<n; i++) {
                int a, b, r=1;
                a = c[i].first;
                b = c[i].second;
                for (int j=i+1; j<n; j++) {
                        if (a+b >= c[j].first) {
                                if (c[j].first + c[j].second > a+b) {
                                        a = c[j].first;
                                        b = c[j].second;
                                }
                                r++;
                        }
                }
                cout << r << endl;
        }

        return 0;
}

The problem with this solution is in its quadratic O-complexity. Is it possible to reduce it somehow? I've been thinking of the divide and conquer approach, but I'm fairly new to the concept, and I have no idea if that's the right approach for this problem, and how I may implement that.

I plan to think up a better algorithm later and post an answer on that, but for now, I'd like to offer a critique of your existing code.

Typically using namespace std; should be avoided in favor of either using the std:: qualifier or just importing the identifiers you plan to use at a function level. (E.g. inside of main, doing something like using std::cout;.)

You should verify that your input reading succeeds:

if (!(cin >> n)) { return EXIT_FAILURE; }

(And something similar in your loop.)

On the off chance that something goes wrong, you'd rather bail out with an exit code than do nothing. If you really wanted, you could even put out an error message (though for contest-style problems like these that would be more for your use than the end purpose).

It's a good habit in C++ to use ++i whenever you can as opposed to i++. For an int or any other built in, it doesn't particularly matter, but for complex types, ++i can avoid an expensive copy.

The most common manifestation of this is iterators. ++i increments an iterator in place. i++ creates a copy of the iterator, increments the original one and then returns the copy. If you're not actually capturing the copy, there's no reason to create one.

Try to give variables more useful names. Imagine coming back to this 6 months from now, or imagine if I hadn't read the problem statement. I would be left wondering what in the world all these 1 character variable names are. Descriptive variables assist the reader in more quickly comprehending what code is doing.

Rather than pushing back pairs over and over again, I would take advantage of vector's values constructor and std::pair's default constructor:

std::vector<std::pair<int, int> > attendees(numAttendees);
for (int i = 0; i < numAttendees; ++i) {
    if (!(cin >> attendees[i].first >> attendees[i].second)) {
        return 1;
    }
}

Not only with this have a bit better performance (since it doesn't have to keep resizing the vector), it is a bit cleaner. (If the pair default ctor actually turns out to be expensive, you could use reserve() to avoid the vector resizing but still allow you to use make_pair to avoid the default construction of each pair.)

Is your input guaranteed to be sorted? If not, you need to either sort or scan over the entire data set rather than starting at i + 1.

This comes down to opinion, but I find space around operators (e.g. i+1 as i + 1) much easier to read. For example for (int i = 0; i < n; ++i) vs for(int i=0; i<n; ++i).

You need to include utility for pair, I believe.

Whenever you have a deep nesting of loops, it's typically a sign that something should be pulled into a function. For example, I would pull the for(int j = i+1; ...) part of the loop into a function called countInvolvedAttendees (or something hopefully better named -- I'm drawing a blank).

std::pair is a double edged sword. It's very, very convenient for holding any pair, but it's generality is also it's downfall. In particular, it basically murders decent naming. When I see a std::pair<int, int>, I have no idea what in the world the two ints are at all. Compare that to a Attendee that has a seatPosition and voiceDistance, and suddenly the meanings are very clear. Unfortunately though, creating a custom struct or class for this seems a bit overkill.

What I'm trying to get across is please be mindful of the problem of std::pair's vagueness. In all but the simplest of circumstances, it's typically better to create something with proper names. Otherwise, it's too easy to get caught up in first and second and flip them, forgot what one means, and so on.

std::endl is a common case of I don't think that does what you think it does.

In particular, std::endl is equivalent to os << '\n' << std::flush;.

For lots of outputting, this constant flushing can be shockingly slow. In cases like this, where you're outputting a giant batch and not responding to human input as it comes, I would use '\n' instead.

Since this is a lot to take in, I might write out a revised version in a bit when I get some more time :).


How can I improve this PHP MySql Rank generation function? How can I improve this PHP MySql Rank generation function?
(Mostly looking to improve speed, but any feedback is very welcome)

Right now I keep stats and ranks in 2 separate tables. The only problem with this is I don't know how combine the select and update queries to possibly increase speed by cutting the PHP processing of the results and the update out of it. Also this function is very time consuming. It takes about 5-10 minutes to process 300,000+ ranks. This isn't really too bad, except I have to do this 50+ times. I do like how it doesn't lock up my database because getting the ranks only takes a few seconds, and all the updates are done individually, which leaves room for other queries to process instead of locking for 5-10 minutes each time.

function get_ranking($rank, $rank_order, $DBH, $test_mode=true)
{
    $test_mode == true ? $limter = ' LIMIT 50' : $limter = '';

    $query0 = 'SET @rank=0';
    $query1 = 'SELECT @rank:=@rank+1 AS rank, player_id FROM player_stats ORDER BY '.$rank_order.$limter;

    try
    {
        $DBH->exec($query0);
        $qh = $DBH->prepare($query1);
        $qh->setFetchMode(PDO::FETCH_ASSOC);

        if ($qh->execute())
        {
            while ($result = $qh->fetch())
            {
                $query2 = 'UPDATE `player_ranks` SET '.$rank.' = \''.$result['rank'].'\' WHERE `player_id` =\''.$result['player_id'].'\'';

                $qh2 = $DBH->prepare($query2);
                $qh2->execute();
            }

            return true;
        }

        return false;
    }
    catch (PDOException $e)
    {
        save_PDO_error_to_file($e);
        return false;
    }
}

Thanks for any advice/feedback/help!

Something you might try is keeping everything inside the database. Using the SELECT, create a temporary table:

CREATE TEMPORARY TABLE tmp_ranks (rank int auto_increment primary key, player_id int)
AS SELECT NULL AS rank, player_id AS player_id FROM player_stats
ORDER BY '.$rank_order.$limter

This should give you a table that looks like

| rank | player_id |
+------+-----------+
|    1 | 123456789 |
|    2 | 123456780 |

Now, using an UPDATE with a JOIN, update your player_ranks table:

UPDATE player_ranks, tmp_ranks SET player_ranks.$rank = tmp_ranks.rank
WHERE player_ranks.player_id = tmp_ranks.player_id;

This should run fairly quickly, but given your data size, could take a while. I'd suggest testing it on a non-production box first, in case it locks things up for a good while.

Note that I left your PHP variables inline, so you'll need to fix those.


Display the relative time difference between two dates Goal: To create a function that returns the relative time difference between two given timestamps, using the DateTime class.

Current approach:

function time_elapsed($date1, $date2) {
    $dt1 = new DateTime($date1);
    $dt2 = new DateTime($date2);

    $diff = $dt1->diff($dt2);

    $ret = $diff->format('%y years, %m months, %a days, %h hours, %i minutes, %S seconds');

    $ret = str_replace(
        array('0 years,',' 0 months,',' 0 days,',' 0 hours,', ' 0 minutes,'),
        '',
        $ret
    );

    $ret = str_replace(
        array('1 years, ',' 1 months, ',' 1 days, ',' 1 hours, ',' 1 minutes'),
        array('1 year, ','1 month, ',' 1 day, ',' 1 hour, ',' 1 minute'),
        $ret
    );

    return $ret;
}

As you can see, the function works fine. I'm currently using a couple of str_replace() calls to remove the unnecessary parts from the formatted string, but I don't think that's a very good approach. I'm sure what I have written can be cleaned up and improved, using a different approach, perhaps.

I guess you have a bug here:

$ret = $diff->format('%y years, %m months, %a days, %h hours, %i minutes, %S seconds');

%a should be %d. With %a

time_elapsed("2012-07-08 11:14:15.889342", "2014-09-10 13:15:17.889342");

prints total number of days:

2 years, 2 months, 794 days, 2 hours, 1 minute, 02 seconds

Using %d changes 794 days to 2 days.
Another approach is iterating through of an array of units:

function time_elapsed2($start, $end) {
    $start = new DateTime($start);
    $end = new DateTime($end);

    $interval = $end->diff($start);

    $units = array(
        "%y" => sp("year", "years"),
        "%m" => sp("month", "months"),
        "%d" => sp("day", "days"),
        "%h" => sp("hour", "hours"),
        "%i" => sp("minute", "minutes"),
        "%s" => sp("second", "seconds")
    );

    $result = array();
    foreach ($units as $format_char => $names) {
        $formatted_value = $interval->format($format_char);
        if ($formatted_value == "0") {
            continue;
        }
        $result[] = get_formatted_string($formatted_value, $names);
    }

    return implode(", ", $result);
}

function sp($singular, $plural) {
    return array("singular" => $singular, "plural" => $plural);
}

function get_formatted_string($formatted_value, $names) {
    $result = $formatted_value . " ";
    if ($formatted_value == "1") {
        $result .= $names["singular"];
    } else {
        $result .= $names["plural"];
    }
    return $result;
}

(In production code I'd use constants instead of singular and plural.) The array also helps if you want to translate your page to other languages.


XSS Protection from Query Strings in Java Looking for input on the code below. Generally, the idea is to take a String representing a URL query string such as param1=blah&param2=blah and encode them to protect against possible XSS attack. Using existing utilities (i.e. UrlEncodedUtils) is not an option.

public static Map<String, List<String>> getParamsFromQueryString(String queryString) throws UnsupportedEncodingException {

    Map<String, List<String>> params = new HashMap<>();

    for (String param : queryString.split("&")) {
        String[] pair = param.split("=");
        String key = ESAPI.encoder().encodeForHTML(URLDecoder.decode(pair[0], "UTF-8"));
        String value = "";
        if (pair.length > 1) {
            value = ESAPI.encoder().encodeForHTML(URLDecoder.decode(pair[1], "UTF-8"));
        }

        // skip ?& and &&
        if ("".equals(key) && pair.length == 1) {
            continue;
        }

        List<String> values = params.get(key);
        if (values == null) {
            values = new ArrayList<>();
            params.put(key, values);
        }
        values.add(value);
    }

    return params;
}

In addition to RotoRa's answer:

Instead of

    String[] pair = param.split("=");

you should call

    String[] pair = param.split("=", 2);

Otherwise you will trim base64-encoded data, which may happen in some applications, especially if the client forgets to properly encode the = characters.

Instead of a HashMap, use a LinkedHashMap, since that will keep the parameters in the same order as they appeared in the original URL. This makes the code easier to test (you do have unit tests, I hope) and more predictable in general.


Singly-linked list queue I recently learned the core .NET Queue type uses arrays internally, and that this is actually much more efficient. I've had a go at writing a singly-linked list version of a queue, MyQueue.

Running basic stopwatch times against this type and the .NET type shows about 50ms difference when queuing 1 million items and checking the length.

How can it be improved?

MyQueueNode

namespace MyQ.Core
{
    public class MyQueueNode<T> : IMyQueueNode<T>
    {
        private MyQueueNode<T> _nextNode;

        private T _value;

        public MyQueueNode(T value)
        {
            _value = value;
        }

        public void SetNextNode(ref MyQueueNode<T> nextNode)
        {
            _nextNode = nextNode;
        }

        public MyQueueNode<T> GetNextNode()
        {
            return _nextNode;
        }

        public T GetValue()
        {
            return _value;
        }
    }
}

MyQueue

namespace MyQ.Core
{
    public class MyQueue<T> : IMyQueue<T> where T : class
    {
        private volatile MyQueueNode<T> _headNode, _tailNode;

        public void Enqueue(T item)
        {
            MyQueueNode<T> newNode = new MyQueueNode<T>(item);

            if (_headNode == null)
            {
                _headNode = newNode;
                _tailNode = _headNode;
            }
            else
            {
                _tailNode.SetNextNode(ref newNode);
                _tailNode = newNode;
            }

            _length++;
        }

        public T Dequeue()
        {
            if(_headNode == null)
            {
                throw new QueueEmptyException();
            }

            T value = _headNode.GetValue();

            _headNode = _headNode.GetNextNode();

            _length--;

            return value;
        }

        public T Peek()
        {
            if(_headNode == null)
            {
                return null;
            }

            return _headNode.GetValue();
        }

        private long _length = 0;

        public long Length => _length;
    }
}

Don't use volatile in the below line

    private volatile MyQueueNode<T> _headNode, _tailNode;

Link https://www.infoworld.com/article/3229360/application-development/how-to-use-the-volatile-keyword-in-c.html


Longest Palindromic Substring I wrote the following code for this problem. The problem is to find the longest palindromic substring.
I think it is correct and is working correctly for all the test cases I entered myself. Now, the code is producing a Time Limit Exceeded Error. I know that there exists Manacher's Algorithm which does it in linear time but I thought of the following solution myself and wanted to implement it. I believe that it's time complexity is less as compared to \$\mathcal{O}(n^3)\$ and other \$\mathcal{O}(n^3)\$ solutions have been accepted which makes me feel that I am missing something important.

string = input()

longest = 0
position = 0
string = string.replace('', '^')
length = len(string)

##print(string)

for index, letter in enumerate(string):
##    print()
##    print('Index:', index, 'Letter:', letter)

    stretch = 1

    while string[index-stretch:index+stretch+1] == string[index-stretch:index+stretch+1][::-1]\
          and index - stretch >= 0 and index + stretch < length:
##        print('Matching Strings:', string[index-stretch:index+stretch+1], string[index-stretch:index+stretch+1][::-1], 'Stretch:', stretch)
        stretch += 1

##    print('Not Matched:', string[index-stretch:index+stretch+1], string[index-stretch:index+stretch+1][::-1], 'Stretch:', stretch)

    stretch -= 1

    if stretch > longest:
##        print('Highest Till Now')
        longest = stretch
        position = index - stretch

##print(position, longest)

print(string[position:position+(2*longest+1)].replace('^', ''))

stretch is the extension of the palindrome on either side of its center. longest stores the length of the longest substring found till then. position stores the index of the starting letter of longest. Please ask if you have any doubts.

What I want to know is that am I missing something which is causing the program to lose its efficiency? How can I overcome it? Can you suggest some improvements to the algorithm.

Thanks.

First things first, I'll highlight areas I felt you should change.

Make a function.
Don't litter the program with blackbox debugging 'comments'.
Don't use enumerate if you don't need both index and letter.
It's easier to read one slice rather than two.
You could change your while loop to a for loop, this reduces the amount of statements in the if.

Most of these are pretty simple, and are mostly due to your debugging.
I'll elaborate on 5 as it's the only one that I should need to explain.

If we rearrange both \$\text{index} - \text{stretch} >= 0\$ and \$\text{index} + \text{stretch} < \text{length}\$ to get stretch.
This gets \$\text{index} >= \text{stretch}\$ and \$\text{length} - \text{index} > \text{stretch}\$.
As we know the input are integers we can get both to use the same sign, >.
With \$\text{index} + 1 > \text{stretch}\$.
If we merge them both together we get: \$\text{min}(\text{index} + 1, \text{length} - \text{index}) > \text{stretch}\$.

This basically means you can use the following range:

for stretch in range(1, min(index + 1, length - index)):

You should note, it won't go above the largest possible stretch and so you should only do stretch -= 1 if you break from the loop.

All of this together should get you:

def get_longest_palendrome(string):
    string = string.replace('', '^')
    longest = 0
    position = 0
    length = len(string)
    for index in range(length):
        stretch = 0
        for stretch in range(1, min(index + 1, length - index)):
            text = string[index - stretch:index + stretch + 1]
            if text != text[::-1]:
                stretch -= 1
                break
        if stretch > longest:
            longest = stretch
            position = index
    position -= longest
    return string[position:position + (2*longest + 1)].replace('^', '')

The second I looked at string = string.replace('', '^') I thought this can't be an \$O(n^3)\$ algorithm.
I don't know the actual complexity, but you instantly change the length of string to \$2n + 1\$.
This means your function is more \$O((2n)^3)\$ or \$O(8n^3)\$

I know you normally ignore the coefficient but I think it's quite important to highlight the pitfall on doubling the size of the input.


Various simple JavaScript solutions Please critique my solutions to these simple exercises. I have no programming experience, so the more anal you are, the more I'll learn. I'm teaching myself, this is not for school:

  Write a function translate() that will translate a text into
  "rövarspråket". That is, double every consonant and place an
  occurrence of "o" in between. For example, translate("this is fun")
  should return the string "tothohisos isos fofunon".

var translate = function(x) {
  var string = x.toLowerCase();
  var vowels = ["a", "e", "i", "o", "u", " "];
  var y = "";
  for (i = 0; i < string.length; i++) {
     var current = string.charAt(i);
    if (vowels.indexOf(current) != -1)
    y = (y + (current));
   else
    y = (y + (current + "o" + current));
  }
  return y;
}

  Define a function sum() and a function multiply() that sums and
  multiplies (respectively) all the numbers in an array of numbers. For
  example, sum([1,2,3,4]) should return 10, and multiply([1,2,3,4])
  should return 24.

var sum = function(array) {
  var length = array.length;
  var total = 0;
  for (i = 0; i < length; i++) {
    total += array[i];
  }
  return total;
};

var multiply = function(array) {
  var length = array.length;
  var total = 1;
  for (i = 0; i < length;  i++) {
    total *= array[i];
  }
  return total;
};

  Define a function reverse() that computes the reversal of a string.
  For example, reverse("jag testar") should return the string "ratset
  gaj".

var reverse = function(string) {
  var length = string.length;
  var reversed = [];
  var joined = ("");
  for (i = length; i > 0; i--){
    reversed.push(string.charAt(i-1));
  };
  for (i = 0; i < (length) ; i++){
    joined += (reversed[i]);
  }
  return joined;
}

  Write a function findLongestWord() that takes an array of words and
  returns the length of the longest one.

var findLongestWord = function(array) {
  var elements = array.length;
  var count = 0;
  for (i = 0; i < elements; i++) {
    if (array[i].length > count)
      count = array[i].length;
  }
  return count;
}

  Write a function filterLongWords() that takes an array of words and
  an integer i and returns the array of words that are longer than
  i.

var filterLongWords = function(array, int){
  var length = array.length;
  var longestWords = [];
  for (i = 0; i < length; i++) {
    if (array[i].length > int) {
      longestWords[longestWords.length] = array[i];
    }
  }
  return longestWords;
}

  Write a function charFreq() that takes a string and builds a
  frequency listing of the characters contained in it. Represent the
  frequency listing as a JavaScript object. Try it with something like
  charFreq("abbabcbdbabdbdbabababcbcbab").

var charFreq = function(string){
  var list = {};
  var length = string.length;
  for (var i = 0; i < length; i++) {
  if (string.charAt(i) in list)
    list[string.charAt(i)] += +1;
  else
    list[string.charAt(i)] = 1;
  }
  return list;
}

EDIT: I forgot to mention that rather than use global methods, I tried to as much as possible code for the result I wanted, like reverse, and join, etc.

Here one way to sum a list of integers using ECMAScript 5's functional support

// Sum all values in a list by reducing the list to a single value
array.reduce(function(prev, curr) {
  return prev + curr;
});

Not faster than your approach due to the repeated function calls, but definitely shorter.

Your approach can be further simplified:

// Move everything into the parenthesis of for-loop!
var sum = function(array) {
  for (
    var i = 0, len = array.length, total = 0;
    i < len;
    total += array[i++]
  );
  return total;
};

Not recommended because it's not very readable. I just think it's interesting :)

I'll give you an alternative approach to the "Find length of longest string" problem:

// Sort it by length descending and then get the first
var longestStrLen = array.sort(function(a, b) {
    a.length - b.length;
})[0].length;

Hope these open your eyes to some of the possibilities!

Now, here's an actual review:

var charFreq = function(string){
  var list = {}; // Not a list (implies order). Should name it a dictionary or map
  var length = string.length; // no need to have this be separate line
  for (var i = 0; i < length; i++) {
  if (string.charAt(i) in list)
    // What's with the +1??
    list[string.charAt(i)] += +1;
  else
    // No need to call string.charAt multiple times!
    list[string.charAt(i)] = 1;
  }
  return list;
}

Revised version:

var charFreq = function(string){
  var dict = {};
  for (var i = 0, len = string.length; i < len; i++) {
    var currChar = string.charAt(i);
    if (currChar in dict) {
      dict[currChar] += 1;
    } else {
      dict[currChar] = 1;
    }
  }
  return dict;
}

Hope that helps!


Taking 2 command line inputs and verifying that they are integers, within the correct range of values I'm taking 2 command line inputs, which have to be integers between 2 and 1,000,000. These two values are then passed into the getPrimes function, which is just a Sieve of Eratosthenes algorithm, printing all prime numbers within the range specified by the two inputs. The two inputs, n and k, must be positive integers, where n > k. How can I better optimise my approach to collecting these inputs and verifying them, to decrease execution time?

#include <stdio.h>
#include <stdlib.h>
#include <math.h>
#include <ctype.h>
#define MAXVALUE 1000000

int main(int argc, char *argv[]){

    if (argc != 3){
        printf("Warning: Check number of arguments.\n");
        return 1;
    }

    int i, j;
    for (j = 1; j <= 2; j++){
        for (i = 0; argv[j][i] != '\0'; i++){
            if (!isdigit(argv[j][i])) {
                printf("Warning: Please enter a valid integer.\n");
                return 1;
            }
        }
    }

    int n = atoi(argv[2]), k = atoi(argv[1]);

    if (n > MAXVALUE || k == 1 || k >= n){
        printf("Warning: Bad input.\n");
        return 1;
    }

    // getPrimes(n, k);

    return 0;

}

This is unrelated to performance, but it may still be helpful in general:

You should print those errors to stderr via fprintf():

fprintf(stderr, "Warning: Check number of arguments.\n");

fprintf(stderr, "Warning: Please enter a valid integer.\n");

fprintf(stderr, "Warning: Bad input.\n");

Are you using a pre-C99 version of C?  If not, you should be initializing the loop counter variables within the loop conditions, not outside of them.
It's common to provide the types of expected arguments (starting with whatever gets printed from argv[0]) if this check fails. You could also specify the number of expected arguments so that the user isn't left guessing, in case the objective isn't already clear.

fprintf(stderr, "Warning: Check number of arguments (must be 2).\n");

You shouldn't try to do everything in main(), and it doesn't look like those checks after the arguments check belong there.

Instead, consider providing separate functions for both that get called from main(). They can return a boolean value and, if it's an erroneous one, the program can then terminate from main().
If I'm not mistaken, the for loop is supposed to check both argv[1] and argv[2] for valid data types.

If so, you could either:

Do two separate checks without a loop (to maybe make it a little more cleaner)
If you may consider expanding it for additional arguments, use argc as part of the outer loop sentinel value instead of hard-coding the 2 (which could be seen as a "magic number" instead anyway)


Detect if ie10+ and edge is using inPrivate mode This is an odd case scenario, but I would like to display a message that tells if indexedDB is not supported or if it is removed due to being in private mode (yes ie10+/edge remove indexedDB in private mode, crazy uh?).

I think found a way without doing crazy UA sniffing or documentMode hacks that I think could work.

if(!window.indexedDB && (window.PointerEvent || window.MSPointerEvent))
  document.body.innerHTML = "Can't use indexedDB because you are in private mode"
else if(!window.indexedDB)
  document.body.innerHTML = "Can't use indexedDB because you don't have it"
else
  document.body.innerHTML = "woho"
  // Other browser in private mode can block indexedb
  // but the message gets handled from db.onerror instead

IE/Edge is the only browser that removes indexedDB when using inPrivate mode..?
so this is a way to detect private mode, but then you have to figure out if it is ie10+ or edge.
so ie10 was the latest browser with support for indexedDB
and ie10+edge is the only browser that support pointerEvent [caniuse]

And I assume there is no other browser with support for pointerEvent and not having indexedDB at the same time that could yield wrong message.

What do you think about this? Good, bad? Any suggestions? It would help if you could test if this works in your browser (I don't have Windows).

Instead of blacklisting why not whitelist? If you are really just trying to post a message to the user saying your browser doesn't support something, why not just check if indexedDB is available and then if it isn't post something like:

"IndexedDB is only supported in Chrome 24.0, Firefox 16.0, IE 10/Edge, Opera 15 and Safari 7.1 or higher. It may also not work in certain 'Private' mode browsers. Please upgrade your browser to use the full power of our website."

This way you are avoiding lots of attempts of browser detection (which will more than likely break after browsers implement cross compatibility) and provides action for your user on what they can do next (try a different browser, upgrade, not be in private mode, etc).


Function to return a string legal for a range name I wrote an Excel VBA function that will take a string argument and return a string that is legal for naming a range.  Upon looking through this, if you're wondering what I've been smoking, I should add a disclaimer that I'm new to VBA and I have not yet come across a less restrictive way to obtain this functionality.  And, of course, please feel welcome to modify and use it for your own purposes.

Function Namify(inputName As String) As String
'Takes a string argument and returns a modified string suitable for use as a name

    'Create a string to be modified and returned
    Dim workingName As String
    workingName = inputName

    'Create a string array containing all the valid characters in an Excel name
    Dim validchars() As Variant
    validchars = Array("_", "1", "2", "3", "4", "5", "6", "7", "8", "9", "0", _
        "a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m", _
        "n", "o", "p", "q", "r", "s", "t", "u", "v", "w", "x", "y", "z", _
        "A", "B", "C", "D", "E", "F", "G", "H", "I", "J", "K", "L", "M", _
        "N", "O", "P", "Q", "R", "S", "T", "U", "V", "W", "X", "Y", "Z")

    'Iterate through each character of the string argument and replace that character
    'with an underscore if it's an illegal character.
    Dim i As Long
    For i = 1 To Len(inputName)

        'Is the character illegal? If so replace it in workingName
        If IsError(Application.Match(Mid(inputName, i, 1), validchars, 0)) Then
            workingName = Replace(workingName, Mid(inputName, i, 1), "_")
        End If

    Next i

    'Detect whether the first character in workingName is a number.
    Dim numbers() As Variant
    numbers = Array("1", "2", "3", "4", "5", "6", "7", "8", "9", "0")

    If Not IsError(Application.Match(Mid(workingName, 1, 1), numbers, 0)) Then
        workingName = "_" & workingName
    End If

    'return the string that is now legal to use as a name.
    Namify = workingName

End Function

How I use it:

I have a macro that gets a text name from the user, instantiates a new object, and displays the user-input name in a cell followed by the object's property values in cells to the right. Then the sub calls Namify (user-selected name) to create a prefix for assigning a unique name to all these cells so their values can be updated with changes to the object's properties. The prefix itself is stored as a property of the object for later retrieval.

Some potential problems for this function (feel free to add to this list by editing or asking me to add in a comment):

It could return a name that is also a valid cell address.
It would return the same string from arguments with different illegal characters in the same position. (Example arguments: "User;Name", "User=Name")
It does not return all possible arrangements of valid characters.
It would return any of the one-character strings "C", "c", "R", "r"
There is no limit on the length of the returned string

What I'd like to know:

Is this a reasonable approach for allowing the user to create whatever name they like and then using a version of that name to name objects and ranges?
What ideas do you have to make this function more rigorous or more useful?

Disclaimer: I accepted Mat's Mug's answer because it worked well for me. AlexR's answer does appear to be the ideal way to handle this, however VB regular expressions aren't natively supported on my version of Excel (for Mac 2011). Please have a look at his answer if you're interested in optimizing something like my function for yourself.

Follow Up: With some great answers on the board, I revised this function and posted it in a follow up question.  Please have a look at it before posting additional answers here.

Using a VBScript.RegExp will greatly help clean your algorithm:

Static reg As Object
If reg Is Nothing Then
    Set reg = CreateObject("VBScript.RegExp")
    reg.Pattern = "(?:^(?=\d)|\W)" 'A Digit at the start or any illegal character
    reg.Global = True 'Replace all matches
End If
Namify = reg.Replace(inputName, "_")

This does the same as your entire function. Read more about RegExps here

If you additionally write a function ContainsName(name As String) As Boolean wich traverses ActiveWorkbook.Names and returns True if name is a valid name, you can add an additional feature to generate a valid new name as Mat suggests:

Static trailingNum As Object
Dim number As String
If trailingNum Is Nothing Then
    Set trailingNumm = CreateObject("VBScript.RegExp")
    trailingNum.Pattern = "\d*$"
End If
While ContainsName(Namify)
    number = trailingNum.Execute(Namify)(0).Value
    If Len(number) = 0 Then number = "0"
    Namify = trailingNum.Replace(Namify, CStr(CInt(number) + 1))
End While


Collapse long text (with more/less links) I wrote a little function to compact long text. I am relatively new to JavaScript so I am not sure I wrote it as elegantly as possible. I wrote it to be runnable on the server side (Node.js) as well, so can't use jQuery or such. Can the code be improved?

// change a long text into  "bla bla bla... (more)" where (more) is a link to open the rest of the text
// compact text if over maxWords words (split by ' ', default 25)
//           or if over maxRows lines (split by '
', default 2)
// but avoid compacting if no more than "almostFinished" (default 5) words to the end of the text
compactText = function(text, prefix, maxWords, maxRows, almostFinished) {
    if (!maxWords) {
        maxWords = 25;
    }
    if (!maxRows) {
        maxRows = 2;
    }
    if (!almostFinished) {
        almostFinished = 5;
    }

    var result = [];
    var compacted = false;
    // escape the html code in the text
    text = (text || '').replace(/</g, '&lt;').replace(/>/g,'&gt;').replace(/\r/g, ' ').replace(/\n/g, '
 ')
    var pn = text.split(' ');
    var row = 1;
    for (var word=0; word< pn.length; word++) {
        if (pn[word] === "
") {
            row++;
        }
        if (!compacted && (row > maxRows || word == maxWords) && word< pn.length - almostFinished) {
            compacted = true;
            /* too long - add a (more) link and open hidden span for rest of text */
            result.push('...(show more)');
            result.push('');
        }

        result.push(pn[word]);
    }
    if (compacted) {
        // compacted: add link to toggle back (less) and close the hidden span.
        result.push('(show less)');
    }

    return result.join(' ');
}

Instead of pn use a longer variable name. A variable like this with such a big scope really deserves a longer name. It would make the code easier to read.

var pn = text.split(' ');

Instead of commenting use the comment name as a function name:

// escape the html code in the text
text = (text || '').replace(/</g, '&lt;').replace(/>/g,'&gt;').replace(/\r/g, ' ').replace(/\n/g, '
 ')

I'd write this:

function escapeHtml(text) {
    if (!text) {
        return "";
    }
    return text.replace(/</g, '&lt;').replace(/>/g,'&gt;').replace(/\r/g, ' ').replace(/\n/g, '
 ')
}

I'd create a createToggleLink function.

function createToggleLink(prefix, linkText) {
    return '(" + linkText + ")');
}

It removes some code duplication and furthermore makes it more explicit that you close the span tag:

if (compacted) {
    var toggleLink = createToggleLink(prefix, '(show less)');
    result.push(toggleLink);
    result.push('');
}

So, the comment is unnecessary, the code says the same.


Subset pandas DataFrame based on two columns ignoring what order the match happens in I have two Pandas DataFrames and I want to subset df_all based on the values within to_keep.  Unfortunately this isn't straight forward pd.merge() or df.join() because I have multiple columns that I want to match on, and I don't care what order the match happens.

I don't care if df_all['source'] matches in either to_keep['from'] OR 'to_keep['to']
And then df_all['target'] matches in either to_keep['from'] OR to_keep['to'].

What I have below currently works, but it seems like a lot of work and hopefully this operation could be optimized.

import pandas as pd
import numpy as np

# create sample dataframe
df_all = pd.DataFrame({'from': ['a', 'a', 'b', 'a', 'b', 'c', 'd', 'd', 'd'],
                       'to': ['b', 'b', 'a', 'c', 'c', 'd', 'c', 'f', 'e'],
                       'time': np.random.randint(50, size=9),
                       'category': np.random.randn(9)
                       })

# create a key based on from & to
df_all['key'] = df_all['from'] + '-' + df_all['to']

df_all

    category    from    time    to  key
0   0.374312    a   38  b   a-b
1   -0.425700   a   0   b   a-b
2   0.928008    b   34  a   b-a
3   -0.160849   a   44  c   a-c
4   0.462712    b   4   c   b-c
5   -0.223074   c   33  d   c-d
6   -0.778988   d   47  c   d-c
7   -1.392306   d   0   f   d-f
8   0.910363    d   34  e   d-e

# create another sample datframe
to_keep = pd.DataFrame({'source': ['a', 'a', 'b'],
                        'target': ['b', 'c', 'c']
                       })

to_keep

    source  target
0   a   b
1   a   c
2   b   c

# create a copy of to_keep
to_keep_flipped = to_keep.copy()

# flip source and target column names
to_keep_flipped.rename(columns={'source': 'target', 'target': 'source'}, inplace=True)

# extend to_keep with flipped version
to_keep_all = pd.concat([to_keep, to_keep_flipped], ignore_index=True)

to_keep_all

    source  target
0   a   b
1   a   c
2   b   c
3   b   a
4   c   a
5   c   b

# create a key based on source & target
keys = to_keep_all['source'] + '-' + to_keep_all['target']

keys

0    a-b
1    a-c
2    b-c
3    b-a
4    c-a
5    c-b
dtype: object

df_all[df_all['key'].isin(keys)]

    category    from    time    to  key
0   0.374312    a   38  b   a-b
1   -0.425700   a   0   b   a-b
2   0.928008    b   34  a   b-a
3   -0.160849   a   44  c   a-c
4   0.462712    b   4   c   b-c

First, some comments on your question-asking: in your initial statement, you talk about matching 'col1' etc., but in your actual code, you have 'from, 'to', 'source', and 'target'. You talk about subsetting, but then you talk about pd.merge(), and those are completely different things. You formatting is poor, resulting your column names not lining up with the actual columns when you show the output in your code. When you generate random sample data, you should set a seed so that people will get the same data, and can check whether their code is doing the same thing as yours. Your test data is poorly chosen. For instance, does row for which 'from' matches one of the to_keep columns have to match the row in which 'to' matches, or can they match different rows? Can they both match the same column (e.g. 'from' and 'to' both match 'target'), or do they have to match different columns.  Neither your test cases nor your problem description are clear on those points; one has to go through your code to figure out what you mean.

Assuming that they have match on the same row, and in different columns, this code should work:

def check(row):
    forward = (to_keep['source'] == row['from']) & (to_keep['target'] == row['to'])
    reverse = (to_keep['source'] == row['to']) & (to_keep['target'] == row['from'])
    return any(forward) | any(reverse)

kept_df = df_all.loc[[check(row) for row in df_all.iterrows()]]


How should I refactor my image uploader script? I come from a C background and need help refactoring the following imgur image uploader script I wrote.

It checks if a token_file with access_token and refresh_token exists. If not, it instructs the user to go to a webpage, allow access to my app and enter the PIN generated from the site back to my program so that I can exchange it with access and refresh tokens.

I upload the picture taken with the 'scrot' utility to the users album, put the direct link on clipboard, and show a system notification. I'm not using the libnotify gem because it segfaults.

If the upload failed because the access_token is expired, which lasts only 60 minutes, I use the refresh token I got from the first authorization step, get a new one and update the token_file as well.

I know I should be using a class to keep things more organized.

I just don't know what's the best way to deal with info like client_id and token_file. Should they be instance or class methods? Also what should I put in the initialize method, the reading of tokens from the file or launch scrot?

I know I don't error check for everything yet.

#!/usr/bin/env ruby

require 'httparty'
require 'json'
require 'clipboard'

$client_id     = 'xxxxxxxxxxx'
$client_secret = 'xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx'
$token_file    = '.imgur_token'

def auth_app

  puts 'Follow the link to allow the application access to your account and enter the pin',
  "https://api.imgur.com/oauth2/authorize?client_id=#{$client_id}&response_type=pin"

  print 'Pin: '
  pin = STDIN.gets.chomp

  response = HTTParty.post 'https://api.imgur.com/oauth2/token',
    :body => { 'client_id'     => $client_id,
               'client_secret' => $client_secret,
               'grant_type'    => 'pin',
               'pin'           => pin }

  if response['access_token'] == nil
    puts 'Authorization failed'
  else
    tokens = { 'access_token'  => response['access_token'],
               'refresh_token' => response['refresh_token'] }
    File.write($token_file, tokens.to_json)
  end
  tokens
end

def refresh_token(refresh_token)

  response = HTTParty.post 'https://api.imgur.com/oauth2/token',
    :body => { 'refresh_token' => refresh_token,
               'client_id'     => $client_id,
               'client_secret' => $client_secret,
               'grant_type'    => 'refresh_token' }

  response['access_token']
end

def upload_image(access_token)

  response = HTTParty.post 'https://api.imgur.com/3/upload.json',
    :headers => { 'Authorization' => "Bearer #{access_token}" },
    :body    => { 'image' => Base64.encode64(File.read('imgur.png')) }

  response['data']['link']
end

abort('scrot not found') unless system('scrot -s imgur.png')
tokens = File.exists?($token_file) ? JSON.parse(File.read($token_file)) : auth_app

if (link = upload_image(tokens['access_token'])) == nil
  tokens['access_token'] = refresh_token(tokens['refresh_token'])
  link = upload_image(tokens['access_token'])
  File.write($token_file, tokens.to_json)
end

if link != nil
  Clipboard.copy link
  system('notify-send Upload complete')
else
  system('notify-send Upload error')
end

File.delete('imgur.png')

I'd do it something like this. Comments are sprinkled throughout:

#!/usr/bin/env ruby

require 'httparty'
require 'json'
require 'clipboard'

Use constants instead of globals:

CLIENT_ID     = 'xxxxxxxxxxx'
CLIENT_SECRET = 'xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx'
TOKEN_FILE    = '.imgur_token'

Move strings that are subject to change to the top of the file for easy access:

AUTHORIZE_URL = "https://api.imgur.com/oauth2/authorize?client_id=#{ CLIENT_ID }&response_type=pin"
TOKEN_URL = 'https://api.imgur.com/oauth2/token'

def auth_app

I prefer to see separate strings being printed on individual lines, rather than split them with , and let puts do it. It helps the code reflect what the output will be:

  puts 'Follow the link to allow the application access to your account and enter the pin:'
  puts AUTHORIZE_URL

  print 'Pin: '
  pin = STDIN.gets.chomp

I know it's common/popular in Ruby to not use parenthesis to delimit parameters to methods, but I always use them. It visually separates the parameters from the method name, and also forces the correct parsing in ambiguous cases when blocks are also passed in.

Also, I prefer to see the parameters on separate lines from opening/closing braces and brackets when there is more than a trivial array or hash. Again, it's a readability thing:

  response = HTTParty.post(
    TOKEN_URL,
    :body => {
      'client_id'     => CLIENT_ID,
      'client_secret' => CLIENT_SECRET,
      'grant_type'    => 'pin',
      'pin'           => pin
    }
  )

I reversed the order of the test for clarity and simplicity. Rather than test for nil, I test for the "positive" condition, which is a simpler test:

  if response['access_token']
    tokens = {
      'access_token'  => response['access_token'],
      'refresh_token' => response['refresh_token']
    }
    File.write(TOKEN_FILE, tokens.to_json)
  else
    puts 'Authorization failed'
  end
  tokens
end

def refresh_token(refresh_token)

  response = HTTParty.post(
    TOKEN_URL,
    :body => {
      'refresh_token' => refresh_token,
      'client_id'     => CLIENT_ID,
      'client_secret' => CLIENT_SECRET,
      'grant_type'    => 'refresh_token'
    }
  )

  response['access_token']
end

def upload_image(access_token)

  response = HTTParty.post 'https://api.imgur.com/3/upload.json',
  :headers => { 'Authorization' => "Bearer #{access_token}" },
  :body    => { 'image' => Base64.encode64(File.read('imgur.png')) }

  response['data']['link']
end

abort('scrot not found') unless system('scrot -s imgur.png')

if File.exists?(TOKEN_FILE)
  tokens = JSON.parse(File.read(TOKEN_FILE))
else
  tokens = auth_app
end

I do NOT like seeing an assignment inside the conditional test. It's confusing and an error-in-waiting in the future if someone things you meant to do an equality check and "corrects" the code:

link = upload_image(tokens['access_token'])

Again, I reversed the test:

if (!link)
  tokens['access_token'] = refresh_token(tokens['refresh_token'])
  link = upload_image(tokens['access_token'])
  File.write(TOKEN_FILE, tokens.to_json)
end

I reversed the test and assign a message to a string so I can fall through and use a single system invocation:

if link
  response_msg = 'notify-send Upload error'
else
  Clipboard.copy link
  response_msg = 'notify-send Upload complete'
end
system(response_msg)

File.delete('imgur.png')

In general I think your code is pretty good, with just a few tweaks that I'd do. I'd rather write my code as clearly and cleanly as I can, even for trivial stuff, because I never know when I'll need to revisit it. Sometimes I have to tweak something a year or two later, so being able to comprehend what the heck I was thinking quickly is important.


Ordering colors into a multidimensional list The code below works fine but seems really hard coded and I feel like I over-complicated the way it should be, I have a list containing hex and non-hex colors.

example : ['#333333', '#ff034a', 'red']

Basically, hex colors are converted to RGB, the nouns are compared in the database and returns a list of hex if the noun matches a query.

Here's the Output I want to keep (could be a numpy output too) :

[[(38, 30, 38)], [(245, 245, 245)], [(255, 51, 51), (255, 153, 153), (217, 38, 38), (242, 13, 13), (255, 62, 62)]]

Here's the hard coded junk I want to keep short and make faster :

list_of_colors = ['#333333', '#ff034a', 'red'] #can contain from 1 to 3 values

def orderDimentionalRGB(list_of_colors):
    dominant = []
    secondary = []
    complementary = []

    for index, color in enumerate(list_of_colors):
        if Color.objects.filter(color=color).exists(): #check for noun
            colors = Color.objects.filter(color=color)
            lst_colors = numpy.array(colors.first().color_list.replace(',', '').split())

            for lc in lst_colors:
                rgb = hex_to_rgb(lc) #returns from hex string -> to tuple rgb
                if index == 0:
                    dominant.append(rgb)
                elif index == 1:
                    secondary.append(rgb)
                else:
                    complementary.append(rgb)

        else: #check for rgb
            color = hex_to_rgb(color)

            if index == 0:
                dominant.append(color)
            elif index == 1:
                secondary.append(color)
            elif index == 2:
                complementary.append(color)

    colors = [dominant, secondary, complementary]
    set_colors = [x for x in colors if x]

    return set_colors

Maybe using that much if, elif, else and array variables becomes redundant and makes things slower, how can I improve this code ?

As far as performance goes, you definitely need to understand your bottlenecks - profile the code properly and try to see what takes the most time - querying the database, converting hex to rgb or something else.

Here are few observations though:

don't do exists() and then filter() - just do filter() right away - it will return an empty queryset if no results
I am not sure if there is much sense in making a numpy.array - see if removing it would make a positive impact on performance
you don't need to keep track of indexes and then append to different lists based on an index - what if you would just collect the current results and then append to the resulting list at the end (see what I mean in the code below)
if a color starts with # you probably don't need to query the database at all

At the end, you may get something along these lines:

def orderDimentionalRGB(list_of_colors):
    results = []

    for color in list_of_colors:
        current_result = []

        if color.startswith("#"):
            current_result.append(hex_to_rgb(color))
        else:
            db_result = Color.objects.filter(color=color)
            if db_result:
                db_colors = db_result.first().color_list.replace(',', '').split()
                current_result += [hex_to_rgb(db_color) for db_color in db_colors]

        if current_result:
            results.append(current_result)

    return results


Checking if a git repository needs to be updated I made a small Vim distribution that uses vundle. I decided to make a small function that checks if the repository must be updated.

""""""""""""""""""""""""""""""""""""""""""""""""""
"Update the repository
""""""""""""""""""""""""""""""""""""""""""""""""""
"A function to update if necessary
function! Get_update()
    "Get the current path and change the directory to update the good
    "repository
    let l:current_path = expand("<sfile>:p:h")
    let l:path = '~/.vim-mahewin-repository'
    exec 'cd' l:path

    let l:last_local_commit = system('git rev-parse HEAD')
    let l:last_remote_comit = system('git ls-remote origin -h refs/heads/master |cut -f1')

    if l:last_local_commit != l:last_remote_comit
        echo  'Need to be updated, launches updated'
        :!git pull `git remote` `git rev-parse --abbrev-ref HEAD`
    endif

    exec 'cd' l:current_path
endfunction

It's not very nice, but I have not found out how to do it better. I'm open to advice.

Your function has the side-effect of switching the directory to the file's path.  To be side-effect-free, you should let l:current_path = getcwd() instead.  Better yet, use the --git-dir and --work-tree options to git(1) to avoid having to cd at all.

You always query the remote named origin, but pull from `git remote`.  That inconsistent looks like a bug.

I don't see any reason to use --abbrev-ref when the result is not intended for human consumption.  Just use the full commit id.


Definite Integral Approximation using the Trapezoidal Method I wrote a program to calculate the value of Definite Integral of a function from a to b. It used the trapezoidal approximation method where the function area is divided into 'n' trapeziums and area of each is added.

def calculate_area(f, a, b, n):
    # Trapezoidal approximation: divide area of function into n trapeziums and add areas.
    # Large n value means better approximation

    # Finds the value of a definite integral from lower_lim (a) to upper_lim (b)
    # integral f(x) from lower_lim to upper_lim is =
    # 0.5 * w * ( f(a) + f(b) ) + <----- part_1
    # w * SUMMATION(x varies from 1 to n-1): f(a + x * w) <----- part_2
    # where w = (b - a) / n

    w = (b - a) / float(n)
    part_1 = 0.5 * w * ( f(a) + f(b) )
    part_2 = 0
    for x in range(1, n):
        part_2 += f(a + x * w)
    part_2 *= w
    total_area = part_1 + part_2
    return total_area

For example, this can be used to calculate the area of a circle (or \$\pi\$ if the radius of the circle is 1):

  Consider the circle \$x^2 + y^2 = 1\$

  Area of this circle = \$\pi\$

  Expressed as a function of x is: f(x) = (1 - x^2) ^ 0.5

  Integrating this from 0 to 1 gives us a quarter of a circle whose
  area is \$\frac\pi  4\$ (or the approximate value if we use approximate integration).

  On multiplying 4, we get the value of \$\pi\$.

  Pass in the following as arguments :

f = lambda x: (1 - x**2) ** 0.5; a = 0; b = 1

Now I face the following issues:

Code is not very Pythonic.
Code is not very efficient and fast.
For better precision of PI value, the value of n must be increased. Python doesn't comply when n takes higher values (say 100 million). n = 10 million worked and I got a value of PI that was accurate upto 8 digits after the decimal.
I feel that using floating point like datatypes which provide higher precision will increase precision of PI value. Correct me if I'm wrong.

I know that this is a very poor method of approximating the value of \$\pi\$ (this is the best I could come up with high school level knowledge). Let me know when I'm going to reach the limits. That is, when I will no longer be able to get noticeably more precise value of \$\pi\$.

If you are writing mathematical code in Python, then it is worth looking at NumPy, which is a library implementing (among other things) fast arithmetic operations on arrays of floating-point numbers.

In NumPy you'd write:

import numpy as np

def calculate_area(f, a, b, n):
    """Return an approximation to the definite integral of f from a to b
    using the trapezium rule with n intervals.

    """
    w = (b - a) / n                # width of the intervals
    x = np.linspace(a, b, n + 1)   # x coords of endpoints of intervals
    y = f(x)
    return w * (y.sum() - (y[0] + y[-1]) / 2)

Here np.linspace returns an array of numbers evenly spaced between two endpoints. For example, if we are asked to integrate between 0 and 1 with 4 intervals, then the endpoints of the intervals are:

>>> np.linspace(0, 1, 5)
array([0.  , 0.25, 0.5 , 0.75, 1.  ])

Under the assumption that f is a straightforward arithmetic function of its argument, then f(x) computes an array of the corresponding y values for each x. For example, if we have

def f(x):
    return x ** 2 + x + 1

Then:

>>> x = np.linspace(0, 1, 5)
>>> f(x)
array([1.    , 1.3125, 1.75  , 2.3125, 3.    ])

However, NumPy already has a built-in function np.trapz that computes an integral using the trapezium rule. Using this function, we could write:

def calculate_area(f, a, b, n):
    """Return an approximation to the definite integral of f from a to b
    using the trapezium rule with n intervals.

    """
    x = np.linspace(a, b, n + 1)   # x coords of endpoints of intervals
    return np.trapz(f(x), x)


Writing billions of lines to text file in java I'm trying to write billions of strings lines to a file, it works for up to 40 million lines, but it's throwing out "java.nio.BufferOverflowException" error for 400 million lines. I also think my solution is slow as it takes 45 seconds to write 40 million lines. Below is the code.

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

byte[] buffer = "Help I am trapped in a fortune cookie factory\n".getBytes();
int number_of_lines = 400000000;

FileChannel rwChannel = new RandomAccessFile("textfile.txt", "rw").getChannel();
ByteBuffer wrBuf = rwChannel.map(FileChannel.MapMode.READ_WRITE, 0, buffer.length * number_of_lines);
for (int i = 0; i < number_of_lines; i++) {
    wrBuf.put(buffer);
}
rwChannel.close();
}

How can I make it be faster if possible and also write more than 2 billion lines of string?

It sure sounds like you're running out of memory as you try to build (or allocate) the buffer.

You need to figure out how big a buffer you can (or want to) use. You'll need to ask the system how much RAM it has, and how much is available, and make your buffer that large.
You might get a small boost by opening the file as explicitly write-only.
You'll need to flush the buffer as you go. Something shaped like this psudo-code might work:

free = system.free_ram();
use = hungry * free;
buffer = new bytes_buffer_to_file("filename", size=use);
try{
  current_buffer = 0;
  while(line = get_bytes_needing_to_be_written()){
    if(current_buffer + line.length > use){
      buffer.flush();
      current_buffer = 0;
    }
    buffer.append(line);
    current_buffer += line.length;
  }
}
finally{
  buffer.flush();
  buffer.destroy();
}


Boost Threads - Producer Consumer threads with synchronization Please review this for concurrency  correctness:

#include <iostream>
#include <queue>

#include "boost\thread.hpp"
#include "boost\timer.hpp"

std::queue<int> itemQ;

boost::mutex m;
boost::condition_variable qFull, qEmpty;

const int max_size_q = 5;

void producer()
{
    int i = 0;
    while (1)
    {
        boost::this_thread::sleep(boost::posix_time::millisec(1000));
        boost::mutex::scoped_lock lock(m);
        if (itemQ.size() <= max_size_q)
        {
            itemQ.push(++i);
            qEmpty.notify_one();
        }
        else
        {
            std::cout << "Q Full.notify_one Producer Waiting" << std::endl;
            qFull.wait(lock);
            std::cout << "Producer Notified to Continue" << std::endl;
        }
    }

}

void consumer()
{
    while (1)
    {
        boost::this_thread::sleep(boost::posix_time::millisec(4000));

        boost::mutex::scoped_lock lock(m);

        if (itemQ.size() == 0)
        {
            std::cout << "Q Empty. Consumer " << boost::this_thread::get_id() <<" Waiting" << std::endl;
            qEmpty.wait(lock);
            std::cout << "Consumer Notified to Continue" << std::endl;
        }
        else
        {
            std::cout << itemQ.front() << std::endl;
            itemQ.pop();
            qFull.notify_one();
        }

    }
}

int main()
{
    boost::thread producerthread(producer);

    boost::thread consumerthread1(consumer);
    boost::thread consumerthread2(consumer);
    boost::thread consumerthread3(consumer);
    boost::thread consumerthread4(consumer);
    boost::thread consumerthread5(consumer);

    consumerthread1.join();
    consumerthread2.join();
    consumerthread3.join();
    consumerthread4.join();
    consumerthread5.join();
}

Your usage of the condition variable will probably work but is a-typical.

This is what you basically have:

while (1)
{
    boost::mutex::scoped_lock lock(m);
    if (<Test OK>)
    {
        <Do Work>
        <Notify Consumer>
    }
    else
    {
        <Wait for consumer to signal conditional>
    }
}

Notice that here you lock the mutex m. If the test is not OK then you wait for the consumer to signal the condition variable. This releases the lock on m. Once the condition variable is signaled then your thread must wait to re-aquire the lock to continue. Once it does it releases the lock and then re-starts the loop which immediately try to re-aquire the lock.

A more typical pattern would be:

// pseudo code.
std::unique_ptr<WORK> getWork()
{
    boost::mutex::scoped_lock lock(m);
    while(! <Test OK> )
    {
       <Wait for consumer to signal conditional>
    }
    return <getWorkObjectFromQueue>;
}
.....
while(1)
{
    work = getWork();

    <Do Work>
    <Notify Consumer>
}

This way when you are waiting on the conditional variable and are signaled you do not have multiple attempts to re-acquire the lock before you do the work. As soon as you are signaled and have acquired the lock you can do a bit of work.


End of line converter I recently wrote a python script to convert multiple files EOL's from Unix to DOS and vice versa.

I am looking for tips to improve my code or if there is a better way of doing something that I have missed.

#!/usr/bin/env python3

import sys

def main():
    command, *filenames = sys.argv[1:]
    valid_commands = ['-d', '-u']
    sys.tracebacklimit = None

    if not command in valid_commands:
        error = """'{command}'
        Provide the following arguments -u|d file [file2] [file3] ...
        flags:
            -u : converts DOS to UNIX
            -d : converts UNIX to DOS
        example command:
            ./eol -u foo.py bar.py""".format(command=command)

        raise ValueError(error)
        sys.exit(1)

    if filenames:
        convert(filenames, command)
    else:
        print("> no files to convert")

def convert(files, command):
    for file in files:
        text = open(file, 'r').read()

        with open(file, 'w') as current:
            if command == '-u':
                format = 'UNIX'
                current.write(text.replace('\r\n', '\n'))
            elif command == '-d':
                format = 'DOS'
                current.write(text.replace('\n', '\r\n'))

        print("> converting file {filename} to {format} ...".format(
              filename=file, format=format))

if __name__ == "__main__":
    main()

The code in the post does not work, because the files are opened in text mode, and in text mode Python 3 translates newlines by default. To quote the Python documentation:

newline controls how line endings are handled. It can be None, '', '\n', '\r', and '\r\n'. It works as follows:

When reading input from the stream, if newline is None, universal newlines mode is enabled. Lines in the input can end in '\n', '\r', or '\r\n', and these are translated into '\n' before being returned to the caller. If it is '', universal newlines mode is enabled, but line endings are returned to the caller untranslated. If it has any of the other legal values, input lines are only terminated by the given string, and the line ending is returned to the caller untranslated.
When writing output to the stream, if newline is None, any '\n' characters written are translated to the system default line separator, os.linesep. If newline is '' or '\n', no translation takes place. If newline is any of the other legal values, any '\n' characters written are translated to the given string.

This means that the code in the post never gets to see the original line endings and so it does not behave as intended when run on Windows. (This makes me suspect that it has not been tested in all four configurations: Unix → DOS on Unix; DOS → Unix on Unix; Unix → DOS on Windows; DOS → Unix on Windows.)
In order to operate on the original line endings, you could open the file in binary mode (both for reading and writing), or open it in text mode but set newline='' so that newlines are not translated.


My Sal's Shipping challenge on Codecademy This is my attempt on the Python 3 Sal's Shipping challenge on Codecademy (Challenge Link (instructions in the link)).

I'd really appreciate anyone to take the time to review my code. I'm a very new to programming and I really wanna improve my coding, so all feedback is apreciated.

def groundCost(weight):
  if weight > 10:
    cost = weight * 4.75
  elif weight > 6:
    cost = weight * 4
  elif weight > 2:
    cost = weight * 3
  else:
    cost = weight * 1.5
  cost += 20
  return cost

def droneCost(weight):
  if weight > 10:
    cost = weight * 14.25
  elif weight > 6:
    cost = weight * 12
  elif weight > 2:
    cost = weight * 9
  else:
    cost = weight * 4.5
  return cost

def costCalc(weight):
  g = groundCost(weight)
  p = 125
  d = droneCost(weight)
  gS = "Ground Shipping"
  pS = "Premium Ground Shipping"
  dS = "Drone Shipping"
  if d == g or d == p or g == p:
    if d == g and d == p:
      print("All shipping methods cost $125 according to your items weight")
      return
    if d == g:
      same1 = dS
      same2 = gS
      cost = d
    if d == p:
      same1 = dS
      same2 = pS
      cost = d
    if g == p:
      same1 = gS
      same2 = pS
      cost = g
    print("Youre cheapest shipping method is "+same1+" and "+same2+" as they both cost $"+str(cost))
    return
  elif (g < d) and g < p:
    cheapMeth = gS
    cost = g
  elif (p < d) and p < g:
    cheapMeth = pS
    cost = p
  elif (d < g) and d < p:
    cheapMeth = dS
    cost = d
  print("Youre cheapest shipping method is "+cheapMeth+" costing $"+str(cost))
  return
weight = int(input())
costCalc(weight)

Ok, first of all you should've take care of variable names. Naming them like p, g, e, etc is really a bad habbit.

Once you change them, it's much easier to read.

def cost_calc(weight):
  ground_cost = calculate_ground_cost(weight)
  drone_cost = calculate_drone_cost(weight)
  premium_cost = 125
  ground_shipping_name = "Ground Shipping"
  premium_shipping_name = "Premium Ground Shipping"
  drone_shipping_name = "Drone Shipping"

Second python naming convention tells you to use underscores while naming variables or functions.

Third, your functionc that calculates costs can be minimized in many ways, when one of them is:

import bisect

def calculate_ground_cost(weight):
  limits = [2, 6, 10]
  multipliers = [1.5, 3, 4, 4.75]
  idx = bisect.bisect_left(limits, weight)
  return (weight * multipliers[idx]) + 20

def calculate_drone_cost(weight):
  limits = [2, 6, 10]
  multipliers = [4.5, 9, 12, 14.25]
  idx = bisect.bisect_left(limits, weight)
  return (weight * multipliers[idx])

Then you should take care of IFs statements that are really messy.

if drone_cost == ground_cost or drone_cost == premium_cost or ground_cost == premium_cost:

Is equvalent to

if drone_cost == ground_cost == premium_cost:

And the next rule good to follow is always return ASAP.

if drone_cost == ground_cost == premium_cost:
    return "All shipping method cost %s according to your items weight" % premium_cost

Then all of this mess:

if  d == g and d == p:
      print("All shipping methods cost $125 according to your items weight")
      return
    if d == g:
      same1 = dS
      same2 = gS
      cost = d
    if d == p:
      same1 = dS
      same2 = pS
      cost = d
    if g == p:
      same1 = gS
      same2 = pS
      cost = g
    print("Youre cheapest shipping method is "+same1+" and "+same2+" as they both cost $"+str(cost))
    return

Can be changed to

if ground_cost == drone_cost:
    return "Youre cheapest shipping method is %s and %s as they both cost$%s" %(ground_shipping_name, premium_shipping_name, ground_cost)

Because this is the last "IF" that is matching the statement.

And at the end, where u try to determine what is the lowest cost:

  elif (g < d) and g < p:
    cheapMeth = gS
    cost = g
  elif (p < d) and p < g:
    cheapMeth = pS
    cost = p
  elif (d < g) and d < p:
    cheapMeth = dS
    cost = d
  print("Youre cheapest shipping method is "+cheapMeth+" costing $"+str(cost))
  return

You can change it to

costs = sorted([
   (ground_cost, ground_shipping_name),
   (drone_cost, drone_shipping_name),
   (premium_cost, premium_shipping_name)])

min_cost, method = costs[0]
return "Your cheapest shipping method is %s costing $%s" % (method, min_cost)

So at the end, your code look like:

import bisect

def calculate_ground_cost(weight):
  limits = [2, 6, 10]
  multipliers = [1.5, 3, 4, 4.75]
  idx = bisect.bisect_left(limits, weight)
  return (weight * multipliers[idx]) + 20

def calculate_drone_cost(weight):
  limits = [2, 6, 10]
  multipliers = [4.5, 9, 12, 14.25]
  idx = bisect.bisect_left(limits, weight)
  return (weight * multipliers[idx])

def cost_calc(weight):
  ground_cost = calculate_ground_cost(weight)
  drone_cost = calculate_drone_cost(weight)
  premium_cost = 125
  ground_shipping_name = "Ground Shipping"
  premium_shipping_name = "Premium Ground Shipping"
  drone_shipping_name = "Drone Shipping"
  if drone_cost == ground_cost == premium_cost:
    return "All shipping methodrone_shipping_name cost $125 according to your items weight"

  if ground_cost == drone_cost:
    return "Youre cheapest shipping method is %s and %s as they both cost$%s" %(ground_shipping_name, premium_shipping_name, ground_cost)

  costs = sorted([
    (ground_cost, ground_shipping_name),
    (drone_cost, drone_shipping_name),
    (premium_cost, premium_shipping_name)])

  min_cost, method = costs[0]

  return "Your cheapest shipping method is %s costing $%s" % (method, min_cost)

weight = int(input("Enter weight: "))
print(cost_calc(weight))


How to better indent multiline strings with many string formatting args? I have a function, that is used in flask, for logging application errors:

    def log_exception(self, exc_info):
        self.logger.error('''
Time:                 %s
Path:                 %s
HTTP Method:          %s
POST_ARGS:            %s
GET_ARGS:             %s
Client IP Address:    %s
User Agent:           %s
User Platform:        %s
User Browser:         %s
User Browser Version: %s
Username:             %s
Email:                %s''' % (
               datetime.now(),
               request.path,
               request.method,
               request.form,
               request.args,
               request.remote_addr,
               request.user_agent.string,
               request.user_agent.platform,
               request.user_agent.browser,
               request.user_agent.version,
               current_user,
               getattr(current_user, 'email', 'no email'),
        ), exc_info=exc_info)

As you can see the indentation is quite ugly and the long list of formatting args makes it even worse.

Using textwrap.dedent helped the situation, but the formatting args is still problematic:

def log_exception(self, exc_info):
    self.logger.error(
        dedent('''Time:                 %s
                  Path:                 %s
                  HTTP Method:          %s
                  POST_ARGS:            %s
                  GET_ARGS:             %s
                  Client IP Address:    %s
                  User Agent:           %s
                  User Platform:        %s
                  User Browser:         %s
                  User Browser Version: %s
                  Username:             %s
                  Email:                %s''') % (datetime.now(),
                                                  request.path,
                                                  request.method,
                                                  request.form,
                                                  request.args,
                                                  request.remote_addr,
                                                  request.user_agent.string,
                                                  request.user_agent.platform,
                                                  request.user_agent.browser,
                                                  request.user_agent.version,
                                                  current_user,
                                                  getattr(current_user, 'email', 'no email')),
          exc_info=exc_info
    )

Is this the best I can do?

How about this?

def log_exception(self, exc_info):

    msg = ''

    msg += 'Time:                 %s\n' % datetime.now()
    msg += 'Path:                 %s\n' % request.path
    msg += 'HTTP Method:          %s\n' % request.method
    msg += 'POST_ARGS:            %s\n' % request.form
    msg += 'GET_ARGS:             %s\n' % request.args
    msg += 'Client IP Address:    %s\n' % request.remote_addr
    msg += 'User Agent:           %s\n' % request.user_agent.string
    msg += 'User Platform:        %s\n' % request.user_agent.platform
    msg += 'User Browser:         %s\n' % request.user_agent.browser
    msg += 'User Browser Version: %s\n' % request.user_agent.version
    msg += 'Username:             %s\n' % current_user
    msg += 'Email:                %s\n' % getattr(current_user, 'email', 'no email')

    self.logger.error(msg, exc_info=exc_info)

I guess, logging an exception doesn't happen every often, so efficiency shouldn't be a concern.

Alternatively, you can emulate variable interpolation and avoid formatting altogether:

now = datetime.now()
email = getattr(current_user, 'email', 'no email')

msg = _('''\
    Time:                 {now}
    Path:                 {request.path}
    HTTP Method:          {request.method}
    POST_ARGS:            {request.form}
    GET_ARGS:             {request.args}
    Client IP Address:    {request.remote_addr}
    User Agent:           {request.user_agent.string}
    User Platform:        {request.user_agent.platform}
    User Browser:         {request.user_agent.browser}
    User Browser Version: {request.user_agent.version}
    Username:             {current_user}
    Email:                {email}
''')

self.logger.error(dedent(msg), exc_info=exc_info)