Filling random values in two sheets from a single sheet via Excel VBA I fill out random values in two sheets (Testfall-Input_Vorschlag) and (Testfall-Input_Antrag) out of another sheet (ADMIN_ARB11).

I have 371 rows in sheet (Testfall-Input_Vorschlag) and I have 488 rows in sheet (Testfall-Input_Antrag). I have 859 columns in sheet (ADMIN_ARB11).

I pick a random value from each of the 1st 371 columns (from ADMIN_ARB11) and I put them in the 371 rows in sheet (Testfall-Input_Vorschlag) and then I pick a random value from each of the next 488 columns (from ADMIN_ARB11) and put them in 488 rows in sheet (Testfall-Input_Antrag).

Sub Random_Befüllung_Vorschlag_ARB11()
Dim sh1 As Worksheet, sh2 As Worksheet, i As Long, j As Long, LB As Long, UB As Long
Set sh1 = Sheets("Testfall-Input_Vorschlag")
Set sh2 = Sheets("ADMIN_ARB11")

Application.ScreenUpdating = False
    For j = 7 To 300
        LB = 2
        If sh1.Cells(1, j) = "ARB11" Or sh1.Cells(1, j) = "ARB13" Or sh1.Cells(1, j) = "FVB1" Or sh1.Cells(1, j) = "FVB1E" Or sh1.Cells(1, j) = "FVB4" Or sh1.Cells(1, j) = "FVB4E" Then
            sh1.Cells(2, j) = sh1.Cells(1, j) & "_Schicht 1"
            sh1.Cells(3, j) = "TPL maximale Eingaben"
            If j = 7 Then
                sh1.Cells(6, j) = 1
            Else
                sh1.Cells(6, j) = sh1.Cells(6, j - 1) + 1
            End If
            sh1.Cells(5, j) = "TF " & sh1.Cells(6, j)
            sh1.Cells(7, j) = "Test_GE"
            sh1.Cells(8, j) = "x"

            For i = 11 To 382
            UB = sh2.Cells(Rows.Count, i - 10).End(xlUp).Row 'i - 10 controls column in Admin start at col 1.

            sh1.Cells(i, j).Value = sh2.Cells(Int((UB - LB + 1) * Rnd + LB), i - 10)

            Next

        End If

    If sh1.Cells(1, j) = vbNullString Then
    Exit For
    End If
    Next
Application.ScreenUpdating = False
End Sub

Sub Random_Befüllung_Antrag_ARB11()
Dim sh1 As Worksheet, sh2 As Worksheet, i As Long, j As Long, LB As Long, UB As Long
Dim wb As Workbook
Dim ws As Worksheet

Set wb = ThisWorkbook
Set ws = wb.Sheets("Testfall-Input_Vorschlag")
Set sh1 = Sheets("Testfall-Input_Antrag")
Set sh2 = Sheets("ADMIN_ARB11")

Application.ScreenUpdating = False
    'Testfallinfo in Testfall-Input_Antrag kopieren
    For j = 7 To 300
    If Sheets("Testfall-Input_Vorschlag").Cells(1, j) = "ARB11" Or Sheets("Testfall-Input_Vorschlag").Cells(1, j) = "ARB13" Or Sheets("Testfall-Input_Vorschlag").Cells(1, j) = "FVB1" Or Sheets("Testfall-Input_Vorschlag").Cells(1, j) = "FVB1E" Or Sheets("Testfall-Input_Vorschlag").Cells(1, j) = "FVB4" Or Sheets("Testfall-Input_Vorschlag").Cells(1, j) = "FVB4E" Then
    Union(ws.Cells(1, j), ws.Cells(2, j), ws.Cells(3, j), ws.Cells(4, j), ws.Cells(5, j), ws.Cells(6, j), ws.Cells(7, j), ws.Cells(8, j)).Copy
    sh1.Range("IV1").End(xlToLeft).Offset(, 1).PasteSpecial xlValues
    End If

        LB = 2
        If sh1.Cells(1, j) = "ARB11" Then
            For i = 13 To 501
                UB = sh2.Cells(Rows.Count, i + 364).End(xlUp).Row 'i - 10 controls column in Admin start at col 1.
                sh1.Cells(i, j).Value = sh2.Cells(Int((UB - LB + 1) * Rnd + LB), i + 364)

            Next
        End If

    If sh1.Cells(1, j) = vbNullString Then
    Exit For
    End If
    Next j
Application.ScreenUpdating = True
End Sub

It works as expected but it takes 5 min to run the code. How can I optimize this?

Accessing Objects such as Sheets, Cells & Range takes considerable time. Try to reduce their use as much as you can by assigning their values to a variable before entering a For loop.

If Sheets("Testfall-Input_Vorschlag").Cells(1, j) = "ARB11" Or Sheets("Testfall-Input_Vorschlag").Cells(1, j) = "ARB13" Or Sheets("Testfall-Input_Vorschlag").Cells(1, j) = "FVB1" Or Sheets("Testfall-Input_Vorschlag").Cells(1, j) = "FVB1E" Or Sheets("Testfall-Input_Vorschlag").Cells(1, j) = "FVB4" Or Sheets("Testfall-Input_Vorschlag").Cells(1, j) = "FVB4E" Then

These chained If statements will each be getting the value of your test cell in turn, each one accessing Sheets and Cells individually over and over for the same value, this will likely be having an impact on your performance.

Try putting the cell value in a variable before your tests:

testCell = Sheets("Testfall-Input_Vorschlag").Cells(1, j).Value
If testCell = "ARB11" Or testCell = "ARB13" Or testCell = "FVB1" Or testCell = "FVB1E" Or testCell = "FVB4" Or testCell = "FVB4E" Then

For i = 13 To 501
    UB = sh2.Cells(Rows.Count, i + 364).End(xlUp).Row 'i - 10 controls column in Admin start at col 1.
    sh1.Cells(i, j).Value = sh2.Cells(Int((UB - LB + 1) * Rnd + LB), i + 364)
Next

Within this loop, it appears that each iteration is finding the bottom row number with UB = sh2.Cells(Rows.Count, i + 364).End(xlUp).Row, could this be done before the loop?


ASCII to Binary and reverse in java Here is a simple app that i am trying to implement in java.
my goal is to write a program that can convert ASCII to Binary and back.

Questions:

Am i violating any OOP Rule.
Is my code readable ?
How can i make it more efficient ?
Anything that want to suggest.

Code:

public class FXMLDocumentController implements Initializable {

@FXML
TextArea asciiTextArea;
@FXML
TextArea binaryTextArea;
@FXML
StackPane animationPane;
private static final int AMOUNT_OF_BITS = 8;

@FXML
private void handleBinaryToText(ActionEvent event) {
    Callable<String> task = () -> binaryToAscii(binaryTextArea.getText());
    try {
        runTask(task, asciiTextArea);
    } catch (InterruptedException | ExecutionException ex) {
        asciiTextArea.setText(ex.toString());
    }

}

@FXML
private void handleTextToBinary(ActionEvent event) {
    Callable<String> task = () -> asciiToBinary(asciiTextArea.getText());
    try {
        runTask(task, binaryTextArea);
    } catch (InterruptedException | ExecutionException ex) {
        binaryTextArea.setText(ex.toString());
    }
}

@Override
public void initialize(URL location, ResourceBundle resources) {
    //Validate the Binary TextArea
    final Pattern binaryRegex = Pattern.compile("\\A[01\\n]*\\Z");
    Predicate<String> tester = binaryRegex.asPredicate();
    binaryTextArea.setTextFormatter(new TextFormatter<>(change -> {
        if (!tester.test(change.getControlNewText())) {
            return null;
        }
        return change;
    }));
       //binaryTextArea.setTextFormatter();

}

// Create the Binary To Text Task task
private String binaryToAscii(final String input) {

    if (input.length() % AMOUNT_OF_BITS != 0) {
        String msg = "Input must be a multiple of " + AMOUNT_OF_BITS;
        binaryTextArea.setText(msg);
        throw new IllegalArgumentException(msg);
    }
    final int INPUT_LEN = input.length();
    final int BUILDER_SIZE = INPUT_LEN / 8;
    StringBuilder result = new StringBuilder(BUILDER_SIZE);

    for (int i = 0; i < INPUT_LEN; i += AMOUNT_OF_BITS) {
        char charCode = (char) Integer.parseInt(input.substring(i, i + AMOUNT_OF_BITS), 2);
        result.append(charCode);
    }
    return result.toString();
}

// Create the Binary To Text Task
private String asciiToBinary(String text) {
    final byte[] bytes = text.getBytes();
    StringBuilder result = new StringBuilder(bytes.length * 8);
    for (byte b : bytes) {
        int val = b;
        for (int i = 0; i < AMOUNT_OF_BITS; i++) {
            result.append((val & 128) == 0 ? 0 : 1);
            val <<= 1;
        }
    }
    return result.toString();
}

private void runTask(Callable<String> task, TextArea textArea) throws InterruptedException, ExecutionException {
    ExecutorService thread = Executors.newFixedThreadPool(1);
    final String result = thread.submit(task).get();
    Platform.runLater(() -> {
        textArea.setText(result);
    });
    thread.shutdown();
}

}

Github link

Just a few quick observations:

You mix business logic with UI code. Normally, you'd go for separation of concerns and move the conversion functions into a separate class which is used by the UI, but does not belong to the UI.
Running a separate thread: how long does the conversion take? 1 ms? 2? I really doubt that the overhead of thread-creation is necessary here. (Though it may indeed serve as an excercise!)
Binary to ascii validation: if the amount of bits does not match, you overwrite the user input? Thus, if I, the user, make a little mistake, type 319 bits instead of 320, you actually force me to start over? /me pissed user :-)

That's it for now (probably not complete.)


Binary Search Tree implementation in Python 3 Please review my BST Code

1. Class node

class Node:

2. Constructor

def __init__(self, data):
    self.left = None
    self.right = None
    self.data = data

3. Insert node

def insert(self, data):
    if self.data:
        if data < self.data:
            if not self.left:
                self.left = Node(data)
            else:
                self.left.insert(data)
        elif data > self.data:
            if not self.right :
                self.right = Node(data)
            else:
                self.right.insert(data)
    else:
        self.data = data

4. Node in delete any value

def getMinValue(self,node):
    current = node
    while(current.left is not None):
        current = current.left
    return current
def delValue(self,data):
    if data < self.data:
        self.left = self.left.delValue(data)
    elif data > self.data:
        self.right = self.right.delValue(data)
    else:
        if self.left is None:
            temp = self.right
            self = None
            return temp
        elif self.right is None:
            temp = self.left
            self = None
            return temp
        temp = self.getMinValue(self.right)
        self.data = temp.data
        self.right = self.right.delValue(temp.data)
    return self

5. Node in search any value

def getSearchValue(self,data):
    if data == self.data:
        return print(self.data,"True")
    if data < self.data:
        if self.left:
            self.left.getSearchValue(data)
    if data > self.data:
        if self.right:
            self.right.getSearchValue(data)

6. Print tree

def print_tree(self):
    if self.left:
        self.left.print_tree()
    print(self.data)
    if self.right:
        self.right.print_tree()

