Concatenate two containers, e.g. vector or strings I'd like to create a function that can concatenate vectors or strings. It must

Keep correct order
Be optimal

This is the solution I came up with:

template<typename V>
V concatenate( V&& v )
{
  return v;
}

template<typename V1, typename V2, typename... Rest>
V1 concatenate( V1 v1, V2&& v2, Rest&&... rest)
{
  v1.insert( v1.end(), v2.begin(), v2.end() );
  return concatenate( std::move(v1), std::forward<Rest>(rest)... );
}

// usage:
std::vector<int> v1 = { 1, 2, 3 };
std::vector<int> v2 = { 4, 5, 6 };
std::vector<int> v3 = { 7, 8, 9 };
std::vector<int> v12 = concatenate( v1, v2 );
std::vector<int> v123 = concatenate( v1, v2, v3 );
std::vector<int> vm12 = concatenate( std::move(v1), v2 );
std::vector<int> vm23 = concatenate( v2, std::move(v3) );

Is my argument forwarding correct here? Is this the simplest solution?

Your current strategy will cause more reallocations than you need to, which is not efficient. You already have all of your vector parameters, so you simply need to get their size, and reserve that amount for the output vector.

template<typename V1, typename V2, typename... Rest>
std::size_t get_reserve_amount( V1 const& v1, V2 const& v2, Rest const&... rest ) noexcept
{
    using exp = int[];

    std::size_t size_sum{ v1.size() + v2.size() };
    (void)exp{ 0, ( ( size_sum += rest.size() ), void(), 0 )... };

    return size_sum;
}

Create an output vector, call reserve with the result from get_reserve_amount().
Use the same technique from above to insert elements into the vector.
We also remove the single element concatenate() overload.

Using those suggestions, we get rid of recursion completely in the concatenate function.

template<typename V1, typename V2, typename... Rest>
std::decay_t<V1> concatenate( V1&& v1, V2&& v2, Rest&&... rest )
{
    using exp = int[];

    std::decay_t<V1> v;
    v.reserve( get_reserve_amount( v1, v2, rest... ) );

    v.insert( v.cend(), v1.cbegin(), v1.cend() );
    v.insert( v.cend(), v2.cbegin(), v2.cend() );

    (void) exp
    { 0, ( ( v.insert( v.cend(), rest.cbegin(), rest.cend() ) ), void(), 0 )... };

    return v;
}

Usage remains the same.

Here's a couple of timing (in milliseconds) tests to show you how big of a difference this makes.

Before reserve: 1061, 1062, 1058

After reserve: 332, 331, 330

Here is my timing test code:

#include <iostream>
#include <vector>
#include <chrono>

template <class TimeUnit, class Clock, class F, class... FArgs>
TimeUnit measure( F&& f, FArgs&&... f_args )
{
    auto t0{ Clock::now() };
    std::forward<F>( f )( std::forward<FArgs>( f_args )... );
    return std::chrono::duration_cast<TimeUnit>( Clock::now() - t0 );
}

