"Time Conversion" Python implementation Problem Statement

  You are given time in AM/PM format. Convert this into a 24 hour format.

  Note Midnight is \$ 12:00:00AM \$ or \$ 00:00:00 \$ and 12 Noon is \$ 12:00:00PM. \$

Input Format

  Input consists of time in the AM/PM format i.e. hh:mm:ssAM or hh:mm:ssPM
  where
  \$ 01 \le hh \le 12 \$,
  \$ 00 \le mm \le 59 \$,
  \$ 00 \le ss \le 59 \$

Output Format

  You need to print the time in 24 hour format i.e. hh:mm:ss
  where

  \$ 00 \le hh \le 23 \$,
  \$ 00 \le mm \le 59 \$,
  \$ 00 \le ss \le 59 \$

Sample Input

  \$ 07:05:45PM \$

Sample Output

  \$ 19:05:45 \$

Solution

class AbsTime(object):

    def __init__(self, hh, mm=0, ss=0, pm=False):
        self.hh = hh
        self.mm = mm
        self.ss = ss
        self.pm = pm

    def add(self, time):
        self.hh += time.hh
        self.mm += time.mm
        self.ss += time.ss

        if self.ss >= 60:
            self.ss -= 1
            self.mm += 1
        if self.mm >= 60:
            self.mm -= 1
            self.hh = self.hh % 24
        return self

    def midnight(self):
        return not self.pm and str(self) in ['00:00:00', '12:00:00']

    def noon(self):
        return self.pm and str(self) == '12:00:00'

    def __str__(self):
        return "{0:02d}:{1:02d}:{2:02d}".format(self.hh, self.mm, self.ss)

    def get(self):
        if self.midnight():
            return AbsTime(0)
        if self.noon():
            return AbsTime(12)
        if self.pm:
            if self.hh == 12:
                hh = self.hh
            else:
                hh = self.hh + 12
            return AbsTime(hh, self.mm, self.ss)
        else:
            return AbsTime(self.hh % 12, self.mm, self.ss)

    @classmethod
    def create_from_string(cls, time):
        pm = True if time[-2:] == 'PM' else False
        hh, mm, ss = map(int, time[:-2].split(':'))
        return cls(hh, mm, ss, pm)

def main(time):
    if time == '00:00:00':
        return time

    abstime = AbsTime.create_from_string(time)
    return abstime.get()

if __name__ == '__main__':
    TIME = raw_input().strip()
    print main(TIME)
    #print main('00:00:00')   #00:00:00
    #print main('12:00:00PM') #12:00:00
    #print main('12:00:00AM') #00:00:00
    #print main('02:00:00PM') #14:00:00
    #print main('02:00:00AM') #02:00:00
    #print main('11:59:00AM') #11:59:00
    #print main('12:59:00PM') #12:59:00
    #print main('01:00:00PM') #13:00:00
    #print main('11:59:59PM') #23:59:59
    #print main('12:59:59AM') #00:59:59

It really took me a while to do this simple solution, its fast when we do just structural programming but once I thought to step into OOP world things really got confusing. How can I improve this design?

Using an internal representation of time that is as simple as possible would simplify your work. For example, using internally seconds since midnight, the add method could become

def add(self, other):
    self.seconds = (self.seconds + other.seconds) % SECONDS_IN_DAY


C++ choose between runtime and compile time implementation functions I wrote a small wrapper function to choose between runtime (std) sqrt and a constexpr version, as a continuation of the vector class I published here earlier:

C++ mathematical n-dimensional vector template using fold expressions

It seems to work fine and Compiler Explorer shows that the sqrt function is completely compiled out.

The relevant code (sqrt coming from the standard library):

constexpr T cx_sqrt(const T& m) const {
    int i = 0;
    while((i*i) <= m) {
        i++;
    }
    i--;
    T d = m - (i * i);
    T p = d / (2 * i);
    T a = i + p;
    return a - (p * p) / (2 * a);
}

constexpr T choose_sqrt(const T& m) const {
    return (__builtin_constant_p(m) && __builtin_constant_p(*m)) ?  cx_sqrt(m) : sqrt(m);
}

template <size_t... I>
constexpr T magnitude_impl(std::index_sequence<I...>) const {
    return choose_sqrt(((values[I] * values[I]) + ...));
}

Any gotchas using a method like this?

As of C++20, you can use std::is_constant_evaluated(), which is more portable and considerably easier to read.  I realize you have only tagged C++17, but I just wanted to point out that there is now a cleaner solution to this problem.
#include <type_traits>
#include <cmath>

namespace example
{
    namespace impl
    {
        template<typename T>
        constexpr auto sqrt(const T& m) noexcept -> decltype(std::sqrt(m)) {
            // implementation details
        }
    }

    template<typename T>
    constexpr auto sqrt(const T& m) noexcept {
        if (std::is_constant_evaluated())
            return impl::sqrt(m);
        else
            return std::sqrt(m);
    }
}


Concise code to perform action on every element, when need index When I read this answer, which has code:

linqObject.Where((obj, index) => {
  DoWork(obj, index);
  return true;
}).ToArray(); //MUST CALL ToArray() or ToList() or something to execute the lazy query, or the loop won't actually execute

My initial reaction was similar to Mark Sowul's comment:

  "Wastes memory (allocating a list or array) and risks bugs (if you don't call ToList or ToArray). Why?".

But on further thought, what is a concise approach, avoiding the downsides of the above?
My first attempt:

int index = 0;
foreach (var item in someEnumerable) {
    DoWork(item, index);
    index++;
}

Compared to calling a Linq operator, that seems verbose, low-level. I'm defining and manipulating an index, whereas Linq has an Operator that "almost" does what I want - takes care of the index tracking for me.

Personally, I would write the explicit foreach loop - but maybe that is only because I've been programming a long time, and am comfortable with that syntax.

Is there a more concise solution, that doesn't have the downsides of the first version shown (Where / ToArray)?

