Animated JavaScript/jQuery game This was a code test I was asked to complete by employer, and since they rejected me, I wonder what should I have done differently.

var w = 7, h = 6;
var currentPlayer = 1;
var animationInProgress = 0;
var winner = 0;

var map = new Array(h);
var mapHtml = "";

for (var i = 0; i < w; i++) {
    map[i] = new Array(h);
    mapHtml = mapHtml + "
";
    for (var j = 0; j < h; j++) {
        map[i][j] = 0;
        mapHtml = mapHtml + "

";
    }
    mapHtml = mapHtml + "
";
}

$("#map").html(mapHtml);

function makeTurn(col) {
    if (animationInProgress == 0 && winner == 0) {
        for (var i = h-1; i >= 0; i--) {
            if (map[col][i] == 0) {
                map[col][i] = currentPlayer;
                animateTurn(col, i);
                checkForWin(col, i);
                currentPlayer = 3 - currentPlayer;
                break;
            }
        }
    }
}

function animateTurn(col, row) {
    animationInProgress = 1;
    $("#map #col"+col+" .next-ball").addClass("animation-in-progress");

    $("#map #col"+col+" .next-ball").animate({
        top: '+='+(52*(row+1)-1)
    }, {
        duration: Math.round((row+1)*500/h),
        complete: function() {
            $("#map #col"+col+" div:nth-child("+(row+2)+")").html("");
            $("#map #col"+col+" .next-ball").css("top", "0px");
            $("#map .animation-in-progress").removeClass("animation-in-progress");
            $("#map .next-ball").removeClass("player"+(3-currentPlayer));
            $("#map .next-ball").addClass("player"+currentPlayer);
            animationInProgress = 0;
        }
    });
}

function checkForWin(x, y) {
    var delta, dx, dy, n = 0;
    //horizontal win
    for (i = 0; i < w; i++) {
        if (map[i][y] == currentPlayer) {
            n++;
            if (n == 4) win(currentPlayer);
        } else {
            n = 0;
        }
    }

    //vertical win
    n = 0;
    for (i = 0; i < h; i++) {
        if (map[x][i] == currentPlayer) {
            n++;
            if (n == 4) win(currentPlayer);
        } else {
            n = 0;
        }
    }

    //diagonal / win
    n = 0;
    delta = Math.min(x, h-y-1);
    for (dx = x-delta, dy = y+delta; dx < w && dy > 0; dx++, dy--) {
        if (map[dx][dy] == currentPlayer) {
            n++;
            if (n == 4) win(currentPlayer);
        } else {
            n = 0;
        }
    }

    //diagonal \ win
    n = 0;
    delta = Math.min(x, y);
    for (dx = x-delta, dy = y-delta; dx < w && dy < h; dx++, dy++) {
        if (map[dx][dy] == currentPlayer) {
            n++;
            if (n == 4) win(currentPlayer);
        } else {
            n = 0;
        }
    }
}

function win(player) {
    alert('Win by player '+player+'. Refresh page to restart');
    winner = 1;
}

What are the problems with this code?

Non-descriptive variable names?
Too many constants in code?
Not using comments?

I'll start with a list of observations:

Hardly any comments.  There are several places where it would be nice if you added some comments to explain how the main logic works for someone who has not seen the code before.
A bunch of global variables and ones using very generic names - lots of opportunities for clashes.  I'd suggest either using a function closure to isolate your variables or make them properties of one global object or eliminate the need for them.
There is NO need for a selector like this: "#map #col2".  It should just be "#col2".
A bunch of places where you're evaluating the same selector more than once.  For example, in the animateTurn function, you evaluate $("#map .next-ball") multiple times when you could just chain.  This:

$("#map .next-ball").removeClass("player"+(3-currentPlayer));
$("#map .next-ball").addClass("player"+currentPlayer);

could be changed to this:

$("#map .next-ball").removeClass("player"+(3-currentPlayer))
    .addClass("player"+currentPlayer);

Or, in the whole function this:

function animateTurn(col, row) {
    animationInProgress = 1;
    $("#map #col"+col+" .next-ball").addClass("animation-in-progress");

    $("#map #col"+col+" .next-ball").animate({
        top: '+='+(52*(row+1)-1)
    }, {
        duration: Math.round((row+1)*500/h),
        complete: function() {
            $("#map #col"+col+" div:nth-child("+(row+2)+")").html("");
            $("#map #col"+col+" .next-ball").css("top", "0px");
            $("#map .animation-in-progress").removeClass("animation-in-progress");
            $("#map .next-ball").removeClass("player"+(3-currentPlayer));
            $("#map .next-ball").addClass("player"+currentPlayer);
            animationInProgress = 0;
        }
    });
}

could be changed to this to use chaining and avoid re-evaluating the same selector multiple times:

function animateTurn(col, row) {
    animationInProgress = 1;
    $("#map #col"+col+" .next-ball").addClass("animation-in-progress")
        .animate({top: '+='+(52*(row+1)-1)}, {
            duration: Math.round((row+1)*500/h),
            complete: function() {
                $("#col"+col+" div:nth-child("+(row+2)+")").html("");
                $("#col"+col+" .next-ball").css("top", "0px");
                $("#map .animation-in-progress").removeClass("animation-in-progress");
                $("#map .next-ball").removeClass("player"+(3-currentPlayer))
                    .addClass("player"+currentPlayer);
                animationInProgress = 0;
            }
        });
}

I generally find it cleaner to use eventListeners rather than onclick handlers because you don't have to worry about multiple listeners stomping on each other.
You have a bunch of constants in the code that aren't very self documenting.  There's a 500 and a 52 which I have no idea where they came from or when they would have to be changed if the game was modified.
Is there a reason that the HTML for the game is generated via javascript rather than specified separately?  Usually, it's advantageous for the presentation of the app to be separate from the code for the app and for the code to adapt to whatever the presentation is.  This allows separate people to work on the visual aspects of the app vs. the logic of the app and allows you to change one without affecting the other.


Simple program to insert values in a linked list I tried to write a simple program using linked list where I just insert the value stored in every node and then through a function print the values.

This is the code:

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

typedef struct Node {
    int value;
    struct Node* link;
} Node;

void print_node(Node * );

int main() {

    Node* head = NULL;
    head=malloc(sizeof(Node));
    if (head==NULL) {
       return 0;
    }
    printf("Insert the number of nodes :\n");
    int N;
    scanf("%d",&N);
    int i;
    Node * Node_c=head;
    for (i=0;i<N;i++) {
        if (i<N-1) {
            int temp;
            printf("Insert value stored into the node %d:\n",i+1);
            scanf("%d",&temp);
            Node_c->value=temp;
            Node_c->link=malloc(sizeof(Node));
            Node_c=Node_c->link;
        } else if (i=N-1) {
            int temp;
            printf("Insert value stored into the node %d:\n",i+1);
            scanf("%d",&temp);
            Node_c->value=temp;
            Node_c->link=NULL;
        }
    }
    printf("\n");
    print_node(head);
    return 0 ;
}

void print_node(Node * head) {
    Node * Node_current = head;
    int i=1;
    while (Node_current != NULL) {
        printf("Value stored into node %d:  %d\n",i,Node_current->value);
        Node_current = Node_current->link;
        i++;
    }
}

The code runs just fine but I want to know if this part where I insert the values stored in each node can be done in an shorter way

Node * Node_c=head;
for(i=0;i<N;i++){
    if(i<N-1){
        int temp;
        printf("Insert value stored into the node %d:\n",i+1);
        scanf("%d",&temp);
        Node_c->value=temp;
        Node_c->link=malloc(sizeof(Node));
        Node_c=Node_c->link;
    } else if (i=N-1) {
        int temp;
        printf("Insert value stored into the node %d:\n",i+1);
        scanf("%d",&temp);
        Node_c->value=temp;
        Node_c->link=NULL;
    }
}

I want to know if this part where I insert the values stored in each node can be done in an shorter way

Definitely it can be done in a shorter way!

the problem is that in your code you are taking the pain to do the same thing twice while inserting. in the for loop :

