Array that stores values from two ranges and compares if one cell is blank This is my first Array in VBA for Excel, but I need some help to optimize the code and try to reduce the number of If statements.

The long and short of the code is that it checks to see if there is a Customer name in Column B and checks that against Column A; if there is no value in Column A then  an input box opens to prompt the user to add the CIF number to the specific named range.

Cells B2 through B9 will always have a value, but not every cell B2 through B9 will be used.  Cells A2 through A9 will not always have a value.

Below is the Code:

Sub CheckCIF()
    Dim fileArray
    Dim finalRow As Long
    Dim targetCol As Long

    With Sheets("Loan Data")
        finalRow = .Range("B" & Rows.Count).End(xlUp).Row
        targetCol = .Range("A1").EntireRow.Find("CIF #").Column
        fileArray = Array(.Range("B2:B" & finalRow), _
                    .Range(.Cells(2, targetCol), .Cells(finalRow, targetCol)))
    End With

    'fileArray(1)(1) Number in first parenthesis is the column and the number in the second parenthesis is the row

    If fileArray(1)(1) = vbNullString And fileArray(0)(1) <> vbNullString Then
        With Sheets("Loan Data")
            .Range("CIF_1") = InputBox("Please enter the CIF Number for " & vbCrLf & fileArray(0)(1))
        End With
    End If

    If fileArray(1)(2) = vbNullString And fileArray(0)(2) <> vbNullString Then
        With Sheets("Loan Data")
            .Range("CIF_2") = InputBox("Please enter the CIF Number for " & vbCrLf & fileArray(0)(2))
        End With
    End If

    If fileArray(1)(3) = vbNullString And fileArray(0)(3) <> vbNullString Then
        With Sheets("Loan Data")
            .Range("CIF_3") = InputBox("Please enter the CIF Number for " & vbCrLf & fileArray(0)(3))
        End With
    End If

    If fileArray(1)(4) = vbNullString And fileArray(0)(4) <> vbNullString Then
        With Sheets("Loan Data")
            .Range("CIF_4") = InputBox("Please enter the CIF Number for " & vbCrLf & fileArray(0)(4))
        End With
    End If

    If fileArray(1)(5) = vbNullString And fileArray(0)(5) <> vbNullString Then
        With Sheets("Loan Data")
            .Range("CIF_5") = InputBox("Please enter the CIF Number for " & vbCrLf & fileArray(0)(5))
        End With
    End If

    If fileArray(1)(6) = vbNullString And fileArray(0)(6) <> vbNullString Then
        With Sheets("Loan Data")
            .Range("CIF_6") = InputBox("Please enter the CIF Number for " & vbCrLf & fileArray(0)(6))
        End With
    End If

    If fileArray(1)(7) = vbNullString And fileArray(0)(7) <> vbNullString Then
        With Sheets("Loan Data")
            .Range("CIF_7") = InputBox("Please enter the CIF Number for " & vbCrLf & fileArray(0)(7))
        End With
    End If

    If fileArray(1)(8) = vbNullString And fileArray(0)(8) <> vbNullString Then
        With Sheets("Loan Data")
            .Range("CIF_8") = InputBox("Please enter the CIF Number for " & vbCrLf & fileArray(0)(8))
        End With
    End If

End Sub

The fileArray variable can be replaced with 2 variables. When looking at fileArray in the locals window, from the IDE menu at the top View>Locals Window, you can see that it's it a Variant/Variant(0 to 1). Expanding that by clicking on the + icon to the left of the variable name it it contains two sub arrays. fileArray(0) and fileArray(1) are both of type Variant/Object/Range. These are distinct areas that should be named respectively. customerArea for those in column B and cifArea for those in the same column as targetCol let you refer to these ranges directly.

Because these variables house information stored vertically, that is X rows tall by 1 column wide, you can reference the rows by customerArea(checkRow).Value2. Doing this allows you to write self documenting code. Rather than

If fileArray(1)(1) = vbNullString And fileArray(0)(1) <> vbNullString Then

You have

If customerArea(checkRow).Value2 = vbNullString And cifArea(checkRow).Value2 <> vbNullString Then

I'm torn between how to go about checking each of the rows. Mulling it over I decided on extracting the check into a Sub. I added a guard clause, endRow > customerArea.Rows.Count Or endRow > cifArea.Rows.Count, that raises an error if the end row exceeds the number of rows in each area that's checked. You could also add a guard clauses to ensure that startRow >= 1, or ensure that customerArea.Rows.Count = cifArea.Rows.Count or any others you deem appropriate, but that I'll leave for you.

Instead of checking each row explicitly I condensed that with a For...Next statement and used a descriptive variable checkRow to let you know which row you're checking.

Private Sub CheckForEmptyCellsIn(ByVal customerArea As Range, ByVal cifArea As Range, ByVal startRow As Long, ByVal endRow As Long)
    Const InputMessage As String = "Please enter the CIF Number for "

    Const InvalidArgument As Long = 5
    If endRow > customerArea.Rows.Count Or endRow > cifArea.Rows.Count Then
        Err.Raise InvalidArgument, "CheckForEmptyCells Sub", "endRow argument exceeded number of rows in"
    End If

    Dim checkRow As Long
    For checkRow = startRow To endRow
        If customerArea(checkRow).Value2 = vbNullString And cifArea(checkRow).Value2 <> vbNullString Then
            customerArea.Parent.Range("CIF_" & checkRow).Value2 = InputBox(InputMessage & vbCrLf & cifArea(1).Value2)
        End If
    Next
End Sub

This refactoring leaves you with

Public Sub CodeReview()
    Dim loanData As Worksheet
    Set loanData = Worksheets("Loan Data")
    Dim finalRow As Long
    finalRow = loanData.Range("B" & Rows.Count).End(xlUp).Row
    Dim targetCol As Long
    targetCol = loanData.Range("A1").EntireRow.Find("CIF #").Column

    CheckForEmptyCellsIn loanData.Range("B2:B" & finalRow), _
                        loanData.Range(loanData.Cells(2, targetCol), loanData.Cells(finalRow, targetCol)), _
                        1, 8
End Sub


Least recent use cache in python Below is LRU Cache class implementation.

# Design and implement a data structure for Least Recently Used (LRU) cache.
# It should support the following operations: get and put.
#
# get(key) - Get the value (will always be positive) of the key if the key exists in the cache,
# otherwise return -1.
# put(key, value) - Set or insert the value if the key is not already present.
# When the cache reached its capacity,

I've left out a lot of the details, but most of the methods you can just delegate to the internal ordered dict self.store.

https://leetcode.com/problems/lru-cache/description/

Could you do both operations in O(1) time complexity?

class LRUCache(object):

    def __init__(self, capacity):
        """
        :type capacity: int
        """
        self.capacity = capacity
        self.dic = {}
        self.history_dic = {}
        self.key_dic = {}
        self.current = 0
        self.count = 0
    def get(self, key):
        """
        :type key: int
        :rtype: int
        """
        if not key in self.dic:return -1
        self.update_history(key)
        return self.dic[key]

    def put(self, key, value):
        """
        :type key: int
        :type value: int
        :rtype: void
        """
        self.update_history(key)
        if key in self.dic:
            self.dic[key] = value
            return
        if len(self.dic)==self.capacity:
            i = self.current
            while not self.history_dic[i]:
                i += 1
            self.dic.pop(self.history_dic[i])
            self.key_dic.pop(self.history_dic[i])
            self.current = i+1
        self.dic[key] = value

    def update_history(self,key):
        if key in self.key_dic:
            self.history_dic[self.key_dic[key]] = None
        self.history_dic[self.count] = key
        self.key_dic[key] = self.count
        self.count += 1

# Time:  O(1), per operation.
# Space: O(k), k is the capacity of cache.

# put(key, value) - Set or insert the value if the key is not already present.
# When the cache reached its capacity,
# it should invalidate the least recently used item before inserting a new item.
#
# Follow up:
# Could you do both operations in O(1) time complexity?
#
# Example:
#
# LRUCache cache = new LRUCache( 2 /* capacity */ );
#
# cache.put(1, 1);
# cache.put(2, 2);
# cache.get(1);       // returns 1
# cache.put(3, 3);    // evicts key 2
# cache.get(2);       // returns -1 (not found)
# cache.put(4, 4);    // evicts key 1
# cache.get(1);       // returns -1 (not found)
# cache.get(3);       // returns 3

This cache feels like it could be used like a dict but is nowhere near it. Renaming get to __getitem__ and put to __setitem__ would provide a neater feeling here. Even better, since your main storage is a dict, why not subclass it directly. You would benefit of its interface with almost no additional cost. Just replace every self.dic with a simple self.

Then, to support your requirements, you would implement get based on dict.get and put would be plain __setitem__:

class LruCache(dict):
    ...
    def get(self, key):
        return super().get(key, -1)

    put = __setitem__

Second, you seem to envision using this cache only to store integer keys and integer values. This is a strong assumption here and the cache will misbehave if you ever try to .put(None, 42); or even .put(0, 42) for that matter as you do not distinguish it in you test while not self.history_dic[i]:. Instead, you could delete the associated entry and check for existence (while i not in self.history:).

Lastly, your naming can be improved. _dic as part of names is implementation detail and does not really add meaningful value here; count is the number of updates and should probably be named after that fact; as well as current being the currently know "index" of the least recently used element.

Proposed improvements:

from contextlib import suppress
from typing import Any

class LruCache(dict):
    def __init__(self, capacity: int):
        super().__init__()
        self.capacity = capacity
        self._least_recent = 0
        self._number_of_updates = 0
        self._history = {}
        self._keys_indexes = {}

    def get(self, key: Any) -> Any:
        return super().get(key, -1)

    def __getitem__(self, key: Any) -> Any:
        value = super().__getitem__(key)
        self._update_history(key)
        return value

    def __setitem__(self, key: Any, value: Any):
        self._update_history(key)
        while key not in self and len(self) >= self.capacity:
            while self._least_recent not in self._hitory:
                self._least_recent += 1
            del self[self._history[self._least_recent]]
        super().__setitem__(key, value)

    put = __setitem__

    def __delitem__(self, key: Any):
        super().__delitem__(key)
        index = self._keys_indexes.pop(key)
        del self._history[index]
        if index == self._least_recent:
            self._least_recent += 1

    def _update_history(self, key: Any):
        with suppress(KeyError):
            del self._history[self._keys_indexes[key]]
        self._history[self._number_of_updates] = key
        self._keys_indexes[key] = self._number_of_updates
        self._number_of_updates += 1

You’ll note that I changed the check in capacity into a while loop in case someone would change the cache size halfway through the program such as:

cache = LruCache(10)
cache.put(…)
…
cache.put(…)
cache.capacity = 2
cache.put(…)  # cache shrink from 10 to 2 capacity

Dropping elements as soon as the cache.capacity = 2 line is executed is left as an exercise to the reader.


Optimizing breadth-first-search code I wrote code using some kind of breadth-first-search algorithm in order to find a path (any path!) from a start point to an end point. There are no weights/different distances involved from path-to path-so I am not sure I should use Dijkstra's algorithm.

However, I am required to find a solution that can search for and return a path within a second (while this current code can complete it in five seconds). Assuming my code is inefficient, what can I do better?

