Find the first common ancestor of 2 nodes in a binary tree TreeNode* firstCommonAncestor(TreeNode* root, TreeNode* q, TreeNode* p){

if(root == p || root == q){
    return p;
}

if(root == NULL){
    return NULL;
}
TreeNode* temp = firstCommonAncestor(root->left,q,p);
TreeNode* temp1 = firstCommonAncestor(root->right,q,p);

if(temp1 != NULL && temp1 != p){
    return temp1;
}
if(temp != NULL && temp != p){
    return temp;
}

if((temp == NULL && temp1 != NULL)||(temp1 == NULL && temp != NULL)){
    return p;
}

if(temp != NULL && temp1 != NULL){
    return root;
}

return NULL;

}

I tested this quite thoroughly and it seems to work but looks quite messy. Can anyone give me input on how to improve the style and the performance?

I see some things that may help you improve your code.

Provide complete code

This is really more about getting a good review rather than anything particularly wrong with your code, but as was mentioned in one of the comments, having complete code and a short example definitely helps people both understand your code and shows the intended context.  These are both useful for both clarity and completeness of your question.

Consider a different algorithm

There is a much simpler way to go about this than either the code currently implements or has been suggested by others.  First, consider a post-order traversal.  In a post-order traversal, we visit left nodes, then right nodes, then root nodes.  Consider the following tree:

In it, left nodes are green and right nodes are red.  They are just the names of the months in English (an in-order traversal would display the months in alphabetical order.)  Imagine that we wish to find the first common ancestor of the "February" and "November" nodes.  The algorithm works as follows:

traverse the tree in post-order looking for one of the target nodes
after one of the target nodes is found, keep track of the lowest tree level encountered until the other target node is found
continue the traversal until a node is encountered that is one level above the highest one recorded in step 2.  That node is the first common ancestor.

This requires, at worst case, exactly one traversal of the tree which makes it considerably more efficient that the current algorithm.  In fact, it should be obvious that for an unordered tree, there is no faster algorithm possible.

I've created the following version of a TreeNode structure for this demonstration.

struct TreeNode {
    TreeNode *left;
    TreeNode *right;
    std::string label;
};

The traversal code looks like this:

enum states { LOOKING_FOR_FIRST, LOOKING_FOR_LAST, FINISHING, DONE };

TreeNode *postOrder(TreeNode *n, int depth, TreeNode *t1, TreeNode *t2, states &state)
{
    static int highest;
    static TreeNode *answer;
    if(n != nullptr)
    {
        postOrder(n->left, depth+1, t1, t2, state);
        postOrder(n->right, depth+1, t1, t2, state);
        switch (state) {
            case LOOKING_FOR_FIRST:
                if (n == t1 || n == t2) {
                    state = LOOKING_FOR_LAST;
                    highest = depth;
                }
                answer = nullptr;
                break;
            case LOOKING_FOR_LAST:
                highest = std::min(highest, depth);
                if (n == t1 || n == t2) {
                    state = FINISHING;
                    --highest;
                }
                break;
            case FINISHING:
                if (depth == highest) {
                    state = DONE;
                    answer = n;
                }
                break;
            case DONE:
                return answer;
        }
    }
    return answer;
}

The equivalent to your routine could be written like this:

TreeNode *fca2(TreeNode * root, TreeNode * q, TreeNode * p)
{
    if (root == NULL || q == NULL || p == NULL) {
        return NULL;
    }
    states state = LOOKING_FOR_FIRST;
    return postOrder(root, 0, q, p, state);
}

Now for test code:

testancestor.cpp

#include <iostream>
#include <string>

// code above goes here

int main()
{
    TreeNode tree[]{
        {&tree[1],  &tree[2],  "March"},                //  0
        {&tree[3],  &tree[4],  "January"},              //  1
        {&tree[5],  &tree[6],  "October"},              //  2
        {&tree[7],  &tree[8],  "August"},               //  3
        {&tree[9],  nullptr,   "June"},                 //  4
        {nullptr,   &tree[10], "May"},                  //  5
        {nullptr,   nullptr,   "September"},            //  6
        {nullptr,   nullptr,   "April"},                //  7
        {&tree[11], nullptr,   "February"},             //  8
        {nullptr,   nullptr,   "July"},                 //  9
        {nullptr,   nullptr,   "November"},             // 10
        {nullptr,   nullptr,  "December"},              // 11
    };

    // What is the common node for February and November?
    TreeNode *root = &tree[0];
    TreeNode *a = &tree[8];
    TreeNode *b = &tree[10];
    TreeNode *answer = fca2(root, a, b);
    std::cout << "common node of " << a->label
              << " and " << b->label << " is "
              << answer->label << "\n";
    // What is the common node for February and July?
    b = &tree[9];
    answer = fca2(root, a, b);
    std::cout << "common node of " << a->label
              << " and " << b->label << " is "
              << answer->label << "\n";
}

Results

common node of February and November is March
common node of February and July is January

Possible enhancements

Note that this test code is not particularly great, but I'm hoping that it's clear enough to demonstrate the basic algorithm.  Improvements could be made.  For example, the code doesn't currently actually stop the traversal when the ancestor node is found, but it certainly could.  Also, the static data would prevent this from being used as-is in a multithreaded environment.  A better alternative would likely be to encapsulate this within a class to avoid both the static variable and the global states.


Parsing playlists efficiently I have this regexp working, simple, but I feel like it may not be the best way to code it. Basically, I have a playlist separated by line breaks returned as tcp data like so:

file: http://192.168.100.214/Music/justchill.mp3
Artist: Brian G
Title: Just Chill
Track: 1
Date: 2014
Genre: Instrumental
Pos: 0
Id: 0
file: http://192.168.100.214/Music/justchill.mp3
Pos: 1
Id: 1
file: http://streaming.radionomy.com/ABC-Lounge
Pos: 2
Id: 2
OK

Now you can see the fields aren't always the same, but they always start with file and have the pos and id, always ending with id. I'm, parsing this data by line break to create an object, and then slicing it out of the string. Then I run the regexp on the new string, repeat while checking for Id: If it finds Id, I create a new object and start the whole process over.

It works fine, but I don't know, just feels like there may be a more efficient way to do it.

var parseplaylist = function(data)
{
    var pattern = /(.+): (.+)\n/;
    var result = pattern.exec(data);
    var playlistarray = [];
    if (result !== null)
    {
        var playlistobject = {};
        while (result !== null)
        {
            playlistobject[result[1]] = result[2];
            if (result[1] == "Id")
            {
                playlistarray[playlistobject.Pos] = playlistobject;
                playlistobject = {};
            }
            data = data.substr(result[0].length);
            result = pattern.exec(data);
        }
    }
    return playlistarray;
};

You can use the String.prototype.replace function to "loop" through all the relevant lines of the data. It accepts a regex pattern, and a function to handle the the matched text and return the replacement text. Here, however, we don't care about actually replacing anything; we just use it to find each key/value pair

function parsePlaylist(data) {
  var tracks = [], currentTrack;

  // do a multi-line replace-all for "(key): (value)", but don't
  // bother actually replacing anything, or storing the resulting
  // string. We're just using it to walk through the data
  data.replace(/^([^:]+):\s*(.+)$/gmi, function (match, key, value) {
    // if this is the start of a new track (indicated
    // by a "file" field), or the start of the loop,
    // create a new object to hold the data
    if( !currentTrack || key === "file" ) {
      currentTrack = {};
      tracks.push(currentTrack);
    }

    // add the key/value pair
    currentTrack[key] = value;
  });

  return tracks;
}

That'll give you this:

[ { file: 'http://192.168.100.214/Music/justchill.mp3',
    Artist: 'Brian G',
    Title: 'Just Chill',
    Track: '1',
    Date: '2014',
    Genre: 'Instrumental',
    Pos: '0',
    Id: '0' },
  { file: 'http://192.168.100.214/Music/justchill.mp3',
    Pos: '1',
    Id: '1' },
  { file: 'http://streaming.radionomy.com/ABC-Lounge',
    Pos: '2',
    Id: '2' } ]


Optimizing iteration through linked lists I have a data structure composed of several std::lists, each sorted by a specific criterion. When a new item is inserted into this data structure, there is a need for a reference item already in this data structure. Also all the items between the reference and insertee need to be collected into sets.

According to the profiler, this insertion is the bottleneck of the entire program. Here I present a simplified version of my insertion method.

upperReferenceSet_.clear();
lowerReferenceSet_.clear();
for (int objective = 0; objective < nObjectives_; ++objective)
{
    const  Real referenceValue = reference_->value(objective);
    const  Real  inserteeValue =  insertee_->value(objective);
    const bool lower = referenceValue >= inserteeValue;
    const  int directionTowardsInsertee = lower ? -1 : 1;
    Iterator it = referencePositions_[objective];

    ReferenceSet& referenceSet = lower ? lowerReferenceSet_:
                                         upperReferenceSet_;
    const  auto comparison = lower ? [](Real a, Real b){return a >= b;}:
                                     [](Real a, Real b){return a <= b;};

    while (comparison(it->value(), inserteeValue))
    {
        referenceSet.insert(it->individual());
        std::advance(it, directionTowardsInsertee);
    }

    lists_[objective].insert(it, Node(insertee_, inserteeValue));
}

Each list is of type std::list<Node> where the Node is defined by:

class Node
{
public:
    Node(Individual* ind, Real val)       :individual_(ind),value_(val){}
    const Individual* individual() const  {return individual_;}
    Real value() const                     {return value_;}

private:
    Individual* const individual_;
    const Real value_;
};

The Individual is an entity which has n values (objectives), where each value can be accessed by an Individual::value(int index) member function. There are n lists, each containing the same set of Individuals, each sorted by one objective (value). Also the objective pertaining to that list is cached in the node and can be accessed by the Node::value() member function.

I would like to ask how can I make this method faster. I suspect that there are three sources of slowness:

The access to reference sets indirectly using a C++ reference.
The use of lambda functions which produce another indirection.
The use of std::advance just to perform one step.

I am thinking of refactoring the function to pass the data structure in two passes, first for the upper reference sets and then for the lower ones. This would solve all three problems.

I am hesitating since it would result in some code duplicity. Also I am curious about any opinions on my theories and my code.

I haven't tested this solution, but I believe that your code could be simplified by using a reverse_iterator.

upperReferenceSet_.clear();
lowerReferenceSet_.clear();
for (int objective = 0; objective < nObjectives_; ++objective)
{
    const  Real referenceValue = reference_->value(objective);
    const  Real  inserteeValue =  insertee_->value(objective);
    const bool lower = referenceValue >= inserteeValue;
    Iterator it = referencePositions_[objective];
    if (!lower)
    {
        it = std::reverse_iterator<Iterator>(it);
    }
    ReferenceSet& referenceSet = lower ? lowerReferenceSet_:
                                         upperReferenceSet_;

    do
    {
        referenceSet.insert(it->individual());
    } while (it++->value() != inserteeValue);

    lists_[objective].insert(--it, Node(insertee_, inserteeValue));
}


