Number to words in Python 3 Here is a sample program I have written to convert numbers to words:

ONES = [None, 'one', 'two', 'three', 'four',
        'five', 'six', 'seven', 'eight', 'nine']

# Empty string for zero will be replaced in main function

TEENS = ['ten', 'eleven', 'twelve', 'thirteen', 'fourteen',
         'fifteen', 'sixteen', 'seventeen', 'eighteen', 'nineteen']

TENS_PREFIXES = ['_', '_', 'twen', 'thir', 'for',
                 'fif', 'six', 'seven', 'eigh', 'nine']

# Tens places words without "-ty". ex. twenty > twen, thirty > thir

POWERS_OF_1000 = ['', 'thousand', 'million', 'billion', 'trillion',
                  'quadrillion', 'quintillion', 'sextillion',
                  'septillion', 'octillion']

def to_base_1000(number):
    # To split a number into chunks of three digits
    # ie. (fifty three thousand   two hundred one)

    digits = []
    while number:
        number, digit = divmod(number, 1000)
        digits.append(digit)

    return digits

def to_hundreds_tens_ones(number):

    assert(0 <= number < 1000)

    return number // 100, number // 10 % 10, number % 10

def two_digit_to_words(tens, ones):

    if tens == 0:
        return ONES[ones]

    elif tens == 1:
        return TEENS[ones]

    elif ones == 0:
        return '{}ty'.format(TENS_PREFIXES[tens])
        # This won't index to the underscores (positions 0 and 1)
        # because of the first two ifs

    else:
        return '{}ty {}'.format(TENS_PREFIXES[tens], ONES[ones])

def three_digit_to_words(hundreds, tens, ones):

    last_two_digits = two_digit_to_words(tens, ones)
    if hundreds == 0:
        return last_two_digits

    else:
        if last_two_digits is None:
            return '{} hundred'.format(ONES[hundreds])

        else:
            return '{} hundred {}'.format(ONES[hundreds], last_two_digits)

def chunk_to_words(chunk, position):

    multiplier = POWERS_OF_1000[position]
    # Thousand, million, etc.

    if multiplier:
        multiplier = ' ' + multiplier

    # Add space if it exists so that it's not "onemillion" "twothousand" etc.

    hundreds, tens, ones = to_hundreds_tens_ones(chunk)

    return three_digit_to_words(hundreds, tens, ones) + multiplier

def number_to_words(number):

    if number == 0:
        return 'zero'

    chunks = to_base_1000(number)

    chunk_words = []

    for position, chunk in enumerate(chunks):
        chunk_words.append(chunk_to_words(chunk, position))

    return ' '.join(chunk_words[::-1])

    # Reverse because to_base_1000 returns chunks from smallest to largest

I would like some advice on improving my programming style and my commenting.

Bug

I generally like your logic.  You have a bug, though:

>>> number_to_words(1_000_001)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "cr175527.py", line 94, in number_to_words
    chunk_words.append(chunk_to_words(chunk, position))
  File "cr175527.py", line 81, in chunk_to_words
    return three_digit_to_words(hundreds, tens, ones) + multiplier
TypeError: unsupported operand type(s) for +: 'NoneType' and 'str'

The problem is that for middle chunk of three digits, three_digit_to_words(0, 0, 0) evaluates to None, which isn't properly handled.

Comments and style

I find it annoying that your comments often appear after the corresponding code.  It is more conventional to either put the comment before the code, or at the end of the same line.

The most important kind of comment is the docstring, and you didn't write any.  (Your comment on to_base_1000() was almost like a docstring.)  Fortunately, your function names were clear enough to indicate their purpose.  Still, one feature of docstrings that you are missing out on is the ability to embed doctests, which would be extremely helpful for this kind of code.  For each function, a simple example, a complex example, and some corner cases would clarify exactly how it behaves.

I would change your brace style for the definitions of ONES, TEENS, etc. so that they all have the same indentation.  Also, if you write ONES = [None, 'one', …], then you should also be consistent and write TENS_PREFIXES = [None, None, 'twen', …].

Logic and organization

I am slightly annoyed by your

TENS_PREFIXES = ['_', '_', 'twen', 'thir', 'for',
                 'fif', 'six', 'seven', 'eigh', 'nine']

# Tens places words without "-ty". ex. twenty > twen, thirty > thir

There is no situation in which you would ever output "twen" rather than "twenty", so why skimp on those letters, when you could have written 'twenty' and avoided the need for that apologetic comment?

If you use "chunk" as your terminology for a block of three digits, consider using that name instead of to_base_1000().  Then, your numbers_to_words() function would be a composition of chunk_to_words() and number_to_chunks(), which I find pleasing.

In my opinion, you would be better off rolling the helper functions to_hundreds_tens_ones(), two_digit_to_words(), and three_digit_to_words() into chunk_to_words().  The code would be more readable with less jumping around.

A lot of the conditions exist to ensure that exactly one space appears between words.  For example:

if last_two_digits is None:
    return '{} hundred'.format(ONES[hundreds])

else:
    return '{} hundred {}'.format(ONES[hundreds], last_two_digits)

I would prefer to yield each word as a generator, then let ' '.join(…) take care of inserting the spaces.

Suggested solution

ONES = [
    None, 'one', 'two', 'three', 'four',
    'five', 'six', 'seven', 'eight', 'nine'
]

TEENS = [
    'ten', 'eleven', 'twelve', 'thirteen', 'fourteen',
    'fifteen', 'sixteen', 'seventeen', 'eighteen', 'nineteen'
]

TENS = [
    None, None, 'twenty', 'thirty', 'forty',
    'fifty', 'sixty', 'seventy', 'eighty', 'ninety'
]

POWERS_OF_1000 = [
    None, 'thousand', 'million', 'billion', 'trillion',
    'quadrillion', 'quintillion', 'sextillion', 'septillion', 'octillion'
]

def number_to_chunks(number):
    """
    Split an integer into little-endian base-1000 chunks.

    >>> list(number_to_chunks(1234))
    [234, 1]
    >>> list(number_to_chunks(31415000))
    [0, 415, 31]
    >>> list(number_to_chunks(0))
    []
    """
    while number:
        number, n = divmod(number, 1000)
        yield n

def chunk_to_words(chunk, scale):
    """
    Generate English words given a chunk (an integer less than 1000) and its
    scale (power of 1000).

    >>> list(chunk_to_words(31, 1))
    ['thirty', 'one', 'thousand']
    >>> list(chunk_to_words(50, 2))
    ['fifty', 'million']
    >>> list(chunk_to_words(0, 0))
    []
    >>> list(chunk_to_words(12, 3))
    ['twelve', 'billion']
    """
    assert 0 <= chunk < 1000
    hundreds, tens, ones = chunk // 100, chunk // 10 % 10, chunk % 10

    if hundreds:
        yield ONES[hundreds]
        yield 'hundred'

    if tens == 1:
        yield TEENS[ones]
    else:
        if tens:
            yield TENS[tens]
        if ones:
            yield ONES[ones]

    if chunk and scale:
        yield POWERS_OF_1000[scale]

def number_to_words(number):
    """
    Convert a nonnegative integer less than 10**30 to an English phrase.

    >>> number_to_words(103)
    'one hundred three'
    >>> number_to_words(0)
    'zero'
    >>> number_to_words(1_000_001)
    'one million one'
    >>> number_to_words(31_415_926_535)
    'thirty one billion four hundred fifteen million nine hundred twenty six thousand five hundred thirty five'
    """
    chunks = [
        ' '.join(chunk_to_words(chunk, i))
        for i, chunk in enumerate(number_to_chunks(number))
    ]
    return ' '.join(c for c in chunks[::-1] if c) or 'zero'


Ajax plugin that binds to click and page load I have an app that has a number ajax requests and instead of writing them all out, I created a plugin that takes options based on the request I am making.

I would like the ajax requests to occur when page loads and when the user clicks, mouses over certain buttons.

I decided to use the jquery.bind() method to bind the ajax request to what event takes place. But I am running into a problem when the event is 'load' and the jquery selector is $('window') or document.

Currently, I put a conditional statement to make it work, but then I have write the same function twice just for the 'load' event like this:

(function($) {

    $.fn.getData = function(options) {

        var defaults = {
            eventType : "load",
            // more default options
        }

        var options = $.extend(defaults, options);

        if (options.eventType == "load")
        {
            $.ajax({
                // ajax object with options
            });
        }
        else {
            return this.bind(options.eventType, function() {

                $.ajax({
                    // ajax object with options
                });

            return false;

            });
        }
    }
})(jQuery);

So when on my view pages, I call the plugins like this:

// for click events
$('#selector').getData({
    eventType : 'click',
    // other default options
});

// need ajax when page loads
$('window').getData({
    eventType : 'load',
    // other default options
});

I don't know if there is anyway around writing the function twice and I'm also unsure that it's even good practice to bind event handler to the Window's page load event, but not sure else how to make this plugin flexible to work with browser events like page load and user events like click, mouseover.

Any help would be greatly appreciated.
Thanks

Edit: The part below was, I think, me not seeing the forest for all the trees. The issue is quite simply that you're passing the string 'window' to jQuery, rather than the object window. So jQuery tries to bind an event listener to a <window> HTML element - which, obviously, does not exist.

What you want, is

$(window).bind("load", function () { .... });

or, better:

$(window).on("load", function () { .... });

or

$(window).load(function () { .... });

or, simply

$(function () { .... });

basically, anything other than trying to pass the string 'window'

Actually, the least flexible part of your code are the ajax requests themselves. If the ajax options are hard-coded in the plugin, you still don't have much flexibility; it's the same ajax requests each time. For instance, if you want to load ajax on a scroll event, you still have to edit the plugin to add the ajax request options and/or add an if(options.eventType === "scroll"). So you're splitting your code up, but not gaining any real flexibility.

I'd suggest making a simple function that takes an element, an event type, and some ajax options, so you can bind any event to any ajax request. Something like

function bindAjax(element, event, ajaxOptions) {
  ajaxOptions = $.extend({
    // default ajax options, if any
  }, ajaxOptions);

  $(element).on(event, function (event) {
    $.ajax(ajaxOptions);
    event.preventDefault();
  });
}

bindAjax(window, "load", { ... });

You don't gain that much by doing this, though, compared to "raw" jQuery code.


Secret hangman homework Recently I was given homework to make a hangman game, where I wrote the below.
How can I refactor this to use classes?

import secrets

def guess_the_word():
    tries_left = 9
    output = ["_"] * len(secret_word)

    print(str(' '.join([letter for letter in output])).strip())

    while tries_left >= 0 and not secret_word == str(''.join([letter for letter in output])):
        # print tries left
        print("\nYou have {} tries left".format(tries_left))
        # display the _ which is length of the secret_word and fill in _ as and when the user
        # types correct word

        guessed_letter = input("Guess a letter from secret word:")

        if guessed_letter in secret_word:
            print("You guessed correct")
            for i, x in enumerate(secret_word):
                if x is guessed_letter:
                    output[i] = guessed_letter
        else:
            tries_left -= 1
            print("You guessed incorrect")

        print(str(' '.join([letter for letter in output])).strip())

print("\nWelcome to Hangman Game!\n")
# open words.txt file to read words
with open("words.txt") as words_file:
    words_list = words_file.read().split()
# choose a random word from words_list
secret_word = secrets.choice(words_list)
guess_the_word()

How can I refactor this to use classes?

You don't. There is absolutely no need for that in this program as you don't share a common state between multiple functions. On the same subject, see also this PyCon talk.

Instead, we should focus on clearing the obvious mistakes made in this function:

