Search object literal and return first key/value pair that matches regex This does what I want:

function test(data){
    for(var key in data){
        if(/_is$/.test(key)){
          var obj = {}
          obj[key] = data[key];
          return obj;
        }
    }
}

test( {a: 'a', user_id_is: '1', b: 'b'} )
> Object {user_id_is: "1"}

But 8 lines of code for such a simple task looks inefficient. Is there a more concise way to achieve such a basic task in JavaScript, something like how Ruby does it?

data.detect {|item| item =~ /_is$/}

To be fair, the entire thing in Ruby would be a few more method calls:

{:a => 1, :b => 2}.detect { |i| i != :b }.each_slice(2).to_h

Can we do this in a similar number of lines in JavaScript?

First, we can shorten the 8 line original down to 5 lines if we use ES2015 syntax:

function test(data) {
  for(let key of data) {
    if(/_is$/.test(key)) return {[key]: data[key]};
  }
}

Adding in ES2015 methods we can use reduce and an arrow function to get rid of the return at the cost of making this always loop over all the keys in the object (now down to 3 lines):

const test = data => Object.keys(data).reduce((obj, key) =>
  obj || /_is$/.test(key) ? {[key]: data[key]} : undefined, undefined
);

If we're willing to use ES2017, we can use Object.entries and a helper function to get the line count down to 2 at the expense of a little legibility:

const toObj = ([key, value] = []) => key ? {[key]: value} : undefined;
const test = data => toObj(Object.entries(data).find(([key, value]) => /_is$/.test(key)));


Caesar Cipher encoding process (customizable input and shifting) in Python So this code already works, but I would like to shorten it using list comprehension. Would it be better if it was in 1 line, or many? (I do know that readability is more important than short code, but I would like to master list comprehension, ternary operators, and conditional expressions).
Oh yes, if there is ANYTHING I can do to make my code better, more efficient, more readable and shorter, I would LOVE to know!

#Caesar Cipher, only lowercase and spaces
listString = [x for x in input('What would you like to encode? ')]
change = int(input('How much would you like to shift? '))
change %= 26
def encode(listString,change):
  encoded = []
  for c in listString:
    if ord(c) + change > 122:
      encoded.append(chr(ord(c) + (change - 26)))
      continue
    encoded = [" " if x == chr(32 + change) else x for x in encoded]
    encoded.append(chr(ord(c) + change))
  return "".join(encoded)

print(encode(listString,change))

Some suggestions:

Pull out a function to shift a single character, and your top level code ends up being simply print(''.join([cipher(character) for character in input(…)]))
With an array containing the alphabet and a shifted array ("{}{}".format(alphabet[offset:], alphabet[:offset])) you can use str.translate to avoid any ord, chr, punctuation handling etc.
Use at least one linter like pycodestyle or flake8 to produce more idiomatic code.


Doing a re_sub in C using pcre I'm trying to implement a re_sub and it's been quite tricky for me to do. I wanted to take the python code to replace a string --  re.sub(r";.+", " ", s) -- and do it in C. Examples of grabbing a single regex match were pretty easy to find (and do) but doing multiple matches and then substituting proved to be quite tricky (for me at least).
Here is what I have so far:
#include <pcre.h>
#include <stdio.h>
#include <string.h>
#include <errno.h>
#include <stdarg.h>

#define OFFSETS_SIZE 90
#define BUFFER_SIZE (1024*10)
char buffer[BUFFER_SIZE];

void exit_with_error(const char* msg, ...)
{
    va_list args;
    va_start(args, msg);
    vfprintf(stderr,  msg, args);
    exit(EXIT_FAILURE);
}

void re_sub(char buffer[], const char* pattern, const char* replacement)
{
    const char *error;
    int error_offset;
    int offsets[OFFSETS_SIZE];
    size_t replacement_len = strlen(replacement);

    // 1. Compile the pattern
    pcre *re_compiled = pcre_compile(pattern, 0, &error, &error_offset, NULL);
    if (re_compiled == NULL)
        exit_with_error(strerror(errno));

    // 2. Execute the regex pattern against the string (repeatedly...)
    int rc;
    size_t buffer_size = strlen(buffer);
    char tmp[buffer_size * 2];
    int tmp_offset=0, buffer_offset=0;

    while ((rc = pcre_exec(re_compiled, NULL, buffer, strlen(buffer), buffer_offset, 0, offsets, OFFSETS_SIZE)) > 0) {
        int start=offsets[0];
        int end=offsets[1];
        int len =  start - buffer_offset;

        // 1. copy up until the first match if the first match starts after
        //    where our buffer is
        printf("Start: %d | End: %d | Len: %d | BufferOffset: %d\n", start, end, len, buffer_offset);
        if (start > buffer_offset) {
            strncpy(&tmp[tmp_offset], buffer+buffer_offset, len);
            tmp_offset += len;
        }

        // 2. copy over the replacement text instead of the matched content
        strncpy(&tmp[tmp_offset], replacement, replacement_len);
        tmp_offset += replacement_len;
        buffer_offset = end;
    };

    // exit early if there was an error
    if (rc < 0 && rc != PCRE_ERROR_NOMATCH) {
        free(re_compiled);
        exit_with_error("Matching error code: %d\n", rc);
    }
    // now copy over the end if leftovers
    if (buffer_offset < buffer_size-1)
        strcpy(&tmp[tmp_offset], buffer+buffer_offset);

    free(re_compiled);
    strcpy(buffer, tmp);
}

int main(int argc, char* argv[])
{
    strcpy(buffer, "Hello ;some comment");
    re_sub(buffer, ";.+", " ");
    printf("Buffer: %s", buffer);
    return 0;
}

And a working example: https://onlinegdb.com/SJtW_8Tr_ (note you have to go in the upper-right hand corner, click settings gear and add -lpcre into the Extra Compiler Flags (they don't persist on the share link).
A few comments about the code:

I find it very tedious and quite hard to keep track of all the offsets, for example: &tmp[tmp_offset]. Is there a better way to do this? I tried adding a ptr of char *tmp_ptr = tmp but for whatever reason I couldn't get that way to work properly.
I think I need to have the user pass in the size_t buffer_len otherwise it's very easy to get a buffer overflow, as the only thing I know about the buffer is the strlen. In this example here I have 10K in the buffer, so it's not going to overflow, but in other examples it's very possible that it could.
Is there something like a 'package manager' (like pip or npm) for C? For example, if I'm to move this code onto another server, how do I 'build it with pcre, which may not even be on the server? Is this what cmake is for, or what's a common way to 'manage' non stdlib packages?

You were so close! So close to having re-entrant code. You declare buffer as a global, but also pass it into a function. It should be easy enough to just move it to main and keep your functions as-is.
You have two different pointer offset styles (right next to each other) that accomplish the exact same thing:
&tmp[tmp_offset],
buffer+buffer_offset

I prefer the latter, but pick one and stick with it.
This:
size_t buffer_size = strlen(buffer);
char tmp[buffer_size * 2];

leverages a feature of C, variable-length stack arrays, not offered until C99. It's not wrong, but it occupies the stack and I find that for any non-trivial amount of memory the heap is a safer bet.
re_sub and exit_with_error should be marked static.
I don't find this statement:
while ((rc = pcre_exec(re_compiled, NULL, buffer, strlen(buffer), buffer_offset, 0, offsets, OFFSETS_SIZE)) > 0) {

particularly nice to read; you'd be better-off avoiding combined assignment-predicate statements like this, and just separating it out. It will have no impact on your compiled code.


Changing background colour I am currently changing the background colour of a sidebar each time a menu item is clicked using the following code:

$( 'a[href = "#masthead"' ).click(function() {
  $('#sidebar').css('background-color', '#3b6dac');
});

$( 'a[href = "#section2"' ).click(function() {
  $('#sidebar').css('background-color', '#23d4d7');
});

$( 'a[href = "#section3"' ).click(function() {
  $('#sidebar').css('background-color', '#59d078');
});

It does what I want, but I am relatively new to JavaScript and jQuery, and I imagine there is likely to be a better way...

jsFiddle

You can do something like this:

Masthead

Section 2

Section 3

and js:

$( 'a[data-bg-color]' ).click(function() {
  $('#sidebar').css('background-color', $(this).attr("data-bg-color"));
});

or if you cannot change html:

$( '.main-nav a' ).click(function() {

    var color = "blue"; //default;

    switch($(this).attr("href")) {
        case '#masthead' : color = "blue"; break;
        case '#section2' : color = "pink"; break;
        case '#section3' : color = "orange"; break;
    }
    $('#sidebar').css('background-color', color);
});

Improved solution

If you can do the first option but if you don't like to write in HTML the color of background (according to best practice, the style should be inside css), then you can use the same approach but using the class to add on sidebar.

For example:

Masthead

Section 2

Section 3

and js:

$( 'a[data-sidebar-class]' ).click(function() {

    // http://stackoverflow.com/questions/2644299/jquery-removeclass-wildcard
    $('#sidebar').removeClass(function (index, css) {
        return (css.match (/(^|\s)sidebar-\S+/g) || []).join(' ');
    }).addClass("sidebar-"+$(this).attr("data-sidebar-class"));

});

and css:

.sidebar-master { background-color: blue }
.sidebar-section2 { background-color: pink }
.sidebar-section3 { background-color: orange }

demo: http://jsfiddle.net/9vtxutth/1/


Java solution to check if a Sudoku solution is valid Here's my solution to checking if a Sudoku board is valid. The algorithm checks the following,

Rows add up to 45
Columns add up to 45
Rows have no duplicates
Cols have no duplicates
Every 3X3 sub-grid has no duplicate

Invite comments.

public class Checker {
    private int size;
    private int[][] board;

    public Checker(int size){
        this.size = size;
        board = new int[size][size];
    }

    private boolean rowCheck(){
        int sum = (size*(size+1))/2;
        int count = 0;
        for (int i = 0; i <size ; i++) {
            for (int j = 0; j < size ; j++) {
                count += board[i][j];
            }
            if(count != sum) return false;
            count = 0;
        }
        return true;
    }

    private boolean columnCheck(){
        int sum =0;
        int count = 0;
        for (int i = 0; i <size ; i++) {
            for (int j = 0; j <size ; j++) {
                count += board[j][i];
            }
            if(count != sum) return false;
            count = 0;
        }
        return true;
    }

    private boolean checkRowDuplicates(){
        HashSet<Integer> set = new HashSet<>();
        for (int i = 0; i <size ; i++) {
            for (int j = 0; j < size ; j++) {
               set.add(board[i][j]);
            }
            if(set.size() != size) return false;
            set = null;

        }
        return true;
    }
    private boolean checkColumnDuplicates(){
        HashSet<Integer> set = new HashSet<>();
        for (int i = 0; i <size ; i++) {
            for (int j = 0; j < size ; j++) {
                set.add(board[j][i]);
            }
            if(set.size() != size) return false;
            set = null;

        }
        return true;
    }

    private boolean checkSubBoardDuplicates(){

        return (subBoardTraversal(0,3,0,3) && subBoardTraversal(0,3,3,6) && subBoardTraversal(0,3,6,9) && subBoardTraversal(3,6,0,3)
                && subBoardTraversal(3,6,3,6) && subBoardTraversal(3,6,6,9) && subBoardTraversal(6,9,0,3) && subBoardTraversal(6,9,3,6)
                && subBoardTraversal(6,9,6,9));

    }
    private boolean subBoardTraversal(int rowStart, int rowFinish, int colStart, int colFinish){
        HashSet<Integer> set = new HashSet<>();
        for (int i = rowStart; i < rowFinish  ; i++) {
            for (int j =colStart ; j < colFinish; j++) {
                set.add(board[i][j]);
            }
            if(set.size() != 9) return false;

        }
        return true;
    }
    public boolean isValid(){

        return(rowCheck() && columnCheck() && checkRowDuplicates() && checkColumnDuplicates() && checkSubBoardDuplicates());

    }

    public void trace(){
        for (int i = 0; i < size; i++) {
            for (int j = 0; j < size; j++) {
                System.out.print(board[i][j] + " ");
            }
            System.out.println();
        }
    }
}

Your setting of count = 0 should be immediately before the appropriate for loop, not after it, and I would actually call that variable sum (renaming your sum to target) since it's not really a "count" at all, e.g:

private boolean rowCheck(){
    int target = size * (size + 1) / 2;
    for (int i = 0; i <size; i++) {
        int sum = 0;        // declaration moved, too
        for (int j = 0; j < size ; j++) {
            sum += board[i][j];
        }
        if (sum != target) return false;
    }
    return true;
}

The point being that you should be setting the variable in advance of the loop, not resetting it afterwards (that being a pointless operation on the final iteration).

I would probably also move target into a class member, calculated in the constructor.


Vertical JButton allignment I have created a simple component in Swing that lets me place other components such as buttons on top of each other.

They fill their parent horizontally but not vertically, which is exactly as I want it. I called this parent component VerticalPanel and this is how it looks:

I can't shake the feeling though that there could be a more elegant solution. Especially the need for my VerticalPanel to know the number of components it will hold in advance is something I'm not entirely happy with.

Here's the VerticalPanel:

public class VerticalPanel extends JPanel {
    private static final long serialVersionUID = 1L;

    public VerticalPanel(int nrOfComps) {
        LayoutManager layout = getVerticalLayout(nrOfComps);
        setLayout(layout);
    }

    @Override
    public Component add(Component comp) {
        return add(comp, getComponentCount());
    }

    @Override
    public Component add(Component comp, int index) {
        final GridBagConstraints gbc = new GridBagConstraints();
        gbc.fill = GridBagConstraints.HORIZONTAL;
        // Allocate extra width to the buttons
        gbc.weightx = 1.0;
        gbc.anchor = GridBagConstraints.NORTH;
        gbc.gridx = 0;
        gbc.gridy = index;
        add(comp, gbc);
        return comp;
    }

    private LayoutManager getVerticalLayout(int nrOfComps) {
        GridBagLayout layout = new GridBagLayout();

        double[] rowWeights = new double[nrOfComps];
        rowWeights[nrOfComps - 1] = Double.MIN_VALUE;
        layout.rowWeights = rowWeights;
        return layout;
    }
}

This creates the example from the screenshot above:

public class VerticalButtons {
    public static void main(String[] args) {
        SwingUtilities.invokeLater(new Runnable() {
            @Override
            public void run() {
                try {
                    UIManager.setLookAndFeel(UIManager
                            .getSystemLookAndFeelClassName());
                } catch (ClassNotFoundException | InstantiationException
                        | IllegalAccessException
                        | UnsupportedLookAndFeelException e) {
                    e.printStackTrace();
                }
                createAndShowGUI();
            }
        });
    }

    private static void createAndShowGUI() {
        JFrame frame = new JFrame();

        frame.getContentPane().add(createVerticalButtons());
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);

        frame.pack();
        frame.setVisible(true);
    }

    private static JComponent createVerticalButtons() {

        VerticalPanel buttons = new VerticalPanel(2);

        JButton b1 = new JButton("1");
        JButton b2 = new JButton("2");
        buttons.add(b1);
        buttons.add(b2);

        return buttons;
    }
}

