Becoming the greatest tower builder I am trying to learn how to write proper code in python, and stumbled upon the following problem.

  Bob is given a set of building blocks b, and a number n of towers
  to build. Since we are in communist land the towers should have as
  equal of an height as possible. The catch is that you are only given
  a maximum of 300 seconds to build the towers. The goal is to minimize the std of
  the tower height.

Say you are given the following vector

bricks = [8     8     8     6     6     4     4     4     4     3     3     2]

and asked to build 5 towers. One output to the problem can be

 8     4     0
 8     4     0
 8     4     0
 6     4     2
 6     3     3

Which has an std of std = ([12-12]^2+[12-12]^2+...+[12-12]^2)^1/2=0. for 3 towers the output should be

 8     4     4     4
 8     6     3     3
 8     6     4     2

Which again is optimal. Note however I am not asking for an optimal solution..
So this is similar to the knapsack problem. But here the goal is not to fill each knapsack to the brim, but rather to have each knapsack have as close weight as possible.

I came up with two possible codes

def TowerGreed(bricks,nTower):

    bricks.sort(reverse=True)
    lengthBricks = len(bricks)

    if lengthBricks > nTower:
        Tower = [[] for i in range(0,nTower)]
        for i in range(0,lengthBricks):
            if i < nTower:
                Tower[i].append(bricks[i])
            elif i == nTower:
                Height = bricks[0:nTower]
                Tower[i-1].append(bricks[i])
                Height[nTower-1] = bricks[i] + Height[nTower-1]
            else:
                lowest = Height.index(min(Height))
                Tower[lowest].append(bricks[i])
                Height[lowest] = bricks[i] + Height[lowest]

    return Tower

This is a standard Greed algorithm. It first sort the elements, then it adds one element to each tower. Then it starts checking which tower is the smallest. The smallest tower is given the next piece.

A modified version of the code above, is given below

def towerGreedImproved(bricks,numberTowers):

    # First we sort the number of bricks from greatest to smallest
    bricks.sort(reverse = True)

    # Only runs the algorithm if we have more bricks than towers to build
    if len(bricks) >= numberTowers:
        Towers = [[] for i in range(0,numberTowers)] #Empty vector [[],[],...]
        Height = [0] * numberTowers # Zero vector [0,0,...]
        avgHeight = sum(bricks)/numberTowers

        """ Loops through each tower, adds elements until that tower is just
            bellow the average tower height """
        for j in range(0,numberTowers):
            while Height[0] + bricks[0] < avgHeight:
                Towers[j].append(bricks[0])
                Height[j] = bricks[0] + Height[j]
                del bricks[0]; #Deletes used bricks

        for n in bricks:
            lowest = Height.index(min(Height))
            Towers[lowest].append(n)
            Height[lowest] = n + Height[lowest]

    print(Towers)
    return Towers

Here we also start by sorting the bricks, but the difference is we build one tower at a time. We stop building before we reach the average towerheight. This average is given as the sum of the bricks divided by the number of towers being built.

and third option was to randomize the input. The code above is somewhat slow for large vectors. To test my code, I tried the following three vectors. 2 towers, 3 towers, and finnaly 23 towers. Being more familiar with MATLAB i wrote the following code snippet. However I am not quite sure how to impliment it into python, nor if it provides any improvement.

Being a beginner in writing proper code, I have  a couple of questions

Which improvements can I make to my coding style?
People talked about dynamic solutions to this problem. Are there better algorithms for this problem?
How can I determine which algorithm is best suited for a given dataset?

Perhaps:

def towerGreedImproved(bricks, numberTowers):
    # First we sort the number of bricks from greatest to smallest
    obricks = list(bricks)
    obricks.sort(reverse = True)

    # Only runs the algorithm if we have more bricks than towers to build
    if len(obricks) < numberTowers:
        return obricks

    towers = tuple([] for i in range(numberTowers))  # Empty vector ([],[],...)
    height = [0] * numberTowers  # Zero vector [0,0,...]
    avgHeight = sum(obricks)/numberTowers

    """ Loops through each tower, adds elements until that tower is just
        below the average tower height """
    for j in range(numberTowers):
        while height[0] + obricks[0] < avgHeight:
            # Deletes used bricks
            brick = obricks.pop(0)
            towers[j].append(brick)
            height[j] += brick

    for n in obricks:
        lowest = min(enumerate(height), key = lambda kv: kv[1])[0]
        towers[lowest].append(n)
        height[lowest] += n

    return towers

bricks = (8, 8, 8, 6, 6, 4, 4, 4, 4, 3, 3, 2)
print(towerGreedImproved(bricks, 5))

Of note:

Python variable names should be lowercase
It's not playing nice to chew up someone's input list - make a copy
towers can be a tuple because we don't expect the contents to change
you should do something more reasonable on brick shortage, like return the brick list
range doesn't need a first argument of 0
use pop() to both get and delete an element
use += for addition and assignment
use enumerate with min so that you can get the min's index in one shot
don't automatically print the result on the inside of the function; let the user decide


Basic smoking money saver calculator I am quite new to Java and I wanted to try and make a smoking calculator that tells the user how much they would save over a certain time period. I would appreciate if someone could tell me where they would improve my code and what other features they would add?

Should I always use objects when coding in java even for a basic program like this one?

import java.util.Scanner;

public class Calculator {
    public static void main(String args[]) {

        Scanner scan = new Scanner(System.in);

        //Packets
        System.out.println("How many packets do you smoke a week?");
        double packets = scan.nextDouble();

        //cost
        System.out.println("How much is each packet?");
        double cost = scan.nextDouble();

        //weekly
        double weekly;
        weekly = (cost*packets);
        System.out.println("In one week you would save: €" + weekly);

        //monthly
        double monthly;
        monthly = (weekly*4);
        System.out.println("In one month you would save: €" + monthly);

        //threeMonths
        double threeMonths;
        threeMonths = (weekly*12);
        System.out.println("In three months you would save: €" + threeMonths);

        //sixMonths
        double sixMonths;
        sixMonths = (weekly*26);
        System.out.println("In six months you would save: €" + sixMonths);

        //yearly
        double yearly;
        yearly = (weekly*52);
        System.out.println("In one year you would save: €" + yearly);

        //user input
        System.out.println("Enter a number of years and see how much you will save over that time period...");
        double user = scan.nextDouble();
        double userInput;
        userInput = (yearly*user);
        System.out.println("You would save €" + userInput + " in " + user + " years.");

          scan.close();
    }
}

Should I always use objects when coding in java even for a basic program like this one?

How would you not?  Note that you are using an object in this program now, a Scanner object.  You also use a Calculator class.  Because of course that's how Java works.  There are no methods outside of classes.

Now, should you make an intermediate class and objects?  Maybe.  Maybe not.  I would like to see more methods here.  For example, consider

    public static void displayForTimePeriod(String message, double amount, double multiplier) {
        System.out.println(message, amount * multiplier);
    }

That would allow you to simplify most of your code to something like

        double weekly = cost * packets;
        displayForTimePeriod("In one week you would save: €", weekly, 1.0);
        displayForTimePeriod("In one month you would save: €", weekly, 30/7.0);
        displayForTimePeriod("In three months you would save: €", weekly, 13.0);
        displayForTimePeriod("In six months you would save: €", weekly, 26.0);
        displayForTimePeriod("In one year you would save: €", weekly, 52.0);

You could go further and make a class, but the main thing that it would hold would be the amount.  Note that that stays the same in every call.

I might add

        System.out.println("On average, you'll save €" + (weekly/7) + " every day.");

If you wanted, you could make your years calculation more accurate by including leap years.  That would require you to get the current date from the system time.  Remember to handle leap centuries.  2000 was a leap year, but 1900 was not and 2100 won't be.

I made the monthly number more accurate.  Unless the month is February, there are more than twenty-eight days (four weeks) in a month.  The usual approximation is thirty.

I fixed the quarterly (three months) number as well.  There are thirteen weeks in most quarters (the exception is the first quarter when February does not have a leap day).  Twelve weeks will be too few for every quarter.


RPN-Stack-Based Recursive Calculator Needs Tuneup This function takes an array of strings and numbers and recursively processes it as a kind of "calculating program".  The structure is based on Reverse Polish Notation, so the top object in the stack (last in the array) determines what to do with the next one (or two) items in the program.  For example, if the array was [1, 2, "+"], the solution would be 3.

In the case of errors, it should return an NSString describing the
error that happened.
If more than one error occurs in processing, it is not clear which
error should receive priority, so I've chosen to prioritize the first
error found and ignore any further errors.

I think that this code could be made much more simple, but I'm not sure how best to do that in Objective C.  What suggestions do you have to improve this code?

 + (id)popOperandOffProgramStack:(NSMutableArray *)stack
    {
        id result;
        double dResult;

        id topOfStack = [stack lastObject];
        if (topOfStack) [stack removeLastObject];

        if ([topOfStack isKindOfClass:[NSNumber class]])
        {
            result = topOfStack;
        }
        else if ([topOfStack isKindOfClass:[NSString class]])
        {
            id rightSide = [self popOperandOffProgramStack:stack];
            if (![rightSide isKindOfClass:[NSNumber class]]) {
                return rightSide;
            }
            double r = [(NSNumber *)rightSide doubleValue];

            NSString *operation = topOfStack;
            if ([[self class] isBinaryOperation:operation]) {
                id leftSide = [self popOperandOffProgramStack:stack];
                if (![leftSide isKindOfClass:[NSNumber class]]) {
                    return leftSide;
                }
                double l = [(NSNumber *)leftSide doubleValue];
                if([operation isEqualToString:@"+"]) {
                    dResult =  l + r;
                } else if([operation isEqualToString:@"-"]) {
                    dResult = l - r;
                } else if([operation isEqualToString:@"*"]) {
                    dResult = l * r;
                } else if([operation isEqualToString:@"/"]) {
                    if(r == 0.0) {
                        return @"Error: Tried to divide by zero";
                    } else {
                        dResult = l / r;
                    }
            }
            } else if ([operation isEqualToString:@"+/-"]) {
                dResult = -r;
            } else if([operation isEqualToString:@"sin"]) {
                dResult = sin(r);
            } else if([operation isEqualToString:@"cos"]) {
                dResult = cos(r);
            } else if([operation isEqualToString:@"sqrt"]) {
                if(r < 0) {
                    return @"Error: tried to sqrt a negative number";
                }
                dResult = sqrt(r);
            } else if([operation isEqualToString:@"π"]) {
                dResult = 3.141592654;
            } else {
                /* unknown operations and unset variables will return 0 */
                dResult = 0.0;
            }
            result = [NSNumber numberWithDouble:dResult];
        }
        return result;
    }

Instead of returning an error-String, I would pass in a nil'ed-NSError object by reference, and write an error to it, in case it occurred. the returned object would be nil

+ (NSNumber *)popOperandOffProgramStack:(NSMutableArray *)stack error:(NSError **)error
{
    //…
    error = [[NSError alloc] int…] ;
}

