Count the number of biggest numbers on the board I coded two answers to this challenge. The first one was a standard attempt, but after it timed out, I figured out the "trick" to solving the problem much more simply. However, I'd still appreciate comments and critiques on my first attempt, because there will be other problems that will need a long approach and I'd like to improve how I'd code the answer.

Challenge
Julia is playing a game on an infinite 2-dimensional grid with the bottom left cell referenced as (1, 1). All the cells contain a value of zero initially. The game consists of n steps. In each step, Julia is given two integers a and b. The value of each of the cells in the coordinate (u, v) satisfying 1 ≤ u ≤ a and 1 ≤ v ≤ b, is increased by 1. After n such steps, if x is the largest number in any cell on the board, how many instances of x are there on the board?

Complete the function countX that has one parameter, a string array, steps, denoting the values of a and b for each of steps of the game. The function should return the total number of occurrences of greatest integer x in the grid after n steps.

Sample Input and Output *
Input: [ '18 29', '32 17', '34 9', '38 15', '36 22', '7 14', '5 100' ]
Output: 2

First Attempt

function countX(stepArr){
    let board = []; //2 dimensional array
    let bigNum = 0;
    let bigNumCount = 0;

    const stepArrCount = stepArr.length;

     for(let k = 0; k < stepArrCount; k++){
        const sArr = stepArr[k].split(" ");
        let s1 = sArr[0]; //vertical steps
        let s2 = sArr[1]; //horiz steps

        for(let i = 0; i < s1; i++){
            let stopCompare = false;
            for(let j = 0; j < s2; j++){
                if(board[i] !== undefined){
                    if(board[i][j] !== undefined){
                        board[i][j]++;
                    }
                    else{
                        board[i][j] = 1;
                    }
                }
                else{
                    board[i] = [];
                    board[i][j] = 1;
                }
                if((k + 1 === stepArrCount) && !stopCompare){
                    if(board[i][j] > bigNum){
                        bigNum = board[i][j];
                        bigNumCount = 1;
                    }
                    else if(board[i][j] === bigNum){
                        bigNumCount++;
                    }
                    else{
                        stopCompare = true;
                    }
                }
            }
        }
     }

    return bigNumCount;
}

Second Attempt

function countX(stepArr){
    let sArr = stepArr[0].split(" ");
    let smallColA = parseInt(sArr[0]);
    let smallColB  = parseInt(sArr[1]);

    if(sArr.length > 1){
        for(let i = 1; i < stepArr.length; i++){
            sArr = stepArr[i].split(" ");
            let s1 = parseInt(sArr[0]);
            let s2 = parseInt(sArr[1]);
            if(s1 < smallColA){
                smallColA = s1;
            }
            if(s2 < smallColB){
                smallColB = s2;
            }
        }
    }

    return smallColA * smallColB;

}

* Explanation of Input and Output (provided by challenger)
Assume that the following board corresponds to cells (i, j) where 1 ≤ i ≤ 4 and 1 ≤ j ≤ 7.
At the beginning the board is in the following state:

0 0 0 0 0 0 0
0 0 0 0 0 0 0
0 0 0 0 0 0 0
0 0 0 0 0 0 0

After the first step we obtain:

0 0 0 0 0 0 0
0 0 0 0 0 0 0
1 1 1 0 0 0 0
1 1 1 0 0 0 0

After the second step we have:

0 0 0 0 0 0 0
1 1 1 1 1 1 1
2 2 2 1 1 1 1
2 2 2 1 1 1 1

Finally, after the last step the board will look like this:

1 0 0 0 0 0 0
2 1 1 1 1 1 1
3 2 2 1 1 1 1
3 2 2 1 1 1 1

So, the maximum number is 3 and there are exactly two cells which contain 3. Hence the answer is 2.

The second solution

If the input array is empty, the program will crash with an exception.

It's recommended to specify the radix to parseInt calls:

let smallColA = parseInt(sArr[0], 10);

The if(sArr.length > 1){ condition is unnecessary,
the loop condition naturally takes care of that.

Even better, no need to treat the first element specially,
you can initialize smallColA and smallColB to Infinity.
Not only it will be simpler,
but in it will work even when the input array is empty.

The first (naive) solution

You could write this simpler:

if(board[i][j] !== undefined){
    board[i][j]++;
}
else{
    board[i][j] = 1;
}

... using a technique you already used in your other recent question:

board[i][j] = (board[i][j] || 0) + 1;


Alarm clock printing I know there must be an easier way to write this but I'm stuck in over-complicating mindset instead of just following the Zen of Python. Please help me simplify this.

  Given a day of the week encoded as 0=Sun, 1=Mon, 2=Tue, ...6=Sat, and
  a boolean indicating if we are on vacation, return a string of the
  form "7:00" indicating when the alarm clock should ring. Weekdays, the
  alarm should be "7:00" and on the weekend it should be "10:00". Unless
  we are on vacation -- then on weekdays it should be "10:00" and
  weekends it should be "off".

alarm_clock(1, False) → '7:00'
alarm_clock(5, False) → '7:00'
alarm_clock(0, False) → '10:00'

def alarm_clock(day, vacation):

    weekend = "06"
    weekdays = "12345"
    if vacation:
        if str(day) in weekend:
            return "off"
        else:
            return "10:00"
    else:
        if str(day) in weekend:
            return "10:00"
        else:
            return "7:00"

You don't use weekdays.
You can have two return statements. (Shown below).

This keeps the same logic, it just removes the need for so meany return statements.

def alarm_clock(day, vacation):
    weekend = "06"
    if vacation:
        return "off" if str(day) in weekend else "10:00"
    else:
        return "10:00" if str(day) in weekend else "7:00"

I would improve it further by adding a check, that you enter a number 0-6.

if not (0 <= day <= 6):
    return "-:--"


Custom multiple div appends in JavaScript I hope you can get me out of my current predicament which involves coding style when it comes to appending many elements in a pretty flexible and typesafe way.

Now I see my code below and I feel that it can be written in a more compact and shorter manner, especially in my set function. Also, I'm not sure if I should merge the arrays and keep it in one line.

What makes me wonder most however, is how to append all those divs to a document fragment before placing them to the real DOM as it is fairly complex.

Would you say I'm right about the possible improvements that can be made? If so, then how can I improve it?

jsFiddle

var superContainer = document.getElementsByClassName('superContainer');
    var article = document.getElementsByTagName('article');
    var obj = {};

    var wrappers = ['a', 'b', 'c'];

    var a1 = ['fruits', 'meat', 'milk', 'soup', 'sweets'];
    var a2 = ['troll', 'gollum', 'imp', 'ork', 'dolly'];
    var a3 = ['foobar'];

    var misc = ['boring', 'fun', 'annoying', 'great'];

    function Constructor() {
        function set() {
            for(var i = 0; i < article.length; i++) {
                var tempSC = document.createElement('div');

                tempSC.appendChild(article[i].parentNode.replaceChild(tempSC, article[i]));
                tempSC.className = 'superContainer';

                create(superContainer[i], wrappers, 0, 0);
                create(obj.a[i], a1, 0, 0);
                create(obj.b[i], a2, 0, 0);
                create(obj.c[i], a3, 0, 0);
                create(obj.milk[i], misc, 1, 2);
                create(obj.sweets[i], misc, 0, 1);
                create(obj.milk[i].children[0], misc, 3, 0);
                create(obj.sweets[i].children[1], misc, 3, 0);
            }
        }

        set();

        function create(parent, elem, loopStart, loopEnd) {
            for(var i = loopStart; i < elem.length - loopEnd; i++) {
                var result = document.createElement('div');
                result.className = elem[i];

                parent.appendChild(result);
                obj[elem[i]] = document.getElementsByClassName(elem[i]);
            }
        }
    }

    Constructor();
}

Indentation

I don't understand the indentation. The second line should be indented the same as the first line.

Multiple var declarations

If you declare multiple variables, you can use the var keyword only once, and separate variables using the comma operator, for example:

var article = document.getElementsByTagName('article')
   ,obj = {}
   ,wrappers = ['a', 'b', 'c']

Semicolons