Time complexity of common ancestor algorithm I have an algorithm here to find the common ancestor of two nodes in a binary tree.

def commonAncestor(self, nodeA, nodeB, root):
            #checking if nodeB is a descendant of nodeA
            checkLeft = self.checkSubtree(nodeA, nodeB, root.leftChild)
            checkRight = self.checkSubtree(nodeA, nodeB, root.rightChild)
            if checkLeft == 1 and checkRight == 1:
                return root
            else:
                if checkLeft == 2:
                    # pdb.set_trace()
                    result = self.commonAncestor(nodeA, nodeB, root.leftChild)
                if checkRight == 2:
                    result = self.commonAncestor(nodeA, nodeB, root.rightChild)
                else:
                    raise ValueError("One of the values you have entered is not in the binary tree")

            return result

def checkSubtree(self, nodeA, nodeB, node):
            res = 0 #res is being initialized as a separate variable each time
            if node is None:
                return 0
            if node is nodeA or node is nodeB:
                res += 1
            res += self.checkSubtree(nodeA, nodeB, node.leftChild)
            res += self.checkSubtree(nodeA, nodeB, node.rightChild)
            return res

I think that the runtime of this algorithm is \$O(\log^2 n)\$. The first algorithm will run \$O(\log n)\$ times because it recurses through the binary tree, touching every node, and it subsequently calls the second function on each recursion which again recurses \$O(\log n)\$ times, correct?

I know this is a not an efficient algorithm, so how do I go about improving this? Specifically, I want to know how to approach the problem of thinking about how to improve this versus just seeing the code of the improved version.

Definition of node class

class TreeNode():
    def __init__(self, val, left=None, right=None):
        self.value = val
        self.leftChild = left
        self.rightChild = right

    def insertLeftChild(self, val):
        self.leftChild = val

    def insertRightChild(self, val):
        self.rightChild = val

    def hasLeftChild(self):
        return self.leftChild is not None

    def hasRightChild(self):
        return self.rightChild is not None

The runtime of the code in the post is \$Ω(n)\$, even if the tree is balanced, because checkSubtree has to visit all the nodes in the subtree.

In the worst case (where none of the nodes has a left child), the code is the post is \$O(n^2)\$ for the same reason.

So if you are going to visit all \$n\$ nodes, you might as well do this in just one pass over the tree, to make sure that you don't have quadratic behaviour in the worst case. See this answer for how to go about it.


Check if an integer equals the sum of its digits to the fifth power I can do this in one line in Python:

sum([i for i in range(9,9999999) if i == sum([int(x)**5 for x  in str(i)])])

Not very fast, but works nicely. I thought it might be quicker to do it in Haskell, a language I'm quite new to:

-- helper func
toDigits    :: Int -> [Int]
toDigits 0 = []
toDigits x = toDigits (x `div` 10) ++ [x `mod` 10]

isSumofFifth n = n == sum (map (^5) (toDigits n))
sum (filter isSumofFifth [9..9999999])

