Roman numeral converter in Ruby For a Project Euler problem, I made a Ruby roman numeral converter:

def romanize(num)
    digits = {
        1000 => "M",
        900  => "CM", 500 => "D", 400 => "CD", 100 => "C",
        90   => "XC", 50  => "L", 40  => "XL", 10  => "X",
        9    => "IX", 5   => "V", 4   => "IV", 1   => "I"
    }
    digits.reduce("") do |acc, digit|
        key, numeral = digit
        occurances, num = num.divmod(key)
        acc + (numeral * occurances)
    end
end

This works great, but I'm not sure it's good Ruby. Specifically, I'm wondering if the body of the reduce block could be shortened, and if it could made more clear.

For example, I don't think its that obvious that num is being modified in the middle of the reduce block.

Also, I know I can monkey patch this into the Fixnum class, but that's not what I'm looking for.

Update #1

@Tokland's answer taught me some new things. His answer works well, and is pretty clever.

If I take just two of his concepts and apply them to mine, the digits.reduce("") ... block can be simplified to:

digits.map do |key, numeral|
    occurances, num = num.divmod(key)
    numeral * occurances
end.join

This needs one less line because it deconstructs the digit key/value in the block arguments. Also, it creates an array which is joined into a string at the end. For big numbers (around 100-trillion), this is much quicker.

The only problem I still see is that it's modifying the num variable, which is essentially global state to the block.

Some notes:

Use 2 spaces for indentation, not 4.
key, numeral = digit: You can unpack block arguments: do |acc, (key, numeral)|.
Concating strings is costly, better build an array and finally join it.
You are using inject, which is great because it'a functional abstraction, but you are modifying num on each iteration, so it ends up being a weird mixture of functional and imperative style. I know it's a bit cumbersome, but conceptually it's better to keep also num in the accumulator.

I'd write:

digits.reduce(:output => [], :num => num) do |state, (key, numeral)|
  ocurrences, remainder = state[:num].divmod(key)
  {:output => state[:output] << (numeral * ocurrences), :num => remainder}
end[:output].join


Calculate shipping cost Could I code differently to slim down the point of this Python source code? The point of the program is to get the user's total amount and add it to the shipping cost. The shipping cost is determined by both country (Canada or USA) and price of product: The shipping of a product that is $125.00 in Canada is $12.00.

input ('Please press "Enter" to begin')

while True: print('This will calculate shipping cost and your grand total.')

totalAmount = int(float(input('Enter your total amount: ').replace(',', '').replace('$', '')))
Country = str(input('Type "Canada" for Canada and "USA" for USA: '))

usa = "USA"
canada = "Canada"
lessFifty = totalAmount <= 50
fiftyHundred = totalAmount >= 50.01 and totalAmount <= 100
hundredFifty = totalAmount >= 100.01 and totalAmount <= 150
twoHundred = totalAmount

if Country == "USA":
    if lessFifty:
        print('Your shipping is: $6.00')
        print('Your grand total is: $',totalAmount + 6)
    elif fiftyHundred:
        print('Your shipping is: $8.00')
        print('Your grand total is: $',totalAmount + 8)
    elif hundredFifty:
        print('Your shipping is: $10.00')
        print('Your grand total is: $',totalAmount + 10)
    elif twoHundred:
        print('Your shipping is free!')
        print('Your grand total is: $',totalAmount)

if Country == "Canada":
    if lessFifty:
        print('Your shipping is: $8.00')
        print('Your grand total is: $',totalAmount + 8)
    elif fiftyHundred:
        print('Your shipping is: $10.00')
        print('Your grand total is: $',totalAmount + 10)
    elif hundredFifty:
        print('Your shipping is: $12.00')
        print('Your grand total is: $',totalAmount + 12)
    elif twoHundred:
        print('Your shipping is free!')
        print('Your grand total is: $',totalAmount)

endProgram = input ('Do you want to restart the program?')
if endProgram in ('no', 'No', 'NO', 'false', 'False', 'FALSE'):
    break

I would not hard-code the fee logic, but instead store it as pure data. It's easier to maintain, even allowing to load it from a file.
Then, it boils down to a range-based lookup, which is quite classical (cf HLOOKUP in spreadsheet software, with so called "approximate search").

In Python, we can perform such a search via bisect, relying on lexicographic order (and infinity as an unreachable upper bound).

Separated core logic would look like :

from bisect import bisect

#For each country, list of (x,y) = (cost_threshold, fee)
#For first x such cost <= x, pick y for fee.
inf = float("inf")
shippingFees = { 'USA' :    [ (50, 6), (100, 8),  (150, 10), (inf, 0) ],
                 'CANADA' : [ (50, 8), (100, 10), (150, 12), (inf, 0) ]
               }
#Make sure it is sorted (required for lookup via bisect)
#FIXME : Actually it would be better to assert it is already sorted,
#        since an unsorted input might reveal a typo.
for fee in shippingFees.values() : fee.sort()

def getShippingFee(amount, country):
   fees = shippingFees[country.upper()] #raise KeyError if not found.
   idx = bisect(fees, (amount,) )
   return fees[idx][1]

Update