for (i=0;i<N;i++)
{
    if (i<N-1)
    {

        //here
        int temp;
        printf("Insert value stored into the node %d:\n",i+1);
        scanf("%d",&temp);
        Node_c->value=temp;
        // to here

        Node_c->link=malloc(sizeof(Node));
        Node_c=Node_c->link;

    }
    else if (i==N-1) //Note: it must be == operator not =
    {

        //the same thing again from here
        int temp;
        printf("Insert value stored into the node %d:\n",i+1);
        scanf("%d",&temp);
        Node_c->value=temp;
        //to here

        Node_c->link=NULL;

}

The below part appears twice in your insertion logic :

    int temp;
    printf("Insert value stored into the node %d:\n",i+1);
    scanf("%d",&temp);
    Node_c->value=temp;

Now can we avoid writing twice the same thing?

... Definetely you can write that logic only once and change your insertion logic to :

int i;
Node * Node_c=head;
for(i=0;i<N;i++)
{
    if(i!=0) //not required once again for head
    {
        Node_c->link=malloc(sizeof(Node));
        Node_c=Node_c->link;
    }

    //you don't require temp as you can directly access Node_c->value
    printf("Insert value stored into the node %d:\n",i+1);
    scanf("%d",&Node_c->value);

    Node_c->link=NULL;

}

the logic is quite simple to understand :

first we create a memory for a node and save it's  address to the link of previous node(we don't if its a head node as it's already allocated with memory in the before part of the code and it has no previous nodes to be linked with)
then we take in the value and store in the value member of structure Node_c
then we point the link member of the structure Node_c to NULL... this goes on for N number of times


Is there a better way of showing multiple NSDictionaries? I have a method that contains a bunch of NSDictionaries (in fact, the only reason I have that method is to create those NSDictionaries).  While I don't believe there are enough NSDictionaries initialized to visibly slow the code down as of yet, that is a good possibility in the future.  I am currently just creating all the NSDictionaries separately, but is there a better way to do it while keeping its readability?

- (void)items {

    NSDictionary *item1 = @{
    @"label"     : @"item1",
    @"icon"      : [UIImage imageNamed:@"item1-Icon"],
    @"identifier": @"item1"};

    NSDictionary *item2 = @{
    @"label"     : @"item2",
    @"icon"      : [UIImage imageNamed:@"item2-Icon"],
    @"identifier": @"item2"};

    NSDictionary *item3 = @{
    @"label"     : @"item3",
    @"icon"      : [UIImage imageNamed:@"item3-Icon"],
    @"identifier": @"item3"};

    NSDictionary *item4 = @{
    @"label"     : @"item4",
    @"icon"      : [UIImage imageNamed:@"item4-Icon"],
    @"identifier": @"item4"};

    items = [ @[item1, item2, item3, item4] mutableCopy];

}

«Two or more, use a for»
  — Edsger W. Dijkstra

Okay, I am not using a for-loop, but an enumeration

NSArray *itemArray = @[
    @[@"item1",@"item1-Icon",@"item1"],
    @[@"item2",@"item2-Icon",@"item2"],
    @[@"item3",@"item3-Icon",@"item3"],
    @[@"item4",@"item4-Icon",@"item4"]
];

NSMutableArray *items = [NSMutableArray array];
[itemArray enumerateObjectsUsingBlock:^(NSArray *itemProperties, NSUInteger idx, BOOL *stop) {
    [items addObject: @{ @"label":  itemProperties[0],
                         @"icon":  [UIImage imageNamed:itemProperties[1]],
                         @"identifier":  itemProperties[2]
      }];
}];


checks validity of credit card in python (cs50 pset6) I'm new to using python and wanted to know how I could improve my program. I'm using the Luhn algorithm, the beginning numbers of the credit cards, and the number of digits in the credit card.
from cs50 import get_string
import re
import sys

number = get_string("NUMBER: ")
check = []

if (len(number)) not in (13,15,16):
    print("INVALID")
    print(len(number))
    sys.exit(1)

for i in range(len(number)): # for the length of the string,
    if i%2 == (-2+len(number))%2: # if a counter is modulo 1
        digits = str(int(number[i])*2) #obtains the value of the multiplication
        for j in range(len(digits)):
            check.append(int(digits[j]))
    else:
        check.append(int(number[i]))

total = str(sum(check))

if re.search("0$", total):
    if re.search("^3(4|7)", number):
        print("AMEX")
    elif number[0] == str(4):
        print("VISA")
    elif re.search("^5(1|2|3|4|5)", number):
        print("MASTERCARD")
else:
    print("INVALID")

Your program would benefit from breaking up logic into functions. This also eliminates the global variables involved.
if i%2 == (-2+len(number))%2

This line is strange and not immediately obvious what it's doing. A reversed iteration would be easier to understand.
import re

def luhn_digit(digit,double):
    if double:
        doubled = digit*2
        return doubled-9 if doubled>9 else doubled
    else:
        return digit

def is_valid_luhn(card):
    check_digit = int(card[-1])
    luhn_sum = sum([
        luhn_digit(int(digit),i%2==0)
        for i,digit in enumerate(reversed(card[:-1]))
    ])
    total = check_digit+luhn_sum
    return total%10==0

def is_valid_card(card):
    return (
        len(card) in (13,15,16) and
        is_valid_luhn(card)
    )

def get_card_type(card):
    if not is_valid_card(card):
        return "INVALID"
    if re.search("^3(4|7)", card):
        return "AMEX"
    if card[0] == str(4):
        return "VISA"
    if re.search("^5(1|2|3|4|5)", card):
        return "MASTERCARD"
    return "INVALID"

print(get_card_type(input("NUMBER: ")))


Account inactivity reminder I wanted to know if this is the best approach for the Single Responsibility Principle.
The responsibility of this class is to remind users whose accounts are about to expire due to inactivity.

On the 30th day of inactivity, the user gets an email.
On the 45th day of inactivity, the user does not get an email; they get disabled.

public class AccountReminder
{

    private readonly IUnitOfWork _uow;
    private readonly ISendExpiringEmail _sendExpiringEmail;

    public AccountReminder(IUnitOfWork uow, ISendExpiringEmail sendExpiringEmail)
    {
        _uow = uow;
        _sendExpiringEmail = sendExpiringEmail;
    }

    public void NotifyUpcomingExpiringAccounts()
    {

        // Establish dates
        DateTime accountWarningDateTime = DateTime.Now.AddDays(-30).Date;
        DateTime expirationDateTime = DateTime.Now.AddDays(-45).Date;

        // Retrieve users whose accounts are either about to expire or have already expired
        var users = _uow.UserRepository
            .FindBy(element => (DbFunctions.TruncateTime(element.LastLoginDate) == accountWarningDateTime));

        // Send a reminder
        foreach (var user in users)
        {
            string htmlBody = "";
            htmlBody += "<meta http-equiv=...";
            htmlBody += "";

            //following line for only outlook to display image
            htmlBody += "<v:shape id=\>...</v:shape>";
            htmlBody += "<table width='100%'...
";
            htmlBody += "";

            AlternateView htmlView = AlternateView.CreateAlternateViewFromString(htmlBody, null, MediaTypeNames.Text.Html);
            AlternateView plainView = AlternateView.CreateAlternateViewFromString(htmlBody, null, "text/plain");
            LinkedResource pic1 = new LinkedResource("images/Logo.jpg", MediaTypeNames.Image.Jpeg);
            pic1.ContentId = "Logo";
            htmlView.LinkedResources.Add(pic1);

            _sendExpiringEmail.SendTo(new[] { user.Email }, "EmailSubject", htmlView, plainView);

        }

        DisableExpiringAccounts(expirationDateTime);
    }

    private void DisableExpiringAccounts(DateTime expirationDateTime)
    {
        var expiredUsers = _uow.UserRepository.FindBy(element => element.Enabled && (DbFunctions.TruncateTime(element.LastLoginDate) <= expirationDateTime));

        foreach (var user in expiredUsers)
        {
            user.Enabled = false;
        }

        if (expiredUsers.Count > 0)
        {
            _uow.SaveChanges();
        }
    }

}

I'm going to go with no. There's at least two responsibilities in your description alone:

Determine accounts that are about to expire due to inactivity.

and

Notify users of a situation (in this case, the inactivity expiry).

And then in your code, you have a third:

Disable expired account.

I can potentially see the first and third items being in a class together. But user notifications lumped into this smells and likely should be separated.


Filling a container with objects using pointers / iterators I have a series of nested objects that I need to serialize into a byte (unsigned char) array to send out to another computer over UDP.

Right now I each object create a std::list, and pass that list to the next object up the 'tree' and then it merges the lists to create the final packet.

class Item {
  unsigned char value;
}

class Sublist {
  std::vector<Items*> items;
  std::list<unsigned char> subpacket;

  std::list<unsigned char> Sublist::serialize() {
    subpacket.clear();
    for(auto item : items) {
      subpacket.push_back(item->value);
    }
    return subpacket;
  }
}

class Superlist {
  std::queue<Sublist> sublists;
  std::list<unsigned char> serializedSublist;
  std::list<unsigned char> packet;

  std::list<unsigned char> Superlist::serialize() {
    packet.clear();
    for(auto sublist : sublists) {
      serializedSublist = sublist.serialize();
      packet.insert(packet.end(), serializedSublist.begin(), serializedSublist.end());
    }
    return packet;
  }

}

However, this feels really inefficient with unnecessary clear() and insert() calls, and in generally feels a bit sloppy.

I feel like a more efficient approach would be for the Superlist to have a unsigned char container (vector, list, deque?), and then give each Sublist a pointer to where it should write data to.

I feel like perhaps something like:

class Sublist {
  std::vector<Items*> items;

  void Sublist::serialize(std::vector::iterator begin) {
    for(auto item : items) {
      begin->push_back(item->value);
      ++begin;
    }
  }
}

class Superlist {
  std::queue<Sublist> sublists;
  std::vector<unsigned char> packet;

  std::list<unsigned char> Superlist::serialize() {
    packet.clear();
    for(auto sublist : sublists) {
      sublist.serialize(packet.end());
    }
    return packet;
  }

}

Problems with this exact code include:

I don't think I can call push_back on a iterator
It'd be nice if I could create the final packet of a final size first, and then send pointers off to the correct location in the packet for each sublist to begin writing.

Would an approach like this work?  Is there an even more concise and clever way to place all of the objects in a buffer?

std::list is usually implemented as a doubly-linked list which is very inefficient for your purpose.  You should definitely use std::vector for this (or similar dynamic array structure, possibly some stream).

Your second approach is the idea I got while reading your question. You don't need an iterator (and I don't think you can call push_back on it), but a reference to the vector - vector<byte>& buffer as the argument.

The same applies to serizalize() on the main list with possible default argument creating the vector. If you want to have the final packet somewhere, you can track some dirty flag (if you can modify the structure) to re-serialize it as needed.

You could do that last approach in two phases:

Track the sizes to compute the total size needed
Allocate the buffer (you could use byte* or vector)
Do the same again, but passing index together with the vector (buf[i++] = data)

...it is a bit more low-level solution, but possibly fastest. I would personally not worry that much and use the direct approach with vector (it should be fast enought).

Some notes about errors in the code:

class Sublist {
  std::vector<Items*> items;
  std::list<unsigned char> subpacket;