But it seems as slow, or even slower (I haven't done exact profiling).

I realise I could optimise it with a more refined upper bound, but aside from that, is there a better way to write this in Haskell?

I'm not familiar with either Haskell or Python, but I'd like to challenge the way you're tackling this problem.

First of all, seven 9s will give you a sum of 7 * 95 = 413343. That's six digits, so searching up to one million (instead of ten million) would already be enough.

But we can do better. Instead of analyzing all million numbers, you can reduce that number by realizing that 123456 will give the same sum as 654321. The order of the digits doesn't matter. The sums you actually need to compute are the combinations with repetition; there are 'only' 5005 of them. Python has a standard function to list them in the itertools package, e.g. combinations_with_replacement('0123456789', 6).

When you have computed the sum of the combination, you need to sort its digits and check if they match the combination. If so, the sum is a true positive and can be added to the list.


Sorting a long string with a composite of strings and integers + symbols The code below is to sort a file containing the following info (eg. input.txt):

rango burrito apri%cot 1 -10 3.5 8 5 tesla 10 hyphen -4.7 2 bus 20 cat vul$ture m0nkey -9999999

The output should have the symbols removed and then strings and integers sorted in ascending order, but the order retaining the type of the original list. for example, the first item is a string in both input and output and the final item for example is an int.

When the script is run the output looks as follows:

$ ./sort_input.py input.txt
apricot burrito bus -9999999 -47 -10 1 2 cat 5 hyphen 8 10 m0nkey 20 rango tesla vulture 35

I've written code that looks as follows and I'm sure this can be improved a lot:

Reading the file first, then splitting on white space into an array of strings: \$O(n)\$ complexity where \$n\$ is the length of the original string

def listify(input_file):
    with open(input_file) as f:
        for line in f:
            list_of_strings = line.strip().split()

    return list_of_strings

Using the list of strings and converting it into a typed list, but removing any symbols first using the method below: \$O(n)\$ complexity for the typed list, then \$O(k)\$ for each string in that list to remove symbols - so total complexity is \$O(n)*O(k)\$.

def typed_list(untyped_list):
    """ converts an untyped list to a typed list of strings and integers """
    typed_list = []
    for item in untyped_list:
        item_without_symbol = remove_any_symbols(item)
        try:
            typed_list.append(int(item_without_symbol))
        except ValueError:
            typed_list.append(item_without_symbol)

    return typed_list

Method to remove any symbols, which I use in the function above. \$O(k)\$ complexity where \$k\$ is the length of the string:

def remove_any_symbols(s_string):
    """We take a string and remove any symbols from it. """
    acceptable_characters = string.ascii_letters + string.digits
    no_s_string_list = [c for c in s_string if c in acceptable_characters]
    if s_string.startswith("-"):
        return "-"+''.join(no_s_string_list)
    else:
        return ''.join(no_s_string_list)

I then use the typed list that's generated above to sort integers and strings separately. Then using the original list to generate a list with the same type items in their original order. \$O(n log n)\$ complexity for both sorting functions and then \$O(n)\$ for adding to the final output list.

def sort_em_up(no_symbol_list=None):
    """we take a list here, note the type, sort and then return a sorted
    list"""
    sorted_int = sorted([int(i) for i in no_symbol_list if isinstance(i, int)])
    sorted_str = sorted([s for s in no_symbol_list if isinstance(s, str)])
    final_sorted_list = []
    i = j = 0
    for item in no_symbol_list:
        if isinstance(item, int):
            final_sorted_list.append(str(sorted_int[i]))
            i += 1
        else:
            final_sorted_list.append(sorted_str[j])
            j += 1

    return ' '.join(final_sorted_list)

if __name__=="__main__":
    input_file = sys.argv[1]

    list_of_strings = listify(input_file)

    print(sort_em_up(typed_list(list_of_strings)))

listify function

Since, as you mentioned in the comments, the function is meant to read a single first line from a file only - you can use the next() built-in function:

def listify(filename):
    """Reads the first line from a file and splits it into words."""
    with open(filename) as input_file:
        return next(input_file).strip().split()

remove_any_symbols function

You can actually pre-define the allowed characters as a constant - no need to re-define them for every function call. You can also make it a set for faster lookups:

ACCEPTABLE_CHARACTERS = set(string.ascii_letters + string.digits)

def remove_any_symbols(input_string):
    """Removes any symbols from a string leaving the leading dashes."""
    filtered_characters = [c for c in input_string if c in ACCEPTABLE_CHARACTERS]
    prefix = "-" if input_string.startswith("-") else ""
    return prefix + ''.join(filtered_characters)

Or, a regex-based version (less understandable overall, but see if it is going to be faster):

PATTERN = re.compile(r"""
    (
        (?<!^)-  # dash not at the beginning of a string
        |  # or
        [^A-Za-z0-9\-]  # not letters, digits and dashes
    )+
""", flags=re.VERBOSE)
def remove_any_symbols(input_string):
    """Removes any symbols from a string leaving the leading dashes."""
    return PATTERN.sub("", input_string)

Pre-process the complete string

With regexes, it would also be possible to pre-process the input string as a whole, checking for the dashes at the beginning of words. This may lead to applying remove_any_symbols() on the complete input string read from a file:

PATTERN = re.compile(r"""
    (
        (?<!(?:^| ))-  # dash not at the beginning of a word
        |  # or
        [^A-Za-z0-9\- ]  # not letters, digits, dashes and spaces
    )+
""", flags=re.VERBOSE)
def remove_any_symbols(input_string):
    """Removes any symbols from a string leaving the leading dashes for each word."""
    return PATTERN.sub("", input_string)

if __name__=="__main__":
    input_file = sys.argv[1]

    with open(input_file) as f:
        data = next(f).strip()

    list_of_words = remove_any_symbols(data).split()

    print(sort_em_up(typed_list(list_of_words)))


Text centered in a circular component I have surrounded my initials with a circle. This is intended to be a re-usable component in a responsive layout. (When it is re-used the text will always be two letters, and the font won't change. It behaves sensibly when the browser zooms in and out, but I haven't yet considered if a different component size is needed for very small screens.)

I tried two approaches, firstly with flex layout, and secondly using the box model, and I'm not sure which I prefer.

.flex-profile-icon {
  display: flex;
  flex: 0 0 auto;
  width: 6em;
  height: 6em;
  border-style: solid;
  border-radius: 50%;
  justify-content: center;
}

.flex-profile-icon-text {
  align-self: center; /* center on cross-axis, i.e. vertically */
  font-size: 300%;
}

.trad-profile-icon {
  width: 6em;
  border-style: solid;
  border-radius: 50%;
  line-height: 0;
  position: relative;
}
.trad-profile-icon:after {
  content: "";
  display: block;
  padding-bottom: 100%; /* hack for equal height to width */
}
.trad-profile-icon-text {
  position: absolute;
  bottom: 50%;
  width: 100%;
  text-align: center;
  font-size: 300%;
}

/*
Copyright 2019-2020 by dcorking and Code Boxx
*/

  Aim is for the text to be centered in the circle

Flexbox layout

    DC

Traditional box layout

    DC

  Credit for the traditional box layout approach due to How to Create CSS Circles (And Add Text To It), W.S. Toh of Code Boxx

I will be interested in your recommendations on maintainability, cross-browser compatibility, and compatibility with layout frameworks such as Foundation (XY Grid or Float Grid) and Bootstrap.
Codepen

Try this approach. It uses CSS Grid layout. It requires less code and is easier to maintain.

.circle {
  display: grid;
  place-content: center;
  border: 0.3em solid black;
  border-radius: 50%;
  width: 6em;
  height: 6em;
}

.initials {
  font-size: 300%;
}

DC


String manipulation in Java Here is the question followed by my working program:

  Given a string, return a version without the first 2 chars. Except
  keep the first char if it is 'a' and keep the second char if it is
  'b'. The string may be any length. Harder than it looks.

deFront("Hello") → "llo"
deFront("java") → "va"
deFront("away") → "aay"

Can this code be improved?

public String deFront(String str) {
    if (str.length() >= 2) {
        char firstCharacter = str.charAt(0);
        char secondCharacter = str.charAt(1);
        if (firstCharacter == 'a' && secondCharacter == 'b') {
            // donot skip
            return str;
        } else if (firstCharacter == 'a' && secondCharacter != 'b') {
            // skip first character only
            return str.substring(0, 1) + str.substring(2);
        } else if (firstCharacter != 'a' && secondCharacter == 'b') {
            // skip second character only
            return str.substring(1);
        } else {
            return str.substring(2);
        }
    } else if (str.length() == 1 && str.charAt(0) == 'a') {
        return str;
    } else {
        return str;
    }
}

Just recoded it, doing simply what your explanation said

  Except keep the first char if it is 'a' and keep the second char if
  it is 'b'.

Since a possible deletion of a character will affect indices, we check the second character before we check the first.
The resulting code is much simpler and readable simply because the extracted method deleteCharIfNotEqualTo() does away with some code duplication and at the same time heightens readability of the deFront() method, by explaining what is happening.

public String deFront(String str) {
    StringBuilder builder = new StringBuilder(str);
    deleteCharIfNotEqualTo(builder, 1, 'b');
    deleteCharIfNotEqualTo(builder, 0, 'a');
    return builder.toString();
}

private void deleteCharIfNotEqualTo(StringBuilder builder, int index, char character) {
    if (builder.length() > index && character != builder.charAt(index)) {
        builder.deleteCharAt(index);
    }
}


dplyr -- tidyverse, pipes, correlation, same variable across time "a" "b" Have a look at this dataset. The"a" and "b" are used to make possible differentiate when the same variable was measured. In this case X1a and X1b access the same variable, but "a" was (suppose..) in the last year and "b" was this year.

I just want to correlate "a" and "b" and plot it. Simple like that and I really imagine the following code can be improved.

The final plot will be this one:

The data is fake, but it's virtually equal as the original dataset I'm working on.

 all_items <- data.frame("1a" = sample(1:5),
                            "2a" = sample(1:5),
                            "3a" = sample(1:5),
                            "1b" = sample(1:5),
                            "2b" = sample(1:5),
                            "3b" = rep(sample(1:5),10))

    #matrix with correlation
    all_correlation <- cor(all_items, method = "spearman") %>%
      as.data.frame()

    #filter
    all_correlation <- all_correlation %>% select(-c(ends_with("a"))) #columns

    #create a colum with the now name
    all_correlation <- all_correlation %>%
      mutate(item = row.names(.)) %>% select(item, everything())

    #supress some rows
    all_correlation <- all_correlation %>%  filter(!grepl("b", item))

    #filter(stringr::str_detect(row.names(.), "b"))
    #get only the diagonal
    all_correlation <- data.frame(item=1:3,Result=diag(as.matrix(all_correlation[, -1])))

    #P Value
    all_correlation_p_value <- Hmisc::rcorr(as.matrix(all_items))$P %>% as.data.frame()

    #filter
    all_correlation_p_value <- all_correlation_p_value %>% select(-c(ends_with("a")))
    all_correlation_p_value <- all_correlation_p_value %>% mutate(item = row.names(.)) %>% select(item, everything())
    all_correlation_p_value <- all_correlation_p_value %>%  filter(!grepl("b", item))
    all_correlation_p_value <- data.frame(item=1:3,P_Valor=diag(as.matrix(all_correlation_p_value[, -1])))

    #General table with the correlation  results
    all_correlation <- right_join(all_correlation,all_correlation_p_value, by = "item")

#Plot
ggplot(all_correlation, aes(x=item, y=Result)) +
  geom_point(aes(color=Result)) +
  geom_line() +
  annotate("text", x = all_correlation$item,
           y=all_correlation$Result,
           label = paste("P-value =",round(all_correlation$P_Valor,3)), hjust = -0.1, colour = "red") +
  scale_x_continuous(breaks = seq(1,3,1))

Here are two possible rewrites. For the first one, notice how Hmisc::rcorr already computes correlations ($r) and p-values ($P) matrices so you can base all your work from it, provided you extract the rights rows (a_vars) and columns (b_vars) and only keep the diagonal values:

correlations <- rcorr(as.matrix(all_items), type = "spearman")
a_vars <- ends_with("a", vars = names(all_items))
b_vars <- ends_with("b", vars = names(all_items))
all_correlation <- data.frame(
  item    = seq_along(a_vars),
  Result  = diag(correlations$r[a_vars, b_vars]),
  P_Valor = diag(correlations$P[a_vars, b_vars])
)

What I don't like too much about this approach is that it only uses 3 of the 36 correlations that were computed by rcorr. In this second approach, I start by splitting the data into two tables (one for a and one for b) so as to compute only 3 correlations via Map. I also switched from rcorr to the base cor.test which has (IMO) a more intuitive behavior: when given two vectors as input, it computes one correlation, not four.

a <- all_items %>% select(ends_with("a"))
b <- all_items %>% select(ends_with("b"))
cor_test <- Map(cor.test, a, b)
all_correlation <- data.frame(
  item    = seq_along(a),
  Result  = sapply(cor_test, `[[`, "estimate"),
  P_Valor = sapply(cor_test, `[[`, "p.value")
)


Django Team Sport class So this isn't EXACTLY my code but it is close enough to show what I have. Also the naming is only for this site so if its not really clear forgive me. Its much clearer in the actual code.

class Member(models.Model):
    team = models.IntegerField(default=0)
    match = models.ForeignKey(Match, null=True, blank=True)

    def engage(self, match_instance, team_number):
        self.match = match_instance
        self.team = team_number
        self.save()

class NPCGeneric(models.Model):
    speed = models.IntegerField(default=1)
    difficulty = models.IntegerField(default=0)

class NPCInstance(Member):
    generic = models.ForiegnKey(NPCGeneric);
    status = models.CharField(max_length=50)

    def engage(self, match_instance, team_number):
        new_instance = NPCInstance(generic=self)
        new_instance.save()
        return new_instance.engage(match_instance, team_number)

class Player(Member):
    name = models.CharField(max_length=100)

class Match(models.Model):
    event_members = []

    def __init__(self, *args, **kwargs):
        retval = super(Match, self).__init__(*args, **kwargs)
        if(args[0] and hasattr(args[0], '__iter__')):
            teams = args[0]
            args = args[1:]
            self.__init_new__(teams)
        elif(self.id):
            self.__init_existing__()
        return retval

    def __init_new__(self, teams):
        new_teams = []
        for team in teams:
            new_team = []
            new_teams.append(new_team)
            team_number = new_teams.index(new_team)
            for event_member in team:
                team_member = event_member .engage(self, team_number)
                team_member .save()
                new_team.append(team_member)
        self.event_members = new_teams

    def __init_existing__(self):
        new_teams = []
        def add_list(list, member, team):
            while(list.__len__() < (team + 1)):
                list.append([])
            list[team].append(member)

        for event_member in self.member_set.all():
            try:
                member = event_member.npcinstance
            except:
                member = event_member.player
            add_list(new_teams, member, member.team)
        self.event_members= new_teams

So this is where I start thinking that there has to be a better cleaner way. Unfortunately my brain just keeps getting stuck on what I already have.

I'd love to know if anyone can suggest a cleaner/Pythonic way for holding the Members in the Match class. specifically:

The initialising just seems very odd and clumsy. I'm afraid I will introduce errors this way.
a better way of representing teams other than a list of lists event_members

Sorry, it just seems to me that issues lie outside of the piece of code that you're suggesting to review. The code itself is more or less fine, but some assumptions behind it seem not right.

  I had thought about Team objects but I was hoping not to get too complex. i can't access the filtering in the templates either.

Well, you still have teams anyway, you're just passing them around "implicitly", by their numbers. IMO it actually makes it more complex, and is less "pythonic" (remember, explicit is better than implicit).

And, although I don't know the exact requirements, but maybe what you're trying to do in templates could be achieved another way?

Update in response to comment:

  Also I want to point out that the code works. It just doesn't "mesh" in my head well.

The code may work, but it looks very… unconventional. And in Python conventions mean a lot. And in Django they mean even more: it's a framework, and, as any other framework, it has lots of design decisions already made for you. It allows you to focus on your data structures and your business logic, but it does that by taking from you some freedom.

So when you're starting to write such complex initialization methods, then probably something is going wrong. I'd say this, and "implicit" teams, are cases when you're beginning to "fight the framework". It's usually the losing battle. The conventional way is to just have a Team class. It will also make code more maintainable, which is very important. (What if the requirements change and you'll need to store more information about the team, not only its number?)

  i am also trying to keep the number of different objects in the DB to a minimum but its not THAT important

Seems like premature optimization. I doubt that could be a performance bottleneck. Although it depends on your requirements.


Assigning a value to a class variable I have tried the following code:

SystemUser user = (SystemUser)Session["CurrentUser"];
Data.Configuration configuration = db.Configurations.Single(c => c.ID == user.ID);
MailAddress from = new MailAddress("noreply@test.com", "Test");
if (configuration.EmailFromAddress != null)
{
    from = new MailAddress(configuration.EmailFromAddress, "Test");
}

Here I declare the from variable of the MailAddress class with a default value, and if configuration.EmailFromAddress exists, I am assigning that value to the from.

Is this following best practice?

Here, the MailAddress class is calling twice to assign the value.  Is it good performance and memory wise? Another implementation is using a string variable:

SystemUser user = (SystemUser)Session["CurrentUser"];
Data.Configuration configuration = db.Configurations.Single(c => c.ID == user.ID);
string fromAddress = "noreply@test.com";
if (configuration.EmailFromAddress != null)
{
    fromAddress = configuration.EmailFromAddress;
}
MailAddress from = new MailAddress(fromAddress, "Test");

Which one follows best practice?

If you are sure that configuration.EmailFromAddress will always be valid or null, I'd suggest using the null-coalescing operator (??)

SystemUser user = (SystemUser)Session["CurrentUser"];
Data.Configuration configuration = db.Configurations.Single(c => c.ID == user.ID);
var fromAddress = configuration.EmailFromAddress ?? "noreply@test.com";
MailAddress from = new MailAddress(fromAddress, "Test");

In the case that configuration.EmailFromAddress can also be empty, I'd use the ternary operator

SystemUser user = (SystemUser)Session["CurrentUser"];
Data.Configuration configuration = db.Configurations.Single(c => c.ID == user.ID);
var fromAddress = String.IsNullOrEmpty(configuration.EmailFromAddress) ? configuration.EmailFromAddress : "noreply@test.com";
MailAddress from = new MailAddress(fromAddress, "Test");


Form validation using IIFE and closure Problem statement

  Write an IIFE in which we have to validate the input provided from a form using closures. The form consists of name, email, phone, address in which name, email and phone number should not be left blank. Also validate that the given email is a valid email id using JavaScript.

  Objective

  This assignment will help you to master the following concepts:

  function
  closures

Solution

<!DOCTYPE html>

        <title>Form validation</title>
        <meta charset="UTF-8">

Fill the form

            <form action="javascript:void(0)" method="POST" enctype="multipart/form-data">
                Last Name: <input type="text" placeholder="Last name" id="lastname" required >

                First Name: <input type="text" placeholder="First name" id="firstname" required >

                Email: <input type="email" placeholder="Email" id="email" required
                                        onblur="(function(id){
                                            var email_id = document.getElementById(id).value;
                                            function validate(){
                                                var regpat = /^[a-zA-Z_\.\-]+@[a-zA-Z_\-\.]+?\.[a-zA-Z]{2,4}$/;
                                                return regpat.test(email_id);
                                            }
                                            alert(validate());
                                        })('email');">

                Phone: <input type="tel" placeholder="Phone" id="phone" required>

                address: <input type="text" placeholder="Address" id="address">

                <input type="submit" value="Submit form">
            </form>