Usage:

NSError *error = nil;
if ([Calculator popOperandOffProgramStack:parameters error:&error])
{
    //returned object not nil -> success
} else {
    //nil-object -> let's check the error
}

this avoids type checking.


Tic Tac Toe victory check I am writing Python code for a Tic Tac Toe game.  I need to write a function that takes in three inputs: board, x, and y.  Board being the current display of the board and then x and y being values of 0, 1, or 2.  The game is set up to ask the user for coordinates.

def CheckVictory(board, x, y):

    #check if previous move was on vertical line and caused a win
    if board[0][y] == ('X') and board[1][y] == ('X') and board [2][y] == ('X'):
        return True
    if board[0][y] == ('O') and board[1][y] == ('O') and board [2][y] == ('O'):
        return True

    #check if previous move was on horizontal line and caused a win
    if board[x][0] == ('X') and board[x][1] == ('X') and board [x][2] == ('X'):
        return True
    if board[x][0] == ('O') and board[x][1] == ('O') and board [x][2] == ('O'):
        return True

    #check if previous move was on the main diagonal and caused a win
    if board[0][0] == ('X') and board[1][1] == ('X') and board [2][2] == ('X'):
        return True
    if board[0][0] == ('O') and board[1][1] == ('O') and board [2][2] == ('O'):
        return True
    #check if previous move was on the secondary diagonal and caused a win
    if board[0][2] == ('X') and board[1][1] == ('X') and board [2][0] == ('X'):
        return True
    if board[0][2] == ('O') and board[1][1] == ('O') and board [2][0] == ('O'):
        return True

    return False
#end of CheckVictory function

The function is called in the game loop like so:

p_x, p_y = playerTurn(board)    #let player take turn
displayBoard(board)             #show board after move has been made
if CheckVictory(board, p_x, p_y):   #see if user has won
    print("CONGRATULATIONS, you win!")
    newGame(board)  #game over start new one
    continue

and it's similar for the computer turn.

I feel like there is a better way to write this function.  I feel like I should be using x and y more or there is a better way to check rather than writing all the possibilities. What's a better way to write this to make it short and concise?

You know that a mark has been placed at board[x][y]. Then you only need this to check for a win on vertical line y:

if board[0][y] == board[1][y] == board [2][y]

Your comments state "check if previous move was on the main/secondary diagonal", but you don't actually check. You can use the expressions x == y and x + y == 2 to check that.

Simplified code:

def CheckVictory(board, x, y):

    #check if previous move caused a win on vertical line
    if board[0][y] == board[1][y] == board [2][y]:
        return True

    #check if previous move caused a win on horizontal line
    if board[x][0] == board[x][1] == board [x][2]:
        return True

    #check if previous move was on the main diagonal and caused a win
    if x == y and board[0][0] == board[1][1] == board [2][2]:
        return True

    #check if previous move was on the secondary diagonal and caused a win
    if x + y == 2 and board[0][2] == board[1][1] == board [2][0]:
        return True

    return False


Array whose values are the product of every other integer I was asked the following interview question over the phone:

  Given an array of integers, produce an array whose values are the
  product of every other integer excluding the current index.

  Example:

[4, 3, 2, 8] -> [3*2*8, 4*2*8, 4*3*8, 4*3*2] -> [48, 64, 96, 24]

and I gave the following answer

import java.math.BigInteger;
import java.util.Arrays;

public class ProductOfAnArray {

    public static void main(String[] args) {

        try {
            System.out.println(Arrays.toString(ProductOfAnArray
                    .calcArray(new int[] { 4, 3, 2, 8 })));
            System.out.println(Arrays.toString(ProductOfAnArray
                    .calcArray(new int[] { 4, 0, 2, 8 })));
            System.out.println(Arrays.toString(ProductOfAnArray
                    .calcArray(new int[] { 4, 0, 2, 0 })));
            System.out.println(Arrays.toString(ProductOfAnArray
                    .calcArray(new int[] {})));
            System.out
                    .println(Arrays.toString(ProductOfAnArray
                            .calcArray(new int[] { 4, 3, 2, 8, 3, 2, 4, 6, 7,
                                    3, 2, 4 })));
            System.out
                    .println(Arrays.toString(ProductOfAnArray
                            .calcArray(new int[] { 4, 3, 2, 8, 3, 2, 4, 6, 7,
                                    3, 2, 4 })));
            System.out.println(Arrays.toString(ProductOfAnArray
                    .calcArray(new int[] { 4432432, 23423423, 34234, 23423428,
                            4324243, 24232, 2342344, 64234234, 4324247,
                            4234233, 234422, 234244 })));
        } catch (Exception e) {
            // debug exception here and log.
        }
    }

    /*
     * Problem: Given an array of integers, produce an array whose values are
     * the product of every other integer excluding the current index.
     *
     * Assumptions. Input array cannot be modified. input is an integer array
     * "produce an array" - type not specified for output array
     *
     * Logic explanation:
     *
     * Assume we have array [a,b,c,d] Let multiple be multiple of each element
     * in array. Hence multiple = 0 if one of the element is 0; To produce the
     * output. Ans at i -> multiple divided by the value at i. if 2 numbers are
     * 0 then entire output will be 0 because atleast one 0 will be a multiple
     * if 1 number is 0 then every thing else will be 0 except that index whole
     * value is to be determined
     *
     */
    public static BigInteger[] calcArray(final int[] inp) throws Exception {
        if (inp == null)
            throw new Exception("input is null");

        int cnt = 0;
        BigInteger multiple = new BigInteger("1");
        boolean foundZero = false;

        for (int i : inp) {
            if (i == 0) {
                cnt++;
                foundZero = true;
                if (cnt < 2)
                    continue;
                else
                    break;
            }
            multiple = multiple.multiply(BigInteger.valueOf(i));
        }

        BigInteger ans[] = new BigInteger[inp.length];

        for (int i = 0; i < inp.length; i++) {
            if (foundZero) {
                if (cnt < 2) {
                    ans[i] = (inp[i] == 0) ? multiple : new BigInteger("0");
                } else {
                    ans[i] = new BigInteger("0");
                }
            } else {
                ans[i] = multiple.divide(BigInteger.valueOf(inp[i]));
            }
        }
        return ans;
    }

}

But I was not selected. I would like to get feedback on my answer and see what's wrong.

For starters, you have very weird line breaks:

System.out
        .println(Arrays.toString(ProductOfAnArray
                .calcArray(new int[] { 4, 3, 2, 8, 3, 2, 4, 6, 7,
                        3, 2, 4 })));

That's not layout, that's obfuscation! When dealing with long method chains, it may be preferable to break before each method invocation:

someObject
  .thisIs()
  .aFluid()
  .interface();

but that's not the case here, and this layout does not increase readability. Instead, break after the opening paren of the argument list:

System.out.println(
    Arrays.toString(ProductOfAnArray.calcArray(new int[] {
        4, 3, 2, 8, 3, 2, 4, 6, 7, 3, 2, 4
    }))
);

or something like that. Note that you could have written this as

System.out.println(
    Arrays.toString(ProductOfAnArray.calcArray(
        4, 3, 2, 8, 3, 2, 4, 6, 7, 3, 2, 4
    ))
);

(i.e. without the explicit array) when declaring your function with varargs: calcArray(int... inp).