as I mentioned in my comment I was playing with a LayoutManager. Based off of what you are hoping to achieve I would suggest going down this path as you are trying to control how the components are laid out which is what the LayoutManager does. Here is a version that works, but could stand to be cleaned up a bit more, and adapted a tad more to cover more scenario's (padding, margins, isVisible...etc)

public class VerticalLayout implements LayoutManager {
    private final Dimension minimumSize = new Dimension();

    @Override
    public void addLayoutComponent(String name, Component comp) {
    }

    @Override
    public void removeLayoutComponent(Component comp) {
    }

    @Override
    public Dimension preferredLayoutSize(Container parent) {
        Dimension dimension = new Dimension(getTotalPaddingWidth(parent), getTotalPaddingHeight(parent));
        minimumSize.height = dimension.height;
        for(Component c :parent.getComponents()){
            if(!c.isVisible()){
                continue;
            }

            Dimension preferredSize = c.getPreferredSize();
            dimension.height += preferredSize.getHeight();
            if(preferredSize.width > dimension.width){
                dimension.width = preferredSize.width;
            }
        }
        minimumSize.width = dimension.width;
        return dimension;
    }

    private int getTotalPaddingWidth(Container container) {
        Insets padding = container.getInsets();
        return padding.left + padding.right;
    }
    private int getTotalPaddingHeight(Container container){
        Insets padding = container.getInsets();
        return padding.top + padding.bottom;
    }
    @Override
    public Dimension minimumLayoutSize(Container parent) {
        return minimumSize;
    }

    @Override
    public void layoutContainer(Container parent) {
        Insets padding = parent.getInsets();
        int y = padding.top;
        for(Component c : parent.getComponents()){
            if(c.isVisible()) {
                c.setBounds(padding.left, y, parent.getWidth(), c.getPreferredSize().height);
                y += c.getHeight();
           }
        }
    }
}


Simple calculator in Object-oriented JavaScript There is a simple form which takes two numbers num1, num2 and an operation op. After submission, a new object is created. I'm new to OOJS and any correction and improvements are appreciated. Is this the best way to code this small calculator in Javascript using Object Oriented approach?

window.onload = function() {

  let form = document.forms['cal-form'];
  form.addEventListener('submit', calculate);

  function calculate(e) {
    //prevent default form submission
    e.preventDefault();
    //get form values
    let num1 = parseInt(document.getElementsByTagName('input')[0].value);
    let num2 = parseInt(document.getElementsByTagName('input')[1].value);
    let op = document.getElementsByTagName('select')[0].value;

    //create object constructor function
    function Calculate(num1, num2, op){
      this.num1 = num1;
      this.num2 = num2;
      this.op = op;
    }

    Calculate.prototype.result = function() {
      let res;
      switch (op) {
        case 'add':
          res = this.num1 + this.num2;
          break;
        case 'sub':
          res = this.num1 - this.num2;
          break;
        case 'mul':
          res = this.num1 * this.num2;
          break;
        case 'div':
          res = this.num1 / this.num2;
          break;
        default:
          res= 'Error! No operation selected.';
      }
      return res;
    };

    //create an object
    let cal = new Calculate(num1, num2, op);
    //display result
    document.getElementById('result').innerHTML = cal.result();

  }
};

I don't really see any purpose to forcibly using a class setup here. All that's doing is allowing you to provide the data before result is called. That's not necessary here though, and there are other ways of achieving that. If you needed to create multiple Calculate objects, and retain them, and call result on them multiple times, there may be a purpose. I can't see any gain here though.

I would just collapse everything down to a function that accepts the three bits of data (op, num1 and num2) and just call the function directly when needed.

There's cleaner ways of handling the "dispatching" than a switch. I'd just use a regular JavaScript object here:

// Associate the operator strings
//  with functions
var strToOp = {"+": (x, y) => x + y,
               "-": (x, y) => x - y,
               "*": (x, y) => x * y,
               "/": (x, y) => x / y};

var num1 = 2;
var num2 = 5;
var op = "*";

// Get a function from the map
var func = strToOp[op];

// func will be undefined if they supply a bad operator string
// This is roughly equivalent to your "default" case
if(func) {
    // And give the numbers to it
    var result = func(num1, num2);

    // Prints 10
    console.log(result);

} else {
    // Handle bad operator
}


Finding the runtime of a task by name Can this query be improved? I iterate over a couple of thousand times, and if I can improve it, it should decrease the run time.  Otherwise I might look into using a batch instead of doing one at a time. I'm using MS SQL 11.

job
****
id
name

job_stats
*************
id
job_id
median_runtime

select js.median_runtime from job_stats js
      join job j
      on j.id = js.job_id
      where j.name = '%s'

With a query this simple, it's going to be hard to make any major improvements.

Firstly, you should use INNER JOIN as opposed to JOIN it makes the query much easier to understand at a glance.

I would argue that you don't need to alias job_stats or job, they are both already fairly short and are much more meaningful names than J and JS

You should use consistent indentation and you SQL is typically written with all the keywords in uppercase like this:

SELECT job_stats.median_runtime
FROM job_stats
INNER JOIN job
    ON job.id = job_stats.id
WHERE job.name = '%s'

Lastly, is the final statement supposed to be this:

WHERE job.name LIKE '%s'

or is it correct as is, I can not tell.


Program for sending emails Disclaimer: This question is very much like the one posted here. I gathered some opinions about my options from the answer there. Here, I just want validation about the choices I'm deciding to stick to and see what people think about the decisions specifically. Also, gather suggestions about other parts of the code I'm not specifically asking about, since I'm new to Python OOP.

Scenario: I'm writing a program that will send emails. For an email, the to, from, text and subject fields will be required and other fields like cc and bcc will be optional. Also, there will be a bunch of classes that will implement the core mail functionality, so they will derive from a base class (Mailer).

Following is my incomplete code snippet:

class Mailer(object):
    __metaclass__ == abc.ABCMeta

    def __init__(self,key):
        self.key = key

    @abc.abstractmethod
    def send_email(self, mailReq):
        pass

class MailGunMailer(Mailer):
    def __init__(self,key):
        super(MailGunMailer, self).__init__(key)

    def send_email(self, mailReq):
        from = mailReq.from
        to = mailReq.to
        subject= mailReq.subject
        text = mailReq.text
        options = getattr(mailReq,'options',None)
        if(options != None):
            if MailRequestOptions.BCC in options:
                #use this property
                pass
            if MailRequestOptions.CC in options:
                #use this property
                pass

class MailRequest():
    def __init__(self,from,to,subject,text):
        self.from = from
        self.to = to
        self.subject = subject
        self.text = text

    def set_options(self,options):
        self.options = options

class MailRequestOptions():
    BCC = "bcc"
    CC = "cc"

I've made the following decisions about code designs. What do you think about them?

The send_email() method will take four required parameters - to, from, subject and text, and bunch of other optional parameters like cc, bcc etc. So I decided to create the MailRequest wrapper, which will take the 4 required fields in the constructor, and the other optional parameters will go in the options dict.

Is this an acceptable way of doing this?  Why not use **kwargs?