I'm trying to get comfortable with encapsulating such stuff into methods, often by using Action<> or Func<>, which are newer [to me] than the style of programming that became my habit. So that instead of repeating similar snippets of code, I just call a method.

  As an aside, Linq is also newer than my habits. Linq is concise, but not in a way that I find helpful; I find loops clearer to read; a better separation of the aspects of what is happening. But I'm open to seeing situations where its expressive power is appropriate.
  [OFF-TOPIC: In contrast, I spent some time with F#, and found it to be fantastic. I LOVE the combination of functional with OO; just not the way Linq jams a handful of functional constructs into C#'s syntax. But this question is about making good use of C#, so all advice is appreciated.]

I think this becomes:

public static void ForEachWithIndex<T>(IEnumerable<T> e, Action<int, T> a)
{
    int index = 0;
    foreach (T v in e) {
        a(index, v);
        index++;
    }
}

which might be used like this:

ForEachWithIndex(e, (int index, T item) => {
    // Do Work based on index and item.
});

Or if an appropriate "DoWork" method already exists, simply:

ForEachWithIndex(e, DoWork);

Comments?  Improvements, or alternatives?
Is there a way to use Linq more concisely/efficiently than in the original code sample?

NOTE: I did not make this an "extension method" on "IEnumerable", due to comments people made in stackoverflow threads regarding "ForEach" as a possible extension method: dislike of a syntax that looks similar to a Linq operator - which traditionally don't have side-effects - for a method that clearly DOES have side-effects; indeed is all about the side-effects.

linqObject.Where((obj, index) => {
  DoWork(obj, index);
  return true;
}).ToArray();

Never ever! This is the worst possible abuse of an API. I 100% agree with the comment.

int index = 0;
foreach (var item in someEnumerable) {
    DoWork(item, index);
    index++;
}

Yes, this is the way it should be, although this can be shortened... keep reading.

public static void ForEachWithIndex<T>(IEnumerable<T> e, Action<int, T> a)
{
    int index = 0;
    foreach (T v in e) {
        a(index, v);
        index++;
    }
}

This is better but the naming is still quite wrong and the Action parameters are in the wrong order. They should be incremental. This means that T is always there so it should be first, then any other parameters may follow, like the index.

I'm not sure what's wrong with using normal loops where necessary but if you really want to encapsulate them then...

Try to be consistant with the LINQ API and don't invent new method suffixes like WithIndex. Use overloads instead. In fact you can implement it once and call the core method with other overloads.

Notice the order of Action parameters. It's consistant with current APIs. T first then index. There is also no explicit parameter checking because it uses an empty collection if the source is null and the ? operator to not call the delegate if this one is null. You can also throw if you want.

public static class Loop
{
    public static void ForEach<T>(IEnumerable<T> source, Action<T, int> action)
    {
        var i = 0;
        foreach (var item in source ?? Enumerable.Empty<T>()) action?.Invoke(item, i++);
    }

    public static void ForEach<T>(IEnumerable<T> source, Action<T> action)
    {
        ForEach(source, (x, i) => action(x));
    }
}


Finding smallest number of moves We are given a map of sides of length n and m (both n and m are less than 1000), divided into n*m fields (squares of side 1). The map looks (for example) like this:

. z x . x
. x x . .
. . x . x
x . . n .

Here n=4 and m=5. We start from the field with "z" and we have to get to the field with "n". Suppose we are at position (x, y). Then, we can only move to fields with coordinates: (x+1, y+2), (x+1, y-2), (x-1, y+2), (x-1, y-2), (x+2, y+1), (x+2, y-1), (x-2, y+1), (x-2, y-1).

The next restriction is that we can only move to fields with "." (or "n") - we must avoid fields with "x". We have to compute the smallest number of moves required to get from the field with "z" to the field with "n" (if it's impossible, we have to output "NO"). The correct answer for the given sample map is 3.
My program works as follows: we input the map, then we represent it as graph - and then we run the breadth-first search. As far as I've tested, my program outputs correct values.

My biggest concern is that I use too much memory. Also, I wonder if there's a better method for solving such problems.

Here's the code:

#include <iostream>
#include <vector>
#include <queue>

using namespace std;

int n, m, start, finish;

vector <int> graph[990000];

bool visited[990000];

int mydistance[990000];

queue <int> myqueue;

void BFS(int s){

    mydistance[s]=0;

    myqueue.push(s);

    while (!myqueue.empty()){

        if (visited[finish]){

            return;

        }

        int u=myqueue.front();

        myqueue.pop();

        for (int i=0; i<graph[u].size(); i++){

            if (!visited[graph[u][i]]){

                mydistance[graph[u][i]]=mydistance[u]+1;

                myqueue.push(graph[u][i]);

            }

        }

        visited[u]=true;

    }

}

int main(){

    cin >> n >> m;

    char map[n][m];

    for (int i=0; i<n; i++){

        for (int j=0; j<m; j++){

            char x;

            cin >> x;

            map[i][j]=x;

        }

    }

    for (int i=0; i<n; i++){

        for (int j=0; j<m; j++){

            int a=i*m+j;

            if (map[i][j]=='z'){ start=a; }
            if (map[i][j]=='n'){ finish=a; }

            if ((j<m-1) && (i<n-2)) { if (map[i+2][j+1]!='x'){ graph[a].push_back(a+2*m+1); }} // x+1 y+2
            if ((j<m-1) && (i>1)) { if (map[i-2][j+1]!='x'){ graph[a].push_back(a-2*m+1); }} // x+1 y-2
            if ((j>0) && (i<n-2)) { if (map[i+2][j-1]!='x'){ graph[a].push_back(a+2*m-1); }} // x-1 y+2
            if ((j>0) && (i>1)) { if (map[i-2][j-1]!='x'){ graph[a].push_back(a-2*m-1); }} // x-1 y-2
            if ((j<m-2) && (i<n-1)) { if (map[i+1][j+2]!='x'){ graph[a].push_back(a+m+2); }} // x+2 y+1
            if ((j<m-2) && (i>0)) { if (map[i-1][j+2]!='x'){ graph[a].push_back(a-m+2); }} // x+2 y-1
            if ((j>1) && (i<n-1)) { if (map[i+1][j-2]!='x'){ graph[a].push_back(a+m-2); }} // x-2 y+1
            if ((j>1) && (i>0)) { if (map[i-1][j-2]!='x'){ graph[a].push_back(a-m-2); }} // x-2 y-1

            mydistance[a]=-1;

        }

    }

    BFS(start);

    if (mydistance[finish]==-1) {

        cout <<  "NO";

    } else {

        cout << mydistance[finish];

    }

    return 0;

}

I see a number of things that could help you improve your code.

Use consistent formatting

Almost every other line in the program is a blank line.  This doesn't really help readability, and then we have lines like these:

if ((j<m-1) && (i<n-2)) { if (map[i+2][j+1]!='x'){ graph[a].push_back(a+2*m+1); }} // x+1 y+2
if ((j<m-1) && (i>1)) { if (map[i-2][j+1]!='x'){ graph[a].push_back(a-2*m+1); }} // x+1 y-2

First, add some whitespace and second, don't jam nested if statements all on one line like that.  These could be reformatted like this:

if ((j < m - 1) && (i < n - 2)) {
    if (map[i + 2][j + 1] != 'x') {
        graph[a].push_back(a + 2 * m + 1);
    }
}                   // x+1 y+2
if ((j < m - 1) && (i > 1)) {
    if (map[i - 2][j + 1] != 'x') {
        graph[a].push_back(a - 2 * m + 1);
    }
}                   // x+1 y-2

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Avoid the use of global variables

graph, visited, mydistance and others are declared as global variables rather than as local variables. It's generally better to explicitly pass variables your function will need rather than using the vague implicit linkage of a global variable. For example, the visited is only used within BFS, so it should be a local variable there instead of a global.

Eliminate "magic numbers"

In a number of cases, the code uses "magic numbers" such as 990000 that have no obvious meaning.  These would be better as named constants.

Fix the error

This line may be accepted by your compiler, but it is not compliant with the C++ standard:

char map[n][m];

The reason is that the standard will only allow this if both n and m are const and they are not in this program.  One could address that by dynamically allocating the object, as:

char *map = new char[m*n];

And then re-doing all references to change from this:

map[i][j] = x;

To this:

map[i * m + j] = x;

However, I think the better way is in the following suggestion.

Use object orientation

Because you're writing in C++, it would make sense to have methods that operate on a class such as Board and Knight (because the legal moves you describe are those that a knight can make in the game of chess) be member functions rather than separate functions.  You may not yet have learned about objects or classes, but they're one of the main strengths of C++ and something you should learn soon if you haven't already.  Use objects where they make sense.

Have functions return something useful

Instead of a void function, it would make more sense to have BFS() return the distance.

Omit return 0

When a C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no reason to put return 0; explicitly at the end of main.


Test with array and mock Here is my function I want to test:
template<typename T>
void free_items(std::vector<T*>& items)
{
    std::for_each(items.begin(), items.end(), [](T* (&item)){delete item; item = nullptr; });
}

I expect the each pointer in my result vector will be freed and be set null correctly. So I write down the following test code.
This is my mock class:
struct FreeItemMock
{
    FreeItemMock(bool& destroyed):m_destroyed (destroyed) {}
    ~FreeItemMock(){ m_destroyed = true;}
    bool& m_destroyed ;
};

Here is my test code:
BOOST_AUTO_TEST_CASE(test_free_items)
{
    bool destroyed[3] = {};
    std::vector<FreeItemMock*> mocks;

    mocks.push_back(new FreeItemMock(destroyed[0]));
    mocks.push_back(new FreeItemMock(destroyed[1]));
    mocks.push_back(new FreeItemMock(destroyed[2]));

    BOOST_CHECK_EQUAL(destroyed[0], false);
    BOOST_CHECK_EQUAL(destroyed[1], false);
    BOOST_CHECK_EQUAL(destroyed[2], false);

    free_items(mocks);

    BOOST_CHECK_EQUAL(destroyed[0], true);
    BOOST_CHECK_EQUAL(destroyed[1], true);
    BOOST_CHECK_EQUAL(destroyed[2], true);

    BOOST_CHECK(mocks[0] == nullptr);
    BOOST_CHECK(mocks[1] == nullptr);
    BOOST_CHECK(mocks[2] == nullptr);
}

The following are my questions:

Is the my so-called FreeItemMock a mock or a stub? I am not sure whether I pick the right name on my testee-class. According to the book The art of unit testing with examples in C#. A mock is used to test whether the interaction with other classed happened. I use my FreeItemMock to test its destructor is called, but after invoke the free_item my FreeItemMock actually does not exist anymore. It looks the destroyed[3] array variable is more like a mock.

The function I want to test is doing job on a vector, so I need to prepare a array of data to test. Is it correct to hand write a serious of fixture like data[0], data[1], data[2] and assert the result? Because the books also says a unit test should not have "logic" (like while, for-loop, if) to increase its complexity.
Or should I make a testing utility to help me produce the array data, and make another test for the utility?

Is it necessary to check the pre-test value of destroyed? Is it clearer to tell the reader that this function would make destroyed from 0 to 1? Or It is just too tedious?

I think that if you accept definition Fowler uses you should name them "stubs":

  Stubs provide canned answers to calls made during the test, usually not responding at all to anything outside what's programmed in
  for the test. Stubs may also record information. (Mocks Aren't Stubs )

Given the structure of the code if you want to cover with a suite of test should at least test how it works under a large set of input:

Empty array
Single element array
Array filled with null element
Array with a arbitrary number of element

with a different test for each of this cases

For this reason I suppose you should write a function that "allocate" an array, to avoid code repetition in the test (also tests must have code that you can maintain!). So I think yes you should put all the logic that can help you inside the test.

I'm not a fan of test not that test themselves, but in this case all the test make sense only if you invoke FreeItemMock with a reference to a bool, so yes I think make sense to test it, but I suggest you put it destructor, before changing the value, in this you are sure that you transaction from false to true is a really transaction and not a false to false


Read and display data from MySQL table Is there any security threat here?

$con = mysqli_connect("***", "***", "***", "***");
mysqli_query($con, "set names 'utf8'");
$id = $_GET["id"];
$result = mysqli_query($con,"SELECT * FROM document_$id");
while($row = mysqli_fetch_array($result)) {// echo table }

I am curious about my use of the $_GET variable, since that can be altered by a user. If there is a danger here, could something like this work?

If $_GET['id'] is not an integer -> redirect to error page

The comments have got it right - you're vulnerable to an SQL injection. If I navigate to your page with the id parameter set to something like this:

http://yoursite.com/database-operation?id=0;DROP%20TABLE%20%`Students`;--

then your SQL query will end up looking like this:

SELECT * FROM document_0;
DROP TABLE `Students`;

If you have a table called Students, that's about to get totally deleted.

Instead, you should be looking at using prepared statements. Both the mysqli and PDO extensions for PHP have this functionality; a quick Google search will turn up plenty of tutorials on it.

You can also help prevent this via user privileges: if all you're ever doing is SELECTs, then the MySQL user that takes these queries doesn't need the privilege to DROP tables. Use the principle of least privilege: give users (and any other entity) only the privileges they need to perform their task, nothing more.


Regex to check IP address in Java I'm toying with Regexes to parse IP Addresses.  I've got it to test for anything over 255:

public static boolean isIpAddress(String address)
{
    String regex = "^((25[0-5])|(((1[0-9]|2[0-4])|[1-9])?[0-9]))(\\.((25[0-5])|(((1[0-9]|2[0-4])|[1-9])?[0-9]))){3}$";
    try{
        Pattern p = Pattern.compile(regex);
        Matcher m = p.matcher(address);

        return m.find();
    }catch(Exception ex)
    {
        ex.printStackTrace();
    }
    return false;
}

Is there a more efficient way to write that regex?

I understand that I could probably run something like this:

String[] addressChunks = address.split(".");
String regex = "^((25[0-5])|(((1[0-9]|2[0-4])|[1-9])?[0-9]))$";
Pattern p = Pattern.compile(regex);
for(String s: addressChunks)
{
    Matcher m = p.matcher(s);
    if(!m.find(s))
    {
      return false;
    }
}
return true;

But I'd rather keep it to one line of regex and compare it all instead of doing a split.


Printing out Pascal's Triangle in row-major format Here's the problem statement:

  Given an integer value, print out Pascal's Triangle to the
  corresponding depth in row-major format:

  Input Sample:

6

  Output Sample:

1 1 1 1 2 1 1 3 3 1 1 4 6 4 1 1 5 10 10 5 1

I need some practice with recursion, so I tried that approach:

var pascalDepth = parseInt(line, 10); //The input integer is provided on a single line as a text value, so this handles it.
var testTriangle = pascalTriangle(pascalDepth);
var output = [];

for(var i = 0; i < testTriangle.length; i++){
    for(var j = 0; j < testTriangle[i].length; j++){
        output.push(testTriangle[i][j]);
    }
}

console.log(output.join(" "));

function pascalTriangle(totalLines, triangle) {
    if (typeof triangle === 'undefined') {
        triangle = [];
    }

    if (triangle.length === totalLines) {
        return triangle;
    }

    if (triangle.length > 0) {
        var triangleLine = [];
        for (var i = 0; i < triangle.length; i++) {
            if (typeof triangle[triangle.length - 1][i - 1] === 'undefined') {
                triangleLine.push(1);
            } else {
                triangleLine.push((triangle[triangle.length - 1][i - 1] + triangle[triangle.length - 1][i]));
            }
        }
        triangleLine.push(1);
        triangle.push(triangleLine);
    } else {
        triangle.push([1]);
    }
    return pascalTriangle(totalLines, triangle);
}

Like the Fibonacci sequence, calculating Pascal's triangle recursively using a pure functional approach wouldn't be very efficient, since you would repeatedly compute many of the entries.

You've implemented a recursive solution with memoization, which is smarter.  However, with memoization, the solution is stylistically iterative.  Consider the following function, which behaves identically to yours, but is written in a more straightforward manner:

function pascalTriangle(totalLines) {
    var triangle = [];
    while (triangle.length < totalLines) {
        if (triangle.length > 0) {
            var triangleLine = [];
            for (var i = 0; i < triangle.length; i++) {
                if (typeof triangle[triangle.length - 1][i - 1] === 'undefined') {
                    triangleLine.push(1);
                } else {
                    triangleLine.push((triangle[triangle.length - 1][i - 1] + triangle[triangle.length - 1][i]));
                }
            }
            triangleLine.push(1);
            triangle.push(triangleLine);
        } else {
            triangle.push([1]);
        }
    }
    return triangle;
}

You could consider your "recursion" to be a fancy goto.  Alternatively, you could conclude that Pascal's triangle does not lend itself to recursion.

That said, your implementation could be more compact.  Think of deriving the next line this way:

  [ , 1, 3, 3, 1]
+ [1, 3, 3, 1,  ]
—————————————————
  [1, 4, 6, 4, 1]

Then, you can use higher-level thinking: make two arrays with the appropriate offset, and find the sums of the corresponding elements.

function pascalTriangle(totalLines, triangle) {
    // Helper function to add corresponding elements of two arrays
    function sum(a, b) {
        return a.map(function(_, i) {
            return a[i] + b[i];
        });
    };

    if (triangle == null) {                 // Initial call with just one parameter
        triangle = [[1]];
    }
    if (totalLines === triangle.length) {   // Final case
        return triangle;
    }
    var prevLine = triangle[triangle.length - 1];
    var nextLine = sum([0].concat(prevLine), prevLine.concat([0]));
    return arguments.callee(totalLines, triangle.concat([nextLine]));
}

I've used arguments.callee for the function to refer to itself, so that if you rename the function (or make it anonymous), it can still work.


Linked List Operations in C Read Proof I am trying to write up all the linked list basic operations (push, pop, add_at_end, pop_from_end, add_at_index, pop_from_index). This is not a school assignment, even though it may look like one. I have written the code. I have tested it myself quite a bit, although I am no C guru. I would like it if you could tell me if there are any suggestions / corrections you have for the code in terms of efficiency, memory handling, readability, clarity etc.

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

typedef struct node {
    int data;
    struct node * next;
} node_t;

void print_list(node_t * head);
void push(node_t ** head, int data);
int pop(node_t ** head);
int add_at_end(node_t ** head, int data);
int remove_last(node_t * head);
int add_at_index(node_t ** head, int n, int data);
int remove_by_index(node_t ** head, int n);

int main()
{
    node_t * head = NULL;

    head = malloc(sizeof(node_t));
    if (head == NULL) {
      return 1;
    }
    head->data = 1;
    head->next = NULL;
    print_list(head);
    return 0;
}

void push(node_t ** head, int data) {
    node_t * new_node;
    new_node = malloc(sizeof(node_t));
    new_node->data = data;
    new_node->next = *head;
    *head = new_node;
}

void print_list(node_t * head) {
    node_t * current = head;

    while (current != NULL) {
        printf("%d\n", current->data);
        current = current->next;
    }
}

int add_at_end(node_t ** head, int data) {
    node_t * new_node = malloc(sizeof(node_t));
    new_node->data = data;
    new_node->next=NULL;
    if(*head==NULL){
        *head = new_node;
        return 1;
    }
    node_t * current = *head;
    while (current->next != NULL) {
        current = current->next;
    }
    current->next = new_node;
    return 1;
}

int add_at_index(node_t ** head, int n, int data) {
    node_t * current = *head;
    node_t * temp_node = NULL;
    if(n == 0 || *head==NULL){
        push(head, data);
        return 1;
    }
    while(n>1 && current->next!=NULL){
        current = current->next;
        n--;
    }
    if(n>1){
        return -1;
    }

    temp_node = malloc(sizeof(node_t));
    temp_node->data = data;
    temp_node->next = current->next;
    current->next = temp_node;
    return 1;
}

int remove_by_index(node_t ** head, int n) {
    node_t * current = *head;
    node_t * curr_head = NULL;
    if(n == 0||*head==NULL){
        pop(head);
        return 1;
    }
    while(n>0 && current->next!=NULL){
        curr_head = current;
        current = current->next;
        n--;
    }
    if(n>0){
        return -1;
    }
    curr_head->next = current->next;
    free(current);
    return 1;
}

int pop(node_t ** head) {
    int retval = -1;
    if (*head == NULL) {
        return -1;
    }
    if ((*head)->next == NULL) {
        printf("here");
        retval = (*head)->data;
        free(*head);
        *head = NULL;
        return retval;
    }
    node_t * next_node = NULL;
    next_node = (*head)->next;
    retval = (*head)->data;
    free(*head);
    *head = next_node;
    return retval;
}

int remove_last(node_t * head) {
    int retval = 0;
    if (head->next == NULL) {
        head->data;
        free(head);
        head = NULL;
        return retval;
    }

    node_t * current = head;

    while (current->next->next != NULL) {
        current = current->next;
    }
    retval = current->next->data;
    free(current->next);
    current->next = NULL;
    return retval;
}

1. You can add some comments !

2. In my opinion you should create another struct that will be named like linked_list_t which will have two pointers, node_t * start ,node_t* end.In terms of efficiency, by doing this, you don't have to iterate all the list on add_at_end to add a single node (this is a common practice when you create a linked_list)

3. When you malloc you can check the returned pointer and if it is NULL you should perror() the message.

4. The code

typedef struct node {
    int data;
    struct node * next;
} node_t;

void print_list(node_t * head);
void push(node_t ** head, int data);
int pop(node_t ** head);
int add_at_end(node_t ** head, int data);
int remove_last(node_t * head);
int add_at_index(node_t ** head, int n, int data);
int remove_by_index(node_t ** head, int n);

should be put on a header file as it just contains declarations

5. Use Sentinel Nodes in order not to iterate on int remove_last(node_t *)


EF IQueryable Extension Methods I've started building extension methods for my EF classes like below because it makes building queries so easy, and the intellisense provided helps keep people from writing a new Where() clause in code every time they do to do something.

Example Extensions to an EF User Class

public static class UserIQX
{
    public static IQueryable<User> WhereInSearch(this IQueryable<User> value, string search)
    {
        search = search.ToLower();
        return value.Where(user => user.FirstName.ToLower().Contains(search) || user.LastName.ToLower().Contains(search) || user.EmailAddress.ToLower().Contains(search));
    }

    public static IQueryable<User> WhereInRole(this IQueryable<User> value, RoleEnum role)
    {
        return value.Where(user => user.RoleID == (int)role);
    }
}

Examples - "Get all admins named Bob"

protected void Page_Load(object sender, EventArgs e)
{
    var search = "Bob";

    //With Extensions
    var activeAdmins = db.Users
    .WhereInRole(RoleEnum.Admin)
    .WhereInSearch(search)
    .Select(u => new { EmailAddress = u.EmailAddress })
    .ToList();

    //Without extensions
    search = search.ToLower();
    var pq = db.Users
        .Where(u => (
            u.FirstName.ToLower().Contains(search)
            || u.LastName.ToLower().Contains(search)
            || u.EmailAddress.ToLower().Contains(search))

            && u.RoleID == (int)RoleEnum.Admin)
        .Select(u => new { EmailAddress = u.EmailAddress })
        .ToList();
}

Results

The results are comparable with the inline one edging out ahead in speed. It's interesting that the extension-based one used a parameter for the roleID and the inline one did not.  That's the only real difference I can see.

I really like the fluent-like style the Extension method provides, and how it encapsulates logic to prevent erroneous queries from being written (like forgetting to ToLower() the string fields or something).  Is there a good reason why this would be a Really Bad Idea?  If not, I prefer the easy maintainability of the Extension methods over inline.

By moving this question from StackOverflow to here, usr's sensible remarks were lost. I restore them here as community wiki:

No, this is fine and recommended. You'll find a limitation of this approach: You cannot use these extension methods in places where they get turned into an expression:

db.Something.Any(x => x.Users.WhereInRole(RoleEnum.Admin))

This does not work because EF does not know the WhereInRole method.

The AsExpandable extension method can help with this. Search the web to find it as a small library.

Using the same approach you should be able to inline constants such as the role ID into the query as well.

Note, that the design of WhereInRole is not really ideal. Normally, you'd write a method bool IsUserInRole(User, int). No need to operate on sequences at all. The only reason this is not possible here is that you need to find a way to marshal the filtering logic into the final expression tree. Shoving the logic into the argument to Where is a hack to do that. This issue/concern is a "dirty secret" of LINQ. It's quite hacky and arbitrary.


Simple Language Interpreter I've been playing around with Python off and on for about the past year and recently came up with the following 68 (was 62) lines. I think I'll try making a calculator out of it. I'd really like to know what readers here think of its attributes such as coding style, readability, and feasible purposefulness.

# notes: separate addresses from data lest the loop of doom cometh

class Interpreter:

  def __init__(self):
    self.memory = { }
    self.dictionary = {"mov" : self.mov,
                       "put" : self.put,
                       "add" : self.add,
                       "sub" : self.sub,
                       "clr" : self.clr,
                       "cpy" : self.cpy,
                       "ref" : self.ref }
    self.hooks = {self.val("0") : self.out }

  def interpret(self, line):
    x = line.split(" ")
    vals = tuple(self.val(y) for y in x[1:])
    dereferenced = []
    keys_only = tuple(key for key in self.memory)
    for val in vals:
      while val in self.memory: val = self.memory[val]
      dereferenced.append(val)
    vals = tuple(y for y in dereferenced)
    self.dictionary[x[0]](vals)

  def val(self, x):
    return tuple(int(y) for y in str(x).split("."))

  def mov(self, value):
    self.ptr = value[0]

  def put(self, value):
    self.memory[self.ptr] = value[0]

  def clr(self, value):
    if self.ptr in self.hooks and self.ptr in self.memory:
      x = self.hooks[self.ptr]
      y = self.memory[self.ptr]
      for z in y: x(z)
    del self.memory[self.ptr]

  def add(self, values):
    self.put(self.mat(values, lambda x, y: x + y))

  def sub(self, values):
    self.put(self.mat(values, lambda x, y: x - y))

  def mat(self, values, op):
    a, b = self.memory[values[0]], self.memory[values[1]]
    if len(a) > len(b): a, b = b, a
    c = [op(a[x], b[x]) for x in xrange(len(b))] + [x for x in a[len(a):]]
    return [tuple(x for x in c)]

  def cpy(self, value):
    self.put(value)

  def out(self, x):
    print chr(x),

  def ref(self, x):
    self.put(x)

interp = Interpreter()
for x in file(__file__.split('/')[-1].split(".")[-2] + ".why"):
  interp.interpret(x.strip())

Sorry for bluntness, intended or not, but here we go!

Not very good readability IMO. It takes a while to understand what the code does. Unit tests or at least some sample input and output may make it obvious.

In general, naming should be improved. It's difficult to understand what a function or variable does from its name.

There's also much shuffling of data back and forth that seems unnecessary. It looks like this can be simplified quite a bit, see inline comments.

Also, you use tuples in many situations where a list would be more natural.

As I had problems understanding the code, I may have misunderstood things in it, so read the following inline comments with that in mind.

# notes: separate addresses from data lest the loop of doom cometh

Not a very helpful comment, is it?

class Interpreter:

Can you find a class name that better tells what this class does? It not only interprets.

  def __init__(self):
    self.memory = { }

I'd like a comment on what self.memory is.

    self.dictionary = {"mov" : self.mov,
                       "put" : self.put,
                       "add" : self.add,
                       "sub" : self.sub,
                       "clr" : self.clr,
                       "cpy" : self.cpy,
                       "ref" : self.ref }

No shit, it's a dictionary. But can you find a name that tells what kind of dictionary it is?

To reduce code size, you may consider self.dictionary = dict(self.getattr(attr) for attr in 'mov put add sub clr cpy ref'.split()), but some people may find it too "clever."

    self.hooks = {self.val("0") : self.out }

Very unclear what hooks is for. This is the only value that is every put in it. self.val("0") seems to be just a complicated way of saying (0,). What's up with that?

  def interpret(self, line):
    x = line.split(" ")

Bad variable name: x. Should it be tokens instead?

    vals = tuple(self.val(y) for y in x[1:])

Here, vals could be a list instead, like vals = [self.val(y) for y in x[1:]]. Making it a tuple just makes the code a little bit more complicated for no reason.

    dereferenced = []
    keys_only = tuple(key for key in self.memory)

Consider keys_only = self.memory.keys()? Oh, and by the way, keys_only is never used, so just remove it.

    for val in vals:
      while val in self.memory: val = self.memory[val]

Looks like this while loop will be infinite if it runs at all. Is it supposed to be an if?

      dereferenced.append(val)
    vals = tuple(y for y in dereferenced)

Pointless conversion of dereferenced to tuple.

    self.dictionary[x[0]](vals)

I think the whole interpret method can be rewritten to something like the following. Note the changed names.

  def interpret(self, line):
      command, *tokens = line.split()  # Python 3 only
      values = [self.val(y) for token in tokens]
      dereferenced = [self.memory.get(value, value) for value in values]
      self.dictionary[command](dereferenced)

Some may say it's a bit too terse. I could have written the dereferenced = ... line as a couple of more explicit statements perhaps.

I won't go into the rest, but the comments about naming and unnecessary complexity applies.


Optimize Array.indexOf How can I optimize checking a value in recursion before operating on it? I'm using Array.indexOf (this is Javascript)

var nums = [], lengths = [];
function findNumLength(n) {
    //preliminary check to see if
    //we've done this number before
    var indexOf = nums.indexOf(n);
    if(indexOf !== -1) {
        return lengths[indexOf];
    }
    function even (n2) { return n2%2===0; }
    if(n===1) {
        return 1;
    }
    if(even(n)) {
        l = findNumLength(n/2) + 1;
        if(indexOf===-1) {
            lengths.splice(0,0 ,l);
            nums.push(n);
        }
        return l;
    }
    else {
        l = findNumLength(3*n + 1) + 1;
        if(indexOf===-1){
            lengths.splice(0,0,l);
            nums.push(n);
        }
        return l;
    }
}

(note: I've answered my own question with one solution I've found; it is by no means the only solution (though it may be the best. I don't know). Please, still answer.)

The faster way to know if a number is already in a data structure is to use an object, not an array.  So, if you don't have to have the numbers in an array, then this would be a lot faster, particular when the number of items gets big because the object look up is massively faster than the linear search of .indexOf():

var numbers = {};
function recurse(newNumber) {
    if(newNumber in numbers) {
        return newNumber;
    }
    //do stuff to number
    numbers[newNumber] = true;
    return recurse(modified_number);
}

You can then iterate through the items in numbers like this:

for (var num in numbers) {
    if (numbers.hasOwnProperty(num) {
        // process num here
    }
}

FYI, if speed is really your goal here, recursion is not the fastest way to do this (because functions calls are kind of slow in javascript), particularly if you don't have much or any local state in the function so you could just use a while loop of sorts.


C permutation generator I am making a permutation generator in c. Any ideas how I can make it faster?

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

int main(int argc, char *argv[]){
    char s[] = "abcdefghijklmnopqrstuvwxyz";
    int n = 8, c[n], i, l = strlen(s);
    char w[n];

    for(i = 0; i < n; i++){
        c[i] = 0;
        w[i] = '\0';
    }
    w[n] = '\0';
    while(c[0] < l){
        for(i = 0; i < n; i++){
            w[i] = *(s + c[i]);
        }
        printf("%s\n", w);
        c[n - 1] += 1;
        for(i = n - 1; i >= 0; i--){
            if(c[i] > l - 1 && i != 0){
                c[i] = 0;
                c[i - 1] += 1;
            }
        }
    }
}

Buffer overrun

Your array w should be one larger than it currently is to hold a null terminating character.  Otherwise when you write to w[n] you are writing past the end of the array.

Excess copying

Currently, every time you generate a new permutation, you rewrite all of the w array.  You actually only have to rewrite the parts that changed since the last permutation.  To do this, you should write to w[i] only when c[i] changes.  So for example:

while(c[0] < l){
    c[n - 1] += 1;
    w[n - 1] = s[c[n - 1]];
    for(i = n - 1; i >= 0; i--){
        if(c[i] > l - 1 && i != 0){
            c[i] = 0;
            w[i] = s[0];
            c[i - 1] += 1;
            w[i - 1] = s[c[i - 1]];
        }
    }
}

Alternate implementation

If you are looking to find a faster way to generate permutations, I suggest you look at this codereview question and in particular my answer to it.


jQuery dialog for help popup I used jQuery-UI and jQuery to create a dialog popup for help snippets on search form inputs.

HTML (Calling Page)

    Name Search

Script to call dialog

   $("#Help").click(function () {
       $("#dialog-confirm").load("/help/nameSearch.htm").dialog({
           resizable: false,
           height: screen.height - 300,
           position: {
               my: "left top",
               at: "left top",
               of: "#Table1"
           },
           width: '790px',
           height: 'auto',
           modal: true,
           buttons: {
               "Okay": function () {
                   $(this).dialog("close");
               }
           },
           hide: { effect: "drop", direction: "up" },
           show: { effect: "drop", direction: "down" }
       });
   })

   $("#dialog-confirm").keypress(function (e) {
       if (e.keycode == $.ui.keyCode.ENTER) {
           $(this).dialog("close");
       }
   })

This works nicely to load the page from another folder in my site, but I get this weird glitchy feeling on the first time, which is going to probably be the most anyone will use these really awesome popup dialogues. I want them even better.

Is there any way to make it load more smoothly?

I am not opposed to using AJAX.

I would suggest loading the external file and initializing the dialog prior to the event...kind of like:

$(document).ready(function(){
    // make the dialog
    $("#dialog-confirm").load("/help/nameSearch.htm").dialog({
           autoOpen: false,
           resizable: false,
           height: screen.height - 300,
           position: {
               my: "left top",
               at: "left top",
               of: "#Table1"
           },
           width: '790px',
           height: 'auto',
           modal: true,
           buttons: {
               "Okay": function () {
                   $(this).dialog("close");
               }
           },
           hide: { effect: "drop", direction: "up" },
           show: { effect: "drop", direction: "down" }
       });
    }).keypress(function (e) {
       if (e.keycode == $.ui.keyCode.ENTER) {
           $(this).dialog("close");
       }
    });

    $("#Help").click(function () {
        $("#dialog-confirm").dialog( "open" );
    });

});

That way, any delay caused by the loading of the file/creation of the dialog is handled on page load instead of on button click.

Note the addition of the autoOpen:false option. That keeps it from opening on init.


Generic Property class Some time ago I wrote a little property class for a then abandoned project. I just stumbled over it again and I would like to know if the design is using good, efficient C++.

#pragma once

#ifndef NO_THREAD_SAFETY
#include <atomic>
#endif

template<typename T1>
class Property {
public:

#ifndef NO_THREAD_SAFETY
    typedef std::atomic<long> PropertyID;
    typedef std::atomic<T1>   PropertyValue;
#else
    typedef long              PropertyID;
    typedef T1                PropertyValue;
#endif

    Property(T1 value): value(value), id(id_count++) {
    }
    Property(): value(), id(id_count++) {
    }
    Property(const Property &rhs): id(id_count++) {
        value = static_cast<T1>(rhs.value);
    }
    Property(Property &&rhs): value(std::move(rhs.value)), id(id_count++) {
    }

    const Property &operator=(const Property &rhs) {
        value = std::move(static_cast<T1>(rhs.value));
        return *this;
    }
    const Property &operator=(const T1 &rhs) {
        value = rhs;
        return *this;
    }
    const Property &operator=(T1 &&rhs) {
        value = std::move(rhs);
        return *this;
    }

    bool operator==(const Property &rhs) const {
        return value == rhs.value;
    }
    bool operator==(const T1 &rhs) const {
        return value == rhs;
    }
    bool operator< (const Property &rhs) const {
        return value < rhs.value;
    }
    bool operator< (const T1 &rhs) const {
        return value < rhs;
    }
    void set(const T1 &value) {
        this->value = value;
    }

    T1 get() const {
        return value;
    }
    operator T1() const {
        return value;
    }
    PropertyID getID() const {
        return id;
    }

private:

    PropertyValue value;
    PropertyID id;
    static PropertyID id_count;
};

template<typename T1>
typename Property<T1>::PropertyID Property<T1>::id_count (0);

These are the things that stuck out to me:

Your copy constructor and move constructor don't result in an identical Property<T1> as they increment the id. Why is that? (There is no documentation anywhere in this that helps the consumer of your Property class.)
Your value constructor Property(T1 value) should probably initialize value with std::move(value). It has already received a copy (perhaps moved), so it is safe to move from value.
Your copy constructor uses what appears to be a superfluous static_cast<T1> and uses the constructor body instead of an initialization list. I would suggest this be more like the move constructor just without the call to std::move.
As nothing appears to change the value of id after it has been constructed, I don't see the benefit of id being atomic. I do understand the value of id_count being atomic, but making them both atomic seems to be asking for unnecessary overhead. Related, perhaps the usage of id_count should be moved to a static function so that it cannot be subverted.

static PropertyID getNextId() {      // assumes PropertyID is non-atomic
    static AtomicPropertyID id = 0;  // and AtomicPropertyID is conditionally atomic
    return id++;
}

I don't quite understand how Property<T1> is expected to be used. Does it go in a collection of properties? Is it used to declare as members of another type? If the former, the implementation of operator< doesn't seem to provide any useful characteristics for finding the right property. If the latter, the encapsulation seems to be a very long winded way of conditionally making a member atomic - since there is no business logic in the setter or getter methods, and no way to provide that other than inheriting, I'm unclear what the benefit is of using this class.


Convert .NET DateTime to a string using ordinals With a DateTime object, it's easy to get, for example, 11 October 2011 by using:

d.ToString("d MMMM yyyy");

However, there seems to be no built-in method to get the output 11th October 2011.

So here's a possible extension method.

public string ToStringWithOrdinal(this DateTime d) {
  var sb = new StringBuilder(d.Day);
  switch (d.Day) {
    case 1:
    case 21:
    case 31:
      sb.Append("st");
      break;
    case 2:
    case 22:
      sb.Append("nd");
      break;
    case 3:
    case 23:
      sb.Append("rd");
      break;
    default:
      sb.Append("th");
      break;
  }
  sb.Append(" ").Append(d.ToString("MMMM yyyy"));
  return sb.ToString();
}

Can anyone think of any improvements, either to performance or to add functionality for a format parameter, so we can call e.g. d.ToStringWithOrdinal("d^ MMMM");?

Thanks.

You can simplify it a bit and make a trivial improvement to performance (this will be noticeable if you call this method a lot).

public static string ToStringWithOrdinal(this DateTime d) {
  switch (d.Day) {
    case 1: case 21: case 31:
      return d.ToString("dd'st' MMMM yyyy");
    case 2: case 22:
      return d.ToString("dd'nd' MMMM yyyy");
    case 3: case 23:
      return d.ToString("dd'rd' MMMM yyyy");
    default:
      return d.ToString("dd'th' MMMM yyyy");
  }
}

As a side note this format doesn't read well to me. Normally I would write it as follows:

  October 11th 2011

Is this format common in the UK?


Java hashcode without using the Object#hashCode method For an assignment I have to make a HashTable using double hashing, but for the hashing part I am not allowed to use Object#hashCode(), so I have to write my own.

So far I have the following code and I am looking for feedback if I am on the right track or if I should go for a different approach (see hash and hash2 methods).

public class DoubleHashing<Key, Value> {

private int N;
private int M;
private Key[] keys;
private Value[] vals;

public DoubleHashing(final int capacity) {
    M = capacity;
    keys = (Key[]) new Object[M];
    vals = (Value[]) new Object[M];
}

public int hash(final Key key) {
    int hashKey = 0;
    if (key instanceof String) {
        final String k = (String) key;
        for (int i = 0; i < k.length(); i++) {
            int charCode = k.charAt(i);
            hashKey = ((hashKey * 27) + charCode) % M;
        }
    }
    return hashKey;
}

public int hash2(final Key key) {
    int hashKey = 0;
    if (key instanceof String) {
        final String k = (String) key;
        for (int i = 0; i < k.length(); i++) {
            int charCode = k.charAt(i);
            hashKey = 5 - ((hashKey * 27) + charCode) % 5;
        }
    }
    return hashKey;
}

public void put(final Key key, final Value val) {
    if (N == M) {
        System.err.println("Table is full");
        return;
    }
    int i;
    for (i = hash(key); keys[i] != null; i = (i + 1) % M)
        if (keys[i].equals(key)) { vals[i] = val; return; }
    keys[i] = key;
    vals[i] = val;
    N++;
}

public Value get(final Key key) {
    for (int i = hash(key); keys[i] != null; i = (i + 1) % M)
        if (keys[i].equals(key))
            return vals[i];
    return null;
}

public int size() {
    return N;
}
}

Use meaningful names for your fields:

private int N;
private int M;

What do those fields hold? What is the purpose of them?
Ask yourself those questions and use names that represent the meaning of your field:

private int size;
private int capacity;

Don't cast where you don't need to:

keys = (Key[]) new Object[M];
vals = (Value[]) new Object[M];

This makes no sense; why do you cast a generic Object[] array to a Key[]/Value[] array?
Simply create them without casting:

keys = new Key[M];
vals = new Value[M];

Your hashing methods don't return hashes

Your code is entirely broken in that regard. They only work for Strings and you use them for getting your index. This is not how hashing works!
I don't know if this is a restriction of your assignment, but no matter how large the capacity of your table is, your hashing method should always return the same value in order to be comparable to other objects.


Dice Calculation Dialect As a sample of dialects and teaching myself parse with block! values, I wrote the following dice dialect with the following grammar:

[
    word!
|
    integer! word!
]

Where the word!s are defined as integers (d10: 10) so an expression of this dialect would look like:

dice: [d2 20 d100 d10 5 d6]

This would sum the totals of one 2-sided die roll, 20 hundred-sided dice, one 10-sided die and five 6-sided dice.   I wrapped this in the function below

Rebol [
    Title: "Dice Rolling Dialect"
]
random/seed now/precise

d2: 2
d4: 4
d6: 6
d8: 8
d10: 10
d12: 12
d20: 20
d100: 100

calculate: function [b] [
    dice-rule: [
        (total: 0)
        some [
            copy die word!
            (total: total + roll 1 get die/1)
        |
            copy n integer!
            copy die word!
            (total: total + roll n/1 get die/1)
        ]
    ]

    roll: func [times [integer!] die [integer!]] [
        result: 0
        repeat i times [result: result + random die]
        result
    ]

    parse b dice-rule
    total
]

repeat i 400 [
    print calculate [d100 10 d10 d2 4 d6]
]

I was looking for improvements in the handling of the variables in parsing to make it more elegant than requiring the get die/1 notation.

To first answer your question about the die/1, you are using COPY to get a range of multiple values out of the source block.  You probably want SET to get a single value:

>> parse [d12] [copy die word! (probe die)]
[d12]
== true

>> parse [d12] [set die word! (probe die)]
d12
== true

So concretely:

dice-rule: [
    (total: 0)
    some [
        set die word!
        (total: total + roll 1 get die)
    |
        set n integer!
        set die word!
        (total: total + roll n get die)
    ]
]

Broad dialect design concern: you've decided that a symbolic word that looks up to an integer will represent a dice roll of that integer value.  You then have another meaning for literal integer values.  This ties your hands a bit because it means you can't do ordinary variable substitution.

For instance, what if instead of:

dice: [d2 20 d100 d10 5 d6]

I wanted to write:

x: 20
y: 5
dice: [d2 x d100 d10 y d6]

You could use get-word for this form of non-dice substitution:

dice: [d2 :x d100 d10 :y d6]

But the general heuristic of coming up with dialects is to only dig out a small bit of keyword space, so such an "ugliness" isn't necessary most of the time.  Most dialects pick out a finite set of keywords, but there's probably nothing universally wrong with taking all the d* words.

Yet consider a possible alternative, more like the substitutions in parse, where brackets would be optional:

dice: [
    die 2
    20 [die 100]
    [die 10]
    5 die 6
]

Which you might then allow as substitutions, maybe:

d2: [die 2]
d100: [die 100]
d10: [die 10]
d6: [die 6]

dice: [d2 20 d100 d10 5 d6]

Further, I'd think that the dialect (which you might call dice-roll) should provide all the values of the roll.  It's easy enough to sum if that's what people want...but sometimes you might want the highest value, or lowest, or know if there were any doubles of the same value/etc.

Other quick notes.  If you know b is a block, say so in the function specification:

calculate: function [b [block!]] [
    ...

That checks the type at the callsite, and in Red it offers more of an advantage for static typing in the compiler.

I'd define roll before I defined the dice-rule.  Also, by using a func instead of a function you are allowing result to bleed into the calculate function body.  If you don't have a good reason for that, I'd avoid it... func shouldn't be used for performance reasons, but rather when you really do not want to introduce a new context.

Especially if you go with my suggestion to make die a word in your dialect, I might use another word for the integer representing the number of possibilities on the die...perhaps something like radix.


Translate boolean flags to collection of numbers in scala I'm having trouble to implement the following requirement in a functional way in scala. Given a predefined number of boolean flags, I want to build up a collection of numbers. For each (true) flag, a hardcoded number should be included in the collection.

This is the unsatifying solution I have so far:

  def createCodeArray(flagA:Boolean, flagB:Boolean, flagC:Boolean): Seq[Int] = {
    val codes = ListBuffer.empty[Int]

    if(flagA) codes += 2
    if(flagB) codes += 7
    if(flagC) codes += 12

    return codes
  }

I'm trying to achieve to same with pattern matching, but I don't want to list all the possible combinations. Also, I'd like to avoid mutable data-structures.

Any suggestion on how to write this in a functional way is much appreciated

You could use tuples to pair your flags with an Int. Throw those into a Seq and then perform a filter and then a map on that Seq. Here is how that would look:

def f(a: Boolean, b: Boolean, c: Boolean): Seq[Int] = {

 val xs = Seq((a, 2), (b, 7), (c, 12))

 xs.filter(t => t._1).map(t => t._2)

}

Note that in Scala you don't need to use a return statement as long as the last bit of code in your function returns a new value.


Rails refactoring has_many I have a RoR 4 app, and its model have these associations:

has_many :accreditations
has_one :legal, -> { where(kind_cd: Accreditation.legal) }, class_name: 'Accreditation'
has_many :departments, -> { where(kind_cd: Accreditation.department) }, class_name: 'Accreditation'

So, you see that these associations similar, but legal & departments have more conditions than accreditations. Can I replace class_name: 'Accreditation' with something like using :accreditations?

There are two ways to tackle this situation. One of them is what you did: follow the Law of Demeter; I don't have any advice to improve your code. The second way is move the logic to the other model. In that case you'd write:

class Company
  has_many :accreditations
end

class Accreditation
  scope :departments, where(kind_cd: department)

  def self.legal
    where(kind_cd: legal).first
  end
end

Both are ok even though I prefer the second because the logic of departments/legal is within the accreditation model.


Large Number Limit Extravaganza I am writing a program that computes

$$n-n \cdot \left(\prod_{i=1}^n (1-\frac{1}{p_i})\right)$$

which is rewritten in my code as:

$$\left(1-\left(\prod_{i=1}^n(1-\frac{1}{p_i})\right)\right) \cdot n$$

There are lots of primes involved here especially as \$n\$ gets larger and larger. I do not want to use Atkin's method for generating them since my teacher and I will probably lose oversight as to what is going on, but would still like to optimize my algorithm for speed and accuracy (not sure if the latter can be better).

i = 0
j = 3
old = 1.0
primes = [2]
while True:
    if (j>240000): break
    cont = True
    enumerator = 0
    for e in primes:
        if j%e==0: cont = False
        if enumerator>=len(primes)/2 + 1: break
        enumerator += 1
    if cont: primes.append(j)
    j+=2
#print primes

while True:
    if (i>len(primes)-1): break
    old = old * (1 - 1.0/primes[i])
    print old
    i+=1;
primeslessthan = j-1
amta = 1-old

print "The convergence: " + str(primeslessthan)
print "All the way up to the number: " + str(amta)
print "The amount of active inhibitors are: " + str((1-amta)*primeslessthan)

I am open to multithreading, and to translating this to a different language like C++, but do not know what optimizations I could make there either.

Your method of generating primes is horribly slow. Running your code takes on the order of 5 minutes on my machine.

Consider as an alternative a prime sieve, even a simple one like the Sieve of Eratosthenes will do (you don't need to go full Atkinson on it):

def prime_sieve(limit):
    a = [True] * limit
    a[0] = a[1] = False

    for i, isprime in enumerate(a):
        if isprime:
            yield i
            for n in xrange(i * i, limit, i):
                a[n] = False

if __name__ == "__main__":
    prod = 1
    for p in prime_sieve(240000):
        prod *= (1 - 1.0 / p)
        print prod
    primes_less_than = p - 1
    amta = 1 - prod

    print "The convergence:", primes_less_than
    print "All the way up to the number:", amta
    print "The amount of active inhibitors are:", prod * primes_less_than

This runs in less than a second.

Accuracy

In your question you said you want to calculate $$n \cdot \left(1-\left(\prod_{i=1}^n(1-p_i)\right)\right)$$ But in your code you implemented $$n \cdot \left(1-\left(\prod_{i=1}^n(1-1/p_i)\right)\right)$$.

You do amta = 1 -old and then later (1 - amta) * primeslessthan. Here you are needlessly loosing precision, because 1 - (1 - x) != x due to floating point precision. Better use old directly in the output.

Review:
Python has an official style-guide, which programmers are encouraged to follow, PEP8.

Your code violates it in the following points:

Always put the expression after an if in a new line
Use spaces around operators
Use lower_case names for variables and functions
old is a bad name for a variable, amta even worse

In addition, the following are discouraged for iterating in python:

l = [1, 2, 3, ...]

for i in range(len(l)):
    print l[i]

i = 0
while True:
    if i > len(l) - 1:
        break
    print l[i]
    i += 1

Just use this simple syntax:

for x in l:
    print x

Note that the if does not need any parenthesis.

Finally, the print statement can take multiple expressions, separated by commas. Alternatively, you can use str.format:

print "The convergence: {}".format(primeslessthan)
print "All the way up to the number: {}".format(amta)
print "The amount of active inhibitors are: {}".format((1 - amta) * primeslessthan)


Unity3d class that inherits from button and add OnHold Event I made my own class that inherits from button and has an OnHold event. It seems to work ok but I'm wondering if I can improve it or if you can spot any glaring mistakes that I made.

Here is the code:

using UnityEngine;
using UnityEngine.UI;
using UnityEngine.EventSystems;
using UnityEngine.Events;
[AddComponentMenu("UI/Hold Button")]
public class HoldButton : Button
{
    private bool isBeingPressed;
    private float currTime;
    private static float TimeMax = 2;
    private bool startTimer;
    public ButtonHoldEvent onHold { get; set; }
    public override void OnPointerDown(PointerEventData eventData)
    {
        startTimer = true;
        currTime = 0;
    }
    public override void OnPointerUp(PointerEventData eventData)
    {
        startTimer = false;
        currTime = 0;
        isBeingPressed = false;
    }
    void Update()
    {
      if(startTimer)
        {
            currTime += Time.deltaTime;
            if(currTime >= TimeMax)
            {
                isBeingPressed = true;
            }else
            {
                isBeingPressed = false;
            }
        }
        if (isBeingPressed)
        {
            onHold.Invoke();
        }
    }

    public class ButtonHoldEvent : UnityEvent
    {
        public ButtonHoldEvent() : base()
        {

        }
    }
}

There is a few thing you can improve :

TimeMax is used like a constant here, but is not given the access modifier const.
Try to group your class members in a meaningful way. It could depend on their function(member of same interface), access-modifier, or what makes sense to you.

public ButtonHoldEvent onHold { get; set; }

private static float TimeMax = 2;

private bool isBeingPressed;
private float currTime;
private bool startTimer;

The name currTime doesn't reflect what it contains. heldDuration is more appropriate. The same applies for TimeMax and should be renamed TriggerTime.
isPressed is fine, but isHeldDown fits more the theme here.

Full code :

[AddComponentMenu("UI/Hold Button")]
public class HoldButton : Button
{
    public ButtonHoldEvent onHold { get; set; }

    private const float TriggerTime = 2;

    private bool isHeldDown;
    private float heldDuration;

    public override void OnPointerDown(PointerEventData eventData)
    {
        isHeldDown = true;
        heldDuration = 0;
    }
    public override void OnPointerUp(PointerEventData eventData)
    {
        isHeldDown = false;
        heldDuration = 0;
    }

    void Update()
    {
        if (!isHeldDown)
        {
            heldDuration += Time.deltaTime;

            if (heldDuration >= TriggerTime)
            {
                onHold.Invoke();

                // uncomment next line, to make `onHold` trigger only once per mouse press-hold
                //isHeldDown = false;
            }
        }
    }

    public class ButtonHoldEvent : UnityEvent
    {
        public ButtonHoldEvent() : base()
        {

        }
    }
}


Using method chains when rendering a scene Is writing Java code in chain like _hud.getStage().getCamera().combined an ugly practice?

@Override
public void render(float delta) {
    Gdx.gl.glClearColor(1,0,0,1);
    Gdx.gl.glClear(GL20.GL_COLOR_BUFFER_BIT);
    _batch.setProjectionMatrix(_hud.getStage().getCamera().combined);
    _hud.getStage().draw();
}

If yes, what would be the pretty way to write the code? And why is it ugly? What are the consequences of this?

Is writing Java code in chain like _hud.getStage().getCamera().combined an ugly practice?

The general design guideline (Law of Demeter) is to avoid such method chains. In some cases it can be justified.

The ugliest thing on this line is the _ prefix of a field which I suppose you intended as private, but there is no such convention in Java. Use camelCase for fields and methods, regardless of visibility, that's the convention.

Another unusual thing on this line is the direct use of the public field combined instead of a getter. It's unusual because a getter would give more flexibility for future extensions, as methods can be overridden by derived classes, while fields cannot be.

In any case, the chained call and the public field are not your "fault". The GDX library designers decided to provide this usage, and I assume they knew what they were doing. If you meant your question as "should I break up the chain to local variables and call them like that?" like this:

Stage stage = _hud.getStage();
Camera camera = stage.getCamera();
Matrix4 projection = camera.combined;

No, that wouldn't change the fact of the coupling, this is still a chained call, just expanded, and this is worse than the compact writing style, with no benefits within this snippet.

  If yes, what would be the pretty way to write the code?

Going just a tad further than @rolfl:

@Override
public void render(float delta) {
    Gdx.gl.glClearColor(1, 0, 0, 1);
    Gdx.gl.glClear(GL20.GL_COLOR_BUFFER_BIT);
    Stage stage = hud.getStage();
    batch.setProjectionMatrix(stage.getCamera().combined);
    stage.draw();
}

That is:

Avoid the _ prefix for all fields
Add a space after commas in parameter lists
Extract shared code (hud.getStage()) to local variable

It's not much prettier than the original, but you asked for it.

  What are the consequences of this?

The wiki page of Law of Demeter explains this in detail, but in a nutshell, chained method calls are tight coupling, unless the chained calls return the same object, such as in a fluent Builder or in a Java 8 Stream (thanks @sylvain-boisse, good point). It's ok that your class knows that hud has a Stage, but it's not ok that it knows that Stage has a Camera and further, that it has a projection named combined. The recommendation would be to provide a getCombinedCameraProjection method on hud, which would hide the class hierarchy, giving the designer the freedom to redesign the elements used behind that method (the Camera, etc), without affecting users of the API.

However, not following the Law of Demeter to the letter doesn't imply that this in itself is bad design. Applying LoD systemically everywhere would not be practical, as it could lead to an explosion of getters for the permutation of all uses of a Camera in this example. Multiple design recommendations often conflict (here encapsulation and usability), balancing them is extremely difficult. You have to choose between tradeoffs, and the designers of GDX have made their own, probably good choices.


Send email just once per day I want to send an email just once per day.  There is one button and any user can click that button.  The email must not be sent twice.  When the first user clicks the button, the email is sent.  If some other user clicks the button, the email will not be sent again.

This is how I implemented, and it's working.  There are some cases, once a month, when 2 email are sent, but I don't know why.

I am sure that there is more elegant way to resolve this problem.  Please share with me your ideas.

protected void Send(string email, String allRecipients, String subject, String body)
{
    //Be sure that email is send just once:
    //Lock an object, and if its already locked just continue,
    if (Monitor.TryEnter(syncLock))
    {
        try
        {
            //Send email only if is not send already.
            //Check in DB if is send.
            //IsSend function return true if email is send already, else false.
            if (!IsSent())
            {
                SendEmails(email, allRecipients, subject, body);
            }
        }
        finally
        {
            Monitor.Exit(syncLock);
        }
    }
}

Function that sends email:

public static void SendEmails(string email, String allRecipients, String subject, String body)
{
    //Flag if email is sent or not:
    Boolean isEmailSend = false;

    SmtpClient client = new SmtpClient();
    //Message:
    MailMessage message = new MailMessage(email, allRecipients, subject, body);
    try
    {
        isEmailSend = true;
        client.Send(message);

    }
    catch (SmtpException e)
    {
        isEmailSend = false;
        throw new SmtpException(e.ToString());
    }
    catch (TimeoutException e)
    {
        //Problem: Not sure if is email send or not:
        isEmailSend = false;
        throw new TimeoutException("TIME OUT:" + e.ToString());
    }
    catch (Exception e)
    {
        //Problem: Not sure if is email send or not:
        isEmailSend = false;
        throw new Exception(e.ToString());
    }
    finally
    {
        if (isEmailSend)
        {
            //Update DB.
            //Set that email is send.
            //This update in DB that email for current day is send already.
            SetAsSend();
        }
    }
}

IsSent function

    //Check if meeting is sent today.
    public static bool IsSent()
    {
        string connectionString = ConfigurationManager.ConnectionStrings["connectionString"].ConnectionString;
        bool isSent = false;

        const string CHECK_IF_SEND = "SELECT TOP 1 IsSent FROM MessageTable m WHERE m.date =@todayDate ";

        SqlConnection conn = new SqlConnection(connectionString);
        SqlCommand command = new SqlCommand(CHECK_IF_SEND, conn);
        command.Parameters.Add("@todayDate", DbType.Date).Value = DateTime.Today;

        try
        {
            conn.Open();
            Object result = command.ExecuteScalar();
            if (result != null)
            {
                isSent = (bool)result;
            }
        }
        finally
        {
            conn.Close();
            conn.Dispose();
        }
        return isSent;
    }

Couple of remarks:

IsSent() smells like a property, not a method. It should be more descriptive as well: what  object are you calling this on? It seems to indicate Email.IsSent but that's not what you do: your description makes it sound more like EmailManager.IsDailyEmailSent.
Dispose your SmtpClient instance.
Don't set isEmailSend to true before you actually send the email. You're already synchronizing using the monitor so that's not a reason. Set isEmailSent (notice the t instead of d to indicate past tense) to false from the get-go and set it to true after the sending inside the try block. Now you won't have to repeat the isEmailSent = false anymore either.
Don't rethrow exceptions like that, you're going to lose the stacktrace. If all you do is rethrow it without logging or handling then you might as well not catch it in the first place.


Screen recorder producing huge AVI files I'm currently trying to make a screen recorder in C#, and so far it works but the problem is that something as simple as a 20second video will take about 1GB of space. I have it setup so a timer continuously takes screenshots with this method:

void takeScreenshot()
{
    Rectangle bounds = Screen.FromControl(this).Bounds;
    using (Bitmap bitmap = new Bitmap(bounds.Width, bounds.Height))
    {
        using (Graphics g = Graphics.FromImage(bitmap))
        {
            //Add screen to bitmap:
            g.CopyFromScreen(new Point(bounds.Left, bounds.Top), Point.Empty, bounds.Size);
        }
        //Create and save screenshot:
        string name = path + "//screenshot-" + fileCount + ".jpeg";
        bitmap.Save(name, ImageFormat.Jpeg);
        inputImageSequence.Add(name);
        fileCount++;

        //Dispose of bitmap:
        bitmap.Dispose();
    }
}

And then it stores those pictures in a temporary folder in the D:// drive, and then when it's done it takes all the pictures and creates an AVI video out of them like this:

//Set bounds of video to screen size:
Rectangle bounds = Screen.FromControl(this).Bounds;
int width = bounds.Width;
int height = bounds.Height;

var framRate = 5;

using (var vFWriter = new VideoFileWriter())
{
    //Create new video file:
    vFWriter.Open(outputPath+"//video.avi", width, height, framRate, VideoCodec.Raw);

    //Make each screenshot into a video frame:
    foreach (var imageLocation in inputImageSequence)
    {
        Bitmap imageFrame = System.Drawing.Image.FromFile(imageLocation) as Bitmap;
        vFWriter.WriteVideoFrame(imageFrame);
        imageFrame.Dispose();
    }
    vFWriter.Close();
}
//Delete the screenshots and temporary folder:
DeletePath(path);

Any help on reducing the inefficiency of this is appreciated, I'm fairly new to this kind of programming.

takeScreenshot()

Based on the .NET Naming Guidelines methods should be named using PascalCase casing takeScreenshot() => TakeScreenshot()
You are enclosing the usage of the Bitmap inside a using statement which is the way to go but calling Dispose() on that Bitmap is superfluous because that is what a using statement is doing.
If you have two using statements without any code between you can stack them which saves one level of indentation like so (already removed the Dispose())

using (Bitmap bitmap = new Bitmap(bounds.Width, bounds.Height))
using (Graphics g = Graphics.FromImage(bitmap))
{
    //Add screen to bitmap:
    g.CopyFromScreen(new Point(bounds.Left, bounds.Top), Point.Empty, bounds.Size);

    //Create and save screenshot:
    string name = path + "//screenshot-" + fileCount + ".jpeg";
    bitmap.Save(name, ImageFormat.Jpeg);
    inputImageSequence.Add(name);
    fileCount++;
}

Comments should explain why something is done in the way it is done. Let the code itself tell what is done by using meaningful named things. In this way your comments won't lie like e.g //Create and save screenshot: which isn't what that code does. Saving yes but no creating of a screenshot is taking place.

Creating the video

Try to be consistent. Here you create a Bitmap and call Dispose() instead of using a using statement. In addition, sometime you use var and sometimes you use the concrete type although the type is seen at first glance.
If you are using the as operator you should always add a null check for that object because an as operator won't throw an exception but the resulting object may be null which will trigger an exception somewhere else.

Implementing these points will look like this:

Rectangle bounds = Screen.FromControl(this).Bounds;
var width = bounds.Width;
var height = bounds.Height;

var framRate = 5;

using (var vFWriter = new VideoFileWriter())
{
    vFWriter.Open(outputPath+"//video.avi", width, height, framRate, VideoCodec.Raw);

    foreach (var imageLocation in inputImageSequence)
    {
        using(var imageFrame = (Bitmap)System.Drawing.Image.FromFile(imageLocation))
        {
            vFWriter.WriteVideoFrame(imageFrame);
        }
    }
    vFWriter.Close();
}
DeletePath(path);

My guess about the big file size is that you are using the VideoCodec.Raw. Try to change it to some other codec and see if this helps.


Project Euler # 67 Maximum path sum II (Bottom up) in Python This is Project Euler #67: Maximum path sum II:

  By starting at the top of the triangle below and moving to adjacent numbers on the row below, the maximum total
  from top to bottom is 23.

  3
  7 4
  2 4 6
  8 5 9 3

  That is, 3 + 7 + 4 + 9 = 23.

  Find the maximum total from top to bottom in triangle.txt (right click and 'Save Link/Target As...'), a 15K text
  file containing a triangle with one-hundred rows.

  NOTE: This is a much more difficult version of Problem 18. It is not possible to try every route to solve this
  problem, as there are 299 altogether! If you could check one trillion (1012) routes every second it would take
  over twenty billion years to check them all. There is an efficient algorithm to solve it. ;o)

I previously posted a solution to problem 18 using a greedy algorithm, here's the optimal solution using bottom up recursion.

from time import time

def get_triangle(triangle_filename):
    """Return a list of lists containing rows of the triangle."""
    triangle = open(triangle_filename).read().split('\n')
    triangle = [[int(number) for number in row.split()] for row in triangle]
    return triangle

def maximize_triangle(triangle, start_index=-1):
    """Return the maximum triangle path sum(bottom up)."""
    if start_index == - len(triangle):
        return triangle[0][0]
    for index in range(len(triangle[start_index]) - 1):
        maximum = max(triangle[start_index][index], triangle[start_index][index + 1])
        triangle[start_index - 1][index] += maximum
    return maximize_triangle(triangle, start_index - 1)

if __name__ == '__main__':
    start_time = time()
    triangle_file = get_triangle('p067_triangle.txt')
    print(f'Maximum Path: {maximize_triangle(triangle_file)}')
    print(f'Time: {time() - start_time} seconds.')

The algorithm is very efficient, your program computes the result in fractions of a second.

Reading the file into a nested list can be simplified to

def get_triangle(triangle_filename):
    """Return a list of lists containing rows of the triangle."""
    triangle = [[int(number) for number in row.split()]
                for row in open(triangle_filename)]
    return triangle

because open() returns a file object that can be iterated over. As an additional advantage, no empty list is appended. Your reading function actually returns

[[59], [73, 41], [52, 40, 9], ... , []]

Here

triangle_file = get_triangle('p067_triangle.txt')

I find the naming confusing: The return value is not a file, and the nested list is called triangle everywhere else. Therefore I would change that here as well:

triangle = get_triangle('p067_triangle.txt')

In the main routine

def maximize_triangle(triangle, start_index=-1):

I would use a nested loop instead of recursion. That makes the start_index parameter with its default value obsolete, and the logic is easier to understand (in my opinion). Instead of start_index and index I would use row and col as variable names. Finally the function does not “maximize a triangle” but computes the “maximal path sum,” so let's name it accordingly:

def maximal_path_sum(triangle):
    """Return the maximum triangle path sum(bottom up)."""
    # Starting with the last row ...
    for row in range(len(triangle) - 1, 0, -1):
        # ... add maximum of adjacent entries to the corresponding entry
        # in the preceding row:
        for col in range(len(triangle[row]) - 1):
            maximum = max(triangle[row][col], triangle[row][col + 1])
            triangle[row - 1][col] += maximum
    return triangle[0][0]


Evaluating association strengths of items I have a list of animals with their counts:

import numpy as np
import pandas as pd
from random import randint

table = np.zeros((5,1), dtype=int)
for i in range(5):
    table[i]=randint(10, 20)

df1 = pd.DataFrame(columns=['Animal', 'Count'])
df1['Animal'] = animal_list
df1['Count'] = table
df1

And I have a matrix of how many times they appear together:

table = np.zeros((5,5), dtype=int)
animal_list = ['Monkey', 'Tiger', 'Cat', 'Dog', 'Lion']

for i in range(5):
    for j in range(5):
        table[i][j]=randint(0, 9)

df2 = pd.DataFrame(table, columns=animal_list, index=animal_list)
df2

I want to find the animals' association strength, which is defined like so - if Lion and Cat appear together 5 times, and Lion's count is 10 and Cat's count is 15, then Lion -> Cat association strength is 5/10=0.5, and Cat -> Lion association strength is 5/15=0.33.

I do it like so:

assoc_df = pd.DataFrame(columns=['Animal 1', 'Animal 2', 'Association Strength'])
for row_word in df2:
    for col_word in df2:
        if row_word!=col_word:
            assoc_df = assoc_df.append({'Animal 1': row_word, 'Animal 2': col_word,
                                        'Association Strength': df2[col_word][row_word]/df1[df1.Animal==row_word]['Count'].values[0]}, ignore_index=True)

assoc_df

The problem is, if I have a large number (say, 1000) of animals to loop on, it takes hours to finish computing the association strength table.

So, how to do I better optimize this last code block, the creation/generation process of this association table?

P.S.: In most practical use cases, df2 is a symmetric matrix, as "X appears together with Y" generally also means the same as "Y appears together with X". So, I am ok with solutions that assume that df2 is symmetric, and cut down the running time by half. In the above example, df2 is not a symmetric matrix, which is the more general case, and applicable for situations where we want to express meanings such as "X appears after Y" and "Y appears after X", which may not be the same.

Even in the non-symmetric case you can speed this up by not having the explicit double for loop. Since your case is not even normalized (apparently an animal can appear together with more than one other animal at a time), you do need your df1 as the normalization (instead of just e.g. df2.sum(axis=1). This gives you the matrix of the associations:

assoc_matrix = (df2.T / df1.set_index("Animal").Count).T
print(assoc_matric)
#           Monkey     Tiger       Cat       Dog      Lion
# Monkey  0.461538  0.692308  0.307692  0.692308  0.615385
# Tiger   0.200000  0.150000  0.200000  0.250000  0.250000
# Cat     0.062500  0.312500  0.250000  0.312500  0.000000
# Dog     0.000000  0.133333  0.333333  0.200000  0.466667
# Lion    0.500000  0.388889  0.333333  0.000000  0.333333

(I figured out the needed transposes by trial and error until I got the same values as in your post...)

And then you can use pandas.melt to get it into your format:

assoc_df2 = pd.melt(assoc_matrix.reset_index(), id_vars="index")
assoc_df2.columns = "Animal 1", "Animal 2", "Association Strength"
print(assoc_df2)

#    Animal 1 Animal 2  Association Strength
# 0    Monkey   Monkey              0.461538
# 1     Tiger   Monkey              0.200000
# 2       Cat   Monkey              0.062500
# 3       Dog   Monkey              0.000000
# 4      Lion   Monkey              0.500000
# 5    Monkey    Tiger              0.692308
# 6     Tiger    Tiger              0.150000
# 7       Cat    Tiger              0.312500
# 8       Dog    Tiger              0.133333
# 9      Lion    Tiger              0.388889
# 10   Monkey      Cat              0.307692
# 11    Tiger      Cat              0.200000
# 12      Cat      Cat              0.250000
# 13      Dog      Cat              0.333333
# 14     Lion      Cat              0.333333
# 15   Monkey      Dog              0.692308
# 16    Tiger      Dog              0.250000
# 17      Cat      Dog              0.312500
# 18      Dog      Dog              0.200000
# 19     Lion      Dog              0.000000
# 20   Monkey     Lion              0.615385
# 21    Tiger     Lion              0.250000
# 22      Cat     Lion              0.000000
# 23      Dog     Lion              0.466667
# 24     Lion     Lion              0.333333

Note that the order is different to your solution and this also contains the association of each animal with itself (and it is not one in this example, due to the normalization). You can just filter it out, though, if needed:

assoc_df2 = assoc_df2[assoc_df2["Animal 1"] != assoc_df2["Animal 2"]]

Finally, all of this only works as long as the dataframes still fit into memory, of course.


Create histogram with greater than symbol in x axis in pyplot I wanted to create a histogram from a list of positive integers. I want to bin it so that I show all single numbers, say K through N, with more than k elements in the data set, as well as the number of elements greater than N.

'''
The goal is to max a histogram from integer data.
The last bin should represent all cases with at least K elements.
 x x                    x x
 x x x                  x x x x
 x x x   x x    ---->   x x x x
____________           __________
 1 2 3 4 5 6            1 2 3 >3
'''
import matplotlib.pyplot as plt
import numpy as np

# Insert your favorite integer data here
data = [1, 1, 1, 2, 2, 2, 3, 3, 5, 6]

# Vanilla histogram for reference
hist, bins = np.histogram(data, bins=np.arange(1, 15))
center = (bins[:-1] + bins[1:]) / 2 - 0.5
f, ax = plt.subplots()
ax.bar(center, hist, align='center', edgecolor='k')
ax.set_xticks(center)
ax.set_title('vanilla hist')
plt.savefig('vanillahist')
plt.clf()

# Select the point after the last time we see at least k elements
K = 2
maxnum = bins[1:-1][np.abs(np.diff(hist >= K)) > 0][-1]

# filter the bins from numpy to only contain this point and those prior
center = bins[bins <= maxnum]

# filter frequency data from numpy;
# bins/hist are ordered so that the first entries line up
newhist = hist[(bins[:-1] <= maxnum)]
newhist[-1] += np.sum(hist[(bins[:-1] > maxnum)])

# make the plot, hopefully as advertised!
f, ax = plt.subplots()
ax.bar(center, newhist, align='center', edgecolor='k')
ax.set_xticks(center)
ax.set_xticklabels(list(center[:-1].astype(int)) + ['> %i' % (maxnum - 1)])
plt.savefig('myhist')
plt.clf()

This involved a lot of trial and error, and I'm still not 100% sure this can handle all cases, though it's passed every test I've tried so far. Could I have made this code more readable? I feel particularly unsure about lines 28-38. My justification for the [:-1] line is that the first entry of bins corresponds to the first entry of hist.

Nice work! You might also be interested in Matplotlib histogram with collection bin for high values.

I like the ascii-art explanation :-)

Things I see that could improve the code:

Put the histogram building in a function. This way others can import it / use it / copy-paste it more easily. Then it also becomes clearer what is the required input (data) and what are parameters that could be set by default (K, bins, title).
The name center is misleading. It is a list. And in fact those are the bins. So I would call it bins, overwriting the old value.
Instead of newhist you could call it bin_values or bin_heights.


Find same characters in 2 strings I have a solution, which compares 2 given strings and returns the number of total matching characters.

I know I haven't initialized the variables and arrays and that this (correctly) yields warnings. I know the variables aren't self-explanatory as they should be.

function commonCharacterCount($s1, $s2) {
    $a = (strlen($s1) > strlen($s2)) ? $s2 : $s1;
    $b = (strlen($s1) > strlen($s2)) ? $s1 : $s2;

    foreach(count_chars($a, 1) as $i => $v) {
        $c[chr($i)] = $v;
    }

    foreach (count_chars($b, 1) as $i => $v) {
        $d[chr($i)] = $v;
    }

    $t = 0;
    foreach($c as $k => $v) {
        if($c[$k] <= $d[$k]) {
            $t += $c[$k];
        } else {
            $t += $d[$k];
        }
    }
    return $t;
}

For instance, I have the two strings:

$s1 = "abacadeee"; and $s2 = "aabbccddee";, the expected output would be 7.

As required, this solution works so far and you can test it here:
sandbox

Which steps are unnecessary and how can I improve this algorithm?

I would recommend a process like this:

Code: (Demo)

$string1 = "abacadeee";
$string2 = "aabbccddee";

$counts2 = count_chars($string2, 1);

$tally = 0;
foreach (array_intersect_key(count_chars($string1, 1), $counts2) as $charcode1 => $count1) {
    $tally += min($counts2[$charcode1], $count1);
}
echo $tally;

count_chars() lends itself beautifully to this task, so using array functions onward is a sensible choice.

It is important to try to minimize iterations and not perform any useless iterations.  By calling array_intersect_key() on the two count_chars() results, the foreach() loop is only going to iterate elements with keys which are shared between the two arrays.  In doing this, you don't need to check which array is smaller (which is otherwise how you would choose which array to iterate).

$tally is incremented by the lesser of the two counts for each char.

p.s. calling chr() is irrelevant to your objective.


Variadic strcat for c++17 On some video from CPPcon, some one said that there should be variadic overload for operator+ as it can solve allocation when there are multiple + with the containers like std::string. So I just gave it a try, but I am not seeing a major performance boost. Are there any other ways to improve this?

#include <string>
#include <string_view>
#include <string.h>

using string_type = std::string_view;

inline std::string::size_type total_len(const string_type& str) {
    return str.length();
}

template<typename... String>
inline std::string::size_type total_len(const string_type& str, const String... strs) {
    return str.length() + total_len(strs...);
}

inline void append(size_t l, std::string& str1, const string_type& str2) {
    memcpy(str1.data() + l, str2.data(), str2.size());
    l += str2.size();
}

template<typename... String>
void append(size_t l, std::string& str1, const string_type& str2, String... strs) {
    void * p1 = str1.data() + l;
    const void * p2 = str2.data();
    memcpy(p1,p2, str2.size());
    l += str2.size();
    append(l, str1, strs...);
}

template<typename... String>
std::string StrCat(const string_type& str1, const string_type& str2, const String... strs) {
    std::string answer(total_len(str1, str2, strs...), 0);
    append(0, answer, str1, str2, strs...);
    return answer;
}

int main()
{
  StrCat("Hello, ","name","!\n");
}

Benchmarking is a tricky business. I'll focus more on the code part, you tell me if you get an improvement on the performance part too.

Containers or c-style strings?

  with containers like std::string

If you do want to work with containers, you can simplify your code a lot and avoid the use of std::string_view, which I believe triggers a good amount of conversions in your code.

For instance, your total_len function can be written as concisely as:

template<typename... String>
std::string::size_type total_len(const String&... strs) {
    return (strs.size() + ...);
}

Besides, I'm a bit skeptical about using memcpy on data() and such when you don't really need to. You won't beat std::string implementations that way, and you won't bring much flexibility either because at the end of the line, the product is still a std::string, not a newly malloced char*. But if a small error has gotten into your code, you'll be the guy who reinvented the wheel one time too many again.

Folding the right left way

Fold expressions are by the way more powerful than you seem to think. You don't have to be explicit about recursion. If you hang on to your std::strings, you can write a left folding in three lines:

template <typename... String>
void my_strcat_impl(String&&... strs) {
   (... += strs);
}
// <=> (((str1 += str2) += str...) += strN)

That means that the complete working example is rather short too:

#include <string>
using namespace std::string_literals;

template<typename... String>
std::string::size_type total_len(const String&... strs) {
    return (strs.size() + ...);
}

template <typename... String>
void my_strcat_impl(String&&... strs) {
    (... += strs);
}

template <typename... String>
std::string my_strcat(const String&... strs) {
    std::string result;
    result.reserve(total_len(strs...));
    my_strcat_impl(result, strs...);
    return result;
}

int main()  {
    auto test = my_strcat("Hello, "s,"name"s,"!\n"s);
}


Java game that generates and compares random numbers, looking for matches I have written some Java code for a school assignment. It is a game, and it works as follows:

The game makes 4 random numbers, all different, as the users ticket.
It then generates 4 other random numbers, again all different, to compare it to. If they match, you win. If they don't match, then you lose.

A = random 0-9
B = random 0-9
C = random 0-9
D = random 0-9

A Cannot be equal to B / C / D
B Cannot be equal to A / C / D
C Cannot be equal to A / B / D
D Cannot be equal to A / B / C

Here is the code:

import java.util.Random;
import java.util.Scanner;

public class Lottery {

   public static void main(String[] args) {

      Scanner scan = new Scanner(System.in);

      System.out.println("Lottery Game");

      Random rand = new Random();
      // Creating the four individual balls
      int ball1 = 0;
      int ball2 = 0;
      int ball3 = 0;
      int ball4 = 0;
      int guess1 = 0;
      int guess2 = 0;
      int guess3 = 0;
      int guess4 = 0;

      // THIS IS THE USERS TICKET NUMBER

      System.out.println("Generating Ticket");

      guess1 = rand.nextInt(10);
      guess2 = rand.nextInt(10);
      guess3 = rand.nextInt(10);
      guess4 = rand.nextInt(10);

      do {

          guess1 = rand.nextInt(10);
          guess2 = rand.nextInt(10);
          guess3 = rand.nextInt(10);
          guess4 = rand.nextInt(10);

      if ( guess1 == guess2 || guess1 == guess3 || guess1 == guess4 ) {
          guess1 = rand.nextInt(10);
      }
      if ( guess2 == guess3 || guess2 == guess4 ) {
          guess2 = rand.nextInt(10);
      }
      if ( guess3 == guess4 ) {
          guess3 = rand.nextInt(10);
      }

      } while (guess1 == guess2 || guess2 == guess3 || guess3 == guess4 || guess4 == ball1);

      System.out.println(guess1 + " " + guess2 + " " + guess3 + " " + guess4);

      System.out.println("Are you happy with the selected numbers?");

      int confirmation = 0;

      System.out.println("Please Type '1' For 'YES' Or '2' For 'NO");

      confirmation = scan.nextInt();

      System.out.println(confirmation);

      if (confirmation == 1) {

          ball1 = rand.nextInt(10);
          ball2 = rand.nextInt(10);
          ball3 = rand.nextInt(10);
          ball4 = rand.nextInt(10);

          do {

              ball1 = rand.nextInt(10);
              ball2 = rand.nextInt(10);
              ball3 = rand.nextInt(10);
              ball4 = rand.nextInt(10);

          if ( ball1 == ball2 || ball1 == ball3 || ball1 == ball4 ) {
              ball1 = rand.nextInt(10);
          }
          if ( ball2 == ball3 || ball2 == ball4 ) {
              ball2 = rand.nextInt(10);
          }
          if ( ball3 == ball4 ) {
              ball3 = rand.nextInt(10);
          }

          } while (ball1 == ball2 || ball2 == ball3 || ball3 == ball4 || ball4 == ball1);

          System.out.println("The Winning Numbers Are As Follows...");
          System.out.println(ball1 + " " + ball2 + " " + ball3 + " " + ball4);

          if ( ball1 == guess1 &&  ball2 == guess2 &&  ball3 == guess3 &&  ball4 == guess4) {
              System.out.println("Congratulations! You've Won £1,000,000!");
              System.exit(0);
          }
          else {
              System.out.println("Unfortunately, You Did Not Win. Better Luck Next Time!");
              System.exit(0);
          }

      }
      else if (confirmation == 2) {
          System.out.println("Exiting Game, Thanks for playing!");
      }
      else {
          System.out.println("ERROR");
      }

      }

   }

Specifically, I would like to improve the algorithm that I've used here. I don't think it is very elegant, even though it does work.

I'm also interested in other ways that my code can be improved. I would appreciate any suggestions.

Besides minor improvements like consistent formatting and proper indentation I would like to target your random number generation:

int guess1 = 0;
int guess2 = 0;
int guess3 = 0;
int guess4 = 0;

guess1 = rand.nextInt(10);
guess2 = rand.nextInt(10);
guess3 = rand.nextInt(10);
guess4 = rand.nextInt(10);

do {

    guess1 = rand.nextInt(10);
    guess2 = rand.nextInt(10);
    guess3 = rand.nextInt(10);
    guess4 = rand.nextInt(10);

if ( guess1 == guess2 || guess1 == guess3 || guess1 == guess4 ) {
    guess1 = rand.nextInt(10);
}
if ( guess2 == guess3 || guess2 == guess4 ) {
    guess2 = rand.nextInt(10);
}
if ( guess3 == guess4 ) {
    guess3 = rand.nextInt(10);
}

} while (guess1 == guess2 || guess2 == guess3 || guess3 == guess4 || guess4 == ball1);

While not incorrect, that certainly looks a bit clumsy.

Since you want four unique random numbers, a better choice would be to create a list of numbers from 0-9, shuffle it and then simply take the first four elements. And while you're at it, convert your four individual variables guess1 to guess4 to a single int[] array of guesses.

This allows you to replace the above code with simply:

List<Integer> numbers = IntStream.range(0, 9).boxed()
        .collect(Collectors.toList());
Collections.shuffle(numbers);
int[] guesses = new int[4];
for (int i = 0; i < 4; i++) {
    guesses[i] = numbers.get(i);
}

(Refactor that into a separate method and you can re-use it for your balls generation as well.)

When guesses and balls are both arrays, the comparison could also be simplified to:

Arrays.equals(guesses, balls);

Arrays.equals:

  Returns true if the two specified arrays of ints are equal to one another. Two arrays are considered equal if both arrays contain the same number of elements, and all corresponding pairs of elements in the two arrays are equal. In other words, two arrays are equal if they contain the same elements in the same order.

(Emphasis by me)