Count unique characters in a String and group the counts The code counts the amount of unique characters that fit in the character set ACGT (stemming from nucleobases).

We take a string as input that must contain only either 'A', 'C', 'G', or 'T'.

For instance, "AACCC" should return the Map that looks like this:
Map('A' -> 2, 'C' -> 3, 'G' -> 0, 'T' -> 0).
More detail can be found in a gist with specs.

class DNA(code: String) {
  val nucleotides = Map('A' -> 0, 'T' -> 0, 'C' -> 0, 'G' -> 0)

  code foreach valid_nucleotide

  def nucleotideCounts(): Map[Char, Int] = {
    nucleotides ++ code.filter(nucleotides.contains(_)).groupBy(identity).mapValues(_.length)
  }

  def count(nucleotide: Char): Int = {
    valid_nucleotide(nucleotide)
    nucleotideCounts()(nucleotide)
  }

  private def valid_nucleotide(char: Char) = {
    if (!nucleotides.contains(char)) {
      throw new IllegalArgumentException("DNA is wrong");
    }
  }

}

For the most part, there isn't much that I would change - well done!

First, minor things:

Remove the semi-colon in valid_nucleotide.
Next, you can change nucleotides.contains(_) (an anonymous function) to just nucleotides.contains (a method value).
The reuse of valid_nucleotide in count will result in any call with an invalid parameter getting an IllegalArgumentException("DNA is wrong"). I think this error would be confusing in this context.

The biggest single change I would make is to precompute the nucleotide counts. Currently, the code recomputes them each and every time either count or nucleotideCounts is called. Since code is immutable, it is only necessary to compute them once.

In a Java program, the "empty" nucleotides map could be made into a static member (rather than having a copy of it in every instance of the DNA class). The equivalent approach in Scala is to use a companion object (an object with the same class name as the class it is a companion to).

My version of your code looked like this:

object DNA {
  val nucleotides = Map('A' -> 0, 'T' -> 0, 'C' -> 0, 'G' -> 0)
}

class DNA(code: String) {

  code foreach validNucleotide

  val counts = countNucleotides()

  def nucleotideCounts(): Map[Char, Int] = counts

  def count(nucleotide: Char): Int = {
    validNucleotide(nucleotide)
    nucleotideCounts()(nucleotide)
  }

  private def countNucleotides(): Map[Char, Int] = {
    DNA.nucleotides ++ code.filter(DNA.nucleotides.contains).groupBy(identity).mapValues(_.length)
  }

  private def validNucleotide(char: Char) = {
    if (!DNA.nucleotides.contains(char)) {
      throw new IllegalArgumentException("Character not found in valid nucleotide set")
    }
  }
}


Returning all possible player moves on a board Recently I started to learn Haskell. I did this by making exercises on the Internet. The problem of making these exercises is that I never know if I solved it the correct way. I currently wrote a function which I think looks really ugly, but I don't know how to improve it. I hope that someone could give me some tips to improve it.

In this exercice I need to make a tic-tac-toe-game. Wikipedia

The question of the function I wrote is as follows:

  Write a function moves :: Player -> Board -> [Board] that, given the
  current player and the current state of the board, returns all
  possible moves that player can make expressed as a list of resulting
  boards. (For now, you should continue making moves, even if one of the
  players has already won.)

I wrote the following code:

data Field = X | O | B
    deriving (Eq, Ord)

instance Show Field where
    show X = "X|" :: String
    show O = "O|" :: String
    show B = " |" :: String

symbol :: Player -> Field
symbol P1 = X
symbol P2 = O

type Row   = (Field, Field, Field)
type Board = (Row, Row, Row)

I only want to improve this function: (Because the above code is part of bigger code)