Because if you define something like:

def foo(**kwargs):
    pass

Then to call it, you can't do something like:

options = []
options["one"] = 1
options["two"] = 2
foo(options)

Right?

You'd have to do something like:

foo(one=1,two=2)

Now if I have 15 parameters that foo accepts, then **kwargs isn't a good way to do this right?
I've created the MailRequestOptions class to contain static strings. The reason behind the existence of this class is, even if the user knows he has to pass some options in the options dict of the MailRequest object, how would he know which options can he set. This class could probably help the user know about what options can be set. This will also be helpful if the user has auto complete in an IDE or something. Do you think I'm thinking right? Or is this a somewhat unusual way of doing things?

Because if you define something like:

def foo(**kwargs):
    pass

  Then to call it, you can't do something like:

options = []
options["one"] = 1
options["two"] = 2
foo(options)

  Right?

The aswer is Yes. But there are ways,

**name is used in two ways.

To define optional keyword argumnets in function definiton.

def f(**kwargs): pass. Here the argument of function f, kwargs is optional. You can legitimately call f like f() i.e without arguments. It's utility is you can pass unlimited number of keyword arguments like one=1,...

To unpack a dict to pass arguments at function invocation.

If a function is defined with some arguments whether it's required or optional, its arguments can be passed by **dict when invoking the function. f.e

def f(a,b=2): pass

This function has two arguments one is required. To call it with one argument, you can do,

arg = dict(a=111)
f(**arg)

With two argumnets,

arg = dict(a=111,b=222)
f(**arg)

Combining the two **name form.

def f(**kwargs): pass

can be called as follows,

f()
f(a=1,b=2,c=3)
arg = dict(foo=1,bar=2)
f(**arg)

The send_email method can be simplified.

def send_email(self, from, to, sub, body, **options):
    '''
        This method sends an email by taking four req. args
        from, to, sub, body and optional 'cc' and 'bcc' keyword args
    '''
    from = from
    to = to
    subject= sub
    text = body
    cc = options.get('cc')
    if cc is not None:
            #use this property
    bcc = options.get('bcc')
    if bcc is not None:
            #use this property

The 'docstring' can be checked by calling,

help('classname or obj'.send_email)
'classname or obj'.send_email.__doc__

and can be called as follows,

obj.send_email('me@example.com', 'you@example.com', 'hi', 'howdy', cc='he@example.com')

or

mail1 = dict(from='she@example.com', to='he@example.com', sub='date', body='free tonight?', bcc='you@example.com')
obj.send_email(**mail1)


Base 256 array addition Recursively - Follow up This is a follow up code review request from Base 256 array addition Recursively

I have made the changes that were suggested for the RecursiveAdd method. Please take some time to review and help me to make it better.

public class Base256Calculator : IBase256Calculator
{
    private const int Base = 256;

    /// <summary> Adds 2 byte arrays together. </summary>
    /// <param name="first">First array.</param>
    /// <param name="second">Second array.</param>
    /// <returns>Result array containing the sum of the two.</returns>
    public byte[] Add(byte[] first, byte[] second)
    {
        byte[] result = new byte[first.Length];
        return RecursiveAdd(first, second, first.Length - 1, result);
    }

    private byte[] RecursiveAdd(byte[] first, byte[] second, int index, byte[] result, int carry = 0)
    {
        if (index < 0)
        {
            if (carry != 1)
            {
                return result;
            }

            List<byte> resultList = result.ToList();
            resultList.Insert(0, 1);
            return resultList.ToArray();
        }

        int sum = first[index] + second[index] + carry;
        carry = Math.DivRem(sum, Base, out int remainder);
        result[index] = (byte)remainder;
        return RecursiveAdd(first, second, index - 1, result, carry);
    }
}

Limitations

The program has an important limitation:
the two byte arrays must have the same size,
otherwise the result may be incorrect.
If the caller is lucky, the second is shorter than the first,
and the program will crash with an exception.
If the caller is unlucky, the first is shorter,
and the result will be incorrect,
quietly,
without any warning whatsoever.

At the minimum, this limitation should be documented.

Copying an array

When you reach the beginning and there is a carry,
so you need to grow the array to hold the extra value,
converting the array to a list is wasteful.
It would be better to allocate a new array with +1 size,
and use the Array.Copy method to copy the content.

Division is expensive

The use of Math.DivRem may seem clever,
but know that division and remainder computation is usually expensive.
If this operation was natural for your purpose,
that would be fine,
but it's not natural.
What you really need is check that sum >= Base,
and then perform sum -= Base if needed.
It would be better to write the code like that,
simply because that will be more natural to read.


Authenticating to Amazon EC2 Container Registry New to bash. The script I wrote works, but I don't believe it is written in the best way possible.

The script only assesses how to execute the aws ecr get-login command depending on the parameters and versions of the aws-cli and docker currently installed on the host machine.

#!/usr/bin/env bash

set -e

declare aws_region="${AWS_REGION:?''}"
declare registry_id="${REGISTRY_ID:?''}"
declare docker_version=$(docker version -f {{.Client.Version}} | sed 's/[^0-9.]//g')

if ! [[ -z $aws_region ]]; then
    $aws_region = "--region $aws_region"
else
    echo "Need to set AWS_REGION (eg. us-west-2)"
fi

if ! [[ -z $registry_id ]]; then
    $registry_id = "--registy-id $registry_id"
fi

if [[ $docker_version < '17.06' ]]; then
    options = "--no-include_email"
fi

query="aws ecr get-login $aws_region $registry_id $options"
echo $query
$($query)

if ! [[ $? == 0 ]]; then
    echo "---ERROR! Please login to AWS with myob-auth or ADFS."
    exit 1
fi

Any ideas for improvements? I'm personally getting bad smells in the code from the 3 if statements and the way the $query variable works.

Syntax errors

$aws_region = "--region $aws_region"

This is not how you set a variable in Bash. The correct syntax is

aws_region="--region $aws_region"

Likewise for the later statements. Note that there are no spaces before and after the =, and you drop the sigil before the variable name when you're assigning to it.

Use the appropriate shell parameter expansion

If you consult the Shell Parameter Expansion section of the Bash  manual, you'll see that

declare registry_id="${REGISTRY_ID:?''}"
if ! [[ -z $registry_id ]]; then
    $registry_id = "--registy-id $registry_id"
fi

can be expressed more succinctly as

declare registry_id=${REGISTRY_ID:+--registy-id "${REGISTRY_ID}"}

and similarly for aws_region.

Use debugging prints

If you don't like the way the $query variable works, you can just execute the command without assigning it to a variable and still have Bash print the executed command (to stderr) with set -x in a subshell environment.

(set -x; aws ecr get-login "$aws_region" "$registry_id" "$options")

Redirect error messages to stderr

It is customary to print error messages to stderr for easy filtering, so write the last echo as

>&2 echo "---ERROR! Please login to AWS with myob-auth or ADFS."

Other notes

I personally prefer writing typeset over declare because the latter is a bashism whereas the former would also work in ksh, and it is the same amount of typing.
Looking at the Docker Engine API, it seems that the Docker version follows the common major.minor.build-patch format, so I would choose the more light-weight and simpler in syntax cut over sed and move the self-evident version-getting command into the test conditional.

if [[ $(docker version -f {{.Client.Version}} | cut -d'-' -f1) < 17.06 ]]
then
    options="--no-include_email"
fi

Don't write if ! [[ $? == 0 ]] and if ! [[ -z $aws_region ]]. Write if [[ $? != 0 ]] and if [[ -n $aws_region ]]. Okay, more leeway to the second one because I believe fewer people know about test -n than test -z, but at least it is not an inferior option.


Rock, Paper, Scissors 3-part Write a program that lets a user play "Rock,
  Paper, Scissors" against the computer. The program should ask the user
  to choose one of the three choices, and then the computer randomly
  picks one (without knowing what the user has chosen). For this
  problem, the user should be asked to enter an integer: 0 for rock, 1
  for paper, or 2 for scissors. Rock beats scissors, scissors beats
  paper, paper beats rock. The program should say who wins, and then
  keep playing until someone (the user or the computer) has won 10
  rounds. The computer needs to keep track of the current score and
  should also display it before each round.

import java.util.Scanner;
public class rock_paper
{

public static void main(String args[])
{
    int count_win=0;
    int user_choice;
    String choice=" ";
    int num_comp_wins=0;
    int num_user_wins=0;
    while(num_comp_wins!=10||num_user_wins!=10)
    {
    /* Read user input and determine choice */

    System.out.println("\n Let us play Rock Paper Scissors \n");
    System.out.println("\n Enter your choice 0 for rock, 1 for paper, or 2 for scissors \n");
    Scanner keyboard = new Scanner(System.in);
    user_choice=keyboard.nextInt();
    System.out.println("\n Your choice was \n"+user_choice);
    if(user_choice==0)
    {
        choice="rock";
    }
    else if(user_choice==1)
    {
        choice="paper";
    }
    else if(user_choice==2)
    {
        choice="scissor";
    }
    else
    {
        System.out.println("Incorrect choice");
        choice=null;
    }
    System.out.println(choice);

    /* Randomly generate computer input */

    Random rand = new Random();
    int computer_choice = rand.nextInt(3) + 1;
    String choice1="";
    if(computer_choice==0)
    {
        choice1="rock";
    }
    else if(computer_choice==1)
    {
    choice1="paper";
    }
    else if(computer_choice==2)
    {
    choice1="scissor";
    }
    else
    {
        System.out.println("Incorrect choice");
        choice1=null;
    }
    System.out.println(choice1);
   /* Compare user and computer response*/
    if (choice.equalsIgnoreCase("rock")) {
    if (choice1.equalsIgnoreCase("scissor"))
    {
        System.out.println("\nThe user wins!");
        num_user_wins++;
    }
    else if (choice1.equalsIgnoreCase("paper"))
    {
        System.out.println("\nThe computer wins!");
        num_comp_wins++;
    }
    else
        System.out.println("Tie");
}
else if (choice.equalsIgnoreCase("paper"))
{
    if (choice1.equalsIgnoreCase("scissor"))
    {
        System.out.println("\nThe computer wins!");
        num_comp_wins++;
    }
    else if (choice1.equalsIgnoreCase("rock"))
    {
        System.out.println("\nThe user wins!");
        num_user_wins++;
    }
    else
         System.out.println("Tie");
}
else if (choice.equalsIgnoreCase("scissor"))
{
    if (choice1.equalsIgnoreCase("rock"))
    {
        System.out.println("\nThe computer wins!");
        num_comp_wins++;
    }
    else if (choice1.equalsIgnoreCase("paper"))
    {
        System.out.println("\nThe user wins!");
        num_user_wins++;
    }
    else
         System.out.println("Tie");
}

}

}
}

