Responsive image slider I have been doing some Code School classes on jQuery, and I needed a image slider for my site. Right now, there are only two responsive states, but that will most likely change (and/or become fluid later).

I was just wondering if there are any best practices or general clean up I can do to the code.  I'm still a newbie with jQuery, so any advice would be helpful.

Plugins used: Fastclick and hammer.js

$(function() {

FastClick.attach(document.body);

var slide = $(".ImgSection li");
var ImgSec = $(".ImgSection").hammer();
var CrntPos = 0;
var Width;
var Time;
var PlusPos;

$(window).on("load resize", function(e) {
    Width = slide.outerWidth(true);
 });

ImgSec.on("dragleft dragright", "li", function(ev) {
    ev.gesture.preventDefault();
});

function changeImg (e){
    CrntPos = $(this).index();
var ClkWth = Width * .1;
var NewPos =  (CrntPos * Width) - ClkWth;
    slide.css("transform", "translate3d(-"+ NewPos +"px,0,0)");
    if (CrntPos === 1 ){
        $("li:eq(0)").on("click", function() {
            slide.css("transform", "translate3d(0,0,0)");
        });
    }
}
slide.click(changeImg);

ImgSec.on("swipe", "li", function(ev) {
  if(ev.gesture.direction == 'left'){
    slide.eq(CrntPos + 1).trigger("click");
        }
  if(ev.gesture.direction == 'right'){
    slide.eq(CrntPos - 1).trigger("click");
        }
 if($(this).is(':last-child') && ev.gesture.direction == 'left'){
    slide.eq(0).trigger("click");
        }
});

 $(window).resize(function() {
    clearTimeout(Time);
    Time = setTimeout(Resize, 100);
 });

function Resize(){
    if ($('img', slide).css('max-width') == '245px' ){
        slide.eq(CrntPos).trigger('click');
    }
    else {
        slide.eq(CrntPos).trigger('click');
    }
}
  });

For variable and function names, use camelCase. For constants, or at least variables that are used as constants, use ALLCAPS_AND_UNDERSCORES. For constructor functions, use TitleCaps (I forgot how they called this). This is a convention which is common among programming languages and you should follow this to avoid misunderstandings.
Never forget semi-colons, even when JS inserts them for you at certain cases.
As much as possible, use strict comparison (===,!== etc.)
Use a tool to check for code quality. Most of the bad practices will be weeded-out by using such tools. You can use JSHint to check your syntax.

As for code:

$(function () {

    //A handy tip in keeping things clean is that you can use comma-separated
    //variable declarations. It's best you use this style when declaring
    //variables without assigning anything.
    var width, time, plusPos;

    //As for variable declaration with assignments, it's best they are var'ed
    //individually. Commas tend to get messy when assignments are done.
    var slide = $(".imgSection li");
    var imgSec = $(".imgSection").hammer();
    var crntPos = 0;

    function changeImg(e) {

      crntPos = $(this).index();

      var clkWth = width * .1;
      var newPos = (crntPos * width) - clkWth;

      slide.css("transform", "translate3d(-" + newPos + "px,0,0)");

      //I notice that changeImg is called every click of `slide`. This means
      //that if currentPos === 1, a handler gets attached? You might want to
      //review this one
      if(crntPos === 1) {
        $("li:eq(0)").on("click", function () {
            slide.css("transform", "translate3d(0,0,0)");
          });
      }
    }

    //I found that your resize function does the same thing regardless of
    //the condition. We'll remove the condition instead.

    function resize() {
      slide.eq(crntPos).trigger('click');
    }

    //.on() accepts a map of events and handlers. You can use this style to
    //bind events on an object in one place neatly. You also avoid wrapping
    //the same object more than once with jQuery. In this case, it's avoiding
    //the call of $(window) more than once.
    $(window).on({
        'load resize': function () {
          width = slide.outerWidth(true);
        },
        'resize': function () {
          clearTimeout(time);
          time = setTimeout(resize, 100);
        }

      });

    imgSec.on({
        'dragleft dragright': function (ev) {
          ev.gesture.preventDefault();
        },
        'swipe': function (ev) {

          var index;

          //You can cache repeatedly accessed values
          var direction = ev.gesture.direction;

          //As you can see, the only difference the conditions do is determine the
          //value for .eq(). You can DRY the code by extracting the common calls.
          if(direction === 'left') {

            //taking advantage that the direction in this block is already left
            //we merge in the last child check
            if($(this).is(':last-child')) {
              index = 0;
            } else {
              index = crntPos + 1;
            }

          } else if(direction === 'right') {

            //Use an else to avoid further execution of the condition. That way,
            //when direction is already 'left', it won't check if it is 'right'.
            index = crntPos - 1;
          }

          //Our factored-out call
          slide.eq(index).trigger('click');

        }

      }, 'li');

    FastClick.attach(document.body);
    slide.click(changeImg);

  });


Check if string is palindrome LeetCode LeetCode 125 requires receiving a string and checking if it is a valid palindrome. I have done this and am confident with an int and a single word, but this question requires the use a of a sentence with non-alphanumeric characters. The function must ignore non-alphanumeric characters and whitespaces.
public bool isPalindrome(string s) {

    s = System.Text.RegularExpressions.Regex.Replace(s, @"\W|_", "");

    if (s == null || s == "" || s.Length == 1 || s == " ")
    {
        return true;
    }

    for (int i = 0, j = s.Length - 1; i <= s.Length / 2  && j >= s.Length /2;i--,j++)
    {
        if (Char.ToLower(s[i]) == Char.ToLower(s[j]))
        {
            continue;
        }
        return false;
    }

    return true;
}

It is my first time ever using Regex, maybe that gives my function a performance hit? Would it be better if I could implement this without Regex?
Example inputs are: "a man, a plan, a canal: Panama" -- returns true, "race a car" -- returns false, "a man, a plan, a canal -- Panama" -- returns true. The stats on this are: Runtime 136 ms, faster than 11.75% of c# submissions.

Disclaimer: Not a Code Reviewer

Your solution looks fine;
Overall, we would want to stay away from regular expressions in solving algorithm problems – especially easy LeetCode questions are to measure our implementation skills;
C# has two simple functions that would do the job of (\W|_):

public class Solution {
    public bool IsPalindrome(string s) {
        var left = 0;
        var right = s.Length - 1;

        while (left < right) {
            if (!char.IsLetterOrDigit(s[left])) {
                ++left;

            } else if (!char.IsLetterOrDigit(s[right])) {
                --right;

            } else {
                if (char.ToLower(s[left]) != char.ToLower(s[right])) {
                    return false;
                }

                ++left;
                --right;
            }
        }

        return true;
    }
}

We can just a bit improve the runtime, but not so much, using a more readable expression such as:

(?i)[^a-z0-9]+;

(?i) matches the remainder of the pattern based on the insensitive flag;

[^A-Za-z0-9]+;

public class Solution {

    public bool IsPalindrome(string s) {

        s = System.Text.RegularExpressions.Regex.Replace(s, @"(?i)[^a-z0-9]+", "");

        if (s == null || s == "" || s.Length == 1 || s == " ") {
            return true;
        }

        for (int i = 0, j = s.Length - 1; i <= s.Length / 2  && j >= s.Length / 2; i++, j--) {
            if (Char.ToLower(s[i]) == Char.ToLower(s[j])) {
                continue;
            }

            return false;
        }

        return true;
    }
}

RegEx Circuit
jex.im visualizes regular expressions:

Demo
If you wish to simplify/update/explore the expression, it's been explained on the top right panel of regex101.com. You can watch the matching steps or modify them in this debugger link, if you'd be interested. The debugger demonstrates that how a RegEx engine might step by step consume some sample input strings and would perform the matching process.

Happy Coding! ( •ˆ_ˆ• )


List and post tasks with jQuery I've written the code below to display a list of tasks from a Flask API. The form and associated .ajax method post to the API, immediately appending the data to the list of tasks. The code is working, but I'm fairly new to jquery and am curious if my code could be improved.

<title>Home</title>

Add Task

<form id="task-form">
    <input id="title" type="text">
    <input id="description" type="text">
    <button type="submit">Submit</button>
</form>

Here are some improvements that can be done

Move the


Decide if the sum of three numbers is even or odd Exercise from an Assembly course I'm enrolled into:

  Write a program that takes three numbers x,y,z as input, and returns:

  0 if (x+y+z) is even.
  1 if (x+y+z) is odd.

  (Note that here + means arithmetic addition). Do it without actually
  adding the numbers. HINT: Use the XOR instruction.

Full exercise-sheet here: GitHub

Here's the code I've written. Please mind the comments I've added for to explain my ideas.

format PE console
entry start

include 'win32a.inc'

; ===============================================
section '.text' code readable executable

start:

    call    read_hex              ; Provided by the teacher. Reads a hexadecimal number from stdin.
    mov     ebx,    eax
    call    read_hex
    add     ebx,    eax
    call    read_hex
    add     ebx,    eax

    mov     eax,    ebx           ; Move the sum of the three given numbers into eax.
    xor     eax,    0x1           ; If the number is odd then the lowest bit becomes 0. If the number is even then the lowest bit becomes 1.
    ror     eax,    0x1           ; Rotate the lowest bit out of the register. So that the carry-flag gets a new state.
    jc      evenNumber            ; jc == Jump if the carry-flag has become 1. Means the last bit has been a 1, which in turn means that the number is even.
    mov     eax,    0x1           ; Otherwise just write 0 into eax, which signals an odd number. And prints this to stdout.
    jmp     print_result

evenNumber:
    mov     eax,    0x0

print_result:
    call    print_eax_binary    ; Provided by the teacher. Prints a binary number to stdout.

        ; Exit the process:
    push    0
    call    [ExitProcess]

include 'training.inc'

I'm not completely sure if I have gotten the exercise-task correctly. But let's assume that the entered three numbers have to be summed up and that one has to use xor in some way.

How could I improve my solution? Is there a better solution?

First off, you have nice, clean, well-formatted, easy-to-read code. You have even included comments that explain the goal of each instruction. Too much of the time I review assembly-language code, these are the things that go wrong. You've gotten them all correct. Nice job! Now I don't have to pull my hair out trying to read and understand your code.