Here is a sample of "working application" using the helper function, assuming you have saved the code snippet above as prices.py (which should be stored in a module, but that's another story).

NB : I have dropped the exit part, since I don't like to type no when I can hit CTRL+C.

#!/usr/bin/python2
""" Your description here """

from prices import getShippingFee

print('This will calculate shipping cost and your grand total.')

while True:

   #TODO : proper input processing, python3 compatible.
   totalAmount = float(raw_input('Enter your total amount: ').replace(',', '').replace('$', ''))
   country = raw_input('Type "Canada" for Canada and "USA" for USA: ').strip().upper()

   try :  #TODO : proper country check.
      shippingFee = getShippingFee(totalAmount, country)
      grandTotal = totalAmount + shippingFee
      if shippingFee :
         print('Your shipping cost is: %.2f' % shippingFee)
      else :
         print('Your shipping is free!')
      print('Your grand total is: %.2f' % grandTotal)

   except KeyError :
      print ("Sorry, we don't ship to this hostile country : %s" % country)


Circular Queue Implementation Using Array in java Here is my code of Array Implementation of Queue in java.

import java.util.Scanner;
class CircularQueue
{
int maxSize;
int rear;
int front;
int aQueue[];
{
    rear = -1;
    front = -1;
}
CircularQueue(int maxSize)
{
    this.maxSize = maxSize;
    this.aQueue = new int[maxSize];
}
void enQueue(int item)
{
    if(((rear+1) % maxSize) == front)
    {
        System.out.println("Queue is Full");
    }
    else
    {
        if (rear == front && front == -1)
        {
            front += 1;
        }
        rear = (rear+1) % maxSize;
        aQueue[rear] = item;
    }
}
void deQueue()
{
    if(rear == front && rear == -1)
    {
        System.out.println("Queue is Empty.");
    }
    else
    {
        int item = aQueue[front];
        if(rear == front)
        {
            rear = -1;
            front = -1;
        }
        else
        {
            front = (front + 1) % maxSize;
        }
        System.out.println(item + " is deQueued from the Queue");
    }
}

String elementOrElements()
{
    String send = (rear == front)? (" ") :("s ");
    return send;
}

void display()
{
    int tmpfront = front;
    if(rear == front && rear == -1)
            {
                    System.out.println("Queue is Empty.");
            }
            else
    {
        System.out.println("The element"+ elementOrElements() + "on the Queue are:- ");
        for(int i=0; i<maxSize ; i++)
        {
            if(tmpfront != rear)
            {
                System.out.println(aQueue[tmpfront]);
                tmpfront = (tmpfront + 1) % maxSize;
            }
            else
            {
                System.out.println(aQueue[rear]);
                break;
            }
        }
    }
}
public static void main(String []args)
{
    Scanner input = new Scanner(System.in);
    System.out.print("Enter total no of elements to be in the queue:- ");
    int maxSize = input.nextInt();
    CircularQueue queue = new CircularQueue(maxSize);
    int select;
    int term = 0;
    while (term == 0)
    {
        System.out.print("\nOption:\tTo Do:\n1\tTo push element.\n2\tTo POP element.\n3\tTo Display the Queue elements.\n4\tTo Exit the Program.\nEnter your option:- ");
                select = input.nextInt();
        switch(select)
        {
            case 1:
            {
                System.out.print("Enter element to insert in the Queue:- ");
                int ele = input.nextInt();
                queue.enQueue(ele);
                break;
            }
            case 2:
            {
                queue.deQueue();
                break;
            }
            case 3:
            {
                queue.display();
                break;
            }
            case 4:
            {
                term = 1;
                System.out.println("Thank you!");
                break;
            }
            default:
                System.out.println("Enter a valid options");
        }
    }
  }
}

Is there anything I can do to improve it? Code written for DSA class practical.

Since this is in Java and for academic purposes, my guess would be that you're using Eclipse. Hit CTRL+SHIFT+F to format your code, and move any lines that it chops up back into one where appropriate. The key binding is similar for Netbeans and IntelliJ's IDEA.
Too many useless variables. You have a bad habit of creating variables for one off-uses, e.g. in elementOrElements(). Just return what send equals. If you want to describe what the code does, then use comments (which you don't use at all in your implementation btw).
If the functionality isn't obvious, then use comments. Most programmers can discern what generic methods and operations look like, but when in doubt comment. Remember that you're writing software primarily for other humans. This is critical in a professional environment.
Delete elementOrElements(). In the single print call in which it's used, just say, The contents of the Queue are:.
Consistent variable naming is required. You have tmpfront, which should be tempFront according to Java conventions. Not a huge deal since you seem to adhere to this throughout the program. I add the extra e out of personal preference (and the fact that it helps other humans ever so slightly).
Use proper Queue naming conventions, i.e. use enqueue instead of enQueue. Creating a couple of helper methods like isEmpty() and isFull() would help your implementation in a couple of places.
Move CircularQueue to CircularQueue.java and add some public modifiers to your methods. This is mainly for convention training.


Scrapy Username Spider This currently starts out with the speed of 2000 pages/min but shortly after starting it becomes very slow with a speed of about 200 pages/min. Why is this happening? How can I improve this scraper?

import scrapy
from scrapy.crawler import CrawlerProcess
from time import time

def isAscii(s):
    return all(ord(c) < 128 for c in s)

region = input('Region? ')
start = int(input('Start page? '))
end = int(input('End page? '))

startTime = time()

class LolSpider(scrapy.Spider):
    name = 'lolspider'
    start_urls = ['http://lolprofile.net/leaderboards/{}/{}'.format(region, page) for page in range(start, end)]
    def parse(self, response):
        for name in response.xpath('//span[not(@class)]/text()').extract():
            with open('lolusernames.txt', 'a') as f:
                if (' ' not in name) and (name != 'LoL') and isAscii(name):
                    f.write(name+'\n')

process = CrawlerProcess({})

process.crawl(LolSpider)
process.start()

endTime = time()
print('Completed in {:.2f} seconds'.format(endTime-startTime))

My main goal is to be as fast as possible and that's why I used scrapy. If anyone knows of a faster alternative I'd love to hear it.

The main problem is that you are writing/appending to the file inside the spider's parse() method - this is not how this should be done in Scrapy - there is a special place - Item Pipelines. Check this answer to see how to include a custom pipeline when running Scrapy from a script.

Note that in the pipeline you should not be appending to a file, but instead collecting the data in the process_item() method and dumping it when the spider is finished in spider_closed() method. You may also do the writing in chunks to avoid potential memory problems on large output data sizes.

Other things to improve:

adjust/increase the number of concurrent requests
turn the logging off
enable HTTP cache
Scrapy running on PyPy?


Replace spaces with '%20' This is my attempted solution to Question 1.3 from "Cracking the Code Interview", 6th ed.

Question:

  Write a method to replace all spaces in a string with '%20'. You may assume that the string has sufficient space at the end to hold the additional characters, and that you are given the true length of the string.

#include <stdio.h>
#include <stdlib.h>
#include <assert.h>
#include <string.h>
// Input:  "Mr John Smith    ", 13
// Output: "Mr%20John%20Smith"

int countSpaces(char* str) {
  int i = 0, j = 0;
  while (str[i] != '\0')
    if ( str[i++] == ' ')
      j++;

  return j;
}

char* replaceChars(char* str, int length) {
  int x = countSpaces(str);
  int stringLength = length + x - x/3;

  char* urlifiedString = malloc((stringLength + 1) * sizeof(char));
  int i = 0, j = 0;
  while (i < stringLength) {
    if (str[j] != ' ') {
      urlifiedString[i++] = str[j];
    } else if (str[j] == ' '){
      urlifiedString[i++] = '%';
      urlifiedString[i++] = '2';
      urlifiedString[i++] = '0';
    }
    j++;
  }
  urlifiedString[stringLength] = '\0';
  return urlifiedString;
}

int main() {
  char* result1 = replaceChars("Mr John Smith    ", 13);
  printf("%s\n", result1);
  printf("%li\n", strlen(result1));
  assert(strcmp("Mr%20John%20Smith", result1) == 0);
  free(result1);

  char* result2 = replaceChars("lol  lol lol      ", 12);
  printf("%s\n", result2);
  assert(strcmp("lol%20%20lol%20lol", result2) == 0);
  free(result2);

  char* result3 = replaceChars("", 0);
  assert(strcmp("", result3) == 0);
  free(result3);

  return 0;
}

I think you have misunderstood the part about "assume that the string has sufficient space at the end", and as a result, you have some memory management issues.  I think that the intention is to assure you that your replaceChars() function will be handed a buffer of sufficient size such that you may write the result in place, without allocating more memory.

In that light, your solution and tests look a bit confused — and in fact the code is wrong in the general case.  If the input is "Mr␠John␠Smith␠␠␠␠", and you countSpaces() without taking the true length (13) into account, then countSpaces() would report that there are 6 spaces rather than 2.  To compensate for the overestimate, you then use the formula int stringLength = length + x - x/3; — allocating roughly \$\frac{2}{3}\$ of an additional byte for each space, rather than two extra bytes per space.  And you are also relying on that dubious estimate of the result length to terminate your loop, with while (i < stringLength).

Suggested solution

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

// Count the spaces in the first n bytes of str
int countSpaces(const char* str, int n) {
  int count = 0;
  while (n --> 0) {
    if (*str++ == ' ') {
      count++;
    }
  }
  return count;
}

char* replaceChars(char* str, int length) {
  int spaces = countSpaces(str, length);
  char *in = str + length - 1,
       *out = str + length + 2 * spaces;
  *out-- = '\0';
  while (in != out) {
    if (*in == ' ') {
      out -= 3;
      in--;
      *(out + 1) = '%';
      *(out + 2) = '2';
      *(out + 3) = '0';
    } else {
      *out-- = *in--;
    }
  }
  return str;
}

int main() {
  const char *test1 = "Mr John Smith";
  char buf1[18] = {0};
  strcpy(buf1, test1);
  replaceChars(buf1, strlen(test1));
  assert(strcmp("Mr%20John%20Smith", buf1) == 0);

  const char *test2 = "lol  lol lol";
  char buf2[19] = {0};
  strcpy(buf2, test2);
  replaceChars(buf2, strlen(test2));
  assert(strcmp("lol%20%20lol%20lol", buf2) == 0);

  const char *test3 = "";
  char buf3[1] = {0};
  strcpy(buf3, test3);
  replaceChars(buf3, strlen(test3));
  assert(strcmp("", buf3) == 0);
}


Retrieving context menus from the form I have the following code which is a private method inside the form and retrieve all context menus from the form. I feel, that it is not that concise as it should be. Would be grateful for any suggestions.

  private IEnumerable<ContextMenuStrip> GetContextMenus()
    {
        var type = this.GetType();
        var fields = type.GetFields(BindingFlags.NonPublic | BindingFlags.Instance);
        var contextMenus = fields.Where(f => f.GetValue(this).GetType() == typeof(ContextMenuStrip));
        var menus = contextMenus.Select(f=> f.GetValue(this));
        return menus.Cast<ContextMenuStrip>();
    }

I can't imagine why do you do this via Reflection, I believe the same can be done by walking through Controls tree. Your method will not work if context menu has property Generate member set to false in designer.

var contextMenus = fields.Where(f => f.GetValue(this).GetType() == typeof(ContextMenuStrip));
var menus = contextMenus.Select(f=> f.GetValue(this));
return menus.Cast<ContextMenuStrip>();

Can be replaced with:

return fields.Select(f => f.GetValue(this)).OfType<ContextMenuStrip>();


Assigning default values to function arguments which may be "empty" In perl, is there a canonical practice or idiom for assigning value to an argument which, if "empty", is assigned a default value?

Here, "empty" is defined as an argument value which is either undef or the null string (''). Notably, 0 (or perl false) is a valid, non-"empty" value, which rules out the construction $min_perl_version = (shift ||= '5.008009').

This is my current construction (note the use of the v5.10+ logical-or operator, //):

my $min_perl_version = do {$_ = shift // q{}; (length) ? $_ : '5.008009'};

as in the example function:

sub is_CORE { # ( $:MIN_PERL_VERSION , $:MODULE )
    use 5.008009; # perl v5.8.9; required for Module::CoreList to be available in CORE
    my $min_perl_version = do {$_ = shift // q{}; (length) ? $_ : '5.008009'}; # "empty" => default to version 5.008009
    my $module = shift;
    return ($Module::CoreList::VERSION >= 2.99) && Module::CoreList::is_core($module, undef, $min_perl_version);
}

I don't want warnings or to duplicate the use of the default value.

Is there a simpler / more elegant solution, say... without a named temporary variable?

A succinct solution which is functional back to earlier perl versions (5.6 or 5.8) would be a plus as well.

edit: for clarity

For clarification, the function interface and definition of argument validity (eg, what "empty" means) are set constants. While I appreciate any critique, a conversation about the line:

my $min_perl_version = do {$_ = shift // q{}; (length) ? $_ : '5.008009'};

and how it might be better coded is the real point of the question.

use 5.008009 is insufficient here, since you're using the // operator, which was introduced in Perl 5.10.

This function feels awkwardly designed.  Generally, parameters that have default values appear after all required parameters.  Here, I would need to explicitly pass in undef as the first parameter to take advantage of the default.  Also note that the parameters for your is_CORE are backwards relative to Module::CoreList::is_core.

I also don't understand the benefit of having the empty string trigger the default.  I can understand having undef trigger the default.  I could also agree with having all "falsy" values trigger the default.  But this particular design feels weird.  If I had to do it, though, I'd write it like this:

sub is_CORE {
    use 5.008009; # perl v5.8.9; required for Module::CoreList to be available in CORE
    my ($module, $min_perl_version) = @_;
    $min_perl_version = '5.008009' unless defined $min_perl_version && length $min_perl_version;
    $Module::CoreList::VERSION >= 2.99 && Module::CoreList::is_core($module, undef, $min_perl_version);
}


Apply function to every subset combination and return square matrix I do not know how to do this without four nested for loops.

I'd like to apply a function to every possible combination of subsets for hour and day, return that value, and then pivot the data frame into a square matrix. However, these for loops seem excessive so I'm looking for a more efficient way to do this. The data I have is fairly large so any gain in speed would be beneficial.

I took a stab at compression lists but that seems excessive too.

The following code runs fine with smaller groups (days=1:2 and hours=1:2) but with large groups, say years of data, performance is lost.

Note: I'm requesting help given any custom function, not just a cross-product solution as suggested here, Performant cartesian product (CROSS JOIN) with pandas

Sample data

import pandas as pd
import numpy as np
dat = pd.DataFrame({'day': {0: 1, 1: 1, 2: 1, 3: 1, 4: 1, 5: 1, 6: 1, 7: 1, 8: 1, 9: 1, 10: 2, 11: 2, 12: 2, 13: 2, 14: 2, 15: 2, 16: 2, 17: 2, 18: 2, 19: 2}, 'hour': {0: 1, 1: 1, 2: 1, 3: 1, 4: 1, 5: 2, 6: 2, 7: 2, 8: 2, 9: 2, 10: 1, 11: 1, 12: 1, 13: 1, 14: 1, 15: 2, 16: 2, 17: 2, 18: 2, 19: 2}, 'distance': {0: 1.2898851269657656, 1: 0.0, 2: 0.8371526423804061, 3: 0.8703856587273138, 4: 0.6257425922449789, 5: 0.0, 6: 0.0, 7: 0.0, 8: 1.2895328696587023, 9: 0.0, 10: 0.6875527848294374, 11: 0.0, 12: 0.0, 13: 0.9009031833559706, 14: 0.0, 15: 1.1040652963428623, 16: 0.0, 17: 0.0, 18: 0.0, 19: 0.0}})0}})

Code

def custom_fn(x, y):
    x = pd.Series(x)
    y = pd.Series(y)
    x = x**2
    y = np.sqrt(y)
    return x.sum() - y.sum()

# Empty data.frame to append to
dmat = pd.DataFrame()

# For i, j = hour; k, l = day
for i in range(1, 3):
    for j in range(1, 3):
        for k in range(1, 3):
            for l in range(1, 3):
                x = dat[(dat['hour'] == i) & (dat['day'] == k)].distance
                y = dat[(dat['hour'] == j) & (dat['day'] == l)].distance

                # Calculate difference
                jds = custom_fn(x, y)

                # Build data frame and append
                outdat = pd.DataFrame({'day_hour_a': f"{k}_{i}", 'day_hour_b': f"{l}_{j}", 'jds': [round(jds, 4)]})
                dmat = dmat.append(outdat, ignore_index=True)

# Pivot data to get matrix
distMatrix = dmat.pivot(index='day_hour_a', columns='day_hour_b', values='jds')

Output

> print(distMatrix)

 day_hour_b     1_1     1_2     2_1     2_2
day_hour_a
1_1        -0.2609  2.3782  1.7354  2.4630
1_2        -2.1118  0.5273 -0.1155  0.6121
2_1        -2.4903  0.1488 -0.4940  0.2336
2_2        -2.5557  0.0834 -0.5594  0.1682

I'm afraid that I'm not familiar with pandas, so there are definitely some things that I'm missing. However, I've done a bit of poking this code with a profiler, and I have a two suggestions that I expect would be helpful.

In my timings, over half of the runtime (3.5s out of 6.5s for 100 repetitions) in your example was spent on these two lines:

x = dat[(dat['hour'] == i) & (dat['day'] == k)].distance
y = dat[(dat['hour'] == j) & (dat['day'] == l)].distance

If I understand the code correctly, (dat['hour'] == i) is passing over the whole dataset searching for indexes with the specified hour. Especially given that this is in the middle of a very hot loop, that seems like prime work to avoid doing! Consider changing the data structure to allow for quicker look ups: perhaps have a dictionary that maps your chosen hour to a list or set of indexes.

Second, let's take a look at this custom_fn

def custom_fn(x, y):
    x = pd.Series(x)
    y = pd.Series(y)
    x = x**2
    y = np.sqrt(y)
    return x.sum() - y.sum()

Now, I would normally not pay much attention to this function, because according to the profiler it just used up 8% of the total runtime. However, I did notice that it is almost completely seperable. The last line uses data derived from both x and y but until that you just use x for x things and y for y things. That suggests that there is considerable opportunity to cache the relevant x.sum() and y.sum() components, with the relevant calculations done outside of so deeply nested a loop.

For reference, the following code is my initial go at using some of this caching approach. There is still plenty of opportunity to improve, including changing data structure as mentioned, but it is already significantly faster. It has come down from 6.5 seconds to 3.5 seconds in total, of which 2 seconds is packing the dmat table.

import pandas as pd
import numpy as np

dat = pd.DataFrame({'day': {0: 1, 1: 1, 2: 1, 3: 1, 4: 1, 5: 1, 6: 1, 7: 1, 8: 1, 9: 1, 10: 2, 11: 2, 12: 2, 13: 2, 14: 2, 15: 2, 16: 2, 17: 2, 18: 2, 19: 2}, 'hour': {0: 1, 1: 1, 2: 1, 3: 1, 4: 1, 5: 2, 6: 2, 7: 2, 8: 2, 9: 2, 10: 1, 11: 1, 12: 1, 13: 1, 14: 1, 15: 2, 16: 2, 17: 2, 18: 2, 19: 2}, 'distance': {0: 1.2898851269657656, 1: 0.0, 2: 0.8371526423804061, 3: 0.8703856587273138, 4: 0.6257425922449789, 5: 0.0, 6: 0.0, 7: 0.0, 8: 1.2895328696587023, 9: 0.0, 10: 0.6875527848294374, 11: 0.0, 12: 0.0, 13: 0.9009031833559706, 14: 0.0, 15: 1.1040652963428623, 16: 0.0, 17: 0.0, 18: 0.0, 19: 0.0}})

def x_sum(x):
    x = pd.Series(x)
    x = x**2
    return x.sum()

def y_sum(y):
    y = pd.Series(y)
    y = np.sqrt(y)
    return y.sum()

def custom_fn(x_s, y_s):
    return x_s - y_s

def get_hour(i):
    return (dat['hour'] == i)

def get_day(k):
    return (dat['day'] == k)

def get_day_hour(hour, day):
    x = dat[hour & day].distance
    return x

# Empty data.frame to append to
dmat = pd.DataFrame()

day_indices = {k : get_day(k) for k in range(1, 3)}
hour_indices = {i : get_hour(i) for i in range(1, 3)}

x_sum_indices = { (i, j): x_sum(get_day_hour(hour_indices[i], day_indices[j])) for i in range(1, 3) for j in range(1, 3)}
y_sum_indices = { (i, j): y_sum(get_day_hour(hour_indices[i], day_indices[j])) for i in range(1, 3) for j in range(1, 3)}

# For i, j = hour; k, l = day
for i in range(1, 3):
    for j in range(1, 3):
        for k in range(1, 3):
            for l in range(1, 3):
                x_s = x_sum_indices[(i, k)]
                y_s = y_sum_indices[(j, l)]

                # Calculate difference
                jds = custom_fn(x_s, y_s)

                # Build data frame and append
                outdat = pd.DataFrame({'day_hour_a': f"{k}_{i}", 'day_hour_b': f"{l}_{j}", 'jds': [round(jds, 4)]})
                dmat = dmat.append(outdat, ignore_index=True)
return dmat

# Pivot data to get matrix
distMatrix = dmat.pivot(index='day_hour_a', columns='day_hour_b', values='jds')

print(distMatrix)

If you had a different custom_fn function that was not so easily separable, you could still benefit from caching the inputs to the function. E.g.

x_indices = { (i, j): get_day_hour(hour_indices[i], day_indices[j]) for i in range(1, 3) for j in range(1, 3)}
y_indices = x_indices

...
    jds = custom_fn(x_indices[(i, k)], y_indices[(j, l)])


Adding SecureString support to Lib2GitSharp A library that I use doesn't support SecureString in its credentials for connecting to GitHub, so I'm submitting a pull request to add it. I know it's not ideal to pull the actual string out, but the idea here is to minimize the amount of time the string is in memory. Also, allowing client code to utilize the fact that WPF uses a SecureString for passwords without having to do this conversion themselves.

Am I handling the SecureString "correctly" here? I know very little about unmanaged memory.
Is it a poor decision to store the Username in one as well? I'm second guessing that decision now. NetworkCredential uses a plain string for this and the secure version only for the password.

Some notes:

I would use a constructor, but it's the style of the project to use an object initializer.
It's also in the style of the project to place the properties at the bottom like you'll see below.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using LibGit2Sharp.Core;
using System.Security;
using System.Runtime.InteropServices;

namespace LibGit2Sharp
{
    /// <summary>
    /// Class that uses <see cref="SecureString"/> to hold username and password credentials for remote repository access.
    /// </summary>
    public sealed class SecureUsernamePasswordCredentials : Credentials
    {
        /// <summary>
        /// Callback to acquire a credential object.
        /// </summary>
        /// <param name="cred">The newly created credential object.</param>
        /// <returns>0 for success, &lt; 0 to indicate an error, &gt; 0 to indicate no credential was acquired.</returns>
        protected internal override int GitCredentialHandler(out IntPtr cred)
        {
            if (Username == null || Password == null)
            {
                throw new InvalidOperationException("UsernamePasswordCredentials contains a null Username or Password.");
            }

            IntPtr passwordPtr = IntPtr.Zero;
            IntPtr userNamePtr = IntPtr.Zero;

            try
            {
                passwordPtr = Marshal.SecureStringToGlobalAllocUnicode(Password);
                userNamePtr = Marshal.SecureStringToGlobalAllocUnicode(Username);

                return NativeMethods.git_cred_userpass_plaintext_new(out cred, Marshal.PtrToStringUni(userNamePtr), Marshal.PtrToStringUni(passwordPtr));
            }
            finally
            {
                Marshal.ZeroFreeGlobalAllocUnicode(passwordPtr);
                Marshal.ZeroFreeGlobalAllocUnicode(userNamePtr);
            }

        }

        /// <summary>
        /// Username for username/password authentication (as in HTTP basic auth).
        /// </summary>
        public SecureString Username { get; set; }

        /// <summary>
        /// Password for username/password authentication (as in HTTP basic auth).
        /// </summary>
        public SecureString Password { get; set; }
    }
}

I don't see a problem with how the SecureString objects are handled - seems pretty solid to me, but I've never used those so I'm possibly missing something that someone else could point out here.

Sealed Class
I haven't looked at the actual project's source code, but general design guidelines have this to say about sealing classes:

DO NOT seal classes without having a good reason to do so.

However it also defines a number of "good reasons" for sealing a class, including:

The class stores security-sensitive secrets in inherited protected members.

In other words, a sealed class seems warranted here.

InvalidOperationException
Out of all possible existing exception classes in the framework, and instead of creating your own derived exception class, you chose to throw an InvalidOperationException in the implementation of the GitCredentialHandler override.
MSDN describes this exception as follows:

The exception that is thrown when a method call is invalid for the object's current state.

Turns out this was, in my own humble opinion, the best decision to make: throwing any other exception type here would violate POLS - when client code is calling this method and the object's state shouldn't allow it, the programmer has all rights to expect an InvalidOperationException to be thrown.

XML Documentation
This is an API - having complete and accurate XML documentation is ideal.

    /// <summary>
    /// Callback to acquire a credential object.
    /// </summary>
    /// <param name="cred">The newly created credential object.</param>
    /// <returns>0 for success, &lt; 0 to indicate an error, &gt; 0 to indicate no credential was acquired.</returns>

I think this documentation could be better, but looking at the base class on GitHub I see that you've copied most of it from the base class, which is a good idea.
However the method can throw an InvalidOperationException - you could add something like this:
/// <exception cref="InvalidOperationException">
/// Thrown if this method is called when either <see cref="UserName"/> or <see cref="Password"/> is <c>null</c>.
/// </exception>

Style
You know me, I think this is too verbose:

IntPtr passwordPtr = IntPtr.Zero;
IntPtr userNamePtr = IntPtr.Zero;

Compared to:
var passwordPtr = IntPtr.Zero;
var userNamePtr = IntPtr.Zero;

Which is just as clear IMO. Not much gain here though. And given that the rest of the code seems to prefer the verbose way, sticking to the existing style is the best decision you could make.
And not only for usage of var:

I would use a constructor, but it's the style of the project to use an object initializer.
It's also in the style of the project to place the properties at the bottom like you'll see below.

UserName vs. Password
A user name can be sensitive information in certain circumstances - and although I don't think this is one, there's a saying that summarizes everything I'd have to say about whether or not having the user name as a SecureString is overkill:

Better safe than sorry.
-- unknown

However, the ironic part is this:
return NativeMethods.git_cred_userpass_plaintext_new(...);

Given that credentials end up being passed as plain text, I think I'd keep the user name as a plain string.


Condense multiple if statements with or operators? I created a weather widget in JavaScript that requests the current weather from Yahoo. The weather request returns a code which corresponds with a current weather image. I don't like Yahoo's images, so I use my own CSS classes that correspond with my own images. Consequently, the following code returns the correct CSS class which I then deal with later:

function returnClass(c) {
    if (c === "0" || c === "19" || c === "23" || c === "24") {
        return ["basecloud", "wind"];
    } else if (c === "1" || c === "2") {
        return ["basecloud", "rain"];
    } else if (c === "3" || c === "4" || c === "37" || c === "38" || c === "39" || c === "45" || c === "47") {
        return ["basecloud", "thunder"];
    } else if (c === "5" || c === "6" || c === "7" || c === "8" || c === "10" || c === "18") {
        return ["basecloud", "sleet"];
    } else if (c === "9") {
        return ["basecloud", "drizzle"];
    } else if (c === "11" || c === "12" || c === "40") {
        return ["basecloud", "rain"];
    } else if (c === "13" || c === "14" || c === "15" || c === "16" || c === "41" || c === "42" || c === "43" || c === "46") {
        return ["basecloud", "snow"];
    } else if (c === "17" || c === "35") {
        return ["basecloud", "hail"];
    } else if (c === "20" || c === "21" || c === "22") {
        return ["mist"];
    } else if (c === "25") {
        return ["basecloud", "frost"];
    } else if (c === "26" || c === "27" || c === "29" || c === "33" || c === "28" || c === "30" || c === "34" || c === "44") {
        return ["cloud"];
    } else if (c === "31") {
        return ["moon"];
    } else if (c === "32" || c === "36") {
        return ["sun"];
    } else if (c === "3200") {
        return ["none"];
    }
}

Is there any way to simplify the above code? As you can tell, multiple codes can represent the same CSS class, which is why I have so many or operators. The only requirement is that it returns the correct array depending on the code.

This sort of thing, with a finite set of coditions, is typically done using an array:

var codeIcons=[
      ["basecloud", "wind"],
      ["basecloud", "rain"],
      ["basecloud", "rain"],
      ["basecloud", "thunder"],
      ["basecloud", "thunder"],
      ....
    ];

Then your method simply becomes:

function returnClass(c) {
    c = parseInt(c);
    return (c < codeIcons.length) ? codeIcons[c] : ["none"];
}

Just adding to a set of advantages this method has:

sure, it is hard-coding logic, but, the presentation is better than multiple if-statements.
it is more managable - you can easily add conditions
you know exactly what conditions lead to certain output (you do not need to scan pages of code to find the right conditions
you cannot have bugs where multiple conditions lead to the same result (these types of bugs tend to creep in after maintaining the code - you have multiple or-conditions where c == xxx.
if, further down the road, you want to migrate the logic for what classes/icons to use, and when, you can easily just change where the array-of-values is configured (i.e. it can become a configuration value, not hard-coded.)


Top 2 Frequent Elements in JavaScript I got the original problem from Leetcode for top k frequent elements,  But I decide to simplify the problem just top 2 elements to make it simpler to solve.

// Given an array, return an array of the top 2 most elements.
// Input: [1, 2, 3, 4, 5, 3, 4] | Output: [3, 4]
// Input: [2, 2, 3, 2, 4, 3, 4] | Output: [2,4]

const topTwoElement = nums => {
  const arr = [];
  nums.forEach(num => {
    arr[num] ? arr[num][1]++ : arr[num] = [num, 1];
  });

  arr.sort((a, b) => b[1] - a[1]);
  return [arr[0][0], arr[1][0]];
};

console.log(topTwoElement([3, 2, 3, 4, 5, 3, 4])); //[3,4]
console.log(topTwoElement([2, 2, 3, 2, 4, 3, 4])); //[2,4]

Avoid the sort
You are almost there. But there is room for some improvements
Sparse array
Using an array to store number frequency works. Javascript will know to create a sparse array, and will sort such arrays using only the elements in it.
A spare array is an array that is not continuous. This is done to reduce the memory use. However there is a penalty for the memory saved, and that is it takes longer to access each item, as the index is used to find the real index, which then can get the item.
For a standard array, the index is used directly to get the item.
Sort is complex
The sort is an expensive operation and as you are only needing the top two you can store the top two results as you count the frequency of numbers.
This also means you don't need to store the number you are counting with each count, just how many there are.
The logic goes like this, see rewrite for implmentation
if frequency greater than second place
   if frequency greater than first place
      if first place the same value
          set the new first place frequency
      else
          move first place to second
          set first place to the new first
   else
       set the new second place

When done just return the first and second values.
It could also be done using a Map, but you would do that if the items to count were not able to be indexes (eg words).
More than one correct answer
The is a possibility that all values have the same frequency. That means that any two will be correct. Or as your second test [2, 2, 3, 2, 4, 3, 4] there are two correct answers [2,3] or [2,4]. As there are no rules for this any of the correct answers will do.
for loops are better than Array.forEach
Using a for loop to iterate over the array is more performant than using the array.forEach function.
A rewrite.
The rewrite avoids the sort
const topTwoElement = nums => {
    const arr = [];

    const first = {count : 0, value : null};
    const second = {count : 0, value : null};
    for (const num of nums) {

        if (arr[num]) { arr[num] += 1 }
        else { arr[num] = 1 }

        // to avoid having to index into the sparse array in the
        // code to test for top two
        const count = arr[num];

        if (count > second.count) {
            if (count > first.count) {
                if (first.value === num) {
                    first.count = count
                } else {
                    second.count = first.count;
                    second.value = first.value;
                    first.count = count;
                    first.value = num;
                }
            } else {
                second.count = count;
                second.value = num;
            }
        }
    }
    return [first.value, second.value];
}


Plain-text file encryption and decryption I have created an Encryption and Decryption Algorithm that encrypts plain text using a given cipher and also decrypts it back to the original notation. It works great.

This was my code for a coding test and the reviewer told me that the class structure is inverse of what is the right way of doing it and that it is sloppy at some places.

I want to know what is wrong with my structuring of classes, if anything. Does it look sloppy? Can someone please help me identify places where it is lacking Best Practices for Java?

I have a MyBuilder class which creates an instance of MyAlgorithm.
MyAlgorithm has the methods to encrypt and decrypt text, and it extends another class called EncryptionAndDecryption where the actual logic of the algorithm resides.

public class MyAlgorithm extends EncryptionAndDecryption {

    public enum Action {
        ENCRYPT,
        DECRYPT
    }

    protected MyAlgorithm(final String cipherCharSet) {
        super(cipherCharSet);
    }

    public String encryptText(final String cipher, final String plainText) {
        return encryptOrDecryptText(cipher, plainText, Action.ENCRYPT);
    }

    public String decryptText(String cipher, String encryptedText) {
        return encryptOrDecryptText(cipher, encryptedText, Action.DECRYPT);
    }

    public boolean encryptDirectory(String cipher, String baseDirectory) {
        // for each files and sub files in each sub directories, call the following method;
    // encryptOrDecryptFile(cipher, file, baseDirectory, Action.ENCRYPT));
    }

    public boolean decryptDirectory(String cipher, String baseDirectory) {
        // for each files and sub files in each sub directories, call the following method;
    // encryptOrDecryptFile(cipher, file, baseDirectory, Action.DECRYPT));
    }
}

public class EncryptionAndDecryption {

    private final String CIPHER_CHAR_SET;

    protected EncryptionAndDecryption(final String cipherCharSet) {
        this.CIPHER_CHAR_SET = cipherCharSet;
    }

    protected final String encryptOrDecryptText(final String cipher, final String inputText, final MyAlgorithm.Action action) {
         // method logic here
    }

    protected final void encryptOrDecryptFile(final String cipher, final Path filePath, final String baseDirectory, final MyAlgorithm.Action action) {
        // method logic here
    }
}

public class MyBuilder {

    private String CIPHER_CHAR_SET;

    private MyBuilder() {}

    public static MyBuilder builder() {
        return new MyBuilder();
    }

    public MyBuilder withCipherSet(String cipherSet) {
        this.CIPHER_CHAR_SET = cipherSet;
        return this;
    }

    public MyAlgorithm build() {
        return new MyAlgorithm(CIPHER_CHAR_SET);
    }
}

public class MyEncryptionApp {

    public static final String CIPHER_CHAR_SET = "abcdefghijklmnopqrstuvwxyz";

    public static void main(String args[]) {
        if (args.length != 3) {
            System.out.println("Exact 3 parameters required - [action] [key] [target]");
            System.exit(1);
        }

        String action, key, target;
        action = args[0];
        key = args[1];
        target = args[2];

        MyAlgorithm myAlgorithm = MyBuilder.builder()
                .withCipherSet(CIPHER_CHAR_SET)
                .build();

        if ("encrypt".equalsIgnoreCase(action)) {
            System.out.println(myAlgorithm.encryptText(key, target));

        } else if ("decrypt".equalsIgnoreCase(action)) {
            System.out.println(myAlgorithm.decryptText(key, target));

        } else if ("encryptDir".equalsIgnoreCase(action)) {

            if(myAlgorithm.encryptDirectory(key, target)) {
                System.out.println("Directory encryption Successful");
            }

        } else if ("decryptDir".equalsIgnoreCase(action)) {

            if(myAlgorithm.decryptDirectory(key, target)) {
                System.out.println("Directory decryption Successful");
            }

        } else {
            System.out.println("action [" + action + "] not implemented");
        }

    }
}

Hello and welcome to Code Review! I try to give a possible explanation of what the reviewer meant when he was talking about inverse class structure; in the main method this code is present:

MyAlgorithm myAlgorithm = MyBuilder.builder()
               .withCipherSet(CIPHER_CHAR_SET)
               .build();

But from your code MyAlgorithm is a subclass of EncryptionAndDecryption and this is the class where you defined the operations of encryption and decryption , so the method shoud have return an instance of class EncryptionAndDecryption instead of one subclass.
Other problem: in the class EncryptionAndDecryption we have the below methods:

protected final String encryptOrDecryptText(final String cipher, final String inputText, final MyAlgorithm.Action action)

protected final void encryptOrDecryptFile(final String cipher, final Path filePath, final String baseDirectory, final MyAlgorithm.Action action)

With parameter MyAlgorithm.Action action the class EncryptionAndDecryption will be dependant from its subclass MyAlgorithm and without its subclass it cannot be even compiled.
In the class EncryptionAndDecryption I would make two distinct methods for encryption and decryption of strings, putting these related to files and directory in the subclass MyAlgorithm, deleting the enum value you are using to distinguish between the two operations.
Style problem : in the main method is present a list of if like this below:

if ("encrypt".equalsIgnoreCase(action)) {}

Normally it is the string compared to the string literal like:

if (action.equalsIgnoreCase("encrypt")) {}

You can substitute the list of if with a switch like documented in strings-switch.


Testing a shell command interpretor I've written tests that use Bash for testing a custom shell. I think the tests work but I'm sure you know better because this is my first test I write this way.

#!/bin/bash
echo "-- Testing our implementation of OpenShell --"
echo ""
echo "- If you have any problem in passing a test read the corresponding"
echo "- source file to understand what the test is checking"
echo ""
echo -n "********************* PRESS ENTER TO RUN TESTS  ... "
read answ
echo ls -al openshell.*|./shell
echo -n "********************* TEST ALGORITHMS ... "
read answ
./shell < <(echo "ls -al|grep open|awk '{print \$9}'")
echo -n "********************* TEST DONE. YOU SHOULD SEE FILENAMES ABOVE ... "

Now I run the test and see what I expect, my command interpreter executes the pipeline from the Bash script.

$ ./RUN_TESTS
-- Testing our implementation of OpenShell --

- If you have any problem in passing a test read the corresponding
- source file to understand what the test is checking

********************* PRESS ENTER TO RUN TESTS  ...
Current working dir: /home/dac/ClionProjects/shell2/openshell
'PATH' is set to /usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin.
stdin is a file or a pipe

p[0][0] ls
p[0][1] -al
p[0][2] openshell.h

-rw-rw-r-- 1 dac dac 1439 maj  1 16:45 openshell.h
********************* TEST ALGORITHMS ...
Current working dir: /home/dac/ClionProjects/shell2/openshell
'PATH' is set to /usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin.
stdin is a file or a pipe
 {ls} {-al} {|} {grep} {open} {|} {awk} {{print $9}} {|}
p[0][0] ls
p[0][1] -al
p[1][0] grep
p[1][1] open
p[2][0] awk
p[2][1] {print $9}

openshell-0.16430.tar.gz
openshell.h
********************* TEST DONE

Instead of multiple echo statements, consider using a here document:

cat <<EOF
-- Testing our implementation of OpenShell --

- If you have any problem in passing a test read the corresponding
- source file to understand what the test is checking
EOF

To echo an empty line, you can write echo without parameters instead of echo.

The flags of echo are not portable. Instead of echo -n, you can use printf.

If you don't need the user input, you don't need a parameter for read.

Parsing the output of ls -l is considered a bad practice.

Instead of this:

./shell < <(echo "...")

You can use a here string:

./shell <<< "..."

Some vertical space (blank lines) can improve the readability of your code.

Putting it all together, your script would be better this way:

#!/bin/bash
cat << EOF
-- Testing our implementation of OpenShell --

- If you have any problem in passing a test read the corresponding
- source file to understand what the test is checking

EOF
printf "********************* PRESS ENTER TO RUN TESTS  ... "
read

echo ls -al openshell.* | ./shell
printf "********************* TEST ALGORITHMS ... "
read

./shell <<< "ls -al|grep open|awk '{print \$9}'"
printf "********************* TEST DONE. YOU SHOULD SEE FILENAMES ABOVE ... "

Update

As per your follow-up question, and as @Mat pointed out, neither <(cmd) nor here-strings will work in dash. To make the last statement work, you could use a here document:

./shell << EOF
ls -al|grep open|awk '{print \$9}'
EOF


How to re-factor dynamic menu code to OOP in a MVC framework? I have a MVC framework, and I am struggling with the following dynamic menu code.

A menu is pretty much HTML, so technically it should reside in the View, but the dynamic part is driven by PHP and a config class that holds all the menu items.

Where would I place this class? (model layer, view, library, or helper folder)
How would I re-factor this to be OOP?
How would I deploy this from my View?

Note: It should all be in regards to MVC principles.

class Menu {
    public static function get() {
        $URI = $_SERVER['REQUEST_URI'];

        $user = new User();

        if (!$user->isLoggedIn()) {
            $navArr = Config::get('navigation');

            $navigation = '
' . PHP_EOL;

            foreach ($navArr as $name => $path) {
                $navigation .= '
' . $name . '
' . PHP_EOL;
            }

            $navigation .= '
' . PHP_EOL . PHP_EOL;
        } else {
            //logged off navigation
        }

        return $navigation;
    }
}

Ok, firstly, you never want HTML in your controller. Always keep your output in your view, your application flow in the controller and your business logic in the model. Otherwise, this breaks separation of concerns (SoC). See https://stackoverflow.com/questions/1376643/mvc-separation-of-concerns for a relevant answer.

Below, I am defining two classes - URI and User. These two should exist in your MVC platform to handle URI and User based actions (checking if a user is logged in, checking for the current URI or Permalink, etc). These would likely be helpers. We will use these later on by injecting them into our Menu::get() function.

# Placeholder classes URI and User, which handle all URI and User activity
class URI{
    # ...
}

class User{
    # ...
}

Below is the Menu class. Note that I namespaced it, considering its a helper, this would allow it to be called with full namespace path if you placed the helper directory into your autoloader.

Note how we are injecting an instance of URI via parameter $uri, and an instance of User via parameter $user. This is Dependency Injection (see https://stackoverflow.com/questions/130794/what-is-dependency-injection).

# Helper class "Menu"
namespace KidDiamond\MVC\Helpers;

class Menu {
    # Rather than defining a new User object, just inject the user object as a parameter
    public static function get(URI $uri, User $User){
        if (!$user->isLoggedIn()) {
            $navArr = Config::get('navigation');

            $menu = '
' . PHP_EOL;
            foreach ($navArr as $name => $path) {
                $xor_active = ((in_array($URI, $path)) ? ' class="active"' : false);
                $menu .= '\t
' . $name . '
' . PHP_EOL;
            }
            $menu .= '
' . PHP_EOL . PHP_EOL;
        } else {
            # logged off navigation menu
        }

        return $menu;
    }
}

Now, we move on to the Controller. Notice how all its doing is maintaining an initialized $menu_helper variable. You could technically inject the Menu helper...but I personally prefer to initialize it to a property.

The $data variable inside the index() method is automatically returned, and injected into our view, below.

# Controller class HomeController
namespace KidDiamond\MVC\Controllers;

class HomeController extends \BaseController{
    private $menu_helper;

    public function __construct(){
        $this->menu_helper = new KidDiamond\MVC\Helpers\Menu;
    }

    public function index(){
        $data['dynamic-menu'] = $this->menu_helper();

        return $data;
    }
}

This is our (simple) view. Notice that we're using a templating system (Blade, Mustache, etc) to output our variable dynamic-menu which was in the $data array in our controller method.

# View for /index

            {{dynamic-menu}}

I hope this helps. Do note that the code above is completely untested and is only there to help point you in the right direction. There may be syntax errors and other general errors.


Basic IMDb scraper and movie generator I just built my first scraper and I'd like to get your thoughts on the structure and the way I went about it. The basic premise of the script:

Get a random movie from IMDb's Top 250
Ask the user whether they've seen it before. If yes, print another random movie. If no, tell them to enjoy the movie and the script ends.

from lxml import html
import requests

def moviePicker():
    hasSeen = False
    while hasSeen == False:
        import random
        page = requests.get('http://www.imdb.com/chart/top')
        tree = html.fromstring(page.content)
        random = random.randint(1, 250)

        title = tree.xpath('//*[@id="main"]/div/span/div/div/div[3]/table/tbody/tr[' + str(random) +']/td[2]/a/text()')
        yearReleased = tree.xpath('//*[@id="main"]/div/span/div/div/div[3]/table/tbody/tr[' + str(random) + ']/td[2]/span/text()')

        # Parse the movie's url and pull the summary from the details page
        movieUrl = str(tree.xpath('//*[@id="main"]/div/span/div/div/div[3]/table/tbody/tr[' + str(random) + ']/td[2]/a/@href'))
        removeFront = movieUrl.replace("['", "")
        cleanUrl = 'http://www.imdb.com' + removeFront.replace("']", "")
        moviepage = requests.get(cleanUrl)
        details = html.fromstring(moviepage.content)
        movieSummary = details.xpath("normalize-space(//div[@class='summary_text']/text())")

        print(title)
        print(yearReleased)
        # print(cleanUrl)
        print(movieSummary)

        while True:
            answer = str.upper((input("Have you already watched this movie? Enter Y or N. : ")))
            if answer in ['Y', 'N']:
                break
            else:
                print("Invalid input. Please enter a Y or a N. ")
                continue
        if answer == 'Y':
            hasSeen = False
        elif answer == 'N':
            print("Enjoy the movie!")
            hasSeen = True

moviePicker()

The first thing that comes to attention is that you are repeating stuff unnecessarily.

Every loop, you try to import random and download the movie charts.

Importing random has only a small performance penalty, since Python will quickly notice that it is already imported, but there is some penalty nevertheless.

Downloading the charts again and again depends highly on the speed of your internet.

It is usually also good to separate your concerns into modular functions. This way you can easily swap out implementations or reuse them later.

Python has an official style-guide, PEP8, which programmers are encouraged to adhere to. It recommends using lower_case_with_underscores for variables and functions, instead of camelCase.

I would also use str.format to put the n into the path string and make the path strings themselves constants, which makes it easier to change them if IMDB ever changes their website.

Doing this, your code becomes:

from lxml import html
import requests
import random

TITLE ='//*[@id="main"]/div/span/div/div/div[3]/table/tbody/tr[{}]/td[2]/a/text()'
YEAR = '//*[@id="main"]/div/span/div/div/div[3]/table/tbody/tr[{}]/td[2]/span/text()'
URL = '//*[@id="main"]/div/span/div/div/div[3]/table/tbody/tr[{}]/td[2]/a/@href'

def get_summary(url):
    moviepage = requests.get(url)
    details = html.fromstring(moviepage.content)
    return details.xpath("normalize-space(//div[@class='summary_text']/text())")

def get_movie(tree, n):
    title = tree.xpath(TITLE.format(n))
    year = tree.xpath(YEAR.format(n))
    url = str(tree.xpath(URL.format(n))).replace("['", "").replace("']", "")
    url = 'http://www.imdb.com' + url
    return title, year, url

def get_user_yn(message):
    while True:
        answer = input(message).upper()
        if answer in ('Y', 'N'):
            return answer == 'Y'
        print("Invalid input. Please enter a Y or a N. ")

def movie_picker():
    page = requests.get('http://www.imdb.com/chart/top')
    tree = html.fromstring(page.content)
    while True:
        # Parse the movie's url and pull the summary from the details page
        n = random.randint(1, 250)
        title, year, url = get_movie(tree, n)
        summary = get_summary(url)
        print(title)
        print(year)
        print(summary)
        if not get_user_yn("Have you already watched this movie? Enter Y or N. : "):
            print("Enjoy the movie!")
            break

if __name__ == '__main__':
    movie_picker()

With this done, next I will focus on the actual implementation of these functions.

The parser you use is very manual. You have to pass the whole path to the element you are looking for. A more user-friendly parser is BeautifulSoup from the module bs4. You can install it on Ubuntu/other Debian derivatives via sudo apt install python-beautifulsoup.

It has a method called find which returns the first tag matching some criteria. So to find the title, we can just do soup.find("td", class_="titleColumn"). Or use the find_all command to get a list of all matching tags. You will see that the parsing is a lot easier to follow with this, compared to lxml.

I would shuffle the movies list and then iterate over the shuffled list, guaranteeing that a movie will not be picked twice (during one run of the script).

from bs4 import BeautifulSoup
import requests
import random

def get_movies(url):
    page = requests.get(url)
    soup = BeautifulSoup(page.text, 'html.parser')
    movies = soup.find_all("td", class_="titleColumn")
    random.shuffle(movies)
    return movies

def get_summary(url):
    movie_page = requests.get(url)
    soup = BeautifulSoup(movie_page.text, 'html.parser')
    return soup.find("div", class_="summary_text").contents[0].strip()

def get_movie_info(movie):
    title = movie.a.contents[0]
    year = movie.span.contents[0]
    url = 'http://www.imdb.com' + movie.a['href']
    return title, year, url

def get_user_yn(message):
    while True:
        answer = input(message).upper()
        if answer in ('Y', 'N'):
            return answer == 'Y'
        print("Invalid input. Please enter a Y or a N. ")

def movie_picker():
    for movie in get_movies('http://www.imdb.com/chart/top'):
        # Parse the movie's url and pull the summary from the details page
        title, year, url = get_movie_info(movie)
        summary = get_summary(url)
        print(title, year)
        print(summary)
        if not get_user_yn("Have you already watched this movie? Enter Y or N. : "):
            print("Enjoy the movie!")
            break

if __name__ == '__main__':
    movie_picker()


Android log in file for testing purpose I have created a method which shows log in logcat and also writes into the file. Every time this method is called it appends String in file. There are so many read/write operations on disk/non-volatile storage/SD card happening. I want to batch these operations so that minimum SD card read/write happens and i do not miss any log info.
   I have a idea to store logs in static StringBuilder variable and check if its length>200000 then write to file otherwise append it in variable. Will that work perfectly, how other developer handle this task?

public static void log(String tag, String s, @ErrorType int error) {
    Log.d(tag, s);
    if (MyApplication.sExternalStorageDirectory == null) {
        new NullPointerException("sExternalStorageDirectory is null")
                .printStackTrace();
        return;
    }
    String fileName = "logs";
    String fileExtension = ".txt";
    if (error == ErrorType.WEBSERVICE_RESPONSE_ERROR) {
        fileName = "web_api_error_log";
    } else if (error == ErrorType.CLIENT_SIDE_ERROR) {
        fileName = "client_log";
    }
    File f = new File(MyApplication.sExternalStorageDirectory, fileName + fileExtension);
    try {
        if (!f.exists()) {
            f.createNewFile();
        }
        BufferedWriter bfw = new BufferedWriter(new FileWriter(f, true));
        bfw.append(tag + "->" + s);
        bfw.newLine();
        bfw.close();
    } catch (IOException e) {
        e.printStackTrace();
    }
}

This review will consist of two parts: One part that addresses the code you present and one part that will make the whole code completely superfluous...
The second part is easy:
Use a logging framework
Use the Android standard Log class. Everything else is just a configuration file. Other than that you won't need any configuration... This whole method becomes completely useless as soon as you do that..

Now for the first part:

if (MyApplication.sExternalStorageDirectory == null) {
    new NullPointerException("sExternalStorageDirectory is null")
            .printStackTrace();
    return;
}

This part of the code is basically useless (at least in this place). You should ensure on startup (or to be more correct in onCreate) that this "ExternalStorageDirectory" exists and is not null. Then you should obtain a lock on it and only give that away for onDestroy. If you can guarantee that this exists during the lifetime of your app, you don't need that check.

if (error == ErrorType.WEBSERVICE_RESPONSE_ERROR) {
    fileName = "web_api_error_log";
} else if (error == ErrorType.CLIENT_SIDE_ERROR) {
    fileName = "client_log";
}

This code implies that ErrorType is an enum. If it isn't, it should be. int-flags are a thing of the last millennium.

File f = new File(MyApplication.sExternalStorageDirectory, fileName + fileExtension);
try {
    if (!f.exists()) {
        f.createNewFile();
    }
    BufferedWriter bfw = new BufferedWriter(new FileWriter(f, true));
    bfw.append(tag + "->" + s);
    bfw.newLine();
    bfw.close();
} catch (IOException e) {
    e.printStackTrace();
}

This is where your performance problem is. What you actually want to do is cache the BufferedWriter instances in the static context to avoid the significant I/O overhead you incur when you create them.
You're looking for a "static initializer block":
class YourClass {
    private static final BufferedWriter webserviceLog;
    private static final BufferedWriter clientLog;
    private static final BufferedWriter defaultLog;
    static {
        // ensure external storage directory exists
        // wrap this in a try-catch block!
        webserviceLog = new BufferedWriter(new FileWriter(new File(MyApplication.sExternalStorageDirectory, "web_api_error_log.txt", true)));
        clientLog = new BufferedWriter(new FileWriter(new File(MyApplication.sExternalStorageDirectory, "client_log.txt", true)));
        defaultLog = new BufferedWriter(new FileWriter(new File(MyApplication.sExternalStorageDirectory, "logs.txt", true)));
    }

It should be rather obvious how you have to adapt the code of the method you showed to work with these instances instead of building filenames...


Faster Sieve of Eratosthenes This is an implementation of the Sieve of Eratosthenes :

It takes advantages of the fact that all primes from 5 and above can be written as 6X-1 or 6X+1,
For better space complexity, it uses a pretty accurate upperbound. Better estimations of the upper bound can be found here. I've observed a very slight increase in performance with this.

func eratosthenesSieve(to n: Int) -> [Int] {

    guard 2 <= n else { return [] }

    var composites = Array(repeating: false, count: n + 1)
    var primes: [Int] = []
    let d = Double(n)
    let upperBound = Int((d / log(d)) * (1.0 + 1.2762/log(d)))
    primes.reserveCapacity(upperBound)
    let squareRootN = Int(d.squareRoot())

    //2 and 3
    var p = 2
    let twoOrThree = min(n, 3)
    while p <= twoOrThree {
        primes.append(p)
        var q = p * p
        let step = p * (p - 1)
        while q <= n {
            composites[q] = true
            q += step
        }
        p += 1
    }

    //5 and above
    p += 1
    while p <= squareRootN {
        for i in 0..<2 {
            let nbr = p + 2 * i
            if !composites[nbr] {
                primes.append(nbr)
                var q = nbr * nbr
                var coef = 2 * (i + 1)
                while q <= n  {
                    composites[q] = true
                    q += coef * nbr
                    coef = 6 - coef
                }
            }
        }
        p += 6
    }

    while p <= n {
        for i in 0..<2 {
            let nbr = p + 2 * i
            if nbr <= n && !composites[nbr] {
                primes.append(nbr)
            }
        }
        p += 6
    }

    return primes
}

It was inspired by this code by Mr Martin.

Using the same benchmarking code in that answer, adding a fourth fractional digit in the timing results, plus some formatting, here are the results :

        ---------------------------------------------------------------
        |                |       Nbr   |          Time (sec)          |
        |      Up to     |        of   |------------------------------|
        |                |    Primes   |   Martin's   |      This     |
        |----------------|-------------|------------------------------|
        |        100_000 |        9592 |       0.0008 |        0.0004 |
        |----------------|-------------|--------------|---------------|
        |      1_000_000 |      78_498 |       0.0056 |        0.0026 |
        |----------------|-------------|--------------|---------------|
        |     10_000_000 |     664_579 |       0.1233 |        0.0426 |
        |----------------|-------------|--------------|---------------|
        |    100_000_000 |   5_761_455 |       1.0976 |        0.5089 |
        |----------------|-------------|--------------|---------------|
        |  1_000_000_000 |  50_847_534 |      12.1328 |        5.9759 |
        |----------------|-------------|--------------|---------------|
        | 10_000_000_000 | 455_052_511 |     165.5658 |       84.5477 |
        |----------------|-------------|--------------|---------------|

Using Attabench, here is a visual representation of the performance of both codes while n is less than 2^16:

One thing I observe is some elements in the composites array are marked with true multiple times. This is expected (but unwanted) behavior since 6X-1 or 6X+1 aren't all primes.

What I'm looking for is making this Sieve of Eratosthenes quicker. I'm well aware of faster methods of finding primes.

Naming, code clarity, conciseness, consistency, etc, are welcome but are not the main point here.

It takes advantages of the fact that all primes from 5 and above can be written as 6X-1 or 6X+1,

I don't think it does, really. It structures the code around that fact, but to take advantage of it, at a minimum you should replace

    while p <= twoOrThree {
        primes.append(p)
        var q = p * p
        let step = p * (p - 1)
        while q <= n {
            composites[q] = true
            q += step
        }
        p += 1
    }

with

    while p <= twoOrThree {
        primes.append(p)
        p += 1
    }

which in my testing gives a significant speedup.

To maximise the advantage, you could reduce composites to only store flags for \$6X \pm 1\$. Proof of concept code (could be tidier):

    var pidx = 1
    p = 5
    while p <= squareRootN {
        if !composites[pidx] {
            primes.append(p)

            var qidx = 3 * pidx * (pidx + 2) + 1 + (pidx & 1)
            let delta = p << 1
            let off = (4 - 2 * (pidx & 1)) * pidx + 1
            while qidx < composites.count {
                composites[qidx - off] = true
                composites[qidx] = true
                qidx += delta
            }
            if qidx - off < composites.count {
                composites[qidx - off] = true
            }
        }

        pidx += 1
        p += 2 + 2 * (pidx & 1)
    }

    while p <= n {
        if !composites[pidx] { primes.append(p) }
        pidx += 1
        p += 2 + 2 * (pidx & 1)
    }

This gives a moderate speedup in my testing.


Physics particle collisions I have a large file that I want to refactor.  My idea is to make helper classes with helper methods so that I can modularize methods.

Is that a good idea? For instance before refactoring:

public void collision(PhysicsCollisionEvent event) {
    if (event.getObjectA() instanceof BombControl) {
        final Spatial node = event.getNodeA();
        effect.killAllParticles();
        effect.setLocalTranslation(node.getLocalTranslation());
        BombControl bc = (BombControl) event.getObjectA();
        effect.emitAllParticles();
    } else if (event.getObjectB() instanceof BombControl) {
        final Spatial node = event.getNodeB();
        effect.killAllParticles();
        effect.setLocalTranslation(node.getLocalTranslation());
        effect.emitAllParticles();
    }
}

After refactoring:

public void collision(PhysicsCollisionEvent event) {
    CollisionHelper.collisionHelp(event, effect);
}

Is that a good idea or must I use a more elaborate recfactoring? I think it would work but I don't know of specific refactoring patterns.

A couple of simple things I notice right off the bat.

If you have a method which only calls another method, why bother having the first method at all?  I'm personally against these one-line wrapper methods and see them as unnecessary noise.
Your methods should be verb phrases, in general (e.g., CollisionHelper.helpCollision or even performCollision, in this case, since it's doing all of your processing).
I believe there's a standard for replacing instanceof usage with an actual method from the Class API, as follows: BombControl.class.isInstance(event.getObjectA()).  You can see more on this Stack Overflow Q&A.  (Apparently you're using it correctly here, however.)

From the comments: you obviously should move the entire huge method body to the helper method, otherwise you have the same problem.  Just extract 'til you drop.


Search by parts of Arabic word on JSON I've just implemented a search box, so I can search on employee object by (name, part of the name, phone, email ...).
Ex. if the name: Moamen Abdulraouf Mansour, I can search with ( moamen | moa | moamen mansour ...).
The Idea I've followed is:

split some values from the object to be not included in the search.
convert each object of the array to one string,
then replace extra spaces and some special Arabic letters with another letter (آ أ إ => ا) and so on.
then convert the search text string to an array of words.
After that, going through each object and find if it includes all words on the search text
finally, return this object to be the result.

the JSON looks:
{
      "id": "0",
      "ORGANIZATION_NAME": "ORGANIZATION NAME",
      "ORGANIZATION_NAME_LINK": "....",
      "ORGANIZATION_TYPE_CODE": "MIN",
      "ORGANIZATION_TYPE": "type",
      "pid": "99999",
      "ORIGINAL_ORGANIZATION_LEVEL": 0,
      "DeptUrl": "MI0",
      "IsCreated": true,
      "ManImageUrl": "https://....jpg",
      "EMPLOYEECODE": "90000419",
      "EMP_NO_ERP": "90000419",
      "TITLE": "السيد",
      "USER_FULL_NAME": "employee name",
      "USER_POSITION_TITLE": "title",
      "USER_EMAIL": "..@domain.com",
      "SEX_CODE": "M",
      "USER_EXTENSION": "4999",
      "USER_MOBILE": "1234",
      "USER_HOME_PHONE": "1234"
    },...

and this is the JS:
let searchBtn = document.getElementById('searchBtn');
let myList = document.getElementById('myList');
let arr = [];
let searchKeyword;
var resultsSize = 5;

async function getUserAsync(keywords) {
  let response = await fetch('http://localhost:3000/users');
  let data = await response.json();
  return data;
}

searchBtn.addEventListener('keyup', (e) => {
  // get serach text, remove spaces from start and end and remove more than one space between, then convert to array of words
  searchKeyword = e.target.value
    .replace(/\s+/g, ' ')
    .replace(/[\u0623|\u0622|\u0625]/g, 'ا')
    .replace(/[\u0649]/g, 'ي')
    .replace(/[\u0629]/g, 'ه')
    .trim()
    .split(' ');

  getUserAsync(searchKeyword).then((data) => {
    arr = data;

    if (searchKeyword[0].length > 2) {
      let filtered = arr.filter(function (el) {
        let {
          id,
          DeptUrl,
          pid,
          IsCreated,
          TITLE,
          SEX_CODE,
          ManImageUrl,
          ORGANIZATION_TYPE,
          ORGANIZATION_TYPE_CODE,
          ORIGINAL_ORGANIZATION_LEVEL,
          fortags,
          ...rest
        } = el;

        let elCompind = Object.values(rest)
          .join()
          .replace(/\s+/g, ' ')
          .replace(/[\u0623|\u0622|\u0625]/g, 'ا')
          .replace(/[\u0649]/g, 'ي')
          .replace(/[\u0629]/g, 'ه');

        let checkAllWords = searchKeyword.map((item) => {
          return elCompind.includes(item);
        });

        return checkAllWords.includes(false) ? false : true;
      });
      appendFiltered(filtered.slice(0, resultsSize));
    } else {
      appendFiltered([]);
    }
  });
});

function appendFiltered(filteredArray) {
  // reset container HTML
  myList.innerHTML = '';

  filteredArray.forEach((element) => {
    // console.log(element);
    let node = document.createElement('div'); // Create a node

    node.innerHTML = `

${element.USER_FULL_NAME}

${element.ORGANIZATION_NAME}

${element.USER_EMAIL}

${element.USER_POSITION_TITLE}

    `;
    myList.appendChild(node); // Append to body
  });
}

the code works well, but I believe this code can be improved.

You didn't post your HTML, but the element with the id searchBtn doesn't seem to be a button, so it shouldn't be called "btn" in it's id or variable name.
There are many variables that don't change their value, so you should be using const instead of let (or var).
The variables arr and searchKeyword are only used inside the event handler, so they should be declared in there and not outside.
The function getUserAsync doesn't use its parameter.
You should use the event input instead of keyup, because the value of the input can change without a keypress.
The same .replace calls are used on the keywords and the content being searched, so that should be extracted into their out function.
The requirement that only the first keyword must be longer than two characters seems a bit arbitrarily. The user won't get any results, even if there are more valid (longer) keywords, but they will get results from invalid (shorter) keywords if they are not in the first position. It probably would make more sense to filter out words shorter than three out of the list of keywords.
Using a deconstructing assignment to create variables that you don't use and in order to exclude certain object properties seems strange. It would probably make more sense to do something like:
const IGNORED_PROPERTIES = ["id", "DeptUrl", "pid" /* etc. */ ];
const elCompind = Object.entries(el)
  .filter(([k, v]) => !IGNORED_PROPERTIES.includes(k))
  .map(([k, v]) => v)
  .join()
  /* etc. */;

The expression checkAllWords.includes(false) ? false : true can be simplified to !checkAllWords.includes(false), however it is not necessary at all, because instead of mapping all keywords to a boolean and then checking of there is a false item, you can use the .every array method. This also has the advantage, that it will stop once a single false has been found and won't necessarily check more keywords:
return searchKeyword.every(item => elCompind.includes(item)});

(Also notice the simplified lambda.)


The sniper: a Zombie searcher for specific tags Sometimes, it's not enough to know what the easy prey is. You also need to know whether you will be able to hunt it down. What good is an almost turned haskell zombie if you cannot read the functional incantations of the language? How will you as a vba guru handle a python behemoth?

That's why I present to you the tag-based zombie sniffer:

SELECT
  q.Id AS [Post Link],
  q.Tags
FROM
  Posts q
    INNER JOIN Posts a   ON q.Id           = a.ParentId
    INNER JOIN PostTags  ON q.Id           = PostTags.PostId
    INNER JOIN Tags      ON PostTags.TagId = Tags.id
WHERE
      q.AnswerCount > 0
  AND q.AcceptedAnswerId IS NULL
  AND Tags.TagName   = '##TagName##'
GROUP BY
  q.Id, q.Tags
HAVING
      MAX(a.Score) >= ##MinScore:int?0##
  AND MAX(a.Score) <= ##MaxScore:int?0##

-- TagName: Tag name "as you would use in your post"
-- MinScore: Highest answer has at least votes:
-- MaxScore: Highest answer has at most votes:

You can find it on SEDE.

As it's predecessor, it looks for easy prey by default:

questions that have at least one answer
no answer has a score of more than 0
the highest scored answer has a score of at least 0
has no accepted answer

However, this one also takes a tag name so that you can search for C++ zombies or similar. This is the first time I've ever written an SQL query with three JOINs, so I have no idea whether this is considered good style. Also, I group by the q.Tags to show them in the result, but I think that's a hack and not really good style.

When using "simple" Aliases for table names in a query you should alias all the tables, not just some of them. This makes the query appear more consistent. In your query, you have a and q aliases for the two facets of the Posts table (questions and answers, it makes sense), but you should also alias the PostTags and the Tags tables to keep things consistent.

Stylistically, though, your query is otherwise neat, and reads quite well.

When querying tables, you should always offer the optimizer as much information as possible. You're cheating in your queries by ignoring the PostTypeID column (1 for questions, 2 for answers). Don't rely on inferred values like null, or non-null ParentID values to fix a query.

Further, note that you can put constant conditionals in the JOIN syntax for a table. You don't need to force these constants in to the regular where-block. I prefer putting related conditions together even if they are not strictly part of the join between tables. This is a coder-preference thing... you may disagree.

The most significant issue I have with the query is the use of the having-clause, which in my mind has always been a tool-of-last-resort in a SQL query.

Common Table Expressions (CTEs) have made this sort of complexity much simpler by allowing the aggregation in a virtual table before the actual query runs. CTE's are easier to show, than describe, so consider this query with a CTE instead:

WITH BestAnswer as (
  select ParentId, Max(Score) as Score
  from Posts
  where PostTypeId = 2
  group by ParentId
)

SELECT
  q.Id AS [Post Link],
  q.Tags
FROM
  Posts q
    INNER JOIN PostTags pt
    ON    q.Id = pt.PostId
    INNER JOIN Tags t
    ON    pt.TagId = t.id
      AND t.TagName = '##TagName##'
    INNER JOIN BestAnswer a
    ON    q.Id = a.ParentId
      AND a.Score >= ##MinScore:int?0##
      AND a.Score <= ##MaxScore:int?0##
WHERE
      q.PostTypeId = 1
  AND q.AnswerCount > 0
  AND q.AcceptedAnswerId IS NULL

-- TagName: Tag name as you would use in your post
-- MinScore: Highest answer has at least votes:
-- MaxScore: Highest answer has at most votes:

See it in SEDE here.


Get the brightest rectangle within an image I wrote an extension method for bitmaps which finds the brightest rectangle (with specified width & height) within an image.

Hopefully my code comments eliminate any confusion. If not, please let me know what I need to elaborate on.

I would like some help reducing repetitive calls to the conjoined Bitmap.GetPixel & Color.GetBrightness methods during the scanning process. (subsequently improving performance tremendously)

Method:

    public static Rectangle GetBrightestRectangle(this Bitmap bitmap, int width, int height)
    {
        // Each rectangle's value is its average pixel color.
        var rectangles = new Dictionary<Rectangle, float>();

        // Iterate through all possible rectangle points.
        for (var x = 0; x < bitmap.Width - width; x++)
            for (var y = 0; y < bitmap.Height - height; y++)
            {
                var brightnesses = new List<float>();

                // Iterate through all rectangle pixels.
                for (var w = x; w < x + width; w++)
                    for (var h = y; h < y + height; h++)
                    {
                        brightnesses.Add(bitmap.GetPixel(w, h).GetBrightness());
                    }

                rectangles.Add(new Rectangle(x, y, width, height), brightnesses.Average());
            }

        // The brightness ranges from 0.0 through 1.0, where 0.0 represents black and 1.0 represents white.
        return rectangles.OrderByDescending(pair => pair.Value).First().Key;
    }

Demo:

        var bitmap = (Bitmap)Image.FromFile(@"C:\Steve.png");
        var stopwatch = Stopwatch.StartNew();
        var brightestRectangle = bitmap.GetBrightestRectangle(50, 25);
        stopwatch.Stop();
        using (var graphics = Graphics.FromImage(bitmap))
        {
            graphics.DrawRectangle(new Pen(Color.Lime, 5), brightestRectangle);
        }
        bitmap.Save(@"C:\Jobs.png");
        Console.WriteLine($"bitmap.Size: {bitmap.Size}");
        Console.WriteLine($"brightestRectangle: {brightestRectangle}");
        Console.WriteLine($"stopwatch.Elapsed.Seconds: {stopwatch.Elapsed.Seconds}");

Steve.png

Jobs.png

  bitmap.Size: {Width=500, Height=500}

  brightestRectangle: {X=194,Y=99,Width=50,Height=25}

  stopwatch.Elapsed.Seconds: 37

Performance

Here is one simple suggestion, which doesn't require any real thought, and is simple enough to implement. It will result in only calling GetPixel once per pixel, but obviously has some other costs.

Instead of convolving a whole w*h rectangle over the image, first compute the sum of each horizontal strip of length w and store it in a buffer (essentially the same size as the image). Then compute the average of each h tall vertical strip in this buffer, each of which represents a w*h rectangle in the original image.

My only hesitation with suggesting this, is that currently you seem to deliberately call Average on a collection of pixels, which could give you slightly different results owing to joys of float-point maths (assuming nothing about the implementation). Any such error would, however, be imperceptible, but we don't know what your application is, so it is not for us to say that this isn't an issue (but I would wager it isn't).

Because this is just as an example, and I don't know much about floating point maths, I'll follow suit with the OP and only ever perform additions: I'll keep a queue (e.g. ring buffer) of elements as we scan across and down. You could instead keep a running sum, instead of computing sums over a queue, which would make the algorithm complexity order 2 rather than order 3 (the code in the OP is order 4). This is probably the most sensible change if performance is a serious concern, but I'll let you work out the changes necessary.

I'll ignore the joys of considering the actual memory layout of bitmaps: it might be better to sum across the columns first, and then the width, but I can't remember.

int hSumWidth = bitmap.Width - rectWidth + 1;
int hSumHeight = bitmap.Height;
float[,] hSums = new float[hSumWidth, hSumHeight];

Queue<float> queue = new Queue<float>(rectWidth);

// I would never use `var` in a for-loop; makes it immediately obvious we are using ints and not floats
for (int y = 0; y < bitmap.Height; y++)
{
    for (int x = 0; x < bitmap.Width; x++)
    {
        // this is the only time we call bitmap.GetPixel(,)
        queue.Enqueue(bitmap.GetPixel(x, y).GetBrightness());

        if (x >= rectWidth)
        {
            hSums[x - rectWidth, y] = queue.Sum();
            // if the ring-buffer isn't fixed size, we may have to manually evict the old value at this point
            queue.Dequeue();
        }
    }

    queue.Clear(); // we don't need to do this, but it is much clearer if we do
}

Now we have our 2D buffer of sums of horizontal strips of width w. Now we perform a similar operation to scan down them, and find the 'brightest' rectangle.

I don't see any real reason to keep an actual record of all the rectangles; we can just remember the best as we go along. It will reduce the time complexity (from O(n^2 log(n)) to O(n^2)) and won't clutter the code too much.

queue = new Queue<float>(rectHeight);

float bestAverage = float.NegativeInfinity;
Rectangle bestRectangle = default(Rectangle);

for (int x = 0; x < hSumWidth; x++)
{
    for (int y = 0; y < bitmap.Height; y++)
    {
        queue.Enqueue(hSums[x, y]);

        if (y >= rectHeight)
        {
            float rectangleAverage = queue.Average();

            if (rectangleAverage > bestAverage)
            {
                bestRectangle = new Rectangle(x, y - rectHeight, rectWidth, rectHeight);
                bestAverage = rectangleAverage;
            }

            // if the ring-buffer isn't fixed size, we may have to manually evict the old value at this point
            queue.Dequeue();
        }
    }

    queue.Clear();
}

return bestRectangle;

I have tested this quickly against your test image, and it finds a rectangle in the same place (quick visual inspection, no promises). Your code has an order 4 time complexity; this only has order 3 time complexity, 2 if you replace the queues with a running count. There is really nothing clever going on, but I think it ought to work. A gist of the complete method is provided.

Other

I've changed width and height into rectWidth and rectHeight, so that there can be no confusion.

I think you have an off-by-one error:

for (var x = 0; x < bitmap.Width - width; x++)

Consider if you had a 1*1 image, looking for 1*1 rectangles. This loop would never run, no rectangle would be found, and .First() would crash on the last line.


Object-oriented web scraping with Python I usually write function-only Python programs, but have decided on an OOP approach (my first thereof) for my current program, a web-scraper. Here is the working code I wrote which I am looking to have critiqued:

import csv
import urllib2

NO_VACANCIES = ['no vacancies', 'not hiring']

class Page(object):
    def __init__(self, url):
        self.url = url

    def get_source(self):
        self.source = urllib2.urlopen(url).read()
        return self.source

class HubPage(Page):
    def has_vacancies(self):
        return not(any(text for text in NO_VACANCIES if text in self.source.lower()))

urls = []
with open('25.csv', 'rb') as spreadsheet:
    reader = csv.reader(spreadsheet)
    for row in reader:
        urls.append(row[0].strip())

for url in urls:
    page = HubPage(url)
    source = page.get_source()
    if page.has_vacancies():
        print 'Has vacancies'

Some context: HubPage represents a typical 'jobs' page on a company's web site. I am subclassing Page because I well eventually subclass it again for individual job pages, and some methods that will be used only to extract data for individual job pages.

Here's my issue: I know from experience that urllib2, while it has its critics, is fast - very fast - at doing what it does, namely fetch a page's source. Yet I notice that in my design, processing of each url is taking a few orders of magnitude longer than what I typically observe.

Is it the fact that class instantiations are involved (unnecessarily,
perhaps)?
Might the fact that HubPage is inherited be at cause?
Is the call to any() expensive?

Use top-level functions

Reading URLs from a file and checking each of them for vacancies is done at the top-level of the file. You should enclose this code into functions and put the remaining top-level code under if __name__ == '__main__' to:

separate the intents;
reuse the code more easily;
not have random code execution when importing the module.

Use filter

You are only interested in knowing which URL have vacancies. The easiest way to perform that is to:

interesting_urls = filter(has_vacancies, url)

with the proper definition of has_vacancies.

You also only print 'Has vacancies' when a given URL has so, letting the user try to figure out which one actually has. You might want to print the URL instead to make your script more usable:

print 'Following URLs have vacancies'
for url in filter(has_vacancies, urls):
    print url

Use generators

There is no need to store all the URLs you’ll be checking up-front. Yield them from a generator since only one is processed at a time:

def read_urls(filename):
    with open(filename, 'rb') as spreadsheet:
        reader = csv.reader(spreadsheet)
        for row in reader:
            yield row[0].strip()

It also allows you to parametrize with the name of the file to read, in case you have several of them.

Do not force OOP when not required

You are not interested in storing a state about each URL. You just want a simple question answered: "does this URL points to a page that provide vacancies?" Thus a simple function will do, for each URL:

def has_vacancies(url):
    source = urllib2.urlopen(url).read()
    return not(any(text for text in NO_VACANCIES if text in source.lower()))

See what I did there? This is the exact function we need for the filter I introduced earlier.

Remove extraneous computation

Calling lower() on a string has its cost. Calling it once for every piece of text your are looking for in said string is even more costly. You should call it only once after fetching the source of the URL.

Of a lower interest, any is converting pieces of text to their truthy value after a test text in source already returned True. You also store source, self.url, self.source (and, to a lesser extend, page) without really needing it. But I’ve shown ways to avoid it.

Proposed improvements

import csv
import urllib2

NO_VACANCIES = ['no vacancies', 'not hiring']

def has_vacancies(url):
    source = urllib2.urlopen(url).read().lower()
    return not any(text in source for text in NO_VACANCIES)

def read_urls(filename):
    with open(filename, 'rb') as spreadsheet:
        reader = csv.reader(spreadsheet)
        for row in reader:
            yield row[0].strip()

def check_vacancies(filename):
    print 'Following URLs have vacancies'
    for url in filter(has_vacancies, read_urls(filename)):
        print url

if __name__ == '__main__':
    check_vacancies('25.csv')

Going further

You might be interested in trying the re module to perform text matches, there may be a performance improvement.

Alternatively, the multiprocessing module provide a map_async performed in parallel by a pool of workers. You might want to change has_vacancies a bit and use that instead of filter to increase overall performances.

You may be interested into handling exceptions from urllib2 so a sudden networking failure or an invalid URL won't crash everything. Something simple can do:

def has_vacancies(url):
    try:
        source = urllib2.urlopen(url).read()
    except urllib2.URLError:
        print 'Error processing', url
        return False
    else:
        source = source.lower()
    return not any(text in source for text in NO_VACANCIES)


Comparing version numbers contained in a text string I was trying to make something to compare two version numbers. The version numbers should consist of three parts (1.4.3). I also wanted to take into account if there is badly formatted data or if 1.4 is used instead of 1.4.0. Any bad data will be converted to a 0. I wanted to find something simple and fast. This is what I came up with:

version(a,b){

    // insure that there are enough decimal points
    a += ".0.0.0"
    b += ".0.0.0"

    //take only the first three elements
    a_arr = a.split(".",3)
    b_arr = b.split(".",3)

    a_int = a_arr.map(function (value) {
        val = parseInt(value, 10)
        return val ? val : 0
    })

    b_int = b_arr.map(function (value) {
        val = parseInt(value, 10)
        return val ? val : 0
    })

    console.log(a_int)
    console.log(b_int)

    status = "equal"
    i = 0

    do {
        if (a_int[i] < b_int[i])
            status = "less"
        else if (a_int[i] > b_int[i])
            status = "greater"
        i++
    }
    while (status == "equal" && i < 3)

    console.log("Status:" + status)

}

What are your thoughts? Is it a bit too hackish? What would be a better approach?

a_int = a_arr.map(function (value) {
    val = parseInt(value, 10)
    return val ? val : 0
})

b_int = b_arr.map(function (value) {
    val = parseInt(value, 10)
    return val ? val : 0
})

This part can be extracted into its own function.

// insure that there are enough decimal points
a += ".0.0.0"
b += ".0.0.0"

Instead of appending an arbitrary number of segments, try using array.map against an array of 3 zeroes instead. Then either copy the segment from a_int/b_int if present or retain the zero.

status = "equal"
i = 0

do {
    if (a_int[i] < b_int[i])
        status = "less"
    else if (a_int[i] > b_int[i])
        status = "greater"
    i++
}
while (status == "equal" && i < 3)

This whole operation can be done with a reduce. Carry an "equal" string as initial value. Then loop through the 3 items in the array, comparing each, and updating the carried value to "greater" or "less".

version(a,b){

We don't really know from this alone which one is the reference, and which one is being compared. If the function returned "greater", is a greater than b or is b greater than a. Name the function and the variables properly.

Lastly, you forgot to define variables with either a var, let or const. The problem with not doing so, particularly with browser JS, is that declaring variables without them will shoot the variables to the global scope, defining them there instead of within this function. That means your variable is open for clobbering or is clobbering other variables of the same name.

This is how I would have carried this one out:

// Used as filler value and for us to loop 3 times when using array methods.
const filler = [0, 0, 0];

// Splits, normalizes into 3 numbers, zeroes invalid values
function versionToArray(version){
  const versionArray = version.split('.');
  return filler.map((fill, index) => {
    const segment = parseInt(versionArray[index], 10);
    return Number.isNaN(segment) ? fill : segment;
  });
}

function compareVersion(subject, reference){
  const ref = versionToArray(reference);
  const sub = versionToArray(subject);

  // Static values into constants
  const EQUAL = 'equal';
  const GREATER = 'greater';
  const LESS = 'less';

  return filler.reduce((status, segment, index) => {
    // One nice thing about ternaries is that they can be written this way.
    // Acts like a switch statements that cascade, while looking like a
    // key-value pair of condition-value.
    return status !== EQUAL ? status  // A greater segment already decided
                : sub > ref ? GREATER
                : sub < ref ? LESS
                            : EQUAL;  // If not less or greater, well...
  }, EQUAL); // Assume equal unless proven otherwise
}

console.log(compareVersion('1', '2'));
console.log(compareVersion('1.2', '2'));
console.log(compareVersion('2.2', '2'));
console.log(compareVersion('2.2.2', '2'));
console.log(compareVersion('2.0.0', '2'));
console.log(compareVersion('4.0.0', '2'));
console.log(compareVersion('1.1.1.1', '2'));