Easier user input in C++ A more proper version of this utility can be found at the following link here: Giveth me thy easier user input in C++ - follow up.

I've always been a little bothered by the fact, that in order to get user input in C++, one had to use up to three lines of code in an ugly manner like the below to get user input for a specific type, with a prompt:

int user_input;
std::cout << ">> ";
std::cin << user_input;

So, in order to make this process easier, I've created an easy_input function which allows for the user to specify a type, a prompt, and set user input to a variable, all in one line.

easy_input.h

#ifndef EASY_INPUT_H_
#define EASY_INPUT_H_
#pragma once

#include <iostream>
#include <string>

// We're simply "re-defining" the standard namespace
// here so we can patch our easy_input function into
// it for the user's sake.
namespace std
{
    template <typename TInput>
    TInput easy_input(const std::string& prompt);
}

/**
 * This function serves as a wrapper for obtaining
 * user input. Instead of writing three lines every
 * time the user wants to get input, they can just
 * write one line.
 * @param {any}    TInput - The type of the input.
 * @param {string} prompt - The prompt to use for input.
 * @returns - Whatever the user entered.
 */
template <typename TInput>
TInput std::easy_input(const std::string& prompt)
{
    TInput user_input_value;
    std::cout << prompt;
    std::cin >> user_input_value;
    return user_input_value;
}

#endif

main.cpp (tests)

#include <iostream>
#include <string>
#include "easy_input.h"

int main()
{
    const std::string user_input1 = std::easy_input<std::string>(">> ");
    std::cout << user_input1 << "\n";

    const int user_input2 = std::easy_input<int>(">> ");
    const int user_input3 = std::easy_input<int>(">> ");
    std::cout << user_input2 + user_input3 << "\n";
}

I'd (preferably) like to know the following things:

Am I using templates appropriately? I feel like I might have done something wrong here in the process.
Is there anything that can be improved performance-wise?
Is there a need for include guards?
Is it okay to patch easy_input into std without problems? Is this a good practice?
Is there anything else that's glaringly wrong?

namespace std

Others have said this already but it's important enough to be repeated: Don't put your own definitions into namespace std.  It's undefined behavior.

About the only thing I'm aware of that you may put into namespace std are specializations for templates already defined by the standard.  So, for example, if you have

struct MyType
{
  int a;
};

inline constexpr bool
operator==(const MyType& lhs, const MyType& rhs) noexcept
{
  return lhs.a == rhs.a;
}

then you may do

#include <functional>  // std::hash

namespace std
{

  template <>
  struct hash<MyType>
  {
    using argument_type = MyType;
    using result_type = std::size_t;

    result_type
    operator()(const argument_type& mt) const noexcept
    {
      return mt.a;
    }
  };

}

so you can, say, use MyType as key in a std::unordered_map.

The whole point of namespaces is to separate stuff. So put your own stuff into your own namespace. For example, Boost has its stuff in namespace boost and sub-namespaces thereof. You could use namespace ethan_bierlein or something. In other languages, it is common practice to use one's domain as in package com.example.myproduct (assuming you own example.com) but I haven't seen this practice in C++ yet.

Correctness

Consider the following program.

int
main()
{
  const auto age = easy_input<int>("How old are you? ");
  std::cout << "Hello, " << age << " year old!\n";
}

If compiled and run as this

$ ./a.out
How old are you? 17
Hello, 17 year old!

everything seems fine.  However, if the user inputs nonsense,

$ ./a.out
How old are you? don't care
Hello, 0 year old!
$

the result is probably not what you'd expect from a well-designed user interface.  However, it gets worse.

int
main()
{
  const auto ounces = easy_input<int>("How many ounces of beer dou you want? ");
  const auto age = easy_input<int>("How old are you? ");
  //std::cout << "ounces = " << ounces << ", age = " << age << "\n";
  if (age >= 18)
    std::cout << "Here are your " << ounces << " ounces of beer.\n";
  else
    std::cout << "Sorry, you're not old enough.\n";
}

In action:

$ ./a.out
How many ounces of beer dou you want? too many
How old are you? Here are your 0 ounces of beer.
$

Don't worry if you cannot reproduce the results of the above two examples result of the second example. It's undefined behavior so anything (including but not limited to your cat getting pregnant) could happen.

What's the reason for this?

There are two problems.  First, in

TInput user_input_value;        // (1)
std::cout << prompt;            // (2)
std::cin >> user_input_value;   // (3)
return user_input_value;        // (4)

if TInput is a builtin type like int, the variable user_input_value is not initialized on line 1. If the input on line 3 succeeds, the value is set to the input, which is fine. However, if the input is invalid, no value will be assigned and you'll return the uninitialized value. If the stream is good(), the extraction operator on line 3 will either successfully extract and assign the value or, if invalid input is given, set the failbit and assign 0. Therefore, if the first input is invalid, 0 will be returned (for ounces) and the failbit set. Then, on the second entry, nothing is assigned to user_input_value and an uninitialized int returned (for age).  This results in undefined behavior.

Running the above examples second (“too many ounces of beer”) example through a tool like Valgrind can unveil the error. (To my surprise, neither ASan nor UbSan were able to detect the error.)

  I originally thought that if input fails for whatever reason, the destination value would never be changed. This seemed to be the case but apparently was changed in C++11 such that now 0 is assigned for invalid input provided the stream was good() to begin with. Thanks to Mooing Duck for discovering this (see comments).

The seemingly simple fix to this problem is to use value-initialization for user_input_variable.

TInput user_input_value {};

Since your question is tagged with C++14, we can now at least use one C++11 feature (uniform initialization) with pride.

However, while this “solution” fixes the undefined behavior, it still has its issues. If the user enters invalid input, it should probably be yelled at and not 0 (or whatever a default-constructed TInput is) returned silently.  So you should really check the stream after the operation.

TInput user_input_value {};  // value-initialization not strictly needed any more
std::cout << prompt;
if (std::cin >> user_input_value)
  return user_input_value;
throw std::istream::failure {"bad user input"};

You could have the same effect by setting the exceptions mask of std::stdin but this would also affect other uses of std::cin even outside your function so it might surprise users of your function. I'd consider a utility function messing with the mask bad.

Some people will argue that a user inputting invalid data is by no means an “exceptional” event so throwing an exception is inappropriate. If you're like them, you might prefer returning an std::experimental::optional<TInput> result.  Unfortunately, it is not standard yet but many implementations support it and there is a readily available version in Boost.Optional.

But this is still not as useful as it could be.  Consider the “beer” example again.

$ ./a.out
How many ounces of beer dou you want? 4 2
How old are you? Sorry, you're not old enough.
$

The first invocation of your function happily consumes the 4, stops at the white-space and leaves the 2 on the stream. The second invocation then sees the 2 readily available and without waiting for any user input, immediately and equally happily returns 2.

cr_oag has suggested you address this by calling std::cin.ignore.  However, I don't think that this is the ideal solution. If you ask the user how many ounces of beer she wants and she inputs 4 2, it is unlikely she really meant 4.  It would be better to treat this as an error and ask for clarification.

There is another, closely related, problem with your current version. Consider how this seemingly reasonable program…

int
main()
{
  const auto name = easy_input<std::string>("What's your name? ");
  std::cout << "Hello, " << name << "!\n";
}

…behaves not quite as expected.

$ ./a.out
What's your name? Ethan Bierlein
Hello, Ethan!
$

While it might create a warm and cozy feeling to be called with the first name by the program, most of us would probably consider this a bug.

For least surprise, I recommend that you always treat user input line-oriented: Read a single, but complete, line, and treat its entirety as one input. You can easily do this via std::getline. You can then try to convert that string into the target type and see if it has any characters remaining.

std::cout << prompt;
std::string line {};
if (!std::getline(std::cin, line))
  throw std::istream::failure {"I/O error"};
std::istringstream iss {line};
TInput value {};
if (!(iss >> value) || !iss.eof())
  throw std::istream::failure {"bad user input"};
return value;

This is still not ideal. While it behaves as expected for, say, numbers, the example asking for a name would now reject any input longer than one word. Not quite what we want. You would need some template meta-programming to special-case string input. But explaining how that would work is too much to go through in a code review.