moves :: Player -> Board -> [Board]
moves p ((a,b,c),(d,e,f), (g,h,i)) = (moves' a (((symbol p),b,c), (d,e,f), (g,h,i))) ++
                                     (moves' b ((a,(symbol p),c), (d,e,f), (g,h,i))) ++
                                     (moves' c ((a,b,(symbol p)), (d,e,f), (g,h,i))) ++
                                     (moves' d ((a,b,c), ((symbol p),e,f), (g,h,i))) ++
                                     (moves' e ((a,b,c), (d,(symbol p),f), (g,h,i))) ++
                                     (moves' f ((a,b,c), (d,e,(symbol p)), (g,h,i))) ++
                                     (moves' g ((a,b,c), (d,e,f), ((symbol p),h,i))) ++
                                     (moves' h ((a,b,c), (d,e,f), (g,(symbol p),i))) ++
                                     (moves' i ((a,b,c), (d,e,f), (g,h,(symbol p))))

moves' :: Field -> Board -> [Board]
moves' c m = if c == B then [m] else []

Is there a way to improve my code?

Your function suffer from the retained structure (tuples of tuples). Tuples were not thought to be searched in or to be examined using recursion.

By converting it to a List on the fly, you may work with recursion and all the functions working on Lists (map, fold, etc.).

Here’s an example:

import Data.Maybe (catMaybes)

boardToList :: Board -> [Field]
boardToList ((a,b,c), (d,e,f), (g,h,i)) = [a,b,c,d,e,f,g,h,i]

ListToBoard :: [Field] -> Board
ListToBoard [a,b,c,d,e,f,g,h,i] = ((a,b,c), (d,e,f), (g,h,i))
ListToBoard _ = undefined

moves :: Player -> Board -> [Board]
moves player board = map ListToBoard $ catMaybes [boardSet i | i <- [0..8]]
    where (new, lboard) = (symbol player, boardToList board)
          boardSet i = case splitAt i lboard of
                            (start, B:end) -> Just (start ++ [new] ++ end)
                            _              -> Nothing


Conditionally creating new variables for data manipulation My problem is related to data manipulation: on the basis of the existing variables in a given data set, I would like to create new ones. Bonus points for performance as well as code readability.

Let's consider a reproducible example for the famous "iris" data set. My goal is to create a new variable, which can be described by the following set of conditions:

if species="setosa" or species="virginica" and sepal.length < 5 then
    new variable = "Species1",
if (species="setosa" or species="virginica") and 5<= sepal.length <
10 then new variable = "Species2",
if (species="setosa" or species="virginica") and sepal.length >= 10
then new variable = "Species3",
otherwise, new variable = species (in this case "versicolor").

In order to achieve this, I have written the following conditional instructions:

iris$Speciec2 <- ifelse(iris$Species %in% c("setosa", "virginica"),
                       ifelse(iris$Sepal.Length < 5, "Species1",
                              ifelse(iris$Sepal.Length >= 5 & iris$Sepal.Length < 7,
                                     "Species2", "Species3")), as.character(iris$Species))

and it's really hard to read. I wonder if there are any better solutions.

You can make a couple easy changes to improve readability. First, use with or transform so you don't have to carry a lot of iris$. Second, reorganize your logical tests so each nested ifelse appear in the "if-false" part of the previous ifelse, not in the "if-true" part. Hopefully, this will make sense:

iris$Speciec2 <- with(iris,
  ifelse(!Species %in% c("setosa", "virginica"), as.character(Species),
  ifelse(Sepal.Length < 5,                       "Species1",
  ifelse(Sepal.Length < 7,                       "Species2",
                                                 "Species3")))
)

I think it reads a lot easier this way, where each line contains a test and the output value if the test is passed. If the test fails, we move onto the next line. And at the end, the last line contains the value if all tests have failed. I also like this layout because it looks a lot like how you would nest Perl's ternary operator:

result = condition1 ? value_if_condition1_is_true
       : condition2 ? value_if_condition2_is_true
       : condition3 ? value_if_condition3_is_true
       :              value_if_condition3_is_false

Another solution would be to use the cut or findInterval functions to handle the conditions you have on Sepal.Length. These functions become really handy when you have many cases ("breaks") in mind, although here you only have three. You could do:

new_labels <- cut(iris$Sepal.Length, breaks = c(-Inf, 5, 7, +Inf),
                  labels = paste0("Species", 1:3), right = FALSE)
iris$Speciec2 <- with(iris, ifelse(Species %in% c("setosa", "virginica"),
                                   as.character(new_labels),
                                   as.character(Species)))

Now about performance, since you mentioned it. Know that ifelse and cut are vectorized functions so unless you are handling many millions of rows or doing some high frequency trading, you should be fine :-)

Minor speed improvements might be made by replacing the ifelse with in-place replacements (x[idx] <- value), I let you decide if this is more readable:

iris$Speciec2 <- with(iris, {
  res <- as.character(Species)
  need_change <- Species %in% c("setosa", "virginica")
  res[need_change & Sepal.Length < 5]                     <- "Species1"
  res[need_change & Sepal.Length >= 5 & Sepal.Length < 7] <- "Species2"
  res[need_change & Sepal.Length >= 7]                    <- "Species3"
  res
})

Finally, switching to a data.table (from the R package of the same name) is always recommended when dealing with large size data.frames and when speed is a concern.


Implementing the CutHill-McKee Algorithm I am very much interested in the Reverse Cuthil McKee Algorithm. I have seen Fortran and C or C++ implementations of it, and I decided that it would be a nice exercise to implement it in Python.
I know this algorithm is quite domain specific, but I would still be happy to see what kind of comments I get regarding:

Correctness - I am not sure my only test case works for others, although I did some comparison to the Octave and Matlab version.
Speed - Of course a C version would be faster. However, is there some Python improvements which can be done?
Readability - Is this code clear enough to other peer programmers?

The code:

import numpy as np

def getDegree(Graph):
    """
    find the degree of each node. That is the number
    of neighbours or connections.
    (number of non-zero elements) in each row minus 1.
    Graph is a Cubic Matrix.
    """
    degree = [0]*Graph.shape[0]
    for row in range(Graph.shape[0]):
        degree[row] = len(np.flatnonzero(Graph[row]))-1
    return degree

def getAdjcncy(Mat):
    """
    return the adjacncy matrix for each node
    """
    adj = [0]*Mat.shape[0]
    for i in xrange(Mat.shape[0]):
        q=np.flatnonzero(Mat[i])
        q=list(q)
        q.pop(q.index(i))
        adj[i] = q
    return adj

def RCM_loop(deg,start, adj,pivots,R):
    """
    Reverse Cuthil McKee ordering of an adjacency Matrix
    """
    digar=np.array(deg)
    # use np.where here to get indecies of minimums
    if start not in R:
        R.append(start)
    Q=adj[start]
    for idx, item in enumerate(Q):
        if item not in R:
            R.append(item)
    Q=adj[R[-1]]
    if set(Q).issubset(set(R)) and len(R) < len(deg) :
         p = pivots[0]
         pivots.pop(0)
         return RCM_loop(deg,p,adj,pivots,R)
    elif len(R) < len(deg):
         return RCM_loop(deg,R[-1],adj,pivots,R)
    else:
         R.reverse()
         return R

def test():
    """
    test the RCM loop
    """
    A = np.diag(np.ones(8))
    print A
    nzc=[[4],[2,5,7],[1,4],[6],[0,2],[1,7],[3],[1,5]]

    for i in range(len(nzc)):
        for j in nzc[i]:
            A[i,j]=1
    # define the Result queue
    R = ["C"]*A.shape[0]
    adj = getAdjcncy(A)
    degree = getDegree(A)
    digar=np.array(degree)
    pivots = list(np.where(digar == digar.min())[0])
    inl=[]
    Res = RCM_loop(degree,0, adj,pivots,inl)
    print degree
    print adj
    print "solution:", Res
    print "correct:", [6,3,7,5,1,2,4,0]

if __name__ == '__main__':
    test()

Regarding readability, it is generally a good practice to avoid using unnecessarily shortened forms like adjcncy, as it is only two characters short of adjacency. Also using R for result_queue is discouraged in general (subjective opinion).

If there is no constraint on it being pure python, you may try Cython to get speeds closer to that of C, without moving too far away from the python syntax.


MergeSort implementation with java.util.List passed as argument, not returned I'm working on an implementation of MergeSort which sorts a List given as an argument. As follows

public class MergeSort implements Sort {
    @Override
    public <E extends Comparable<E>> void sort(List<E> list) {
        final int length = list.size();
        List<E> left = null;
        List<E> right = null;
        if (length == 1 || length == 0) {
            return;
        } else {
            left = new ArrayList<>(list.subList(0, length/2));
            right = new ArrayList<>(list.subList(length/2, length));
            sort(left);
            sort(right);
        }
        // can these be improved?
        list.clear();
        list.addAll(merge(right, left));
    }

    private <E extends Comparable<E>> List<E> merge(List<E> right, List<E> left) {
        List<E> newList = new LinkedList<>();
        int rightIndex = 0, leftIndex = 0;
        while (rightIndex < right.size() && leftIndex < left.size()) {
            E rightElement = right.get(rightIndex);
            E leftElement = left.get(leftIndex);
            if (rightElement.compareTo(leftElement) > 0) {
                newList.add(leftElement);
                leftIndex++;
            } else {
                newList.add(rightElement);
                rightIndex++;
            }
        }

        // add remaining
        while (leftIndex < left.size()) {
            newList.add(left.get(leftIndex));
            leftIndex++;
        }
        while (rightIndex < right.size()) {
            newList.add(right.get(rightIndex));
            rightIndex++;
        }
        return newList;
    }
}

I want to do the sorting on the List that's passed as an argument, not return a new List. I feel, however, that calling clear() and addAll() a number of times might slow things down. How can I avoid these calls?

You could do

list.clear();
merge(right, left, list);

with

private static <E extends Comparable<E>> List<E> merge(List<E> source1, List<E> source2, List<E> destination) {
    // ...
    destination.add(element);
    // ...
}

The merge() function should be declared static to indicate that it doesn't use any instance variables.


Quicksort application What do you think about this code?

#include <iostream>
using std::cout;
using std::endl;

#include <vector>
using std::vector;

#include <ctime>
#include <cstdlib>

template < typename T >
vector < T > concatenate( vector < T > _a_, T piv, vector < T > _b_ )
{
  vector < T > v_list;
  v_list.insert( v_list.begin(), _a_.begin(), _a_.end() );
  v_list.push_back( piv );
  v_list.insert( v_list.end(), _b_.begin(), _b_.end() );
  return v_list;
}

template < typename T >
vector< T > quick( vector< T > vec )
{
  vector< T > v_less, v_greater, v_list;
  vector< T > aux1, aux2;
  typename vector < T >::const_iterator iter_Const;
  if( vec.size() <= 1 )
    return vec;
  int piv = *(vec.end() - 1);
  vec.pop_back();
  for( iter_Const = vec.begin(); iter_Const != vec.end(); ++iter_Const )
  {
    if( *iter_Const < piv )
      v_less.push_back( *iter_Const );
    else
      v_greater.push_back( *iter_Const );
  }
  return concatenate( quick( v_less ), piv, quick( v_greater ) );
}

int main() {
  vector < int > lista;
  srand( 1 );
  cout << "VECTOR" << endl;
  for( int i = 0; i < 10000; i++ )
  {
      lista.push_back( rand() % 100 );
      cout << lista[i] << " | ";
  }
  cout << endl;
  cout << "******" << endl;
  cout << "QUICKSORT" << endl;
  lista = quick( lista );
  vector < int > :: const_iterator it;
  for( it = lista.begin(); it != lista.end(); ++it )
    cout << *it << " | ";
  cout << endl;
}

If this is not just a learning exercise, I have to point out that C++ already has a sorting function in its standard library and in real code you should use that. So for the rest of this answer I'm going to assume that this is in fact just a learning exercise.

Now first a note on the algorithm: Usually quicksort is implemented in-place for better performance. Even if you want a sorting function which does not modify its argument, it's quicker to just copy the argument and then run an in-place quicksort on the argument.

Further you're creating a lot more copies of your vectors than you need to, even when using this choice of algorithm: Every time you pass a vector by value, it is copied. Every time you pass a vector, which you do not want to modify, you should rather pass it by const reference, so as not to create an unnecessary copy.

On the same note concatenate also creates an unnecessary copy by returning the result vector by value. This can be avoided by passing an empty vector by non-const reference and filling that instead of using a return value.

Lastly there is a bit of code duplication in your main function: You're printing a vector separated by pipes twice - once during its creation and once after sorting. I would define a helper function print_vector, which prints a vector separated by pipes, and call that twice: once after creating the vector and once after sorting it. This way the first printing will have to happen after the vector creation, not during, but that's okay as doing it during the creation gives very little performance benefit (if any) and having the creation be a separate step from printing the vector improves code cleanliness.


Simplifying a web service method I have the following method in a web service class.  I'm a little unhappy about the big block of new JProperty(...) calls in the for loop.  Is there a way to simplify that?

public string UserCatalog(string numericSessionId, JObject incomingRequestJson)
{
        JObject json = new JObject();
        JObject returningJson = new JObject();
        JArray userCatalogArray = new JArray();

        string deviceId = incomingRequestJson.SelectToken("deviceId", true).ToString();
        string version = getOptionalData(incomingRequestJson, "version", "1.0.0");
        requireMinVersion(incomingRequestJson);

        IEnumerable<Bookcard> catalog = readerBLL.UserCatalog(numericSessionId, deviceId);
        var publicKeyRSA = readerTools.GetPublicKey(HttpUtility.HtmlDecode(numericSessionId), HttpUtility.HtmlDecode(version));

        //Build the userCatalog json array
        foreach (Bookcard card in catalog)
        {
            JObject arrayEntry = new JObject(
                new JProperty("bookThumbnailUrl", card.bookThumbnailUrl),
                new JProperty("bookId", card.bookId),
                new JProperty("bookTitle", card.bookTitle),
                new JProperty("titlePrefix", card.titlePrefix),
                new JProperty("author", card.author),
                new JProperty("annotation", card.annotation),
                new JProperty("publisher", card.publisher),
                new JProperty("numPages", card.numPages),
                new JProperty("returnDate", card.returnDate),
                new JProperty("downloaded", card.downloaded),
                new JProperty("deviceId", card.deviceId),
                new JProperty("currentPageLabel", card.currentPageLabel),
                new JProperty("furthestPageLabel", card.furthestPageLabel),
                new JProperty("currentReadPosition", card.currentReadPosition),
                new JProperty("furthestReadPosition", card.furthestReadPosition),
                new JProperty("lastReadTimestamp", card.lastReadTimestamp),
                new JProperty("bookLength", card.bookLength),
                new JProperty("ttsEnabled", card.ttsEnabled),
                new JProperty("mackinCheckoutID", card.mackinCheckoutID),
                new JProperty("runtime", card.runtime),
                new JProperty("readerType", card.readerType),
                new JProperty("dop", card.dop),
                new JProperty("externalId", card.externalId),
                new JProperty("externalSessionKey", card.externalSessionKey),
                new JProperty("externalCheckoutSessionKey", card.externalBookSessionKey),
                new JProperty("externalCheckoutId", card.externalCheckoutId),
                new JProperty("externalAccountId", card.externalAccountId + findawayBLL.AccountSuffix)
                );
            if (card.printAllowed && !readerTools.isOldVersion(1, 3, 0, version))
            {
                arrayEntry.Add(new JProperty("printAllowed", readerTools.EncryptRSA("print_OK_" + card.bookId, publicKeyRSA)));
            }
            userCatalogArray.Add(arrayEntry);
        }

        returningJson.Add("userCatalog", userCatalogArray);
        return returningJson.ToString();
    }

It is not clear (since you don't show the definition of card) however if all you are doing is creating properties for each public property of card then try this:

JObject arrayEntry = JObject.FromObject(card);

You might need a few more lines to add or remove additional properties based on your business rules. But this would cut down on a lot of the clutter.


C random walk the alphabet on a small grid I'm teaching myself C, with an older edition of K. N. King's C Programming: A Modern Approach. I come from a Python and JavaScript background as far as programming goes, but not a strong foundation in theoretical CS concepts. I'm currently up to Chapter 8 of the book, which introduces one- and multi-dimensional arrays in C. The final exercise of the chapter is to write a program that takes a 10x10 grid and prints the letters of the alphabet in sequence, but picking one of the open adjacent squares to the last-placed character in order to place the next character in sequence. The result resembles a "random walk" through the grid. The program must terminate early if it reaches a point with no open spaces to continue the sequence. My attempt is below. I'm looking for general feedback. I know that there are almost certainly better data structures I could be using, but bear in mind that I haven't even started reading the chapter on functions: so I've only been exposed to elementary types, arrays, some operators, conditional expressions, etc. Bear those constraints in mind if you can.

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

#define N 10
#define TRUE 1
#define FALSE 0
#define START 'A'
#define END 'Z'

typedef int Bool;

int main() {
  char grid[N][N] = {};
  int i = 0;
  int j = 0;
  int next_j = 0;
  int next_i = 0;
  int move, open_spaces;
  Bool placed = FALSE;
  Bool examined_spaces[4] = {};
  // Initialise a random number generator using current time,
  // to ensure it's different each loop
  srand((unsigned) time(NULL));
  // Loop alphabet
  for (char c = START; c <= END; c++) {
    open_spaces = 4;
    memset(examined_spaces, 0, sizeof(Bool)*4); // Empty examined spaces array

    grid[i][j] = c;
    // Determine the next position
    if (c == END) {
      // end of iteration
      placed = TRUE;
    } else {
      // Need to determine the position of the next character
      placed = FALSE;
    }
    // Placed, calculating position of next character
    while ((!placed) && (open_spaces > 0)) {
      next_i = i;
      next_j = j;
      move = rand() % 4;
      if (examined_spaces[move] == TRUE) {
        // Already examined this move
        continue;
      }
      if (examined_spaces[move] == FALSE) {
        // A new move
        open_spaces--;
        examined_spaces[move] = TRUE;
      }
      switch (move) {
        case 0: next_j--; break; // up
        case 1: next_i++; break; // right
        case 2: next_j++; break; // down
        case 3: next_i--; break; // left
      }
      if (next_j + 1 > N || next_j < 0 || next_i + 1 > N || next_i < 0) {
        // Out of bounds
        continue;
      } else if (grid[next_i][next_j] != 0) {
        // Already occupied by a character
        continue;
      } else {
        // Successful placement
        placed = TRUE;
        i = next_i;
        j = next_j;
      }
    }
    if (open_spaces <= 0 && !placed) {
      // No more empty spaces
      break;
    }
  }
  for (i = 0; i < N; i++) {
    for (j = 0; j < N; j++) {
      if (grid[i][j] == 0) {
        printf(". ");
      } else {
        printf("%c ", grid[i][j]);
      }
      if (!((j + 1) % 10)) {
        printf("\n");
      }
    }
  }
  return 0;
}

Only Use Conditions When It Is Necessary
In the for loop that displays the grid the condition

if (!((j + 1) % 10)) {

is unnecessary. Move the new line printf outside the inner loop and it will execute when the inner loop ends.

Algorithm
The algorithm doesn't seem to be using the diagonals which means there are 4 additional spots a character  might go to.

It's not clear that the program works completely when I ran the program it went up to H and then quit so
the grid was mostly ..

Variable and Symbolic Constant Names
There are some pretty good names such as open_spaces, examined_spaces, placed, grid, TRUE, FALSE, START, END and then there are names such as N, i, j, next_j and next_i. In particular i and j seem to indicate
directions so horizontal and vertical might be better names. N might be better called GRID_SIZE.

There also seems to be a missing symbolic constant for 4 (OPEN_SPACES_SIZE).

Functions
Just because you haven't read the chapter on functions doesn't mean you shouldn't use them if you are accustomed to writing code using functions. It definitely would help the code.


Merge hashes that have the same description field, summing the other fields I have an array of hashes of form:

{ description: 'string', b: float, c: float }

I need to get an array of merged hashes so that if hashes have same value at key description sum values of b and c, (of course keeping value of a unaltered), otherwise (if value of description is unique) just add hash to array.

Current solution:

array_of_hashes.inject([]) do |lines, line|
  if lines.none? { |l| l[:description] == line[:description] }
    lines << line
  else
    lines
      .find { |l| l[:description] == line[:description] }
      .merge!(line) { |key, old_v, new_v| key == :description ? old_v : old_v + new_v }
    lines
  end
end

I am 100% sure there's a nicer solution.

Besides the code being a little more complex than it needs to be, I see two issues:

a performance problem: the .find { |l| l[:description] == line[:description] } is called for each element in the array of hashes, leading to an O(n^2) time algorithm (because the array lookup takes linear time)
a more subtle thing: the initial hashes are mutated by your code. When you do old_v + new_v that modifies the very hashes that have been provided as an input to your function/code. This might not be a problem for you, but I think it's good practice to avoid mutability where possible.

To solve these things, let's use a hash of hashes. We'll have a hash that has the :description values as key (thus allowing to look things up in constant time) and a hash containing the values to be cummulated.

Also, to save the verification of whether a certain key already exists or not, I will use the new {|hash, key| block } syntax when creating the outside hash.

Here's the code:

results = Hash.new { |h, k| h[k] = {b: 0, c: 0} }
array_of_hashes.each do |h|
  cummulative_hash = results[h[:description]]
  cummulative_hash[:b] += h[:b]
  cummulative_hash[:c] += h[:c]
end

At this point, results looks like this:

{"string"=>{:b=>3, :c=>6}, "other_string"=>{:b=>1, :c=>2}}

This can be easily converted to your array format, like this:

results.map{|k, v| {description: k}.merge v}

=> [{:description=>"string", :b=>3, :c=>6},
    {:description=>"other_string", :b=>1, :c=>2}]

In the example above I hardcoded the :b and :c keys.
For a more generic treatment of the keys to be cummulated, here's the (slightly adjusted) code:

results = Hash.new { |h, k| h[k] = {} }
array_of_hashes.each do |h|
  cummulative = results[h[:description]]
  h.each do |k, v|
    cummulative[k] = (cummulative[k] || 0) + h[k] if k != :description
  end
end
results.map{|k, v| {description: k}.merge v}


Sum of all possible concatenations of array values Given an array of non-negative integers, we need to find the sum of concatenation of elements in the array.

For example -
given [11, 22] the result should be - 6666
i.e.,
11 + 11 = 1111
11 + 22 = 1122
22 + 11 = 2211
22 + 22 = 2222

Sum of all those numbers would result in 6666

My code is as below -

function concatenationsSum(a) {
  var sum = 0;
  for (var i = 0; i < a.length; i++)
    for (var j = 0; j < a.length; j++)
        sum += Number("" + a[i] + a[j]);
  return sum;
}

Problems like these require brute force since we need to sum each and every combination. This works sure fine for smaller inputs. But if the array is longer, say, 1 ≤ lengthOfArray ≤ 25, how can I handle such large inputs faster? Any suggestions would be appreciated.

I'm always leery of the claim that problems require brute force algorithms. There is, for example, a formula for directly calculating the Nth Fibonacci number.

In the case of this problem, there is a much simpler way to perform the calculation that does not require O(n^2) time. Each element of the array pairs with every other element exactly twice - once as the most significant part, and once as the least significant part. These two parts can be calculated separately in one pass.

The low part is the sum of the elements times the number of elements.

The high part is the sum of (the "offset" for each element times the sum of the elements). The offset is the digit base raised to the number of digits (expressed generally, but this problem always uses base 10). So for a two digit number (11 or 22) the offset is 100.

function concatenationsSum2(a) {
    var lowSum = 0;
    for (var i = 0; i < a.length; i++)
        lowSum += a[i];

    var sum = lowSum * a.length;

    for (var i = 0; i < a.length; i++) {
        var size = a[i].toString().length;
        var offset = iPower(10, size);
        sum = sum + lowSum * offset;
    }

    return sum;
}

function iPower(base, power) {
    var result = 1;
    for (var i = 1; i <= power; i++)
        result *= base;

    return result;
}

And from there, we can simplify even further by combining the two parts.

function concatenationsSum3(a) {
    var lowSum = 0;
    var offsetSum = 0;
    for (var i = 0; i < a.length; i++) {
        lowSum += a[i];

        var size = a[i].toString().length;
        var offset = iPower(10, size);
        offsetSum += offset;
    }

    return lowSum * a.length + lowSum * offsetSum;
}


HackerRank - Array Manipulation This is my implementation for this hacker rank problem. (And a follow-up.)

Problem

  You are given a list(1-indexed) of size n, initialized with zeroes.
  You have to perform m operations on the list and output the maximum of
  final values of all the  elements in the list. For every operation,
  you are given three integers a, b and  and you have to add value k to
  all the elements ranging from index  to (both inclusive).

Example input

5 3
1 2 100
2 5 100
3 4 100

Expected output

200

Explanation

After first update list will be:

100 100 0 0 0

After second update list will be:

100 200 100 100 100

After third update list will be:

100 200 200 200 100

So the required answer will be: 200

First solution

My first attempt I think is quite readable (ignoring the way input and output is handle in hacker rank)

#include <bits/stdc++.h>
#include <algorithm>

using namespace std;

int main() {
    int n, m;
    cin >> n >> m;
    vector<long long> v(n);
    for(int a0 = 0; a0 < m; a0++){
        int start, end, val;
        cin >> start >> end >> val;
        auto it_start = v.begin()+(start-1);
        auto it_end = v.begin()+(end);
        transform(it_start, it_end , it_start, [k](long long &x){return x+=k;});
    }
    cout << *max_element(v.begin(),v.end());
    return 0;
}

Final solution

But this approach, although readable, is too slow for what it is actually being ask, which is the maximum value that it would be achieved. So I wrote this, which passed the tests, but which is more difficult to understand in my opinion.

#include <algorithm>
#include <vector>
#include <iostream>

int main() {
    int n; int m;
    std::cin >> n >> m;
    using val_type = long long;
    std::vector<val_type> v(n);
    while(m--){
        val_type start, end, val;
        std::cin >> start >> end >> val;
        auto it_start = v.begin()+(start-1);
        auto it_end   = v.begin()+ end;
        *it_start     += val;
        *it_end       -= val;
    }
    val_type max{0};
    auto accumulate_max_val = [x=val_type(0),&max](val_type y) mutable{x+=y; if (x>max) max=x;};
    std::for_each(v.begin(),v.end(),accumulate_max_val);
    std::cout << max;
    return 0;
}

What would you do to improve it? Am I using lambdas and the for_each appropriately, or is there a clearer way to express what I want to do?

Kudos for figuring out the correct algorithm.

However you can streamline it by not using a v vector:

You correctly treated an operation a, b, k as a pair of operations: add k from a to the end, and subtract k from b+1 to the end. Now, instead of storing them in v, collect decoupled operations in a vector of their own. Sort it by index. std::partial_sum it, and find the maximum in the resulting array.

This will drive the space complexity down from \$O(n)\$ to \$O(m)\$, and change the time complexity from \$O(n+m)\$ to \$O(m\log m)\$. According to constraints, the time complexity seems to be better. One should also keep in mind that accesses to v could be all over the place with no particular order, and a well crafted sequence of operations may incur too many cache misses. I didn't profile though.
It is possible that spelling the loop out (rather than using for_each and lambda) would improve readability.
The algorithm would fail if k was allowed to be negative. Even it is not the case, it still is a good habit to initialize max and x to v[0], and start the loop at v.begin() + 1.


Draw a spark line with SwiftUI I am using a set of data points (currently randomly generated), and drawing a line graph inside a box:

struct DrawLine: View {
    var body: some View {
        ZStack {
            Rectangle()
                .stroke(lineWidth: 1.0)
                .fill(Color.purple)
                .frame(width: InstrumentPrices.boxWidth, height: InstrumentPrices.boxHeight, alignment: .center)

            SparkLine()

        }
        .frame(width: 0, height: 0).border(Color.black)
    }
}

struct SparkLine: View {
    @State var myPoints = InstrumentPrices.points

    static let gradientStart = Color(red: 239.0 / 255, green: 120.0 / 255, blue: 221.0 / 255)
    static let gradientEnd = Color(red: 239.0 / 255, green: 172.0 / 255, blue: 20.0 / 255)

    var body: some View {
        ZStack {
            Rectangle()
                .fill(Color.white)
                .onTapGesture {
                    // print ("Tap!")
                    InstrumentPrices.resetPoints()
                    self.myPoints = InstrumentPrices.points
                }

            GeometryReader { geometry in
                Path { path in
                    path.move(to: InstrumentPrices.points[0])

                    self.myPoints.forEach {
                        path.addLine(
                            to: CGPoint(x: $0.x, y: $0.y)
                        )
                    }
                }
                .stroke(lineWidth: 2)
                .fill (LinearGradient (
                    gradient: .init(colors: .init([Self.gradientStart, Self.gradientEnd])),
                    startPoint: .init(x: 0, y: 0),
                    endPoint: .init(x: 1.0, y: 0)
                ))
            }
        }
    }
}

SwiftUI is new enough to me. I'm always looking for better code conventions. Any comments are welcome!

The role of your ZStack and border rectangle could be replaced with a .background or .overlay modifier on the Sparkline itself - would allow more flexibility in presentation at the point-of-usage.


Migrate data between tables efficiently I wrote a very simple quick and easy script to migrate a bunch of data to another table in my database.

Can I accomplish this task without using so many foreach loops?
I realize I'm imputting similar data in each of my loops and it just seems redundant and wrong.

I thought of using array_merge on talkers, pros & anons and I thought that   because each of the second loops use the same field, I would write code to retrieve getYI and then write code to determine if it belonged to a talker, anon, or pro.
However I wasn't sure if either of those solutions would make my script any more efficient/better.

Certainly any insight would be appreciated. Thanks in advance!

And if it helps I'm using Symfony 2.3.

public function migratingDataScript()
{
    $em = $this->getDoctrine()->getManager();

    $talkers = $em->getRepository('AppBundle:Talker')->findAll();
    $anons = $em->getRepository('AppBundle:Anon')->findAll();
    $pros = $em->getRepository('AppBundle:Pro')->findAll();

    foreach ($talkers as $talker) {
        echo "Talker: ".$talker->getId()."
";
        foreach ($talker->getYI() as $year => $status) {
            $rounds = $em->getRepository('AppBundle:Round')->findByYear($year);
            foreach ($rounds as $round) {
                $newTalkerInvitation = new Invitation($round);
                $newTalkerInvitation->setStatus($status);
                $newTalkerInvitation->setTalker($talker);
                // $newTalkerInvitation->setNumAt(); No Data For Talker
                // $newTalkerInvitation->setStart(); No Data For Talker
                $em->persist($newTalkerInvitation);
            }
        }
    }//Ends Talker Migration

    foreach ($anons as $anon) {
        echo "Anon: ".$anon->getId()."
";
        foreach ($anon->getYI() as $year => $status) {
            $rounds = $em->getRepository('AppBundle:Round')->findByYear($year);
            foreach ($rounds as $round) {
                $newAnonInvitation = new Invitation($round);
                $newAnonInvitation->setStatus($status);
                $newAnonInvitation->setAnon($anon);
                // $newAnonInvitation->setNumAt(); No Data For Anon
                // $newAnonInvitation->setStart(); No Data For Anon
                $em->persist($newAnonInvitation);
            }
        }
    }//Ends Anon Migration

    foreach ($pros as $pro) {
        echo "Pro: ".$pro->getId()."
";
        foreach ($pro->getYI() as $year => $status) {
            $rounds = $em->getRepository('AppBundle:Round')->findByYear($year);
            foreach ($rounds as $round) {
                $newProInvitation = new Invitation($round);
                $newProInvitation->setStatus($status);
                $newProInvitation->setPro($pro);
                if(is_null($pro->getNumOfAttempts())){
                    $newProInvitation->setNumAt('0');
                }else {
                    $newProInvitation->setNumAt($pro->getNumOfAttempts());
                }
                // $newProInvitation->setStart(); No Data For Pro
                $em->persist($newProInvitation);
            }
        }
    }//Ends Pro Migration

    $em->flush();
    echo "Completed.";
    die;
}

Can I accomplish this task without using so many foreach loops?

Depends.

If you want to stick with database-agnostic abstraction such as Doctrine, there is no way to avoid loops.

But if you want an efficient solution, you can write a database-specific query to copy all records in a single query. For example, for mysql it would be something like

INSERT INTO invitation (field1, field2, status, talker)
  SELECT field1, field2, ?, ? FROM talker

and then bind $status and $talker to this query.


A simple lotto game in Python I have a simple task of creating a simple lotto game to practice python. I had some troubles with return of functions so i dropped few because of not understanding the scopes maybe.

In my next version i want to try to make it fully in functions, and in the next version i want to do it in OOP principles.

By posting this code I would like to hear maybe some suggestions where my logic was wrong and some PEP comments.

Game rules

Each player is given 15 numbers, in increasing order. This is why i use sorted() on the lists of player and computer. Also there is a common list with numbers in it from 1 to 90.

A random number is picked and removed from this list and printed to players. The player who has this number in his card crosses it out or skips its turn.  This continues until either player or computer crosses out all his numbers, or for some strange reason they are out of numbers :) I think in other parts of the world it is also called BINGO but I might be wrong.

import random

number_pool = list(range(1, 91))
computer_card = random.sample(number_pool, 15)
computer_card_sorted = sorted(computer_card)
player_card = random.sample(number_pool, 15)
player_card_sorted = sorted(player_card)

def display_cards():
    print("Computer card:\n")
    print(computer_card_sorted)
    print("====================================")
    print("Player card:\n")
    print(player_card_sorted)

def lotto_choice():
    choice = random.choice(number_pool)
    number_pool.remove(choice)
    return choice

def the_game():
    while number_pool:
        choice = lotto_choice()
        print("The random lotto is: " + str(choice))
        display_cards()
        cross_number = input("Do you want to cross out a number")
        cross_number.lower()
        if cross_number == "y":
            if choice in player_card_sorted:
                player_card_sorted.remove(choice)
            elif choice in computer_card_sorted:
                computer_card_sorted.remove(choice)
        if cross_number == "n":
            if choice in computer_card_sorted:
                computer_card_sorted.remove(choice)
            else:
                continue
    else:
        if len(player_card_sorted) == 0:
            print("Congratulations Player ! You won")
        elif len(computer_card_sorted) == 0:
            print("The computer have won, too bad !")
        else:
            print("It is a tie you both ran out of numbers, very straange !")

the_game()

For your current purposes, the biggest problem is that you are relying on global variables exclusively.  This makes it a lot tougher to understand, maintain, and debug your code.  This question actually has some great answers that discuss why this can cause real problems in code:

https://softwareengineering.stackexchange.com/questions/148108/why-is-global-state-so-evil

As a specific example, this bit of code had me confused for a few minutes:

while number_pool:

Because number_pool wasn't used anywhere in your while loop, and so I had no idea why this wasn't just an infinite loop.  I eventually realized that it is getting modified inside the lotto_choice method which is called within the while loop.  This is pretty much exactly why you don't want to use global state: if any bit of code can modify any variable anywhere, you can quickly lose track of who modifies what and when it happens, and bugs can creep in very quickly.  You also will have your own trouble if you come back to the code after a while and find that you no longer remember exactly what gets modified by what functions.

So, always make sure and keep input into functions and output out of functions clear.  In the case of lotto_choice you could easily do something like this:

def lotto_choice( number_pool ):
    choice = random.choice(number_pool)
    number_pool.remove(choice)
    return ( choice, number_pool )

Then you would call it like this inside your while loop:

( choice, number_pool ) = lotto_choice( number_pool )

Now you can easily see where number_pool gets modified.  Since you are just now getting started, now is the time to establish good habits.  So take the time and understand how variable scope works and (especially in a learning exercise like this) make sure not to use any global state.  My biggest suggestion is to fix that and then post a new code review question with your updated code.

Also, small change. You want to only execute your game if called in the 'main' context, like this:

if __name__ == '__main__':
    the_game()


Book constructor pattern I am learning how to do object-orientated programming in JS. In particular, I am learning about the constructor pattern.

In an attempt to exercise what I have been learning, I made this:

var assert = require("assert");

var Book = (function() {
  // private static variable (why this exhibits static behaviour, I have no clue)
  var books = [];

  var constructor = function(doc) {
    // guard clauses
    assert.ok(doc.title, "title cannot be undefined");
    assert.ok(doc.author, "author cannot be undefined");
    assert.ok(doc.genre, "genre cannot be undefined");

    // public properties
    this.title = doc.title;
    this.author = doc.author;
    this.genre = doc.genre;
    this.read = doc.read || false;
    this.id = books.length;
    this.description = this.title + " is a " + this.genre + " by " + this.author;

    // public function
    this.save = function() {
      books.push(this);
    };
  };

  // public static functions
  constructor.findById = function(id, callback) {
    function cloneBook(book) {
      return {
        id: book.id,
        title: book.title,
        author: book.author,
        genre: book.genre,
        read: book.read
      };
    }
    var book = books.filter(function(book) {
      return book.id === id;
    })[0];
    var clone = cloneBook(book);
    clone.save = function() {
      book.title = this.title;
      book.author = this.author;
      book.genre = this.genre;
      book.read = this.read;
    };
    callback(clone);
  };

  constructor.find = function(conditions, callback) {
    function areNoConditions(obj) {
      return Object.keys(obj).length === 0;
    }
    if (areNoConditions(conditions)) {
      callback(books);
      return;
    }
    var foundBooks = [];
    for (var prop in conditions) {
      books.forEach(function(book) {
        if (book[prop] === conditions[prop]) {
          foundBooks.push(book);
        }
      });
      callback(foundBooks);
    }
  };

  // return the function (the constructor)
  return constructor;
}());

...

var book = new Book("The Great Gatsby", "F. Scott Fitzgerald", "Novel");
book.save();
var books = Book.find();
book.read = false;
book.save();
console.log(books);

My aim was to emulate the Mongoose Model API.

My particular concerns are:

Is the constructor pattern the most applicable pattern here?
Is my code idiomatic?

I have decorated my code with comments for the purposes of this answer so you can know what I am thinking. I come from a C# background.

It looks ok at first glance (proper use of camelCase and PascalCase, proper indentation, etc.), but there are some issues when you dig a little deeper.

You have a bug: Because you define a book's ID as simply books.length, and because you set the ID in the constructor, you're assuming/relying on every book instance to either be added ("saved") immediately after instantiation or discarded forever.

If I were to do this:

book1 = new Book({ ... });
book2 = new Book({ ... });

I get two books with the same ID. Of course, id is just another property, but in this case, I think it's safe to assume that it's supposed to be a unique ID.

For now, you could simply move the assignment of a book's ID to the save method, so it only gets assigned there.

However, if you ever add the ability to delete books, you'll run into trouble again, since removing something from the array would change its length, and you'd end up with multiple books sharing the same ID.

Perhaps the simplest thing would be to declare yet another closure variable (aka a private, static variable) simply called nextId or something. Then, whenever a book is saved, give it that value as its ID, and increment nextId. Same basic principle as auto-incrementing IDs in a database.

Of course there's still the caveat that anyone can just change a book's id property to "monkey" after the fact, but one step at a time.
How can anyone change the ID after the fact even with the extra precautions I mentioned? Well, if I call Book.find with no conditions, I get a reference to an otherwise private variable: books. Then I can do whatever I want with its contents. If I call it with some conditions, I don't get the books array itself, which is good, but I still get some of the same book objects that books holds. So again, I'm free to do what I want.

More simply, though, I can just alter the instance I created after it was saved since save just does books.push(this). So I can do this:

var book = new Book({title: "War and Peace", author: "Leo Tolstoy", genre: "novel"});
book.save();
book.id = "monkey"; // cue mischievous snickering

The book instance I changed after saving is the same instance the books array holds a reference to, so the damage is done.

Of course, right now, I could also just set book.id before saving, and achieve the same, since save doesn't care what it's stuffing in the array.

Also, if I do this:

var book = new Book({title: "War and Peace", author: "Leo Tolstoy", genre: "novel"});
book.id = "monkey";
book.save();
book.save();
book.save();

I end up with three identical books in the books array, all of which have the ID "monkey". It's not even that it's three copies; it's the same exact object appearing three times. Hmm....
You have another bug: Book.findById assumes it always finds something. When it doesn't, the script breaks because it calls cloneBook(undefined).
The description is fixed once defined, so changing, say, the title doesn't update the description. Description would be better off as a method.
The assert module isn't really necessary. I'd rather see the constructor throw a more specific exception of some kind (you can throw anything in JavaScript, so the "exception" can just be a string to start with). Also, your message says "cannot be undefined", but assert.ok just checks for general thruthiness, not undefined specifically. So it'll balk at false, 0, NaN, "", and null as well as undefined. Most of those are fine to complain about in this case, except maybe the empty string (maybe you don't want to put a genre in there, but still want a book instance). Besides, checking for thruthiness still doesn't check for type. So the title could be an array, the genre could be a Date object, author could be Number.NEGATIVE_INFINITY etc... However, that's simply the nature of dynamic typing, so it's up to you how far you want to go.

Still, it might be more beneficial to move the validity checks to the save method. Right now, you can't make a "blank" book by saying new Book; you have to know most of its details ahead of time.

... which begs the question: If you have all the details in an object already, do you need to make a Book out of em? Eh, this is an exercise, so I'll just go with it.
Why the callbacks? None of what you're doing is asynchronous, and JavaScript is single-threaded. So you can just return. It'd make the API a lot simpler. I suppose it's because you're mimicking Mongoose and its functions are asynchronous, but still. It's a little too cargo cult'y. (Note that using callbacks allows for really neat stuff, and learning to work with asynchronous JavaScript is a worthy goal; in this case, however, it complicates matters.)
The idea of returning a clone from findById to keep some properties "safe" is fine in and of itself. However, because you've hardcoded which properties are editable, it's not great for maintainability.
Why bother with the areNoConditions function? For one, it's a really odd name, but it also just wraps a single condition you might as well do inline.
Why not use filter in find?

Long story short, there are some issues here, but for a learning exercise it's not too shabby! You just have to decide how far you want to go with it, because a large portion of this seems to be about controlling access to properties. But that's kinda hard to do really well in JavaScript. Mongoose is forced to do all those things, but it's not the "path of least resistance" in JavaScript. Hence, it may not be the best thing to mimic.

You can "sort of" do strictly typed, access-controlled stuff in JavaScript. But unless you really need to, why bother? It's going against the grain, and often just encourages headaches.

By the way:

  // private static variable (why this exhibits static behaviour, I have no clue)

Because it's a closure. All the functions defined while it's in-scope will retain a reference to it. So while you return the constructor function, that function still has access to books.


Column selection and URL modification I have an HTML table representing a database table and a sidebar which holds different anchors ready to perform CRUD operations on click.

Because the hrefs in the anchors aren't in general valid for all columns I have written a script which automatically inserts the id of a selected column into the hrefs:

/localhost/image/var/edit => /localhost/image/4/edit

The code I have written works but it isn't very clean and smart :( Could somebody take a look and say where I have space of improvement?

How could I improve the td:nth-child(2) selector and the long line modifying the link?

EDIT:

    {% if image is defined %}

Bilder verwalten

Bild betrachten

Bild hinzufügen

Bild bearbeiten

Bild löschen

    {% else %}

Bilder verwalten

Bild betrachten

Bild hinzufügen

Bild bearbeiten

Bild löschen

    {% endif %}

As you see when having an available singe object (while editing, showing, deleting) the server side renders the right id/name into the href. Under circumstances when I only have a collection of objects or none object available, jquery has to do the rendering of the right urls. Also some URLs don't require an id (new, index)

<script type="text/javascript">
$(function () {
    var $lastSelected = $([]);
    $('tr').click(function () { //tr tag is always inside a table
        $lastSelected.removeClass('selected'); //by caching this variable in the closure we don't need to traverse for it again
        $lastSelected = $(this).addClass('selected');
    });

    $('.image_bar a').click(function (e) {
        var url = $(this).attr('href').split('var'), //both checks if var is there and prepares for the replacement
            name;
        if ($lastSelected.length === 0) {
            return url.length==1; //any urls that do not have var in them will still work
        }

        name = $lastSelected.children().eq(2).html();
        window.location = url.join(name);
    });
});
</script>

Here I am assuming your toolbar is outside of your table.
About the changes:

$(function () { lets you move this script tag elsewhere in your document (perhaps to an external script)
By caching $lastSelected, fewer DOM traversals need to happen.
Knowing $lastSelected enables moving the $.fn.each call out of the tr click event by setting name inside of it.
Since the .each call is no longer in the click event, it can be a click event of its own (no more need to iterate over it).
Instead of modifying the a tags we can just set window.location since we are now in the event where the user has clicked.
Finally, if a row is not selected up front, the $lastSelected variable will not provide a valid name. Thus we shouldn't allow the link to be clicked if it contains var (done by the return statement).


Logging util with static lambda function So again, I am a bit confused about some notes that i'am getting from my reviewer. I have some kind of utils, to print/prepare log messages. Yea, maybe I am a bit fanatic java 8 lambda guy but I think it is still under control. :)

So its looks like that [I hide 'sensitive' data]

public class ExceptionUtils {

  private static final BiFunction<String, String, String> CUSTOM_EXCEPTION_MESSAGE_FORMAT =
    (typeOfError, typeOfEndpoint) -> "[XXX] " + typeOfError + " Error during process bulk " + typeOfEndpoint + " events. "
        + "Event "
        + "occurrences time: %d "
        + "event retry count: %d, "
        + "session: %s";

  public static String validationMessageException(String endpoint, long timestamp, int retryCount, String sessionToken) {
    return String.format(CUSTOM_EXCEPTION_MESSAGE_FORMAT.apply("Validation", endpoint), timestamp, retryCount, sessionToken);
  }

  public static String unknownMessageException(String endpoint, long timestamp, int retryCount, String sessionToken) {
    return String.format(CUSTOM_EXCEPTION_MESSAGE_FORMAT.apply("Unknown", endpoint), timestamp, retryCount, sessionToken);
  }
}

What is the point of adding some parameters as part of format string when others are simply passed to String.format? You might as well just do the latter:

public class ExceptionUtils {

    private static final String CUSTOM_EXCEPTION_MESSAGE_FORMAT
            = "[XXX] %s Error during process bulk %s events. Event occurrences time: %d event retry count: %d, session: %s";

    private static String formatMessage(String type, String endpoint, long timestamp, int retryCount, String sessionToken) {
        return String.format(CUSTOM_EXCEPTION_MESSAGE_FORMAT,
                type, endpoint, timestamp, retryCount, sessionToken);
    }

    public static String validationMessageException(String endpoint, long timestamp, int retryCount, String sessionToken) {
        return formatMessage("Validation", endpoint, timestamp, retryCount, sessionToken);
    }

    public static String unknownMessageException(String endpoint, long timestamp, int retryCount, String sessionToken) {
        return formatMessage("Unknown", endpoint, timestamp, retryCount, sessionToken);
    }
}

or even just:

public class ExceptionUtils {

    private static final String CUSTOM_EXCEPTION_MESSAGE_FORMAT
            = "[XXX] %s Error during process bulk %s events. Event occurrences time: %d event retry count: %d, session: %s";

    public static String validationMessageException(String endpoint, long timestamp, int retryCount, String sessionToken) {
        return String.format(CUSTOM_EXCEPTION_MESSAGE_FORMAT,
                "Validation", endpoint, timestamp, retryCount, sessionToken);
    }

    public static String unknownMessageException(String endpoint, long timestamp, int retryCount, String sessionToken) {
        return String.format(CUSTOM_EXCEPTION_MESSAGE_FORMAT,
                "Unknown", endpoint, timestamp, retryCount, sessionToken);
    }
}


Tuple/Lookup conundrum I have a list of plans and a list of contacts, and I have to check whether contact.ProviderId matches plan.ProviderId. If they match I need to store in a contactUI the plan.Name.

Provider to plan is a 0..1-to-many relationship, and that's why I couldn't use the Dictionary I tried at first instance.

For retrieving the list of object I need to call the DB. So I want to avoid calling it more than needed.

I came up with this

var offeredPlans = new List<Tuple<int, string>>();
foreach (var plan in plans)
{
    // ....Some code...
    offeredPlans .Add(new Tuple<int, string>(providerId, plan.Name));
}

var compareTo = offeringPlans.ToLookup(pair => pair.Item1, pair => pair.Item2);

foreach(var contact in contacts)
{
    var plansAttachedTo = Check(providerId.Value, compareTo);

    foreach (var plan in plansAttachedTo)
    {
        // New contactUI with plan.Name as one of its properties
    }
}

Being

private static IEnumerable<string> Check(int providerId, ILookup<int, string> plans)
{
    return offeringPlans.Where(p => p.Key == providerId).SelectMany(p => p);
}

Is this terrible or does it make sense?

I've never used before these classes (Tuple, Lookup...)
I would like my code to be the clearest I can even it it's not the cleverest of the solution I can think of which is not this case for sure, actually I realize now I could have done some kind of SQL joins, right? (but I would like to know if someone can point any obvious error in the current code).

Edit

The code inside the loop is this one (in this case I don't even omit code as I'm not sure if it's possible what you suggest p.s.w.g). I am using reflection as only some of the classes that inherit from plan have the ProviderId property.

foreach (var plan in plans)
{
    var offeringDetail = new OfferingDetail();

    if (plan.GetType() == typeof (OwnedProductSummary))
    {
        var productSummary = plan as OwnedProductSummary;

        offeringDetail = _offeringBLL.GetById(productSummary.OfferingID);
    }

    if (plan.GetType() == typeof (OwnedServiceSummary))
    {
        var serviceSummary = plan as OwnedServiceSummary;

        offeringDetail = _offeringBLL.GetById(serviceSummary.OfferingID);
    }

    // For the other types of summary will be 0
    var providerId = offeringDetail.ProviderID;

    offeredPlans .Add(new Tuple<int, string>(providerId, plan.Name));
}

Once you've gotten your code into a ILookup you can just call Item property (which in C# is called with [...]) to get all values with a given key. So the Check can be entirely replaced by using the ILookup like this:

ILookup<int, string> plansLookup = ...
IEnumerable<string> plansForProvider = plansLookup[providerId]; // Finds all plans for this provider

However, it's not clear that you need to be creating the List<Tuple<int, string>> in the first place. You can just use Linq to generate your ILookup from scratch:

var plansLookup =
    (from plan in plans
     let productSummary = plan as OwnedProductSummary
     let serviceSummary = plan as OwnedServiceSummary
     let offeringDetail =
         (productSummary != null) ? _offeringBLL.GetById(productSummary.OfferingID) :
         (serviceSummary != null) ? _offeringBLL.GetById(serviceSummary.OfferingID) :
         new OfferingDetail()
     select new
     {
         offeringDetail.ProviderID,
         plan.Name
     })
    .ToLookup(x => x.ProviderId, x => x.Name);


Iterative Radix-2 FFT in C I have a small and weak microcontroller. I also don't have access to the complex library. I wrote this iterative version of the FFT to hopefully get better performance than the recursive version, and also just to learn how the FFT works, and brush up on C.
I'd like to know if the code itself is understandable, and if there are some performance improvements I can make.
#include <stdio.h>
#include <math.h>

#define FALSE 0
#define TRUE 1
#if !defined(M_PI)
#   define M_PI 3.14159265358979323846
#endif

struct complex {
    double real;
    double imaj;
};
typedef struct complex complex_t;

complex_t complex_init(const double real, const double imaj) {
    complex_t temp;
    temp.real = real;
    temp.imaj = imaj;
    return temp;
}

complex_t complex_add(const complex_t a, const complex_t b)
{
    return complex_init(a.real + b.real, a.imaj + b.imaj);
}

complex_t complex_subtract(const complex_t a, const complex_t b) {
    return complex_init(a.real - b.real, a.imaj - b.imaj);
}

complex_t complex_multiply(const complex_t a, const complex_t b) {
    return complex_init(a.real * b.real - a.imaj * b.imaj, a.real * b.imaj + a.imaj * b.real);
}

_Bool is_power_of_2(const unsigned int x) {
    return x != 0 && (x & (x - 1)) == 0;
}

_Bool fft(complex_t* input, complex_t* output, const unsigned int size) {
    if (!is_power_of_2(size))
        return FALSE;

    if (size == 1) {
        output[0] = input[0];
        return TRUE;
    }

    const unsigned int half_size = size / 2;

    // Initial loop. Do the input shuffle and first butterfly at the same time.
    // shuffle is the bit reversed representation of i. If i is 11000, then shuffle is 00011.
    for (unsigned int skip = size, i = 0, shuffle = 0; i < half_size; ++i) {
        const complex_t even = input[shuffle];
        const complex_t odd = input[shuffle + half_size];
        output[i * 2] = complex_add(even, odd);
        output[i * 2 + 1] = complex_subtract(even, odd);

        if (i == 0 || is_power_of_2(i + 1)) {
            skip /= 2;
            shuffle = skip / 2;
        } else
            shuffle += skip;
    }

    // Do the rest of the butterfly operations
    for (unsigned int even_to_odd = 2; even_to_odd < size; even_to_odd *= 2) {
        const double angle = -M_PI / even_to_odd;
        const complex_t partial_rotation = complex_init(cos(angle), sin(angle));
        complex_t current_rotation = complex_init(1, 0);
        for (unsigned int i = 0, to_even = 0; i < half_size; ++i, ++ to_even) {
            if (i % even_to_odd == 0) {
                to_even = 2 * i;
                current_rotation = complex_init(1, 0);
            }
            complex_t* even = output + to_even;
            complex_t* odd = even + even_to_odd;
            const complex_t odd_rotated = complex_multiply(*odd, current_rotation);
            *odd = complex_subtract(*even, odd_rotated);
            *even = complex_add(*even, odd_rotated);
            current_rotation = complex_multiply(current_rotation, partial_rotation);
        }
    }
    return TRUE;
}

Bit-reversal permutation bug
Keeping both a normal counter and the "reversed counter" is the right idea, but this implementation isn't quite right. For example, it might result in a sequence such as 0, 4, 2, 6, 1, 3, 5, 7 while the correct one is 0, 4, 2, 6, 1, 5, 3, 7.
A   B
000 000
100 100
010 010
110 110
001 001
011 101 <<<
101 011 <<<
111 111

There are various ways to do this without that bug, but some of them rely on instructions that are unlikely to be efficient (or even exist) on microcontrollers, such as leading zero count and shift-by-variable. I don't know what you can make work, and make efficient, since I don't know your microcontroller. Perhaps a recursive implementation of the bit-reversal permutation is better, but maybe not, given the often high cost of recursion on microcontrollers. I can only recommend trying some options, I don't know how they will work out in your specific case.
Also, see at the bottom for algorithmic changes.
Periodic test in the inner loop
By that I mean the condition of this if statement:

if (i % even_to_odd == 0) {
    to_even = 2 * i;
    current_rotation = complex_init(1, 0);
}

While it is the case that a remainder operation with a power of two on the right hand side can be efficient, that only applies when the compiler knows it's dealing with a power of two. Hypothetically, a compiler could detect this case, but there is not much hope for that happening. GCC 10 compiling for x64 (I realize you're not targeting that, but that's not really the point, it's about the compilers ability to reason above the set of values even_to_odd could have and how to use that information) makes this for example:
    mov     eax, ecx
    xor     edx, edx
    div     ebx
    test    edx, edx
    jne     .L41

That's not good, and it would be worse on a microcontroller. Since you know that even_to_odd is a power of two, you could write the condition like this: (i & (even_to_odd - 1)) == 0. If GCC 10 cannot do this automatically, it is not likely that the various vendor-specific compilers for some microcontrollers can do it either, as they are usually weaker in that regard.
Benchmarking note

Seems to be 20-40% faster on my desktop than the recursive version I wrote

By all means test it on your desktop, but when option B is faster than option A on a desktop, that is no guarantee at all that that will still be true on the microcontroller. Even between different desktops there is no such guarantee, if they have processors with a different micro-architecture, for example AMD Zen vs Intel Skylake. Benchmarking the code on your desktop can easily trick you into making decisions that are bad for the microcontroller.
Algorithmic changes
The Stockham FFT avoids an explicit bit-reversal permutation, removing the need to implement it efficiently. On the other hand, it accesses memory in a different way and needs temporary work-space. Maybe it's good for your application, or maybe not, I'm not sure.


jQuery form.find radio class In my application we have four inputs radio controls and on the load the value is checked to true. When the user select another radio and then save the form I want to get the new value. I am using the find class but then I need to use checked[0]. Is there a better way to do this?
JS
$("#form").submit(function (event) {
   event.preventDefault();
     console.log('free-text-form');
            var sortOrder = $(this).data('sortorder');
            console.log(sortOrder);
            var form = $(this);
            console.log('Processing Current Check');
            var checked = form.find(".radio-Class:checked");
            var currentChecked = checked[0];
            console.log(currentChecked);
            console.log(currentChecked.value);
            if (checkedRegRef.length == 1) {
                var NewId = currentChecked.value;
                 console.log(NewId);
            }
});

Working Example
https://jsfiddle.net/tjmcdevitt/vh9n5srL/87/

With checked[0] you are getting a reference to the actual DOM element (instead of the jQuery object) but that is unnecessary in your case since jQuery provides a val() method, which returns the current value of the first element in the set of matched elements. So in your case, you could simply do:

var checkedVal = form.find(".radio-Class:checked").val();

A better way to get the selected value as pointed out here by @Peter J is to use the input[name=radioName]:checked selector. Selecting through name attributes ensures that you select the desired radio group since these are meant to be unique. For better performance, you can pass in the form id as the second argument inside the selector method, which is used as a context here (this is same as if you would use $("#form").find("input[name=radioName]:checked")), here is the refactored code:

$("#form").submit(function(event) {
  var $formEl = $(this);
  var $labelEl = $formEl.find('#label1');
  var radioVal = $('input[name=RegimenReferences]:checked', $formEl).val();

  event.preventDefault();
  $labelEl.text(radioVal);
});
body {
  background: #20262E;
  padding: 20px;
  font-family: Helvetica;
}

#form {
  background: #fff;
  border-radius: 4px;
  padding: 20px;
  font-size: 25px;
  text-align: center;
  transition: all 0.2s;
  margin: 0 auto;
  width: 300px;
}

button {
  background: #0084ff;
  border: none;
  border-radius: 5px;
  padding: 8px 14px;
  font-size: 15px;
  color: #fff;
}

#banner-message.alt {
  background: #0084ff;
  color: #fff;
  margin-top: 40px;
  width: 200px;
}

#banner-message.alt button {
  background: #fff;
  color: #000;
}

<form id="form" class="form-horizontal" data-sortorder="1">

Update Values

  1

      <input type="radio" id="radio1" name="RegimenReferences" value="1" class="radio-Class" checked='true'>
      (a) aaaaaa

      <input type="radio" id="radio2" name="RegimenReferences" value="2" class="radio-Class">
      (b) bbbbbb

      <input type="radio" id="radio3" name="RegimenReferences" value="3" class="radio-Class">
      (c) cccccc

      <input type="radio" id="radio4" name="RegimenReferences" value="4" class="radio-Class">
      (c) cccccc

    <button type="submit" class="btn btn-primary modal-submit-btn">Save</button>
    <button type="button" class="btn btn-default modal-close-btn" data-dismiss="modal">Close</button>

</form>

Edit 1:
Since you already got the form selected you can pass that as the context therefore you don't need the form id.


Mandatory dependency in a class I have a class with some mandatory properties which I set via constructor, like this (properties omitted for sake of simplicity):

    public SqlDependencyEx(
        string connectionString,
        string databaseName,
        string tableName,
        string schemaName = "dbo",
        NotificationTypes listenerType = NotificationTypes.Insert | NotificationTypes.Update | NotificationTypes.Delete,
        bool receiveDetails = true,
        int identity = 1)
    {
        this.ConnectionString = connectionString;
        this.DatabaseName = databaseName;
        this.TableName = tableName;
        this.SchemaName = schemaName;
        this.NotificaionTypes = listenerType;
        this.DetailsIncluded = receiveDetails;
        this.Identity = identity;
    }

I am trying to use SqlDependencyEx class as a dependency in one of the service like this:

    private readonly IMemoryCache _memoryCache;
    private readonly IAlbumRepo _albumRepo;
    private readonly SqlDependencyEx _listener;
    private readonly string _masterTable = "Armada_Album";
    public AlbumService(IMemoryCache memoryCache, IAlbumRepo albumRepo)
    {
        _memoryCache = memoryCache;
        _albumRepo = albumRepo;
        _listener = new SqlDependencyEx(connectionString: "SERVER=localhost;DATABASE=dbsample;User Id=usr;pwd=cE$6adadZ'[:3;", databaseName: "KEMITEKS", tableName: _masterTable);
    }

The problem is, I don't think it is good to creating a new instance of SqlDependencyEx class in AlbumService class, but at the same time, I want to force consumer of the SqlDependencyEx class to use those properties, since they are all mandatory in order to SqlDependencyEx class to work. So, I don't want to remove those properties and instead of using them in a constructor of SqlDependencyEx class, setting them in separate method etc. I am also using a Ninject as a DI. Is there a better code design to solve this problem, or is it good to leave it as it is?

If you are using a dependency injection framework then you are not using it correctly. AlbumService's constructor should have in this case another parameter that is the SqlDependencyEx. You should not be creating it by yourself. You are already injecting the other two services. Why not this one too?

And I'm wondering where you have the _masterTable parameter from? I don't see it to be passed to the AlbumService. It has only two arguments.

_listener = new SqlDependencyEx(
    connectionString: "SERVER=localhost;DATABASE=dbsample;User Id=usr;pwd=cE$6adadZ'[:3;",
    databaseName: "KEMITEKS",
    tableName: _masterTable);

Another thing is that the name of the SqlDependencyEx does not match the field. Why do you call it a _listener? It's very confusing.


OutputStream class for use with subprocess.Popen I've recently been writing an application which does a lot of work with the output of commands.  To call the commands I've been using the subprocess module, and originally, was using subprocess.PIPEs for the stdout and stderr, and then using the communicate method once the process was complete to retrieve the output to work with it, log it, etc.  Unfortunately, I had a problem with deadlocks when a particular command generated a long output stream.

For this reason, I've written an OutputStream class, an instance of which can be provided as the stdout and stderr params of subprocess.Popen.  In the background, it writes to a cStringIO buffer which I can retrieve the value from later, and it seems to work well.  However, this is the first time I've worked with anything like this and I'm concerned that I might be doing something potentially unsafe.  I'd appreciate any feedback anybody has.

from cStringIO import StringIO
import os
import threading

class OutputStream(threading.Thread):
    def __init__(self):
        super(OutputStream, self).__init__()
        self.done = False
        self.buffer = StringIO()
        self.read, self.write = os.pipe()
        self.reader = os.fdopen(self.read)
        self.start()

    def fileno(self):
        return self.write

    def run(self):
        while not self.done:
            self.buffer.write(self.reader.readline())

        self.reader.close()

    def close(self):
        self.done = True
        os.close(self.write)

I would then use it something like this:

import subprocess
import time

stream = OutputStream()
proc = subprocess.Popen(['ls', '-l'], stdout=stream, stderr=subprocess.STDOUT)

while proc.poll() is None:
    time.sleep(0.05)

stream.close()
output = stream.buffer.getvalue()
print output

Edit: When running the test code above, Python doesn't exit once the command output has been printed.  Such behaviour isn't exhibited by my application in which this class is being used.

I don't know enough about threading to help with that part, but it looks like your class could benefit from implementing the __enter__ and __exit__ functions that would allow it to be used with the with keyword.

There's a short section in the docs about it as well as an entry in the PEP from when it was added to Python. with allows you to instantiate an object that involves some connection or file that needs to be closed and will always ensure that even if errors arise, the object will be closed accordingly regardless of those errors. It's like wrapping your document with try: finally: stream.close(), but without needing to add that every time you call the stream.

Here's how it might look in your object:

def __enter__(self):
    # Theoretically could be used to set up things not in __init__
    return self

def __exit__(self, exc_type, exc_val, exc_tb):
    self.close()

The parameters that __exit__ takes will be passed to it are just exception information, you could use them if you wanted to but they have to be included in the definition as they'll be passed as part of the with syntax.

Now you could use the object like this:

with OutputStream() as stream:
    proc = subprocess.Popen(['ls', '-l'], stdout=stream, stderr=subprocess.STDOUT)

    while proc.poll() is None:
        time.sleep(0.05)


Initializing and printing an array using pointers and dynamic allocation in C I have written a code for basic array creation and printing it's values in C. I would appreciate comments on how it can be improved, and the industry standards.

One issue I'm encountering is having no way to tell printArr() what the size of the array is.

Some questions:

Can I use arr[i] anywhere in the functions instead of **arr? (Gave error when I tried it)
What is the most common way of dealing with arrays when writing professional code? Is my approach correct?

int * initArr (int ** arr, int n)
{
    *arr=(int *) malloc(sizeof(int*)*n);
    printf("size is %d\n", sizeof(*arr));
    int i;
    for (i=0;i<n;i++)
    {
        *(*arr+i)=i+10;
    }
    return *arr;
}

void ** printArr (int ** arr, int n)
{
    int i;
    for (i=0;i<n;i++)
    {
        printf("Arr element %d is %d\n", i, *(*arr+i));
    }
    return *arr;
}

int main(void) {

    int *arr2;
    arr2=initArr(&arr2,10);
    printArr(&arr2,10);

    return 0;
}

Running the above code (with includes for stdio and stdlib) produces:

size is 8
Arr element 0 is 10
Arr element 1 is 11
Arr element 2 is 12
Arr element 3 is 13
Arr element 4 is 14
Arr element 5 is 15
Arr element 6 is 16
Arr element 7 is 17
Arr element 8 is 18
Arr element 9 is 19

There are a number of things I would change here, so I'm going to go through the process incrementally, applying changes to the whole file in passes rather than in chunks. I think this should make it more clear why the changes are implemented.

C99 Note

You should have access to, at the very least, a C99 compiler. If that's not true, you can ignore this first point, but if it is, you should change your counter declarations to be inside the first for clause.

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

Pointers

In initArr, you both return the int array and assign it to the provided out parameter, arr. Why? Out parameters make code confusing and hard to follow! They make sense if you're performing the array allocation somewhere else, but since you're performing the array allocation inside initArr, just drop the arr parameter and return the value.

As a side effect of this, since you no longer need a double pointer, you can replace your dereferences with simple array accesses.

int * initArr (int n)
{
    int * arr = (int *) malloc(sizeof(int*)*n);
    printf("size is %d\n", sizeof(arr));
    for (int i=0;i<n;i++)
    {
        arr[i]=i+10;
    }
    return arr;
}

Next, why are you passing a double pointer (int**) to printArr? More importantly, why does printArr have a return value if it's a purely side-effectful function? Change the parameter to int* and make the function return void.

void printArr (int * arr, int n)
{
    for (int i=0;i<n;i++)
    {
        printf("Arr element %d is %d\n", i, arr[i]);
    }
}

This also simplifies the main function.

int main(void) {
    int * arr2 = initArr(10);
    printArr(arr2,10);

    return 0;
}

Next, let's take a look at the allocation itself (the one in initArr). First of all, you cast to (int *) manually, which is unnecessary and downright discouraged in C. If you're using C++, it's necessary (though you shouldn't need malloc in C++, anyway), but with a C compiler, just drop it.

Second of all, you are not actually allocating the right data! You're allocating n slots for int* values—int pointers. You actually want int data. This might not matter depending on your architecture and compiler, but it's still poor code. Fortunately, you can actually fool-proof this—don't pass an explicit type of sizeof at all! Just deference arr itself, and the compiler will calculate that value's size.

Those changes simplify the allocation to this:

int * arr = malloc(sizeof(*arr)*n);

Finally, the line just after that—the printf line—is useless. It will always print the same value because it's checking the size of a pointer, which is always the same size regardless of type (though it can change on different architectures). That said, the line doesn't make any sense there. A function called initArr shouldn't have side effects, anyway. Just take it out.

Style and Warnings

I'm not sure if you just omitted it from your code, but your code depends on functions declared in external header files in the standard library. Include these at the top of your file.

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

Next, let's talk about code style. You are writing some very densely-packed expressions in some places. Some whitespace can make code much more readable! For example, change the for loops to this:

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

Similarly, change your allocation to this:

int * arr = malloc(sizeof(*arr) * n);

Proper spacing makes code more readable and therefore more maintainable!

Finally, let's talk about pointer declarations. You are declaring your pointers with the asterisks just sort of "floating". This is actually not a bad compromise. Some people prefer them to be grouped with the type (int* foo), others with the name (int *foo). I prefer the former style, so in my final copy of the code, I changed them accordingly, but this is often merely personal preference.

Result

Here is all the code as it stands with the above changes implemented! Not only is it more readable due to style, but it's easier to understand the control flow since it no longer unnecessarily indirects variables via reference.

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

int* initArr(int n)
{
    int* arr = malloc(sizeof(*arr) * n);
    for (int i = 0; i < n; i++) {
        arr[i] = i + 10;
    }
    return arr;
}

void printArr(int* arr, int n)
{
    for (int i = 0; i < n; i++)
    {
        printf("Arr element %d is %d\n", i, arr[i]);
    }
}

int main(void) {
    int* arr2 = initArr(10);
    printArr(arr2, 10);

    return 0;
}

Extra Notes

  Can I use arr[i] anywhere in the functions instead of **arr? (Gave error when I tried it)

You can! But * has lower precedence than array subscript ([]), so you'd need to use grouping operators

(*arr)[i]

This became unnecessary with the other changes, though.

  What is the most common way of dealing with arrays when writing professional code? Is my approach correct?

Depends on the situation. Honestly, the best away to handle arrays is to not use them—if you're using a language with any more power than C, you should have higher-level language constructs (std::vector, ArrayList, etc.). However, if you have to use C, abstracting them behind an API is a good start.

  One issue I'm encountering is having no way to tell printArr() what the size of the array is.

Yes, in C, arrays are just blocks of memory, so you can't get their length at runtime because it's not stored anywhere! If you wanted to do this, though, you could encapsulate your array in a struct.

typedef struct my_array {
    unsigned int length;
    int* data;
} my_array_t;

You could then return a my_array_t from initArr and pass a my_array_t to printArr, which could use arr.length to get the length.


Basic Subnet Mask Calculator + A Little Extra For my Networking class, we learned about subnetting and subnet masks.

I decided to write a little "calculator" (a generous term) that when given n-many network bits, generates a 32-bit subnet mask, then displays it as octets, and as binary with the leading 0s removed. Example main:

int main() {
    int nNetworkBits = 26;
    BitField mask = generateSubnetMask(nNetworkBits);

    printf("%u\n", mask);  // Prints 4294967232
    printf("\n");

    printOctets(mask);  // Prints 255.255.255.192
    printf("\n");

    printBinary(mask);  // Prints 11111111111111111111111111000000
    printf("\n");
}

I'm pretty terrible at C, so I'd like comments on anything and everything that is worth mentioning. I figured out the math/bitshifting as I was writing the code, so if I'm doing anything "roundabout", I'd definitely like to know about it.

The only thing that I don't need advice on is my "abuse" of printf. I know I should be accumulating a string in a buffer then returning that buffer instead of printing directly from the functions. That's a pain in C though and detracted from the main problem that I was trying to solve, so I decided to go for the quick and dirty way.

#include <stdio.h>
#include <stdbool.h>
#include <stdint.h>

#define TOTAL_BITS 32
#define N_OCTETS ((TOTAL_BITS / 8))

typedef uint32_t BitField;

BitField generateOctetMask(int octetI) {
    return (BitField)0xFF << (octetI * 8);
}

BitField generateSubnetMask(int nNetworkBits) {
    int nHostBits = TOTAL_BITS - nNetworkBits;
    return ~0 << nHostBits;  // TODO: Cast 0 to BitField?
}

void printOctets(BitField maskAddress) {
    for (int octetN = N_OCTETS - 1; octetN >= 0; octetN--) {
        BitField mask = generateOctetMask(octetN);
        printf("%d", (maskAddress & mask) >> (octetN * 8));

        if (octetN > 0) {
            printf(".");
        }
    }
}

void printBinary(BitField n) {
    bool areDropping = true;  // So we can drop leading 0s.

    for (BitField mask = 1u << 31; mask > 0; mask >>= 1) {
        bool isSet = n & mask;

        if (isSet) {
            areDropping = false;
            printf("1");

        } else if (!areDropping) {
            printf("0");
        }
    }
}

int main() {
    int nNetworkBits = 26;
    BitField mask = generateSubnetMask(nNetworkBits);

    printf("%u\n", mask);  // Prints 4294967232
    printf("\n");

    printOctets(mask);  // Prints 255.255.255.192
    printf("\n");

    printBinary(mask);  // Prints 11111111111111111111111111000000
    printf("\n");
}

Here are some things that may help you improve your program.

Prefer const to #define

For constants such as TOTAL_BITS, it's generally better to use a named const value rather than a #define.  The primary reason is that the const definition enforces type checking, while the macro does not.  However, see the next suggestion.

Use sizeof for portability

The code currently contains these lines:

#define TOTAL_BITS 32
#define N_OCTETS ((TOTAL_BITS / 8))

typedef uint32_t BitField;

However, I think I'd define everything instead as const values all derived from the type.  That is, I'd suggest this:

typedef uint32_t BitField;
const int BitFieldOctets = sizeof(BitField);
const int BitFieldBits = 8 * BitFieldOctets;

Think carefully about signed vs. unsigned

If generateSubnetMask is passed a negative number, what should it do?  I'd suggest that the passed number should be unsigned instead of int and that the value could be checked to make sure it does not exceed the naximum number of bits.

Be careful when shifting negative values

By default, constants are int, so this code:

return ~0 << nHostBits;

Should probably instead be written like this:

return ~0u << nHostBits;

Avoid pointless recalculations

The code has this function:

void printOctets(BitField maskAddress) {
    for (int octetN = BitFieldOctets - 1; octetN >= 0; octetN--) {
        BitField mask = generateOctetMask(octetN);
        printf("%d", (maskAddress & mask) >> (octetN * 8));

        if (octetN > 0) {
            printf(".");
        }
    }
}

This is more complex than it needs to be.  First, the mask can be generated just once for the high byte and then shifted, eliminating a number of calculations that don't need to be done:

void printOctets(BitField maskAddress) {
    BitField mask = generateOctetMask(BitFieldOctets - 1);
    for (unsigned i=BitFieldOctets; i; --i) {
        printf("%d", (maskAddress & mask) >> (BitFieldBits - 8));
        maskAddress <<= 8;
        if (i != 1) {
            printf(".");
        }
    }
}

This works by keeping the mask in place over the most significant byte and then shifting the address to the left each iteration.  An alternative would be to use a union as the BitField type to allow access to each byte very simply.  That approach, however, requires that you account for the endian-ness of your machine, which this code does not.

Consider restructuring for simplicity and clarity

The printBinary code is currently like this:

void printBinary(BitField n) {
    bool areDropping = true;  // So we can drop leading 0s.

    for (BitField mask = 1u << 31; mask > 0; mask >>= 1) {
        bool isSet = n & mask;

        if (isSet) {
            areDropping = false;
            printf("1");

        } else if (!areDropping) {
            printf("0");
        }
    }
}

Instead, I'd break this into two different loops: the first to skip leading zeroes and the second to actually print the bits.  Here's one way to do that:

void printBinary(BitField n) {
    BitField mask = 1u << (BitFieldBits - 1);
    // skip leading zeroes
    for (; mask && !(mask & n); mask >>= 1);
    // print all other bits
    for (; mask; mask >>= 1) {
        putchar(n & mask ? '1' : '0');
    }
}

Note that here I'm using the named constants rather than the "magic number" 31.


Project Euler #4 - Largest Palindrome Product in Java I am seeing that this is actually not in accord with the difficulty level said there. Firstly I cannot list all possible improvements it could have.

/*
 * A palindromic number reads the same both ways. The largest palindrome
 * made from the product of two 2-digit numbers is 9009 = 91 × 99. Find the
 * largest palindrome made from the product of two 3-digit numbers.
 */
public static int get4() {
    int max = 1;
    for (int i = 100; i < 999; i++) {
        for (int j = i; j < 999; j++) {
            int n = i * j;
            int len = (int) Math.floor(Math.log10(n));
            boolean ispalindrome = true;
            for (int k = 0; k < 3; k++) {// 100^2=10^4,999^2=998001. 5-6
                                            // digits middle atmax 3
                int digitatk = (int) ((n % Math.pow(10, k + 1) - n
                        % Math.pow(10, k)) / Math.pow(10, k));
                // eg. digit at 3 of 12345 =
                // (12345%10^4-12345%10^3)/10^3=(2345-345)/10^3=2000/10^3=2
                int l = len - k;
                int digitatl = (int) ((n % Math.pow(10, l + 1) - n
                        % Math.pow(10, l)) / Math.pow(10, l));
                if (digitatl != digitatk) {
                    ispalindrome = false;
                }
            }
            if (ispalindrome && n > max) {
                max = n;
            }
        }
    }
    return max;
}

Output:

Answer is 906609
Time taken 2.82342934 seconds

Current problems (not exhaustive):

Time
\${\mathcal O}(n^2)\$ where \$n=999\$ for looping only.
Length-checking method
Checking palindrome
Digit-finding method

Algorithm
Your time and complexity problems are probably the more serious ones. Your code starts small (at 100), and then works up to 1000. This is not the most efficient way of brute-forcing the problem. A better solution would be to start at 999 and work back to 100, but limit it when the result becomes impossible.... let me explain:
    int max = Integer.MIN_VALUE;
    for (int i = 999; i > 100 && i * 999 > max; i--) {
        for (int j = 999; j > i; j--) {
            ....

start high (999 x 999)
Work backwards from there where the second number is larger than the first
track your maximum
terminate the loops if the current i (or smaller) values will never beat the maximum.

Using the above logic, you will eliminate a huge number of loops.
Length check & Finding digit
The Log function is a good check for the length of the value, it's fine. On the other hand, it may not be necessary at all... same applies to the "finding digit"
Palindrome
I like the following method for identifying palindromic numbers:
private static final int reverse(int value) {
    int result = 0;
    while (value != 0) {
        result *= 10;
        result += value % 10;
        value /= 10;
    }
    return result;
}

Using that function, your palindrome check simply becomes:
boolean ispalindrome = n == reverse(n);


Can someone improve this C# code Its working but I don't think its the best way of doing it. I also changed variable names just to let you know,

       string [] columnN = AnotherString.Split(',');  //Another String is something like this = "HEhehehehehE heeh, aSKdjhkaaksjd, asldkhja slkdlk, asdajsdlka, asdljkasd, asdkasjdasd, asdasdasdl, askdjasd"
                AnotherString = "";
                int i = 0;

            foreach (string cN in columnN)
            {
                if (!string.IsNullOrEmpty(Res.ResourceManager.GetString(cN.ToLower().Trim())))
                    AnotherEmptyString += Res.ResourceManager.GetString(cN.ToLower().Trim());
                else
                    AnotherString += cN;
                i++;

                if (i < columnN.Length)
                    AnotherString += ",";
            }

I saved all resources in lower case, also if in case cN doesn't got any resource won't it will give an exception.

Your code should work ok. However, I would have used a list to store the intermediate results, and then string.Join to create the result. (Also perhaps Linq to filter the data, but then someone not familiar with Linq will have trouble maintaining the code.) Something like

string[] columnN = AnotherString.Split(',');
string[] stringList = new string[columnN.Length];

for (int i = 0; i < columnN.Length; i++)
{
    string cN = columnN[i];
    string resource = Res.ResourceManager.GetString(cN.ToLower().Trim()));
    stringList[i] = resource ?? cN;
}

AnotherString = string.Join(",", stringList);


Using d3.drag() to enable dragging on an SVG group I'm using the following function to enable dragging on an SVG group.

var dragcontainer = d3.drag()
                        .on("start", function () { })
                        .on("drag", function (d, i) {

                            this.x = this.x || 0;
                            this.y = this.y || 0;

                            this.x += d3.event.dx;
                            this.y += d3.event.dy;
                            d3.select(this).attr("transform", "translate(" + this.x + "," + this.y + ")");

                        })
                        .on("end", function () {});

d3.select("#svg-group-id").call(dragcontainer);

Is there a better (simpler) way to achieve this than manually updating the translate destination?

Yes, there is a simpler way to achieve this: just use d3.event.x and d3.event.y, holding their values in the bound data.

Thus, your drag function can be reduced to only this:

var dragcontainer = d3.drag()
    .on("drag", function(d, i) {
        d3.select(this).attr("transform", "translate(" + (d.x = d3.event.x) + ","
        + (d.y = d3.event.y) + ")");
    })

And, since you don't use them, just drop your "start" and "end" listeners.

Here is the demo (click and drag the group around):

var dragcontainer = d3.drag()
  .on("drag", function(d, i) {
    d3.select(this).attr("transform", "translate(" + (d.x = d3.event.x) + "," + (d.y = d3.event.y) + ")");
  });
var g = d3.select("g").datum({x: 0, y: 0}).call(dragcontainer)
<script src="https://d3js.org/d3.v4.min.js"></script>
<svg style="border: 1px solid gray">
  <g>
    <circle cx="50" cy="50" r="20" fill="teal"></circle>
    <circle cx="100" cy="50" r="20" fill="tomato"></circle>
    <circle cx="150" cy="50" r="20" fill="tan"></circle>
  </g>
</svg>