A few quick points that came to my mind when looking at your code:

Documentation

Your code has no documentation whatsoever. Python has so called documentation strings, which are basically """triple quoted text blocks""" immediately following def whatever(...). Example:

def print_tree(self):
    """Print the content of the tree

    This method performs an in-order traversal of the tree
    """
    # ... your code here

Since your question title indicates that you're working with Python 3, also consider using type hints to document your code.

Naming

There is the infamous Style Guide for Python Code (aka PEP 8) which recommends to use lower_case_with_underscores for variable names and (member) functions. You do this for print_tree, but use camelCase for the other member functions.

Searching the tree

Your getSearchValue function is a little bit awkward in that it always returns None. Although your code promises to "get" the value, you instead print it to the console (together with the string "True") and return the return value of print which is None (aka no return value in that case). Your function also only returns something (other than the implicit None) if the value was found. In my opinion something like

def has_value(self, data):
    """Return True or False indicating whether the value is in the BST"""
    if data == self.data:
        return True
    if data < self.data:
        if self.left is not None:
            return self.left.has_value(data)
    if data > self.data:
        if self.right is not None:
            return self.right.has_value(data)
    return False

would be a more appropriate approach. As you can see, this function returns an appropriate bool value to signal the result.

Another minor tweak: this implementation uses if ... is not None: to explicitly check for None as signaling value. Since None is a singleton in Python, you should always use is (not) to check for equality.

Unnecessary parentheses

The parentheses around the condition in while(current.left is not None): are not needed. while works the same way as if in that regard. They are sometimes used for longer conditions that span multiple lines, since Python does implicit line joining in that case.

I'm also not fully convinced about your delValue function, but unfortunately I'm a little bit short on time at the moment.


palindrome code in C times out in program challenge? Hello I am making a code in C to find the value of the last sequence of palindromes of a specified size (d) and I need to optimize this code as it is for an exercise platform called URI Online Judge and is giving Time Limit exceeded.
https://www.urionlinejudge.com.br/judge/en/problems/view/1686

Given a string s[1..N], we define a palindromic sequence of length p and displacement d (1 <= p <= d) as a sequence of k (k >= 1) disjoint substrings of s (each of which is a palindrome of length p) distant d characters from each other.
More formally, it can be written as the following sequence of disjoint substrings of s : A= (s[i..i+p-1], s[i+d..i+d+p-1], s[i+2d..i+2d+p-1], ...), where each element of A is a palindrome of length p. Recall that a string is called a palindrome if it reads the same forwards and backwards.
The value of a palindromic sequence is the total number of characters from string s it uses (i.e., if the sequence has k palindromes of length p, its value is k*p). Given a fixed displacement value D, calculate the largest value of a palindromic sequence contained in a string S.
Input
Each test case is described using two lines. The first line contains two integers N and D (1 <= N <=10^5), 1 <= D <=10^5) representing respectively the length of the string S and the required displacement value. The next line contains the string S of length N consisting only of lowercase letters.
The last test case is followed by a line containing two zeros.
Output
For each test case output a line with the maximum value of a palindromic sequence with displacement D in string S.
Samples
Input
5 1
abbbc
Output
5

#include <stdio.h>

char palavra[100000];

int palindrome(const int inicio, const int fim) {
    if (inicio >= fim) {
        return 1;
    }
    if (palavra[inicio] != palavra[fim - 1]) {
        return 0;
    }
    return palindrome(inicio + 1, fim - 1);
}

int maior_sequencia(const int n, const int d) {

    int valor = 0, temp;
    int i, j;

    for (i = 1; i <= d; i++) {
        temp = 0;
        for (j = 0; j < n; j += d) {
            if (palindrome(j, j + i)) {
                temp++;
            }
        }
        if (temp * i > valor) {
            valor = temp * i;
        }
    }

    return valor;
}

int main() {

    int n, d;

    while (1) {

        scanf("%d %d", &n, &d);

        if (n == 0 || d == 0) {
            return 0;
        }

        scanf("%s", palavra);
        printf("%d\n", maior_sequencia(n, d));

    }

    return 0;

}

edit: Now i have a functional code, but give Time Limit exceeded again :/
edit 2: I added another version of the code, how could I optimize one of these two versions?

General Observations
It might be better to use an iterative solution rather than a recursive solution for this problem. Recursion can be expensive in terms of memory resources for the stack and it could also result in stack overflow. Calling a function is more expensive in terms of time than looping through a string as well. If the calling function created a reverse string it might be possible to use strcmp() or strstr() in the palindrome() function, either of those functions are very fast. It would also be faster to base the palindrome() function on strings rather than indexes.
While the program description uses n and d it would better to use more descriptive names in the program to make it more readable, examples might be length and displacement.
Since this question is performance based I won't recommend this, but memory for palavra be allocated in the while (1) { loop in main() since the length of the string is read in during each iteration.
Since English variable and function names are generally considered a global programming standard it might be better to use English variable and function names. I can figure out inicio and fim as start and end, but I don't have a clue what palavra or maior_sequencia (for maior_sequencia I'm guessing that it means major sequence). This is especially true since the problem statement is in English.
Avoid Global Variables
It is very difficult to read, write, debug and maintain programs that use global variables. Global variables can be modified by any function within the program and therefore require each function to be examined before making changes in the code. In C and C++ global variables impact the namespace and they can cause linking errors if they are defined in multiple files. The answers in this stackoverflow question provide a fuller explanation.
The variable palavra should be declared in main and passed into functions as necessary.
Declare Variables as Necessary
The original version of C required all variables to be declared at the top of a logic block, but this is no longer the case. In almost all programming languages it is now considered better to declare and initialize variables as they are needed. In the C programming language they need to be initialized because no local variable have a default value assigned. Each variable should be declared and initialized on its own line to make the code easier to read, write and maintain. In the function maior_sequencia the variables i and j can be defined in their respective loops:
int maior_sequencia(const size_t length, const size_t displacement, char *palavra) {

    size_t valor = 0;
    size_t temp = 0;

    for (size_t i = 1; i <= displacement; i++) {
        temp = 0;
        for (size_t j = 0; j < length; j += displacement) {
            if (palindrome(j, j + i, palavra)) {
                temp++;
            }
        }
        if (temp * i > valor) {
            valor = temp * i;
        }
    }

    return valor;
}

Use Unsigned Integer Based Variables for Indexes and Sizes
Rather than using integers that can go negative for indexes and sizes it is better to use types such as size_t which are based on unsigned integers. The type size_t is what is returned by the sizeof(OBJECT) function as an example.
Magic Numbers
There are Magic Numbers function (100000), it might be better to create symbolic constants for them to make the code more readable and easier to maintain. These numbers may be used in many places and being able to change them by editing only one line makes maintenance easier.
Numeric constants in code are sometimes referred to as Magic Numbers, because there is no obvious meaning for them. There is a discussion of this on stackoverflow.
Don't Ignore Library Function Return Values
The function scanf() returns an integer that indicates the number of values read or end of file (EOF). This value can be used to control the while loop in main() rather than having the while (1) loop with a break.
#define MAX_LENGTH 10000

int main() {
    size_t length = 0;
    size_t displacement = 0;

    while (scanf("%d %d", &length, &displacement) != EOF) {
        char palavra[MAX_LENGTH + 1];

        if (scanf("%s", palavra) == EOF)
        {
            break;
        }
        else
        {
            printf("%d\n", maior_sequencia(length, displacement, palavra));
        }
    }

    return 0;
}


I wrote a class to implement auto_ptr Help me review this code:

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

template <typename T>
class auto_ptr
{
public:
    explicit auto_ptr(T* p = NULL):m_p(p){}
    auto_ptr(auto_ptr& other);
    auto_ptr& operator=(auto_ptr& other);
    T* get() const { return m_p; }
    void reset(T* p = NULL);
    T& operator * (){ return *m_p; }
    T* operator -> (){ return m_p; }
    ~auto_ptr()
    {
        if(m_p)
            delete m_p;
        m_p = NULL;
    }
private:
    T* m_p;
    T* release();
};

int main()
{
    auto_ptr<int> p(new int(3));
    cout<<*p<<endl;

    int *b = new int(6);
    p.reset(b);
    cout<<*p.get()<<endl;

    auto_ptr<int> p1(new int(10));
    p = p1;
    cout<<*p<<endl;
    assert(NULL == p1.get());

    auto_ptr<int> p2(new int(100));
    auto_ptr<int>p3(p2);
    cout<<*p3<<endl;
    assert(NULL == p2.get());
}

template <typename T>
T* auto_ptr<T>::release()
{
    T* temp = this->m_p;
    this->m_p = NULL;
    return temp;
}

template <typename T>
auto_ptr<T>::auto_ptr(auto_ptr& other)
{
    m_p = other.release();
}

template <typename T>
auto_ptr<T>& auto_ptr<T>::operator=(auto_ptr& other)
{
    if(this == &other)
        return *this;
    delete m_p;
    m_p = other.release();
    return *this;
}

template <typename T>
void auto_ptr<T>::reset(T* p)
{
    if(m_p != p)
    {
        delete m_p;
        m_p = p;
    }
}

Assuming you are going to put this in a header file don't do thus

using namespace std;

See: https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice

These should be const as they don't change the state of your auto_ptr

T& operator * (){ return *m_p; }
T* operator -> (){ return m_p; }

The destructor

~auto_ptr()
{
    if(m_p)            // No need to check for NULL
                       // delete on a NULL pointer is valid.
        delete m_p;

    m_p = NULL;        // This is pointless.
                       // The object is about to leave scope.
}

Why the sudden use of this!

template <typename T>
T* auto_ptr<T>::release()
{
    T* temp = this->m_p;
          //  ^^^^^^
    this->m_p = NULL;
 // ^^^^^^
    return temp;
}

Assignment not exception safe.

template <typename T>
auto_ptr<T>& auto_ptr<T>::operator=(auto_ptr& other)
{
    if(this == &other)
        return *this;

    // This is not exception safe.
    // T is completely unknown to you.
    // Therefore you must assume the person who wrote T is an idiot
    // and therefore may throw an exception from there destructor.
    delete m_p;

    // If the above statement throws then your object is now in an undefined
    // state with m_p pointing at an invalid object.

    m_p = other.release();
    return *this;
}

The correct way to do this.

template <typename T>
auto_ptr<T>& auto_ptr<T>::operator=(auto_ptr& other)
{
    if(this == &other)
        return *this;

    auto_ptr<T> tmp = release();
    m_p = other.release();

    // After the state of your (and other objects)
    // are completely defined and are in a good state.
    // then you can tidy up any left over objects.

    return *this;

    // the destructor of tmp should now release it properly.
}


Method returning IEnumerable should ToList() or not I can't decide whether my method that returns an IEnumerable<T> should itself be lazy or whether it should build a list and return that. I often opt for the latter so I can be sure the enumeration of each value isn't performed multiple times.
For example:

public IEnumerable<UserProfile> GetUsers()
{
    var allDepartments = GetAllDepartments(active: true); // Returns IEnumerable<Department>
    var allUsers = GetAllUserDepartments(active: true); // Returns IEnumerable<User>
    var users
        = allDepartments
            .Join(allUsers, x => x.Department, y => y.Department, (x, y) => y, StringComparer.OrdinalIgnoreCase)
            .Distinct()
            .Select(x => GetUserProfile(x))
            .ToList(); // Is this recommended or not

    return users;
}

The key part is each enumeration is doing something (GetUserProfile) non trivial, it might be expensive, it might not, but what's the recommendation for a method that returns an IEnumerable<T>? Should it care about whether the caller might enumerate it several times or not?

Assume only IEnumerable<T> functionality is required by the caller and my question can be reworded into:

How do I signify to the caller that each enumeration may be expensive?

How expensive may change their implementation decisions, if they enumerate the whole lot several times then a ToList() makes sense to them to perform, if they don't enumerate the whole lot, then a ToList() would be a waste for me (or the caller) to perform?

Does code that calls the method always expect List functionality (access by index, etc.)? Return a List. Does code that calls the method only expect to iterate over it? Return an IEnumerable.

You shouldn't care about what the caller does with it, because the return type clearly states what the returned value is capable of doing. Any caller that gets an IEnumerable result knows that if they want indexed access of the result, they will have to convert to a List, because IEnumerable simple isn't capable of it until it's been enumerated and put into an indexed structure. Don't assume that the callers are stupid, otherwise you end up taking functionality away from them. For example, by returning a List, you've taken away the ability to stream results which can have its own performance benefits. Your implementation may change, but the caller can always turn an IEnumerable into a List if they need to.

Now that you've decided what the return type should be, you have to decide what the implementation should use. Deferred execution has benefits, but can also be confusing. Take this example:

public static IEnumerable<int> GetRecordIds()
{
    return dbContext.Records.Select(r => r.Id);
}

IEnumerable<int> currentRecordIds = GetRecordIds();

dbContext.Add(new Record { Id = 7 });

// Includes "7", despite GetRecordIds() being called before the Add():
currentRecordIds.Dump();

This can be "remedied" by GetRecordIds calling ToList before returning. The "correct" use here simply depends on what is expected of the class ("live" results, or time of call results).

Again, don't assume the caller is stupid, and don't take away functionality from the caller by making assumptions about how it will be used. Remember, the interface tells you what is expected to be returned. IEnumerable only means you are getting something that can be iterated over (potentially streaming results and making use of deferred execution), and List only means you're getting an in-memory collection that can be added to, removed from, accessed by index, etc.

Edit - To address your "Update 1":

If you are really concerned, you might add some information into the documentation, but I don't think it's necessary. The IEnumerable interface doesn't claim to make any promises about performance on repeat enumeration (or even each iteration for that matter), so it's on the caller to handle it intelligently.


Find pair in array with maximum sum using recursion Not sure the best way to solve this using recursion. Below is what I have tried so far:
public static void findSum(int[] a, int sum, int i, int j) {

    if (i == a.length)
        return;
    if (j == a.length)
        return;

    sum = (a[i] + a[j] > sum) ? (a[i] + a[j]) : sum;
    System.out.println("Sum:" + sum + ", i:" + i + ",j:" + j);

    findSum(a, sum, i, j + 1);
    i++;
    j = i;
    findSum(a, sum, i, j + 1);

    return;
}

Given input : int[] a = {5, 4, 7, 3, 9, 2}
Expected output : 7+9 -> 16
Plus I am getting many number of iterations which must be reduced.

Notice that the answer is the sum of the two greatest elements in the array. You only need one pass to find both of them.
The idea is that you go from through the array and keep track of top 2 elements in the array. The code will be simplier if the function requires to be called on a valid input data (an array of length >= 2) and first two elements already compared. The function then takes the next element of the array, updates the top 2 pair and moves onto the next element.
I assume you have to use recursion, but the code could be easily rewritten using a loop.
public static void findSum(int[] a, int greatest, int secondGreatest, int currentIndex) {
    if (currentIndex == a.length) {
        System.out.println("Sum:" + (a[greatest] + a[secondGreatest]) + ", i:"+greatest+",j:"+secondGreatest);
        return;
    }
    if (a[currentIndex] > a[greatest]) {
        // found the new top; the current top becomes the second
        secondGreatest = greatest;
        greatest = currentIndex;
    } else if (a[currentIndex] > a[secondGreatest]) {
        // found the new second-greatest, but so the current top stays the same
        secondGreatest = currentIndex;
    }
    findSum(a, greatest, secondGreatest, currentIndex + 1);
}

And the usage would be:
if (a.length >= 2) {
    // Rank the first two elements in the array and initiate the recursion
    int greatest = a[0] > a[1] ? 0 : 1;
    int secondGreatest = greatest == 0 ? 1 : 0;
    findSum(a, greatest, secondGreatest, 2);
}


Passing variable to BackgroundWorker and use the same value later I create multiple BackgroundWorker within a for-loop and each of them needs to know a special value. To simplify, it is just i in this example. When the BackgroundWorker is finished, I need to read that i again. I thought of subclassing BackgroundWorker and creating a class MyBW for that purpose which is able to store the i as value.

My example below works, but I am interested, if this is the best way to do this?

Edit: I have to add, that the variable I need to pass is a simple String, not a large object.

Fully working minimal example

using System;
using System.Text;

namespace MultiThreadTest
{
    class Program
    {
        static void Main(string[] args)
        {
            for (int i = 0; i < 10; i++)
            {
                MyBW bw = new MyBW();
                bw.value = i;
                bw.WorkerReportsProgress = false;
                bw.DoWork += delegate(object sender, System.ComponentModel.DoWorkEventArgs eargs)
                {
                    Console.WriteLine(String.Format("Thread {0} started", bw.value));
                    System.Threading.Thread.Sleep(bw.value * 1000);
                };
                bw.RunWorkerCompleted += delegate(object sender, System.ComponentModel.RunWorkerCompletedEventArgs eargs)
                {
                    Console.WriteLine(String.Format("Thread {0} finished", bw.value));
                };

                bw.RunWorkerAsync();
            }

            // Only as a run loop that I can see the output.
            while (true)
            {
                System.Threading.Thread.Sleep(1000);
                Console.WriteLine("Sleeping...");
            }
        }
    }

    class MyBW : System.ComponentModel.BackgroundWorker
    {
        /// <summary>
        /// Here I store the value.
        /// </summary>
        public int value;
    }
}

BackgroundWorker already has a mechanism for passing and retrieving arguments.  The DoWorkEventArgs has a property for a passed in method argument and a property for the DoWork result.  The RunWorkerCompletedEventArgs has a property for retrieving the result.  No sub-classing is necessary, although you may need to make a custom container if you need to pass/return more than one object.

for (int i = 0; i < 10; i++)
{
  BackgroundWorker bw = new BackgroundWorker ();
  bw.WorkerReportsProgress = false;
  bw.DoWork += delegate(object sender, System.ComponentModel.DoWorkEventArgs eargs)
  {
    int i = (int)eargs.Arugment;  //get argument
    Console.WriteLine(String.Format("Thread {0} started", bw.value));
    System.Threading.Thread.Sleep(bw.value * 1000);
    eargs.Result = i;            //set result
  };
  bw.RunWorkerCompleted += delegate(object sender, System.ComponentModel.RunWorkerCompletedEventArgs eargs)
  {
    int i = (int)eargs.Arugment;  //get argument
    Console.WriteLine(String.Format("Thread {0} finished", i));
  };

  bw.RunWorkerAsync(i);
}


OpenCV 3: Using k-Nearest Neighbors to analyse RGB image I'm new to computer vision and numpy.

I wrote a simple script to seperate red, green and blue colors from the original image by using the kNN algorithm.
After reading through some numpy tutorials, I'm under the impression that the performance of my script can be improved heavily.
So here are my questions:

Is there a way to avoid the call to np_pixel = np.asarray(...) for each of the pixels? I can't find a format for the trainData that would allow me that.
Is there a way to feed the whole image into the findNearest method?
This would make it possible to avoid the slow python loop.
Do you see any other ways to improve the performance or length of this script?

import cv2
import numpy as np

trainData = np.array([[255, 0, 0], [0, 255, 0], [0, 0, 255]], dtype=np.float32)
# Labels each one of the BGR channels as numbers 0, 1, 2 respectively
responses = np.array([[0], [1], [2]], dtype=np.float32)

imgPath = "../Images/RGB.png"
img = cv2.imread(imgPath)
if img is None:
    raise FileNotFoundError("'{0}' could not be opened!".format(imgPath))

knn = cv2.ml.KNearest_create()
knn.train(trainData, cv2.ml.ROW_SAMPLE, responses)

height, width, depth = img.shape
colorLayers = dict()
colorLayers[0] = np.zeros((height, width, 3), dtype=np.float32)
colorLayers[1] = np.zeros((height, width, 3), dtype=np.float32)
colorLayers[2] = np.zeros((height, width, 3), dtype=np.float32)

for row in range(height):
    for col in range(width):
        np_pixel = np.asarray([img[row, col]], dtype=np.float32)
        ret, results, neighbours, dist = knn.findNearest(np_pixel, 1)
        int_result = int(results)

        if int_result == 0:
            color = np.asarray([255, 0, 0]).astype(np.float32)
        elif int_result == 1:
            color = np.asarray([0, 255, 0]).astype(np.float32)
        else:
            color = np.asarray([0, 0, 255]).astype(np.float32)

        colorLayers[int_result][row, col] = color

cv2.imshow('Original', img)
cv2.imshow('Blue', colorLayers[0])
cv2.imshow('Green', colorLayers[1])
cv2.imshow('Red', colorLayers[2])
cv2.waitKey(0)

I found a way to get rid of the python loop. np.vstack() converts the image to a single row which in this case is the format findNearest() expects.

Running below code through IPythons timeit function yields a huge perfomance boost:
The version in the original answer takes 2.5s per loop.
The new version runs at 15.3ms per loop.

import cv2
import numpy as np

imgPath = r"../Images/landscape2.jpg"
img_BGR = cv2.imread(imgPath)
if img_BGR is None:
    raise FileNotFoundError("'{0}' could not be opened!".format(imgPath))

BGR_COLORS = dict(blue=(255, 0, 0), green=(0, 255, 0), red=(0, 0, 255), black=(0, 0, 0), white=(255, 255, 255))
LABELS = dict(blue=np.array([0]), green=np.array([1]), red=np.array([2]), black=np.array([3]), white=np.array([4]))
trainData = np.array([BGR_COLORS['blue'], BGR_COLORS['green'], BGR_COLORS['red'], BGR_COLORS['black'], BGR_COLORS['white']], dtype=np.float32)
responses = np.array([[LABELS['blue']], LABELS['green'], LABELS['red'], LABELS['black'], LABELS['white']], dtype=np.float32)

knn = cv2.ml.KNearest_create()
knn.train(trainData, cv2.ml.ROW_SAMPLE, responses)

img_vstacked = np.vstack(img_BGR).astype(np.float32)
ret, results, neighbours, dist = knn.findNearest(img_vstacked, 1)

height, width, depth = img_BGR.shape
results_int = results.reshape(height, width).astype(np.uint8)