From another answer of mine:

  You're using many semicolons. JavaScript has a nice feature called
  automation semicolon insertion, which means you almost don't need to
  use semicolons at all, with just a few exceptions. From npm style
  guide:

    Don't use them [semicolons] except in four situations:

    for (;;) loops. They're actually required.
    null loops like: while (something) ; (But you'd better have a good reason for doing that.)
    case 'foo': doSomething(); break
    In front of a leading ( or [ at the start of the line. This prevents the expression from being interpreted as a function call or
    property access, respectively.

  Note that it's only a matter of style. You don't have to follow it if
  you don't like it, but personally I think that if something is not
  required, there's no reason to use it.

Meaningless identifiers

Identifiers like a1, a2, a3 doesn't tell much about the purpose of the variable. Try to name them in a way that explain what they are used for.

Only class names should begin with an uppercase letter

It's recommended to use identifiers starting with an uppercase letter (like Constructor in your case) only to declare classes. As you don't use new operator when calling that function, I assume it's not a class and therefore should have a name starting with a lowercase letter.

Use const instead of var to declare variables that should never be changed

You don't intend to change most of the variables, especially these that refer to the DOM elements. You could use const keyword instead of var to make sure don't override them accidentally.

Using variables or functions before they are declared

You're using create function before you define it. It is recommended not to use variables or functions before declaring them. Therefore, you should move your create function declaration to the top.

Use for...of loop

If you don't need array indexes, you can use the for...of loop instead of the traditional loop.

Example:

for (let element of array) {
  console.log(element)
}

Miscellaneous

I don't see a point in declaring the set function and immediately calling it. If you want to create a separate scope, use IIFE, but I don't think you need it here. The same is with the Constructor function.

a1, a2, and a3 should definitely merged in one array.


Refactoring method in ApplicationController for Rails App Here is a method I have in my ApplicationController, but it's "ugly" and needs some cleanup.

  def user_root_path
    return root_url unless user_signed_in?
    return admin_root_url if current_user.admin?
    return contributor_dashboard_url if current_user.athlete_contributor? && !current_user.class.name.eql?("Athlete")
    return school_admin_athletes_path if current_user.role?("School Admin")

    case current_user.class.name
    when "Athlete"
      if current_user.username.present?
        edit_vitals_athlete_profile_path
      else
        account_finalize_path
      end
    when "HighSchoolCoach"
      school_admin_athletes_path
    when "CollegeCoach"
      if current_user.sports.blank? || current_user.college_id.blank?
        account_finalize_path
      else
        search_index_path
      end
    else
      root_url
    end
  end

I would solve this with a combination of inheritance and duck typing.

Use a base method on a User class root_path to be your entry point.
Pull out that nasty "signed out" user concept to a NullUser object, it is safer than passing nils around or constantly calling user_signed_in? everywhere.
Delegate from the root_path method in the base class to specific_root_path in subclasses once you are past your "early exit" logic.

# application_controller.rb

class ApplicationController < ActionController::Base
  before_filter :set_current_user

  def user_root_path
    @user.root_path
  end

  def set_current_user
    @user = current_user
    @user ||= NullUser.new
  end
end

# user.rb

class User < ActiveRecord::Base
  include Rails.application.routes.url_helpers

  def admin?
    # your admin logic
  end

  def root_path
    return admin_root_url  if self.admin?
    return school_admin_athletes_path if self.role?("School Admin")
    if self.athlete_contributor? &&
      !self.class.name.eql?("Athlete")
      return contributor_dashboard_url
    end

    specific_root_path
  end

  def specific_root_path
    root_url
  end
end

class NullUser
  include Rails.application.routes.url_helpers

  def root_path
    root_url
  end
end

# athlete.rb

class Athlete < User

  def specific_root_path
    if self.username.present?
      edit_vitals_athlete_profile_path
    else
      account_finalize_path
    end
  end

end

# high_school_coach.rb

class HighSchoolCoach < User

  def specific_root_path
    school_admin_athletes_path
  end

end

# college_coach.rb

class CollegeCoach < User
  def specific_root_path
    if self.sports.blank? || self.college_id.blank?
      account_finalize_path
    else
      search_index_path
    end
  end
end

This logic should be equivalent to your original logic, if it isn't, it is pretty close.


My implementation of a memmem() like function Since memmem() isn't cross-platform, I'm trying to rewrite it from scratch. How does this look?
#define MemContainsStr(Str, StrLen, Substr) MemContainsMem(Str, StrLen, SubStr, sizeof(SubStr))
void *MemContainsMem(const void *StrStart, register unsigned long StrLen, const char *Substr, register const unsigned char SubstrLen) {
    if (StrLen < SubstrLen) {
        return 0;
    }
    register const unsigned char *Str = memchr(StrStart, *Substr, StrLen);
    do {
        if (!Str || StrLen < SubstrLen) {
            return 0;
        }
        if (!memcmp(Str, Substr, SubstrLen)) {
            return Str;
        }
        register void *NewStr = memchr(Str+1, *Substr, StrLen);
        StrLen -= NewStr-Str;
        Str = NewStr;
    } while (Str);
    return 0;
}

Are there register variables that shouldn't be, how can I further optimize this code?

I'm trying to rewrite it from scratch. How does this look?

Mis-matched function signature
OP code uses
void *MemContainsMem(const void *StrStart, register unsigned long StrLen,
    const char *Substr, register const unsigned char SubstrLen);

The referenced code uses
void *memmem(const void *haystack, size_t haystacklen,
    const void *needle, size_t needlelen);

Recommend to use the same function signature.  Unclear why OP changed from size_t to unsigned long and changed pointer type.
Use of Str... hints at a string, yet this function does not operate on strings.  Maybe Mem...?  Hard to beat the names needle/haystack.
Invalid access in corner case
The first *Substr in memchr(StrStart, *Substr, StrLen); is UB when SubstrLen is 0.
Code should gracefully and correctly  handle SubstrLen == 0 and SubLen == 0 cases.
register
register is a good keyword to use when the rare programmer better understands the compilation situation better than the compiler.  For the other 99.99% of us, drop register and let the compiler do its job.  Do not tie the hands of the compiler.
0 vs. NULL
Minor: Since the return type is a pointer, consider returning NULL rather than 0 as being more idiomatic.  Either works.
Unneeded code
The first if (StrLen < SubstrLen) { return 0;  appears unnecessary as that case is detected in later code.
Redundant test
} while (Str); can be replaced with } while (1);.  Alternatively make loop a while()
//register const unsigned char *Str = memchr(StrStart, *Substr, StrLen);
//do {
//    if (!Str || StrLen < SubstrLen) {
//        return 0;
//    }

const unsigned char *Str;
while ((Str = memchr(StrStart, *Substr, StrLen)) != NULL && (StrLen >= SubstrLen)) {

Invalid pointer subtraction
Not portable to subtract void * in NewStr-Str.  Use unsigned char *NewStr instead.
Array or string
The define treats Substr as an array and not a string.
Good macro practice uses a () arround each parmeter.
// #define MemContainsStr(Str, StrLen, Substr) \
//    MemContainsMem(Str, StrLen, SubStr, sizeof(SubStr))

#define MemContainsStr(Str, StrLen, Substr) \
    MemContainsMem((Str), (StrLen), (SubStr), strlen(SubStr))

Be wary now of double evaluation of SubStr

Are there register variables that shouldn't be, how can I further optimize this code?

I recommend to drop all register usages and simple enable greater compiler optimizations levels.
A fundamental weakness to this code is that it is O(m*n) where m, n are the lengths of the needle and haystack.
Better algorithms are O(m+n) and oblige significant code changes.

ToDo
Bug??
Str+1 is suspicious in NewStr = memchr(Str+1, *Substr, StrLen); as it is not clear that StrLen > 0 always at this point.  e.g. StrLen, SubstrLen both are 0.
Hmmm


Dictionary synonym lookup via networking I have a small TCP service running on port 4444 which, given a word, will look up synonyms in a dictionary. Below is my Scala code for connecting to the socket, sending the lookup command and parsing the response. If the word exists, then the last line of the response from the service will contain a string with 200 as the status code, otherwise it will return 404.

How can I make this code easier to read and more idiomatic?

private def lookupWord(word: String): Option[String] = {
        val socket = new Socket(InetAddress.getByName("localhost"), 4444)
        val out = new PrintStream(socket.getOutputStream)
        val reader = new BufferedReader(new InputStreamReader(socket.getInputStream, "utf8"))
        out.println("LOOKUP "  + word)
        out.flush()

        var curr = reader.readLine()
        var response = ""
        while(!curr.contains("200") && !curr.contains("404")) {
          response += curr + "\n"
          curr = reader.readLine()
        }
        socket.close()
        curr match {
          case code if code.contains("200") => {
            Some(response)
          }
          case _ => None
        }
      }

What about something like this?

val socket = new Socket(InetAddress.getByName("localhost"), 4444)
val out = new PrintStream(socket.getOutputStream)
val bs = BufferedSource(new InputStreamReader(socket.getInputStream, "utf8"))
out.println("LOOKUP "  + word)
out.flush()
val output = bs.getLines.find(_.contains("200"))
socket.close()
output

FWIW in the general case you could replace that sort of match construct you're using with something like this:

Option(curr).filter(_.contains("200"))


Flaten array to 1D and assigning 'associative' keynames recursively I would like to convert ($data):

Array
(
    [login] => Log in
    [logout] => Log out

    [label] => Array
        (
            [email] => test@test.com
            [name] => Some name
        )

    [controllers] => Array
        (
            [page] => Array
                (
                    [title_new] => New page
                    [title_edit] => Edit page
                    [submit_button] => Submit page
                )
        )
)

To ($result):

Array
(
    [login] => Log in
    [logout] => Log out
    [label[email]] => test@test.com
    [label[name]] => Some name
    [controllers[page][title_new]] => New page
    [controllers[page][title_edit]] => Edit page
    [controllers[page][submit_button]] => Submit page
 )

The following code works "fine":

$data = some_data;
$result = array();

foreach ($data as $k => $v) {
  if (is_array($v))
    foreach ($v as $_k => $_v)
      if (is_array($_v))
        foreach ($_v as $__k => $__v)
          if (is_array($__v))
            foreach ($__v as $___k => $___v)
              $result[$k.'['.$_k.']['.$__k.']['.$___k.']'] = $___v;
          else
            $result[$k.'['.$_k.']['.$__k.']'] = $__v;
      else
        $result[$k.'['.$_k.']'] = $_v;
  else
    $result[$k] = $v;
}

The code work nicely for four levels, and after that it returns "Array" as the value. I think I need recursion, but I would like for you to review my current code, and optionally present me with a solution to go even deeper.

So I probably need some kind of recursion, but I have no clue how to get around that with PHP

Recursion in PHP is the same as basically any language (well... at a very high level).

I'm very torn on if I think your question is on topic or not, but it's been a long time since I've written anything recursive, and I'm a shameless rep-whore, so here's my go:

function flatten(array $arr, $prefix = '')
{
    $out = array();
    foreach ($arr as $k => $v) {
        $key = (!strlen($prefix)) ? $k : "{$prefix}[{$k}]";
        if (is_array($v)) {
            $out += flatten($v, $key);
        } else {
            $out[$key] = $v;
        }
    }
    return $out;
}

Or, if you don't like the magical second param:

function _flatten(array &$out, array $arr, $prefix)
{
    foreach ($arr as $k => $v) {
        $key = (!strlen($prefix)) ? $k : "{$prefix}[{$k}]";
        if (is_array($v)) {
            _flatten($out, $v, $key);
        } else {
            $out[$key] = $v;
        }
    }
}

function flatten(array $arr)
{

    $flat = array();
    _flatten($flat, $arr, '');
    return $flat;

}

You could also optimize a bit and make a lot of the things in there references (the for loop values and function params).  I tend to avoid references in PHP though unless I have an extremely strong reason for them.  (If you plan on using this function on arrays larger than a few hundred elements, that may begin to enter into strong reason land.)

(And flatten is a horrible name, but... yeah.)


"Edit Distance" algorithm I have a piece of code that calculates the edit distance between words and works, but it's apparently not fast enough.

ClosestWords.java:

import java.util.LinkedList;
import java.util.List;

public class ClosestWords {
  LinkedList<String> closestWords = null;

  int closestDistance = -1;

  public int dynamicEditDistance(char[] str1, char[] str2){
        int temp[][] = new int[str1.length+1][str2.length+1];

        for(int i=0; i < temp[0].length; i++){
            temp[0][i] = i;
        }

        for(int i=0; i < temp.length; i++){
            temp[i][0] = i;
        }

        for(int i=1;i <=str1.length; i++){
            for(int j=1; j <= str2.length; j++){
                if(str1[i-1] == str2[j-1]){
                    temp[i][j] = temp[i-1][j-1];
                }else{
                    temp[i][j] = 1 + Math.min(temp[i-1][j-1], Math.min(temp[i-1][j], temp[i][j-1]));
                }
            }
        }
        return temp[str1.length][str2.length];

    }

  public ClosestWords(String w, List<String> wordList) {
    for (String s : wordList) {
      int dist = dynamicEditDistance(w.toCharArray(), s.toCharArray());
      if (dist < closestDistance || closestDistance == -1) {
        closestDistance = dist;
        closestWords = new LinkedList<String>();
        closestWords.add(s);
      }
      else if (dist == closestDistance)
        closestWords.add(s);
    }
  }

  int getMinDistance() {
    return closestDistance;
  }

  List<String> getClosestWords() {
    return closestWords;
  }
}

Main.java:

import java.io.BufferedReader;
import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStreamReader;
import java.util.LinkedList;
import java.util.List;

public class Main {

  public static List<String> readWordList(BufferedReader input) throws IOException {
    LinkedList<String> list = new LinkedList<String>();
    while (true) {
      String s = input.readLine();
      if (s.equals("#"))
        break;
      list.add(s);
    }
    return list;
  }

  public static void main(String args[]) throws IOException {
    BufferedReader stdin = new BufferedReader(new InputStreamReader(System.in, "UTF-8"));
    List<String> wordList = readWordList(stdin);
    String word;
    while ((word = stdin.readLine()) != null) {
      ClosestWords closestWords = new ClosestWords(word, wordList);
      System.out.print(word + " (" + closestWords.getMinDistance() + ")");
      for (String w : closestWords.getClosestWords())
        System.out.print(" " + w);
      System.out.println();
    }
  }
}

Example input:

skål
skålar
skålen
skålens
skålform
#
kål
k
b
sklfrm
skala

Example Output:

kål (1) skål
k (3) skål
b (4) skål
sklfrm (2) skålform
skala (2) skål skålar

The upper part of the input are all the "correct" words and the lower part of the input (below the '#') are the words that need to be corrected. The output lines are of the form:

misspeltWord (minEditDistance)
listOfPossibleCorrectWordsThatShareTheSameEditDistance

Is there a way to make this code more efficient? I am running the code through a sort of "speed test" and it keeps failing on the last part of the test.

Preliminaries: there's strategy, and there's tactics.
A somewhat common procedure to tackle performance problems is to look at "inner" loops first - not entirely wrong, but the golden rule is
measure.
(And, when turning to others for support, provide measurement results and a test data generator or test data.)

Some statements regarding edit distance:
• difference in length gives a lower bound on insertions+deletions
• accumulated differences in frequency gives a lower bound on 2*replacements+insertions+deletions

Review proper using m = len(str1) and n = len(str2):

Document your code. In the code.
design
• I don't like heavyweight constructors
 From the preparation possible, I'd prefer a constructor taking a set of words and
• a "query function" with a word as a parameter (String rather than char[]) -
 too bad returning a multipart result gets verbose, ugly or both in Java
program against interfaces, not implementations
List<String> closestWords;
dynamicEditDistance()

I can guess what's dynamic about it, but that's an implementation detail; such does not belong in a method name: editDistance()
does not use any instance member: make it static
with "the usual" cost model you don't need a full m×n array
If your edit cost is symmetric (cost(insertion) == cost(deletion) && cost(replace(a, b)) == cost(replace(b, a))), you don't need previous row(s) and previous column(s).
you iterate the first index in the outer loop and the second one in the inner:
That's the sequence I'd arrange initialisation
(I'd even use j in the 2nd loop)

work currently done in ClosestWords():

code the way you think about the procedure/solution
- I'd think w doesn't change, let's get the chars exactly once
 (a nifty language system might be doing this for you)
prefer List.clear() over instantiation
(today,) I'd prefer redundantly checking for minimum distance over repeating the add():

          closestWords = new LinkedList<>();
          closestDistance = Integer.MAX_VALUE;
          char[] chars = w.toCharArray();
          for (String s : wordList) {
            int dist = editDistance(chars, s.toCharArray());
            if (dist < closestDistance) {
              closestDistance = dist;
              closestWords.clear();
            }
            if (dist == closestDistance)
              closestWords.add(s);
          }

A "slightly" weirder approach is to handle words from the word list in order of length, first descending from same length, then above and increasing; terminating both when "more extremal length words" can't possibly have a smaller edit distance.
Trying to avoid duplicating the code now extracted as handleDistance() "in line" got out of hand - not pleased, still.
Don't do like I do (not documenting tally (, words) & init()),
do like I say (better than handleDistance() & query(), still)

    final Comparator<String> tally = new Comparator<String>() { @Override
            public int compare(String l, String r) {
                if (l.equals(r))
                    return 0;
                final int ll = l.length(), rl = r.length();
                return ll < rl ? -1
                    : rl < ll ? 1
                      : l.compareTo(r);
            }
        };
    String[] words;
    void init(Collection<String> allWords) {
        words = allWords.toArray(NOSTRINGS);
        Arrays.sort(words, tally);
    }
    /** handles the distance between one pair of Strings
     *  updating closestDistance and closestWords
     * @param chars chars of the query String
     * @param s String from words
     */
    void handleDistance(final char[] chars, String s) {
        // System.out.println(">>>" + s + '<');
        final int dist = editDistance(chars, s.toCharArray());
        if (dist < closestDistance) {
            closestDistance = dist;
            closestWords.clear();
        }
        if (dist == closestDistance)
            closestWords.add(s);
    }

    /** queries words for lowest edit distance to w
     *  updating closestDistance and closestWords
     * @param w String to find closest words to
     * @return closest words
     */
    public Collection<String> query(String w) {
        final char[] chars = w.toCharArray();
        int sameLength = Arrays.binarySearch(words, w, tally);
        if (0 <= sameLength) {
            closestDistance = 0;
            return closestWords = Collections.singletonList(words[sameLength]);
        }
        closestDistance = Integer.MAX_VALUE;
        sameLength = -sameLength; // insert index + 1
        for (int i = sameLength ; 0 <= --i ; ) {
            final String s = words[i];
            if (closestDistance <= chars.length - s.length())
                break;
            handleDistance(chars, s);
        }
        for (int i = sameLength ; ++i < words.length ; ) {
            final String s = words[i];
            if (closestDistance <= s.length() - chars.length)
                break;
            handleDistance(chars, s);
        }
        return closestWords;
    }


Clojure Tic Tac Toe solver I am going through the 4clojure problems and I currently just solved the Tic Tac Toe analyzer:

(fn [board]
    (let [vr (for [x (range 3)]
              (get-in board[(+ x 0) (+ x 0)]))
          vl (for [x (range 3)]
               (get-in board [(+ x) (- 2 x)]))
          columns (apply map vector board)
          checks (concat board [vl] [vr] columns)]
      (letfn [(check-board [[row & res]]
                (if row
                  (let [dist (distinct row)]
                      (if (or (= dist [:o]) (= dist [:x]))
                        (first dist)
                        (check-board res)))
                  nil))]
        (check-board checks))))

I'm looking for any feedback about how I could make the code more terse.

Could I have created the diagonal vectors in one expression?

I don't like that I had to use recursion but I could not think of a way of using something like reduce because I don't think there is a way to short circuit reduce if you find the result midway through processing all the values.

Is there a way that I could avoid recursion with this problem?

Some Minor Points

(+ x 0) and (+x) can be replaced by just x, in (get-in board[(+ x 0) (+ x 0)])) and (get-in board [(+ x) (- 2 x)])) respectively.

You can omit the nil in else position in if : (if row ... nil) \$\rightarrow\$ (if row ...)

Idiomatic way of checking if something is one of a number of things is using a set of those values as a predicate: (or (= dist [:o]) (= dist [:x])) \$\rightarrow\$ (#{[:o] [:x]} dist)

Rename row to something not misleading. Since you named the collection checks you could name the iteration variable check.

Your Questions

  Could I have created the diagonal vectors in one expression?

Yes. If you factor out the differing portions, namely [x x] and [x (- 2 x)], you get:

(let [diagonals (for [f [#(list % %) #(list % (- 2 %))]]
                  (for [x (range 3)]
                    (get-in board (f x))))

Since diagonals is already a sequence, you replace
checks (concat board [vl] [vr] columns)] with checks (concat board diagonals columns)

  Is there a way that I could avoid recursion with this problem?

Recursive processing of collections/sequences can be straightforwardly transformed to for expressions. Your :

(letfn [(check-board [[row & res]]
                (if row
                  (let [dist (distinct row)]
                      (if (or (= dist [:o]) (= dist [:x]))
                        (first dist)
                        (check-board res)))
                  nil))]
        (check-board checks))))

becomes this:

(first
  (for [row checks
        :let [dist (distinct row)]
        :when (or (= dist [:o]) (= dist [:x]))]
    (first dist)))

Note for is lazy, therefore (first (for ... is equivalent to your short-circuit return.

These Changes Applied Altogether

(fn [board]
  (let [diagonals (for [f [#(list % %) #(list % (- 2 %))]]
                    (for [x (range 3)]
                      (get-in board (f x))))
        columns (apply map vector board)
        checks (concat board diagonals columns)]
    (first
      (for [check checks
            :let [dist (distinct check)]
            :when (#{[:o] [:x]} dist)]
        (first dist)))))

Final Point

Also check out some, every?, keep, etc. which process collections with a predicate; and which this exercise probably wants you to learn. For example some is probably what you mean by short-cut evaluation reduce.


All combinations of 2-value variables Introduction
This code was part of one of my projects where it was used to generate boolean input values for truth tables. I have since generalized it for combinations of variables with only 2 values, and added a dash of general Scala-isms. This has been unit-tested with ScalaTest, and has proper documentation.
I rewrote my original code which used bitmasking and shifts because I thought it was too "magical", i.e., difficult to understand at first glance. The bitmasking method also does not give a proper ordering of the values as I would have liked it, which this method does.
This also embodies the concept of a multi-line expression-bodied method.
Notes
My IDE (IntelliJ Idea) recommends not using superfluous parentheses, but I leave them in for clarity.
I would like reviews on any and all aspects of the code and documentation, including but not limited to performance (not really important over clarity to me here, otherwise I'd have used the bitmasking version instead), clarity and idiomatic nature of the code.
Running
Just copy into a file with extension .scala and run it in the Scala REPL as a script. Example usage:
allCombinations(4, true, false) // duplicates the use case I had in my project.

(Run the above in the REPL after the script as beem loaded (scala <file name>.scala, or put it at the bottom of the file <file name>.scala)

Code
/**
    * Generates all possible distinct combinations of `n` variables which have only 2 values,
    * `value` and `alternativeValue`.
    *
    * @param n                the number of variables to generate combinations for
    * @param value            one possible value for a variable
    * @param alternativeValue the other possible value for a variable
    * @return all possible distinct combinations of `n` booleans
    */
def allCombinations[T](n: Int, value: T, alternativeValue: T): Seq[Seq[T]] = {
    for (i <- 0 to n) yield {
      // Make the first i values true and the rest false,
      // and then take all their permutations.
      // This gives us all the required combinations.
      (Seq.tabulate(n)(x =>
                       if (x < i) value
                       else alternativeValue)
       ).permutations
    } // flatten 1 level into a 1-deep list
}.flatten

Why is the function named allCombinations?  The "all" seems superfluous.  Also, it's actually returning permutations rather than combinations, so the second half isn't really accurate either.  I suggest naming it something else entirely, though: product, based on the function in Python's itertools module that does something similar.  As for the first parameter, I find dims (or dimensions) to be more descriptive than n.

Why limit yourself to two possible values?  Why not also support a tri-state, for example?  You could make values variadic.  As a bonus, you wouldn't have to manually toggle between value and alternativeValue; you could just use normal iteration tools like .map.

def product[T](dims: Int, values: T*): Seq[Seq[T]] = {
  dims match {
    case 0 => Seq()
    case 1 => values.map(Seq(_))
    case _ => {
      val lesserProduct = product(dims - 1, values:_*)
      values.flatMap(v => lesserProduct.map(_.+:(v)))
    }
  }
}

Note that the ordering convention is different from yours; i.e. product(4, false, true) is equivalent to allCombinations(4, true, false).  I don't consider that to be a drawback.


Typesafety with dlsym function loading Aim:

Use C++0x features to make function interposition safer. The problem is that it's easy to make a typo when wrapping and interposing on functions.

Prototype Implementation:

#define MAKE_WRAPPER(x) static const wrapper<decltype(::x), ::x> x(#x)

namespace {
  template<typename Sig, Sig& S>
  struct wrapper;

  template<typename Ret, typename... Args, Ret(&P)(Args...)>
  struct wrapper<Ret(Args...), P> {
         typedef Ret(*real_func)(Args...);
         real_func f;
         wrapper(const std::string& sym) : f(NULL) {
                dlerror();
                void *ptr = dlsym(RTLD_NEXT, sym.c_str());
                f = (real_func)ptr;
                if (NULL == f) {
                  std::cerr << "dlsym(): " << dlerror() << std::endl;
                }
                std::cout << "constructing wrapper: " << this << " for: " << sym << "(at " << ptr << ")" << std::endl;
         }

         Ret operator()(const Args&... args) const {
                return f(args...);
         }
  };
}

Example usage:

extern "C" {
  void glXSwapBuffers(Display *dpy, GLXDrawable drawable) {
         MAKE_WRAPPER(glXSwapBuffers);
         // Do stuff here before making the call to the wrapper
         glXSwapBuffers(dpy, drawable);
  }
}

Questions:

Is it worth the effort?
How can it be improved?

[ I'm assuming your example is meant to return x(dpy, drawable); rather than call itself recursively. ]

It's worth it if you have a use for it I guess.
Printing to std::cerr is not error handling, it's error logging. Pick a real error handling strategy and stick to it. Similarly is printing to std::cout necessary? I also believe that the fewer library calls there are in the constructor, the fewer risks of your wrapper calling itself recursively during static initialization of x, which will be disastrous.

Posix recommends the following way to cast when dealing with function types with dlsym:

*(void**)&f = dlsym(RTLD_NEXT, sym.c_str());

operator() can use perfect forwarding:

Ret
operator()(Args... args)
{
    f(std::forward<Args>(args));
}

The wrapping will cost two moves or one copy + one move per argument passed by value and one forward for reference types. It is transparent to the client; he or she won't need to use std::move unless the original function would already have him required to. Notice that the first copy or move of a by-value argument must be paid no matter what; the real opportunity cost of the wrapper is the (potential) move that comes after that, to pass the argument to the real function.

By comparison using Args const&... breaks if the wrapped function is void foo(T&&);, due to reference collapsing rules the signature will be void operator()(T&);

Perhaps the macro should take the name of the variable as an argument; but that may not be necessary depending on your planned usage.

Minor pet peeve: meaningful names. I would have called the constructor parameter symbol. The time it takes to type three more characters each time is simply negligible compared to the time it takes to write and maintain code. function_type instead of real_func possibly, too.


Optimizing error-sorting method I just made this sort method. It runs fine and the code looks okay in my eyes.

Is is possible to optimize it so it runs faster?  If it's \$O(n^3)\$ now, it would be interesting changing it to \$O(n^2)\$ or something like that.

Side question: This is \$O(n^3)\$, right (I only like \$O(n^2)\$ and below)?

List<Log> errors = _logHandler.GetErrors(daysBack);
        List<Log> errorsSorted = new List<Log>();
        for (int i = 0; i < errors.Count; i++)
        {
            bool inserted = false;
            for (int j = 0; j < errorsSorted.Count; j++)
            {
                if (errors[i].Date < errorsSorted[j].Date)
                {
                    List<Log> tempList = new List<Log>();
                    tempList.AddRange(errorsSorted.GetRange(j,
                        errorsSorted.Count - j));

                    errorsSorted[j] = errors[i]; // replace at bigger date index
                    inserted = true;

                    errorsSorted.RemoveRange(j+1, errorsSorted.Count - (j+1));

                    // move all bigger dates one place to the right..
                    foreach (Log log in tempList)
                    {
                        errorsSorted.Add(log);
                    }
                    break;
                }
            }
            if (!inserted)
            {
                errorsSorted.Add(errors[i]); // No date is bigger, add at the end
            }
        }

You are just sorting by date, correct? I'd use

using System.Linq;
// ....
List<Log> errors = _logHandler.GetErrors(daysBack);
List<Log> errorsSorted = errors.OrderBy(l => l.Date).ToList();

According to this stackoverflow question, OrderBy uses QuickSort in this case, which usually has a run time of O(n log n).


FizzBuzz in MIPS, unneccessary branches I'm just starting out in MIPS (as is evident by the fact that I'm writing a FizzBuzz program), and my program runs fine and gives the desired output, but I feel like there is a way to do it without as many branches.

.data

    fstring:    .asciiz     "Fizz\n"
    bstring:    .asciiz     "Buzz\n"
    hstring:    .asciiz     "FizzBuzz\n"

.text

.globl main

main:
    li  $t0, 0

loop:
    bgt     $t0, 99, exit
    add     $t0, $t0, 1
    rem     $t1, $t0, 15
    rem     $t2, $t0, 3
    rem     $t3, $t0, 5
    beq     $t1, 0, fizzbuzz
    beq     $t2, 0, fizz
    beq     $t3, 0, buzz
    li      $v0, 1
    move    $a0, $t0
    syscall
    move    $t0, $a0
    li      $a0, 10
    li      $v0, 11
    syscall
    b       loop

fizzbuzz:
    la      $a0, hstring
    li      $v0, 4
    syscall
    b       loop

fizz:
    la      $a0, fstring
    li      $v0, 4
    syscall
    b       loop

buzz:
    la      $a0, bstring
    li      $v0, 4
    syscall
    b       loop

exit:
    li      $v0, 10
    syscall

Is there a way to sure up this program or is it relatively concise for what it's supposed to do?

Your code looks clean. Fizzbuzz is a very simple program and the most frequent reasons people get it wrong is because they go for optimizations that do not exist, rather than focusing on solving a rather simple problem.
Anything you do to optimize away those 3 branch statements is just calling for potential bugs down the line. However, in the spirit of codereview, I will give a suggestion:
DRY (Don't Repeat Yourself)
Two repetitions I noticed.

Printing a string (all the li $v0, 4)
Printing a new line.

These can be done away with by coordinating between branching and jump instructions i.e. function calls.
.data

    fstring:    .asciiz     "Fizz\n"
    bstring:    .asciiz     "Buzz\n"
    hstring:    .asciiz     "FizzBuzz\n"
    nl:         .asciiz     "\n"

.text

.globl main

main:
    li  $s0, 0

loop:
    bgt     $s0, 99, exit
    addi    $s0, $s0, 1
    move    $a0, $s0       # the function takes input with registers $a0 to $a3
    jal     check_fizzbuzz # call the function
    move    $a0, $v0       # the function returns a string in register $v0, so move it to $a0 so that it can be printed
    li      $v0, 4
    syscall                # print the string
    b loop

check_fizzbuzz:
    la      $v0, hstring
    rem     $t1, $a0, 15
    beq     $t1, 0, return_fizzbuzzz

    la      $v0, fstring
    rem     $t1, $a0, 3
    beq     $t1, 0, return_fizzbuzzz

    la      $v0, bstring
    rem     $t1, $a0, 5
    beq     $t1, 0, return_fizzbuzz

    li      $v0, 1
    syscall
    la      $v0, nl

return_fizzbuzz:
    jr $ra

exit:
    li      $v0, 10
    syscall


Radix 4 reordering function I have a bit reordering function for a radix 4 algorithm which takes around 600 000 cycles to do its job for 4096 elements of floats (real and imaginary parts):

void bit_r4_reorder(float* x, float* y, int N)// N = 4096
{
  int bits = 0;
  int i, j, k;
  float tempr, tempi;

  for (i = 0; i < MAXPOW; i++)//MAXPOW = 24
      if (pow_2[i] == N)
          bits = i;

  for (i = 0; i < N; i++)
  {
      j = 0;
      for (k = 0; k < bits; k += 2)
      {
         if (i & pow_2[k])     j += pow_2[bits - k - 2];
         if (i & pow_2[k + 1]) j += pow_2[bits - k - 1];
      }

  if (j > i)
  {
    tempr = x[i];
    tempi = y[i];
    x[i] = x[j];
    y[i] = y[j];
    x[j] = tempr;
    y[j] = tempi;
  }
 }
}

I've counted the cycles for the different parts of this function and it turns to be (maybe pretty obvious) that the last for() loop containing the swap part is what takes the most part of cycles.

Can you look at this code and give me some hints or suggestion on how I can optimize the code (if it is even possible)?

First of all, my apologies. I completely misread your code.

Ultimately the goal is to reverse bit pairs. The task is very similar to reversing the bits (this discussion is really inspiring), and the approach should be along the same lines.

For example, the first solution can be adopted along the lines of (sorry for sloppy coding):

unsigned int radix4_reverse(unsigned int index)
{
    x = (((x & 0xcccccccc) >> 2) | ((x & 0x33333333) << 2));
    x = (((x & 0xf0f0f0f0) >> 4) | ((x & 0x0f0f0f0f) << 4));
    x = (((x & 0xff00ff00) >> 8) | ((x & 0x00ff00ff) << 8));
    x = (x >> 16) | (x << 16));
    return x >> (32 - bits);
}

(notice that the 0xaaaaaaaa/0x55555555 line is omitted for it will swap bits within a pair).

Regarding your code,

ifs inside the loop are serious performance killers
1 << k is preferable to pow_2[k]. The compiler has better chances to optimize sequential shifts than sequential memory fetches. Besides, it relaxes the memory consumption.
You should operate on bit pairs (e.g. i & (0x03 << k) rather than individual bits.
| is cleaner than + (IMnsHO).
I recommend to pass bits instead of N.


WinSCP IDisposable Wrapper I have written a simple wrapper for WinSCP in C#. I have written it to simplify SFTP connections I will needing to perform.

public class WinSCPConn : IDisposable
{
        //private fields
        private SessionOptions sessionOptions;
        private TransferOptions transferOptions;
        private Session session;

        //public properties
        public bool IsSessionOpen { get { return session.Opened; } }

        public WinSCPConn(string hostname, string username, string password)
        {
            sessionOptions = new SessionOptions();
            sessionOptions.Protocol = Protocol.Sftp;
            sessionOptions.HostName = hostname;
            sessionOptions.PortNumber = 22;
            sessionOptions.Password = password;
            sessionOptions.UserName = username;
            sessionOptions.Timeout = new TimeSpan(0, 3, 0);
            sessionOptions.GiveUpSecurityAndAcceptAnySshHostKey = true;

            transferOptions = new TransferOptions();
            transferOptions.TransferMode = TransferMode.Binary;

            try
            {
                session = new Session();
                session.ExecutablePath = Properties.Settings.Default.WinSCPPath;
            }
            catch (SessionLocalException)
            {
                throw;
            }

        }

        public void Open()
        {
            try
            {
                if (session.Opened == false)
                {
                    session.Open(sessionOptions);
                }
            }
            catch (SessionRemoteException)
            {
                throw;
            }
        }

        public TransferOperationResult SendFile(string SourceFile, string DestFile)
        {
            TransferOperationResult result;
            try
            {
                 result = session.PutFiles(SourceFile, DestFile, false, transferOptions);
                 return result;
            }
            catch (SessionRemoteException)
            {
                 throw;
            }
        }

        public void CreateDirectory(string FolderPath)
        {
            try
            {
                if (!session.FileExists(FolderPath))
                {
                    session.CreateDirectory(FolderPath);
                }
            }
            catch (SessionRemoteException)
            {
                throw;
            }
        }

        public void Close()
        {
            if (session.Opened == true)
            {
                session.Dispose();
            }
        }

        public void Dispose()
        {
            Close();
        }
 }

It would be used as follows:

 using (WinSCPConn conn = new WinSCPConn("host", "username", "password"))
 {
      conn.Open();
      conn.CreateDirectory("/path/");
      conn.SendFile(@"C:\file.txt","/path/file.txt");
 }

My questions are as follows:

How is my exception handling?
Is there too much going on in the constructor?

a condition to be met like if (someBoolean == true) can be simplified to if (someBoolean). For reverse check use the Not operator ! like if (!someBoolean).
Dipsose() should not only call Close() but also get rid of session, sessionOptions and transferOptions by calling Dispose() if they implement IDisposible or at least setting them to null.
SendFile()

you should declare the variable as near as possible to its usage and instead of catching .. rethrowing the exception, let it bubble up the tree. So it can be simplified to

public TransferOperationResult SendFile(string SourceFile, string DestFile)
{
    return session.PutFiles(SourceFile, DestFile, false, transferOptions);
}

if comments don't add any value, like explaining why something is done (what is done should be explained by the code itself), they should be removed like

//public properties


Math Calculus in ruby I have several calculus to do in my ruby app.
My code is currently working but I find it very ugly.

@guestreviews = GuestReview.where(:reviewed_id => @user.id)
@hostreviews = HostReview.where(:reviewed_id => @user.id)
@hospitalityrate = 0
@foodrate = 0
@placerate = 0
@hostreviews.try(:each) do | rate |
  @hospitalityrate += rate.people_rate
  @foodrate += rate.food_rate
  @placerate += rate.place_rate
end
@hospitalityrate = (@hospitalityrate / @hostreviews.size).round
@foodrate = (@foodrate / @hostreviews.size).round
@placerate = (@placerate / @hostreviews.size).round
@averagerate = ((@hospitalityrate + @foodrate + @placerate) / @hostreviews.size).round

Any idea how to make it still readable but less ugly ?

It's actually easier to do this in the database layer. It's exceedingly good at these kinds of things.

@guest_reviews = GuestReview.where(:reviewed_id => @user.id)
@host_reviews  = HostReview.where(:reviewed_id => @user.id)

@hospitality_rate = @guest_reviews.average('people_rate')
@food_rate        = @guest_reviews.average('food_rate')
@place_rate       = @guest_reviews.average('place_rate')

@average_rate = [@hospitality_rate, @food_rate, @place_rate].inject(:+).to_f / @host_reviews.count

It'll result in more database queries, but overall it's more efficient.

I've left out the round() call on the @average_rate only because that seems like a presentation concern to me. Let the controller figure out the average (decimals and all), and let the view decide how to round and display it.

You could also add a has_many :guest_reviews, :foreign_key => "reviewed_id" to the User model, so you can just call @user.guest_reviews.average(...). Same for host_reviews

If you do that, you could also consider simply moving the calculations to the User model, so you don't need to do it all in the controller. And perhaps you could cache the results on the user record and only recalculate when reviews are added/deleted/changed. Assuming this happens less often than the averages being displayed, it'll speed up things a bit.

Lastly, (and this is just small stuff) I'd probably rename some of the variables. I.e. call it @food_rate_average instead of @food_rate in order to a) indicate that it's an average, and b) so it's not confused with the database column of the same name.
Similarly, I'd call the first average people_rate_average rather than hospitality_rate; the column is called people_rate so it's probably best to keep it consistent with that name (or rename the database column, if "hospitality" is a more fitting name)

(Edit: Actually, it's more natural to prefix the average_ rather than suffixing it, i.e. average_food_rate, average_rate, etc.)

In the code above, I've also underscored the names, just to keep things consistent with general Rails/Ruby conventions (FooBar → foo_bar).

Edit: Just for fun, you could go fully meta (though it's probably more confusing than anything - the above is much more readable)

@guest_reviews = GuestReview.where(:reviewed_id => @user.id)
@host_reviews  = HostReview.where(:reviewed_id => @user.id)

@average_rate = %w(people_rate food_rate place_rate).map { |column|
  instance_variable_set(:"@average_#{column}", @guest_reviews.average(column))
}.inject(:+).to_f / @host_reviews.count

That'll set @average_people_rate, @average_food_rate and so on


Checking if any permutation of a string can make it palindrome I was writing code to check if any permutation of a string can make it palindrome and came up with the following logic:

anagram = [ x for x in input() ]
count_alphabets = [ anagram.count(x) for x in set(anagram) ]
middle_element = [x for x in count_alphabets if x % 2 == 1]
print("YES") if len(middle_element) == 0 or len(middle_element) == 1 else print("NO")

The logic is to count the number of times each character appears. ONLY one or none of the characters can appear odd number of times.

The above logic works fine and passes all test cases. Is there a better logic for this?

You may simply use Counter method of collections module to get the frequency of each character in the given word and then simply create a list of the frequency of a given character modulo 2, and then get the sum of the generated list, So the list will have only 2 values, either 0 or 1, So we calculate the sum of the list, and evaluate the condition of <=1 is True

from collections import Counter

s = "summs"

print sum([i%2 for i in Counter(s).values()]) <= 1


Replace an element of a List with a concrete Id I want to replace an object of a List with another one. To find the item to replace I need to check the field id of the object to find what item is the same.

public void changeItemInformation(@NonNull Suggestion suggestion) {
    if (mSuggestions != null) {
        int itemChanged = -1;
        int index = -1;
        for (Suggestion suggestionItem : mSuggestions) {
            index++;
            if(suggestionItem.getId() == suggestion.getId()){
                itemChanged = index;
                break;
            }
        }
        if (itemChanged != -1) {
            mSuggestions.set(itemChanged, suggestion);
            notifyItemChanged(itemChanged);
        }
    }
}

mSuggestions is the list of Suggestions.
suggestion is the new item to replace the old one.
notifyItemChanged() is an Android method to notify a RecyclerView that an element changed.

Is there a better way to do this? There aren't any perform problems, the list always has a maximum of 100 elements.

I don't like your itemChanged and index variables.
If you iterate over a list and keep track of the index anyway, why don't you use an ordinary for-loop?
In addition to that, you also just copy the value of your index variable and break out of the loop; you could easily merge all that to one loop with one condition.
This:

    int itemChanged = -1;
    int index = -1;
    for (Suggestion suggestionItem : mSuggestions) {
        index++;
        if(suggestionItem.getId() == suggestion.getId()){
            itemChanged = index;
            break;
        }
    }
    if (itemChanged != -1) {
        mSuggestions.set(itemChanged, suggestion);
        notifyItemChanged(itemChanged);
    }

would become this:

for (int index = 0; index < mSuggestions.size(); index++) {
    if(mSuggestions.get(index).getId() == suggestion.getId()){
        mSuggestions.set(index, suggestion);
        notifyItemChanged(index);
        break;
    }
}

You don't even need to check if there is an item to replace, because if the condition is never met, it's never going to execute the code inside of it.


Application for converting markdown to PDF First time writing C in a long time and would like some code review to try and improve my code. I have some specific questions, but first an introduction to the code.

The application is a simple command line application for converting markdown to pdf.
The source code can be found in the repository on Github.
The main logic is in main.c, and is hopefully pretty straight forward.

My first question is, have I made any common C errors, like buffer overflows or unreleased memory?
I don't think so, I don't use any malloc calls in my code, but would like someone more experienced to take a look.

I'm compiling the application and dependencies statically, how common is this?
My main reason for this is that one of the dependencies (wkhtmltopdf) is a bit tricky to compile, so I want to be able to provide a binary release that doesn't depend on a shared library.

I have added the dependencies to the repository, not what I'm used to from Ruby or Go, but it seems like the most straight forward way. One other solution I tried was to download the dependencies using git in the Makefile, like this. So they did not have to be added, but that did not feel like a good C solution. Feels a bit like a "roll you own" solution. How would you do this?

Is there anything in the Makefile I could do to make it easier to build for various package managers? E.g. apt-get, homebrew, ports, pkg?

Are there anything else I should do to be a good *nix citizen? E.g. the man page.

This block in main.c feels a bit clunky, and I would like to extract it to a couple of methods instead, to get a better flow when reading the main application logic. But I could not find a good way to do so. The logical parts are read_markdown, create_renderer, generate_filename and write_html. But the code seems to be doing to much for a simple extract method. Do you have any suggestions?

/* Read SOURCE(s) */

hoedown_buffer *ib = hoedown_buffer_new(HOEDOWN_IUNIT);
if (n_files == 1) {
    int error_code = read_file_to_buffer(ib, in);
    if (error_code > 0) {
        fprintf(stderr, "I/O errors found while reading input.\n");
        hoedown_buffer_free(ib);
        fclose(in);
        exit(error_code);
    }
} else {
    int i;
    for (i = 0; i < n_files-1; i++) {
        FILE *ir = fopen(file[i], "r");
        int error_code = read_file_to_buffer(ib, ir);
        if (error_code > 0) {
            fprintf(stderr, "I/O errors found while reading input.\n");
            hoedown_buffer_free(ib);
            fclose(ir);
            exit(error_code);
        }
        if (page_break_between_sources) {
            if (i < n_files-1) {
                hoedown_buffer_puts(ib, "\n\n

\n\n");
            }
        }
        fclose(ir);
    }
}

hoedown_renderer *renderer = hoedown_html_renderer_new(0, 0);
hoedown_document *document = hoedown_document_new(renderer, HOEDOWN_EXTENSIONS, HOEDOWN_MAX_NESTING);

hoedown_buffer *ob = hoedown_buffer_new(HOEDOWN_OUNIT);
hoedown_document_render(document, ob, ib->data, ib->size);
hoedown_buffer_free(ib);
fclose(in);

/* Generate a tmpfile for html output */
char nameBuff[23];
char filePath[30];
int filedesc = -1;
memset(nameBuff, 0, sizeof(nameBuff));
memset(filePath, 0, sizeof(filePath));

strncpy(nameBuff, "/tmp/mdpdf-XXXXXX.html", 22);

filedesc = mkstemps(nameBuff, 5);
FILE *out = fdopen(filedesc, "w");

/* Use tmp file to build a file:// path for wkhtml */
sprintf(filePath, "file://%s", nameBuff);

/* Write tmp html output to file */
write_html_to_file(out, ob, verbose, stylesheet);
fclose(out);

/* Cleanup hoedown */
hoedown_buffer_free(ob);
hoedown_document_free(document);
hoedown_html_renderer_free(renderer);

Answers to your specific questions:

  My first question is, have I made any common C errors, like buffer
  overflows or unreleased memory?

A leak is always connected to a resource. A resource is by definition something that you acquire manually, and that you must release manually. Memory is a prime example, but there are other resources, too (file handles, mutex locks, network connections, etc.).

A leak occurs when you acquire a resource, but subsequently lose the handle to the resource so that nobody can release it. A lesser version of a leak is a "still-reachable" kind of situation where you don't release the resource, but you still have the handle and could release it. That's mostly down to laziness, but a leak by contrast is always a programming error.

I don't see any leaks in your code, but I'm only using my half-human side to analyze that.  I would use a more thorough tool such as Valgrind to provide more substantive confirmation.

  I'm compiling the application and dependencies statically, how common
  is this? My main reason for this is that one of the dependencies
  (wkhtmltopdf) is a bit tricky to compile, so I want to be able to
  provide a binary release that doesn't depend on a shared library.

  I have added the dependencies to the repository, not what I'm used to
  from Ruby or Go, but it seems like the most straight forward way. One
  other solution I tried was to download the dependencies using git in
  the Makefile, like this. So they did not have to be added, but that
  did not feel like a good C solution. Feels a bit like a "roll you own"
  solution. How would you do this?

I had a project that was somewhat similar to yours in this aspect.  As long as all those dependencies are available, then there is no problem

The problem for my project to compile was that I couldn't guarantee that. I also didn't want the user scrounging around trying to figure out how to get my application to simply compile.  Here's what I did, I used CMake to handle all of that.  It took a bit of work, but here's the jist of what it does: it takes all of the dependencies that your application relies on and searches the host computer for them.  If it finds them, it stores that location for it's use later when compiling.  If it can't find them, then it manually downloads them and performs it's own build of the dependency which it will then use later for compiling.  The real "fun" part comes in where you dependency has sub-dependencies, which I also had to deal with...

It's a lot more work, but also a whole lot more portable and less stressful for the end user (which should be the ultimate goal really for all developers, making the usage of your application as hassle-free as possible for your consumers).

  Is there anything in the Makefile I could do to make it easier to
  build for various package managers? E.g. apt-get, homebrew, ports,
  pkg?

Another reason to use CMake or another similar build system, so that it generates a Makefile for you based on the system it's running on.

  Are there anything else I should do to be a good *nix citizen? E.g.
  the man page.

man pages are nice.  Besides, how else are you supposed to tell your users to RTFM?

A few comments on the code posted in the question (hopefully not conflicting with @ferada):

I notice some suspiciously similar code in a few places:

if (error_code > 0) {
    fprintf(stderr, "I/O errors found while reading input.\n");
    hoedown_buffer_free(ib);
    fclose(in);
    exit(error_code);
}

The stuff within the conditional could be extracted to a function so that reuse would be possible, and would help your code follow the DRY principles.  This should also shorten your code a bit
You should be declaring your counter variable (int i in this case, within your for loops), so that it is in the smallest scope possible.(C99)
You don't have to memset your char arrays to 0, just initialize them that way.

char nameBuff[23] = "";

Or the more "universal" way to zero out an array (since it works with every type):

char nameBuff[23] = {};

Nice job generating a temporary file safely!  I have one big problem with it though, and it has to do with this line

strncpy(nameBuff, "/tmp/mdpdf-XXXXXX.html", 22);

See that /tmp/ part?  That isn't guaranteed to be the temporary file storage location for every Unix system, making your code a lot less portable.  For example, the location my system prefers to point to is /var/folders/xp/j1l6q2z12n54k9y4r7574l0w0000gn/T/. Here's how I solved the problem, abstracted away in a function:

const char* getTmpDir(void)
{
    char *tmpdir = NULL;
    if ((tmpdir = getenv("TEMP"))) return tmpdir;
    else if ((tmpdir = getenv("TMP"))) return tmpdir;
    else if ((tmpdir = getenv("TMPDIR"))) return tmpdir;
    else return "/tmp/";
}

To which I created a file like such:

// Creates temporary file safely
char flacFile[FILENAME_MAX] = "";
const char *fileRoot = getTmpDir();
snprintf(flacFile, FILENAME_MAX, "%sXXXXXXXXXXXXX.flac", fileRoot);
mkstemps(flacFile, 5); // the 5 is for the length of the suffix ".flac"

Since I can never actually know the length of fileRoot for my static array creation, I just used the maximum length that a filename could be, very helpfully stored in the predefined macro FILENAME_MAX.


Removing duplicate characters from a string I implemented a function removeDuplicates which takes 1 parameter, a std::string, and returns a std::string without the duplicate characters.

Example I/O might look like this:

removeDuplicates("hello"); //--> "helo"
removeDuplicates("this is easy"); //--> "this eay"

Here is the function:

std::string removeDuplicates(const std::string& str)
{
    //Map for storing character count
    std::map<char, unsigned int> ctable{};

    //Count characters
    std::for_each(str.begin(), str.end(), [&ctable](const auto& value) {
        ++ctable[value];
    });

    std::string newstring = "";
    newstring.resize(str.size());

    //Only copy one character of each type to the new string
    std::copy_if(str.begin(), str.end(), newstring.begin(), [&ctable](const auto& value) {
        return ctable.erase(value) > 0;
    });

    //newstring is bigger than necessary if it has duplicates
    newstring.shrink_to_fit();

    return newstring;
}

Is there a more efficient way to do it?

And what can be improved about the actual code of the function (like variable names/comments, ...).

Design.

Do you actually need to count all the characters?
I would just record that I had already moved them as I moved them from source to destination.

std::set<char>  moved;
std::copy_if(str.begin(), str.end(), newstring.begin(), [&moved](const auto& value) {
    return moved.insert(value).second;
});

Comments

To be honest I don't find your comments objectionable. Maybe slightly too many but not egregiously so.

Remember do not make your comments describe the code. If I can read the code the I will not need the comments. You should use comments to describe WHY and the code shows HOW.

 // Loop over the array
 for_each(array.begin(), array.end() ,

That's a bad comment as I can read the code and see that it loops over the array I don't need the comment to tell me that. Bad comments are worse than no comments. So best to write Self Documenting Code (this just means your variables and function names should be descriptive so I can read the code and understand what is happening).

Code Review.

Don't need the {} to default construct an empty table.

    std::map<char, unsigned int> ctable{};

Rather than std::for_each I would use the new range based for.

    std::for_each(str.begin(), str.end(), [&ctable](const auto& value) {
        ++ctable[value];
    });

    // try
    for(auto value: str) {
        ++ctable[value];
    }

I would avoid the resize() and prefer reserve() in this situation.

    std::string newstring = "";
    newstring.resize(str.size());

Then when inserting into newstring I would use a back inserter. The reserve() makes sure that you will never need to reallocate the space but you don't make the string too long. Then use the back inserter to increase the size of the string as needed. At the end you don't need to shrink to fit.

    std::copy_if(str.begin(), str.end(), std::back_inserter(newstring), [&ctable](const auto& value) {
        return ctable.erase(value) > 0;
    });

The new version of the standard has added std::begin() and std::end(). Prefer to use these rather than the member functions. This will help when your code is templatized and somebody passes an array rather than a standard container.

std::string removeDuplicates(const std::string& str)
{
    std::set<char> moved;
    std::string    result;
    result.reserve(str.size());

    //Only copy one character of each type to the new string
    std::copy_if(std::begin(str), std::end(str),
                 std::back_inserter(result),
                 [&moved](const auto& value) {return moved.insert(value).second;}
                );

    return newstring;
}


Simulating gravitational attraction between bodies - follow up I've decided to revisit a chunk of code I wrote a while back, and see what I could do to make some changes. As a result, I know have slow, but working piece of code which is able to simulate gravitational attractions between \$n\$ amount of bodies. Like usual, I'm using the formula described in Newton's Law of Gravitation, as seen below:

$$F=G\frac{m_1m_2}{r^2}$$

using UnityEngine;

/// <summary>
/// Gravity component.
/// </summary>
[RequireComponent(typeof(Rigidbody))]
public class GravityComponent : MonoBehaviour
{
    public float gravityConstant = 1;
    public Vector3 startingForce;

    /// <summary>
    /// Applies attraction to the object.
    /// </summary>
    public void ApplyAttraction()
    {
        foreach(GameObject body in GameObject.FindGameObjectsWithTag("body"))
        {
            Vector3 force = body.transform.position - this.transform.position;
            float attraction =
                  (this.gravityConstant * this.rigidbody.mass * body.rigidbody.mass)
                / (Mathf.Pow(force.magnitude, 2));
            this.rigidbody.AddForce(force * attraction);
        }
    }

    public void Start()
    { this.rigidbody.AddForce(this.startingForce); }

    public void FixedUpdate()
    { this.ApplyAttraction(); }
}

Ideally, I'd like to know the following things:

Can the performance be better? Right now it runs at an okay speed with about 10 bodies on my laptop, but once it starts getting past that, it begins to slow down rather drastically.
Am I doing anything wrong Unity3d-wise?
Is my code written in an idiomatic manner?
Anything else?

For anyone who might be interested, you can find the repository on GitHub here.

Disclaimer: I have no idea about Unity3D.

that being said, if there is no reason regarding Unity3D to keep this public

public float gravityConstant = 1;
public Vector3 startingForce;

then make it private.

I don't like this method writing style

public void Start()
{ this.rigidbody.AddForce(this.startingForce); }

public void FixedUpdate()
{ this.ApplyAttraction(); }

IMO if something doesn't fit good into the same line, one should use 4 lines of code.

The only obvious place where you can gain some performance boost would be at calculating the attraction.

float attraction =
              (this.gravityConstant * this.rigidbody.mass * body.rigidbody.mass)
            / (Mathf.Pow(force.magnitude, 2));

instead of calling Mathf.Pow(force.magnitude, 2) a simple force.magnitude * force.magnitude will be faster, because there won't be done any checks if e.g the power contains a valid value.


Adding fruits to a basket, allowing selection from ID and name I'm writing a tool which users can select multiple choices at once. Whilst I can't provide the actual content, I've replicated it with fruit. I'm replicating some functionality from Nikto where it allows different flags like "-mutation abcd".

for example here we have three fruits:

1- apple
2- orange
3- kiwi

I wanna let users to type "12" or "appleorange" or "21" or "orangeapple" for selecting apple and orange, or "123" or "appleorangekiwi" or vice versa to select apple, orange and kiwi and etc.

Assume that there are more than 3 choices, so it would be pain in the neck if I want to write elif statement for all multiple selections.
How can I make my code simpler and shorter?

Here is my code:

fruits_ls = []

def add_fruits():
    while True:
        print("choose your favorite fruits:\n"
            "1- apple\n"
            "2- orange\n"
            "3- kiwi\n"
            "4- exit\n")

        my_fruits = input()
        if my_fruits == str(1) or my_fruits == "apple":
            fruits_ls.append("Apple")
        elif my_fruits == str(2) or my_fruits == "orange":
            fruits_ls.append("Orange")
        elif my_fruits == str(3) or my_fruits == "kiwi":
            fruits_ls.append("Kiwi")
        elif my_fruits == str(12) or my_fruits == "appleorange" or my_fruits == str(21) or my_fruits == "orangeapple":
            fruits_ls.append("Apple")
            fruits_ls.append("Orange")

        elif my_fruits == str(13) or my_fruits == "applekiwi" or my_fruits == str(31) or my_fruits == "kiwiapple":
            fruits_ls.append("Apple")
            fruits_ls.append("Kiwi")

        elif my_fruits == str(23) or my_fruits == "orangekiwi" or my_fruits == str(32) or my_fruits == "kiwiorange":
            fruits_ls.append("Orange")
            fruits_ls.append("Kiwi")

        elif my_fruits == str(4) or my_fruits == "exit":
            break

add_fruits()

print(fruits_ls)

Creating a mapping from input to output can be very useful here. A dictionary is the ideal data structure for this

VALID_INPUTS = {
    '1': 'Apple',
    'apple': 'Apple',
    '2': 'Orange',
    'orange': 'Orange',
    '3': 'Kiwi',
    'kiwi': 'Kiwi',
}
EXIT_INPUTS = ['4', 'exit']

Then you can loop over this mapping and append the matching inputs

if my_fruits in EXIT_INPUTS:
    break
for valid_input, fruit in VALID_INPUTS.items():
    if valid_input in my_fruits:
         fruits_ls.append(fruit)


Secure way to store passwords Here I'm trying to develop a simple framework for my own purpose. I'm using PHP and PDO. But, I'm still worried about my password security codes. Below I have posted my codes. it stored like these in my database $1$sr1.F90.$hXd.9DheSMtYmI52KT7fy0. Is this the right way to store passwords? Or Am I make anything wrong here?

I am worried about SQL injection and XSS attacks. But, here I'm using PDO. Is this code good enough to prevent attacks?

Insert script

$password = $_POST['pwd'];
$crypted_pass = crypt($password);
date_default_timezone_set('Asia/Kolkata');
$date = date('Y-m-d h:i:s');
$insert_query = $dbh->prepare("INSERT INTO admin ( user_name, user_pwd, user_email, profile_created_on ) VALUES ( :username, :userpwd, :useremail, :date )");
$insert_query->bindValue(':username', 'Administrator');
$insert_query->bindValue(':userpwd', $crypted_pass);
$insert_query->bindValue(':useremail', 'something@gmail.com');
$insert_query->bindParam(':date', $date);
$insert_query->execute();

Access script

if(isset($_POST['admin_login']))
{
        $username = mysql_real_escape_string($_POST['admin_user_name']);
        $password = mysql_real_escape_string($_POST['admin_user_pwd']);
        try
        {
            $select_query = $dbh->prepare("SELECT * FROM admin WHERE user_name = '$username'");
            $result = $select_query->execute();
            if(count($result) > 0)
            {
                while($row = $select_query->fetch(PDO::FETCH_ASSOC))
                {
                    if(crypt($password, $row['user_pwd']) == $row['user_pwd'] && $row['user_name'] == $username)
                    {
                        session_start();
                        $_SESSION['sessionname'] = $row['user_name'];
                        header('Location: dashboard.php');
                        return true;
                    }
                    else
                    {
                        echo '
Access Denied. Wrong Username or Password. Go Back
';
                        return false;
                    }
                }
            }
        }

This is absolutely insecure.

I will list only a handful of things which I think need to be absolutely rewritten.

Do not use crypt. Instead, please use password_hash. Along with password_verify, your code could be simplified a great deal.
If you're going to use PDO, use it. Do not mix in other database access modules (MySQL). Especially not MySQL, it is not current and is not supported nor will it ever be supported in the future. mysql_real_escape_string gives a false sense of protection, but in reality, it does nothing for you.
Along with number 2, comes the possibility of SQL injection in your app. If you follow what I've said in number 2 (Use PDO (bind parameters, prepare statements), get rid of anything MySQL), you should be good.
If you're going to try something, catch it! I want to know when something goes wrong.


Merging list tails in Scheme In my last post, I wrote a long, non-general tail-merging† facility for insertion sort. I thought I should instead write a general-purpose tail-merging function, that works even when the two lists are of different lengths. So here it is (requires SRFI 1):

;;; Returns a list that has the same content as `lst`, sharing tail with `base`.
;;; Element comparisons use `eqv?`.
(define (merge-tail lst base)
  ;; `uniq1` and `uniq2` are guaranteed not to be `eqv?` to anything else.
  ;; This ensures that `breakpoint` always has a value, without any hacks.
  (define uniq1 (list 'uniq1))
  (define uniq2 (list 'uniq2))
  (define breakpoint (list-index (lambda (x y) (not (eqv? x y)))
                                 (reverse (cons uniq1 base))
                                 (reverse (cons uniq2 lst))))
  (append (drop-right lst breakpoint) (take-right base breakpoint)))

(In Racket, you can replace (list 'uniq1) with (box 'uniq1) (and similarly for uniq2), and (lambda (x y) (not (eqv? x y))) with (negate eqv?).)

Is there a more stylish or performant way to write this code? (Note, I'm trying to not mutate either list. If the lst list is linear-update, then of course you can use append! and drop-right!.)

† The process of merging tails is to ensure that two lists share the longest tail possible. This is especially useful for immutable lists.

A minor detail: In output it can be hard to tell the difference between (cons #f #f) and (cons #f #f) i.e. between uniq1 and uniq2. An alternative is to write (list 'uniq1) in order to get a unique value.

For comparison here is a version of merge-tail written using named let rather than using indices. Here the common tail between xs and ys are found, and two new lists using the shared tail is returned.

(define (reverse-append rxs ys)
  (if (null? rxs)
      ys
      (reverse-append (cdr rxs) (cons (car rxs) ys))))

(define (merge-tail xs ys)
  (let loop ([tail '()]
             [rxs  (reverse xs)]
             [rys  (reverse ys)])
    (cond
      [(eqv? (car rxs) (car rys))
       (loop (cons (car rxs) tail)
             (cdr rxs)
             (cdr rys))]
      [else
       ;; tail is now equal to the common tail of xs and ys
       (values (reverse-append rxs tail)
               (reverse-append rys tail))])))

This solution will fail if xs and ys are cyclic lists with equal elements.
So in the terminology of srfi 1, the solution only works for proper lists.


Cache with limited size; If full remove the oldest Here is a class that stores a single value into the limited cache. If cache is full, it will remove the oldest element to make a space for a new value.
I'm using 2 structures to accomplish this because I need a fast lookup. I'm not very confident this is the best approach.
Cache will contain a small amount of data (lets say 1000 elements).

#include <unordered_set>
#include <queue>

template <class T>
class Cache{

private:
    int _maxSize;
    std::unordered_set<T> _set;
    std::queue<T> _queue;

public:
    explicit Cache(int maxSize) : _maxSize(maxSize){}

    bool add(T value){
        if(_set.find(value) == _set.end()){ // // if not exists in set
            if(_set.size() == _maxSize){ // if set is 'full'
                _set.erase(_queue.front()); // delete the oldest value from the set
                _queue.pop(); // delete oldest value from queue
            }
            _queue.push(value); // push new value into queue
            _set.insert(value);  // insert new value into set
            return true;
        }
        return false;
    }

    int size(){
        return static_cast<int>(_set.size());
    }
};

Naming of data members: I would avoid duplicating the information that carries with the type. Can you think of something more meaningful?
Unsigned vs. signed integer for handling the size: as the maximum size is passed to the contructor as an int, static_cast<int>(_set.size()) should be safe. However, when it's foreseeable that you'll implement the class template using containers from the standard library (they all use std::size_t for their sizes), why not go with std::size_t for Cache, too?
Related to the above: always compile with warnings enabled. clang++ -Wall -pedantic -Wsign-conversion -Wextra -std=c++11 gives me

  warning: comparison of integers of different signs [...]

if(_set.size() == _maxSize){ // if set is 'full'

Again, as _maxSize is an int, this can be considered a non-issue in practice, but it's still somewhat brittle, if you change some implementation details later on.
You can only instantiate the Cache template when T mets the standard hash container requirements (i.e., a std::hash specialization is available as well as a comparison operator). To make your class template more reusable, consider adding template parameters that can be passed to std::unordered_set. If they have sensible defaults, you don't need to specify them most of the time.

template <class T, class Hash = std::hash<T>, class EqualTo = std::equal_to<T>>
class Cache {
    std::unordered_set<T, Hash, EqualTo> _set;
    // ...
};

When caching an object, you save it twice, one in _queue and once in _set. For small objects, the duplication might be acceptable, but what if T is large? Remember that std::unordered_map doesn't invalidate iterators/pointers upon insertion and removal of entries. Consider a std::queue of iterators into _set.
Passing T by value in bool add(T value) can be a reasonable choice as long as you std::move it when it's used the last time and T supports move semantics. In your case:

bool add(T value) {
   // ...
   _queue.push(value); // Don't move here
   _set.insert(std::move(value));  // ... but here!
}

You could also consider an overload set bool add(const T&) and bool add(T&&) instead as std::vector::push_back does for example.
When you worry about performance and you have varying max. cache sizes known at compile time, consider adding a non-type std::size_t template parameter to Cache. This way, you could specialize on smaller caches to e.g. use std::vector instead of std::unordered_map, which can be faster due fewer cache (ha!) misses. You need to do some benchmarking first though, and take different sizeof(T) into account.
A last note on performance: std::unordered_set was designed as a drop-in replacement for std::set. That's why it doesn't invalidate iterators/pointers into an instance, but that's also why it's not the fastest hash set. Of course, make sure to benchmark this properly, but you might want to have a look at some open address hash set, e.g. the one provided by abseil could be an option.


Substitution of different occurrences of a string with changing patterns I need to parse an invalid JSON string in which I find many repetitions of the same key, like the following snippet:

[...]
"term" : {"Entry" : "value1", [.. other data ..]},
"term" : {"Entry" : "value2", [.. other data ..]},
[...]

I thought of appending a suffix to each key, and I do it using the following code:

word = "term"
offending_string = '"term" : {"Entry"'
replacing_string_template = '"%s_d" : {"Entry"'

counter = 0
index = 0
while index != -1:
    # result is the string containing the JSON data
    index = result.find(offending_string, index)
    result = result.replace(offending_string,
                replacing_string_template % (word, counter), 1)
    counter += 1

It works, but I'd like to know if it is a good approach and if you would do this in a different way.

import json

def fixup(pairs):
   return pairs

decoded = json.loads(bad_json, object_pairs_hook = fixup)

object_pairs_hook is a more recent addition. If you have an older version of python you may not have it. The resulting python object will contain lists of pairs (including duplicates) rather then dictionaries.


Design of class hierarchy for a object formatter API forces derivatives to know too much While learning object oriented design I'm judging my own design critically. This framework should be able to print objects in either XML, or JSON, I've stubbed in a basic implementation to avoid getting into details of XML and JSON parser APIs for now.

So I made the Formatter be the base class. But with my current design, all derivatives of this base class would need to know that they have to call: getFormattedValue() to get output. Also I don't feel comfortable with all of those if else statements in the Formatter constructor. The clients would need to know to pass in either an "XML" or "JSON" in all derivatives of this class. How can I improve this design to conform to all object oriented design principles?

public class Formatter {

    private String output;

    public Formatter(Object object, String formatType){
        if(formatType.equals("xml")){
            output = getXMLFormat(object);
        } else if(formatType.equals("json")) {
            output = getJSONFormat(object);
        }
    }

    private String getXMLFormat(Object object){

        return "<title>"+object.toString()+"<title>"; // simplified
    }

    private String getJSONFormat(Object object){
        return "{"+object.toString()+"}"; // simplified
    }

   protected  String getFormattedValue(){
        return output;
    }
}

The derivative class:

public class ItemFormatter extends Formatter {

    public ItemFormatter(Employee item, String formatOutput) {
        super(item, formatOutput);
    }

    public void printItem(){
        System.out.println(getFormattedValue());
    }
}

This one is screaming for an enum. What you want is what you get:

public enum FormatType {
    XML, JSON, //maybe further support later
}

And your constructor then becomes:

public Formatter (Object object, FormatType formatType) {
    switch (formatType) {
        case XML:
            output = getXMLFormat(object);
            break;
        case JSON:
            output = getJSONFormat(object);
            break;
    }
}

In addition to that a small nitpick on naming:
Your get...Format() methods break the camelCase you kept until then. I suggest you ignore that these formats are acronyms and instead use following method names:

getXmlFormat (Object object);
getJsonFormat (Object object);

But I'd say that's personal preference. All else you didn't give us much to review :(


Separating values in string and returning them as comma separated string I am iterating through a set of values that are in the following string format:

69;#Statements;#44;#Clarifications;#45;#Suggestions;

(fields with multiple values returned from SharePoint.)

I've created a function that both adds these values to a list and returns them as a comma separated string:

Category = formatLookup( Category_Raw, categoryRow )

The function works, but is probably far from efficient. Any suggestions on how to optimize this function, used for thousands of elements?

function formatLookup( input ) {
    var result = ''
    if (typeof input != "undefined" && input != "" ) {
        if (input.indexOf(";#") >= 0) { //detect if there is more than one value in column
            if (input.match(/;#/g).length > 1) {
                var array = input.split(';#');
                for (i = 0; i < array.length; i++) {
                    if (array[i].match(/^\d+/)) { //do nothing
                    } else {
                        var output = array[i].replace(/^\d+/, '')
                        if (output != '') {
                            result += output
                            result += ', '
                        }
                    }
                }
            } else {
                result = output = input.split(';#')[1]
            }
        } else {
            result = input
        }
        if (result.slice(-2) == ', ') result = result.substr(0, result.length - 2) //if the last item has a comma, remove this
    }
    return result
}

Stuff I noticed:

Missing semicolons. Perhaps the code doesn't need them, but you certainly want them. My argument is always that if JS truly didn't need semi-colons, they'd be completely optional (or cause outright syntax errors). But JS's automatic semi-colon insertion is (in my opinion) a crutch meant to fix bad code; ideally it should never come into play at all (unless you're a JS minifier or something).
You've got an implicit global: i is never declared. And i is veery likely to used in loops elsewhere, so you've got a really tricky bug that's just waiting to strike.
You've also got some other declaration-confusion: output is declared in one branch of the code, but is also used in another branch. Since the branches are mutually exclusive, the latter branch is basically using output without it having been declared. Again, JS does it's best to fix your code, but it shouldn't be necessary.
Just declare all your local variables at the top of the function.
Strict comparisons (=== and !==). You might as well use them.
Also be consistent with your use of double- and single-quotes. There's no reason to mix and match. Doens't do any harm, but neither does consistency.
Your first conditional can be written as simply: if(input) { .... You're checking for undefined and an empty string, but both of those are false'y, so it's needlessly complex. If anything, you could consider checking typeof input === 'string', if you want to check types.
But instead of wrapping everything in that if block, just return early, and save yourself a lot of indentation.

var result = '';
if(!input) { return result; }
// ...

In a similar vein: You check if the input contains the ;# delimiter. If it doesn't, result is just set to be the input. So once again, just return early; send the input right back.
Don't glue together a string yourself; use join(', '). Saves you the trouble of going back and chopping off stray commas and whitespace afterward.
This doesn't make sense to me:

if (array[i].match(/^\d+/)) {
  //do nothing
} else {
  var output = array[i].replace(/^\d+/, '')

So, if array[i] begins with some digits, do nothing. If it doesn't begin with digits, then... remove those non-existent digits? What?
Most of all: If you're going for performance, don't use indexOf, match, replace and split over and over to find the same stuff.

I'd say just split the input string right away. There's no reason to branch and check whether there's a reason to split; just split, and process the resulting array. If there's only one item in the array, so be it - no need to treat it any differently than if there were 2, 3, or 789 items in the array.

It also looks to me like you intend to discard all the parts that are just digits (e.g. "69", "44" and "45" in your example), and just keep the text parts. If the pattern is always "number;#text;#number;#text ... etc", you could simply include the numbers in your split-pattern. The code below will split the string at ;#<number>;#, <number>;# or just ;#

input.split(/(?:;#)?\d*;#/); // => [ '', 'Statements', 'Clarifications', 'Suggestions;' ]

though, as you can see, that leaves you with a blank string in the array, because your input begins with a number.

Alternatively, you can replace all the numbers ahead of time:

input.replace(/\d+;#/g, ''); // => "Statements;#Clarifications;#Suggestions;"

so now you can split that string easily.

Your example input does however have a trailing ; which I don't see being handled anywhere. You current function returns a string with that semicolon intact ("Statements, Clarifications, Suggestions;"), which is doubt is the intention.

Assuming you only want the "words", you could flip the whole thing around, and, instead of splitting, use match to just pick out the parts you want, ignoring the numbers and delimiters:

input.match(/[a-z]\w+/ig) // matches a starting letter, and goes until it hits something that's not a letter or number.

which, if that's the appropriate pattern to use for your data, reduces the entire function to this:

function formatLookup(input) {
  return input.match(/[a-z]\w+/ig).join(', ');
}

Done.

Edit: Here's a more liberal regex (see comments)

function formatLookup(input) {
  return input.match(/([^#]*?)[a-z][^;]*/ig).join(', ');
}

This one will match everything between a "#" and a ";" provided there's just a single letter in there (i.e. it won't match all-number parts). It's still not perfect, and will for instance not match anything in an input string like "45;#3. 555-3443;" (don't know if that's relevant, but caveat emptor, etc.).

If that proves problematic, it might well be simpler to just split the string, and discard every 2nd part (if indeed the number-text-number-text... pattern holds true):

function formatLookup(input) {
  return input.split(/;#?/).filter(function (value, i) {
    return value && i % 2;
  }).join(', ');
}

Note that the above will likely be slightly faster (i.e. probably-not-worth-it faster) if you use an old-school for-loop to loop through and push to an array, rather than use filter:

function formatLookup(input) {
  var values = input.split(/;#?/),
      result = [],
      i = 1,
      l = values.length;

  for(; i < l ; i += 2) { // notice the increment
    if(values[i]) { result.push(values[i]); }
  }

  return result.join(', ');
}

Addendum: In an earlier iteration of the question, you were adding the values to a global array as well. If that's still useful, then I'd suggest simplifying the function even further, and just let it return an array:

function parseLookup(input) { // note the different name
  return input.match(/[a-z]\w+/ig);
}

Then, where needed, you can do something like this

var values = parseLookup(input);

someGlobalArray = someGlobalArray.concat(values); // append to global array

var formatted = values.join(", "); // same end-result as before

Even if you don't need the global array, I'd still suggest separating parsing the input, and formatting the output. Your current function does both, but it'd be cleaner and more modular to have one function for one thing, and another function for the other (or just inline the join(...) if it's only used once). Your current function basically takes all the data from one informal string format... and outputs another informal string format. Sure, something happens along the way, but overall it's a bit like two steps forward, one back.