The next question is why you are returning an array – a List<BigInteger> would be more flexible – arrays have little use in modern Java (the usage of varargs implies arrays, and high-performance code may need them. But in all other cases, they are too inflexible – e.g. arrays and generics don't work together).

But why BigInteger? Yes, you don't want to overflow, but we can assume that your interviewers would be content with an int. If you want to allow larger numbers, the first step would be long. Big Integers are truly awkward to use and necessarily less performant that native types, so I would avoid them wherever possible.

Your code inside calcArray is unnecessarily clever. Let's look at this loop:

for (int i : inp) {
    if (i == 0) {
        cnt++;
        foundZero = true;
        if (cnt < 2)
            continue;
        else
            break;
    }
    multiple = multiple.multiply(BigInteger.valueOf(i));
}

Notice that foundZero is equivalent to cnt > 0, and that you aren't using braces around the bodies of the inner if/else. Your continue is rather confusing, and the following expresses the control flow much better:

for (int i : inp) {
    if (i != 0) {
        multiple = multiple.multiply(BigInteger.valueOf(i));
    }
    else {
        cnt++;
        if (cnt >= 2) {
            break;
        }
    }
}

This conditional in the next loop is also too complex, and could be simplified to

if (cnt < 2 && inp[i] == 0) {
    ans[i] = multiple;
} else {
    ans[i] = new BigInteger("0");
}

When applying these criticisms, the Simplest Thing That Could Work might be this:

public static List<Long> specialProduct(int... numbers) {
    long product = 1;
    int zeroesCount = 0;
    for (int n : numbers) {
        if (n != 0) {
            product *= n;
        }
        else {
            zeroesCount++;
        }
    }

    List<Long> output = new ArrayList<>(numbers.length);

    switch (zeroesCount) {
    case 0:
        // with no zeroes, we don't have to perform any checks
        for (int n : numbers) {
            output.add(product / n);
        }
        break;
    case 1:
        // with one zero, the all others will become zero too
        for (int n : numbers) {
            if (n == 0) {
                output.add(product);
            }
            else {
                output.add(0L);
            }
        }
        break;
    default:
        // two or more zeroes make everything zero
        for (int i = 0; i < numbers.length; i++) {
            output.add(0L);
        }
        break;
    }

    return output;
}

Oh, and naming! Have vocals become so precious that we must write cnt instead of count? Must we write ans instead of answer if you can actually write a long word like multiple? And what the heck is inp?

Your code is not bad, but it isn't good, and could certainly have been written better. The next time, maybe :)


Comparison of incoming error strings to known set and transformation into 3rd string I have a function where I am comparing an error string returned from a web service to several lists of known strings, and then based on which list the error is a member of, returning from the function a more 'user-friendly' error to display.

I am hoping someone could offer some tips on making it shorter and more efficient.  I think I'm missing some obvious shortcuts, but I am not sure what.

def interpret_error(error):
    display_error_1 = 'User error 1'

    display_error_2 = 'User error 2'

    display_error_3 = 'User error 3'

    error_list_1= ["Failure: 'mean_va' is not in list",
                   "Failure: No JSON object could be decoded",
                   "Failure: Expecting ':' delimiter: line 1 column 14410098 (char 14410097)",
                   "Failure: cannot concatenate 'str' and 'ContentDecodingError' objects",
                   "Failure: Unknown error",
                   "Failure: Error returned by EPA WATERS web service: No Results Returned"]

    error_list_2 = ["Failure: Error returned by EPA WATERS web service: Flowline comid 4505568 at measure 79.99388 not found in NHDPlus stream network",
                    "Failure: Error returned by EPA WATERS web service: No Catchments match navigation results"]

    error_list_3 = ["Failure: Error returned by EPA WATERS web service: Start permid must have a hydroseq greater than the hydroseq for stop permid",
                    "Failure: Error returned by EPA WATERS web service: Start measure must be greater than stop measure when navigating a single reach"]

    match1 = 0.0
    match2 = 0.0
    match3 = 0.0
    temp_match = 0.0

    for internal_error in error_list_1:
        temp_match = fuzz.token_set_ratio(error,internal_error)
        match1 = max(temp_match, match1)

    temp_match = 0.0

    for internal_error in error_list_2:
        temp_match = fuzz.token_set_ratio(error,internal_error)
        match2 = max(temp_match, match2)

    temp_match = 0.0

    for internal_error in error_list_3:
        temp_match = fuzz.token_set_ratio(error,internal_error)
        match3 = max(temp_match, match3)

    if match1 > match2 and match1 > match3:
        return display_error_1
    elif match2 > match1 and match2 > match3:
        return display_error_2
    elif match3 > match1 and match3 > match2:
        return display_error_3
    else:
        return "Error of unknown cause"

The inner for loops can be made easier:

match1 = max(fuzz.token_set_ratio(error, internal_error) for internal_error in error_list1)

And similar for match2 and match3.

However, the real complixity in your implementation lies in the determining of the error message to show.

I would suggest building a dictionary mapping the display error to internal errors:

def interpret_error(error):
    error_mapping = {
        "User error 1": [
            "Failure: 'mean_va' is not in list",
            "Failure: No JSON object could be decoded",
            "Failure: Expecting ':' delimiter: line 1 column 14410098 (char 14410097)",
            "Failure: cannot concatenate 'str' and 'ContentDecodingError' objects",
            "Failure: Unknown error",
            "Failure: Error returned by EPA WATERS web service: No Results Returned",
        ],
        "User error 2": [
            "Failure: Error returned by EPA WATERS web service: Flowline comid 4505568 at measure 79.99388 not found in NHDPlus stream network",
            "Failure: Error returned by EPA WATERS web service: No Catchments match navigation results",
        ],
        "User error 3": [
            "Failure: Error returned by EPA WATERS web service: Start permid must have a hydroseq greater than the hydroseq for stop permid",
            "Failure: Error returned by EPA WATERS web service: Start measure must be greater than stop measure when navigating a single reach",
        ],
    }

    display_error = None
    score = 0.0
    for message, internal_errors in error_mapping.items():
        tmp = max(fuzz.token_set_ratio(error, internal_error) for internal_error in internal_errors)

        # This matches better than any earlier internal_error.
        if tmp >= score:
            score = tmp
            display_error = message

    return display_error

Ideally: move the mapping to a separate .py file, or some form of config file (.json, .yaml or even .xml), or the database.


Finding the nth triangular number def triangularNumber(n):
    if(n == 1):
        return 1
    else:
        return triangularNumbers(n-1)+n

works great to find the nth triangular number up to 996...
I get some error in the recursion after 996. But can tell what it is because it just keeps running through the loop. Anyone see the error?

Using recursion is generally advised against, as it takes longer that iterating, and can run into what you ran into, a RuntimeError: maximum recursion depth exceeded. Note you can change the maximum recursion depth, but it is generally advised against, and can lead to stack or memory overflows. In this case, you should iterate.

def triangular_number(n):
    i = n
    while True:
        if i == 1:
            return n
        i -= 1
        n += i

(This keeps running in the same function call, reducing the addition factor by one each time.)

Or better yet, use a range (or xrange for Python 2):

def triangular_number(n):
    for i in range(n):  # range(3) is a generator for [0, 1, 2]
        n += i
    return n

Or better yet, use sum:

def triangular_number(n):
    return sum(range(n + 1))

Or, even better, use the formula for triangle numbers.

def triangular_number(n):
    return n * (n + 1) // 2  # Convert to int for Python 2


Implementing foldl1 in Haskell As an exercise (scroll to the first set of exercises) for learning Haskell, I have to implement foldl1.

I believe I have implemented it successfully and while there is an answer available, it would be great to have the eye of an expert and more importantly, the thought process of why certain decisions were made.

Below is my code.

foldl1' :: (a -> a -> a) -> [a] -> a
foldl1' f [a] = a
foldl1' f xs =
  foldl1' f ((f (xs !! 0) (xs !! 1)):(drop 2 xs))

First, I like the explicit statement of the type signature. That's a good habit to get into, and makes it easier to capitalise on perhaps the greatest strength of using Haskell which is all the compile time checking. The provided signature is as general as it can be for lists.
Second, the single element base case is written cleanly and correctly.
The recursive case has room for a bit of picking apart. It is convention to use pattern matching syntax x:xs (or x1:x2:xs) for list recursive functions. As well as being cleaner to read, the behaviour is slightly different in that it can work out the first, second, and remainder of the list in a single pass without having to separately call !! twice and drop once.
foldl1' f (x1:x2:xs) = foldl1' f ((f x1 x2):xs)

One other improvement that I would suggest, taken directly from the prelude function by the same name, is explicitely handling the failure case when provided with an empty list. For comparison the inbuilt function produces an exception:

foldl1 (+) []
*** Exception: Prelude.foldl1: empty list


Spawning child processes and event-listening This function scrapes data from a webpage by spawning a process that executes a CasperJS web scraping script. The spawned process outputs data to stdout.

This function has an event listener for stdout and this is how the parent process (Node) gets the scraped data back. How can I make this more modular? Also, would creating a child_process class make more sense?

function scrapeLinks(location, callback) {

    // stores any data emitted from the stdout stream of spawned casper process
     var processData = "";

     // stores any errors emitted from the stderror stream of spawned casper process
     var processError = "";

    // initialises casperjs link scraping script as spawned process
    var linkScrapeChild = child_process.spawn(casperjsPath, ['casperLinkScript.js ' + location]);

     linkScrapeChild.stdout.on('data', function onScrapeProcessStdout(data) {
         processData += data.toString();
         console.log(data.toString())
     });

     linkScrapeChild.stderr.on('data', function onScrapeProcessError(err) {
         processError += err.toString();
     });

     linkScrapeChild.on("error", function onScrapeProcessError(err) {
         processError = err.toString();
     });

    //once spawned casper process finishes execution call the callback
    linkScrapeChild.on('close', function onScrapeProcessExit(code) {

        console.log('Child process - Location Scrape:  ' + location + ' - closed with code: ' + code);

         processData = convertToArray(processData);

         // filter out non valid listing links
         listingLinks = filterLinks(processData);

         //console.log(listingLinks);

         // filter duplicates
         var uniqueLinks = [ ...new Set(listingLinks) ];

        if(uniqueLinks.length === 0){
            processError += 'No valid listings found for ' + location
        }

         logScrapeResults(processError, uniqueLinks, location);

         callback(processError || null, uniqueLinks);
    });
}

First of all, I think spawning a child process just to scrape for links is overkill. If you're scraping links from multiple online pages, the bottleneck isn't the processing speed but your network latency. Once you grab the page contents, scraping a page is a breeze. A single node process written asynchronously can do this easily.

Now if you really need to spawn that child process, then I might suggest you use Promises, arrow functions and the latest version of node to make this code a bit more compact. Use template strings to construct strings instead of concatenation. Don't forget to define var/let/const.

function scrapeLinks(location) {

  return new Promise((resolve, reject) => {

    var processData = "";
    var errors = "";
    var command = `casperLinkScript.js ${location}`;
    var linkScrapeChild = child_process.spawn(casperjsPath, command);

    linkScrapeChild.stdout.on('data', (data) => processData += data.toString());
    linkScrapeChild.stderr.on('data', (err) => errors += err.toString());
    linkScrapeChild.on("error", (err) => errors = err.toString());
    linkScrapeChild.on('close', function onScrapeProcessExit(code) {

      var uniqueLinks = [...new Set(filterLinks(convertToArray(processData)))];

      if (!uniqueLinks.length) errors += `No valid listings found for ${location}`;

      if (errors)
        reject({ code, errors });
      else
        resolve({ code, uniqueLinks });
    });
  });
}

// Usage
scrapeLinks('http://yahoo.com').then((result) => {
  // result.code
  // result.uniqueLinks
}, (result) => {
  // result.code
  // result.errors
});


Print the lexicographically smallest permutation that satisfies the given formation Problem Statement:

  A DOTA game has N heroes, each with a distinct rank from [1..N]. In
  DOTA every formation is characterized as a permutation [1...N] of
  ranks of players.  A formation is Imba when the sum of ranks of every
  two consecutive players is less than or equal to (N+1). Given N, you
  are to print the lexicographically smallest permutation of ranks
  [1...N] that makes the formation Imba.

  Input Format

  The first line will contain an integer T, i.e. the number of the test
  cases followed by T lines, each containing the value of N.

  Constraints

  \$1 \le T \le 5\$
  \$2 \le N \le 105\$

  Output Format

  \$T\$ lines each containing the permutation; the numbers in each line
  should be seperated by a single space.

  Sample Input

2
2
3

  Sample Output

1 2
2 1 3

  Explanation

  In the first case there are two possible permutations [1,2] and [2,1].
  Both of the given permutations satisfy the given constraints and [1,2]
  is lexicographically smaller than [2,1].  In the second case, the two
  possible permutations are [2,1,3] and [3,1,2], of which the former is
  lexicographically smaller.

Is there a more efficient way to go about doing this?

from itertools import permutations
def check_condition(perm,checksum):
    for i in range(len(perm)-1):
        if perm[i] + perm[i+1] <= checksum:
            continue
        else:
            return False
    return True
testcases = int(raw_input())
current = 1
while current <= testcases:
    max_ = int(raw_input())
    checksum = max_ + 1
    list_ = range(1,max_+1)
    for perm in permutations(list_):
        #print(perm)
        if check_condition(perm,checksum):
            print(" ".join(map(str,perm)))
            break
    current += 1

If you print out the solutions for the first couple of numbers,
a pattern becomes obvious:

def print_first_imba(max_):
    """
    >>> print_first_imba(2)
    1 2
    >>> print_first_imba(3)
    2 1 3
    >>> print_first_imba(4)
    2 3 1 4
    >>> print_first_imba(5)
    3 2 4 1 5
    >>> print_first_imba(6)
    3 4 2 5 1 6
    >>> print_first_imba(7)
    4 3 5 2 6 1 7
    """

The pattern looks like:

given an input sequence 1, 2, 3, ..., n, and an empty output list
pop the last number from the sequence and prepend to the output
pop the first number from the sequence and prepend to the output
keep popping until there are no elements left in the sequence

That will be substantially faster than searching the permutation space.

The doc tests I wrote above should help a lot in implementing this alternative algorithm. Your current implementation passes these tests btw, if you reorganize your code to fit it into this method:

checksum = max_ + 1
list_ = range(1, max_ + 1)
for perm in permutations(list_):
    if check_condition(perm, checksum):
        print(" ".join(map(str, perm)))
        break

My alternative implementation also passes these tests:

from collections import deque

imba = deque()
highest = max_
lowest = 1
while lowest <= highest:
    imba.appendleft(highest)
    highest -= 1
    if lowest < highest:
        imba.appendleft(lowest)
        lowest += 1
print(" ".join(map(str, imba)))

In case you don't know, you can run doc tests with python -m doctest yourfile.py.

I also recommend to go a bit further and separate printing from the main logic, by renaming this method to get_first_imba and make it return perm instead of printing it.


Tips for Python build scripts? File manipulation code, I've noticed, has two salient properties:

  It's everywhere, and vitally important to functional software
  It has lots of exceptions.

Please give me some pointers on how I can better improve the usability of this rough draft I wrote yesterday. It assumes some very basic things, and was scrapped together after browsing Google as I stumbled through the concepts.

makeproject.py

'''create a simple setup folder for a new project

Follows the guide posted at:
http://guide.python-distribute.org/quickstart.html#lay-out-your-project

The structure built will match the following:

| ProjectName/
|---> LICENSE.txt
    | README.txt
    | setup.py
    | projectname/
    | ---> __init__.py

#For the sake of best practices, __init__.py is left empty.
'''
#! usr/bin/env python

import sys, os, errno

class NewProject(object):
    '''interface for building a directory with a new project in it'''

    def __init__(self, projectname, directory):
        self.project = projectname
        self.base = check_path(directory)
        #project defaults: I don't want them here as class attributes!
        self.SETUP = {

'README.txt' :
'''This is an unmodified README text file.
It contains a list of changes to the program.
It also contains help for dealing with the application.
''',

'LISCENSE.txt' :
'''Creative Commons Attribution-NonCommercial-ShareAlike 3.0 Unported License
'''
}

    def newproject(self):
        '''creates a file structure for a new project at `directory`
        '''
        self.name = self.project.replace(' ', '')
        self.path = os.sep.join([self.base, self.name])

        sub = self.name.lower()
        subpath = os.sep.join([self.path, sub])

        check_build_path(subpath)
        for filename, content in self.SETUP.items():
            self.buildfile(filename, content, directory=self.path)
        #setup takes arguments, it has its own method
        setup = self.buildsetup()
        self.buildfile('setup.py', setup)
        self.buildfile('__init__.py', directory=subpath)

    def buildfile(self, name, content="", directory = ""):
        '''opens and creates a new file at `directory` with `contents`'''
        #assumes bad directories have been purified
        if directory == "":
            loc = os.sep.join([self.path, name])
            w = open(loc, 'w')
        else:
            directory = os.sep.join([directory, name])
            w = open(directory, 'w')
        w.write(content)
        w.close()

    def buildsetup(self):
        return '''from distutils.core import setup

setup(
    name='{0}',
    version='0.1dev',
    packages=['{1}',],
    license=open('LISCENSE.txt').read(),
    long_description=open('README.txt').read(),
)
'''.format(self.project, self.name.lower())

def check_path(loc):
    '''recursively check if last character in `loc` is like os.sep character.
    If so, remove.'''
    if loc[-1] == os.sep:
        return check_path(loc[:-1])
    return loc

def check_build_path(loc):
    d = os.path.normpath(loc)
    if not os.path.exists(d):
        os.makedirs(d)
    else:
        try:
            os.rmdir(d)
        except OSError as ex:
            if ex.errno == errno.ENOTEMPTY:
                print "Directory specified must be new or empty"
                sys.exit(1)
        #if delete was successful, build the directory
        check_build_path(loc)

if __name__ == '__main__':
    if len(sys.argv) != 3:
        print "Usage: string 'Project Name', string '/abs/path'"
        sys.exit(1)
    project = NewProject(sys.argv[1], sys.argv[2])
    project.newproject()

'''create a simple setup folder for a new project

The recommendation is to always use """ not ''' for docstrings

Follows the guide posted at:
http://guide.python-distribute.org/quickstart.html#lay-out-your-project

The structure built will match the following:

| ProjectName/
|---> LICENSE.txt
    | README.txt
    | setup.py
    | projectname/
    | ---> __init__.py

For the sake of best practices, __init__.py is left empty.

'''

#! usr/bin/env python

import sys, os, errno

class NewProject(object):
    '''interface for building a directory with a new project in it'''

    def __init__(self, projectname, directory):

The python style guide recommends underscores to separate words
            self.project = projectname

I'd call it self.name not self.project. The name project doesn't convey much useful information.

        self.base = check_path(directory)
        #project defaults: I don't want them here as class attributes!
        self.SETUP = {

'README.txt' :
'''This is an unmodified README text file.
It contains a list of changes to the program.
It also contains help for dealing with the application.
''',

'LISCENSE.txt' :
'''Creative Commons Attribution-NonCommercial-ShareAlike 3.0 Unported License
'''
}

I wouldn't mix large docstrings into my python files. I think that makes it rather hard to read.

    def newproject(self):
        '''creates a file structure for a new project at `directory`
        '''
        self.name = self.project.replace(' ', '')

I dislike adding object attirbutes not present in the constructor. It makes it harder to know what attributes an object will have

        self.path = os.sep.join([self.base, self.name])

Use os.path.join for joining paths together. It'll handle a few corner cases that os.sep.join won't.

        sub = self.name.lower()
        subpath = os.sep.join([self.path, sub])

        check_build_path(subpath)
        for filename, content in self.SETUP.items():
            self.buildfile(filename, content, directory=self.path)
        #setup takes arguments, it has its own method
        setup = self.buildsetup()
        self.buildfile('setup.py', setup)
        self.buildfile('__init__.py', directory=subpath)

I'd probably pass a blank content for __init__.py rather then  having it be the default. I figure its rate that you want to have a file like that, so making it default doesn't make sense.

    def buildfile(self, name, content="", directory = ""):
        '''opens and creates a new file at `directory` with `contents`'''
        #assumes bad directories have been purified
        if directory == "":
            loc = os.sep.join([self.path, name])
            w = open(loc, 'w')
        else:
            directory = os.sep.join([directory, name])

That's not a directory if you've saved the file name over it.
                w = open(directory, 'w')

If you use os.path.join, you won't need to handle these two cases separately.
            w.write(content)
            w.close()

I recommend you use with statement to make sure the file closes correctly

    def buildsetup(self):
        return '''from distutils.core import setup

setup(
    name='{0}',
    version='0.1dev',
    packages=['{1}',],
    license=open('LISCENSE.txt).read(),
    long_description=open('README.txt').read(),
)
'''.format(self.project, self.name.lower())

Again, I don't like putting huge docstrings in the middle of the code. I recommend having a template folder that you copy over to the target. Just do search/replace on filenames and contents for a few keywords like NAME or MODULE.

def check_path(loc):
    '''recursively check if last character in `loc` is like os.sep character.
    If so, remove.'''
    if loc[-1] == os.sep:
        return check_path(loc[:-1])
    return loc

This is the sorta thing that os.path.join will do for you. I'd also avoid recursion,  because its pretty easy to make this a while loop. Also what happens when loc is "/"

def check_build_path(loc):
    d = os.path.normpath(loc)

I discourage single letter variable names whenever I can

    if not os.path.exists(d):
        os.makedirs(d)
    else:
        try:
            os.rmdir(d)
        except OSError as ex:
            if ex.errno == errno.ENOTEMPTY:
                print "Directory specified must be new or empty"
                sys.exit(1)

You should reraise the exception if it was something else.

        #if delete was successful, build the directory
        check_build_path(loc)

This function would be simpler if done like this:

try:
    os.rmdir(d)
except OSError as error:
    if error == ERROR_CODE_FOR_NOT_EXIT:
        pass # OK
    else:
        print "Failed to delete because: ", error
        sys.exit(1)
os.makedirs(d)

I think that conveys the logic better

if __name__ == '__main__':
    if len(sys.argv) != 3:
        print "Usage: string 'Project Name', string '/abs/path'"
        sys.exit(1)
    project = NewProject(sys.argv[1], sys.argv[2])
    project.newproject()


Getting current weather data using OpenWeatherMap API I would really appreciate if someone could take a look at this code. I am not very advanced, so ANY kind of feedback and critique would be precious to me.

What this program is supposed to do is to go to a given URL, download the list of cities, for which it will download the data, based on a https://openweathermap.org/api . It should be run on a regular basis and write all of the results to the CSV file.

It is divided into two parts. The first one consists only of the scheduler, final function which is being run by it and the list of columns I want to get as the final result.

Code is written for Python 3.6.

from API.helpers import get_weather_data, json_to_df, create_dict
import schedule, time

URL = 'https://pm1aapplicantsdata.blob.core.windows.net/databases/CitiesWeather/CitiesWeather.csv'
columns = ["name","sys.country","main.temp",
           "main.humidity","main.pressure",
           "visibility", "wind.speed"]
#Writing results to CSV
def weather_api(URL):
    dict = create_dict(URL)
    for city, code in dict.items():
        data = get_weather_data(city, code)
        json_to_df(data, columns)
schedule.every().day.at("10:30").do(weather_api, URL)
while True:
    schedule.run_pending()
    time.sleep(1)

Here is the second part, which is my "helper" file.

import json
import requests
from pandas.io.json import json_normalize
import pandas as pd
import os
import requests
import csv
api_key = "xxxxxxxxxxxxxxxxxxxxxxx"

#function to build api requests
def build_request(city, code):
    base_url = "http://api.openweathermap.org/data/2.5/weather?"
    complete_url = base_url + "appid=" + api_key + "&q=" + city +"," + code
    return complete_url

#function to get weather data
def get_weather_data(city, code):
    url = build_request(city, code)
    try:
        response = requests.get(url)
        response.status_code
    except requests.exceptions.HTTPError:
        print('Error occured while downloading data')
    except requests.exceptions.URLError:
        print('Error occured while downloading data')
    citydataJSON = response.text
    citydata = json.loads(citydataJSON)
    return citydata

def json_to_df(data, columns):
    df = pd.DataFrame.from_dict(json_normalize(data), orient='columns')
    new_df = df[columns]
    new_df.insert(0, 'TimeStamp', pd.datetime.now().replace(microsecond=0))
    if not os.path.isfile('weather.csv'):
        return new_df.to_csv('weather.csv', header='column_names', index=False)
    else:
        return new_df.to_csv('weather.csv', mode='a', header=False, index=False)
#creating a dictionary of cities and city codes(based on the CSV file downloaded from a given URL
def create_dict(URL):
    with requests.Session() as s:
        dict = {}
        download = s.get(URL)
        decoded_content = download.content.decode('utf-8')
        cs = csv.reader(decoded_content.splitlines(), delimiter=',')
        next(cs, None)
        my_list = list(cs)
        for row in my_list:
            dict[row[0]] = row[1]
        return dict

Your code is very nicely split into functions. As a next step I would split it to classes. What you want to do is

Collect the data for a given city
Convert it into a schema that fits your purposes and write it to a file

In my opinion, the scheduling of this job should be done elsewhere, e.g. cron

Although the +-sign concatenates strings correctly it is not the most readable way to do it. There are many ways to format strings in Python3 and I tend to use f-strings.

Pandas is a great library for data analysis and data "wrangling" but to use it to write csv files is simply an overkill. Usually, you want to keep your virtual environmets as small as possible and use the standard library as much as possible. In this case the csv library is all you need.

I think you got somehow lost when modifying the API response. There is absolutely no need to convert it from dictionary to json and then back to tabular form. Simply use the dictionary that requests gives you, get the data you need and write it to a file.

Here's my code:

from os.path import isfile
from io import StringIO
from datetime import datetime
from csv import DictWriter
import requests

URL = 'https://pm1aapplicantsdata.blob.core.windows.net/databases/CitiesWeather/CitiesWeather.csv'

class CityWeatherCollector(object):
    """Class to collect weather information for a given city from openweathermap.org
    """

    base_url = "http://api.openweathermap.org/data/2.5/weather?"
    apikey = 'xxxxxxxxxxxxxxxxxxxxxxx'

    def __init__(self, city, code):
        self.city = city
        self.code = code

    @property
    def full_url(self):
        return self.base_url + f"appid={self.apikey}&q={self.city},{self.code}"

    def fetch_data(self):
        response = requests.get(self.full_url)
        if response.status_code == 200:
            #returns a dictionary
            return response.json()
        else:
            raise WeatherCollectionError(f"Response from API was: {response.status_code}")

class WeatherCollectionError(Exception):
    pass

class WeatherDataWriter(object):

    def __init__(self, full_filepath):
        self.full_filepath = full_filepath

class WeatherData(object):
    """Class for a representation of the data"""

    def __init__(self, name, country, temp, humidity, pressure, visibility, wind_speed, timestamp=datetime.now()):
        self.name = name
        self.country = country
        self.temp = temp
        self.humidity = humidity
        self.pressure = pressure
        self.visibility = visibility
        self.wind_speed = wind_speed
        self.timestamp = timestamp

    @staticmethod
    def create_from_json(json_dict, timestamp=datetime.now()):
        return WeatherData(
            name=json_dict['name'],
            country=json_dict['sys']['country'],
            temp=json_dict['main']['temp'],
            humidity=json_dict['main']['humidity'],
            pressure=json_dict['main']['pressure'],
            visibility=json_dict['visibility'],
            wind_speed=json_dict['wind']['speed'],
            timestamp=timestamp
        )

    def write_one(self, outfile):
        weather_data = self.__dict__
        # if file exists, append
        csv_writer = DictWriter(
            out_file,
            fieldnames=list(weather_data.keys()),
            delimiter=',',
            lineterminator='\n'
            )
        #if the file is empty write header
        if outfile.tell() == 0:
            csv_writer.writeheader()
        csv_writer.writerow(weather_data)

def get_cities(url=URL):
    response = requests.get(url)
    if response.status_code == 200:
        decoded_response = StringIO(response.content.decode('utf8'))
        # pop the headings
        next(decoded_response)
        for line in decoded_response:
            city, code = tuple(line.strip().split(','))
            yield CityWeatherCollector(city, code)

if __name__ == '__main__':
    timestamp = datetime.now()
    with open('data.csv', 'a') as out_file:
        for collector in get_cities(URL):
            # add error handling
            full_data = collector.fetch_data()
            data = WeatherData.create_from_json(json_dict=full_data, timestamp=timestamp)
            data.write_one(out_file)


Find minimum window size which includes the subset of characters Given a set T of characters and a string S, find the minimum window in S which will contain all the characters in T in complexity O(n).

eg,
S = “ADOBECODEBANC”
T = “ABC”

Minimum window size is 4 - “BANC”.

public final class MinSubstringWindow {

    private MinSubstringWindow() {}

    private static void addCharCount(Map<Character, Integer> map, Character ch) {
        if (!map.containsKey(ch)) {
            map.put(ch, 1);
        } else {
            int val = map.get(ch);
            map.put(ch, val + 1);
        }
    }

    private static Map<Character, Integer> toFindMap(String subset) {
        Map<Character, Integer> toFind  = new HashMap<Character, Integer>();
        for (char ch : subset.toCharArray()) {
            addCharCount(toFind, ch);
        }
        return toFind;
    }

    public static int mindWindowSize(String str, String subset) {
        final Map<Character, Integer> toFind = toFindMap(subset);
        final Map<Character, Integer> hasFound  = new HashMap<Character, Integer>();

        char[] ch  = str.toCharArray();

        int matchCtr = 0;
        boolean matchFound = false;
        char currLeftMostChar = 0;

        int j = 0; // the trailing position of the sliding window
        int i = 0; // the leading position of the sliding window.

        int min = Integer.MAX_VALUE;

        for (i = 0; i < ch.length; i++) {

            if (!toFind.containsKey(ch[i])) continue;

            if (!matchFound) {
                currLeftMostChar = ch[i];
                matchFound = true;
                j = i;
            }

            addCharCount(hasFound, ch[i]);

            matchCtr++;

            // check if match has been completed.
            if (matchCtr >= subset.length()) {
                if ((i - j + 1) < min) {
                    min = i - j + 1;
                }
            }

            // does the first element exceed value ?
            if (hasFound.get(currLeftMostChar) > toFind.get(currLeftMostChar)) {
                // advance the left pointer, such the window (i-j) is as small as possible.
                while (!toFind.containsKey(ch[j]) || hasFound.get(ch[j]) > toFind.get(ch[j])) {
                    if (hasFound.containsKey(ch[j])) {
                        int val = hasFound.get(ch[j]);
                        hasFound.put(ch[j], val - 1);
                    }
                    j++;
                }
                currLeftMostChar = ch[j];
            }
        }

        if (matchCtr < subset.length()) {
            throw new IllegalArgumentException("The subset is not found in the input string.");
        }

        // note: here we dont do (i-j+1) since i has been incremented additionally in a for loop.
        return min > (i - j) ? i - j : min;
    }
}

And test section

public class MinSubstringWindowTest {

    @Test
    public void test1() {
        assertEquals(3, MinSubstringWindow.mindWindowSize("apbqcbsa", "ab"));
        assertEquals(4, MinSubstringWindow.mindWindowSize("apbqcbsa", "abc"));
        assertEquals(3, MinSubstringWindow.mindWindowSize("apbqcbsa", "pq"));
    }

    @Test
    public void test2() {
        assertEquals(2, MinSubstringWindow.mindWindowSize("ameya", "ay"));
        assertEquals(3, MinSubstringWindow.mindWindowSize("ameya", "my"));
        assertEquals(3, MinSubstringWindow.mindWindowSize("ameya", "ae"));
    }

}

The first big issue that crosses my mind: Naming. In particular, commenting variables is always a sign that you did a poor job on giving them good names:

int j = 0; // the trailing position of the sliding window
int i = 0; // the leading position of the sliding window.

There is no reason to name loop variables i and j just because. If they have a meaning beyond just being there for the sake of looping, give them a name representing this. It makes the following code easier to read because the reader won't have to look up and remember their meaning.

After giving them proper names you can also get rid of declaring them outside the loop. Declaring them inside the loop makes the coder smaller, which is good not for the sake of having less lines (LoC is a terrible measurement of quality), but it reduces the overhead when trying to understand what is going on.

This applies especially to i and j, but also to many other names in your program:

What is ch supposed to tell me? I don't need information about the type of a variable – the compiler and IDE do a lot of the work for me here; what I, as a reader, care about is the semantics of variables.
Is there really any need for shortening matchCounter to matchCtr? Your IDE will help you avoid having to type too much. Of course experienced programmers have "ctr" already in their vocabulary and automatically translate it to "counter", but there is not really any reason these days to add this extra translation step.
I'll just leave out the rest, typically the same arguments apply as the ones I stated above.

Let's move on. What was the reasoning for making everything static? You forced yourself to do this:

addCharCount(hasFound, ch[i]);

This is called a side-effect method and unless there is no way around it, it should be avoided. It is counter-intuitive that this method actually mutates  the passed map (and from experience: this can easily cause very messy code and ugly bugs).

Instead, just make your class a normal (non-static) class with state. It allows you to get rid of some method parameters as they just become class members.

What you can do is make the constructor private (as it is now) and add a static method that creates a new instance and calls the proper method on it. This ensures that the caller doesn't call the method twice on the same instance and you won't have to deal with resetting members.

Refactoring this to a proper class will also allow you to avoid this as well as the block following it:

// check if match has been completed.
if (matchCtr >= subset.length()) {
    if ((i - j + 1) < min) {
        min = i - j + 1;
    }
}

Again, comments are good for mostly one thing: they signal code smell. If you have an entire block that needs a comment to be explained, it is time to think about whether the better way to go here is to extract a method with a good name that replaces the comment, because – to me – the single most profound fact about comments is that they rot faster than anything else and even the best comments will soon start lying. In other words: comments tend to become not just useless, but dangerous.

And your method is already big enough; too big, I would say. It does too many things on the same level of abstraction, so I would try splitting it up further.

Now to this:

if (matchCtr < subset.length()) {
    throw new IllegalArgumentException("The subset is not found in the input string.");
}

Is it really an illegal argument? You can argue that it is, but I tend to think that it's not. Is an exception even the right way to go here? Is this exceptional behavior you want the caller to deal with because you can't decide what to do here?

Finally, the tests. Again, the thing that really jumps into my eyes are names. What is test and test2 supposed to tell me about the test case here? Choose proper names describing the test cases, e.g

testShortestWindowAtTheEndOfStringIsFound
testShortedWindowWithinStringIsFound
…

Also, at first glance I fail to see why you chose to split your test cases into two tests as you did. I would say that every single assertion here deserves a test on its own (with proper name). This way, if a test fails, the test name alone is enough to tell you what isn't working, you won't have to first find out which assertion went wrong and you can be sure that the other cases are still working.


Is This a Sufficient Demonstration of the Effects of Exception Handling We have some developers in house that believe it is best practice to use exception handing as flow control, as well as, thinking that catching and re-throwing exceptions is effective error handling.

In an effort to educated, I attempted to come up with a simple sample to demonstrate to them the negative effects of doing so.  The code sample below is what I came up with.  It consistently demonstrates that there is a penalty for gratuitous usage of try/catch blocks; but the effect is much greater than I had anticipated.  I can't help but think I've done something very wrong in this sample.  I would appreciate any suggestions of improvement.

In this sample, I am comparing the cost of catching and re-throwing an exception with just allowing the exception to bubble up.  I needed to execute the methods in parallel to avoid waiting days for the code to execute on high values.

    static void Main(string[] args)
    {
        TimeSpan ts1 = new TimeSpan();
        TimeSpan ts2 = new TimeSpan();
        List<Task> tasks = new List<Task>();

        for (int i = 0; i < 100; ++i)
        {
            Task a = new Task(() =>
            {
                Stopwatch watch1 = new Stopwatch();
                watch1.Start();
                function1(function2);
                watch1.Stop();
                ts1 += watch1.Elapsed;
            });
            a.Start();
            tasks.Add(a);

            Task b = new Task(() =>
            {
                Stopwatch watch2 = new Stopwatch();
                watch2.Start();
                function1(function3);
                watch2.Stop();
                ts2 += watch2.Elapsed;
            });
            b.Start();
            tasks.Add(b);
        }

        while (!tasks.All(t => t.IsCompleted)) ;
        Console.WriteLine("Watch1: " + ts1);
        Console.WriteLine("Watch2: " + ts2);
        Console.ReadLine();
    }

    static void function1(Action action)
    {
        try
        {
            action();
        }
        catch
        {
        }
    }

    static void function2()
    {
        try
        {
            int.Parse(null);
        }
        catch
        {
            throw;
        }
    }

    static void function3()
    {
        int.Parse(null);
    }

The while loop you are using is impacting performance.  Sleep that check a bit beforehand to give your scenarios some time to think.

do { Thread.Sleep(3000); } while (!tasks.All(t => t.IsCompleted));

You may want to suggest some best practices reading to your team, throwing an exception should be reserved for an exceptional circumstance.  Use of the throw for rethrowing an exception is correct to keep the stack trace information complete, but again, should be seldom and when they do finally bubble up to the top, the exception information should be captured in a way that they are not just ignored.

http://www.codeproject.com/KB/architecture/exceptionbestpractices.aspx

https://stackoverflow.com/questions/409563/best-practices-for-exception-management-in-java-or-c

http://www.codethinked.com/how-do-you-deal-with-exceptions


Autoclicker Tkinter Program I made my first program that I might actually use and I was wondering if anyone had some tips on how I could improve it.

My code is an auto-clicker that uses Python3 ctypes for the clicking and Tkinter for the GUI. I made this in 2 hours so there might be a lot of optimization I could do.

from tkinter import *
from tkinter import ttk
import ctypes
import time

mouse = ctypes.windll.user32
state = False

def StateFalse():
     global state
     global x
     if state == True:
          state = False
          x = 0

def StateTrue():
     global state
     global x
     if state == True:
          state = True
     else:
          x = 0
          state = True

root = Tk()
root.title("AutoClicker")
root.resizable(width=FALSE, height=FALSE)

amount = StringVar()
speed = StringVar()

amount.set(0)
speed.set(100)
mainframe = ttk.Frame(root)
mainframe.grid(padx=5, pady=5)

amountLabel = ttk.Label(mainframe, text="Number of Clicks\n(set to 0 for infinite)").grid(column=1, row=1, sticky=W)
speedLabel = ttk.Label(mainframe, text="Click interval\n(In milliseconds)").grid(column=1, row=2, sticky=W)
amountEntry = ttk.Entry(mainframe, textvariable=amount, width=5).grid(column=2, row=1)
speedEntry = ttk.Entry(mainframe, textvariable=speed, width=5).grid(column=2, row=2)
startButton = ttk.Button(mainframe, text="Start", width=10, command=StateTrue).grid(column=1, row=3,columnspan=2,sticky=W)
stopButton = ttk.Button(mainframe, text="Stop", width=10, command=StateFalse).grid(column=2, row=3, columnspan=2,sticky=E)

mainframe.bind("<F1>",StateTrue())
mainframe.bind("<F3>",StateFalse())

timekeeper = 0
x = 0

while True:
     timekeeper += 1
     if state == True:
          Amount = int(amount.get())
          Speed = int(speed.get())
          print(state)
          if Amount == 0:
               time.sleep(Speed / 1000)
               mouse.mouse_event(2, 0, 0, 0, 0)  # left mouse button down
               mouse.mouse_event(4, 0, 0, 0, 0)  # left mouse button up
               x += 1
               print("Clicked %s Times" % (x))

          elif x < Amount and state == True:
               time.sleep(Speed / 1000)
               mouse.mouse_event(2, 0, 0, 0, 0)  # left mouse button down
               mouse.mouse_event(4, 0, 0, 0, 0)  # left mouse button up
               x += 1
               print("Clicked %s Times" % (x))
               if x == Amount:
                    state = False
     root.update()

You are using global variables and connecting them with the functions StateFalse and StateTrue. It would be better to read and write the configuration options to a file.

def set_option(value):
    with open(CONFIG_FILE,"w+") as f:
        f.write(value)

def get_option():
    with open(CONFIG_FILE) as f:
        return f.read() == "True"

Where CONFIG_FILE is a filename of your choice that you choose to store the "state" (maybe you should find a better name).


SICP Exercise 1.3: Sum of squares of two largest numbers out of three The exercise 1.3 of the book Structure and Interpretation of Computer Programs asks the following:

  Exercise 1.3.  Define a procedure that takes three numbers as arguments and returns the sum of the squares of the two larger numbers.

My answer is this:

    var sumSquareLargest = function (x, y, z) {
        var numbers = [x, y, z];
        numbers.sort();
        return numbers[1] * numbers[1] + numbers[2] * numbers[2];
    };

Am I doing unnecessary work here? How could I improve this answer, even if only stylistically-speaking?

Finding the minimum of three numbers is easier than sorting three numbers, and it doesn't require creating a temporary array.

This runs about 10 times faster.

var sumSquareNotMin = function(x, y, z) {
    var min = Math.min(x, y, z);
    return -min * min + x * x + y * y + z * z;
};

As @Corbin mentioned, floating-point arithmetic is tricky.  I've arranged the negative term first, which should cope better with overflow.  However, in extremely unusual cases (such as when dealing with a mix of tiny and huge numbers), the result could differ from the original.


Opening a CSV file in Python, with error handling Trying to improve my function, as will be used by most of my code. I'm handling most common exception (IOError) and handling when data has no values.

READ_MODE = 'r'

def _ReadCsv(filename):
  """Read CSV file from remote path.

  Args:
    filename(str): filename to read.
  Returns:
    The contents of CSV file.
  Raises:
    ValueError: Unable to read file
  """
  try:
    with open(filename, READ_MODE) as input_file:
      data = input_file.read()
      if not data:
        raise ValueError('No data available')
  except IOError as e:
    logging.exception(e)
  return data

Don't handle the exception in this part of the code, since it will hide errors. Just let it bubble up.


Finding all contiguous sublists such that each member appears an even number of times The program needs to find all contiguous sublists of numbers where the number of occurrences of every member in the contiguous sublist is divisible by 2.

The first line of input is N (the number of ai, where N ≤ 353535).  The second line of input contains the ai, delimited by spaces (where each ai < 109).

Here is an example of input and what the output should be:

# N is the user input, and a is the user input with
#numbers separated by space and converted to list with .split(" ") but
#we'll look for examles first :
N = 8
a = [3,5,3,5,9,9,5,3]

The output of this input should be 5 because there are 5 contiguous sublists that meet the conditions:

[3,5,9,9,5,3]
[3,5,3,5,9,9]
[5,9,9,5]
[3,5,3,5]
[9,9]

Here is my solution:

import time
total = 0
try:
    listlength = int(raw_input("Enter the number of list elements >> "))
    listofnumbers   = raw_input("Enter the list elements , space separated >>").split(" ")
    start = time.clock()
    for i in range(listlength,1,-1):
        for i1 in range(0,listlength + 1 - i):
            numbers = []
            countofnumbers=[]
            currentlist = listofnumbers[i1:listlength-abs(i-listlength+i1)]
            #print "I:",i,",TR nIZ ",tr_niz
            for k in currentlist:
                if k not in numbers:
                    numbers.append(k)
                    countofnumbers.append(currentlist.count(k) if currentlist.count(k)%2==0 else 0)
            if 0 not in countofnumbers:
                if sum(countofnumbers)%2==0:
                    total+=1

    print total
    print "Time: ",time.clock()-start

except ValueError:
    print "Data wrong"

My script solves small test cases really well.  However, when there are 350350 numbers, It was working all the night, and it didn't turn up with result.  How can I fix my algorithm to finish in a reasonable amount of time? Maybe implement multiprocessing because it's using 100% od my core when running , but doesn't use any of 3 other cores available  ? And maybe some enhanced algorithm you suggest :)

The algorithm can be slimmed by tracking only those numbers for which the number of occurrences in the current window is unbalanced (i.e. odd). Whenever this auxiliary structure is empty, everything is balanced and the current window can be added to the set of solutions. Here's some pseudo code to illustrate the approach:

static void print_balanced_contiguous_subsequences (int[] a)
{
    var unbalanced = new HashSet<int>();

    for (int i = 0, e = a.Length - 1; i < e; ++i)
    {
        unbalanced.Clear();

        for (int j = i; j <= e; ++j)
        {
            if (unbalanced.Contains(a[j]))
                unbalanced.Remove(a[j]);
            else
                unbalanced.Add(a[j]);

            if (unbalanced.Count == 0) // print the newly found sequence
                for (int k = i; k <= j; ++k)
                    Console.Write("{0}{1}", a[k], k < j ? ' ' : '\n');
        }
    }
}

However, this algorithm is still quadratic in n (20 ms for 1000 elements, 2 seconds for 10000) and the key idea for a proper solution still eludes me...

The original problem description might contain a clue, or some restriction that can be leveraged to design a faster solution. It would be nice if it could be quoted verbatim in the question, or at least linked (even if it is not in English).


Comparing string arrays My code does what I need it to do, but I think I am missing the "right" way to do this with comparing the arrays. Essentially what I need this to do is compare a textbox entry against two different arrays and redirect based on which value matches.

Protected void Button1_Click(object sender, EventArgs e)
{
      string[] str1 = new string[] {"87002", "87001", "87005"};
      string[] str2 = new string[] {"97002", "97003", "97004"};

      for (int i = 0; i < str1.Length; i++)
      {
          string comp1 = str1[i];
          if (comp1 == TextBox1.Text.ToString())
          {
              Response.Redirect("Page1.aspx");
          }
      }

      for (int i = 0; i < str2.Length; i++)
      {
          string comp2 = str2[i];
          if (comp2 == TextBox1.Text.ToString())
          {
              Response.Redirect("Page2.aspx");
          }
      }
      Response.Redirect("Unknown.aspx");
}

I would do this:

protected void Button1_Click(object sender, EventArgs e)
{
    string[] str1 = new string[] {"87002", "87001", "87005"};
    string[] str2 = new string[] {"97002", "97003", "97004"};

    if (str1.Contains(TextBox1.Text))
        Response.Redirect("Page1.aspx");

    if (str2.Contains(TextBox1.Text))
        Response.Redirect("Page2.aspx");

    Response.Redirect("Unknown.aspx");
}

You don't need the ToString() calls - TextBox.Text should already be a string.


Using third party library named "nsetools" for fetching stock data I want to make the code shorter.  Is there any way to loop through the stocks and qty dictionaries, access the closing prices of each individual stock and then calculate the current value of the investment preferably in one or two lines?

from nsetools import Nse
nse = Nse()

#stock name and buy price pair, but not used in my method below. I would  like a method that uses it though.
stocks = {
    "ADANIPORTS" : 284.95,
    "HINDALCO"   : 152.30,
    "ONGC"       : 279.70,
    "SBIN"       : 259.70
}

#stock name and quantity of stocks
qty = {
    "ADANIPORTS" : 20,
    "HINDALCO"   : 20,
    "ONGC"       : 20,
    "SBIN"       : 20
}

### Adaniports
#Fetches a dictionary with data such as best buy/sell prices, 'closePrice', etc.
stock1      = nse.get_quote('ADANIPORTS')
#Accessing the 'closePrice' from the above dictionary.
stock1Close = stock1['closePrice']

### Hindalco
stock2      = nse.get_quote('HINDALCO')
stock2Close = stock2['closePrice']

### ONGC
stock3      = nse.get_quote('ONGC')
stock3Close = stock3['closePrice']

### SBIN
stock4      = nse.get_quote('SBIN')
stock4Close = stock4['closePrice']

current_value = (stock1Close * qty['ADANIPORTS']) + (stock2Close * qty['HINDALCO']) + (stock3Close * qty['ONGC']) + (stock4Close * qty['SBIN'])

You never use stocks, so I removed it. It would also help if your qty actually had the exact same keys as you need for nse.get_quote (namely lowercase names), so I did that as well.

In addition, you can convert your whole code into one generator, which you can then sum over:

from nsetools import Nse
nse = Nse()

def get_closing(stock):
    return nse.get_quote(stock)['closePrice']

stock_qty = {"ADANIPORTS": 20,
             "HINDALCO": 20,
             "ONGC": 20,
             "SBIN": 20 }

stock_gen = (get_closing(stock) * qty for stock, qty in stock_qty.items())
current_value = sum(stock_gen)

If you already have stocks, you could just do:

stocks = {
    "ADANIPORTS" : 284.95,
    "HINDALCO"   : 152.30,
    "ONGC"       : 279.70,
    "SBIN"       : 259.70
}

#stock name and quantity of stocks
qty = {
    "ADANIPORTS" : 20,
    "HINDALCO"   : 20,
    "ONGC"       : 20,
    "SBIN"       : 20
}
current_value = sum(val * qty[stock] for stock, val in stocks.items())


Clone array elements when multiplying by a scalar I am building an array of strings to form the "empty" representation of a puzzle onto which I will overwrite single characters as necessary for debugging purposes. However, building the array using scalar multiplication results in shared string instances among the rows which breaks when I modify them.

> grid = [["... " * 3] * 3, ""].flatten * 3

This produces the desired output

> puts grid * "\n"

... ... ...
... ... ...
... ... ...

... ... ...
... ... ...
... ... ...

... ... ...
... ... ...
... ... ...

But the array contains only two string instances: nine of "... ... ..." and two of "". To solve this I use map to clone each string:

grid = ([["... " * 3] * 3, ""].flatten * 3).map(&:clone)

Is there a better way?

The main question here is how best to make sure each array element is a separate string instance.

I might do this:

grid = 9.times.map { "... " * 3 }

Or, better yet, as Naklion proposes in the comments:

grid = Array.new(9) { "... " * 3 }

to get the rows (as separate string instances), and this

grid.each_slice(3) { |rows| puts rows, "\n"  }

to print it similar to yours. But I wouldn't call either approach "idiomatic", just variations on a theme. I don't know if anything really idiomatic can be said to exist for this

However, it might be easier to approach this from a higher level, rather than rely on a string representation. I.e. parse the input grid into Cell, Row etc. objects (I'm obviously assuming this is sudoku we're talking about).
Of course, if you have a clever idea that relies on string manipulation, then go for it!

My point is more that I know you can get far with, say, bitwise representations of sudoku, but implementing that in Ruby (while totally possible) seems "crude". Ruby's not geared toward twiddling bits, but toward more high-level OOP abstractions. It'd be right at home in C/C++, though.

*) I'd go with "rubyesque" or just "ruby" as in "the ruby way of doing things" :)


API call return type and error handling I wrote the following method which would just call an API which would then update a user's subscription to something.
void updateSubscription(String userId, boolean subscribed){

        try{
            // some stuff
            restTemplate.exchange(.....)
           // some stuff
        }catch (HttpClientErrorException exception){
            log.error("API EXCEPTION - Received Status: {} - Stack Trace:",exception.getStatusCode(), exception);
            throw new ExternalAPIException("Service unavailable", HttpStatus.SERVICE_UNAVAILABLE);
        }catch (Exception e){
            log.error("API EXCEPTION - Stack Trace:", e);
            throw new ExternalAPIException("Internal Server Error", HttpStatus.INTERNAL_SERVER_ERROR);
        }
    }

I have two questions in particular:

Is the void return type okay? I am actually leaning towards returning a boolean indicating if the request was successful.
Is the error handling okay?

Note - it is a service class without a controller, so I can't use @controller's advice.

"okay" in programming often depends on context.
You chose to perform error handling by throwing exceptions, so there is no point in having a boolean return value - there will be no case where false is returned.
If you are asking whether it is better to return false rather than throw exception, that largely depends on the situation.
Your current error handling is already more granular than just success / failure - you want to detect different types of errors and respond differently, so the exception method is more appropriate.
Note that if you were to switch to return and maintain same granularity, you would need to use an Enum of possible results.
As for "Is error handling ok" - again, that depends.
How much 'some stuff' do you have around the API call?
What kind of stuff is it?
You may want to limit the amount of code in the catch block, and have separate catch blocks for other parts of code that may fail independently of any server errors.
Then again, how granular do you want to make your error handling?
Usually, the more precise the error and the more information user of the function has regarding why it failed the better, but if there are some constraints or type of errors that are not important they may be omitted, consolidated, or handled silently.


Delete lines from a CSV file that contain fields listed in a text file I wrote a powershell script to compare words from a text-file with a csv-column. If the word in the column matches, the line is deleted.

$reader = [System.IO.File]::OpenText($fc_file.Text)
try {
    for() {
        $line = $reader.ReadLine()
        if ($line -eq $null) { break }
        if ($line -eq "") { break }
        # process the line
        $fc_suchfeld = $fc_ComboBox.Text
        $tempstorage = $scriptPath + "\temp\temp.csv"
        Import-Csv $tempfile -Delimiter $delimeter -Encoding $char | where {$_.$fc_suchfeld -notmatch [regex]::Escape($line)} | Export-Csv $tempstorage -Delimiter $delimeter -Encoding $char  -notypeinfo
        Remove-Item $tempfile
        Rename-Item $tempstorage $tempfile_ext
    }
}
finally {
    $reader.Close()
}

My code works great, but it is very slow, due to saving and copying the csv file after every line. Is there a way to improve it?

[Side note: It's helpful to present complete, working code when asking questions, together with any input files. That makes it easier for us. We can use the same names for files and so on. It makes things easier and less confusing. In the following, I'm just going to show you the basic code with all the irrelevant details stripped out.]

There's no need to walk through the words file line-by-line. We can just load it once into an array at the beginning:

$words = Get-Content WordsFile.txt

And then we can look in that array when we are processing the CSV records. That's pretty simple:

Import-Csv CsvFileIn.txt |
    where TheField -notin $words |
    Export-Csv CsvFileOut.txt

Notes:

where TheField -notin $words is short for where {$_.TheField -notin $words}. That syntax was introduced in PowerShell 3.0 I think.
The -notin operator is case-insensitive. If you want it to be case-sensitive, then use -cnotin instead. (That is the same with all string comparisons in PowerShell: they are all case-insensitive.)
I'm assuming the words file is not huge. If it's huge, then the -notin operator may be too slow because it searches the array record-by-record (it does a "linear" lookup, as we say in the business). If it were huge, it would be better to use a .NET HashSet instead of an array, but if not, there's no need for the extra complexity.


Extract all GPU name, model and GPU ram I currently use this code to extract the required GPU data. Can you review this method for efficiency or best coding practices?

Code

import wmi
c = wmi.WMI()
def gpu_check():
    gpu_ouy =["fu","7"]
    gpu_a =""
    gpu_b =""
    ass   = str(c.Win32_VideoController()[0])
    asss  = str(c.Win32_VideoController()[1])
    aq = ass.split()
    for x in range(len(c.Win32_VideoController())):
        ass   = str(c.Win32_VideoController()[x])
        saa = ass.split()
        saa.remove("instance")
        saa.remove("of")
        saa.remove("Win32_VideoController")
        saa.remove("{")
        saa.remove("AdapterCompatibility")
        saa.remove("AdapterDACType")
        while "=" in saa:
            saa.remove("=")
        saa.remove("};")
        saa.remove("SystemName")
        saa.remove("Description")
        saa.remove("InstalledDisplayDrivers")
        saa.remove("DeviceID")
        saa.remove("DriverDate")
        saa.remove("Name")
        saa.remove("Status")
        saa.remove("Caption")
        saa.remove("InfFilename")
        saa.remove("ConfigManagerUserConfig")
        saa.remove("ConfigManagerErrorCode")
        saa.remove("InfSection")
        saa.remove("DriverVersion")
        if "CurrentScanMode" in saa:
            saa.remove("CurrentScanMode")
        saa.remove("SystemCreationClassName")
        saa.remove("CreationClassName")
        if "CurrentRefreshRate" in saa:
            saa.remove("CurrentRefreshRate")
        if "CurrentHorizontalResolution" in saa:
            saa.remove("CurrentHorizontalResolution")
        if "CurrentVerticalResolution" in saa:
            saa.remove("CurrentVerticalResolution")
        if "CurrentNumberOfColors" in saa:
            saa.remove("CurrentNumberOfColors")
        if "MinRefreshRate" in saa:
            saa.remove("MinRefreshRate")
        if 'colors";' in saa:
            saa.remove('colors";')
        if "CurrentBitsPerPixel" in saa:
            saa.remove("CurrentBitsPerPixel")
        if "MaxRefreshRate" in saa:
            saa.remove("MaxRefreshRate")
        saa.remove("VideoProcessor")
        saa.remove("PNPDeviceID")
        if "CurrentNumberOfColumns" in saa:
            saa.remove("CurrentNumberOfColumns")
        if "CurrentNumberOfRows" in saa:
            saa.remove("CurrentNumberOfRows")
        saa.remove("Monochrome")
        saa.remove("Availability")
        del saa[5]
        if '"Integrated' in saa:
            saa.remove('"Integrated')
        if '"Internal";' in saa:
            saa.remove('"Internal";')
        if 'VideoModeDescription' in saa:
            saa.remove('VideoModeDescription')
        saa.remove('AdapterRAM')
        if 'RAMDAC";' in saa:
            saa.remove('RAMDAC";')
            sqa =int(((int(str(saa[1]).replace(";",""))/1024)/1024)/1000)
            saa[1] = sqa
        else:
            sqa =int(((int(str(saa[2]).replace(";",""))/1024)/1024)/1000)
            saa[2] = sqa
        if 'Corporation";' in saa:
            saa.remove('Corporation";')
        while "FALSE;" in saa:
            saa.remove("FALSE;")
        gpu_a =""
        gpu_b =""
        if saa[6] =="0;":
            gpu_a = str(saa[2]).replace('"',"")+" "+saa[3]+" "+saa[4]+" "+str(saa[5]).replace('";',"")+" "+str(saa[1])+" GB"
            gpu_ouy[0] = str(gpu_a)
        else:
            gpu_b = str(saa[2]).replace('"',"")+" "+saa[3]+" "+saa[4]+" "+str(saa[5]).replace('";',"")+" "+str(saa[6]).replace('";',"")+" "+str(saa[1])+" GB"
            gpu_ouy[1] = gpu_b
    return gpu_ouy

Let's ignore efficiency. It took me five minutes to figure out what you were trying to do. In the kindest way possible, this is a mess. But let's fix that!

Your variable names are terrible. I have no idea what an ass is. What about an asss? Surely that's a bad idea, because it's very easy to mistake one for the other. Even if it wasn't, what are they? A variable name should describe what it holds. A bunch of random characters aren't helpful. Same goes for gpu_ouy. Is that a typo? What are gpu_a and gpu_b? What is aq? What is c?  What is saa? None of these variables names help someone reading your code understand their function.

Don't use len in a for loop. The idiomatic python approach is to just iterate over the collection (and let it deal with lengths and indices). So instead of:

for x in range(len(c.Win32_VideoController())):
    ass = str(c.Win32_VideoController()[x])

Use this:

for controller in c.Win32_VideoController():
   # do something with controller...
   pass

Some random points before we get to the big issue:

In sqa =int(((int(str(saa[1]).replace(";",""))/1024)/1024)/1000), use // instead of calling int so much. This does floor division. So int(int(x / 1024) / 1024) is the same as x // 1024 // 1024. But then you could just merge them: x // (1024 * 1024). But what are those numbers? I suspect you're converting from bytes to GB (expect 1000 MB = 1 GB isn't correct unless you're dealing with harddrives, which sometimes define the measurements like so). So instead use a constant: BYTES_PER_GB = 1024 * 1024 * 1000. Then do: int(vram_size_bytes) // BYTES_PER_GB.
As @IsmaelMiguel points out, not all cards support more than 1GB of RAM. This math will report 0 for them, which you may not want. One solution is to not use floored division (int(vram_size_bytes) / BTYES_PER_GB). When printing using the format specifier .02f so you don't get numbers with tons of decimal places (ex. "Video RAM: {:.02f}".format(vram_size_bytes / BYTES_PER_GB)).
Fix your spacing and formatting. PEP8 your code!
What is saa[1] above? You have some many magic indices that no one can discern without understanding the value of str(controller). Prefer giving things descriptive names (use variables, heck overuse variables to make things clear) to magic indices.
remove can fail (it raises a ValueError if the element isn't found in the list).
You don't need to initialize gpu_a and gpu_b (ex. gpu_a = "" is unnecessary), because later you assign stuff to them. They will be initialized on first assignment.
Given code this fragile, you definitely want to include tests. I'd imagine some integration tests would help. You can amass a list of outputs of str(c.Win32_VideoController()[0])s and manually do this string parsing. Then assert in some tests that your code outputs the thing that you expect for each. unittest will help you with this.

But now the largest issue: you're doing a ton of extra work that you almost certainly don't have to do! I can't seem to find good docs for wmi's wrapping around this, but according to the microsoft docs, this probably returns an object of some type. That means instead of all of this convoluted and fragile string parsing you're attempting, you can probably get at the information you want by just doing controller.AdapterRAM // BYTES_PER_GB or controller.Description (where controller comes from for controller in wmi.WMI().Win32_VideoController()). Open up a Python REPL by running python3 and then run the following:

>>> import wmi
>>> controller = wmi.WMI().Win32_VideoController()[0]
>>> help(controller)
>>> dir(controller)

That should give you an idea of all of the information you can get from it. You don't need to be doing all that string parsing! The information is already available on properties of the object!


Improving python3 processing speed (against a reference perl script) In order to create fast map/reduce processor for hadoop, I'm evaluating many languages. I'm learning python so I would like my python processor to go as fast as my perl processor.

So, for this question, the point is to increase the performance of simpleprocessor.py. To measure improvement, there is a benchmarking suite available there: https://github.com/Fade78/Stream-processing-benchmark

simpleprocessor.py

#!/usr/bin/python3
import sys
import re

scripttitle="PYTHON SIMPLE PROCESSOR (regex parsing)"

linecount=0
commentary=0
unknownline=0
DATA={}

pattern_data=re.compile(r"^(\d+)\s+(\S+)\s+(\S+)\s+(\d+(?:\.\d+)?)")

print("#",scripttitle,"\n#TRANSFORMED INPUT",sep='')

for line in sys.stdin:
    linecount+=1
    if line.startswith("#"):
        commentary+=1
        continue
    line=line.replace(',','')
    m=re.match(pattern_data,line)
    if m:
        i,k1,k2,value = m.group(1,2,3,4)
        i=int(i)
        value=float(value)
        try:
            DATA[k1][k2]+=value
        except KeyError:
            if k1 not in DATA: # Can't automaticaly create missing key and do the insert?
                DATA[k1]={}
            if k2 not in DATA[k1]:
                DATA[k1][k2]=value
            else:
                DATA[k1][k2]+=value
        print("{0},{1:.0f},{2},{3}".format(i,value,k2,k1))
    else:
        unknownline+=1

print("#DATADUMP")

keystat=0

for k1 in sorted(DATA):
    print(k1,':',sep='',end='')
    for k2 in sorted(DATA[k1]):
        keystat+=1
        print(' (',k2,':',int(DATA[k1][k2]),')',sep='',end='')
    print()

report="#{0}\n#{1}\nparsed line: {2}, commentary line: {3}, unknown line: {4}, keystat: {5}.".format(
               scripttitle, sys.version.replace("\n"," "), linecount, commentary, unknownline, keystat)

print("#REPORT\n"+report,file=sys.stdout)
print(report,file=sys.stderr)

In the benchmark output you can see that the python processor is three time slower than the perl processor.

To test you can run the benchmark and directly test your own modification. You can also add other script (in other language). Please, read the README at github.

Regards.

Fade.

P.S.: You may write a processor in your favorite language too, I'll be glad to put it in the suite.

#!/usr/bin/python3
import sys
import re

scripttitle="PYTHON SIMPLE PROCESSOR (regex parsing)"

Python convention is to put constants in ALL_CAPS

linecount=0
commentary=0
unknownline=0
DATA={}

This isn't a constant, so it really shouldn't be all caps.

pattern_data=re.compile(r"^(\d+)\s+(\S+)\s+(\S+)\s+(\d+(?:\.\d+)?)")

print("#",scripttitle,"\n#TRANSFORMED INPUT",sep='')

Its best to put all your actual logic inside a function rather then at the main level of a script. It'll run a bit faster that way.

for line in sys.stdin:
    linecount+=1
    if line.startswith("#"):
        commentary+=1
        continue

I find code is almost always more readable when you put thing in the else block rather then use continue

    line=line.replace(',','')
    m=re.match(pattern_data,line)
    if m:

Typically we'd explicit check for none with if m is not None

        i,k1,k2,value = m.group(1,2,3,4)

Actually you could use m.groups() here. I'd also avoid such unhelpnames as i, k1, and k2

        i=int(i)

I'm not sure why you bother doing this if you are just going to print it out anyways

        value=float(value)
        try:
            DATA[k1][k2]+=value
        except KeyError:
            if k1 not in DATA: # Can't automaticaly create missing key and do the insert?
                DATA[k1]={}
            if k2 not in DATA[k1]:
                DATA[k1][k2]=value
            else:
                DATA[k1][k2]+=value

Python has a useful class called defaultdict. It lets you provide the default value for a dictionary. It also has a class called Counter for counting things So you could do this:

DATA = collections.defaultdict(collections.Counter)

Then

DATA[k1][k2] += value

will always work because the default cases are handled.

        print("{0},{1:.0f},{2},{3}".format(i,value,k2,k1))

It'd probably be easier to follow using sep=',' rather then what you've done here

    else:
        unknownline+=1

print("#DATADUMP")

keystat=0

for k1 in sorted(DATA):

Instead use for k1, items in sorted(DATA.items): Then items will be DATA[k1] and you can relooking up the data

    print(k1,':',sep='',end='')
    for k2 in sorted(DATA[k1]):

Same here, use the .items() to fetch keys and values together

        keystat+=1
        print(' (',k2,':',int(DATA[k1][k2]),')',sep='',end='')
    print()

report="#{0}\n#{1}\nparsed line: {2}, commentary line: {3}, unknown line: {4}, keystat: {5}.".format(
               scripttitle, sys.version.replace("\n"," "), linecount, commentary, unknownline, keystat)

print("#REPORT\n"+report,file=sys.stdout)
print(report,file=sys.stderr)

As for performance, remember that Perl is the practical extraction and report language. This kinda thing is perl's bread and butter, so its gonna be hard for python to win. Doesn't mean I'm not gonna try though.

EDIT: Performance

I've played with improving performance, a few points:

    m=re.match(pattern_data,line)

A better way is to use

    m = pattern_data.match(line)

They both do the same thing, but the first has a speed penalty associated with it.

        print(' (',k2,':',int(DATA[k1][k2]),')',sep='',end='')

The print function is expensive, probably due to its versatility. Rewriting your code to use sys.stdout.write() directly gave much better performance.

    try:
        DATA[k1][k2]+=value
    except KeyError:
        if k1 not in DATA: # Can't automaticaly create missing key and do the insert?
            DATA[k1]={}
        if k2 not in DATA[k1]:
            DATA[k1][k2]=value
        else:
            DATA[k1][k2]+=value

Replacing this with defaultdict or counter harmed performance. I rewrote it as

        try:
            row = DATA[k1]
        except KeyError:
            row = DATA[k1] = {}
        try:
            row[k2] += value
        except KeyError:
            row[k2] = value

Which gave me a speed boost because it avoids looking up the same values in the dictionary more then once.

With those changes I was able to get within one second of the speed of the perl script. But I was still slower. My semi-educated guess is that perl wins due to builtin support for sorting the keys of a hash during iteration. In python the sorting is done in an seperate function and may not be able to take advantage of the same things the perl version can.

FURTHER PERFORMANCE

Put everything in a function. Python optimizes functions more then other code outside of functions.

Replace

for key, value in sorted(data.items()):

with

for key in sorted(data):
    value = data[key]

The first looks nicer, but it requires python to sort a list of tuples rather then a list of strings which ends up more expensive.

Replace

sys.stdout.write(' ({}: {})'.format(k1, math.trunc(v)))

With

sys.stdout.write(''.join([' (', k1, ': ', str(math.trunc(v)), ')']))

String formatting is expensive since python has to parse through the string every time to find the formatting positions.

You can add

write = sys.stdout.write

And then use write instead of sys.stdout.write for a bit of a speed boost. See my tweaked version here: http://pastebin.com/wmaR2Bmx.