def colorPixels(image, results, colorName):
    image[results[:, :] == LABELS[colorName]] = BGR_COLORS[colorName]

img_clustered = img_BGR.copy()
for colorName in BGR_COLORS.keys():
    colorPixels(img_clustered, results_int, colorName)

cv2.imshow("Original vs. Clustered", np.hstack((img_BGR, img_clustered)))
cv2.waitKey(0)
cv2.destroyAllWindows()


using 2D array to read datatable ok? or repeatedly call function that reads record by record? I am asking this for coding style.  I have a program class that calls a function(of a data access class) that reads from a table (using entity framework by the way), and I'm preparing to write the DataAccess.ReadMethod().

Is it better to pass the function a 2d string array string[rows][fields], reading each field and each row, and put it in the DataAccess.ReadMethod's out parameter string[rows][fields] to return to the main program and have the main program display it? or just repeatedly call the DataAccess.ReadMethod(string[fields]) and in the main program print each record out individually to the console?

I imagine that my ReadMethod() will look something like this:

public class DataAccess
{
    public Exception ReadMan(TestDatabaseEntities dbEntities, IQueryable myQuery, out string sManDetails)
    {
        Exception ex = null;
        sManDetails = "";
        try
        {
            foreach (Man m in myQuery)
            {
                sManDetails += m.ManID.ToString() + " " + m.Name + "\n";
            }
        }
        catch (Exception myException)
        {
            ex = myException;
        }
        return ex;
    }
}

the calling code in program class looks like this:

class program
{
    static private void DoRead()
    {
        var dbEntities = new TestDatabaseEntities();
        UserInterface MyUI = new UserInterface();
        DataAccess MyDA = new DataAccess();
        Exception ex = null;
        string sMyMen = "";

        var query = from person in dbEntities.Men
                    where true
                    select person;
        MyUI.DisplayDivider();
        ex = MyDA.ReadMan(dbEntities, query, out sMyMen);
        if (ex != null)
            sMyMen = ex.ToString();
        MyUI.DisplayMessage(sMyMen);
        MyUI.DisplayDivider();
    }
}

I have discovered, with thanks to you each, that it is better to call the display method within the DataAccess.ReadMethod() than to pass strings, Here's what I think i should do:

public class DataAccess
{
    public bool CreateMan(TestDatabaseEntities dbEntities, Man M)
    {
        return TryDataBase(dbEntities, "Man created successfully",
            () =>
            {
                dbEntities.Men.Add(new Man { ManID = M.ManID, Name = M.Name });
            });
    }

    public bool UpdateMan(TestDatabaseEntities dbEntities, IQueryable<Man> query, Man man)
    {
        return TryDataBase(dbEntities, "Man updated Successfully",
            () =>
            {
                foreach (Man M in query)
                {
                    M.Name = man.Name;
                }
            });
    }

    public bool DeleteMan(TestDatabaseEntities dbEntities, IQueryable myQuery)
    {
        return TryDataBase(dbEntities, "Man deleted Successfully",
            () =>
            {
                foreach (Man M in myQuery)
                {
                    dbEntities.Men.Remove(M);
                }
            });
    }

    public bool ReadMan(TestDatabaseEntities dbEntities, IQueryable myQuery)
    {
        UserInterface MyUI = new UserInterface();
        bool bSuccessful;

        bSuccessful = TryDataBase(dbEntities, "Records read successfully",
            () =>
            {
                MyUI.DisplayDivider();
                foreach (Man m in myQuery)
                {
                    MyUI.DisplayMessage(new string[] { m.ManID.ToString(), m.Name });
                }
                MyUI.DisplayDivider();
            });

        return bSuccessful;
    }

    public bool TryDataBase(TestDatabaseEntities MyDBEntities, string SuccessMessage,  Action MyDBAction)
    {
        UserInterface MyUI = new UserInterface();
        try
        {
            MyDBAction();
            MyUI.DisplayMessage(SuccessMessage);
            return true;
        }
        catch (Exception e)
        {
            MyUI.DisplayMessage(e.ToString());
            return false;
        }
    }
}


Average across direction-specific average speeds and probabilities I have a wind rose with 72 evenly-spaced directions, spanning 360°, each describing a direction-specific average wind speed and associated probability. We must condense this information to 36 evenly-spaced direction bins. "probability" is the normalized frequency of occurrences. The probabilities should sum to one. If 5° has a 0.1 probability. that means that the wind came from that direction 10% of the time.

I average across the midpoints of each bin, which results in undesirably shifting the wind rose so that the center is at 2.5 degrees. Is there a more elegant way to do this in theory or implementation?

import pandas as pd
import numpy as np
import wget

# Load data!
url = 'https://raw.githubusercontent.com/scls19fr/windrose/master/samples/amalia_directionally_averaged_speeds.txt'
wget.download(url, 'amalia_directionally_averaged_speeds.txt')
dat = pd.read_csv('./amalia_directionally_averaged_speeds.txt',
                  sep=r'\s+', header=None, skiprows=[0])
dat.columns = ['direction', 'average_speed', 'probability']

# two steps
#   1. average frequency across speeds
#   2. compute average speed weighted by frequencies
#     (the scheme is to average across both midpoints. this
#      shift the centers by 2.5 degrees and bin by 10)
directions, speeds, frequencies = [], [], []
for i in range(dat.shape[0] / 2):
    directions.append(dat.direction[i * 2] + 2.5)
    frequencies.append(np.mean(dat.probability[[i * 2, i * 2 + 1]].values))
    speeds.append((dat.direction[i * 2] * dat.average_speed[i*2] +
                  dat.direction[i*2 + 1] * dat.average_speed[i * 2 + 1]) /
                  (dat.direction[i * 2] + dat.direction[i * 2+1]))

# save data
done = pd.DataFrame({'direction': directions,
                    'speed': speeds, 'probability': frequencies})
done.to_csv('wind_rose.csv')

The biggest issue with this code is it iterates over a DataFrame. Iterating over a DataFrame negates any performance improvements gained by using pandas. As much as possible, use operations that apply to the entire Series or an entire DataFrame at once. Allow the C code to do the looping for you so the data doesn't get pulled into slow Python objects.

There is no reason to download the data into a file using wget. This data can be downloaded directly into memory, and pandas supports http urls.

Look at where you place comments before blocks of code. These are perfect places to split up your code into functions.

"""Process unweighted wind data to file."""
import pandas as pd

URL = ('https://raw.githubusercontent.com/scls19fr/windrose/master/samples/'
       'amalia_directionally_averaged_speeds.txt')
OUTPUT_PATH = 'wind_rose.csv'

def load_wind_data(data_url):
    """Load wind data from url into a dataframe.

    Args:
        data_url(str|io):
            A URL, path, or buffer containing wind data.

    Returns:
        A dataframe containing wind data
    """

    dat = pd.read_csv(data_url, sep=r'\s+', header=None, skiprows=1,
                      names=['direction', 'average_speed', 'probability'])
    return dat

You can use the names argument of read_csv instead of setting columns.
wget is not used and the url is passed into read_csv since read_csv supports http.

def average_wind_data(dat):
    """Average and weight wind data.

    Args:
        dat(pd.DataFrame):
            A dataframe containing unweighted wind data.
    Returns:
        A dataframe containing weighted, averaged wind data.

    1. average frequency across speeds
    2. compute average speed weighted by frequencies
    (the scheme is to average across both midpoints. this
     shift the centers by 2.5 degrees and bin by 10)
    """

    avg_df = pd.DataFrame()
    # shift(-1) leaves the NaN value at the end of the dataframe
    adj_sum_dat = dat + dat.shift(-1)
    avg_df['direction'] = (dat['direction'][::2] + 2.5).values
    avg_df['probability'] = (adj_sum_dat['probability'][::2]/2).values
    unweighted_speed = dat['direction'] * dat['average_speed']
    adj_speed_sum = unweighted_speed + unweighted_speed.shift(-1)
    avg_df['speed'] = (adj_speed_sum/adj_sum_dat['direction'])[::2].values
    return avg_df

Use shift() on Series or DataFrame objects to do vector operations on adjacent data. Use slice notation to get even data points ([::2] means all the entire index stepped over by 2). Do vector operations on entire series instead of each item on the series. This section of code is about 6x faster than the original iteration code.

def main(input_url, output_path):
    """Load wind data at input_url and save processed data to csv.

    Args:
        input_url(str|io):
            Location of wind data.
        output_path(str):
            File location to save processed wind data.
    """
    dat = load_wind_data(input_url)
    processed_df = average_wind_data(dat)
    processed_df.to_csv(output_path)

if __name__ == "__main__":
    main(URL, OUTPUT_PATH)

IO should be done at the top level of execution.


Conditional Create: must be the only statement in the batch I only want to create this SQL function if the dependent Assembly exists.
I can do it using dynamic SQL, but it seems messy and I lose syntax checking (in
management studio). This function's dependency (for various reasons) may or may
not be available on an individual developers machine, and I don't want to
interrupt our local "get latest" process with an error if this is the case.

Is there a better way?

--Attempt 1 (FAILS)
IF  EXISTS (SELECT * FROM sys.assemblies WHERE name = 'SQL_CLR_Functions')
BEGIN
    CREATE FUNCTION dbo.CLR_CharList_Split(@list nvarchar(MAX), @delim nchar(1) = N',')
    RETURNS TABLE (str nvarchar(4000)) AS EXTERNAL NAME SQL_CLR_Functions.[Library.SQL.CLR.Functions].CLR_CharList_Split
END

--Attempt 2 (FAILS)
BEGIN TRY
    CREATE FUNCTION dbo.CLR_CharList_Split(@list nvarchar(MAX), @delim nchar(1) = N',')
    RETURNS TABLE (str nvarchar(4000)) AS EXTERNAL NAME SQL_CLR_Functions.[Library.SQL.CLR.Functions].CLR_CharList_Split
END TRY
BEGIN CATCH
END CATCH

--Attempt 3 (FAILS)
IF NOT EXISTS (SELECT * FROM sys.assemblies WHERE name = 'SQL_CLR_Functions')
BEGIN
 GOTO END_CLR;
END
GO
CREATE FUNCTION dbo.CLR_CharList_Split(@list nvarchar(MAX), @delim nchar(1) = N',')
RETURNS TABLE (str nvarchar(4000)) AS EXTERNAL NAME SQL_CLR_Functions.[Library.SQL.CLR.Functions].CLR_CharList_Split
GO
END_CLR:

--Attempt 4 (WORKS!!!)
IF EXISTS (SELECT * FROM sys.assemblies WHERE name = 'SQL_CLR_Functions')
BEGIN
    EXEC (N'CREATE FUNCTION dbo.CLR_CharList_Split(@list nvarchar(MAX), @delim nchar(1) = N'','')
            RETURNS TABLE (str nvarchar(4000)) AS EXTERNAL NAME SQL_CLR_Functions.[Library.SQL.CLR.Functions].CLR_CharList_Split')
END

You could use the SET NOEXEC statement, which controls whether the subsequent statements are executed (OFF) or not (ON). Use it in conjunction with your EXISTS test like this:

IF NOT EXISTS (SELECT * FROM sys.assemblies WHERE name = 'SQL_CLR_Functions')
  SET NOEXEC ON
GO
CREATE FUNCTION dbo.CLR_CharList_Split(@list nvarchar(MAX), @delim nchar(1) = N',')
RETURNS TABLE (str nvarchar(4000)) AS EXTERNAL NAME SQL_CLR_Functions.[Granite.SQL.CLR.Functions].CLR_CharList_Split
GO
SET NOEXEC OFF


RPG Skeleton, Part 2 - The Entity, and a SpecType This question is a followup to Simple RPG skeleton, Part 1 - The Character.

This is the second "part" of my development of this skeleton, and it now includes two new classes, Entity, and SpecType. The Entity class essentially serves as a base class for any character-like object, and has the following attributes and methods:

name - The name of the entity.
health - How much health the entity has.
toString() - Return a string representation of the entity.

The SpecType class serves as a small utility class for creating number-related specs that apply to character-like object, and has the following attributes and methods:

specValue - The value of the spec itself.
changeValue() - Change the value of the spec, and make sure it stays in the range \$0\rightarrow\infty\$.

I'm wondering the following things:

Should SpecType be an immutable class? Right now, it's just modifying itself when changeValue is called.
Is the Entity class designed in an appropriate way?
Am I following standard C++ practices?

entity.h

#ifndef RPGGAME_ENTITY_H_
#define RPGGAME_ENTITY_H_

#include <string>
#include "spectype.h"

/// <summary>
/// A base class that represents an entity, and implements
/// some base attributes, like health or name, and a few helper
/// methods for apply damage.
/// </summary>
class Entity
{
public:
    std::string name;
    SpecType    health;

    Entity(const std::string& name, SpecType health);
    std::string toString();
};

#endif

entity.cpp

#include <string>
#include <random>
#include "entity.h"

/// <summary>
/// Constructor for the Entity class.
/// </summary>
/// <param name="name">The name of the entity.</param>
/// <param name="health">The health of the entity.</param>
Entity::Entity(const std::string& name, SpecType health):
    name(name),
    health(health)
{}

/// <summary>
/// Return a string value representing the entity.
/// </summary>
/// <remarks>
/// This is mostly not used unless a raw Entity is being
/// used by itself, which should be very uncommon, or not
/// happen at all.
/// </remarks>
std::string Entity::toString()
{
    return "Name: "   + name                             + ", " +
           "Health: " + std::to_string(health.specValue) + "\n";
}

spectype.h

#ifndef RPGGAME_SPECTYPE_H_
#define RPGGAME_SPECTYPE_H_

/// <summary>
/// The SpecType class describes a non-constant unsigned
/// integer spec that can be used in any entity-derived class.
/// or structure.
/// </summary>
class SpecType
{
public:
    int specValue;

    SpecType(int specValue);
    void changeValue(int value);
};

#endif

spectype.cpp

#include "spectype.h"

/// <summary>
/// Constructor for the SpecType class.
/// </summary>
/// <param name="specValue">The value that the SpecType contains.</param>
SpecType::SpecType(int specValue):
    specValue(specValue)
{}

/// <summary>
/// Change the Spec's value by a certain amount.
/// <summary>
/// <param name="value">The value to change specValue by.</param>
void SpecType::changeValue(int value)
{
    if (specValue + value >= 0) { specValue += value; }
    else { specValue = 0; }
}

main.cpp (tests)

#include <iostream>
#include "spectype.h"
#include "entity.h"

int main()
{
    SpecType testSpec1(0);
    testSpec1.changeValue(10);
    testSpec1.changeValue(-11);
    std::cout << testSpec1.specValue << "\n";

    SpecType testSpec2(51);
    testSpec2.changeValue(-2143);
    testSpec2.changeValue(10);
    std::cout << testSpec2.specValue << "\n";

    std::cin.get();

    Entity testEntity1("FooBarBlahName", 30);
    std::cout << testEntity1.toString();

    Entity testEntity2("Goku", 9001);
    std::cout << testEntity2.toString();

    std::cin.get();
}

It's not much code, so there's little else to change or fix here. The only tiny mistake you made was forgetting to mark toString with const, so that it can be called on a const object, very tiny and easy fix:

std::string toString() const;
                       ^^^^^

And the same on the definition. I'm assuming you're already familiar with the meaning of const in a method, but if not, here's an explanation. In summary, a const method is not allowed to mutate the object, only to inspect its member data, thus callable if the object itself is const.

Right now, there's not much reason for the existence of SpecType. In my opinion, it does too little to be a separate class. Assuming you are already extending that class, then it makes sense, but otherwise, it is better to avoid predicting the future and don't bother adding boilerplate classes that end up unused.

The suggestion made by a previous answer to the other question of creating a template class was interesting because each template instantiation has a unique type, so that would prevent, for instance, mixing health with mana because those would be unrelated types. Going that way, then I'd say the template Spec type would be a nice addition, even if just as a tiny wrapper, since it adds type-safety to the code.

Commenting for auto-generated documentation

I get it that some companies really value generating documentation from comments in the code, but take a piece like this one for example:

/// <summary>
/// Constructor for the SpecType class.
/// </summary>
/// <param name="specValue">The value that the SpecType contains.</param>
SpecType::SpecType(int specValue):
    specValue(specValue)
{}

What is that comment adding to the understanding of the code, that couldn't be inferred just by reading the code? Nothing whatsoever. And those XML tags are simply atrocious! They completely distract me from the actual comment text and code.

If you really must comment mechanically to generate documentation, then I'd suggesting using a better tool: doxygen.

Doxygen uses a less invasive markup than those XML tags (actually, you can choose from a few different styles), and more, it doesn't force you to exhaustively comment every method. Methods you don't comment are copied verbatim to the output doc, so obvious things like a constructor don't have to be commented with "this is the constructor for class XYZ".


Python generator function that yields combinations of elements in a sequence sorted by subset order In Python, itertools.combinations yields combinations of elements in a sequence sorted by lexicographical order.  In the course of solving certain math problems, I found it useful to write a function, combinations_by_subset, that yields these combinations sorted by subset order (for lack of a better name).

For example, to list all 3-length combinations of the string 'abcde':

>>> [''.join(c) for c in itertools.combinations('abcde', 3)]
['abc', 'abd', 'abe', 'acd', 'ace', 'ade', 'bcd', 'bce', 'bde', 'cde']

>>> [''.join(c) for c in combinations_by_subset('abcde', 3)]
['abc', 'abd', 'acd', 'bcd', 'abe', 'ace', 'bce', 'ade', 'bde', 'cde']

Formally, for a sequence of length \$n\$, we have \$\binom{n}{r}\$ combinations of length \$r\$, where \$\binom{n}{r} = \frac{n!}{r! (n - r)!}\$

The function combinations_by_subset yields combinations in such an order that the first \$\binom{k}{r}\$ of them are the r-length combinations of the first k elements of the sequence.

In our example above, the first \$\binom{3}{3} = 1\$ combination is the 3-length combination of 'abc' (which is just 'abc'); the first \$\binom{4}{3} = 4\$ combinations are the 3-length combinations of 'abcd' (which are 'abc', 'abd', 'acd', 'bcd'); etc.

My first implementation is a simple generator function:

def combinations_by_subset(seq, r):
    if r:
        for i in xrange(r - 1, len(seq)):
            for cl in (list(c) for c in combinations_by_subset(seq[:i], r - 1)):
                cl.append(seq[i])
                yield tuple(cl)
    else:
        yield tuple()

For fun, I decided to write a second implementation as a generator expression and came up with the following:

def combinations_by_subset(seq, r):
    return (tuple(itertools.chain(c, (seq[i], ))) for i in xrange(r - 1, len(seq)) for c in combinations_by_subset(seq[:i], r - 1)) if r else (() for i in xrange(1))

My questions are:

Which function definition is preferable? (I prefer the generator function over the generator expression because of legibility.)
Are there any improvements one could make to the above algorithm/implementation?
Can you suggest a better name for this function?

Rather converting from tuple to list and back again, construct a new tuple by adding to it.

def combinations_by_subset(seq, r):
    if r:
        for i in xrange(r - 1, len(seq)):
            for cl in combinations_by_subset(seq[:i], r - 1):
                yield cl + (seq[i],)
    else:
        yield tuple()


C++ - Graph implementation for data types other than int I am learning graphs and most of the implementations that I see are meant for int. So, I decided to code an example that can take other data types such as char or float. I did see this prior to posting my example (but not prior to coding my example). The example in the link is similar to creating a Tree where a separate Node class is used to keep track of node value, left and right child. In the link, instead of left and right child, the GraphNode class keeps track of neighbors.

My code does not use that approach and instead keeps the Adjacency List completely to Graph class, similar to usual implementations meant for int type. The idea is that if anyone needs to define a custom data type, it can be coded later (So, my code does not have a counterpart for GraphNode yet). The Graph is initialized by a vector of edges. The edges are represented by std::pair of nodes/ vertices involved. I have posted my code below. I am interested in knowing if there is a way to represent adjacency list, better than a map of lists, using stl containers, and can still handle different data types. However, any reviews or improvements or suggestions are welcome:

#include <iostream>
#include <vector>
#include <list>
#include <map>

template<typename T>
void print_list(std::list<T> l)
{
    typename std::list<T>::const_iterator it;
    for(it = l.cbegin(); it != l.cend(); it++)
    {
        std::cout << *it << "\t";
    }
    std::cout << std::endl;
}

template<typename T>
class Graph
{
    //Undirected Graph
    std::map<T, std::list<T>> adj_list;
public:
    Graph(std::vector<std::pair<T, T>>);
    void add_edge(std::pair<T, T>);
    size_t size();
    void print_adj_list();
};

template<typename T>
Graph<T>::Graph(std::vector<std::pair<T, T>> edges)
{
    for(size_t i = 0; i < edges.size(); i++)
    {
        this -> add_edge(edges[i]);
    }
}

//Undirected Graph
template<typename T>
void Graph<T>::add_edge(std::pair<T, T> edge)
{
        adj_list[edge.first].push_back(edge.second);
        adj_list[edge.second].push_back(edge.first);
}

template<typename T>
size_t Graph<T>::size()
{
    std::cout << "Size of Graph(no. of vertices): " << std::endl;
    return adj_list.size();
}

template<typename T>
void Graph<T>::print_adj_list()
{
    std::cout << "The Adjacency List: " << std::endl;
    typename std::map<T, std::list<T>>::const_iterator it;
    for(it = adj_list.cbegin(); it != adj_list.cend(); it++)
    {
        std::cout << it -> first << "\t";
        print_list<T>(it -> second);
    }

}
int main()
{
    /*std::vector<std::pair<int, int>> v;
    v.push_back({1, 2});
    v.push_back({2, 8});
    v.push_back({2, 5});
    v.push_back({2, 4});
    v.push_back({3, 4});
    v.push_back({5, 9});
    v.push_back({5, 7});
    v.push_back({5, 6});
    Graph<int> g{v};*/

    std::vector<std::pair<char, char>> v;
    v.push_back({'a', 'b'});
    v.push_back({'b', 'h'});
    v.push_back({'b', 'e'});
    v.push_back({'b', 'd'});
    v.push_back({'c', 'd'});
    v.push_back({'e', 'i'});
    v.push_back({'e', 'g'});
    v.push_back({'e', 'f'});
    Graph<char> g{v};
    g.print_adj_list();
    std::cout << g.size() << std::endl;
    g.add_edge({'i', 'k'});
    g.add_edge({'f', 'k'});
    g.print_adj_list();
    std::cout << g.size() << std::endl;
    return 0;
}

Pass parameters by const &

In print_list and your Graph constructor, you pass in the parameters by value.  This will make unnecessary copies of the vectors.  Passing in the values by const reference will avoid making these copies.

Make use of the auto keyword or range-based for loops

You have several places with a long type name to get the type to use for an iterator.  These would be great spots to make use of the auto keyword.  For example, auto it = l.cbegin().

However, in these cases, you can do better.  You can use the range-based for statement instead.

Combining these for the print_list function gives us

template<typename T>
void print_list(const std::list<T> &l)
{
    for (auto v: l)
        std::cout << v << '\t';
    std::cout << std::endl;
}

Similar changes can be made to your Graph constructor and print_adj_list.

You can also pass the pair to add_edge by const reference, although with the small types you're using that won't really gain much.

add_edge assumes that all your edges are bidirectional.  Some adjacency lists have edges that only go in one direction.

Do you need that cout in size?  All it is really doing is telling you that the function has been called.

You should be able to call print_list without having to specify the type.  The compiler should be able to deduce the template type to use.


Generic MemoryCache class I want to have a caching class to cache different types.  I want each type to be cached in a different MemoryCache but in a generic way.

Am I doing it right?

internal static class RecordsCache
{
    private static Dictionary<string, ObjectCache> cacheStore;
    static private CacheItemPolicy policy = null;

    static RecordsCache()
    {
        cacheStore = new Dictionary<string, ObjectCache>();

        ObjectCache activitiesCache = new MemoryCache(typeof(Activity).ToString());
        ObjectCache lettersCache = new MemoryCache(typeof(Letter).ToString());
        ObjectCache contactssCache = new MemoryCache(typeof(Contact).ToString());

        cacheStore.Add(typeof(Activity).ToString(), activitiesCache);
        cacheStore.Add(typeof(Letter).ToString(), lettersCache );
        cacheStore.Add(typeof(Contact).ToString(), contactssCache );

        policy = new CacheItemPolicy();
        policy.Priority = CacheItemPriority.Default;
        policy.AbsoluteExpiration = DateTimeOffset.Now.AddHours(12);
    }

    public static void Set<T>(string userID, int year, List<T> records)
    {
        var cache = cacheStore[typeof(T).ToString()];
        string key = userID + "-" + year.ToString();
        cache.Set(key, records, policy);
    }

    public static bool TryGet<T>(string userID, int year, out List<T> records)
    {
        var cache = cacheStore[typeof(T).ToString()];
        string key = userID + "-" + year.ToString();

        records = cache[key] as List<T>;
        return records != null;
    }

    public static void Remove<T>(string userID, int year)
    {
        var cache = cacheStore[typeof(T).ToString()];
        string key = userID + "-" + year.ToString();
        cache.Remove(key);
    }
}

Instead of having a cache

private static Dictionary<string, ObjectCache> cacheStore;

you could have one keyed of the types:

private static Dictionary<Type, ObjectCache> cacheStore;

This means you don't need to call typeof(T).ToString() everywhere.
You duplicate the code of building the key in all 3 methods - it should be extracted into a private BuildKey(string userId, int year) method. This means if you need to change how the key is built you only need to touch one method rather than all of them.
I would provide a Register method which accepts a type for which to create a new cache rather than hard-coding it in.
The cache policy is also not configurable - which means you are stuck with it unless you want to change the code.
You really really should think hard about whether a static cache is the right way to go. It will give you trouble in unit testing and it will also make it impossible for multiple classes to have different cache with different policies for example.


Factory for object init and configuration I want to extract the creation of different objects in a factory class for the reason of reusability. The problem is that within the current code the objects are configured also for the specific problem. Now I want to reuse only the creation of the objects within another code place.

 if (FieldConfiguration.FIELD_TYPE.TEXTFIELD == type)
            {
                TextField textField = new TextField(caption);
                textField.setNullRepresentation("");
                bind(textField, propertyId);
                field = textField;
                if (field.getValue() == null && isNestedProperty)
                {
                    ObjectProperty property = new ObjectProperty("", String.class);
                    textField.setPropertyDataSource(property);
                }
            }
            else if (FieldConfiguration.FIELD_TYPE.SEARCHFIELD == type)
            {
                SearchBox searchField = new SearchBox(caption);
                bind(searchField, propertyId);
                field = searchField;
            }
            else if (FieldConfiguration.FIELD_TYPE.DATEFIELD == type)
            {
                DateField dateField = new DateField();
                dateField.setCaption(caption);
                dateField.setDateFormat(UilibI18N.dateFormat.getText());
                dateField.setResolution(Resolution.DAY);
                bind(dateField, propertyId);

I want to reuse only the creation of the objects at another code place too, can anybody give me a hint what's the right way to extract the creation logic?

I find the right word in you question - 'extract'.

Code style

Logic section

Use blank lines to separate your code into logical sections

Instead of:

DateField dateField = new DateField();
dateField.setCaption(caption);
dateField.setDateFormat(UilibI18N.dateFormat.getText());
dateField.setResolution(Resolution.DAY);
bind(dateField, propertyId);

You can use:

// creation logic part
DateField dateField = new DateField();

// setting the field's properties logic part
dateField.setCaption(caption);
dateField.setDateFormat(UilibI18N.dateFormat.getText());
dateField.setResolution(Resolution.DAY);

// bind operation logic part
bind(dateField, propertyId);

Design issue

I read your code very carefully and I've found that you use 2 variables in all your 3 if/else conditions.

They are caption and propertyID.

Assume that these 2 properties will be an input of some method which we'll call (I don't know the context, that's why I'll use the random name) makeOperationWithField

public void makeOperationWithField(int propertyID, String caption) {
 // some code
}

I'll move deeply and remove this big if/else if statements.

Replace conditional with polymorphism.

Lets do it step by step.

Step1

Create an abstract class that will create objects depending on field's type.

abstract class FieldConfigurationType {
  static final int TEXTFIELD = FieldConfiguration.FIELD_TYPE.TEXTFIELD;
  static final int SEARCHFIELD = FieldConfiguration.FIELD_TYPE.SEARCHFIELD;
  static final int DATEFIELD = FieldConfiguration.FIELD_TYPE.DATEFIELD;

  abstract void makeOperationWithField(int propertyID, String caption);

  public static FieldConfigurationType newType(int code) {
    switch (code) {
      case TEXTFIELD:
        return new TextFieldConfiguration();
      case SEARCHFIELD:
        return new SearchFieldConfiguration();
      case DATEFIELD:
        return new DateFieldConfiguration();
      default:
        throw new IllegalArgumentException("Incorrect Field Type");
    }
  }
}

Now, we can use the simple construction:

FieldConfigurationType fieldType = FieldConfigurationType.newType(type);
fieldType.makeOperationWithField(propertyID, caption);

Step 2

Add separate implementation for each field type.

public class TextFieldConfiguration extends FieldConfigurationType {
  @Override public void makeOperationWithField(int propertyID, String caption) {
    TextField textField = new TextField(caption);
    textField.setNullRepresentation("");

    bind(textField, propertyId);

    field = textField;

    if (field.getValue() == null && isNestedProperty) {
      ObjectProperty property = new ObjectProperty("", String.class);
        textField.setPropertyDataSource(property);
    }
  }
}

public class SearchFieldConfiguration extends FieldConfigurationType {
  @Override public void makeOperationWithField(int propertyID, String caption) {
    SearchBox searchField = new SearchBox(caption);
    bind(searchField, propertyId);

    field = searchField;
  }
}

public class DateFieldConfiguration extends FieldConfigurationType {
  @Override public void makeOperationWithField(int propertyID, String caption) {
    DateField dateField = new DateField();

    dateField.setCaption(caption);
    dateField.setDateFormat(UilibI18N.dateFormat.getText());
    dateField.setResolution(Resolution.DAY);

    bind(dateField, propertyId);
  }
}

If you need to add a new field type, all you need to do is just add new subclass without rewriting the existing code. Open/Closed Principle


Safe Thread Synchronization I think the following is thread safe. Am I right? How would you improve this code? Are there new APIs or methods I can use?

Data:

public class Data {
  private int value;
  public Data(int value) {
    this.value = value;
  }

  public synchronized int getValue(int mTrhead) {
    System.out.println("MyTrhead " + mTrhead + " gets value " + value);
    return value;
  }

  public synchronized void setValue(int value, int mTrhead) {
    System.out.println("MyTrhead " + mTrhead + " sets value " + value);
    this.value = value;
  }
}

MyTrhead:

public class MyTrhead extends Thread {
  private Data mData;
  private int id;

  public MyTrhead(Data mData, int id) {
    this.mData= mData;
    this.id = id;
  }

  @Override
  public synchronized void run() {
    try {
      for (int a = 1; a <= 10; a++)
      {
        mData.setValue(mData.getValue(id) + 1, id);
      }
      Thread.sleep(1000);
    } catch (InterruptedException e) {
      e.printStackTrace();
    }
  }
}

Is it thread-safe? Yes
Is it going to do what you expect? Probably not
Is there a better way? Yes.
Safe
Your code will correctly lock, and access the mData value each time, and this makes it thread safe.... but, you will not get the results you expect.
Expectations
I suspect that your plan is to start, say, 10 threads, each of them incrementing from 0 through 9, adding 1 each time. You would expect the end result for the value to be 100.
It may not be. This is because, while your code is thread-safe, it is not atomic.
Atomic means that an operation starts and completes as a single logical step. You have the code:

mData.setValue(mData.getValue(id) + 1, id);

The apparent expectation is that you get the value, add one, and then set the new value. You would expect the value to increment from 0 through 10.
But, in a multi-threaded system, it may not. This is because two threads may each call mData.getValue(...) before the other has then reset the value... For example, consider the following:
Time      mData value  Thread 1  Thread 2
========  ===========  ========  ========

Initial             0

t1 get              0         0
t1 add              0         1
t1 set              1         1

t1 get              1         1
t1 add              1         2
*t2* get            1                   1
t1 set              2         2
t2 add              2                   2
t2 set              2                   2

Notice how the thread-2 can get in after the thread-1 has got the value, and before it has set the value?
What you need is an atomic operation, one which gets, adds, and sets the values all in one locked process. You can do it with your code using:
  for (int a = 1; a <= 10; a++) {
      synchronized(mData) {
          mData.setValue(mData.getValue(id) + 1, id);
      }
  }

That way all the get, add, and set operations are in a single synchronized block.
Better way
Note, in the above code, that we are synchronizing the block on the mData:

synchronized(mData) {
    ....

This illustrates a potential bug in your usage.... anyone can synchronize on your class. This may, or may not be a good thing. Your get and set methods are already synchronized on the mData instance, so now you have multiple classes all using the same synchronization point ("monitor").
The bottom line is that Java already identifies atomic operations to be a problem, and there is a subset of the standard library dedicated to ensuring that Atomic operations are safe. In your case, use AtomicInteger


Searching for a substring in a ring queue I am implementing a function which searches through a ring queue for a given substring. The function returns true if the substring is found, otherwise false. There is a cell containing null between the tail and head to signify the end of string. Please give feedback on this implementation.

bool SoftwareSerial::contains(const char *substr){
    char *pBuffer =_receive_buffer + _receive_buffer_head;
    char *pSubstr = substr;

    for(; *pBuffer != 0 && *pSubstr != 0; ++pBuffer ){
        if(pBuffer == (_receive_buffer + _SS_MAX_RX_BUFF)){
            pBuffer = _receive_buffer + _receive_buffer_head;
        }
        if(*pBuffer != *pSubstr){
            pSubstr = substr;
            continue;
        }
        ++pSubstr;
    }
    return (*pSubstr == 0);
}

There were few bugs in the function.
1. pStr should point to the start of the buffer once it reaches the end of the buffer.
2. A flag should be set when a character matches and unset the flag when next coming character doesn't match.
Here the modified version that I believe is correct:

bool Buffer::contains(const char *pcSubstr){

const char *pcCurrentSubstr = pcSubstr;
const char *pStr = buffer + head;
bool seen = false;
for (; *pStr != 0 && *pcCurrentSubstr != 0; ++pStr){
    if(pStr == (buffer + MAX_BUFFER_SIZE)){
        pStr = buffer;
    }
    if(*pStr != *pcCurrentSubstr){
        pcCurrentSubstr = pcSubstr;
        if(seen){
            --pStr;
        }
        seen = false;
        continue;
    }
    seen = true;
    ++pcCurrentSubstr;
}
return (*pcCurrentSubstr == 0);
}


Bash script to reconfigure the screen orientation when entering or exiting tablet mode I'm writing a bash script for my tablet laptop to flip the screen depending on a file in Manjaro Linux. My current script is the following:

while true
do
    state=$(cat /sys/devices/platform/thinkpad_acpi/hotkey_tablet_mode) #This file indicates if it's in tablet mode or not
    if [[ $state == 0 && $oldState == 1 ]]
    then
        xrandr --output LVDS1 --rotate normal
        xsetwacom --set "$a" Rotate none
        xsetwacom --set "$b" Rotate none
        xsetwacom --set "$c" Rotate none
        oldState=0
    elif [[ $state == 1 && $oldState == 0 ]]
    then
        xrandr --output LVDS1 --rotate inverted
        xsetwacom --set "$a" Rotate half
        xsetwacom --set "$b" Rotate half
        xsetwacom --set "$c" Rotate half
        oldState=1
        /home/eto/scripts/backlight 0
    fi
    sleep 1s
done

The code in the if statements don't matter, but what I'm worried about is the entire logic of how it checks the tablet mode. Is there a better way to check the tablet mode indicator file rather than a while loop? I feel like this is a very inefficient way of doing so as it reads the disk quite frequently, but I don't see how else I could approach this. Any input is appreciated as I'm very much an amateur with coding.

Good

I like most of what you've done so far as is:

Using [[ (double square brackets) for conditionals is a good practice.
Using $() for command substitution instead of the classic backticks is also a good modern shell practice.
Most of your variable substitutions are quoted.  This is a good habit in case the variable contains spaces it won't get broken up by shell parsing.

Could be better

There are some minor things I'd improve:

include the #! line at the top
typically you see folks add their thens to the end of the previous line like if [[ cond ]]; then.  Your outer loop could also be done as while true; do.
the comment gets sort of lost out on the right.  Why not put it on the line above?

Think about

The script is getting invoked every second and chewing up some amount of CPU.  There's probably not much CPU being burned in this case, but one thing to consider is whether this sort of polling could be avoided.  Linux includes inotify as a function to let processes known when a file has changed so they don't have to constantly check the file themselves.  This is available in the shell through inotifywait.  If the file you're checking works with inotifywait you could avoid the sleep.

A bonus you get beyond conserving CPU from skipping sleep is that the script should run with lower latency.  Instead of needing to wait for up to a second to detect the change your script would be invoked within a small fraction of a second.


Merging rows in excel using VBA For the first time I've worked in Excel VBA to find rows in my dataset that contain the same adress as another entry in a cluster. These entries have to be merged and the row then is deleted. I've come up with the following, which works (As far as I can tell from the testing I did on small samples of the set):

Sub Merge_Orders()

Application.ScreenUpdating = False

Application.DisplayStatusBar = False

Dim lastrow As Long
lastrow = Cells(Rows.Count, "A").End(xlUp).Row
Dim y As Long
Dim x As Long
Dim j As Long
Dim k As Long

For i = 2 To lastrow //for each row, starting below header row
j = 1

y = (Cells(i, 9)) //this is the clusternumber
Do While y = (Cells(i + j, 9)) //while the next row is in the same cluster
 x = (Cells(i, 12)) //this is the adresscode
 k = 1
    Do While x = (Cells(i + k, 12)) //while the current adresscode is the same as the next, iterating until another adresscode is hit
     Cells(i, 16) = Val(Cells(i, 16)) + Val(Cells(i + k, 16)) //update cell value
     Cells(i, 18) = Cells(i, 18) + Cells(i + k, 18)  //update cell value
     Cells(i, 19) = Cells(i, 19) + Cells(i + k, 19)  //update cell value
     If Cells(i, 20) > Cells(i + k, 20) Then
     Cells(i, 20) = Cells(i + k, 20)  //update cell value
    End If
    If Cells(i, 21) > Cells(i + k, 21) Then
     Cells(i, 21) = Cells(i + k, 21)  //update cell value
    End If
    Cells(i, 22) = Cells(i, 22) + Cells(i + k, 22)  //update cell value
    Cells(i, 23) = Cells(i, 23) + Cells(i + k, 23)  //update cell value

    Rows(i + 1).EntireRow.Delete //Delete the row from which data was pulled
    k = k + 1
    Loop
 j = j + 1
Loop

Next i

Application.ScreenUpdating = True
Application.DisplayStatusBar = True
End Sub

The problem I'm facing is time. Testing this on a small sample of ~50 rows took over 5 minutes. My entries total over 100K rows. I would expect it has to run for close to 10 days. It's been running for over a day with no end in sight.

Is there a way to optimize this so I don't have to wait until I'm grey?

Note: I've added //comments just for CR/SO, they are not in VBA.

Correctness

First of all, I have some concerns whether your code actually does what you want it to do. Let me explain why I think this.

In your inner Do While Loop you delete the next row, but still increment k. This means that you are skipping more and more rows. I think your intention was to always look at the next row, i.e. at k = 1.
In the inner loop you do not check whether you are still in the same cluster. So, you will end up aggregating over different clusters if the addresses match.
The inner loop does not depend on the middle loop.
Since you delete rows, your outer loop will run past the last remaining row.

Performance

Next, let me give some performance tips that do not change your code radically:

Accessing a worksheet in Excel VBA is expensive. You should load as much as possible into the memory at once. To achieve this you can load an entire range into a two dimentional Variant array by assigning the Value property of the range to a variant variable, or better the Value2 property. (See this blog post for some reason to use Value2.) You can write the array back to a range by assigning to the Value property of the range. If you go down this road, you will have to employ some collection to save the rows to delete for later. (When you remove the rows, you should do it from the bottom to the top as this guarantees that the row numbers of the remaining rows are preserved.)
Your middle loop is redundant. This is the case because the loop body does not depend on the index variable j. I think what you actually wanted to do is determine whether x = (Cells(i + 1, 12)) And y = (Cells(i + 1, 9)) and only do one loop. (I already set k and j to 1 in accordance with my first comment from above.)
You can save some redundant itterations by decrementing lastrow by 1 whenever you delete a row.

Style

Now, let me come to some general remarks regarding the programming style.

It is a very good practice to give variables meaningful names. This helps the next reader of your code a lot trying to understand what the code is doing. In your case, it would make most comments unnecessary. E.g. you could use currentCluster instead of y and currentAddress instead of x. Moreover, If you load the columns into variant arrays, you can call the array for row 9 clusterIds and that for column 12 adresses.
You should avoid to use the active sheet implicitely. You are referring to it with your calls to Cells. This can lead to very hard to detect errors. Instead you could explicitly use it by writing ActiveSheet.Cells or enclosing everything in a With ActiveSheet block and then using .Cells. Personally, I think the first alternative is better since the period can be overlooked or forgotten easily.
You should also avoid to use default properties, like the Value property of the range object returned by Cells. Using default properties does not only hurt the readability, it can also lead to obscure errors. Instead, you should use the Value property directly, or better Value2 when you are reading the data, especially when querying cells formatted as currency. These get crippled by Value. (Again, see this blog post.)
Always declare your variables. Otherwise, they are implicitly Variant. In your code, y and i are not declared. To ensure that you always declare variables, you can add Option Explicit. Then you get a compiler error for undeclared variables. This has the added advantage that you cannot inadvertedly introduce a new variable by a typo. (Such bugs are very hard to find.)
Get into the habit not to use underscores in method names unless there is a syntactic reason for it. This would be the case when writing an event handler or implementing an interface. The problem with underscores surfaces when using classes in VBA. You actually cannot implement a method of an interface if it contains an underscore. (This is a bug of the VBE.)

Alternative Approaches

Finally, I would like to present a completely different approach to your problem: to use the SQL support for Excel via ADODB. How to use it, can be found in this Microsoft article.

Using it the output you seem to want could be produced with a SQL statement similar to the following.

SELECT Cluster, Address, SUM(Value1), SUM(Value2), SUM(Value3)
FROM [ActiveSheetName$]
GROUP BY Cluster, Address

Here, ActiveSheetName should be the name of the active sheet, which you can get via ActiveSheet.Name and ValueX should be the names in the header row  of the columns you want to sum for matching clusters and addresses.

An alterntive approach would be not to use Excel at all and instead use a real database, e.g. Access for starters. They are made to handle requests like yours efficiently and also provide ways to enforce data integrity.


A utility class for fractions I've been working though Project Euler problems using JavaScript, and along the way I've found it useful to create "classes" for different types of numbers (like an integer that can be arbitrarily large). I recently decided to create a sort of class for Fractions to avoid floating point error.

I wrote the code using a more functional/procedural approach; however, I'm concerned that since JavaScript can't use types to distinguish functions that the function names will overlap with other code I write. I'm also looking for shorter names or better syntax to simplify how messy the math can get as shown in my tests. Here's a JSFiddle if you'd like to see the code in action.

General Global Functions

// Calculates the greatest common factor of two positive integers
function gcf(p, q) {
  if (p > q) return gcf(q, p);
  else if (p === 0) return q;
  else return gcf(q % p, p);
}

// Reduces two numbers by their greatest common factor
function gcd(p, q) {
  var f = gcf(p, q);

  return [
    p / f,
    q / f
  ];
}

Fraction Constructor

function createFraction(n, d) {
  if (d < 0) {
    n *= -1;
    d *= -1;
  }

  var f0 = reduce({
    n: n,
    d: d
  });
  return f0;
}

Fraction Utility Functions

function reduce(f0) {
  var f = gcf(Math.abs(f0.n), f0.d);
  f0.n /= f;
  f0.d /= f;
  return f0;
}

function invert(f0) {
  return createFraction(
    f0.d,
    f0.n
  );
}

function negate(f0) {
  return createFraction(
    f0.n * -1,
    f0.d
  );
}

Fraction Basic Math Functions

function multiply(f1, f2) {
  return createFraction(
    f1.n * f2.n,
    f1.d * f2.d
  );
}

function divide(f1, f2) {
  return multiply(f1, invert(f2));
}

function add(f1, f2) {
  var r = gcd(f1.d, f2.d);

  return createFraction(
    f1.n * r[1] + f2.n * r[0],
    f1.d * r[1]
  );
}

function subtract(f1, f2) {
  return add(f1, negate(f2));
}

Testing Code

function equals(f1, f2) {
  return f1.n === f2.n &&
    f1.d === f2.d;
}

function toString(f0) {
  return f0.n + "/" + f0.d;
}

function testEquals(line, f1, f2) {
  if (!equals(f1, f2)) console.log(line + " failed: " + toString(f1) + " !== " + toString(f2));
  else console.log(line + " good: " + toString(f1));
}

var line = 0;
testEquals(line++, add(createFraction(10, 40), createFraction(3, 30)), createFraction(7, 20));
testEquals(line++, subtract(createFraction(8, 3), createFraction(11, 30)), createFraction(69, 30));

testEquals(line++, multiply(createFraction(8, 3), createFraction(11, 30)), createFraction(88, 90));
testEquals(line++, divide(createFraction(8, 3), createFraction(11, 30)), createFraction(240, 33));

testEquals(line++, add(createFraction(-5, 8), createFraction(-1, -2)), createFraction(-1, 8));
testEquals(line++, subtract(createFraction(10, 40), createFraction(-3, 30)), createFraction(7, 20));

testEquals(line++, multiply(createFraction(-8, 3), createFraction(-11, 30)), createFraction(88, 90));
testEquals(line++, divide(createFraction(8, -3), createFraction(11, -30)), createFraction(240, 33));

I find a couple of the names misleading. What you call gcf I have only previously heard called HCF (highest common factor) or, more commonly, GCD (greatest common divisor). So I expect that function to be called gcd, and the one which is called gcd to be reduce. Of course, you also have a function called reduce which is basically just gcd with a different calling convention. I would be tempted to move the sign-handling into gcd, eliminate reduce, and then rename.

I'd also prefer to call invert something like recip (for reciprocal). It's less likely to cause a name collision, and also IMO describes better what it does.

function reduce(f0) {
  var f = gcf(Math.abs(f0.n), f0.d);
  f0.n /= f;
  f0.d /= f;
  return f0;
}

I presume that you modify the input and also return it because you want to support a fluent style, but I can't actually see any evidence of that here. I've already suggested refactoring this function away, but if I hadn't then I would suggest making it return a new object and treating fraction objects as immutable. That favours maintainability and probably also helps JIT.

function multiply(f1, f2) {
  return createFraction(
    f1.n * f2.n,
    f1.d * f2.d
  );
}

This function is worth special-casing in a fraction implementation. You already know that each fraction is reduced, so the only common factors can be crosswise. By separately reducing (f1.n, f2.d) and (f2.n, f1.d) you can keep the intermediate values smaller and reduce the risk of overflow. (I see you've made the corresponding optimisation in add).


Strictly typed dynamic proxy to call dynamic object GitHub and NuGet

I would like to use DynamicObject derived types to execute  REST API/NodeJS module calls. It would also help to capture API shape in a strictly typed manner, so here comes an interface proxy which hides low level machinery allowing to do tricks like this:

[TestMethod]
public void Call()
{
    dynamic c = new ExpandoObject();
    c.Add = (Func<int, int, int>)((a, b) => a + b);

    ICalculator proxy = Proxy.Create<ICalculator>(c);
    Assert.AreEqual(3, proxy.Add(1, 2));
}

public interface ICalculator
{
   int Add(int a, int b);
}

Library code is:

using Castle.DynamicProxy;
using Dynamitey;
using System;

namespace Dynoproxy
{
    public static class Proxy
    {
        public static T Create<T>(this object source) where T : class
        {
            var proxyGenerator = new ProxyGenerator();
            return proxyGenerator.CreateInterfaceProxyWithoutTarget<T>(
                ProxyGenerationOptions.Default,
                new Interceptor(source));
        }

        class Interceptor : IInterceptor
        {
            public Interceptor(object target) => Target = target;
            object Target { get; }
            public void Intercept(IInvocation invocation) =>
                invocation.ReturnValue = Dynamic.InvokeMember(
                    Target, invocation.Method.Name, invocation.Arguments);
        }
    }
}

It would be very useful to see the associated MethodInfo instance while implementing source in the situation like this:

interface IMyWebApi
{
    [Description("GET orders/{0}")]
    Order GetOrder(int id);
}

IWebApi api = Proxy.Create<IMyWebApi>(new RestApi("http://example.com/api"));
Order order = api.GetOrder(33);

So I made the proxy injecting it when DescriptionAttribute is provided:

[TestMethod]
public void Call()
{
    dynamic c = new ExpandoObject();
    c.Add = (Func<int, int, int>)((a, b) => a + b);
    c.Divide = (Func<MethodInfo, int, int, int>)((mi, a, b) => a / b);

    ICalculator proxy = Proxy.Create<ICalculator>(c);
    Assert.AreEqual(3, proxy.Add(1, 2));
    Assert.AreEqual(2, proxy.Divide(4, 2));
}

public interface ICalculator
{
    int Add(int a, int b);

    [Description]
    int Divide(int a, int b);
}

It should provide access to all interface method attributes and return types.

Updated Interceptor looks like this now:

class Interceptor : IInterceptor
{
    public Interceptor(object target) => Target = target;
    object Target { get; }
    public void Intercept(IInvocation invocation) =>
        invocation.ReturnValue = Dynamic.InvokeMember(
            Target,
            invocation.Method.Name,
            invocation.Method.IsDefined(typeof(DescriptionAttribute))
                ? invocation.Arguments.Prepend(invocation.Method).ToArray()
                : invocation.Arguments);
}


Merge Sort without pointers in C As part of an online course exercise, I was supposed to implement Merge Sort in C. Pointers were still not discussed prior to this exercise, so I have to work with arrays. Memory efficiency is not something I'm supposed to be focused on.

#include <cs50.h>
#include <stdio.h>

#define DEBUG

void getArray(int[], int);
void printArray(int[], int);
void mergesort(int[], int);
void merge(int[], int, int[], int, int[]);

int main(void)
{
    printf("Length of array: ");
    int len = GetInt();

    int array[len];
    getArray(array, len);

    printf("Before sort: ");
    printArray(array, len);

    mergesort(array, len);

    printf("After sort: ");
    printArray(array, len);

    return 0;
}

void getArray(int values[], int len)
{
    for (int i = 0; i < len; i++)
    {
        printf("Number %i: ", i + 1);
        values[i] = GetInt();
    }
}

void printArray(int values[], int len)
{
    for (int i = 0; i < len; i++)
    {
        printf("%i ", values[i]);
    }

    printf("\n");
}

void mergesort(int array[], int len)
{
    if (len <= 1)
        return;

    // right side and left side lengths
    int rlen, llen;

    //calculate lengths of the half-arrays
    rlen = len / 2;
    llen = len - rlen;

    int rhalf[rlen];
    int lhalf[llen];

    // copy first half of array into rhalf
    for (int i = 0; i < rlen; i++)
    {
        rhalf[i] = array[i];
    }

    // copy the remainder of array into lhalf
    for (int i = 0; i < llen; i++)
    {
        lhalf[i] = array[i + rlen];
    }

    #ifdef DEBUG
        printf("len: %i\nrlen: %i\nllen: %i\n", len, rlen, llen);
        printf("rhalf: ");
        printArray(rhalf, rlen);
        printf("lhalf: ");
        printArray(lhalf, llen);
    #endif

    mergesort(rhalf, rlen);
    mergesort(lhalf, llen);
    merge(rhalf, rlen, lhalf, llen, array);
}

void merge(int rhalf[], int rlen, int lhalf[], int llen, int array[])
{
    int i = 0, j = 0, k = 0;

    // iterate over rhalf and lhalf until one counter reaches its end
    while (i < rlen && j < llen)
    {
        if (rhalf[i] < lhalf[j])
        {
            array[k] = rhalf[i];
            i++;
        }

        else
        {
            array[k] = lhalf[j];
            j++;
        }

        k++;
    }

    // place the remaining elements (if any) in either half in their place
    while (i < rlen)
    {
       array[k] = rhalf[i];
       i++;
       k++;
    }

    while (j < llen)
    {
        array[k] = lhalf[j];
        j++;
        k++;
    }

    #ifdef DEBUG
        printf("merged: ");
        printArray(array, llen + rlen);
    #endif
}

What can I do to improve the code in terms of readability, style and performance?

What comes to performance, in order to merge two subranges, you copy them first and then merge. All in all, you make \$\Theta(n \log n)\$ array component copies. One thing to remedy this is to allocate the entire range before actual sorting, copy the input array contents to it and proceed making the array swapping: at one recursion level, you merge from one array to another, at the next recursion level you merge in opposite direction. See what I mean:

#include <stdio.h>
#include <stdlib.h>
#include <time.h>

static void getArray(int* values, int len)
{
    for (int i = 0; i < len; i++)
    {
        values[i] = rand() % 100;
    }
}

static void printArray(int* values, int len)
{
    for (int i = 0; i < len; i++)
    {
        printf("%i ", values[i]);
    }

    printf("\n");
}

static void merge(int* source_left,
                  int* source_middle,
                  int* source_end,
                  int* target)
{
    int* left      = source_left;
    int* right     = source_middle;
    int* left_end  = source_middle;
    int* right_end = source_end;

    while (left < left_end && right < right_end)
    {
        *target++ = *left < *right ?  *left++ : *right++;
    }

    while (left < left_end) *target++ = *left++;
    while (right < right_end) *target++ = *right++;
}

static void mergesort_impl(int* source, int* target, int len)
{
    if (len < 2) return;

    int left_range_length = len / 2;
    /* No copying here whatsoever. */
    mergesort_impl(target,
                   source,
                   left_range_length);

    mergesort_impl(target + left_range_length,
                   source + left_range_length,
                   len - left_range_length);

    merge(source,
          source + left_range_length,
          source + len,
          target);
}

static void mergesort_(int* array, int len)
{
    if (len <= 1) return;

    int* aux = malloc(sizeof(int) * len);
    memcpy(aux, array, sizeof(int) * len);
    mergesort_impl(aux, array, len);
    free(aux);
}

int main(void)
{
    int len = 10;
    srand(time(NULL));

    int* array = malloc(sizeof(int) * len);
    getArray(array, len);

    printf("Before sort: ");
    printArray(array, len);

    mergesort_(array, len);

    printf("After sort: ");
    printArray(array, len);

    return 0;
}