$visited = Array.new
$queue = Array.new
def find_path (start, endpoint)
  returnArray = Array.new
  start = start.to_s
  endpoint = endpoint.to_s
  $queue.push(start)
  nextPaths = get_next_paths(start)
  if start == endpoint
    return returnArray.push(endpoint)
  else
    $visited.push(start)
    $queue.pop(start.to_i)
    nextPaths.each { |i| next if $visited.include?(i);
    $queue.push(i)
    returnArray = find_path(i.to_i, endpoint.to_i)
        if returnArray == nil
            next
        elsif !returnArray.empty?
            returnArray.unshift(start)
            return returnArray
        end
    }
  end

  if !$queue.empty?
    return []
  else
    return nil
  end
end

def get_next_paths(id)
  return $h[id.to_s]
end

def direct_connection?(id1, id2)
  next_paths = get_next_paths(id1)
  return (next_paths!=nil and next_paths.include?(id2.to_s))
end

def valid_path?(path)
  while (path.length>1)
    current_element = path.first
    next_element = path[1]

    if (!direct_connection?(current_element, next_element))
      return false
    end

    path.shift
  end
  return true
end

I'm not sure if I understand your input data correctly, since I don't get this $queue.pop(start.to_i), but:

I won't mention codestyle issues -- I'll talk about algorithm.
Looks like your search is really Depth-first, since it doesn't find the shortest path:

$h = {?0=>[?1,?2,?3], ?1=>[?3], ?3=>[?4]}
p find_path ?0,?4
=> ["0", "1", "3", "4"]

it is because:

it calls recursion immediately before iterating the rest of child nodes
in real Breadth-first you have to store a lot of paths at the same time

Use Set instead of Array for visited, since it looks up immediately instead of searching the value from the beginning of the list.
The easiest way to implement both of these searches with no risk of stack overflow is to use stack of the next states, that we should analyze instead of recursion. Also in this way we don't need global variables or other namespace tricks, since we just do the loop, where path is used instead of $queue and returnArray:

Take this code:

def find_path start, endpoint
  require "set"
  visited = Set.new
  stack = [[start.to_s]]
  until stack.empty?
    path = stack.shift
    start = path.last
    get_next_paths(start).each do |i|
      return path.push i if i == endpoint.to_s
      next if visited.include? i
      visited << start
      stack.push path+[i]
    end
  end
end

And run this:

$h = {?0=>[?3,?2,?1], ?1=>[?5], ?3=>[?4], ?5=>[?4], ?2=>[], ?4=>[]}
p find_path_ ?0,?4

The fun thing is how it is now easy to switch between Depth and Breadth by just replacing .shift with .pop:

["0", "3", "4"]
["0", "1", "5", "4"]


Bash delete folder if not present in other folder imaging a folder structure like:

Peter
|- Cats
   |- Mini
   |- Maunz
   |- Tippsi
|– Dogs
   |- Brutus

Eva
|- animals
   |- Mini
   |- Brutus

I'd like to delete all subfolders in Peter/*/ if they not present in Eva/animals/  (Maunz and Tippsi).
This is what I have so far. It's working but it feels as my combination of loop and find is not the best solution:

for dir in ./Peter/*/*/
do
   dir=${dir%*/}
   if find './Eva/animals' -type d -name ${dir##*/} -exec false {} +
   then
      echo "${dir%*/} not found and deleted"
      rm -rf ${dir%*/}
    fi
done

For learning, can this code be optimized?

find is good for finding files and directories recursively in a filesystem tree.
Based on your description and example, you probably don't need that.
If you want to check that some directory foo exists directly under /some/path,
it's much simpler and more efficient to use the test or [ builtin.

There are some other minor issues in the posted code:

It's a good practice (and often necessary) to double-quote variables in command parameters. For example instead of rm -rf ${dir%*/} write rm -rf "${dir%*/}"
Instead of writing ${dir%*/} 3 times, it would be better to store in a variable. In fact you did store the result of the first "call" in a variable, and since subsequent calls on the modified value will be the same, you could have used $dir.
I think dir=${dir%/} is equivalent to dir=${dir%*/} but simpler.

Putting the above together, a cleaner, more efficient alternative:

#!/usr/bin/env bash

for dir in ./Peter/*/*/
do
    dir=${dir%/}
    name=${dir##*/}
    if ! [ -d "./Eva/animals/$name" ]
    then
        echo "$dir not found and deleted"
        rm -rf "$dir"
    fi
done


Find minimal modulo-sum of pairs from two lists Recently I appeared for an job challenge organised on HackerEarth.
Question:
There are two teams T1 and T2. Power of each player is represented in array. The rules of game are as follow.
There are only N fights.

No player from any team can fight twice.
At any instance, only one fight can occur.
For each fight score is calculated as (P1+P2)%N where P1, P2 is the power of player from T1 and T2 respectively and % is modulo operator.

Task in Hand is to arrange T2 players in such a way so that score is minimum. We have to print the score of the match after every fight. T2 is aware of the sequence of T1 players.
Input format is first line contains T denoting the number of test cases. It also denotes the number of times we have to run our function.
Second line contains N i.e. the number of fights
Third line contains the power of players from Team T1. The sequence in which T1 player power is shown is the sequence in which T1 players would fight.
Fourth line contains the power of players from Team T2.
Constraints:

1<=T<=10^5
1<=N<=10^5
0<=T1[i],T2[i]<= N for all i's.

I wrote the following code:
def solve (N, T1, T2):
    # Write your code here
    final_list=''
    for each in T1:
        temp_list=[]
        flag=True
        for i in range(0,len(T2)):
            if (each + T2[i])%N==0:
                T2.pop(i)
                final_list+='0'
                flag=False
                break
            else:
                temp_list.append((each + T2[i])%N)
        if flag:
            j=temp_list.index(min(temp_list))
            final_list+=str(min(temp_list))
            T2.pop(j)

    return final_list

T = int(input())
for _ in range(T):
    N = int(input())
    T1 = list(map(int, input().split()))
    T2 = list(map(int, input().split()))

    out_ = solve(N, T1, T2)
    print (' '.join(map(str, out_)))

It took 0.1 seconds for few test cases and more than 10.2 seconds for few test cases(so basically the test case failed for taking too much time).
Looking for optimal code

Style:
This is the exact same algorithm, with the code-format, variable-names, and the use of strings cleaned up. I'm not actually sure this is the exact same function as yours, because the way you were using strings was probably wrong (because of the ' '.join(...) in the surrounding use-code).
def original_solve(N, T1, T2):
    final_list = []
    for item_1 in T1:
        temp_list = []
        already_appended = False
        for i in range(0, len(T2)):
            if (item_1 + T2[i]) % N == 0:
                T2.pop(i)
                final_list.append(0)
                already_appended = True
                break
            else:
                temp_list.append((item_1 + T2[i]) % N)
        if not already_appended:
            best_index = temp_list.index(min(temp_list))
            final_list.append(min(temp_list))
            T2.pop(best_index)

    return final_list

Benchmarking:
We should have a quick way to test out any improvements we make.
from random import randint  # insecure RNG is fine for this task.
from time import time

def make_test(N, t_len):
    return (N,
            [randint(0, N) for _ in range(t_len)],
            [randint(0, N) for _ in range(t_len)],
            None)

def time_function(f, *args):
    start = time()
    retval = f(*args)
    end = time()
    return retval, end - start

def main():
    candidates = {
        "Original": original_solve
    }
    tests = [
        (3, [1, 2, 3], [1, 2, 3], [0, 0, 0]),
        (4, [1, 2, 3], [1, 2, 3], [0, 0, 0]),
        (3, [1, 1, 3], [1, 2, 3], [0, 1, 1]),
        make_test(100, 100),
        make_test(100, 100),
        make_test(100, 100),
        make_test(10000, 10000),
        make_test(10000, 10000),
        make_test(10000, 10000),
    ]
    print('   T   ' + ''.join(f'{k:>10} (s)' for k in candidates.keys()) + '    Answer')
    for N, T1, T2, known in tests:
        results = [time_function(f, N, list(T1), list(T2))  # Copy these lists, since these algorithms modify T2.
                   for f in candidates.values()]

        assert 1 == len(set(
            tuple(answer)
            for answer in [known, *(r[0] for r in results)]
            if answer is not None
        ))
        print(f'{len(T1):^7}'
              + ''.join(f'{r[1]:>14.9f}' for r in results)
              + f'  {results[0][0][:4]}')

if __name__ == '__main__':
    main()

Improvements to the existing algorithm:
Your current code is O(n^2). I suspect we can do better than that, but first lets consider some simplifications.

Checking separately for (item_1 + T2[i]) % N == 0 and short-circuiting is unlikely to save us much time, and it clutters up the function. Let's get rid of all of that.
Finding the min of temp_list, and then searching again to get its index, is stinky. One option would be to build temp_list as a list of t2_index, computed_value tuples, but I think re-doing one of the modulo-sum computations is cheap enough to not worry about. So we can just ask for the minimum index using a key-function.

Improving the algorithm:
The modulo operator in the scoring function is what makes this problem non-trivial, but it doesn't really change the fact that numbers are easy to sort, and sorting is usually O(n log(n)). If our list (T2) were already sorted, could we do the rest of the problem in better-than O(n^2) complexity?

Sort T2, and give it a structure to keep track of what's already been used. This is bank in the bellow code. Using ints/flags to track what's already been used is (probably) better than just dropping elements from the structure, because python's list's pop-intermediate is O(n).
For each item in T1:

Calculate the ideal paring: N - item (mod N)
Use a binary search (O(log(n)) to find where that value is or should be in the bank-structure.
Search upward from there for a value that hasn't been used yet. In the worst-case this is O(n), but for randomly generated data the rate of collisions will be "low" (I haven't looked up the formula)

So yes, we can write a sub-quadratic algorithm, in fact it's O(n log(n)), which is pretty good for most purposes. Further refinement may be possibly, but I suspect there's not an algorithm better than log-linear.
Results:
from itertools import count, groupby
from random import randint  # insecure RNG is fine for this task.
from time import time

def original_solve(N, T1, T2):
    final_list = []
    for item_1 in T1:
        temp_list = []
        already_appended = False
        for i in range(0, len(T2)):
            if (item_1 + T2[i]) % N == 0:
                T2.pop(i)
                final_list.append(0)
                already_appended = True
                break
            else:
                temp_list.append((item_1 + T2[i]) % N)
        if not already_appended:
            best_index = temp_list.index(min(temp_list))
            final_list.append(min(temp_list))
            T2.pop(best_index)

    return final_list

def clean_solve(N, T1, T2):
    final_list = []
    for item_1 in T1:
        def key_function(index):
            return (item_1 + T2[index]) % N
        best_index = min(range(len(T2)), key=key_function)
        final_list.append(key_function(best_index))
        T2.pop(best_index)

    return final_list

def sub_quadratic_solve(N, T1, T2):
    # Cases of `% length` are wrapping indexes back to the start of the list.
    # Cases of `% N` are wrapping any exact N values back to 0.
    bank = tuple([t2_item, len(list(copies))]  # using list as an easily-mutable object.
            for t2_item, copies
            in groupby(sorted(t % N for t in T2)))
    length = len(bank)

    def binary_search_index(value):  # https://www.codementor.io/@info658/the-binary-search-algorithm-in-python-19jvyrt5ly
        lower_bound = 0
        upper_bound = length - 1
        while lower_bound < upper_bound:
            midpoint = (lower_bound + upper_bound) // 2
            if bank[midpoint][0] == value:
                return midpoint
            elif bank[midpoint][0] < value:
                lower_bound = midpoint + 1
            else:
                upper_bound = midpoint - 1
        return lower_bound if bank[lower_bound][0] >= value else lower_bound + 1

    def find_best(value):
        ideal_index = binary_search_index(value % N)
        useable_index = next(i % length
                             for i in count(ideal_index)
                             if bank[i % length][1])
        bank[useable_index][1] -= 1
        return bank[useable_index][0]

    return [(t1_item + find_best(N - t1_item)) % N
            for t1_item in T1]

def make_test(N, t_len):
    return (N,
            [randint(0, N) for _ in range(t_len)],
            [randint(0, N) for _ in range(t_len)],
            None)

def time_function(f, *args):
    start = time()
    retval = f(*args)
    end = time()
    return retval, end - start

def main():
    candidates = {
        "Original": original_solve,
        "Cleaned": clean_solve,
        "Sub-Quad": sub_quadratic_solve,
    }
    tests = [
        (3, [1, 2, 3], [1, 2, 3], [0, 0, 0]),
        (4, [1, 2, 3], [1, 2, 3], [0, 0, 0]),
        (3, [1, 1, 3], [1, 2, 3], [0, 1, 1]),
        make_test(100, 100),
        make_test(100, 100),
        make_test(100, 100),
        make_test(10000, 10000),
        make_test(10000, 10000),
        make_test(10000, 10000),
    ]
    print('   T   ' + ''.join(f'{k:>10} (s)' for k in candidates.keys()) + '    Answer')
    for N, T1, T2, known in tests:
        results = [time_function(f, N, list(T1), list(T2))  # Copy these lists, since these algorithms modify T2.
                   for f in candidates.values()]
        assert 1 == len(set(
            tuple(answer)
            for answer in [known, *(r[0] for r in results)]
            if answer is not None
        ))
        print(f'{len(T1):^7}'
              + ''.join(f'{r[1]:>14.9f}' for r in results)
              + f'  {results[0][0][:4]}')

if __name__ == '__main__':
    main()

Prints:
   T     Original (s)   Cleaned (s)  Sub-Quad (s)    Answer
   3      0.000027657   0.000033140   0.000041246  [0, 0, 0]
   3      0.000026226   0.000004768   0.000029802  [0, 0, 0]
   3      0.000004292   0.000024319   0.000008106  [0, 1, 1]
  100     0.000726700   0.000534534   0.000247002  [0, 2, 0, 0]
  100     0.000698566   0.000530243   0.000190973  [0, 0, 4, 0]
  100     0.000658035   0.000528812   0.000208855  [0, 5, 0, 2]
 10000    7.761836529   5.691253662   0.064605951  [0, 0, 1, 0]
 10000    7.386102200   5.667595863   0.067104578  [0, 1, 0, 2]
 10000    7.601329803   5.666092873   0.056614161  [0, 0, 0, 0]


Show a search input bar depending on which path the user currently is viewing I'm using Laravel with the Blade template engine. I'm trying to show a search input bar, depending on which path the user currently is viewing. The three paths are all, snes and nes.

This is what I have done so far:

@if (request()->route()->getName() === 'snes')
    @include('partials._search')
@endif

@if (request()->route()->getName() === 'nes')
    @include('partials._search')
@endif

@if (request()->route()->getName() === 'all')
    @include('partials._search')
@endif

It doesn't seem so effective nor like good looking code.

How can I shorten this?

You could use PHPs in_array() function to test the current route against your required routes:

@php ( $searchRoutes = ['foo', 'bar'] )
@if( in_array(request()->route()->getName(), $searchRoutes))
   @include('partials._search')
@endif

You could define the allowed routes array in your controller and pass this into the blade template.


Finding an equilibrium of an index of an array Equilibrium index of an array is an index such that the sum of elements at lower indexes is equal to the sum of elements at higher indexes. P is an index of an array A.

Input:

A[0] = -1
A[1] =  3
A[2] = -4
A[3] =  5
A[4] =  1
A[5] = -6
A[6] =  2
A[7] =  1

Output:

1
3
7

1 is an equilibrium index of this array, because:

A[0] = −1 = A[2] + A[3] + A[4] + A[5] + A[6] + A[7]

3 is an equilibrium index of this array, because:

A[0] + A[1] + A[2] = −2 = A[4] + A[5] + A[6] + A[7]

7 is also an equilibrium index, because:

A[0] + A[1] + A[2] + A[3] + A[4] + A[5] + A[6] = 0

My solution for review:

def isEquilibrium(alist, ind):
    listLen = len(alist)
    left = getSum(0, ind, alist)
    right = getSum(ind+1, listLen, alist)
    # print left, right
    if left == right:
        return True

def getSum(start, end, lis):
    tot = 0
    for i in xrange(start, end):
        tot = tot + lis[i]
    return tot

def solution(alist):
    for i in xrange(0, len(alist)):
        result = isEquilibrium(alist, i)
        if(result):
            print i

solution([-1,3,-4,5,1,-6,2,1])

You can utilize built-in features of Python to refactor the code.

Use the sum() function

Python already provides a sum() function to add all elements of a list that you can use.

example_list = [1, 2, 3, 4, 5]
result = sum(example_list)

Slicing

Python allows selecting a range of values of a list. The syntax is:

list[start:end:step]

All these are optional. By default, start is 0, end is the length of the list, and step is 1.

Using this, you can select elements before the index by list[:index] and select elements after the index using list[index+1:].
Idiomaticity

Here, a and b are compared for equality. If a is equal to b, the expression a == b will be True, so True is returned.

if a == b:
    return True

# Refactored
return a == b

Naming conventions

snake_case is preferred over camelCase for naming in Python community. Use is_equilibrium instead of isEquilibrium.


Is it possible to refactor these two similar methods that handle T and List? Is there a way to unify or refactor two methods that do basically the same but the difference being of handling a type of object or a collection of the same type of object?
The following is an implementation of two methods that have very similar behavior. They handle a cache for an API, so when someone asks for a value or a set of values, it verifies that the entry is in cache already. If not, they call the method that will return from an external API: a value or a set of values. Finally the methods, update the cache and return the value/values.
public async Task<T> GetValue<T>(string cacheKey, Func<Task<T>> getItem, MemoryCacheEntryOptions memoryCacheEntryOptions = null)
{
    if (_memoryCache.TryGetValue(cacheKey, out T cacheEntry))
    {
        return cacheEntry;
    }
    else
    {
        cacheEntry = await getItem();

        return SetValue(cacheKey, cacheEntry, memoryCacheEntryOptions);
    }
}

public async Task<IList<T>> GetValues<T>(string cacheKey, Func<Task<IList<T>>> getItems, MemoryCacheEntryOptions memoryCacheEntryOptions = null)
{
    if (_memoryCache.TryGetValue(cacheKey, out IList<T> cacheEntries))
    {
        return cacheEntries;
    }
    else
    {
        cacheEntries = await getItems();

        return SetValues(cacheKey, cacheEntries, memoryCacheEntryOptions);
    }
}

I'm not sure I fully understand the question, but the first thing that leapt to mind is:
<T> can encompass a IList<SomeType> already.

I mean, the first 75% of GetValue() already works with using IList as the generic type:
public async Task<T> GetValue<T>(
        string cacheKey,
        Func<Task<T>> getItem,
        MemoryCacheEntryOptions memoryCacheEntryOptions = null)
{
    if (_memoryCache.TryGetValue(cacheKey, out T cacheEntry))
    {
        return cacheEntry;
    }
    else
    {
        cacheEntry = await getItem();

        return SetValue(cacheKey, cacheEntry, memoryCacheEntryOptions);
    }
}

... memoryCache.TryGetValue() works exactly the same, and will operate correctly.  cacheEntry will be the correct type (an IList of something.)  getItem() works perfectly, too, since it's a Func that returns the appropriate IList.
Literally the only thing that's different is SetValue vs SetValues.  Which, from your original post, looks to be the call into the 3rd party API.
Well, it begs the question of whether SetValue() in the vendor API will work for an IList?
If it does, your probably is immediately solved: just use GetValue for everything and call it a day.
Assuming that it doesn't, maybe the question would be whether something like this would work:
public async Task<T> GetValue<T>(string cacheKey, Func<Task<T>> getItem, MemoryCacheEntryOptions memoryCacheEntryOptions = null)
{
    return GetValue(cacheKey, getItem, x => SetValue(cacheKey, x, memoryCacheEntryOptions));
}
public async Task<T> GetValues<T>(string cacheKey, Func<Task<T>> getItem, MemoryCacheEntryOptions memoryCacheEntryOptions = null)
{
    return GetValue(cacheKey, getItem, x => SetValues(cacheKey, x, memoryCacheEntryOptions));
}

private async Task<T> GetValue<T>(string cacheKey, Func<Task<T>> getItem, Func<T, Task<T>> setItem)
{
    if (_memoryCache.TryGetValue(cacheKey, out T cacheEntry))
    {
        return cacheEntry;
    }
    else
    {
        cacheEntry = await getItem();

        return setItem(cacheEntry);
    }
}


Handling various types of nodes when traversing a DOM tree How to minimize the following code using java's features ... looking for some workaround with the switch-case statement

I've seen several question regarding switch-case design pattern / best practices but because I am new to java, I am having difficulties in implementing them.

Here is the code:

protected void readTree(Node node, Branch current)
{
  Element element = null;
  Document document = null;

  if(current instanceof Element)
  {
     element = (Element) current;
  }
  else
  {
     document = (Document) current;
  }

  String nodeVal = node.getNodeValue();
  String nodeName = node.getNodeName();

  switch(node.getNodeType())
  {
  case ELEMENT_NODE:
     readElement(node, current);

     break;

  case PROCESSING_INSTRUCTION_NODE:

     if(current instanceof Element)
     {
        element.addProcessingInstruction(nodeName, nodeVal);
        break;
     }
     document.addProcessingInstruction(nodeName, nodeVal);

     break;

  case COMMENT_NODE:

     if(current instanceof Element)
     {
        element.addComment(nodeVal);
        break;
     }

     document.addComment(nodeVal);

     break;

  case DOCUMENT_TYPE_NODE:

     DocumentType domDocType = (DocumentType) node;

     document.addDocType(domDocType.getName(), domDocType.getPublicId(), domDocType.getSystemId());

     break;

  case TEXT_NODE:
     element.addText(nodeVal);

     break;

  case CDATA_SECTION_NODE:
     element.addCDATA(nodeVal);

     break;

  case ENTITY_REFERENCE_NODE:

     if(node.getFirstChild() != null)
     {
        element.addEntity(nodeName, node.getFirstChild().getNodeValue());
        break;
     }
     element.addEntity(nodeName, "");

     break;

  case ENTITY_NODE:
     element.addEntity(nodeName, nodeVal);

     break;

  default:
     System.out.println("WARNING: Unknown node type: " + node.getNodeType());
  }
}

Consider creating an object which can handle each case:

interface NodeHandler {
    void handle(Node node, Element current, ...);
}

class ElementHandler implements NodeHandler {
    public void handle(Node node, Element current, ...) {
        readElement(node, current);
    }
}

You can then populate a map, like this, to look up the appropriate handler for a node:

private static Map<Integer, NodeHandler> handlerMap;
static {
    handlerMap = new HashMap<Integer, NodeHandler>();
    handlerMap.put(Node.ELEMENT_NODE, new ElementHandler());
    //...
}

and then...

handlerMap.get(node.getNodeType()).handle(...);

I don't think this makes the code shorter, but it does make the code more maintainable. Each object has a single, clear responsibility, and you don't have to deal with issues like fall-through cases, or state getting mingled between different handlers. You could conceivably invent new handlers without actually changing the implementation of the readTree() method, or impacting the behavior of existing handlers.


1 Vase 3 Vessels game in C++ Here is the premise:

  Three women robbed a gentleman of a vase, containing 24 ounces of
  balsam. Whilst running away they met a glass-seller, of whom they
  purchased three vessels. On reaching a place of safety they wished to
  divide the booty, but found that their vessels contained 5, 11, and 13
  ounces respectively. How could they divide the balsam into equal
  portions?

  Basically you have to get the Vase = 8, Vessel2 =8, and Vessel3 = 8.

  But it has to be in this format:

Pour 1 2

Show

Pour 1 3

Show

Pour 2 4

Show

Pour 4 1

Show

...

"Show" means display the contents of the jars
"Pour 1 2" means pour as much of the contents of container 1 into container 2 as possible

The pour function should protect itself from bad commands:

Pour 1 1
Pour 0 3
Pour 1 5 // where there are only 4 jars

I finished the program but I was wondering if anyone thinks I could improve the performance or formatting, and how. In particular, the protection in the range because it kinda seems like it messes with the flow since the user no longer enters pour. They just enter the source and destination, and if they decide to enter the pour source destination, the while loop goes into space.

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

const int VASE = 0;
const int VESSEL1 = 1;
const int VESSEL2 = 2;
const int VESSEL3 = 3;

const int capacity[] = { 24, 5, 11, 13 };
const string names[] = { "VASE", "VESSEL 1", "VESSEL 2", "VESSEL 3" };
int volume[] = { 24, 0, 0, 0 };

void display(int count);
void pour(int source, int destination);
bool isEmpty(int container);
bool isFull(int container);

int main()
{
int count = 0;

cout << "----------The commands available are pour from source to destination or show the contents of all the jars.-----------" << endl;
cout << "\nFOR EXAMPLE:" << endl;
cout << "'Pour 1 2' means pour as much of the contents of container 1 into container 2 as possible." << endl;
cout << "'Show' means display the contents of the jars." << endl;

cout << "The starting contents of the jars are ";
display(count);

do
{
    int source, destination;
    string command;
    cout << "\nPLEASE ENTER COMMAND NUMBER " << count + 1<< endl;
    cin >> command;
    cin.ignore();

    if (command == "Pour" || command == "pour" || command == "POUR")
    {
        cin >> source >> destination;
        pour(source, destination);
    }
    else if (command == "Show" || command == "show" || command == "SHOW")
        display(count);
    else
        cerr << "YOU HAVE NOT ENTERED A VALID COMMAND." << endl;

    count++;
} while ((volume[VASE] != volume[VESSEL2]) || (volume[VESSEL2] != volume[VESSEL3]));
system("pause");
return 0;
}

void pour(int source, int destination)
{
    while (source < VASE || source >VESSEL3 || destination < VASE ||                  destination > VESSEL3)
{
    cerr << "You have enterered a wrong source or a wrong destination." << endl;
    cout << "Please note that the range of both source and destination is from 0-3." << endl;
    cout << "Please enter source and destination now i.e for example (1 3) " << endl;
    cin >> source >> destination;
}
while (!isEmpty(source) && !isFull(destination))
{
    volume[source]--;
    volume[destination]++;
}
}
 bool isEmpty(int container)
 {
     return volume[container] == 0;
 }
 bool isFull(int container)
 {
    return volume[container] == capacity[container];
 }

void display(int count)
{
    cout << ": ";
    for (int index = VASE; index <= VESSEL3; index++)
         cout << volume[index] <<  " : ";
    cout << endl;
}

A couple of things that could help.

A menu and a switch block will simplify the main function.

A class to represent a vessel with quantity and a state that would be empty, full or partially full, would help clean up the rest of the code.

Make the menu options a '1' or a '2'.  This way you don't have to rely on the case.

And instead of explaining to the user how to enter the parameters, simply ask them which vessel to pour from and which vessel to pour to, and validate and correct, their answers before calling the functions.

Here's an example of using a class a menu and a switch block.  Some of the functions will need to be fleshed out, but you can easily see how mush easier it is to follow the flow of the program this way:

class Vessel
{
    map<char , string> states =
    { {
        {{'E',"Empty"}},
        {{'P',"Partial"}},
        {{'F',"Full"}}
    } };
    int _quantity = 0;
    int _capacity = 0;
public:
    Vessel( int capacity );
    string GetState();
    int GetCapacity();
    int GetQuantity();
    void SetQuantity( int volume );
    void Pour( Vessel other );
};
void DisplayVessels( map<int , Vessel> vessels )
{
    cout << "The state of the vessels:\n";
    cout << setw( 4 ) << "Size\t" << setw( 5 ) << "State\t" << setw( 4 ) << "Used\n";
    int counter = 1;
    for ( auto v : vessels )
    {
        cout << counter++ << setw( 4 ) << v.second.GetCapacity() << '\t' << setw( 5 ) << v.second.GetState() << '\t' << setw( 4 ) << v.second.GetQuantity() << '\n';
    }
}
void Pour( map<int , Vessel>& vessels )
{

}
int main()
{
    map<int , Vessel> vessels =
    { {
        {{24,Vessel( 24 )}},
        {{5,Vessel( 5 )}},
        {{11,Vessel( 11 )}},
        {{13,Vessel( 13 )}}
    } };
    cout << "Welcome to the Vessel Game\n"
        << "Three women stole a vase full of balsam(24 oz.)\n"
        << "During their get away they meet a glass merchant and buy 3 vessels from him.\n"
        << "Upon reaching a safe place to divvy up their spoils, they discover that the vessels are of odd sizes(5,11, and 13 oz.)\n"
        << "Your job is to figure out how to divide the balsam into 3 vessels each containing 8 oz.\n\n";
    vessels[24].SetQuantity( 24 );
    char choice;
    bool done = false;
    while ( !done )
    {
        cout << "1. Pour\n"
            << "2. Display\n"
            << "0. Quit\n";
        cin >> choice;
        switch ( choice )
        {
            case '1':
                Pour( vessels );
                break;
            case '2':
                DisplayVessels( vessels );
                break;
            case '0':
                done = true;
                break;
            default:
                cout << "Wrong choice.\n";
                break;
        }
    }

}


Function that shortens a String based on a term/abbreviation mapping I have a function that takes a description as a String and returns a shortened version of the description.

The shortening is done by checking if a word matches the key of the HashMap<String,String> map and replaces the word by its abbreviation defined as the value in the HashMap. If the word cannot be found it in the HashMap it will not be replaced (it stays the same). If the value of a word is <remove> the word will be removed from the new description.

There can be multiple abbreviations for the same word.

Example:

  AUTOMATIC GAIN CONTROL --> AGC

  CONTROL --> CTRL

In any case the function should always take the abbreviation which takes the most words.

Consider this example mapping:

  AUTOMATIC GAIN CONTROL --> AGC

  CONTROL --> CTRL

  IDENTIFIER --> ID

  THE --> <remove>

The input String:

  The Automatic Gain Control identifier value

will result in the output String:

  AGC ID value

The String's in the HashMap are all uppercase and the abbreviation in the output String should also be uppercase. However, any word that was not found in the HashMap should stay the same as it was in the input String.

My function seems to do the task correctly but I'm not sure if it could have been written better and/or more efficiently.

private static HashMap<String,String> map = new HashMap<String, String>();

public static String shortenText(String input){

    ArrayList<String> listUpperCase = new ArrayList<String>(Arrays.asList(new String(input).toUpperCase().split(" ")));
    ArrayList<String> list = new ArrayList<String>(Arrays.asList(input.split(" ")));

    //Remove any words that map to <remove>
    for (int i=0;i<list.size();i++){
        String value = map.get(listUpperCase.get(i));
        if (value!=null){
            if(value.equals("<remove>")){
                list.remove(i);
                listUpperCase.remove(i);
            }
        }
    }
    //Replace words by their abbreviation
    StringBuilder builder = new StringBuilder();
    for (int i=0;i<list.size();i++){
        StringBuilder tempBuilder = new StringBuilder();
        String longestStringToShort="";
        int count=0;
        //Find abbreviations for terms with multiple words
        for(int k=0;k<list.size()-i;k++){
            tempBuilder.append(" "+listUpperCase.get(i+k));
            if(k==0) tempBuilder.deleteCharAt(0);
            String value = map.get(tempBuilder.toString());
            if (value!=null){
                longestStringToShort = value;
                count = k;
            }
        }
        if (!longestStringToShort.equals("")){
            builder.append(" "+longestStringToShort);
            i=i+count;
        }
        else builder.append(" "+list.get(i));

    }

    builder.deleteCharAt(0);
    return builder.toString();

}

Some observations (partly covered in @RoToRa's answer):

A placeholder String such as "<remove>" doesn't communicate at a glance whether a subsequence can be removed or not. Consider if you are scanning through your code:

map.put("Another Bored Cat", "ABC");
map.put("Another Bad Coral", "<remove>");
map.put("Another Berry Can", "ABC");

You may miss out that "Another Bad Coral" needs to be removed, purely by looking at the length of the line. It's arguably easier to read as such:

map.put("Another Bored Cat", "ABC");
map.put("Another Bad Coral", "");
map.put("Another Berry Can", "ABC");

In this case, it also removes the ambiguity (as unlikely as that may seem initially) that "Another Bad Coral" will be substituted with "<remove>".
Not particularly efficient uses for StringBuilder, especially when you builder.append(" " + aString) when you can do builder.apppend(' ').append(aString). Furthermore, you are also creating a StringBuilder for every tokenized word.
Related to the previous point, your explicit concatentation of " " characters forces you to repetitively do builder.deleteCharAt(0), which also leads to buggy derivation.
Usage of Map/HashMap can be enhanced so as to reduce the explicit looping that you are currently doing by tokenizing the input String. As you mentioned, you want to "takes the most words as the key in the HashMap", so if there's a way to actually iterate in that order from the Map...

Alternative implementation (Java 8 features ahead)

You can then use a TreeMap with a suitable Comparator implementation to persist and iterate with. For example:

private static final Comparator<String> KEY_COMPARATOR = (s1, s2) -> {
    int diff = Integer.compare(s2.length(), s1.length());
    return diff == 0 ? s1.compareToIgnoreCase(s2) : diff;
};

Or in the form of an anonymous class:

private static final Comparator<String> KEY_COMPARATOR = new Comparator<String>() {

    @Override
    public int compare(String s1, String s2) {
        int diff = Integer.compare(s2.length(), s1.length());
        return diff == 0 ? s1.compareToIgnoreCase(s2) : diff;
    }
};

This literally does what is required: if the lengths of both Strings are the same, we'll use a case-insensitive comparison between the both of them, else we simply rank the longer String to be greater than the other. This means that a Map such as:

private static final Map<String, String> MAPPER = getMapper();

private static Map<String, String> getMapper() {
    Map<String, String> mapper = new TreeMap<>(KEY_COMPARATOR);
    mapper.put("THE", "");
    mapper.put("AUTOMATIC", "AUTO");
    mapper.put("ZOOKEEPER", "ZK");
    mapper.put("AUTOMATIC GAIN CONTROL", "AGC");
    mapper.put("IDENTIFIER", "ID");
    return mapper;
}

Will give us the following desired iteration order:

// MAPPER.forEach((key, value) -> System.out.printf("%s => %s\n", key, value));
AUTOMATIC GAIN CONTROL => AGC
IDENTIFIER => ID
AUTOMATIC => AUTO
ZOOKEEPER => ZK
THE =>

With this Map implementation, the overall solution is half-complete. What is left is to replace any matching keys (as a subsequence) in the input String with the shortened value. For this, you can consider using regular expressions to perform the substitution on just one instance of StringBuilder:

private static String shorten(String input) {
    StringBuilder result = new StringBuilder(input);
    MAPPER.forEach((key, value) -> {
        Matcher matcher = Pattern.compile("(?i)\\b" + key + "\\b").matcher(result);
        while (matcher.reset().find()) {
            result.replace(matcher.start(), matcher.end(), value);
            if (value.isEmpty() && result.length() != 0) {
                result.deleteCharAt(Math.max(0, matcher.start() - 1));
            }
        }
    });
    return result.toString();
}

Use a case-insensitive ((?i)) Pattern instance to apply on the StringBuilder via Pattern.matcher(CharSequence). The \\b word boundary is important to make sure we only match on a complete expression demarcated by whitespaces.
Loop through each match, taking care to reset() the Matcher instance first.
Replace the contents of the StringBuilder instance using the results matcher.start() and matcher.end().
For cases where we are removing text, we also need to remember to delete the previous character, or the first, so long as the StringBuilder instance is non-empty.

Unit-testing

In case this is not done yet, remember to unit test this method too! For example, using a combination of TestNG and Hamcrest matchers library to perform the required assertions:

@Test
public void testInput() {
    String test = "There is the Automatic Automatic Gain Control Zookeeper Identifier";
    assertThat(shorten(test + " " + test),
                    equalsTo("There is AUTO AGC ZK ID There is AUTO AGC ZK ID"));
    // the middle whitespace is effectively trimmed away
    assertThat(shorten("the the"), equalsTo(""));
    assertThat(shorten("The Automatic Gain Control identifier value"),
                    equalsTo("AGC ID value"));
}


A Cipher with 2 methods for encryption/decryption I'd like advice on how this could be written more efficiently, idiomatically, or just anything that could be improved upon.

I'd also like to know if there's an alternative to my forString method; I couldn't find anything similar in the library (and Scalex seems to be down, so I couldn't search).

There are 2 provided ways of encrypting a message:

val message:String = "Hello World!"
val key:String = "Key"

val encrypted = Cipher(message).simpleOffset(5)
println(encrypted) "Mjqqt%\twqi&"
val decrypted = Cipher(encrypted).simpleOffset(-5)
println(decrypted) "Hello World!"

val encrypted2 = Cipher(message).encryptWith(key)
println(encrypted2) "5l?Yv;Dv?Yk<"
val decrypted2 = Cipher(encrypted2).decryptWith(key)
println(decrypted2) "Hello World!"

and the code:

class Cipher(message:String) {
    val cHARMIN = 32 //Lower bound
    val cHARMAX = 126 //Upper bound

    //"Wraps" a character, so it doesn't go under cHARMIN, or over cHARMAX
    //Takes an Int so the Char can't underflow prior to being passed in
    private def wrap(n:Int):Char =
        if (n > cHARMAX) (n - cHARMAX + cHARMIN).toChar
        else if (n < cHARMIN) (n - cHARMIN + cHARMAX).toChar
        else n.toChar

    private def forString(str:String)(f:Char => Char):String = {
        var newStr = ""
        for(c <- str) { newStr += f(c) }
        newStr
    }

    //Creates a repeated key that's at least as long as the message to be encrypted
    private def getRepKey(key:String):String =
        if (key.length >= message.length) key
            else key * (message.length / key.length + 1)

    //Modifies a letter in the message, based on the respective letter in the key,
    //    and the given binary operator
    private def modifyWithStringWith(f:(Int,Int) => Int)(strKey:String):String = {
        var newString = ""
        (message zip getRepKey(strKey)).foreach { case (c1,c2) =>
                newString += wrap( f(c1,c2) ) }
        newString
    }

    //Offsets each character in a String by a given offset
    def simpleOffset(offset:Int):String =
        forString(message) { (c:Char) =>
            wrap((c + offset).toChar)
    }

    //Encrypts/Decrypts using another String as the key
    def encryptWith(key:String):String =
        modifyWithStringWith(_+_)(key)

    def decryptWith(key:String):String =
        modifyWithStringWith(_-_)(key)
}

object Cipher {
    def apply(msg:String):Cipher = new Cipher(msg)
}

forString

As discussed already in comments, your forString method is not needed because map is available to Scala Strings.  But if you do need to construct such a method, your design is not best practice.  Specifically, you create a local var, append to it repeatedly from within a for block and return that.

This block

    var newStr = ""
    for(c <- str) { newStr += f(c) }
    newStr

can be written simply as

    for (c <- str) yield f(c)

Updating an external var from within a for block is a stateful, potentially error-prone, very non-FP practice which you should use with care.  It is not needed here.  Leave the compiler to construct and return the string - it will be more efficient and your code is cleaner as a result.

(It will be more efficient because in the for-yield block, the compiler knows it is creating a sequence one item at a time and begins each step with a ready pointer to the end of the collection.  In contrast, when your code explicitly appends to the end of a sequence each time, it has to traverse the entire sequence each time.  No handy reference to the end of the string is cached. Appending to strings is expensive.)

wrap

Both the original version and the modulo alternative in your answer use if... else if... then chains.  These are a bad smell.  All three of your options depend upon the value of the input parameter, but nothing about an if-elseif-then chain forces you to cite that parameter in each successive condition.  Pattern matching would at least ensure that the same object is being assessed for each action.  That would look something like this:

n match {
    case _ if (n > cHARMAX) => (cHARMIN + (n - cHARMAX) % cRANGE)
    case _ if (n < cHARMIN) => (cHARMAX - (cHARMIN - n) % cRANGE)
    case _ => n
}

However, although I am still not sure why you have this function in there, I think this is what you are trying to do:

n match {
    case _ if (n > cHARMAX) => cHARMIN + ((n - cHARMIN) % cRANGE)
    case _ if (n < cHARMIN) => cHARMAX - ((cHARMIN - n) % cRANGE)
    case _ => n
}

Key length

Repeating the key until it is as long as the message is a waste of resources (more so, the longer the message becomes).  Just leave the key in its original size.  There are several ways to iterate through the message and (effectively or literally) use modulo arithmetic to fetch the corresponding character from the key.

Create an endlessly repeating Stream from the key (fast and efficient).  Then you can just iterate through both in step (for example, using zipped)
for (i <- message.indices) yield myFunction(message(i), key(i % key.length)) (reasonably fast and efficient)
Use zipWithIndex and map to turn the message into a list of tuples (each character bundled with its index) and then iterate through that, using modulo arithmetic on the index to fetch the right key character (less efficient, slower).

Option 1 is best

Cipher class design

The most curious thing about your Cipher class is that its Apply method takes a message as a parameter.  This is not good OO design and it also inverts the meaning of the word (OK, cipher can mean an encrypted text but more commonly it means one particular encryption method with one particular key choice).  It would make more sense for a cipher object to have the key (and possibly some other parameters like, for example, the range of valid characters) as a field.  So you could create a cipher instance containing the key "MyAmazingSecretCode" and then use that object to encrypt or decrypt as many messages as you want.  As things stand, Cipher is mostly a java-like utility class containing static methods, offering no way to maintain any state.

Having the two different encryption options in one class is also less good.  Are you going to rewrite this class every time you add another variant on the encryption method?

Consider, instead, a basic Cipher class or trait or even interface, which describes the key interface methods (setting the secret, encrypting, decrypting).  If it is a class or trait, you could also design an "encryption strategy" trait from which specific encryption methods (your simple offset and your substitution key, as two examples) could be derived.  You could then mix the appropriate strategy trait into the Cipher class to provide a working cipher.  If you choose to make Cipher an interface, then your two strategies could be separate classes which each incorporate that interface.  Which to choose depends on larger design considerations.


Fast string formatting in for loop I have a function that loops through large objects of raw data, translating coded strings into readable words. The function is very basic, and I keep wondering if I should use an object instead containing the words to be detected and translated. This would be cleaner-looking and more flexible, but it would not like it to harm the performance of the script, since it needs to run through potentially thousands of items.

function formatTitle(input) {
    var input_lowercase = input.toLowerCase()
    if (input == "total") {
        return "Title"
    } else if ( input_lowercase == "type" || input_lowercase == "dokumenttype" ||  input_lowercase == "km_type"  ) {
        return label_documenttype
    } else if ( input_lowercase.indexOf("tema_") >= 0 || input_lowercase == "spesfikasjon" ) {
        return label_theme
    } else if ( input_lowercase.indexOf("prosess_") >= 0) {
        return label_process
    } else if (input.indexOf("SM_") >= 0) {
        return input.substr(3);
    }
    else {
        return input.charAt(0).toUpperCase() + input.slice(1)
    }
}

There are more exceptions in the list, I've shortened it a bit down for practical reasons. I use the function in the following context (when t_formatheader is switched on):

    if ( skipping_mechanism == "lookup" ) { //42% faster than switch block in IE
        for (var key in arr_firstnode) {
            if (key in keysToIgnore) continue;
            if ( t_formatheader == "1" ) {
                arrayPushUnknown( columns, formatTitle( key ), "", true, key )
            }
            else {
                arrayPushUnknown( columns, key, "", true, key )
            }
        }
    }

I think you need a mix of lookup object(s) and regular if statements; there are clearly direct matches, matches on the prefix and the rest. Also, to write else if is pointless since you return anyway in previous if checks, so you can simply use

if(condition){
  return value
}
if(condition2){ // <- no else here
  return value2
}

All that together you could consider something like this:

var formatTitleDirectMapping = {
  'total' : 'title',
  'type' : label_documenttype,
  'dokumenttype' : label_documenttype,
  'km_type' : label_documenttype,
  'spesfikasjon' : label_theme,
}

var formatTitlePrefixMapping = {
  'tema_' : label_theme,
  'prosess_' : label_process,
  'dokumenttype' : label_documenttype,
  'km_type' : label_documenttype,
}

function formatTitle(input) {
  var lower = input.toLowerCase();
  //Is there a straight forward conversion? Then get out
  if( formatTitleDirectMapping[lower] )
    return formatTitleDirectMapping[lower];
  //Is there a straight forward conversion for the prefix? Then get out
  var prefix = lower.split("_")[0];
  if( formatTitlePrefixMapping[prefix] )
    return formatTitlePrefixMapping[prefix];
  //Put the conversions that dont lend themselves to lookup structures here
  if( prefix == "sm") {
    return input.substr(3);
  }
  //Catch all
  return input.charAt(0).toUpperCase() + input.slice(1)
}


YouTube Search Result Scraper This is a program I wrote in Python using the BeautifulSoup library. The program scrapes YouTube search results for a given query and extracts data from the channels returned in the search results.

I'm just looking for some tips on how to make my code look (and function) better. I removed most of the redundancies but to me the code still feels ugly.

Suggestions?

#!/usr/bin/python
# http://docs.python-requests.org/en/latest/user/quickstart/
# http://www.crummy.com/software/BeautifulSoup/bs4/doc/

import csv
import re
import requests
import time
from bs4 import BeautifulSoup

# scrapes the title
def getTitle():
    d = soup.find_all("h1", "branded-page-header-title")
    for i in d:
        name = i.text.strip().replace('\n',' ').replace(',','').encode("utf-8")
        f.write(name+',')
        print('\t\t%s') % (name)

# scrapes the subscriber and view count
def getStats():
    b = soup.find_all("li", "about-stat ") # trailing space is required.
    for i in b:
        value = i.b.text.strip().replace(',','')
        name = i.b.next_sibling.strip().replace(',','')
        f.write(value+',')
        print('\t\t%s = %s') % (name, value)

# scrapes the description
def getDescription():
    c = soup.find_all("div", "about-description")
    for i in c:
        description = i.text.strip().replace('\n',' ').replace(',','').encode("utf-8")
        f.write(description+',')
        #print('\t\t%s') % (description)

# scrapes all the external links
def getLinks():
    a = soup.find_all("a", "about-channel-link ") # trailing space is required.
    for i in a:
        url = i.get('href')
        f.write(url+',')
        print('\t\t%s') % (url)

# scrapes the related channels
def getRelated():
    s = soup.find_all("h3", "yt-lockup-title")
    for i in s:
        t = i.find_all(href=re.compile("user"))
        for i in t:
            url = 'https://www.youtube.com'+i.get('href')
            rCSV.write(url+'\n')
            print('\t\t%s,%s') % (i.text, url)

f = open("youtube-scrape-data.csv", "w+")
rCSV = open("related-channels.csv", "w+")
visited = []
base = "https://www.youtube.com/results?search_query="
q = ['search+query+here']
page = "&page="
count = 1
pagesToScrape = 20

for query in q:
    while count <= pagesToScrape:
        scrapeURL = base + str(query) + page + str(count)
        print('Scraping %s\n') %(scrapeURL)
        r = requests.get(scrapeURL)
        soup = BeautifulSoup(r.text)
        users = soup.find_all("div", "yt-lockup-byline")
        for each in users:
            a = each.find_all(href=re.compile("user"))
            for i in a:
                url = 'https://www.youtube.com'+i.get('href')+'/about'
                if url in visited:
                    print('\t%s has already been scraped\n\n') %(url)
                else:
                    r = requests.get(url)
                    soup = BeautifulSoup(r.text)
                    f.write(url+',')
                    print('\t%s') % (url)
                    getTitle()
                    getStats()
                    getDescription()
                    getLinks()
                    getRelated()
                    f.write('\n')
                    print('\n')
                    visited.append(url)
                    time.sleep(3)
        count += 1
        time.sleep(3)
        print('\n')
    count = 1
    print('\n')
f.close()

I'm pretty much a n00b to programming myself, so take my advice with a grain of salt... but I would try making each of your "get..." functions into a method of a class (let's say YoutubeVid). It's __init__ would set all the attributes at once, without printing. A seperate function, let's say print_attributes could do the printing. Once you code that part, you would replace this:

            else:
                r = requests.get(url)
                soup = BeautifulSoup(r.text)
                f.write(url+',')
                print('\t%s') % (url)
                getTitle()
                getStats()
                getDescription()
                getLinks()
                getRelated()
                f.write('\n')
                print('\n')
                visited.append(url)
                time.sleep(3)

With something like this:

            else:
                r = requests.get(url)
                soup = BeautifulSoup(r.text)
                video_page = YoutubeVid(soup)
                print_attributes(video_page)

I'm sorry I don't have the time to work out a more detailed example, but if that makes any sense to you, maybe you can give it a try and post what you come up with.

Also, a minor detail regarding function names... Mixed case like getTitle() is depricated. Lowercase with underscores like get_title() is prefered. See the PEP Style Guide.


Pascal's triangle in PHP Are there any optimization suggestions?

<?php
# Pascal's Triangle using Recursion.
$rows_to_generate = 15; // Define number of rows you want to generate.
$storage = array();
function get_elem($row,$pos)    // generate element having number of row and number of position.
{
    global $storage;

    if(isset($storage[$row][$pos]))
    {
        return $storage[$row][$pos];
    }

    if($row==1 and $pos==1) // for first row.
    {
        return 1;
    }
    else if($pos==0)    // for left most row element that doesn't exist.
    {
        return 0;
    }
    else if($pos>$row)  // for right most row element that doesn't exist.
    {
        return 0;
    }
    return get_elem($row-1,$pos-1)+get_elem($row-1,$pos);   // Recursion
}
?>

<?php
for($i=1;$i<=$rows_to_generate;$i++)
{
    for($j=1;$j<=$i;$j++)
    {
        echo $storage[$i][$j]=get_elem($i,$j);  // generate
        if($i!=$j)
        {
            echo ' ';
        }
    }
    echo '
';
}
?>

These conditions can be simplified:

if($row==1 and $pos==1) // for first row.
{
    return 1;
}
else if($pos==0)    // for left most row element that doesn't exist.
{
    return 0;
}
else if($pos>$row)  // for right most row element that doesn't exist.
{
    return 0;
}

To this:

if ($pos == 1 or $row == $pos)
{
    return 1;
}

Coding style

The expressions are too packed.
It would be better to add more spaces:

around operators
after semicolons
after commas in parameter lists
before opening ( of if conditions and for loops

So instead of this:

for($i=1;$i<=$rows_to_generate;$i++)
{
    for($j=1;$j<=$i;$j++)
    {
        echo $storage[$i][$j]=get_elem($i,$j);  // generate
        if($i!=$j)

This is easier to read:

for ($i = 1; $i <= $rows_to_generate; $i++)
{
    for ($j = 1; $j <= $i; $j++)
    {
        echo $storage[$i][$j] = get_elem($i, $j);  // generate
        if ($i != $j)


Is it a good practice to put an exception handler in every method? I always add an exception handler when a new method is created, and I don't know if it is a good or bad practice.  I thought it may be a bit of an annoyance when viewing the source code.

Is it a good practice to put logging functions in the exception block?

def insert_new_camera(self, camera, records, file_hdlr):
    try:
        pass
    except BaseException  as e:
        traceback.print_exc(file=sys.stdout)
        raise e

Don't do that.

If you log your exceptions at every function, you'll print the same exception over and over and over again. Its only useful the first time, after that its a pain.

It will also make your code much harder to read and write.

You should log the exception when you would otherwise lose it. At some point, your exception has to be caught and not rethrown. This may happen in the default exception handler. That's where you should log. Don't log exceptions that you just rethrow. Trust whoever catches them to log them.


Am I using Q the right way or is there a better way to do what I'm doing? I'm using Q to flatten out some callbacks in my unit tests and return a promise to mocha (which knows to wait until the promise is resolved before running the next test).

Originally I had this code:

it ('resetCount should cause the count to reset as if there were no documents yet.', function () {
    // Now save user and check if its _id is what nextCount said.
    user.save(function (err) {
        should.not.exists(err);
        user._id.should.eql(0);

        // Call nextCount to check the next number. Should be one.
        user.nextCount(function (err, count) {
            should.not.exists(err);
            count.should.eql(1);

            // Now reset the count.
            user.resetCount(function () {
                // Call nextCount again to check that the next count is reset. Should be zero.
                user.nextCount(function (err, count) {
                    should.not.exists(err);
                    count.should.eql(0);
                    done();
                });
            });
        });
    });
});

With Q I turned it into this:

it('resetCount should cause the count to reset as if there were no documents yet.', function () {
    var userSchema = new mongoose.Schema({
        name: String,
        dept: String
    });
    userSchema.plugin(autoIncrement.plugin, 'User');
    var User = db.model('User', userSchema);

    // Create user and save it.
    var user = new User({name: 'Charlie', dept: 'Support'});
    // Now save user and check if its _id is what nextCount said.
    q.nmcall(user, 'save')
        .then(function (user) {
            user._id.should.eql(0);
            return q.nmcall(user, 'nextCount');
        })
        .then(function (count) {
            count.should.eql(1);
            return q.nmcall(user, 'resetCount');
        })
        .then(q.nmcall(user, 'nextCount'))
        .then(function (count) {
            count.should.eql(0);
        }, function (err) {
            should.not.exists(err);
        });

});

I no longer have to call the done callback because of how mocha understands to wait for the promise to resolve. I am new to Q so I'm wondering if I've done things the most efficient way. It seemed like calling node-style mongoose methods with q.nmcall (alias for q.invoke) made the most sense. However, if you think you could write the same code better then please let me know where I can improve it.

.then(q.nmcall(user, 'nextCount'))

is wrong. The nextCount call does return a promise (and starts its execution right away), while then does expect a [callback] function. You'd rewrite it to

.then(function(result_of_resetcount) {
    return q.nmcall(user, 'nextCount');
})

or shorten it with nfbind/nbind:

.then(q.nbind(user.nextCount, user))


Scoring system of a pool of dice I want to do some Monte Carlo simulations on a system based on a pool of dice. For each trial, a number of dies are rolled, and the winner is the highest value, getting as many points as dice are over the second, including ties.

Examples:

[1, 2], [5, 1]  One point for the second (5 is the only value over 2).
[20, 12, 20, 14], [17, 15, 19, 18] Two points for the first one (two 20 over 19).
[20, 12, 20, 14], [17, 15, 20, 18] Two points for the first one (one tie at 20, one die over 18).

Here is my algorithm. Can you think of a better way?

from collections import Counter
import numpy as np

def check_rolls(one, other):
    one_c = Counter(one)
    other_c = Counter(other)
    ties = Counter()

    # Remove ties:
    for tie_val in set(one_c.keys()).intersection(other_c.keys()):
        toremove = min(one_c[tie_val], other_c[tie_val])
        one_c[tie_val] -= toremove
        other_c[tie_val] -= toremove
        ties[tie_val] = toremove

        # Clear if 0
        if one_c[tie_val] == 0:
            del one_c[tie_val]
        if other_c[tie_val] == 0:
            del other_c[tie_val]

    max_one = max(one_c.keys()) if one_c else 0
    max_other = max(other_c.keys()) if other_c else 0

    winner = max_one > max_other
    threshold = min(max_one, max_other)

    if winner:
        # One wins
        result = sum(val for key, val in one_c.iteritems()
                   if key > threshold) + \
                   sum(val for key, val in ties.iteritems()
                       if key >= threshold)
    else:
        result = -(sum(val for key, val in other_c.iteritems()
                    if key > threshold) +
                    sum(val for key, val in ties.iteritems()
                        if key >= threshold))
    return result

def roll(d_player, d_gm):
    return check_rolls(np.random.randint(1, 21, size=d_player),
                       np.random.randint(1, 21, size=d_gm))

def test():
    assert check_rolls([1, 1, 2], [1, 1]) == 3
    assert check_rolls([1, 2, 4, 5], [3, 4]) == 2
    assert check_rolls([1, 2, 4, 5], [3, 5]) == 2
    assert check_rolls([1, 2, 4, 5], [3, 6]) == -1
    assert check_rolls([20, 12, 20, 14], [17, 15, 20, 5, 18, 2]) == 2
    assert check_rolls([20, 10, 2, 4], [ 5, 20, 6, 4, 19, 3]) == -2

I am just using it for a quick and dirty script plotting histograms for different configurations. Performance improvements are welcomed, but they are not critical. Style and integration are of low priority.

Disclaimer : this is not quite an answer but more than a comment.

You've written tests and I'd like to start by thanking your for this. This definitly makes reviewing easier.

Good think to know if that you can make easily double the amount of test you have without thinking too much.

Indeed, it seems like by inversing the parameters of your function, the return value should be the opposite value.

You can write this :

for d1, d2, res in [
        ([1, 1, 2], [1, 1],  3),
        ([1, 2, 4, 5], [3, 4],  2),
        ([1, 2, 4, 5], [3, 5],  2),
        ([1, 2, 4, 5], [3, 6],  -1),
        ([20, 12, 20, 14], [17, 15, 20, 5, 18, 2],  2),
        ([20, 10, 2, 4], [ 5, 20, 6, 4, 19, 3],  -2)
    ]:
    assert check_rolls(d1, d2) == res
    assert check_rolls(d2, d1) == - res

Also, I thought I'd take this chance to test check_rolls(d1, d1) and it seems like it always returns - len(d1). Shouldn't you handle perfect tie situation in a different way ?

Edit : still not quite a review but you got me interested and I wondered if this could be written in a more simple way. Here's what I got translating my understanding of the problem into Python code :

def check_rolls(d1, d2):
    c1, c2 = Counter(d1), Counter(d2)
    res = 0 # nb points
    winner = 0 # player winning so far : 1 if 1, 2 if 2, 0 otherwise
    for key in reversed(sorted(set(c1) | set(c2))):
        v1, v2 = c1[key], c2[key]
        if v1 == v2:
            res+= v1 # ties count for whoever is winning
        else:
            local_winner = 1 if v1 > v2 else 2
            if winner and local_winner != winner:
                break
            else:
                winner = local_winner
                res+= max(v1, v2)
    return res if winner == 1 else -res


Processing command line parameters This code works, but I'm curious what feedback you have about style and doing things in a more "pythonic" way.

def printUsage():
    print("Usage: goodproxy.py -f <proxyfile> -u <url> -t <timeout>")

def main(argv):

    filename    = ""
    url         = ""
    timeout     = None

    try:

        # USE GETOPS FOR EASIER PARAMETER PROCESSING
        opts, args = getopt.getopt(argv, "f:u:t:",["file=", "url=", "timeout="])

    except getopt.GetoptError:

        printUsage()
        sys.exit(2)

    # CHECK FOR MISSING PARAMETERS
    # THE 'OPTS' OBJECT CONSISTS OF A LIST OF TUPLES LIKE (PARAM,VAL)
    # LIST COMPREHENSION COLLECTS FIRST ELEMENTS FROM EACH TUPLE INTO A NEW LIST
    # THEN TESTS THE NEW LIST FOR THE REQUIRED PARAMS

    if not any(f in [opt[0] for opt in opts] for f in ['-f','--file']):
        printUsage()
        print("Error: -f parameter missing")
        sys.exit(2)

    if not any(u in [opt[0] for opt in opts] for u in ['-u','--url']):
        printUsage()
        print("Error: -u parameter missing")
        sys.exit(2)

    if not any(t in [opt[0] for opt in opts] for t in ['-t','--timeout']):
        printUsage()
        print("Error: -t parameter missing")
        sys.exit(2)

    # CONFIGURE SETTINGS BASED ON THE PASSED IN PARAMETERS
    for opt, arg in opts:

        if opt in ('-f', '--file'):

            print("Using proxies in: " + arg)
            filename = arg

        elif opt in ('-u', '--url'):

            print("Using URL: " + arg)
            url = arg

        elif opt in ('-t', '--timeout'):

            print("Timeout: {0}s".format(arg))
            timeout = arg

if __name__ == "__main__":
    main(sys.argv[1:])

I also found PyLint to assist with improving styling and code smells.

Using argparse, your script can be greatly simplified:

from argparse import ArgumentParser

def main():
    parser = ArgumentParser(description='TODO')
    parser.add_argument('-f', '--file', metavar='PROXYFILE', required=True)
    parser.add_argument('-u', '--url', required=True)
    parser.add_argument('-t', '--timeout', required=True)
    args = parser.parse_args()

    print("Using proxies in: " + args.file)
    print("Using URL: " + args.url)
    print("Timeout: {0}s".format(args.timeout))

if __name__ == "__main__":
    main()


Possibly bubble sort algorithm I'm trying to figure out what to call this sorting algorithm:

function sort(array) {
  array = array.slice();

  for (let i = 0; i < array.length; i++) {
    for (let j = 0; j < array.length - 1; j++) {
      if (array[j] > array[i]) {
        //swap
        [array[i], array[j]] = [array[j], array[i]]
      }
    }
  }

  return array;
}

console.log(sort([8, 4, 5, 2, 3, 7]));

I wrote it while trying to figure out bubble sort which is a lot different. Tho will have slightly the same running time as the actual bubble sort. I might be wrong :(

To me, that's exactly Bubblesort: it takes care the largest element moves to the end of the array, and then operates on length-1 elements.

Edit: this does look quite similar to Bubblesort, but - as a diligent reader noticed - is not quite Bubblesort, as the algorithm does not compare (and swap) adjacent elements (which indeed is the main characteristic of Bubblesort). If you replace array[j] > array[i] with array[j] > array[j+1], you will get Bubblesort.

This implementation will fail if less than two input elements are given (0 or 1) - hint: the array is already sorted in these cases (just add an if).

A small improvement would be to add a flag in the i loop which records if any swapping happened at all - the outer for loop may terminate if the inner loop didn't perform any swaps. (Time) performance of Bubblesort is considered to be awful in comparison to other algorithms, but it must be noted it's the fastest algorithm on an already sorted array - if you add that flag ;)


How to avoid mounting SQL by concatenating strings? I found myself mounting a query by concatenating strings. This just seems wrong. Any ideas of how to avoid this kind of situation?

I'm using Rails 4, and I don't see how can Arel make things better for my case.

 def self.search(params)
    # query example
    # SELECT "jobs".* FROM "jobs"  WHERE (telecommute = 'f' AND ( freelance = 't' ) AND country = 'German')
    query = 'telecommute = ?'
    args = [params[:telecommute] == 'true']

    work_types = %W(full_time contractor freelance)
    accepted_work_types = params.find_all{ |k,v| work_types.include?(k) }.map{ |k,v| k }
    if accepted_work_types.any?
      query += " AND ( #{accepted_work_types.map{ |wt| "#{wt} = ?"}.join(" OR ")} )"
      args += accepted_work_types.map{ |wt| true }
    end

    [:city, :state, :country].each do |index|
      unless params[index].blank?
        query += " AND #{index} = ?"
        args << params[index]
      end
    end

    Job.where(query, *args)
  end

Yeah, that does feel kinda wrong. But OR queries in Rails are tricky.

There are two things here: One is creating the query, the other is parsing the params.

I'd probably start1 by preparing/parsing the params so building the query is cleaner. I'd add a method - or several - to the controller, rather than the model, to do this. Both model and controller are involved in this, but I'd rather not pass params to the model. Things like checking the string value of the telecommute param seems like a job for the controller, whereas the model's search method should only have to deal with a boolean. For instance, you might want to call Job.search from code, where it'd be silly to use string values.

There's some cleanup you can do here too. For instance, I'd strongly consider adding constants or methods for the work_types array and the location keys.

I'd also be more consistent; in one place you use %w(...) to make an array of (string) keys, and in another you use [...] to make an array of (symbol) keys.

Here's an example, if you were to just use one method:

def search_params
  search_terms = {}

  # put the "AND" terms in the hash
  search_terms[:telecommute] = params['telecommute'] == 'true'

  Job.LOCATION_KEYS.each_with_object(search_terms) do |key, memo|
    memo[key] = params[key] if key.present?
  end

  # put the "OR" terms in a nested hash
  Job.WORK_TYPES.each_with_object(search_terms) do |key, memo|
    if params[key].present?
      memo[:work_types] ||= {}
      memo[:work_types][key] = true
    end
  end

  search_terms
end

It ain't pretty, but at least it's all in one place.

As for the searching itself, you can use regular ActiveRecord's query interface without descending to Arel:

def self.search(hash = {})
  # we'll be modifying the hash, so work on a dup
  # (you'll first want to check if it's empty, though!)
  terms = hash.dup

  # grab the work types
  work_types = terms.delete(:work_types)

  # add the AND clauses
  query = self.where(terms)

  # add the OR clauses (if any)
  if work_types
    subquery = self.where(work_types).where_values.inject(:or) # magic!
    query = query.where(subquery)
  end

  query
end

I get this:

Job.search(telecommute: true, country: "US", work_types: { full_time: true, freelance: true }).to_sql
# => "SELECT "jobs".* FROM "jobs"  WHERE "jobs"."telecommute" = 't' AND "jobs"."country" = 'us' AND (("jobs"."full_time" = 't' OR "jobs"."freelance" = 't'))"

1 Edit: Actually, that's not where I'd start if I were writing this from scratch. It is however where I started my rewrite, but that's because it was already there. Otherwise, I'd start by defining the search method as I'd want it to look and work - without worrying about the params -, and then worry about how to make the params fit.


Java Slick StateBasedGame managing resources across states I was wondering if it's normal/efficient to have this many static collections in my state based game?
I started doing it for ease of access but it looks sloppy to me and I was just wondering what conventions other people use to maintain references to game objects?

public class DungeonDigger extends StateBasedGame {
    public static NetworkPlayer myCharacter;
    public static Server SERVER = new Server();
    public static Client CLIENT = new Client();
    public static int CHOSEN_GAME_STATE;
    public static final int MAINMENU = 0;
    public static final int LOBBY = 3;
    public static final int SINGLEPLAYERDUNGEON = 1;
    public static final int MULTIPLAYERDUNGEON = 2;
    public static String ACCOUNT_NAME;
    public static String IP_CONNECT;
    public static int MAX_MESSAGE_LENGTH = 50;
    public static ConnectionState STATE;
    public static HashMap<String, Image> IMAGES = new HashMap<String, Image>();
    public static ArrayList<TextPacket> TEXTS = new ArrayList<TextPacket>();
    public static LinkedList<ChatPacket> CHATS = new LinkedList<ChatPacket>();
    public static HashMap<String, NetworkPlayer> CHARACTERBANK = new HashMap<String, NetworkPlayer>();
    public static HashSet<String> ACTIVESESSIONNAMES = new HashSet<String>();
    public static HashMap<Integer, String> KEY_BINDINGS = new HashMap<Integer, String>();
    public static HashMap<String, String> SLOT_BINDINGS = new HashMap<String, String>();
    public static HashMap<String, Ability> ABILITY_TEMPLATES = new HashMap<String, Ability>();
    public static HashMap<String, Mob> MOB_TEMPLATES = new HashMap<String, Mob>();
    public static Vector<Ability> ACTIVE_ABILITIES = new Vector<>();
    public static AbilityFactory ABILITY_FACTORY = new AbilityFactory();
    public static MobFactory MOB_FACTORY = new MobFactory();

    public DungeonDigger(String title) {
        super(title);
    }

    // Start game
    public static void main(String[] args) {
        try {
            System.setProperty("org.lwjgl.librarypath", new File(System.getProperty("user.dir"), "natives").getAbsolutePath());
            System.setProperty("net.java.games.input.librarypath", System.getProperty("org.lwjgl.librarypath"));
            AppGameContainer app = new AppGameContainer(new DungeonDigger("Dungeon Digger"));
            app.setDisplayMode(640, 640, false);
            app.setTargetFrameRate(40);
            app.setUpdateOnlyWhenVisible(false);
            app.setAlwaysRender(true);
            app.start();
        } catch (SlickException e) {
            e.printStackTrace();
        }
    }

Overall, separate your code, make it modular. Store state where it needs to be stored, but keep it separate, and expose the minimum amount of functionality that you have to.

I would highly recommend that you read the book Effective Java, and then read it again. Feedback below, separate thoughts separated by . . .

. . .

First, don't use ints for storing flags, use enums.

public static int CHOSEN_GAME_STATE;
public static final int MAINMENU = 0;
public static final int LOBBY = 3;
public static final int SINGLEPLAYERDUNGEON = 1;
public static final int MULTIPLAYERDUNGEON = 2;

What does (MAINMENU - LOBBY * SINGLEPLAYERDUNGEON / MULTIPLAYERDUNGEON) mean? It's nonsense, but it's a valid expression in your API.

Instead, it would be much better annotated as:

enum GameState {
    MAIN_MENU, LOBBY, SINGLEPLAYER_DUNGEON, MULTIPLAYER_DUNGEON
}
private static GameState sCurrentGameState = GameState.MAIN_MENU;

. . .

With

public static HashMap<String, Image> IMAGES = new HashMap<String, Image>();

You have no control over who modifies your images. Instead I'd make your images stored and loaded via their own structure, something like:

class ImageCache {
    private HashMap<String, Image> mImages = new HashMap<String, Image>();

    public Image getImage(String imageName) {
        Image result = mImages.get(imageName); //returns null if image not present.
        if (result == null) {
             result = Image.loadFromFile(imageName);
             mImages.put(imageName, result);
        }
        return result;
    }

    public boolean imageCached(String imageName) {
        return sImages.contains(imageName);
    }
}

This will give you a lot of things. First, let's say that you have too many images to store in memory, now you can easily implement an LRU cache or something to load on demand. And better yet, so long as getImage and imageCached work as they did before, this will all be hidden from the people using the ImageCache.

. . .

Instead of instantiating factories, I would recommend that you use static factories. So instead of (assuming MobFactory returns Mob objects):

public static MobFactory MOB_FACTORY = new MobFactory();

You would have something like:

class Mob {
    //Private ctor means that nobody can instantiate this directly.
    private Mob(String name, int hp, int mp) { ... }

    //Create the Ability, give it to the entity
    public static Mob create(String name, int hp, int mp) {
        assert hp > 0 : "Tried to create a mob with " + hp + " HP!";
        assert mp >= 0 : "Tried to create a mob with " + mp + " MP!";
        Mob mob = new Mob(name, hp, mp);
        //optionally manipulate the mob further
        mob.giveAbility(Ability.DANCE);
        //...
        return mob;
    }
}

You could be even more clever, and roll your templates into this, so that they are entirely hidden from the user.

. . .

Don't use Vector, it is a concurrent data structure. Since it doesn't look like you are using concurrency, stick with ArrayList.

. . .

I find it's useful to have a static settings class for things like MAX_MESSAGE_LENGTH. That way it's easy to figure out where constants are defined, and to save/load them from disk.
. . .


Using a cronjob on node for storing a json file, parsing it and update a database I need to retrieve a file from a website every minute that contains boats location.

Then, I parse the file, delete all the boats from my database and re-create all of them at the new position indicated in the file.

Here's my code so far - in the server.js of my loopbackjs app:

var CronJob = require('cron').CronJob;
var exec = require("child_process").exec;
var reload = require('require-reload')(require);
var app = module.exports = loopback();

// Bootstrap the application, configure models, datasources and middleware.(loopbackjs mandatory code)
boot(app, __dirname, function (err) {
    if (err)
        throw err;

    var Boat = app.models.Boat;
    var ais_url_france = 'http://data.aishub.net/ws.php?username=XXXXXXXXX&format=1&output=json&compress=0&latmin=40&latmax=52&lonmin=-5&lonmax=10';

    //start the cronjob on a node env every min
    new CronJob('* * * * *', function () {
                // get the file and save as a .json
                exec('wget "' + ais_url_france + '" -O aishub_france.json', function (error, stdout, stderr) {
                    // I need to use reload here because the file is stored in memory otherwise and only read once after the first download...
                    var file = reload('../aishub_france.json');
                    if (file[0].ERROR === false) {
                        var boats = file[1];
                        if (boats.length > 0) {
                            Boat.destroyAll({typeId: 'aishub_boats'}, function (err, info) {
                                for (var i = 0; i < boats.length; i++) {
                                    Boat.create({
                                        name: boats[i].NAME,
                                        latlng: {
                                            lat: boats[i].LATITUDE,
                                            lng: boats[i].LONGITUDE
                                        },
                                        typeId: 'aishub_boats'
                                    });
                                }
                            });
                        }
                    }
                });
            }, null, true, null);
});

I'm not sure the code here is so great but here are my troubles:

The downloaded file is huge (1,3Mo)
I need to remove and re-add all the boats because they change location and some might appear/disappear (maybe there is a better solution)
Not sure using require-reload is the best solution, but if I don't the file is read only once in the cronjob even if the donwloaded file changed each time...
I think I might have some memory leak because I'm having strange stacks trace recently after I added this code. Here is the trace:

33503717 ms: Mark-sweep 1288.0 (1403.6) -> 1288.0 (1404.6) MB, 672.5 / 0.0 ms [allocation failure] [GC in old space requested].

33504750 ms: Mark-sweep 1288.0 (1404.6) -> 1288.0 (1404.6) MB, 847.3 / 0.0 ms [allocation failure] [GC in old space requested].

33505597 ms: Mark-sweep 1288.0 (1404.6) -> 1288.0 (1403.6) MB, 846.4 / 0.0 ms [last resort gc].

33506438 ms: Mark-sweep 1288.0 (1403.6) -> 1288.0 (1403.6) MB, 841.4 / 0.0 ms [last resort gc].

OK, thank you James!

I was able to get rid of require-reload using request, and use the compression with adm-zip on the fly:

var CronJob = require('cron').CronJob;
var request = require('request');
var AdmZip = require('adm-zip');

var ais_url_france = 'http://data.aishub.net/ws.php?username=XXX&format=1&output=json&compress=1&latmin=40&latmax=52&lonmin=-5&lonmax=10';
new CronJob('* * * * *', function () {
     request({url: ais_url_france, encoding: null}, function (error, response, data) {
            if (!error && response.statusCode === 200) {
                var zip = new AdmZip(data);
                var zipEntries = zip.getEntries();
                if (zipEntries && zipEntries.length) {
                    var file = JSON.parse(zip.readAsText(zipEntries[0]));
                    if (file[0].ERROR === false) {
                        var boats = file[1];
                        if (boats.length > 0) {
                            Boat.destroyAll(function (err, info) {
                                for (var i = 0; i < boats.length; i++) {
                                    Boat.create({
                                        name: boats[i].NAME,
                                        latlng: {
                                            lat: boats[i].LATITUDE,
                                            lng: boats[i].LONGITUDE
                                        }
                                    });
                                }
                            });
                        }
                    }
                }
            }
        });
    }, null, true, null);


Fractional knapsack implementation in Python def get_optimal_value(capacity, weights, values):
    value = 0.
    remaining_capacity = capacity
    # compute value per unit
    values_per_unit = [v / float(w) for w, v in zip(weights, values)]
    while remaining_capacity > 0:
        value_and_index = get_max_value_per_unit(values_per_unit)
        replace_max_value(values_per_unit, value_and_index["index"])

        weight = weights[value_and_index["index"]]
        value_per_unit = value_and_index["value"]

        if remaining_capacity >= weight:
            remaining_capacity = remaining_capacity - weight
            value = value + (weight * value_per_unit)
        elif remaining_capacity < weight:
            value = value + (min(remaining_capacity, weight) * value_per_unit)
            acceptable_capacity = min(remaining_capacity, weight)
            remaining_capacity = remaining_capacity - acceptable_capacity
    return value

def get_max_value_per_unit(values_per_unit):
    idx = values_per_unit.index(max(values_per_unit))
    value = values_per_unit[idx]
    return {'value': value, 'index': idx}

def replace_max_value(values_per_unit, idx):
    values_per_unit[idx] = -1

if __name__ == "__main__":
    data = list(map(int, sys.stdin.read().split()))
    n, capacity = data[0:2]
    values = data[2:(2 * n + 2):2]
    weights = data[3:(2 * n + 2):2]
    opt_value = get_optimal_value(capacity, weights, values)
    print("{:.10f}".format(opt_value))

Input:

5 100
60 20
100 50
120 30
90  10
10  10

Run:

python [filename].py < input.txt

Question:

The goal of the algorithm above, is to make sure the knapsack has the highest value. Can we further improve the implementation above?

I think some better organization will help.  Also, a slightly different algorithm will help simplify.

Separation of concerns (e.g. code splitting)

I think the way you have split everything up could use some work.  You only split out two bits of functionality to separate functions, and both are so simple they would probably just be better off inline.  This one in particular:

def replace_max_value(values_per_unit, idx):
    values_per_unit[idx] = -1

Used as:

replace_max_value(values_per_unit, value_and_index["index"])

This total combination is much more verbose then simply doing it inline:

values_per_unit[value_and_index['index']] = -1

The same is really true for your other method, get_max_value_per_unit.  To be clear, I'm not saying that you shouldn't split up parts of your program into separate functions.  However, splitting a single line (or even just three lines) is usually a sign of choosing the wrong "seams" to break up your application on, unless that functionality is needed in different places.  Obviously, that isn't the case here.

Although, this is simple enough that you might not really have to break anything apart, especially after some simplifications (coming soon).  The division between your __main__ calling function and your get_optimal_value is probably sufficient.

Separate input and output

Regarding that replace_max_value() method, even though it should go away, there is another lesson here.  I'm generally not a fan of using function parameters for output.  I.e., you are relying upon pythons pass-by-reference style to modify the passed in list.  As a general rule of thumb this is often error-prone in the long run.  Having a return value makes it clear what is getting modified and when it is being modified, but relying on modification of a function parameter is much less obvious.  As a result, it is easy to come back later and get confused about how/where your values_per_unit variable is being modified.  This syntax, however, is much more clear (even if more verbose):

values_per_unit = set_max_value( values_per_unit, value_and_index['index'] )

It is obvious exactly what is input, and what is being changed.  Now with python this is mildly trickier because you have no choice but to pass-by-reference, but a simple copy operation fixes the issue:

def set_max_value( values_per_unit, idx ):
    new_list = list( values_per_unit )
    new_list[idx] = -1
    return new_list

Granted, this is slightly slower (because of the array copy), but unless performance is known to be an issue, I think it is better for long-term code maintenance to do the copy and have a clear input/output for your function.

Sort!!

In cases like this sorting is your friend.  Before starting your loop take your values_per_unit and sort it.

sort_indexes = reversed( [i[0] for i in sorted(enumerate(values_per_unit), key=lambda x:x[1])] )
weights = [ weights[i] for i in sort_indexes ]
values = [ values[i] for i in sort_indexes ]
values_per_unit = [ values_per_unit[i] for i in sort_indexes ]

for ( weight, value, value_per_unit ) in zip( weights, values, values_per_unit ):

    weight_to_use = min( weight, remaining_capacity )
    remaining_capacity -= weight_to_use
    value += weight_to_use*value_per_unit

    if remaining_capacity <= 0:
        break

return value

Although it is worth noting that I am looping over sort_indexes three times.  Some might prefer doing that with one loop for performance, but I find this easier to read, and would stick to it as long as I knew performance wasn't going to be a big concern.