Can this solution be improved by avoiding inline JS?

Note: IIFE -Immediately-Invoked Function Expression

Semantics
Put the <meta charset="UTF-8"> before anything else that is being parsed. Some sources say it must be in the first 512 characters, some sources say it must be in the first 1024 characters. To be safe, it is wise to just put it at the start. (source)
You are using inline styling. I recommend to do this via CSS.
Events
Whenever an event is fired, the this keyword is set to the current element. Your use of id in the function definition in the onblur property is thus not required. If you insist on using a closure there, you could do this instead:
(function( element ) {
   //blah
 })( this );

You can avoid using inline javascript by binding an event to an element after the DOM has loaded. See this question on StackOverflow for more details on how to figure out when the DOM has loaded.
Input validation
Your solution does not actually do input validation. This can be easily demonstrated by changing the types of your input fields to "text" and trying to submit the form. Input validation should happen on the submit event (mdn) for a form. You could still use your blur event(s) to make this process easier.


First missing positive integer in linear time and constant space This problem was asked by Stripe.

  Given an array of integers, find the first missing positive integer in
  linear time and constant space. In other words, find the lowest
  positive integer that does not exist in the array. The array can
  contain duplicates and negative numbers as well.

  For example, the input [3, 4, -1, 1] should give 2. The input [1, 2,
  0] should give 3.

  You can modify the input array in-place.

class DailyCodingProblem5 {

    public static void main(String args[]) {
        int[] arr = { 3, 4, -1, 1 };
        int res = solution(arr);
        System.out.println(res);

        int[] arr2 = { 1, 2, 0 };
        res = solution(arr2);
        System.out.println(res);
    }

    private static int solution(int[] arr) {
        int n = arr.length;
        int i = 0;
        for (i = 0; i < n; i++) {
            int val = arr[i];

            if (val <= 0 || val > n)
                continue;
            while (val != arr[val - 1]) {
                int nextval = arr[val - 1];
                arr[val - 1] = val;
                val = nextval;
                if (val <= 0 || val > n) {
                    break;
                }
            }
        }
        for (i = 0; i < n; i++) {
            if (arr[i] != i + 1) {
                return i + 1;
            }
        }
        return n+1;

    }
}

How can i improve the above solution? Is there any improvements i can have in my code ?

It would be good to have some comments in the code explaining why (a) it works; (b) it takes linear time.

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

I would prefer to use for (int i = 0; ...) and similarly for the other loop over i: they don't need to use the same variable, and keeping scopes as small as possible helps understanding and maintenance. But if you prefer to keep the variable in the wider scope, there's no need to initialise it twice. Either of

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

or

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

works fine.

            if (val <= 0 || val > n)
                continue;
            while (val != arr[val - 1]) {
                int nextval = arr[val - 1];
                arr[val - 1] = val;
                val = nextval;
                if (val <= 0 || val > n) {
                    break;
                }
            }

It's a bit inelegant to apply the range test twice. A simple refactor gives

        while (val > 0 && val <= n && val != arr[val - 1]) {
            int nextval = arr[val - 1];
            arr[val - 1] = val;
            val = nextval;
        }

        }
        return n+1;

    }

For consistency I would add whitespace around +. The blank line makes more sense to me before the return rather than after it, and I would also add a blank line separating the two loops.

The fact that I'm nitpicking whitespace like this is a good sign: I haven't found anything which I consider to be a major problem.


Multi-user server connection handling I'm writing a program and I have a server that needs to be able to handle multiple client connections at once doing many different things.

I'm using the Qt library, so I handle new connections like this:

In the server constructor:

Server::Server(QObject* parent): QObject(parent)
{
  connect(&server, SIGNAL(newConnection()), //&server is a QTcpServer object
          this, SLOT(acceptConnection()));
  qDebug() << "[" << currentTime() << "] " << "Server started.";
  server.listen(QHostAddress::Any, PORT_NUMBER);
  qDebug() << "[" << currentTime() << "] " << "Server listening.";
}

The acceptConnection slot:

void Server::acceptConnection()
{
  client = server.nextPendingConnection(); //client is a QTcpSocket* object

  connect(client, SIGNAL(readyRead()), //When there is data to be read
    this, SLOT(startRead()));
}

The startRead slot:

void Server::startRead()
{

    char serverReceiveBuf[65536]; //A buffer for the data sent to the server
    client->read(serverReceiveBuf, client->bytesAvailable());
    handleConnection(serverReceiveBuf); //Do something with that data
}

I'm not going to give the handleConnection function because it's a bit long and I don't think it's necessary for the question.

My worry is latency. If I have one or two users, it's fine. But I haven't tested many concurrent users. There's a synchronization signal sent by the client that asks the server to verify the data the client has matches the data on the server. I'm sending this request every 250 ms, so with a few more concurrent users the client may end up waiting too long every 250 ms because of the other clients sending in synchronization requests.

At least, that's what I believe. Qt is pretty powerful, but I'm not entirely sure if when a signal is received that a new thread is automatically created to handle the connection. If I'm not mistaken, the current way I handle connections involves queueing, so that when the server receives a signal it'll process the requests in the order received. It's important to note that I don't call client->close() until the client terminates the connection themselves, so once client = server.nextPendingConnecting(); is called client will always exist until the client terminates it.

Is this a good way of handling multiple connections, given that I'll be regularly having users send synchronization requests often? If not, how should I handle the connections instead?

I was not able to find this question on SO, so let me try to give an answer here:

A general idea:

Depending on the nature of the data your are synchronizing, it might be better that the server sends out a synchronization signal and the clients synchronize to the data received from the server.

Specifics to the code of the question

I see two problems in the slot function acceptConnection() according to the documentation:

the function nextPendingConnection() may return 0. This may lead to a problem with the following connect statement. You should check for 0 response.
nextPendingConnection() will not return the exact same connection for which the signal was sent, it just returns "the next one". This may lead to a problem when multiple connections are made at the same time, or your code cannot handle one of them in time. It might be better that you handle all pending connections in a loop.

With these additions your code would perhaps look like this:

void Server::acceptConnection()
{
   while(client = server.nextPendingConnection()) //client is a QTcpSocket*
   {
      connect(client, SIGNAL(readyRead()), this, SLOT(startRead()));
   }
}

This brings us directly to the next problem: the client variable - I assume that this is a member variable of the Server class - will be overwritten for each new connection.

The Better approach is to follow the advice in the documentation and override incomingConnection(). We could subclass QTcpSocket and implement the handleConnection() functions from there. The documentation describes what the base class function normally does.


Am I using my data source array correctly? When I want to make some quick tests for my UITableViewControllers, I usually create an NSArray that holds some dummy data. I would like to know if I'm doing anything wrong here:

First in MasterViewController.h:

#import <UIKit/UIKit.h>

@class DetailViewController;

@interface MasterViewController : UITableViewController

@property (nonatomic, retain) NSArray *dataSourceArray;

@end

and then in MasterViewController.m:

#import "MasterViewController.h"

@implementation MasterViewController

@synthesize dataSourceArray = _dataSourceArray;

- (id)initWithNibName:(NSString *)nibNameOrNil bundle:(NSBundle *)nibBundleOrNil
{
    self = [super initWithNibName:nibNameOrNil bundle:nibBundleOrNil];
    if (self) {
        self.title = NSLocalizedString(@"Master", @"Master");
        _dataSourceArray = [[NSArray alloc] initWithObjects:@"obj 1", @"obj 2", @"obj 3", nil];
    }
    return self;
}

- (void)dealloc
{
    [_dataSourceArray release];
    [super dealloc];
}

So, the real question, as long as I'm not assigning _dataSourceArray to something else, I'm safe in terms of memory management here, right?

Yes. Everything seems correct here.

Like you mention yourself: make sure you use self.dataSourceArray to assign new values, and the synthesized setter will take care of memory management.


Checking if the access token is valid or expired Basically, the server gives me a token object. I am planning to check the token on the client if it is expired/valid before making a request to the server.
The function below works as expected. I just want to know if my code catch-all edge cases? I don't want, even 1 second that will request will fail due to an expired access token.
I am using moment for comparing dates. I also converted the current date and expiry date to UTC so that I'm making sure that they are in the same timezone before comparing them.
const moment = require('moment');

/**
The token object passed in the function looks like this:
{
  access_token: '2hbssMdXDpwQX5WcnZ-iJlO754MLkEeDCmF-f1A-MaU',
  token_type: 'Bearer',
  expires_in: 604800,
  refresh_token: 'VxnN9uBVIcNMpuwRVpvXo2YxWuNFEayHqfnCM7aCTSI',
  scope: 'public',
  created_at: 1603604241
}
*/

export default function tokenValid(token = {}) {
  const currentDate = moment().utc();
  const expiryDate = moment.unix(token.created_at).add(token.expires_in, 'seconds').utc();

  return currentDate < expiryDate;
}

