Factory Girl in Rails I have a FactoryGirl page that generates same data but in two different languages. The variable names are the same. How do I write it in the smartest way possible without having to write a huge bunch of code?

    FactoryGirl.define do
      factory :static_page do
        language ""

        trait :purchase_loan do
          page_name "Purchase Loan Wizard"
          titlebar_text "Home Loan Wizard -Compare and find the best home loan in Indonesia"
         language "en"
        end

        trait :refinance_loan do
          page_name "Purchase Loan Wizard"
          titlebar_text "Rumah Kita Wizard -Dah Bayar rumah Best di Indonesia"
          language "ms"
        end

      end
    end

You can use I18n directly in factory definitions

FactoryGirl.define do
  factory :static_page do
    language "en"
    trait :purchase_loan do
      page_name { I18n.t("static_page.page_name.purchase_loan", locale: language) }
      titlebar_text { I18.t("static_page.titlebar_text.purchase_loan", locale: language) }
    end

    trait :refinance_loan do
      page_name { I18n.t("static_page.page_name.refinance_loan", locale: language) }
      titlebar_text { I18.t("static_page.titlebar_text.refinance_loan", locale: language) }
    end
  end
end

This will allow you to share locales in views / application code and in your tests.

But you could go 1 step further and in your model store only locale keys, ie.

class StaticPage < ActiveRecord::Base
  #attribues in db :pagename, :titlebar, :language
  def pagename_text
    I18n.t(pagename, locale: language)
  end

  def titlebar_text
    I18n.t(titlebar, locale: language)
  end
end

then you can use it this way:

purchase_loan_en = StaticPage.create(language: 'en', page_name: 'static_page.page_name.purchase_loan')

this will allow you to remove translations directly from factories.

Have a look on I18n Rails guide for how to use I18n. Maybe you don't even need to store language in database.


Measure website home page total network size in bytes We have a requirement to check the size of some of our website home pages, in MB. This is to ensure that they are not growing too large, and that large images haven't been uploaded. I couldn't find much info on this, but have come up with following which is working as required.

The code is designed to either run locally on my machine, or by leveraging our Selenium grid. It simply loads home page, and from the browser Performance log, we strip out the Network.dataReceived (bytes) information and sum it.

Finally, we have set levels which the site should fall within.

#!/usr/bin/python
""" This script will simply check the download page size (bytes) of a Home page."""
import argparse
import re
import sys

from selenium import webdriver
from selenium.webdriver.common.desired_capabilities import DesiredCapabilities

parser = argparse.ArgumentParser(description="This script will measure size of a home page.")
parser.add_argument('--site', default='somewebsite.com', required=True)
parser.add_argument('--local', action='store_true', default=False)
parser.add_argument('--datacentre', choices=['dc1', 'dc2'])
args = parser.parse_args()

logging_prefs = {'performance' : 'INFO'}

if args.local:
    caps = DesiredCapabilities.CHROME.copy()
    caps['loggingPrefs'] = logging_prefs
    driver = webdriver.Chrome(desired_capabilities=caps)
else:
    profile = webdriver.FirefoxProfile()
    profile.set_preference('plugin.state.flash', 0)
    profile.set_preference("webdriver_enable_native_events", False)
    profile.update_preferences()
    caps = DesiredCapabilities.FIREFOX.copy()
    caps['loggingPrefs'], caps['acceptSslCerts'] = logging_prefs, False
    if args.datacentre == 'dc1':
        driver = webdriver.Remote(
            command_executor='http://selenium/hub',
            desired_capabilities=caps,
            browser_profile=profile)
    elif args.datacentre == 'dc2':
        driver = webdriver.Remote(
            command_executor='http://selenium/hub',
            desired_capabilities=caps,
            browser_profile=profile)

driver.implicitly_wait(30)
driver.set_page_load_timeout(30)
url = "http://" + args.site + "/"
driver.get(url)
total_bytes = []

try:
    for entry in driver.get_log('performance'):
        if "Network.dataReceived" in str(entry):
            r = re.search(r'encodedDataLength\":(.*?),', str(entry))
            total_bytes.append(int(r.group(1)))
except Exception:
    print 'error'
    driver.close()
    sys.exit(3)
if total_bytes is not None:
    mb = round((float(sum(total_bytes) / 1000) / 1000), 2)

if args.local:
    from datetime import datetime
    d = (datetime.today()).strftime("%d-%m-%y-%H-%M")
    filename = 'results_{}.txt'.format(str(d))
    with open(filename, 'a') as f:
        f.write("{}, {}\n".format(args.site, mb))

try:
    if mb < 2.0:
        print "OK. Total Network Data Received size for {}: {}MB".format(args.site, str(mb))
        sys.exit(0)
    elif mb >= 2.0 and mb < 4.0:
        print "Warning. Total Network Data Received size for {}: {}MB".format(args.site, str(mb))
        sys.exit(1)
    elif mb > 4.0:
        print "CRITICAL. Total Network Data Received size for {}: {}MB".format(args.site, str(mb))
        sys.exit(1)
except Exception:
    print "UNKNOWN. Something went wrong."
    sys.exit(3)
finally:
    driver.close()

You're using the wrong shebang. According to this StackOverflow answer:

  Correct usage for Python 3 scripts is:

#!/usr/bin/env python3

  This defaults to version 3.latest. For Python 2.7.latest use python2 in place of python3.

Don't use #!/usr/bin/python. From the Tutor mailing list:

  Consider the possiblities that in a different machine, python may be installed at /usr/bin/python or /bin/python in those cases, the above #! will fail.
  For those cases, we get to call the env executable with argument which will determine the arguments path by searching in the $PATH and use it correctly.

I'd move command line argument handling to the bottom, possibly in a main(). The rest of the code could be separated into function 'units'.
Certain variables and literals could be replaced by constants. From what I can tell:

logging_prefs could be a constant;
Waiting times could be constants;
Exit codes could be constants.

Catching Exception is bad practice.
These parentheses:

d = (datetime.today()).strftime("%d-%m-%y-%H-%M")

are superfluous.
This cast:

filename = 'results_{}.txt'.format(str(d))

is pointless.
Don't mix single and double quotes.
Don't assign multiple variables on a single line.
The same driver will be instantiated, regardless of the 'datacentre' argument. Therefore, the if-statement can be left out altogether.
Error messages should be written to stderr, not stdout.
Imports should be placed at the top of a file, and rarely in a function.

Here's my take on it (untested):

#!/usr/bin/env python3

""" This script will simply check the download page size (bytes) of a Home page."""

from __future__ import print_function

import argparse
from datetime import datetime
import re
import sys

from selenium import webdriver
from selenium.webdriver.common.desired_capabilities import DesiredCapabilities

LOGGING_PREFS = {"performance": "INFO"}

WAITING_TIME = 30
TIMEOUT = 30

DATA_LENGTH_REGEX = r"encodedDataLength\":(.*?),"

EXIT_OK = 0
EXIT_WARNING = 1
EXIT_FAILURE = 2

def get_driver(local):
    if local:
        caps = DesiredCapabilities.CHROME.copy()
        caps["loggingPrefs"] = LOGGING_PREFS
        return webdriver.Chrome(desired_capabilities=caps)
    else:
        profile = webdriver.FirefoxProfile()
        profile.set_preference("plugin.state.flash", 0)
        profile.set_preference("webdriver_enable_native_events", False)
        profile.update_preferences()

        caps = DesiredCapabilities.FIREFOX.copy()
        caps["loggingPrefs"] = LOGGING_PREFS
        caps["acceptSslCerts"] = False

        return webdriver.Remote(
            command_executor="http://selenium/hub",
            desired_capabilities=caps,
            browser_profile=profile
        )

def get_homepage_size(driver, website):
    driver.implicitly_wait(WAITING_TIME)
    driver.set_page_load_timeout(TIMEOUT)
    url = "http://" + website + "/"
    driver.get(url)
    total_bytes = []

    try:
        for entry in driver.get_log("performance"):
            entry = str(entry)
            if "Network.dataReceived" in entry:
                r = re.search(DATA_LENGTH_REGEX, entry)
                total_bytes.append(int(r.group(1)))
    except Exception:
        # TODO Find a more specific exception.
        # What could fail?
        driver.close()
        print("Failed to get data size for {}".format(website), file=sys.stderr)
        sys.exit(EXIT_FAILURE)

    if total_bytes is not None:
        return round((float(sum(total_bytes) / 1000) / 1000), 2)

def write_results_to_file(website, size_in_mb):
    date = datetime.today().strftime("%d-%m-%y-%H-%M")
    filename = "results_{}.txt".format(date)
    with open(filename, "a") as f:
        f.write("{}, {}\n".format(website, size_in_mb))

