Rewriting the rent system of a building I'm trying to improve my coding practices and I was just wondering if (I'm definitely sure there are) there are better ways of doing the following task.

I'm rewriting the rent system of the building I live in, which currently (still) uses an Access '97 database. This is the schema that I've constructed and the following code fetches the data to be used in the section just below the nav.

I just have a few questions.

Am I doing too many queries? I'm pretty sure I cannot get all the information needed in just a single query (I've tried).
Naming conventions, should I be using $query1, $result1 etc or $person_query, $result_query. My logic is that, I don't need the resource variable again, why not reuse it? It's not like I create a new $conn each time.
Is there a simpler way of getting data, without using any framework?  I'm trying to solidify my own practices first before learning CakePHP. But perhaps CakePHP will force me to do that anyway.

<?php header('content-type: application/json; charset=utf-8');

//$_POST = json_decode(file_get_contents('php://input'));

$host = "127.0.0.1:3306";
$db = "bettenhaus";
$user = "root";
$pass = "";

$rent_types[0] = "Mieter";
$rent_types[1] = "Untermieter";
$rent_types[2] = "Zwischenmieter";

$json_id = $_GET['id'];

$conn = new PDO("mysql:host=$host;dbname=$db", $user, $pass, array(PDO::MYSQL_ATTR_INIT_COMMAND => "SET NAMES utf8"));

// get basic info
$query = "SELECT people.id, people.surname, people.firstname, people.email, leases.room_id, DATE_FORMAT(leases.in_date, '%e %M %Y') as in_date, DATE_FORMAT(leases.out_date, '%e %M %Y') as out_date, leases.rent_type FROM people, leases WHERE people.id = :json_id AND leases.person_id = :json_id";
$result = $conn->prepare($query);
$result->bindValue(":json_id", $json_id);
$result->execute();

// prepare to merge
$person_info = $result->fetch(PDO::FETCH_ASSOC);

// get lease info
$query = "SELECT num as room_num, wg, rent FROM rooms WHERE id = :room_id";
$result = $conn->prepare($query);
$result->bindValue(":room_id", $person_info['room_id']);
$result->execute();

$room_info = $result->fetch(PDO::FETCH_ASSOC);

// merge two results
$all_info = array_merge($person_info, $room_info);
$all_info['rent_type'] = $rent_types[$all_info['rent_type']];

// get all bills charged to this tenant
$query = "SELECT SUM(amount) as debt FROM money_due WHERE person_id = :json_id";
$result = $conn->prepare($query);
$result->bindValue(":json_id", $json_id);
$result->execute();

$debt_info =  $result->fetch(PDO::FETCH_ASSOC);

// get total amount of money paid into the system by tenant
$query = "SELECT sum(money_paid.amount) as sum FROM money_paid, money_due WHERE money_paid.money_for = money_due.id AND money_due.person_id = :json_id";
$result = $conn->prepare($query);
$result->bindValue(":json_id", $json_id);
$result->execute();

$paid_info = $result->fetch(PDO::FETCH_ASSOC);

// calculate money owed to the house
$all_info['balance'] = $paid_info['sum'] - $debt_info['debt'];

$conn = null;

echo json_encode($all_info);

?>

Functions!

Functions are what this code needs. You've already started separating your code logically, which is the first step in creating functions. Just look at your comments. You can start by making each commented section its own function. Of course, this isn't the end of it. We still have to make our PDO connection and query parameters available to these functions, and ideally we will want to make them reusable as well, but it will help us get started. I'll do the first one for you.

function getPeople( $conn, $json_id ) {
    $query = "SELECT people.id, people.surname, people.firstname, people.email, leases.room_id, DATE_FORMAT(leases.in_date, '%e %M %Y') as in_date, DATE_FORMAT(leases.out_date, '%e %M %Y') as out_date, leases.rent_type FROM people, leases WHERE people.id = :json_id AND leases.person_id = :json_id";
    $result = $conn->prepare($query);
    $result->bindValue(":json_id", $json_id);
    $result->execute();
    return $result->fetch( PDO::FETCH_ASSOC );
}

A function is a reusable script. Each function must have a unique name that should be indicative of its purpose. For instance, the above function sets up a query to get a list of people from a database and then returns the results. Thus its name tells us exactly what it is doing and it should do no more and no less. This is a principle called Single Responsibility. As the name implies, the function has one responsibility. Any additional tasks must be delegated to other functions. The two parameters, $conn and $json_id, are components that it needs to do its task. Instead of fetching these components each time, which is beyond the scope of this function's responsibilities, we inject them into our function via the parameters.

Just like PHP, MySQL and PDO do not care about whitespace. I'm still unfamiliar with the SQL syntax to properly space that out for you, but know that you can add as many newlines and whitespace as you need to make that more legible, in fact I would encourage you to.

Reusing our Functions

So, I created the first function for you. If you went ahead and made the other functions, like I suggested, then you might hate me for this, but they will be unnecessary. It was good practice though. You only need one function here. There is a general programming principle called "Don't Repeat Yourself" (DRY). As the name implies, your code should not repeat itself. There are many different ways you can keep your code DRY (variables, loops, and functions to name a few), but we are specifically going to talk about not repeating functionality right now. However, these concepts should be easily transferable to the other aspects of DRY.

How do we know if our code is DRY? If you look at all of those queries, you should begin to notice a pattern. They are all almost identical. This is a clear sign that we can apply the DRY principle. In order to do it with our functions we are going to have to abstract anything that is not exactly the same and inject those properties into our script through some other means. With functions we should do this with the parameters.

function query( $conn, $query, $params ) {
    $result = $conn->prepare( $query );

    foreach( $params AS $key => $value ) {
        $result->bindValue( ":$key", $value );
    }

    $result->execute();
    return $result->fetch(PDO::FETCH_ASSOC);
}

//usage
$params = array(
    ':json_id' => $json_id
);
query( $conn, $query, $params );

As you should be able to see from this, you will no longer need to manually fetch each query, instead you can use this function and pass in the required components. But, as I mentioned above, DRY doesn't stop here. If we were to leave it at this then already our code would be 10 times better, but we are still violating DRY by manually calling our new query function each time we need it. Remember that I mentioned DRY didn't have to be all about functions? Well, how would you handle this? I'll give you a hint, but I'm not going to write this next bit for you.

Mouseover for hint:

   Try using an array and foreach loop.

Bonus

Another thing we can do to help ensure we get the right kind of parameters is to implement something called type hinting. This is a little more advanced and not something you really need to know right now, but simply put, if you add the type of variable you are expecting to get as a parameter, PHP will throw errors and stop execution if any other type is used. This is a good way to ensure our function can do what we are asking of it. For example, if we passed in a non PDO object we should not expect it to be able to use PDO methods, so we could prevent this by explicitly requiring the PDO type.