int main()
{
    std::vector<int> v0( 11'000, 1 );
    std::vector<int> v1( 33'000, 3 );
    std::vector<int> v2( 55'000, 5 );
    std::vector<int> v3( 77'000, 7 );
    std::vector<int> v4( 99'000, 9 );

    std::size_t loop_count{ 1'000 };

    auto test = [ &, lc = loop_count ]
    {
        std::vector<int> v;
        for ( std::size_t i{ 0 }; i != lc; ++i )
        {
            v = concatenate( v0, v1, v2, v3, v4 );
        }
    };

    using ms = std::chrono::milliseconds;
    using hrc = std::chrono::high_resolution_clock;

    std::cout << measure<ms, hrc>( test ).count() << '\n';
}

You can most likely optimize this further by using move operations when concatenating.

The expanding pattern is explained very well here:

https://stackoverflow.com/a/25683817/2296177


Seconds between datestimes excluding weekends and evenings The basic idea here is that I want to measure the number of seconds between two python datetime objects. However, I only want to count hours between 8:00 and 17:00, as well as skipping weekends (saturday and sunday). This works, but I wondered if anyone had clever ideas to make it cleaner.

START_HOUR = 8
STOP_HOUR = 17
KEEP = (STOP_HOUR - START_HOUR)/24.0

def seconds_between(a, b):

    weekend_seconds = 0

    current = a
    while current < b:
        current += timedelta(days = 1)
        if current.weekday() in (5,6):
            weekend_seconds += 24*60*60*KEEP

    a_stop_hour = datetime(a.year, a.month, a.day, STOP_HOUR)
    seconds = max(0, (a_stop_hour - a).total_seconds())
    b_stop_hour = datetime(b.year, b.month, b.day, STOP_HOUR)
    if b_stop_hour > b:
        b_stop_hour = datetime(b.year, b.month, b.day-1, STOP_HOUR)

    seconds += (b - b_stop_hour).total_seconds()

    return (b_stop_hour - a_stop_hour).total_seconds() * KEEP + seconds - weekend_seconds

1. Issues

Your code fails in the following corner cases:

a and b on the same day, for example:

>>> a = datetime(2012, 11, 22, 8)
>>> a.weekday()
3          # Thursday
>>> seconds_between(a, a + timedelta(seconds = 100))
54100.0    # Expected 100

a or b at the weekend, for example:

>>> a = datetime(2012, 11, 17, 8)
>>> a.weekday()
5          # Saturday
>>> seconds_between(a, a + timedelta(seconds = 100))
21700.0    # Expected 0

a after STOP_HOUR or b before START_HOUR, for example:

>>> a = datetime(2012, 11, 19, 23)
>>> a.weekday()
0          # Monday
>>> seconds_between(a, a + timedelta(hours = 2))
28800.0    # Expected 0

Also, you count the weekdays by looping over all the days between the start and end of the interval. That means that the computation time is proportional to the size of the interval:

>>> from timeit import timeit
>>> a = datetime(1, 1, 1)
>>> timeit(lambda:seconds_between(a, a + timedelta(days=999999)), number=1)
1.7254137992858887

For comparison, in this extreme case the revised code below is about 100,000 times faster:

>>> timeit(lambda:office_time_between(a, a + timedelta(days=999999)), number=100000)
1.6366889476776123

The break even point is about 4 days:

>>> timeit(lambda:seconds_between(a, a + timedelta(days=4)), number=100000)
1.5806620121002197
>>> timeit(lambda:office_time_between(a, a + timedelta(days=4)), number=100000)
1.5950188636779785

2. Improvements

barracel's answer has two very good ideas, which I adopted:

compute the sum in seconds rather than days;
add up whole days and subtract part days if necessary.

and I made the following additional improvements:

handle corner cases correctly;
run in constant time regardless of how far apart a and b are;
compute the sum as a timedelta object rather than an integer;
move common code out into functions for clarity;
docstrings!

3. Revised code

from datetime import datetime, timedelta

def clamp(t, start, end):
    "Return `t` clamped to the range [`start`, `end`]."
    return max(start, min(end, t))

def day_part(t):
    "Return timedelta between midnight and `t`."
    return t - t.replace(hour = 0, minute = 0, second = 0)

def office_time_between(a, b, start = timedelta(hours = 8),
                        stop = timedelta(hours = 17)):
    """
    Return the total office time between `a` and `b` as a timedelta
    object. Office time consists of weekdays from `start` to `stop`
    (default: 08:00 to 17:00).
    """
    zero = timedelta(0)
    assert(zero <= start <= stop <= timedelta(1))
    office_day = stop - start
    days = (b - a).days + 1
    weeks = days // 7
    extra = (max(0, 5 - a.weekday()) + min(5, 1 + b.weekday())) % 5
    weekdays = weeks * 5 + extra
    total = office_day * weekdays
    if a.weekday() < 5:
        total -= clamp(day_part(a) - start, zero, office_day)
    if b.weekday() < 5:
        total -= clamp(stop - day_part(b), zero, office_day)
    return total


Web scraper for harvesting a full-site I've written some code using python. My goal was to harvest name, phone, and web address from extracting all the links available in that webpage. The scraper  is so far following the instructions nicely and harvesting data as it is supposed to.

The logic I applied here is crystal and easy to maintain. If any flaw is found in it's basic design,  It is my incapability I could not shake that off. Hope I'll have any suggestion to improve the design. Thanks a lot for going it through.

The complete code:

import requests
from lxml import html

class HouzzParser:
    main_url = "https://www.houzz.com/professionals/"

    def __init__(self):
        self.keeper = []
        self.session = requests.Session()

    def Scrape(self):
        self.receiver(self.main_url)

    def receiver(self, link):
        # Collecting side-bar links.
        response = self.session.get(link)
        tree = html.fromstring(response.text)

        for item in tree.xpath("//a[@class='sidebar-item-label']"):
            itm_lnks = item.xpath(".//@href")
            for page in itm_lnks:
                self.profile_links(page)     # Sending links to create new hrefs for getting to each profile of first page
                self.pagination_links(page)  # Sending links to process hrefs to the next page

    def pagination_links(self, link):
        # Creating links for the next page
        response = self.session.get(link)
        tree = html.fromstring(response.text)

        for n_url in tree.xpath("//ul[@class='pagination']"):
            items = n_url.xpath(".//a[@class='pageNumber']/@href")
            for item in items:
                self.profile_links(item)

    def profile_links(self, links):
        # Creating links for the profile page
        response = self.session.get(links)
        tree = html.fromstring(response.text)
        for titles in tree.xpath("//div[@class='name-info']"):
            links = titles.xpath(".//a[@class='pro-title']/@href")
            for doc in links:
                self.target_page(doc)

    def target_page(self, urls):
        # Extracting necessary docs from here
        response = self.session.get(urls)
        tree = html.fromstring(response.text)

        for titles in tree.xpath("//div[@class='profile-cover']"):
            name = titles.findtext(".//a[@class='profile-full-name']")
            phone = titles.xpath(".//a[contains(concat(' ', @class, ' '), ' click-to-call-link ')]/@phone")[0] if titles.xpath(".//a[contains(concat(' ', @class, ' '), ' click-to-call-link ')]/@phone") else ""
            web = titles.xpath(".//a[@class='proWebsiteLink']/@href")[0] if titles.xpath(".//a[@class='proWebsiteLink']/@href") else ""
            print(name, phone, web)
            data = name, phone, web
            if data not in self.keeper:
                self.keeper.append(data)

    def __str__(self):
        return self.keeper

if __name__ == '__main__':
    scraper = HouzzParser()
    scraper.Scrape()
    for item in scraper.keeper:
        print(item)

The code is quite clean and understandable - even though I'd work on the following:

Improve variable naming - for instance, Scrape() should become scrape() - in Python there is a lower_case_with_underscores notation for variable, function and method names.
You can cut down on the nested loops. For example:

for item in tree.xpath("//a[@class='sidebar-item-label']"):
    itm_lnks = item.xpath(".//@href")
    for page in itm_lnks:
        self.profile_links(page)     # Sending links to create new hrefs for getting to each profile of first page
        self.pagination_links(page)  # Sending links to process hrefs to the next page

will become:

for link in tree.xpath("//a[@class='sidebar-item-label']//@href"):
    self.profile_links(link)     # Sending links to create new hrefs for getting to each profile of first page
    self.pagination_links(link)  # Sending links to process hrefs to the next page

I would use the EAFP approach when reading the name and web values - you would at least avoid repeating your XPath expressions this way.
Convert your method comments into proper documentation strings.

Modified code:

import requests
from lxml import html

class HouzzParser:
    main_url = "https://www.houzz.com/professionals/"

    def __init__(self):
        self.keeper = []
        self.session = requests.Session()

    def scrape(self):
        self.receiver(self.main_url)

    def receiver(self, link):
        """Collecting side-bar links."""
        response = self.session.get(link)
        tree = html.fromstring(response.text)

        for link in tree.xpath("//a[@class='sidebar-item-label']//@href"):
            self.profile_links(link)     # Sending links to create new hrefs for getting to each profile of first page
            self.pagination_links(link)  # Sending links to process hrefs to the next page

    def pagination_links(self, link):
        """Creating links for the next page."""
        response = self.session.get(link)
        tree = html.fromstring(response.text)

        for pagination_link in tree.xpath("//ul[@class='pagination']//a[@class='pageNumber']/@href"):
            self.profile_links(pagination_link)

    def profile_links(self, links):
        """Creating links for the profile page."""
        response = self.session.get(links)
        tree = html.fromstring(response.text)

        for profile_link in tree.xpath("//div[@class='name-info']//a[@class='pro-title']/@href"):
            self.target_page(profile_link)

    def target_page(self, urls):
        """Extracting necessary docs from here."""
        response = self.session.get(urls)
        tree = html.fromstring(response.text)

        profile_info = tree.xpath("//div[@class='profile-cover']")[0]

        name = profile_info.findtext(".//a[@class='profile-full-name']")
        try:
            phone = profile_info.xpath(".//a[contains(concat(' ', @class, ' '), ' click-to-call-link ')]/@phone")[0]
        except IndexError:
            phone = ""

        try:
            web = profile_info.xpath(".//a[@class='proWebsiteLink']/@href")[0]
        except IndexError:
            web = ""

        print(name, phone, web)
        data = name, phone, web
        if data not in self.keeper:
            self.keeper.append(data)

    def __str__(self):
        return self.keeper

if __name__ == '__main__':
    scraper = HouzzParser()
    scraper.scrape()
    for item in scraper.keeper:
        print(item)


Uva Online Judge problem 100, 3n + 1 I am having a tough time submitting this code to UVa Online judge and having it pass. I keep getting the message - Your program used more CPU time than what is allowed for this problem. That means that your algorithm is not fast enough or that it entered into an infinite loop.

The challenge: for each line of input, which contains two integers i and j, find the length of the longest collatz-sequence for all Collatz sequences starting with i, i + 1, i + 2, …, j.  You can assume that no operation overflows a 32-bit integer.

I have no idea where the problem is! Any help would be appreciated. Also, I am trying to analyze the algorithm, and I believe it should be O(n2).

 #include <iostream>
    using namespace std;

    int cycleLength(int n) {
      int i = 1;

      while (n != 1) {
        if (n % 2 == 0) {
          n = n/2;
        } else {
          n = 3*n + 1;
        }
        i++;
      }
      return i;
    }

    int main() {

      int fOriginal, sOriginal;
      int first, second, max, temp;

     while (cin >> fOriginal >> sOriginal) {

        if (fOriginal > sOriginal) {
          temp = fOriginal;
          first = sOriginal;
          second = temp;
        }

        max = cycleLength(first);

        for(int i = first + 1; i <= second; i++) {
          temp = cycleLength(i);
          if (temp > max) {
            max = temp;
          }
        }

        cout << fOriginal << " " << sOriginal << " " << max << endl;
      }

      return 0;
    }

For each number in a first, second range you count how long does the sequence take to reach 1. It is definitely too much of the work: you'd surely encounter same numbers over and over again. Instead of recalculating sequence tails for them, it is much faster to remember and reuse such intermediate results. Since it is a contest problem I am not going to share any code.

Also, I am very interested in how did you come up with \$O(n^2)\$.


“Space and Time Efficient” way of finding all anagram groups I have written a program to find all anagram groups in a text file. I used first 26 Prime numbers as a mapper for 26 characters for finding anagram groups (Since character_set of all anagrams of a particular word are same, the products of the anagrams are also same ). After finding product of anagram I put (product, anagrams) pair into HashMap for retrieving later. But I think my solution of retrieving anagrams for particular character_set is not efficient although it is working. I need best practice for my case for iterating through HashMap. Here is the code:

public class Find_All_Anagrams_In_File {

// First 26 Primes for corresponding Alphabet letters
private static final int[] PRIMES = new int[] { 2, 3, 5, 7, 11, 13, 17, 19, 23, 29, 31, 37, 41, 43, 47, 53, 59,
                                        61, 67, 71, 73, 79, 83, 89, 97, 101, 103, 107, 109, 113 };

private static long calcProduct(char[] letters) {
    long res = 1L;
    for (char c: letters) {
        if (c < 65) { // ASCII A = 65
            return -1L;
        }
        int index = c - 65;
        res = res * PRIMES[index];
    }
    return res;
}

public static Map<Long, StringBuffer>  findAllAnagrams(String[] file) {

    Map<String, Long> anagrams = new HashMap<String, Long>();
    Map<Long, StringBuffer> result = new HashMap<Long, StringBuffer>();

    for (String word: file) {
        long product = calcProduct( word.toUpperCase().toCharArray() );
        anagrams.put(word, product);
    }

    StringBuffer sb = new StringBuffer();

    for (String word: file) {
        long product = calcProduct( word.toUpperCase().toCharArray() );

        if (result.get(product) == null)
            result.put(product, sb.append("" + word));
        else
            result.put(product, sb.append(result.get(product) + "," + word));
        sb = new StringBuffer();
    }
    return result;
}

@SuppressWarnings({"rawtypes" })
public static void main(String[] args) {

    /*
     *  for,fro,rof
     *  tentak,takten
     *  aks,kas
     *  xew,wex,xwe
     *  marza,maraz
     *  nakra,karan
     *  simple,mislep
     *
     */
    String[] file = {"for", "fro", "aks", "ten", "xew", "kas",
                     "uvn", "marza", "take", "random", "tentak", "nakra",
                     "recursion", "basket", "simple", "karan", "rof", "bike",
                     "wex", "xwe", "takten", "mislep", "maraz", "kasal"};

    Map<Long, StringBuffer> result = findAllAnagrams(file);

    Iterator iter = result.entrySet().iterator();
    while (iter.hasNext()) {
        Map.Entry pair = (Map.Entry) iter.next();
        String[] anagrams = pair.getValue().toString().split(",");
        if (anagrams.length > 1) {
            for (int i=0;i<anagrams.length; i++) {
                System.out.print(anagrams[i] + " ");
            }
            System.out.println();
        }

    }

  }
}

Result :
> nakra karan
> tentak takten
> xew wex xwe
> marza maraz
> for fro rof
> aks kas
> simple mislep

calcProduct

You don't need the L on these lines:

long res = 1L;
return -1L;

Instead of explaining what 65 is in a comment:

    if (c < 65) { // ASCII A = 65

You can use 'A' directly. And checking c < 'A' is not enough. You should also check c > 'Z'.

The method rewritten:

private static long calcProduct(char[] letters) {
    long res = 1;
    for (char c : letters) {
        if (c < 'A' || c > 'Z') {
            return -1;
        }
        int index = c - 'A';
        res = res * PRIMES[index];
    }
    return res;
}

findAllAnagrams

Instead of StringBuffer, it's recommended to use StringBuilder.

Instead of new HashMap<Long, StringBuffer>() you can use the diamond operator <> to simplify.

The anagrams map is written to but never read. You can remove it.

file is a poor name for an array of words. (words would be better)

The concatenation in sb.append("" + word) is pointless, as sb.append(word) has the same effect.

In general, instead of concatenation inside sb.append(...), it's recommended to chain .append(...).append(...) calls.

Instead of creating a new StringBuffer in every iteration of the loop, you could just create when it's actually needed: when putting the first word in the map.

Finally, it's recommended to always use braces on single-statement if-else conditions.

The method rewritten:

public static Map<Long, StringBuilder> findAllAnagrams(String[] words) {
    Map<Long, StringBuilder> result = new HashMap<>();

    for (String word : words) {
        long product = calcProduct(word.toUpperCase().toCharArray());

        StringBuilder sb = result.get(product);
        if (sb == null) {
            result.put(product, new StringBuilder(word));
        } else {
            sb.append(",").append(word);
        }
    }
    return result;
}

But, this is not nearly good enough. It would be better to use a List<String> as the values:

public static Map<Long, List<String>> findAllAnagrams(String[] words) {
    Map<Long, List<String>> result = new HashMap<>();

    for (String word : words) {
        long product = calcProduct(word.toUpperCase().toCharArray());

        List<String> anagrams = result.get(product);
        if (anagrams == null) {
            anagrams = new ArrayList<>();
            result.put(product, anagrams);
        }
        anagrams.add(word);
    }
    return result;
}

main

You should not suppress the raw types warning but fix it. The loop could have been written like this:

for (Map.Entry<Long, StringBuilder> entry : result.entrySet()) {

And since you don't need the keys in the loop body, it's better to iterate over the values:

    for (List<String> anagrams : findAllAnagrams(file).values()) {
        if (anagrams.size() > 1) {
            for (String word : anagrams) {
                System.out.print(word + " ");
            }
            System.out.println();
        }
    }


Improve random password generator Can you please take a look at my code and improve it (if necessary)?

http://jsfiddle.net/U6R6E/

Javascript (with jQuery)

function random(min, max) {
    return min + parseInt(Math.random() * (max - min + 1), 10);
}
function generatePassword() {
    var length = parseInt($('#pwLength').val(), 10),
        charset = $('#pwChars').val(),
        password = "";
    while (length > 0) {
        length -= 1;
        console.log(length);
        password += charset[random(0, charset.length - 1)];
    }
    return password;
}
function getNewPassword() {
    $('#pwResult').html(generatePassword());
}
$(document).ready(function () {
    getNewPassword();
    $('#getNewPw').click(function () {
        getNewPassword();
        return false;
    });
});

HTML

    <input type="text" id="pwChars" value="AaBbCcDdEeFfGgHhiJjKkLMmNnoPpQqRrSsTtUuVvWwXxYyZz23456789!?$%#&@+-*=_.,:;()" />

<input type="text" id="pwLength" value="10" />

<button id="getNewPw">New</button>

Math.random() doesn't return cryptographically secure numbers on all browsers. If this is intended for production use, you'll want to use a library that has a secure PRNG.

If you're not going to go with Jerry's suggestion to make pronounceable passwords, I'd recommend at least getting rid of 1/l/I and O/0, which a number of password generators do by default, because people often mis-read them and then request a password reset.


Dynamic resizeable array How can I improve this code for a dynamic and re-sizable array data structure?

Array.h

#ifndef _ARRAY_H_
#define _ARRAY_H_

class Array
{
private:
    int * m_ArrayContainer;//These two variables
    int m_Size;            //work in pairs.

private:
    void SetSize(int size);
    void AllocateMemoryOfSize(int size);
    void DeallocateMemory();

public:
    Array();
    Array(int size);
    Array(Array & arr);
    void operator = (Array & arr);
    void Resize(int newSize);
    int GetSize();
    void SetItem(int index, int value);
    int GetItem(int index);
    ~Array();
};

#endif

Array.cpp

#include "Array.h"
#include <iostream>

#pragma region Private Methods

void Array :: SetSize(int size)
{
    this->m_Size = size;
}
void Array :: AllocateMemoryOfSize(int size)
{
    this->m_ArrayContainer = new int[size];
    this->SetSize(size);
}
void Array :: DeallocateMemory()
{
    if(this->GetSize() > 0)
    {
        delete [] this->m_ArrayContainer;
        this->SetSize(0);
    }
}
void Array :: SetItem(int index, int value)
{
    this->m_ArrayContainer[index] = value;
}
#pragma endregion

Array :: Array()
{
    //std::cout<<"Array()...\n";

    this->m_ArrayContainer = NULL;

    this->SetSize(0);
}
Array :: Array(int size)
{
    //std::cout<<"Array(int size)...\n";

    this->SetSize(0);

    this->Resize(size);
}
void Array :: Resize(int newSize)
{
    int oldSize = this->GetSize();

    for(int i=0 ; i<oldSize ; i++)
    {
        this->SetItem(i, NULL);
    }
    this->DeallocateMemory();

    this->AllocateMemoryOfSize(newSize);
    this->SetSize(newSize);
}
Array :: Array(Array & arr)
{
    //std::cout<<"Array(Array & arr)...\n";

    this->SetSize(0);

    int size = arr.GetSize();

    this->Resize(size);

    for(int i=0 ; i<size ; i++)
    {
        this->SetItem(i, arr.GetItem(i));
    }
}
void Array :: operator = (Array & arr)
{
    //std::cout<<"operator = (Array & arr)...\n";

    this->SetSize(0);

    int size = arr.GetSize();

    this->Resize(size);

    for(int i=0 ; i<size ; i++)
    {
        this->SetItem(i, arr.GetItem(i));
    }
}
int Array :: GetSize()
{
    return this->m_Size;
}
int Array :: GetItem(int index)
{
    return this->m_ArrayContainer[index];
}
Array :: ~Array()
{
    //std::cout<<"~Array()...\n";
    this->DeallocateMemory();
}

You have a couple of memory leaks.

First, the DeallocateMemory() will only delete the object's memory block if the size is greater than 0:

void Array :: DeallocateMemory()
{
    if(this->GetSize() > 0)  // GetSize() will return zero at this point
    {
        delete [] this->m_ArrayContainer;
        this->SetSize(0);
    }
}

However, in C++ it's legal to allocate a memory block with 0 size. (See the Q&A C++ new int[0] -- will it allocate memory? on Stack Overflow for more information.) When you allocate memory, you actually get two things: the memory that you asked for, and some data that the C++ runtime uses to keep track of that memory block.

If you have a sequence of code like:

Array a(0);
a.Resize(1);

the constructor will allocate a 0-sized block of memory and the extra data for the C++ runtime. When Resize() calls DeallocateMemory(), it'll see that your data block is 0-sized, won't invoke delete[], and leak that memory when AllocateMemoryOfSize() overwrites the object's m_ArrayContainer pointer:

void Array :: AllocateMemoryOfSize(int size)
{
    this->m_ArrayContainer = new int[size]; // Memory leak!
    this->SetSize(size);
}

Second, in your assignment operator, the first thing you do is to set the size of this to zero:

void Array :: operator = (Array & arr)
{
    this->SetSize(0);
    int size = arr.GetSize();
    this->Resize(size);
    /// [stuff elided].
}

Next, you call Resize with the size of the other object:

void Array :: Resize(int newSize)
{
    int oldSize = this->GetSize();

    for(int i=0 ; i<oldSize ; i++)
    {
        this->SetItem(i, NULL);
    }
    this->DeallocateMemory();

    this->AllocateMemoryOfSize(newSize);
    this->SetSize(newSize);
}

Note that the for-loop at the start doesn't actually do anything because the size of this has been set to zero by the time you get here. Even if it didn't, it would be unnecessary because the intent is to delete[] that memory soon afterwards, and once it has been returned to the heap, the contents of memory are irrelevant.

Then you call DeallocateMemory(), which won't delete any memory because the size field in this has already been set to zero. Finally, you call AllocateMemoryOfSize() which unconditionally overwrites the pointer to the array, leaking memory the same way as in my first point above.

To fix these bugs, I would:

Check if m_ArrayContainer is a null pointer to decide if I need to delete[] it.

void Array :: DeallocateMemory()
{
    if(this->m_ArrayContainer != NULL)
    {
        delete [] this->m_ArrayContainer;
        this->m_ArrayContainer = NULL;
        this->SetSize(0);
    }
}

Don't zero out the memory in this just before deleting it; your Resize() method then becomes:

void Array :: Resize(int newSize)
{
    this->DeallocateMemory();

    this->AllocateMemoryOfSize(newSize);
    this->SetSize(newSize);
}

Don't set the size field independently of the allocation or deallocation of memory. You have calls to SetSize() in several places in your code: remove all except the ones in AllocateMemoryOfSize() and DeallocateMemory().

In the context of your assignment operator, this becomes:

void Array :: operator = (Array & arr)
{
    int size = arr.GetSize();

    this->Resize(size);

    for(int i=0 ; i<size ; i++)
    {
        this->SetItem(i, arr.GetItem(i));
    }
}

Note that there are still other ways that this class could fail, for example if an exception is thrown during allocation. I recommend reading about exception guarantees: in a nutshell, do the memory allocation and copying of the other instance into temporary memory first, then if that succeeds, free up the resources in this instance and assign the temporary values to them. Scott Meyers covers it well in his book, Effective C++.

Speaking of the assignment operator, it's customary to have it return a reference to this:

Array &Array::operator= (Array & arr)
{
    // [rest of the assignment operator elided]
    return *this;
}

This will allow you to chain together assignments like:

Array a, b, c;
// ... stuff ...
a = b = c;


Backing up two tables using SQL transactions I'm using a transaction statement in SQL Server 2012 for the first time.

I want to insert Table1's data in Table1_Backup before I delete everything from Table1.  Same process for Table2. Should I name my transactions?  Does this transaction code even look right?

DECLARE @err int
BEGIN TRANSACTION Table1
insert into [Table1_BU]
select * from [Table1] order by ID
SET @err = @@ERROR

IF @err = 0
BEGIN
delete from Table1
END

IF @err = 0
COMMIT TRANSACTION Table1
ELSE ROLLBACK TRANSACTION Table1

SET @err = 0
BEGIN TRANSACTION Table2
insert into [Table2_BU]
select * from Table2 order by ID
SET @err = @@ERROR

IF @err = 0
BEGIN
delete from Table2
END

IF @err = 0
COMMIT TRANSACTION Table2
ELSE ROLLBACK TRANSACTION Table2

First of all, you don't check @@ERROR after deleting which is not a good idea.

Second, you don't need to name transactions, but you can if you want to. Although, I'd recommend to avoid the same naming as you use for tables. For example, Table1_Backup is preferable in this case.

Let's make it more readable and compact. We have two ways.

1. XACT_ABORT

If you SET XACT_ABORT ON, transaction will automatically roll back on every run-time error.

SET XACT_ABORT ON

BEGIN TRANSACTION
    INSERT INTO [Table1_BU] SELECT * FROM [Table1] ORDER BY ID
    DELETE FROM Table1
COMMIT TRANSACTION

BEGIN TRANSACTION
    INSERT INTO [Table2_BU] SELECT * FROM [Table2] ORDER BY ID
    DELETE FROM Table2
COMMIT TRANSACTION

SET XACT_ABORT OFF

2. TRY ... CATCH

Second approach is to get it under control.

BEGIN TRY
    BEGIN TRANSACTION
        INSERT INTO [Table1_BU] SELECT * FROM [Table1] ORDER BY ID
        DELETE FROM Table1
    COMMIT TRANSACTION
END TRY
BEGIN CATCH
    ROLLBACK TRANSACTION
END CATCH

-- and, of course, the very same for the second table


State machine for upvoting/downvoting I have a list of resources which can be voted on. There are 3 voting states: upvoted, downvoted and no vote.

In a fashion identical to Stack Exchange voting, it follows these rules:

If you upvote a post you've already upvoted, it will set it to 'no vote'
If you upvote a post set to 'no vote' it will set it to upvote
if you downvote a post you've upvoted, it will set it to 'downvote' (I bolded this because in another post everyone gave me grief that this way is 'stupid', yet every voting site I have seen uses this style... Reddit, Digg. Stack Overflow, etc)

The exact same rules (except inverse) hold true for downvote.

Here's how I call my sendVote() function, when you click on upvote or downvote:

  // Voting
  $('.res-list').delegate('.vote .up', 'click', function() {
    flashColor($(this),'#fffc00');
    sendVote($(this).parent(),'upvoted');
    return false;
  });

  $('.res-list').delegate('.vote .down', 'click', function() {
    flashColor($(this),'#fffc00');
    sendVote($(this).parent(),'downvoted');
    return false;
  });

I believe that above code is fine, but his code below is what I feel is awful and could really be refactored, but I don't know how. I'm kind of new to JavaScript and jQuery.

function sendVote(object, direction) {

  var votesContainer = $(object).find('.m p');
  var votes = parseInt($(votesContainer).html());

  // UPVOTED
  if (direction == 'upvoted') {

    if ($(object).hasClass('downvoted')) {
      // Overwrite Downvote
      $(votesContainer).html(votes + 2);
      $(object).removeClass('downvoted');
      $(object).addClass(direction);
    } else if ($(object).hasClass(direction)) {
      // Undo Upvote
      $(votesContainer).html(votes - 1);
      $(object).removeClass(direction);
    } else {
      // Do Upvote
      $(votesContainer).html(votes + 1);
      $(object).addClass(direction);
    };

    // Ajax function here to /votes/upvote?id=x

  // DOWNVOTED
  } else {

    if ($(object).hasClass('upvoted')) {
      // Overwrite Upvote
      $(votesContainer).html(votes - 2);
      $(object).removeClass('upvoted');
      $(object).addClass(direction);
    } else if ($(object).hasClass(direction)) {
      // Undo Downvote
      $(votesContainer).html(votes + 1);
      $(object).removeClass(direction);
    } else {
      // Do Downvote
      $(votesContainer).html(votes - 1);
      $(object).addClass(direction);
    };

    // Ajax function here to /votes/downvote?id=x

  }
}

Both portions of code could be simplified if you use numbers, instead of words, to represent upvotes and downvotes. So I would change your calls to sendVote to look like this:

sendVote(this.parentNode, 1);
sendVote(this.parentNode, -1);

When modifying elements with javascript, I suggest using element.style.x (or element.style.cssText for multiple attributes) instead of classes, where possible. This is more efficient (browser doesn't have to re-test css selectors for the rest of the page), simpler (don't have to polyfill classList), and it means putting javascript-only css in js files, which means they aren't loaded in browsers that don't support javascript and therefore don't want them anyway. But the main reason is that is allows you to shorten your code to this:

var voteCSS = {
    '1': 'background:blue;',
    '0': 'background:grey;',
    '-1': 'background:red;'
};

function sendVote(element, newVote) {
    var countEl = element.querySelector('.m p'),
        prevVote = +element.dataset.prevVote || 0;

    if (newVote == prevVote)
        newVote = 0;
    countEl.innerHTML = +countEl.innerHTML - prevVote + newVote;
    element.dataset.prevVote = newVote;
    element.style.cssText = voteCSS[newVote];
    // ajax here to /votes/submit?id=x&direction=`newVote`
}

Alternatively, if you want to keep all your CSS in one place, you can make sure that "upvoted" or "downvoted" is the only class the element has, then just use .className. You could still apply default styling by selecting it from above, something like this: .votecontainer div {}.

ps: notice that I didn't use jQuery even once. Try to solve your problems without jQuery before you let yourself use it - a lot of the time you'll find that your solutions are simpler and more efficient without it.


Building a frequency dictionary from a Pandas dataframe I need to make a frequency dictionary from a pandas series (from the 'amino_acid' column in dataframe below) that also adds an adjacent row for each entry in the dictionary (from 'templates' column).

    templates   amino_acid
0   118         CAWSVGQYSNQPQHF
1   635         CASSLRGNQPQHF
2   468         CASSHGTAYEQYF
3   239         CASSLDRLSSGEQYF
4   51          CSVEDGPRGTQYF

My current approach of iterating through the dataframe seems to be inefficient and even an anti-pattern. How can I improve the efficiency/use best practice for doing this?

My current approach:

sequence_counts = {}
seqs= list(zip(df.amino_acid, df.templates))

for seq in seqs:
    if seq[0] not in sequence_counts:
        sequence_counts[seq[0]] = 0
    sequence_counts[seq[0]] += seq[1]

I've seen people the below way, but can't figure out how to adjust it to add each respective 'templates' entry:

sequence_counts = df['amino_acid'].value_counts().to_dict()

Any help/feedback would be greatly appreciated! :)

Yes, iterating through a dataframe usually is at odds with the spirit of dataframes and numpy arrays. They are best suited for vectorized operations, which are operations applied in bulk to rows/columns of the data structure.

Assuming that you've cast templates as an int64 or similar, you can use pd.groupby to do the same sort of grouping as you are doing with your dictionary:

import pandas as pd

df
        amino_acid  templates
0  CAWSVGQYSNQPQHF        118
1    CASSLRGNQPQHF        635
2    CASSHGTAYEQYF        468
3  CASSLDRLSSGEQYF        239
4    CSVEDGPRGTQYF         51
5  CASSLDRLSSGEQYF         66    # I've added this extra row here to show the effect

# these act as Series objects, so you can add together the
# grouped templates values
df.groupby('amino_acid')
# pd.groupby object

# use pd.Series.sum() to do this:
df.groupby('amino_acid').sum()

                 templates
amino_acid
CASSHGTAYEQYF          468
CASSLDRLSSGEQYF        305    # this was added for the two amino acids
CASSLRGNQPQHF          635
CAWSVGQYSNQPQHF        118
CSVEDGPRGTQYF           51

So what's going on in df.groupby? Well, you give it what to group on first. In this case, you group on the value for amino_acid. This creates a data structure that looks quite familiar if you've used itertools.groupby: tuples of (grouping key, DataFrame) pairs, where the DataFrame is a subset that matches the key. For example:

tmp = [(x, y) for x,y in df.groupby('amino_acid')]

[('CASSHGTAYEQYF',       amino_acid  templates
2  CASSHGTAYEQYF        468),
('CASSLDRLSSGEQYF',         amino_acid  templates
3  CASSLDRLSSGEQYF        239
5  CASSLDRLSSGEQYF         66),
('CASSLRGNQPQHF',       amino_acid  templates
1  CASSLRGNQPQHF        635),
('CAWSVGQYSNQPQHF',         amino_acid  templates
0  CAWSVGQYSNQPQHF        118),
('CSVEDGPRGTQYF',       amino_acid  templates
4  CSVEDGPRGTQYF         51)]

And per the docs, [df.sum] will return the sum on the specified axis (1 by default). So, for tmp[1], which contains two rows:

tmp[1][1].sum()

amino_acid    CASSLDRLSSGEQYFCASSLDRLSSGEQYF
templates                                305
dtype: object

Where 305 is the sum. The pd.groupby object as a whole supports the .sum call, so we are able to call it like we did above.

# Now, using `to_dict`, you can see that you want what's inside templates
df.groupby("amino_acid").sum().to_dict()

# {'templates': {'CASSHGTAYEQYF': 468, 'CASSLDRLSSGEQYF': 305, 'CASSLRGNQPQHF': 635, 'CAWSVGQYSNQPQHF': 118, 'CSVEDGPRGTQYF': 51}}

# so use the templates attribute to grab it
df.groupby("amino_acid").sum().templates.to_dict()
# {'CASSHGTAYEQYF': 468, 'CASSLDRLSSGEQYF': 305, 'CASSLRGNQPQHF': 635, 'CAWSVGQYSNQPQHF': 118, 'CSVEDGPRGTQYF': 51}

This applies the operations within the dataframe, which is more efficient than a loop. The analogue that you were trying to use could leverage defaultdict from the collections module, as well. It prevents the need to check for a key's existence, speeding up the loop immensely:

from collections import defaultdict

# specify your input type here
sequence_counts = defaultdict(int)

# it is more pythonic to use tuple-unpacking in loops
# as indexing is less readable
for amino, template in zip(df.amino_acid, df.templates):
    sequence_counts[amino] += template

Also, by iterating over the zip object directly, you don't copy data into memory. list will aggregate all of the members into a data structure, whereas zip is a generator that will just produce the members one at a time until exhausted. It's like the difference between for x in range and for x in list(range):

# This will just run for a really really really long time
for i in range(1000000000000):
    print(i)

# This will crash your computer, it will never
# get to the print statement, because it must evaluate
# list(range) before it starts the loop
for i in list(range(1000000000000)):
    print(i)


Clever way to build a extension method that ordenate my IQueryable? I want to create a ExtensionMethod to ordenate my linq query, called Ordenar. I'll sort it depending what columns is in sortColumns ListDictionary.

I tried some ways, but the best way I arquieve was this :

static class ClienteBusinessExtensions
    {
        public static IQueryable<Model.View_Clientes> Ordernar(this IQueryable<Model.View_Clientes> viewClientes, ListDictionary sortColumns)
        {
            foreach (System.Collections.DictionaryEntry item in sortColumns)
            {
                if (item.Value.ToString()=="ASC")
                {
                    switch (item.Key.ToString())
                    {
                        case "ClienteID":
                            viewClientes = viewClientes.OrderBy(v => v.ClienteID);
                            break;
                        case "Nome":
                            viewClientes = viewClientes.OrderBy(v => v.Nome);
                            break;
                        default:
                            break;
                    }
                }
                else //item.Value.ToString() will be DESC
                {
                    switch (item.Key.ToString())
                    {
                        case "ClienteID":
                            viewClientes = viewClientes.OrderByDescending(v => v.ClienteID);
                            break;
                        case "Nome":
                            viewClientes = viewClientes.OrderByDescending(v => v.Nome);
                            break;
                        case "CpfCnpj":
                            viewClientes = viewClientes.OrderByDescending(v => v.CpfCnpj);
                            break;
                        default:
                            break;
                    }
                }

            }
            return viewClientes;
        }

It is used like this :

var clientesVip = (from vips in db.View_Clientes
                               .Ordernar(sortColumns)
                           select vips);

Are there a clever way to do it? I don't want to use switch statement. I want to sort any field of View_Clientes. It is used in a Asp.NET gridView, when user sort a column.

And View_Clientes entity is :

public partial class View_Clientes
{
    public System.Guid ClienteID { get; set; }
    public string Codcfo { get; set; }
    public string Nome { get; set; }
    public string CpfCnpj { get; set; }
    public string CLI_Origem { get; set; }
}

You definitely could use DynamicLinq to solve this problem.  See Scott Guthries blog post about it at:
http://weblogs.asp.net/scottgu/archive/2008/01/07/dynamic-linq-part-1-using-the-linq-dynamic-query-library.aspx


Should I re-throw exceptions within a method, or is there a better way to handle it? When handling errors in C#, is it acceptable/appropriate to re-throw exceptions that are caught in a try/catch?

My scenario is that I am making an HTTP request and then checking the content of the response for a specific key (access_token). If that key is missing, I throw an error.

However, as all of the code within the method is in a try/catch block (just in case HTTP requests go awry), the exception that I throw is being caught. The solution that I've settled on for now is to re-throw the error that is caught, to ensure that I don't lose additional detail that was included with the custom exception. You can see a full example of my code at the bottom of this question.

I've found a number of other possibilities though, and I'd be interested to know what the C# community thinks is the best way to handle such scenarios (even if it turns out that none of them are appropriate!). There are of course other questions/resources that are similar to this, but some of the information is ambiguous and being a novice at C#, I'd like get a bit of extra context with a real-life example.

Possibilities

A separate catch block to re-throw the known error (see example 1).
Check for the type of exception in a single catch block (see example 2).
Return an exception should 'access_token' not be present (as opposed to throwing it).
Don't wrap all the code in a try\catch.

Example 1

catch (WhException ex)
{
    // Capture and rethrow a 'WhException', ensurig that the detail is not lost.
    throw ex;
}
catch (Exception ex)
{
    // Capture any generic exception and retrhow it as a 'WhException'.
    throw new WhException("An error occurred whilst attempting to generate a bearer access token.", ex.Message, ex);
}

Example 2

catch (Exception ex)
{
    if (ex is WhException)
        // Capture and rethrow a 'WhException', ensurig that the detail is not lost.
        throw ex;
    else
        // Capture any generic exception and retrhow it as a 'WhException'.
        throw new WhException("An error occurred whilst attempting to generate a bearer access token.", ex.Message, ex);
}

Full working example

public static async Task<string> GetAzureAccessTokenAsync(this HttpClient http, WhConfigRaw config)
{
    try
    {
        Dictionary<string, string> form = new Dictionary<string, string>()
        {
            { "grant_type", "client_credentials" },
            { "client_id", config.ServicePrincipalId },
            { "client_secret", config.ServicePrincipalKey },
            { "resource", AppSettings.Get("AzureCoreUri") }
        };
        FormUrlEncodedContent requestContent = new FormUrlEncodedContent(form);
        string uri = string.Format(AppSettings.Get("ClientCredentialsUri"), config.TenantId);

        // Query the Azure management API to get client credentials.
        HttpResponseMessage response = await http.PostAsync(uri, requestContent);

        // Read the content of the response for the Azure management API.
        JObject content = await response.Content.ReadAsJsonAsync<JObject>();

        // Check whether or not the content of the response contains and 'access_token'.
        if (!content.ContainsKey("access_token"))
        {
            throw new WhException("An error occurred whilst attempting to generate a bearer access token.", content);
        }

        return content["access_token"].ToString();
    }
    catch (WhException ex)
    {
        // Capture and rethrow a 'WhException', ensurig that the detail is not lost.
        throw ex;
    }
    catch (Exception ex)
    {
        // Capture any generic exception and retrhow it as a 'WhException'.
        throw new WhException("An error occurred whilst attempting to generate a bearer access token.", ex.Message, ex);
    }
}

Deciding which exceptions to handle, rethrow, wrap in other exceptions is a design decision. Some exceptions might be caught to branch to a different flow. Naively catching all exceptions is a bad call. Guidelines for handling and throwing exceptions

Some exceptions should be propagated all the way up, because of the integrity of the application or even your machine cannot be preserved. Be careful when encountering OutOfMemoryException, BadImageFormatException, StackOverflowException.

Would you really want to wrap/hide the aforementioned exceptions in a custom exception?

catch (Exception ex)
{
    // Capture any generic exception and retrhow it as a 'WhException'.
    throw new WhException("An error occurred whilst ..", ex.Message, ex);
}

Keep in mind that detail is lost when you catch and throw ex;. A new stacktrace is created.

catch (WhException ex)
{
    // Capture and rethrow a 'WhException', ensurig that the detail is not lost.
    throw ex;
}

Use throw; to preserve stacktrace.

catch (WhException ex)
{
    throw; // preserves stacktrace
}

If you want to rethrow outside the catch block you could use ExceptionDispatchInfo. Suppose Exception error is declared before the try:

catch (WhException ex)
{
    error = ExceptionDispatchInfo.Capture(ex);
}

error?.Throw(); // preserves stacktrace

Consider throwing these well-known exceptions for common situations:

ArgumentNullException : when a mandatory argument is not provided
ArgumentException: when an argument does not meet the preconditions to run
AggregateException: when packing multiple exceptions together


Logarithmic growth counter I'm looking for a way to have an logarithmic incremental counter. The counter goal is related to a numeric value than goes from 1 to Infinity. The counter maximum value should be 1000.
The sequence looks like this:

number is below 10, increment by 1:

1,2,3...7,8,9,10
number is above 10, increment by ^10 when the number reaches the counter

100, 1000 stop at 1000

This is what I wrote so far (javascript):

let i = 0
let mod = 10
let result = document.getElementById('result')
result.innerText = ''

while (++i < 7322) {
  mod *= mod < 1000 && i === mod * 10 ? 10 : 1

  if (i < 10 || i % mod === 0) {
    //the further the `i` value is, the less this will be called
    result.innerText += i + ' pass the test. Mod is ' + mod + '.\n'
  }
}

I'm sure there is a more elegant way of doing this but can't get my hand on it. Do you have ideas on how to simplify the algorithm?

Your solution is a bit wasteful by generating and testing all integers in the desired range while you actually only need a handful of them.

A big improvement in terms of performance is to avoid modulo arithmetic and increase the step-size tenfold whenever you finished iterating a sub-range 1-9, 10-99, 100-999 and so on:

let max = 7322;
let step = 1;
let next = 10;

while (step < 1000 && next < max) {
  for (let i = step; i < next; i += step) {
    console.log(i);
  }
  step = next;
  next *= 10;
}

for (let i = step; i < max; i += step) {
  console.log(i);
}

Of course, you might want to encapsulate above code in a function or even more versatile, a generator function and replace console.log(i); with yield i;.

However, since you only have four different step-sizes of 1, 10, 100, and 1000 you can encode them manually and come up with an even simpler generator function as follows:

// Generate logarithmic range from 1 to max (excluded):
function* logRange(max) {
  for (let i = 1; i < max && i < 10; i += 1) yield i;
  for (let i = 10; i < max && i < 100; i += 10) yield i;
  for (let i = 100; i < max && i < 1000; i += 100) yield i;
  for (let i = 1000; i < max; i += 1000) yield i;
}

// Example:
for (let i of logRange(7322)) console.log(i);

This avoids modulo arithmetic, computes only the desired numbers and is pretty easy to understand.


Trying for an efficient and readable merge-sort implementation My first implementation of merge sort in C++.
I'm looking for any sort of optimization and/or conventional practice tips, such as using the appropriate data types and naming my variables appropriately.
#include <iostream>

void print_array(int[], size_t);

void merge_sort(int[], size_t, size_t);

int main() {
    size_t size;

    std::cout << "Enter the size of the container: ";
    std::cin >> size;

    int arr[size];
    for (size_t i = 0; i < size; ++i) {
        std::cout << "[" << i+1 << "] Enter the next integer element: ";
        std::cin >> arr[i];
    }

    std::cout << std::endl << "Unsorted array: ";
    print_array(arr, size);
    std::cout << std::endl << std::endl;

    merge_sort(arr, 0, size-1);

    std::cout << "Sorted array: ";
    print_array(arr, size);
}

void print_array(int arr[], size_t size) {
    for (size_t i = 0; i < size; ++i) {
        std::cout << arr[i] << ' ';
    }
}

void merge_sort(int arr[], size_t start, size_t end) {
    if (end > start) {
        size_t middle = (start+end)/2;

        merge_sort(arr, start, middle);
        merge_sort(arr, middle+1, end);

        int l[middle-start+1];
        int r[end-middle];

        for (size_t i = start; i <= middle; ++i) {
            l[i-start] = arr[i];
        }

        for (size_t i = middle+1; i <= end; ++i) {
            r[i-(middle+1)] = arr[i];
        }

        size_t i = 0, j = 0, k = start;
        while (i < middle-start+1 && j < end-middle) {
            if (l[i] < r[j]) {
                arr[k] = l[i++];
            } else {
                arr[k] = r[j++];
            }
            ++k;
        }

        for (size_t z = i; z < middle-start+1; ++z) {
            arr[k] = l[z];
            ++k;
        }

        for (size_t z = j; z < end-middle; ++z) {
            arr[k] = r[z];
            ++k;
        }
    }
}

The single most important advantage of merge sort is its stability. The elements compared equal retain their original order. Your code loses stability. If l[i] compares equal to r[j], the latter is merged first.

Your code assumes closed ranges (that is, end is a valid index). That leads to some unpleasant fiddling with indices. Semi-open ranges (that is, end is the first invalid index) are much cleaner to deal with.

size_t middle = (start+end)/2; is vulnerable to arithmetic overflow. Consider
  size_t middle = start + (end - start)/2;

Avoid VLAs. They are vulnerable to stack overflow, and besides, they are not supported by c++ standard.

No naked loops, please. Every loop implements an algorithm, and therefore deserves a name. The before and after the actual merge in fact implement copy.

I don't see anything resembling c++.  The entire code is plain old c.


my code is messy and wasteful of lines, how do I make my code simple can this script be made simpler? I'm just learning about javascript, so I don't really know which syntax to use.
My HTML I have several id consisting of "doc1, doc2, doc3, .." and there is another id "icon-cek1, icon-ceck2, icon-cek3". "icon-cek" is used to display the place of a checkmark icon when the upload is successful / according to the extension that has been specified and if the file fails / doesn't match the extension will display a crossmark.
 DOcument 1
<input type="file"id="doc1"/>
 Choose

 DOcument 2
<input type="file"id="doc2" />
 Choose

 DOcument 3
<input type="file"id="doc3"/>
 Choose

    $.fn.checkFileType = function (options) {
    var defaults = {
      allowedExtensions: [],
      success: function () {},
      error: function () {},
    };
    options = $.extend(defaults, options);

    return this.each(function () {
      $(this).on("change", function () {
        var value = $(this).val(),
          file = value.toLowerCase(),
          extension = file.substring(file.lastIndexOf(".") + 1);

        if ($.inArray(extension, options.allowedExtensions) == -1) {
          options.error();
          $(this).focus();
        } else {
          options.success();
        }
      });
    });
  };

  $("#doc1").checkFileType({
    allowedExtensions: ["pdf"],
    success: function () {
      document.getElementById("icon-cek1").className = "checkmark";
    },
    error: function () {
      document.getElementById('doc1').value='';
      document.getElementById("icon-cek1").className = "crossmark";
    },
  });

    $("#doc2").checkFileType({
    allowedExtensions: ["pdf"],
    success: function () {
      document.getElementById("icon-cek2").className = "checkmark";
    },
    error: function () {
      document.getElementById("doc2").value = "";
      document.getElementById("icon-cek2").className = "crossmark";
    },
  });

  $("#doc3").checkFileType({
    allowedExtensions: ["pdf"],
    success: function () {
      document.getElementById("icon-cek3").className = "checkmark";
    },
    error: function () {
    alert("Just PDF");
      document.getElementById("doc3").value = "";
      document.getElementById("icon-cek3").className = "crossmark";
    },
  });

or is there a simpler code to handle the case I have?

Because the <div> associated with each input not its adjacent sibling, but its next sibling, you can dynamically navigate from the element being validated to its associated icon with .nextElementSibling calls. But your checkFileType isn't set up to call the success callback with a calling context of the element it's attached to, so fix that first. Change the success and error calls to:
options.success.call(this);

options.error.call(this);

and then the rest of your code simplifies to the following:
$('input[type="file"]').checkFileType({
    allowedExtensions: ["pdf"],
    success() {
        $(this).next().next().attr('class', 'checkmark');
    },
    error() {
        $(this).val('');
        $(this).next().next().attr('class', 'crossmark');
    },
});

If you're going to use jQuery, go ahead and use it instead of the native DOM methods like getElementById if you want, native DOM methods are a bit more verbose than jQuery's $.
Using the above method, you'll also be able to remove the IDs entirely from your HTML. (Numeric indexed IDs are quite a code smell anyway.)
This is a pretty simple problem though, it seems pretty odd to need to use a big library like jQuery for something this trivial. (If you're just learning JavaScript, consider focusing on learning just built-in JavaScript rather than a particular library like jQuery, which is somewhat obsolete for most things it's used for nowadays - built-in methods work just fine and are usually easier to understand in my experience)
Just as one example, rather than
if ($.inArray(extension, options.allowedExtensions) == -1) {

you can use
if (!options.allowedExtensions.includes(extension)) {


Project Euler 4 - Largest palindrome product I submitted a solution to Project Euler 4 exercise: Largest palindrome product in Java. The content of following exercise:

  A palindromic number reads the same both ways. The largest palindrome
  made from the product of two 2-digit numbers is 9009 = 91 × 99.

  Find the largest palindrome made from the product of two 3-digit
  numbers.

I wrote a method called isPalindrome that checks is reversing of string the same as output string. I have to obviously convert int to String to check is given number is palindrome. I created palindromes list and I loop for all possible three-digits numbers that would be multiplied and I check are these products palindromes. If yes, then I add it product to list. Next, I select only maximum palindrome from list and I print it to console.

Main.java:

package pl.hubot.projecteuler.problem4;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

public class Main {
    public static void main(String[] args) {
        List<Integer> palindromes = new ArrayList<>();
        for (int i = 100; i <= 999; i++) {
            for (int j = 100; j <= 999; j++) {
                if (isPalindrome(Integer.toString(i * j)))
                    palindromes.add(i * j);
            }
        }
        System.out.print(Collections.max(palindromes));
    }

    private static boolean isPalindrome(String str) {
        return str.equals(new StringBuilder(str).reverse().toString());
    }
}

We don't need a List

        List<Integer> palindromes = new ArrayList<>();

This could be just

        int largest = -1;

As we just need to determine the largest one.

        System.out.print(Collections.max(palindromes));

This just becomes

        System.out.print(largest);

Work down, not up

        for (int i = 100; i <= 999; i++) {
            for (int j = 100; j <= 999; j++) {

This does more work than necessary.  Instead, consider

        for (int i = 999; i >= 100; i--) {
            for (int j = i; j >= 100; j--) {

You want to find the maximum.  So start with the largest possible candidate and work your way down.

Also, you don't have to check \$999\cdot 998\$ and \$998\cdot 999\$.  It's enough to test one.  So we can start j as i.

There will be more that we can do later.

Convert after calling isPalindrome

                if (isPalindrome(Integer.toString(i * j)))
                    palindromes.add(i * j);

Consider

                int product = i * j;
                if (product <= largest) {
                    break;
                }

                if (isPalindrome(product)) {
                    largest = product;
                }

If product is smaller than the largest palindrome found so far, we don't need to check these values anymore.  Also, we only update largest if product is larger.  So we don't need to do the Collections.max after.

We don't repeatedly calculate the same product (compiler probably fixes that for you anyway, but I find this easier to follow).

You said

  I have to obviously convert int to String to check is given number is palindrome.

But it is not obvious to me that we have to convert to a String to check if it is a palindrome.  That's a conceptually simple way, but we could do it more efficiently with math.  And this way, we can drop in a math-based replacement without altering this method.

And to fix the mismatched parameters without using a replacement, we could just write

    private static boolean isPalindrome(int candidate) {
        return isPalindrome(Integer.toString(candidate));
    }

And as I said, we can replace that method with a more efficient one without changing the caller.


Simplify my class mapping common class properties between model and viewmodel classes I have an ASP.NET MVC 5 app and I map properties between model and viewmodel and vice versa.

I could use automapper to do this but since I don't need mapping rules to convince me to use it as I mostly do simple 1 to 1 mapping the  performance overhead that automapper will add is not worthwhile.If I have mapping differences I'll map it manually where I reference it in my controller.

Here is my code

public class Dog
{
    public int Id { get; set; }

    public string Name { get; set; }

    public DateTime? BirthDate { get; set; }

    public int? CreatedBy{ get; set; }

    public DateTime? DateCreated { get; set; }
}

public class DogViewModel
{
    public int Id { get; set; }

    public string Name { get; set; }

    public DateTime? BirthDate { get; set; }

    public static implicit operator DogViewModel(Dog dm)
    {
        var vm= new DogViewModel{
            Id = dm.Id,
            Name = dm.Name,
            BirthDate = dm.BirthDate
       };
        return vm;
    }

    public static implicit operator Dog(DogViewModel vm)
    {
        var dm = new Dog
        {
            Id = vm.Id,
            Name = vm.Name,
            BirthDate = vm.BirthDate
        };
        return dm;
    }
}

This allows me to in my controller actionmethods easily map one class to another.

e.g.

  public async Task<IActionResult> Edit(int? id)
    {
        var dog= await _db.Dogs.FirstOrDefaultAsync(d => md.Id == id);
        DogViewModel dogVm = dog;
        ...

     }

    [HttpPost]
    public async Task<IActionResult> Edit(DogViewModel dogVm)
    {
       Dog dog= dogVm;
       ...
    }

In my index ActionMethod I need to map a list of items.

  public async  Task<IActionResult> Index()
    {
        var dogs = await _db.Dogs.ToListAsync();

        var dogVms = new List<DogViewModel>();
        foreach (var dog in dogs)
        {
            DogViewModel dogVm = dog;
            dogVms.Add(dogVm);
        }
        return View(dogVms);
    }

Question: Is there a way in LINQ or some other way in C# to eliminate the above foreach clause to simplify this code?

Providing

var dogVms = new List<MonthViewModel>();

is meant to actually be

var dogVms = new List<DogViewModel>();

Then to use LINQ you could do

List<DogViewModel> dogVms = (from dog in dogs select (DogViewModel)dog).ToList();


PatternMatcher class I thought of writing a utility class where I can implement patterns that are useful for day-to-day parsing of strings, without having to to import java.util.regex everywhere. So I wrote this simple class as a model.

This was inspired by a (now deleted) Stack Overflow question:

  For ex: String str="asd14sd67fgh007"; I want the output like: 1467007asdsdfgh

Right now I only added two patterns, with the intention of adding more as the need calls for it. I used regex for these, but other pattern-matching methods could [potentially] also be used, if the need calls for it.

I would be interested to know if there is a smarter way of doing this that I have not thought of, and in general, how can this be improved, to make adding more patterns and methods as easy as possible, keep the code clear and concise, and in general make it better for Mr. or Mrs. Maintainer.

Note: I wrote this using repl.it for the time being, and not actually added to a code base yet, hence the class not being declared as public. That will be rectified once it's added to a code base.

PatternMatcher class

import java.util.regex.*;

class PatternMatcher {

    public String inputString;

   /**
    * Constructor.
    */
    public PatternMatcher(String inputString) {
        this.inputString = inputString;
    }
    /**
     * Get only upper and lower case letters from inputString.
     * @return  String containing the upper and lower case letters from inputString
     */
    public String getLetters() {
        final Pattern LETTERS = Pattern.compile("[A-Za-z]");
        StringBuilder outputBuilder = new StringBuilder();
        Matcher letterMatcher = LETTERS.matcher(inputString);
        while (letterMatcher.find()) {
            outputBuilder.append(letterMatcher.group());
        }
        return outputBuilder.toString();
    }
    /**
     * Get only numbers/digits from inputString.
     * @return  String containing the numbers/digits from inputString
     */
    public String getNumbers() {
        final Pattern NUMBERS = Pattern.compile("[0-9]");
        StringBuilder outputBuilder = new StringBuilder();
        Matcher letterMatcher = NUMBERS.matcher(inputString);
        while (letterMatcher.find()) {
            outputBuilder.append(letterMatcher.group());
        }
        return outputBuilder.toString();
    }
}

Main class (for testing)

class Main {
      public static void main(String[] args) {
          String input = "asd14sd67fgh007";
          String expectedOutput = "1467007asdsdfgh";
          PatternMatcher inputMatcher = new PatternMatcher(input);
          StringBuilder actualOutputBuilder = new StringBuilder();
          actualOutputBuilder.append(inputMatcher.getNumbers());
          actualOutputBuilder.append(inputMatcher.getLetters());
          String actualOutput = actualOutputBuilder.toString();
          if (actualOutput.equals(expectedOutput)) {
              System.out.println("Pattern match succeeded");
          } else {
              System.out.println("Pattern match failed");
          }
      }
}

final Pattern LETTERS = Pattern.compile("[A-Za-z]");

Every time either getLetters or getNumbers is called, a new regex is created and compiled.

This is not very efficient, considering regex compilation can be fairly slow. I would recommend moving the compiling to the constructor. Then, you can set the new Patterns to be fields of the class.

Now, the regexes will be compiled once and only once.

I don't quite understand your design.

Initialize the class with a string.
Use the class methods to get different parts of the string.

What if I want to check another string? Well, considering the fact that you made inputString public, I'm assuming you want the code to just set inputString to something else:

myPatternMatcher.inputString =

Just to quickly say, the inputString should be private and there should be a setter for it. This is common Java practice.

However, that is not the point of this tip. My main point is that it is weird that I have to change a property of the class and then call another method of that class to work on the string I just passed in a second ago.

With setters, that would look like this:

myPatternMatcher.setInputString("foo");
myPatternMatcher.getLetters();

Isn't that a little weird having to call another method just to use the class?

I think a better approach would be to make the methods static. You could get rid of having to pass the inputString via the constructor, and you can move the pattern compilation to the fields themselves. Then, for each method, you can have it accept the string to check.

Now, your class would look like this:

public class PatternMatcher {
    public static String getLetters(String str) {
        ...
    }
    public static String getNumbers(String str) {
        ...
    }
}

Now, to use this class, it would look something like this:

PatternMatcher.getLetters("Foo");

This design makes more sense.


Song search app with dynamic tables I decided to make a Ruby app to search for songs, which uses the tinysong API. Things got really cryptic and weird in the view. I managed to get dynamic tables that adjust to data length, but I feel like there must be a much better way to accomplish this output.

require "rubygems"
require 'json'
require "httparty"

def search(search_terms)
  search_string = search_terms.split(' ').join('+')
  response = HTTParty.get("http://tinysong.com/s/#{search_string}?format=json&key=API_KEY_OMITTED_FOR_REVIEW")
  song_hash = JSON.parse(response.body)
  #puts "#{song_hash}"
end

def view_song_search(song_hash)
  term_width       = %x'tput cols'.to_i # Executes terminal
  total_char_limit = term_width - 9
  field_limit      = total_char_limit / 3

  # Sets width of each column to the width of it's highest data.
  artist_width = album_width = song_width = 0
  song_hash.each do |song|
    if song["ArtistName"].length > artist_width then artist_width = song["ArtistName"].length end
    if song["AlbumName"].length > album_width then album_width = song["AlbumName"].length end
    if song["SongName"].length > song_width then song_width = song["SongName"].length end
  end

  # Sets the minimum column width equal to header width.
  if artist_width < "Artist".length then artist_width = "Artist".length end
  if album_width < "Album".length then album_width = "Album".length end
  if song_width < "Song".length then song_width = "Song".length end

  # If total width is higher than terminal width, reduce total width to fit.
  if artist_width + album_width + song_width > total_char_limit
    artist_width = album_width = song_width = field_limit
  end

  # Where things get cryptic.
  printf("+%s+%s+%s+\n", "-" * (artist_width + 1), "-" * (album_width + 2), "-" * (song_width + 2))
  printf("\e[7m|%-#{artist_width}s | %-#{album_width}s | %-#{song_width}s |\e[0m\n", "Artist", "Album", "Song")
  printf("+%s+%s+%s+\n", "-" * (artist_width + 1), "-" * (album_width + 2), "-" * (song_width + 2))

  song_hash.each do |song|
    # Sets data of each column to not exceed column width.
    if song["ArtistName"].length > artist_width ||
       song["AlbumName"].length > album_width ||
       song["SongName"].length > song_width
      artist_name = (song["ArtistName"])[0...artist_width]
      album_name = (song["AlbumName"])[0...album_width]
      song_name = (song["SongName"])[0...song_width]
    else
      artist_name = song["ArtistName"]
      album_name  = song["AlbumName"]
      song_name   = song["SongName"]
    end

    printf("|%s%s | %s%s | %s%s |\n",artist_name, ' ' * (artist_width - artist_name.length),
                                     album_name, ' ' * (album_width  - album_name.length),
                                     song_name,   ' ' * (song_width   - song_name.length))
    printf("+%s-%s-%s+\n", "-" * (artist_width + 1), "-" * (album_width + 2), "-" * (song_width + 2))
  end
end

puts "Enter your search terms: "
view_song_search(search(gets.chomp))

And here is an example of the output:

I'm going to assume that search returns a data structure like this:

data = [
  { 'ArtistName' => 'Crash Test Dummies',
    'AlbumName' => 'God Shuffled His Feet',
    'SongName' => 'Mmmm_Mmmmm...',
  },
  { 'ArtistName' => 'John Powell',
    'AlbumName' => 'How to Train Your Dragon',
    'SongName' => 'Test Drive',
  },
  …
]

In that case, using song_hash as the name of the parameter to view_song_search is misleading, since it's actually an array.

There's no reason why view_song_search needs to know anything about songs, or why it should be tied to artists, albums, and songs at all.  You should write a generic table-formatting function, like this:

puts table(data, [['ArtistName', 'Artist'],
                  ['AlbumName', 'Album'],
                  ['SongName', 'Song']])

Here's an implementation of such a function.  Remarks:

A large chunk of the code is the block in the middle to shrink overwide tables.  I've tried to scale each column proportionally instead of giving one-third to each column.
The heading row resembles a data row in many ways, and should be handled using a common codepath.
The horizontal rules are printed many times; build it just once.
Use sprintf '%-*s', width, text to space-pad the text to the desired width.

def table(hashes, headings, width_limit=%x'tput cols'.to_i - 5)
  attrs = headings.map { |h| h.first }
  headings = Hash[headings]

  # Minimum widths to accommodate the heading and data of each column,
  # excluding leading space, trailing space, and vertical dividers
  col_widths = attrs.map do |attr|
    [headings[attr].size, hashes.map { |h| h[attr].size }.max].max
  end

  # If the desired width, including delimiters, exceeds the limit,
  # reduce column widths proportionally
  desired_width = col_widths.inject(:+) + 3 * col_widths.size + 1
  if desired_width > width_limit
    col_widths.each_with_index do |width, c|
      col_widths[c] = (width.to_f * width_limit / desired_width).to_i - 2
    end

    # Readjust last column for round-off errors
    desired_width = col_widths.inject(:+) + 3 * col_widths.size + 1
    col_widths[-1] += width_limit - desired_width
  end

  output = hrule = '+' + col_widths.map { |width| '-' * (width + 2) + '+' }.join
  ([headings] + hashes).each_with_index do |row, r|
    output += "\n"
    output += "\e[7m" if r == 0         # Highlight heading row
    output += '|'
    attrs.each_with_index do |attr, c|
      output += sprintf(' %-*s |', col_widths[c], row[attr][0...col_widths[c]])
    end
    output += "\e[0m" if r == 0         # Turn off highlight
    output += "\n" + hrule
  end
  output
end


Uploading and downloading files with FTP I'm learning Python and decided to rewrite my FTP tool from Perl to Python. The program is able to up/download files, change the directory and list the files of the current directory.

Is there anything I can improve?

#!/usr/bin/python

import time
import sys
import os
from ftplib import FTP
import getpass

def menue():
    print "1) Verzeichnisse auflisten"
    print "2) Verzeichnis wechseln"
    print "3) Datei hochladen"
    print "4) Datei runterladen"
    print "9) Programm beenden"
    choice = raw_input("Was wollen Sie tun? ")
    return choice

def connect(counter):
    user = raw_input("User: ")
    password = getpass.getpass()
    ftp = FTP('xxxxxx')
    ftp.login(user, password)
    counter += 1
    return ftp, counter

def list_dir( ftp ):
    ftp.retrlines('LIST')

def chd_dir( ftp ):
    change_choice = raw_input("In welches Verzeichnis moechten Sie wechseln? ")
    if change_choice not in ftp.nlst():
        print "Das angegebene Verzeichnis existiert nicht!"
    else:
        ftp.cwd(change_choice)

def upl_file( ftp ):
    file_upl = raw_input("Bitte geben Sie den Dateinamen an: ")
    curr_dir = os.curdir
    if file_upl not in os.listdir(curr_dir):
        print "Die angegebene Datei existiert nicht!"
    else:
        ftp.storbinary('STOR ' +file_upl, open(file_upl, 'rb'))

def dwl_file( ftp ):
    file_dwl = raw_input("Bitte geben Sie den Dateinamen an: ")
    if file_dwl not in ftp.nlst():
        print "Die angegebene Datei existiert nicht!"
    else:
        ftp.retrbinary('RETR ' +file_dwl, open(file_dwl, 'wb').write)

counter = 0
while True:
    choice = menue()
    if choice == "9":
        print "Programm wird beendet..."
        time.sleep(1)
        sys.exit()
    if counter == 0:
        ftp, counter = connect(counter)
    if choice == "1":
        list_dir(ftp)
    elif choice == "2":
        chd_dir(ftp)
    elif choice == "3":
        upl_file(ftp)
    elif choice == "4":
        dwl_file(ftp)
    else:
        print "Falsche Eingabe!"

Python version 2.6.

You aren't doing proper error handling.  For example, changing the directory could fail due to a permission problem.  In addition to pre-screening each commands, you need to check the status returned by the server.  In RFC 959, linked from the ftplib documentation, you'll see that status codes ≥ 400 indicate an error.  (You'll need to include an English-to-German translation table for the errors listed in Section 4.2.2 of the RFC.  Unfortunately, the errors aren't very specific as to the exact cause.)


Checking HTTP headers with asyncio and aiohttp This is one of my first attempts to do something practical with asyncio. The task is simple:

  Given a list of URLs, determine if the content type is HTML for every URL.

I've used aiohttp, initializing a single "session", ignoring SSL errors and issuing HEAD requests to avoid downloading the whole endpoint body. Then, I simply check if text/html is inside the Content-Type header string:

import asyncio

import aiohttp

@asyncio.coroutine
def is_html(session, url):
    response = yield from session.head(url, compress=True)
    print(url, "text/html" in response.headers["Content-Type"])

if __name__ == '__main__':
    links = ["https://httpbin.org/html",
             "https://httpbin.org/image/png",
             "https://httpbin.org/image/svg",
             "https://httpbin.org/image"]
    loop = asyncio.get_event_loop()

    conn = aiohttp.TCPConnector(verify_ssl=False)
    with aiohttp.ClientSession(connector=conn, loop=loop) as session:
        f = asyncio.wait([is_html(session, link) for link in links])
        loop.run_until_complete(f)

The code works, it prints (the output order is inconsistent, of course):

https://httpbin.org/image/svg False
https://httpbin.org/image False
https://httpbin.org/image/png False
https://httpbin.org/html True

But, I'm not sure if I'm using asyncio loop, wait and coroutines, aiohttp's connection and session objects appropriately. What would you recommend to improve?

IMO your code should look more like this:

import asyncio
import aiohttp
URLS = [...]

if __name__ == "__main__":
    print(
        asyncio.get_event_loop().run_until_complete(
            asyncio.gather(*(foo(url) for url in URLS))))

Where individual URL is processed something like:

async def foo(url):
    async with aiohttp.ClientSession() as s:
        async with s.head(...) as r:
            return url, r.headers[...]

Note separate session for each URL.

Additionally, exception handling may be needed, in which case, it should be encapsulated inside foo.


Find maximum element in bitonic array An array is bitonic if it is composed of an increasing sequence of integers followed immediately by a decreasing sequence of integers. Write a program that, given a bitonic array of N distinct integer values, determines whether a given integer is in the array.

I have divided the problem in two sub-problems:

Find the maximum.
Do the binary search on left and right half.

Here is the complete solution:

def bin_search(ary, elem, low, high):
    """ Search element in array of ascending order."""
    # Gaurd clause, if element is not in array,
    if low > high:
        return None
    mid = low + (high - low) / 2
    if (ary[mid] < elem):
        return bin_search(ary, elem, mid + 1, high)
    elif (ary[mid] > elem):
        return bin_search(ary, elem, low, mid - 1)
    else:
        return mid

def bin_search_reverse(ary, elem, low, high):
    """ Search element in array of descending order."""
    # Gaurd clause, if element is not in array,
    if low > high:
        return None
    mid = low + (high - low) / 2
    if (ary[mid] > elem):
        return bin_search_reverse(ary, elem, mid + 1, high)
    elif (ary[mid] < elem):
        return bin_search_reverse(ary, elem, low, mid - 1)
    else:
        return mid

def find_max_bitonic(ary):
    def recurse(low, high):
        mid = low + (high - low) / 2
        # Handle base cases first.
        if (high - low == 1):
            return high

        if (ary[mid] < ary[mid+1]):
            return recurse(mid, high) # Go right.
        else:
            return recurse(low, mid)
    return recurse(0, len(ary) - 1)

def search(ary, elem):
    maximum = find_max_bitonic(ary)
    if (ary[maximum] == elem):
        return maximum
    left = bin_search(ary, elem, 0, maximum - 1)
    right = bin_search_reverse(ary, elem, maximum + 1, len(ary) - 1)
    return left or right

ary = (5, 15, 25, 35, 40, 45, 20, 10, 2)
print search(ary, 20)

I think there is a scope of lot of improvements. I personally think writing two separate methods for binary search can be avoided but don't know if there is any elegant alternative.

Running time

\$log_{2}O(N)\$(Finding maximum) + 2\$log_{2}O(N)\$(Binary search) = \$3log_{2}O(N)\$

Some comments based on my first impression:

Your code is a bit sparse on comments or documentation. Some more of that would be nice, in particular explain what arguments I’m supposed to supply to functions, and how I should interpret the return values.
Don’t skimp on characters for variable names. ary ~> array, elem ~> element. More expressive variable names are better; there's no need not to use full words.
The problem states “determine whether a given integer is in the array”, which sounds like a boolean. As such, I expected your search() function to return True/False.

Instead it returns a number (the index of the element if present) or None. That’s not wrong, but a docstring would have been helpful there.

On to more substantive comments:

You can definitely reduce the amount of code duplication in the two binary search functions.

Right now the binary search functions are responsible for picking the subset of the array that they search. I think that would be better handled by the caller – they should choose which subset of the array to inspect. That allows you to tidy up the search:

def binary_search(array, element):
    """Given an array of elements sorted in ascending order,
    perform a binary search to find this element.
    """
    ...

and then when you want to do a search in descending order, you can reverse the array before passing it in. Alternatively, you could do a check for the direction of the array when it’s passed into this function, and reverse it if necessary.

Then in your search function, take slices of the array and pass it into this function.
You can speed up some cases with a simple bounds check on the two halves of the array. Check that the element is between the first and last elements of the array.

For example: if I have a sorted array of 100 elements [1, ..., 1000], I know that –5 isn’t in that array just by inspecting the first and last numbers, without doing a binary search.

Adding a check that a number could plausibly be in an array will save you some operations.
In the search function, you check both the left and right-hand arrays before returning a result. If you find the element in the left-hand array, the result won’t change based on the right-hand array, so checking it is wasting CPU cycles.

A better approach would be something like:

if bin_search(ary, elem, 0, maximum - 1):  # left-hand array
    return True
elif bin_search_reverse(ary, elem, maximum + 1, len(ary) - 1)  # right-hand
    return True
else:
    return False

This is called short-circuit evaluation. You might be able to fit it onto one line – the or statement does this automatically – but the example above illustrates the principle.


SOXXY: a daemon stated IRC client project This script is basically a proxy service; except it doesn't require an active client connection. In fact it expects the client connection to connect before, and disconnect after every transaction. Once running the proxy will buffer all incoming bytes, and will dump those bytes to the client upon request. Before I go any further and begin working on the IRC client intended to be used with this, I am posting here to persuade any feedback regarding the project in its current state from readers here:

soxxy.py

#! /usr/bin/python3

from socket import socket
from threading import Thread as thread
from sys import getdefaultencoding as enc
from sys import byteorder
from sys import argv

enc = enc()
CNCT, SEND, RECV, DISC, STAT = 0, 1, 2, 3, 4

class sox:
        def recv_t(self, s):
                while self.run:
                        try: self.buff += s.recv(8192)
                        except: continue
        def process(self, lo):
                try: bs = lo[0].recv(8192)
                except: return
                op, data = bs[0], bs[1:]
                if op is CNCT:
                        data = str(data, enc)
                        host, port = data.split(":")
                        self.sock = socket()
                        self.sock.connect((host, int(port)))
                        self.run = True
                        thread(target=self.recv_t, args=(self.sock, )).start()
                elif op is SEND: self.sock.send(data)
                elif op is RECV: lo[0].send(self.buff); self.buff = bytes()
                elif op is DISC: self.run = False; self.sock.close()
                elif op is STAT: bytes("error: %s" % (str(self.serr), ), enc)
        def server(self, port, host):
                ss = socket()
                ss.bind((host, int(port)))
                while True:
                        ss.listen(1)
                        self.process(ss.accept())
        def __init__(self, argv):
                self.sock = None
                self.buff = bytes()
                self.serr = False
                self.server(argv[1], argv[2])

if __name__ == "__main__":
        sox([argv + ["127.0.0.1", ], argv][len(argv) == 3])

IRC

#! /usr/bin/python3

from soxxy import *

lo_host = None
lo_port = None
remote_host = None
remote_port = None

def send(bs):
        s = socket()
        s.connect((lo_host, int(lo_port)))
        s.send(bs)
        x = s.recv(8192)
        s.close()
        return x

if __name__ == "__main__":
        lo_host, lo_port, remote_host, remote_port = argv[1:]
        send(bytes([CNCT, ] + [ord(c) for c in remote_host + ":" + remote_port]))
        from time import sleep
        sleep(8)
        print(str(send(bytes([RECV, ])), enc))
        send(bytes([DISC, ]))

Test case

$ cat test.sh
./soxxy.py 8000 & sleep 2 && \
./irc 127.0.0.1 8000 irc.gnu.org 6667
$ sh test.sh
:morgan.freenode.net NOTICE * :*** Looking up your hostname...
:morgan.freenode.net NOTICE * :*** Checking Ident
:morgan.freenode.net NOTICE * :*** Found your hostname
:morgan.freenode.net NOTICE * :*** No Ident response

I believe enc = enc() is considered an anti-pattern, because the semantics of enc changes. I'd suggest renaming the variable to encoding.

Rather than a long if..elif..else block you could use a simple handle_operation(op) call.

Since there's effectively no exception handling it will be difficult to diagnose errors.

Using argparse would make it possible to understand how to use the code without reading it. As a bonus it adds -h/--help support by default so users can see what the expected call structure is.

sleeping to wait for an end point to be available is an anti-pattern. To guarantee that an end point is up and that you get started processing as quickly as possible it's common to poll the end point until it responds correctly or fail if there's a timeout.


Sendgrid + Mako email templates quick code review I created small module to help me create rich sendgrid html emails using the mako templating engine.  Any feedback on the design would be appreciated:

makosg.py

from sendgrid.message import Message
from mako.template import Template
from mako.lookup import TemplateLookup

class MakoMessage(Message):

    def __init__(self, addr_from, subject, vars={}, text="",
                 temp_path=None, temp_filename=None):
        html = self.render_template_output(vars, temp_path, temp_filename)
        subject = self._render(subject, vars)
        text = self._render(text, vars)
        super(MakoMessage, self).__init__(addr_from, subject, text, html)

    def render_template_output(self, vars, temp_path, temp_filename):
        if temp_path and temp_filename:
            lookup = TemplateLookup([temp_path])
            template = lookup.get_template(temp_filename)
            return template.render(**vars)
        return ""

    def _render(self, temp_data, vars):
        template = Template(temp_data)
        return template.render(**vars)

tests.py

import unittest

class MakoMessageTests(unittest.TestCase):

    def _callUT(self, addr_from, subject, vars={}, text="", temp_path=None,
                temp_filename=None):
        from makosg import MakoMessage
        return MakoMessage(addr_from, subject, vars, text, temp_path,
                           temp_filename)

    def test_should_render_basic_txt_template(self):
        message = self._callUT('test@example.com', 'Hello ${name}',
                               vars={'name': 'bob'},
                               text="How's it going ${name}?",
                               )
        self.assertTrue('bob' in message.subject, message.subject)
        self.assertTrue('bob' in message.text, message.text)
        self.assertTrue(not message.html, message.html)

    def test_should_render_html_file(self):
        message = self._callUT('test@example.com', 'Hello ${name}',
                               vars={'name': 'jim'},
                               text="",
                               temp_path='./makosg/test_templates/',
                               temp_filename='example.mako')
        self.assertTrue('
jim
' in message.html, message.html)
        self.assertTrue(not message.text, message.text)

    def test_should_render_rich_html_message(self):
        message = self._callUT('test@example.com', 'Hello ${name}',
                               vars={'name': 'Brian', 'fruits':
                                     ['apple', 'pear', 'berry']},
                               text="My favorite fruit is an ${fruits[0]}",
                               temp_path='./makosg/test_templates/',
                               temp_filename='example2.mako')
        self.assertTrue('
apple
' in message.html and \
                        '
pear
' in message.html and \
                        '
berry
' in message.html and \
                        '
Hello Brian
' in message.html, message.html)
        self.assertTrue('apple' in message.text, message.text)
        self.assertTrue('Brian' in message.subject, message.subject)

https://github.com/bcajes/makosg

Here a few things I spotted (not necessarily design related):

vars is a builtin, and should be avoided as a variable name.  Many template systems call this the context (what you are calling vars), so that's probably a better name.
Defaulting function parameters to mutable objects (as you do in the line below) is dangerous (can produce unexpected results, if you're not familiar with how python handles function-param defaults).  See Why are default arguments evaluated at definition time in Python? For more information.

def __init__(self, addr_from, subject,context={},text="",

Instead, you should do something like this:

def __init__(self, addr_from, subject, context=None, text="",
             temp_path=None, temp_filename=None):
    context = context or {}

I'm not sure the if temp_path ... / return "" is the best design.  It took me a bit longer than it should to figure out why you were doing that.  One alternative would be to move the conditional into the __init__ function, so it doesn't look like an error is being handled with return "".


Small routing system I've written a small routing system in PHP but I'm not sure if it's done the right way and what changes can they be done to the code and improvements.

<?php
namespace Twelve\Router;

    use Twelve\Utils\Utils;
    class Router
    {
        public
            $routeMaps = [],
            $controller;
        private $_route = [];

        public function add($name, $pattern, $controller, array $params = [])
        {
            if(!isset($this->routeMaps[$name]))
                $this->routeMaps[$name] =
            [
                'pattern' => $pattern,
                'controller' => $controller,
                'params' => $params,
            ];
        }
        public function findMatch($url)
        {
            foreach($this->routeMaps as $routeMap)
            {
                $this->regex = $this->buildRegex($routeMap['pattern'], $routeMap['params']);
                // Let's test the route.
                if(preg_match($this->regex, $url))
                {
                    return (array) $routeMap['controller'];
                }
            }
        }
        public function buildRegex($uri, array $params)
        {
            // FInd {params} in URI
            if(preg_match_all('/\{(?:[^\}]+)\}/', $uri, $this->matches, PREG_SET_ORDER))
            {
                foreach($this->matches as $isMatch)
                {
                    // Swap {param} with a placeholder
                    $this->uri = str_replace($isMatch, "%s", $uri);
                }
                // Build final Regex
                $this->finalRegex = '/^' . preg_quote($this->uri, '/') . '$/';
                $this->finalRegex = vsprintf($this->finalRegex, $params);

                return $this->finalRegex;
            }
        }
        public function dispatch(array $route = [], $url)
        {
            $this->setController($route['0']);
            $this->setAction($this->getController());
            $this->setParams(explode('/', $url));
            $this->controller = $this->getController();
            array_pop($this->controller);
            $this->controller = implode('\\', $this->controller);
            call_user_func_array([new $this->controller, $this->action], $this->params);
        }

        public function setController($controller)
        {
            $this->controller = explode(':', $controller);
        }

        public function getController()
        {
            return $this->controller;
        }
        public function setAction($action)
        {
            $this->action = end($action);
        }
        public function getAction()
        {
            return $this->action;
        }

        public function setParams($params)
        {
            $this->params = array_slice($params, 4);
        }

        public function getParams()
        {
            return $this->params;
        }
        public function run($uri, array $route = null)
        {
            $route = $this->findMatch($uri);
            Utils::var_dump($route);
            $this->dispatch($route, $uri);
        }
    }

Here are my observations

NOTE I did all this via stream-of-conscientiousness and I haven't tested any of these changes (or your original code) so take these as guidance and not exact changes :)

router.php

<?php
class Router
{
    // TODO You need to use `private` a lot more for encapsulation
    private $routeMaps = [];

    // TODO This function needs to document the expected format of inputs
    // TODO is initial controller format supposed to be 'full:class:path:method' ?
    public function add($name, $pattern, $controller, array $params = [])
    {
        // TODO What if it is already set?
        if(!isset($this->routeMaps[$name]))
        {
            $this->routeMaps[$name] =
            [
            'pattern'    => $pattern,
            'controller' => $controller,
            'params'     => $params,
            // Build the regex now so we ensure we only build it once
            'regex'      => $this->buildRegex($pattern, $params),
            ];
        }
        else
        {
            throw new Exception("Route '{$name}' already defined");
        }
    }

    // TODO Document function

# why the '$route = null'?  It is not used
#?? public function run($uri, array $route = null)
    public function run($uri)
    {
        $routeMap = $this->findMatch($uri);
#       Utils::var_dump($routeMap);
        // TODO check to see if we actually found a match
        if (null !== $routeMap)
        {
            // TODO we should probably parse the parms before calling dispatch
            $this->dispatch($routeMap, $uri);
        }
        // TODO probably want to notify user somehow
        else
        {
            ; // ??
        }
    }

    // TODO Why are you calling this parm $uri instead of $pattern?
    private function buildRegex($uri, array $params)
    {
        // FInd {params} in URI
        if(preg_match_all('/\{(?:[^\}]+)\}/', $uri, $matches, PREG_SET_ORDER))
        {
            foreach($matches as $isMatch)
            {
                // Swap {param} with a placeholder
# matching $uri and setting $this->uri means only the last match is saved
# Is that what you want?  If so, document it
#??             $this->uri = str_replace($isMatch, "%s", $uri);
                $uri = str_replace($isMatch, "%s", $uri);
            }
            // Build final Regex
            $finalRegex = '/^' . preg_quote($this->uri, '/') . '$/';
            $finalRegex = vsprintf($finalRegex, $params);

            return $finalRegex;
        }
        // TODO Need to throw error if uri doesn't match
        else
        {
            throw new Exception("Invalid uri: '{$uri}'");
        }

    }

    // TODO Why does (did) this return an array of one element, containing just the
    //      controller?
    // TODO Shouldn't this return the routemap instead of just the controller?
    private function findMatch($url)
    {
        foreach($this->routeMaps as $routeMap)
        {
            // Let's test the route.
            if(preg_match($routeMap['regex'], $url))
            {
                return $routeMap;
            }
        }

        return null; // TODO We should return something if no match
    }

    // TODO This should take the already parsed $parms
    private function dispatch($routeMap, $url)
    {
        $parts = explode(':', $routeMap['controller']);
        $action = array_pop($parts);
        $clazz = implode('\\', $parts);
        $params = $this->parseParams($url);
        call_user_func_array([new $clazz, $action], $params);
    }

    // TODO This needs to be based on the pattern in the rout map
    // TODO Since it appears that the 'params' in the route map have to
    //      match in order for the url to match, we should propably use
    //      the length of those in our parsing here.
    // TODO Perhaps we could even use the regex to tell these values at
    //      match time?
    private function parseParams($url)
    {
        return  array_slice(explode('/', $url), 4);
    }

}