Be sure to pass information to functions using parameters: global variable can be fine for constants, not so much for information to work on; besides if I import your code in an interactive session, I am greeted with a game but any other attempt to call guess_the_word() will reuse the same word to guess.
This also lead to how you call your code: running the program on the command line will play a game, but importing it will play it too; which may be undesirable. To avoid this side effect, get yourself familiar with the if __name__ == '__main__': construct.
secrets is a module designed for cryptographically secure sources of random numbers. You don't need such strong randomness here so you can avoid the associated overheads using the simpler random module.
You can join directly on lists: no need to write a list-comprehension which will just copy the underlying list before feeding it to join. In the same vein, striping whitespace and converting to str is superfluous here as the result of join already fulfill the two conditions.
Checking if a letter is in a string is an \$\mathcal{O}(n)\$ operation, so you can skip that and directly perform the iteration over each letter it won't cost more. You can use a flag to know if you have found a matching letter.
A message at the end of the while loop to conclude would be a great addition to the user experience.

All in all, this could yield the following code:

import random

def guess_the_word(secret_word):
    tries_left = 9
    output = ["_"] * len(secret_word)

    while tries_left >= 0 and secret_word != ''.join(output):
        print(' '.join(output))
        print("\nYou have", tries_left, "tries left")

        guessed_letter = input("Guess a letter from secret word: ")

        found = False
        for i, letter in enumerate(secret_word):
            if guessed_letter == letter:
                output[i] = guessed_letter
                found = True

        if found:
            print("You guessed correct")
        else:
            tries_left -= 1
            print("You guessed incorrect")

    if tries_left < 0:
        print("\nNo more tries, you lose")
    else:
        print("\nCongratz! You found the secret word:", secret_word)

def main(filename="words.txt"):
    print("\nWelcome to Hangman Game!\n")

    with open(filename) as words_file:
        words = words_file.read().split()

    guess_the_word(random.choice(words))

if __name__ == '__main__':
    main()

An other nice addition to have is the ability to repeat the games automatically. You could change your main part to:

def main(filename="words.txt", repeat=sys.maxsize):
    print("\nWelcome to Hangman Game!\n")

    with open(filename) as words_file:
        words = words_file.read().split()

    for _ in range(repeat):
        guess_the_word(random.choice(words))

if __name__ == '__main__':
    main(repeat=1)

to keep the current behaviour but allows importing the file to run main() the desired amount of time without having to reload the file each time.


Kill All Connections For Domain Account Users I have an application that has hundreds of users. Every now and again we need all users out of the application to run apply a fix on the data in DB (edit tables, causing triggers to be disabled during fix, etc). We don't want to set the DB to SINGLE_USER mode, which would kill the connections and ROLL BACK TRANSACTIONS, since often the work is being done by a third part remotely. So, we wrote the script below to remove Domain users and then set it to RESTRICTED_USER. I'm not overly concerned with performance since the query shouldn't take long, but am concerned with unanticipated results, underlying flaws that I'm not aware of, and what would be best practice in this case.

SET QUOTED_IDENTIFIER ON
GO
SET ANSI_NULLS ON
GO

declare @domain varchar(64) = 'myDomain'

--used to exclude domain logins running on the server, like an agent account, etc...
declare @server varchar(64) = 'theServer'

--create temp to store sp_who2 results
if object_id('tempdb..#Who2') is not null drop table #Who2
create table #Who2 (
                    SPID int,
                    Stat varchar(4000),
                    LoginUser varchar(256),
                    HostName varchar(256),
                    BlkBy varchar(256),
                    DBName varchar(256),
                    Command varchar(4000),
                    CPUTime bigint,
                    DiskIO bigint,
                    LastBatch varchar(64),
                    ProgramName varchar(256),
                    SPID2 int,
                    RequestID int
                    )

--load sp_who2 results
insert into #Who2 exec sp_who2

--delete all users who aren't domain accounts
--remove your SPID from the list to be deleted
--remove anything running locally on the box
delete from #Who2
where
    LoginUser not like + @domain + '%'
    or HostName = @server
    or SPID = @@SPID

--return the logins that will be dropped
select LoginUser as LoginsToBeDropped from #Who2

--cursor for the SPIDs
declare cur cursor fast_forward for
    select
        SPID
    from #Who2

declare @Spid varchar(16)
declare @sql varchar(4000)

--loop through the SPIDs and kill them
open cur
fetch next from cur into @Spid

while @@FETCH_STATUS = 0

    begin
        --select @Spid
        set @sql = 'kill ' + @Spid
        exec(@sql)
        fetch next from cur into @Spid
    end

close cur
deallocate cur
go

--see what connections remain
exec sp_who2

When you want users out of the database, do you have a timeframe as to when they should be allowed back in? The script you have above, will get rid of the users, but those users can just create a new connection to the database. What will you do when the user reconnects? Do you need to keep them out of the database for the entire time of the maintenance? What is the goal with keeping the users out of the database during the maintenance?

Additionally, if you kick people off out of the database, are you ok with losing the transaction that may be in flight?

What kind of maintenance do you want to do the database? Is it being done during a time of day when users should not be using the database?


Customised Java UTF-16 I have implemented customized encoding mechanism for javaUTF16. Does this implementation support all the characters?

public class Encoding {
    public static void main(String[] args) {

        byte [] arr = new byte[1000];

        String str = "abcde" ; //even this encoding works supplementary characters
        Encode(arr,0,str);
        System.out.println(Decode(arr,str.length()));
    }

public static byte[] Encode(byte[] ByteArray , int offset ,String str) {

        char[] ch = str.toCharArray();
        for(char c : ch) {
        ByteArray[offset++] = (byte) (c >>> 8);
        ByteArray[offset++] = (byte) (c & 0xff);
        }
        return ByteArray;
    }

    public static String Decode(byte[] ByteArray ,int len) {

        char [] res = new char[len*2];
        int i = 0;
        int offset = 0;
        while(i < len) {
    res[i] = (char) ((ByteArray[offset++] << 8) | (ByteArray[offset++] & 0xff));
            i++;
        }

        return new String(res);
    }
}

Question of Completeness

Yes, your code covers all Unicode characters, including the supplementary characters U+10000 to U+10FFFF, because you "inherit" that functionality from the way such characters would be stored in Java's String class:

  Unicode Character Representations

  The char data type (and therefore the value that a Character object
  encapsulates) are based on the original Unicode specification, which
  defined characters as fixed-width 16-bit entities. The Unicode
  Standard has since been changed to allow for characters whose
  representation requires more than 16 bits. The range of legal code
  points is now U+0000 to U+10FFFF, known as Unicode scalar value.
  (Refer to the definition of the U+n notation in the Unicode Standard.)

  The set of characters from U+0000 to U+FFFF is sometimes referred to
  as the Basic Multilingual Plane (BMP). Characters whose code points
  are greater than U+FFFF are called supplementary characters. The Java
  platform uses the UTF-16 representation in char arrays and in the
  String and StringBuffer classes. In this representation, supplementary
  characters are represented as a pair of char values, the first from
  the high-surrogates range, (\uD800-\uDBFF), the second from the
  low-surrogates range (\uDC00-\uDFFF).

  A char value, therefore, represents Basic Multilingual Plane (BMP)
  code points, including the surrogate code points, or code units of the
  UTF-16 encoding. An int value represents all Unicode code points,
  including supplementary code points. […]

Reinventing the Wheel

Since you did not tag your question as reinventing-the-wheel, I'm obligated to mention that you could accomplish the task more simply using the built-in support for charsets.

private static final Charset UTF_16 = Charset.forName("UTF-16BE");

public static byte[] Encode(byte[] ByteArray , int offset ,String str) {
    byte[] bytes = str.getBytes(UTF_16);
    System.arraycopy(bytes, 0, ByteArray, offset, bytes.length);
    return ByteArray;
}

public static String Decode(byte[] ByteArray ,int len) {
    return new String(ByteArray, 0, 2 * len, UTF_16);
}


finding boundary elements in cartesian grid I have a 2D cartesian grid (see picture below). The colorful cells are filled element. I would like to detect blue cells. Blue cells are bound  elements if you scan from bottom to top and left to right and vice versa.

I wrote following code when I do scan from bottom, but I do not know what would be the best for all four side scanning. I try to simulate on the simple example below with 8 cells in x direction and 5 cells in y direction. However, the real problem is order of (10000 x 10000) cells. The following code only find the two bottom blue cells.

#include "stdafx.h"
#include <vector>
#include <set>

#include "iostream"

struct cell
{
  cell(size_t i_x, size_t i_y)
    {
    x = i_x;
    y = i_y;
    }
  size_t x;
  size_t y;
  bool operator==(cell a)
    {
    if (a.x == x && a.y == y)
      return true;
    else
      return false;
    };
};

int main()
  {
  std::vector<cell> cartesian{ cell(0, 1), cell(0, 2), cell(0, 3),
    cell(1, 2), cell(1, 3), cell(1, 4), cell(1, 5),
    cell(2, 1), cell(2, 2), cell(2, 3), cell(2, 4), cell(2, 5),
    cell(3, 0), cell(3, 1), cell(3, 2), cell(3, 3), cell(3, 4), cell(3, 5), cell(3, 6),
    cell(4, 2), cell(4, 3), cell(4, 4), cell(4, 5), cell(4, 6) };
  size_t max_X = 8;
  size_t max_Y = 5;

  std::vector<size_t> bound_elements;
  for (size_t i = 0; i < max_X; ++i)
    {
    bool found = false;
    for (size_t j = 0; j < max_Y; ++j)
      {
      auto is_filled = std::find(cartesian.begin(), cartesian.end(), cell(i, j));
      if (is_filled == cartesian.end())
        {
        continue;
        }
      size_t position = (j + (max_X* i));

      if (bound_elements.size() == 2)
        {
        bound_elements[1] = position;
        }
      else
        {
        bound_elements.push_back(position);
        }
      found = true;
      }
    if (found)
      {
      break;
      }
    }
  for (auto element : bound_elements)
    {
    std::cout  << element << std::endl;
    }
  return 0;
  }

For top to down It is just opposite side for loop iteration. This strategy seems to be too verbose, is there a better way in terms of performance or coding style?

Headers

I had to ditch the non-standard and missing "stdafx.h", add <algorithm> and fix the spelling of <iostream> to compile this.

Correct std::size_t

This type is consistently written as size_t without its namespace prefix, which is not portable.

Cell type