def display_results(website, size_in_mb):
    if size_in_mb < 2.0:
        print("OK. Total Network Data Received size for {}: {}MB".format(
            website, size_in_mb)
        )
        sys.exit(EXIT_OK)
    elif size_in_mb >= 2.0 and size_in_mb < 4.0:
        print("Warning. Total Network Data Received size for {}: {}MB".format(
            website, size_in_mb
        )
        sys.exit(EXIT_WARNING)
    elif size_in_mb >= 4.0:
        print("CRITICAL. Total Network Data Received size for {}: {}MB".format(
            website, size_in_mb)
        )
        sys.exit(EXIT_WARNING)

def main():
    parser = argparse.ArgumentParser(description="This script will measure size of a home page.")
    parser.add_argument("--site", default="somewebsite.com", required=True)
    parser.add_argument("--local", action="store_true", default=False)
    parser.add_argument("--datacentre", choices=["dc1", "dc2"])
    args = parser.parse_args()

    driver = get_driver(args.local)
    size_in_mb = get_homepage_size(driver=driver, website=args.site)
    if args.local:
        write_results_to_file(args.site, size_in_mb=size_in_mb)
    display_results(args.site, size_in_mb=size_in_mb)

if __name__ == "__main__":
    main()


Text-based RPG game in C++ This is a text-based RPG game I've made in C++ and would love some review on it. I am a 100% complete newbie with maybe 2 weeks of C++ experience so I'd love some lessons on what I did wrong and how I can improve! I mainly just use a lot of switch statements and if statements.

If there is anything wrong or things that could change and improve I'd very much appreciate if you could walk me through what was wrong, why it was wrong and how to prevent it from happening again. I really don't know a lot of C++ terms so ELIF would be awesome.

Here's my simple main.cpp. I think that's called a constructor and I'm using it to call my MeadowGiant.cpp down the road.

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

int main()
{
MeadowGiant obj1;
}

MeadowGiant.h

#define MEADOWGIANT_H

class MeadowGiant
{
public:
    MeadowGiant();
protected:
};

#endif

MeadowGiant.cpp

#include "MeadowGiant.h"
#include <iostream>
#include <string>
#include <time.h>
#include <stdio.h>
#include <stdlib.h>

MeadowGiant::MeadowGiant()
{
int trail, caveEnter;
int fight1;
int loot;
int goblinAttack, goblinHP, HP, attack, action1, ability;
goblinAttack = 5;
goblinHP = 10;
HP = 100;

std::cout << -Insert story narrative- Choose a trail to go down.\n";
std::cin  >> trail;

    if(trail==1)
    {
    std::cout << "-Insert story narrative- Come across a cave. Do you go in?\n";
    std::cin  >> caveEnter;

        if(caveEnter==1)
        {
        std::cout << "You are ambushed by a goblin. Run or fight?\n";
        std::cin >> fight1;

        goblinFight:
        loot = rand() % 4 + 1;
        srand(time(NULL));

        if(fight1==1)
        {
        std::cout   << "Your HP: " << HP << std::endl;
        std::cout   << "Enemy HP: " << goblinHP << std::endl;
        std::cout   << "What do you do?\n
                    << "[1] Attack [2] Run \n";
        std::cin    >> action1;
        }
            if(goblinHP<=0)
            {
            std::cout << "You have slain the goblin!\n"
                      << "You head towards the chest and take the spoils\n"
                      << "of battle!\n";
                 switch(loot)
            {
            case 1: std::cout << "You find a bastard sword!\n";
                              attack = attack + 7;
                              goto exitCave;
            case 2: std::cout << "You find an Enchanted Staff!\n";
                              attack = attack + 10;
                              goto exitCave;
            case 3: std::cout << "You find an Obsidian Dagger!\n";
                              attack = attack + 9;
                              goto exitCave;
            case 4: std::cout << "You find a Holy Mace!\n";
                              attack = attack + 10;
                              goto exitCave;
            }
    else if(action1==1)
    {
    std::cout   << "You successfully hit the goblin!\n"
                << "He strikes back!\n";
    attack = rand() % 10 + 1;
    srand(time(NULL));

    goblinHP = goblinHP - attack;
    HP = HP - goblinAttack;
    goto goblinFight;
    }
    else if(action==2)
    {
    std::cout   << "You take the cowards way out and leave the cave.\n";
    goto exitCave;
    }
    }
else if(caveEnter==2)
{
exitCave:
std::cout << "You have exited the cave.\n";
}
else
{
goto exitCave;
}
}
}

The format of my actual code doesn't look like this in my IDE. The code is the same, but the tabs are much cleaner in my compiler so if that's an issue, I'm already on it.

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

Fix typos

There were so many typos in this code that I had to do quite a number of fixes before the code would even compile.  I'd recommend composing the question offline, using a text editor that changes tabs to spaces. Then cut and paste the whole thing into CodeReview.  It will save many people lots of time, including and especially you!

Don't put everything into the constructor

The MeadowGiant class has all of its code in the constructor.  That's not very good design.  Instead, it would be better to instantiate one or more of them and then use member functions to manipulate the object.

Use only necessary #includes

The #include <stdio.h> line is not necessary and can be safely removed.

Don't reseed the random number generator more than once

The program currently calls srand after every call to rand().  This is really neither necessary nor advisable.  Instead, just call it once when the program begins and then continue to use rand() to get random numbers.  Better yet, see the next suggestion.

Consider using a better random number generator

If you are using a compiler that supports at least C++11, consider using a better random number generator.  In particular, instead of rand, you might want to look at std::uniform_real_distribution and friends in the <random> header.

Use constant string concatenation

This code currently has a number of instances that look like this (or would if the typos were fixed):

std::cout << "You have slain the goblin!\n"
    << "You head towards the chest and take the spoils\n"
    << "of battle!\n";

This calls the << operator three times.  Instead, you could write this:

std::cout << "You have slain the goblin!\n"
    "You head towards the chest and take the spoils\n"
    "of battle!\n";

This only calls << once.  The compiler automatically concatenates the string literals together.

Avoid using goto

Having a proliferation of goto statements is usually a sign of bad design.  Better would be to eliminate them entirely -- it makes the code easier to follow and less error-prone.  In this code, it's probable that you could use a loop instead:

for (goblinHP = 10; goblinHP > 0; ) {
    // code to implement goblin attack goes here
}

Use C++-style includes

Instead of including stdlib.h you should instead use #include <cstdlib>.  The difference is in namespaces as you can read about in this question.

Eliminate unused variables

Unused variables are a sign of poor quality code, and you don't want to write poor quality code. In this code, ability is unused.  Your compiler is smart enough to tell you about this if you ask it nicely.


Optimizing variadic template pack subsequence matching algorithm I'm building a small MPL module in one of my utility libraries for fun and learning experience.

One of the problems I'm trying to solve is getting a list of all indices where a sequence of types appears inside another sequence of types.

Example:

static_assert(is_same
<
    List<int, char, int, int, char, int, double, int, char>
        ::IdxsOfSeq<int, char>,
    ListInt<0, 3, 7>
>());

The above code is valid and works as intended. However, finding out the indices of a subsequence in a long type sequence becomes very slow very fast.

Searching in a list with 20 or 30 elements can take up to 30 seconds of compilation time.

That would rarely be useful, but I'm also experimenting with compile-time strings implemented as character lists.

The current implementation of IdxsOfSeq basically uses std::index_sequence to perform a naive string matching algorithm over the type sequence.

The naive algorithm requires no preprocessing time, but the execution of the type matching requires a lot of computation time.

This is my current implementation of the string matching algorithm.

// Matches<bool, int> returns a ListInt<TStart> is the boolean
// is true, otherwise an empty ListInt<>
template<bool TAllTrue, int TStart>
struct Matches;

template<int TStart>
struct Matches<true, TStart>
{
    using Type = ListInt<TStart>;
};

template<int TStart>
struct Matches<false, TStart>
{
    using Type = ListInt<>;
};

// GetMatch<TS, TM, TStart, TMIdxs>::Type checks if every type
// of TS (the source type sequence) starting from the index TStart
// matches every type in TM (the subsequence to find)
template<typename TS, typename TM, int TStart, typename TMIdxs>
struct GetMatch;

template<typename TS, typename TM, int TStart, SizeT... TMIdxs>
struct GetMatch<TS, TM, TStart, IdxSeq<TMIdxs...>>
{
    using Type = typename Matches
    <
        // True if all the types inside AreAllTrue are std::true_type
        AreAllTrue
        <
            // Type checking with index sequence expansion
            std::is_same_t
            <
                typename TS::template At<TStart + TMIdxs>,
                typename TM::template At<TMIdxs>
            >...
        >{}(),
        TStart
    >::Type;
};

// Main helper template
// Finds every occurrence of the list TM inside TS and returns its index
template<typename TS, typename TM, typename TSIdxs, typename TMIdxs>
struct IdxsOfSeqHlpr;

template<typename TS, typename TM, SizeT... TSIdxs, typename TMIdxs>
struct IdxsOfSeqHlpr<TS, TM, IdxSeq<TSIdxs...>, TMIdxs>
{
    // ConcatLists (obviously) concatenates the contents of the
    // lists passed as template parameters in a single list
    using Type = typename ConcatLists
    <
        ListInt<>,
        typename GetMatch<TS, TM, TSIdxs, TMIdxs>::Type...
    >::Type;
};

// If the subsequence to match is empty, return an empty ListInt<>
template<typename TS, SizeT... TSIdxs, typename TMIdxs>
struct IdxsOfSeqHlpr<TS, List<>, IdxSeq<TSIdxs...>, TMIdxs>
{
    using Type = ListInt<>;
};

// Main typedef
//
template<typename TS, typename TM>
using IdxsOfSeq = typename IdxsOfSeqHlpr
<
    // Source list
    TS,

    // Subsequence to match
    TM,

    // Index sequence of the source list
    // Starts from 0, ends at (source_list_size - matching_list_size + 1)
    std::make_index_sequence
    <
        getClampedMin(int(TS::size - TM::size + 1), 0)
    >,

    // Index sequence of the subsequence to match
    std::make_index_sequence<TM::size>
>::Type;

Is there any part of the algorithm that can easily be optimized?

Or is it better to rewrite the solution from scratch, using another algorithm?

I'm going to throw out a fairly complicated solution here by breaking the problem into lots of smaller parts. The end result is that the compilation of the following:

using T = typelist<int, char, int, int, char, int, double, int, char,
                   int, char, int, int, char, int, double, int, char,
                   int, char, int, int, char, int, double, int, char,
                   int, char, int, int, char, int, double, int, char>;

static_assert(std::is_same<
    matches<T, typelist<int, char>>,
    intlist<0, 3, 7, 9, 12, 16, 18, 21, 25, 27, 30, 34>
    >::value, "!");

is still almost instantaneous.

The algorithm is actually going to come from an idea in Python's itertools. What we want to do is walk our typelist N at a time (where N is the match length), and just use std::is_same to compare. That is, taking the original problem's list:

using SRC = typelist<int, char, int, int, char, int, double, int, char>;

we want to turn it into something that looks like:

using X = typelist<typelist<int, char>,
                   typelist<char, int>,
                   typelist<int, int>,
                   ...
                   >;

And then pass that into a metafunction taking an appropriate typelist<Ts...> and std::index_sequence<Is...> do:

using type = concat<
    std::conditional_t<std::is_same<T, SUB>::value,
        typelist<std::integral_constant<std::size_t, Idx>>,
        typelist<>
        >...>;

That's the basic idea anyway. Plus, the smaller pieces are probably going to be useful in other contexts.

The Small Algorithms

These are very straightforward and I will omit them here for brevity: head, tail, size, concat (variadic!), any_of, and is_empty.

take_by

To generate the X typelist above, we need something like:

template <typename TL, int I>
using take_by = ???

The implementation of this is to make a typelist that is I copies of TL, each one dropping a steadily larger prefix off the top. Once we have that, we need to zip all the iterators together. So really, I need to start with the smaller pieces...

skip_n

Given a typelist, basically just call tail n times:

template <typename TL, int I>
struct skip_n_impl : skip_n_impl<tail<TL>, I-1> { };

template <typename TL>
struct skip_n_impl<TL, 0> {
    using type = TL;
};

template <typename TL, int I>
using skip_n = typename skip_n_impl<TL, I>::type;

There may be a better way to do this, but I'm not sure. So, skip_n<typelist<int, char, double>, 2> is typelist<double>.

zip

This is the tricky part. Basically, we have a list of lists of types. If any of those typelists is empty, we're done. Otherwise, we concat all the heads with a recursive call against all the tails. We can't use std::conditional_t here due to eager evaluation of both parts, so I'm introducing two different impl types:

template <typename TL>
struct zip_impl;

template <typename TL, bool done>
struct zip_impl2;

template <typename TL>
struct zip_impl2<TL, true> {
    using type = typelist<>;
};

template <typename... Ts>
struct zip_impl2<typelist<Ts...>, false> {
    using type = concat<typelist<typelist<head<Ts>...>>,
                        typename zip_impl<typelist<tail<Ts>...>>::type
                        >;
};

template <typename... Ts>
struct zip_impl<typelist<Ts...>>
    : zip_impl2<typelist<Ts...>, any_of<is_empty<Ts>::value...>::value>
{ };

template <typename T>
using zip = typename zip_impl<T>::type;

This gives us the pieces to get back to take_by:

template <typename TL, int I, typename = std::make_index_sequence<I>>
struct take_by_impl;

template <typename TL, int I, std::size_t... Idx>
struct take_by_impl<TL, I, std::index_sequence<Idx...>>
{
    using iters = typelist<skip_n<TL, Idx>...>;
    using type = zip<iters>;
};

template <typename TL, int I>
using take_by = typename take_by_impl<TL, I>::type;

Hopefully that makes sense. First, we built up our "iterators" - which are all offset by one from each other. Then we just zip them up. Now we have the X, and all we need is:

**matches**:

template <typename SRC, typename SUB>
struct matches_impl
{
    template <typename TL, typename = std::make_index_sequence<size<TL>::value>>
    struct impl;

    template <typename... T, std::size_t... Idx>
    struct impl<typelist<T...>, std::index_sequence<Idx...>>
    {
        using type = concat<
            std::conditional_t<std::is_same<T, SUB>::value,
                typelist<std::integral_constant<std::size_t, Idx>>,
                typelist<>
                >...>;
    };

    using type = typename impl<take_by<SRC, size<SUB>::value>>::type;
};

template <typename SRC, typename SUB>
using matches = typename matches_impl<SRC, SUB>::type;

Basically, we end up creating a meta-list internally that would look like:

intlist<0>,
typelist<>,
typelist<>,
intlist<3>,
typelist<>,
...

But concat will drop the empties, so when that gets combined, we get what we wanted - intlist<0, 3, 7>.


Is there a more efficient way of implementing this clock in Raphael JS? Can someone let me know if this is a proper (and efficient) way of doing it? It works, but I wanted to understand if there's a way it be made better (I can use a for loop for the clones, but my browser gets stuck if I do it).

Here's the fiddle

window.onload = function() {
  rot();
}
var rot = function(){
var paper = new Raphael(document.getElementById('canvas_container'), 500, 500);
var c1 = paper.circle(250,250,150).attr({"stroke-width":2});
var c2 = paper.circle(250, 250, 5).attr({"fill":"black"});
var lMark = paper.path("M 250 250 m 0 -130 l 0 -20").attr({"stroke-wid th":2});
lMark.clone().attr({transform:"r90 250 250"});
lMark.clone().attr({transform:"r180 250 250"});
lMark.clone().attr({transform:"r270 250 250"});
lMark.clone().attr({transform:"r360 250 250"});
var d = new Date();
var hourAngle = (d.getHours() * 60 + d.getMinutes())/2, minAngle = (d.getMinutes() * 6), secAngle = (d.getSeconds() * 6);
var secHand = paper.path("M 250 250, l 0 -110, M 250 250, l 0 15").attr({"stroke":"red", "stroke-width":2});
var minHand = paper.path("M 250 250, l 0 -120").attr({'stroke-width':2}).transform("r" + minAngle + " 250 250");
var hourHand = paper.path("M 250 250, l 0 -80").attr({'stroke-width':3, 'stroke-linecap':'round'}).transform("r" + hourAngle + " 250 250");
var secAnim = Raphael.animation({transform: "r360 250 250"}, 60000).repeat(Infinity);
var minAnim = Raphael.animation({transform: "r360 250 250"}, 3600000).repeat(Infinity);
var hourAnim = Raphael.animation({transform: "r360 250 250"}, 43200000).repeat(Infinity);
secHand.animate(secAnim);
minHand.animate(minAnim);
hourHand.animate(hourAnim);
}

I think not, this is how it is meant to be.

That being said, you

Have a few missing semicolons
Capture c1 & c2 for no good reason
Calculate secAngle for no good reason
Have a veritable legion of hard coded mystical constants
Have a few weirdly named variables

After some thought, it appears that the biggest problem with this code is that you count on the animation to take 0 (milli)seconds and the execution/scheduling of the animations is instantaneous. Have you tried keeping this clock up for an hour or more? I would expect there to be discrepancies between the real time and the animated clock.


Sampling from a distribution with given probabilities I have found myself needing to sample numbers from a given distribution and was wondering what the most efficient method of doing so is.

I have a set of N elements 0 to N-1 and an array mapping those elements to the probability of their selection, pp.

My approach is to create a new array, cdf, and accumulate the entries of pp so that the first element of cdf is pp[0] and the last element is the sum of all the elements in pp, namely 1. Then, since this array is necessarily sorted, I choose a random number between 0 and 1 and perform a binary search in the array.

This is my code so far (in Java):

public class Sample {
    /**
     * Selects q elements chosen at random according to the probabilities
     * given in pp.
     *
     * @param pp  The probabilites corresponding to each index
     *            (item 0 has probability pp[0] and so forth)
     * @param q   The number of elements to sample
     * @return    An array containing the elements sampled
     */
    public static int [] sample(double [] pp, int q) {
        double [] cdf = pp.clone();
        for (int i = 1; i < cdf.length; i++)
            cdf[i] += cdf[i - 1];

        int [] values = new int[q];
        for (int i = 0; i < q; i++) {
            // binarySearch returns a negative number at the insertion point
            // if the exact number isn't found. We don't expect to hit an
            // exact match with Math.random(), so the Math.abs() call
            // can't be too expensive.
            values[i] = Math.abs(Arrays.binarySearch(cdf, Math.random()));
        }

        return values;
    }
}

I'm mostly unsure about the behavior of the Arrays.binarySearch function. Am I at risk of missing the first/last element, or going out of bounds? The running time of this algorithm is \$O(N)\$ for the cdf construction, plus \$O(q \log N)\$ for the actual sampling. The second operation dominates (\$q\$ could be greater than \$N\$), so this is an \$O(q \log N)\$ algorithm, which is slower than sorting! Is there any way to do this more quickly?

Just a suggestion:

I think you can use better variable names than pp and q.

cdf can be clonedArray, and the same for other variable names.

This doesn't matter so much but increases the readability of the program.


a function to partition images into patches I wrote a function that partitions an image into multiple patches. Here is the function, are there any suggestions to make it better or more production ready in terms of the best software practice?

def cal_patches(img,num_colums):
   patch_list = []
   num_rows = num_colums
   x,y,w,h = 0,0, int(img.shape[1]/num_colums), int(img.shape[0]/ num_rows)
   ind = 0
   for i in range(num_colums):
     x= 0
     for j in range(num_rows):
       ind += 1
       y_end = min(y+h, img.shape[0])
       x_end = min(x+w, img.shape[1])
       patch = img[y: y_end, x: x_end]
       patch_list.append(patch)
       x += w
     y += h
   return patch_list

PEP-8 Guidelines

Your code violates the PEP-8 standard in several area, including (but not limited to):

Operators should have 1 space before and after the operator (y + h instead of y+h)
One space after commas (x, y, w, h not x,y,w,h)
Slices should not have any spaces around the colon (y:y_end not y: y_end)

Spelling

Columns is spelt with an n.

Meaningful names

I don't know what cal_patches means.  Is cal short for "calculate"?  Your question title says "partition"; perhaps that should be part of the function name.

docstring

Add a doc-string to help() the user understand what the function does, and what the function returns ... without having to consult the source code.

Type Hints

With Python 3.6+, you can add type hints to the function to further improve understanding about what the function requires and what it returns:

def cal_patches(img: Image, num_colums: int) -> List[Image]:

Useful arguments

The function can partition the image into a number of vertical columns.  What if I wanted a number of rows as well?  I can't request that.

Oh wait!  It does do rows too.  But always the same number of rows and columns.  Perhaps add num_rows to the argument list too?

Integer division

int(n / d) is verbose and unnecessary.  Use the integer division operator n // d.

Unused variables

ind is never used anywhere, and could be removed.

i and j are never used anywhere, so could be replaced by the "throwaway" variable _.  Even better, you could use the w and h values as the step size for the range(), and get rid of the x += w and y += h statements:

Avoid calculating unchanging values

In the inner loop, y is not changing, so y_end results in the same value each iteration of the inner loop.  It can be moved out of the inner loop.

Bug?

The inner loop goes over the number rows, but is increase x by w, which is a column increment.  Did you accidentally swap the rows and columns?  It works as long as num_rows == num_columns, but it will cause grief when that is relaxed.

Updated code

def create_image_patches(img, num_rows, num_columns):
    """
    Partition an image into multiple patches of approximately equal size.
    The patch size is based on the desired number of rows and columns.
    Returns a list of image patches, in row-major order.
    """

    patch_list = []
    width, height = img.shape[1], img.shape[0]
    w, h = width // num_columns, height // num_rows

    for y in range(0, height, h):
        y_end = min(y + h, width)
        for x in range(0, width, w):
            x_end = min(x + w, height)
            patch = img[y:y_end, x:x_end]
            patch_list.append(patch)

    return patch_list

Add type hints using the typing module.

Generator

As suggested by @Graipher, this function could be turned into a generator, which may be useful depending on how the patches are processed downstream:

def generate_image_patches(img, num_rows, num_columns):
    """
    Generate image patches of approximately equal size from a source image.
    The patch size is based on the desired number of rows and columns.
    Patches are generated in row-major order.
    """

    width, height = img.shape[1], img.shape[0]
    w, h = width // num_columns, height // num_rows

    for y in range(0, height, h):
        y_end = min(y + h, width)
        for x in range(0, width, w):
            x_end = min(x + w, height)
            yield img[y:y_end, x:x_end]

A "helper" function could be used to turn the generator back into a list, if required for existing callers of the function:

def create_image_patches(img, num_rows, num_columns):
    """
    Partition an image into multiple patches of approximately equal size.
    The patch size is based on the desired number of rows and columns.
    Returns a list of image patches, in row-major order.
    """

    return list(generate_image_patches(img, num_rows, num_columns))


LeetCode Count number of set bits solution Question - Write a function that takes an unsigned integer and return the number of '1' bits it has (also known as the Hamming weight).

int hammingWeight(uint32_t n) {
        int count = 0;
        while(n>0){
            count+=n&1;
            n>>=1;
        }
        return count;
    }

Can you come up with a better method?

One gentleman came up with this

int hammingWeight(uint32_t n) {
    int count = 0;

    while (n) {
        n &= (n - 1);
        count++;
    }

    return count;
}

How does one come up with such creative solutions?

And do you have a better one or any improvements I can make to my solution?

(Because of the way Code Review works, only your code can be reviewed.  Therefore, I will not say anything on the code written by "one gentleman".)

The first thing I see is: use consistent indentation and add spaces.  Your code becomes much more readable if you format it like this:

int hammingWeight(uint32_t n) {
    int count = 0;
    while (n > 0) {
        count += n & 1;
        n >>= 1;
    }
    return count;
}

Also, it is probably a good idea to use std::uint32_t instead of uint32_t in C++.

You can make this function constexpr and noexcept.  Simple computations like this can be done at compile time, and benefit from inlining.

This function can also be generalized to take any unsigned integer type.  A template version may look like:

template <typename T, std::enable_if_t<std::is_integral_v<T> && std::is_unsigned_v<T>>>
constexpr int hammingWeight(T n) noexcept
{
    // the implementation is the same
}

The implementation is good enough and very readable IMO.  You don't have to get very creative unless measuring reveals the necessity of optimization.

In C++20, we have a standard function for this — std::popcount, in header <bit>.


Backbone subviews with non-uniform layout I have a “featured projects” view that requires its subviews conform to a non-uniform grid: one narrow column with 3 items, then one wide column with 2 items, followed by another narrow column with 3 items. The width that is used for each subview is determined by the model’s display attribute. If there are any extra items in the collection, they are disregarded for the purposes of this view.

Here's the relevant portion of the code:

class App.Views.FeaturedProjects extends Backbone.View
  el: '#featured'
  template: '

'

  initialize: ->
    ...
    @collection.on 'reset', @addAll, this

  addOne: (item, el) =>
    view = new App.Views.Project model: item
    el.append view.render().el

  addAll: ->
    # … clear out existing subviews, then …

    # separate collection into wide & narrow
    narrow = @collection.where display: 'narrow'
    wide = @collection.where display: 'wide'

    # only display the grid if there are enough items to fill it
    if narrow.length >= 6 and wide.length >= 2
      column1 = $(@template).addClass('column-narrow')
      column2 = $(@template).addClass('column-wide')
      column3 = $(@template).addClass('column-narrow')

      # 3 narrow / 2 wide / 3 narrow
      @addOne(item, column1) for item in narrow[0..2]
      @addOne(item, column2) for item in wide[0..1]
      @addOne(item, column3) for item in narrow[3..5]

      @$el.append(column1).append(column2).append(column3)

My current approach works, but is there a better way of handling something like this?

EDIT

I took Marco's suggestions a bit further and ended up with the following:

class App.Views.FeaturedProjects extends Backbone.View
  columnTemplate: '

'

  defaults: ->
    columns: [
      { type: 'narrow', itemsPer: 3 }
      { type: 'wide', itemsPer: 2 }
      { type: 'narrow', itemsPer: 3 }
    ]

  initialize: (options) ->
    @options = _.extend({}, @defaults(), options)
    ...
    @collection.on 'reset', @addAll, this

  addAll: ->
    # … clear out existing subviews, then …
    items = {}
    valid = true
    # get unique column types
    uniqueColTypes = _.uniq _.pluck @options.columns, 'type'

    # separate collection by column type and verify there are enough items of each type
    for type in uniqueColTypes
      items[type] = @collection.where display: type
      valid = false if items[type].length < @minItems type

    # display the grid
    if valid
      for column in @options.columns
        columnHtml = $(@columnTemplate).addClass "column-#{column.type}"
        itemsPer = column.itemsPer
        @addOne items[column.type].shift(), columnHtml while itemsPer--
        @$el.append(columnHtml)

  # returns number of items with specified column type required to fill the grid
  minItems: (type) ->
    minItems = 0
    minItems += col.itemsPer for col in _.where @options.columns, {type: type}
    return minItems

This looks pretty good. My main suggestion for improvement would be we how you set and create the columns. I would try and make this more dynamic so the amount of columns can more easily change, and can be set from code using this view.

To do this I'd create an array to represent the columns. Each column would have information about whether its narrow, or wide and then how many items it should have in it:

columns = [{type: 'narrow', itemsPer: 3}, {type: 'wide', itemsPer: 2},
  {type: 'narrow', itemsPer: 3}]

And then process the array rather then have variables named for each column. This would also add the items to the columns so it can be done it one loop. I used "shift" to remove the first element of the array so the narrow collection could be used without referencing exact indexes, which would allow it to work for any amount of items per column. I also changed the structure of the narrow and wide variables so they are properties on a items object, which makes the referenced with the same column.type name.

items[narrow] = @collection.where display: 'narrow'
items[wide] = @collection.where display: 'wide'

if narrow.length >= 6 and wide.length >= 2
  for column in columns
    columnHtml = $(@template).addClass('column-' + column.type)
    for i in range(column.itemsPer)
      @addOne(items[column.type].shift(), columnHtml)

    @$el.append(columnHtml)

Another thing, I found the template property to be confusing, because it was the template for each of the three columns where I expected it to be the template for the whole view (which would only be created once). I suggest naming it to something like "columnTemplate"

columnTemplate: '

',

My code examples are in pseudo code, as I'm less familiar with CoffeeScript, but I think you can understand generally what I'm saying.


Wrapper that accepts both scalar and collection Can I somehow refactor this into one class? Some things are common here like Metadata property.

public class Resource<TEntity>
{
    public TEntity Data { get; set; }

    public Dictionary<string, string> Metadata { get; set; }

    public Resource(TEntity data)
    {
        Data = data;
    }
}

public class ResourceCollection<TEntity>
{
    public ICollection<TEntity> Data { get; set; }

    public Dictionary<string, string> Metadata { get; set; }

    public int Count { get; set; }

    public ResourceCollection(ICollection<TEntity> data)
    {
        Data = data;

        Count = data.Count;
    }
}

Is this possible to make interface that expects either ICollection<TEntity> or TEntity alone?

This wrapper class is used for serialization it simply acts as a wrapper, but still I have some doubts about it that it could be done better

I also came up with this:

public class Resource<T>
{
    public T Data { get; set; }

    [JsonExtensionData]
    public Dictionary<string, object> Metadata { get; set; } = new Dictionary<string, object>();

    public Resource(T data)
    {
        Data = data;

        if (data is ICollection collection)
        {
            Metadata["count"] = collection.Count;
        }
    }
}

I guess you could use inheritance if you really need that extra property:

public class ResourceCollection<TEntity> : Resource<ICollection<TEntity>>
{
    public ResourceCollection(ICollection<TEntity> data) : base(data)
    {
        //modify metadata if needed
    }

    public int Count => Data.Count;
}

While technically this is still two classes, you can now cast your collections to Resource<T> and process them in generic manner with your other resources.


Construct a tree given pre and inorder or post and inorder I want you to pick my code apart and give me some feedback on how I could make it better or simpler. For a full tree (all nodes with 0 or 2 children) it works deterministically:

public class Construct {

    private TreeNode root;

    private static class TreeNode {
        TreeNode left;
        TreeNode right;
        int item;

        public TreeNode(TreeNode left, TreeNode right, int item) {
            this.left = left;
            this.right = right;
            this.item = item;
        }
    }

    public void treeFromPreOrderAndInorder(int[] pre, int[] inorder) {
        if (pre == null || inorder == null) {
            throw new IllegalArgumentException("Your tree is not accurate");
        }
        if (pre.length != inorder.length) {
            throw new IllegalArgumentException("Your tree is not accurate");
        }
        // should this be overloaded + testing.
        root =  treeFromPreOrderAndInorder(pre, 0, inorder, 0, inorder.length - 1);
    }

    /**
     * Private functions are not overloaded.
     * Check binary0.
     */
    private TreeNode treeFromPreOrderAndInorder(int[] pre, int pos, int[] inorder, int lb, int hb) {
        if (lb > hb) return null;

        // step 1: fetch parent
        int parent = pre[pos];
        // step 2: fetch position
        int parentPosInorder;
        for (parentPosInorder = lb; parentPosInorder <= hb; parentPosInorder++) {
            if (inorder[parentPosInorder] == parent) {
                break;
            }
        }

        if (parentPosInorder > hb) {
            throw new IllegalArgumentException("Your tree is not accurate");
        }

        // proof in diagram : https://bitbucket.org/ameyapatil/pointstonote/commits/a3137e1e5c9f40f28c53b9c62e27f0e8bf81a000
        int numElemsInLeftSubTree = parentPosInorder - lb;

        /**
         * Basically absolutely same as construct Balanced BST from sorted array
         * with this extra junk to be added.
         */
        int leftParentPosPre = pos + 1;
        // proof in diagram : https://bitbucket.org/ameyapatil/pointstonote/commits/458d044f96226e8df1c47775dbe3c8c5ad9ecce4
        int rightParentPosPre = pos + numElemsInLeftSubTree + 1;

        // step 3: recurse
        TreeNode node = new TreeNode(null, null, parent);
        node.left = treeFromPreOrderAndInorder(pre, leftParentPosPre, inorder, lb, parentPosInorder - 1);
        node.right = treeFromPreOrderAndInorder(pre, rightParentPosPre, inorder, parentPosInorder + 1,  hb);

        return node;
    }

The algorithm looks good to me. Regarding the code, here are a few points:

You don't need to overload the function. Semantically it's a tree builder, and probably the private function can be termed as buildTree.
The following condition should throw the IllegalStateException instead of IllegalArgumentException:

if (parentPosInorder > hb) {
    throw new IllegalArgumentException("Your tree is not accurate");
}

You can make the code generic by using a generic data type for the node value.  Accordingly, the API will be extensible enough.


Python recursive function that receives list of strings, and returns a nested list of anagrams I'm learning Python and would appreciate any (and all) feedback to improve myself.
input/output example:
>>> list_of_words = ['deltas', 'retainers', 'desalt', 'pants', 'slated', 'generating', 'ternaries', 'smelters', 'termless', 'salted', 'staled', 'greatening', 'lasted', 'resmelts']

>>> sort_anagrams(list_of_words)
[['deltas', 'desalt', 'slated', 'salted', 'staled', 'lasted'], ['retainers', 'ternaries'], ['pants'], ['generating', 'greatening'], ['smelters', 'termless', 'resmelts']]

Here is the code I wrote, it worked well for the above list. Looking for feedback on complexity, overall design, readability and any other thing that pops to mind when reviewing, thanks!
# process list of strings, construct nested list of anagrams.

list_of_strings = ['deltas', 'retainers', 'desalt', 'pants', 'slated', 'generating', 'ternaries', 'smelters', 'termless', 'salted', 'staled', 'greatening', 'lasted', 'resmelts']

list_of_lists = []
temp_list = []

def sort_anagrams(list_of_strings):
    if len(list_of_strings) > 1:
        temp_list = [list_of_strings[0]] # create temp list to be eventually appended to main list
        popped_list = list_of_strings # list of anagrams to remove from source list after iteration, without deleting source list
        popped_list.pop(0) # remove the first item as it's already in the temp_list
        strings_to_pop = []

        for string in popped_list:
            if sorted(string) == sorted(temp_list[0]):
                temp_list.append(string) # passing anagram test so append to temp_list
                strings_to_pop.append(string) # create list of items to remove popped_list

        list_of_lists.append(temp_list) # append temp_list as a list element

        for string in strings_to_pop:
            popped_list.remove(string) # remove all strings that were deemed anagram to remove possible duplications

        sort_anagrams(popped_list)    # running the function again on list remainder

    elif len(list_of_strings) == 1:
        temp_list.append(list_of_strings[0])
        list_of_strings.pop(0)
        list_of_lists.append(temp_list) # append temp_list as a list element
        sort_anagrams(popped_list)

    else:
        print('you have reached the end and sorted all anagrams. here is the list of anagram lists: \n \n', list_of_lists)

sort_anagrams(list_of_strings)
```

I think a much simpler approach would be beneficial to you here, as well as if you were to utilize a dictionary to hold the results as you go along. The below code does not make use of recursion though, so if you were trying to learn about recursion in python then this would not help. But if you're just trying to learn what a simple and pythonic function to solve this problem would look like then I think this will help you. Notably it is much less code, easier to reason about, and does not make use of global variables.
def sort_anagrams2(list_of_strings):
    result = {}

    for string in list_of_strings:
        sorted_string = "".join(sorted(string))

        if sorted_string in result:
            result[sorted_string].append(string)
        else:
            result[sorted_string] = [string]

    return list(result.values())


Sorted grep in Haskell This program implements a sorted grep, that is, a specialized version of grep for sorted files. It uses binary search for the lines of a file that begin with a certain string.

You can copy and paste the code in a file and run it as:

$> runhaskell sgrep.hs "string to find" sorted_file

I'm looking for suggestions about style, efficiency, and correctness.

module Main where

import Data.List (isPrefixOf)
import Data.Maybe (isNothing, fromJust)
import System.Environment (getArgs)
import System.IO

-- Chunk of a file
data Chunk = Chunk Handle Integer Integer

-- Is char newline?
isNL :: Char -> Bool
isNL c = c == '\n'

-- Are we at the beginning of file?
isBOF :: Handle -> IO Bool
isBOF = (fmap (== 0)) . hTell

-- Go to beginning of line
goToBOL :: Handle -> IO ()
goToBOL h = do
        bof <- isBOF h
        if bof
           then return ()
           else do
                   eof <- hIsEOF h
                   if eof
                      then do
                              hSeek h RelativeSeek (-2)
                              goToBOL h

                      else do
                              c <- hGetChar h
                              if isNL c
                                 then return ()
                                 else do
                                         hSeek h RelativeSeek (-2)
                                         goToBOL h

getCurrentLine :: Handle -> IO String
getCurrentLine h = goToBOL h >> hGetLine h

getPrevLine :: Handle -> IO (Maybe String)
getPrevLine h = do
        goToBOL h
        bof <- isBOF h
        if bof
           then return Nothing
           else do
                   hSeek h RelativeSeek (-2)
                   goToBOL h
                   bof <- isBOF h
                   if bof
                      then return Nothing
                      else do
                              hSeek h RelativeSeek (-2)
                              goToBOL h
                              line <- hGetLine h
                              return $ Just line

goTo :: Handle -> Integer -> IO ()
goTo h i = do
        hSeek h AbsoluteSeek i

search :: Chunk -> String -> IO (Maybe String)
search (Chunk h start end) str
        | start >= end = return Nothing
        | otherwise = do
                if mid == (end - 1)
                   then return Nothing
                   else do
                           goTo h mid
                           midLine <- getCurrentLine h
                           prevLine <- getPrevLine h
                           --  putStrLn $ "*** " ++ show start ++ " " ++ show end ++ " " ++ show mid ++ " " ++ midLine ++ ", " ++ show prevLine
                           if str `isPrefixOf` midLine && ((isNothing prevLine) || not (str `isPrefixOf` (fromJust prevLine)))
                              then return $ Just midLine
                              else if str < midLine
                                      then search (Chunk h start mid) str
                                      else search (Chunk h mid end) str
           where mid = (start + end) `div` 2

sgrep :: Handle -> String -> IO ()
sgrep h s = do
        len <- hFileSize h
        match <- search (Chunk h 0 len) s
        --  putStrLn $ show match
        c <- hGetContents h
        putStrLn . unlines $ takeWhile (isPrefixOf s) (lines c)

main :: IO ()
main = do
        args <- getArgs
        let s = head args
        putStrLn s
        let fname = head $ tail args
        withFile fname ReadMode (\h -> sgrep h s)

Use your monads! Your code exhibits the walking-right antipattern. You can avoid it with when and guard. Consider goToBOL. This is how I would write it:

-- Go to beginning of line
goToBOL :: Handle -> IO ()
goToBOL h = do
        bof <- isBOF h
        when (not bof) $ do
        eof <- hIsEOF h
        if eof then do hSeek h RelativeSeek (-2)
                       goToBOL h
               else do c <- hGetChar h
                       when (not $ isNL c) $ do
                       hSeek h RelativeSeek (-2)
                       goToBOL h

In your other functions, namely getPrevLine and search, you'd better use MaybeT IO x instead of IO (Maybe x) as you can use the monadic combinators better when you do so.


Elastic Beanstalk configuration using ebextensions container_commands combining leader_only and test For our Django app running on AWS Elastic Beanstalk, we use a web-server/worker setup. The same code is deployed to the web-server and worker environments, and environment-specific configuration is achieved with .ebextensions config files, using the test option for container_commands, among other things.

Now there are some container commands we would like to execute only during deployment or environment-creation (so we use leader_only: true, see bottom), but, in addition, they should never be executed on the worker environment.

The latter could be achieved using the test option, but that cannot be used not in combination with leader_only, according to the docs:

  ... A command can be leader-only or have a test, but not both (leader_only takes precedence).

Now, to work around this, we do something like the following, in .ebextensions/foo.config:

option_settings:
  aws:elasticbeanstalk:application:environment:
    ENV_NAME: '`{ "Ref" : "AWSEBEnvironmentName" }`'  # get the current environment name

container_commands:
  0100_set_flag_if_instance_is_leader:
    command: touch /tmp/is_leader  # create empty file
    leader_only: true
  0200_execute_command_on_leader_only_but_never_on_worker:
    command: foo bar
    test: '[[ $ENV_NAME != "worker" && -e /tmp/is_leader ]]'

where ENV_NAME contains the environment name, which is automatically retrieved with the help of option_settings (see docs).

Note that the config file is in YAML format and the commands and tests execute in a shell (I assume bash, since the extended test construct [[ works, but could not find any specifics in the AWS docs).

As I am not an expert, I would really appreciate your comments on the following points:

The solution above works, but is this the simplest way to do it?
Are there any compelling reasons not to use this kind of set-up?
I tried setting a variable instead of creating a file (in command 0100), but the variable does not appear to persist between container commands. Is there some way to achieve this? (I tried to export, but that didn't help. Is each container command executed in a separate shell, or how does this work?)

Some general background from the AWS docs:

  You can use leader_only to only run the command on a single instance, or configure a test to only run the command when a test command evaluates to true. Leader-only container commands are only executed during environment creation and deployments, while other commands and server customization operations are performed every time an instance is provisioned or updated. Leader-only container commands are not executed due to launch configuration changes, such as a change in the AMI Id or instance type.

It turns out there is an environment variable called EB_IS_COMMAND_LEADER which can be used in a test. See e.g. this post on AWS forums.
So, the two container commands from the OP can be replaced by this:
container_commands:
  0100_execute_command_on_leader_only_but_never_on_worker:
    command: foo bar
    test: '[[ $ENV_NAME != "worker" && $EB_IS_COMMAND_LEADER == "true" ]]'

I believe this environment variable is only available during deployment, although I could not find any documentation.
NOTE: The original question was aimed at Amazon Linux AMI (AL1), which is now deprecated. In the new Amazon Linux 2 we can use shell scripts in .platform/hooks/ as an alternative to container_commands. The EB_IS_COMMAND_LEADER variable can be used there as well.


Determining whether (x,y) points in the input constitute a function I'm learning Haskell, and I've finally gotten around to coding up some Haskell. This code passed my tests.

This code takes a line from standard in which tells it how many cases it's going to have (1 to 5), and for each case, it takes a line telling it how many pairs (max of 100) of x's and y's it will get (integers from 0 to 500), and it should print YES if x to y is a function, and NO otherwise.

e.g. with a file called input:

2
2
1 1
2 1
2
1 1
1 2

The following command should work like this:

$ runhaskell main.hs < input
YES
NO

In my source code the only difference is I have two blank lines for style instead of one between each group of code. My main.hs:

import Control.Monad (replicateM_, replicateM, forM)
import Data.String (words)

main = do
  times <- readLn :: IO Int
  replicateM_ times input_is_a_function

input_is_a_function = do
  n_inputs <- readLn :: IO Int
  inputs <- replicateM n_inputs getLine
  if (null inputs) || (is_function inputs) then
    putStrLn "YES"
  else
    putStrLn "NO"

is_function inputs = is_func ([map read $ words i | i <- inputs] :: [[Int]])

is_func (x_y : rest)
  | rest == [] = True
  | otherwise  = (no_other_y x_y rest) && (is_func rest)

no_other_y [x1, y1] rest
  | rest == [] = True
  | otherwise  = this_y_ok && (no_other_y [x1, y1] (tail rest))
      where
        this_y_ok = not (x_is_target && y_not_equal)
        x_is_target = x1 == head (head rest)
        y_not_equal = y1 /= ((head rest) !! 1)
        -- if target is false, y is ok.
        -- if target is True, y must be equal

The algorithm starts with the first pair of x and y, and does a search for a repetition of x and determines if it matches y or not. Any found x with a mismatch on y should stop the algorithm.

I come from a Python background, so I tend to use snake-case and idioms I am more immediately familiar with (list comprehensions FTW!), but I want to learn idiomatic Haskell.

I also value readability. I could probably have better names. Perhaps more typing would improve readability, but I tried to take as much advantage of Hindley-Milner type inference as I could.

Please address efficiency, typing, style, naming, or any other important issues you identify.

On my workflow: I've been using a basic ghc (7.10.3) from the Ubuntu repos with a test file and that command mentioned above.

Is there a better approach? e.g. a way to watch the file and run tests as I code and save? In Haskell?

Let's condense this down. What you're trying to check is that for any given x, the y values you're told are all the same.

Now let's try to express this as succinctly as possible:

Let's start with a few simplifying assumptions. All your x and y values are going to be integers. Furthermore, let's assume we handle them as Tuples and not as Lists. This enables a somewhat cleaner signature for our types

We start with a list of points and in the end we want a boolean to drop out. Our type signature accordingly looks like this:

isFunction :: [(Int, Int)] -> Bool

This function can be decomposed into a few steps. First we need to group by the x values.

isFunction points = -- we'll fill this in later
   where
   grouped = groupWith fst points -- fst accesses the first item in a tuple

now that we have all points grouped, we need to examine their y values

   ys = map (map snd) grouped -- snd gets the second item in a tuple

And now we want to make sure that every group of y values is always the same value.
For this I'll use a bit of a hack

   areEqual = map (\l -> all (== (head l)) (tail l)) ys

What this does is a bit hard to grasp at first glance, but types will help us understand.

Let's fill back the types for all the stuff we had until now:

grouped :: [[(Int, Int)]]  -- List of List of Points
ys :: [[Int]]  -- List of List of Integers

Now let's examine what this does in areUnique. We know that none of the lists in ys are empty. That's very useful for accessing the first element in the list and setting it as baseline for the rest of the list we're examining. All items in a single list (and accordingly belonging to a single x) must be pairwise equal. This explicitly means they must be equal to the first item.

That's how we can use all (== (head l)). That line could also be written as the following:

   areEqual = map (\yGroup -> all (== yGroup!!0) yGroup)

Now we have in areUnique a [Bool], each of them indicating, whether the group is consistent.

That allows us to formulate the final result of isFunction we omitted above:

isFunction points = all id areEqual

So now we have a pretty function that will do the work of your is_func, including all the work of no_other_y. We need to import GHC.Exts for it to work, because it provides groupWith. If you don't have GHC on your system, you will need to write a replacement, but that's probably a good exercise in itself. Only that much: sorting before grouping helps a lot.

A nice sideeffect of this is that we're reducing the time-complexity from \$\mathcal{O}(n^2)\$ to \$\mathcal{O}(n \log n)\$

Now aside from this move away from list comprehension into a more explicit model that should additionally be quite a bit faster, there's not that many things to say about your code.

Granted, you largely ignore types (which is something that I personally do exactly the opposite of) and you're using python naming conventions, which only clashes with readLn, getLine and replicateM_.

This is what we got for now:

import GHC.Exts  (groupWith)

isFunction :: [(Int, Int)] -> Bool
isFunction points = all id areEqual
   where
   grouped = groupWith fst points
   ys = map (map snd) grouped
   areEqual = map (\l -> all (== l!!0) l) ys

Now let's get a bit fancier. We can inline a few of these definitions:

areEqual = map (\l -> all (== l!!0) l) (map (map snd) grouped)

notice the repeated use of map. We can get around that by using function composition (.):

areEqual = map ((\l -> all (== l!!0) l) . (map snd)) grouped

interestingly it's not necessary to map to the y value alone. While semantically appropriate, the x values already should be the same. This allows us to drop the mapping:

areEqual = map (\l -> all (== l!!0) l) grouped

Now we can inline grouped to obtain (which uses $, the self closing parenthesis):

areEqual = map (\l -> all (== l!!0) l) $ groupWith (fst) points

And to top it off, this can be inlined into our all, which gets us to:

isFunction points = all id $ (map (\l -> all (== l!!0) l)) $ groupWith fst points

At this point we replace our fancy self-closing braces with function composition to obtain

isFunction points = (all id) . (map (\l -> all (== l!!0) l)) . (groupWith fst) points

which can then be reduced to:

isFunction = (all id) . (map (\l -> all (== l!!0) l)) . (groupWith fst)

That in turn can be simplified to (hat tip to @Gurkenglas):

isFunction = all (\l -> all (== l!!0) l) . groupWith fst


Concatenate function arguments with "AND" string if they're not None. Basically, if the function parameter is None, I ignore it, else I concatenate all arguments like arg1:arg1_value AND arg2:arg2_value AND arg4:arg4_value considering arg3==None when the function got called.

def _get_query_string_for_get_config(self, host, app_name, instance, config_source, config_name):
    query_string = ""
    if host is not None:
        query_string += "host:{} AND ".format(host)
    if app_name is not None:
        query_string += "app_name:{} AND ".format(app_name)
    if instance is not None:
        query_string += "instance:{} AND ".format(instance)
    if config_source is not None:
        query_string += "config_source:{} AND ".format(config_source)
    if config_name is not None:
        query_string += "config_name:{}".format(config_name)

    return query_string

Concatenating strings with a common connector (such as ' AND ' here) is most idiomatically done using join. So the idea should be to:

return ' AND '.join(<some list with the right `text:value` formatting>)

The easiest way to get such list should be to pair the text part and the associated value using zip and then use a list comprehension to filter out undesired variables:

parameters = zip(
    ('host', 'app_name', 'instance', 'config_source', 'config_name'),
    (host, app_name, instance, config_source, config_name)
)
query = ['{}:{}'.format(text, value) for text, value in parameters if value is not None]
return ' AND '.join(query)

You can also simplify a bit by turning the list-comprehension into a generator expression and feeding it directly to join.

Lastly, the self parameter suggest that you are using this as a method of a class. But since you are not using self in the method, you should either make it a regular function or a saticmethod:

@staticmethod
def _get_query_string_for_get_config(host, app_name, instance, config_source, config_name):
    parameters = zip(
        ('host', 'app_name', 'instance', 'config_source', 'config_name'),
        (host, app_name, instance, config_source, config_name)
    )
    return ' AND '.join(
        '{}:{}'.format(text, value)
        for text, value in parameters
        if value is not None)


Converting any CSS colour to RGB(a) For a plugin I'm writing in jQuery, I have two optional parameters. For each parameter I do a check for its value. However, I'm curious if I can't write it shorter.

jsFiddleiddle

(function ($) {
    $.rgbGenerator = function (color, options) {
        var args = $.extend({
            returnAsObj: false,
            addAlpha: false
        }, options);

        var d = $("<div>");
        d.css("color", color).appendTo("body");
        var rgb = d.css("color");
        rgb = rgb.replace(/(^rgba?\(|\)$|\s)/g, "").split(",").map(Number);
        d.remove();
        if (args.addAlpha === false) {
            if (rgb.length == 3) {
                if (!args.returnAsObj === true) {
                    return "rgb(" + rgb[0] + "," + rgb[1] + "," + rgb[2] + ")";
                } else {
                    return {
                        r: rgb[0],
                        g: rgb[1],
                        b: rgb[2]
                    };
                }
            } else if (rgb.length == 4) {
                if (!args.returnAsObj === true) {
                    return "rgba(" + rgb[0] + "," + rgb[1] + "," + rgb[2] + "," + rgb[3] + ")";
                } else {
                    return {
                        r: rgb[0],
                        g: rgb[1],
                        b: rgb[2],
                        a: rgb[3]
                    };
                }
            }
        } else {
            if (!args.returnAsObj === true) {
                return "rgba(" + rgb[0] + "," + rgb[1] + "," + rgb[2] + "," + args.addAlpha + ")";
            } else {
                return {
                    r: rgb[0],
                    g: rgb[1],
                    b: rgb[2],
                    a: args.addAlpha
                };
            }
        }
    };
})(jQuery);

Basically, what it does is:

Check args.addAlpha whether it's false (default)
Check the length of rgb (when it's 3 it's RGB, when it's 4 it's rgba)
Check args.returnAsObj. If it's set to true, return an object rather than a string

Especially the part where I check the length of rgb seems unnecessary, but I'm not sure how I could write it any other way. Something like this would be nice:

return {
    r: rgb[0],
    g: rgb[1],
    b: rgb[2],
    a: function () {
        if (rgb.length == 4 || !args.addAlpha === false) {
            return rgb[3] || args.addAlpha;
        }
    }
};

But I assume that's not possible.

You are right that returning an object where a is a function would not work, but it would work to return the result of a call to a function!

You can also restructure things a bit to reduce duplicated code. By having a starting string or a starting object, and optionally adding to that string or object if some conditions are fulfilled.

This is untested code, but I think it should do the same as your original.

function result(addAlpha, rgb, asObject) {
    if (asObject) {
        var obj = { r: rgb[0], g: rgb[1], b: rgb[2] };
        if (addAlpha !== false) {
            obj.a = addAlpha;
        }
        else if (rgb.length == 4) {
            obj.a = rgb[3];
        }
        return obj;
    }

    var a = (addAlpha !== false) || (rgb.length == 4) ? "a" : "";
    var alpha = "";
    if (addAlpha !== false) alpha = "," + addAlpha;
    else if (rgb.length == 4) alpha = "," + rgb[3];
    return "rgb" + a + "(" + rgb[0] + "," + rgb[1] + "," + rgb[2] + alpha + ")";
}


How can this  be set up to maximize inheritance if additional classes will be passed in from a JSON file? Objective

Currently I have the flags hardcoded. But want to optimize my code for production. So I am wondering if I have this code done clean enough to leverage CSS inheritance while maintaining support for older browsers.

I have this menu on my CodePen

whose size will vary per device (media queries not shown yet). I restructured my code from
's to this

HTML

North America

On the div with the flag, I want to be able to pass its CSS rules without giving it the class of intlFlag and for the intlButton I want to also pass rule without giving it the class. I hesitated to use the :last-of-type selector because it is not supported below IE8, and many users of the site will unfortunately use IE8.

Image from w3schools.com

As far as the first
 I want the class to be passed in based on what country they are detected to be coming from. So the script would for example detect that you are in North America. And then pass in that class which then sets your flag. In production I will be using MustacheJS templates. So I plan to do something like this

{{region}}

my JSON values might be something like

"flag":"usFlag",
"region":"North America"

That usFlag class would add this class to display the respective flag

.usFlag { background-position: 0px 0px; }

the other rules passed into that
 are currently passed in with

.intlFlag {
  background-color: #fff;
  background-image: url('https://s3-us-west-2.amazonaws.com/s.cdpn.io/101702/flag-sprite.png');
  height: 28px;
  width: 44px;
}

Here are the complete set of my CSS rules for this element

.international {
  background-color: rgba(96, 80, 80, 0.3);
  font-size: 18px;
  line-height: 42px;
  list-style-type: none;
  margin: auto;
  padding-left: 0;
  position: absolute;
  bottom: 0; left: 0; right: 0;
  width: 100%;
}
/* only on tablet */
.international { max-width: 240px; }

.international > li {
  display: inline-block;
  line-height: 42px;
  margin-left: 6px;
  vertical-align: middle;
}

.intlFlag {
  background-color: #fff;
  background-image: url('https://s3-us-west-2.amazonaws.com/s.cdpn.io/101702/flag-sprite.png');
  height: 28px;
  width: 44px;
}

.usFlag { background-position: 0px 0px; }

.intlButton {
  float: right;
  font-size: 24px;
  margin-right: 6px;
}
.intlButton a { color: #444; }
.intlButton i {
  background-color: rgba(225, 225, 225, .7);
  border-radius: 3px;
  padding: 4px;
}

Some quick notes:

You can use list-style: none; instead of list-style-type: none; – Just a bit shorter
You mention in a comment /* only for tablet */, but there are no media queries in sight.

@media screen and (max-width: 480px) {
    .international {
        max-width: 240px;
    }
}

It's common practise to use dash-delimited class names in CSS, not CamelCase. This completely personal and you can do whatever you want.
Omit the units on zero values: background-position: 0 0; instead of background-position: 0px 0px;
I recommend writing single-line CSS: Every property declaration on a new line. There is no reason to write rule declarations with only one property declaration on one line. Also I don't find it helpful writing top, right, bottom and left on one line.

If you want to save file size and improve performance, you should minimize your CSS. That's so much more efficient plus you keep the legibility and maintainability in your CSS file.

That's it for the moment. Feel free to add a comment to discuss something or visit our great chat.


Parsing user name and password from binary in Erlang + Cowboy I am working on a web app that uses a WebSocket to log in users. I would like to minimize bandwidth usage by using binary messages. I came up with the following, and would like to know whether there is something better I could be doing:

-define(LOGIN, 1).

websocket_handle({binary, <<?LOGIN:8,LengthUser:8, Rest/bitstring>>}, Req, State) ->
    LU2 = LengthUser*8,
    <<Username:LU2/bitstring, Password/bitstring>> = Rest,
    % Do the actual logging in...

I did a decent amount of research, and couldn't find a way to avoid using the Rest piece in order to accommodate variable length bitstring patterns. The web app uses an ArrayBuffer and a few DataViews in order to construct the message.

Your approach is necessary because of the need to extract the length of the Username field before using it to extract the Username field itself. However, you needn't use bitstring anywhere because you're only sending whole bytes, not bitfields. You can change all occurrences of bitstring to binary instead, and with that change you also no longer need to specify the length of Username in bits, so you no longer need LU2 and can just use LengthUser directly:

-define(LOGIN, 1).

websocket_handle({binary, <<?LOGIN:8,LengthUser:8, Rest/binary>>}, Req, State) ->
    <<Username:LengthUser/binary, Password/binary>> = Rest,
    %% Do the actual logging in...

And finally, note that by convention, Erlang comments on a line by themselves at the current indentation level start with %%, not %. Single % should be used only for end-of-line comments. Not sure about other editors but the emacs erlang-mode indents comments this way, so if an emacs user worked on your code your comments might get wildly indented if the emacs developer auto-indented the buffer and didn't check everything afterwards.


A "safe" copy function for Python I have a Python script for doing "safe" file copying.

There are a couple of rules for this copy function:

Existing files should never be overwritten. So if I try to copy $file1 to $location, and there is already $file2 at $location, then $file2 will not be overwritten.
Exact filenames are not important. So if I try to copy a file to $location.txt, and because there’s already a different file there, it instead gets copied to $location-1.txt, that’s okay.
Copying should be idempotent. That is, if I try to copy a file to $location, and it's already there, nothing else happens. I'm not going to create more copies at $location-1, $location-2, $location-3, etc.

An example use case: I'm importing some camera photos into a folder, but some of the photos may be duplicates (and I don't want two copies) or have the same filename as another photo (and I want to keep both, but small changes to the name aren't important).

import filecmp
import os
import shutil

def increment_filename(filename, marker="-"):
    """Appends a counter to a filename, or increments an existing counter."""
    basename, fileext = os.path.splitext(filename)

    # If there isn't a counter already, then append one
    if marker not in basename:
        components = [basename, 1, fileext]

    # If it looks like there might be a counter, then try to coerce it to an
    # integer and increment it. If that fails, then just append a new counter.
    else:
        base, counter = basename.rsplit(marker, 1)
        try:
            new_counter = int(counter) + 1
            components = [base, new_counter, fileext]
        except ValueError:
            components = [base, 1, fileext]

    # Drop in the marker before the counter
    components.insert(1, marker)

    new_filename = "%s%s%d%s" % tuple(components)
    return new_filename

def copyfile(src, dst):
    """Copies a file from path src to path dst.

    If a file already exists at dst, it will not be overwritten, but:

     * If it is the same as the source file, do nothing
     * If it is different to the source file, pick a new name for the copy that
       is distinct and unused, then copy the file there.

    Returns the path to the copy.
    """
    if not os.path.exists(src):
        raise ValueError("Source file does not exist: {}".format(src))

    # Create a folder for dst if one does not already exist
    if not os.path.exists(os.path.dirname(dst)):
        os.makedirs(os.path.dirname(dst))

    # Keep trying to copy the file until it works
    while True:

        # If there is no file of the same name at the destination path, copy
        # to the destination
        if not os.path.exists(dst):
            shutil.copyfile(src, dst)
            return dst

        # If the namesake is the same as the source file, then we don't need to
        # do anything else
        if filecmp.cmp(src, dst):
            return dst

        # There is a namesake which is different to the source file, so pick a
        # new destination path
        dst = increment_filename(dst)

    return dst

Particular areas I’m unsure about:

Does it make sense? Is it clear what the code is doing?
Can I replace any of this with standard library functions?
I wrote it in Python 2, but will run it in 2 and 3. Brief testing suggests that it runs fine in Python 3, but pointers to anything I've missed would be helpful.
Is this good Python?

As the documentation for shutil.copyfile(src, dst) says,

  If dst already exists, it will be replaced.

You have attempted to work around that by checking if not os.path.exists(dst) first.  However, that logic is vulnerable to a race condition if the destination file springs into existence just after the check.

A way to ensure that you do not overwrite an existing file is to use os.open() with the os.O_EXCL flag.  You would need to write the read-write loop yourself to transfer the file contents, but that is not difficult.


Review internet connection pinging method This is some C code I have to simply test the internet connection.  Any comments/tips on efficiency and refactoring this program down would be greatly appreciated.

int testConnection(void)
{
    int status;
    struct addrinfo host_info;
    struct addrinfo *host_info_list;
    memset(&host_info, 0, sizeof host_info);
    #ifdef DEBUG
    fprintf(stdout,"Setting up the structs...");
    #endif
    host_info.ai_family = AF_UNSPEC;     // IP version not specified. Can be both.
    host_info.ai_socktype = SOCK_STREAM; // Use SOCK_STREAM for TCP or SOCK_DGRAM for UDP.

    status = getaddrinfo("www.google.com", "80", &host_info, &host_info_list);
    if (status != 0)  fprintf(stdout, "Address info error:: %s\n", gai_strerror(status));

    #ifdef DEBUG
    fprintf(stdout, "Creating a socket...\n");
    #endif
    int socketfd ;
    socketfd = socket(host_info_list->ai_family, host_info_list->ai_socktype, host_info_list->ai_protocol);
    if (socketfd == -1)  fprintf(stderr, "Socket error\n");

    #ifdef DEBUG
    fprintf(stdout, "Connecting...");
    #endif
    status = connect(socketfd, host_info_list->ai_addr, host_info_list->ai_addrlen);
    if (status < 0) fprintf(stderr, "Error while connecting.\n");

    #ifdef DEBUG
    fprintf(stdout, "Sending message...\n");
    #endif
    const char *msg = "GET / HTTP/1.1\nhost: www.google.com\n\n";
    int len = strlen(msg);
    ssize_t bytes_sent = send(socketfd, msg, len, 0);
    if (bytes_sent == 0) fprintf(stderr, "No bytes sent.\n");
    #ifdef DEBUG
    fprintf(stdout, "Bytes sent: %d\n", bytes_sent);
    fprintf(stdout, "Waiting to recieve data...\n");
    #endif
    char incomming_data_buffer[1000];
    ssize_t bytes_recieved = recv(socketfd, incomming_data_buffer,1000, 0);
    // If no data arrives, the program will just wait here until some data arrives.
    if (bytes_recieved == 0) fprintf(stderr, "Host shut down.\n");
    if (bytes_recieved == -1) fprintf(stderr, "Recieve error.\n");
    incomming_data_buffer[bytes_recieved - 2] = '\0';
    #ifdef DEBUG
    fprintf(stdout, "Bytes recieved: %d\n", bytes_recieved);
    fprintf(stdout, "%s\n", incomming_data_buffer);
    fprintf(stdout, "Receiving complete. Closing socket...\n");
    #endif
    freeaddrinfo(host_info_list);
    close(socketfd);
    #ifdef DEBUG
    fprintf(stdout, "Socket closed.\n");
    #endif
    return 0;
}

Some comments, mostly minor:

I would extract the
server connection to a function:

static int connect_server(const struct addrinfo *host_info)
{
    struct addrinfo *host_info_list;
    int fd = -1;
    int status = getaddrinfo(...);
    while(...) {
        fd = socket(...);
        status = connect(...);
    }
    freeaddrinfo(host_info_list);
    return fd;
}

All that DEBUG stuff is distracting.  Maybe it is temporary, but if you
wanted to leave it in, I suggest extracting it:

#include <stdarg.h>

static inline void debug(const char *format, ...)
{
#ifdef DEBUG
    va_list ap;
    va_start(ap, format);
    vfprintf(stdout, format, ap);
    va_end(ap);
#endif
}

and calling it:

debug("Bytes recieved: %ld\n%s\nReceiving complete. Closing socket...\n",
      bytes_recieved,
      incomming_data_buffer);

If DEBUG is undefined, the inline debug function will be empty and will
be excluded during compilation - it disappears.

You clearly need to loop to read the whole message...  After reading you
throw away the last two bytes.

buffer[bytes_recieved - 2] = '\0';

The recv call filled the buffer, so to \0 terminate properly you need to
specify a smaller buffer:

char buffer[1000];
ssize_t bytes_recieved = recv(socketfd, buffer, sizeof buffer - 1, 0);
...
if (bytes_recieved > 0) {
    buffer[bytes_recieved] = '\0';
}

Note the use of sizeof instead of an explicit 1000

And some other things...

The man-page for getaddrinfo suggests looping through the list of
addresses returned instead of just using the first (in case the first does
not work).
When something fails you should exit rather than continuing.
instead of using strlen on a constant string, use sizeof:

const char msg[] = "GET / HTTP/1.1\nhost: www.google.com\n\n";
ssize_t bytes_sent = send(socketfd, msg, sizeof msg - 1, 0);

note the msg[], not *msg
perhaps use perror or strerror on failure to read etc.
camelCase function name but separate_word names elsewhere.


Follow Up post: Proper handling of exceptions in MVP This is a follow up post of my early question "Proper handling of exceptions in MVP"

So based on the answers I re edited the post and now my exception handling code looks like this...

DAL

    public bool InsertAccount(IBankAccount ba)
    {
string insertStatement = @"IF NOT EXISTS (SELECT ac_no FROM BankAccount WHERE ac_no=@ac_no) BEGIN INSERT INTO BankAccount (ac_no, emp_id, ac_name, bank_name, ac_type," +
                                  "ent_date, ent_by, remarks, file_no, status) VALUES (@ac_no, @emp_id, @ac_name, @bank_name, @ac_type, @ent_date, @ent_by, @remarks, @file_no, @status) END";

        using (SqlConnection sqlConnection = new SqlConnection(db.ConnectionString))
        {
            using (SqlCommand sqlCommand = new SqlCommand(selectStatement, sqlConnection))
            {
                try
                {
                    sqlConnection.Open();
                    sqlCommand.Parameters.Add("@ac_no", SqlDbType.Char).Value = ba.AccountNumber;
                    sqlCommand.Parameters.Add("@emp_id", SqlDbType.Int).Value = ba.EmployeeID;
                    sqlCommand.Parameters.Add("@ac_name", SqlDbType.VarChar).Value = ba.AccountName;
                    sqlCommand.Parameters.Add("@bank_name", SqlDbType.VarChar).Value = ba.BankName;
                    sqlCommand.Parameters.Add("@ac_type", SqlDbType.VarChar).Value = ba.AccountType;
                    sqlCommand.Parameters.Add("@ent_date", SqlDbType.DateTime).Value = ba.EnteredDate;
                    sqlCommand.Parameters.Add("@ent_by", SqlDbType.VarChar).Value = "ABCD";
                    sqlCommand.Parameters.Add("@remarks", SqlDbType.VarChar).Value = ba.Remarks;
                    sqlCommand.Parameters.Add("@file_no", SqlDbType.Int).Value = ba.FileNo;
                    sqlCommand.Parameters.Add("@status", SqlDbType.Bit).Value = ba.Active;

                    sqlCommand.ExecuteNonQuery();
                    return true;
                }
        catch (SqlException)
        {
            throw new Exception("DataBase related Exception occurred");
        }
        catch (Exception)
        {
            throw new Exception("Exception occurred");
        }
    }

BankAccountPresenter

    private void SaveBankAccount()
    {
        try
        {
           _DataService.InsertAccount(_model);
        }
        catch (Exception e) { MessagingService.ShowErrorMessage(e.Message); }
    }

Why I've caught exceptions in DAL is that even at the moment I'm not logging errors, I may have to do it in future.

And also this way I can differentiate the error massages in DAL, whether it's sql related or general.

I've used messaging service in my presenter when showing error messages.

Does this meaning full? Can this be simplified?

I can follow the reasoning behind the catch and rethrow even if I might not fully agree with it - without a fuller understanding of what you would like to achieve I will not try to offer an alternative.

The only comments I can see at the moment are:

HouseKeeping
InsertAccount() still has a return value even though it is never used.  It can/should be removed.

Don't Repeat Yourself (DRY)
If you have a lot of calls to database, you do not want to have to repeat the exception handling (logging) code in each of them.

At the simplest level you can have a member in the data service that takes in an Action, executes it and handles any exceptions

private void ExecutNonQuery(Action nonQueryAction) {
    try {
       nonQueryAction();
    }
    catch (SqlException) {
        throw new Exception("DataBase related Exception occurred");
    }
    catch (Exception) {
        throw new Exception("Exception occurred");
    }
}

public void InsertAccount(IBankAccount ba)    {
  ExecuteNonQuery(()=> {
         string insertStatement = @"IF NOT EXISTS (SELECT ac_no FROM BankAccount WHERE ac_no=@ac_no) BEGIN INSERT INTO BankAccount (ac_no, emp_id, ac_name, bank_name, ac_type," +
                              "ent_date, ent_by, remarks, file_no, status) VALUES (@ac_no, @emp_id, @ac_name, @bank_name, @ac_type, @ent_date, @ent_by, @remarks, @file_no, @status) END";

        //...
  });
}

As I said, this is a minimal version.  The next level is to refactor out the common code for connection and command creation (a bit more work but it can be done).

MessageService
As I said, I favour the MessageService approach but with one caveat.  It is not obvious from the code if MessageService.ShowErrorMessage() is a static method on a static class (equivalent to the call the MessageBox) or if MessageService is a property of the class containing SaveBankAccount().  If a static, you still have the problem with the unit tests as a MessageBox is still shown each time.


PHP code to scale images and generate thumbnails I have written a piece of code by taking help from this link. For generating thumbnail, I create a temporary scaled image and then copy from that image. I am assuming that it is resource intensive.

Settings: the maximum values for target dimensions of scale images are predefined. Also, the thumbnail is always square and its side's value is also predefined.

The logic for my code is:
Maximum Target dimensions - 700X524
Thumbnail - 90X90

Get source image dimensions
Check if either of height or width is larger than 524 & 700 respectively
If yes,
Check if height> width.If yes,set height=524 and scaled width;else set width=700 and scale height.
create true color image from imagecreatettruecolor.
Generate scaled image with new dimensions from the original image

For thumbnail:

Check if either of scaled height and width is greater than 90
If yes, scale the shorter side to 90 and longer side accordingly.
Create a true color canvas of the further scaled dimensions.
Create a short image with the shorter side being 90 and larger one scaled accordingly
Create a true color canvas of dimensions 90X90
Copy thumbnail image to this canvas from the recently created shorter scaled image.

The code:

<?php

// default dimensions
//height : 524px
//width: 700px

$source_image = imagecreatefromjpeg("bg.jpg");
$source_imagex = imageSX($source_image);
$source_imagey = imagesy($source_image);

//destination image size calculation, should not exceed the target

//check if height and width are larger than required and then scale
if($source_imagey>524 || $source_imagex > 700) {
if($source_imagey>$source_imagex){
   $new_height = 524;
       $new_width = 524/$source_imagey*$source_imagex;
}else{
   $new_width = 700;
       $new_height = 700/$source_imagex*$source_imagey;
    }
 }

 $dest_image = imagecreatetruecolor($new_width, $new_height);

 //poor quality but fast
 imagecopyresized($dest_image, $source_image, 0, 0, 0, 0, $new_width,
 $new_height, $source_imagex, $source_imagey);
 imagejpeg($dest_image,"final.jpg", 80);

 //better quality but slow
 imagecopyresampled($dest_image, $source_image, 0, 0, 0, 0, $new_width,
 $new_height, $source_imagex, $source_imagey);
 imagejpeg($dest_image, 'final2.jpg', 80);

 //create square thumbnail
 // resize image to scale shorter side to 90px
 if($new_width>90 || $new_height>90) {
    if($new_height>$new_width)
    {
       $thumb_height = (90/$new_width)*$new_height;
       $thumb_width = 90;
       $top = ($tHeight - 90)/2;
       $left = 0;
    }else{
   $thumb_width = (90/$new_height)*$new_width;
   $thumb_height = 90;
       $left = ($thumb_width-90)/2;
       $top = 0;
    }
 }
 $image_t = imagecreatetruecolor($thumb_width,$thumb_height);
 imagecopyresampled($image_t, $dest_image, 0, 0, 0, 0, $thumb_width,
 $thumb_height, $new_width, $new_height);

 $thumb_image = imagecreatetruecolor(90,90);
 imagecopy($thumb_image, $image_t, 0, 0, $left, $top,
 $thumb_width,$thumb_height);
 imagejpeg($thumb_image, 'thumb.jpg', 80);

 imagedestroy($thumb_image);
 imagedestroy($dest_image);
 imagedestroy($image_t);

Is this approach of creating a temporary image of smaller size and then copying thumbnail from it, correct ? Is there an alternative to this ? How can it be further optimized ?

UPDATE: The class that I have created, with the above code and some help and insight gained from the link at nettuts, can be found on github

Please don't count this as an answer. I've never done anything with any of the image libraries before, so I can't help you with the specifics. I would have put this into a comment, but its obviously too big. However, here are a few things to ponder.

First, in regards to the comments Keiran Lovett made, a class may make this a little simpler to work with, but it may also add an unnecessary level of overhead or abstraction. This seems like a relatively simple application, thus the overhead and abstraction that a class would introduce may not be worth it. It depends on a number of things, but the main ones I would contemplate are if I were going to be doing a lot of different things with these images, or how much simpler or faster/slower it would make it. Either way, I definitely would start using functions, and then if you decide to use a class it will make the transition easier.

Your maximum width and height should be made into constants, or at the very least variables. This avoids the "magic numbers" and makes it easier to use and update these values should you ever decide to change them.

define( 'MAX_Y', 524 );
define( 'MAX_X', 700 );

How do you treat images that have been rotated? An image flipped to a different aspect ratio would result in a smaller image on one axis and a skewed thumbnail. Or at least, so it would seem to me. I'm not sure how to do this, but it is something you may want to look in to.

And finally, in regards to how memory intensive this is going to be, I don't know. The best way to find out would be to run some tests and profile it. Create 100 (or more) oversize images and run the necessary code in a loop to convert each one. This is where having functions would be handy.

$start = microtime( TRUE );
for( $i = 0; $i < 100; $i++ ) {
    //functions to scale and convert
}
$total = microtime( TRUE ) - $start;


Determining if a set of 2d vectors is a valid function My idea was to take in the function and perform a self cross product so that I could check every pair against every other pair. I'm new to Haskell so this solution is probably rough.

My question is, is there a better way to emulate a nested for loop rather than a cross product? Also, is there any way to collapse my ugly valid and valid' into one clean function?

valid :: [(Int, Int)] -> Bool
valid f = valid' $ [(x,y) | x <- f, y <- f]

valid' :: [((Int,Int),(Int,Int))] -> Bool
valid' [] = True
valid' (((x,y),(x1,y1)):xs) = valid' xs && if x == x1 then (if y == y1 then True else False) else True

To start, if y == y1 then True else False can be simplified to y == y1:

valid' (((x,y),(x1,y1)):xs) = valid' xs && if x == x1 then y == y1 else True


3D plot from lm model with a nonlinear parameter I am trying to plot a 3Dplot of a linear regression with two variables.

My linear model:

ls = lm(mpg ~ disp + qsec, data = mtcars)

The minimum and maximum values to predict:

mmd = min(mtcars$disp):max(mtcars$disp)
mmq = min(mtcars$qsec):max(mtcars$qsec)

The linear function:

flm = function(x1, x2) ( (ls$coefficients[1]) + (ls$coefficients[2] * x1) + (ls$coefficients[3] * x2))

The outer matrix:

t = outer(X = mmd, Y = mmq, FUN = flm)

The plot:

persp3D(z = t, ticktype = "detailed")

Now I would like to control for the non-linearity of the qsec parameter.

Is this the right way to do it?

ls = lm(mpg ~ disp + poly(qsec, degree = 2), data = mtcars)
mmd = min(mtcars$disp):max(mtcars$disp)
mmq = min(mtcars$qsec):max(mtcars$qsec)

flm = function(x1, x2) ( (ls$coefficients[1]) + (ls$coefficients[2] * x1) +
                           (ls$coefficients[3] * x2)
                         + (ls$coefficients[4] * x2)
                         )

t = outer(X = mmd, Y = mmq, FUN = flm)
persp3D(z = t, ticktype = "detailed")

If I understand correctly, shouldn't ls$coefficients[4] * x2 be ls$coefficients[4] * x2 * x2?

These details could be easily avoided if instead of writing your function to evaluate the fit with new data, you used predict.lm. This is what I think the code should look like, making other small improvements I will comment about below:

disp.density <- 100L
qsec.density <- 100L
disp.range <- with(mtcars, seq(from       = min(disp),
                               to         = max(disp),
                               length.out = disp.density))
qsec.range <- with(mtcars, seq(from       = min(qsec),
                               to         = max(qsec),
                               length.out = qsec.density))
data.grid <- expand.grid(disp = disp.range,
                         qsec = qsec.range)

fit <- lm(mpg ~ disp + qsec, data = mtcars)
predicted.vec <- predict(ls, data.grid)
predicted.mat <- matrix(predicted.vec, nrow = disp.density,
                                       ncol = qsec.density,
                                   dimnames = list(disp.range,
                                                   qsec.range))
library(???)
persp3D(z = predicted.mat, ticktype = "detailed")

And the same code should still work if you replace the formula mpg ~ disp + qsec with mpg ~ disp + poly(qsec, degree = 2). Some more notes:

It is recommended you avoid variable names like ls and t since they are very basic functions from R. Or you run the risk of shadowing them.
disp and qsec are not integers so using min(mtcars$disp):max(mtcars$disp) is a bit unpredictable. I feel it is better to use seq like I did, and with control over the grid density.
I was not able to test my code because you did not mention the package name for persp3D so it is possible it will need some adjustment. In the future, please make sure to give us everything we need to run your code.