function getPeople( PDO $conn, $json_id ) {

You'll notice that only $conn has been type hinted. This is because $json_id is a scalar type and is unnecessary to type hint due to PHP's loose typing. If that variable happened to be an int and you needed a string you could just treat it as a string and it would work. And vice-versa. Sometimes this is undesirable and you will have to explicitly check for these types, but you can not do this with typehinting. Instead you would have to use one of the is_*() functions. Replace the wildcard with the type of variable you are looking for (int, string, etc...). You don't really need to understand this right now, but it is a good thing to keep in mind.

Another, much more advanced topic, is OOP. Though I tell this to everyone, get proficient with functions and the concepts I mentioned above before trying to tackle OOP. Once you are fluent with functions, then you should take a look at classes and different OOP strategies.

UPDATE

Sorry for the delay in my answer. I'm typically unavailable over the weekend. This is much better, you still have a minor violation of DRY with that second query(), but overall it is much much better. Let's look at a couple things I missed last time and something new you are doing this time.

Connection

This is something I forgot to mention last time, but you should also abstract all of your connection information so that you can set this up to come from anywhere. Remove the following from your PDO instantiation and replace them with variables.

$dsn = "mysql:host=$host;dbname=$db";
$params = array( PDO::MYSQL_ATTR_INIT_COMMAND => "SET NAMES utf8" );

The benefit of doing it like this is that you can now abstract all of your connection information to a config file or to any other source to allow for easier extension. The config file would be my suggestion for now as it is fairly simple to implement while still being a pretty powerful tool. I wont go into the specifics on how to implement one here, but a quick Google search should yield immediate results. You can also set up a PHP file with constants to serve as a config file.

Arrays

Another thing I missed was your array. First the $rent_types and now the $queries. Always define your array initially before pushing anything onto it.

$queries = array();
$queries[ 'general' ] = //etc...

Of course, then the better way to do what you are trying to do is to just do it all at once. For instance.

$queries = array(
    'general' => /*general SQL*/,
    'owed'    => /etc...
);

The reason we do this is to avoid accidentally manipulating a preexisting array or string. With an array it probably wont be as noticeable, unless you end up needing that array again elsewhere, but the string should become immediately apparent. Strings can be accessed with array syntax to easier allow for specific character accessing. However, I believe trying to do so with an associative key would cause errors. Likewise, trying to later access a string's contents like an array would yield unexpected results, such as a single letter or NULL. Even if you are 100% sure that you aren't going to have any conflicts, just in case you ever need to extend this later, it is always better to explicitly define it. Besides, you are probably getting a warning initially about how that array is undefined. Make sure your error reporting is turned up so that you can catch these kinds of things.

Variable-Variables

${$label}
//or
$$label

What you are doing here is called variable-variables. For the most part variable-variables, or variable-functions, should never be used. Assume they are, 99% of the time, a bad idea. There are exceptions (MVC Controllers rendering variables for Views and Factories dynamically accessing unknown functions are pretty common), but this is not one of them. The reason variable-variables are so disliked is because of how easy they are to miss, and thus debug. There is also a security concern, because the information you are typically converting to variables is from userland and thus untrustworthy.

For instance, imagine this scenario: The very first thing you did was load your config file for your database connection, but, for some reason, you had to load variable-variables into scope before connecting. It could have been an accident, or a malicious user, but now your connection variables have been overwritten and break the system, or point to another database altogether. The problems just become bleaker and can get down right ugly.

So, instead of using variable-variables, or variable-functions, what we are going to want to do is either do something with the information immediately, thus creating a temporary generic variable, or do the same thing you are currently doing, only with an array.

${$label} = //etc...
//becomes
$query_results[ $label ] = //etc...

As you can see, this is almost identical, except we are using a more verbose array instead. There is less security concern and ambiguity here. For instance, now if we have a key that conflicts with a variable in the current scope it wont interact with it in any way. It has been abstracted, or perhaps namespaced, and thus is safer. It also becomes easier to debug. Instead of having variable-variables that could easily hide among the current scope variables, all we have to do is var_dump() the array to peek at its contents. Everything is self-contained.


Implementation of a deque using an array in Python 3 I created this double-ended queue using list. I supported the standard operations such as push, pop, peek for the left side and the right side.

I used an integer mid to track the midpoint of the deque. This way elements can be inserted to the left or to the right appropriately. Here is a list of efficiencies according to Python's documentation for list.

class Deque():
    def __init__(self):
        self.array = []
        self.mid = 0

    def push_left(self, val):
        self.array.insert(self.mid, val)

    def push_right(self, val):
        self.array.insert(self.mid, val)

        self.mid = self.mid + 1

    def pop_left(self):
        # If mid is the rightmost element, move it left.
        if self.mid == len(self.array) - 1:
            self.mid = self.mid - 1

        if (len(self.array) > 0):
            return self.array.pop()
        else:
            return None

    def pop_right(self):
        # If the mid is not the leftmost element, move it right.
        if self.mid > 0:
            self.mid = self.mid - 1

        if (len(self.array) > 0):
            return self.array.pop(0)
        else:
            return None

    def peek_left(self):
        if (len(self.array) > 0):
            return self.array[-1]
        else:
            return None

    def peek_right(self):
        if (len(self.array) > 0):
            return self.array[0]
        else:
            return None

    def __str__(self):
        ret = "["

        if len(self.array) == 0:
            return "[]"

        for idx, val in enumerate(self.array):
            if idx == self.mid:
                ret = ret + "({}) ".format(str(val))
            else:
                ret = ret + str(val) + " "

        return ret.strip() + "]"

# Test case
d = Deque()

d.push_left(19)
d.push_left(11)
d.push_left(23)
print(d)

d.push_right(17)
d.push_right(13)
print(d)

d.pop_left()
d.pop_left()
print(d)

d.pop_left()
d.pop_right()
print(d)

# Edge Case where 13 is the only element. Insert 29 left of it.
d.push_right(29)
print(d)

d.push_left(11)
print(d)

d.pop_right()
d.pop_right()
print(d)

# Edge Case where 13 is the only element. Insert 21 right of it.
d.push_left(21)
d.push_left(29)
print(d)

d.push_right(13)
d.push_right(21)
print(d)  # Symmetry!

I'm aware that the __str__ method isn't particularly efficient as it is O(n).

I specifically want to know:

Are there any edge cases I missed?
Are there any obvious optimisations I missed?
Are there any bugs in the data structure itself?
Is the code properly "Pythonic"?

It's not clear at all to me what the mid pointer represents.  It doesn't seem like it stays in the middle of the array.
Why not use collections.deque?  https://docs.python.org/2/library/collections.html#collections.deque
Implementing an efficient deque is much easier with a linked list than with an array IMO.


One of iteration Conway's Game of Life I would really like to get some feedback on how you would improve my code's style, readability, and efficiency. I failed an interview recently and was unable to get any feedback so I thought I would try asking here. I have cleaned up the code a bit already based on advice from friends but would like to have more critical eyes take a look.

My task was to write a function that takes a matrix of 1's and 0's and returns a resulting matrix after one iteration of Conway's Game of Life.

Note: Since the goal was to only return 1 iteration, I was told to not create cell objects and hold references to neighbors. There is not a need to set this up because we only want to run 1 iteration and doing so would waste space.

I would like advice on what you would change in terms of the function organization and how I decided to break down the problem. Specifically, it feels a little awkward how I have to pass the matrix all the way down to the isInBounds function. Is there a way to avoid this?

This or any other readability/optimization advice would be greatly appreciated.

function oneIterationConway(matrix) {
  const height = matrix.length;
  const width = matrix[0].length;
  const resultMatrix = [];

  for (let row = 0; row < height; row++) {
    resultMatrix[row] = [];
    for (let col = 0; col < width; col++) {
      const neighborCount = countNeighbors(row, col, matrix);
      let isAlive = 0;
      if (matrix[row][col]) {
        // current cell is alive
        isAlive = neighborCount > 1 && neighborCount < 4 ? 1 : 0;
      } else {
        // current cell is dead
        isAlive = neighborCount === 3 ? 1 : 0;
      }

      resultMatrix[row][col] = isAlive;
    }
  }

  return resultMatrix;
}

function countNeighbors(row, col, matrix) {
  let neighbors = 0;

  for (let i = -1; i <= 1; i++) {
    for (let j = -1; j <= 1; j++) {
      if (i === 0 && j === 0) continue;
      if (isInBounds(row + i, col + j, matrix) && matrix[row + i][col + j]) {
        neighbors++;
      }
    }
  }

  return neighbors;
}

function isInBounds(row, col, matrix) {
  const height = matrix.length;
  const width = matrix[0].length;
  return row >= 0 && col >= 0 && row < height && col < width;
}

const testMatrix = [
  [1, 0, 0, 0, 0],
  [0, 1, 1, 0, 0],
  [0, 1, 0, 0, 0]
];

const result = oneIterationConway(testMatrix);

for (let row of result) {
  console.log(row);
}

There are a couple of ways of solving this:

Nest the other functions inside of oneIterationConway

function oneIterationConway(matrix) {
  const height = matrix.length;
  const width = matrix[0].length;
  function isInBounds(row, col, matrix) {
    return row >= 0 && col >= 0 && row < height && col < width;
  }

this way the first definition of height is in scope (available) inside isInBounds. Hiding your 'private' functions is a good idea anyway.
Don't use isInBounds at all, take advantage of the fact that items out of bounds are undefined.

function countNeighbors(row, col, matrix) {
  let neighbors = -matrix[row, col];

  for (let i = -1; i <= 1; i++) {
    for (let j = -1; j <= 1; j++) {
      let value = matrix[row+i, col+j];
      if (value) neighbors++;
    }
  }
  return neighbors;
}

If I am in control of the design, something I often do is add a boundary around my matrix. Something like:

const testMatrix = [
  [0, 0, 0, 0, 0, 0, 0],
  [0, 1, 0, 0, 0, 0, 0],
  [0, 0, 1, 1, 0, 0, 0],
  [0, 0, 1, 0, 0, 0, 0],
  [0, 0, 0, 0, 0, 0, 0],
];

My loops would then be from 1 to size-2

The only other obvious thing I would be would be to factor out part of the main loop:

for (let row = 0; row < height; row++) {
  resultMatrix[row] = [];
  for (let col = 0; col < width; col++) {
    resultMatrix[row][col] = isAlive(row, col);
  }
}

function isAlive(row, col) {
  const neighborCount = countNeighbors(row, col);
  if (matrix[row][col]) {
    // current cell is alive
    return neighborCount > 1 && neighborCount < 4 ? 1 : 0;
  } else {
    // current cell is dead
    return neighborCount === 3 ? 1 : 0;
  }
}

btw, There is a nice feature in most environments. You can use: console.table(results)


Improving Least Recently Used implementation in Java I wrote implementation of LRU in java and want to know if it can be improved further both in space and time complexity.

Input: Number of frames and reference string for pages.

Output: Number of page faults.

Also I am a little skeptical about its correctness because I got 6/10 in my practical lab.

Kindly ignore OOP's aspect of the program.

import java.util.*;
import java.io.*;
class Lru
{
 private static ArrayList<Integer> arl=new ArrayList<Integer>();
 private static BufferedReader br = new BufferedReader(new InputStreamReader(System.in));
 public static void main(String []args)throws IOException
 {
     String [] arr;int n,f,pf=0;
     System.out.println("enter the number of frames");
     n=Integer.parseInt(br.readLine());
     System.out.println("enter the reference string");
     arr = br.readLine().split(" ");
     for(String s:arr)
     {
         f=Integer.parseInt(s);
         if(arl.size()<n)
         { ++pf;arl.add(f);}
         else if(arl.contains(f))
         {
          arl.remove(arl.indexOf(f));
          arl.add(f);
         }
         else { arl.remove(0); arl.add(f);++pf;}
     }
     System.out.println("the total no of page faults are "+pf);
 }
}

Bug
If the input consisted of the same page number over and over, your program would return multiple faults instead of 1, because you made this condition be first:

     if(arl.size()<n)
     { ++pf;arl.add(f);}

You should switch the first and second conditions:
     if(arl.contains(f)) {
         arl.remove(arl.indexOf(f));
         arl.add(f);
     } else if(arl.size()<n) {
         ++pf;
         arl.add(f);
     } else {
         arl.remove(0);
         arl.add(f);
         ++pf;
     }


AES-256-CBC encrypt and decrypt files in Python I wrote a simple algorithm to encrypt and decrypt files in Python using aes-256-cbc.

from Crypto import Random
from Crypto.Cipher import AES
import base64

def pad(s):
    return s + b"\0" * (AES.block_size - len(s) % AES.block_size)

def encrypt(message, key, key_size=256):
    message = pad(message)
    iv = Random.new().read(AES.block_size)
    cipher = AES.new(key, AES.MODE_CBC, iv)
    return iv + cipher.encrypt(message)

def decrypt(ciphertext, key):
    iv = ciphertext[:AES.block_size]
    cipher = AES.new(key, AES.MODE_CBC, iv)
    plaintext = cipher.decrypt(ciphertext[AES.block_size:])
    return plaintext.rstrip(b"\0")

def encrypt_file(file_name, key):
    with open(file_name, 'rb') as fo:
        plaintext = fo.read()
    plaintext = base64.b64encode(plaintext)
    print(plaintext)
    enc = encrypt(plaintext, key)
    with open("file.txt" + ".enc", 'wb') as fo:
        fo.write(enc)

def decrypt_file(file_name, key):
    with open(file_name, 'rb') as fo:
        ciphertext = fo.read()
    dec = decrypt(ciphertext, key)
    dec = base64.b64decode(dec)
    with open(file_name[:-4]+ ".dec", 'wb') as fo:
        fo.write(dec)
    return dec.decode()

key = b'1234sfsfdsafasdf'

encrypt_file('file.txt', key)
decrypt_file('file.txt.enc', key)

However, I am definitely not a cryptography expert and I suppose my code may be missing something.

Is it safe to use this code for encryption of my files and what could be improved in this algorithm?

Also what do you think about This Implementation, is it secure and could be used in production?

def pad(s):
    return s + b"\0" * (AES.block_size - len(s) % AES.block_size)

Generally a good library should have you covered when it comes to padding and unpadding. Prefer the Python 3 bytearray types rather than the stringified b"\0" trick.

Worse, you're implementing Zero padding, which would strip off zero valued bytes from your text files. That may not harm text files much (well, unless UTF-16LE I suppose), but for binary files that's very very dangerous.

Everybody tends to use PKCS#7 compatible padding for CBC mode.

def encrypt(message, key, key_size=256):

The key size is usually required for C-style languages but not here. You're already giving it a key, and the key has a certain size. The only thing you can do is to test the key against the key size if you're (overly) afraid that you might pass a 128 bit / 16 byte key by accident. But in general you should not pass parameters that can be derived from other parameters anyway.

    message = pad(message)

This creates a copy of the message. Now that's all right for small messages, but for files over 4 GiB it becomes "a bit of a nuisance". Especially if you decide to use your logical cores to encrypt many files. In theory AES/CBC pads the messages, but in practice we only pad the last 16 byte block that we need to encrypt.

    iv = Random.new().read(AES.block_size)

It's not all gloom and doom, you used a cryptographic random number generator to generate an unpredictable IV and you used AES.block_size instead of 16. And then prefixed it to the ciphertext. Well done!

    return plaintext.rstrip(b"\0")

And there's the Zero padding problem being executed. If you have a file consisting of all zero's, it would empty entirely. Oops!

    plaintext = base64.b64encode(plaintext)

Ah yes, that saves us from the padding problem, as base 64 doesn't contain zero valued bytes. The disadvantage is of course that your ciphertext will now expand 33%, and that you create another copy in memory for that size. Sure, the raw plaintext can now be freed up, but your system will still not be happy with you.

    with open("file.txt" + ".enc", 'wb') as fo:

The "file.txt" name is a bit out of wag with the 'wb' flag, don't you thing? And besides, although you base 64 encoded your plaintext, you didn't do it with the ciphertext. And you should not need to, just keeping everything in binary is fine for files.

Besides that, you probably should do something with a filename parameter. At least create a note to fix things later when you're experimenting.

    with open(file_name[:-4]+ ".dec", 'wb') as fo:

Ah, we just replace the last 4 characters now, without checking if the filename even has an extension?

    return dec.decode()

What, why? And what are we decoding?

key = b'1234sfsfdsafasdf'

Strings are not keys. If you want to create a test key, create one by decoding hexadecimal encoded bytes. And those look like 16 characters which means AES-128 rather than 256. If you want to use a password, you need a password based key derivation function (to perform so called Password Based Encryption, see the PKCS#5 standard).

CBC mode is fine for keeping files confidential, but generally cryptographers prefer authenticated modes such as GCM or EAX nowadays. Your files are likely kept confidential, but there is no message integrity or authenticity implemented for the files. Note that CBC mode is entirely unsuitable to send unauthenticated messages over a network.

Files can be large, so generally we try and use an API that provides update and final methods or a streaming API (not to be confused with the update method specified for pycryptodome's AEAD ciphers). That way you can encrypt files piecemeal.

Quite often for files it makes sense to use a specific container format such as CMS or PGP. In that case most of your errors are avoided (although, truth to be said, many container formats are starting to show their age).


A double search function that looks in a list for one value and then the other alternatively I'm writing in Python (3.4) a double search function that looks in a list for one value and then the other alternatively. That is, start searching the second value once you find first the first value (and start searching from where you found the first value) and vice-versa.

My first approach was to write a recursive function to do it, only to find out they don't end up as neat in Python as they do in Haskell. This is a basic sketch of the function:

def double_search(input_list, first_condition, second_condition, condition_to_search, output_list):
    if input_list == []:
        return output_list
    else:
        if condition_to_search == 1:
            if input_list[0] == first_condition:
                return output_list + double_search(input_list[1:], first_condition, second_condition, 2, output_list + ['First'])
            else:
                return output_list + double_search(input_list[1:], first_condition, second_condition, 1, output_list + ['None'])
        if condition_to_search == 2:
            if input_list[0] == second_condition:
                return output_list + double_search(input_list[1:], first_condition, second_condition, 1, output_list + ['Second'])
            else:
                return output_list + double_search(input_list[1:], first_condition, second_condition, 2, output_list + ['None'])

I have several problems with the way it turned out, looks forced and ugly in Python. And produces different outputs in Python interpreter and pydev debugger!

Pydev debugger gives the proper result while in the interpreter I have to take the tail of the result:

result[-len(input_list):]

To get a list of the appropriate length, which I find unacceptable because I have no clue where the garbage in front comes from.

Would appreciate any suggestion to improve this code, make it more pythonic and easier to maintain as well as any performance improvements you can suggest.

EDIT: As per request, the following is an example of the function in use with some sample data.

example_list = [5,4,1,2,0,3,1,3,4,2,5,0]
double_search(example_list, 1, 3, 1, [])

Should result in: ['None', 'None', 'First', 'None', 'None', 'Second', 'First', 'Second', 'None', 'None', 'None', 'None']

While:

double_search(example_list, 1, 5, 1, [])

Results in: ['None', 'None', 'First', 'None', 'None', 'None', 'None', 'None', 'None', 'None', 'Second', 'None']

Notice how in the second case only matches the second to last '5', because it needs to find first a '1'.

Python has this wonderful module called itertools. Whenever you need to iterate over some data in a specific way, check if there isn't a function or recipe that could help you.

For this problem, I’m thinking about itertools.cycle: being an iterator, you can easily call next on it whenever you need to grab the "other" value. e.g.:

input_list = [1, 2, 3, 2, 4, 2, 3]
values = itertools.cycle(('First', 'Second'))
for element input_list:
    if element == 2:
        print(next(values))
    else:
        print('None')

will print:

None
First
None
Second
None
First
None

So you can easily compare elements to the current condition.

The only thing missing, then, is associating the value to store in the output list to the condition to check. You have two choices: either you use zip with a second itertools.cycle, or you use a single itertools.cycle but you define it with a couple of couples like so: itertools.cycle(((cond1, 'First'), (cond2, 'Second'))). I’ll go with the former as I feel it is cleaner, but both are equally valid:

import itertools

def double_search(iterable, condition1, condition2):
    output = []
    conditions = itertools.cycle((condition1, condition2))
    results = itertools.cycle(('First', 'Second'))
    iterator = zip(conditions, results)

    # Initialize the current value to search for
    condition, value = next(iterator)

    for element in iterable:
        if element == condition:
            output.append(value)
            condition, value = next(iterator)
        else:
            output.append('None')
    return output

It is however considered bad practice to use the pattern empty list + append in a loop. But the use of next makes it hard to use a list-comprehension instead.

You can, however, turn your function into a generator instead

import itertools

def double_search(iterable, condition1, condition2):
    conditions = itertools.cycle((condition1, condition2))
    results = itertools.cycle(('First', 'Second'))
    iterator = zip(conditions, results)

    # Initialize the current value to search for
    condition, value = next(iterator)

    for element in iterable:
        if element == condition:
            yield value
            condition, value = next(iterator)
        else:
            yield 'None'

and call it like:

input_list = list(range(5))
result = list(double_search(input_list, 2, 3))


Rails partial inheritance hack I have a STI table and want to be able to render a collection using

render @collection

with every elements of the collection using their own partial or, if they don't have one, the parent partial. You know, partial inheritance.

So I added to my parent model :

def to_partial_path
  self.class._to_partial_path
end

protected

# a little hack to get partial inheritance in views, check if the partial exist, if not
# return the parent's one. Provide class level cache, be sure to restart your app when
# adding/removing partials.
def self._to_partial_path
  @_to_partial_path ||= begin
    element = ActiveSupport::Inflector.underscore(ActiveSupport::Inflector.demodulize(self))
    collection = ActiveSupport::Inflector.tableize(self)
    if File.exists?(Rails.root.join('app', 'views', collection, "_#{element}.html.erb")) ||
    self.superclass == ActiveRecord::Base

      "#{collection}/#{element}"
    else
      self.superclass._to_partial_path
    end
  end
end

The original code is here.

This seems to work great so far but I want to have some opinions.

Should I have gone another way?

Are there any "gotchas" I might encounter doing it this way?

Any way to refactor this?

edit: No problem whatsoever, it's working great for now. I'm looking for a way to achieve something similar with form (ie render form in new and edit views render the child _form partial if their is one, the parent one otherwise).

This actually looks pretty solid.

Another approach would be to sub-class the Renderer and make it responsible for retrieving the base classes' to_partial_path.

For me this feels a bit cleaner because it is sub-classing versus monkey-patching and because the renderer is supposed to find and load the template anyway while the model class shouldn't case about the (non-)existence of the view at all.

After looking at it for a while, I would the make sub-classes partial_path check if the template exists and find the template of the base class if it doesn't.


Script for initialising i18n for a Flask site using pybabel and goslate I haven't thought of a nice way to remove #, fuzzy from the files. I think process_babel_file might possibly become its own module, but on the other hand, having all methods in one script is quite nice for the mean time. I expect there to be bugs, although it works for the languages I've tried.

#!/usr/bin/env python
# -*- coding: utf-8 -*-
"""
setuptranslations.py

Generate and manipulate translation files for a flask app.
Assumes areas in jinja2 templates are marked with {% trans %}..{% endtrans %}
or gettext, _, or other gettext variants.
Related: flask, flask-babel, pybabel, jinja2, i18n, gobabslate
    1. Run pybabel with extract option
        pybabel extract -F babel.cfg -o messages.pot .
    2. Run pybabel with init option for each language
        pybabel init -i messages.pot -d translations -l fr
        pybabel init -i messages.pot -d translations -l de
    3. Pull translations from Google Translate API into babel files
    4. Remove #fuzzy from message.po files.
    5. Run pybabel with compile option
        pybabel compile -d translations
"""
import sys
import codecs
from subprocess import Popen
from goslate import Goslate

pybabel_extract = ['pybabel', 'extract', '-F' 'babel.cfg',
                                         '-o' 'messages.pot', '.']
pybabel_init_lang = ['pybabel', 'init', '-i', 'messages.pot',
                                        '-d', 'translations', '-l']
pybabel_compile = ['pybabel', 'compile', '-d', 'translations']

def get_language_codes():
    try:
        from config import LANGUAGES, LANGUAGE_DEFAULT
        return [lang for lang in LANGUAGES.keys() if lang != LANGUAGE_DEFAULT]
    except:
        return ['fr', 'de']

def return_code(args):
    return Popen(args).communicate()[0]

def process_babel_file(lang, remove_fuzzy=False):
    gs = Goslate()
    filename = 'translations/' + lang + '/LC_MESSAGES/messages.po'
    file_stream = codecs.open(filename,          'r', 'utf-8')
    file_output = codecs.open(filename + '.tmp', 'w', 'utf-8')
    finding_input, data = False, u''
    for line in file_stream:
        if finding_input is True and line == u'msgstr ""\n':
            finding_input = False
            new_text = gs.translate(data, lang).replace('"', '\\"')
            file_output.write(u'msgstr "' + new_text + u'"\n')
            continue
        elif finding_input is True:
            data += line.replace('\n', ' ').replace('\\n"', ' ').strip('"')\
                        .replace('\\"', '"')
        elif line == u'msgid ""\n' or line[:7] == u'msgid "':
            finding_input = True
            data = u'' if line == u'msgid ""\n' else line[7:].strip()\
                .strip('"').replace('\\"', '"')
        elif line == '#, fuzzy\n' and remove_fuzzy:
            continue
        file_output.write(line)
    file_stream.close()
    file_output.close()
    Popen(['cp', filename + '.tmp', filename]).communicate()

def main():
    if return_code(pybabel_extract) is not None:
        sys.exit('pybabel failed to extract translations')
    for language_code in get_language_codes():
        if return_code(pybabel_init_lang + [language_code]) is not None:
            print('Failed to add language: %s', language_code)
            continue
        process_babel_file(language_code, remove_fuzzy=True)
    if return_code(pybabel_compile) is not None:
        sys.exit('pybabel was unable to compile translations')

if __name__ == '__main__':
    main()

Nice script! Found it very helpful to get started and test flask babel and see first translations.

A few "bugs" I discovered for your consideration

Any string with a ' will cause problems
Same with other special characters, e.g. ()

The resulting po file mixes entries later on and generates strings like:

#: dedri/templates/contact.html:29
msgid "Your email"
msgstr "Aboutmsgstr \"about (modificado por Raimundo)\" #: dedri / templates / about.html:            13 msgid \"Aquí añadimos unas pocas palabras acerca de cada uno de nosotros.\" msgstr \"Aquí añadimos Unas Palabras Sobre CADA UNO de Nosotros here.\" #: Dedri / templates / contact.html: 29 msgid \"Your email\""


Image similarity measure I'm attempting to speed up my genetic algorithm that modifies images. After searching, I've found that my fitness function is the bottleneck, taking sometimes up to 11 seconds to complete.

The fitness function accepts two images and returns a float representing the total difference between the images' corresponding pixels, measured as distance in Cartesian RGB space.

To test, I used some large images of the same size (1254 x 834). What can I do to speed this process up?

def fitness(original, new):
    fitness = 0

    for x in range(0, width):
        for y in range(0, height):
            r1, g1, b1 = original.getpixel((x, y))
            r2, g2, b2 = new.getpixel((x, y))

            deltaRed = abs(r1 - r2)
            deltaGreen = abs(g1 - g2)
            deltaBlue = abs(b1 - b2)

            pixelFitness = pixelFitness = math.sqrt(deltaRed ** 2 + deltaGreen ** 2 + deltaBlue ** 2)

            fitness += pixelFitness

    return fitness

Thanks to @wondra for the help. I rewrote the function without the getpixel and just loaded the pixels using the image.load() method. This caused the execution time to reduce down to 3.09 seconds.

Rewritten function:

def fitness_new(new):
    fitness = 0

    new_data = new.load()

    for x in range(0, width):
        for y in range(0, height):
            r1, g1, b1 = optimal_data[x, y]
            r2, g2, b2 = new_data[x, y]

            deltaRed = abs(r1 - r2)
            deltaGreen = abs(g1 - g2)
            deltaBlue = abs(b1 - b2)

            pixelFitness = pixelFitness = math.sqrt(deltaRed ** 2 + deltaGreen ** 2 + deltaBlue ** 2)

            fitness += pixelFitness

    return fitness

optimal_data is preloaded as a global variable instead of being passed as an argument, because it is accessed elsewhere. I know this is probably not the most ideal method.


Get the highest role of a user across all the organizations he is member of I have a function that looks like this:

def getMaxUserLevelByUserId(userId: Muid)(
      implicit connection: Connection): Option[OrganizationMembership.Level.Value] = {
    val organizationMemberships = getAllByUserId(userId)

    if (organizationMemberships.exists(_.level == OrganizationMembership.Level.Admin)) {
      Some(OrganizationMembership.Level.Admin)
    } else if (organizationMemberships.exists(_.level == OrganizationMembership.Level.Member)) {
      Some(OrganizationMembership.Level.Member)
    } else if (organizationMemberships.exists(_.level == OrganizationMembership.Level.Guest)) {
      Some(OrganizationMembership.Level.Guest)
    } else {
      None
    }
  }

The gets the highest role of a user across all the organizations he is member of. As you can see, the function is pretty straightforward and works. However, it has a complexity of O(n3). I was wondering if there is a way to get it to O(n) or at least a bit lower?

Well, this is what I did. Not sure if that's the best way to go.

def getMaxUserLevelByUserId(userId: Muid)(
      implicit connection: Connection): Option[OrganizationMembership.Level.Value] = {
    val organizationMemberships = getAllByUserId(userId)

    if (organizationMemberships.nonEmpty) {

      val levelMap = Map(
          OrganizationMembership.Level.Guest -> 0,
          OrganizationMembership.Level.Member -> 1,
          OrganizationMembership.Level.Admin -> 2
      )

      val maxLevel = organizationMemberships.foldLeft(OrganizationMembership.Level.Guest) {
        (level, organizationMembership) =>
          if (levelMap(organizationMembership.level) > levelMap(level)) {
            organizationMembership.level
          } else {
            level
          }
      }

      Some(maxLevel)

    } else {

      None

    }
  }

The idea behind the code above, is the fact that we are iterating through the collection only once, regardless of the collection size.
The levelMap was created strictly for the reason of easier comparison.

The cool thing about foldLeft in this case is that we don't need to create mutable variables while iterating through the collection and it is actually very short and quite readable code.


Priority in pair inside priority_queue on basis of both elements I want a priority queue such that first it should be done on basis of first element (increasing order)and when clash occurs then on basis of second element(decreasing order). I came up with the following code :

#include<bits/stdc++.h>
using namespace std;

struct Compare {
    constexpr bool operator()(pair<int, int> const & a,
                              pair<int, int> const & b) const noexcept
    { return a.first > b.first && a.second<b.second; }
};

int main()
{
    priority_queue<pair<int,int>,
                   std::vector<pair<int,int> >,
                   Compare> Q;
    int n;
    cin>>n;

    for(int i=0;i<n;i++)
    {
        ll a,b;
        cin>>a>>b;
        Q.push(make_pair(a,b));
    }

    for(ll i=0;i<n;i++)
    {
        pair<ll,ll> m;
        m=Q.top();
        cout<<m.first<<" "<<m.second<<"\n";
        Q.pop();
    }
}

For eg - If I have following pairs : (1,10),(1,7),(1,5),(2,8),(2,6),(3,5),(3,9)

My output should be as follows on calling Q.top() and Q.pop()

1 10
1 7
1 5
2 6
2 8
3 5
3 9

But instead I'm getting the following output:

1 10
1 5
3 9
1 7
2 8
2 6
3 5

Can someone help me out ?

This test is wrong:

return a.first > b.first && a.second<b.second;

Try:

return a.first > b.first || (a.first == b.first && a.second < b.second)

Some other things

This is not a legal header

#include<bits/stdc++.h>

Remove this immediately your code is invalid with it there.

Never do this:

using namespace std;

Its a bad habit that will cause you problems in real code. Always pre-fix things from the standard library with std:: (Its short for a reason).

see: https://stackoverflow.com/q/1452721/14065

Use menaingful variable names.

Names like i n m have no meaning. Also when you have longer code searching the source for them will turn up a lot of false negatives thus making it harder to keep your code working.

Prefer prefix ++ over the postfix version.

    for(int i=0;i<n;i++)

    for(int loop = 0; loop < inputElementCount; ++loop)

A bit of white space makes things easier to read.

        cin>>a>>b;

        // Why not
        std::cin >> value1 >> value2;

When poping from a queue you should check its not empty not assume it has a particular size:

for(;!queue.empty(); queue.pop())
{
    auto const& item = queue.top();
    std::cout << item.first << " " << item.second << "\n";
}


JavaScript function to change an element tag I wrote this JavaScript function to change an element tag name to a div.

function divilize(elem) {
    var elemDiv = document.createElement("div");
    var elemAttrs = elem.attributes;
    for (var attr = 0; attr < elemAttrs.length; attr++)
        elemDiv.setAttribute(elemAttrs[attr].name, elemAttrs[attr].value);
    elemDiv.className += 'divilize-'+elem.tagName.toLowerCase();
    while (elem.firstChild)
        elemDiv.appendChild(elem.firstChild);
    elem.parentNode.replaceChild(elemDiv,elem)
}

divilize(document.getElementById('one'));

Basically, it receives an element, creates a div, copies the attributes, adds in a class with the original tag name for styling purposes (I can make a .divilize-span {display: inline;}), and copies over all of the child elements. I designed with fast reusable code in mind and to avoid losing any event handlers.

Let me know your thoughts...

You will have a css concatenation issue if the passed element already has a class, for example :

will turn into :

Then on the actual efficiency of the function, I would use standard calls and quicker logic to speed things up :

function divilize(elm) {

   var clone = elm.cloneNode(true);
   var tagName = clone.tagName.toLowerCase();

   clone.className += " divilize-"+tagName;
   elm.outerHTML = clone.outerHTML.replace(tagName, "div");

}

@ Fiddle


ASP.NET account login system implemented via Masterpage and Panels I have been tasked with implementing a secure account login system for a website's content management system. I have built a solution around an access masterpage that handles all account access functions. Here is the Page_Load

Protected Sub Page_Load() Handles Me.Load
    If Not Request.IsSecureConnection Then
        Response.Redirect(Request.Url.ToString.Replace("http://", "https://"))
    End If
    UpdateSecurity()
End Sub

And the definition of UpdateSecurity()

Private Sub UpdateSecurity()
    PageContentPanel.Visible = Session("access")
    PleaseLogInPanel.Visible = Not Session("access")
End Sub

As you can probably infer from the snippet, having "access" set in the Session makes an asp:Panel containing the content of whatever child page the user requested visible, otherwise, the PageContentPanel is hidden, and the PleaseLogInPanel with a username and password prompt is made visible. In the handler for the login button (also on the login Panel), the posted credentials are verified and if correct, Session values are added.

I was wondering if this implementation is fundamentally sound or not. Page requests from not logged in users still execute (I think), but the results are hidden inside an invisible asp:Panel. I'm not familiar enough with the ASP.NET security model to know if this poses risks or not from wayward POSTs. If a child page utilizes this masterpage, what access do not logged in users have to resources on the page?

What you are doing right now is not fundamentally correct. This link http://learn.iis.net/page.aspx/170/developing-a-module-using-net/ explains developing a custom authentication module.

Also you should read about page life cycle while working asp webforms. http://msdn.microsoft.com/en-us/library/ms178472.aspx


Combine lists of objects with no duplicates I have 2 groovy sql resultset, I need to combine the result set so that project_no should be unique and case_no can have multiple elements if there is a duplicate project_no

Below are the 2 groovy sql resultset

[[project_no:0-10001,case_no:00492268],[project_no:0-10160,case_no:01957580],[project_no:1-10014,case_no:02022686]]

[[project_no:0-10160,case_no:01957590],[project_no:1-10014,case_no:019126],[project_no:1-2896337,case_no:02039596]]

Desired List

[[project_no:0-10001,case_nos:[00492268]], [project_no:0-10160,case_nos:[01957580,01957590]] ,[project_no:1-10014,case_nos:[02022686,019126]], [project_no:1-2896337,case_nos:[02039596]]]

This is what I have tried

caseResultForAnalysis.each { ca ->
    def ptmp = [:], caseList = []
    tempPrList.add(ca["project_no"])
    ptmp["project_no"] = ca["project_no"]
    caseList.add(ca["case_no"])
    if (caseList.size() > 0) {
        ptmp["case_nos"] = caseList
        mergedCaseResult.push(ptmp)
    }
}
mergedCaseResult.each { ma ->
    def ptmp = [:], caseList = []
    caseResultForUploads.each { cp ->
        if (!tempPrList.contains(cp["project_no"])) {
            ptmp["project_no"] = ma["project_no"]
            caseList.add(cp["case_no"])
        } else if (ma["project_no"] == cp["project_no"]) {
            //if (!ma["case_nos"].contains(cp["case_no"]))
            List tmp = ma["case_nos"]
            if (!tmp.contains(cp["case_no"]))
                ma["case_nos"].add(cp["case_no"])
        }
    }
    if (caseList.size() > 0) {
        ptmp["case_nos"] = caseList
        mergedCaseResult.push(ptmp)
    }
}

//1st list caseResultForAnalysis
//2nd List caseResultForUploads
//desired List mergedCaseResult

Is there a better way to do this, for better readability and less resource consumption?

Formulate your SQL query such that you get exactly the combined results you want. Don't use Groovy to do work that the database should be doing for you.  It will be faster and easier.


Sort a stack in descending order Maintain current element as the top element in the input stack. Pop all the elements on the ordered stack which are greater than the current element and push them in to the input stack and maintain a count of these elements. Now push the current element into the ordered stack and pop the count number of elements from the input stack and push them into the ordered stack and return the ordered stack.

public class sortStackInAscending {

    public static void main(String args[]) {
        Stack<Integer> inputStack = new Stack<>();
        int size = 1000;
        Random ran = new Random();
        while (size > 0) {
            inputStack.push(ran.nextInt(size));
            size--;
        }
        Stack<Integer> orderedStack = sortStack(inputStack);
        printStack(orderedStack);
    }

    public static void printStack(Stack<Integer> anyStack) {
        int size = anyStack.size();
        Stack<Integer> tempStack = new Stack<>();
        int temp;
        while (size > 0) {
            temp = anyStack.pop();
            System.out.print(temp + " ");
            tempStack.push(temp);
            size--;
        }
        System.out.println();
        anyStack = tempStack;

    }

    public static Stack<Integer> sortStack(Stack<Integer> inputStack) {
        Stack<Integer> orderedStack = new Stack<>();
        int count = 0;
        int currentElement;
        while (!inputStack.isEmpty()) {
            currentElement = inputStack.pop();
            // pop elements > current element from ordered stack and
            // push them into input stack and maintain the count
            while (!orderedStack.isEmpty()
                    && currentElement < (int) orderedStack.peek()) {
                inputStack.push(orderedStack.pop());
                count++;
            }
            // insert the current element into its correct position in ordered
            // stack
            orderedStack.push(new Integer(currentElement));
            // pop count number of elements from input stack and push them into
            // ordered stack
            while (count > 0) {
                orderedStack.push(inputStack.pop());
                count--;
            }
        }
        return orderedStack;
    }
}

1 Code

In my opinion, your code would be a little more readable if you had a blank line before and after such constructs as while, for and if. So, instead of

Random ran = new Random();
while (size > 0) {
    inputStack.push(ran.nextInt(size));
    size--;
}
Stack<Integer> orderedStack = sortStack(inputStack);

you could have

Random ran = new Random();

while (size > 0) {
    inputStack.push(ran.nextInt(size));
    size--;
}

Stack<Integer> orderedStack = sortStack(inputStack);

2 printStack

Collections.<Integer>reverse(inputStack);
System.out.println(inputStack);

does the job with funky brackets and commas.

3 Naming

sortStackInAscending is not a very good name. Perhaps StackSorter?

4 Sorting

Usually, sorting routines do not return the sorted result, but rather modify its argument collection. That's the way most production sorts work in practice.

Summa summarum

A sub-point: making your stack sort generic is not hard; see below.

All in all, I had this in mind:

public class StackSorter {

    public static void main(String args[]) {
        Stack<Integer> inputStack = new Stack<>();
        int size = 10;
        Random ran = new Random();

        while (size > 0) {
            inputStack.push(ran.nextInt(size));
            size--;
        }

        sortStack(inputStack);

        System.out.println("Stack is sorted: " + stackIsSorted(inputStack));
        Collections.<Integer>reverse(inputStack);
        System.out.println(inputStack);
    }

    public static <E extends Comparable<? super E>>
        boolean stackIsSorted(final Stack<E> stack) {
        if (stack.isEmpty()) {
            // Trivially sorted.
            return true;
        }

        final Stack<E> workStack = new Stack<>();
        workStack.addAll(stack);

        E previousElement = workStack.pop();

        while (!workStack.isEmpty()) {
            final E currentElement = workStack.pop();

            if (currentElement.compareTo(previousElement) > 0) {
                return false;
            }

            previousElement = currentElement;
        }

        return true;
    }

    public static <E extends Comparable<? super E>>
        void sortStack(final Stack<E> inputStack) {
        final Stack<E> orderedStack = new Stack<>();
        E currentElement;
        int count = 0;

        while (!inputStack.isEmpty()) {
            currentElement = inputStack.pop();

            while (!orderedStack.isEmpty()
                    && currentElement.compareTo(orderedStack.peek()) < 0) {
                inputStack.push(orderedStack.pop());
                count++;
            }

            orderedStack.push(currentElement);

            while (count > 0) {
                orderedStack.push(inputStack.pop());
                count--;
            }
        }

        inputStack.addAll(orderedStack);
    }
}

Hope that helps.


Functions to hide and reclaim first visible publication on a page using Selenium I have the following functions in JavaScript:

function SelectFirstVisiblePublicationAndHideIt ( PageObj )
{
    var hideButtonXpath = "//img[@title='To hide this item in your profile, mark it as invisible']";
    var hideButtonArray;
    var numberOfHideButton;
    var publicationTitle;

    aqUtils.Delay(500);
    hideButtonArray = PageObj.EvaluateXpath(hideButtonXpath);
    numberOfHideButton = hideButtonArray.length;
    hideButtonArray[0].Click();
    publicationTitle = hideButtonArray[0].parent.parent.FindChildByXPath("//a").innerText;

    return publicationTitle;
}

function SelectFirstVisiblePublicationAndReclaimIt ( PageObj )
{
    var reclaimButtonXpath = "//button[@title='Claim this publication and stay on this page']";
    var reclaimButtonArray;
    var numberOfReclaimButton;
    var publicationTitle;

    aqUtils.Delay(500);
    reclaimButtonArray = PageObj.EvaluateXpath(reclaimButtonXpath);
    numberOfReclaimButton = reclaimButtonArray.length;
    reclaimButtonArray[0].Click();
    publicationTitle = reclaimButtonArray[0].parent.parent.FindChildByXPath("//a").innerText;

    return publicationTitle;
}

SelectFirstVisiblePublicationAndHideIt and SelectFirstVisiblePublicationAndReclaimIt are very similar except for the Xpath expression.

I am wondering if I should merge them into one function by:

re-naming variables and functions names in a more generic way
introduce an additional input argument that serves as a flag to indicate which Xpath expression is to be used

Doing so has pros and cons:

Pros: reduce the number of function by one
Cons: purpose of a function can no longer be read from its name; having one more input argument; become further away from the principle of "one function should do one thing and one thing only"

Any suggestions?

As the code is similar, they can be merged together.

I'll suggest you to break down the functions into smaller reusable chunks which will follow the Single Responsibility Principle (SRP). These functions will be testable and reusable.

Both functions are

Adding a delay of 500 milliseconds
Selecting elements from provided page
Clicking the first element from the collection
Finding an element from its ancestor and returning the innerText of it.

As these are the steps, we can similarly divide those functions into smaller functions each doing the task of one step above.

For first step (to add delay) I've not created a new function as I guess that is already created in utils module.

The first function (Step #2) will select the elements from the page using the provided xpath and return the first of them.

function getFirstVisiblePublication(page, xpath) {
    aqUtils.Delay(500);

    var elements = page.EvaluateXpath(xpath);
    return elements[0];
}

The second function combines Steps #3 & #4; as #3 only requires to click the element I haven't created new function.

The below function will accept the page in which the target element is to be searched and xpath of the element. This will call the above function to get the first element and click it.

This function will return the text/label of the anchor element which is obtained by using the first element's ancestors.

function getFirstVisiblePublicationAnchorLabel(page, xpath) {
    // Get first element
    var firstElement = getFirstVisiblePublication(page, xpath);
    firstElement.Click();

    return firstElement.parent.parent.FindChildByXPath('//a').textContent;
}

Usage:

Function can be called as follows:

var hideButtonXpath = "//img[@title='To hide this item in your profile, mark it as invisible']";
var publicationTitle = getFirstVisiblePublicationAnchorLabel(PageObj, hideButtonXpath);


ConcurrentDictionary, Store Key and Compression So i have a  ConcurrentDictionary<int, string>

The "int" is merely the way i store the key, which i want more recommendations on.

Currently i do like this.

        public string memoize(Func<string, uint, string> functor, string code, uint id)
    {
        //Used for the lookup to identify if it's been inputed before
        int codeHash = code.GetHashCode();
        string functor_return;

        if (_compilerCache.TryGetValue(codeHash, out functor_return))
            return functor_return;

        functor_return = functor?.Invoke(code, id);
        _compilerCache[codeHash] = functor_return;

        return functor_return;
    }

Why do i use GetHashCode()?
I don't know, it's just the only way i could figure out rather than store the string value itself.

I also save this "Cache", and hence why the compression and key is important.

    //Serialize and Compress object to file
    public static void SerializeObject<T>(string filename, T obj)
    {
        using (Stream stream = File.Open(filename, FileMode.Create))
        using (var cStream = new GZipStream(stream, CompressionLevel.Optimal))
        {
            BinaryFormatter binaryFormatter = new BinaryFormatter();
            binaryFormatter.Serialize(cStream, obj);
        }
    }

    //DeSerialize and Decompress object from file
    public static T DeSerializeObject<T>(string filename)
    {
        T objectToBeDeSerialized = default(T);
        try
        {
            using (Stream stream = File.Open(filename, FileMode.Open))
            using (var cStream = new GZipStream(stream, CompressionMode.Decompress))
            {
                BinaryFormatter binaryFormatter = new BinaryFormatter();
                objectToBeDeSerialized = (T)binaryFormatter.Deserialize(cStream);
            }

        }
        catch (Exception e)
        {
            MessageBox.Show(e.Message + " : Error DeSerializing Cache, corrupted?");
        }

        return objectToBeDeSerialized;
    }

It all works, but i would like to improve it.
I don't really know if even GetHashCode is a good way to do this thing,
i just took it randomly as before i just stored the value itself.

The values are usually around 10-20 characters i guess, usually very similar (which should be compression friendly i guess).

So can anything be done to improve this?

GetHashCode is used internally by ConcurrentDictionary to speed up access to the real key.

But, there is no guarantee that 2 different strings will return 2 different HashCode, you may encounter collisions! Such collisions are handled by the ConcurrentDictionary.

So with your code, as the _compilerCache is a Dictionary<int,functor> (it seems to be) and not a Dictionary<string,functor>, you may encounter rare events where two different codes will give the same key. If such a collision happends, you have a subtle and rare bug.

I suggest the following :

stick to a Dictionary<string,functor>, use the real key
don't input a HashCode instead of the real key
you may also input your own GetHashCode in Dictionary constructor

Using code instead of int as a key has a price : memory footprint is larger, but with small strings (10-20 char), it should not be an issue.

Hope this helps !


Merging two hashes with the mean values of each key if both hashes exist There are two methods, hash1 and hash2, that return hashes or nil. The hashes have the same keys. I want to merge them with their mean value only if both methods have hashes.

(I added Array#mean to calculate the average value of an array).

def merged_hashes
    return if hash1.nil? && hash2.nil?
    return hash2 if hash1.nil?
    return hash1 if hash2.nil?
    hash1.merge(hash2) do |key, v1, v2|
      [v1, v2].compact.mean
    end
end

Is there more succinct way to write this?

There are many ways to do this, but all have two common elements. Firstly, since hash1 and hash2 are methods, you want to only call those methods once, which means saving the return values in variables:

h1 = hash1
h2 = hash2

Secondly, there are four possible courses of action, depending on the values of h1 and h2. You may wish to represent these with a case statement:

def merged_hashes(hash1,hash2)
  h1 = hash1
  h2 = hash2
  case [h1.is_a?(Hash), h2.is_a?(Hash)]
  when [false, false] then nil
  when [false, true]  then h2
  when [true, false]  then h1
  else compute_avg(h1, h2)
  end
end

Note that it is necessary to compute both h1 and h2 even if the first one computed is found to not be a hash.

All that remains is to construct the method compute_avg. Here are three ways that might be done:

Merge h1 and h2, using the form of Hash#merge that employs a block to determine the values of keys that are present in both hashes being merged, which here is all keys.

compute_avg(h1,h2)
  h1.merge(h2) { |_, v1, v2| [v1, v2].compact.mean }
end

You used merge in this way in your solution.

Extract values of h2 and send Enumerable#each_with_object to h1 to build the desired hash

compute_avg(h1,h2)
  values2 = h2.values
  h1.each_with_object({}) { |(k,v1),h| h[k] = [v1, values2.shift].compact.mean }
end

Extract the values of h1 and h2, compute their averages, element-by-element, then construct the hash to be returned

compute_avg(h1,h2)
  keys = h1.keys
  averages = { h1.values.zip(h2.values).map { |x1,x2| [x1, x2].compact.mean  } }
  keys.zip(averages).to_h
end


Best practice for asserting a positive IF condition What's better - to leave the IF block blank to do nothing, or to add a statement that basically does nothing (each link already has a href attribute)?

export function fixRelativeLinksOutsideOfEpub(dom): void {
    const content = dom.window.document.querySelectorAll('a');
    content.forEach((element: HTMLElement) => {
        const href = element.getAttribute('href');
        if (href.indexOf('#') === 0) {
            // do nothing
        } else {
            element.setAttribute('target', '_blank');
        }
    });
}

export function fixRelativeLinksOutsideOfEpub(dom): void {
    const content = dom.window.document.querySelectorAll('a');
    content.forEach((element: HTMLElement) => {
        const href = element.getAttribute('href');
        if (href.indexOf('#') === 0) {
            element.setAttribute('href', `${href}`);
        } else {
            element.setAttribute('target', '_blank');
        }out
    });
}

You should NEVER! add superfluous or unused code.

Your code should have the single if statement checking for not equal.

    const href = element.getAttribute('href');
    if (href.indexOf('#') !== 0) {
        element.setAttribute('target', '_blank');
    }

or better yet avoid the overly complex referencing via get set attribute, and not using a search to find a known location state the whole thing becomes.

    if (element.href[0] === "#") { element.target = "_blank" }

And as a whole there is a huge amount of noise that can be removed

dom.window.document.querySelectorAll("a")
   .forEach(el => el.href[0] === "#" && (el.target = "_blank"));


Print array of sentences vertically Suppose I have an array of sentences:

  [ "How To Format", "put returns between paragraphs", "for linebreak add
  2 spaces at end" ]

I want to print it like this:

How        put            for
To         returns        linebreak
Format     between        add
           paragraphs     2
                          spaces
                          at
                          end

Here's my code:

public static void solve(String[] sentences) {
    int n = sentences.length;

    if(n == 0)
        return;

    List<List<String>> matrix = new ArrayList<List<String>>();
    int maxLength = Integer.MIN_VALUE;

    for(String sentence : sentences) {
        List<String> words = Arrays.asList(sentence.split(" "));
        maxLength = Math.max(maxLength, words.size());

        matrix.add(words);
    }

    for(int col = 0; col < maxLength; col++) {
        for(int row = 0; row < matrix.size(); row++) {
            try {
                System.out.format("%-15s", matrix.get(row).get(col));
            }
            catch(IndexOutOfBoundsException e) {
                System.out.format("%-15s", "");
            }
        }
        System.out.println();
    }
}

Is this easy to understand? Can I make it simpler, such as by using different data structures?

Use if statements rather than try/catch

Your method is understandable, you shouldn't use try catch, instead simply check that the index does not exceed the length.
e.g.

if (col < matrix.get(row).size()) {
   // print what you want
} else {
   // print just a space
}

Better Regex

Rather than sentence.split(" "); The regex you want is sentence.split("\\s+)
This ensures that you get all spaces and don't have an incorrect array length due to splitting on multiple empty spaces in a row. You can read more on Regex here.

Though this only applies if you don't want spaces to delimit words, ever, which is the assumption. The input itself sounds like it has to do with the specifications of the problem, but I simply took it as input and nothing more.

Use two dimensional arrays

You also don't need the list overhead for this.
Simply use a two dimensional of the same size as the original array and make the inner arrays the result of calling the aforementioned split() method.

public static String[][] convertToWordArray(String[] input) {
        String[][] result = new String[input.length][];

        for (int i = 0; i < result.length; i++) {
            result[i] = input[i].trim().split("\\s+");
        }

        return result;
}

Alternative Implmentation:

public class Format {
    public static void main(String[] args) {
        String[] target = {
            "How To Format",
            "put returns between paragraphs",
            "for linebreak add 2 spaces at end"
        };

        printWordsVertically(target);
    }
/* These next two methods are relics of my initial implementation
   You can adjust for the use of 2d arrays and only need 1 method. */
    public static int wordCount(String input) {
        return input.trim().split("\\s+").length;
    }

    public static int longestWordCount(String[] input) {
        int result = 0;

        for (String s : input) {
            if (result < wordCount(s)) {
                result = wordCount(s);
            }
        }

        return result;
    }

    public static void printWordsVertically(String[] input) {
        int longestWordCount = longestWordCount(input);
        int spacesBetweenColumns = 7; // use needed int, or pass as argument
        String formatSpecifier = "%-" + (longestWordCount + spacesBetweenColumns) + "s";
        String[][] words = convertToWordArray(input);

        for (int i = 0; i < longestWordCount; i++) {
            for (int j = 0; j < words.length; j++) {
                if (i < words[j].length) {
                    System.out.printf(formatSpecifier, words[j][i]);
                } else {
                    System.out.printf(formatSpecifier, "");
                }
            }
            System.out.println();
        }
    }

    public static String[][] convertToWordArray(String[] input) {
        String[][] result = new String[input.length][];

        for (int i = 0; i < result.length; i++) {
            result[i] = input[i].trim().split("\\s+");
        }

        return result;
    }
}


JavaScript: Better way to sort an array by setting custom index I'm trying to create a sorted array based on the below input data:
Input:
[{"title": "100_baz"}, {"title": "01_foo"}, {"title": "05_bar"}]
function indexSort (input) {
    let output = [];
    input.forEach(value => {
        if (value.title && value.title.indexOf('_') !== -1) {
            let targetIndex = parseInt (value.title.split('_')[0]);
            let title = value.title.substring(value.title.indexOf('_') + 1, value.title.length); // extract the string next to '_'
            output[targetIndex] = title;
        }
    })
    console.log(output)
}

Desired Output:

I'm able to get the desired output using the above code snippet, however, I'm looking for a better way to achieve this in terms of good practice, or a concise and clear method in ES5 or ES6.

Updating my comments on current implementation, Thanks @Mast for pointing out.

You have used let for output, targetIndex and title. None of
them never updated. const is better suitable here.
Usage of value.title.indexOf('_') is repeated. you can use variable.
You can avoid substring usage because you are already did value.title.split('_') and you can use index 1 value from result
    array.

Apart from these minor points, code looks good to me.

As you asked, Here is alternative ES6 implementation.

Use arrow function
With reduce and using destructure in input function arguments and have the default value for title.
In reduce function use destucture to get the values from split result.

const data = [{"title": "100_baz"}, {"title": "01_foo"}, {"title": "05_bar"}];

const indexSort = data => data.reduce((acc, {title = ''}) => {
  const [num, value] = title.split('_');
  acc[Number(num)] = value;
  return acc;
}, []);

console.log(indexSort(data));


Thread Safe File Operation I am trying to make file read and update operations thread safe and prevent race conditions in Python. Am I missing out something, or would this work in production?

import os
import time
from contextlib import contextmanager
import errno

current_milli_time = lambda: int(round(time.time() * 1000))
lock_retry_wait = 0.05 # in seconds

@contextmanager
def exclusive_file(filename, mod, lock_wait_timeout=3000):

    lockfile = filename + ".lock"

    wait_start = current_milli_time()

    while True:
        try:
            # acquire lock
            fd = os.open(lockfile, os.O_CREAT|os.O_EXCL)
            break

        except OSError as exception:
            if exception.errno != errno.EEXIST:
                raise

            else:
                t = current_milli_time()
                if (t - wait_start) > lock_wait_timeout:
                    raise

                time.sleep(lock_retry_wait)

    with open(filename, mod) as f:
        yield f

    os.close(fd)
    os.unlink(lockfile)

with exclusive_file("test.txt","a+") as f:
    previous_value = f.read()
    f.truncate()
    f.write(previous_value + str(time.time()))
    time.sleep(20)

1. Introduction

If you really mean what you say in the post about making access "thread safe", then creating lockfiles is overkill: it would be better to use threading.Lock objects. I am going to assume, in the rest of this review, that you mean "multiprocess safe" rather than "thread safe"

2. Review

There's no documentation. What does exclusive_file do? What guarantees does it provide? What operating systems does it run on?
If code raises an exception while the lock is held, then the lock is not released.
The choice of lock_retry_wait = 0.05 could end up rate-limiting operations involving a file so that they can happen at most 20 times a second. It would be better to make this into a keyword argument to the exclusive_file function, so that a user with different requirements can specify a different value.
It seems perverse that lock_retry_wait is in seconds but lock_wait_timeout is in milliseconds. This seems likely to lead to confusion. It is better for all time values to have the same units.
The current_milli_time function is written with lambda rather than def. There does not seem to be a good reason for this. It's better to use def because you can provide a docstring.
Using Python's built-in datetime.datetime objects, you could avoid the need for the current_milli_time function.
When programming a timeout, it is more efficient to compute a deadline once, in advance, than to repeatedly compute an elapsed time.
When you have a timeout argument to a function, it's often convenient to have a special value (for example None) meaning "keep retrying indefinitely".
Instead of catching all OSError and then re-raising the ones that aren't errno.EEXIST, catch FileExistsError instead.
The built-in function open takes more arguments than mode — there's also buffering, encoding, errors, newline, closefd, opener, and maybe others in forthcoming releases. It would be better for exclusive_file to take *args and **kwargs and forward these to open.
Because exclusive_file is a wrapper around the built-in open, it would be better for it to be named exclusive_open.
The code in the post only works if the process has write access to the directory containing the file. But sometimes you want to exclusively open a file in a directory to which you do not have write access.

3. Revised code

from contextlib import contextmanager
from datetime import datetime, timedelta
import os
from time import sleep

@contextmanager
def exclusive_open(filename, *args, timeout=3, retry_time=0.05, **kwargs):
    """Open a file with exclusive access across multiple processes.
    Requires write access to the directory containing the file.

    Arguments are the same as the built-in open, except for two
    additional keyword arguments:

    timeout -- Seconds to wait before giving up (or None to retry indefinitely).
    retry_time -- Seconds to wait before retrying the lock.

    Returns a context manager that closes the file and releases the lock.

    """
    lockfile = filename + ".lock"
    if timeout is not None:
        deadline = datetime.now() + timedelta(seconds=timeout)
    while True:
        try:
            fd = os.open(lockfile, os.O_CREAT|os.O_EXCL)
            break
        except FileExistsError:
            if timeout is not None and datetime.now() >= deadline:
                raise
            sleep(retry_time)
    try:
        with open(filename, *args, **kwargs) as f:
            yield f
    finally:
        try:
            os.close(fd)
        finally:
            os.unlink(lockfile)

Notes:

I chose to document the issue in 2.12 rather than fixing it.
Possibly the second try: finally: is overkill (close rarely fails), but I think it's wise to make every effort to release a lock. Deadlock is rarely fun to debug.


Should Domain objects for a public API be able to save/send themselves? My main questions:  FileWithSave handles its saving itself, but delegates it to a repository object.  This allows implementation details to be available without being present in the API when saving (transpose this to a Mail sending API as well, available for send but not public).  Is this OK in terms of OOP practices?  Are there any maintainence gotchas I'm missing?  Does it need reorganization?

Without save:

package com.example.external;

public interface File {
    /**
     * Get the unique identifier.
     *
     * @return unique identifier.
     */
    String getId();

    /**
     * Get the file name.
     *
     * @return file name.
     */
    String getName();

    /**
     * Sets name for the file.
     *
     * @param name file name.
     */
    void setName(String name);

    /**
     * Get the file size.
     *
     * @return file size.
     */
    Long getSize();
}

Without save implementation:

package com.example.internal;

public class FileImpl implements File {
    // I don't want it in my public signature but I want it when I'm saving for some reason :(
    private Something somethingPrivate;
    private final String id;
    private final String name;
    private final Long size;

    public FileImpl(String id, Long size) {
        this.id = id;
        this.size = size;
    }

    public String getId() {
        return id;
    }

    public String getName() {
        return name;
    }

    public void setName(String name) {
        this.name = name;
    }

    public Long getSize() {
        return size;
    }
}

Repository for File:

package com.example.external;

public interface FilingCabinet {
    /**
     * Get a file from the cabinet.
     *
     * @param id unique identifier.
     * @return file.
     */
    File get(String id);

    /**
     * Store a file in the cabinet.
     *
     * @param file.
     */
    void store(File file);
}

Without save client sample code:

FilingCabinet cabinet = room.getFilingCabinet();
File file = cabinet.get("my-unique-id");
file.setName("my-new-name");
cabinet.store(file);

With save:

package com.example.external;

public interface FileWithSave extends File {
    /**
     * Save the changes.
     */
    File store();
}

With save implementation:

package com.example.internal;

public class FileWithSaveImpl implements FileWithSave {
    private Something somethingPrivate;
    private final String id;
    private final String name;
    private final Long size;
    private final PrivateFilingCabinet repo;

    public FileWithSaveImpl(String id, Long size, PrivateFilingCabinet repo) {
        this.id = id;
        this.size = size;
        this.repo = repo;
    }

    @Override
    public String getId() {
        return id;
    }

    @Override
    public String getName() {
        return name;
    }

    @Override
    public void setName(String name) {
        this.name = name;
    }

    @Override
    public Long getSize() {
        return size;
    }

    @Override
    public void store() {
        repo.save(this);
    }
}

Wrapper class:

package com.example.external;

/**
 * Wraps PrivateFilingCabinet and injects PrivateFilingCabinet in FileWithSave object.
 */
public interface Secretary {
    /**
     * Get the file for someone.
     *
     * @param unique identifier.
     * @return file.
     */
    FileWithSave getFile(String id);
}

Repository injected in to FileWithSave and not part of the public API package:

package com.example.internal;

public interface PrivateFilingCabinet {
    /**
     * Get a file from the cabinet.
     *
     * @param id unique identifier.
     * @return file.
     */
    FileWithSaveImpl get(String id);

    /**
     * Store a file in the cabinet.
     *
     * @param file.
     */
    void store(FileWithSaveImpl file);
}

With save client sample code:

FileWithSave file = company.getSecretary().getFile("my-unique-id");
file.setName("my-new-name");
file.store();

This is a clear setup:

FilingCabinet cabinet = room.getFilingCabinet();
File file = cabinet.get("my-unique-id");
file.setName("my-new-name");
cabinet.store(file);

You can get files from the cabinet, modify the file,
and put the file back into the cabinet for storage.
The cabinet is in charge of storing the file.

This is an unclear, confusing setup:

FileWithSave file = company.getSecretary().getFile("my-unique-id");
file.setName("my-new-name");
file.store();

There is a company.
The company has a secretary.
You can get files from the secretary, modify the file,
and the file can store itself.

The second setup makes me wonder about several things:

Who's in charge of storing the file?
After I do .store(), does the secretary still have the same vision of the file as me?
What if I want to store the file somewhere else? For example, at a different company, or in a cabinet.

In the first setup I don't need to ask such questions,
as the answers are obvious.

Single responsibility principle

Another warning sign is that the second setup violates the single responsibility principle. The FileWithSave is not just a file anymore, it's also in charge of storage.
You might say, that's kind of the point, hence the name file-with-save.
Inheritance is not really about giving additional responsibilities to classes.
The purpose of inheritance is more about specialization.
The best sub-classes do the same things as their parents, just differently.
Giving a file the additional responsibility to save itself seems too much.
Even if this is not inherently a bad design,
you can replace with a better one (the first one), and so you should.

Bad practices

Use interface types instead of implementations.
This is especially crucial in interface definitions.
So instead of this:

public interface PrivateFilingCabinet {
    FileWithSaveImpl get(String id);
    void store(FileWithSaveImpl file);
}

It should have been:

public interface PrivateFilingCabinet {
    FileWithSave get(String id);
    void store(FileWithSave file);
}


CodeChef PRIME1 problem in Python PRIME1 is a CodeChef problem which states:

  Shridhar wants to generate some prime numbers for his cryptosystem.
  Help him! Your task is to generate all prime numbers between two given
  numbers.

  Input

  The first line contains t, the number of test cases (less then or
  equal to 10). Followed by t lines which contain two numbers m and n (1
  <= m <= n <= 1000000000, n-m<=100000) separated by a space.

  Output

  For every test case print all prime numbers p such that m <= p <= n,
  one number per line. Separate the answers for each test case by an
  empty line.

I have used segmented sieve and my code doesn't give TLE on any CodeChef IDE etc. yet I am getting a TLE. How can I optimise this code?

import math
max_n=10**9
m = int(math.sqrt(max_n))
arr = [1]*m
arr[0] = arr[1] = 0
#Calculate primes upto math.sqrt(max_n)
for x in xrange(m):
    for i in xrange(2,int(x**0.5)+1):
        if x%i==0:
            arr[x]=0
            break
list_prime=[]
for i,each in enumerate(arr):
    if each==1:
        list_prime.append(i)

#sieve b/w m and n
for _ in xrange(int(raw_input().strip())):
    m,n = map(int,raw_input().split())
    arr = range(m,n+1) if m>1 else range(2,n+1)
    for each in list_prime:
        firstfactor = (arr[0]/each)*each
        for x in xrange(firstfactor,n+1,each):
            try:
                if x!=each:
                    arr.remove(x)
            except:
                pass
    for each in arr:
        print each
    print

Using the same variables for different purposes in the same block of code is confusing to the reader. Split the code in 2-3 functions so that they become local variables.
You use trial division to compute the list of small primes even though you know about sieving. Sieving is faster.
arr.remove(x) in the innermost loop is slow because it scans through the whole list (to find the element to remove, and to move subsequent elements back). Use a set instead, or a list of boolean values similar to arr in the first part of the code.


Store open days and times Currently I'm developing my business entities. I feel that the structure is a tad cumbersome.

What I mean is that a store on a particular day can have different open and closing times. So it's not as simple as a store opens and closes at 09:00 and 17:00 respectively every day at the week. A store may open at 09:00 and 19:00 on only Thursdays etc.

Entities

public class Store
{
    public int StoreID { get; set;}
    public string StoreName { get; set; }
    public ICollection<DayOfWeek> DaysOpen { get; set; }
    public List<KeyValuePair<DayOfWeek, StoreTimes>> DaysAndTimes { get; set; }

}

public class StoreTimes
{
    public string StartTime { get; set;}
    public string EndTime { get; set; }
}

The initialisation is a but cumbersome, see below.

Initialisation

 Store store = new Store()
            {
                StoreID = 1,
                StoreName = "Some Store",
                DaysOpen = new List<DayOfWeek>() { DayOfWeek.Monday, DayOfWeek.Tuesday, DayOfWeek.Wednesday },
                DaysAndTimes = new List<KeyValuePair<DayOfWeek, StoreTimes>>()
                {
                    new KeyValuePair<DayOfWeek, StoreTimes>(DayOfWeek.Monday, new StoreTimes()
                    {
                        StartTime = "0900", EndTime = "1700"
                    }),
                    new KeyValuePair<DayOfWeek, StoreTimes>(DayOfWeek.Tuesday, new StoreTimes()
                    {
                        StartTime = "1000", EndTime = "1900"
                    })
                }

            };

Yes the initialisation needn't be a problem since the BLL will deal with it but how can this be refactored?

how can this be refactored?

Improved application of Single Responsibility Principle, encapsulation, and domain modeling.

Start With Good Data Structures

public class StoreTimes {
    public DayOfWeek Day {get; set;}
    public string StartTime { get; set; }
    public string EndTime { get; set; }
}

It is clear that an "hours of operation" must consist of a day along with open and close times.
So make a proper structure (class) that expresses this concept.
A complete data structure tends to be very flexible. In our case it will solve the problem @mjolka pointed out.
It simplifies the hell out of client code. i.e. dealing with "a list of key-value-pairs of days and store-times"

Ensure Objects Are Constructed Correctly

Constructor parameters forces the client (code) to supply the required things.
Arguments (the values passed via parameters) are easily validated up front before attempting to use bad or incomplete data.

.

public StoreTimes(DayOfWeek theDay, string startTime, string endTime)
    {
        Day = theDay;
        StartTime = startTime ?? "0000";
        EndTime = endTime ?? "0000";
    }

Don't allow Client to Arbitrarily Change State

non-public setters.

.

public class StoreTimes {
    public DayOfWeek Day {get; protected set;}
    public string StartTime { get; protected set; }
    public string EndTime { get; protected set; }

Use Domain Language

Stores open and close, they do not start and stop.

public class StoreTimes {

    public DayOfWeek Day {get; protected set;}
    public string OpenTime { get; protected set; }
    public string CloseTime { get; protected set; }
}

Make Cohesive Classes

OO programming is an exercise in self-reliance, for the classes. A class should know how to do things that is should be doing itself. The client should only have to tell to do it.

At a restaurant you tell them to cook food, you don't do it for them.
You ask to see a menu, you don't write a menu for them.

.

public class StoreTimes {
    public string HoursOfOperation()
    {
        return String.Format("{0} : {1} to {2}", Day, OpenTime, CloseTime);
    }
}

New Class Applying All the Points Above

public class HoursOfOperation
{
    protected List<StoreTimes> TheHours { get; set; }

    public HoursOfOperation()
    {
        TheHours = new List<StoreTimes>();
    }

    public void Add(StoreTimes newOpsHours)
    {
        if (newOpsHours == null) return;

        TheHours.Add(newOpsHours);
    }

    public override string ToString()
    {
        StringBuilder me = new StringBuilder();

        foreach (StoreTimes openTime in TheHours)
            me.AppendLine(openTime.HoursOfOperation());

        return me.ToString();
    }
}

Lessons Learned Applied to Store Class

public class Store
{
    protected int StoreID { get; set; }
    protected string StoreName { get; set; }
    protected HoursOfOperation OperatingHours { get; set; }

    public Store(string storeName, HoursOfOperation operatingHours, int storeID ){
        StoreID = storeID;
        StoreName = storeName ?? "NoNameStore";

        if (operatingHours == null)
            throw new ArgumentNullException("Operating Hours is null");

        OperatingHours = operatingHours;
    }

    public string HoursOfOperation()
    {
        return this.OperatingHours.ToString();
    }

    public override string ToString()
    {
        StringBuilder me = new StringBuilder();
        me.AppendLine(string.Format("Store {0}  ID {1}", StoreName, StoreID ));
        me.AppendLine("Hours Of Operation:");
        me.AppendLine(HoursOfOperation());

        return me.ToString();
    }
}

Client Code

   static void Main(string[] args)
    {
        HoursOfOperation opsHours = new HoursOfOperation();
        opsHours.Add(new StoreTimes(DayOfWeek.Monday, "0900", "1130"));
        opsHours.Add(new StoreTimes(DayOfWeek.Monday, "1300", "1800"));
        opsHours.Add(new StoreTimes(DayOfWeek.Saturday, "0800", "2100"));
        opsHours.Add(new StoreTimes(DayOfWeek.Sunday, null, null));

        Store CostingTons = new Store("CostingTons", opsHours, 1);
        Console.WriteLine(CostingTons.ToString());
        Console.ReadLine();
  }


Multithreaded for loop I am trying to multithread a for loop, and this works so far. How can it be improved? Have I done anything incorrectly?

The work is broken up into blocks and the block variable is the amount of objects to process per thread.

Test.class

public class Test extends MultiThreaded {

    public static void main(String args[]) {

        List<TestObject> values = new ArrayList<TestObject>();

        for(int i = 1;i < 7;i++)
            values.add(new TestObject(String.valueOf(i)));

        int block = 2;
        int cursor = 0;

        List<TestObject> objectList = new ArrayList<TestObject>();

        for(TestObject v : values) {
            cursor++;
            objectList.add(v);

            if(cursor < block)
                continue;

            final List<TestObject> objectListCopy = new ArrayList<TestObject>();
            objectListCopy.addAll(objectList);

            Task task = new Task() {
                @Override
                public void run() {
                    for(TestObject o : objectListCopy)
                        o.print();
                }
            };

            startTask(task);
            objectList.clear();

            cursor = 0;
        }

        while(MultiThreaded.processing()) { continue; }

        System.out.println("move on");
    }

    public static class TestObject {
        String line;

        public TestObject(String string) {
            this.line = string;
        }

        public void print() {
            System.out.println(line);
        }
    }

}

MultiThreaded.class

public class MultiThreaded {

    public static HashSet<Task> tasks = new HashSet<Task>();

    public static class Task extends Thread {
        public boolean running;
        public void run(){
            return;
        }
    }

    public static void startTask(Task task) {
        task.start();
        tasks.add(task);
    }

    public static Task getTask(int id) {

        for(Task task : tasks){
            if(task.getId() == id)
                return task;
        }

        return null;
    }

    public static boolean processing() {

        boolean processing = false;

        for(Task task: tasks){
            if(task.isAlive())
                processing = true;
        }

        return processing;
    }

}

OUTPUT

1
2
5
6
3
4
move on

Multi-threaded concepts are complicated at times. If you really want to learn multi-threading, it is important to get it right. Some things can be extremely difficult to get right. I can highly recommend reading Java Concurrency in Practice.

Overall

It is not that often I see "multi-threaded code" without a single synchronized keyword, or a single use of an atomic variable or concurrent data structure. I suggest you read up on all three of those to better understand what you will have to deal with sooner or later.

I think that you are adding a lot of overhead with the way you are constructing your tasks, that it will remove any benefits you get by running it multi-threaded. Creating all the lists that you use simply takes more time than actually running the code.

It is also important to consider that you are using System.out.println, this is a synchronized call, which means that only a single thread can only run it a time. This tends to be a big concurrency bottle-neck in a lot of multi-threaded code. As your task(s) seems to only be to use System.out.println, I think your code would be better of as single-threaded.

Oops, missed some!

In your current code, it does not run the print method for the last x items in the list. There already, it is broken.

If you have 1024 items and use a "block" value of 100, 24 items will be missed. (Hint: What is the value of your cursor variable after your for-loop to start the tasks is finished?)

Singleton mindset

This code:

public static HashSet<Task> tasks = new HashSet<Task>();

By making it public static it seems like you are in the "singleton mindset". Avoid this. This has no business being either public or static. Imagine if some code somewhere at some point would call tasks.clear(); - That could break a lot.

This field is also a big memory leak. You are only adding tasks to it, you are not removing any tasks.

Extending for no reason

You are extending the MultiThreaded class, but all methods and fields in that class are static. Extending it has no real effect besides giving you quick-access to the methods and variables, by not having to write MultiThreaded. each time. This is a bad reason for extending a class.

getTask

Your public static Task getTask(int id) method, although it is not used, suggests that you are using the wrong data-structure. You could use a Map<Integer, Task> which will make the getTask lookup much more efficient (from \$O(n)\$ to \$O(1)\$).  As you are dealing with multi-threading, better make it a ConcurrentMap as well. You should however ask yourself if you really need this in the first place.

Are we working? Are we working? Are we, are we, are we?

This code:

while(MultiThreaded.processing()) { continue; }

Is a busy-loop. This code will run as quickly as possible and waste CPU power for you. Running this will essentially remove any benefits you get by using multi-threading in the first place. You should check into the wait-notify construct. This can be compared to three kids in the backseat of your car constantly asking you "Are we there yet? Are we there yet? Are we, are we, are we?", I bet you can drive better without such a distraction. (No offense intended against kids)

This busy-loop does not get better by the fact that your processing() method is an inefficient way to check if any task is still working. Use Atomic variables to keep track of the number of running tasks.

The wheel...

Java 8 provides the concept of parallel streams which significantly makes this easier.

Your main method can be replaced with this:

List<TestObject> values = new ArrayList<TestObject>();
for (int i = 1; i < COUNT; i++)
    values.add(new TestObject(String.valueOf(i)));
values.parallelStream().forEach(o -> o.print());
System.out.println("move on");