Naming
Java classes start with a capital letter and do not use underscores, so rock_paper should be RockPaper. Java uses camelCase, not snake_case, for all variable names except constants (static finals), so user_choice should be userChoice.
Spacing
The convention in java is to include whitespace before and after operators such as +, =, != or ==, so count_win=0 should be countWin = 0. Curly braces should be on the same line as an if or else.
Variable Declarations
You declare a new Random() inside your loop, so you might wind up making 19 Random objects, when you only need one. Declare it outside the loop. Conversely, you declare user_choice and computer_choice outside the loop, but they're only used inside. They should have the smallest possible scope.
Branching
You're branching (using if conditional statements) a lot to look up the name of a numeric selection. If you've learned about arrays, you could use a String[] of choices to simplify that lookup. There are a ton more branches to determine who wins. You only need one comparison to determine if there's a tie, and then only three specific checks to see if the user wins. Otherwise, the computer wins.
Making all those changes might look something like this:
import java.util.Random;
import java.util.Scanner;

public final class RockPaperScissors {

    private static final String[] CHOICES = { "rock", "paper", "scissors" };

    public static void main(final String[] argv) {
        final Random random = new Random();
        final Scanner keyboard = new Scanner(System.in);

        int computerWins = 0;
        int userWins = 0;

        System.out.println("\n Let us play Rock Paper Scissors \n");
        while (computerWins <= 10 && userWins <= 10) {

            /* Read user input and determine choice */
            System.out.println("\n Enter your choice 0 for rock, 1 for paper, or 2 for scissors \n");
            final int userChoice = keyboard.nextInt();
            if (userChoice < 0 || userChoice > 2) {
                System.out.println("Incorrect choice");
                continue;
            } else {
                System.out.println("\n Your choice was \n" + CHOICES[userChoice]);
            }

            /* Randomly generate computer input */
            final int computerChoice = random.nextInt(3) + 1;
            System.out.println("\n The computer choice was \n" + CHOICES[computerChoice]);

            if (userChoice == computerChoice) {
                System.out.println("Tie");
                continue;
            }

            if (userChoice == 0 && computerChoice == 1
                    || userChoice == 1 && computerChoice == 2
                    || userChoice == 2 && computerChoice == 0) {
                System.out.println("\nThe user wins!");
                userWins++;
            } else {
                System.out.println("\nThe computer wins!");
                computerWins++;
            }
        }
    }
}

Review Note: I intentionally don't close the Scanner because I'm assuming that hasn't been covered yet.


Determine children within edge and return a list of tuple pairs I've been studying hard and asking a lot of questions - I finally came a cross an exercise in LYAH that looked like it was easy enough but a perfect candidate for practicing.

The below program has a bunch of helper funcs in there, but the one of interest is neighbors/3.  It takes a Pair type and Edge number, and a BiMorph type. It then determines all of the children within that edge and returns it as a list of tuple pairs.

I would like a more experienced Haskeller to come in and tear it apart or to give me constructive criticism.

import Control.Monad
import Control.Applicative

data Pair a    = Pair a a deriving (Show, Eq)
data BiMorph a = BiMorph Func Func

type Neighbors = [(Int, Int)]
type Matrix    = [[Int]]
type Func      = (Int -> Int -> Int)

instance Functor Pair where
   fmap f (Pair a b) = Pair (f a) (f b)

instance Applicative Pair where
   pure a = Pair a a
   (Pair fa fb) <*> (Pair a b) = Pair (fa a) (fb b)

matrix :: Int -> Int -> Matrix
matrix xl yl = replicate yl $ [1..xl]

cartesianPairs :: Func -> Pair Int -> [Pair Int]
cartesianPairs f p =
    [(f <$> (Pair x y) <*> p) | x <- [1,0], y <- [0,1], (x,y) /= (0,0)]

cartesianDoubles :: Func -> Func -> Pair Int -> [Pair Int]
cartesianDoubles f g (Pair x y) = [Pair (f x 1) (g y 1), Pair (g x 1) (f y 1)]

dropP :: Pair Int -> (Int, Int)
dropP (Pair x y) = (abs x, abs y)

liftP :: (Int, Int) -> Pair Int
liftP (x, y) = Pair x y

neighbors :: Pair Int -> Int -> BiMorph Int -> Neighbors
neighbors p e (BiMorph f g) =
    map dropP $ concat [(cartesianPairs f p), (cartesianPairs g p), (cartesianDoubles f g p)]
        >>= (\(Pair x' y') -> guard (x' <= e && y' <= e)
            >> return (Pair x' y'))

Run hlint tool on your source to help you with getting rid of redundant brackets, $ and other minor suggestions.

In neighbors your using of guard in list monad can be changed to filter function:

neighbors :: Pair Int -> Int -> BiMorph Int -> Neighbors
neighbors p e (BiMorph f g) = map dropP $ filter ff $ concat [cartesianPairs f p, cartesianPairs g p, cartesianDoubles f g p] where
    ff (Pair x' y') = x' <= e && y' <= e

You can also hardcode the list into cartesianPairs as it's short:

cartesianPairs :: Func -> Pair Int -> [Pair Int]
cartesianPairs f p =
    [f <$> Pair x y <*> p | (x, y) <- [(1,0), (0,1), (1,1)]]

You can even use pairs directly:

cartesianPairs :: Func -> Pair Int -> [Pair Int]
cartesianPairs f p =
    [f <$> pair <*> p | pair <- [Pair 1 0, Pair 0 1, Pair 1 1]]

I also have an impression that cartesianPairs can be further simplified but I couldn't find anything that has clear meaning. Here are two attempts:

cartesianPairs f p =
     (<*> p) <$> (fmap . fmap) f [Pair 1 0, Pair 0 1, Pair 1 1]

cartesianPairs f p =
     (<*> p) . fmap f <$> [Pair 1 0, Pair 0 1, Pair 1 1]

cartesianDoubles can be shrinked the same way if you want:

cartesianDoubles f g (Pair x y) = [Pair (ff x) (gg y), Pair (gg x) (ff y)] where
    ff x = f x 1
    gg x = g x 1

cartesianDoubles f g p = [Pair ff gg <*> p, Pair gg ff <*> p] where
    ff x = f x 1
    gg x = g x 1

cartesianDoubles f g p = (<*> p) <$> [Pair ff gg, Pair gg ff] where
    ff x = f x 1
    gg x = g x 1

Yet another improvement. You can notice that there are versions of cartesianDoubles and cartesianPairs sharing the same (<*> p) <$> part:

cartesianPairs f p =
    (<*> p) <$> (fmap . fmap) f [Pair 1 0, Pair 0 1, Pair 1 1]

cartesianDoubles f g p = (<*> p) <$> [Pair ff gg, Pair gg ff] where
    ff x = f x 1
    gg x = g x 1

Then notice that <$> in (<*> p) <$> is for Functor [] instance, so it's just plain map, and map has a property of commuting withconcat:map f . concat == concat . map f`.

So you can move the common fragment out:

cartesianPairs f p =
    (fmap . fmap) f [Pair 1 0, Pair 0 1, Pair 1 1]

cartesianDoubles f g p = [Pair ff gg, Pair gg ff] where
    ff x = f x 1
    gg x = g x 1

neighbors :: Pair Int -> Int -> BiMorph Int -> Neighbors
neighbors p e (BiMorph f g) = map dropP $ filter ff $ map (<*> p) $ concat [cartesianPairs f p, cartesianPairs g p, cartesianDoubles f g p] where
    ff (Pair x' y') = x' <= e && y' <= e

Now p argument is not used:

cartesianPairs f =
    (fmap . fmap) f [Pair 1 0, Pair 0 1, Pair 1 1]

cartesianDoubles f g = [Pair ff gg, Pair gg ff] where
    ff x = f x 1
    gg x = g x 1

neighbors :: Pair Int -> Int -> BiMorph Int -> Neighbors
neighbors p e (BiMorph f g) = map dropP $ filter ff $ map (<*> p) $ concat [cartesianPairs f, cartesianPairs g, cartesianDoubles f g] where
    ff (Pair x' y') = x' <= e && y' <= e

If you want, you can convert neighbors to use list comprehensions:

neighbors :: Pair Int -> Int -> BiMorph Int -> Neighbors
neighbors p e (BiMorph f g) =
    [ dropP xx
    | x <- concat [cartesianPairs f, cartesianPairs g, cartesianDoubles f g]
    , let xx @ (Pair x' y') = x <*> p
    , x' <= e && y' <= e ]

Or even

neighbors :: Pair Int -> Int -> BiMorph Int -> Neighbors
neighbors p e (BiMorph f g) =
    [ dropP xx
    | xx @ (Pair x' y') <- (<*> p) <$>
        concat [cartesianPairs f, cartesianPairs g, cartesianDoubles f g]
    , x' <= e && y' <= e ]

But I find it less clear.


How to improve this functional python trial division routine? The following functional program factors an integer by trial division. I am not interested in improving the efficiency (but not in decreasing it either), I am interested how it can be made better or neater using functional constructions. I just seem to think there are a few tweaks to make this pattern more consistent and tight (this is hard to describe), without turning it into boilerplate.

def primes(limit):
        return (x for x in xrange(2,limit+1) if len(factorization(x)) == 1)

def factor(factors,p):
        n = factors.pop()
        while n % p == 0:
                n /= p
                factors += [p]
        return factors+[n] if n > 1 else factors

def factorization(n):
        from math import sqrt
        return reduce(factor,primes(int(sqrt(n))),[n])

For example, factorization(1100) yields:

[2,2,5,5,11]

It would be great if it could all fit on one line or into two functions that looked a lot tighter -- I'm sure there must be some way, but I can not see it yet. What can be done?

A functional recursive implementation:

def factorization(num, start=2):
    candidates = xrange(start, int(sqrt(num)) + 1)
    factor = next((x for x in candidates if num % x == 0), None)
    return ([factor] + factorization(num / factor, factor) if factor else [num])

print factorization(1100)
#=> [2, 2, 5, 5, 11]

Check this.


Database query with Eloquent I have got two tables articles and tags in the database. My goal was to retrieve all fields from articles table and one category field from tags table with Eloquent. I couldn't do this so instead I came up with the following method in my controller.

