Counting ways to form an amount using coins This is just a fun little exercise I had to do for a homework once (in Java rather than Clojure though). Basically, the goal is to find the number of different coin stacks you can build with the coins 1,2,5 and 10 to form a number N (I believe a closed form solution for this exists, but this isn't what this is about).

My solution here works, but I'm not quite happy with it:

Explicit loop. Not sure how to do away with it though.
A few functions there that I think should be core functions, but I can't find them.
Even if they aren't in core, my functions could probably be written somewhat more concisely

Anyways, here's my code (by the way, the function is called "fast-iterative" because the exercise was about calculating these values recursively. Which I think is insane due to the branching factor, but whatever):

(ns test.coin-stacks)

(def maximum (partial reduce max))

(defn shift-vector
  "Shifts v one to the left, insert shift-val at the right"
  [v shift-val]
  (conj (vec (rest v)) shift-val))

(defn update
  "Updates ks in m with f applied to ks.
  I can't believe I have to write this myself."
  [m ks f]
  (apply assoc m (interleave ks (map (comp f m) ks))))

(defn fast-iterative-coinstacks
  "Returns the number of ways to form a coin stack with a total
  value of n with coins"
  [n coins]
  (let [max-coin (maximum coins)
        initial-stacks (apply conj [1] (repeat max-coin 0))]
    (loop [stacks initial-stacks iteration 0]
      (if-not (< iteration n)
        (first stacks)
        (recur (shift-vector (update stacks coins (partial +' (stacks 0))) 0)
               (inc iteration))))))

How about something like this:

(defn shift-vector
  "Shifts v one to the left, insert shift-val at the right"
  [v shift-val]
  (conj (subvec v 1) shift-val))

(defn update-in-all
  "Updates ks in m with f applied for each value at k."
  [m ks f]
  (reduce (fn [m' k] (update-in m' [k] f)) m ks))

(defn fast-iterative-coinstacks
  "Returns the number of ways to form a coin stack with a total
  value of n with coins"
  [n coins]
  (let [max-coin (reduce max coins)
        initial-stacks (into [1] (repeat max-coin 0))
        process-stacks (fn [stacks]
                         (-> stacks
                             (update-in-all coins (partial + (first stacks)))
                             (shift-vector 0)))]
    (-> (iterate process-stacks initial-stacks)
        (nth n)
        first)))

Some explanations:

update-in works both with vectors and maps, and can update nested values as well,
the threading macro -> makes the flow of your code easier to follow,
iterate is a really convenient function that given f and x returns a lazy sequence of x, (f x), (f (f x))...
you can do a lot of things with reduce!


8051 RFID reader for Atmel microcontroller I am currently using an AT89S52 microcontroller for a major project that involves a use of an RFID reader that outputs data in a serial format (9600 baud, no parity, 8 data bits, 1 stop bit).

The only hardware UART embedded in the microcontroller is used for other purposes and I don't want to redesign my circuit board or pay higher dollars only for a second hardware UART so my only option is to use a software UART.

I made a routine employing the software UART technique that is able to constantly read data from an RFID card module. Fortunately I am able to read the data correctly (example: the 1st and last bytes of the data packet are correct).

The problem I have with this code is that it is synchronous because of all the halting loops (see the DJNZ lines).

The card reader (#RDM6300) produces at least 13 bytes of data per card.

Each bit at 9600bps is 0.1ms. which is 1.0ms/byte (due to adding the start and stop bit) which is 13.5ms delay for all 13 bytes (provided the reader module sends them all at once). I want to somehow modify my code so that I have a lower delay (1-2ms is more tolerable). There are also buttons on the board that users may press very fast and I don't want the buttons to appear frozen (unresponsive) only because the system is stuck in the middle of reading an RFID card.

What can I do to my code to improve it so that the required delay isn't large?

  ; crystal used = 22.1184Mhz
  ; micro used = AT89S52
  ; 1 clock cycle = 12 oscillator cycles

  ; Took calculation partly from
  ; https://www.8051projects.net/wiki/8051_Software_UART_Tutorial
  ; 1 bit time = (((crystal/baud)/12) - 5) / 2
  ; 1 bit time = ((22118400/9600/12) - 5) / 2 = 187
  ; was told on a forum to use half as full bit time...
  ; 187/2 = 93.5 = 5Dh
  ; 1/2 = 93.5 / 2 = 46.75 = 2Eh
  ; 1/2 bit time seemed to read correct data

  BITTIM equ 05Dh ;1 bit time for 9600 baud
  BITTIMH equ 2Eh ;1/2 bit time
  RX equ Px.x ;serial data

  main:

; -- insert asynchronous function here --

initserial:

;wait for serial start (data=0)
jb RX,$

;wait 1/2 bit time
mov R2,#BITTIMH
djnz R2,$

;If data isn't zero after 1/2 bit time
;we assume out of sync and start over
jb RX,initserial

mov R3,#8h ;setup to read 8 bits
clr A      ;and clear byte

readnextbit:

  ;wait 1 full bit time
  mov R2,#BITTIM
  djnz R2,$

  ;add 1 clock cycle waiting time (about .5uS)
  ; -- I'm trying to align data here --
  nop

  ;get bit and shift into accumulator
  mov C,RX
  RRC A

  ;continue until all 8 bits are read
DJNZ R3,readnextbit
;result stored in accumulator
  sjmp main

Here are some ideas that may be useful to you.

Decide what's most important

Since the 8051, including your AT89S52 variant, has few resources, you'll have to make some hard decisions on how to use them.  If you're already using the single serial port for some other purpose and it's fixed for some reason (e.g. can't respin boards), then perhaps it's out of your hands.  If not, then taking a good look at both UART uses would likely be useful.  It would be most useful to put the less frequent, lower bit rate, less important traffic on a software UART.  Sometimes the right choice is readily apparent; other times not so much.  Another possibility is to multiplex them into the single hardware UART, but of course that only works if it's acceptable to block reception of one UART while the other is receiving.

Use an interrupt

The sole purpose for the delays is timing, so I'd suggest using a timer instead.  As you know, this CPU only has three timers and Timer 2 is tied to the Baud Rate Generator for the hardware UART, so that leaves Timer 0 and Timer 1.  You may be able to use one of those in either mode 0 (13-bit timer) or mode 1 (16-bit timer).  One way to do this would be to use two interrupts.  One interrupt would be set to trigger on the falling edge of RX.  Once that's detected, disable that interrupt and enable a timer interrupt for half bit time.  When the timer triggers, read the bit and if it's not a 0, that is, not a true start bit, disable the timer interrupt and re-enable the falling edge interrupt.  If it is a zero, count it as a start bit and set the timer for a full bit time.  Thereafter, just read the bits for each timer interrupt until you get to the stop bit.  If the stop bit is not 1 when it's supposed to be, you'll probably want to signal a framing error.  If it is, the byte is probably good and can be stored.  Either way, disable the timer interrupt and re-enable the falling edge interrupt.  It's not ideal, but it should work, given the limitations.


Find the second largest number in an array with at most n + log2 n - 2 comparisons This exercise is from Roughgarden's (excellent) course on algorithms on Coursera:

  You are given as input an unsorted array of n distinct numbers, where
  n is a power of 2. Give an algorithm that identifies the
  second-largest number in the array, and that uses at most \$n + \log_2(n) - 2 \$ comparisons.

Note that it doesn't ask just ask to solve the problem in linear time. If we use \$2 n\$ comparisons the "big-O" time complexity would be linear but greater than \$n + \log_2(n) - 2\$.

Any feedback on the algorithm, implementation, style, and Python is very welcome.

import random
import sys

from math import log
count = 0
def is_greater(a, b):
    global count
    count += 1
    return a > b

def _get_highest_two(arr0, arr1):
    if is_greater(arr0[0], arr1[0]):
        first = arr0[0]
        seconds = arr0[1]
        seconds.append(arr1[0])
    else:
        first = arr1[0]
        seconds = arr1[1]
        seconds.append(arr0[0])
    return (first, seconds)

def get_second(*args):
    if len(args) == 1:
        seconds = args[0][1]
        second_best = seconds.pop()
        for sb in seconds:
            if is_greater(sb, second_best):
                second_best = sb
        return second_best

    _pairs = zip(*[iter(args)]*2)
    _out = [_get_highest_two(*p) for p in _pairs]
    return get_second(*_out)

def main():

    n = int(sys.argv[1])
    arr = [random.randint(0,100000000) for _ in xrange(n)]

    print arr
    print n, sorted(arr)[-2]
    print count
    # is_greater(0,0)
    _pairs = zip(*[iter(arr)]*2)
    _pairs = [(a, [b]) if is_greater(a, b) else (b, [a]) for (a, b) in  _pairs]
    print get_second(*_pairs)
    print count, n + log(n, 2) - 2

    return 0

if __name__ == '__main__':
    sys.exit(main())

Algorithm and implementation

The algorithm is correct. Essentially it compares the elements pairwise, recursively until all pairs are eliminated, while at each comparison, tracking the compared elements in a list. At the end, the algorithm finds the maximum value, and a list of \$\log_2(n)-1\$ elements, which must contain the 2nd maximum value.

The implementation looks more complicated than it needs to be, and I find it hard to read. I suggest to organize and implement differently:

Create a function find_max_with_tracking that takes a list of values and returns a tuple of the maximum value and the list of values the maximum was compared to:

Map the input list of values to a list of tuples: [(x, []) for x in input]
Recursively reduce the list of tuples, comparing then pairwise, keeping the tuple with the higher value, and adding to its list the value of the other tuple. This could be implemented either with a recursive function, or with a loop.
Return the single tuple that remains, containing the maximum element, and a list of \$\log_2(n)-1\$ values that must contain the second largest value.

This point is reached by n - 1 comparisons, and we have \$\log_2(n)\$ values to scan to find the second largest value. You can use a simple linear scan for this step.

This implementation should be simpler and easier to read.

Programming in general

Avoid global variables as much as possible. If you need to track something, such as the count in this example, then you can wrap the operation in an abstract data type that manages the state and operations (functions) on the state.

Excessive array creation in a loop or recursive function as in get_second is usually a bad sign, to investigate closely and try to eliminate. In this example this was an important hint that the implementation is probably inefficient.

Programming in Python

The implementation uses Python 2 features that are obsolete today, such as print without parentheses (the old keyword instead of the modern function) and xrange instead of range. I suggest to test your scripts using Python 3.


Deeply nested search through Document tree I have this code block, where I search for the value of Ordnungsbegriff. I have to search through a tree of my CurrentDocument.

How can I rewrite it to be more dynamic? I don't know at which level I will find my value.

string xy = "";
try
{

    if (string.IsNullOrEmpty(CurrentDocument.Ordnungsbegriff))
    {
        if (string.IsNullOrEmpty(CurrentDocument.ParentDocument.Ordnungsbegriff))
        {
            if (string.IsNullOrEmpty(CurrentDocument.ParentDocument.ParentDocument.Ordnungsbegriff))
            {
                if (string.IsNullOrEmpty(CurrentDocument.ParentDocument.ParentDocument.ParentDocument.Ordnungsbegriff))
                {
                    if (string.IsNullOrEmpty(CurrentDocument.ParentDocument.ParentDocument.ParentDocument.ParentDocument.Ordnungsbegriff))
                    {
                        if (string.IsNullOrEmpty(CurrentDocument.ParentDocument.ParentDocument.ParentDocument.ParentDocument.ParentDocument.Ordnungsbegriff))
                        {
                            if (string.IsNullOrEmpty(CurrentDocument.ParentDocument.ParentDocument.ParentDocument.ParentDocument.ParentDocument.ParentDocument.Ordnungsbegriff))
                            {
                                xy = "Nach 6. Ebene, Suche abgebrochen.";
                            }
                            else
                                xy = CurrentDocument.ParentDocument.ParentDocument.ParentDocument.ParentDocument.ParentDocument.ParentDocument.Ordnungsbegriff;
                        }
                        else
                            xy = CurrentDocument.ParentDocument.ParentDocument.ParentDocument.ParentDocument.ParentDocument.Ordnungsbegriff;
                    }
                    else
                        xy = CurrentDocument.ParentDocument.ParentDocument.ParentDocument.ParentDocument.Ordnungsbegriff;
                }
                else
                    xy = CurrentDocument.ParentDocument.ParentDocument.ParentDocument.Ordnungsbegriff;
            }
            else
                xy = CurrentDocument.ParentDocument.ParentDocument.Ordnungsbegriff;
        }
        else
            xy = CurrentDocument.ParentDocument.Ordnungsbegriff;
    }
    else
      xy = CurrentDocument.Ordnungsbegriff;

}
catch (Exception)
{
    xy = "Fehler in  Suche";
}

I find recursion complicated You can use a while loop:

private string SearchDocument(Document doc)
{
    while (doc != null)
    {
        if (!string.IsNullOrEmpty(doc.Ordnungsbegriff))
        {
            return doc.Ordnungsbegriff;
        }
        doc = doc.ParentDocument;
    }
    return null;
}

Edit: As a further point - it's better not to mix languages in your code: Ordnungsbegriff should be Keyword (according to Google).


Mocking Interfaces Golang I thought of doing the following to mock/test functions in Go and wanted some advice. Let's say you have an interface for a repository:
    type Creator interface {
        CreateUser(User) (*User, error)
    }

    type Updater interface {
        UpdateUser(User) (*User, error)
    }

    type Retreiver interface {
        GetUserByEmail(string) (*User, error)
        GetUserByID(uint) (*User, error)
    }

    type Repository interface {
        Creator
        Updater
        Retreiver
    }

The mock interface implements the Repository interface. It provides mock functions that can be swapped out depending on the test:
type MockUserRepo struct {
    CreateUserMock     func(user.User) (*user.User, error)
    UpdateUserMock     func(user.User) (*user.User, error)
    GetUserByEmailMock func(string) (*user.User, error)
    GetUserByIDMock    func(uint) (*user.User, error)
}

func (m *MockUserRepo) CreateUser(u user.User) (*user.User, error) {
    return m.CreateUserMock(u)
}

func (m *MockUserRepo) UpdateUser(u user.User) (*user.User, error) {
    return m.UpdateUserMock(u)
}

func (m *MockUserRepo) GetUserByEmail(s string) (*user.User, error) {
    return m.GetUserByEmailMock(s)
}

func (m *MockUserRepo) GetUserByID(i uint) (*user.User, error) {
    return m.GetUserByIDMock(i)
}

Then in the test function, you can create a mock for the single function that can be called. This can sometimes cause a nil pointer error if the function wasn't implemented/mocked by the testcase.
mockUserRepo := &MockUserRepo{
    GetUserByEmailMock: func(s string) (*user.User, error) {
        return nil, user.ErrUserDoesNotExist
   },
}

Any thoughts on if this is a bad/good practice?

This seems fine to me: it's nice and simple, and I've seen this pattern used "in the wild" before. One thing you could do is, in the MockUserRepo proxying functions, if the function variable is nil, either execute a default implementation, or return a more specific error (or even panic). For example:
func (m *MockUserRepo) GetUserByEmail(s string) (*user.User, error) {
    if m.GetUserByEmailMock == nil {
        // default implementation: return a dummy user object
        return &user.User{ID: generateID(), Email: s}, nil
    }
    return m.GetUserByEmailMock(s)
}

// or:

func (m *MockUserRepo) GetUserByEmail(s string) (*user.User, error) {
    if m.GetUserByEmailMock == nil {
        // default implementation: panic with a more specific error
        panic("MockUserRepo: GetUserByEmail not set")
    }
    return m.GetUserByEmailMock(s)
}

The reason I'd be okay with a panic here is because not setting GetUserByEmail is almost certainly a coding bug in the test, not really a runtime error condition. This latter approach doesn't gain you much over just letting the nil pointer access panic, except that the error message is a bit more specific.


Calculating the perfect squares between two given numbers This code is for calculating the perfect squares between two given numbers. User can add the interval as well like for how many intervals he want to check the perfect squares. For example if user enter 4, it means there are 4 different intervals and after that he will insert the tow number for interval 1, similarly two numbers for interval 2 and respectively 3 and 4. I just want to reduce the execution time

import java.io.*;
import java.util.*;

class node{
    private int a;
    private int b;
    node(int a, int b){
        this.a = a;
        this.b = b;
    }
    int getA(){return a;}
    int getB(){return b;}
}

public class Solution {

    public static void main(String[] args) {

        int size;
        Scanner S1 = new Scanner(System.in);
        size = S1.nextInt();
        node a[] = new node[size];
        for(int i = 0; i<a.length; i++){
        /*int number = S1.nextInt();
        int number2 = S1.nextInt();*/
        a[i] = new node(S1.nextInt(), S1.nextInt());
        }
        int total = 0;
        int sqrt = 0;
        for(int i = 0; i<a.length; i++){
                for(int j = a[i].getA(); j<=a[i].getB(); j++){
                    sqrt = (int) Math.sqrt(j);
                    if(sqrt*sqrt == j) {
                        total++;
                    }
                }
             System.out.println(total);
            total = 0;
        }
        }
}

First things first. You have to organize your code and group functionality into methods. So I would suggest creating a method int[] getPerfectSquaresBetween(int start, int end).

Let's enhance your algorithm. The key idea is to get the square root of the next perfect square number. This can be achieved by ceil the sqrt of the starting point:

int candidate = (int) Math.ceil(Math.sqrt(start));

This variable is the basis for our calculations. All we have to do now is to check if the candidate squared is in the given range.

int square;
while ((square = candidate * candidate) < end) {
  //...
  candidate++;
}

Putting everything together looks like this:

public static int[] getPerfectSquaresBetween(int start, int end) {
  if (start > end || start < 0) {
    throw new IllegalArgumentException();
  }
  int[] perfectSquares = new int[end - start];
  int nr = 0;
  int candidate = (int) Math.ceil(Math.sqrt(start));
  int square;
  while ((square = candidate * candidate) < end) {
    perfectSquares[nr++] = square;
    candidate++;
  }
  return Arrays.copyOf(perfectSquares, nr);
}

Let's check the algorithm: getPerfectSquaresBetween(0, 101) (which completes in microseconds)

  [0, 1, 4, 9, 16, 25, 36, 49, 64, 81, 100]


Should I call this extension method Remove or RemoveMany? I know this is extremely trivial, but would you call the extension method below Remove or RemoveMany? Please provide justification.

public static void Remove<TEntity>(this DbSet<TEntity> set, IEnumerable<TEntity> entities)
    where TEntity : class
{
    foreach (var entity in entities)
        set.Remove(entity);
}

-- or --

public static void RemoveMany<TEntity>(...) { ... }

Your method is removing a collection of entities that exist in some set.

When I see Remove, I'd say it removes a single item from something. Now if that collection actually existed in the set, then it would be perfect, but alas it's a collection of items to remove.

One thing to consider, assuming you're making an extension method for the EF5 DbSet, it already has a Remove method that removes a single item, I think an overload to remove more items doesn't sound like a bad idea to me.

On the other hand, LINQ to XML defines a Remove() method on enumerables of XNode.  But that's an extension on the collection of entities, not the parent set.  I'm a little on the fence on this one.

RemoveMany, while better as it indicates we're removing many items, doesn't fit well with me.  It mimics SelectMany by name but I'd associate the "Many" part with a projection, certainly not with a remove operation.  I'd avoid that one.

I might consider ExceptEntities to be similar to the LINQ method Except.  Except returns the set difference of two collections where the result contains unique values.  Since this operation isn't quite the same, we'll still need to make this distinguishable hence the added "Entities".  Since you are dealing with a set (?), this sounds perfect to me.  But after thinking about this more, I'm not quite sure this is the better option since this modifies the collection.

I think RemoveEntities might be a better fit.  "Remove" makes it sound as though we're modifying something.  RemoveAll doesn't cut it for me as it is named the same as List.RemoveAll which removes all that matches a predicate.  There's also a List.RemoveRange which removes a range of consecutive values in the collection, not quite the same.

So I'd go with RemoveEntities but an overload of the Remove name might be fine, just beware of the similarities of the names in other objects.


Binary tree level order traversal algoritm I am trying to solve this Binary tree lever order traversal:

Given binary tree {3,9,20,#,#,15,7},
    3
   / \
  9  20
    /  \
   15   7
return its bottom-up level order traversal as:
[
  [15,7]
  [9,20],
  [3],
]

How can I improve the speed of the code below? It always gives me a time exceed error.

/**
 * Definition for binary tree
 * public class TreeNode {
 *     int val;
 *     TreeNode left;
 *     TreeNode right;
 *     TreeNode(int x) { val = x; }
 * }
 */
public class Solution {

    public ArrayList<ArrayList<Integer>> levelOrderBottom(TreeNode root) {
        ArrayList<ArrayList<Integer>> tree = new ArrayList<ArrayList<Integer>>();
        Stack<Integer> stack = new Stack<Integer>();
        ArrayList<Integer> node = new ArrayList<Integer>();

        this.iterateAll(root, stack);

        while (!stack.empty()) {
            if (stack.peek() != null) {
                node.add(stack.pop());
                if (!stack.isEmpty()) {
                    if (stack.peek() != null) {
                        tree.add(node);
                        node = new ArrayList<Integer>();
                    } else {
                        stack.pop();
                        if (!stack.isEmpty()) {
                            if (stack.peek() != null) {
                                tree.add(node);
                                node = new ArrayList<Integer>();
                            }
                        }
                    }
                } else {
                    tree.add(node);
                }
            } else {
                stack.pop();
            }
        }
        return tree;
    }

        public Stack<Integer> iterateAll(TreeNode root, Stack<Integer> stack) {
        if (root != null) {
            stack.push(root.val);
            if (root.right != null) {
                iterateAll(root.right, stack);
            } else {
                stack.push(null);
            }

            if (root.left != null) {
                iterateAll(root.left, stack);
            } else {
                stack.push(null);
            }
        }
        return stack;
    }
}

Depending on number of nodes in your tree, recursive post-order traversal of the tree, might let you run into problems on the stack. If the number of nodes is huge, you might need to consider a non-recursive post-order traversal.

Have a look at http://leetcode.com/2010/10/binary-tree-post-order-traversal.html, which describes multiple solutions, both iterative and non-recursive.

On think to keep in mind is that the non-recursive algorithms use more memory, as it need to push nodes to a stack.

You might also consider doing a post-order tree traversal enumerator, but that might be for another day.


An ES6 array of numbers - Double last number, delete the first number I'm writing a JavaScript function for an array, based on a geographic sequence. \$[1, 2, 4, 8, 16]\$, etc.

  This function needs to populate the next number in the sequence
  $$item_n = item_{n-1} * 2$$
  If there are more than \$10\$ items in the array, remove the first
  (smallest) number.

  If the array reaches \$2^{15}\$ (no idea what this is), reset the array to
  its original contents - \$[1, 2, 4, 8, 16]\$.

I have it working, but I feel this could be written more efficiently...

updateSequence() {
  var sequence = this.get("geometricSequence");

  // Modify the sequence here
  function* values(sequence) {
    for (let prop of Object.keys(sequence))
    yield sequence[prop];
  }
  let arr = Array.from(values(sequence));

  const lastIndex = arr.length - 1;
  const lastValue = parseInt(arr[lastIndex]) * 2;
  arr.push(lastValue);
  if (lastIndex > 9) {
    arr.slice(0, 1);
  }
  if (lastValue > 32768) {
    arr = [1, 2, 4, 8, 16];
  }
  this.set('geometricSequence', arr);
}


CMakeLists and Clang sanitization options for an ncurses program I would like some feedback on a CMakeLists.txt file I created for compiling my project. I have pasted the CMakeLists as well as my source code below. One thing I would specifically appreciate feedback on the sanitization options I have enabled. Are there more I should enable, and/or should I should reduce? I know -fsanitize=address, -fsanitize=thread, and -fsanitize=memory groups can't be used with others (according to the clang documentation). Would one of the other groups be better preferred to use on a first-pass rather than the one I chose (address)?

Also - the blob feature I am using, I have based on a StackOverflow answer I read - I understand that this doesn't detect new C source files and I'm fine with that, but besides that subtle detail is this an okay practice to follow?

CMakeLists.txt

cmake_minimum_required(VERSION 3.13)
project(FirstProject C)

find_package(Curses REQUIRED)
include_directories(${CURSES_INCLUDE_DIR})

set(CMAKE_C_COMPILER clang)
set(CMAKE_C_STANDARD 99)
set (CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Weverything -fsanitize=undefined,integer,implicit-conversion,nullability,address,leak,cfi -flto -fvisibility=default")

FILE(GLOB Sources *.c)
add_executable(${CMAKE_PROJECT_NAME} ${Sources})
target_link_libraries(${CMAKE_PROJECT_NAME} ${CURSES_LIBRARIES})

main.c (Code comes from here)

#include <ctype.h>
#include <errno.h>
#include <inttypes.h>
#include <limits.h>
#include <ncurses.h>
#include <stdint.h>
#include <stdlib.h>
#include <string.h>

// Generous estimate of the maximum number of digits
// https://stackoverflow.com/a/10536254
#define ULL_DIGITS (3 * sizeof(unsigned long long))
#define ERR_MSG_MAX_LENGTH 32
#define NUL '\0'
#define NUL_SIZE 1

int ask_ull(unsigned long long *result, const char *prompt);

/**
 * Prints a prompt then reads an unsigned long long, using ncurses.

 * Returns 0 on success.  Returns errno on failure, which is set to

 * ERANGE, EDOM, or EIO.
 */
int ask_ull(unsigned long long *result, const char *prompt) {
    char buf[ULL_DIGITS + NUL_SIZE];
    char *endptr;
    printw("%s", prompt);
    getnstr(buf, ULL_DIGITS);
    *result = strtoull(buf, &endptr, 10);
    if (errno == ERANGE) {
        // Overflow or underflow
        return errno;
    }
    if (endptr == buf || strchr(buf, '-')) {
        // Unsuccessful conversion
        errno = EDOM;
        return errno;
    }
    while (isspace(*endptr)) endptr++;
    if (*endptr) {
        // Trailing junk
        errno = EIO;
        return errno;
    }
    errno = 0;
    return errno;
}

int main(void) {
    unsigned long long height, width, length;
    height = width = length = 0;

    char errmsg[ERR_MSG_MAX_LENGTH];
    errmsg[0] = NUL;

    initscr();
    printw("--- Volume Calculator --\n");
    if (!ask_ull(&length, "Enter length: ")) {
        sscanf(errmsg, "%s", "Unable to scan length");
    }
    if (!ask_ull(&width, "Enter width: ")) {
        sscanf(errmsg, "%s", "Unable to scan width");
    }
    if (!ask_ull(&height, "Enter height: ")) {
        sscanf(errmsg, "%s", "Unable to scan height");
    }

    if (errmsg[0] != NUL) {
        refresh();
        endwin();
        perror(errmsg);
        return errno;
    }

    unsigned long long volume = length * width * height;
    printw("Volume: %llu", volume);
    refresh();
    getch();
    endwin();
}

I am not familiar with the Clang sanitization command-line options so I can't give any feedback about those,
but regarding the CMake code I would suggest the following CMakeLists.txt file

cmake_minimum_required(VERSION 3.13)
project(FirstProject C)

find_package(Curses REQUIRED)

add_executable(${CMAKE_PROJECT_NAME} main.c)

target_include_directories(${CMAKE_PROJECT_NAME} PRIVATE ${CURSES_INCLUDE_DIR})
target_link_libraries(${CMAKE_PROJECT_NAME} PRIVATE ${CURSES_LIBRARIES})

if(NOT CMAKE_C_COMPILER_ID STREQUAL "Clang")
  message(WARNING "Use the Clang compiler instead. "
                  "FirstProject officially supports Clang "
                  "(although other compilers might work).")
endif()

target_compile_features(${CMAKE_PROJECT_NAME} PRIVATE c_std_99)
target_compile_options(${CMAKE_PROJECT_NAME} PRIVATE
  $<$<C_COMPILER_ID:Clang>:
    -Weverything
    -fsanitize=undefined,integer,implicit-conversion,nullability,address,leak,cfi
    -flto
    -fvisibility=default>)
target_link_options(${CMAKE_PROJECT_NAME} PRIVATE
  $<$<C_COMPILER_ID:Clang>:
    -fsanitize=undefined,integer,implicit-conversion,nullability,address,leak,cfi
    -flto>)

Some comments

Avoid using FILE(GLOB) to specify source code files

Avoid using FILE(GLOB), instead specify the source code files explicitly either by

add_executable(${CMAKE_PROJECT_NAME} main.c)

or

add_executable(${CMAKE_PROJECT_NAME})
target_sources(${CMAKE_PROJECT_NAME} PRIVATE main.c)

Śee also https://stackoverflow.com/questions/32411963/why-is-cmake-file-glob-evil

Use target_* commands

Avoid

using include_directories()
setting CMAKE_C_FLAGS directly

Instead use

target_compile_features()
target_compile_options()
target_include_directories()
target_link_libraries()
target_link_options()

The FindCurses module does not yet support imported targets as of today (2 January 2019, CMake 3.13.2) so it needs to
be used in the old style

target_include_directories(${CMAKE_PROJECT_NAME} PRIVATE ${CURSES_INCLUDE_DIR})
target_link_libraries(${CMAKE_PROJECT_NAME} PRIVATE ${CURSES_LIBRARIES})

In the future (when support has been added to CMake for imported targets in FindCurses) the two lines should be replaced by the line:

target_link_libraries(${CMAKE_PROJECT_NAME} PRIVATE Curses::Curses)

Instead of setting the CMake variable CMAKE_C_STANDARD

set(CMAKE_C_STANDARD 99)

is good practice to use  target_compile_features() instead

 target_compile_features(${CMAKE_PROJECT_NAME} PRIVATE c_std_99)

In this very case it makes no practical difference  but for a
 C++ header-only library, such compile features could be specified in the INTERFACE

 add_library(myheaderonly INTERFACE)
 target_compile_features(headeronlylib INTERFACE cxx_std_11)

to provide usage requirements for consumers of the library (see also https://cmake.org/cmake/help/latest/manual/cmake-buildsystem.7.html)

The target_link_options() line was added to be able to build the executable. (I am not sure it is correct).

Use generator expressions

The generator expression

$<$<C_COMPILER_ID:Clang>:-Weverything -fsanitize=undefined,integer,implicit-conversion,nullability,address,leak,cfi -flto -fvisibility=default>

is expanded to

-Weverything -fsanitize=undefined,integer,implicit-conversion,nullability,address,leak,cfi -flto -fvisibility=default

when the Clang compiler is used, but for other compilers it is expanded to nothing.

Avoid setting CMAKE_C_COMPILER

Instead of setting the CMake variable CMAKE_C_COMPILER, give a WARNING or a FATAL_ERROR whenever a  non-supported C compiler is used.

if(NOT CMAKE_C_COMPILER_ID STREQUAL "Clang")
  message(WARNING "Use the Clang compiler instead. FirstProject officially supports Clang (although other compilers might work).")
endif()

(WARNING could be replaced by FATAL_ERROR to prevent the use of any other C compiler than Clang)

To compile the project, specify the C compiler with the environment variable CC

mkdir /tmp/build
cd /tmp/build
CC=clang cmake -G Ninja ~/FirstProject
ninja -v

Use the ninja command-line flag -v  if you want to see the actual commands being run.


Multi producer/consumer queue (without Boost) in C++11 Not lock-free, but still only C++11 and no Boost. It also supports timeouts.

#pragma once

#include <cstddef>
#include <chrono>
#include <memory>
#include <queue>
#include <mutex>
#include <condition_variable>

template<typename T>
class blocking_queue {
public:
    explicit blocking_queue(std::size_t max_size) :_max_size(max_size), _q()
    {}
    blocking_queue(const blocking_queue&) = delete;
    blocking_queue& operator=(const blocking_queue&) = delete;
    ~blocking_queue() = default;

    std::size_t size()
    {
        std::lock_guard<std::mutex> lock(_mutex);
        return _q.size();
    }
    //Return false if failed to push due to full queue after the timeout have passed
    bool push(const T& item, const std::chrono::milliseconds& timeout)
    {
        std::unique_lock<std::mutex> ul(_mutex);
        if (_q.size() >= _max_size) {
            if (_item_popped_cond.wait_for(ul, timeout) == std::cv_status::timeout || _q.size() >= _max_size)
                return false;
        }
        _q.push(item);
        if (_q.size() <= 1)
            _item_pushed_cond.notify_all();
        return true;
    }
    //Return false if failed to pop due to empty queue after the timeout have passed
    bool pop(T& item, const std::chrono::milliseconds& timeout)
    {
        std::unique_lock<std::mutex> ul(_mutex);
        if (_q.empty()) {
            if (_item_pushed_cond.wait_for(ul, timeout) == std::cv_status::timeout || _q.empty())
                return false;
        }
        item = _q.front();
        _q.pop();
        if (_q.size() >= _max_size - 1)
            _item_popped_cond.notify_all();
        return true;
    }
private:
    std::size_t _max_size;
    std::queue<T> _q;
    std::mutex _mutex;
    std::condition_variable _item_pushed_cond;
    std::condition_variable _item_popped_cond;
};

Your size is not mutating. So should declare it const.
std::size_t size() const
     //            ^^^^^

Still in the size() I don't see any in point in lock on such short term. As soon as the function exits the value can be mutated and will thus be out of date (so if you get a value as another thread is mutating it you will be in no better state).
// There is no point in this function.
// It has no intrinsic value alone but can only be used within a set of commands.
// But since you lock per call, if you make multiple calls another thread can
// mutate the object and make this value stale.
{
    std::lock_guard<std::mutex> lock(_mutex);
    return _q.size();
}

Not sure your timed push will work as expected:
if (_item_popped_cond.wait_for(ul, timeout) == std::cv_status::timeout || _q.size() >= _max_size)

Say you set a timeout of 10 seconds. Now another threads pops at 1 second and signals the condition variable. This can potentially release this thread (this does not guarantee your thread will require the lock before another third thread manages to call push). This means that your size would still be max (when you eventually reacquire the lock) and thus you exit with false after only one second.
You need to use the other version of wait_for() that has a test function.
if (!_item_popped_cond.wait_for(ul, timeout, [&q,&_max_size](){return _q.size() < _max_size;})) {
    return false;
}

Or put it inside a loop:
while(_q.size() >= _max_size) {
    if (_item_popped_cond.wait_for(ul, timeout) == std::cv_status::timeout) {
        return false;
    }
    // probably need to adjust timeout here.
}

Don't see the need to test before notifying after a push. Also why are you notifying all threads, why not just notify one (there is only one new item available).
if (_q.size() <= 1)                 // Why test. Always notify
    _item_pushed_cond.notify_all(); // notify_one();

All the same comments apply to pop.
I don't like the name of your identifier names. Technically nothing wrong with them. But there are so many situations where identifiers with an initial underscore is incorrect that I avoid them. And most C++ code follows the same rules. What are the rules about using an underscore in a C++ identifier?
Note: The std lib with the compiler usually uses underscores but they are specifically allowed to (as they are part of the implementation). This is where most beginners pick up this hobbit; But as you are writing code that is not part of the implementation the same rules do not appy and thus this is not a reason to follow that convention.
Expansion of my point on locking above
The only way to use the lock is:
std::size_t  count = que.size();   // locks getting size.
for(int loop = 0;loop < count; ++loop)
{
    que.pop();                     // locks while popping.
}

I see very little use for the size() method in this context. Between the call to the size() method and other methods your object could have been mutated thus making your results stale. Any function that queries state like this is not returning you anything useful because it can immediately by out of date as soon as the lock is released.
So what should you do?
Where you have operations that can be combined together into a larger piece of work. What you normally do is not provide the interface directly. You provide a way to lock the object that returns an interface that you can use (because while you hold the interface object you hold the lock).
class blocking_queue
{
      friend class blocking_queue_interface;
      // Private interface can not be used directly.
      std::size_t p_size() const;
      bool p_push(T& item, const std::chrono::milliseconds& timeout, std::lock_guard<std::mutex>& lock);
      bool p_pop(T& item, const std::chrono::milliseconds& timeout, std::lock_guard<std::mutex>& lock);

      ......
      public:
          // Public interface
          // Just gets lock and calls private interface.
          // Should only be used as a last resort.
          // Personally I would not provide this interface at all.
          std::size_t size() const
          {
              std::lock_guard<std::mutex> lock(_mutex);
              return p_size();
          }
          bool p_push(T& item, const std::chrono::milliseconds& timeout)
          {
              std::lock_guard<std::mutex> lock(_mutex);
              return p_push(item, timeout, lock);
          }
          bool p_pop(T& item, const std::chrono::milliseconds& timeout)
          {
              std::lock_guard<std::mutex> lock(_mutex);
              return p_pop(item, timeout, lock);
          }
 };

 // A locked interface.
 // You can acquire the lock and make multiple calls without
 // the data becoming stale between calls.
 class blocking_queue_interface
 {
     blocking_queue              queue;
     std::lock_guard<std::mutex> lock;

     blocking_queue_interface(blocking_queue& q): queue(q), lock(queue._mutex) {}
     ~blocking_queue_interface()                                               {}
     // delete remove all copy and assignment operators.

      // Calls the private version of these methods
      // as it has already aquired the lock
      std::size_t size() const                                     {return queue.p_size());
      bool push(T& item, const std::chrono::milliseconds& timeout) {return queue.p_push(item, timeout, lock);}
      bool pop(T& item, const std::chrono::milliseconds& timeout)  {return queue.p_pop(item, timeout, lock);}
 };


Making a copy constructor more flexible for ADT queue I have constructed a copy constructor for an ADT queue. The copy constructor works fine. I would want to improve my code, but I don't really know how to shorten it to make it more flexible.

template <typename T>
Queue <T>::Queue(const Queue & other)
{
    if (other.first == nullptr)
    {
        first = nullptr;
        nrOfElements = 0;
    }

   else
   {
    Node* saveFirst;
    Node* walker;
    first = other.first;
    walker = new Node(first->data);
    saveFirst = walker;
    while (first->next != nullptr)
    {
        walker->next = new Node(first->next->data);
        walker = walker->next;
        first = first->next;
    }
    walker->next = nullptr;
    first = saveFirst;
    }
this->nrOfElements = other.nrOfElements;
}

The class Queue also contains an inner private Node class which contains the pointers first, next, etc:

private:
    int nrOfElements;
    class Node
    {
    public:
        Node* next;
        T data;
        Node(T data)
        {
            this->data = data;
        }
    };
    Node* first;

So, I would appreciate any suggestions/examples of how the copy constructor code above could be improved, as I'm a bit lost on the task.

Remove redundancy

   {
    Node* saveFirst;
    Node* walker;
    first = other.first;
    walker = new Node(first->data);
    saveFirst = walker;
    while (first->next != nullptr)
    {
        walker->next = new Node(first->next->data);
        walker = walker->next;
        first = first->next;
    }
    walker->next = nullptr;
    first = saveFirst;
    }
this->nrOfElements = other.nrOfElements;

You create a temporary variable saveFirst.  You then use a class field as temporary variable.  When done, you set the class field to the value set in the temporary variable.  Why not do it the other way around?

   {
       Node* current = new Node(other.first->data);
       first = current;

       for (Node *otherCurrent = other.first; otherCurrent->next != nullptr; otherCurrent = otherCurrent->next)
       {
           current->next = new Node(otherCurrent->next->data);
           current = current->next;
       }

       current->next = nullptr;
       nrOfElements = other.nrOfElements;
    }

This uses the temporary variable to hold the changing current variable.  It sets first to the correct value immediately and directly.  It changes the loop to say for every node in other.  Now you don't have to set the class field at the end.

Note that I also moved the number of elements assignment inside the else.  In the original it would have tried to dereference a null other.

Reuse other methods

You don't mention other functions in the class, but there are certain ones that could be used here (assuming C++11).

    {
        for (auto i : other)
        {
            this.append(i.data);
        }
    }

This would replace the entire else block.  If you have to create the begin, end, and append functions anyway, then this drops the amount of code that you have to maintain considerably.  Note that begin and end are required to make the range-based for loop work.


Simplifying `if` statements in Tic Tac Toe function I'm currently trying to improve my coding skills, so I'm trying to code a Tic Tac Toe game. At one point a "win check" is needed.I'm using this code to check:

public void CheckWin(States state)
{
    if(fieldA1.GetState == state && fieldA2.GetState == state && fieldA3.GetState == state)
        ReportWin(currentPlayer);
    else if(fieldB1.GetState == state && fieldB2.GetState == state && fieldB3.GetState == state)
        ReportWin(currentPlayer);
    else if (fieldC1.GetState == state && fieldC2.GetState == state && fieldC3.GetState == state)
        ReportWin(currentPlayer);
    else if (fieldA1.GetState == state && fieldB1.GetState == state && fieldC1.GetState == state)
        ReportWin(currentPlayer);
    else if (fieldA2.GetState == state && fieldB2.GetState == state && fieldC2.GetState == state)
        ReportWin(currentPlayer);
    else if (fieldA2.GetState == state && fieldB2.GetState == state && fieldC2.GetState == state)
        ReportWin(currentPlayer);
    else if (fieldA3.GetState == state && fieldB3.GetState == state && fieldC3.GetState == state)
        ReportWin(currentPlayer);
    else if (fieldA1.GetState == state && fieldB2.GetState == state && fieldC3.GetState == state)
        ReportWin(currentPlayer);
    else if (fieldA3.GetState == state && fieldB2.GetState == state && fieldC1.GetState == state)
        ReportWin(currentPlayer);
    else
        PlayerSwitch();
}

This code looks very unclean to me.  Is there a better way to handle these if instructions?

There should only be eight ways to win; you have nine ReportWin()s.  This one got listed twice:

fieldA2.GetState == state && fieldB2.GetState == state && fieldC2.GetState == state

Considering that you've named your squares as independent variables, there's not much you can do to generalize CheckWin().  Start by turning the board into an  array or two-dimensional array, then read the advice in Tic-Tac-Toe design .  Personally, I like the suggestion to list all eight winning configurations, as it results in the least code.


Reading arbitrarily long lines from a file that is redirected as input So I have a file with a list of dates that are to be redirected as input to stdin and what the program does is it searches for valid dates, appends them to the output file and then appends all the dates in the input file to the output file. In other words, at the end of the program there would be a list of all the valid dates in the output file and list of all the dates that were in the input file. An important command line argument is the number of entries to search for. 0 here is the default case where we look through the entire input file. Any value greater than 0 represent the number of entries we have to look at. If we find one that is invalid, we skip it.

Command Example: ./program < input.txt 0 > output.txt

Here are all the relevant functions.

getDates Function

char ** getDates(char **lines, size_t *numOfLines, size_t *lineMax, int numOfEntries)
{
    char *strDate = NULL;
    int allEntries = FALSE, errorStatus = FALSE; // errorStatus is so I can report memory failures.

    if(numOfEntries == 0)
    {
        allEntries = TRUE; // A flag for the default case zero.
    }

    while((strDate = readLine(stdin, &errorStatus)) != NULL)
    {
        if(numOfEntries >= 0 || allEntries)
        {
            if(checkDate(strDate))
            {
                fputs(strDate, stdout);
            }

            numOfEntries--;
        }

        /*
       Keep a record of how many lines we have process.
       We will need this variable when we decide to print
       everything that we have in the variable lines.
        */
        lines[*numOfLines] = strDate;
        (*numOfLines)++;

        if((*numOfLines) == (*lineMax))
        {
            errorStatus = expand(lines, lineMax); // try to expand.

            if(errorStatus == MEMFAILURE)
            {
                fprintf (stderr,"Error, couldn't allocate any more memory.");
                return NULL;
            }
        }
    }

    if(errorStatus == MEMFAILURE)
    {
        fprintf (stderr,"Error, couldn't allocate any more memory.");
        return NULL;
    }

    if(ferror(stdin))
    {
        fprintf(stderr,"Error, couldn't completely process the entire file.\n");
        return NULL;
    }

    if(strDate != NULL)
    {
        free(strDate);
    }

    return lines;
}

readLine Function

char *readLine(FILE *stream, int *errorStatus)
{
    size_t size = 0;
    char *buffer = NULL;
    char *newLineFound = NULL;

    do
    {
        char *temp = realloc(buffer, size + BUFSIZ); // BUFSIZE is a define constant in the stdio library.

        if(!temp)
        {
            free(buffer);
            *errorStatus = MEMFAILURE; // an enum constant to record our error.
            return NULL;
        }

        buffer = temp;

        // One small problem. If there is no new line at the end of the file. it won't add whatever contents is there.
        if(!fgets(buffer + size, BUFSIZ, stream))
        {
            free(buffer);
            return NULL;
        }

        newLineFound = strchr(buffer + size, '\n');

    }
    while(!newLineFound && (size += BUFSIZ - 1)); // this is to simply update size at the same time that we don't find a newline.

    return buffer;
}

expand Function

int expand(char **lines, size_t *lineMax)
{
    char **tmp = realloc (lines, (*lineMax) * 2 * sizeof(*lines));

    if(!tmp)
    {
        return MEMFAILURE;
    }

    lines = tmp;
    (*lineMax) *= 2; // Update the size

    return SUCCESS;
}

What can I do to reduce my code as I feel that it is getting too bloated.
What did I forget to do, what didn't I do right and what did I do that was unnecessary.
Performance tips are welcome.

What can I do to reduce my code as I feel that it is getting too bloated.

No need for test.  Just call free();.  It handles free(NULL);

// if(strDate != NULL) {
//  free(strDate);
// }
free(strDate);

Minor: Style: I found the vertical spacing excessive creating challenges seeing the overall flow.  YMMV

What did I forget to do, what didn't I do right and what did I do that was unnecessary.

Certain functional error, lines is assigned a value that is not used.  Need to pass a "reference".

int expand(char **lines, size_t *lineMax) {
  char **tmp = realloc (...);
  ...
  lines = tmp;
  ...
  return SUCCESS;
}

Functional error (possible part of your "One small problem." comment).  If fgets() returns NULL, code should only dispose of the buffer if this is the first fgets() in readLine().  Or: if the reason for NULL is due to ferror() and not feof().
Functional corner case: strchr(buffer searches the buffer and can get fooled by fgets() reading a null charter and then a end-of-line such as "\0\n".  No true way to solve this and still use fgets().

  newLineFound = strchr(buffer + size, '\n');
}
while(!newLineFound && (size += BUFSIZ - 1));

Minor: Format to the presentation width.

// avoid
int allEntries = FALSE, errorStatus = FALSE; // errorStatus is so I can report memory failures.

// better
// errorStatus is so I can report memory failures.
int allEntries = FALSE, errorStatus = FALSE;

Use bool.  It is standard and well defined.

#include <stdbool.h>
// int allEntries = FALSE, errorStatus = FALSE;
bool allEntries = false, errorStatus = false;

Not all stderr are unbuffered. Suggest appending '\n' to your print to insure timely display.  Note either call below may make the same code with smart compilers.  2nd has the advantage of no problems with '%'

// fprintf (stderr,"Error, couldn't allocate any more memory.");
fprintf(stderr,"Error, couldn't allocate any more memory.\n");
// or
fputs("Error, couldn't allocate any more memory.\n", stderr);

Confusing code: simplification suggestion

// char **tmp = realloc (lines, (*lineMax) * 2 * sizeof(*lines));
size_t newsize = *lineMax * 2;
void *tmp = realloc (lines, sizeof(*lines) * newsize);
....
*lineMax = newsize;

Lost memory

char **tmp = realloc (lines, (*lineMax) * 2 * sizeof(*lines));

if(!tmp) {

  // Add
  free(lines);

  return MEMFAILURE;
}

Functional corner case:  If *lineMax could ever be 0.  Returning NULL is OK when the size of 0 and not a MEMFAILURE.

char **tmp = realloc (lines, (*lineMax) * 2 * sizeof(*lines));

// if(!tmp) {
if(!tmp && *lineMax) {
      return MEMFAILURE;
}

Functional: To be more resilient, check size before using the array.  getDates() does not control input of the count/size.

// move here
if((*numOfLines) >= (*lineMax)) ...

lines[*numOfLines] = strDate;
(*numOfLines)++;

// if((*numOfLines) == (*lineMax)) ...

Performance tips are welcome.

I/O is often a candidate for speed improvements.  For this code, if there is no other calls to stdin, I would use fread() and then process the lines  myself, reading large blocks.
On systems that use '\n' translation, improvement can be had by reading in binary mode.
When a file ends without a final '\n', code is probably faster  appending one, insuring the rest of code does not have to check for that expectation.

GTG


Calculate Twitter Follower Changes Over Time I have a code that grabs quickly the twitter followers for 12 different users. After appending to a Pandas DataFrame, the data is compared to the same file pulled the day before.

It allows me to see which users have gained, lost, and returned followers.

The code works fine. However, the for loop to compare changes between days is slow. Any ideas how to help me out with the for loop section?

TRANSFORMATION FUNCTIONS

# Transformation Functions

# Gained Users

def gained_users(account, new, old):

    gained_followers = []

    old_user_ids = old.Follower_ID[old.Handles == account].unique().tolist()
    new_user_ids = new.Follower_ID[new.Handles == account].unique().tolist()

    gained = list(set(new_user_ids) - set(old_user_ids))

    gained_followers.extend(map(str, gained))

    return gained_followers

# Lost Users

def lost_users(account, new, old):

    old_user_ids = old.Follower_ID[old.Handles == account].unique().tolist()
    new_user_ids = new.Follower_ID[new.Handles == account].unique().tolist()

    lost = list(set(old_user_ids) - set(new_user_ids))

    return lost

# Returned Users

def returned_users(account, new, old):

    returned_followers = []

    new_user_ids = new.Follower_ID[new.Handles == account].unique().tolist()
    returned_user_ids = old.Follower_ID[(old.Handles == account) & (old.End_Date.notnull() == True)].unique().tolist()

    returned =  list(set(returned_user_ids).intersection(new_user_ids))

    returned_followers.extend(map(str, returned))

    return returned_followers

FOR LOOP SECTION

# Add Returned Users

for username in lookup_users:

    returned_ids = returned_users(username, new_followers_df, historical_followers_df)

    if returned_ids:

        historical_followers_df.loc[(historical_followers_df["Handles"] == username) & (historical_followers_df["Follower_ID"] == ids), "Start_Date"] = today
        historical_followers_df.loc[(historical_followers_df["Handles"] == username) & (historical_followers_df["Follower_ID"] == ids), "Returned_After_Days"] = pd.to_datetime(historical_followers_df.Start_Date) - pd.to_datetime(historical_followers_df.End_Date)
        historical_followers_df.loc[(historical_followers_df["Handles"] == username) & (historical_followers_df["Follower_ID"] == ids), "End_Date"] = np.NaN

# Add Lost Users

for username in lookup_users:

    lost_ids = lost_users(username, new_followers_df, historical_followers_df)

    if lost_ids:

        for ids in lost_ids:

            historical_followers_df.loc[(historical_followers_df["Handles"] == username) & (historical_followers_df["Follower_ID"] == ids), "End_Date"] = today

# Add Gained Users

for username in lookup_users:

    new_ids = gained_users(username, new_followers_df, historical_followers_df)

    if new_ids:

            gained_users_list = pd.DataFrame({
            "Handles": username,
            "Follower_ID": new_ids,
            "Start_Date": today})

            historical_followers_df = gained_users_list.append(historical_followers_df, ignore_index=True)

EDIT:

Hi @Graipher thanks for your help. The reasoning makes sense and the structure is much neater! Hopefully if you don't mind, can you please answer these three questions:

1. Could you explain the old_account = old.Handles == account passage, as I have never seen it before!

2. If I run the code as it is it says that old_followers = user_followers & (historical_followers_df["Follower_ID"] == ids) is this because ids in the filter is not specified?

If so, Is it correct to say that:

a. I have to amend the categorize_users function above. Adding:  old = map(str, old_user_ids)  and   return old, gained, lost, returned
b. In the for loop where I call a function I add a new variable ids so that it looks like: ids, new_ids, lost_ids, returned_ids = categorize_users(username, new_followers_df, historical_followers_df)

3. Finally, I think that there's a problem with new_ids as it says that TypeError: object of type 'map' has no len()

a. Is this a code problem? I can't really figure that part out.

You should try to avoid calculating the same things over and over. The first example are your transformation functions. While it is nice that you have separated the concerns, it probably costs you quite a bit here. It might be better to make this one function:

def categorize_users(new, old):
    """
    Return the gained, lost and returned followers
    from the two `pandas.DataFrame`s `old` and `new`.
    """
    old_user_ids = set(old.Follower_ID)
    new_user_ids = set(new.Follower_ID)
    returned_user_ids = set(old.Follower_ID[old.End_Date.notnull()])

    gained = map(str, new_user_ids - old_user_ids)
    lost = map(str, old_user_ids - new_user_ids)
    returned = map(str, returned_user_ids.intersection(new_user_ids))
    return gained, lost, returned

Note that I re-used everything that was used more than once. I also skipped the unique part, because the set already does it, but you might want to add it back in to see if it is any faster.

Now, to your actual for loop:

for username in lookup_users:
    new_user_followers = new_followers_df["Handles"] == username
    old_user_followers = historical_followers_df["Handles"] == username
    new_ids, lost_ids, returned_ids = categorize_users(
        username,
        new_followers_df[new_user_followers],
        historical_followers_df[old_user_followers])

    # Add Returned Users
    if returned_ids:
        for ids in returned_ids:
            old_followers = user_followers & (historical_followers_df["Follower_ID"] == ids)
            historical_followers_df.loc[old_followers, "Start_Date"] = today
            historical_followers_df.loc[old_followers,"Returned_After_Days"] = pd.to_datetime(historical_followers_df.Start_Date) - pd.to_datetime(historical_followers_df.End_Date)
            historical_followers_df.loc[old_followers, "End_Date"] = np.NaN
    # Add Lost Users
    if lost_ids:
        for ids in lost_ids:
            old_followers = user_followers & (historical_followers_df["Follower_ID"] == ids)
            historical_followers_df.loc[old_followers, "End_Date"] = today
    # Add New Users
    if new_ids:
        gained_users_list = pd.DataFrame({
            "Handles": username,
            "Follower_ID": new_ids,
            "Start_Date": today})
        historical_followers_df = gained_users_list.append(
            historical_followers_df, ignore_index=True)

I made your three for loops into one, because now we can actually get all the three different classes in one go. I also put the selection of the relevant old followers which need to be modified into its own variable as well, so it is not computed four times per loop.

I also moved the filtering of the dataframes to one user already in the loop, so that categorize_users becomes more general.


Interactive Rock Paper Scissors I am a beginner in programming and this is the third program I've written.  This is an interactive game of rock, paper, scissors, capable of being modified to any number of turns.  I would love to see how a more experienced programmer could simplify this and make it smaller.  Please let me know how I could change this program to be easier to understand for other programmers or any general layout/design rules I should keep in mind when writing future programs.  I just have the slightest feeling that this code could be more compact.

args = ['rock', 'paper', 'scissors']
user_score = 0
comp_score = 0
turn = 3
print 'ROCK, PAPER, SCISSORS!'
print 'Best out of %d' % (turn)
while turn > 0:
    user = raw_input('Choose ' + ", ".join(args) + ' > ')
    comp = random.choice(args)
    proof = 'Computer: %s' % (comp)
    if user == 'q':
        break
    elif (user == args[0] and comp == args[2]) or (user == args[1]\
          and comp == args[0]) or (user == args[2] and comp == args[1]):
        print proof
        print 'Win \n'
        turn -= 1
        user_score += 1
    elif user == comp:
        print proof
        print 'Tie \n'
        turn -= 1
    elif user not in args:
        print 'INVALID INPUT \n'
    else:
        print proof
        print 'Lose \n'
        turn -= 1
        comp_score += 1
print '\n User scored: %d \n Computer scored: %d \n' % (user_score,comp_score)
if user_score > comp_score:
    print 'Winner!'
elif user_score < comp_score:
    print 'Loser'
else:
    print 'Draw'

This is pretty good, but there are a few things that needed to be improved. Here are a few of those things, specifically regarding your, odd, design.

Using % for string formatting is deprecated. To format strings, you should use str.format. For example, you'd do this: print 'Best out of {0}'.format(turn).
Your naming is, strange. For example, I'd rename args to possible_moves. Good variable names should reflect the purpose of the variable.
The way you've designed this is odd. I'd reccomend an object-oriented-design. with a Game class with certain methods and attributes to manage a game. A few things that I'd reccomend implementing would be:

A total_turns attribute which would allow there to be a custom amount of turns.
A Game.pick_move to choose a random choice.

That's about all I can think of for improvement. If there's anything that you want me to comment on, just tell me about it. I hope this helps!


Efficient URL Escape (Percent-Encoding) Here I have a simple algorithm to percent-encode any string.

(Specification from Wikipedia; note that this is not compatible with URLEncoder.encode())

Is this an efficient solution to the problem?

Using a StringBuilder should be efficient, but it doesn't seem great since every character is added to the StringBuilder individually.  Could this have any significant impact?

This method works only for ASCII characters; that is sufficient for my use case.

private static String urlEscape(String toEscape){
    //if null, keep null (no gain or loss of safety)
    if (toEscape==null)
        return null;

    StringBuilder sb=new StringBuilder();
    for (char character: toEscape.toCharArray())//for every character in the string
        switch (character){//if the character needs to be escaped, add its escaped value to the StringBuilder
        case '!': sb.append("%21"); continue;
        case '#': sb.append("%23"); continue;
        case '$': sb.append("%24"); continue;
        case '&': sb.append("%26"); continue;
        case '\'': sb.append("%27"); continue;
        case '(': sb.append("%28"); continue;
        case ')': sb.append("%29"); continue;
        case '*': sb.append("%2A"); continue;
        case '+': sb.append("%2B"); continue;
        case ',': sb.append("%2C"); continue;
        case '/': sb.append("%2F"); continue;
        case ':': sb.append("%3A"); continue;
        case ';': sb.append("%3B"); continue;
        case '=': sb.append("%3D"); continue;
        case '?': sb.append("%3F"); continue;
        case '@': sb.append("%40"); continue;
        case '[': sb.append("%5B"); continue;
        case ']': sb.append("%5D"); continue;
        case ' ': sb.append("%20"); continue;
        case '"': sb.append("%22"); continue;
        case '%': sb.append("%25"); continue;
        case '-': sb.append("%2D"); continue;
        case '.': sb.append("%2E"); continue;
        case '<': sb.append("%3C"); continue;
        case '>': sb.append("%3E"); continue;
        case '\\': sb.append("%5C"); continue;
        case '^': sb.append("%5E"); continue;
        case '_': sb.append("%5F"); continue;
        case '`': sb.append("%60"); continue;
        case '{': sb.append("%7B"); continue;
        case '|': sb.append("%7C"); continue;
        case '}': sb.append("%7D"); continue;
        case '~': sb.append("%7E"); continue;
        default: sb.append(character);//if it does not need to be escaped, add the character itself to the StringBuilder
        }
    return sb.toString();//build the string, and return
}

You are in dire need of a lookup table. What you want to do is define a mapping somewhere else, and then just percent-encode all characters you have mapped. This gets rid of that unwieldy, huge and btw. hacky switch-statement.

consider the following:

StringBuilder sb = new StringBuilder(toEncode.length());
for (Character c : toEncode.toCharArray()) {
    if (MAPPING.containsKey(c)) {
       sb.append(MAPPING.get(c));
    } else {
       sb.append(c);
    }
}

This should be equally fast, if not faster. Also it makes use of a MAPPING you can change, without significantly affecting how this method works. btw. I am prespecifying the StringBuilder's capacity, since it's more efficient when the backing collection in there does not need to be resized too often.

Oh and another thing. Your switch-case only works by sheer luck. continue; is not the correct keyword to use there, instead you should rely on break; that allows you to add more work in the loop body, which is currently more or less impossible.


Simple Queue Implementation Purpose

As I've never implemented a queue, I decided to create a very simple implementation using a linked list approach.

Discussion

I create a singly linked list, using a private Node class that references some data and a previous Node,
that will be used in my SimpleQueue class.

When enqueuing data, I check to see if the linked list is empty or not (by checking to see if the head of the list is null).
If it's empty, point the head to a newly-created Node with the input data. If the list is not empty, I iterate through
the list until I reach the end (i.e. the previous Node is null) and then I insert a newly-created Node.

When dequeuing, I throw an IllegalStateException if the queue is empty. If the queue is not empty, I set the new head of
the queue to be the old head's previous value, and return the old head's data value.

Implementation

public class SimpleQueue<T> {
    private Node<T> head;

    private class Node<E> {
        private final E data;
        private Node<E> previous;

        public Node(E data, Node<E> previous) {
            this.data = data;
            this.previous = previous;
        }
    }

    public boolean isEmpty() {
        return this.head == null;
    }

    public void enqueue(T data) {
        Node<T> currentNode = this.head;
        Node<T> nodeToInsert = new Node<>(data, null);

        if (this.isEmpty()) {
            this.head = nodeToInsert;
        } else {
            while (currentNode.previous != null) {
                currentNode = currentNode.previous;
            }

            currentNode.previous = nodeToInsert;
        }
    }

    public T dequeue() {
        if (this.isEmpty()) {
            throw new IllegalStateException("Unable to dequeue from empty queue");
        }

        Node<T> currentNode = this.head;
        this.head = currentNode.previous;

        return currentNode.data;
    }

    public T peek() {
        if (this.isEmpty()) {
            throw new IllegalStateException("Unable to peek empty queue");
        }

        return this.head.data;
    }
}

while (currentNode.previous != null) {
                currentNode = currentNode.previous;
            }

            currentNode.previous = nodeToInsert;

You can avoid this iteration if you track the location of the tail element.

            tail.previous = nodeToInsert;
            tail = tail.previous;

Of course, you have to maintain the tail in other places as well.

    private Node<T> tail;

    public void enqueue(T data) {
        Node<T> nodeToInsert = new Node<>(data, null);

        if (isEmpty()) {
            head = nodeToInsert;
        } else {
            tail.previous = nodeToInsert;
        }

        tail = nodeToInsert;
    }

But you no longer have to iterate this way.

Since both branches update tail, I moved that out of the if/else.

I notice that you use this. to specify object fields.  That actually isn't necessary unless there is a conflict with a parameter or local variable.  Of course, if you simply prefer it that way to make it obvious which variables are object fields, you can.

You can also update the tail when you dequeue to an empty queue, but you don't need to do so for functionality.  It does allow the garbage collector to collect the node though.


jQuery Plugin options The user can provide a JSON object to configure the plugin when they initialise it. I want those options to be available whenever a plugin action is performed.

I have read the plugin documentation but I'd like to get confirmation that I am going about this the right way.

Plugin is initialised with:

$(selector).hide_and_peek("init", {options:here});

And individual methods may be called with:

$(selector).hide_and_peek("action", {options:here});

Here's the plugin definition:

(function( $ ) {

// Plugin definition.
$.fn.hide_and_peek = function(action,  options ) {

    //define plugin defaults.
    $.fn.hide_and_peek.defaults = {
        peek_offset: -10,
        peek_position: "bottom",
        default_shown: true,
        peek_show_duration:1000,
        peek_hide_duration:1000,
        center: true
    };

    //check to see if options are provided and extend if required.
    $.fn.hide_and_peek.custom = function(){
        if(typeof options === "undefined"){
            return $.fn.hide_and_peek.custom;
        }else{
            return $.fn.extend($.fn.hide_and_peek.custom,options);
        }
    }();

    //merge defaults and custom options
    var oOptions = $.fn.extend({}, $.fn.hide_and_peek.defaults, $.fn.hide_and_peek.custom);

    // *snip* Plugin implementation
})( jQuery );

I am writing (my first) jQuery plugin.

Congratulations!

  The user can provide a JSON object to configure the plugin when they initialise it.

Terminology is wrong here. JSON is the transport format that looks like a JS object, but serialized into a string. What you use is just a JS object. They look the same because JSON was modeled after the JS literal notation, which (historically) was easily "unserialized" (turned back into an object) with eval. Proper parsers came in later on.

//check to see if options are provided and extend if required.
$.fn.hide_and_peek.custom = function(){
    if(typeof options === "undefined"){
        return $.fn.hide_and_peek.custom;
    }else{
        return $.fn.extend($.fn.hide_and_peek.custom,options);
    }
}();

//merge defaults and custom options
var oOptions = $.fn.extend({}, $.fn.hide_and_peek.defaults, $.fn.hide_and_peek.custom);

So I assume custom is another level of options, maybe set during setup and globally for all instances of the plugin. You don't need to check if it's undefined. You can just merge it in. jQuery will take care of it being undefined and will just skip over it.

// Last will take precedence
var oOptions = $.fn.extend({}, defaults, custom, options);


VBA Text Array - Scan two columns rather than one I have some code which is designed to scan columns F and G for occurrences of words found in an array, the array containing text found in column J.

Column J contains free text from a field in SAP. Being free text, it could be "Kerry John Pub Expenses" or "CATS O/H Kerry John", or even "CATS John Kerry O/H". There is no data entry standard for this field... and this is what makes this task difficult.

Example

Here we have 4 rows of data, "John Citizen" is located in row 3, therefore the blank cells in columns F and G, row 2 can be populated with his first and last name.

I would like some suggestions as to how this code could be re-written to achieve this result more efficiently

    Sub arraycolumnmatch()
    Dim txtArray As Variant, T As Variant
    Dim I As Long, J As Long

    For I = 2 To Range("E50000").End(xlUp).row
        typ = Range("F" & I).Value
    If typ = "" Then
        txt = Range("J" & I).Value
        txtArray = Split(txt, " ")

        For Each T In txtArray
            For J = 2 To Range("G50000").End(xlUp).row
                If Range("G" & J).Value = T Then
                    match_txt = T
                    Range("G" & I).Value = match_txt
                    Exit For
                End If
            Next J
        Next T

        For Each T In txtArray
            For J = 2 To Range("F50000").End(xlUp).row
                If Range("F" & J).Value = T Then
                       match_txt = T

                 'Check for surnames which could match first names such as Mary Michael
                       If Not Range("G" & I).Value = T Then

                          Range("F" & I).Value = match_txt
                          Exit For
                       End If
                End If
            Next J
        Next T
   End If
Next I
End Sub

Consistent Indentation.
The first thing I would address, is the indentation. Improving code that isn't properly formatted is just... not right.
Here are the signs:
Sub arraycolumnmatch()
Dim txtArray As Variant, T As Variant
Dim I As Long, J As Long

And:
Next I
End Sub

But also:
 'Check for surnames which could match first names such as Mary Michael
       If Not Range("G" & I).Value = T Then

The key is consistency - here's your code, with consistent indentation (comments omitted). Notice each code block (If...End If, For...Next, etc.) consistently adds an indentation level, and the end of the block lines up with its beginning - also notice that indentation levels are consistently 4 spaces wide; that's the default VBE setting for the Tab key:
Sub arraycolumnmatch()
|
|   Dim txtArray As Variant, T As Variant
|   Dim I As Long, J As Long
|
|   For I = 2 To Range("E50000").End(xlUp).row
|   |   typ = Range("F" & I).Value
|   |   If typ = "" Then
|   |   |   txt = Range("J" & I).Value
|   |   |   txtArray = Split(txt, " ")
|   |   |
|   |   |   For Each T In txtArray
|   |   |   |   For J = 2 To Range("G50000").End(xlUp).row
|   |   |   |   |   If Range("G" & J).Value = T Then
|   |   |   |   |   |   match_txt = T
|   |   |   |   |   |   Range("G" & I).Value = match_txt
|   |   |   |   |   |   Exit For
|   |   |   |   |   End If
|   |   |   |   Next J
|   |   |   Next T
|   |   |
|   |   |   For Each T In txtArray
|   |   |   |   For J = 2 To Range("F50000").End(xlUp).row
|   |   |   |   |   If Range("F" & J).Value = T Then
|   |   |   |   |   |   match_txt = T
|   |   |   |   |   |   If Not Range("G" & I).Value = T Then
|   |   |   |   |   |   |   Range("F" & I).Value = match_txt
|   |   |   |   |   |   |   Exit For
|   |   |   |   |   |   End If
|   |   |   |   |   End If
|   |   |   |   Next J
|   |   |   Next T
|   |   End If
|   Next I
|
End Sub

Much easier to follow, isn't it?

Meaningful Names.
Next thing I would address is the naming. Improving code where you have to quadruple-check every change you make because you're not sure you're changing the right thing is just not efficient. Using meaningful names for all identifiers fixes that.
Starting with the method's name itself:
Sub arraycolumnmatch()

Typically, VBA (VB in general) method names are PascalCase, for better readability. That would make it ArrayColumnMatch - but good method/procedure names should also start with a verb. What's this method doing exactly? should be answerable by simply looking at it's name.
The problem is, the method in question is doing too many things, so giving it a meaningful name is hard. But I'll get back to that.
Avoid chopping off identifers, like typ when you meant type - but then Type is a reserved keyword.. the solution isn't to make it less readable! I'm not sure which worksheet Range is referring to because Range is an implicit reference to Application.ActiveSheet (and that's very bug-prone!), so I'll just assume you meant to call it documentType.
txt is another meaningless name (heck, people use that as a prefix for TextBox controls!): it stands for that manual entry field you're trying to parse, right? How about targetFieldValue, or manualTextField?
Also avoid single-letter identifiers, like T - assuming txt is manualTextField, I'd go with manualTextFieldValue.
Local variables are typically named in camelCase, so I and J would be i and j - those are typically used for iterating loops as you're doing, so I'm not going to complain about those, except both loops seem to be iterating the very same rows on the active sheet (whatever that is), and they're nested, too... and since they're row numbers I'd typically name them xlRow[WhatItsFor].

Undeclared Identifiers
Always, systematically, consistently, automatically, thoughtlessly stick Option Explicit at the top of every VBA module. Don't question it, just do it: without it, you can have a bug by simply having a little typo in an identifier name. With it, your VBA code will refuse to compile and run if an identifier isn't declared anywhere. Use it. Always.
If match_txt is declared outside the scope of the procedure, then move the declaration inside that scope - it doesn't need to be at module scope. Always declare variables at the smallest possible scope; globals are evil.

vbNullString
A very minor point, but I have to mention it.
If typ = "" Then

Avoid using an empty string "" to mean no value. VBA defines the constant vbNullString specifically for that. And why should you bother typing vbNullString when "" does the same thing?
That's the thing. It doesn't do the same thing. "" is a full-fledged String value that needs its own allocated memory space. vbNullString is a null string pointer that's not allocated anywhere. Of course it's a very minor performance hit to allocate that string, but semantically, vbNullString is the right thing to do.
Don't believe me? Try this in the immediate pane (Ctrl+G):
?"Address for empty string: " & StrPtr("") & vbNewLine & "Address for vbNullString: " & StrPtr(vbNullString)

I get this output:
Address for empty string: 241665720
Address for vbNullString: 0

There's some kind of optimization going on under the hood though, so the pointer for "" will be reused every time it's encountered in your application's lifetime - as I said it's a very minor point. But just as you wouldn't declare an unused variable, you shouldn't allocate a memory address for a built-in constant value.

Twin Loops
You have this:
For Each T In txtArray

And then a few lines further, you have that:
For Each T In txtArray

You're iterating all elements of txtArray twice, when once would suffice. Also, wouldn't the last row of column "G" be the same last row as in column "F"? Wouldn't that last row be the same for every iteration of T? I'm asking, because your data seems to be laid out in a table with records, so you'd only need to find the row for the last record, whichever column you're using for that is irrelevant, no?
For J = 2 To Range("G50000").End(xlUp).row
...
For J = 2 To Range("F50000").End(xlUp).row

I'd extract a variable here, and assign it to Range("G50000").End(xlUp).row before the For Each T In txtArray loop starts, and then do
For J = 2 To xlLastRow

Then you're writing to G[i] if the value of G[j] matches the current txtArray element, and to F[i] if G[i] wasn't written to and the value of F[j] matches the current txtArray element.
Wouldn't that do the same thing then?
If Range("G" & j).Value = T Then
    Range("G" & i).Value = T
Else If Range("F" & j).Value = T Then
    Range("F" & i).Value = T
End If

Lastly, the J loop looks like it could be implemented as a Find instead of a loop.

Extract Methods.

The problem is, the method in question is doing too many things, so giving it a meaningful name is hard. But I'll get back to that.

So here we are. What's that method doing actually?
It...

finds the row number for the last record on the active sheet
iterates all records on the active sheet
splits a value in the current record into an array of values
iterates all values in that array
looks for a match of that value somewhere in columns G and F
writes that value to column G or F, whichever comes first

Extracting methods is a refactoring operation, and doing that manually in VBA is, honestly, a pain in the neck.
I've authored a refactoring tool that does exactly that (and more):

Of course I have a little bit of a vested interest (very little - it's free and open-source actually), but I'd recommend you take a look at what the Rubberduck VBE add-in can do for you.


Convert a binary string, with each char separated by spaces, into a normal C string and print This code is a purely for-fun project and it comes from this challenge at freeCodeCamp. This is simply an algorithm challenge which is usually meant for JavaScript, but I decided to perform in C. It will not be used for any production reasons. Please leave any feedback you see fit, with special attention to the loops and data types. I purposely used pointer arithmetic, while, and for loops to explore different approaches to the problems. I also left the printf about number of tokens in there if you'd like to play with it by uncommenting. Also note that I've provided 2 sample binary strings but only one is currently used by default.

#include <stdio.h>
#include <stdbool.h>
#include <string.h>
#include <stdlib.h>
#include <assert.h>

char* binary_agent(char *bin_str);
char bit_string_decode(char *const str);
bool bit_string_is_valid(char *bit_string);
int main(void)
{
    char str[] = "01000001 01110010 01100101 01101110 00100111 01110100 00100000 "
                 "01100010 01101111 01101110 01100110 01101001 01110010 01100101 "
                 "01110011 00100000 01100110 01110101 01101110 00100001 00111111";
    char str2[] = "01001001 00100000 01101100 01101111 01110110 01100101 00100000 01000110 01110010 01100101 01100101 01000011 01101111 "
                  "01100100 01100101 01000011 01100001 01101101 01110000 00100001";
    char *result = binary_agent(str);
    printf("%s\n", result);
    free(result);
    result = NULL;
    return 0;
}

// Purpose: Return an English translated sentence of the passed binary string
// "Chars" must be space-separated
// E.G. binary_agent("01000001 01110010 01100101");
char* binary_agent(char *bin_str)
{
    assert(bin_str);

    size_t count;
    char *token = NULL;
    char *delim = " ";
    count = 0;
    char *ptr = bin_str;

    while((ptr = strchr(ptr, ' ')) != NULL)
    {
        count++;
        ptr++;
    }

    ptr = NULL;
    //printf("Total number of tokens %lu\n", count);

    char *output_buffer = malloc(sizeof(char)*(count+1));
    assert(output_buffer);

    token = strtok(bin_str,delim); // token is null-terminated
    if(!token) return NULL;
    output_buffer[0] = bit_string_decode(token);
    size_t i;
    for(i = 1; token != NULL; ++i) //convert to for loop
    {
        token = strtok(NULL, delim);
        if(!token) break;
        output_buffer[i] = bit_string_decode(token);
    }
    return output_buffer;
}

//Parses groups of 8 1s or 0s into a char
char bit_string_decode(char *const str)
{
    assert(str && bit_string_is_valid(str));

    char output = 0;
    size_t i;

    //String needs to be valid for sure here
    for(i = 0; i < 8; ++i)
    {
        switch(str[i])
        {
            case '1':
                output |= 1 << 7-i;
                break;
            case '0':
                break;
        }
    }

    return output;
}
bool bit_string_is_valid(char *bit_string)
{
    assert(bit_string && (strlen(bit_string) == 8));

    while(bit_string && *bit_string != '\0')
    {
        if((*bit_string != '0') && (*bit_string != '1')) return false;
        bit_string++;
    }
    return true;
}

OP: Please leave any feedback you see fit, with special attention to the loops and data types.

char type

Although char is signed or unsigned, string functions of the standard library work with the data as if it is was unsigned char.

  ... each character shall be interpreted as if it had the type unsigned char ... C17dr §7.24.1 3

Also: char output = 0; ... output |= 1 << 7-i; is or-ing bits potentially into the sign place.  This really is not a concern, but in general, logical bit manipulation is best done with unsigned types.

int/unsigned, in general, is the preferred type to use for efficient/clean code.  Thus using unsigned/int rather than char output can make for better code.

Putting these ideas together: recommend something more like

char bit_string_decode(char *const str) {
    unsigned output = 0;
    size_t i;
    for(i = 0; i < 8; ++i) {
        switch(str[i]) {
            case '1':
                output |= 1u << 7-i;  // I prefer unsigned types for bit manipulations
                break;
            case '0':
                break;
        }
    }
    return (char) output;
}

const

bit_string_decode(char *const str) does not modify the reference data. Use const for greater functional usage, more function clarity and help weak compilers optimize.

// bit_string_decode(char *const str)
bit_string_decode(const char *const str)
//                ^---^

// bool bit_string_is_valid(char *bit_string)
bool bit_string_is_valid(const char *bit_string)

Simplifications

Some alternative ideas to simplify the loop:

char bit_string_decode(const char *str) {
    const unsigned char *ustr = (const unsigned char *) str;
    unsigned output = 0;
    while (*(const unsigned char*)ustr) { // cast: see note below
        output <<= 1;
        // Given *str is '0' or '1'
        output |= *ustr++ - '0';
    }
    return (char) output;
}

Allocate to the size of the referenced data, not type

Code is easier to code right, review and maintain.

// char *output_buffer = malloc(sizeof(char)*(count+1));
char *output_buffer = malloc(sizeof *output_buffer *(count+1));
//                                  ^------------^  Referenced data

Consider decoding without destruction

binary_agent(char *bin_str) messes up bin_str due to strtok(bin_str,delim).  Use of strspn(), strcspn(), strchr() could be used to find the delimiter without changing bin_str.  This functional change I find would make for more useful code and greater applicability.

Missing validation in production

I would say strlen(bit_string) == 8 (or equivalent) should always test, not in only in assert.  Perhaps as

bool bit_string_is_valid(const char *bit_string) {
  if (bit_string == NULL) {
    return false;
  }
  const char *s = bit_string;
  while(* (const unsigned char*)s) {  // cast: see note below
    if((*s != '0') && (*s != '1')) {
      return false;
    }
    s++;
  }
  return (s - bit_string) == 8;
}

The cast (const unsigned char*) is only useful here for the all but extinct non-2's complement platforms to properly ID the null character.  Drop it, unless code needs that level of portability.

Minor

Spacing

Separate function declarations from functions a bit.

bool bit_string_is_valid(char *bit_string);
// add space here
int main(void)


Update multiple records with Entity Framwork I am wondering what the best way to update multiple records with Entity Framework is.
This is how I normally do it, and it does work:

    private static void SetNcprpCodesAsComplete(string[] ncprpCodes)
    {
        using (var ent = new Data.Entities())
        {
            var query = from ba in ent.BuildingAssessments
                        where ncprpCodes.Contains(ba.NcprpCode)
                        select ba.TabletAssessment;
            foreach (var ta in query) ta.Complete = true;
            ent.SaveChanges();
        }
    }

This query should also work (is one better than the other)

      var query = from ta in ent.TabletAssessments
                  where ncprpCodes.Contains(ta.BuildingAssessment.NcprpCode)
                  select ta;

Another way could be looping through the string[], attaching and updating. Is it possible to attach when doing a multi table query like this?

This is the usual way how to update properties in EF, and in your case, I'd say it seems the only proper way. You can surely attach and update - but then you'll loose all other properties in your entity. To sum up - if you'd need to update one property in entities, you'll need to do SELECT (your query), modify entities (foreach) and then do UPDATE (SaveChanges).


Using Types to Designing Domain I am trying to model a domain of metrics for use in a BI application.  I recently read the Designing with Types series, and felt that it could be useful, so I wanted to give it a try.  What I'm not sure is if I'm trying too hard....

The metrics are application-defined, meaning they are finite and won't change during runtime.  A dozen metrics would probably be a lot.
New metrics will be introduced as the application grows
A user will be selecting which metrics they want to use, and the application has to take that and go "do stuff", like building a query to the backend store (MongoDB, in this case).
There may be application logic associated with metrics that I would like enforced through pattern matching so that when I introduce a new metric, I want the compiler to tell me that I need to do some work.

Here's what I have:

type Metrics =
  | Revenue
  | Volume
  | PriceAsp
  with
    member m.FormatGroupings =
      match m with
        | Revenue -> [("revenue", """$sum: { "$Amount" }" """)]
        | Volume  -> [("volume", """$sum: { "$Quantity" }" """)]
        | PriceAsp -> List.append Revenue.FormatGroupings Volume.FormatGroupings

    member m.FormatProjections =
      match m with
        | Revenue -> [("revenue", "$revenue")]
        | Volume -> [("volume", "$volume")]
        | PriceAsp -> [("priceAsp", """ $divide: ["$revenue", "$volume"]  """)]

let buildQuery groupBy (metrics:Metrics list) =
  let concatenate f =
    let x = metrics
            |> List.collect f
            |> List.map (fun m -> sprintf "{%s: %s}" (fst m) (snd m) )
    System.String.Join(",", x)

  let groupings = concatenate (fun m -> m.FormatGroupings)
  let projections = concatenate (fun m -> m.FormatProjections)

  sprintf """{$group: {_id: {%s: "$%s"}}, %s}, $project: {%s}}""" groupBy groupBy groupings projections

With examples of usage:

buildQuery "foo" [Revenue; Volume]  //Simple
buildQuery "foo" [PriceAsp]         //Compound

Note that PriceAsp is "made up of" both Revenue and Volume.

Please provide any general comments.  Some specific questions I have are:

Have the business rules for the metric calculations be embedded within the types
Show that some metrics are built off of other metrics (for example, PriceAsp), and enforce the explicit relationship.  Above, we see that PriceAsp is made up of Revenue and Volume because it's in that list.  But it would be better to make it enforced explicitly through typing.
There are formatting rules around an "instance" of a specific metric.  For example, when Revenue is shown to a user, it should be rounded to the nearest whole dollar, with thousand separators and a currency symbol (ex:  $1,234,567).
Is there an elegant way (using List.fold, perhaps) for doing what String.Join does?  String.Join is just so convenient...

A base metric would look like this (no $project):

{
  $group: {
    _id: {
      foo: "$Foo"
    },
    revenue: { $sum: "$Amount" }
  }
}

A compound metric may look like this:

{
  $group: {
    _id: {
      foo: "$Foo"
    },
    revenue: { $sum: "$Amount" },
    volume: { $sum: "$Quantity" }
  },
  $project: {
    priceAsp: { $divide: ["$revenue", "$volume"] }
  }
}

Do you have only a limited number of metric types?

In which case, you might want to make a union case of the possible types.
The compound cases can be recursive, referring to the type.

type Metric =
    | Revenue of string * string
    | Volume  of string * string
    | Compound  of string * Metric list

Then your formatMetric function needs to handle each case separately

let formatMetric metric =
    match metric with
    | Revenue (label,formula) ->
        sprintf "%s: {%s}" label formula
    | Volume  (label,formula) ->
        sprintf "%s: {%s}" label formula
    | Compound (label,metrics) ->
        sprintf "%s: {%s}" label "other metrics"

Note that that if you use this approach you don't really need the label any more, as the cases are the label.

type Metric =
    | Revenue of string
    | Volume  of string
    | Compound  of Metric list

And then you could also make formatMetric recursive, printing all the child formulas for a compound formula

let rec formatMetric metric =
    match metric with
    | Revenue formula ->
        sprintf "Revenue={%s}" formula
    | Volume  formula ->
        sprintf "Volume={%s}" formula
    | Compound metrics ->
        let childFormats =
            metrics
            |> List.map formatMetric
            |> List.reduce (+)
        sprintf "Compound: {%s}" childFormats

The formatting is not exactly what you want, but I'm sure you get the general idea.

EDIT

General Comment

Personally, I wouldn't add those member functions to the type. I would focus on designing the types in their own module independent of any particular representation, and then have a separate module that mapped them into the appropriate representation for MongoDB.  That way you are not mixing up different requirements.

String join.

A simple string join is trivial to write:

let stringJoin sep list :string =
    let append s t = s + sep + t
    list |> List.reduce append

stringJoin ", " ["a"; "b"; "c"; ]

Separate the types from the representation

But if you separate the types from the representation, you may find that you naturally create MongoDB specific functions that generate the appropriate representation. The idea is to build up a mini-language that you can compose together to make bigger blocks.
I don't know the MongoDB syntax, but maybe something like this?

let mSum p = sprintf "{ $sum: ""%s"" }" p
let mDivide p q = sprintf "{ $divide: [""%s"", ""%s""] }" p q
let mLabel p q = sprintf "%s : %s" s
let mGroup id ps = sprintf "$group: { %s, %s }" id (stringJoin ", " ps)
let mProject p q = sprintf "$project: { %s }" (stringJoin ", " ps)

If you are doing a lot of this, you might be better off using a JSON library. In which case the workflow would be:

domain type ==>  convert to intermediate type for Mongo API ==> convert to JSON


C++ templated singleton class - properly destroyed? I have written a templated singleton class in C++ but I am afraid that it is not properly destroyed. Can you advise me on that ?

my singleton.h

#ifndef SINGLETON_H
#define SINGLETON_H

template <typename T>
class Singleton
{
public:
    static T& Instance();

protected:
    virtual ~Singleton();
    inline explicit Singleton();

private:
    static T* _instance;
    static T* CreateInstance();
};

template<typename T>
T* Singleton<T>::_instance = 0;

#endif // SINGLETON_H

singleton.cpp

#include "singleton.h"
#include <cstdlib>

template <typename T>
Singleton<T>::Singleton()
{
    assert(Singleton::_instance == 0);
    Singleton::_instance = static_cast<T*>(this);
}

template<typename T>
 T& Singleton<T>::Instance()
{
    if (Singleton::_instance == 0)
    {
        Singleton::_instance = CreateInstance();
    }
    return *(Singleton::_instance);
}

template<typename T>
inline T* Singleton<T>::CreateInstance()
{
    return new T();
}

template<typename T>
Singleton<T>::~Singleton()
{
    if(Singleton::_instance != 0)
    {
        delete Singleton::_instance;
    }
    Singleton::_instance = 0;
}

and that's how I call it (with normal - not templated or anything - class Game )
Singleton<Game>::Instance().run();

A classic singleton looks like this (in C++):

class S
{
    public:
        static S& getInstance()
        {
            static S    instance; // Guaranteed to be destroyed.
                                  // Instantiated on first use.
            return instance;
        }
    private:
        S();
        // Dont forget to declare these two. You want to make sure they
        // are unaccessable otherwise you may accidently get copies of
        // your singleton appearing.
        S(S const&);              // Don't Implement
        void operator=(S const&); // Don't implement
};

It is simple enough that template-ing it is redundant.
Also making it a template start to have other issues that require you to have a very good linker. SO it is worth just doing it explicitly for each object you want to make a singleton.

It is worth noting that it is probably a mistake to use a singleton. The use cases were they actually work well are very limited. Best to create the object in main and pass it as a parameter to everything that needs it.

This article covers a lot of info.

Looking at your code the thing that sticks out is:

template<typename T>
Singleton<T>::~Singleton()
{
    if(Singleton::_instance != 0)
    {
        delete Singleton::_instance;
    }
    Singleton::_instance = 0;
}

You are already in the destructor of the only object. Thus calling delete on _instance is calling delete on yourself (and that must already have been done otherwise you would not be in the destructor).

As you have deduced the problem is that the object is not automatically destroyed. There are several ways of solving this in your code. The best would be to make _instance a std::auto_ptr (std::unique_ptr in C++11). This will mean as a static storage duration object that it will be destroyed at the end of the program (which will call the destructor for you).

That aside C++ code where you have pointers strewn about the place is not real C++ code. You may be using the C++ syntax but you are writing C. There is a very different style attached to C++. You should barely ever see pointers. This is because there is no ownership semantics associated with pointers and one of the big things in C++ is expressing ownership of dynamic storage duration objects.

About the only place you see pointers are in the implementation of containers and smart pointers. As a user you should be using smart pointers or containers (depending on situation) instead of pointers. Where you passing be reference using a pointer (aka C) you should use references (aka C++).