EDIT (based on @hjpotter92's comment)
export default function tokenValid(token = {}) {
  const currentDate = moment().unix();
  const expiryDate = token.created_at + token.expires_in;

  return currentDate < expiryDate;
}

You seem to ignore two facts.

The client clock can be out of sync with the server clock.

The transport of the request from client to server takes more then zero time.

Both these cases mean that no matter how hard you try on the client side, there is always a chance that the server will evaluate the token as expired during its processing.
What you should do is have the client take the same action when server refuses the token as you do when your client side validation fails.
In general, client side validation

is not necesary
is not reliable
reduces network traffic
increases user experience

Whereas backend side validation

is necesary
is the only source of truth

Clients should understand and handle possible server side validation errors prior to deploying their own validation logic.


LikesControler for a Post I have this controller that I call via Ajax, where I update the likes of a post.

I have two methods in the user model:

already_like_post?: Where I pass the posted ID and return true or false if the post is already liked.
dislike_post: Delete the user like for given post.

class LikesController < ApplicationController
    def create
        if !current_user.already_liked_post? params[:post_id]
            like = Like. new user: current_user, post_id: params[:post_id]
            if !like.save
                render json: like.errors
            end
        else
           current_user.dislike_post params[:post_id]
        end
        amount_of_likes = Post.find(params[:post_id]).likes.count
        render json:{status:"success",likes: amount_of_likes}
    end
end

These are the methods in the user model:

def already_liked_post?(post_id)
    self.likes.where(post_id: post_id).size == 1
end

def dislike_post(post_id)
    self.likes.find_by(post_id: post_id).destroy
end

Can you give a review of this? I don't know if this looks good or if it is the Rails way.

Some notes:

Use 2-space indentation.
Move all the logic to the model.
DB deletes can also fail.
No need for self.
If you use object instead of object_id as argument, code is usually more declarative.
In case of error, you are performing a double render.

I'd write:

class LikesController < ApplicationController
  def create
    post = Post.find(params[:post_id])

    if current_user.toggle_like(post)
      render(json: {status: "success", likes: post.likes.count})
    else
      render(json: {status: "error"}, status: :unprocessable_entity)
    end
  end
end

class User < ActiveRecord::Model
  # Toggle like for post. Returns boolean with the status of the operation.
  def toggle_like(post)
    likes_for_post = likes.where(post_id: post.id)

    if likes_for_post.exists?
      likes_for_post.destroy_all.all?(&:destroyed?)
    else
      likes.new(post: post).save
    end
  end
end


Data structure and movement for tilt maze This is a design data structure and movement for a tilt maze. For example, this one.

My major idea is to divide into Ball, Maze, HorizontalWall and VerticalWall classes. Ball is the core class which operates on Maze, HorizontalWall and VerticalWall.

I feel my code is a bit inefficient since each time, I need to enumerate all vertical or horizontal walls, for move right/left/up/down. I'm not sure if there's a more efficient way to represent the data structure and movement implementation.

class VerticalWall: # something like |
    def __init__(self, x, y1, y2):
        self.x=x
        self.y_start = y1
        self.y_end = y2

class HorizontalWall: # soemthing like --
    def __init__(self, x1, x2, y):
        self.x_start = x1
        self.x_end = x2
        self.y = y

class Ball:
    def __init__(self, top, left, maze):
        self.top = top
        self.left = left
        self.maze = maze
    def moveRight(self):
        x_location=[]
        y_location=[]
        for i in maze.vertical_wall:
            x_location.append(i.x)
            y_location.append(i.y_start)
        while True:
            for i,x in enumerate(x_location):
                if self.left + 1 == x and y_location[i] == self.top: # met with a vertical wall
                    return
            if self.left+1==self.maze.width: # border
                    return
            else:
                self.left += 1
    def moveLeft(self):
        x_location=[]
        y_location=[]
        for i in maze.vertical_wall:
            x_location.append(i.x)
            y_location.append(i.y_start)
        while True:
            for i,x in enumerate(x_location):
                if self.left == x and y_location[i] == self.top: # met with a vertical wall
                    return
            if self.left == 0: # border
                return
            else:
                self.left -= 1
    def moveUp(self):
        x_location=[]
        y_location=[]
        for i in maze.horizontal_wall:
            x_location.append(i.x_start)
            y_location.append(i.y)
        while True:
            for i,x in enumerate(x_location):
                if self.left == x and y_location[i] == self.top: # met with a vertical wall
                    return
            if self.top == 0: # border
                return
            else:
                self.top -= 1
    def moveDown(self):
        x_location=[]
        y_location=[]
        for i in maze.horizontal_wall:
            x_location.append(i.x_start)
            y_location.append(i.y)
        while True:
            for i,x in enumerate(x_location):
                if self.left == x and y_location[i] == self.top+1: # met with a vertical wall
                    return
            if self.top + 1 == self.maze.height: # border
                return
            else:
                self.top += 1

class Maze:
    def __init__(self, height, width, target_x, target_y):
        self.height = height
        self.width = width
        self.target_x=target_x
        self.target_y=target_y
        self.horizontal_wall=[]
        self.vertical_wall=[]

if __name__ == "__main__":
    # https://www.mathsisfun.com/games/tilt-maze.html
    maze = Maze(5,5,4,4)
    maze.horizontal_wall.append(HorizontalWall(1, 2, 1))
    maze.horizontal_wall.append(HorizontalWall(4, 5, 2))
    maze.horizontal_wall.append(HorizontalWall(2, 3, 3))
    maze.horizontal_wall.append(HorizontalWall(1, 2, 4))
    maze.horizontal_wall.append(HorizontalWall(4, 5, 4))

    maze.vertical_wall.append(VerticalWall(3, 0, 1))
    maze.vertical_wall.append(VerticalWall(1, 2, 3))
    maze.vertical_wall.append(VerticalWall(3, 4, 5))

    ball = Ball(0,0,maze)
    print ('current location ', ball.top, ball.left)
    ball.moveRight()
    print ('current location ', ball.top, ball.left)
    ball.moveDown()
    print ('current location ', ball.top, ball.left)
    ball.moveLeft()
    print ('current location ', ball.top, ball.left)
    ball.moveUp()
    print ('current location ', ball.top, ball.left)

I think that for this situation you can just represent your maze as a matrix of boolean values. The boolean values can be used to indicate whether there is a wall for that position.

Here's an example (with true indicating that the right side of the cell doesn't have a wall):

  __ __ __ __
 |__   |__   |           [[true,  false, true,  false]
 |  |  |     |            [false, false, true,  false],
 |  |   __|  |      =>    [false, true,  false, false],
 |     |   __|            [true,  false, true,  false],
 |__|__|__ __|            [false, false, true,  false]]

The given example matrix includes values for vertical walls. I only need to include information about the right direction because I can look at the cell at (x - 1) to get wall information for the left side of the cell.

You'll need a separate matrix for horizontal walls (for a total of two matrices). The top, bottom, and left edges implicitly have a wall.

Using this representation, you should be able to calculate your end point in a time proportional to the distance between an end and start point. The ball only needs to know its (x, y) position.

Your walls likely don't have to be their own class for this problem. If you ever want to extend your program such that landing on specific types of walls (say a fire wall) results in different behavior, then objects may be helpful.

Even in that case, I would say that using an enum may be sufficient.