public function showCategory($category){
    $data = DB::select('select articles.*,
                                tags.category
                                from articles
                                join tags on articles.id = tags.article_id
                                where lang = ? AND tags.category = ?', ['ru', $category]);

    return $data;
}

In my Article and Tag models a have those relationships, respectively:

Article.php

  public function tags(){
    return $this->hasMany('\App\Tag');
}

Tag.php

public function articles(){
    return $this->hasMany('App\Article');
}

This implementation works for me, but I am still curious how can I achieve the same result with Eloquent and eager load tag's category.

The relation hasMany required a correspondent field in the target table.

I guess in your case you want manyToMany relationship, you are currently using oneToMany.

Be careful about the grammar, an Article belongsToMany Tags, not an Article hasMany Tags.

After re-coding your relationships with belongsToMany method you can simply bind your articles and tags within a pivot table that contains both the article_id and tag_id.

As an example I would recommend having a look at a package in GitHub.
You should also have a look at this for raw information about Many to Many relationships in Laravel.

When it comes to the query you send via Laravel's Query\Builder;

You can start trying it with an Eloquent\Builder

Article::join('tags', 'articles.id', '=', 'tags.article_id')
    ->where('articles.lang', '=', 'ru')
    ->where('tags.category', '=', $category)
    ->get([
        'articles.*',
        'tags.category'
    ]);

If the select at the end confuses you, you can also use the select method on the Query\Builder to specify the columns you want.

Article::select([
        'articles.*',
        'tags.category'
    ])
    // You could also give each element of this array
    // as a parameter to the `select` method
    // It would look like this:
    //      select('articles.*', 'tags.category')
    ->join('tags', 'articles.id', '=', 'tags.article_id')
    ->where('articles.lang', '=', 'ru')
    ->where('tags.category', '=', $category)
    ->get();


Build a string for a search query I am building a string for a search query and I think I am doing it the wrong way.

So basically I have serveral textfields, dates, integers and so on in one form. On submit I am checking the forms if its empty or not and building the query string.

I am submitting the model and building a kind of "pyramid" by checking if it is null or not, which is ending in a very large if statement (marked emphasis).

public string CreateSearchQuery(SearchViewModel model)
    {
        StringBuilder buildQueryString = new StringBuilder(2048);

        if (model == null)
        {
            //empty model: return newest results
            return buildQueryString.Append("SELECT * FROM newTbl.files").ToString();
        }

        buildQueryString.Append("SELECT * FROM newTbl.files INNER JOIN newTbl.folders ON folders.id = f_folders_id WHERE ");

        if (model.NumberClubs > 0)
        {
            buildQueryString.Append("folders.numberClubs = ");
            buildQueryString.Append(model.NumberClubs);
        }

        if(model.MrDateFrom != null)
        {
             //those if statements will get very large, when I have more then ten fields
            *if (model.NumberClubs > 0)*
            {
                buildQueryString.Append(" AND ");
            }
            buildQueryString.Append("folders.mrDate = '");
            buildQueryString.Append(model.MrDateFrom.Value.ToString("dd/MM/yyyy"));
            buildQueryString.Append("'");
        }

        if (model.Period > 0)
        {
            //those if statements will get very large, when I have more then ten fields
            *if (model.NumberClubs > 0 || model.MrDateFrom != null)*
            {
                buildQueryString.Append(" AND ");
            }
            buildQueryString.Append("folders.period = ");
            buildQueryString.Append(model.Period);
        }

        buildQueryString.Append(" ORDER BY newTbl.period DESC");

        return buildQueryString.ToString();
    }

I don't think that this is the correct way. So I have a model with several values, but to build a query string I have to check if it is empty or not to append the AND? I mean it is working, but it is really complicated (in my opinion).

As t3chb0t suggested, when creating SQL based on user input use ALWAYS SqlParameters to prevent sql injection!

Instead of creating SQL using a StringBuilder, I would try to abstract common structures and create classes to represent them. That makes the code more readable and maintainable.

A more structured solution could look like that:

    private class Condition
    {
        public Condition(string field, string parameterName, object paramerterValue)
        {
            this.Field = field;
            this.ParameterName = parameterName;
            this.ParameterValue = paramerterValue;
        }

        public string Field { get; }
        public string ParameterName { get; }
        public object ParameterValue { get; }

        public override string ToString() => $"{Field} = @{ParameterValue}";
    }

    public class SqlQuery
    {
        public SqlQuery(string sql, SqlParameter[] parameters = null)
        {
            this.Sql = sql;
            this.SqlParameters = parameters ?? new SqlParameter[0];
        }

        public string Sql { get; }
        public SqlParameter[] SqlParameters { get; }
    }

    public SqlQuery CreateSearchQuery(SearchViewModel model)
    {
        if (model == null) return new SqlQuery("SELECT * FROM newTbl.files");

        var sql = "SELECT * FROM newTbl.files fi INNER JOIN newTbl.folders fo ON fo.id = fi.f_folders_id";

        var conditions = new List<Condition>();
        var sqlParameters = new SqlParameter[0];
        var pcount = 1;

        if (model.NumberClubs > 0) conditions.Add(new Condition("fo.numberClubs", $"p{pcount++}", model.NumberClubs));
        if (model.MrDateFrom != null) conditions.Add(new Condition("fo.mrDate", $"p{pcount++}", model.MrDateFrom.Value));
        if (model.Period > 0) conditions.Add(new Condition("fo.period", $"p{pcount++}", model.Period));

        if (conditions.Count > 0)
        {
            var whereClause = " WHERE " + string.Join(" AND ", conditions);
            sqlParameters = conditions.Select(c => new SlqParameter(c.ParameterName, c.ParameterValue)).ToArray();
            sql += whereClause;
        }

        sql += " ORDER BY newTbl.period DESC";

        return new SqlQuery(sql, sqlParameters);
    }


Happy Number Counter Task

Write an implementation that determines the count of happy numbers between a given start and end value.

Implementation

public class HappyNumberCounter {

  public static int getLastDigit(final long number) {
    return Math.toIntExact(number % 10);
  }

  public static long digitSquareSumCalculator(final long number) {
    int remainder = getLastDigit(number);
    long quotient = number / 10;
    long digitSum = remainder * remainder;
    while (quotient > 0) {
      remainder = getLastDigit(quotient);
      quotient = quotient / 10;
      digitSum = remainder * remainder;
    }

    return digitSum;
  }

  public static long countHappyNumbersInRange(final long startInclusive, final long endInclusive) {
    final Map<Long, Boolean> happyNumbers = new HashMap<>();
    long count = 0;
    for (long index = startInclusive; index <= endInclusive; index++) {
      long number = index;

      // case 1: already been cached
      // case 2: has not been cached
      // case 3: has not been cached but starts with 1 or 4
      // case 4: has not been cached and does not start with 1 or 4

      Boolean isHappy = happyNumbers.get(number);
      if (isHappy == Boolean.TRUE) {
        count++;
      } else if (isHappy == null) {
        long lastDigit = getLastDigit(number);
        if (lastDigit == 1) {
          happyNumbers.put(number, Boolean.TRUE);
          count++;
        } else if (lastDigit == 4) {
          happyNumbers.put(number, Boolean.FALSE);
        } else {
          final List<Long> numbers = new ArrayList<>();
          while (lastDigit != 1 && lastDigit != 4 && isHappy == null) {
            numbers.add(number);
            number = digitSquareSumCalculator(number);
            isHappy = happyNumbers.get(number);
            lastDigit = getLastDigit(number);
          }

          for (final Long num : numbers) {
            happyNumbers.put(num, isHappy);
          }

          if (isHappy) {
            count++;
          }
        }
      }
    }
    return count;
  }

}

Feedback

I feel like my implementation is too long (i.e. there's too much logic happening in one method), but I can't seem to find any natural break points.

First of all, your code doesn't work properly: there are 143 happy numbers between 1 and 1000 but your code (after fixing a NullPointerException) will count 614. The logic is indeed too complicated and you can simplify this a lot.

First of all, let's consider the method that calculates the sum of squares of each digits. Instead of digitSquareSumCalculator, a more descriptive name would simply be sumOfSquaredDigits.

public static int getLastDigit(final long number) {
    return Math.toIntExact(number % 10);
}

Why use Math.toIntExact? No overflow can happen here since we're taking the modulo 10 so the result is between 0 and 9. If you're using just to cast the result to an int, it would be clearer to just cast.

public static long digitSquareSumCalculator(final long number) {
    int remainder = getLastDigit(number);
    long quotient = number / 10;
    long digitSum = remainder * remainder;
    while (quotient > 0) {
        remainder = getLastDigit(quotient);
        quotient = quotient / 10;
        digitSum = remainder * remainder;
   }
   return digitSum;
}

This contains duplicated logic: you can see that the first part of the method is the same as what goes in the while loop. You can fix that by not using a temporary variable quotient but reusing the variable number (and thereby dropping the final). The number given to the method won't be changed.

public static long sumOfSquaredDigits(long number) {
    long digitSum = 0;
    while (number > 0) {
        int remainder = (int) (number % 10);
        number /= 10;
        digitSum += remainder * remainder;
    }
    return digitSum;
}

This code shows that actually, you don't need the getLastDigit method.

The main part of the code is indeed clunky. You're missing an obvious method here: you want to determine if a number is happy or not, therefore it would make sense to have a method isHappy(number) that determines if the given number is happy or not.

The code of this method must do the following:

Calculates the sum of squared digits of the number
If the number obtained has already been obtained, return false; else continue. If we hit 1, return true.

We can store the visited numbers in a Set (to have constant-time look-up):

public static boolean isHappy(long number) {
    Set<Long> numbers = new HashSet<>();
    while (number > 1 && !numbers.contains(number)) {
        numbers.add(number);
        number = sumOfSquaredDigits(number);
    }
    return number == 1;
}

With such a method, you can then have a very simple for loop:

public static long countHappyNumbersInRange(final long startInclusive, final long endInclusive) {
    long count = 0;
    for (long index = startInclusive; index <= endInclusive; index++) {
        if (isHappy(index)) {
            count++;
        }
    }
    return count;
}

Now, we can make a simple but big improvement in terms of performance (just like you were trying to in your code). When we are determining if a given number is happy or not, we're also determining whether all the visited numbers are happy or not. So we don't need to check every number between the bounds and we can store a cache of happy or not happy numbers. So let's introduce a cache

private static final Map<Long, Boolean> CACHE = new HashMap<>();

that, for a given number, will hold whether it is a happy number of not. Then, when the method isHappy is running, we can stop early if we encounter a number we've already seen before. If that's the case, then we put in the map all the numbers encountered with the happyness of the starting number.

public static boolean isHappy(long number) {
    if (CACHE.containsKey(number)) {
        return CACHE.get(number);
    }
    Set<Long> numbers = new HashSet<>();
    while (number > 1 && !numbers.contains(number)) {
        numbers.add(number);
        number = sumOfSquaredDigits(number);
        if (CACHE.containsKey(number)) {
            boolean result = CACHE.get(number);
            for (long n : numbers) {
                CACHE.put(n, result);
            }
            return result;
        }
    }
    boolean result = number == 1;
    for (long n : numbers) {
        CACHE.put(n, result);
    }
    return result;
}

I ran this code to determine all the happy numbers between 1 and 1 million and it gave the result instantly.


Return most recent non null field values for multiple unknown fields I am attempting to create a stored procedure that looks at one table and imports changes in that table to another.

This stored procedure is going to be preformed on multiple source tables and will dump out into multiple log tables.  The way the data in the log table is stored looks like this:

ID | Source_PK_field | field_1 | field_2 | field_3 | Update_Type | Updated
01 |        001      |  value  |  value  |  value  |      C      | 2015-02-04
02 |        001      |   null  |  change |   null  |      U      | 2015-02-05
03 |        001      |  change |   null  |  change |      U      | 2015-02-06

The idea being that if a record changes the log table would indicate all the fields that changed with their new values but leave fields that didn't change for the record null.

In order for this to work I need to grab a copy of what the record looked like in the most recent log version vs the current source version (the problem being that some of the values in the most recent log will be null)

In order to accomplish that I am using the current query inside my stored procedure:

DECLARE @latest_sql VARCHAR(MAX) = 'SELECT [ID], [' + @relationKey + '] '

SELECT @latest_sql = @latest_sql + ', (SELECT TOP 1 [' + COLUMN_NAME + '] FROM (SELECT * FROM ' + @changelog + ' x WHERE x.[' + @relationKey + '] = CT.[' + @relationKey +'] AND [' + COLUMN_NAME + '] IS NOT NULL) q ORDER BY UPDATED DESC) AS [' + COLUMN_NAME +'] '
FROM #COLUMN_NAMES
WHERE COLUMN_NAME != @relationKey

SET @latest_sql = @latest_sql + ' INTO ##LATEST_UPDATES FROM ' + @changelog + ' CT'

EXEC(@latest_sql)

The stored procedure requires you to pass it @changelog the log table, @source the source table and @relationKey the primary key that ties the two tables together (usually the source PK). And #COLUMN_NAMES is a temp table created before this is executed in the stored procedure that simply stores column names that match between the source table and the log table (not all fields from the source are being logged).

Now this code behaves as intended, the problem is the query takes about 10-20min to complete (just this part of the stored procedure).  The heaviest query this is used on is looking at 48 fields in a table that only stores 300K records.  There has got to be a way I can make this query faster.

Since you are using a stored procedure, it is not much of a stretch to extend that to be a trigger.

Triggers are the natural tool to use for this type of problem, and are designed to have two 'virtual' copies of the data, one copy is the value before the change, the other copy is the value after the change. In your case, you would have a trigger for insert (create), delete, and update.

Read up on triggers here: DML Triggers and the syntax here: CREATE TRIGGER (Transact-SQL)

In the trigger, you would scan the inserted and deleted virtual tables for the data that is changing, and use that to maintain the audit logs in your logging table. Note that thoese virtual tables contain only the data affected, so scanning them is fast.

For more information on those tables, see The Inserted/Deleted table documentation

Using the above mechanism, you can track the changes as they happen. Each instance will be relatively quick, though the work will happen inside the same transaction as the inserts to the main table.

Using a stored procedure like you have, is a good first step, but you should transfer the relevant logic in to the trigger instead.


Is this binary search close to idiomatic OCaml? let binsearch ~arr ~cmp x = (
    let cmpx = cmp x in
    (* Assuming arr is ordered according to cmp, then (bs min max) is an   *)
    (* index of a value v in arr such that ((cmp x value) = 0) between     *)
    (* min (inclusive) and max (exclusive).                                *)
    let rec bs min max =
      if min >= max then (
        None
      ) else (
        (* mid is always strictly less than max because min < max. *)
        let mid = (min + max) / 2 in
        let delta = (cmpx (Array.get arr mid))
        in (
          if delta = 0 then (
            Some mid
          ) else if delta < 0 then (
            (* since mid < max, we make progress *)
            bs min mid
          ) else (
            bs (mid + 1) max
          )))
    in bs 0 (Array.length arr));;

and then it can be used thus

let a = [|1; 2; 4; 4; 4; 7; 9; 9; 11; 16|];;

let bs = binsearch ~arr:a ~cmp:compare;;

let results = (bs 0, bs 1, bs 2, bs 3, bs 4, bs 5, bs 6, bs 7, bs 8,
               bs 9, bs 10, bs 11, bs 12, bs 13, bs 14, bs 15, bs 16, bs 17);;

 (results = (None, Some 0, Some 1, None, Some 2, None, None, Some 5, None,
             Some 7, None, Some 8, None, None, None, None, Some 9, None));;

Generally speaking, yes. I only have a couple of minor remarks.

let cmpx = cmp x in

Using partially-applied functions is actually slightly slower with the native-code compiler. The cmp function (like most functions) isn't going to do anything until it gets all its arguments, so cmpx under the hood is fun y => cmp x y. It's faster (not by much, but it can matter in an inner loop) if you write let delta = cmpx x (Array.get arr mid) below.

Speaking of which, arr.(mid) is exactly identical to (Array.get arr mid), but more usual. If you're going for speed, write Array.unsafe_get arr mid, which will save a few cycles, at the expense of putting the onus on you to verify that you are not making an array access out of bounds (here, it's easy to see that you are not).

You have many superfluous parentheses; they are a little surprising to a habitual Ocaml programmer. let … in … has lower precedence than anything else, and if … then … else … has lower precedence than arithmetic and function calls. This is how I'd write your function:

let binsearch ~arr ~cmp x =
  (* Assuming arr is ordered according to cmp, then [bs min max] is an   *)
  (* index of a value v in arr such that [(cmp x value) = 0] between     *)
  (* min (inclusive) and max (exclusive).                                *)
  let rec bs min max =
    if min >= max then
      None (* we are down to an empty slice of the array *)
    else
      (* mid is always strictly less than max because min < max. *)
      let mid = (min + max) / 2 in
      let delta = cmp x (Array.unsafe_get arr mid) in
      if delta = 0 then Some mid else
      if delta < 0
      then bs min mid
      else bs (mid + 1) max
  in bs 0 (Array.length arr);;

(Details of when to put newlines, especially relative to in, are matters of taste.)


Calculate highscores based on sales data The background is my question is SO How to create a list of totals for durations?
where I found a recursive solution that I want to review. I could not test it for higher levels than 1 but to the best of my knowledge it should work.

What I'm trying to do is calculate a kind of "highscore", i.e. a level based on sales during the two historical months where sales where the most. So I find the oldest purchase, use the helper methods to calculate the bonus for that period and then check to see whether that period is the entitling to a level upgrade. It's basically going through all the historical orders.

Is my recursion written correctly?

def level(self):
    startdate = model.Order.all().filter('status =', 'PAID'
            ).filter('distributor_id =',
                     self._key.id()).get().created.date()
    last_day_nextmonth = startdate + relativedelta(months=2) \
        - timedelta(days=1)
    total = self.personal_silver(startdate, last_day_nextmonth) + self.non_manager_silver(startdate, last_day_nextmonth)
    if total >= 125:
        level = 5
    elif total >= 75:
        level = 4
    elif total >= 25:
        level = 3
    elif total >= 2:
        level = 2
    else:
        level = 1
    return self.levelHelp(level, last_day_nextmonth + timedelta(days=1))

def levelHelp(self, level, startdate):

    last_day_nextmonth = startdate + relativedelta(months=2) \
        - timedelta(days=1)
    total = self.personal_silver(startdate, last_day_nextmonth) + self.non_manager_silver(startdate, last_day_nextmonth)
    if total >= 125:
        newlevel = 5
    elif total >= 75:
        newlevel = 4
    elif total >= 25:
        newlevel = 3
    elif total >= 2:
        newlevel = 2
    else:
        newlevel = 1

    maxlevel = level if level > newlevel else newlevel

    nextstart = last_day_nextmonth + timedelta(days=1)
    now = datetime.now().date()
    if nextstart > now: #next start in is the future
        return maxlevel
    else: return self.levelHelp(maxlevel, nextstart)

The helper functions are

def non_manager_silver(self, startdate, enddate):
    silver = 0
    timeline = date(startdate.year, startdate.month, 1)
    downline = User.query(User.sponsor == self._key).fetch()
    distributor = self
    while distributor.has_downline():
        downline = User.query(User.sponsor
                              == distributor.key).fetch()
        for person in downline:
            orders = model.Order.all().filter('distributor_id =',
                    person.key.id()).filter('created >',
                    startdate).filter('created <',
                    enddate).filter('status =', 'PAID'
                    ).fetch(999999)
            for order in orders:
                for (idx, item) in enumerate(order.items):
                    purchase = model.Item.get_by_id(long(item.id()))
                    amount = int(order.amounts[idx])
                    silver = silver + amount * purchase.silver \
                        / 1000.000
            distributor = person
    return silver

def personal_silver(self, startdate, enddate):
    p_silver = 0
    timeline = date(startdate.year, startdate.month, 1)  # first day of month
    orders = model.Order.all().filter('distributor_id =',
            self._key.id()).filter('created >',
            timeline).filter('created <', enddate).filter('status ='
            , 'PAID').fetch(999999)
    for order in orders:
        for (idx, item) in enumerate(order.items):
            purchase = model.Item.get_by_id(long(item.id()))
            amount = int(order.amounts[idx])
            p_silver = p_silver + amount * purchase.silver \
                / 1000.000  # remember amounts
    return p_silver

def level(self):
    startdate = model.Order.all().filter('status =', 'PAID'
            ).filter('distributor_id =',
                     self._key.id()).get().created.date()
    last_day_nextmonth = startdate + relativedelta(months=2) \
        - timedelta(days=1)

Continuing lines with \ is generally frowned upon. Either split it onto multiple lines or wrap the expression with parenthesis

    total = self.personal_silver(startdate, last_day_nextmonth) + self.non_manager_silver(startdate, last_day_nextmonth)

I think writing a self.silver(start_date, end_date) function would make sense to get the sum of those two.

    if total >= 125:
        level = 5
    elif total >= 75:
        level = 4
    elif total >= 25:
        level = 3
    elif total >= 2:
        level = 2
    else:
        level = 1

I'd store all of these levels in a list like [0, 2, 25, 75, 125]. Then I'd use functions from bisect to figure out which level I was at.

    return self.levelHelp(level, last_day_nextmonth + timedelta(days=1))

def levelHelp(self, level, startdate):

    last_day_nextmonth = startdate + relativedelta(months=2) \
        - timedelta(days=1)
    total = self.personal_silver(startdate, last_day_nextmonth) + self.non_manager_silver(startdate, last_day_nextmonth)
    if total >= 125:
        newlevel = 5
    elif total >= 75:
        newlevel = 4
    elif total >= 25:
        newlevel = 3
    elif total >= 2:
        newlevel = 2
    else:
        newlevel = 1

Deja Vu! You've got the exact same code again, at the very least, move the common code into another function.

    maxlevel = level if level > newlevel else newlevel

Use maxlevel = max(level, newlevel)

    nextstart = last_day_nextmonth + timedelta(days=1)
    now = datetime.now().date()
    if nextstart > now: #next start in is the future
        return maxlevel
    else: return self.levelHelp(maxlevel, nextstart)

You shouldn't really be using recursion here. This is very naturally a while loop. I think if you rewrite this as a while loop, it'll be clearer and you'll be able to combine these two functions.

def non_manager_silver(self, startdate, enddate):
    silver = 0
    timeline = date(startdate.year, startdate.month, 1)

I'm not clear why you back this up to the start of the month rather then using the startdate.

    downline = User.query(User.sponsor == self._key).fetch()

This has no effect, because you throw it away as soon as you enter the loop

    distributor = self
    while distributor.has_downline():
        downline = User.query(User.sponsor
                              == distributor.key).fetch()

        for person in downline:
            orders = model.Order.all().filter('distributor_id =',
                    person.key.id()).filter('created >',
                    startdate).filter('created <',
                    enddate).filter('status =', 'PAID'
                    ).fetch(999999)

Why 999999?

            for order in orders:
                for (idx, item) in enumerate(order.items):

No need for those parens.

                    purchase = model.Item.get_by_id(long(item.id()))
                    amount = int(order.amounts[idx])

Using int and long seems a little odd. Are you converting from strings?

                    silver = silver + amount * purchase.silver \
                        / 1000.000

Use silver += amount * purchase.silver / 1000.000

            distributor = person

See, here is where you should be using recursion. I assume that you are trying to go through the downlines of all the downlines. However, as your code is written, you'll only go through the downlines of the last member of the downline.

    return silver

def personal_silver(self, startdate, enddate):
    p_silver = 0
    timeline = date(startdate.year, startdate.month, 1)  # first day of month
    orders = model.Order.all().filter('distributor_id =',
            self._key.id()).filter('created >',
            timeline).filter('created <', enddate).filter('status ='
            , 'PAID').fetch(999999)
    for order in orders:
        for (idx, item) in enumerate(order.items):
            purchase = model.Item.get_by_id(long(item.id()))
            amount = int(order.amounts[idx])
            p_silver = p_silver + amount * purchase.silver \
                / 1000.000  # remember amounts

    return p_silver

This function is pretty much exactly the same as the previous function. You should be able to move common parts into a function.

I'd approach this whole thing quite differently.

def find_downline(distributor):
    """
    Given a distributor, collect all the members of his downline into a list
    """
    downline = [distributor]
    immediate_downline = User.query(User.sponsor == distributor.key)
    for member in immediate_downline:
        downline.extend( find_downline(member) )
    return downline

def find_orders(distributor):
    return model.Order.all().filter('distributor_id =',
            self._key.id()).filter('status ='
            , 'PAID').fetch()

def silver_in_order(order):
    total = 0
    for item, amount in zip(order.items, order.amounts):
        purchase = model.Item.get_by_id(long(item.id()))
        total += amount * purchase.silver
    return total

def silver_bimonth(distributor):
    bimonths = collections.defaultdict(int)
    for member in find_downline(distributor):
         for order in find_orders(member):
             date = order.created.date
             bimonth_index = (date.year * 12 + date.month) / 2
             bimonths[bimonth_index] += silver_in_order(order)
    return = max(bimonths.value)

def level(distributor):
    silver = silver_bimonth(distributor)
    return bisect.bisect_left(LEVELS, silver) + 1

Zero testing. Probably doesn't work. But it should give an idea of an alternate approach. Basically, I figure that since you are going to fetch all the orders anyways, it makes more sense to fetch them all figure which bimonthly period they belong to, and figure out the maximum that way.


Correctness of a minimal parallel workers queue in Ruby I've programmed a minimal parallel workers queue in Ruby, and I wanted to know if it's correct, and if there are simpler ways of implementing it.

I'm aware of the existence of MonitorMixin, but it won't save any code, I think.

The design is kept intentionally trivial (e.g. it uses Thread.abort_on_exception).

# Usage:
#
#   queue = ParallelWorkersQueue.new( <threads> )
#   queue.push { <operation_1> }
#   queue.push { <operation_2> }
#   queue.join
#
class ParallelWorkersQueue

  def initialize( slots )
    @max_slots  = slots
    @free_slots = slots

    @mutex              = Mutex.new
    @condition_variable = ConditionVariable.new

    Thread.abort_on_exception = true
  end

  def push( &task )
    @mutex.synchronize do
      while @free_slots == 0
        @condition_variable.wait( @mutex )
      end

      @free_slots -= 1
    end

    Thread.new do
      yield

      @mutex.synchronize do
        @free_slots += 1

        @condition_variable.signal
      end
    end
  end

  def join
    @mutex.synchronize do
      while @free_slots < @max_slots
        @condition_variable.wait( @mutex )
      end
    end
  end

end

Like @Victor Moroz, I'm also a fan of Queue.  I also prefer to keep the threads around: it's a bit more efficient, but more important, the code is simpler:

require 'thread'

class ParallelWorkersQueue

  def initialize(slots)
    @work = Queue.new
    @threads = slots.times.map do
      Thread.new do
        while (work = @work.deq) != :stop
          work[]
        end
      end
    end
  end

  def join
    @threads.each do
      @work.enq(:stop)
    end
    @threads.each(&:join)
  end

  def push(&task)
    @work.enq(task)
  end

end

Example of its use:

pwq = ParallelWorkersQueue.new(2)
5.times do |i|
  pwq.push do
    puts i
  end
end
pwq.join

# => 0
# => 1
# => 2
# => 3
# => 4

Replace Queue with SizedQueue if you wish to limit the amount of work that can be awaiting a worker thread.


Counting the number of digits with a recursion algorithm in c I'm learning recursion and I think that I can finally wrap my head around it.

I had to write a program that given an integer would print out its number of digits and I wanted to know if it is correctly implemented and if there's anything I can do to make it better.

Here's my code:

#include<stdio.h>

int countDigits(int n);

int main(void){
  int n,result;
  printf("What is the number?\n");
  scanf("%d", &n);
  result=countDigits(n);
  printf("%d\n", result);
  return 0;
}

int countDigits(int n){
  if(n>=0&&n<10){
    return 1;
  }
  else{
    return 1+countDigits(n/10);
  }
}

I unfortunately cannot comment (not enough reputation), but in terms of performance and generated assembly code both variants are exactly same:

int countDigitsSane (int n){
  int digits=1;
  for(int i=n; i>9; i/=10)
  {
    digits++;
  }
  return digits;
}

int countDigits(int n){
  if(n<10){
    return 1;
  }
  else{
    return 1+countDigits(n/10);
  }
}

Pay attention, I have removed the redundant check in the recursive snippet.With that correction, the code generated by the compiler is almost 100% same, save for different choice of registers.

Having said that, it is indeed, better to avoid recursion in imperative languages (such as C++), however in functional languages (Haskell, OCaml etc.) , recursion is a normal thing, and the compiler guarantees it can optimise tail-recursive calls.


Return a new array where each element is the number of smaller elements to the right of that element in the original input The task:

  Given an array of integers, return a new array where each element in
  the new array is the number of smaller elements to the right of that
  element in the original input array.

  For example, given the array [3, 4, 9, 6, 1], return [1, 1, 2, 1, 0],
  since:

  There is 1 smaller element to the right of 3
  There is 1 smaller element to the right of 4
  There are 2 smaller elements to the right of 9
  There is 1 smaller element to the right of 6
  There are no smaller elements to the right of 1

const lst = [3, 4, 9, 6, 1];

My solutions:

const numberOfSmallerElem = lst => lst.map((x,i) => lst.slice(i + 1).reduce((acc,y) => y < x ? ++acc : acc, 0));
console.log(numberOfSmallerElem(lst));

function numberOfSmallerElem2(lst) {
  for (let i in lst) {
    lst[i] = lst.slice(i).reduce((acc,y) => y < lst[i] ? ++acc : acc, 0);
  }
  return lst;
}
console.log(numberOfSmallerElem2(lst));

function numberOfSmallerElem3(lst) {
  const ret = [];
  for (let i = 0, len = lst.length; i < len - 1; i++) {
    const reference = lst[i];
    let counter = 0;
    for (let j = i + 1; j < len; j++) {
      if(lst[j] < reference) { counter++; }
    }
    ret.push(counter);
  }
  ret.push(0);

  return ret;
}
console.log(numberOfSmallerElem3(lst));

Performance

The posted implementations are quadratic.
A log-linear solution exists.
Here's a hint:

   What if you insert values from right to left into a sorted list?

Avoid mutations in functional implementations

Can you spot the mutation on this line?

const numberOfSmallerElem = lst => lst.map((x,i) => lst.slice(i + 1).reduce((acc,y) => y < x ? acc++ : acc, 0));

It's actually not easy to spot it when the line is so long!
I suggest to break it up, for readability.
And then eliminate the mutation.

   Replace acc++ with acc + 1


Find the majority element, which appears more than half the time The task:

  Given a list of elements, find the majority element, which appears
  more than half the time (> floor(len(lst) / 2.0)).

  You can assume that such element exists.

  For example, given [1, 2, 1, 1, 3, 4, 0], return 1.

My solution:

const lst = [1, 2, 1, 1, 3, 4, 0];
const findMajorityElem = lst => lst.reduce((acc, x) => {
  acc[x] = acc[x] ? acc[x] + 1 : 1;
// If I can assume that such an element exists, then it's sufficient to check which element occurs the most.
  if (!acc.major || acc.major[1] < acc[x]) { acc.major = [x, acc[x]]; }
  return acc;
}, {major: null}).major[0];

console.log(findMajorityElem(lst));

If it is known that a majority element exists, then the efficient algorithm to use is the Boyer-Moore majority vote algorithm, which requires only O(1) space.


Validation invoking queries? I have a question regarding whether it is viable or deemed correct if a validation object invoked a query.  Essentially, I have some code that looks like this:

public interface ICountryCodeValidator
{
    IEnumberable<ValidationError> Validate(string countryCode);
}

public class CountryCodeValidator : ICountryCodeValidator
{
    private readonly IGetCountryCodeQuery getCountryCodeQuery;

    public CountryCodeValidator(IGetCountryCodeQuery getCountryCodeQuery)
    {
        this.getCountryCodeQuery = getCountryCodeQuery;
    }

public IEnumberable<ValidationError> Validate(string countryCode)
{
    var errors = new List<ValidationError>();

    if (string.IsNullOrEmpty(countryCode) || countryCode.Length > 2)
    {
        errors.Add(new ValidationError { Code = "InvalidRequest", Message = "Country code is invalid." });
    }
    else
    {
        if (this.getCountryCodeQuery(countryCode) == null)
        {
            errors.Add(new ValidationError { Code = "NotFound", Message = "CountryCode not found." });
        }
    }

    return errors;
}
}

Immediately, I am thinking that CountryCodeValidator responsibilities are growing and whether it should be querying the DB is debatable. Is it perfectly OK for an object simply responsible for validation to perform such an operation?

Further elaborating on this, I have unit tests surrounding CountryCodeValidator and assert against the error codes returned ensuring the correct codes are returned depending on the input (which is fine).

The trouble is, within my Web API controller I have to perform checks on the error code being returned back and depending on the error code create the appropriate status code. For example:

public class CountriesContoller : ApiController
{
    private readonly ICountryCodeValidator countryCodeValidator;
    private readonly ICountryCodeRepository countryCodeRepository;

    public CountriesContoller(ICountryCodeValidator CountryCodeValidator, ICountryCodeRepository CountryCodeRepository)
    {
        this.countryCodeValidator = countryCodeValidator;
        this.countryCodeRepository = countryCodeRepository;
    }

    public HttpResponseMessage Get([FromUri]string countryCode)
    {
        var errors = this.countryCodeValidator.Validate(countryCode);

        if (errors.Any())
        {
            //Code smell?
            var countryNotFoundError = errors.FirstOrDefault(x => x.Code == "NotFound");

            if (countryNotFoundError != null)
            {
                return Request.CreateResponse(HttpStatusCode.NotFound, countryNotFoundError);
            }

            return Request.CreateResponse(HttpStatusCode.BadRequest, errors);
        }

        return Request.CreateResponse(HttpStatusCode.OK, this.countryCodeRepository(countryCode));
    }
}

I guess, the bit I am concerned with is the fact I am leaking the error code definitions in each of my domains and tests.

Web API layer I am performing a linq query to check if this error has been added
Domain layer needs to know about this due to validation
Tests are aware as I am testing whether a NotFound is invoked, so when I mock the CountryCodeValidator I am ensuring it returns a collection of errors with a particular error code.

A quick easy thing to do is create constants or an enum so I am not using magic strings but still this doesn't seem right.

I realise I have 2 calls to the database being made:

One to verify the call is not null
Second to get the country and return it back

This is simply pseudo code.

I believe it's ok to use db data inside a validator's body. As long as all you do with database is reading data, and this data is neccesary for validation. But if you're about to change something in database - that's pretty considerable.

Then, if you can do something with this data, like caching it or smth, you may want to consider moving this part of validator's logic into some abstraction, like CountryCodeHandler or something like this.

I would also point out on your working with string values. I don't think it's good to use those "Not found" or "InvalidRequest" strings. If the string is exactly what you need, i'd advise you to use some static class:

public static class MyHttpStatusCode
{
    public const string NotFound = "Not Found";
    public const string InvalidRequest = "Invalid Request";
}

Thus, you'll get rid of duplicates in your code. Or, if those strings aren't that unavoidable, you can just use HttpStatusCode enum from System.Web (as i recall).

Also in modern versions of C# you don't need the "this" keyword to access to fields and properties those names arent match any variable inside the method or constructor. Unless it's your codestyle that declares such thing.

As for your controller part, i'd recommend you to use the Chain of Responsibility pattern. It will allow you to extend validation results and handle them properly.

public interface ICountryCodeValidationResultFilter
{
    ValidationError[] Filter(List<ValidationError> errors, out HttpStatusCode statusCode);
}
public interface IConcreteCodeValidationResultFilter
{
    bool CanFilter(List<ValidationError> errors);
    ValidationError[] Filter(List<ValidationError> errors, out HttpStatusCode statusCode);
}

public class CountryCodeValidationResultFilter : ICountryCodeValidationResultFilter
{
    private readonly IConcreteCodeValidationResultFilter[] filters;

    public CountryCodeValidationResultFilter(IConcreteCodeValidationResultFilter[] filters)
    {
        this.filters = filters;
    }

    public ValidationError[] Filter(List<ValidationError> errors, out HttpStatusCode statusCode)
    {
        if (errors.Any())
        {
            for (var filter in this.filters) // you may just write "filters" here
            {
                if (filter.CanFilter(errors))
                {
                    return filter.Filter(errors, out statusCode);
                }
            }
        }

        statusCode = HttpStatusCode.OK;
        return new ValidationError[0];
    }
}

public NotFoundCodeValidationResultFilter : IConcreteCodeValidationResultFilter
{
    public bool CanFilter(List<ValidationError> errors)
    {
        return errors.Any(e => e.Code == "Not Found");
    }

    public ValidationError[] Filter(List<ValidationError> errors, out HttpStatusCode statusCode)
    {
        statusCode = HttpStatusCode.NotFound;
        return new[] { errors.First(e => e.Code == "Not Found") };
    }
}

public InvalidRequestCodeValidationResultFilter : IConcreteCodeValidationResultFilter
{
    public bool CanFilter(List<ValidationError> errors)
    {
        return errors.All(e => e.Code != "Not Found");
    }

    public ValidationError[] Filter(List<ValidationError> errors, out HttpStatusCode statusCode)
    {
        statusCode = HttpStatusCode.NotFound;
        return errors.ToArray();
    }
}

Thus, your controller will look like this:

public class CountriesContoller : ApiController
{
    private readonly ICountryCodeValidator countryCodeValidator;
    private readonly ICountryCodeRepository countryCodeRepository;
    private readonly ICountryCodeValidationResultFilter countryCodeValidationResultFilter;

    public CountriesContoller(
        ICountryCodeValidator countryCodeValidator,
        ICountryCodeRepository countryCodeRepository,
        ICountryCodeValidationResultFilter countryCodeValidationResultFilter)
    {
        this.countryCodeValidator = countryCodeValidator;
        this.countryCodeRepository = countryCodeRepository;
        this.countryCodeValidationResultFilter = countryCodeValidationResultFilter;
    }

    public HttpResponseMessage Get([FromUri]string countryCode)
    {
        var errors = countryCodeValidator.Validate(countryCode);
        HttpStatusCode resultStatusCode;
        var filteredErrors = countryCodeValidationResultFilter.Filter(errors, out resultStatusCode);

        return Request.CreateResponse(resultStatusCode, filteredErrors.Any() ? filteredErrors : countryCodeRepository(countryCode));
        // i believe countryCodeRepository is not just Func, huh? perhabs it should be "countryCodeRepository.Get(countryCode)" ?
    }
}


Embedding a widget to a page I have the following code that our users will use to embed a widget on their page:

var ttp_number = "1Z123489754897";
var ttp_key = "1234567890123457890123";
var ttp_width = 850;
var ttp_height = 650;
var ttp_m_width = 260;
var ttp_m_height = 200;

(function(){
    document.write('

');
    s=document.createElement('script');
    s.type="text/javascript";
    s.src="//example.com/embed.js";
    setTimeout("document.getElementById('ttp').appendChild(s);",1);
})();

Is there a way to reorganize or make this more compact (other than minifying)?

Right now the big list of variables feels inefficient just declaring them one at a time, but I'm not sure what the alternate would be.

NOTE: I need to keep the script as compact as possible as the people who are copy/pasting this code many times know very little (if any) javascript, so if they some massive block of code, they'll freak out or stand a bigger chance of screwing it up.

Don't make your code more compact, it's a futile exercise. Use a minifier or even a compiler/optimizer. Your source code should be as readable as possible
document.write is evil, don't use it.

Here's how I would write it:

var tpp = {
    number: "1Z123489754897",
    key: "1234567890123457890123",
    width: 850,
    height: 200,
    "m width": 260,
    "m height": 200
};

function init(settings) {
    var div, script;
    div = document.createElement("div");
    div.id = "ttp";
    document.body.appendChild(div);
    script = document.createElement("script");
    script.src = "//example.com/embed.js";
    script.type = "text/javascript";
    document.body.appendChild(script);
}

window.attachEvent("load", function() {
    init(tpp);
});
/* or */
window.addEventListener("load", function() {
    init(tpp);
});

Adding the event listener on window.load avoids having to do the setTimeout later on.
Also, when doing a setTimeout, don't use the eval variant, use an anonymous function:

setTimeout(function () {
    document.getElementById('ttp').appendChild(s);
}, 1);


Manager that collects and handles multiple tasks that run on events like window.scroll, window.resize etc I'm working on a website where different things can occur on resize and scroll events. I only want one event handler that comes into play, whenever something in the DOM must be changed.

So I created a TaskManager where you can register objects and callbacks, that are run, when the specific event is fired.

The TaskManager is injected into other objects, where this.taskManager.registerTask() is called to send all necessary information.

The TaskManager then executes all registered tasks, as soon as an event occurs. It also sends the current scrollTop-value to the callback.

class TaskManager {
  constructor() {
    this.DOM = {
      html: document.getElementsByTagName('html')[0],
    }

    this.tasks = {
      scroll: [],
      resize: [],
    };

    window.addEventListener('scroll', event => {
      window.requestAnimationFrame(timestamp => {
        this.processTasksOnScroll();
      });
    });
  }

  registerTask(type, task) {
    if (!this.tasks[type]) {
      return false;
    }

    this.tasks[type].push(task);
    return true;
  }

  processTasksOnScroll() {
    const scrollTop = window.scrollY || window.scrollTop || this.DOM.html.scrollTop;

    for (const task of this.tasks.scroll) {
      task.target[task.callback](scrollTop);
    }
  }
}

class Test {
  constructor(config) {
    this.taskManager = config.taskManager || null;

    if (this.taskManager) {
        this.taskManager.registerTask('scroll', {target: this, callback: 'onScroll'});
    }
  }

  onScroll(scrollTop) {
    console.log('Callback in Test with scrollTop: ' + scrollTop);
  }
}

const taskManager = new TaskManager();
const test = new Test({taskManager: taskManager});
.test {
  height: 120vh;
  padding: 20px;
  background: rgba(0, 0, 0, 0.2);
  text-align: center;
}

Scroll here …

Is this a good approach or could something be improved?

Some points.

window is the default object. You don't need to use is. It is highlighted by the fact you don't use it sometimes window.document, or window.TaskManager but then for objects you are unsure of window.addEventListener you add it.
It is not a good idea to stack requestAnimationFrame requests.

Events related to mouse movements can fire at rates much higher than the display refresh rate of 60fps.
It introduces an up to 16ms delay on the event (and yes users will notice it as a sluggish response).
If used in conjunction with standard event handlers you will be handling events out of order.
Completely decouples the handler from the event.

You have decoupled (ignoring above point) the event from the handler because you don't pass on the event object. If you are going to manage events you should pass on the event object so that the handler can access the event.
registerTask will happily push events to the stack without checking if the supplied object is a valid. When you come to handling the event eg processTasksOnScroll it will throw if task.target or task.callback do not result in a function reference. If it throws none of the following events will be handled.
If any of the tasks throws an uncaught error then all following tasks will not be called.

Without knowing more about the project its hard to say more about this approch.

I do feel personally that events should not be one to many. For each event object created there should be one unbroken execution path that completely handles the event. Having events trigger more events is just going to result in an unmanageable mess, as you add code to conditional communicate between handlers wanting priority, or exclusivity.

That is not to say you can not have one handler for many events and then selectively redirect the event to a specific handler.


Reading lines from a file and processing them concurrently I'd like to read lines from a file and process them concurrently.  I came up with the following code to do this:

var wg sync.WaitGroup

func main(){
  file := "data/input.txt"
  reader, _ := os.Open(file)
  scanner := bufio.NewScanner(reader)
  for scanner.Scan() {
    wg.Add(1)
    go processLine(scanner.Text())
  }
  wg.Wait()
}

func processLine(line string) {
  time.Sleep(time.Duration(rand.Intn(5)) * time.Second)
  fmt.Println("line:", line)
  wg.Done()
}

I added the random sleep time in there to simulate potential differences in processing times.

Is there any potential drawbacks that I should be aware of with this method of concurrent processing?  Are there any better ways that I should consider concurrently processing lines in a file?

The general approach seems fine. You may want to benchmark your application based on the type of inputs you'll receive and the type of computation that's done. If the computation is CPU intensive, it may not make sense to have too many goroutines running in parallel since they can't all use the CPU at the same time.

If you find that's the case, it could be better to have a channel into which you send the lines that are being read and have a bunch of worker goroutines that read a line from the channel, process it, read another line and so on. Benchmarking should give you a good idea about the right approach.

If each goroutine makes a request to an external resource (such as a web service or a database) you'll have to think about ways to limit the rate of such requests.

Some general comments about the code which I think you should fix before putting it into production:

The code doesn't have any error checking or a way to report errors from the processing of each input line. You'll have to add those in.
You're also using a global variable to share the WaitGroup, which I think you should avoid. Either pass it in as a function parameter or use an anonymous function that calls processLine() and then wg.Done() (instead of calling wg.Done() in processLine())
As I mentioned above you may want to consider limiting the number of concurrent goroutines depending on your use case.