  std::list<unsigned char> Sublist::serialize() {

You do use Sublist:: prefix when you define the methoud outside of the class, not here (remove it).

class Sublist { ...
    std::vector<unsigned char>& serialize(std::vector<unsigned char>& buffer) const { ...

This should be the final signature (taking reference, returning the reference and with const modifier on the method because you are not modifying the class).


Using dynamic methods to build SQL queries in CodeIgniter I'm using this class as Modal in my CodeIgniter project, as I'm newbie in CI, I wanna know if this has some issues, I'm using this modal for all queries, like I call this with parameters and it returns results as I wanted. But I'm not sure if it a good idea or it might give some bad impact to structure and security.

<?php
  defined('BASEPATH') OR exit('No direct script access allowed');
  class Crud extends CI_Model {
      public function Read($TableName, $Condition) {
          if ($Condition == '')
              $Condition = ' 1';
          $this->db->where($Condition);
          $data = $this->db->get($TableName);
          return $data->result();
      }
      public function Create($data, $TableName) {
          if ($this->db->insert($data, $TableName))
              return $this->db->insert_id();
          else
              return false;
      }
      public function Update($TableName, $Fields, $Condition) {
          if ($Condition == '')
              $Condition = ' 1';
          $this->db->where($Condition);
          if ($this->db->update($TableName, $Fields))
              return true;
          else
              return false;
      }
      public function Delete($TableName, $Condition) {
          if ($Condition == '')
              $Condition = ' 1';
          $this->db->where($Condition);
          if ($this->db->delete($TableName))
              return true;
          else
              return false;
      }
      public function Count($TableName, $Condition) {
          if ($Condition == '')
              $Condition = ' 1';
          $this->db->where($Condition);
          $result = $this->db->get($TableName);
          if ($result)
              return $result->num_rows();
          else
              return false;
      }

I call this methods like this in my controller

public function changeStatus() {
   $tableName = $this->uri->segment(4);
   $status = $this->uri->segment(5);
   $conditionId = $this->uri->segment(6);
   $condition = " `id` = '$conditionId'";
   $data = [
       'is_active' => $status
   ];
   if($this->Crud->Update($tableName, $data, $condition))
       $this->session->set_flashdata('success', "Success! Changes saved");
   else
       $this->session->set_flashdata('danger', "Something went wrong");
   $referer = basename($_SERVER['HTTP_REFERER']);
   redirect('admin/seller/'.$referer);
}

Please let me know if I can provide more information

In accordance with PSR-1, your method names should be in camelCase. To avoid function name conflicts (native count() with custom count()), it may be best to prefix the method names' functionality with easy or simple, ci, or ar for Active Record.

"Embrace the curly brace."  Always use curly braces to encapsulate your language construct and function bodies -- even if they only have one line of code inside them.  Not using { and } may lead to developer confusion or unintended/incorrect code logic.

For versatility, I recommend passing an array of arrays to your Crud methods.  This will allow you to separate column names from values, enable you to turn off CI's automatic quoting (protect_identifiers flag) when necessary (3rd element in a respective subarray), and effortlessly pass multiple where conditions when required.  There is a multitude of reasons for this utility -- here is just one: https://stackoverflow.com/q/2489453/2943403.

Granted, your Read() method _could_be as simple as passing the table name and an associative array of conditions to get_where(), but that approach doesn't permit the flexibility of turning off the auto-quoting.

Make the default $condition parameter value an empty array.  I don't like the meaningless WHERE 1 in any project.

Use a foreach loop to only add a WHERE clause when there is actually something useful to write inside it.

public function ciRead($tableName, $conditions = []) {
    foreach ($conditions as $condition) {
        $this->db->where($condition);
    }
    $this->db->get($tableName)->result();
}

insert() should have the table name as the first parameter.

public function ciCreate($tableName, $data) {
    return $this->db->insert($tableName, $data) ? $this->db->insert_id() : false;
}

For update() loop the conditions like in read()  I recommend returning the affected rows so that there is a truer representation of success. I often treat the affected rows value as a boolean or explicitly cast it with (bool) if I only need a truthy/falsey value and I want to make the coding intention very clear.

public function ciUpdate($tableName, $data, $conditions = []) {
    foreach ($conditions as $condition) {
        $this->db->where($condition);
    }
    return $this->db->update($tableName, $data) ? $this->db->affected_rows() : false;
}

Same thinking with delete() regarding a more accurate return value...

public function ciDelete($tableName, $conditions = []) {
    foreach ($conditions as $condition) {
        $this->db->where($condition);
    }
    return $this->db->delete($tableName) ? $this->db->affected_rows() : false;
}

When counting, CI has count_all_results().

public function ciCount($tableName, $conditions = []) {
    foreach ($conditions as $condition) {
        $this->db->where($condition);
    }
    return $this->db->count_all_results($tableName);
}

In your controller (assuming all of the routing is set up properly), you should be collecting the slash-delimited input values as parameters of the method call instead of writing out all of those segment() calls.

Using $_GET data is perfectly fine when calling your Read() or Count() methods, but it is not recommended when you are writing to the database.  For Insert(), Update(), and Delete(), you need to be collecting user-supplied data via $_POST data only.

Be sure to validate and sanitize the incoming data in your controller before allowing access to your model methods.

Don't give your users any more access/privilege than absolutely necessary.  I wouldn't be letting people know what my table or column names are called.   I wouldn't want to offer them the flexibility to nominate the table/column that they want to access -- purely on the grounds of security.

Perhaps provide some kind of alias/whitelist to convert the user data to model entity identifiers (translate the table/column names with in the model) or ideally make all of the table/column names static/hardcoded (provided by you, the developer).

public function fetch($tableName, $conditionId) {
    $result = $this->Crud->ciRead($tableName, [['id', $conditionId]]));
    ...
}

or when writing to the db use post().

public function changeStatus() {
    $tableName = $this->input->post('tableName');  // I don't like it
    $status = $this->input->post('status');
    $conditionId $this->input->post('conditionId');
    if ($this->Crud->ciUpdate($tableName, ['is_active' => $status], [['id', $conditionId]])) {
       $this->session->set_flashdata('success', "Success! Changes saved");
    } else {
       $this->session->set_flashdata('danger', "Something went wrong");
    }
    redirect('admin/seller/' . basename($_SERVER['HTTP_REFERER']));
}


Long-to-string and string-to-long while checking characters within a xlate table I am attempting to come up with a better method for doing this:
public static String longToPlayerName(long name) {
    int i = 0;
    char[] nameCharacters = new char[12];
    while (name != 0L) {
        long ll = name;
        name /= 37L;
        nameCharacters[11 - i++] = playerNameXlateTable[(int) (ll - (name * 37L))];
    }
    return new String(nameCharacters, 12 - i, i);
}

public static long playerNameToInt64(String s) {
    long l = 0L;
    for (int i = 0; (i < s.length()) && (i < 12); i++) {
        final char c = s.charAt(i);
        l *= 37L;
        if ((c >= 'A') && (c <= 'Z')) {
            l += (1 + c) - 65;
        } else if ((c >= 'a') && (c <= 'z')) {
            l += (1 + c) - 97;
        } else if ((c >= '0') && (c <= '9')) {
            l += (27 + c) - 48;
        }
    }
    while (((l % 37L) == 0L) && (l != 0L)) {
        l /= 37L;
    }
    return l;
}

I can't seem to come up with a better way. Any ideas would be excellent!

Pretty good overall, but a few things:

Don't hard-code the 37. Instead, use the translation table's size.
ll is a bad variable name. It's not descriptive at all, and it looks like 11.
Your loop in longToPlayerName can be simplified:

final int base = playerNameXlateTable.length;
while (name != 0L) {
    final long ch = name % base;
    nameCharacters[11 - i++] = playerNameXlateTable[ch];
    name /= base;
}

Be careful with inlined ++i or i++. They can be easy to overlook, and the human brain (mine anyway) doesn't parse arithmetic expressions that cause mutations very well.
I think you've overused parenthesis a bit. For example, I would just do:

if (c >= 'A' && c <= 'Z') {

playerNameToInt64 should be playerNameToLong
s is a bad parameter name. Call it something like name

Other than for loop controls, single letter variables should typically be avoided.
It should also probably be final, but it's immutable, so it doesn't really matter in this case.

Use Character.toLowerCase to eliminate one of the branches in playerNameToInt64.
It might be better to group your shifts like 1 + (c - 97) instead of how they're grouped. It doesn't matter in the current code, but if (1 + c) - 97 can in theory overflow, but 1 + (c - 97) cannot
Rather than 97/65/48, use character literals:

1 + (c - 'a')

The truncation loop is a bit troubling. For a-zA-Z0-9, there never will be any trailing 0 valued base-37 digit. This means that you're worried an invalid character will be provided. But... what if one is in the middle of the string?
If you made a reverse translation table, you could decouple your code from ASCII

no point in doing this though, as it's probably always going to be ASCII (or a superset of ASCII)

Fold the double comparison in the toLong loop into a Math.min() call and use that
Assuming this class is dedicated to player name translation and only player name translation (as it should be), playerNameXlateTable could be shortened to translationTable

In the context of a single-responsibility class, it's obvious what the table is translating
Though partly I'm just saying this because xlate makes me cry, but playerNameTranslationTable does too in all of its 26 character glory.

Will you ever have any other name encodings? If so, it might be worth-while to consider making an interface and having these methods be non-static.

It would make your code a bit more verbose, but it would allow for a bit more decoupling

Make 12 a constant

It would be nice to calculate it since maxLetters = floor(log(2^LONG_BITS) / log(37)), but there's no non-hideous way to calculate this
If it's a constant and you later expand the translation table, you'll only have to change 12 in one place instead of multiple. It's always good to reduce the chance of introducing future bugs.

Since the code is short, I gave it a go at revising it:

public class Review {

    private static char[] translationTable = buildTranslationTable();
    private static int maxLetters = 12;

    public static void main(String[] args)
    {
        System.out.println(playerNameToLong("Corbin92"));
        System.out.println(longToPlayerName(324533943486L));
    }

    public static String longToPlayerName(long name) {
        int i = 0;
        char[] nameCharacters = new char[maxLetters];
        final long base = translationTable.length;
        while (name != 0L) {
            final long ch = name % base;
            nameCharacters[maxLetters - ++i] = translationTable[(int) ch];
            name /= base;
        }
        return new String(nameCharacters, maxLetters - i, i);
    }

    public static long playerNameToLong(final String name) {
        final long base = translationTable.length;
        final int len = Math.min(name.length(), maxLetters);
        long l = 0L;
        for (int i = 0; i < len; ++i) {
            final char c = Character.toLowerCase(name.charAt(i));
            l *= base;
            if (c >= 'a' && c <= 'z') {
                l += 1 + (c - 'a');
            } else if (c >= '0' && c <= '9') {
                l += 27 + (c - '0');
            }
        }
        return l;
    }

    private static char[] buildTranslationTable()
    {
        char[] table = new char[37];
        for (char c = 'a'; c <= 'z'; ++c) {
            table[1 + c - 'a'] = c;
        }
        for (int i = 0; i < 10; ++i) {
            table[i + 27] = (char) (i + '0');
        }
        return table;
    }
}


Reverse an integer, find the absolute difference, then reverse again For one of my university assignments, we were asked to take an unsigned int, reverse its digits, find the absolute difference between the original and reversed, then reverse that, and you receive the "Magic Number".

$$ R(\ \lvert R(A) - A\rvert\ )$$

where \$A\$ is a positive integer and \$R(n)\$ means reverse.

We were also asked to take multiple inputs at once and keep track of the largest absolute difference between all inputs.

#include <stdio.h>

// Reverse an unsigned integer
unsigned int reverseInteger(unsigned int input) {

    unsigned int output = 0;

    while (input != 0) { // Terminate when input is 0
        output = output * 10 + input % 10; // Can't use =* here, order of operations!
        input /= 10;
    }

    return output;
}

// Find the absolute differnece between a reversed number, then reverse the absolute difference
unsigned int generateMagicNumber(unsigned int input) {

    /* Find Absolute Difference */
    if (input < reverseInteger(input)) {
        input = reverseInteger(input); // If the result will be negative, reverse the number before finding the difference
        input -= reverseInteger(input);
    }
    else {
        input -= reverseInteger(input);
    }

    largestAbsoluteDifference(input);

    return reverseInteger(input); // Reverse absolute difference to find the magic number

}

// Find the largest of all values passed to this function
unsigned int largestAbsoluteDifference(unsigned int input) {

    static unsigned int keep = 0; // Must be static!

    return (input > keep) ? (keep = input) : keep; // If the input is greater than the current, store the input. Else, keep the current.

}

int main() {

    int inputs;
    printf("Enter number of inputs: ");
    scanf("%d", &inputs);
    printf("Enter space separated: ");

    for (int i = 0; i < inputs; i++) {
        unsigned int num;
        scanf("%u", &num);

        if (i == 0) { printf("\nOutput: "); } // Append label only on first iteration

        printf("%u ", generateMagicNumber(num)); // Generate and print the magic number
    }

    printf("\nLargest Absolute Difference: %u\n", largestAbsoluteDifference(0)); // Must pass 0 into largestAbsoluteDifference

}

Example Input / Output

Enter number of inputs: 5
Enter space separated: 23 512 454 94 100

Output: 9 792 0 54 99
Largest Absolute Difference: 297

Notes / Concerns

The way I wrote the largestAbsoluteDifference seems slimy me to (having to pass 0 to get the result).
I've got in the habit of calling functions inside a printf instead of storing the value in a variable and then printing the variable (see lines 56, 59). Is this okay?
In generateMagicNumber I had to make sure finding the difference wouldn't result in a negative number (which you can't store in an unsigned int). Is my method the best way to find the absolute value like that?

You write unsigned int enough times that it's probably worthwhile to typedef it, in caseunsigned int turns out to be too small. (Keep in mind that it is only guaranteed to go up to 65535.)

typedef unsigned int number;
#define NUMBER_FMT "%u"

The reverseInteger() function is fine.  I'd rename it for clarity, then remove the superfluous comments.

/**
 * Reverse the digits of a number
 */
number reverseDigits(number input) {
    number output = 0;

    while (input != 0) {
        output = output * 10 + input % 10;
        input /= 10;
    }

    return output;
}

The code in generateMagicNumber() was really hard to follow.  The call reverseInteger(input) appears five times in that function.  One codepath has four of those calls, and the other has three.  Not only is that suboptimal1, it's also confusing because the input refers to a different number each time even though they all look the same.  Please avoid repurposing variables like that.

/**
 * The reverse of the absolute difference between a number and its reverse
 */
number magic(number input) {
    number rev = reverseDigits(input);
    number absDiffWithReverse = (rev > input) ? rev - input : input - rev;
    return reverseDigits(absDiffWithReverse);
}

The ugly part of the problem is the requirement to keep track of the largest \$\lvert R(A) - A \rvert\$.  I see four unpalatable options:

As you have done, insert code inside generateMagicNumber() to capture the intermediate computational result.

The disadvantages are that you are doing this as a side-effect, and using static (or global) state.  That kind of statefulness would become a maintenance headache on larger projects.  You also had to use a funny hack of calling largestAbsoluteDifference(0) to read out the maximum.
Instead of defining generateMagicNumber(), define absDiffWithReverse() to calculate \$\lvert R(A) - A \rvert\$.  Then, let main() be responsible for tracking the maximum of those.

To get the magic numbers, main() would need to finish the job by reversing them.  It's a bit dissatisfying that, for a program whose primary purpose is to compute "magic numbers", there is no magic-number-computing function.
Define a function to compute both the magic number and the \$\lvert R(A) - A \rvert\$ intermediate result.

Since C only allows one return value, you could either return a struct with two numbers or pass back one of the values as an out-parameter.  I find both techniques aesthetically unpleasing, especially since those two results can be trivially derived from each other.
Define a function to compute the magic number.  To get the \$\lvert R(A) - A \rvert\$, reverse the magic number again.

As @MatthieuM has pointed out, reversing the digits twice could change the result, due to the possibility of trailing or leading zeroes.

I think I would prefer option 3:

number magic(number input, number *absDiffWithReverse) {
    number rev = reverseDigits(input);
    *absDiffWithReverse = (rev > input) ? rev - input : input - rev;
    return reverseDigits(*absDiffWithReverse);
}

number max(number a, number b) {
    return a > b ? a : b;
}

int main() {
    int inputs;
    printf("Enter number of inputs: ");
    scanf("%d", &inputs);
    printf("Enter space separated: ");

    number maxDiffRev = 0;
    for (int i = 0; i < inputs; i++) {
        number n, nMagic, absDiffRev;
        scanf(NUMBER_FMT, &n);
        // Print label only on first iteration
        if (i == 0) { printf("\nOutput: "); }

        nMagic = magic(n, *absDiffRev);
        printf(NUMBER_FMT " ", nMagic);
        maxDiffRev = max(maxDiffRev, *absDiffRev);
    }

    printf("\nLargest Absolute Difference: " NUMBER_FMT "\n", maxDiffRev);
}

To address your specific concerns:

Yes, calling largestAbsoluteDifference(0) to read the maximum is slimy.  Ideally, static state should be avoided altogether.
Calling functions inside printf() calls is fine if they are pure functions with no side-effects.  It's not as good a practice if the function being called has side-effects.  I wouldn't worry about it in your case.  What I wrote above, with a variable assignment inside a printf() call, is probably far more controversial.
Your technique is arithmetically correct, but not recommended due to the input variable shifting its meaning.

1 Not that performance really matters here.  What I'm pointing out is silliness more than a serious efficiency problem.


Fibonacci Nth term using tail recursion I have created a program that prints the Nth term of the Fibonacci sequence. The fib function needs to use tail recursion. If what I have coded isn't tail recursion, I would like to know how to change the fib function so it does.

#include <iostream>
#include <sstream>

int fib(int n, int i = 0, int a = 0, int b = 1)
{
    return (i >= n - 1) ? a : fib(n, i + 1, b, a + b);
}

int main(int argc, char* argv[])
{
    if (argc < 2) {
        std::cerr << "Argument 2 must be the Nth term." << std::endl;
        return -1;
    }

    std::stringstream ss_obj(argv[1]);
    unsigned long int number;

    ss_obj >> number;

    std::cout << "\nFibonacci number: " << fib(number) << std::endl;
    return 0;
}

To address your immediate concerns, it is a tail recursion indeed. OTOH, there is no need to be that terse. You may want to be a little more explicit:

    if (i == n) {
        return a;
    }
    return fib(n, i + 1, b, a + b);

Now the tail-recursiveness is obvious.

The error message "Argument 2 must be the Nth term." is misleading. The Nth term definitely refers to the Nth Fibonacci number, rather than the index of the number to be computed.

Besides that, traditionally such message is formatted as

    "Usage: " << argv[0] << " index\n";


Making my Rock, Paper, Scissors game shorter I am looking for advice on making the following code shorter.  Any help is appreciated.

public String determineWinner(String winner) {
    winner = "";
    if (playChoice.equals(compChoice))
        winner = "The result is a tie.";
    else if (compChoice.equals("R"))
    if (playChoice.equals("S") || playChoice.equals("s"))
        winner = "Computer wins because Rock beats Scissors.";
    else
        winner = "Player wins because Paper beats Rock";
    else if (compChoice.equals("S"))
    if (playChoice.equals("P") || playChoice.equals("p"))
        winner = "Computer wins because Scissors beats Paper.";
    else
        winner = "Player wins because Rock beats Scissors";
    else
    if (playChoice.equals("R") || playChoice.equals("r"))
        winner = "Computer wins because Paper beats Rock.";
    else
        winner = "Player wins because Scissors beats Paper.";
    return winner;
}

You code looks unclear from point of view of parameter usage and ignoring case difference is case of "The result is a tie.", so ...

Try this :).

public class testik {
    // No throws clause here
    public static void main(String[] args) {
        // Paper < Rock < Scissors < Paper
        System.out.println("Comp:S vs User:R = " + determineWinnerOtherWay("S","R")); //user win
        System.out.println("Comp:S vs User:P = " + determineWinnerOtherWay("S","P")); // com win

        System.out.println("Comp:P vs User:R = " + determineWinnerOtherWay("P","R")); //com win
        System.out.println("Comp:P vs User:S = " + determineWinnerOtherWay("P","S")); // user win

        System.out.println("Comp:R vs User:S = " + determineWinnerOtherWay("R","S")); //com win
        System.out.println("Comp:R vs User:P = " + determineWinnerOtherWay("R","P")); // user win
    }

    public static String code2Title(String code){
        if ("R".equals(code)) {
            return "Rock";
        } else if ("P".equals(code)) {
            return "Paper";
        } else {
            return "Scissors";
        }
    }

    public static String determineWinnerOtherWay(String compChoice, String playChoice) {
        String winner = "";
        String playerChoiceN = playChoice.toUpperCase();
        if (playChoice.equals(compChoice))
            winner = "The result is a tie.";
        int diff = compChoice.charAt(0) - playerChoiceN.charAt(0);
        if ( diff == 1 || diff == 2 || diff ==-3) {
            winner = "User win bacause:"+code2Title(playerChoiceN)+" wins " + code2Title(compChoice);
        } else {
            winner = "Computer wins because:"+code2Title(compChoice)+" wins " + code2Title(playerChoiceN);
        }
        return winner;
    }

}


Waiting for input with timeout When my program begins, I want to allow the user to hit a key to enter debug mode for one of the modules. Many of the solutions (involving alarm(), in particular) seemed complicated and involved multiple functions. Instead, I adapted this answer:

// Check to see if we should enter GSM de-bug mode
fd_set          s;
struct timeval  timeout;

timeout.tv_sec = 0;
timeout.tv_usec = 100000;
int i = 100000;

do {
    printf("Press [ENTER] for GSM debug mode... %i  \r",i/10000);
    fflush(stdout);
    FD_ZERO(&s);
    FD_SET(STDIN_FILENO, &s);
    select(STDIN_FILENO+1, &s, NULL, NULL, &timeout);
    i--;
} while (FD_ISSET(STDIN_FILENO, &s) == 0 && i > 0);

if ( i <= 0 ) { printf("\nSkipping debug mode...\n"); }
else
{
    printf("\nEntering debug mode...\n");

    // fill in the debug code here

}

This code works, based on a few quick tests, and exhibits the behavior I want. But, have I created any problems for myself?

Must reset timeout
select() is allowed to modify your timeout value.  So for your program, what happens is that the first call to select() takes 0.1 seconds because of the timeout, but the timeout is then reduced to 0.  The next 99999 calls to select() have a zero timeout.  You should reset the timeout right before you call select(), like this:
timeout.tv_sec = 0;
timeout.tv_usec = 100000;
select(STDIN_FILENO+1, &s, NULL, NULL, &timeout);

Once you do that, you no longer need 100000 iterations, because each iteration will have the proper 0.1 second timeout instead of returning "immediately".


Is there a circular reference in a set of template substitutions? I have a configuration engine that allows the user to specify a dictionary with text templates (read from a JSON file) where each placeholder like {x} can be substituted for a value found under a key with the same name e.g. x.

var constants = new Dictionary<string, object>
{
    { "x", "foo {y} baz" },
    { "y", "bar {x} qux" },
};

These are just two sample strings to show how it can look like. The real configuration contains connection strings, email lists, user names, sql etc.

They are resolved at runtime with this method

public static string FormatAll(this string text, Dictionary<string, object> values)
{
    while (text.ToString() != (text = text.Format(values))) ;
    return text;
}

where Format is a more advanced version of my question about string interpolation.

As you can probably see in the example, there is a danger that someone creates a circular template path and the formatting runs forever.

In order to prevent this from happening, I thought I validate the template dictionary before I let the formating method work with it. This is what I've come up with.

The ValidateIsNotCircular extension runs over the reference dictionary and checks each path for recursiveness and as soon as it finds one that is circular, it throws an exception that contains the path. I wanted it to work without recursion so I used a Stack.

Sidenote: This time I'm confined to C# < 6 so no fancy code here :-(

public static void ValidateIsNotCircular(this Dictionary<string, IEnumerable<string>> values)
{
    var visitedKeys = new HashSet<string>();

    var stack = new Stack<Tuple<string, IEnumerator<string>>>();

    var currentRef = new Func<string>(() => stack.Peek().Item2.Current);
    var currentRefs = new Func<IEnumerator<string>>(() => stack.Peek().Item2);

    foreach (var item in values)
    {
        if (!visitedKeys.Add(item.Key)) continue;

        stack.Push(Tuple.Create(item.Key, item.Value.GetEnumerator()));
        do
        {
            while (currentRefs().MoveNext())
            {
                if (stack.Any(x => currentRef() == x.Item1))
                {
                    throw new CircularPathException(stack.Select(x => x.Item1));
                }

                var next = values[currentRef()];
                visitedKeys.Add(currentRef());
                stack.Push(Tuple.Create(currentRef(), next.GetEnumerator()));
            }
            stack.Pop();
        } while (stack.Any());
    }
}

class CircularPathException : Exception
{
    public CircularPathException(IEnumerable<string> path)
        : base(string.Format("Circular path [{0}]!", string.Join(", ", path.Reverse().Select(x => string.Format("'{0}'", x)))))
    { }
}

There is one more method that extracts the placeholders {x} from the templates and builds another dictionary with references (not part of the question), that looks like this:

var values = new Dictionary<string, IEnumerable<string>>
{
    { "c", new string[] {  } },
    { "x", new string[] { "y" } },
    { "y", new string[] { "c", "z" } },
    //{ "y", new string[] { "c", } },
    { "z", new string[] { "x" } },
};

I use this new dictionary to validate the paths:

values.ValidateIsNotCircular();

and this throws (by design):

  CircularPathException: "Circular path ['x', 'y', 'z']!"

If you convert your dictionary into directed graph, you should be able to do a topological sorting. Kahn's algorithm works fairly well, and is easy to implement, especially if you drop all the stuff related to actual sorting.

static void ValidateIsNotCircular(Dictionary<string, IEnumerable<string>> dict)
{
    var edges = dict.SelectMany(x => x.Value.Select(y => Tuple.Create(x.Key, y))).ToList();
    var topLevelNodes = new Stack<string>(dict.Keys.Where(n => !edges.Any(e => e.Item2.Equals(n))));

    while (topLevelNodes.Any())
    {
        var node = topLevelNodes.Pop();
        var outgoingEdges = edges.Where(e => e.Item1.Equals(node)).ToArray();
        foreach(var edge in outgoingEdges)
        {
            edges.Remove(edge);
            var childNode = edge.Item2;
            var childHasIncomingEdges = edges.Any(e => e.Item2.Equals(childNode));
            if (!childHasIncomingEdges)
            {
                topLevelNodes.Push(childNode);
            }
        }
    }

    if (edges.Any())
    {
        throw new CircularPathException(edges.Select(e => e.Item1).Distinct());
    }
}

This way you don't have to deal with iterators.


Am I organising my PHP code effectively? I'm very new to learning PHP & MySQL (I've got past experience with Java) and I'm doubting whether my code is organised well. I've got an index page which has two forms; the first is a form to login and the second is a form to sign up a user. So far I've only completed the signing up of a user. Please comment on my code in general and what I can improve.

The form (table tags removed):

<form action='php/signup.php' method='post' name='signup'>
    <input name='username' type='text' />
    <input name='password' type='password' />
    <input name='signin' type='submit' value='Sign up' />
</form>

signup.php:

<?php
try {
    $db = new PDO(...);
    $db -> setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    foreach ($db->query("SELECT username FROM usertable") as $row) {
        if ($row == $username) {
            header("Location: http://example.com/#");
            return;
        }
    }
    $user = $db->prepare("INSERT INTO users (username, password) VALUES (:username, :password)");
    $user->execute(array(":username" => $_POST["username"], ":password" => $_POST["password"]));
    $db = null;
    header("Location: http://example.com/anotherpage.html");
} catch ( PDOException $e ) {
    echo $e -> getMessage();
}
?>

Iterating through the whole user table could be ineffective:

foreach ($db->query("SELECT username FROM usertable") as $row) {

You could use a WHERE username LIKE :username condition here. (And you might need a database index for the attribute as well)
The code does not check the password. I guess you should do that.
You should hash your passwords.
Calling a table usertable is a little bit redundant. I'd use simply users.
The following is hard to read because it needs horizontal scanning:

$user->execute(array(":username" => $_POST["username"], ":password" => $_POST["password"]));

I would introduce an explanatory variable and some line breaks:

$newuser_parameters  = array(
    ":username" => $_POST["username"],
    ":password" => $_POST["password"]
);
$user->execute($newuser_parameters);

From Code Complete, 2nd Edition, p761:

  Use only one data declaration per line

  [...]

  It’s easier to find specific variables because you can scan a single
  column rather than reading each line. It’s easier to find and fix
  syntax errors because the line number the compiler gives you has
  only one declaration on it.

See also: Chapter 6. Composing Methods, Introduce Explaining Variable in Refactoring: Improving the Design of Existing Code by Martin Fowler; Clean Code by Robert C. Martin, G19: Use Explanatory Variables.
I would validate at least the maximum length of the username.
This probably isn't too useful for your clients:

echo $e -> getMessage();

See #3 in my former answer.


Insertion sort implementation import java.util.Scanner;
import java.lang.Comparable;

class InsertionSort {
    private InsertionSort() {}

    private static boolean less(Comparable v, Comparable w) {
        return (v.compareTo(w) < 0);
    }

    private static void exch(Object[] a, int i, int j) {
        Object swap = a[i];
        a[i] = a[j];
        a[j] = swap;
    }

    private static void sort(Comparable[] a) {
        int length = a.length;
        for (int i = 0; i < length; i++) {
            // Maintain Invariant
            for (int j = i; j > 0; j--) {
                if (less(a[j], a[j - 1])) { // Invariant broken, fix it.
                    exch(a, j, j - 1);
                }
            }
        }
        // Postcondition: assert isSorted(a)
    }

    private static void show(Object[] a) {
        for (int i = 0; i < a.length; i++) {
            System.out.println(a[i]);
        }
    }

    private static Integer[] readAllInts() {
        Scanner scanner = new Scanner(System.in);

        int numberOfItems;
        Integer[] items;
        if (scanner.hasNext()) {
            numberOfItems = scanner.nextInt();
            items = new Integer[numberOfItems];
        } else {
            // mybe throw an error?
            return new Integer[0];
        }

        int i = 0;
        while(scanner.hasNext() && i < numberOfItems) {
            items[i] = scanner.nextInt();
            i++;
        }
        return items;
    }

    public static void main(String[] args) {
        Integer[] items = InsertionSort.readAllInts();
        InsertionSort.sort(items);
        InsertionSort.show(items);
    }
}

Above is the implementation of the insertion sort algorithm. I want learn how to write algorithms as well Clean Code. Now below are the points which I have taken care of or am concerned about.

Kept meaningful names.
Declared variables near their usage.
Suppose, this code needs to consumed by many clients then where and what exception I should handle and throw.
Kept variable scopes to as local as possible for easy GC.
Most of the methods are static because I don't think they belong to a particular object.

That's it for now, I will keep updating the list if something comes to me later.

Ref: Algorithms

readAllInts

This is a wishy washy name. What does it mean, "all ints"? In the universe? In my little black book of ints? Entered by the user on standard input? Aha!

It might be good to split this up to multiple methods:

a method to read and return the number of ints from a scanner and handle errors: print a message in case of invalid input and try again, or simply throw an exception
a method to read specified number of ints from a scanner and return as an array
a method that calls the previous two and returns the array

That way each method will have a simpler, single responsibility.

A bigger problem with this method is that it doesn't belong in the InsertionSort class. The way you read an array of integers has nothing to do with the spring algorithm.

Type safety

By not using type tokens, you sacrificed type safety. For example a call less(4, "hello") would compile, which is undesirable.

You should never use bare types, always specify type parameters.
For example you can make less type safe like this:

private static <T extends Comparable<? super T>> boolean less(T v, T w) {
    return v.compareTo(w) < 0;
}

This effectively prevents less(4, "hello") at compile time.

Correcting sort similarly:

public static <T extends Comparable<? super T>> void sort(T[] a) {
    int length = a.length;
    for (int i = 0; i < length; i++) {
        for (int j = i; j > 0; j--) {
            if (less(a[j], a[j - 1])) { // Invariant broken, fix it.
                exch(a, j, j - 1);
            }
        }
    }
}

And that's it, the other methods are fine,
as they don't use bare types, and they are fine using Object[] for swapping and printing.

Naming

exch is not a great name. " exchange " would be better, but the most common name for this operation is "swap".

less is also not great, as some might interpret it in the sense of " minus ". I suggest "lessThan" instead, which makes the comparison logic perfectly clear.

The variable a is the worst offender of all. "swap" is not great either.


Finding maximum length of continuous string which has same characters I'm solving this problem from codeforces. I've a solution which has complexity of O(n²). But the time limit exceeds on some of the test cases. The problem statement is:

  High school student Vasya got a string of length n as a birthday present. This string consists of letters 'a' and 'b' only. Vasya denotes beauty of the string as the maximum length of a substring (consecutive subsequence) consisting of equal letters.

  Vasya can change no more than k characters of the original string. What is the maximum beauty of the string he can achieve?

  Input

  The first line of the input contains two integers n and k (1 ≤ n ≤ 100 000, 0 ≤ k ≤ n) — the length of the string and the maximum number of characters to change.

  The second line contains the string, consisting of letters 'a' and 'b' only.

  Output

  Print the only integer — the maximum beauty of the string Vasya can achieve by changing no more than k characters.
  Examples

  Sample Input/Output:

  8 1

  aabaabaa

  Output

  5

My solution is:

import java.util.Scanner;
public class CF676C {
public static void main(String args[]){
    Scanner in = new Scanner(System.in);
    int n = in.nextInt(), max = in.nextInt(),a=0,b=0;

    String s = in.next();
    int i,j;
    for(i=0;i<n;i++){
        if(s.charAt(i) == 'a'){
            ++a;
        }
        else{
            ++b;
        }
    }
    char z;
    if(a<=b){
        z='a';
    }
    else{
        z='b';
    }
    int max1;
    int count=0;
    int temp=0;
    for(i=0;i<s.length();i++)
    {
        max1=max;
        count=0;
        for(j=i;j<s.length()&&max1!=-1;j++)
        {
            if(s.charAt(j)==z&&max1!=-1)
            {
                max1--;
            }
            if(max1!=-1)
            {
                count++;
            }
        }
        if(count>temp)
        {
            temp=count;
        }
    }
    System.out.println(temp);
}
}

I can't understand how to reduce time taken by the code. Is there any other solution or approach available?

Yes, you can tackle this problem is a much faster way, with an O(n) algorithm, using a left and a right pointer delimiting the substrings to consider.

For each character in the input String, the idea is to keep an array counting the number of 'a' and 'b' between a given left pointer, that will represent the start of the substring, and the current character.

When the count of each 'a' or of each 'b' is less than the allowed number of characters to change, we can consider making the change and incrementing the answer. Then, when both of those 2 counts becomes greater than the allowed number of characters to change, it means we cannot change more characters so we hit a maximum length: the left pointer is increased, and the count of the character at that pointer is decreased.

The count of 'a' and 'b' characters can be modeled as an integer array (index 0 corresponds to the count of 'a' while index 1 corresponds to the count of 'b').

Scanner in = new Scanner(System.in);
int n = in.nextInt(), max = in.nextInt();
String s = in.next();

int left = 0, answer = 0;
int[] count = { 0, 0 };
for (char c : s.toCharArray()) {
    count[c - 'a']++;
    if (Math.min(count[0], count[1]) > max) {
        count[s.charAt(left) - 'a']--;
        left++;
    } else {
        answer++;
    }
}
System.out.println(answer);

This approach passes all the tests of the challenge.


Avoiding race in a sub-millisecond TOD timer Part of the project I am working on is to maintain the time-of-day, at the sub-millisecond accuracy, while not overwhelming CPU.
The controller has a pretty standard timer, with two registers, COUNTER and PRELOAD. On each system tick COUNTER is decremented; once it reaches 0, the timer posts an interrupt, and reloads COUNTER with the value from PRELOAD.
The value in PRELOAD is calculated off-line, to let the counter expire (and post the interrupt), say, every 100 milliseconds. COUNTER is readable at any time, which allows for a sum-millisecond precision. Yet another timer register, STATUS, has a TIMER_STATUS_IF bit, which determines the state of the timer interrupt.
The timer's registers are memory mapped. Their definitions (along with the definition of timer) come from the manufacturer's #include file. The actual address map is irrelevant.
Here is the code:
static volatile uint32_t ticks;

void ISR timer_isr() {
    ticks++;
    timer->STATUS &= ~TIMER_STATUS_IF;
}

uint64_t usecs() {
    uint32_t counter1 = timer->COUNTER;
    uint32_t ticks1 = ticks;
    uint32_t counter2 = timer->COUNTER;
    uint32_t ticks2 = ticks;

    if (timer->STATUS & TIMER_STATUS_IF) {
        // The interrupt has been asserted, and the counter has been reloaded,
        // but the ISR has not been run (we are called from the higher priority
        // ISR). Increment ticks manually.
        assert(ticks1 == ticks2);
        ticks2 += 1;
    } else if (counter1 > counter2) {
        // We know that between reading counters the reload did not happen.
        // Trust the first reading of ticks.
        ticks2 = ticks1;
    }
    else { // Nothing happens here. It is purely an explanation.
        // We know that between reading counters the reload did happen.
        // Trust the second reading of ticks.
    }

    return ticks2 * US_PER_TICK + (timer->PRELOAD - counter2) / CLOCKS_PER_USEC;
}

This works fine unless some high priority ISR takes more than 100 ms to complete. I am fine with it. If any ISR takes that long, it is much more serious design problem.
What I don't like here.
I am a strong advocate of a comment-less style. To quote myself, "an explanatory comment means that you fail to express the intention in the code". However, I feel I need a large explanatory comment. Any hint on how to remove it is much appreciated.
TL;DR
For a bit of context, the code I inherited (not for a review)

uint64_t usecs() {
    uint32_t counter = timer->COUNTER;
    uint32_t ticks = ticks;
    return ticks * US_PER_TICK + (timer->PRELOAD - counter) / CLOCKS_PER_USEC;
}

once in a while exhibited time travel for about 100 ms into the future.

Carefully examine all possible interactions between interrupts
There are many possible ways interrupts can happen, you should take some time and carefully try to map them out. I think you missed a few corner cases in case where usecs() is called from a higher priority context than timer_isr(). First, given your description, the function usecs() can be called either from a low or a high priority context:
Low priority call to usecs()
It's either not called from an interrupt context, or from an interrupt with a lower priority than the timer overflow interrupt. In this context, usecs() might be interrupted by timer_isr() at any point during its execution. So look at all the possible places where the timer_isr() could fire:
// 1
uint32_t counter1 = timer->COUNTER;
// 2
uint32_t ticks1 = ticks;
// 3
uint32_t counter2 = timer->COUNTER;
// 4
uint32_t ticks2 = ticks;
// 5

Before I am going to look at all 5 possibilities, I'm also going to make some assumptions:

Any higher interrupts together will never take more than 100 milliseconds. This should be reasonable, and avoids us having to worry about the timer overflowing twice while usecs() runs.
The COUNTER effectively gets reset to PRELOAD by the hardware during the interrupt to timer_isr(). You must verify this by reading the documentation of the MCU.

So now for the five possibilities of where the timer reset happens, and after the four statements have run the results will be:

ticks1 == ticks2 and counter1 > counter2 (trivial case)
ticks1 == ticks2 and counter1 < counter2 (only counter2 is valid)
ticks1 != ticks2 and counter1 < counter2 (either ticks1+counter1 or ticks2+counter2 are valid)
ticks1 != ticks2 and counter1 > counter2 (only ticks1 is valid)
ticks1 == ticks2 and counter1 > counter2 (trivial case)

It looks like your code handles all these cases correctly.
High priority calls to usecs()
Here we can never be that timer_isr() interrupts usecs(), but it could be that usecs() interrupted timer_isr(). Let's look at those cases separately.
usecs() did not interrupt timer_isr()
The question is now, what happens when the timer resets? Again, check the documentation of your MCU, but I am assuming that COUNTER will be reset to PRELOAD, just that timer_isr() doesn't fire because we are already in a higher level interrupt. So now ticks1 == ticks2 will always be true. We still have the five positions where the reset can happen:

counter1 > counter2 (ticks1 and ticks2 are behind)
counter1 < counter2 (ticks2 is behind)
counter1 < counter2 (ticks2 is behind)
counter1 > counter2 (everything is valid)
counter1 > counter2 (everything is valid)

Oops, you cannot distinguish case 1 from cases 4 and 5 just by looking at the counter values. The TIMER_STATUS_IF flag also doesn't help in distinguishing what happened, at least not if you check it after reading the counters. So you have to read the counter again:
if (timer->STATUS & TIMER_STATUS_IF) {
    // The counters wrapped somewhere prior to exactly this if-statement.
    // We assume it won't wrap again from here, because all interrupts will
    // be handled in less than 100 ms.
    counter2 = timer->COUNTER;
    ticks2 += 1;
} ...

usecs() did interrupt timer_isr()
In a way this looks simpler, because we know the counter reset just before usecs() was called, so we don't have to worry about the counters being able to reset during usecs(). However, now we have to worry about where exactly in timer_isr() we had the higher level interrupt. Again there are several possibilities:
// 1
ticks++;
// 2
timer->STATUS &= ~TIMER_STATUS_IF;
// 3

Oops, there is no way to distinguish case 1 from case 2 from usecs()! Reordering statements won't help here, there will always be a point right before and one right after the increment. The only way I see is to ensure both ticks++ and timer->STATUS &= ~TIMER_STATUS_IF are executed atomically. Perhaps this can be done by having timer_isr() disable higher level interrupts:
cli(); // disable all interrupts
ticks++;
timer->STATUS &= ~TIMER_STATUS_IF;
sei(); // reenable interrupts

This should make usecs() work correctly.


Linq query performs poor according to performance analyzer in VS2012 I've a Linq query that performs poorly:

var minuteValues = ValuesInMemory.Where(o => o.Key.Day == tmpValue.QuoteDate.Day &&
                                  o.Key.Hour == tmpValue.QuoteDate.Hour &&
                                  o.Key.Minute == tmpValue.QuoteDate.Minute &&
                                  o.Key.Second >= 0).Select(p => p.Value).ToList();

Is there a way to do this better?

private readonly ConcurrentDictionary<DateTime, double> ValuesInMemory = new ConcurrentDictionary<DateTime, double>();

ValuesInMemory holds about 6700 records 24/7. Every second records are added and every 2 hours the oldest values are stored in SQL Server. But the average amount of values is 6700.

I think the crux of your problem here is you are using the wrong tool for the job. The purpose of a dictionary is usually for direct retrieval i.e. give me the value associated with X key. I'm not sure how optimized they are for O(n) queries.

To rule out the ConcurrentDictionary<TKey, TValue> being the bottleneck, you could try switching it out for something like a ConcurrentBag<T>.


Pandoc filter to allow embedding arbitary HTML in latex documents I wrote a Pandoc filter that allows me to embed arbitrary HTML snippets in LaTeX documents, which are then printed out as-is when the document is converted to HTML. It is slightly more generic than it needs to be right now, in case I want to embed other types of things in the future.

It can be used like:

\begin{verbatim}
    %%% html
    <iframe width="560" height="315" src="https://www.youtube.com/embed/dQw4w9WgXcQ" frameborder="0" allowfullscreen></iframe>
\end{verbatim}

In particular, I think the chain of >>= in convertCode could be improved some how, possibly using Applicatives. runReader basically does nothing except rearrange its arguments and call a thing.

import Data.Char ( isSpace )
import Data.List ( dropWhileEnd )
import Data.Maybe ( fromMaybe )

import Text.Pandoc.JSON ( Block(CodeBlock, RawBlock), Format(Format), toJSONFilter )

main = toJSONFilter convertCodeBlocks

type Reader = String -> Maybe [Block]

convertCodeBlocks :: Block -> [Block]
convertCodeBlocks b = case b of
    (CodeBlock _ code) -> fromMaybe [b] $ convertCode code
    _                  -> [b]

-- Given a string of code, try to convert it into something nicer
convertCode :: String -> Maybe [Block]
convertCode code =
    codeBlockType codeHead >>= codeBlockReader >>= runReader codeTail
  where (codeHead, codeTail) = splitWhen (== '\n') $ strip code

-- Get the reader name from the magic header
codeBlockType :: String -> Maybe String
codeBlockType line = if percent == "%%% "
    then Just $ strip typeName
    else Nothing
  where (percent, typeName) = splitAt 4 $ strip line

-- All the known readers and their name from the magic header
codeBlockReader :: String -> Maybe Reader
codeBlockReader "html" = Just rawHtml
codeBlockReader _      = Nothing

-- Make a raw HTML block from some string
rawHtml html = Just [RawBlock (Format "html") html]

-- A mostly useless function, only here because I can't make Applicatives work
runReader :: String -> Reader -> Maybe [Block]
runReader code reader = reader code

-- Strip whitespace from the start and end of a string
strip = dropWhileEnd isSpace . dropWhile isSpace

-- Split an array at a predicate, returning (before, after)
splitWhen :: (a -> Bool) -> [a] -> ([a], [a])
splitWhen pred [] = ([], [])
splitWhen pred (x:xs) = if pred x
    then ([], xs)
    else let (before, after) = splitWhen pred xs in (x:before, after)

I initially implemented something for this as a modification to Pandoc itself (See this PR on Github), but the maintainer suggested a filter instead.

I think it's clearer to just merge all those functions into one since you only really care about one case:

convertCode :: String -> Maybe [Block]
convertCode code =
  case strip codeHead of
    ('%':'%':'%':' ':typeName) -> matchType $ strip typeName
    _                          -> Nothing
 where
   (codeHead, codeTail) = splitWhen (== '\n') $ strip code

   matchType "html" = Just [RawBlock (Format "html") codeTail]
   matchType _      = Nothing

Does this do the same thing? I cant test it without too much effort on my part.


Get top row for muliple registers I came across a piece of code that generates SELECT queries from a template.

The template:

SELECT TOP 1 *
FROM   MRH_0000004 WITH (nolock)
WHERE  MachineRegisterId = ${id}
ORDER  BY UtcTime

When the code has to retrieve multiple records, it generates one large query that gets the union of all individual queries based on the template above.

Example:

SELECT *
FROM   (SELECT TOP 1 *
        FROM   mrh_0000004 WITH (nolock)
        WHERE  machineregisterid = 55
        ORDER  BY utctime) q
UNION ALL
SELECT *
FROM   (SELECT TOP 1 *
        FROM   mrh_0000004 WITH (nolock)
        WHERE  machineregisterid = 56
        ORDER  BY utctime) q
UNION ALL
SELECT *
FROM   (SELECT TOP 1 *
        FROM   mrh_0000004 WITH (nolock)
        WHERE  machineregisterid = 57
        ORDER  BY utctime) q

This is a query that gets the TOP 1 result from each group, without using a GROUP BY clause. These chains can get quite long, resulting in large queries from the client to the server.

Can this query be refactored using GROUP BY?

Table schema:

CREATE TABLE [dbo].[mrh_0000004]
  (
     [machineregisterid] [INT] NOT NULL,
     [utctime]           [DATETIME] NOT NULL,
     [value]             [FLOAT] NOT NULL,
     CONSTRAINT [pk_MRH_0000004] PRIMARY KEY CLUSTERED ( [machineregisterid] ASC
     , [utctime] ASC )
  )

WITH (nolock)

While there is nothing inherently wrong with using WITH (nolock) per-se, it's mostly meant to be used within transactions that have an isolation level that does cause locks, to mark that specific table as an exception to the locking. If your whole transaction is meant to not have any locks, just declare this at the top:

SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED;

Use variables

This will potentially help you make the code more readable and flexible.

--Using a variable for readability and flexibility
--This would make it easier also to turn into a script
--like a stored procedure in the future
DECLARE @top INT;
SET @top = 1;

Then you can do things like SELECT TOP (@top) * FROM ... and that will allow to easily adapt it if you want top 3, top 5, 10 etc. instead of just the top-most row.

Approach

As thoroughly explained and illustrated in Retrieving n rows per group on DBA.StackExchange site, there are two common ways of doing this which are simpler and much more efficient than UNION ALL.

Using ROW_NUMBER()

Per MSDN:

  Returns the sequential number of a row within a partition of a result set, starting at 1 for the first row in each partition.

As applied to your query:

SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED;
DECLARE @top INT;
SET @top = 1;

WITH grouped AS (
    SELECT
      ROW_NUMBER() OVER
        --This is equivalent to GROUP BY + ORDER BY combined
        --where the first item in each group/partition will have
        --row number 1 and so on, based on the ORDER BY clause
        (PARTITION BY machineregisterid
        ORDER BY utctime) AS row_num
      , *
    FROM mrh_0000004
)
SELECT * FROM grouped
--Using <= in case @top was greater than 1
WHERE row_num <= @top;

Using APPLY

According to TechNet:

  The APPLY operator allows you to invoke a table-valued function for each row returned by an outer table expression of a query.

Again, as applied to your query:

SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED;
DECLARE @top INT;
SET @top = 1;

SELECT
    mrh1.machineregisterid,
    mrh2.utctime,
    mrh2.[value] --using brackets because VALUE is an ODBC reserved keyword
FROM mrh_0000004 AS mrh1
CROSS APPLY (
    SELECT TOP (@top) *
    FROM mrh_0000004
    WHERE machineregisterid = mrh1.machineregisterid
    ORDER BY utctime
) AS mrh2
ORDER BY machineregisterid, utctime;


Porting functional programming code to Linq I am very new to C# programming, so for practice I thought I would try to port a simple haskell function over to Linq. From learn you a haskell, there is the following implementation of powerset:

powerset :: [a] -> [[a]]
powerset xs = filterM (\x -> [True, False]) xs

Where filterM is defined like so

filterM :: (Monad m) => (a -> m Bool) -> [a] -> m [a]
filterM _ []     = return []
filterM p (x:xs) = do
  flg <- p x
  ys  <- filterM p xs
  return (if flg then x:ys else ys)

De-sugaring the do notation and specializing to m = list, this is

filterM :: (a -> [Bool]) -> [a] -> [[a]]
filterM _ []     = [[]]
filterM p (x:xs) =
  p x >>= (\flg ->
    filterM p xs >>= (\ys ->
      if flg then [x:ys] else [ys]))

So, porting the above code to Linq, this is what I have:

//prepend taken from a stackexchange post
static class IEnumerableExtensions
{
    static IEnumerable<T> Prepend<T>(this IEnumerable<T> seq, T val)
    {
        yield return val;
        foreach (T t in seq)
        {
            yield return t;
        }
    }
}

class Program
{
    static List<List<T>> FilterND<T>(Func<T, List<bool>> preds, List<T> xs)
    {
        if (xs.Any())
        {
            //unrolling haskell do notation.
            //>>= becomes SelectMany
            return
            preds(xs.First()).SelectMany(b =>
            FilterND(preds, xs.Skip(1).ToList()).SelectMany(ys =>
                b ?
                new List<List<T>> { ys.Prepend(xs.First()).ToList() } :
                new List<List<T>> { ys }
            ))
            .ToList();
        }
        else
        {
            return new List<List<T>> { new List<T> { } }; //[[]]
        }
    }

    static List<List<T>> Powerset<T>(List<T> xs)
    {
        Func<T, List<bool>> preds = (x => new List<bool> { true, false });
        return FilterND<T>(preds, xs);
    }

    static void Main(string[] args)
    {
        var test = new List<int>() {1, 2, 3, 4, 5};
        var pow = Powerset(test);
        foreach (var xs in pow)
        {
            Console.Write("[");
            foreach (var x in xs)
            {
                Console.Write(x);
            }
            Console.Write("]\n");
        }
    }
}

My main questions are:

How do I go about making things lazy in the correct places?
In what ways should I be qualifying my input (ref, out, etc)?
Is it even possible to make this sort of code run fast?

I've hardly written a line of Haskell in my life... but hopefully that isn't too important since you're happy with plain old concrete types here.

As Adriano Repetti said in his comment, working with IEnumerable<T> rather than List<T> - or pretty much anything else - allows for lazy evaluation: your Prepend extension method is a beautiful example of this already. Most implementations of IEnumerable are nice enough to not break when you try to use them in funky ways through LINQ, so don't worry about that. If you are worried about that, then just .ToArray() the input (or use an IReadOnlyList<T>) so that you know you can trust the input.

Doing a quick job of replacing all those lists with IEnumerable<T>s and removing the calls to .ToList() gives this:

private static IEnumerable<IEnumerable<T>> FilterND<T>(Func<T, IEnumerable<bool>> preds, IEnumerable<T> xs)
{
    if (xs.Any())
    {
        return
            preds(xs.First()).SelectMany(b =>
                FilterND(preds, xs.Skip(1)).SelectMany(ys =>
                    b
                        ? new [] { ys.Prepend(head) }
                        : new [] { ys }
                    )
                );
    }
    else
    {
        return new [] { Enumerable.Empty<T>() };
    }
}

Note that I use arrays for 'throw away' fixed-length enumerables: they will be (marginally) faster, and provide a much tidier syntax. Enumerable.Empty<T>() is used rather than new T[0] or new T[] { } just because it is absolutely clear that I just want something empty.

Prepend is lazy, so is SelectMany: this whole package is now lazy. You can kind-of ratify this by putting a print line in Prepend, and writing the following code:

var ps = PowersetNess.PowerSet(new[] { 1, 2, 3 });

int si = 0;
foreach (var s in ps)
{
    Console.WriteLine(si++ + "\t" + string.Join(", ", s));
}

Without the print in Prepend the output will look like this:

0       1, 2, 3
1       1, 2
2       1, 3
3       1
4       2, 3
5       2
6       3
7

With the prepend, we can see that the calls are interspersed everywhere, which implies that something is being lazy.

Prepend!
Prepend!
Prepend!
0       1, 2, 3
Prepend!
Prepend!
1       1, 2
Prepend!
Prepend!
2       1, 3
Prepend!
3       1
Prepend!
Prepend!
4       2, 3
Prepend!
5       2
Prepend!
6       3
7

Naturally, if you insert those .ToList() calls back in, then you will find all the Prepend!s at the top, because you are forcing evaluation and caching.

Now indulge me while I pull out a couple of variables and rename b to includeHead...

private static IEnumerable<IEnumerable<T>> FilterND<T>(Func<T, IEnumerable<bool>> preds, IEnumerable<T> xs)
{
    if (xs.Any())
    {
        T head = xs.First();
        IEnumerable<T> tail = xs.Skip(1);

        return
            preds(head).SelectMany(includeHead =>
                FilterND(preds, tail).SelectMany(ys =>
                    includeHead
                        ? new [] { ys.Prepend(head) }
                        : new [] { ys }
                    )
                );
    }
    else
    {
        // just the empty set
        return new [] { Enumerable.Empty<T>() };
    }
}

Perhaps it is somehow clearer in Haskell to use >>= rather than a classic map, but the inner SelectMany can just be a Select, which will improve performance and reduce clutter rather.

private static IEnumerable<IEnumerable<T>> FilterND<T>(Func<T, IEnumerable<bool>> preds, IEnumerable<T> xs)
{
    if (xs.Any())
    {
        T head = xs.First();
        IEnumerable<T> tail = xs.Skip(1);

        return
            preds(head).SelectMany(includeHead =>
                FilterND(preds, tail).Select(ys =>
                    includeHead
                        ? Cons(head, ys)
                        : ys
                    )
                );
    }
    else
    {
        // just the empty set
        return new [] { Enumerable.Empty<T>() };
    }
}

You can also externalise the includeHead check, which will of course reduce overhead, and (I think) makes the whole method much more understandable.

private static IEnumerable<IEnumerable<T>> FilterND<T>(Func<T, IEnumerable<bool>> preds, IEnumerable<T> xs)
{
    if (xs.Any())
    {
        T head = xs.First();
        IEnumerable<T> tail = xs.Skip(1);

        return
            preds(head).SelectMany(includeHead =>
                includeHead
                    ? FilterND(preds, tail).Select(ys => Cons(head, ys))
                    : FilterND(preds, tail)
                );
    }
    else
    {
        // just the empty set
        return new [] { Enumerable.Empty<T>() };
    }
}

To answer your question 2: you don't need or want any out, ref, or in here. You could write a Decons method with out params to cleanly extract the head and tail of a list (shown below for fun), but it's probably not worth it, and doesn't have the same typing gaurentees as proper pattern matching.

public static bool Decons<T>(this IEnumerable<T> list, out T head, out IEnumerable<T> tail)
{
    if (list.Any())
    {
        head = list.First();
        tail = list.Skip(1);
        return true;
    }
    else
    {
        head = default(T);
        tail = null;
        return false;
    }
}

If I was using such a method in 'real' code it would be called TryDeconstruct or something. The pattern matching feature that are slowly being fed into C# may one day allow code looks like if (vs is (head, tail)), but I don't keep on top of that stuff.

Here is a (in my opinion very readable) version which uses a pair of nested foreach loops, and the Decons method.

private static IEnumerable<IEnumerable<T>> FilterND<T>(Func<T, IEnumerable<bool>> preds, IEnumerable<T> xs)
{
    if (xs.Decons(out var head, out var tail))
    {
        foreach (var includeHead in preds(head))
        {
            foreach (var partialSubSet in FilterND(preds, tail))
            {
                if (includeHead)
                    yield return partialSubSet.Prepend(head);
                else
                    yield return partialSubSet;
            }
        }
    }
    else
    {
        // just the empty set
        yield return Enumerable.Empty<T>();
    }
}

I hope that flood of C# was of some interest!

One final thing: you can tidy the definition of preds a bit (should it be predicates?) by using a local function.

Func<T, List<bool>> preds = (x => new List<bool> { true, false });
bool[] predicates(T x) => new bool[] { true, false };

This is just a matter of taste really (slightly surprised you didn't go with return FilterND<T>(x => new bool[] { true, false }, xs);). You could reduce the number of allocations by creating a single bool[] and always returning it, if performance is a serious concern, but this is the wrong place to be looking for a performance advantage.

P.S.: thanks for posting this, I had a lot of fun with it


Backbone collection filter I've got this Collection in my Backbone application:

var AnswersCollection = Backbone.Collection.extend({
    model: Answer,

    initialize: function() {
        console.log('Hello from new answers collection');
    },

    getCorrect: function() {
        return this.where({correct: true});
    },

    getWrong: function() {
        return this.where({correct: false});
    },

    randomSet: function(correct, wrong) {
        arr1 = _.sample(this.getCorrect(), correct);
        arr2 = _.sample(this.getWrong(), wrong);
        result = _.shuffle(arr1.concat(arr2));
        coll = new AnswersCollection(result)
        return coll;
    }

});

This works, and randomSet is called by a View when config have certain options or user click on 'rerender' button, but I'm wondering that maybe I can improve this code ?
The flow looks like this

Get randomly selected X correct answers
Get randomly selected Y wrong answers
Shuffle all answers
Create array of answers, that can be passed to new Collection

This code doesn't look to cool, maybe you will have some ideas how to make It cooler?:)

Reviewing code for coolness is a first ;)

Some observations:

initialize : if really all you want to do there is Hello World, then you should take it out. initialize is meant for initializing your model with answers.
getCorrect and getWrong are short one-liners that are called once, you should inline them
arr1, arr2, coll and even result are pretty terrible names, how about correctAnswers, wrongAnswers, answers and not creating coll since you could return the results of new AnswersCollection immediately.
The most serious problem though is that you are not declaring the prior mentioned variables with var, that is bad.

Also, from a coolness perspective, braces on their own line is where it's at.

I would counter-propose this:

var AnswersCollection = Backbone.Collection.extend(
{
    model: Answer,

    randomSet: function(correct, wrong)
    {
        var correctAnswers = _.sample(this.where({correct: true}), correct),
            wrongAnswers = _.sample(this.where({correct: false}), wrong),
            answers = _.shuffle(correctAnswers.concat(wrongAnswers));

        return new AnswersCollection( answers );
    }
});


Array data-handling with NPC class I am wondering about the best way to save on RAM/memory when there are many required arrays (I plan to have many more NPCs and various other arrays with text and items). And I am wondering what I can improve on so far.

  #include <iostream>
  #include <cstring>

  #define NUM_NPC 10       // Number of NPC players

  #define DETAILS_HEARTS 0 // Heart Position
  #define DETAILS_BDMON  1 // Birthday Month Position
  #define DETAILS_BDDAY  2 // Birthday Day Position
  #define DETAILS_GENDER 3 // NPC Gender

  using namespace std;

  class npcPlayers {
     public:
        int getHearts(int playerID);
        int getItem(int playerID, int itemID);
        int getBirthdayMonth(int playerID);
        int getBirthdayDay(int playerID);
        char *getName(int playerID);
        npcPlayers();
     private:
        int npcDetails[NUM_NPC][10];
        char npcNames[NUM_NPC][20];
        int updateHearts(int PlayerID,int value);
  };

  npcPlayers::npcPlayers() {
     npcNames = {"Ace","Amber","Benny","Ethan","Michael","Jay","William","Mia","Chloe","Emily"};
     npcDetails = {
        {0,3,5,0},    // Ace
        {0,9,20,1},   // Amber
        {0,1,10,0},   // Bennny
        {0,12,30,0},  // Ethan
        {0,2,17,0},   // Michael
        {0,6,10,0},   // Jay
        {0,8,9,0},    // William
        {0,4,1,1},    // Mia
        {0,5,13,1},   // Chloe
        {0,8,5,1}     // Emily
     };
  }

  int npcPlayers::getHearts(int playerID) {
     return npcDetails[playerID][DETAILS_HEARTS];
  }
  int npcPlayers::getItem(int playerID, int itemID) {

  }
  int npcPlayers::getBirthdayDay(int playerID) {
     return npcDetails[playerID][DETAILS_BDDAY];
  }
  int npcPlayers::getBirthdayMonth(int playerID) {
     return npcDetails[playerID][DETAILS_BDMON];
  }
  char *npcPlayers::getName(int playerID) {
     return npcNames[playerID];
  }

  int main() {
     npcPlayers npc;
     cout << "Player 1 Details:" << endl;
     cout << "Name: " << npc.getName(1) << endl;
     cout << "Hearts: " << npc.getHearts(1) << endl;
     cout << "Birthday: " << npc.getBirthdayMonth(1) << "/" << npc.getBirthdayDay(1) << endl;
     return 0;
  }

C++ is statically typed programming language. Due to this overhead for allocation of class members is very small.
Using of global variables is not a good idea. Such vars like NUM_NPC makes your application non-scalable.
Each domain object (like a player or an item) must be described by a same-named class.

#include <iostream>
#include <string>
#include <vector>

typedef int Hearts;
typedef bool Gender;

struct Date
{
      Date(int month,int day) : month(month), day(day) {}
      int month;
      int day;
};

class Item
{
    // TODO: implement
};

class Player
{
public:
      Player(std::string name, Hearts hearts, Date birthday, Gender gender)
            : name(name), hearts(hearts), birthday(birthday), gender(gender) {}

      Hearts getHearts() const
      {
            return hearts;
      }

      const std::vector<Item>& getItems()
      {
            return items;
      }

      Date getBirthday() const
      {
            return birthday;
      }

      std::string getName() const
      {
            return name;
      }
private:
      std::vector<Item> items;
      Hearts hearts;
      Date birthday;
      std::string name;
      Gender gender;
};

int main()
{
      using namespace std;

      vector<Player> players;
      players.push_back(Player("Ace", 0, Date(3,5), 0));
      players.push_back(Player("Amber", 0, Date(9,20), 1));
      players.push_back(Player("Benny", 0, Date(1,10), 0));
      players.push_back(Player("Ethan", 0, Date(12,30), 0));

      Player& p = players[1];
      cout << "Player 1 Details:" << endl;
      cout << "Name: " << p.getName() << endl;
      cout << "Hearts: " << p.getHearts() << endl;
      cout << "Birthday: " << p.getBirthday().month << "/" << p.getBirthday().day << endl;
      cout << "Item count: " << p.getItems().size() << endl;
}


LeetCode 809: Expressive Words I'm posting my C++ code for LeetCode's Expressive Words. If you have time and would like to review, please do so. Thank you!

Sometimes people repeat letters to represent extra feeling, such as
"hello" -> "heeellooo", "hi" -> "hiiii".  In these strings like
"heeellooo", we have groups of adjacent letters that are all the same:
"h", "eee", "ll", "ooo".
For some given string S, a query word is stretchy if it can be made to
be equal to S by any number of applications of the following extension
operation: choose a group consisting of characters c, and add some
number of characters c to the group so that the size of the group is 3
or more.
For example, starting with "hello", we could do an extension on the
group "o" to get "hellooo", but we cannot get "helloo" since the group
"oo" has size less than 3.  Also, we could do another extension like
"ll" -> "lllll" to get "helllllooo".  If S = "helllllooo", then the
query word "hello" would be stretchy because of these two extension
operations: query = "hello" -> "hellooo" -> "helllllooo" = S.
Given a list of query words, return the number of words that are
stretchy.
Example: Input:  S = "heeellooo" words = ["hello", "hi", "helo"]
Output: 1 Explanation:  We can extend "e" and "o" in the word "hello"
to get "heeellooo". We can't extend "helo" to get "heeellooo" because
the group "ll" is not size 3 or more.
Notes:
0 <= len(S) <= 100.
0 <= len(words) <= 100.
0 <= len(words[i]) <= 100.
S and all words in words consist only of lowercase letters

Accepted C++
class Solution {
public:
    int expressiveWords(std::string base_string, std::vector<string> &words) {
        int count = 0;

        for (auto &word : words)
            if (is_stretchable(base_string, word))
                count++;

        return count;
    }

protected:
    bool is_stretchable(std::string base_string, std::string words) { // 4 pointers
        int base_length = base_string.size();
        int words_length = words.size();
        int left_a = 0, right_a = 0;

        for (int left_b  = 0, right_b = 0; left_a < base_length && right_a < words_length; left_a = left_b, right_a = right_b) {
            if (base_string[left_a] != words[right_a])
                return false;

            while (left_b < base_length && base_string[left_b] == base_string[left_a])
                left_b++;

            while (right_b < words_length && words[right_b] == words[right_a])
                right_b++;

            if (left_b - left_a != right_b - right_a && left_b - left_a < std::max(3, right_b - right_a))
                return false;
        }

        return left_a == base_length && right_a == words_length;
    }
};

Reference
On LeetCode, there is an instance usually named Solution with one or more public functions which we are not allowed to rename those.

Solution
Discussion
Question

Pass by const reference to prevent copying and modification:
int expressiveWords(std::string const& base_string, std::vector<string> const& words)
                                ^^^^^^                                  ^^^^^^

bool is_stretchable(std::string const& base_string, std::string const& words)
                                ^^^^^^                          ^^^^^^

    for (auto const& word : words)
              ^^^^^^

This prevents the strings being copied into the function. It will also catch situations were you accidentally try and modify the string.

Please add braces {} around sub blocks.
It will save you one day without you even knowing. Issues caused by missing braces are notoriously hard to find because they are caused when sombody else does something nasty that affects your code (multi line macros come to mind).

One variable per line:
   int left_a = 0, right_a = 0;

Its not going to hurt you to add an extra line and it makes the job easier for the next person.

Not sure it is worth it.
But just to be complete you could use a standard algorithm to count.
    int count = 0;

    for (auto &word : words)
        if (is_stretchable(base_string, word))
            count++;

    return count;

    // ---

    return std::count(std::begin(words), std::end(words),
                      [&base_string, this](auto const& word){return is_stretchable(base_string, word);});