Small function for getting character value I want to extract some values in i-commas(") from lines like this:

<P k="9,0,1" vt="191" v="100.99936" z="" />

Example:

getCharVal ( cp, char "k=\"", 9)

where cp is a pointer the line above should return "9,0,1"

The function:

#define  ENDTAG   "/>"
#define ICOMMAS   '"'
char * getCharVal ( char *cp, char *att, size_t s)
{
  char * val;
  char * endTagP;
  if (cp == NULL)return NULL;
  cp = strstr(cp, att)+strlen(att);
  if (cp == NULL) return NULL;
  char * endP = strchr(cp, ICOMMAS);
  if (endP == NULL) return NULL;
  endTagP = strstr(cp, ENDTAG);
  if (endTagP == NULL) return NULL;
  if (endP > endTagP) return NULL;
  size_t valsize    = endP - cp ;
  if (valsize > s) return NULL;
  val = malloc(valsize + 1);
  memcpy (val, cp, valsize);
  val[valsize]='\0';
  cp = endTagP;
  return val;

}

This code is ugly.  Could anyone give me hints on how to write it better?

The easiest code to maintain is code that doesn't exist.  For parsing XML, use an XML-parsing library, preferably with XPath support.  Sometimes it might be justified to whip up your own code to extract values from XML, but it clearly does not make sense in this case.  Not only is the code ugly by your own admission, low-level string- and memory-manipulation also distracts you from thinking about the big picture.  Furthermore, your code will be less robust than a proper XML parser, and it will fail if the text is encoded in a semantically equivalent variant (for example, if a value is escaped).


Infinite monkey theorem demonstration in Python This was the assignment:

  Here’s a self check that really covers everything so far. You may have
  heard of the infinite monkey theorem? The theorem states that a monkey
  hitting keys at random on a typewriter keyboard for an infinite amount
  of time will almost surely type a given text, such as the complete
  works of William Shakespeare. Well, suppose we replace a monkey with a
  Python function. How long do you think it would take for a Python
  function to generate just one sentence of Shakespeare? The sentence
  we’ll shoot for is: “methinks it is like a weasel”

  You’re not going to want to run this one in the browser, so fire up
  your favorite Python IDE. The way we’ll simulate this is to write a
  function that generates a string that is 27 characters long by
  choosing random letters from the 26 letters in the alphabet plus the
  space. We’ll write another function that will score each generated
  string by comparing the randomly generated string to the goal.

  A third function will repeatedly call generate and score, then if
  100% of the letters are correct we are done. If the letters are not
  correct then we will generate a whole new string.To make it easier to
  follow your program’s progress this third function should print out
  the best string generated so far and its score every 1000 tries

Soo i have created a python program which generates the string provided, i am in learning phase, can someone please suggest any improvement or any tips or corrections.

Note: I know i have not did the exact method said in that question, but i have created the expected result.
makeRandSent: This will give me random character.
compare: This will give me final result by comparing each user character with randomly generated character and  it will only change those chracter which are not match.

def makeRandSent(userSent):
    counter = 0
    randSent = [""] * (len(userSent) - 1)
    randSent.clear()
    while counter < len(userSent):
        counter = counter + 1
        randSent.append(myalphlist[random.randrange(0,len(myalphlist) - 1)])
    return randSent

def compare(userSent):
    myBoolList = [False] * len(userSent)
    myRandSent = makeRandSent(userSent)
    print("random sent: ",myRandSent)
    myFinalList = [""] * len(userSent)
    while myBoolList.count(False) > 0:
        counter = 0
        myRandSent = makeRandSent(userSent)
        for char in myRandSent:
            if char == userSent[counter]:
                myBoolList[counter] = True
                myFinalList[counter] = char
            counter = counter + 1
        print(myBoolList," Final: ",myFinalList)
    return myBoolList

E.G:
Input: compare("pratik")
Output after several automatic attempts by program: [True, True, True, True, True, True]  Final:  ['p', 'r', 'a', 't', 'i', 'k']

In Python you don't need to declare your variables. So the randSent = [""] * (len(userSent) - 1); randSent.clear() part is completely unnecessary.

Second, Python has an official style-guide, PEP8, which programmers are encouraged to adhere to. It recommends using lower_case_with_underscores as variable and function names.

Python also has a built-in module called string, which has all lowercase characters. With it, generating a random sentence becomes a lot easier. I renamed this function generate, to adhere more closely to the requirements. It uses random.choice, which just chooses a random element from a sequence and _ as a placeholder for the unused loop variable.

import random
import string

ALLOWED_CHARS = string.ascii_lowercase + " "

def generate(n):
    return "".join(random.choice(ALLOWED_CHARS) for _ in range(n))

The requirements also mention a score function, which you have kind of implemented in your main function. I would implement it using sum and iterating over both sentences simultaneously:

def score(user_sentence, sentence):
    return sum(user_char == char for user_char, char in zip(user_sentence, sentence))

Alternatively, you could use the Levenshtein distance as score:

$ pip install python-Levenshtein

Which you can then use like this:

import Levenshtein

def score(user_sentence, sentence):
    return len(user_sentence) - Levenshtein.distance(user_sentence, sentence)

When testing these two score functions, the Levenshtein seems to be about twice as fast:

In [8]: s1 = generate(27)
In [9]: s2 = generate(27)
In [10]: %timeit score(s1, s2)
100000 loops, best of 3: 3.47 µs per loop

In [11]: %timeit score_levenshtein(s1, s2)
1000000 loops, best of 3: 1.75 µs per loop

The actual main function can be a lot simpler, I use itertools.count to have a loop that runs infinitely long and keeps track of how many iterations it has gone through.

I think your code has a bug, in that it seems to set a position as True if you ever found a string that has the correct character in that position. The requirements are quite clear that you should generate new sentence each time and discard the old characters (even if some were correct). I also implemented the output every 1000 iterations. I call the code with a if __name__ == "__main__": guard to allow importing parts of this script from another script.

import itertools

def compare(user_sentence):
    n = len(user_sentence)
    max_score = 0
    best_fit = " " * n

    sentence = generate(n)
    for i in itertools.count():
        if sentence == user_sentence:
            # We are done
            print i
            break

        # Calculate score of current sentence and update best score if necessary
        current_score = score(user_sentence, sentence)
        if current_score > max_score:
            max_score = current_score
            best_fit = sentence

        # Output every 1000 iterations
        if i % 1000 == 0:
            print i, best_fit, max_score

        # Get a new random sentence
        sentence = generate(n)

if __name__ == "__main__":
    compare("methinks it is like a weasel")

As alluded to in the problem statement, this takes quite a long time to run. Mine has been running about 10 minutes now and has gone through about 32 million iterations and has a maximum score of 11/27. But considering that there are \$27^{27} = 443426488243037769948249630619149892803\$ permutations for a 27 character long string with 27 possible characters at each position, this could take quite a bit longer...


Template function for splitting strings in C++ I'd like to write a function to split strings in C++ to check my template learning. I've also read some relavent questions in this site but most of them don't use templates or string_view. A related question in StackOverflow I asked is here.

My goal is to write a function such that:

The function can split std::basic_string, not only std::string.
The delimiter can be a CharT, or const CharT* p or std::basic_string<CharT>, multiple chars means the delimiter should be fully matched (like str.split in Python), not any of the chars.
The results are put into a sequence container from STL.
The user need not do any explicit conversion when parsing arguments, e.g. std::string_view(some_str).
(Optional) The function can split const CharT* p or any string literals.

My major concerns are

To minimize the number of the functions need to be overloaded.
To write template codes in a more modern C++ way. (For example, I've learn a lot by reading boost::algorithm::trim, many generic functions work on iterator ranges. But the boost version of split seems a bit complicate for me right now.)
How string_view in C++17 can help in this example. My version seems not take full advantages of it.
How to design a better function signature of the master template to relieve the pain when writing overloading?

Below is my first attempt.

#include <iostream>
#include <list>
#include <string>
#include <string_view>
#include <vector>

template <typename CharT, typename ContainerT>
void split(
    std::basic_string_view<CharT> str,
    std::basic_string_view<CharT> delimiters,
    ContainerT &conts) {
    conts.clear();
    std::size_t start = 0, end;
    std::size_t len = delimiters.size();
    while ((end = str.find(delimiters, start)) !=
           std::basic_string_view<CharT>::npos) {
        if (end - start) {
            conts.emplace_back(str, start, end - start);
        }
        start = end + len;
    }

    if (start != std::basic_string_view<CharT>::npos && start < str.size()) {
        conts.emplace_back(str, start, str.size() - start);
    }
}

template <typename CharT, typename ContainerT>
void split(
    const std::basic_string<CharT> &str,
    const std::basic_string<CharT> &delimiters,
    ContainerT &conts) {
    split(
        std::basic_string_view<CharT>(str),
        std::basic_string_view<CharT>(delimiters), conts);
}

template <typename CharT, typename ContainerT>
void split(
    std::basic_string<CharT> str, const CharT *delimiter, ContainerT &conts) {
    split(
        std::basic_string_view<CharT>(str),
        std::basic_string_view<CharT>(delimiter), conts);
}

template <typename CharT, typename ContainerT>
void split(
    const std::basic_string<CharT> &str, CharT delimiter, ContainerT &conts) {
    split(
        std::basic_string_view<CharT>(str),
        std::basic_string_view<CharT>(&delimiter, 1), conts);
}

template <typename Iter>
void print(Iter begin, Iter end) {
    for (auto it = begin; it != end; ++it) {
        std::cout << *it << "/";
    }
    std::cout << std::endl;
}

int main(int argc, char **argv) {
    std::string str("haha,ha,,haha,,ha,,,ha,");
    std::vector<std::string> strs;

    split(std::string_view(str), std::string_view(","), strs);
    print(strs.begin(), strs.end());

    split(str, std::string(","), strs);
    print(strs.begin(), strs.end());

    split(str, ',', strs);
    print(strs.begin(), strs.end());

    split(str, ",,", strs);
    print(strs.begin(), strs.end());

    return 0;
}

Well, the formatting is peculiar — I can’t see where the function begins.

ContainerT &cont
const CharT *delimiter

The style in C++ is to put the * or & with the type, not the identifier.  This is called out specifically near the beginning of Stroustrup’s first book, and is an intentional difference from C style.

std::size_t start = 0, end;
  ⋮
while ((end = str.find(delimiters, start)) !=
       std::basic_string_view<CharT>::npos)

⧺ES.10: Declare one name (only) per declaration.

Meanwhile, you can declare end just where it is needed, not ahead of time.

while (size_t end=str.find(delimiters, start); end != str.npos)

You can also make the clauses of the condition more readable by making the names short:  For example, see that I used the same instance str to get npos rather than its class name.

  The function can split std::basic_string, not only std::string.
  How string_view in C++17 can help in this example. My version seems not take full advantages of it.

std::basic_string_view<CharT> str,

Your input argument str will be able to take basic_string of any character type, but only using the default Traits.  You might do that on purpose, if Traits are never used other than the defaults?

A string_view will take a string_view, string, or C-style lexical string.

  The delimiter can be a CharT, or const CharT* p or std::basic_string<CharT>

So this first form will take the latter two types, as well as string_view.  You do not need to overload to take CharT delimiter separately.

You do not need to overload to take basic_string for both parameters, because the string_view will take those!

So, two of the overloads should just be removed.  The last one, that takes a CharT delimiter, needs to take the string_view for str so it will handle different types for that — you have it taking a basic_string only, which will be inefficient (copied!) if passed a const CharT* or string_view.

template <typename CharT, typename ContainerT>
void split ( ⋯, ContainerT& conts)

Don’t use “out” parameters.  Return values (⧺F.20).

I can see the appeal here of getting template argument deduction on the return value, but that does not make up for the lack of composibility.  You can add a default type to the ContainerT template parameter, which is handy for the case where the user doesn’t care.

template <typename CharT, typename ContainerT = vector<basic_string<CharT>>
ContainerT split ( ⋯ )

You are assuming that the container holds elements which have the form of constructor you are expecting.

Anyway, now your first example in main becomes:

auto strs = split (str, ",");

template <typename Iter>
void print(Iter begin, Iter end) {
    for (auto it = begin; it != end; ++it) {
        std::cout << *it << "/";
    }
    std::cout << std::endl;
}

    print(strs.begin(), strs.end());

You should just take the container, not separate begin/end.  That not only makes passing easier, but makes the function easier too!

template <typename R>
void print(const R& rng) {
    for (const auto& item : rng) {
        std::cout << item << "/";
    }
    std::cout << '\n';
}

    print(strs);

Oh, and don’t use std::endl.

I also should point out that it’s good practice how you have unit tests for your function!


DataLayer Interfaces Genericized I have some concerns on my refactoring for the data layer in my current Project. Just for a small info, I am currently building a CRM as "training JEE application". But enough of talk, let's talk code!

IService.java: (7 lines, 160 bytes)

public interface IService<T> {
    public void update(T update);
    public void delete(long id);
    public T getById(long id);
}

IListService.java: (8 lines, 169 bytes)

public interface IListService<T> extends IService<T> {
    public void add(T add);
    public List<T> getAll();
}

IDependentListService.java: (8 lines, 225 bytes)

public interface IDependentListService<T> extends IService<T> {
    void add(T entity, long masterRecordId);
    List<T> getAllByMasterRecordId(long masterRecordId);
}

IAddressService.java: (7 lines, 199 bytes)

public interface IAddressService extends IDependentListService<Address>{
    public void promoteToMainAddress(long addressId);
}

IContactPersonService.java: (7 lines, 167 bytes)

public interface IContactPersonService extends IDependentListService<ContactPerson>{

}

IContractService.java: (9 lines, 234 bytes)

public interface IContractService extends IDependentListService<Contract> {
    public List<Contract> getAllContractsByLoggedInUser();
}

ICustomerService.java: (9 lines, 220 bytes)

public interface ICustomerService extends IListService<Customer> {

}

IProductService.java: (10 lines, 262 bytes)

public interface IProductService extends IListService<Product> {
    public List<Product> getAllCurrentProducts();
    public List<Product> getAllArchivedProducts();
}

IProjectService.java: (9 lines, 228 bytes)

public interface IProjectService extends IDependentListService<Project>{
    public List<Project> getAllProjectsByLoggedInUser();
}

IUserService.java: (7 lines, 129 bytes)

public interface IUserService extends IListService<User>{

}

Update (Code):

The code for the IUserService changed a bit, after reviewing the implementation of the CustomerService:

IUserService.java:

public interface IUserService extends IListService<User>{
    User getLoggedInUser();
}

Questions

Does this code follow good practices, especially concerning inheritance?
Naming. Should I really prepend I before the Interfaces? (Implementations are named without I). Are the names clear enough?
Everything else you see as problematic ;)

Update (Questions):

I am currently in the process of writing Javadoc for this code after refactoring it. Currently I use the interfaces as a Platform to place the Javadoc. In the implementation I would then just @see to the Interface Javadoc. Is it okay to do this, should I instead write out the Javadoc for the Implementations too and just @see to other implementations and the interfaces?

This answer will reflect the naming convention at my work.

Naming Convention

Prefixing I to all your interface seems superfluous, since we use a simple name like ContractService. The implementation class would have Impl suffixed to the name of the service like : ContractServiceImpl. Normally you will use DI to inject the implementation where you need it, so you will only see the interface name.

Prefixing I seems odd to me if I'm looking at the Java collections API where you have : List, Map, etc.

If you only use ContractService as the name of the implementation, it could not be clear at first read that you are using an implementation directly and not the interface. Using Impl as a suffix make it clear that it's the implementation, so if you see it in your code, it can show that something smelly is going on.

Naming of methods

public interface IProductService extends IListService<Product> {
    public List<Product> getAllCurrentProducts();
    public List<Product> getAllArchivedProducts();
}

Those methods are very good! If you can keep clear names like those, using your services will be easy. I like the way you named all you method!

Delete

public void delete(long id);

You could return the object that has been deleted. It could be useful if you want to show the information of the item that has been deleted. The client could decide if he wants to use the object or not.

Edit: In fact, you should not return an object since you're just working with long id and not the object directly. If the signature was public void delete(T delete) this would have been a good advice, but with the current signature, you would have a to retrieve the object, delete it and then return the deleted object.


Padding a hexadecimal string with zeros I'm given a hexadecimal number in string form with a leading "0x" that may contain 1-8 digits, but I need to pad the number with zeros so that it always has 8 digits (10 characters including the "0x").

For example:

"0x123" should become "0x00000123".
"0xABCD12" should become "0x00ABCD12".
"0x12345678" should be unchanged.

I am guaranteed to never see more than 8 digits, so this case does not need to be handled.

Right now, I have this coded as:

padded = '0x' + '0' * (10 - len(mystring)) + mystring[2:]

It works, but feels ugly and unpythonic.  Any suggestions for a cleaner method?

Perhaps you're looking for the .zfill method on strings. From the docs:

Help on built-in function zfill:

zfill(...)
    S.zfill(width) -> string

    Pad a numeric string S with zeros on the left, to fill a field
    of the specified width.  The string S is never truncated.

Your code can be written as:

def padhexa(s):
    return '0x' + s[2:].zfill(8)

assert '0x00000123' == padhexa('0x123')
assert '0x00ABCD12' == padhexa('0xABCD12')
assert '0x12345678' == padhexa('0x12345678')


SSL Mutual Authentication, with one specific Root Certificate For a new server application we are developing; I implemented a method to verify SSL certificates.
The use of this method is Mutual Authentication: to prove that the server and client are who they say they are, using a Root Certificate and Intermediate Certificate that I've created myself.

The SslStream is created as such (server-side):

this.stream = new SslStream(tcpClient.GetStream(), false, SSLVerification.VerifyCertificate, null);
var certificate = new X509Certificate2(Config.Instance.CertificatePath, Config.Instance.CertificatePassword);
stream.AuthenticateAsServer(certificate, true, SslProtocols.Tls, false);

And Client side (almost) the same:

SslStream stream = new SslStream(client.GetStream(), false, SSLVerification.VerifyCertificate, null);
X509Certificate2 certificate = new X509Certificate2(Config.Instance.CertificatePath, Config.Instance.CertificatePassword);
stream.AuthenticateAsClient("our.server.name", new X509Certificate2Collection(certificate), System.Security.Authentication.SslProtocols.Tls, false);

Both snippets make use of the same library, in which the SslVerification.VerifyCertificate() can be found.

The implementation of that method is as follows:

public static bool VerifyCertificate(object sender, X509Certificate certificate, X509Chain chain, SslPolicyErrors sslPolicyErrors)
{
  chain.ChainPolicy.VerificationFlags = X509VerificationFlags.AllowUnknownCertificateAuthority;

  var rootCert = new X509Certificate2(X509Certificate.CreateFromCertFile(rootCertificate));
  var interCert = new X509Certificate2(X509Certificate.CreateFromCertFile(intermediateCertificate));

  chain.ChainPolicy.ExtraStore.Add(interCert);
  chain.ChainPolicy.ExtraStore.Add(rootCert);

  var isChainValid = chain.Build(new X509Certificate2(certificate));

  return isChainValid && chain.ChainElements.OfType<X509ChainElement>().Any(c => c.Certificate.Thumbprint == rootCert.Thumbprint);
}

After the SslStream has been set-up, we run a check (server-side) on the common-name to verify exactly which client we are talking to. We just simply look up the Common Name (Certificate.Subject) in a database table.

My questions are:

Is this the correct way to ensure that the certificate has been signed by our specific Root Certificate, and not any other signing certificate?
Is there any way to fool this implementation?
Maybe any other pitfalls in the implementation above?

The X509 certificate contains by itself a system to specify and verify chain and root certificates, by doing this you are actually creating a certificate without a proper root certificate but implementing a root certificate by yourself.
Looks like a waste of time just for the pleasure of doing something less secure...


Image Flip algorithm in C This is the algorithm I am using to flip an image in-place. Though it works perfectly, I would like if any one can spot any potential problems or offer any points on improvement and optimizations.

/**
 * pixels_buffer - Pixels buffer to be operated
 * width - Image width
 * height - Image height
 * bytes_per_pixel - Number of image components, ie: 3 for rgb, 4 rgba, etc...
 **/
void flipVertically(unsigned char *pixels_buffer, const unsigned int width, const unsigned int height, const int bytes_per_pixel)
{
    const unsigned int rows = height / 2; // Iterate only half the buffer to get a full flip
    const unsigned int row_stride = width * bytes_per_pixel;
    unsigned char* temp_row = (unsigned char*)malloc(row_stride);

    int source_offset, target_offset;

    for (int rowIndex = 0; rowIndex < rows; rowIndex++)
    {
        source_offset = rowIndex * row_stride;
        target_offset = (height - rowIndex - 1) * row_stride;

        memcpy(temp_row, pixels_buffer + source_offset, row_stride);
        memcpy(pixels_buffer + source_offset, pixels_buffer + target_offset, row_stride);
        memcpy(pixels_buffer + target_offset, temp_row, row_stride);
    }

    free(temp_row);
    temp_row = NULL;
}

A few comments:

you use signed/unsigned types inconsistently. Your width and height are
unsigned, while all other integers are signed.  If you are using unsigned
types because the values can never be negative, then the same applies to all
integers in the function and, by that reasoning, they should all be unsigned
(size_t is a common type used for sizes).  There are arithmetic errors that
can occur with mixed signed and unsigned arithmetic, so you should turn on
conversion warnings (eg -Wconversion for gcc).
your naming is inconsistent.  The function and the variable rowIndex use
"camel-case", while the other variables don't.
the return from malloc should not be cast in C (C++ requires casting).
source_offset and target_offset should be defined at the point of first
use (ie. within the loop).  There is no performance penalty for defining a
variable within a loop in C.
there is no check for success from malloc

Here is a slightly simplified version of the function

void flip_vertically(unsigned char *pixels, const size_t width, const size_t height, const size_t bytes_per_pixel)
{
    const size_t stride = width * bytes_per_pixel;
    unsigned char *row = malloc(stride);
    unsigned char *low = pixels;
    unsigned char *high = &pixels[(height - 1) * stride];

    for (; low < high; low += stride, high -= stride) {
        memcpy(row, low, stride);
        memcpy(low, high, stride);
        memcpy(high, row, stride);
    }
    free(row);
}

You can see that I have changed some types, shortened variable names (it is a
short function so short names are appropriate) and simplified the way buffer
offsets are calculated to just addition/subtraction.


Create a collection of binaries with random set positions I would like to create a collection of binaries with n set bits and total length should be between n and 31; also those set bits should be randomly positioned. For example if n = 3, then [1;0;1;0;1], [1;0;0;1;1;] and [1;1;1;0;0] should all be possible with equal chance.
Below is my implementation and any feedback is welcome.

/// <summary>
/// Create a random collection of binaries, between n and 31 elements, with n bits are set.
/// The set positions should be random too.
/// </summary>
/// <param name="n">The number of set bits in the collection; must be positive and less than 32</param>
let createRandomBinary (n : int) =
    if n > 31 || n < 0 then ArgumentException("invalid input: must be positive and less than 32", "n") |> raise
    let ran = new Random()
    let mutable totalLength = ran.Next(n, 32) // [n, 32)
    match totalLength with
    | x when x = n -> Array.init n (fun _-> 1) // all bits are set case
    | _ ->  let res : int array = Array.zeroCreate totalLength // initialise a result with all 0s.
            let indices = ResizeArray(seq{0..totalLength - 1})
            let mutable counter = n
            let rec shuffle = function
                            | 0 ->  res // all n 1s have been set randomly so return the result
                            | _ ->  counter <- counter - 1
                                    let i = ran.Next(0, totalLength) // randomly pick an index and set to 1
                                    res.[indices.[i]] <- 1
                                    indices.RemoveAt(i) // remove the chosen index so it won't be set twice
                                    totalLength <- totalLength - 1 // decrease the length for next random index pick
                                    shuffle counter
            shuffle counter

The logic to generate a length between n and 31 has nothing to do with creating a random list of a given length with a given number of set bits. Split them out into multiple functions.

This highlights the usefulness of making the randomness a parameter of the function, so do that too.

Use better names than n and x.

invalidArg is prettier.

Your matchtotalLength` is unneeded; the second path always works and the first isn't common enough to be worthwhile.

A for loop is a much better expression of intent when mutating res, and you can drop a lot of silliness by doing so.

By now we have

let generateBits (rand : Random) (length : int) (set_bits : int) =
    if not (0 <= set_bits && set_bits <= length) then
        invalidArg "set_bits" "must be nonnegative and no biger than length"

    let res = Array.zeroCreate length
    let indices = ResizeArray(seq{0..length - 1})
    for i in 0 .. set_bits - 1 do
        let random_index = rand.Next(0, length - i)
        res.[indices.[random_index]] <- 1
        indices.RemoveAt(random_index)
    res

let createRandomBinary (rand : Random) (max_length : int) (set_bits : int) =
    generateBits rand (rand.Next(set_bits, max_length)) set_bits

and your old function is equivalent to createRandomBinary (Random()) 32.

indices.RemoveAt is slow. Luckily we don't have to do it; we can get a random sample by using an array and just swapping the used elements to the end:

let indices = Array.init length id
for i in 0 .. set_bits - 1 do
    let random_index = rand.Next(0, length - i)
    res.[indices.[random_index]] <- 1

    let item = indices.[random_index]
    indices.[random_index] <- indices.[length - i - 1]
    indices.[length - i - 1] <- random_index
res

Then note that instead you can just set any bits of the array and shuffle it after:

let shuffle (rand : Random) (array : int []) =
    Array.iteri (fun i elem ->
        let index = rand.Next(i, Array.length array)
        array.[i] <- array.[index]
        array.[index] <- elem
    ) array

let generateBits (rand : Random) (length : int) (set_bits : int) =
    if not (0 <= set_bits && set_bits <= length) then
        invalidArg "set_bits" "must be nonnegative and no biger than length"

    let res = Array.init length (fun i -> Convert.ToInt32 (i < set_bits))
    shuffle rand res
    res

let createRandomBinary (rand : Random) (max_length : int) (set_bits : int) =
    generateBits rand (rand.Next(set_bits, max_length)) set_bits


Finding football team with the smallest difference in goals The file football.dat contains the results from the English Premier League for 2001/2. The columns labeled ‘F’ and ‘A’ contain the total number of goals scored for and against each team in that season (so Arsenal scored 79 goals against opponents, and had 36 goals scored against them). Write a program to print the name of the team with the smallest difference in ‘for’ and ‘against’ goals.

Please give me advice regarding best practices.

import java.io.File
import java.io.Reader

fun main(arg: Array<String>) {
    val inputStream = File("football.dat").inputStream()
    val reader = inputStream.bufferedReader().use { it.readLines() }
    //prints first Name in array.
    println(reader.first().team)
}

fun Reader.readLines(): List<teamAndScoreDifference> {
    val arrayOfTeamsAndGoals = arrayListOf<teamAndScoreDifference>()
    forEachLine {
        //if makePair(it) is not null,
        makePair(it)?.let {
            //add makePair(it)
            arrayOfTeamsAndGoals.add(it)
        }
    }
    val result = arrayOfTeamsAndGoals
            //by lowest (GoalsScored - GoalsTaken)
            .sortedBy { it.scoreDiffere }
    return result
}

data class teamAndScoreDifference(val team: String, val scoreDiffere: Int)

fun makePair(line: String): teamAndScoreDifference? {
    val teamName = line
            .take(23)
            .substringAfter(". ")
            .substringBefore(" ")
    val goalsScoredAndTaken: String = line
            .take(52)
            .substringAfterLast("   ")
    val goalsScored =
            goalsScoredAndTaken
            .substringBefore(" ")
            .toIntOrNull()
    val goalsTaken =
            goalsScoredAndTaken
            .substringAfterLast("  ")
            .toIntOrNull()
    if (goalsScored != null && goalsTaken != null) {
        return teamAndScoreDifference(teamName, Math.abs(goalsScored - goalsTaken))
    }
    //else return null
    return null
}

football.dat:

       Team            P     W    L   D    F      A     Pts
    1. Arsenal         38    26   9   3    79  -  36    87
    2. Liverpool       38    24   8   6    67  -  30    80
    3. Manchester_U    38    24   5   9    87  -  45    77
    4. Newcastle       38    21   8   9    74  -  52    71
    5. Leeds           38    18  12   8    53  -  37    66
    6. Chelsea         38    17  13   8    66  -  38    64
    7. West_Ham        38    15   8  15    48  -  57    53
    8. Aston_Villa     38    12  14  12    46  -  47    50
    9. Tottenham       38    14   8  16    49  -  53    50
   10. Blackburn       38    12  10  16    55  -  51    46
   11. Southampton     38    12   9  17    46  -  54    45
   12. Middlesbrough   38    12   9  17    35  -  47    45
   13. Fulham          38    10  14  14    36  -  44    44
   14. Charlton        38    10  14  14    38  -  49    44
   15. Everton         38    11  10  17    45  -  57    43
   16. Bolton          38     9  13  16    44  -  62    40
   17. Sunderland      38    10  10  18    29  -  51    40
   -------------------------------------------------------
   18. Ipswich         38     9   9  20    41  -  64    36
   19. Derby           38     8   6  24    33  -  63    30
   20. Leicester       38     5  13  20    30  -  64    28

Class names in Java or Kotlin always start with a capital, so TeamAndScoreDifference.  It makes it difficult to read the code otherwise because I assume it's a variable.

The name of readLines() is not quite right since it does many other things.  It does 3 things: read the lines, transform them to pairs and sorts them.  It's awkward that the sorting is in this method, but picking the first element was left in the main method.

I don't like the pattern that you use in the body of readLines() where you initialize an empty list and then fill it with a for-loop (a forEachLine actually).  This is what is usually done in C and Java, but modern functional languages proceed differently.  The pattern that is being executed here is just a mapping: you take a list of objects and return a list of modified objects.  That is done better with the map function; see my code below.

Also it's not clear why your readLines() should be an extension method and not just a plain function.

You don't need to define this function anyway since File.readLines() already exists.

Other programmers might have used a plain Pair instead of a data class TeamAndScoreDifference.  But I'm not recommending one way or another.

makePair could probably have used a better name , but I can't come up with one right now.  You could have used pattern matching to extract the values from the text.  If you don't know how to use regex you will have to learn it at some point as a programmer, either in Java, Kotlin, or any other language.  But it seems that your file has columns of predetermined width, so your solution does work as it is.

The if-else clause in Kotlin is an expression which returns a value, so you can make the final return less awkward at the end of makePair.

You forgot to consider that the answer might not be unique.

You write too many comments.  They just describe what the code is doing.  You can put official documentation for a public API, otherwise you only put comments to explain something quite complicated that another programmer might have a hard time comprehending.  So there are very rarely comments within the code.  Another problem with comments is that they can sometimes be contradictory with the code.  You have such an instance with
//by lowest (GoalsScored - GoalsTaken) where the code instead uses the absolute value.

import java.io.File

fun main(arg: Array<String>) {
    val lines = File("football.dat").readLines()
    val teamsWithScoreDiff = lines.map(::makePair).filterNotNull()
    val minDiff = teamsWithScoreDiff.map(TeamAndScoreDifference::scoreDiffere).min()
    val minTeams = teamsWithScoreDiff.filter { it.scoreDiffere == minDiff }
    println("Team(s) with minimal score difference of %minDiff: %{minTeams}")
}

data class TeamAndScoreDifference(val team: String, val scoreDiffere: Int)

fun makePair(line: String): TeamAndScoreDifference? {
    val teamName = line
            .take(23)
            .substringAfter(". ")
            .substringBefore(" ")
    val goalsScoredAndTaken: String = line
            .take(52)
            .substringAfterLast("   ")
    val goalsScored =
            goalsScoredAndTaken
                    .substringBefore(" ")
                    .toIntOrNull()
    val goalsTaken =
            goalsScoredAndTaken
                    .substringAfterLast("  ")
                    .toIntOrNull()
    return if (goalsScored != null && goalsTaken != null)
        TeamAndScoreDifference(teamName, Math.abs(goalsScored - goalsTaken))
    else
        null
}


.NET - multiple DLLs and one namespace Please have a look at the code below:

ClassLibrary1 has a structure as follows:

Namespace com.app.test
Public Class Class1

End Class
End Namespace

ClassLibrary2 has a structure as follows:

Namespace com.app.test
Public Class Class2

End Class
End Namespace

WindowsApplication1 has a structure like this:

Imports com.app.test

Public Class Form1
    Dim c1 As Class1
    Dim c2 As Class2
End Class

Notice that Class1 and Class2 are in different projects (DLL's), but they have the same namespace.  Is this poor practice? I have never seen it done before.

It is indeed better component design as @Tyriar mentioned, but it is also very common that the namespaces are being used among the projects. This is not always a wrong design.

Let assume that you are implementing plug-ins. And every plug in has own assembly (normally). There, namespaces can be shared.

PluginA.dll

Namespace CompanyName.SolutionName.ProjectName.PlugInNamespace
{
    Class PlugInA {}
}

PluginB.dll

Namespace CompanyName.SolutionName.ProjectName.PlugInNamespace
{
    Class PlugInB {}
}

Better practice for your case would be the sub-namespacing.

ClassLibrary1

Namespace Com.App.Test.FunctionalityGroupA
Public Class Class1

End Class
End Namespace

ClassLibrary2

Namespace Com.App.Test.FunctionalityGroupB
Public Class Class2

End Class
End Namespace

WindowsApplication1

Imports Com.App.Test.FunctionalityGroupA
Imports Com.App.Test.FunctionalityGroupB

Public Class Form1
    Dim c1 As Class1
    Dim c2 As Class2
End Class


Thread test mutex implementation I am trying to make a thread test in which 4 threads are all trying to set a bool. Only one should be allowed to set it. After the first thread has set the bool (m_isRunning) to true, no other threads should set it to true because of the check isRunning().

This implementation works, but I wonder if this is the best/most efficient approach to this common situation.

#include<iostream>
#include<thread>
#include<mutex>

std::mutex mut;
bool m_running = false;

bool isRunning()
{
  return m_running;
}

void setRunning(bool running)
{
  m_running = running;
}

void func1(bool isSet)
{
  mut.lock();
  if(!isRunning())
  {
    setRunning(true);
  }
  mut.unlock();
}

int main()
{
  std::thread t1(func1,true);
  std::thread t2(func1,true);
  std::thread t3(func1,true);
  std::thread t4(func1,true);

  t1.join();
  t2.join();
  t3.join();
  t4.join();

  return 0;
}

Edit: Changed !isRunning to !isRunning() (typo)

In this case, you really just need an atomic variable, and you can do a compare_exchange_strong to set the value once:

#include <thread>
#include <iostream>

std::atomic_bool m_running;
bool wrote[4];

void func1(int num) {
    bool f{ false };
    if (m_running.compare_exchange_strong(f, true))
        wrote[num] = true;
}

int main(){
    std::vector<std::thread> t;

    for (int i = 0; i < 4; i++)
        t.emplace_back(func1, i);

    for (auto &t1 : t)
        t1.join();

    for (bool b : wrote)
        std::cout << std::boolalpha << b << "\n";
}

Given that thread 1 starts running first, and it's not doing very much, chances are that the first thread will do the write to the variable virtually every time. In case you're not familiar with it, atomic_bool is (obviously enough) an atomic Boolean type, and compare_exchange_strong does an atomic comparison and exchange. Specifically, it compares the current value to the first parameter, then if (and only if) they're equal, sets the value to the second parameter. The first parameter has to be passed by reference, because it also writes the new value to that variable. Finally, it returns true if and only if it actually wrote to the variable. In this case I've used that to record which thread actually wrote to the variable, so the final printout should show true once, and false three times.

Looking at the code as you've written it:

void func1(bool isSet)
{
  mut.lock();
  if (!isRunning())
  {
    setRunning(true);
  }
  mut.unlock();
}

You almost certainly want to use an std::lock_guard to automate unlocking the mutex. Right now, the code also ignores the isSet parameter, and always sets the value to true. I'd assume you really want to compare to isSet, and set it to that value if it's not already equal to it. Combining these, the code would come out something like:

void func1(bool isSet) {
     std::lock_guard g(mut);

     if (isRunning() != isSet)
        setRunning(isSet);
}

That's somewhat simpler, but I still think the atomic_bool with compare_exchange_strong is neater. The atomic variable can degenerate to pretty much what you've done with a mutex in the worst case, but in quite a few cases, it'll be substantially more efficient.