As a matter of fact, there is already a function template in Boost.Lexical_Cast that does this (and more) for you (#include <boost/lexical_cast.hpp>).

std::string line {};
std::cout << prompt;
if (!std::getline(std::cin, line))
  throw std::istream::failure {"I/O error"};
return boost::lexical_cast<TInput>(line);

Portability

user86418 has already mentioned that #pragma once is not portable but if you use it, the #include guards become redundant. On the other hand, #pragma once can be more efficient than #include guards because the preprocessor can immediately stop processing the file while with #include guards, it has to process the whole header anyway to find out where the matching #endif is. So if you want the best of both worlds, you could use something like this.

#if HAVE_PRAGMA_ONCE
#pragma once
#endif

#ifndef EASY_INPUT_H
#define EASY_INPUT_H

// ...

#endif

Your users could then compile with -DHAVE_PRAGMA_ONCE to eventually speed up their compilations while still being portable to implementations that don't have #pragma once. If you're used to using GNU Autoconf, you'll be very familiar with those HAVE_${FEATURE} macros.

Another option is to use tricks like

#ifdef __GNUC__
#pragma once
#endif

(GCC defines __GNUC__) but I don't like this kind of being clever very much and prefer having the user have the last word.

As Mooing Duck points out, compilers ought to ignore unknown #pragmas. However, it is still a good idea to make them conditional. For example, if you compile with -Werror=unknown-pragmas (And you should, because it is enabled by -Wall -Werror.), GCC will reject code with unknown #pragmas. This is conforming as in standard configuration, GCC ignores them gracefully as it is supposed to. Your library should not force your users to use less rigorous warning levels. Such “noisy” or “unclean” libraries are very annoying, to say the least.

Genericity

Others have suggested making the stream to read from a parameter or accepting other prompts than strings. I don't think that this would add much value as I've yet to see a program that needs to read interactive user input from anything but standard input. Some lost souls might want to read from std::wcin, though. Those would probably also be interested in a std::wstring prompt that would be streamed to std::wcout. Eventually, there could be good reasons to print the prompt to standard error output (for example if standard output is to be redirected to some file).

Having an overload that uses no (or a default) prompts seems useful but anything else I think is superfluous.  A default parameter comes in handy here.

template <typename TInput>
TInput easy_input(const std::string& prompt = "");

Yes, you'll needlessly construct a temporary std::string if you call the function with a string literal but so what (see next section)?  And I don't think that using, say, an int as prompt would be a very frequent use-case.

Performance

  Is there anything that can be improved performance-wise?

Maybe, but don't worry about it. You are asking for user input. It would take very bad code for an input function that would be slow enough to make any difference compared to how long humans need to read, think and type.

Going Fancy

If you want to make your program fancy, consider adding line-editing support. It is very annoying for a user, having to delete an almost completely typed answer only because you realize that you have a typo in the first few characters. The GNU Readline library provides a de-facto standard mechanism for this. With GNU Readline there also goes GNU History which allows a user to re-fetch a previous answer using the arrow-keys. This is very handy if you have to give the same answer or similar answers many times. Of course, you should make the feature optional because your users might not have installed the library.

Also note the license requirements; quoting the project website:

  Readline is free software, distributed under the terms of the GNU General Public License, version 3. This means that if you want to use Readline in a program that you release or distribute to anyone, the program must be free software and have a GPL-compatible license.

If you don't publish your software at all, you're also fine. So you can always experiment around freely for your personal learning and private usage.

There might be other libraries providing similar line-editing support but I don't know any of them.

Williham Totland commented that there is another free software library for line-editing, Linenoise. While checking its website, I also found the Editline Library (libedit) (also free software). I've never used either of them.

Style

Besides the fact that you shouldn't declare stuff in namespace std, you have put the DocString alongside with the implementation. If you decide to separate declaration and definition, the DocString should go with the declaration because that's what is important for your users.

I assume you wanted to write a DocString that can be processed by Doxygen. If so, note that you should use @tparam to document template arguments.


Insertion sort implementation in C++ The parameters list contain the array that is passed to and the size of the array respectively. Is there anywhere I could improve my code?

void insertionSort(int a[], int s) {
    for (int i = 0; i < s - 1; i++) {
        int j = i + 1;
        int key = a[j];
        while (i >= 0 && a[i] > key) {
            a[i+1] = a[i];
            i--;
        }
        a[i+1] = key;
    }
}

In both this and your bubble sort implementation you need to start using C++ idioms (otherwise this is just C code (that's fine C is a great language it's just not C++)).

I would say this is the perfect C implementation of insertion sort.

But usually when you implement these kinds of algorithms in C++ you use iterators to specify the range (as this allows you to sort several types of container including arrays).

Rather than:

void insertionSort(int a[], int s) {

You should probably use:

template<typename I>
void insertionSort(I begin, I end) {

Then you can sort any type of container with:

int  a[] = {1,4,5,27,3,9,1};
insertionSort(std::begin(a), std::end(a));

std::vector<int> v = {1,4,5,27,3,9,1};
insertionSort(std::begin(v), std::end(v));

etc...

Interestingly this also allows you to sort any type (not just integers). As long as your type supports the comparison operator. The standard library tends to implement any comparison in terms of operator<() so to avoid confusion with potential users you should implement your sort using the less than operator and potentially allow the user to specify the actual comparison operator.

// Unless you have an explicit override.
// std::less<T> uses `operator<()` to do the comparison.
// So this means the algorithm will work for type T under three situations.
//    1) operator< is defined for the type T
//    2) There is an specialization of `std::less<>` for type T
//    3) An explicit type is provided to the sort for comparing type T.
template<typename I, typename Comp = std::less<T>>
void insertionSort(I begin, I end) {
    Comp   comp;
    // Stuff


Texture managing with smart pointers I have an SDL_Wrapper which is working perfectly (it is not broken)! Please suggest how I could improve performance, and how I could make my unique_ptr(s) dispose automatically.

So far, I call my class like this:

CWindow window = std::make_unique<CWindowWrap>("title", 0, 0, 140, 100, 0)

Is it the best way to do it? Will my unique_ptr dispose automatically?

Here's the SDL_Wrapper class:

 #include "LogManager.h"
 #include <memory>
 #include <SDL.h>

 //SDL_Renderer wrapper

 ////SDL_Window wrapper !

 class CWindowWrap
 {

 public:
     CWindowWrap(const char* title, int xpos, int ypos, int width, int height, int flags)
         : ptr_window(SDL_CreateWindow(title, xpos, ypos, width, height, flags))
     {
        LOG("SDL_Window Wrapper", "Constructed Window !");
    }
     virtual ~CWindowWrap()
     {
         if (ptr_window != nullptr){
            SDL_DestroyWindow(ptr_window);
            LOG("SDL_Window Wrapper", "Destroyed Window !");
        }
        else
             LOG("SDL_Window Wrapper", "Window doesn't need to be destroyed !");
     }

     //disable copy constructor
     CWindowWrap(CWindowWrap const&) = delete;
    CWindowWrap& operator=(CWindowWrap const&) = delete;

    //allow move
    CWindowWrap(CWindowWrap&& move)
        : ptr_window(nullptr)
    {
         using std::swap;
         swap(ptr_window, move.ptr_window);
    }

     CWindowWrap& operator=(CWindowWrap&& move){
         using std::swap;
         swap(ptr_window, move.ptr_window);
     }

     operator SDL_Window*() { return ptr_window; } // implicite conversion between CWindowWrap ---> SDL_Window*;

 private:
     SDL_Window* ptr_window = nullptr;
 };

 typedef std::unique_ptr<CWindowWrap> CWindow;

 class CRendererWrap
 {
 public:

     CRendererWrap(CWindowWrap window, int x, int y)
         : ptr_renderer(SDL_CreateRenderer(window, x, y))
     {}
    ~CRendererWrap()
     {
        if (ptr_renderer != nullptr){
            SDL_DestroyRenderer(ptr_renderer);
        }
     }

     //disable copy constructor
     CRendererWrap(CRendererWrap const&) = delete;
     CRendererWrap& operator=(CRendererWrap const&) = delete;

     //allow move
     CRendererWrap(CRendererWrap&& move)
        : ptr_renderer(nullptr)
     {
        using std::swap;
        swap(ptr_renderer, move.ptr_renderer);
     }

     CRendererWrap& operator=(CRendererWrap&& move){
        using std::swap;
        swap(ptr_renderer, move.ptr_renderer);
    }

     operator SDL_Renderer*() { return ptr_renderer; } // implicite conversion between CRendererWrap ---> SDL_Renderer*;

 private:
      SDL_Renderer* ptr_renderer = nullptr;
 };

 typedef std::unique_ptr<CRendererWrap> CRenderer;

 struct Window_param
 {
     const char* title;
     const int width;
     const int height;
     const int xPos;
     const int yPos;
     const int flags;
 };

I think that conceptually you're fine. There are some details that need attention.

Hide the implementation

Your creation syntax:

CWindow window = std::make_unique<CWindowWrap>("title", 0, 0, 140, 100, 0)

requires the user to know that CWindow is a unique_ptr<CWindowWrap> this isn't ideal. I believe that it's a good idea to hide the implementation class.

You should put your CWindowWrap class in an implementation namespace:

namespace detail{
    class CWindowWrap{
        ...
    };
}
using CWindow = std::unique_ptr<detail::CWindowWrap>;

Then you should provide a creator function like this:

CWindow make_window(const char* title, int xpos, int ypos, int width, int height, Uint32 flags){
     return std::make_unique<detail::CWindowWrap>(title, xpos, ypos, width, height, flags);
}

Don't use virtual if you don't need it

CWindowWrap doesn't need a virtual destructor as far as I can tell and I can't imagine why you would inherit from this. So simply remove virtual.

Wrapper classes should match types exactly

You're not matching the types properly for the constructor of CWindowWrap (flags should be Uint32 see here).

Move assignment/construction

As you have already initialized ptr_window in the in-class declaration, you do not need : ptr_window(nullptr) in your move constructor.

Your move assignment operator is missing a return statement.

That said, as you are not supposed to instantiate CWindowWrap directly you could simply = delete the move assignment operator and constructor. They won't be used.

Same comments goes for the other wrapper.


Adding string binary numbers My requirement is to add two binary numbers, say "1001" and "0101" as binary1 and binary2.

Partial Class Default2
    Inherits System.Web.UI.Page
    Dim carry As Boolean = False ' Boolean variable to hold the carry if occured
    Protected Sub Button1_Click(sender As Object, e As System.EventArgs) Handles Button1.Click
        Dim binary1 As String = TextBox1.Text 'First binary number
        Dim binary2 As String = TextBox2.Text 'Second binary number
        Dim result As String = "" 'to store the result
        For i As Integer = Len(binary1 ) - 1 To 0 Step -1
            result = bin_add(getbyte(binary1 , i), getbyte(binary1, i)) & result ' calling function
        Next
        If carry = True Then
            result = "1" & result 'if a carry remains add it to the MSB
        End If
        MsgBox(rslt) 'Display the result
    End Sub

    Public Function bin_add(b1 As Boolean, b2 As Boolean) As String'Function which performs the addition of each single bits form the two inputs which is passed from the calling function
        Dim result As Boolean
        If b1 AndAlso b2 = True Then 'both values are 1/true
            If carry = True Then
                result = True
                carry = True
            Else
                result = False
                carry = True
            End If
        ElseIf b1 = False And b2 = False Then 'Both are 0/false
            If carry = True Then
                result = carry
                carry = False
            Else
                result = False
                carry = False
            End If
        Else
            If carry = True Then
                result = False
                carry = True
            Else
                result = True
                carry = False
            End If
        End If
        If result = True Then
            Return "1" 'return 1 for Boolean true
        Else
            Return "0" 'return 0 for Boolean false
        End If
    End Function

    Private Function getbyte(s As String, ByVal place As Integer) As String'Function for getting each individual letters from the input string. i got it from net.
        If place < Len(s) Then
            place = place + 1
            getbyte = Mid(s, place, 1)
        Else
            getbyte = ""
        End If
   End Function
End Class

Notes:

It gives good results for me only if the no. of digits in both numbers are the same.

Question:

How can I improve the code? Especially, how can I reduce the length of code?

For now, let's ignore the fact that there are easier ways to add binary numbers. There are other issues with this code.

Inherits System.Web.UI.Page

Why is code that adds binary numbers inheriting from a UI class? There's no need for this. Separate the concerns and create a module for this code instead.

carry is scoped to the class level. This is a symptom of problems in this code.

Protected Sub Button1_Click(sender As Object, e As System.EventArgs) Handles Button1.Click
    Dim binary1 As String = TextBox1.Text 'First binary number
    Dim binary2 As String = TextBox2.Text 'Second binary number
    Dim result As String = "" 'to store the result
    For i As Integer = Len(binary1 ) - 1 To 0 Step -1
        result = bin_add(getbyte(binary1 , i), getbyte(binary1, i)) & result ' calling function
    Next
    If carry = True Then
        result = "1" & result 'if a carry remains add it to the MSB
    End If
    MsgBox(rslt) 'Display the result
End Sub

Getting the values from the UI makes sense, but then you loop over bin_add, which obviously doesn't actually add anything, or you wouldn't need to loop or have a class variable. All of this logic should happen inside of bin_add.

bin_add should take in two strings, handle all of the logic, and return a single string representing the output. While I'm at it, methods should have PascalCased verb-noun names. This method should be called AddBinary and I will refer to it as such for the rest of the review.

As I said earlier, I wouldn't expect a Function that adds binary numbers to take in Boolean values. I would rather it actually take in a byte and overload the method to handle string representation, but I'm lazy and you don't seem to need all that.

Putting it all together, the signature line

Public Function bin_add(b1 As Boolean, b2 As Boolean) As String

Should look like this.

Public Function AddBinary(value1 as String, value2 as String) As String

Implementing this change will be left as an exercise for the reader.


Find and replace consecutive string values in a list Suppose I have a list that holds a list of column names:

columns = [
        'Village Trustee V Belleville', 'Unnamed: 74', 'Unnamed: 75',
        'Village President V Black Earth', 'Village Trustee V Black Earth',
        'Unnamed: 78', 'Unnamed: 79', 'Village President V Blue Mounds',
        'Village Trustee V Blue Mounds', 'Unnamed: 82',
        'Village Trustee V Cottage Grove', 'Unnamed: 94', 'Unnamed: 95',
        'Village President V Cross Plains', 'Village Trustee V Cross Plains',
        'Unnamed: 98', 'Unnamed: 99'
        ]

And I want to find all Unnamed: XX values and replace them consecutively with the first valid string. For example:

Input:
'Village Trustee V Belleville', 'Unnamed: 74', 'Unnamed: 75'

Output:
'Village Trustee V Belleville', 'Village Trustee V Belleville', 'Village Trustee V Belleville'

For this I have a function:

def rename_unnamed(columns):
    current_value = None
    pattern = re.compile(r'^Unnamed:\s\d+$')
    for column_index, column in enumerate(columns):
        if re.match(pattern, column):
            columns[column_index] = current_value
        else:
            current_value = column
    return columns

Which does the job pretty fine, however, I was wondering if this could be improved and/or simplified, as this looks like a mouthful for rather a simple task.

Don't mutate input and return.

If I need to pass columns unedited to two different functions, than your function will seem like it's not mutating columns and I wouldn't have to perform a copy of the data. But this isn't the case.
You may want to potentially error if there are no values before the first "Unnamed" value.
You may want to make pattern an argument, to allow reusability of the function. And instead use change it to a callback to allow even more reusability.

Overall your function's pretty good. I don't think it can be made much shorter.

def value_or_previous(iterator, undefined, default=None):
    prev_item = default
    for item in iterator:
        if not undefined(item):
            prev_item = item
        yield prev_item

def rename_unnamed(columns):
    return list(value_or_previous(columns, re.compile(r'^Unnamed:\s\d+$').match))


Routes for a user controller in a Node.js application I have a very simple Node application that routes requests to /users to the following file:

users.js

var express = require('express');
var router = express.Router();
var User = require('models/user');
var sequelizeSuccessHandler = require('lib/sequelizeSuccessHandler');
var sequelizeErrorHandler = require('lib/sequelizeErrorHandler');

router.get('/', function(request, response) {
  User.findAll()
  .then(sequelizeSuccessHandler(response))
  .catch(sequelizeErrorHandler(response));
});

router.post('/', function(request, response) {
  User.create(User.params(request.body))
  .then(sequelizeSuccessHandler(response))
  .catch(sequelizeErrorHandler(response));
});

router.get('/:id', function(request, response) {
  User.findOne({where: {id: request.params.id}})
  .then(sequelizeSuccessHandler(response))
  .catch(sequelizeErrorHandler(response));
});

router.patch('/:id', function(request, response) {
  User.findOne({where: {id: request.params.id}})
  .then(function(user) {
    return user.update(User.params(request.body));
  })
  .then(sequelizeSuccessHandler(response))
  .catch(sequelizeErrorHandler(response));
});

router.delete('/:id', function(request, response) {
  User.findOne({where: {id: request.params.id}})
  .then(function(user) {
    return user.destroy();
  })
  .then(sequelizeSuccessHandler(response))
  .catch(sequelizeErrorHandler(response));
});

module.exports = router;

lib/sequelizeSuccessHandler.js

var Sequelize = require('sequelize');

function successHandler(response) {
  return function(object) {
    response.send(JSON.stringify(object));
  };
}

module.exports = successHandler;

lib/sequelizeErrorHandler.js

var Sequelize = require('sequelize');

function errorHandler(response) {
  return function(error) {
    if (error.constructor == Sequelize.ValidationError) {
      response.status(400).send();
    } else {
      response.status(500).send();
    }
  };
}

module.exports = errorHandler;

Looking at users.js, you can see that I tried to DRY things up by creating sequelizeSuccessHandler and sequelizeErrorHandler (I hate those names), but there's still a lot of repetition. I feel like there is a better way to do this.

How can I clean this up?

Looking at users.js, you can see that I tried to DRY things up by creating sequelizeSuccessHandler and sequelizeErrorHandler (I hate those names), but there's still a lot of repetition.

I suppose you're talking about these repeated lines in the route handlers:

  .then(sequelizeSuccessHandler(response))
  .catch(sequelizeErrorHandler(response));

I don't really know Node,
but it seems to me that you could add another helper function:

function handle(query, response) {
  query
  .then(sequelizeSuccessHandler(response))
  .catch(sequelizeErrorHandler(response));
});

router.get('/', function(request, response) {
  handle(User.findAll(), response);
});

router.post('/', function(request, response) {
  handle(User.create(User.params(request.body)), response);
});

(The names "handle", and "query" might not be the best.)


Creating a Dynamic Select Statement from 2 databases on the same server Can someone improve on my method for creating union select views from 2 databases located within the same server in C#, here is the code for the method:

public override DataSet GetDynamicReportData(string strSQL) {

        var sbSQL = new StringBuilder(strSQL);
        var finalQueryStr = new StringBuilder(strSQL);

        sbSQL.Replace("SELECT DISTINCT ", "");

        var iamNEAS3 =
            new StringBuilder(" UNION SELECT DISTINCT ");

        var queryColumns = sbSQL.ToString().Split(',');

        for (var i = 0; i <= queryColumns.Length - 1; i++) {

            iamNEAS3.Append(queryColumns[i]);

            if (i < queryColumns.Length - 1) {
                iamNEAS3.Append(", ");
            } else if (i == queryColumns.Length - 1) {

                var appendMagic =
                    queryColumns[i].Replace(
                        "From [dbo].[vw_Classified_ApplicationsFiles]",
                        "FROM [project_phoenix].[dbo].[vw_Classified_ApplicationFiles]")

                iamNEAS3.Append(appendMagic);

                // Systematic removal; thank you!
                iamNEAS3.Replace(queryColumns[i], "");
            }
        }

        finalQueryStr.Append(iamNEAS3);

        return DALDynamicReport.GetData(finalQueryStr.ToString());
    }

Now it receives a string parameter where

strSQL = "Select Distinct [Row1], [Row2], [Row3], [Row4] From [project_mongoose].[dbo].[vw_Classified_ApplicationsFiles] where [Row2] <= 200".

I know my programming skills are limited, but is there a way to not code this by hand like I did?

The first thing you should improve is to split this method into two methods, one for parsing the input string and one for  building a new query. Doing both in the same method isn't a good practice.

This means that the first method should return a collection of columns that the other one consumes.


Getting prices and volume I'm learning F# and have a couple of routines much of whose functionality looks common so I am looking to refactor them together.

Here are the routines (which for the record I lifted from elsewhere:

let Prices time ID (polling:float) =
  let sync = System.Threading.SynchronizationContext.Current
  let obs = new Event<int>()
  let raiseEvent (value:int) = sync.Post((fun _ -> obs.Trigger(value)), null)
  let interval = TimeSpan.FromSeconds(polling)
  let rec loop nextTime= async {
    // Generate next value (on the GUI thread)
    do getPrices nextTime
      |> List.filter (fun price -> price.ID = ID)
      |> List.iter (fun price -> raiseEvent price.Price)
    // Wait some short time
    do! Async.Sleep(1000)
    // Continue looping
    do! loop (nextTime.Add(interval)) }
    loop time |> Async.Start
    obs.Publish

and

let Volumes time ID (polling:float) =
  let sync = System.Threading.SynchronizationContext.Current
  let obs = new Event<VolumeDTO>()
  let raiseEvent (value:VolumeDTO) = sync.Post((fun _ -> obs.Trigger(value)),  null)
  let interval = TimeSpan.FromSeconds(polling)
  let rec loop nextTime= async {
    // Generate next value (on the GUI thread)
    do getVolumes nextTime
      |> List.filter (fun volume -> volume.ID = ID)
      |> List.iter (fun volume -> raiseEvent volume)
    // Wait some short time
    do! Async.Sleep(1000)
    // Continue looping
    do! loop (nextTime.Add(interval)) }
    loop time |> Async.Start
    obs.Publish

The external function calls are hopefully reasonable self explanatory retrieving data from a db

let getPrices lastTime =
  createList<PriceDTO>((getPriceSql lastTime), priceReader)

let getVolumes lastTime =
  createList<VolumeDTO>((getVolumeSql lastTime), volumeReader)

The types are very simple, but I'm not sure I've got this 'right':

type IID =
  abstract member ID : int64

type PriceDTO(ID:int64, Price:int) =
  interface IID with
    member this.ID = ID
  member x.ID = ID
  member x.Price = Price

type VolumeDTO(ID:int64, Amount:decimal, Price:int32) =
  interface IID with
    member this.ID = ID
  member x.ID = ID
  member x.Amount = Amount
  member x.Price = Price

I've played around with something but I'm not at all there as yet:

let publish<'a> time ID (polling:float) dataRetriever =
  let sync = System.Threading.SynchronizationContext.Current
  let obs = new Event<'a>()
  let raiseEvent (value:'a) = sync.Post((fun _ -> obs.Trigger(value)), null)
  let interval = TimeSpan.FromSeconds(polling)
  let rec loop (nextTime:DateTime)= async {
    // Generate next value (on the GUI thread)
    do dataRetriever nextTime
      |> List.filter (fun item -> item.ID = ID)
      |> List.iter (fun item -> raiseEvent item)
    // Wait some short time
    do! Async.Sleep(1000)
    // Continue looping
    do! loop (nextTime.Add(interval)) }
    loop time |> Async.Start
    obs.Publish

I may well have not quite implemented this correctly. I'm obviously getting a list of prices or volumes filtering them and publishing them. In order to achieve the filtering I've created an interface, but I'm not really happy with this since I'd prefer to keep the DTOs as simple as possible. Intellisense is telling me that my dataRetriever function takes a DateTime and returns an 'a list, but I'm not sure I quite want that since I need to cast it to an IID interface for filtering and then back to the underlying DTO for publishing.

Is there a more 'functional' (or perhaps just better) way to do this?

This is a bit difficult to answer, because you did not share code sample that fully type-checks (and so it is hard to make sure the answer is correct). However, I think you're very close. When refactoring two similar functions, you just need to abstract out the bits that differ.

In your case, there are two places:

One is the function that is used to retrieve the data (either getPrices or getVolumes)
Second is the projection that is used when triggering the event (either item or item.Price)

If you take these two operations as functions, then you can write the following:

open System

type IID =
  abstract member ID : int64

let Publish (dataRetriever:TimeSpan -> list<#IID>) valueSelector time ID (polling:float) =
  let sync = System.Threading.SynchronizationContext.Current
  let obs = new Event<int>()
  let raiseEvent (value:int) = sync.Post((fun _ -> obs.Trigger(value)), null)
  let interval = TimeSpan.FromSeconds(polling)
  let rec loop nextTime= async {
    // Generate next value (on the GUI thread)
    dataRetriever nextTime
    |> List.filter (fun item -> item.ID = ID)
    |> List.iter (fun item -> raiseEvent (valueSelector item))
    // Wait some short time
    do! Async.Sleep(1000)
    // Continue looping
    do! loop (nextTime.Add(interval)) }
  loop time |> Async.Start
  obs.Publish

The type annotation TimeSpan -> list<#IID> is interesting. It says that the argument is a function that returns a list of some values that implement the IID interface - but it turns this type into a generic type argument that is then passed to the other function (valueSelector), so when you use the function, the compiler knows that the argument of valueSelector has the same type as the thing that you obtain from dataRetriever. The annotation allows you to write item.ID in the filtering function.

The two functions can now be defined as follows:

let Prices = Publish getPrices (fun price -> price.Price)
let Volumes = Publish getVolumes id


Grouping a list by their last element Trying to group elements in a list by the their last element. I think this is a good problem to use groupby() from itertools. Here is my solution:

from itertools import groupby

def match_ends(list1):
    """Returns a dictionary of groups of elements whose end elements match
       Returns empty dic if given list was empty
       Ex: list1 = ["abc", "dbc","aba","baa"]
       matched_ends(list1) = { 'c' : ['abc', 'dbc'], 'a' : ['aba', 'baa'] }
"""
    matched_ends = {}
    list1 = sorted(list1, key = lambda x: x[-1])
    for key, group in groupby(list1, lambda x: x[-1]):
         matched_ends[key] = list(group)
    return matched_ends

Was this a good approach? Am I missing any key points, or any errors I did not forsee that may yield garbage values instead of the program throwing an Error? Is there a quicker way to group elements based on certain criteria that I have yet to see?

docstring

Your original method is adequately docmented, but you can format your docstring also according to a general style. This SO answer covers some of the templates.

doctest

If you format your example in the correct way, you can use doctest to test it. This format is also recognised by Sphinx and other documentation tools,

alternative approach

Another approach would be to use a collections.defaultdict(list)

def match_ends(list1):
    result = defaultdict(list)
    for item in list1:
        result[item[-1]].append(item)
    return result

This way you don't have to sort the list first, but need to append to it. If you use python >3.6, the original method will also return a alphabetically sorted dict, while this method will be sorted along the occurrence in the original list.


Calculating harmonic sum The problem is that I have executed the program for the last hour and it hasn't returned a value till now for input 1022. How can I make the program a bit faster? How can I increase the efficiency of program? Is there a faster algorithm?

I have Intel Core2 Duo (2.93GHz).

#include <stdio.h>
int main() {
    unsigned long long n;
    double m;
    double sum=0;
    scanf("%llu", &n);
    while (n>0) {
        m=1.0/n;
        sum+=m;
        n--;
    }
    printf("%lf",sum);
    return(0);
}

To illustrate my comment about accuracy, I wrote a small test program:

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

int main(int argc, char **argv) {
    unsigned long long n;
    double sum = 0;
    float fsum = 0;
    long double lsum = 0;

    if (argc > 1)
        n = strtoull(argv[1], NULL, 0);
    else
        scanf("%llu", &n);

    while (n > 0) {
        /* computing using double arithmetic */
        double m = 1.0 / n;
        sum += m;
        /* computing using float arithmetic */
        float fm = (float)1 / n;
        fsum += fm;
        /* computing using long double arithmetic */
        long double lm = (long double)1 / n;
        lsum += lm;
        n--;
    }
    printf("long double: %Lf\n", lsum);
    printf("double: %f, delta=%Lg\n", sum, lsum - sum);
    printf("float: %f, delta=%f\n", fsum, sum - fsum);
    return 0;
}

The following tests give this output:

~/dev/stackoverflow > time ./t41 10000000
long double: 16.695311
double: 16.695311, delta=-1.12868e-13
float: 16.686031, delta=0.009280

real  0m0.257s
user  0m0.244s
sys   0m0.004s

~/dev/stackoverflow > time ./t41 100000000
long double: 18.997896
double: 18.997896, delta=4.51783e-13
float: 18.807919, delta=0.189978

real  0m2.585s
user  0m2.558s
sys   0m0.010s

~/dev/stackoverflow > time ./t41 1000000000
make: `t41' is up to date.
long double: 21.300482
double: 21.300482, delta=1.79655e-12
float: 18.807919, delta=2.492563

real  0m25.287s
user  0m25.116s
sys   0m0.073s

My machine is not very fast, but it would take 10^13 seconds to complete the
calculation for 10^22 using your code.  Given the estimated age of the universe,
5.10^17 seconds, you should kill the process and think about a better algorithm.

Looking at the accumulated error for just 10^9 iterations between long double and double,
computing in double for 10^13 more iterations would given useless results.


Treating a short like an uchar[2] I need to create a commandword to be send to hardware and I'd like to make is as efficient as possible (without going to Assembler or similar).
The command is 16Bit long and has to be passed as an array of two bytes to the hardware interfacing library I use (wiringpi). The first four Bit are configuration Bits and the other 12 are the actual command.

My code currently looks like this:

// global variables
short prefix_A = 3 << (4+8);
short prefix_B = 11 << (4+8);
unsigned char dac_cmd[2];
short cmd;

// not thread safe but that's ok in this application
void send_cmd(bool A, short new_cmd){
  assert( new_cmd < 4096);
  cmd = (A?prefix_A:prefix_B) | new_cmd;
  dac_cmd[0] = cmd >> 8;
  dac_cmd[1] = cmd & (255);
  wiringPiSPIDataRW(0, dac_cmd, 2);
}

I especially don't like the additional copy from the short (cmd) to the uchar[2] (dac_cmd).

Well, there's no need to copy the short into an array of (unsigned) char:
Treating any type as an array of (optionally signed/unsigned) char is an explicit exception from the strict-aliasing-rule.
(I assume your targets byte-order is big-endian for that. Or you will need to byte-swap it first.)
Unless prefix_A and/or prefix_B should ever be dynamically changed, mark them as const so you don't change them accidentally, and the compiler knows they cannot change so it will optimize accordingly.

So it becomes:

// constants
const short prefix_A = 3 << (4+8);
const short prefix_B = 11 << (4+8);

void send_cmd(bool A, short cmd){
    assert( cmd < 4096);
    cmd |= A ? prefix_A : prefix_B;
    cmd = htons(cmd); // byte-swap if little-endian
    wiringPiSPIDataRW(0, (unsigned char*)&cmd, 2);
}


Handle concurrent HTTP requests with a generic .ashx I have created a .ashx file that is able to handle concurrent http requests.

Works great, but now I'd like to create a base class that does the common work in order to not repeat myself each time I need to create a new .ashx of the same kind.

Here is my current code for the http handler:

public class BaseAsyncHandler : IHttpAsyncHandler, IRequiresSessionState
{
    public bool IsReusable
    {
        get { return false; }
    }

    public BaseAsyncHandler()
    {
    }

    public IAsyncResult BeginProcessRequest(HttpContext context, AsyncCallback cb, Object extraData)
    {
        BaseAsynchOperation asynch = new BaseAsynchOperation(cb, context, extraData);
        asynch.StartAsyncWork();
        return asynch;
    }

    public void EndProcessRequest(IAsyncResult result)
    {
    }

    public void ProcessRequest(HttpContext context)
    {
        throw new InvalidOperationException();
    }
}

public class BaseAsynchOperation : IRequiresSessionState, IAsyncResult
{
    private bool _completed;
    private Object _state;
    private AsyncCallback _callback;
    public HttpContext _httpContext;

    bool IAsyncResult.IsCompleted
    {
        get { return _completed; }
    }

    WaitHandle IAsyncResult.AsyncWaitHandle
    {
        get { return null; }
    }

    Object IAsyncResult.AsyncState
    {
        get { return _state; }
    }

    bool IAsyncResult.CompletedSynchronously
    {
        get { return false; }
    }

    public BaseAsynchOperation(AsyncCallback callback, HttpContext context, Object state)
    {
        _callback = callback;
        _httpContext = context;
        _state = state;
        _completed = false;
    }

    public void StartAsyncWork()
    {
        ThreadPool.QueueUserWorkItem(new WaitCallback(StartAsyncTask), null);
    }

    protected virtual void OnBeforeStartAsyncTask()
    {
    }

    public void StartAsyncTask(Object workItemState)
    {
        OnBeforeStartAsyncTask();

        _httpContext.Response.ContentType = "application/json";

        _completed = true;
        _callback(this);
    }
}

My new test that inherits from the base class:

public class TestAsyncHandler : BaseAsyncHandler
{
    public TestAsyncHandler()
    {

    }
}

public class TestAsyncOperation : BaseAsynchOperation
{
    public TestAsyncOperation(AsyncCallback callback, HttpContext context, object state) : base(callback, context, state)
    {
    }

    protected override void OnBeforeStartAsyncTask()
    {
        var test = _httpContext.Request["test"]; // code never ends up here...
    }
}

Since all my work is done in the StartAsyncTask() method (similar to ProcessRequest() in a regular generic handler .ashx), I thought that I add a method there which can be overridden. However, it never seems to get called.

What did I miss? Do you have any other suggestions on improvments?

A generic ASP.NET ASHX handler can handle concurrent HTTP requests perfectly fine out of the box. This is partially thanks to the hosting infrastructure and ASP.NET itself. There's no need for all of this. I'm saying this because I'm not sure this is what you really want.

But maybe you're referring to asynchronous handlers which means something else entirely, in which case I'd refer to this: http://msdn.microsoft.com/en-us/magazine/cc164128.aspx


Where will this .offsetTop() function fail? The Javascript libraries have pretty complicated functions to calculate an element's offset on the page. I wrote the following function that works in all the places I need it, and I've tested a few edge cases as well. It adds the element's offsetTop to its parent's offsetTop all the way up the DOM to the document. Can someone give me an example of where this wouldn't work?

This demo shows its results compared to jQuery's offset().top function.

Demo: http://jsfiddle.net/ThinkingStiff/5aGqM/

window.Object.defineProperty( Element.prototype, 'documentOffsetTop', {
    get: function () {
        return this.offsetTop + ( this.offsetParent ? this.offsetParent.documentOffsetTop : 0 );
    }
} );

UPDATE:
I was looking for places where traversing up the DOM and adding the offset doesn't work in current browsers. As @Akkuma pointed out, Object.defineProperty doesn't work in earlier IEs.

For starters, this doesn't work in IE7 or IE8. I'm assuming you knew that, although you never mentioned compatibility if any you were aiming for.

I rewrote what you made, to work in IE7 & IE8, so you can see where it fails. It is available at http://jsfiddle.net/Akkuma/Xem3B/

You'll find the following not working correctly in IE8 compared to the latest Chrome Dev 18 static, static (static wrapped), fixed/absolute (side), absolute/relative/static (absolute/relative wrapped), relative, and fixed/absolute (bottom).

In jQuery they do several different compatibility checks for whether a browser adds the border, for relative/static/fixed positioning, whether the browser subtracts the borders for overflows that are something other than visible. It appears modern browsers should support your approach just fine.

Your code appears to work fine in IE9, so if you aren't supporting IE8 & 7 your code should be good to go. However, if you take a look at jQuery's code you'll find they do not have to recursively traverse up the tree to calculate the offset in modern browsers that support getBoundingClientRect. The trade off is obviously one of simplicity (your own code) vs performance. I haven't verified which is faster, but I would expect jQuery's approach to keep getting faster the deeper the element is in the tree compared to your documentOffsetTop.

Update: If you haven't seen it, here is an older explanation from John Resig on it and what additional things they had to do (note this is from 2008, so the links are broken and I'm sure they may have modified it more) http://ejohn.org/blog/getboundingclientrect-is-awesome/. Additionally, here is the Mozilla documentation detailing how to properly use it cross-browser.


Adding, subtracting, and multiplying a vector by a scalar Exercise 2.46.

  A two-dimensional
  vector v running from the origin to a
  point can be represented as a pair
  consisting of an x-coordinate and a
  y-coordinate. Implement a data
  abstraction for vectors by giving a
  constructor make-vect and
  corresponding selectors xcor-vect and
  ycor-vect. In terms of your selectors
  and constructor, implement procedures
  add-vect, sub-vect, and scale-vect
  that perform the operations vector
  addition, vector subtraction, and
  multiplying a vector by a scalar:

  $$ (r_1, y_1) + (r_2, y_2) = (r_1 + r_2, y_1 + y_2) \\ (r_1, y_1) - (r_2, y_2) = (r_1 - r_2, y_1 - y_2) \\ s \cdot (r, y) = (sr, sy) $$

I wrote the following:

(define (make-vect xcor ycor) (cons xcor ycor))
(define xcor-vect car)
(define ycor-vect cdr)

(define (add-vect v1 v2) (make-vect (+ (xcor-vect v1) (xcor-vect v2))
                                    (+ (ycor-vect v1) (ycor-vect v2))))
(define (sub-vect v1 v2) (make-vect (- (xcor-vect v1) (xcor-vect v2))
                                    (- (ycor-vect v1) (ycor-vect v2))))
(define (scale-vect v1 s) (make-vect (* (xcor-vect v1) s)
                                     (* (ycor-vect v1) s)))

Can it be improved?

No.

If this would be production code, you would be better off by

  (define (sub-vect v1 v2) (add-vect v1 (scale-vect v2 -1)))

However, this is not what the exercise asks for, as everything should be written "in terms of constructors and selectors".


Do I unit test the correct thing Thats my method to test:

   public LessonplannerRequest GetWeeklyLessonplanner(int schoolyearId)
        {
            Schoolyear schoolyear = _unitOfWork.SchoolyearRepository.GetById(schoolyearId);
            DayOfWeek firstDayOfWeek = schoolyear.FirstDayOfWeek;

            DateTime currentDate = DateTime.Now.Date;
            DateTime startDateOfWeek = _dateService.GetFirstDateOfWeek(currentDate, firstDayOfWeek);
            DateTime endDateOfWeek = _dateService.GetLastDateOfWeek(currentDate, firstDayOfWeek);

            var periods = _unitOfWork.PeriodRepository.GetWeeklyPeriods(schoolyearId, startDateOfWeek, endDateOfWeek );
            var weeks = _dateService.GetAllWeekStartingDays(startDateOfWeek, endDateOfWeek, firstDayOfWeek);
            var request = new LessonplannerRequest
            {
                 Periods = periods,
                 Weeks = weeks,
            };

            return request;
        }

Thats my unit test:

 // Arrange
            Mock<IUnitOfWork> unitOfWorkMock = new Mock<IUnitOfWork>();
            Mock<IDateService> dateServiceMock = new Mock<IDateService>();
            Mock<TLPContext> contextMock = new Mock<TLPContext>();
            Mock<IGenericRepository<Schoolyear>> schoolyearRepositoryMock = new Mock<IGenericRepository<Schoolyear>>();
            Mock<ILessonplannerRepository> lessonplannerRepoMock = new Mock<ILessonplannerRepository>();

            int schoolyearId = 1;
            DateTime start = new DateTime(2010, 1, 1);
            DateTime end = new DateTime(2010, 1, 2);

            unitOfWorkMock.Setup(s => s.SchoolyearRepository).Returns(schoolyearRepositoryMock.Object);
            unitOfWorkMock.Setup(s => s.PeriodRepository).Returns(lessonplannerRepoMock.Object);
            dateServiceMock.Setup(s => s.GetFirstDateOfWeek(DateTime.Now.Date, DayOfWeek.Sunday)).Returns(start);
            dateServiceMock.Setup(s => s.GetLastDateOfWeek(DateTime.Now.Date, DayOfWeek.Sunday)).Returns(end);
            schoolyearRepositoryMock.Setup(s => s.GetById(schoolyearId)).Returns(new Schoolyear() { FirstDayOfWeek = DayOfWeek.Sunday });

            ILessonplannerService service = new LessonplannerService(unitOfWorkMock.Object, dateServiceMock.Object);

            // Act
            LessonplannerRequest request = service.GetWeeklyLessonplanner(schoolyearId);

            // Assert
            dateServiceMock.Verify(d => d.GetFirstDateOfWeek(DateTime.Now.Date, DayOfWeek.Sunday));
            dateServiceMock.Verify(d => d.GetLastDateOfWeek(DateTime.Now.Date, DayOfWeek.Sunday));
            lessonplannerRepoMock.Verify(d => d.GetWeeklyPeriods(schoolyearId, start, end));

            Assert.That(request.Periods, Is.Empty);
            Assert.That(request.Weeks, Is.Empty);

In my method to test all calls to a repository or dateService are already unit tested itself thus I am mocking them not to have an integration test...

I ask myself wether my assertion for empty lists make sense. And If not what would you test. Actually in this method everything is already tested just the creation of the LessonplannerRequest is not tested...

Almost everything is perfect. Some notes:

Do use var to avoid repeating in cases like Mock<IUnitOfWork> unitOfWorkMock = new Mock<IUnitOfWork>()
Move mocks initialisation and wiring up to SetUp method to avoid copy-pasting in different test methods, and leave only actual setups that provide test data in test method
It's hard to unit test the code that uses DateTime.Now. For example your test may fail when running near midnight (since you call DateTime.Now several times, and subsequent calls may return the next day). I would suggest to add the UtcNow property to your IDateService and always use it instead of DateTime.Now (I prefer to force using UTC time since it forces you to think about time zones). Then, most likely you won't need methods like GetFirstDateOfWeek in IDateService (if they depend on inputs only), they could be just extension methods to DateTime.
You don't need to verify that GetFirstDateOfWeek and GetLastDateOfWeek were called, just make sure that result matches expected value. The reason is because the call to those methods does not guarantee that returned values were used correctly. And when all tests pass without those calls - it only shows that you should add more tests to verify that returned values were used properly

As a result you'll have something like this:

[Test]
public void WhenWeekStartOnSunday...ShouldReturnSomething
{
    const int schoolyearId = 1;
    var currentDate = new DateTime(2010, 1, 1).ToUniversalTime();
    var periods = new [] {...}; //specify test periods here

    //Arrange
    _schoolyearRepositoryMock.Setup(s => s.GetById(schoolyearId)).Returns(new Schoolyear() { FirstDayOfWeek = DayOfWeek.Sunday });
    _dateServiceMock.Setup(s => s.UtcNow).Returns(currentDate);
    _lessonplannerRepoMock.Setup(lp => lp.GetWeeklyPeriods(schoolyearId, new DateTime(2010, 1, 1),
            new DateTime(2010, 1, 5))) //specify the dates expected according to "currentDate" value.
        .Returns(periods);

    var service = new LessonplannerService(_unitOfWorkMock.Object, _dateServiceMock.Object);

    // Act
    LessonplannerRequest request = service.GetWeeklyLessonplanner(schoolyearId);

    // Assert
    Assert.That(request.Periods, Is.SameAs(periods));
    Assert.That(request.Weeks, Is.Empty); //verify here expected list of weeks according to logic of the method
}


Ensuring file handle is closed in case of an exception fun readLines(path: String): Stream<String> =
    try {
        Files.lines(Paths.get(path)).use { it }
    } catch (ex: Exception) {
        ex.printStackTrace()
        Stream.empty<String>()
    }

I added .use { it } to tack on try-with-resources like behaviour for autoclosing the file handle (?) in case of exceptions.

Is this required?
Are there better approaches?

Your situation
Use closes the resource at the end of the block (lambda).
This means that the stream is closed before returning it.
The problem with reading lines
The file needs to be closed once you have read the needed lines.
The stream could close the file once the stream last item is reached.
The problem with this approach is that functions like limit, take, splititerator etc. are making it possible that the last item is never reached.
Therefor, you can do two things.
The first option is to let the caller close the file or sequence/stream/whatever you return.
The second option is to close inside your function, but then you need to do all the reading inside the function, so before you return from the function.
Options
You can read everything immediately by storing everything in a list using readlines.
(This means you read the whole file into memory, which means you should not do this for huge, huge, huge files, you almost always can ignore this warning).
fun readLines(path: String): List<String> = try{
    Paths.get(path).toFile().readLines()
} catch(ex: Exception) {
    ex.printStackTrace()
    emptyList<String>()
}

The other approaches work by moving the code you would normally do outside the function inside the function.
For example the forEachLine:
fun forEachLine(path: String, action: (String)->Unit){
    try{
        Paths.get(path).toFile().forEachLine(action)
    } catch(ex: Exception) {
        ex.printStackTrace()
    }
}

With that function you could rewrite
val t = readLines("somePath")
    .filter{ it.startsWith("ok:") }
t.forEach{ println(it) }
t.close()

with
forEachLine("somePath"){
    if(it.startsWith("ok:")
        println(it)
}

If you have functions that operates on a sequence of strings, or want more than a simple forEach, you can access the sequence itself by using  useLines, but again move the reading inside the function:
fun useLines(path: String, action: (Sequence<String>)->Unit){
    try{
        Paths.get(path).toFile().useLines(action)
    } catch(ex: Exception) {
        ex.printStackTrace()
    }
}

Can replace the former example with:
useLines("somePath"){ lines->
    lines.filter{ it.startsWith("ok") }
       .forEach{ println("it") }
}

NOTE: this function is not for extracting or returning the sequence.
all the functions needs to be done in the lambda,until you come across a terminal operation, an operation that no longer returns a sequence.
NOTE: sequence is the Kotlin-version of streams with lots of benefits.
Is closing required
from the javadoc

"...Generally, only streams whose source is an IO channel (such as those returned by Files.lines(Path, Charset)) will require closing ..."

so yes.
huge files
You should not use readlines on huge files.
What is a huge file?
The bible has around 31,000 lines which can have 20 bytes reference, so 620,000 bytes or roughly 0,5 mb.
The bible has 3,000,000 characters which is roughly 12 mb.
Acording to this post, Android has a memory limit from around 15mb (in the worst case).
This means that reading the bible in it entirely would theoretically be OK.
So, untill you run into problems, this is nothing to worry about.
Edit:
I don't completely know how flatmap in Java excactly works at the moment.
There was a bug that flatmap was not lazy.
This means that calling fileNames.flatMap(::readLines) with the readLines from your code was calculating the complete list before moving on: link.
This bug is fixed, so I don't know what it does at the moment.
If it still works the same way, it always closes the file for you, but it does so because it always parses the complete file.
In every case, if you are returning the Java-stream, calling close on the returning Stream is enough to close the file.
With sequences, this is not possible, as sequences are not closeable (They also don't know about AutoCloseable/Closeable).


Pulling from data from SQLite DB on Android Apart from merging the 2 get methods, what else can I do to optimise this?
Both input and output cannot be changed. Is there a way to iterate a map while writing to it?

// String condition - "cust" for customer group, "item" for itemGroup
public HashMap<String, ArrayList<PriceTable>> getPromoForViewGroup(
            String condition) {
    String custGroup = "custType";
    String itemGroup = "itemType";
    HashMap<String, ArrayList<PriceTable>> valueMap = new HashMap<String, ArrayList<PriceTable>>();

    if (condition.equals(CUST)) {
        getPromoForViewGroupHelperCust(custGroup, condition, valueMap);
    }

    if (condition.equals(ITEM)) {
        getPromoForViewGroupHelperItem(itemGroup, condition, valueMap);

    }

    return valueMap;
}

private Map<String, ArrayList<PriceTable>> getPromoForViewGroupHelperItem(
            String searchGroup, String condition,
            Map<String, ArrayList<PriceTable>> valueMap) {

    List<PriceTable> allList = new ArrayList<PriceTable>();

    Cursor cursor = database.rawQuery("Select * from PriceTable where "
            + searchGroup + " IS NOT NULL", null);
    cursor.moveToFirst();
    while (!cursor.isAfterLast()) {
        PriceTable pt = cursorToPriceTable(cursor);
        if (!pt.getItemType().equals(""))
            allList.add(pt);

        cursor.moveToNext();

    }

    for (PriceTable pt1 : allList) {
        ArrayList<PriceTable> set = valueMap.get(pt1.getItemType());
        if (set == null) {
            set = new ArrayList<PriceTable>();
            set.add(pt1);
        } else {
            set.add(pt1);
        }
        valueMap.put(pt1.getItemType(), set);

    }
    cursor.close();

    return valueMap;

}

private Map<String, ArrayList<PriceTable>> getPromoForViewGroupHelperCust(
        String searchGroup, String condition,
        Map<String, ArrayList<PriceTable>> valueMap) {

    List<PriceTable> allList = new ArrayList<PriceTable>();

    Cursor cursor = database.rawQuery("Select * from PriceTable where "
            + searchGroup + " IS NOT NULL", null);
    cursor.moveToFirst();
    while (!cursor.isAfterLast()) {
        PriceTable pt = cursorToPriceTable(cursor);
        if (!pt.getCustType().equals(""))
            allList.add(pt);

        cursor.moveToNext();

    }

    for (PriceTable pt1 : allList) {
        ArrayList<PriceTable> set = valueMap.get(pt1.getCustType());
        if (set == null) {
            set = new ArrayList<PriceTable>();
            set.add(pt1);
        } else {
            set.add(pt1);
        }
        valueMap.put(pt1.getCustType(), set);

    }
    cursor.close();
    return valueMap;

}

private PriceTable cursorToPriceTable(Cursor cursor) {
        PriceTable priceTable = new PriceTable();

        priceTable.setPromoNum(cursor.getString(0));
        priceTable.setPromoType(cursor.getString(1));
        priceTable.setPromoRank(cursor.getString(2));
        priceTable.setPromoName(cursor.getString(3));
        priceTable.setItemType(cursor.getString(4));
        priceTable.setItemCode(cursor.getString(5));
        priceTable.setCustType(cursor.getString(6));
        priceTable.setCustCode(cursor.getString(7));
        priceTable.setStartDate(cursor.getString(8));
        priceTable.setEndDate(cursor.getString(9));
        priceTable.setPercent(cursor.getString(10));
        priceTable.setSalesPrice(cursor.getString(11));

        return priceTable;
    }

Updating collections

Many things can be simplified around this snippet:

for (PriceTable pt1 : allList) {
    ArrayList<PriceTable> set = valueMap.get(pt1.getItemType());
    if (set == null) {
        set = new ArrayList<PriceTable>();
        set.add(pt1);
    } else {
        set.add(pt1);
    }
    valueMap.put(pt1.getItemType(), set);

}
cursor.close();

This is equivalent:

for (PriceTable pt1 : allList) {
    String itemType = pt1.getItemType();
    ArrayList<PriceTable> set = valueMap.get(itemType);
    if (set == null) {
        set = new ArrayList<PriceTable>();
        valueMap.put(itemType, set);
    }
    set.add(pt1);
}

What I did:

Moved set.add(pt1); out of the if-else
Moved valueMap.put(itemType, set); inside the if
No need for an else
Extracted pt1.getItemType() to avoid an extra lookup
cursor.close(); doesn't belong here, it should be done before this snippet. It makes the code easier to read if you don't delay actions like this.

Use interfaces types instead of implementations

When the interface type is enough, you should use that instead of the specific implementation type. For example instead of this:

public HashMap<String, ArrayList<PriceTable>> getPromoForViewGroup(
            String condition) {
    String custGroup = "custType";
    String itemGroup = "itemType";
    HashMap<String, ArrayList<PriceTable>> valueMap = new HashMap<String, ArrayList<PriceTable>>();

This is recommended:

public Map<String, List<PriceTable>> getPromoForViewGroup(String condition) {
    String custGroup = "custType";
    String itemGroup = "itemType";
    Map<String, List<PriceTable>> valueMap = new HashMap<String, List<PriceTable>>();

Notice all the rewritten types.

Mutually exclusive if-else

This conditions will not be both true at the same time,
so instead of two separate if, they should be joined with an else if:

if (condition.equals(CUST)) {
    getPromoForViewGroupHelperCust(custGroup, condition, valueMap);
}

if (condition.equals(ITEM)) {
    getPromoForViewGroupHelperItem(itemGroup, condition, valueMap);

}

Don't Select *

It's not recommended to select columns using * wildcard.
Different versions of the database might have columns in different order than you expect. There might be more columns than you expected, selecting more data than necessary, causing slowness. It's recommended to name the columns you need explicitly, and access their values by name.

Misc

  Apart from merging the 2 get methods, what else can I do to optimise this?

Oh definitely merge those methods...

  Is there a way to iterate a map while writing to it?

Kind of. You can iterate over a copy, while writing to the original. For example:

Map<String, Integer> map = new HashMap<>();
map.put("hello", 2);
map.put("world", 7);
for (Map.Entry<String, Integer> entry : new HashMap<>(map).entrySet()) {
    if (entry.getValue() > 5) {
        map.remove(entry.getKey());
    }
}
System.out.println(map);

Prints {hello=2}.


UICollectionView image gallery app I built an app that allows users to add and remove images in a collection view. I want this to be useful online so I save the images into a database and retrieve them every time. I'm looking for constructive criticism on how I solved this problem. It is in swift. The code works just fine, but I notice some lag sometimes, and it may be because of the way I designed the software. This is why I'm looking for criticism.

ViewGallery.swift

import UIKit
import Firebase
import DKImagePickerController

class ViewGallery: UIViewController, UICollectionViewDataSource, UICollectionViewDelegate, UICollectionViewDelegateFlowLayout {

let db = Firebase(url: "mydatabaseurl")
@IBOutlet var collectionView: UICollectionView!

 var imageDatas: [String] = []
 var selectedItems: [NSIndexPath] = []
 var keys: [AnyObject] = []

 override func viewDidLoad() {
  super.viewDidLoad()
  db.childByAppendingPath("Images").observeEventType(.Value, withBlock: {
    snapshot in
    let images = snapshot.value as! NSDictionary
    self.keys = images.allKeys
    self.imageDatas = []
    for key in self.keys {
      self.imageDatas.append(images[key as! NSString as String]!["image"] as! String)
    }
    self.selectedItems = []
    self.collectionView.reloadData()
  })
}

@IBAction func addPictures(sender: AnyObject) {
  let pickerController = DKImagePickerController()

  pickerController.didSelectAssets = { [unowned self] (assets: [DKAsset]) in
    print("didSelectAssets")
    for asset in assets {
      let imageData = asset.rawData?.base64EncodedStringWithOptions(.Encoding64CharacterLineLength)
      self.saveImageStringToDatabase(imageData!)
    }
  }

  self.presentViewController(pickerController, animated: true, completion: nil)
}

@IBAction func removePicture(sender: AnyObject) {
  for indexPath in selectedItems {
    db.childByAppendingPath("Images").childByAppendingPath(self.keys[indexPath.item] as! String).removeValue()
  }
}

func saveImageStringToDatabase(imageString: String) {
  let value = ["image": imageString]
  db.childByAppendingPath("Images").childByAutoId().setValue(value)
}

func collectionView(collectionView: UICollectionView, numberOfItemsInSection section: Int) -> Int {
  if imageDatas.count == 0 {
    return 0
  } else {
    return imageDatas.count
  }
}

func collectionView(collectionView: UICollectionView, cellForItemAtIndexPath indexPath: NSIndexPath) -> UICollectionViewCell {
  if imageDatas.count == 0 {
    let cell = collectionView.dequeueReusableCellWithReuseIdentifier("View Gallery Cell", forIndexPath: indexPath)
    return cell
  } else {
    let base64String = self.imageDatas[indexPath.item]
    let imageData = NSData(base64EncodedString: base64String, options: .IgnoreUnknownCharacters)
    let decodedImage = UIImage(data: imageData!)
    let cell = collectionView.dequeueReusableCellWithReuseIdentifier("View Gallery Cell", forIndexPath: indexPath) as! ImageCell
    cell.imageView.image = decodedImage
    return cell
  }
}

func collectionView(collectionView: UICollectionView, didSelectItemAtIndexPath indexPath: NSIndexPath) {
  let cell = self.collectionView.cellForItemAtIndexPath(indexPath) as! ImageCell
  if selectedItems.contains(indexPath) {
    print("unselected")
    cell.backgroundColor = UIColor.clearColor()
    let indexToBeDeleted = selectedItems.indexOf(indexPath)
    selectedItems.removeAtIndex(indexToBeDeleted!)
  } else {
    print("selected")
    cell.backgroundColor = UIColor.redColor()
    selectedItems.append(indexPath)
  }
}

}

I am not familiar with the Firebase or DKImagePickerController framework, so I cannot say anything
about that. But there are some things that I noticed:

There is no need in Swift to reference properties via self
(unless there is an ambiguity with a local variable, e.g. in an init
method). And you are not consistent here, e.g. both self.selectedItems
and selectedItems is used.

This

func collectionView(collectionView: UICollectionView, numberOfItemsInSection section: Int) -> Int {
    if imageDatas.count == 0 {
        return 0
    } else {
        return imageDatas.count
    }
}

can be simplified to

func collectionView(collectionView: UICollectionView, numberOfItemsInSection section: Int) -> Int {
    return imageDatas.count
}

And in

func collectionView(collectionView: UICollectionView, cellForItemAtIndexPath indexPath: NSIndexPath) -> UICollectionViewCell

there is no need to check for imageDatas.count == 0 (and return a dummy cell in that case). If the array
is empty then numberOfItemsInSection returns zero, i.e. the
collection view displays no item at all and cellForItemAtIndexPath
will not be called.

If the order of the selected items is not relevant then you can
use a set instead of an array:

var selectedItems = Set<NSIndexPath>()

Adding or removing an index path then simplifies slightly to

if selectedItems.contains(indexPath) {
    cell.backgroundColor = UIColor.clearColor()
    selectedItems.remove(indexPath)
} else {
    cell.backgroundColor = UIColor.redColor()
    selectedItems.insert(indexPath)
}

A loop like

self.imageDatas = []
for key in self.keys {
    self.imageDatas.append(...)
}

can be replaced by a mapping:

self.imageDatas = self.keys.map { key in ... }


Performing tasks concurrently I made a class to perform tasks concurrently and have implemented it. From appearances and testing, everything seems to work. How can I improve this? I understand this is somewhat a broad question, so please let me know how I should narrow it down.

Here is the pseudocode:

The general idea is a single Starter spawns multiple TaskWorker to do the work. I want to start the loading process fairly early on and then block EDT until the completion of all TaskWorker.

class ImproveMe {
  private AtomicInteger incompleteTasksCount = new AtomicInteger( -1 );
  /* SwingWorker has a limitation of 10 */
  private static final int MAX_CONCURRENT_THREAD = 20;
  private static ExecutorService threadPool = Executors.newFixedThreadPool( MAX_CONCURRENT_THREAD );

  /* This just starts multiple TaskWorker */
  class Starter extends SwingWorker {
    Object doInBackground() {
      incompleteTasksCount.getAndSet( -1 );

      Tasks[] taskList = getTasks();
      for ( Task task : taskList ) {
        /* Track incomplete tasks */
        if ( incompleteTasksCount.compareAndSet( -1, 1 ) {
          incompleteTasksCount.incrementAndget();
        }
        threadPool.submit( new TaskWorker( task ) );
      }
      threadPool.shutdown();
    }
  }

  /* Performs the actual work -- I don't care about the results since it will be cached elsewhere */
  class TaskWorker implements Runnable {
    Task t;
    TaskWorker( Task t ) {
      this.t = t;
    }

    void run() {
      if ( t != null ) {
        do stuff with 't'
      }

      synchronized( incompleteTasksCount ) {
        if ( incompleteTasksCount.decrementAndGet() == 0 ) {
          /* everything is done! */
          incompleteTasksCount.notifyAll();
        }
      }
    }
  }

  /* User will call this to start the loading */
  public void startLoading() {
    if ( !threadPool.isShutdown() ) {
      Starter s = new Starter();
      s.execute();
    }
  }

  /* User will call this to ensure that everything has complete before continuing */
  public void blockUntilDone() {
    boolean cont = true;
    if ( incompleteTasksCount.get() == 0 || threadPool.isTerminated() ) {
      return;
    } else if ( incompleteTasksCount.get() == -1 ) {
      /* not loaded/started */
      return;
    }

    while ( cont ) {
      synchronized( incompleteTasksCount ) {
        try {
          incompleteTasksCount.wait();
        } catch ( InterruptedException e ) {
          e.printStackTrace();
          cont = false;
        } finally {
          if ( incompleteTasksCount.get() == 0 ) {
            cont = false;
          }
        }
      }
    }
  }
}

In reality, TaskWorker calls PrintService.getSupportedAttributeValues(..) because of the numerous printers and Java caches that so any future calls will return fairly quickly as compared to the first time (some of the network printers took 40+ seconds). I call ImproveMe.startLoading() somewhere fairly early in my program then before the actual program starts (e.g. GUI is shown), I use ImproveMe.blockUntilDone() to ensure everything is loaded.

SwingWorker comes with all the control features you try to emulate here. It already uses an ExecutorService under the hood. I'm not sure why you want to increase the maximum pool size, but given that the code does not compile as given, makes me suspect you did not decide this, based on performance measuring.

So here's what I did :

Launching the workers is lightweight enough to not need
to be run in a separate thread, so I inlined Starter.
Keeping tabs on task progression is done through the Future that you get from
an ExecutorService. SwingWorker exposes this by implementing Future
itself. Waiting until a SwingWorker is done is as simple as calling its
get() method, which will block until the task is done.
You don't need the low level constructs wait() and notifyAll().
Properly handle InterruptedException : put it in your throws clause, or if you catch
it, reset the Thread's interrupted flag by calling
Thread.currentThread().interrupt(); so clients of your code know the thread has been
interrupted, and they can do their own cleanup.
SwingWorker uses generics to know the type of the result. Typically when no result is
given, Void is chosen as a return type. (this makes a comment about not caring about
the return value superfluous).
I've spent attention to the visibility modifiers, tightening it wherever possible,
exposing only as much as is strictly needed.
Future.get() throws an ExecutionException if the underlying task execution
threw an exception. Your code had no means of dealing with those.

Here's my refactoring of your code :

public class ImproveMe {
    private final List<SwingWorker> workers;

    public ImproveMe(Task[] tasks) {
        workers = new ArrayList<>(tasks.length);
        for (Task task : tasks) {
            workers.add(new TaskWorker(task));
        }
    }

    /* User will call this to start the loading */
    public void startLoading() {
        for (SwingWorker worker : workers) {
            worker.execute();
        }
    }

    /* User will call this to ensure that everything has complete before continuing */
    public void blockUntilDone() throws InterruptedException {
        for (SwingWorker worker : workers) {
            waitForWorker(worker);
        }
    }

    private void waitForWorker(SwingWorker worker) throws InterruptedException {
        try {
            worker.get(); // will block until the worker's task is done
        } catch (ExecutionException e) {
            throw new RuntimeException(e); // more appropriate exception handling here
        }
    }

    private static class TaskWorker extends SwingWorker<Void, Void> {
        private final Task t;

        TaskWorker( Task t ) {
            this.t = t;
        }

        public Void doInBackground() {
            if ( t != null ) {
                //do stuff with 't'
            }
            return null;
        }
    }
}


A function that checks if a word is similar to another word and returns a boolean This function will check a word with some letters missing, missing letters will be indicated by an "_ " (notice that there is a space after the underscore).
The word with missing letters needs to be checked against a full word and if both words are found to be identical ( disregarding the "_ " the function should return True otherwise False.

This is part of a problem set from the MIT Online Course.
here is how i managed to do this:

def match_with_gaps(my_word, other_word):
    '''
    my_word: string with "_" characters
    other_word: string, regular English word
    returns: boolean, True if all the actual letters of my_word match the
    corresponding letters of other_word, or the letter is the special symbol "_",
    and my_word and other_word are of the same length;
    False otherwise:
    '''
    my_word = my_word.replace(' ','')
    for char in my_word:
        if char.isalpha():
            if my_word.count(char) != other_word.count(char):
                return False
    return len(my_word) == len(other_word)

An example :

a = match_with_gaps('a p p _ e', 'apple')
b = match_with_gaps('a p p _ c', 'apple')
print(a) >> outputs : True
print(b) >> outputs : False

I also came up with another way:

def match_with_gaps(my_word, other_word):
    '''
    my_word: string with _ characters, current guess of secret word
    other_word: string, regular English word
    returns: boolean, True if all the actual letters of my_word match the
        corresponding letters of other_word, or the letter is the special symbol
        _ , and my_word and other_word are of the same length;
        False otherwise:
    '''
    my_word = my_word.replace(' ', '')
    if len(my_word) != len(other_word):
        return False
    else:
        for i in range(len(my_word)):
            if my_word[i] != '_' and (
                my_word[i] != other_word[i] \
                or my_word.count(my_word[i]) != other_word.count(my_word[i]) \
            ):
                return False
        return True

which will give the same output but using a different method.

Which way is more efficient / will give more accurate results? and is there a third better method?
Please give reason to your answer.
And thanks in advance.

Your 2nd approach is better that 1st as it adds length comparison, but

it has redundant else: branch. The 1st if condition, if positive, has unambiguous statement return False terminating function call
my_word.count(my_word[i]), other_word.count(my_word[i] calls will traverse sequence of chars (word) on each loop iteration for equal characters (trying to "run ahead")

Consider the following approach with builtin any function (returns True if any element of the iterable is true):

def match_with_gaps(my_word, full_word):
    '''
    my_word: string with _ characters, current guess of secret word
    full_word: string, regular English word
    returns: boolean, True if all the actual letters of my_word match the
        corresponding letters of full_word, or the letter is the special symbol
        _ , and my_word and full_word are of the same length;
        False otherwise
    '''
    my_word = my_word.replace(' ', '')

    # check for length OR respective characters mismatch
    if len(my_word) != len(full_word) \
            or any(c1 != '_' and c1 != c2 for c1, c2 in zip(my_word, full_word)):
        return False

    return True

print(match_with_gaps('a p p _ e', 'apple'))   # True
print(match_with_gaps('a p p _ c', 'apple'))   # False
print(match_with_gaps('a _ _ _ _', 'apple'))   # True


Multi producer/consumers lockfree list As per a similar question, following a lockfree list implementation. Note that this list has a pre-defined maximum set of elements which can be inserted (N argument in the template declaration). Please let me know if I've missed anything.

// Lockfree FIFO list, with N pre-allocated slots
template<typename T, size_t N = 128>
class list {
    typedef union {
        struct {
            T   *ptr;
            size_t  cnt;
        } s;
        __int128    intv;
    } ptrcnt;

    volatile size_t _head,
            _tail;
    ptrcnt      _ptrbuf[N];
public:
    // Initialise elements' cnt with a non-equal seed
    list() {
        _head = _tail = 0;
        for(size_t i = 0; i < N; ++i) {
            _ptrbuf[i].s.ptr = 0;
            _ptrbuf[i].s.cnt = i*100 + i*3 + i;
        }
    }

    bool push(const T& in) {
        std::auto_ptr<T>    v(new T(in));
        while(true) {
            // first, prepare pointer to use next
            ptrcnt      cur_ptr = _ptrbuf[_tail%N],
                    next_ptr = cur_ptr;
            cur_ptr.s.ptr = 0;
            next_ptr.s.ptr = v.get();
            ++next_ptr.s.cnt;
            if(_tail - _head  >= N) {
                break;
            }
            // add new element
            if(!__sync_bool_compare_and_swap(&_ptrbuf[_tail%N].intv, cur_ptr.intv, next_ptr.intv)) {
                continue;
            }
            // if we're after this point, no other thread should have
            // accessd the slot, set the tail
            __sync_fetch_and_add(&_tail, 1);
            // all done, return true
            v.release();
            return true;
        }
        return false;
    }

    bool pop(T& out) {
        while(true) {
            // get current pointer, then swap
            ptrcnt      lcl_ptr = _ptrbuf[_head%N],
                    next_ptr = lcl_ptr;
            next_ptr.s.ptr = 0;
            ++next_ptr.s.cnt;
            if(!lcl_ptr.s.ptr || _head == _tail) {
                return false;
            }
            // first, swap pointer to use next
            if(!__sync_bool_compare_and_swap(&_ptrbuf[_head%N].intv, lcl_ptr.intv, next_ptr.intv)) {
                continue;
            }
            // then increment the head; no other thread should access 'head'
            // while performing this increment
            __sync_fetch_and_add(&_head, 1);
            // return the value
            out = *lcl_ptr.s.ptr;
            delete lcl_ptr.s.ptr;
            return true;
        }
        return false;
    }
};

Problem with pop
Your pop() function could return false when there are still elements in the list.  The problem is with this check:

      if(!lcl_ptr.s.ptr || _head == _tail) {
          return false;
      }

Here, lcl_ptr.s.ptr could be NULL if some other thread just popped that element.  If that happens, you should just move on to the next element instead of returning false.  So for example:
      if (_head == _tail) {
          return false;
      }
      if (!lcl_ptr.s.ptr) {
          continue;
      }

This solution spins until the other thread finishes incrementing the head pointer.  There may be better ways of handling this case which involve searching forward for an element to pop.


Given a vector and a target sum, returns zero-based indices of any two distinct elements whose sum is equal to the target sum I am trying to solve 4. Two Sum from this set of C++ interview questions. As far as I can think this solution has a time complexity of \$O(n \log n)\$ can it be further optimized?

  Write a function that, given a vector and a target sum, returns
  zero-based indices of any two distinct elements whose sum is equal to
  the target sum. If there are no such elements, the function should
  return (-1, -1).

  For example, findTwoSum({ 1, 3, 5, 7, 9 }, 12) should return a
  std::pair<int, int> containing any of the following pairs of indices:

  1 and 4 (3 + 9 = 12)
  2 and 3 (5 + 7 = 12)
  3 and 2 (7 + 5 = 12)
  4 and 1 (9 + 3 = 12)

Here is my approach  considering that negative numbers cannot be present in vector

static std::pair<int, int> findTwoSum(const std::vector<int>& list, int sum)
{
    std::vector<int> com;
    std::vector<int>::iterator it;
    for(unsigned count=0; count<list.size(); count++){
        it = std::find(com.begin(), com.end(), list[count]);
        if ( it != com.end() ){
            return std::make_pair(it - com.begin() + 1, --count);
        }
        else{
            if(sum >= list[count])
            com.push_back(sum-list[count]);
        }

    }
    return std::make_pair(-1,-1);
}

Normally, we'd use the vector's index type (std::vector<int>::size_type, i.e. std::size_t) for the return values.  But unfortunately we're required to return negative values when the search fails, so I'd recommend a pair of std::ptrdiff_t instead.  And the question specifically asks for ints, so I'd just insert a comment explaining that were limited to arrays of up to INT_MAX elements (the O(n²) scaling probably reduces the practical range, anyway).

I don't see anywhere that says there can't be negative numbers present - if that's specified, then it would have been wise to quote that part.  As it is, you've introduced a bug - because we're not storing numbers larger than sum into com, the index calculation it - com.begin() + 1 will be incorrect by the amount of omitted large numbers (also, where does the +1 come from? - did you misread zero-based indices in the question?).

The vector com could grow to (in the worst case) the same size as the input vector.  That's quite a lot of extra storage.  It might be more efficient to leave the >= sum elements in place, and just search the beginning half of the input vector (no extra storage needed).

That looks like the following (making a few other simplifications, such as using an iterator instead of count, and reducing the scope of the find result):

static std::pair<int, int> findTwoSum(const std::vector<int>& list, int sum)
{
    for (auto it_b = list.begin();  it_b != list.end();  ++it_b) {
        if (auto it_a = std::find(list.begin(), it_b, sum - *it_b);  it_a != it_b) {
            return {it_a - list.begin(), it_b - list.begin()};
        }
    }
    return {-1, -1};
}

We still have a pretty inefficient algorithm - it's O(n²), where n is the length of list, because for every element in list, we perform a linear search for its complement.  We can reduce that, at the cost of reintroducing extra storage, by maintaining a set of seen values.  That may seem little different to the present approach of maintaining a vector, but the advantage is that search scales much better with size.  What we actually need is a map, as we'll want to note the corresponding index to return as result; the best choice is std::unordered_map:

  Unordered map is an associative container that contains key-value pairs with unique keys. Search, insertion, and removal of elements have average constant-time complexity.

That gives us:

    std::unordered_map<int, int> seen; // value -> index

    for (auto it_b = list.begin();  it_b != list.end();  ++it_b) {
        if (auto it_a = seen.find(sum - *it_b);  it_a != seen.end()) {
            return {it_a->second, it_b - list.begin()};
        } else {
            seen[*it_b] = it_b - list.begin();
        }
    }

Modified code

#include <unordered_map>
#include <utility>
#include <vector>

static std::pair<int, int> findTwoSum(const std::vector<int>& list, int sum)
{
    std::unordered_map<int, int> seen; // value -> index

    for (auto it_b = list.begin();  it_b != list.end();  ++it_b) {
        if (auto it_a = seen.find(sum - *it_b);  it_a != seen.end()) {
            return {it_a->second, it_b - list.begin()};
        } else {
            seen[*it_b] = it_b - list.begin();
        }
    }
    return {-1, -1};
}

And a very simple test program:

#include <iostream>
int main()
{
    auto const [a, b] = findTwoSum({ 1, 3, 5, 7, 9 }, 12);
    std::cout << a << ',' << b << '\n';
}


Recursively replace string placeholders with parameterized phrases I've written a python script to replace certain placeholders in a text with defined phrases. Some of the phrases might contain placeholders themselves, so the script needs to recursively replace them. Additionally the placeholders might allow parameters that need to be inserted at certain positions.

The current approach feels a little slow, because I'm iterating over the text much. It also fails (and crashes) if not enough parameters are provided for a parameterizable placeholder.

import re

parameters_regex = "(?:\[(.*)\])?"

def expand_message(message):
  """Recursivly replaces any occurencys of building blocks with its text."""
  for key, value in phrases.items():
    match = re.search(key + parameters_regex, message)
    if match:
      replacement = ""
      if (match.group(1)):
        parameters = match.group(1).split(",")
        replacement = value.format(*parameters)
      else:
        replacement = value
      replacement = expand_message(replacement)
      message = message.replace(match.group(0), replacement)
  return message

def format_numerators(message):
  """Replaces the numerator placeholders with the actual numerator."""
  numerator = 1
  while ('#.)' in message):
    message = message.replace('#.)', str(numerator) + ".)", 1)
    numerator+=1
  return message

def format_message(message):
  """Formats and returns the message."""
  message = expand_message(message)
  message = format_numerators(message)
  return message

This is how I'm testing it at the moment:

phrases = { }
phrases["bb:WORKDAY"] = "bb:GET_UP\nbb:BRUSH_TEETH\nbb:GO_TO_WORK[train]\nbb:GO_HOME"
phrases["bb:FRIDAY"] = "bb:WORKDAY\nbb:GO_OUT[cinema,John]\nbb:GO_HOME"
phrases["bb:WEEKEND"] = "bb:GET_UP\nbb:GO_OUT[park,Mary]\nbb:GO_HOME"

phrases["bb:GET_UP"] = "#.) Get out of bed."
phrases["bb:BRUSH_TEETH"] = "#.) Brush your teeth."
phrases["bb:GO_TO_WORK"] = "#.) Take the {} and go to work."
phrases["bb:GO_HOME"] = "#.) Go home."
phrases["bb:GET_UP"] = "#.) Get out of bed."
phrases["bb:GO_OUT"] = "#.) Go to the {} with {}."
phrases["bb:GET_UP"] = "#.) Get out of bed."

print(format_message("bb:WORKDAY"))
print("\n")
print(format_message("bb:FRIDAY"))
print("\n")
print(format_message("bb:WEEKEND"))

  1.) Get out of bed.
  2.) Brush your teeth.
  3.) Take the train and go to work.
  4.) Go home.

  1.) Get out of bed.
  2.) Brush your teeth.
  3.) Take the train and go to work.
  4.) Go home.
  5.) Go to the cinema with John.
  6.) Go home.

  1.) Get out of bed.
  2.) Go to the park with Mary.
  3.) Go home.

What if you would use re.sub() with a replacement function until there are no unfulfilled placeholders left. Something along these lines:

PHRASE_PATTERN = re.compile(r"(bb:\w+)(?:\[(.*)\])?")

def expand_message(message):
    """Recursively replaces any occurrences of building blocks with its text."""
    while "bb:" in message:
        message = PHRASE_PATTERN.sub(lambda group: phrases[group[1]].format(*group[2].split(","))
                                                   if group[2] else phrases[group[1]],
                                     message)
    return message

This way you don't need to iterate over all the available phrases and only handling the phrases used in a message. Should scale much better.


Shortening jQuery array In short, the code picks up to 9 different ids from li in jailplayer.

If a hillside, it shall save the value of the id, and then checks if there are more li, up to 9 pcs.

Then I save this in an array, depending on how many ids were found.

This looks like a mess.  Can someone help me shorten this code and make it easier to read?

if ($("#jailplayer").children("li").length >= 1) {
                var player1 = $("#jailplayer").find("li:nth-child(1)").attr("id");
                sender = sender + ',';
                sender = sender + player1;

            } else {
                var player1 = '';
            }
            console.log('done one, searching 2');

            if ($("#jailplayer").children("li").length >= 2) {
                var player2 = $("#jailplayer").find("li:nth-child(2)").attr("id");
                sender = sender + ',';
                sender = sender + player2;

            }else {
                var player2 = '';
            }
            if ($("#jailplayer").children("li").length >= 3) {
                var player3 = $("#jailplayer").find("li:nth-child(3)").attr("id");
                sender = sender + ',';
                sender = sender + player3;

            }else {
                var player4 = '';
            }

            if ($("#jailplayer").children("li").length >= 4) {
                var player4 = $("#jailplayer").find("li:nth-child(4)").attr("id");
                sender = sender + ',';
                sender = sender + player4;
            }else {
                var player4 = '';
            }

            if ($("#jailplayer").children("li").length >= 5) {
                var player5 = $("#jailplayer").find("li:nth-child(5)").attr("id");
                sender = sender + ',';
                sender = sender + player5;
            }else {
                var player5 = '';
            }

            if ($("#jailplayer").children("li").length >= 6) {
                var player6 = $("#jailplayer").find("li:nth-child(6)").attr("id");
                sender = sender + ',';
                sender = sender + player6;
            }else {
                var player6 = '';
            }

            if ($("#jailplayer").children("li").length >= 7) {
                var player7 = $("#jailplayer").find("li:nth-child(7)").attr("id");
                sender = sender + ',';
                sender = sender + player7;
            }else {
                var player7 = '';
            }

            if ($("#jailplayer").children("li").length >= 8) {
                var player8 = $("#jailplayer").find("li:nth-child(8)").attr("id");
                sender = sender + ',';
                sender = sender + player8;
                player8 .= ',';

            }else {
                var player8 = '';
            }

            if ($("#jailplayer").children("li").length >= 9) {
                var player9 = $("#jailplayer").find("li:nth-child(9)").attr("id");
                sender = sender + ',';
                sender = sender + player9;
            }else {
                var player9 = '';
            }

            console.log('adding ize');

            var ize =[sender];

It appears that you're just trying to create a list of players from the id values on a list of <li> tags and also build a comma separated list of those ids.  If that's the case, you can do the entire code block like this:

var players = $("#jailplayer > li").map(function(index, elem) {
    return this.id;
}).toArray();
var sender = players.join(",");

Note: this doesn't populate variables player1, player2, etc...  Instead, it puts the players into an array where they are likely easier to deal with later.

Also, it is unclear what you are trying to do with this:

var ize = [sender];

If you explain the purpose of that, we could offer help on whether that's the right way to to it.  Storing an array of a single value that's a comma separated string list of values seems a bit odd which is why I ask.

Key concepts here:

$("#jailplayer > li") - get all the desired <li> tags into a jQuery collection so we can iterate over them.

.map() - iterates over a jQuery collection building a new jQuery object containing the values that the callback returns.  Note, the callback will be passed this as each successive element in the original jQuery collection and the id value can be referenced simply as this.id.

.toArray() - get the array out of the jQuery object that was returned so you have just an array, not a jQuery object any more.

.join(",") - take all the elements of the array and join them into a string with the passed in separator between them.