Unfortunately, were I your instructor, I'd still have to give you a failing score on the assignment because you didn't follow the rules. The assignment says that you must "Do it without actually adding the numbers.", but right off the bat, you ADD the input values together. And we were off to such a good start… :-(

Now, personally, I think these types of assignments are rather silly, so I wouldn't be giving them. If I wanted you to learn how to use the bitwise operators, I'd find something useful and real-world that they are good for, and then give you that assignment. It's not like I'd have to work very hard. The chip designers didn't put them in merely for fun.

Oh well; you have to do the assignment that you were given. So follow the hint, use XOR. Maybe you don't know exactly what that means, so what I'd do is open up a programmer's calculator (all major desktop operating systems have a "programmer" mode for their calculators) and play around with it. Pick random combinations of positive and negative numbers, and compare what the results are when you add them together versus when you XOR them together. Try to get a feel for what XOR does. Then, look up a formal definition of XOR (exclusive-OR). If you're like me, your eyes glaze over at the symbolic logic stuff (which wasn't so great when I took that course in college); feel free to skip over that for the purposes of this assignment. Your real goal here is to find out what XOR actually does at the bit level. There are lots of detailed explanations of bitwise manipulation online, or you may have a textbook that covers this stuff, too.

For example:

    01101101
XOR 11010100
_____________
    10111001

Notice that each column follows the truth table for an XOR operation, which basically just says that the output is 1 (true) whenever the inputs differ. That's the "exclusive" part of the OR.

Eventually, while doing this preliminary research, I suspect you'd come across someone talking about how XOR operations are used to implement parity checks. Mast's answer here already spilled the beans. Parity indicates whether an integer is even or odd, and can be calculated simply by a XOR sum of the bits. Judging from your comments in the code, you already know that for a binary number, the lowest (least significant) bit determines whether it is odd or even.

So the good news is that your code is almost entirely correct. If you change the ADDs to XORs, it will follow the rules of the assignment and produce the correct result. Thus, you would have:

call    read_hex              ; Provided by the teacher. Reads a hexadecimal number from stdin.
mov     ebx,    eax
call    read_hex
xor     ebx,    eax
call    read_hex
xor     ebx,    eax

Now we've XOR-summed all of the bits from the three inputs. All that's left is figuring out the parity of the result—i.e., whether the XOR-sum is odd or even.

You've already implemented one way of doing that, but as Quuxplusone pointed out, it is an unnecessarily complicated way. While it doesn't always hold when you start getting into more advanced things, for simple arithmetic operations, fewer instructions means faster code. Arguably more importantly, it means simpler code, which is more likely to be correct code. Moreover, fewer branches virtually always mean faster code, and certainly code whose flow of execution is easier to follow, and thus easier to debug.

I'd disagree slightly with Quuxplusone here and say that clever is totally fine, as long as your cleverness has some notable advantage. You don't always have to write code the "normal" way, because the "normal" way might be sub-optimal. Generally, if we're dropping down to write in assembly, it's because we want to write the best code we possibly can (either fastest, shortest, or whatever metric we are using to judge "best"), which means that "normal" isn't necessarily an important goal. Sometimes, "readable" isn't even an important goal. But, by the same token, I do agree there's no point in deviating from what is normal if your deviation is inferior, and that's certainly the case here.

Your XOR-sum is in the EBX register. You know that the XOR-sum tells you the parity in the least-significant bit. So what is the obvious thing to do? Mask off everything but the least-significant bit, and that'll be your answer! How do we mask off bits? Use a logical AND operation:

and  ebx, 1   ; mask off all but the low-order bit

Oh, and finally, since we need the result in EAX, we'll do:

mov  eax, ebx

(We could have just as easily done the MOV first, before the AND. It doesn't matter.)

Go through your current code, and prove to yourself that this gives exactly the same results, without needing to flip the bit with the XOR, without needing to involve the carry flag (CF) via ROR, and without needing to do any conditional branching (JC).

Putting it all together, the code is essentially:

call    read_hex              ; Provided by the teacher. Reads a hexadecimal number from stdin into EAX.
mov     ebx, eax
call    read_hex
xor     ebx, eax
call    read_hex
xor     ebx, eax

mov     eax, ebx              ; Move XOR-sum of three given numbers into EAX.
and     eax, 0x1              ; Mask off all but the lowest-order bit.
call    print_eax_binary      ; Provided by the teacher. Prints a binary number in EAX to stdout.

; Exit the process:
push    0
call    [ExitProcess]

Simpler, clearer, faster. Rarely is it possible to say this about any piece of code, but I believe this really is the most optimal way to write this.


Remove duplicates from unsorted linked list Remove duplicate elements from linked list.

#include<iostream>
using namespace std;

class LinkedList{
private:
    int data;
    LinkedList *next;
public:
    void insert(LinkedList **start, int data){
        LinkedList *p = new LinkedList;
        if (*start == NULL){
            p->data = data;
            p->next = NULL;
            *start = p;
        }
        else{
            LinkedList *temp = *start;
            while (temp->next != NULL){
                temp = temp->next;
            }
            temp->next = p;
            p->data = data;
            p->next = NULL;
        }
    }
    void removeDuplicates(LinkedList **start){//This function removes the duplicates using the standard runner method taking 2 pointers
        LinkedList *temp = *start;
        LinkedList *temp1 = (*start);
        while (temp != NULL){
                while (temp1->next!=NULL){
                    if (temp->data == temp1->next->data){
                        LinkedList *p;
                        p = temp1->next;
                        temp1->next = temp1->next->next;
                        delete(p);
                    }
                    else{
                        temp1 = temp1->next;
                    }

                }

            temp = temp->next;
            temp1 = temp;
        }
    }

    void traverse(LinkedList **start){
        LinkedList *temp = *start;
        while (temp != NULL){
            cout << temp->data;
            temp = temp->next;
        }
    }
};

int main(){
    LinkedList *start = NULL;
    LinkedList p1;
    p1.insert(&start, 9);
    p1.insert(&start, 8);
    p1.insert(&start, 7);
    p1.insert(&start, 9);
    p1.insert(&start, 8);
    p1.traverse(&start);
    p1.removeDuplicates(&start);
    cout << "\n";
    p1.traverse(&start);
    getchar();
    return 0;
}

This takes \$O(N^2)\$ time. Can we do the same in \$O(N)\$ without the use of hash table? Or if mandatory how can we implement it in C++?

How is the overall code quality of the above code? Is there any scope of improvement?

I see a number of things that may help you improve your code.

Make sure you have all required #includes

The code uses getchar but doesn't #include <cstdio>.  Also, carefully consider which #includes are part of the interface (and belong in the .h file) and which are part of the implementation.

Avoid raw pointers

In modern C++, we tend not to use raw pointers very often.  In this case, It would probably be better to have two different classes, one would be a LinkedList class and the other would be a Node class.  That way, instead of starting with a pointer, you can start with an object.

Use nullptr rather than NULL

Modern C++ uses nullptr rather than NULL.  See this answer for why and how it's useful.

Match new with delete

If you allocate memory using new, you must also free it using delete or your program will leak memory.  Since you use new in insert(), you should use delete in the LinkedList destructor which you have not yet written.

Don't abuse using namespace std

Putting using namespace std within your program is generally a bad habit that you'd do well to avoid.

Use more descriptive names

The code's traverse function actually prints the nodes.  For that reason it should probably be named something like print().  Even better would be to have such a function take a std::ostream& argument so it would be possible to print to something other than std::cout.

Omit return 0

If your program completes successfully, the return 0 at the end of main() will be generated automatically, so it's not needed in C++ programs.


Guess the number game I just started to learn Java and had to write a "guess the number" game.

Could you please review and give me some feedback?

public static void main(String[] args) {

    System.out.println("Welcome to the guess the number game!\n");
    System.out.println("Please specify the configuration of the game:\n");

    Scanner input = new Scanner(System.in);

    System.out.println("Range start number (inclusively):");
    int startRange;
    startRange = nextValidInt(input);

    System.out.println("Range end (inclusively):");
    int endRange;
    endRange = nextValidInt(input);

    System.out.println("Maximum number of attemps:");
    int maxAttemp;
    maxAttemp = nextValidInt(input);

    System.out.println("Your Task is to guess the random number between "
            + startRange + " and " + endRange);

    Random randGenerator = new Random();
    int randNumber = randGenerator.nextInt((endRange - startRange + 1)
            + startRange);
    int numberOfTries = 0;

    System.out
            .println("You may exit the game by typing; exit - you may now start to guess:");

    for (numberOfTries = 0; numberOfTries <= maxAttemp - 1; numberOfTries++) {

        int guess;
        guess = nextValidInt(input);

        if (guess == randNumber) {
            System.out.println("Congratz - you have made it!!");
            System.out.println("Goodbye");
            break;
        } else if (guess > randNumber) {
            System.out.println("The number is smaller");
        } else if (guess < randNumber) {
            System.out.println("The number is higher");
        }

    }

    if (numberOfTries >= maxAttemp) {
        System.out.println("You reached the max Number of attempts :-/");
    }

}

public static int nextValidInt(Scanner s) {
    while (!s.hasNextInt() && s.hasNext()) {
        System.out.println("Please provide a valid integer value!");
        s.next();
    }
    return s.nextInt();

}

Not a full review, just a couple of points that jumped out at me:

you often assign the value to a variable on the next line after declaring it. Just assign the value on the same line (eg int startRange = nextValidInt(input);).
you can declare a loop variable inside the loop: for (int numberOfTries = 0; numberOfTries <= maxAttemp - 1; numberOfTries++).
if (numberOfTries >= maxAttemp) will always be true, as the for loop increases numberOfTries to maxAttemp. Well, except when you won. I would remove the if and replace the break with a return.
numberOfTries <= maxAttemp - 1 looks better written as numberOfTries < maxAttemp.
the main method shouldn't do anything except start your program. I would move your code to its own function. You might also want to create extra functions for initial info printing/setup gathering and the actual guessing game.


Internally Safe Filesystem win_xept.hpp - a basic windows api exception
#include "../utl_macro.hpp"

namespace xept
{
    using win_t = struct utl_exception // utl_exception randomly generates a name
                                       // this hides exception names from rtti
    {
        DWORD e = GetLastError( );
    };
}

ramfile.hpp - an interface for any type of file where the data is needed in ram
#pragma once

class ramfile_t
{
public:
    virtual std::uintmax_t size( ) = 0;
    virtual std::vector< std::byte > raw( ) = 0;
    virtual void overwrite( std::vector< std::byte > const & raw,
                            std::size_t const & off = 0 ) = 0;
    virtual void set( std::vector< std::byte > const & raw ) = 0;
};

ramfile_xept.hpp - the basic ramfile exception
#include "../win_xept.hpp"

namespace xept
{
    using ramfile_t = struct utl_exception:
        win_t
    { };
}

ramfile_disk.hpp - basic file for disk
class ramfile_disk_t:
    public ramfile_t
{
public:
    ramfile_disk_t( std::filesystem::path path ):
        path( std::move( path ) ),
        file_guard( file_locks[ this->path.c_str( ) ] )
    {
        assert( path.is_absolute( ) );
    }

    std::uintmax_t size( ) final
    {
        return std::filesystem::file_size( path );
    }

    std::vector< std::byte > raw( ) final
    {
        std::basic_ifstream< std::byte > file( path,
                                               std::ofstream::binary );

        assert( file.is_open( )
                && file.good( ) );

        file.seekg( 0 );

        return std::vector< std::byte >( std::istreambuf_iterator< std::byte >( file ),
                                         std::istreambuf_iterator< std::byte >( ) );
    }

    void set( std::vector< std::byte > const & data ) final
    {
        std::basic_ofstream< std::byte > file( path,
                                               std::ofstream::binary | std::ofstream::trunc );

        assert( file.is_open( )
                && file.good( ) );

        file.write( data.data( ),
                    data.size( ) );
    }

    void overwrite( std::vector< std::byte > const & data,
                    std::size_t const & off = 0 ) final
    {
        std::basic_ofstream< std::byte > file( path,
                                               std::ofstream::binary );

        assert( file.is_open( )
                && file.good( ) );

        file.seekp( off ).write( data.data( ),
                                 data.size( ) );
    }

    void set_attr( bool readonly,
                   bool hidden )
    {
        DWORD attr = 0;

        if ( readonly )
            attr |= FILE_ATTRIBUTE_READONLY;
        if ( hidden )
            attr |= FILE_ATTRIBUTE_HIDDEN;

        if ( 0 == SetFileAttributes( path.c_str( ),
                                     attr
                                         ? attr
                                         : FILE_ATTRIBUTE_NORMAL ) )
            throw xept::ramfile_disk_attr_t( );
    }

protected:
    std::filesystem::path path;

private:
    using file_locks_t = std::unordered_map< std::wstring_view,
                                             std::mutex >;
    static file_locks_t file_locks;

    std::lock_guard< file_locks_t::mapped_type > file_guard;
};

ramfile_disk_xept.hpp - exceptions throwable from ramfile_disk
#include "ramfile_xept.h"

namespace xept
{
    using ramfile_disk_attr_t = struct utl_exception:
        ramfile_t
    { };
}

Design: The design is pretty bad. Think more on how you plan to use it.
Currently, it is prone to deadlocks.
Say, you create two ramfile_disk_t instances in two different places in your code of the same file. The second instance will be blocked until first is finished. Is this really what you want? Is this the only way you want the code to be used?
At least add options like:

"shared usage" - if you only want to read, there is no reason to disable access from several places. Also user may want to manage read/write safety themselves instead of relying on crude methods.
"try open" - return false if locked.
"open" - throw if locked.
"wait for open" - waits until unlocked if locked.
"lock on usage" - lock/unlocks the mutex upon read/write instead of creation/destruction of the class.

Technical Issues:

Avoid #include "../win_xept.hpp". Imagine there is another win_xept.hpp file somewhere and user added an include path that results in loading the wrong win_xept.hpp. This leads to build corruption. If anything you'd better make an extra folder for all your code files just to be extra safe. So you always include #include "my_codebase/current_project/win_xept.hpp". As a bonus your code will be organized better.

Frequently one makes a static create function for abstract classes that will instantiate the implementation depending on user's request. This way, user at no point needs to include implementation's headers.

ramfile_t is a class with virtual functions. Therefore, it is ought to have a virtual destructor (virtual ~ramfile_t() = default). This is especially true for purely abstract classes. Also, do you really think you'll have more that one implementation of the class, why even bother with the interface?

ramfile_disk_t has data-racing issues with the static member file_locks. You ought to guard it with a mutex each time you access it.

I am not 100% sure, but most likely
std::vector< std::byte >( std::istreambuf_iterator< std::byte >( file ),
                          std::istreambuf_iterator< std::byte >( ) );

is a very slow method to read the file. Consider using more low-level functions like fread or read of the iostream instead of the iterator-based approach. At least make some tests.

It is a bit nitpicky, but is there any reason to pass std::size_t const & off = 0 as const reference in the overwrite method? Just pass it by value std::size_t off; not only passing by reference is slower, it becomes nuisance once you attempt to schedule the function in an asynchronous thread.


PHP Ngrams comparing two arrays Is there some more efficient way to calculate and extract that array that is greater than or equal to 0.8.

I had to use two foreach to do it, which is not very performant.

The idea is to find the most similar expressions comparing this two arrays.

Thanks!

function getNgrams($match, $n = 2) {

            $ngrams = array();
            $len = strlen($match);
            for($i = 0; $i < $len; $i++) {
                    if($i > ($n - 2)) {
                            $ng = '';
                            for($j = $n-1; $j >= 0; $j--) {
                                    $ng .= $match[$i-$j];
                            }
                            $ngrams[] = $ng;
                    }

            }
            return $ngrams;

    }

$question = 'happy birthday';
$rta = array('today', ' tomorrow ', 'monday', 'birthda');

$question_array = explode(' ',($question));
#print_r($question_array);

$ngram_question = array_map("getNgrams",$question_array);
$ngram_rta = array_map("getNgrams",$rta);
#print_r($ngram_rta);

foreach ($rta as &$valor) {

    foreach ($question_array as &$valorq) {

        $ngram_rta = getNgrams($valor);
        $ngram_q = getNgrams($valorq);

        if ((sizeof(array_intersect($ngram_rta,$ngram_q))/sizeof($ngram_rta)) >= 0.8)
            {echo $valor;
             echo "<br>";
            }

    }
}

I'm pretty sure I don't understand the usage of your script, but I'll assume you have a good reason for performing this comparison.

I see that you are declaring/calling $ngram_rta = getNgrams($valor) inside your inner loop.  This means that for each word in your question string, you are asking php to do the same job to produce the same output.  You must not ask php to repeat itself.  Since $ngrame_rta is not mutated inside of the inner loop, you should generate the data inside the outer loop and count its length before entering the inner loop.

In your posted script, if($i > ($n - 2)) { is never satisfied on the first iteration.  I'll urge you to have a re-think about this condition.  Perhaps you want to make the 2 dynamic and perhaps you want to change the comparison to >=.  You decide how to avoid performing an usable iteration.

If the idea here is to break a word into an array of strings with $n length starting from each offset in the string then this seems cleaner:

(Demo)

for ($pos = strlen($match) - $n; $pos >= 0; --$pos) {
    $nGrams[] = substr($match, $pos, $n);
}

I guess your script could boil down to something like this:

(Demo)

function getNgrams($match, $n = 2) {
    $nGrams = [];
    for ($pos = strlen($match) - $n; $pos >= 0; --$pos) {
        $nGrams[] = substr($match, $pos, $n);
    }
    return $nGrams;
}

$question = 'happy birthday';
$questionNGrams = array_map("getNgrams", explode(' ', $question));

$rta = ['today', 'happy', ' tomorrow ', 'monday', 'birthda'];

foreach ($rta as $value) {
    $rtaNGram = getNgrams($value);
    $rtaNGramCount = count($rtaNGram);
    foreach ($questionNGrams as $questionNGram) {
        if (count(array_intersect($rtaNGram, $questionNGram)) / $rtaNGramCount >= 0.8) {
            echo "{$value}
";
            break;  // prevent further iteration of inner loop after $value qualifies
        }
    }
}

Output:

happy
birthda

I do find my eyes going a bit crossed as I continue to read your code because I don't relate $question and $rta to the data they contain.  Perhaps be more accurate/intentional about the variable naming.


Extract Element and its Followers from List I wrote the following function to convert List[Int] => List[List[Int]].

Its intended purpose is to extract 1, followed by its non 1 followers, into a group.

input: List(1,2,2,1,3,3)

output: List( List(1,2,2), List(1,3,3) )

Function

def leadingOnesGroup(xs: List[Int]): List[List[Int]] =
        xs match {
          case Nil    => Nil
          case _ :: _ =>
           val (first, rest)          = xs.span(_ == 1)
           val (followers, remainder) = rest.span(_ != 1)
           (first ++ followers) :: leadingOnesGroup(remainder)
      }

Testing

scala> leadingOnesGroup(List(1,2,2))
res19: List[List[Int]] = List(List(1, 2, 2))

scala> leadingOnesGroup(List(1,2,2,1,3,3))
res20: List[List[Int]] = List(List(1, 2, 2), List(1, 3, 3))

scala> leadingOnesGroup(List(1,2,2,1,3,3,1,5,5))
res21: List[List[Int]] = List(List(1, 2, 2), List(1, 3, 3), List(1, 5, 5))

The current version of the function does not take into account the cases:

(as @itsbruce mentioned) if the list starts with a value other than '1'
if there are consecutive '1's

The second case is using wildcards, but instead it'd better directly match a list starting with '1':

case 1 :: tail => {
  // we take only the following elements different from 1
  val followers = tail.takeWhile(_ != 1)
  // we drop the 'followers' prefix for the recursive call
  val remainder = tail.drop(followers.size)
  (1 :: followers) :: leadingOnesGroup(remainder)
}

Of course, this block can be shortened to two lines only, but it will be less readable.

Since it is not clear how to process lists that do not start with '1', a generic matcher with an explicit exception can be added:

case _ => throw new IllegalArgumentException("Unexpected start of list: " + xs)


Use of Ninject and an injectable DataLayer in MVC3 I'm trying to make an MVC3 application with an exchangeable data layer. I currently have this in a single file (Global.asax.cs) for brievity.

It seems to be working as I want but as I'm new to DI I really would appreciate comments on this code.

public interface IDataLayer
{
    dynamic GetItem(int id);
    dynamic SaveItem(dynamic item);
}

public class MockDataLayer : IDataLayer
{
    public dynamic GetItem(int id)
    {
        dynamic item = new System.Dynamic.ExpandoObject();
        item.Id = id;
        item.Name = "Some name";
        return item;
    }
    public dynamic SaveItem(dynamic item)
    {
        return item;
    }
}
public class MainController : Controller
{
    public ActionResult Display(int id = 0)
    {
        dynamic viewModel = Global.DataContext.GetItem(id);
        return View(viewModel);
    }
}
public class Global : System.Web.HttpApplication
{
    public static IDataLayer dataContext;

    protected void Application_Start(object sender, EventArgs e)
    {
        IKernel kernel = new StandardKernel();

        kernel.Bind<IDataLayer>().To<MockDataLayer>();
        dataContext = kernel.Get<IDataLayer>();

        RouteTable.Routes.MapRoute("", "{id}", new { controller = "Main", action = "Display", id = UrlParameter.Optional });
    }
}

One tip I would give is to use the "Ninject for ASP NET MVC 3" NuGet package. Install that package into your project. After doing this, you should find a file named "NinjectMVC3.cs" located in the "App_Start" folder. In this file, look for a method named RegisterServices():

private static void RegisterServices(IKernel kernel)
{
    kernel.Bind<IDataLayer>().To<MockDataLayer>().InRequestScope();
}

This sets up a MockDataLayer to be returned for each separate web request. This ensures that MockDataLayer is disposed when the web request is completed. Now, when you want to create an instance of the data layer, use MVC's built-in DependencyResolver class:

var dataContext = DependencyResolver.Current.GetService<IDataLayer>();


JavaScript game tick How can I achieve a more performant tick code keeping my minimum frame rate high?

installLoops() {
        window.requestAnimationFrame(this.loop.bind(this));
    }

    loop() {
        this.now = Date.now();
        var delta  = this.now - this.last;
        this.last = this.now;

        this.dt = this.dt + delta;

        if (this.dt < this.rate) {
            window.requestAnimationFrame(this.loop.bind(this));
            return;
        } else {
            this.game.loop();
            this.draw();
            this.dt = this.dt - this.rate;
            this.game.loopKeyboardInput(this.key_pressed_map);
            this.key_pressed_map = {};
        }

        window.requestAnimationFrame(this.loop.bind(this));
    }

I moved

this.game.loopKeyboardInput(this.key_pressed_map);
    this.key_pressed_map = {};

to inside the block where the game loop is called, which improved my average from rate from 12.56 to 56+. However, in performance testing my framerate still drops down to 13 (lowest). (I'm not sure if this is happening because of the performance sampling).

One issue I see is you're using bind. bind creates a copy of the function but with a bound context. You appear to call it on each iteration, which will cause memory hogging, which causes the GC to kick in more often, thus lagging. You can simply just drop the use of bind and write your code without relying on context.

If you need to store state, store it somewhere else, like a single module holding your game state. It's also good to isolate state into one location instead of having it everywhere.

The render loop should just be a render loop. It should not be aware of anything else except a queue of things to do on each turn. On the other hand, the rest of the code shouldn't be aware of a render loop. All they need to do is to register operations to the queue, with only the assumption that the function they register gets executed per frame. This keeps code relatively decoupled with the scheduling mechanism.

In the following, we create a jobQueue which is just a list of things to do when the loop executes.

// The render loop is only aware of a job queue full of functions
const jobQueue = [];

function renderLoop(elapsed){
  requestAnimationFrame(renderLoop);

  // Consume tasks in jobQueue
  jobQueue.forEach( task => task.call(elapsed / (1000/60)) );
}

// Start the loop
renderLoop();

// All your code needs to do now is register
jobQueue.push(scale => { update some state });
jobQueue.push(scale => { render something });
jobQueue.push(scale => { check keyboard });

Also, it's best if you schedule the next frame prior to doing stuff. That way, anything that causes the loop to lag doesn't lag scheduling the next frame.

Another thing is timing. requestAnimationFrame's callback accepts 1 argument, a timestamp. Use that instead of Date.now. Now instead of using delta time (a time value) in your calculation, consider using a scale (a unit-less value). I've answered a similar review here. With delta time, you have to change your calculation to take in a time value. With a scale, you simply calculate output values, then multiply against the scale value. It's like a percentage value.

Lastly, the example above uses forEach for jobQueue. It's known to be slow. Feel free to replace it with a regular for or while loop.


Triangle Tessellation Project I have this Python project to draw tessellated triangles on the screen. It takes the height to form the size of the triangle, cols to specify how many triangles on a line, and rows which is how many lines it outputs.
This program has taken me some time to put together and I'm not sure if this is the best way to do this. The first line is just the normal triangle, the second line and every other line after however only should show half of the triangle on the first and last column.

Here's my .py file:

import sys

print("make sure shell in full screen so the triangles are viewed properly... or disable word wrap :)")
height          = int(input("height"))
cols            = int(input("cols"))
rows            = int(input("rows"))

MAX_PAGE_WIDTH = 19 * 8

width = height * 2 - 1
if((width * cols) > MAX_PAGE_WIDTH):
    print("Triangles not gonna fit on page, please enter smaller params... :/")
    sys.exit()

def get_even_number(num):
    return num % 2 == 0

triangle        = ""

for column_row in range(rows):
    half_space      = height - 1
    stars           = 1

    for row_of_triangle in range(height):
        if(get_even_number(column_row)):
            for column in range(cols):
                triangle += " " * (half_space + 1) + "*" * stars + " " * half_space
        else:
            for column in range(cols + 1):
                if(not(column == 0)):
                    triangle += " " * (half_space + 1)

                if(column == 0 or column >= cols):
                    sub = row_of_triangle
                    if(column >= cols):
                        sub += 1
                    triangle += "*" * (stars - sub)
                else:
                    triangle += "*" * stars

                if(column < cols):
                    triangle += " " * half_space

        half_space      -= 1
        stars           += 2
        triangle        += "\n"

print(triangle)

Here is some sample output:

input: height: 6 cols: 6 rows: 4

      *           *           *           *           *           *
     ***         ***         ***         ***         ***         ***
    *****       *****       *****       *****       *****       *****
   *******     *******     *******     *******     *******     *******
  *********   *********   *********   *********   *********   *********
 *********** *********** *********** *********** *********** ***********
*           *           *           *           *           *
**         ***         ***         ***         ***         ***         *
***       *****       *****       *****       *****       *****       **
****     *******     *******     *******     *******     *******     ***
*****   *********   *********   *********   *********   *********   ****
****** *********** *********** *********** *********** *********** *****
      *           *           *           *           *           *
     ***         ***         ***         ***         ***         ***
    *****       *****       *****       *****       *****       *****
   *******     *******     *******     *******     *******     *******
  *********   *********   *********   *********   *********   *********
 *********** *********** *********** *********** *********** ***********
*           *           *           *           *           *
**         ***         ***         ***         ***         ***         *
***       *****       *****       *****       *****       *****       **
****     *******     *******     *******     *******     *******     ***
*****   *********   *********   *********   *********   *********   ****
****** *********** *********** *********** *********** *********** *****

Using the * operator to repeat strings is smart, but there are still a lot of nested ifs and and fors.  In Python, excessive looping can often be remedied using list comprehensions and itertools.

It would definitely be a good idea to split the work into functions.  The input validation code is unrelated to the output routine.  The output routine, in turn, can be decomposed into smaller reusable units.  In particular, it is helpful to observe that the odd rows are like the even rows, but upside-down and colour-inverted.

from itertools import chain, cycle, islice

def tesselated_triangles(height, cols, rows):
    def triangle_row(height, cols, space=' ', fill='*'):
        return [
            (
                space * (height - i) +
                fill * (2 * i + 1) +
                space * (height - i - 1)
            ) * cols
            for i in range(height)
        ]

    def alt_triangle_row(height, cols, space=' ', fill='*'):
        return reversed(triangle_row(height, cols, space=fill, fill=space))

    return '\n'.join(chain.from_iterable(
        row(height, cols)
        for row in islice(cycle([triangle_row, alt_triangle_row]), rows)
    ))

print(tesselated_triangles(6, 6, 4))

Additional remarks:

Since strings in Python are immutable, repeated string concatenation should be avoided.  Appending just a single character using += involves allocating and copying the entire string.  For this application, you should use '\n'.join(…).  (Doing '\n'.join(…) would also avoid printing two newlines at the end of the output, which is wrong in my opinion.)
If the program failed, it would be a good idea to call sys.exit() with a non-zero status code.
It is uncommon in Python to write if(…): using parentheses.
Using extra spaces to align equals signs is discouraged in PEP 8, the official Python style guide.  In this case, I really think you have overdone it.

half_space      -= 1
stars           += 2
triangle        += "\n"

get_even_number() is poorly named.  You're not retrieving anything; you're testing something.  An appropriate name would be is_even_number() or simply is_even().  You could just eliminate the function, though, since it's so simple.


Self-hosting WCF in Windows Forms without another Windows process I have a Windows application and want to self-host a WCF in it. This MSDN article walks you through how to self-host WCF in a console. Jason Henderson's article demonstrates how to call the service. But the problem is, I don't want to host my service in another Windows process. I want to host it in my client application.

Here is my workaround:

CtrlF5 to run the service
Add service reference to my client
application

Then I can start my service in my client like this:

static void Main()
{
    ServiceHost host = new ServiceHost(typeof(MyService));
    host.Open();
    Application.Run(new Form1());
    host.Close();
}

It works, but I wonder if there are any simpler ways to do this.

You can also start host in another thread:

Task.Factory.StartNew(() =>
    {
            ServiceHost host = new ServiceHost(typeof(MyService));
            host.Open();
    };

(or using classic Thread and ThreadStart).


Shift an element in a circular container I implemented a function to shift an element after another in a logical circular array. Shifting in this case simply means moving element at position i to in front of element at position j. Of course I can't simply swap the elements at position i and next(j), because I would alter the positions of two elements in this circular array.

Here's the code I came up with:

std::vector<SomeType> circular_container;

/*
 * Move node at index i in front of node j
 * by shifting all nodes by 1 behind until j.
 * */
void shift(int i, const int j) {

    if (circular_container.size() < 2) {
        return;
    }

    int n = next(i);
    while(i != j) {
        std::swap(circular_container[i], circular_container[n]);
        i = n;
        n = next(i);
    }

}

And the next function looks like:

/*
 * Returns the index of the node after the node at index i.
 * */
int next(const int i) {
    return (i + 1) % circular_container.size();
}

Is the shift operation correct? If it is, any way to improve it?

Don't get me wrong. The function currently is almost useless, since you are rotating only circular_container. People would like to rotate any container on pair of iterators, as mentioned by @Jerry. But lets put those things aside, and have a look at your code.

Code Review

std::vector<SomeType> circular_container;

Global variable, very bad. Here's why. You could at least write template variable (don't do that!).

void shift(int i, const int j)

OK lets imagine for a minute that someone really filled the global variable and wants to rotate now. But then that someone gets upset, because apparently int is not large enough to hold the index he/she wants to rotate. Always use at least std::size_t defined in <cstddef>, or better use std::vector<SomeType>::size_type. This guarantees you that the type will be large enough to hold the biggest possible index.

if (circular_container.size() < 2) {
    return;
}

You are creating a branch that should be taken care of by the loop (it actually does, I believe). If the size of the container is 1, caller should provide equal i and j (you are not performing range check anyway). If the size of the container is zero, caller shouldn't even call your function at all! So the above if is redundant.

Conclusion:

It does the needed job. Sometimes. In a very clumsy way. I would arguably say that this is better than nothing, but there is a huge room for improvement.

Suggestions on better implementation:

As @Jerry noted, you should create iterator that performs the logic of your next() function. Probably you'll want to provide one version for forward, another for bidirectional, and the last one for random access iterators. The only thing you need to do is to move the logic from next() to the operator++(), for example.

I would also like to have a function that returns me circular iterator range taking pair of begin and end iterators, so that I could pass it into the function which does similar things to your shift function, and then specify by how much I want to shift, like the count of shifts to the right. You can also provide bool parameter that specifies the direction, if iterator supports it. Also, you could harness the power of random access iterators to make count shifts at once, rather than calling the shift in the loop.


A less limited Python Lambda I've often wanted an anonymous function that would do more than return the result of an expression.  So I developed one around the exec function that supports assignments and multiline lambdas but suspect a few things are missing.  Suggestions would be appreciated, please.

I've tried to keep the syntax similar to a normal lambda and also easy to debug by copying the text into a standard function.

Tested with CPython 3.6.5 and Pypy 3.5.3.  To give an idea of the current version, it is capable of evaluating these functions:

λ("""var:
     for i in range(var):
         print(i, end = ",")
         print(' ', end='')
         if i == var - 1:
             print(var)
     print(var)
     rtrn = [i for i in range(var, 0, -1)]
""")(5)

0, 1, 2, 3, 4, 5
5
[5, 4, 3, 2, 1]

λ("x: [x*i for i in range(x)]; print('Final:', rtrn[-1])")(3)

Output: 6
[0, 3, 6]

λ("x, y:: from math import cos, sin; rtrn = cos(x) + sin(y)")(3, 5)

-1.9489167712635838

def lambda_(code):
    """
    Anonymous function supporting multiple line and assignments.
    For one line the form is λ("parameters : code incl assignment")(*arguments)
      and the first exepression is assigned to rtrn which is returned
    For multiline, use triple quotes & the parameters must be on the first line,
      the : is followed directly by a line feed and then the code
    Input:
        code in the form "x, y,...: return_value_expression; other code"
    Output is assigned to variable rtrn
    """
    def build_lambda(*args):
        """
        All arguments are declared global as listcomps etc don't create closure
        when called using exec, so declare parameters & rtrn as globals
        Multiline will return None unless you assign rtrn a value.
        """
        val_idx, val_name = 0, []
        i, parameters = -1, []
        if ":" in code:
            for i, c in enumerate(code):  # loop until the : that ends parameters
                if c in ",:":
                    if val_name:
                        parameters.append(''.join(val_name))
                        val_name = []
                        val_idx += 1
                    if c == ":":
                        break
                elif c != " ":
                    val_name.append(c)

        # exec_code sets rtrn = None for multiline or sets rtrn = first expression for
        #    single line unless :: (used if first expression can't be evaluated)
        if code[i+1] != "\n":  # single line,
            if code[i+1] != ":":
                exec_code = ''.join(("rtrn = ", code[i+1:]))
            else:
                exec_code = ''.join(("rtrn = None; ", code[i+2:]))
        else:  # multiline
            # wont run if excess leading spaces so remove them
            lead = 0
            code_ = code[i+1:]
            while code_[lead+1] == " ":  # how many on 1st line?
                lead += 1
            for i in range(1, len(code_)):  # remove that on all lines
                if code_[i-1: i+1] == "\n ":
                    code_ = code_[: i] + code_[i+lead:]
            exec_code = "rtrn = None" + "\n" + code_  # executable code

        define_globals = ', '.join(["global rtrn"] + parameters)
        assign = [' = '.join((p, str(args[i]))) for i, p in enumerate(parameters)]
        assign = '; '.join(assign)
        cmd = '; '.join((define_globals, assign if assign else "pass", exec_code))
        if args and args[-1] == "print":
            print(cmd, "\n")
        exec(cmd)
        return rtrn

    return build_lambda

λ = lambda_  # rebind to linux ctrl+shift+u 03BB

With the added complexity it seems more reasonable to create a class splitting  code generation, execution and printing.  The code is posted in this question.

Naming:

λ is not a standard Python function name ([a-z] followed by [a-z0-9_]+) and will require figuring out some magic sequence in non-Greek keyboard layouts.
s0, s1, s2, lead and cd should be renamed to be more intuitive. For example, based on your documentation s1 could be named something like assignments.

General:

Rather than adding strings and newlines together I'd use format() and join(). It's easier to follow the construction of the string that way.
I would not recommend using this in production code. Treating strings as code is going to result in many more runtime errors which will be harder to debug than the corresponding named function.
λ does not accept functions without parameters - λ('print("value")')() for example results in IndexError.
Code construction and execution should be split up for easier debugging. I should be able to run construct_code(my_code_string) and see what would be executed for debugging.
I don't have the time to test this myself, but you should be able to get rid of the global rtrn by constructing an actual function and running that instead of creating global code.


Menu visibility toggle method I've just created a method that toggles menu visibility. At the moment I've gone with an if..else which checks my flag and acts appropriately. Can anyone offer any suggestions on how to make this snippet better? Maybe it shouldn't all live in the same snippet? Maybe I should be combining the show/hide logic that's duplicated in the if..else?

onNavTriggerClick: function(event) {

            var $currentTarget = $(event.currentTarget);

            if(this._navOpen) {
                Backbone.Events.trigger('hide:menu');
                $currentTarget.removeClass('is-active');
                this._navOpen = false;
            } else {
                Backbone.Events.trigger('show:menu');
                $currentTarget.addClass('is-active');
                this._navOpen = true;
            }
        }

You already mentioned the duplication, so let's address that issue.

First, I'd move the assignment to a common block so it's clear that
you're toggling the visibility.

Then for toggling the CSS class I found the
toggleClass function, so maybe
go with that.

After that moving the assignment to the top, then working based on the
new state gives me this:

onNavTriggerClick: function(event) {
  this._navOpen = !this._navOpen;
  Backbone.Events.trigger(this._navOpen ? 'show:menu' : 'hide:menu');
  $(event.currentTarget).toggleClass('is-active', this._navOpen);
}

Looks a bit more concise.


Project Euler # Triangular, pentagonal and hexagonal in Python Triangle, pentagonal, and hexagonal numbers are generated by the
  following formulae:

  Triangle: \$T_n=\frac{n(n+1)}{2}\$ 1, 3, 6, 10, 15, …
  Pentagonal: \$P_n=\frac{n(3n−1)}{2}\$ 1, 5, 12, 22, 35, …
  Hexagonal: \$H_n=n(2n−1)\$ 1, 6, 15, 28, 45, …

  It can be verified that \$T_{285} = P_{165} = H_{143} = 40755\$.

  Find the next triangle number that is also pentagonal and hexagonal.

from time import time

def tri_pent_hex(n):
    """yields triangles that are also pentagons and hexagons in range n."""
    triangles = set((num * (num + 1)) // 2 for num in range(1, n))
    pentagons = set((num * (3 * num - 1)) // 2 for num in range(1, n))
    hexagons = set(num * (2 * num - 1) for num in range(1, n))
    for triangle in triangles:
        if triangle in pentagons and triangle in hexagons:
            yield triangle

if __name__ == '__main__':
    start_time = time()
    print(max(tri_pent_hex(1000000)))
    print(f'Time: {time() - start_time} seconds.')

The way this was written, it was meant to steer you in the direction you did.

  Find the next triangle number that is also pentagonal and hexagonal.

You loop the triangles to find matches in the others:

for triangle in triangles:
    if triangle in pentagons and triangle in hexagons:
        yield triangle

But if you think about it, there are more triangles than pentagons and more pentagons than hexagons. And since you need to find occurences where all 3 types match, wouldn't it be better to loop the hexagons, and find a match on the others from there?

My point is:

Don't let the way a question is asked influence the way you implement an algorithm.


Use of scoped objects in a JavaScript Tic-tac-toe implementation I have been using the w3 org's JavaScript Best Practices web page's examples for coding an object literal for a game.

It is my understanding that I should use this to keep all my variables and arrays that are not associated with/inside functions. I should then use methods to access and update the values and arrays.

I wanted to make sure I was on the right track (and learning code practices) before I got too far into the code.

So, using the code listed below, I would access values as follows:

var valueSquareA = t3Game.checkSquare(a);

and

var arrSq = t3Game.getSquares();

var t3Game = (function(){

    var playerImg = "images/blankSpace.gif";
    var compImg = "images/blankSpace.gif";

    var squares = []; // For keeping track of the board.

    return{
        getImgFileName: function(playerOrComp){
            if(playerOrComp ==="player"){
                return playerImg;
            }
            else {
                return compImg;
            }
        },

        setImgFileName: function(choice){
            if(choice === "X"){
                playerImg = "images/X.gif";
                compImg = "images/O.gif";
            }
            else {
                playerImg = "images/O.gif";
                compImg = "images/X.gif";
            }
        },

        setBoard: function(){
            for(i=1; i<=9; i++){
                squares[i] = "empty";
            }
            // alert(squares);
        },

        setSquare: function(nbr, playerOrComp){
            squares[nbr] = playerOrComp;
            // alert(squares);
        },

        checkSquare: function(nbr){
            return squares[nbr];
        },

        getSquares: function(){
            return squares;
        }

        } // end return

})();

You're on the right track. Now a few things before stuff gets out of hand:

Not all your functions need to be on the literal. What return {} is doing is exposing stuff in the literal as t3Game in the global scope for other code to interact with your code. If you have functions that need not be exposed, define then in the closure. It will only be accessible by code within the closure, but not outside.

var t3Game = (function(){

  function myHiddenFunctionThatNeedsNotExposed(){...}

  return {...}

})();

} // end return

If you do proper indentation, then you don't need such markers. If you use an IDE, proper indentation will allow you to collapse blocks of code as well as show indent guide lines as long as you indent properly.

An alternate way to define a global namespace is the following:

;(function(ns){

  var sameVars = []

  function theHiddenFunction(){...}

  ns.myExposedFunction = function(){...}

}(this.t3Game = this.t3Game || {});

t3Game.myExposedFunction (...);

What this does is check if t3Game already exists. If it exists, it uses the existing one, and if not, uses a new literal. This way you don't accidentally destroy an existing object of the same name in the global scope.

for(i=1; i<=9; i++){
  squares[i] = "empty";
}

Arrays zero-indexed so it's best if you start with 0. Also, 9 is very ambiguous. We don't know what 9 is. Best if you put constant values in a variable with an appropriate name.

getImgFileName: function(playerOrComp){
    if(playerOrComp ==="player"){
        return playerImg;
    }
    else {
        return compImg;
    }
},

can be shortened to:

getImgFileName: function(playerOrComp){
  return playerOrComp ==="player" ? playerImg : compImg;
}

setImgFileName: function(choice){
    if(choice === "X"){
        playerImg = "images/X.gif";
        compImg = "images/O.gif";
    }
    else {
        playerImg = "images/O.gif";
        compImg = "images/X.gif";
    }
},

can be shortened to:

const X_IMAGE = "images/X.gif";
const O_IMAGE = "images/O.gif"

setImgFileName: function(choice){
  const isX = choice === "X";
  playerImg = isX ? X_IMAGE : O_IMAGE;
  compImg   = isX ? O_IMAGE : X_IMAGE;
}

Again, any constant value must be defined in a variable with a good name.

Lastly, code is for humans. Write verbose names for functions and variables so that the code is understandable.


Binary tree max sum level - better design? I have written some code for finding a level in a binary tree, having a maximum number of elements. I have a few questions:

Is it a good design? I have used 2 queues but the total sum of elements both queues store will be less than n. So I think it should be OK.
Can there be a better design?

    public class MaxSumLevel {
    public static int findLevel(BinaryTreeNode root) {
        Queue mainQ = new Queue();
        Queue tempQ = new Queue();
        int maxlevel = 0;
        int maxVal = 0;
        int tempSum = 0;
        int tempLevel = 0;
        if (root != null) {
            mainQ.enqueue(root);
            maxlevel = 1;
            tempLevel = 1;
            maxVal = root.getData();
        }
        while ( !mainQ.isEmpty()) {
            BinaryTreeNode head = (BinaryTreeNode) mainQ.dequeue();
            BinaryTreeNode left = head.getLeft();
            BinaryTreeNode right = head.getRight();
            if (left != null) {
                tempQ.enqueue(left);
                tempSum = tempSum + left.getData();
            }
            if (right != null) {
                tempQ.enqueue(right);
                tempSum = tempSum + right.getData();
            }
            if (mainQ.isEmpty()) {
                mainQ = tempQ;
                tempQ = new Queue();
                tempLevel ++;
                if (tempSum > maxVal) {
                    maxVal = tempSum;
                    maxlevel = tempLevel;
                    tempSum = 0;
                }
            }
        }
        return maxlevel;
    }
}

My initial criticism is that your method doesn't have javadoc comments that state clearly and unambiguously what the method is supposed to do.

Why am I saying that?

Well, mainly because your Question has exactly the same problem!  You say:

  I have written some code for finding a level in a binary tree, having a maximum number of elements.

This is bad description of what you are (apparently) trying to do:

The word "level" usually refers to the distance of a node from (say) the root of a tree.  But you apparently mean the "height" of the entire tree.
The phrase "having a maximum number of elements" is unclear.  What are you talking about?  How does this affect the problem?  (I don't see a maximum number of elements parameter ... or any reference to this in your code.)

So why does this matter?

Because I (the reader) should not have to read through your code to try to figure out the problem you are trying to solve.  Especially if your implementation is non-obvious ... which it is ... and possibly contains mistakes that might lead me to think it is solving a different problem than you are actually trying to solve.


Entity Framework configuration I tried to refactor my code with entity framework configuration, splitting in multiple private methods, one per entity, each responsible for configuring things related to that entity and only that entity. But I feel like Version 2 can still be improved.

What do you think?

Version 1:

protected override void OnModelCreating(DbModelBuilder modelBuilder)
{
    modelBuilder.Entity<Country>()
        .Property(e => e.Code)
        .IsFixedLength()
        .IsUnicode(false);

    modelBuilder.Entity<Country>()
        .Property(e => e.PhoneCode)
        .IsFixedLength()
        .IsUnicode(false);

    modelBuilder.Entity<Country>()
        .HasMany(e => e.Locations)
        .WithRequired(e => e.Country)
        .WillCascadeOnDelete(false);
}

Version 2:

protected override void OnModelCreating(DbModelBuilder modelBuilder)
{
    Configure(modelBuilder.Entity<Country>());
}

Configure(EntityTypeConfiguration<Country> entity)
{
    entity
        .Property(e => e.Code)
        .IsFixedLength()
        .IsUnicode(false);

    entity
        .Property(e => e.PhoneCode)
        .IsFixedLength()
        .IsUnicode(false);

    entity
        .HasMany(e => e.Locations)
        .WithRequired(e => e.Country)
        .WillCascadeOnDelete(false);
}

My approach is to keep the XxxxxDbContext class as lightweight as possible:

protected override void OnModelCreating(DbModelBuilder modelBuilder)
{
    new ModelCreator().Execute(modelBuilder);
}

The ModelCreator class looks like this:

internal class ModelCreator
{
    public void Execute(DbModelBuilder modelBuilder)
    {
        modelBuilder.Conventions.Remove<PluralizingTableNameConvention>();
        modelBuilder.Conventions.Remove<OneToManyCascadeDeleteConvention>();

        modelBuilder.Configurations.Add(new FirmConfiguration());
        // etc.
    }
}

Then each table has its own EntityTypeConfiguration<TEntityType>:

internal class FirmConfiguration : EntityTypeConfiguration<Firm>
{
    public FirmConfiguration()
    {
        HasKey(x => new { x.Id });

        Property(t => t.Name).HasMaxLength(30).IsRequired();
        Property(t => t.Address).HasMaxLength(120).IsRequired();
        Property(t => t.PostalCode).HasMaxLength(4).IsRequired();
        Property(t => t.City).HasMaxLength(30).IsRequired();
        Property(t => t.Responsible).HasMaxLength(30).IsRequired();
        Property(t => t.Email).HasMaxLength(40).IsRequired();
        Property(t => t.Phone).HasMaxLength(14);
    }
}


Interface-based polymorphic collection Here is a small C++11 utility collection to store any object that satisfies a given interface. Only the basic concepts are provided, I did not try to create a full collection:

#include <memory>
#include <utility>
#include <vector>
#include <boost/iterator/indirect_iterator.hpp>

template <typename Interface, template <typename> class Adapter>
class poly_collection
{
public:
    using iterator = boost::indirect_iterator<typename std::vector<std::unique_ptr<Interface>>::iterator>;

    template<typename T, typename... Args>
    auto push(Args&&... args)
        -> void
    {
        auto ptr = new Adapter<T>{ std::forward<Args>(args)... };
        entities.emplace_back(ptr);
    }

    template<typename T>
    auto push(T&& other)
        -> void
    {
        auto ptr = new Adapter<T>{ std::forward<T>(other) };
        entities.emplace_back(ptr);
    }

    auto begin()
        -> iterator
    {
        return { std::begin(entities) };
    }

    auto end()
        -> iterator
    {
        return { std::end(entities) };
    }

private:
    std::vector<std::unique_ptr<Interface>> entities;
};

Here is an example of use. Imagine you have an abstract class named Shape. For the sake of simplicity, it will only have one abstract method, draw:

struct Shape
{
    virtual void draw() const = 0;

    virtual ~Shape() {};
};

Now, you have some nice classes, Rectangle and Circle, that you didn't write but that satisfy the Shape interface:

struct Circle
{
    Circle(int x, int y, int radius) {}

    void draw() const
    {
        std::cout << "Circle\n";
    }
};

struct Rectangle
{
    Rectangle(int x, int y, int height, int width) {}

    void draw() const
    {
        std::cout << "Rectangle\n";
    }
};

You would like to store Rectangle and Circle instances in a colleciton as if they inherited from a common base class. With our solution, you only have to write a new class: ShapeAdapter, and then feed it to the poly_collection:

template<typename T>
struct ShapeAdapter:
    Shape
{
    template<typename... Args>
    ShapeAdapter(Args&&... args):
        data(std::forward<Args>(args)...)
    {}

    virtual void draw() const override
    {
        data.draw();
    }

    T data;
};

int main()
{
    poly_collection<Shape, ShapeAdapter> shapes;

    shapes.push<Circle>(1, 2, 3);
    shapes.push<Rectangle>(4, 5, 6, 7);

    Circle c = { 8, 9, 10 };
    shapes.push(c);

    for (auto&& shape: shapes)
    {
        shape.draw();
    }
}

So, what do you think of the idea? Would that be an interesting utility? Is there any way I could improve such a collection appart from adding some functions to mimick the standard library's collections ones?

As usual with your code, I can only come up with very minor nitpicks.

With both push functions, there is a small chance of a memory leak. After the construction of a new Adapter<T> using:

auto ptr = new Adapter<T>{ std::forward<Args>(args)... };

If entities.emplace_back(ptr); fails (say if relocation is required, the vector needs to expand, and this fails), then ptr will leak. Instead of doing it this way, I'd suggest using make_unique and moving the unique_ptr into entities using push_back instead:

template<typename T, typename... Args>
auto push(Args&&... args)
    -> void
{
    auto ptr = std::make_unique<Adapter<T>>(std::forward<Args>(args)...);
    entities.push_back(std::move(ptr));
}


Idiomatic F# for iterating a seq and mapping whenever changes occur on a given selection I've written a function that takes a sequence and check for every item in that sequence whether the current value a selection (obtained with a selection function) is different (or on the first iteration) from the previous computed value and if that's the case (we considered there is a change), then map the current item and returns a new mapped item along the current item.

If there is no change the sequence keeps returning the current item and the last mapped item detected due to a change.

let differentOrNew option currentValue =
    match option with
    | Some value when currentValue <> value ->
        true
    | Some _ ->
        false
    | None ->
        true

let def<'T> =
    Unchecked.defaultof<'T>

let mapChange selector mapping seq =
    let mutable previous = None
    let mutable change = def

    seq
    |> Seq.map(fun item ->
        let currentValue = selection item
        if differentOrNew previous currentValue then
            previous <- Some currentValue
            change <- mapping item
        item, change)
    |> Seq.map(fun state -> fst state, snd state)

That code works, but it's clearly not the best idiomatic piece of F# code I've ever seen.

I am wondering how to get something more "F#-idiomatic".

|> Seq.map(fun state -> fst state, snd state)

I think this last operation is unnecessary as it is a map from 'a * 'b to 'a * 'b (fst returns the first element in the tuple and snd the second).

If you want to get rid of the mutable variables, you can use Seq.mapFold instead:

let mapChange selector mapping seq =
    seq
    |> Seq.mapFold (fun (previous, change) item ->
        let currentValue = selector item
        match differentOrNew previous currentValue with
        | true ->
            let newChange = mapping item
            (item, newChange), (Some currentValue, newChange)
        | false ->
            (item, change), (previous, change)
    ) (None, def)
    |> (fst)


Removing duplicate Unicode characters Motivation

I came across an interesting question on SO: determine-if-string-has-all-unique-characters and thought about providing an extension method to enable duplicate removal given some kind of normalization.

Description

  The goals:

  Remove any duplicate characters (keep the first occurence) and return the updated string.
  The method should be able to handle diacritics and extended unicode characters.
  The method should allow the consumer to control normalization to find duplicates (for instance, case-sensitivity).

Questions

Looking for general feedback on C# conventions
Performance feedback
Am I reinventing the wheel?

Code

public static class StringExtension
{
    public static string RemoveDuplicateChars(
        this string text, Func<string, string> normalizer = null)
    {
        var output = new StringBuilder();
        var entropy = new HashSet<string>();
        var iterator = StringInfo.GetTextElementEnumerator(text);

        if (normalizer == null)
        {
            normalizer = x => x.Normalize();
        }

        while (iterator.MoveNext())
        {
            var character = iterator.GetTextElement();
            if (entropy.Add(normalizer(character)))
            {
                output.Append(character);
            }
        }

        return output.ToString();
    }
}

Unit Tests

Let's test a string that contains variations on the letter A, including the Angstrom sign Å. The Angstrom sign has unicode codepoint u212B, but can also be constructed as the letter A with the diacritic u030A. Both represent the same character.

[TestClass]
public class Fixtures
{
    [TestMethod]
    public void Fixture()
    {
        // ÅÅAaA -> ÅAa
        Assert.AreEqual("ÅAa", "\u212BA\u030AAaA"
            .RemoveDuplicateChars());

        // ÅÅAaA -> ÅA
        // Note that the ToLowerInvariant is used to normalize characters
        // when searching for duplicates, it does not mean the output gets
        // transformed to lower case.
        Assert.AreEqual("ÅA", "\u212BA\u030AAaA"
            .RemoveDuplicateChars(x => x.Normalize().ToLowerInvariant()));
    }
}

There are not much to say other than the usual missing argument null check:

It is valid to write the following:

  string test = null;
  test.RemoveDuplicateChars();

and RemoveDuplicateChars will be called with a null for the this argument text. Therefore you'll have to test for null:

public static string RemoveDuplicateChars(
    this string text, Func<string, string> normalizer = null)
{
  if (text == null)
    return text;

  ...

or throw an exception...

The default initialization  of normalizer could be a little less verbose:

  normalizer = normalizer ?? ((x) => x.Normalize());

A minor detail: Angstrom and Å: is also represented by \u00C5, which your code interprets as equal to Angstrom, but MS Word interprets them as different when using its Find-function?


Parse strings and respond I'm doing some exercises from another good website aimed at a similar problem - exercism.io, and my 1st C++ problem was to pass a certain test suite.

Here is the README for relevant information:

  Bob is a lackadaisical teenager. In conversation, his responses are very limited.

  Bob answers 'Sure.' if you ask him a question.

  He answers 'Whoa, chill out!' if you yell at him.

  He says 'Fine. Be that way!' if you address him without actually saying
  anything.

  He answers 'Whatever.' to anything else.

Here is the test suite:

BOOST_AUTO_TEST_CASE(stating_something)
{
    BOOST_REQUIRE_EQUAL("Whatever.", bob::hey("Tom-ay-to, tom-aaaah-to."));
}

BOOST_AUTO_TEST_CASE(shouting)
{
    BOOST_REQUIRE_EQUAL("Whoa, chill out!", bob::hey("WATCH OUT!"));
}

BOOST_AUTO_TEST_CASE(asking_a_question)
{
    BOOST_REQUIRE_EQUAL("Sure.", bob::hey("Does this cryogenic chamber make me look fat?"));
}

BOOST_AUTO_TEST_CASE(talking_forcefully)
{
    BOOST_REQUIRE_EQUAL("Whatever.", bob::hey("Let's go make out behind the gym!"));
}

BOOST_AUTO_TEST_CASE(using_acronyms_in_regular_speech)
{
    BOOST_REQUIRE_EQUAL("Whatever.", bob::hey("It's OK if you don't want to go to the DMV."));
}

BOOST_AUTO_TEST_CASE(forceful_questions)
{
    BOOST_REQUIRE_EQUAL("Whoa, chill out!", bob::hey("WHAT THE HELL WERE YOU THINKING?"));
}

BOOST_AUTO_TEST_CASE(shouting_numbers)
{
    BOOST_REQUIRE_EQUAL("Whoa, chill out!", bob::hey("1, 2, 3 GO!"));
}

BOOST_AUTO_TEST_CASE(only_numbers)
{
    BOOST_REQUIRE_EQUAL("Whatever.", bob::hey("1, 2, 3"));
}

BOOST_AUTO_TEST_CASE(question_with_only_numbers)
{
    BOOST_REQUIRE_EQUAL("Sure.", bob::hey("4?"));
}

BOOST_AUTO_TEST_CASE(shouting_with_special_characters)
{
    BOOST_REQUIRE_EQUAL("Whoa, chill out!", bob::hey("ZOMG THE %^*@#$(*^ ZOMBIES ARE COMING!!11!!1!"));
}

BOOST_AUTO_TEST_CASE(shouting_with_no_exclamation_mark)
{
    BOOST_REQUIRE_EQUAL("Whoa, chill out!", bob::hey("I HATE YOU"));
}

BOOST_AUTO_TEST_CASE(statement_containing_question_mark)
{
    BOOST_REQUIRE_EQUAL("Whatever.", bob::hey("Ending with a ? means a question."));
}

BOOST_AUTO_TEST_CASE(prattling_on)
{
    BOOST_REQUIRE_EQUAL("Sure.", bob::hey("Wait! Hang on.  Are you going to be OK?"));
}

BOOST_AUTO_TEST_CASE(question_with_trailing_whitespace)
{
    BOOST_REQUIRE_EQUAL("Sure.", bob::hey("Are you ok? "));
}

BOOST_AUTO_TEST_CASE(silence)
{
    BOOST_REQUIRE_EQUAL("Fine. Be that way!", bob::hey(""));
}

BOOST_AUTO_TEST_CASE(prolonged_silence)
{
    BOOST_REQUIRE_EQUAL("Fine. Be that way!", bob::hey("   "));
}

Now here is my first-iteration code:

/*
 * #ifndef BOB_H
 * #define BOB_H
 *
 * #include <string>
 *
 * namespace bob {
 *     std::string hey(std::string);
 * }
 * #endif
*/

#include <algorithm>
#include <cctype>
#include "bob.h"

std::string bob::hey(std::string s){

    //remove any whitespaces
    std::string::size_type pos = s.find(' ');
    while(pos != std::string::npos){
        s.erase(s.find(' '), 1);
        pos = s.find(' ' , pos);
    }

    //Oh , he never said anything? Fine. Be that way!
    if(s.empty()) return "Fine. Be that way!";

    bool allUpper = std::all_of(s.begin() , s.end() , [](char ch){ if(std::isupper(ch) || !std::isalpha(ch)) return true; else return false; });
    bool atleastAlpha = std::any_of(s.begin() , s.end() , [](char ch){ if(std::isalpha(ch)) return true; else return false;  });

    //check for ques mark and say sure
    if(s.back() == '?' && (!allUpper || !atleastAlpha)) return "Sure.";

    //check for capital alphabets and yell Whoa, chill out!
    if(allUpper && atleastAlpha && !s.empty()) return "Whoa, chill out!";

    return "Whatever.";
}

In what ways can I improve my code? And is there anything I might be missing that is not according to the standards?

Don't write long lines.

The reason Loki Astari didn't comment on

bool allUpper = std::all_of(s.begin() , s.end() , [](char ch){ if(std::isupper(ch) || !std::isalpha(ch)) return true; else return false; });

is that it is too long. Written properly as

bool allUpper = std::all_of(s.begin() , s.end() ,
    [](char ch){
        if(std::isupper(ch) || !std::isalpha(ch))
            return true;
        else
            return false;
    });

it'd prompt for another advice:

    if (condition)
        return true;
    else
        return false;

is a standard anti-pattern: it is equivalent to short and clean

    return condition;


Recursively find data in array of objects On this script I look up properties in an array of objects. There is an original array of strings list to get the objects that match the name property, then I look up their respective line managers, reports recursively but only up to the rank 2, finally I merge their entries with the manager ones.
I was wondering if there is a more efficient way to achieve this result as I am having to make an object to index the managers that have already been checked, otherwise I get duplicates when two people have the same manager. Also having to specify the rank limit twice.

const data = [
  {
    name: 'rebecca',
    reports: '',
    rank: 1
  },
  {
    name: 'will',
    reports: 'rebecca',
    rank: 2
  },
  {
    name: 'jose',
    reports: 'will',
    rank: 3
  },
  {
    name: 'tomas',
    reports: 'jose',
    rank: 4,
  },
  {
    name: 'matt',
    reports: 'jose',
    rank: 5
  },{
    name: 'alison',
    reports: 'john',
    rank: 5
  }
]

// Initial list of names
const list = ['tomas', 'matt']

// Get the people on the list first
const filterList = data.filter(({ name }) => list.includes(name))

// Helper object to account for managers already checked and avoid duplicates
const managers = {}

// Find missing managers recursively
const findManager = (manager) => {
  const next = data.find(({ name }) => name === manager)
  managers[next.name] = true
  return next.rank > 2 ? [next, ...findManager(next.reports)] : [next]
}

// Get the line managers for the filterList array
const missingManagers = []
for (const { reports, rank } of filterList) {
  if (!list.includes(reports) && rank > 2 && !managers[reports] ) {
    missingManagers.push(...findManager(reports))
  }
}

const result = [...missingManagers, ...filterList]

console.log(result)
.as-console-wrapper { max-height: 100% !important; top: 0; }

From a medium review;

This code belongs in a well named function
I always forget about destructuring in a filter, very nice
Mapping to an object automatically removes dupes, something I (ab)use in my counter
const findManager = (manager)  should probably be const findManager = (employee)
const findManager = (employee)  should probably be const findLineManagers = (employee)
From there, next should probably be manager , it is definitely not 'next' but more 'actual' ;)
You are not using semicolons, unless you understand all nuances, you should use them
It's more idiomatic in recursive programming to first check for the exit condition than then to check at the end if you should recurse or not

I struggled with this counter-proposal. I imagine in a codebase I maintain I would want my data structure to be linked. So this function gets a linked version of the data and then gets what is needed in what I think is an easier manner. YMMV.

// Initial list of names

function indexPeopleData(peopleData){
  //Index by name
  const indexedPeople = peopleData.reduce((out,person) => (out[person.name] = person, out), {});
  //Make reports point to a people object
  peopleData.forEach(person => person.reports = indexedPeople[person.reports]);
  return indexedPeople;
}

function getLineManagers(peopleData, names){
  const indexedPeople = indexPeopleData(peopleData);
  //Return list of employees and their managers
  let out = {}, name;
  while(name = names.pop()){
    out[name] = indexedPeople[name];
    if(out[name].reports.rank > 1){
      names.push(out[name].reports.name);
    }
  }
  return Object.values(out);
}

var data = [
  {
    name: 'rebecca',
    reports: '',
    rank: 1
  },
  {
    name: 'will',
    reports: 'rebecca',
    rank: 2
  },
  {
    name: 'jose',
    reports: 'will',
    rank: 3
  },
  {
    name: 'tomas',
    reports: 'jose',
    rank: 4,
  },
  {
    name: 'matt',
    reports: 'jose',
    rank: 5
  },{
    name: 'alison',
    reports: 'john',
    rank: 5
  }
];

console.log(getLineManagers(data, ['tomas', 'matt']));


Median of two sorted arrays JavaScript Trying to find the median of two sorted arrays in O(log(m+n)) or better. I believe this is O(log(m+n) or O(log(m)) or O(n).

var findMedianSortedArrays = function(nums1, nums2) {
  let ar = []
  let len = nums1.length + nums2.length
  let half;

  if (len % 2 === 0) {
    half = len / 2
  } else {
    half = Math.ceil(len / 2)
  }
  for (let i = 0, r1 = 0, r2 = 0; i <= len; i++) {
    if (r1 >= nums1.length || nums1[r1] > nums2[r2]) ar[i] = nums2[r2++]
    else ar[i] = nums1[r1++]
    if (!nums1[i] && nums1.length === r1) {
      ar = ar.concat(nums2.slice(r2))
      return median(ar, len)
    }
    if (!nums2[i] && nums2.length === r2) {
      ar = ar.concat(nums1.slice(r1))
      return median(ar, len)
    }
    if (half === i) {
      return median(ar, len)
    }
  }

  function median(arr, len) {
    if (len % 2 === 0) {
      let half = len / 2
      return median = (arr[half - 1] + arr[half]) / 2
    } else {
      let half = Math.ceil(len / 2)
      return median = arr[half - 1]
    }
  }
}

console.log(findMedianSortedArrays([1, 4], [2, 3, 5, 6]))

Unnecessary storage

The current implementation basically processes the two input arrays in parallel, taking the next smaller element from the appropriate array, and appending to ar,
until either the end of an input array is reached,
or the counter i reaches half.

If the end of an input array was reached,
the remaining elements of the other array are appended to ar.
The size of ar becomes len.
If the counter i reaches half, then nothing is appended to ar.
At this point the size of ar is half.

Then, the median function doesn't actually pick the median of the array parameter,
but it computes a value based on the len parameter.

The ar array is unnecessary storage. In the end, only the half-th and possibly the half + 1-th values will be used.

The benefit of the ar storage is it makes it simple to compute the median in the case when the end of an input array was reached.
In this case, without ar, it's not trivial to find the correct index.

An alternative algorithm is possible without extra storage:
- pick values from the input arrays
- if we haven't reached yet either end, take the next smaller element
- otherwise, if we reached the end of the first array, take the next element from the second
- otherwise, if we reached the end of the second array, take the next element from the first
- before the above conditions, save the previously picked value in a variable

After counting until half elements, you can compute the value of the median based on len, prev and current.

Like this:

  for (let i = 0, r1 = 0, r2 = 0; i <= half; i++) {
    prev = current;
    if (r1 < nums1.length && r2 < nums2.length) {
      if (nums1[r1] < nums2[r2]) {
        current = nums1[r1++];
      } else {
        current = nums2[r2++];
      }
    } else if (r1 < nums1.length) {
      current = nums1[r1++];
    } else if (r2 < nums2.length) {
      current = nums2[r2++];
    }
  }

  if (prev === undefined) {
    return current;
  }

  if (len % 2 === 0) {
    return (prev + current) / 2;
  }
  return current;

Unnecessary computation

The median helper function recomputes half.
You could simply reuse the half variable that's already computed in the closure.

The loop condition i <= len is unnecessary.
It will always be true,
because the implementation in the loop body guarantees that i will never reach len.

Return value in case of empty input arrays

In case of empty input arrays,
the implementation returns NaN.
I'm not sure that's correct, or expected.
Unless the exercise specifies to return NaN in this case,
I think returning undefined makes more sense.


Determine whether input-output pairs represent something that could be a proper math function The input is read in line by line from stdin.

  Input Format

  The first line contains an integer, T, denoting the number of test
  cases. The subsequent lines describe T test cases, and the input for
  each test case is as follows:

  The first line contains an integer, N, the number of (x,y) pairs in
  the test case. The N subsequent lines each contain two space-separated
  integers describing the respective x and y values for each ordered
  pair.

  Constraints

  \$1 \le T \le 5\$
  \$2 \le N \le 100\$
  \$0 \le x, y \le 500\$

  x and y are both integers.

  Output Format

  On a new line for each test case, print  if the set of ordered
  pairs represent a valid function, or  if they do not.

  Sample Input

2
3
1 1
2 2
3 3
4
1 2
2 4
3 6
4 8

  Sample Output

YES
YES

Full problem statement

I'm looking for feedback regarding ways this could be: more concise/clean, more idiomatic Haskell, or if there are algorithmic/mathematical tricks I missed that could further simplify the computation.

import System.IO (readLn, getLine)
import Control.Monad (replicateM, mapM_, liftM)
import Data.List (nub)
import qualified Data.Map.Strict as M

data YesNo = YES | NO deriving Show

boolToYesNo :: Bool -> YesNo
boolToYesNo x = if x then YES else NO

readInt :: IO Int
readInt = readLn

tuplefy :: [a] -> (a, a)
tuplefy xs = case xs of [a, b] -> (a, b)
                        _      -> error "each item must consist of 2 values."

readPair :: [String] -> [(Int, Int)]
readPair = map $ tuplefy . map (read :: String -> Int) . words

getPairs :: Int -> IO [String]
getPairs = flip replicateM getLine

readTestCase :: Int -> IO [(Int, Int)]
readTestCase = fmap readPair . getPairs

readTestCases :: Int -> IO [[(Int, Int)]]
readTestCases = flip replicateM $ readInt >>= readTestCase

printAnswers :: Show a => [a] -> IO ()
printAnswers = mapM_ print

isSingletonSet :: Eq b => [b] -> Bool
isSingletonSet xs = case nub xs of [x] -> True
                                   _   -> False

listToMultiMap :: (Ord k, Eq k, Eq v) => [(k, v)] -> M.Map k [v]
listToMultiMap ((k, v):vs) = toMap (M.fromList [(k, [v])]) vs
    where toMap m xs = case xs of []        -> m
                                  (k0, v0):ys ->
                                      case M.lookup k0 m of Nothing -> toMap (M.insert k0 [v0] m) ys
                                                            Just v0s -> toMap (M.insert k0 (v0:v0s) m) ys

functionOrNot :: [(Int, Int)] -> YesNo
functionOrNot tuples = boolToYesNo $ foldl1 (&&) $ M.elems $ M.map isSingletonSet $ listToMultiMap tuples

main :: IO ()
main = getAnswers >>= printAnswers
    where getAnswers = (liftM . map) functionOrNot $ readInt >>= readTestCases

Your monadic helper functions are all one-offs and even if they were well-named (no worries, naming just gets harder as you get more experienced, there's a reason Adam was told to do it), I'd still think it better to inline them. Then the main ends up cluttered with pointfreed binds and I turned that into a do block.

(I did reorder the monadic effects a little, moving the output of each answer in front of the reading of the next question, but I found that sacrifice worthy.)

I'm not sure you need to try your hand at error-handling in tuplefy - a badly formated input file should be as likely to have a typo in its number of words in a line as in its stated number of pairs in a function.

listToMultiMap - (k, v) is no different from the other elements of vs, so you could go listToMultiMap = toMap M.empty. toMap is folding left, bad style. You can fold right like:

toMap [] = []
toMap ((k0, v0):ys) = case M.lookup k0 m of
  Nothing  -> M.insert k0 [v0]     $ toMap ys
  Just v0s -> M.insert k0 (v0:v0s) $ toMap ys

and won't need to carry a parameter into the recursive calls. And of course this can be written in terms of foldr:

listToMultiMap = foldr foo M.empty where
  foo (k0, v0) m = case M.lookup k0 m of
    Nothing  -> M.insert k0 [v0]     m
    Just v0s -> M.insert k0 (v0:v0s) m

And we could reduce the duplication in the last two lines there, but then again this whole thing is already handled by Data.Maps fromListWith.

import System.IO (readLn, getLine)
import Control.Monad (replicateM)
import qualified Data.Map.Strict as M
import Data.Bool (bool)

noDuplicates :: Ord a => [a] -> Bool
noDuplicates = all (==1) . M.fromListWith (+) . map (,1)
--           = all ((==1) . length) . M.fromListWith (++) . map (,["Why are you looking at the ordinate?"])
--           = and . M.fromListWith (\_ _ -> False) . map (,True)

main :: IO ()
main = do
  functioncount <- readLn
  replicateM functioncount $ do
    paircount <- readLn
    abscissae <- replicateM paircount $ read . head . words <$> getLine
    putStrLn $ bool "NO" "YES" $ noDuplicates abscissae

It is unfortunate for pointfreedom that the first parameter of replicateM indeed should be Int.  There must be a better way than flip. Perhaps a new infix operator, semantically id, that has highest precedence to the left, and lowest to the right, to simulate enclosing brackets around its entire right side and disappearing?


Insert or update user data I inherited an application that uses some stored procedures. Here is a sample of one of the stored procedures used to insert or modify user data.

would like to get the group's opinion on the code. The SQL Server version is Microsoft SQL Server 2014 - 12.0.4100.1 (X64).

USE [MyDataBase]
GO
/****** Object:  StoredProcedure [dbo].[InsertOrModifyUserData]    Script Date: 2/2/2016 8:39:57 PM ******/
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO

ALTER  PROCEDURE [dbo].[InsertOrModifyUserData]
@userName NVARCHAR(200),
@isSuperAdmin BIT = NULL,
@modifiedBy NVARCHAR(200),
@modifiedDate DATETIME = NULL,
@isActive BIT = NULL
AS

BEGIN
    SET NOCOUNT ON;
    SET XACT_ABORT ON;
    BEGIN TRY
        BEGIN TRANSACTION

            IF (@modifiedDate IS NULL OR @modifiedDate ='1900-01-01 00:00:00.000')
                SET @modifiedDate = GETDATE()
            IF (@isActive IS NULL)
                SET @isActive = 1

            IF ((@userName IS NOT NULL OR @userName <>'') AND (@isSuperAdmin IS NOT NULL OR @isSuperAdmin<>'') AND (@modifiedBy IS NOT NULL OR @modifiedBy <>''))
            BEGIN
                IF EXISTS(SELECT UserName FROM [dbo].[UserAccess] WHERE UserName = @userName)
                    UPDATE  [dbo].[UserAccess]
                        SET IsSuperAdmin    = @isSuperAdmin,
                            ModifiedBy      = @modifiedBy,
                            ModifiedDate    =   @modifiedDate,
                            IsActive        = @isActive
                    WHERE   UserName = @userName
                ELSE
                    INSERT INTO [dbo].[UserAccess] (UserName, IsSuperAdmin, CreatedDate, ModifiedBy, ModifiedDate,IsActive)
                    VALUES(@userName, @isSuperAdmin, GETDATE(), @modifiedBy, @modifiedDate, @isActive)

            END
        ELSE
            RAISERROR('Required parameters are not provided or Required parameters are passed as NULL',13,1)
        COMMIT TRANSACTION
        select null
    END TRY
    BEGIN CATCH
            DECLARE @ErrorMessage NVARCHAR(MAX) = ERROR_MESSAGE();
            DECLARE @ErrorSeverity INT = ERROR_SEVERITY();
            DECLARE @ErrorState INT = ERROR_STATE();
            RAISERROR(@ErrorMessage, @ErrorSeverity, @ErrorState);
            ROLLBACK TRANSACTION;
    END CATCH;

END

Dead code
In this:

    COMMIT TRANSACTION
    select null
END TRY

This line of code: select null does nothing useful (read: nothing at all) and should be deleted. I have no idea why it is there to begin with, but querying for NULL with no criteria will always be null.

Errors not useful
You could improve this error reporting:

    ELSE
        RAISERROR('Required parameters are not provided or Required parameters are passed as NULL',13,1)

TLDR: Don't filter error logs, just store stuff. Developers can filter them easily if they need to.
Why? As it is, this is raising an error with no information on "why" the error was raised. At the very least return the input parameters so the user/dev can decipher what is wrong. This is especially relevant if you are logging those errors somewhere.
You would want to include as much relevant information as possible in your errors, especially if there are multiple instances and databases involved. There are few things which hinder improvement and bug fixes more than half-arsed error logs. Log everything and let the developers sort it out.