struct cell could simply be a std::pair, but there may be benefit to stronger typing as you have it.  I recommend you prefer initialization of members rather than assignment (and that conveniently allows us to simply omit the constructor).  We can also change the == operator to take a reference and make it const (we could lean on std::tuple for the implementation, but it doesn't seem worth it for two members and only one operator).

struct cell
{
    const std::size_t x;
    const std::size_t y;

    bool operator==(const cell& a) const { return a.x == x && a.y == y; }
};

Algorithm

Repeatedly searching for elements looks like a very time-consuming way of examining every cell.  Instead, we could just iterate over the known cells and update a set of minima and maxima for its row and column:

#include <iostream>
#include <map>
#include <set>
#include <utility>
#include <vector>

struct cell
{
    const std::size_t x;
    const std::size_t y;
};

int main()
{
    std::vector<cell> cartesian = {
        {0, 1}, {0, 2}, {0, 3},
        {1, 2}, {1, 3}, {1, 4}, {1, 5},
        {2, 1}, {2, 2}, {2, 3}, {2, 4}, {2, 5},
        {3, 0}, {3, 1}, {3, 2}, {3, 3}, {3, 4}, {3, 5}, {3, 6},
        {4, 2}, {4, 3}, {4, 4}, {4, 5}, {4, 6}
    };

    // map each row and column to (min,max) filled cell in that column
    std::map<std::size_t, std::pair<std::size_t,std::size_t>> row_range;
    std::map<std::size_t, std::pair<std::size_t,std::size_t>> column_range;

    for (const auto& c: cartesian) {
        auto rit = row_range.find(c.y);
        if (rit != row_range.end()) {
            auto& row = rit->second;
            if (c.x < row.first) {
                row.first = c.x;
            } else if (row.second < c.x) {
                row.second = c.x;
            }
        } else {
            row_range.insert({c.y, {c.x, c.x}});
        }

        auto cit = column_range.find(c.x);
        if (cit != column_range.end()) {
            auto& col = cit->second;
            if (c.y < col.first) {
                col.first = c.y;
            } else if (col.second < c.y) {
                col.second = c.y;
            }
        } else {
            column_range.insert({c.x, {c.y, c.y}});
        }
    }

    // show the results for column 0
    const auto zr = column_range[0];

    std::cout  << zr.first << ',' << zr.second << std::endl;
}

We can refactor the part that's common to rows and columns into a small helper method:

using range_map = std::map<std::size_t, std::pair<std::size_t,std::size_t>>;

static void update_min_max(range_map& ranges, std::size_t index,
                           std::size_t value)
{
    auto it = ranges.find(index);
    if (it != ranges.end()) {
        auto& range = it->second;
        if (value < range.first) {
            range.first = value;
        } else if (range.second < value) {
            range.second = value;
        }
    } else {
        ranges.insert({index, {value, value}});
    }
}

And then the loop becomes

    range_map row_range;
    range_map column_range;

    for (const auto& c: cartesian) {
        update_min_max(row_range, c.y, c.x);
        update_min_max(column_range, c.x, c.y);
    }


Word analysis on input from a file How do I improve the below code, which takes input from a file and does some word analysis?  I find a lot of redundancy in it. I want to know a better way of implementing it.

analysis = {}
wordanalysis = {}
found = True

for line in sys.stdin:
   (n1,n2,n3,n4,n5,n6) = re.split("\t+",line.strip())
   tweet = re.split("\s+", n6.strip().lower())
   #adding tweet id to a dictionary

   for i in range(0, len(tweet)):
     print "{", n1, wordanalysis,"}"
     wordanalysis["word"] = tweet[i]
     wordanalysis["charcount"]= len(tweet[i])

     if len(tweet[i]) > 7:
        wordanalysis["longword"] = found
     else:
        wordanalysis["longword"] = not(found)
     #if 1st char is UpperCase
     if tweet[i].istitle():
        wordanalysis["title"] = found
     else:
        wordanalysis["title"] = not(found)

     if tweet[i].isupper():
        wordanalysis["uppercase"] = found
     else:
        wordanalysis["uppercase"] = not(found)

I would wrap the entire part of analyzing in a function:

def analyze_twees(tweets):

Then you could pass in content, from whichever source. It doesn't matter, whether the input comes from sys.stdin or a string, which you just generated. This makes the function easier to test, since you do not need passing a file into your function.
As 200_success said,

for i in range(0, len(tweet)):

is considered poor style in Python. According to (1) you should iterate over the tweets as suggested:

for tweet in tweets.

Your variable naming is very obscure. Think of future you coming up in 6 Months fixing a bug - do you understand your code at first glance? I doubt that.

n6.strip().lower()

One can only guess, what this is about. But if you wrote something like

tweeted_text = re.split("\s+", tweet.strip().lower())

one could imagine, what you intended to do.
You should split your concerns:

you are going to read a file line by line. So that should be one function.
you are printing something to the console. That would be another function.
you are analyzing the tweeted text. That would be the third function.

def print_tweet_analysis(dataset):
    for row in dataset:
        console_out(analyze(row))

That would be your complete algorithm in three lines of code.
If you come in 10 years from now back to your code, you unterstand, what this script is about: it takes data, analyzes it and puts the result to the console.
If you want to know, what analyze means, you look at the function and will see.
The same goes for your analyze function. You could fine grain your logic into small pieces, each sitting on the top of the other and if you read the code, you drill down as far as necessary to understand the code and maybe finding later on bugs and fixing those.

Instead of using a dictionary for your algorithm, I would recommend namedtuple. You could easily define your analysis-tuple as

Analysis = namedtuple("Analysis", "word charcount longword title uppercase")

and return simply:

return Analysis(tweet, len(tweet), len(tweet)>7, tweet.istitle(), tweet.isupper())

And you are done.

If you wrote your code like this, everybody including future you would know, what this is all about.

One thing, I still do not understand is:

You are defining tweet as

tweet = re.split("\s+", n6.strip().lower())

iterating over it and checking for

tweet[i].isupper()

Am I overseeing something or does this make no sense only to me?


Accessing time series development data from World Bank API I wrote a script that connects to World Bank API, gathers some data (indicators) for a number of countries, creates pd.DataFrame, and saves the result as xlsx file.

What I had in mind when I was writing the code:

I wanted to avoid constructing pd.DataFrame within the function or using the append method to store data in-memory (not sure if it helps in this case; is it a good practice in general?).
I thought it was a good idea to split the code into separate functions (one that iterates over data, one that makes requests for every page that is related to the indicator, and finally, the one that iterates over all indicators and calls other functions).
It was the first time I used yield from and I'm not sure if it's the correct way of using the construct.
Not sure about typings.

import os
import requests
import pandas as pd
from time import sleep
from itertools import chain
from datetime import datetime
from typing import List, Iterator

COUNTRIES = {
    "Baltic States": ["EST", "LVA", "LTU"],
    "Central Asia": ["KAZ", "KGZ", "TJK", "TKM", "UZB"],
    "Eastern Europe": ["BLR", "MDA", "UKR"],
    "Eurasia": ["RUS"],
    "Transcaucasia": ["ARM", "AZE", "GEO"]
}
INDICATORS = {
    "Infrastructure": ["EG.ELC.ACCS.RU.ZS", "IT.CEL.SETS"],
    "Medicine": ["SH.DYN.MORT", "SH.DYN.AIDS.ZS"],
    "Public Sector": ["GC.TAX.TOTL.GD.ZS", "MS.MIL.TOTL.TF.ZS"]
}

URL = "http://api.worldbank.org/v2/country/"
DATE = datetime.today().strftime("%Y-%m-%d %H_%M")
CURRENT_DIR = os.path.dirname(os.path.abspath(__file__))
ISO3_PARAM = ';'.join(chain(*COUNTRIES.values()))
INDICATOR_PARAM = list(chain(*INDICATORS.values()))

def mapping(s: pd.Series, metadict: dict) -> str:
    """ Assign categories to Series' values according to dict's keys. """

    return ' '.join((k) for k,v in metadict.items() if s in v)

def item_level(data: List) -> Iterator:
    """ Iterate over data and yield dict containing specific fields. """

    for item in data:
        yield {
            "iso3": item["countryiso3code"],
            "indicator": item["indicator"]["value"],
            "id": item["indicator"]["id"],
            "year": item["date"],
            "value": item["value"]
        }

def page_level(indicator: str) -> Iterator:
    """ Iterate over all pages related to specific indicator. """

    base_url = f"{URL}{ISO3_PARAM}/indicator/{indicator}?format=json"

    next_page = 1
    while True:
        meta, data = requests.get(f"{base_url}&page={next_page}").json()
        yield from item_level(data)

        num_pages, next_page = meta["pages"], next_page + 1
        if next_page > num_pages:
            break

        sleep(1)

def indicator_level(indicators: List) -> Iterator:
    """ Iterate over all indicators. """

    for indicator in indicators:
        yield from page_level(indicator)

def main():
    """ Create pd.DataFrame from generator and save it. """
    df = pd.DataFrame(indicator_level(INDICATOR_PARAM))
    df["year"] = df["year"].astype(int)
    df["region"] = df["iso3"].apply(mapping, metadict=COUNTRIES)
    df["category"] = df["id"].apply(mapping, metadict=INDICATORS)
    df.loc[df["year"] >= 1991].to_excel(f"{CURRENT_DIR}/../../data/raw/dataset_{DATE}.xlsx", index=False)

if __name__ == "__main__":
    main()

I would turn your mappings mapping countries and indicators around. Then you can simply do:

COUNTRIES = {"EST": "Baltic States", "LVA": "Baltic States", "LTU": "Baltic States", ...}

df["region"] = df["iso3"].map(COUNTRIES)
df["category"] = df["id"].map(INDICATORS)

If you are too lazy to manually invert the dictionary, or like the current structure more but still want to have the easier usage, just use this one-line function:

def invert(d):
    return {v: k for k, values in d.items() for v in values}

Instead of

ISO3_PARAM = ';'.join(chain(*COUNTRIES.values()))

Use

ISO3_PARAM = ';'.join(chain.from_iterable(COUNTRIES.values()))

This does not have any memory problems (not that that is a problem here, but it is good practice in general).

And if you follow the previous recommendation, replace values with keys.

I would slightly simplify your page_level function by inlining meta["pages"] and using the params argument of requests.get to pass the parameters

def page_level(indicator: str) -> Iterator:
    """ Iterate over all pages related to specific indicator. """
    url = f"{URL}{ISO3_PARAM}/indicator/{indicator}"
    page = 1
    while True:
        meta, data = requests.get(url, params={"format": "json", "page": page}).json()
        yield from item_level(data)
        page += 1
        if page > meta["pages"]:
            break
        sleep(1)

I must say that while I like that you separated things into their own function, I am not so sure about their names (I know, names are hard!). They convey only at which level a function operates, but not what it does.

My suggestions for names:

indicator_level -> get_dataframe (and directly return the dataframe) or get_data
page_level -> get_indicator_data
item_level -> extract_items


Compute the mean, variance and standard deviation of all pairs, triplets and sets of four My code sample for finding the mean for the case of pairs is:

import numpy as np
Data =[]
with open('testdata.txt') as my_file:
     for line in my_file:
         Data.append(line)
psize = Data.size*Data.size
p_avg =zeros(psize,float)
for i in range(0,Data.size):
    for j in range(0,Data.size):
        if i != j:
           p_avg[(Data.size*i)+j] = (Data[i]+Data[j])/2.0
        else:
           p_avg[(Data.size*i)+j] = 0.0
print p_avg

Any comments on this and hints for finding the mean and variance for triplets and sets of four efficiently would be most appreciated.

If you want to keep all results, this can be done more efficiently using meshgrid.  You can create N-dimensional grids of values, flatten them, then take the mean of the result.

So for the 2D case, meshgrid is pretty straightforward:

>>> a = np.arange(5)
>>> [a1, a2] = np.meshgrid(a, a)
>>> print(a1)
[[0 1 2 3 4]
 [0 1 2 3 4]
 [0 1 2 3 4]
 [0 1 2 3 4]
 [0 1 2 3 4]]
>>> print(a2)
[[0 0 0 0 0]
 [1 1 1 1 1]
 [2 2 2 2 2]
 [3 3 3 3 3]
 [4 4 4 4 4]]
>>> print(a1.ravel())
[0 1 2 3 4 0 1 2 3 4 0 1 2 3 4 0 1 2 3 4 0 1 2 3 4]
>>> print(a2.ravel())
[0 0 0 0 0 1 1 1 1 1 2 2 2 2 2 3 3 3 3 3 4 4 4 4 4]

As you can see, the flattened versions of a1 and a2 combined provide every possible combination.  This can be extended to higher dimensions just by putting more a values:

>>> a = np.arange(3)
>>> [a1, a2, a3] = np.meshgrid(a, a, a)
>>> b = np.vstack([a1.ravel(), a2.ravel(), a3.ravel()])
>>> print(b)
[[0 0 0 1 1 1 2 2 2 0 0 0 1 1 1 2 2 2 0 0 0 1 1 1 2 2 2]
 [0 0 0 0 0 0 0 0 0 1 1 1 1 1 1 1 1 1 2 2 2 2 2 2 2 2 2]
 [0 1 2 0 1 2 0 1 2 0 1 2 0 1 2 0 1 2 0 1 2 0 1 2 0 1 2]]

This can be extended to any number of dimensions, allowing of course for memory constraints.

You can generalize the meshgrid function call by using unpacking.  These two function calls are equivalent:

np.meshgrid(a, b, c)
np.meshgrid(*[a, b, c])

The * in this case means "unpack the list as function arguments" (also works with tuples, iteratirs, and many other things).  You can combine this with the [x]*n syntax, where n is a number.  This means "duplicate the contents of list [x] n times".  So [5]*4 is similar to [5, 5, 5, 5] ("similar", not "identical", but the differences are not relevant to what we are doing).

So you can combine this to make calling meshgrid with multiple values easier.  These three lines of code are the same:

np.meshgrid(a, a, a)
np.meshgrid([a, a, a])
np.meshgrid([a]*3)

You can exclude combinations where all the values are equal using logical indexing.  First, you determine whether each element is equal to the value of the first element in that combination.  Then you find combinations where that is True for all values.  Then you get the columns for which that isn't the case.  So something like:

>>> c = b[:, ~(b[0] == b).all(0)]
>>> print(c)
[[0 0 1 1 1 2 2 2 0 0 0 1 1 2 2 2 0 0 0 1 1 1 2 2]
 [0 0 0 0 0 0 0 0 1 1 1 1 1 1 1 1 2 2 2 2 2 2 2 2]
 [1 2 0 1 2 0 1 2 0 1 2 0 2 0 1 2 0 1 2 0 1 2 0 1]]

If you want to avoid memory issues you can make use of some internal knowledge of how numpy handles arrays.  Arrays are simply continguous (or strided, i.e. continous in steps) blocks of memory.  By working with this sort of memory, numpy can make use of very fast libraries for doing calculations on a block of memory at a time.  But since this is simply a sequence of bytes, there are different ways to look at this block of memory.  The same block could be seen as a 1D array of floats, a 2D array of integers, etc.  Numpy can make use of this to avoid making unnecessary copies.  So arr.ravel(), for example, "flattens" an array by treating the same underlying data as a 1D array.  The catch (or benefit, depending on your use-case) is that changes to a raveled array will affect the original.

meshgrid can make use of this to avoid making unnecessary copies.  Consider this:

>>> a = np.arange(10)
>>> b = np.meshgrid(*[a, a, a, a])
>>> c = np.meshgrid(*[a, a, a, a], copy=False)
>>> b[0].flags['OWNDATA']
True
>>> c[0].flags['OWNDATA']
False

flags['OWNDATA'] tells you whether the array is a view into the data stored in another array or not.  In the case of b, it is, which means each b array takes up memory equal to the size of the array.  In the case of c, however, it isn't, which means c takes almost no additional memory, it is still using the same block of memory as a.  The catch is that any change to any element to any of the arrays in c will change the value in a and any of the values corresponding to that value in any of the c arrays.

If you combine this with ravel, you can get a view as well, but reshape(-1) is more reliable.  Consider this:

>>> c[0].ravel().flags['OWNDATA']
True
>>> c[0].reshape(-1).flags['OWNDATA']
False

Slice of arrays, such as a[5:1000] also return views.  "Fancy" indexing, where you get multiple elements of an array by index or boolean value, return new arrays.  So you can slice out as many values as you want.

Getting the histogram of rows or columns of an array is a bit more difficult.  The np.histogram can't do it.  What you can do is follow the instructions here to get the unique rows (you need unique columns so just transpose), but when they use the unique function add the return_counts=True argument to also get the counts of each unique row.  You will also need to sort each column using d.sort(axis=0) at the beginning so that columns with different orders of the same value are not counted separately.

Note that this doesn't use the highly-optimized numpy array processing libraries, so it will be very slow for large data set.

You can also read all the lines into a numpy array at once using loadtxt

So to implement what you want you can do:

from __future__ import print_function

import numpy as np

n = 4  # This is the number of repetitions
maxn = 10000

data = np.loadtxt('testdata.txt')

vals = np.meshgrid(*[data]*n, copy=False)
vals = np.vstack([val.reshape(-1)[:maxn] for val in vals])
vals = vals[:, ~(vals[0] == vals).all(0)]
p_avg = vals.mean(0)
print(p_avg)

Or as a function:

from __future__ import print_function

import numpy as np

def allmeans(data, n):
    vals = np.meshgrid(*[data]*n)
    vals = np.vstack([val.ravel() for val in vals])
    vals = vals[:, ~(vals[0] == vals).all(0)]
    return vals.mean(0)

data = np.loadtxt('testdata.txt')
print(allmeans(data, 4))

Doing variance or standard deviation just requires replacing mean with var and std, respectively.  You could even pass it as a function:

from __future__ import print_function

import numpy as np

def allagg(data, n, func):
    vals = np.meshgrid(*[data]*n)
    vals = np.vstack([val.ravel() for val in vals])
    vals = vals[:, ~(vals[0] == vals).all(0)]
    return func(vals, 0)

data = np.loadtxt('testdata.txt')
print('Mean: \n', allagg(data, 4, np.mean))
print('Variance: \n', allagg(data, 4, np.var))
print('Standard Deviation: \n', allagg(data, 4, np.std))

NOTE:  The differences between [x, x, x] and [x]*3 come into play when x is itself mutable (a data type that can change, like a list or numpy array).  Consider this example:

>>> a = [[1], [1], [1]]
>>> b = [[1]]*3
>>> a[0][0] = 5
>>> b[0][0] = 5
>>> print(a)
[[5], [1], [1]]
>>> print(b)
[[5], [5], [5]]

This difference happens because in a each [1] is an independent list, while in b each [1] is the same list.  Since you can change lists in-place, changes to a list in one part of your program affect that list everywhere else in the program.  So these are more equivalent:

>>> x = [1]
>>> a = [x, x, x]
>>> b = [x]*3
>>> a[0][0] = 5
>>> print(a)
[[5], [5], [5]]
>>> print(b)
[[5], [5], [5]]

Notice I didn't even have top change b here, changes to a change every x used everywhere.  This can be used to do funny things like create nested infinitely-nested lists:

>>> x = [1, 1]
>>> x[0] = x
>>> x
[[...], 1]

This issue affects lists of numpy arrays as well, but it doesn't matter to use because we aren't changing any of those arrays in-place.

EDIT 1: Change code to exclude duplicated

EDIT 2: Explain unpacking

EDIT 3: Warning about [x]*n syntax

EDIT 4: Added optimization tips and histogram


Kattis "Hitting the Targets" utilizing a polymorphic relashionship In order to learn C++ I have written a solution to this Kattis "Hitting the Targets" problem

Basically the program is to accept some shapes (circles and rectangles) on std in and a set of points and output the number of shapes that include each point.

Seeing as it is my first C++ program I'm sure I have made significant mistakes.

#include <iostream>
#include <regex>
#include <string>
#include <vector>
class Shape
{
public:
    Shape()
    {
    }
    ~Shape()
    {
    }
    virtual bool contains(int, int) = 0;
};

class Circle : public Shape
{
private:
    int x1, y1, r;

public:
    Circle(const int _x1,const int _y1,const int _r)
    {
        x1 = _x1;
        y1 = _y1;
        r = _r;
    }
    bool contains(const int x,const int y) override
    {
        int x_dist = std::abs(x - x1);
        int y_dist = std::abs(y - y1);
        return x_dist * x_dist + y_dist * y_dist <= r * r;
    }
    ~Circle()
    {
    }
};

class Rectangle : public Shape
{
private:
    int x1, x2, y1, y2;

public:
    Rectangle(const int _x1,const int _y1,const int _x2,const int _y2)
    {
        x1 = _x1;
        x2 = _x2;
        y1 = _y1;
        y2 = _y2;
    }
    bool contains(const int x,const int y) override
    {
        return x >= x1 && x <= x2 && y >= y1 && y <= y2;
    }
    ~Rectangle()
    {
    }
};

int main(void)
{
    std::string line;
    std::getline(std::cin, line);
    int num_shapes = std::stoi(line);
    std::vector<std::unique_ptr<Shape> > shapes;
    std::regex rect_pat{ R"(rectangle (-?\d+) (-?\d+) (-?\d+) (-?\d+))" };
    std::regex circ_pat{ R"(circle (-?\d+) (-?\d+) (-?\d+))" };
    for(int a = 0; a < num_shapes; a++) {
        std::getline(std::cin, line);
        std::smatch matches;
        if(std::regex_search(line, matches, rect_pat)) {
            std::unique_ptr<Shape> rect(new Rectangle(std::stoi(matches[1]), std::stoi(matches[2]), std::stoi(matches[3]), std::stoi(matches[4])));
            shapes.push_back(std::move(rect));

        } else if(std::regex_search(line, matches, circ_pat)) {

            std::unique_ptr<Shape> circ(
                new Circle(std::stoi(matches[1]), std::stoi(matches[2]), std::stoi(matches[3])));
            shapes.push_back(std::move(circ));
        }
    }
    std::getline(std::cin, line);

    int num_points = std::stoi(line);
    for(int a = 0; a < num_points; a++) {
        int x, y;
        std::cin >> x >> y;
        int num_containing = 0;

        for(std::vector<int>::size_type i = 0; i != shapes.size(); i++) {
            if((shapes[i]->contains)(x, y)) {
                num_containing++;
            }
        }
        std::cout << num_containing << '\n';
    }
    return 0;
}

I would have preferred to use count_if with a lambda rather than a for loop to count the number of shapes containing the point in order to improve readability but couldn't get it to work.

I'm also quite unsure if my use of unique_ptr and how I parse input is really optimal.

Virtual destructor

As a simple rule of thumb, if a base class contains any virtual functions, its destructor should also be virtual. The primary reason for it to declare a virtual function is so a derived class can override that virtual function. If you have public derivation, you nearly always want the destructor to be virtual--without that, destroying a derived object via a pointer to the base class gives undefined behavior.

Defining empty functions

As another simple rule of thumb, if you don't have a specific idea of what a function is supposed to do, it's probably better not to define it at all. In your case, the base class' constructor isn't doing anything, so it probably isn't needed at all. The destructor isn't currently doing anything either, but it should be defined as virtual (see previous point).

If the derived classes don't have anything to do in their destructors, you probably don't need to define their destructors.

Factory

You might want to consider moving all the code for reading data from the file and creating an object from that data into a class (or possibly just a function) by itself. This sort of thing is usually called a factory.

Error handling

Right now, if you encounter bad input (neither the "Circle" nor "Rectangle" pattern is matched by a line of input) you simply ignore that line of input and proceed to the next. This is fine for a site like you're dealing with here, but it's rarely good behavior in the real world, where bad input should normally produce some message to the user letting them know what happened.

Standard algorithm

You mentioned using std::count_if to do the counting. You could use it something like this:

std::cout << std::count_if(shapes.begin(), shapes.end(),
        [=](std::unique_ptr<Shape> &s) { return s->contains(x, y); }) << "\n";

Note that in this case, the unique_ptr doesn't really work particularly well, so we end up passing it by reference. Otherwise, the pointer would be moved (transferred) to the algorithm, so the pointer in the vector would have released it; on the second iteration, the vector would only contain null pointers.

This could be handled by using shared_ptrs instead. Another (probably preferable) solution would be to look up (and use) Boost ptr_vector instead.

Adding a Point class

It's probably at least worth considering adding a Point class (or maybe struct) to encapsulate an X/Y coordinate pair.

Prefer member initializer lists over assignment

When you have a constructor that initializes members from arguments, it's usually preferable to do the initialization in a member initializer list. For example, your Circle constructor could look like this:

Circle(const int _x1, const int _y1, const int _r) : x1(_x1), y1(_y1), r(_r) {
}

Note that a ctor with an empty body like this is quite common and expected.

Prefer make_unique over unique_ptr<T>(new T(arguments))

There's rarely much (if any) reason to use an unadorned new to allocate your object(s), and doing so can introduce exception safety issues (which don't arise in this exact code, but I'd advise normally using make_unique as it eliminates the possibility).

Putting most of those together, we could end up with code something like this:

#include <iostream>
#include <memory>
#include <regex>
#include <string>
#include <vector>

class Shape {
public:
    virtual ~Shape() { }
    virtual bool contains(int, int) = 0;
};

class Circle : public Shape {
    int x1, y1, r;
public:
    Circle(const int _x1, const int _y1, const int _r)
        : x1(_x1), y1(_y1), r(_r)
    { }

    bool contains(const int x, const int y) override {
        int x_dist = std::abs(x - x1);
        int y_dist = std::abs(y - y1);
        return x_dist * x_dist + y_dist * y_dist <= r * r;
    }
};

class Rectangle : public Shape {
    int x1, x2, y1, y2;
public:
    Rectangle(const int _x1, const int _y1, const int _x2, const int _y2)
        : x1(_x1), y1(_y1), x2(_x2), y2(_y2)
    { }

    bool contains(const int x, const int y) override {
        return x >= x1 && x <= x2 && y >= y1 && y <= y2;
    }
};

struct Factory {
    std::unique_ptr<Shape> build(std::string const &line) {
        static const std::regex rect_pat{ R"(rectangle (-?\d+) (-?\d+) (-?\d+) (-?\d+))" };
        static const std::regex circ_pat{ R"(circle (-?\d+) (-?\d+) (-?\d+))" };

        std::smatch matches;
        if (std::regex_search(line, matches, rect_pat)) {
            return std::make_unique<Rectangle>(std::stoi(matches[1]), std::stoi(matches[2]), std::stoi(matches[3]), std::stoi(matches[4]));
        }
        else if (std::regex_search(line, matches, circ_pat)) {
            return std::make_unique<Circle>(std::stoi(matches[1]), std::stoi(matches[2]), std::stoi(matches[3]));
        }
        // Probably want to add something here to deal with bad data (e.g., throw an exception).
    }

};

int main(void)
{
    std::string line;
    std::getline(std::cin, line);
    int num_shapes = std::stoi(line);
    std::vector<std::unique_ptr<Shape> > shapes;

    Factory f;

    for (int a = 0; a < num_shapes; a++) {
        std::getline(std::cin, line);
        shapes.push_back(f.build(line));
    }
    std::getline(std::cin, line);

    int num_points = std::stoi(line);
    for (int a = 0; a < num_points; a++) {
        int x, y;
        std::cin >> x >> y;

        std::cout << std::count_if(shapes.begin(), shapes.end(),
            [=](std::unique_ptr<Shape> &s) { return s->contains(x, y); }) << "\n";
    }
}


Array of all permutations in C Is this a good compromise between readability and verbosity? Are there any bugs, additional error checking, or extraneous code as well?

// Generate permutations of array of n..n+x
// n must have no repetitions, sorted smallest to largest
// Steinhaus-Johnson-Trotter algorithm
// Results are not in lexicographical order

// Switch to using a generator when n! becomes too large. ~12 or 13 probably.

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

enum directions { LEFT, RIGHT };

int **permute(int array[], int n);
bool mobile(int array[], int dir[], int count, int n);
void swap(int array[], int a, int b);

void printArray(int *array, int n, char c);
long long factorial(int n);

int **permute(int array[], int n) {
  if (n < 1) {
    return NULL;
  }

  int bigInd;    // index of largest mobile num
  int bigNum;    // largest mobile num
  bool ismobile; // If there is still a mobile number
  int counter = 0;

  int fact = factorial(n);

  int *dir;
  int **res;
  if ((dir = malloc(n * sizeof(int))) == NULL) {
    fprintf(stderr, "Error allocating memory\n");
    exit(EXIT_FAILURE);
  }
  for (int i = 0; i < n; i++) {
    dir[i] = LEFT;
  }

  if ((res = malloc(fact * sizeof(int *))) == NULL) {
    fprintf(stderr, "Error allocating memory\n");
    exit(EXIT_FAILURE);
  }
  for (int i = 0; i < fact; i++) {
    if ((res[i] = malloc(n * sizeof(int))) == NULL) {
      fprintf(stderr, "Error allocating memory\n");
      exit(EXIT_FAILURE);
    }
  }

  while (1) {
    memcpy(res[counter], array, n * sizeof(int));
    counter++;
    bigInd = 0;
    bigNum = 0;
    ismobile = false;
    for (int i = 0; i < n; i++) {
      if (mobile(array, dir, n, i)) {
        ismobile = true;
        if (array[i] > bigNum) {
          bigInd = i;
          bigNum = array[i];
        }
      }
    }
    if (!ismobile) {
      break;
    }
    if (dir[bigInd] == LEFT) {
      swap(array, bigInd, bigInd - 1);
      swap(dir, bigInd, bigInd - 1);
    } else {
      swap(array, bigInd, bigInd + 1);
      swap(dir, bigInd, bigInd + 1);
    }

    for (int i = 0; i < n; i++) {
      if (array[i] > bigNum) {
        dir[i] = !(dir[i]);
      }
    }
  }

  free(dir);
  return res;
}

void swap(int array[], int a, int b) {
  int temp = array[a];
  array[a] = array[b];
  array[b] = temp;
}

bool mobile(int array[], int dir[], int count, int n) {
  if ((n == 0 && dir[n] == LEFT) || (n + 1 == count && dir[n] == RIGHT) ||
      (dir[n] == RIGHT && array[n + 1] > array[n]) ||
      (dir[n] == LEFT && array[n - 1] > array[n])) {
    return false;
  }
  return true;
}

void printArray(int array[], int n, char c) {
  if (!array || n < 1) {
    return;
  }
  for (int i = 0; i < n; i++) {
    printf("%d", array[i]);
  }
  printf("%c", c);
}

long long factorial(int n) {
  long long result = 1;
  for (long long i = n; i > 1; i--) {
    result *= i;
  }
  return result;
}

int main(void) {
  int a[] = {1, 2, 3, 4, 5};
  int length = sizeof(a) / sizeof(a[0]);
  int fact = factorial(length);
  int **r = permute(a, length);
  for (long long i = 0; i < fact; i++) {
    printf("%*lld: ", 3, i + 1);
    printArray(r[i], length, '\n');
  }
  for (long long i = 0; i < fact; i++) {
    free(r[i]);
  }
  free(r);
}

verbose vs readable

Why choose?

Just encapsulate all the explicit code and error checkings into functions (if possible) or macros (only when functions can't do it), and have a clean high level function.

Safe usage of malloc()

Malloc is easily misused.  Problems using malloc are the following:

sizeof(type) vs sizeof(*foo):

foo = malloc(sizeof(*foo) * nmemb); is better because if you ever change the type of foo, this call will still be valid, while if not, you would have to change every line where malloc is called with foo.  If you forget any of those lines, good luck!

overflow:

If (sizeof(*foo) * nmemb) > SIZE_MAX, it will silently wrap around, and allocate a very small amount of memory, and you will most likely end up accessing memory that you shouldn't.

Solution:

Use this enclosure around malloc

sizeof(type) vs sizeof(*foo)

As I said above, use the second.  Apart from malloc you also have this problem in memcpy.

fprintf(stderr, ...) + exit()

You could combine those two in just one function call:

error(): GNU extension

err(): BSD extension

I would advise to use the first one: first, GNU extensions are more commonly available (even Clang has most (if not all) of them); second, because err is a common name used in temporary variables or labels used in case of an error.

C99 types

If you don't have a reason to use old C89 (yes, 1989) types, don't use them. What is long long, and why would you use it?

int64_t is clearer, shorter, and better in every way, unless you need to interface a library that hasn't been updated in the last 30 years, which ironically is most of them.

Also instead of int, use int32_t, although if you want factorials that fit into an int64_t, I don't think you need more than int8_t.


Encoding variable-length quanties for MIDI I was reading about the MIDI spec and I challenged myself to implement a function to convert an int32 to a variable length quantity uint32.

I didn't look it up or anything so there might be a way more efficient or tricky way to get the same result, but as far as doing what I set out to accomplish the following code works.

I'm interested to know if anyone can significantly simplify or improve the algorithm that does the conversion, public static uint CalculateEncodedQuantity(int q).

I included the whole thing in case anyone wanted to paste it in to VS.

Specification:

Tests:

using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace VariableLengthRepresentation.Test
{
    [TestClass]
    public class UnitTest1
    {
        [TestMethod]
        public void TestMethod1()
        {
            Assert.AreEqual(0u, new VariableLengthQuantity(0).EncodedQuantity);
            Assert.AreEqual(0x40u, new VariableLengthQuantity(0x40).EncodedQuantity);
            Assert.AreEqual(0x7fu, new VariableLengthQuantity(0x7f).EncodedQuantity);

            //  input:           1000 0000
            // output: 1000 0001 0000 0000
            Assert.AreEqual(0x8100u, new VariableLengthQuantity(0x80).EncodedQuantity);

            //  input: 0010 0000 0000 0000
            // output: 1100 0000 0000 0000
            Assert.AreEqual(0xc000u, new VariableLengthQuantity(0x2000).EncodedQuantity);

            Assert.AreEqual(0xff7fu, new VariableLengthQuantity(0x3fff).EncodedQuantity);
            Assert.AreEqual(0x818000u, new VariableLengthQuantity(0x4000).EncodedQuantity);
            Assert.AreEqual(0xc08000u, new VariableLengthQuantity(0x100000).EncodedQuantity);
            Assert.AreEqual(0xffff7fu, new VariableLengthQuantity(0x1fffff).EncodedQuantity);
            Assert.AreEqual(0x81808000u, new VariableLengthQuantity(0x200000).EncodedQuantity);
            Assert.AreEqual(0xc0808000u, new VariableLengthQuantity(0x8000000).EncodedQuantity);
            Assert.AreEqual(0xffffff7fu, new VariableLengthQuantity(0xfffffff).EncodedQuantity);
        }
    }
}

Implementation:

using System;

namespace VariableLengthRepresentation
{
    public struct VariableLengthQuantity
    {
        public VariableLengthQuantity(int quantity)
        {
            Quantity = quantity;
            EncodedQuantity = CalculateEncodedQuantity(quantity);
        }

        public static uint CalculateEncodedQuantity(int q)
        {
            if (q > 0x0fffffff)
            {
                throw new Exception("Variable length quantity cannot exceed 0x0fffffff.");
            }

            uint n = (uint)q;

            if (n < 128)
                return n;

            var result = new byte[4];

            for (int i = 3; i >= 0; i--)
            {
                result[i] = (byte)(n & 0x7f);

                if(i < 3)
                    result[i] |= 0x80;

                n >>= 7;

                if (n < 1)
                    break;
            }

            if(BitConverter.IsLittleEndian)
                Array.Reverse(result);

            return BitConverter.ToUInt32(result, 0);
        }

        public int Quantity { get; private set; }
        public uint EncodedQuantity { get; private set; }
    }
}

If an argument doesn't fit in the desired range an ArgumentOutOfRangeException should be thrown instead of an Exception.

    for (int i = 3; i >= 0; i--)
    {
        result[i] = (byte)(n & 0x7f);

        if(i < 3)
            result[i] |= 0x80;

        n >>= 7;

        if (n < 1)
            break;
    }

This can be rewritten to remove the if(i < 3) by starting the loop at 2. The check if (n < 1) can be placed in the loop condition like

result[3] = (byte)(n & 0x7f);
n >>= 7;

for (int i = 2; (i >= 0) || (n > 0); i--)
{
    result[i] = ((byte)(n & 0x7f)) | 0x80;
    n >>= 7;
}

You have some magic numbers in your code. The values 0x0fffffff, 128, 0x7f, 0x80 and 7 should be extracted to descriptive named constants.

Also there isn't explicitly written anything in any guidelines about braces {} for single instruction if statements I would encourage you to always use them to make your code less error prone.

You shouldn't shorten variables / parameter names. It is easier to read if the passed in parameter is named quantity instead of q.

The fact that you are taking care about the Endianess of the system should be stated in a xml documentation.


Increasingly Long Runtime for Macro My code works, but the problem is that it is taking an increasingly long time to run, with the time required to complete calculations increasing every time I use the macro. I've tried a variety of variations and modifications with the syntax, but as I'm pretty new to VBA, I haven't made a whole lot of progress.

Here's the code I'm running (Note, it is running as a subset, and ScreenUpdate = False):

Public Sub deleteRows()

    Dim lastRow As Long
    Dim rng As Range
    With ActiveSheet
        .AutoFilterMode = False
        lastRow = .Cells(.Rows.Count, 2).End(xlUp).Row
        '~~> Set the range of interest, no need to include the entire data range
            With .Range("B2:F" & lastRow)
                .AutoFilter Field:=2, Criteria1:="=0.000", Operator:=xlFilterValues
                .AutoFilter Field:=5, Criteria1:="=0.000", Operator:=xlFilterValues
            End With
        .Range("B1:F" & lastRow).SpecialCells(xlCellTypeVisible).EntireRow.Delete
        .AutoFilterMode = False
        Rows("1:1").Select
        Selection.Insert Shift:=xlDown, CopyOrigin:=xlFormatFromLeftOrAbove
    End With
    MsgBox Format(Time - start, "hh:mm:ss")

End Sub

This code basically removes zero-valued results from the data by deleting an entire row. Initially, it ran in about 12 seconds, but that soon became 55 second, which has progressed into increasing long runtimes, with a 'fast' now being in the 5 minuet range. Below is a spreadsheet with the recorded runtimes and corresponding changes made:

Runtime   Changes
6:30  None
7:50  None
5:37  Manually stepped through code
7:45  Run with .cells instead of .range("B1:B" & lastRow)
5:21  Run with .Range(B:B)  instead of .range("B1:B" & lastRow)
9:20  Run with application.calculation disabled/enabled, range unchanged
5:35  Run with application.enableEvents disabled/enabled, range unchanged
11:08 Run with application.enableEvents disabled/enabled, Range(B:B)
5:12  None
7:57  Run with Alternative code (old code)
5:45  Range changed to .Range(cells(2,2), Cells(lastRow,2)
10:25 Range changed to .Range(cells(2,2), Cells(lastRow,2), Application.Calculation Disabled/enabled
5:34  Range set to rngB  for .delete portion (range assigned to variable)
9:59  Range set as rng("B1:F" & lastRow)
5:58  Changed system settings for Excel to "High Priority", code reverted to original
9:41  Rerun of old code for comparison
9:26  Reun with change in old code criteria to "0.000"
0:10  Moved SpecialCells……..Delete into 2nd With/End With
5:15  Rerun  SpecialCells……..Delete into 2nd With/End With
11:31 Rerun  SpecialCells……..Delete into 2nd With/End With
11:38 Excel restart; Rerun  SpecialCells……..Delete into 2nd With/End With
5:18  Excel restart; Rerun  SpecialCells……..Delete into 2nd With/End With
6:49  Removed 2nd with 'loop'; all data put into first with statement

I did some research online, and it looks like this might be a known issue with Excel when working with large datasets, and as mine is ~51k rows, I can see how this might be the case.

  ...A macro that required several seconds to complete in an earlier version of Excel may require several minutes to complete in a later version of Excel. Alternatively, if you run a macro a second time, the macro may take twice as long to run as it did the first time.

Source

Is there any way to make this run faster, like it initially did? Why is this happening?

This won't solve the long run times of your code if the issue is the EntireRow.Delete method.  I assume you still want those rows deleted and stackoverflow would be the place to get a workaround or solution.

That being said your code should be reviewed.

Rows("1:1").Select
Selection.Insert Shift:=xlDown, CopyOrigin:=xlFormatFromLeftOrAbove

Never use Select unless you are making some macro to find a range for the user. Use Rows("1:1").Insert ....  Making these concatenations should be priority #1 after recording a macro.  Also you are not declaring the parent of Rows.  ActiveSheet is implicitly the parent but Rows is already nested in a With statment.  Just put the period .Rows("1:1")... to make it explicit.

With ActiveSheet

As you said, this is being called as a sub function.  If your project grows to include more sheets, assuming ActiveSheet is the sheet you want to manipulate won't be safe.  Any Sub not meant to be an outermost Sub should not use ActiveSheet, ActiveBook, ActiveCell, or Selection.  Sheets, books, and ranges should be passed as an argument.

This variable isn't being used. Delete it.

Dim rng As Range

You get the last row by going to the bottom of the sheet and iterating up until you find the bottom of your data.

lastRow = .Cells(.Rows.Count, 2).End(xlUp).Row

Instead get the bottom of the range containing data:

lastRow = .UsedRange.Rows.Count

Now there is the general steps that your macro takes.  It operates like user interaction, which is usually not the best way to approach it.  The steps you take:

AutoFilter the range

only rows with columns "C" and "F" are visible

Delete all visible rows in the range

It makes perfect sense in a user interface side but from a programming side is unnecessarily indirect.  Rows to delete are marked by remaining visible and then deleted.  What if something else has marked rows as invisible?  I would suggest iterating over all rows in the range and deleting those that should be.

Public Sub DeleteRows()
    ' ActiveSheet or Range("B2:F" & lastRow) should be passed
    Dim sheet as Worksheet
    set sheet = ActiveSheet

    Dim lastRow As Long
    lastRow = sheet.UsedRange.Rows.Count

    Dim table as Range
    set table = sheet.Range("B2:F" & lastRow)

    Dim l as Long
    For l = lastRow to 1 step -1

        If ShouldBeDeleted(table.rows(l)) Then

            table.rows(l).EntireRow.Delete shift:=xlUp

        End If

    Next l

    ' These should be in the outside Sub
    sheet.Range("1:1").Insert Shift:=xlDown, CopyOrigin:=xlFormatFromLeftOrAbove
    MsgBox Format(Time - start, "hh:mm:ss")

End Sub
Function ShouldBeDeleted(row_range as Range) as Boolean

    ShouldBeDeleted = (row_range.cells(1, 2) = 0 And row_range.cells(1, 5) = 0)

End Function

Pulling bits out like ShouldBeDeleted might seem verbose but there is a better name for it.  There is something special about those rows, and I would rename the function IsX where X describes what those rows are.  Also, columns "C" and "F" seem to be special for your worksheet.  If they are so, declare them as constants

Const IMPORTANT_COL_1 As String = "C"
Const IMPORTANT_COL_2 As String = "F"

Sorting Optimizations

If sorting the rows of the table is allowed and 0 is the minimum then you could sort the table and only iterate over the rows that you need to delete.

Public Sub DeleteRows(table as Range)

    Dim lastRow As Long
    lastRow = table.Rows.Count

    With table.Parent.Sort

        .SortFields.Clear
        .SortFields.Add key:=table.Range(IMPORTANT_COL_1 & ":" & IMPORTANT_COL_1)
        .SortFields.Add key:=table.Range(IMPORTANT_COL_2 & ":" & IMPORTANT_COL_2)
        .SetRange table
        .Apply

    End With

    Dim botRow as Long
    botRow = 1

    While ShouldBeDeleted(table.rows(botRow))
        botRow = botRow + 1
    Wend

    table.range("1:" & botRow).Delete shift:=xlUP

End Sub


Time table where workers overstay past midnight My input data comes in the following tabular table:

With this, I make a basic time table where I record arrival hour, leaving hour and total work hours.

Problem is, I can't do it with a simple pivot table because sometimes people will stay past midnight, so if I just get the MIN(Time) I'll sometimes get that the employee arrived at 1am. Another issue is I need to use the STATE to determine if it's a user arriving or leaving (denoted by "* Entrada" for arrivals and "* Salida" for leavers).

To work with this, I arbitrarily decided that the corresponding work day starts at 6am, so first I defined the following named ranges:

tblState: =OFFSET(data!$A$1,1,MATCH("STATE",data!$1:$1,0)-1,COUNTA(data!$A:$A)-1)
tblDate: =OFFSET(data!$A$2,0,0,COUNTA(data!$A:$A)-1)
tblName: =OFFSET(data!$A$1,1,MATCH("NAME",data!$1:$1,0)-1,COUNTA(data!$A:$A)-1)

And then my working formulas are

Arrived:

{=MAX((MIN(IF(((tblName=$B10)*(tblDate>(D$8+0.25))*(RIGHT(tblState,7)="Entrada")*(WEEKDAY(D$8,2)<6)*(tblDate<(D$8+1)))>0,tblDate))-D$8),0)}

Left:

{=MAX((MAX(IF(((tblName=$B10)*(tblDate<(D$8+1.25))*(tblDate>(D$8+0.25))*(RIGHT(tblState,6)="Salida"))>0,tblDate))-D$8),0)}

Hours (I do this to take care of negatives and to substract the lunch hour from the working hours if the employee worked more than a half shift):

=MAX((IF(OR(D10=0,D11=0),0,IF((D11-D10)<(4/24),D11-D10,MAX(D11-D10-(1/24),0)))),0)

It is incredibly inelegant and inefficient — it takes about 5 mins to calculate with ~25k records on my data table.

Another issue is that for the next month, I have to manually check for new records on the data table (add new employees to the time table) and missing records (remove employees who no longer work for the company), though I figure I could easily take care of this with a macro.

What would be a more efficient alternative?

It can be tough to come up with a definitive solution without having your data, but here's a few ideas that I would consider if I was having to solve this problem.

Since you have the full date and time in your data table, you could store that in your time table, and just display the time part of the date/time.  Then your elapsed time calculations would be simplified because you can use the difference between arriving and leaving that you're already calculating in the sheet.

Alternatively, if you could change your time table to allow multiple entries in one day for each person, you could have the person "leave" at midnight (23:59:59 or 24:00:00) the day the arrive, and "arrive" at 00:00:00 the next day.  This would still require some complexity in the time calculations.

If those ideas won't work for you, you can make a small improvement in your "hours" calculation.  The last part of it (MAX(D11-D10-(1/24),0)) is overly complicated.  You've just checked that (D11-D10)<(4/24), you know that D11-D10 will be >= 4/24, so the MAX calculation could just be D11-D10-(1/24).


Obtaining weather station data from a MySQL database I have a PHP page for a weather station that I'm trying to build.

I run multiple queries to pull out data from a MySQL database for min/max/average values for temp, pressure, humidity etc.

All of these are done by multiple MySQL select statements - here are 2 of them:

 $sql_dht22_temp_max = "select DATE_FORMAT(date,'%H:%i'), dht22_temp from weather inner join (select max(dht22_temp) as ts from weather where date(date) = curdate()) as ds on weather.dht22_temp = ds.ts order by date desc limit 1;";

if (!$result = $db->query($sql_dht22_temp_max)){
        die('error 1');
}

while ($row = $result->fetch_assoc()){
        $dht22_temp_max = $row['dht22_temp'];
        $dht22_max_date = $row["DATE_FORMAT(date,'%H:%i')"];
}

$sql_inside_temp_min = "select DATE_FORMAT(date,'%H:%i'), temperature from inside_temp inner join (select min(temperature) as ts from inside_temp where date(date) = curdate()) as ds on inside_temp.temperature = ds.ts order by date desc limit 1;";

if(!$result = $db->query($sql_inside_temp_min)){
    die('error running query');
}

while($row = $result->fetch_assoc()){
    $inside_temp_min = $row['temperature'];
    $inside_min_date = $row["DATE_FORMAT(date,'%H:%i')"];
}

While each statement takes a few milliseconds to run, adding them all up and it takes about 3-4 seconds. So on loading my page, you have to wait 3-4 seconds, which doesn't seem like much but is a pain.

If there a better way of doing this? Running all the statement in parallel? Can the MySQL/PHP code be optimized?

explain select DATE_FORMAT(date,'%H:%i') As MyDate, dht22_temp
    from weather inner join (select min(dht22_temp) as ts from weather
    where date(date) = curdate()) as ds on weather.dht22_temp = ds.ts
    order by date desc limit 1;

# id, select_type, table, type, possible_keys, key, key_len, ref, rows, Extra
'1', 'PRIMARY', '<derived2>', 'system', NULL, NULL, NULL, NULL, '1', NULL
'1', 'PRIMARY', 'weather', 'index', NULL, 'id_date', '9', NULL, '1', 'Using where'
'2', 'DERIVED', 'weather', 'ALL', NULL, NULL, NULL, NULL, '65420', 'Using where'

Here is the table structure:

# Field, Type, Null, Key, Default, Extra
'idweather', 'int(11)', 'NO', 'PRI', NULL, 'auto_increment'
'date', 'datetime', 'YES', 'MUL', NULL, ''
'rainfall', 'float', 'YES', '', NULL, ''
'windspeed', 'float', 'YES', '', NULL, ''
'dewpoint', 'float', 'YES', '', NULL, ''
'pressure', 'float', 'YES', '', NULL, ''
'bmp085_temp', 'float', 'YES', '', NULL, ''
'dht22_temp', 'float', 'YES', '', NULL, ''
'abs_hum', 'float', 'YES', '', NULL, ''
'gust', 'float', 'YES', '', NULL, ''
'uvi', 'float', 'YES', '', NULL, ''
'light', 'float', 'YES', '', NULL, ''
'rel_humidity', 'float', 'YES', '', NULL, ''
'wind_dir', 'varchar(5)', 'YES', '', NULL, ''

You SQL commands seem rather overelaborated. Let me first rewrite it in a more readable way:

SELECT DATE_FORMAT(date,'%H:%i'), dht22_temp
FROM weather
INNER JOIN
  (SELECT max(dht22_temp) as ts
  FROM weather
  WHERE date(date) = curdate())
AS ds ON weather.dht22_temp = ds.ts
ORDER BY date DESC
LIMIT 1

If I understand it correctly it seems you want to know at what time today the maximum temperate was reached and what that temperature was. You use the table weather with date and dht22_temp columns. In contrast to what you might think, I would start by splitting this command in two parts:

SELECT idweather, dht22_temp
FROM weather
WHERE date(date) = curdate()
ORDER BY dht22_temp DESC
LIMIT 1

This will execute quite fast and return the maximum temperature today, and the row where that maximum occured. Please make sure dht22_temp is properly indexed. Also note that this assumes that there's only 1 maximum. This might not be correct, and you can check that by reading more rows (increase LIMIT). Now you can check all kind of stuff that happened when the temperature was at its maximum:

SELECT *
FROM weather
WHERE idweather = <id of max temp>

This is very simple, but it gives you all the weather parameters when the temperature was at its maximum, including the time at which it occured. A lot of data in two simple steps.

Don't overcomplicate your SQL. But the main point is: Use your auto increment index!

I am, of course, making a point here. Once you've seen that we can rewrite the two SQL commands into a single one, reducing it to what was needed:

SELECT DATE_FORMAT(date,'%H:%i') as time, dht22_temp
FROM weather
WHERE date(date) = curdate()
ORDER BY dht22_temp DESC
LIMIT 1

And doesn't that look much simpler? In your case however I would just grab the data for one whole day:

SELECT *
FROM weather
WHERE date(date) = curdate()

and read it in an array. Then do the manipulation, like sorting, in memory. You could even make a histogram this way, without having to access the database again.


Complex boolean logic vs DRY I have some code:

  $matchingEvents = [];
  foreach( $this->recurringEvents as $event ){
    if( $event['RCE_Type'] === 'Month' && $event['RCE_Day'] === $dayOfMonth ){
      array_push( $matchingEvents, $event );
    } else if( $event['RCE_Type'] === 'Week' && $event['RCE_Day'] === $dayOfWeek ){
      array_push( $matchingEvents, $event );
    }
  }

Which duplicates the line:

array_push( $matchingEvents, $event );

This is small duplication, but still duplication. I could reformat it as this:

  $matchingEvents = [];
  foreach( $this->recurringEvents as $event ){
    if( ( $event['RCE_Type'] === 'Month' && $event['RCE_Day'] === $dayOfMonth )
        ||
        ( $event['RCE_Type'] === 'Week' && $event['RCE_Day'] === $dayOfWeek ) ){
      array_push( $matchingEvents, $event );
    }
  }

But I feel like this drastically reduces the readability of the code.

In this type of situation, is DRY or simple more important?

You could move the matching test to a separate method, like this:

private function is_matching_day( $event, $dayOfWeek, $dayOfMonth ) {

    if ( $event['RCE_Type'] === 'Month' && $event['RCE_Day'] === $dayOfMonth )
        return true;

    return $event['RCE_Type'] === 'Week' && $event['RCE_Day'] === $dayOfWeek;
}

Then your foreach statement would be more readable, and the duplicated array_push() is gone:

$matchingEvents = [];
foreach( $this->recurringEvents as $event ) {
    if ( $this->is_matching_day( $event, $dayOfWeek, $dayOfMonth ) )
        array_push( $matchingEvents, $event );
}


Calculate odds of winning (UK) Lottery jackpot I am interested in calculating odds of winning UK Lottery. The format is that 6 numbers from 1-59 are drawn. I am interested only (at this stage) in the odds of winning the jackpot (matching six balls). As an aside, I'm interested in the odds for a total balls count of 49, and 59, to see the change in chance of winning.

The mathematical formula for calculating the odds is (where 49 is total balls, 6 is number drawn:

\$\text{Odds of winning} = \dfrac{49!}{(6!*(49-6)!}\$

The main method of my code is to collect input from the user on parameters of the draw.

I have a class called DrawInfo to store information about the draw. I have a simple method to return the Factorial of a number.

I have a method to calculate the odds of winning the jackpot. This is currently all in the one class, as a small, simple app. I do appreciate that DrawInfo could live in its own class.

class Program
    {
        static void Main(string[] args)
        {
            Console.WriteLine("Enter the total number of balls in the draw: ");
            int totalBalls = int.Parse(Console.ReadLine());

            Console.WriteLine("enter the number of balls drawn: ");
            int ballsDrawn = int.Parse(Console.ReadLine());

            DrawInfo di = new DrawInfo(totalBalls, ballsDrawn);

            int totalWinOdds = FindJackpotWinningOdds(di);
            Console.WriteLine(String.Format("the odds are 1/{0:n0}", totalWinOdds));
            Console.ReadLine();
        }

        static int FindJackpotWinningOdds(DrawInfo di)
        {
            BigInteger totalBallsFactorialSum = Factorial(di.TotalBalls);
            BigInteger ballsDrawnFactorialSum = Factorial(di.BallsDrawn);

            BigInteger JackpotWinningOdds = 0;
            JackpotWinningOdds = totalBallsFactorialSum / ((ballsDrawnFactorialSum * Factorial((di.TotalBalls - di.BallsDrawn))));
            return (int)JackpotWinningOdds;
        }

        static BigInteger Factorial(BigInteger i)
        {
            if (i <= 1)
            {
                return 1;
            }
            return i * Factorial(i - 1);
        }
    }

    public class DrawInfo
    {
        public int TotalBalls { get; set; }
        public int BallsDrawn { get; set; }

        public DrawInfo(int totalBalls, int ballsDrawn)
        {
            this.TotalBalls = totalBalls;
            this.BallsDrawn = ballsDrawn;
        }
    }

A quick note to start:

Console.WriteLine(String.Format("the odds are 1/{0:n0}", totalWinOdds));

Is exactly the same as:

Console.WriteLine("the odds are 1/{0:n0}", totalWinOdds);

Now on to some maths fun... There's a multiplicative version which means you don't have to compute such massive numbers so don't need to use BigIntegers:

private int GetBinomialCoefficient(int totalNumberOfBalls, int numberOfBallsDrawn)
{
    // range checking of arguments omitted.
    var total = 1;
    for (var i = 1; i <= numberOfBallsDrawn; i++)
    {
        total *= (totalNumberOfBalls + 1 - i) / i;
    }
    return total;
}

  GetBinomialCoefficient(59, 6) == 45057474

Edit

I have changed the name of the method away from GetJackpotOdds based on 200_success's excellent answer.


Calculate the first passage time of a sequence consisting of prices I want to calculate the hitting time of a sequence of prices. I define a log price corridor by for forming a double barrier, up_threshold and down_threshold.
When one of the barriers is touched, I note the time and reset the barriers around the last price

My script looks as follows:
import pandas as pd

def speed(prices: pd.DataFrame, threshold: float=0.05):
   times = []
   up_threshold, down_threshold = threshold, -threshold
   returns = (prices['Price'] / prices['Price'].shift(1)).apply(np.log)
   cum_returns = returns.cumsum(axis=0)
   for cum_return in cum_returns:
      if -threshold < cum_return < threshold:
         # this possibility will probably occur the most frequently
         continue
      elif cum_return > threshold:
         times.append(cum_return)
         up_threshold += threshold
         down_threshold += threshold
      else:
         times.append(cum_return)
         up_threshold -= threshold
         down_threshold -= threshold

if __name__ == '__main__':
    prices = np.random.random_integers(10, 25, 100)
    prices = pd.DataFrame(prices, columns=['Price'])
    speed(prices=prices, threshold=0.05)

How can I improve this script? Is it wise to also use an apply function for the cum_returns-part? The speed of the script is important. Thanks in advance

else-after-continue
This:
  if -threshold < cum_return < threshold:
     # this possibility will probably occur the most frequently
     continue
  elif cum_return > threshold:
     times.append(cum_return)
     up_threshold += threshold
     down_threshold += threshold
  else:
     times.append(cum_return)
     up_threshold -= threshold
     down_threshold -= threshold

can be
  if -threshold < cum_return < threshold:
     # this possibility will probably occur the most frequently
     continue

  times.append(cum_return)

  if cum_return > threshold:
     up_threshold += threshold
     down_threshold += threshold
  else:
     up_threshold -= threshold
     down_threshold -= threshold


ProjectEuler Problem #3 I'm working on projecteuler.net problems, and I made this for problem #3:

array = []; // use this to save the prime factors
function factor(number, start){ //begin of the function
  prime=true;
  for(i = start; i < number+1; i++){ //begin of the first loop
    if(number % i == 0){ //is it a divisor of the number?
       for (x = 2; x <= Math.sqrt(i); x++) { //begin of the second lopp
          if (i % x == 0) {prime = false} //is it a prime number?
        }//end of the second loop
       if(prime){ array.push(i); factor(number/i, i); } //if it is a prime number save it and let's do the proccess again.
    }
  }  //end of the first loop
} //end of the function
array; //So you can see the result

My problem is:
I can get the prime factors of all the numbers i pass including the one they put on the example(13195), but when i try to get the prime factors of the one in the exercise(600851475143), my code do save the prime factor in the array but then i get the alert of "script not responding" in mozzila and chromes just stay there doing nothing, any way to solve this?.

Ps: i already solved the exercise but im not satisfied and can't fix it by my own.

You have excessive, unnecessary comments.  You wrote your code clean enough that you don't need them.

for (x = 2; x <= Math.sqrt(i); x++) {
  if (i % x == 0) {prime = false}
}

You are wasting your time here.  Once you know the value is not a prime, you do not need to continue looping.  This will reduce your time greatly:

for (x = 2; x <= Math.sqrt(i); x++) {
  if (i % x == 0) {
    prime = false;
    break;
  }
}

if(prime){ array.push(i); factor(number/i, i); }

I'm not sure why you continue finding all the small factors when only the largest prime factors are needed.  In fact, if you are going to do it manually looping over each number, you should start high and work toward the smaller numbers.  A better way to do it, however, would be to use the Sieve of Eratosthenes, as mentioned in the comments on your post.


Add or subtract two numbers depending on dropdown selection I have a drop down list contain "Add" and "Subtract". When user select either one, it will call a function Calculate()

private int Calculate(string sAction)
{
    int iTotal = 0;

    if(sAction == "Add")
    {
        iTotal = X + Y;
    }
    else if(sAction == "Subtract")
    {
       iTotal = X-Y;
    }

    return iTotal;
}

I do not wish to hard coded it to compare the action. It seem like not fulfill Open Closed Principle. If I change the text in the drop down list, I need to change the function as well. May I know how I can improve this code?

Be sure when you ever have a code that it has the chain of if else there should be an improvement. (even many programmers say you should not use if statement in your code as possible and some others say don't use else statement at all!)

You can use something like this:

First Add a class to manage your actions:

 public class ActionRegistry: Dictionary<string, Func<int, int, int>>
    {
        public ActionRegistry()
        {
            this.Add("Add", (x, y) => x + y);
            this.Add("Subtract", (x, y) => x - y);
        }

    }

Then you can use that class like this:

public class DoStuff
{
    private int Calculate(string sAction)
    {
        var actionRegistry= new ActionRegistry();
        var a = 1;
        var b = 2;
        //var actionResult= actionRegistry[this should come from your drop down].Invoke(a, b);
        var actionResult= actionRegistry[sAction].Invoke(a, b);

        //you can even Register New Action Like this :
        actionRegistry.Add("Multiply",(x,y)=>x*y);

        //then you can use it somewhere else:
        var multiplyResult =  actionRegistry["Multiply"].Invoke(a, b);
        return actionResult;
    }
}

Every time your action has changed you just need to add the new action in your ActionRegistry. With this approach, there is no need for that if-else statement.

By the way, you can even use interface and DI to loosely couple the ActionManager.

UPDATE Here my update using enums:

First Declare an Enum :

enum ActionType
{
 Add,
 Subtract
}

Then use that enum in your ActionRegistry class:

public class ActionRegistry: Dictionary<int, Func<int, int, int>>
{
    public ActionRegistry()
    {
       //it's better to register your actions outside the class
       // and don't use enum inside your class
       // but for simplicity i did that inside the class

       this.Add((int)ActionType.Add, (x, y) => x + y);
       this.Add((int)ActionType.Subtract, (x, y) => x - y);
    }

}

then you should change your calculate method like this :

 private int Calculate(int actionTypeCode)
        {
        var actionRegistry= new ActionRegistry();
        var a = 1;
        var b = 2;
        //var actionResult= actionRegistry[this should come from your drop down].Invoke(a, b);
        var actionResult= actionRegistry[actionTypeCode].Invoke(a, b);

        return actionResult;
    }

Note that you should bind your dropdown list with your enum keys as value.
I prefer to use an integer as my key because I can add more item later without changing my enum but it is not necessary.


Multi threaded Producer Consumer with bounded buffer I've developed the following multi-threaded code and would like a review. It is supposed to be implementing the Producer-consumer problem as defined here.

I have multiple consumers and one producer. I would like to know whether there are any problems with my use of concurrency and also if you have any suggestions on how to better implement the solution. I don't have a lot of experience with concurrent programming and am trying to improve.

package consumerproducer;

import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Deque;
import java.util.List;

public class ConsumerProducer1 {

    private static int QUEUE_SIZE = 10;

    private Deque<Integer> queue = new ArrayDeque<Integer>(QUEUE_SIZE);

    public synchronized boolean addWorkItem(Integer i) {
        if (queue.size() < QUEUE_SIZE) {
            queue.addLast(i);
            return true;
        } else {
            return false;
        }
    }

    public synchronized Integer getWork() {
        if (queue.size() > 0) {
            return queue.removeFirst();
        } else {
            return null;
        }
    }

    public static void main(String[] args) {
        new ConsumerProducer1().go();
    }

    public void go() {
        ConsumerThread ct = new ConsumerThread();
        ct.start();
        ConsumerThread ct2 = new ConsumerThread();
        ct2.start();
        ProducerThread pt = new ProducerThread();
        pt.start();
    }

    class ConsumerThread extends Thread {
        public void run() {
            while(true) {
                Integer work = getWork();
                if (work == null) {
                    Thread.yield();
                } else {
                    System.out.println("Thread: " + this.getId() + " received work: " + work);
                }
            }
        }
    }

    class ProducerThread extends Thread {
        private List<Integer> retryList = new ArrayList<Integer>();
        public void run() {
            while(true) {
                Integer currWork;
                if (retryList.size() == 0) {
                    currWork = (int) (Math.random() * 100);
                    System.out.println("Thread: " + this.getId() + " generated new work: " + currWork);
                } else {
                    currWork = retryList.remove(0);
                    System.out.println("Thread: " + this.getId() + " retrying old work: " + currWork);
                }
                if (!addWorkItem(currWork)) {
                    System.out.println("Thread: " + this.getId() + " could not add work (because buffer is probably full): " + currWork);
                    retryList.add(currWork);
                    Thread.yield();
                } else {
                    System.out.println("Thread: " + this.getId() + " added work to queue: " + currWork);
                }
            }
        }
    }
}

Suggest adding final to the QUEUE_SIZE constant and the collection instance also.

synchronized usage for protecting the ArrayDeque collection from concurrent access, but ArrayDeque is not thread safe. Perhaps two threads, one calls getWork() and another the addWorkItem(Integer i). Suggest using a thread safe collection instead of ArrayDeque.

Multiple threads may not always have access to System.out. Instead can use a concurrent logger or other mechanism to capture output, or have each thread write to a thread safe collection as an instance in ConsumerProducer1. Another option is having a Callable that returns desired output as a String, with the main routine the only thread accessing System.out and echoing that result.


Duplicating byte array multiple times I have this code that duplicates a byte array 5 times.

class Multiple {
    public static void main(String[] args) {
        byte[] result = new byte[] {0x41,0x42,0x43,0x44};
        int len = result.length;
        byte[] multipled = new byte[len*5];
        for (int i = 0; i < 5; i++) {
            for (int j = 0; j < len; j++) {
                multipled[i*len + j] = result[j];
            }
        }
        System.out.println(new String(multipled));
        System.out.println(new String(result));
    }
}

Example:

ABCDABCDABCDABCDABCD
ABCD

The code uses multiple loops and assignment, can it be better or shorter?

It can be made shorter:

public static void main(String[] args) {
    byte[] result = new byte[] {0x41, 0x42, 0x43, 0x44};
    byte[] multipled = new byte[result.length * 5];
    for (int i = 0; i < multipled.length; i++)
        multipled[i] = result[i % result.length];
    ...
}


JSON serializer using generics rather than System.Object I recently created a custom static class in C# to encode, hopefully, any object it is given (or collection of objects), because I was, at the time, unaware of a native C# library that did the same thing. I am using Generics instead of System.Object because I am aware of the overhead that comes with boxing/unboxing.

After discovering System.Web.Helpers, which had to be installed via NuGet, I discovered their methods usually take in an object and Type, so I assume they're using boxing/unboxing:

public static dynamic Decode(string value);

public static T Decode<T>(string value);

public static dynamic Decode(string value, Type targetType);

public static string Encode(object value);

public static void Write(object value, TextWriter writer);

Here is my code:

public static string ToJson<U, T>(U arg) where U : IEnumerable<T>
{
    StringBuilder json = new StringBuilder();
    json.Append(@"{""data"" : [");

    foreach (var item in arg)
    {// iterate through the IEnumerable

        json.Append("{");
        foreach (var property in typeof(T).GetProperties())
        {// iterate over the properties of the class stored in the IEnumerable argument
            var value = property.GetValue(item, null).ToString();
            if (value == null)// make sure value is not null or empty
                json.AppendFormat(@"""{0}"": """",", property.Name);
            else
            {
                if (value.Contains("\r\n"))
                {// convert the new lines into an array of strings, to be joined when read
                    json.AppendFormat(@"""{0}"": [""{1}""],", property.Name,
                        value.Replace("\"", "\\\"").Replace("\r\n", @""","""));
                    // can be read back by join('\n');
                }
                else
                    json.AppendFormat(@"""{0}"": ""{1}"",", property.Name,
                        value.Replace("\"", "\\\""));
            }
        }
        // remove the trailing comma
        json.Remove(json.Length - 1, 1);
        json.Append("},");// add another row in the array (another dictionary)
    }

    if (json.Length > 11)
    {
        // remove the trailing comma
        json.Remove(json.Length - 1, 1);
        json.Append("]}");
    }
    else
        // no results from sql query:
        json.Replace("[", @"""none""}");

    return json.ToString();
}

Is there something that I'm missing out on, or is my code not optimal/pretty?

Nice try, however there are potential optimization and code quality improvements.

First obvious thing is handling of the trailing comma.

If you try to generalize your code for the other possible underlying string output component (Stream, for example), you would discover that a string editing operation is not readilly available. Not all the streams support positioning, for example.
In a good implementation, i would not expect the trailing comma at all:

Following is a pseudocode with an abstracted Append method.

using (var iterator = items.GetEnumerator())
{
    if (iterator.MoveNext())
        Append(json, iterator.Current);
    while (iterator.MoveNext())
    {
        Append(json, ',');
        Append(json, iterator.Current);
    }
}

In this code, json could be a StringBuilder or a Stream, or whatever your implementation would use.

Using formatting is a performance killer.

Append format creates at least one excess array object on heap (as for the current implementation of NET 4.5), and parses the format string, which is suboptimal.
If you want your code to be performant, use just the append method of the string builder:

json.Append('[').Append(value).Append(']');

Following part makes feeling like i would never rely on such an implementation:

if (json.Length > 11)

It's a bad way of checking for an empty collection and a bad usage of magic constant in your code. Solve the former, and get rid of the latter.

Using reflection is another performance killer.
There are many techniques to do all the reflection on a type beforehand, and then use a compiled delegate. You can find information on Expression.Compile, which uses Assembly.Emit to generate code dynamically at runtime only the first time a particular type serialization is used.

I would say code comments inside methods lead to unmaintainable code, and the comments like:

// iterate through the IEnumerable

..

if (value == null)// make sure value is not null or empty

..

json.Append("},");// add another row in the array (another dictionary)

...are pretty much useless.

Just assume that every person reading your code knows that foreach means iterating an ienumerable, if means a conditional statement, and } in json means the end of dictionary.

Using comments on parts of the code blocks inside a method indicates that the method could be composed of smaller ones with descriptive names and possibly add xmldoc comments:

    /// <summary>
    /// Appends multiline value <paramref name="value"/> with special formatting based on newline charactes.
    /// </summary>
    /// <param name="stringBuilder">Output builder.</param>
    /// <param name="value"></param>
    public void AppendMultiline(StringBuilder stringBuilder, string value)
    {
        // ...
    }