Using Array to store calculations in VBA I have the following VBA code, which works perfectly well to calculate "q"

However, the code is very slow and that is due to the large number of q's being calculated (roughly 7.2m q's are being calculated).

So I thought the best way to proceed is to try store the calculated q values in an array and then dump them into the spreadsheet once all of them are calculated.

The q's will vary for each i and j combination. I have tried to add the following to try and store results in an array to the to the main code given below:

Dim results() as variant
Redim results(I,j)
Results (I,j)= q
Range("G5").value=results

This did not work and I know it does not even look half right, but if you could help me spot where I am going wrong it would be really appreciated.

Sub mort()
Dim age As Integer
Dim month As Integer

For i = 0 To ActiveSheet.Range("F5", Range("F5").End(xlDown)).Count

For j = 0 To ActiveSheet.Range("G3", Range("G3").End(xlToRight)).Count

    gender = Range("C5").Offset(i, 0)

    If gender = "F" Then
    mortable = Worksheets("Female Tabs").Range("A3:C122")
    Else
    mortable = Worksheets("Male Tabs").Range("A3:C122")
    End If

month = Range("G3").Offset(0, j)
age = WorksheetFunction.RoundDown(Range("F5").Offset(i, 0) + (month - 3) / 12, 0)

If age < 119 Then

    a = (12 - ((month - 3) Mod 12)) / 12
    a1 = Application.VLookup(age, mortable, 3, False)

    b = ((month - 3) Mod 12) / 12
    b1 = Application.VLookup(age + 1, mortable, 3, False)

Else

a1 = 0
b1 = 0

End If

q = (1 / 12) * (a * a1 + b * b1)

Worksheets("Policy Mortality Qx").Range("G5").Offset(i, j).Value = q

Next j
Next i

End Sub

Most of your variables aren't defined and your indenting isn't great.

When you don't define your variable, VBA will declare it as a Variant, which are objects:

  Performance. A variable you declare with the Object type is flexible
  enough to contain a reference to any object. However, when you invoke
  a method or property on such a variable, you always incur late binding
  (at run time). To force early binding (at compile time) and better
  performance, declare the variable with a specific class name, or cast
  it to the specific data type.

By not declaring variables, you could possibly be paying a penalty.

It's good practice to indent all of your code that way Labels will stick out as obvious.

Here's the first thing I would do -

Option Explicit
Sub mort()
    Dim age As Integer
    Dim month As Integer
    Dim i As Long
    Dim j As Long
    Dim gender As String
    Dim mortable As String
    Dim a As Long
    Dim b As Long
    Dim a1 As Long
    Dim b1 As Long
    Dim q As Long

    For i = 0 To ActiveSheet.Range("F5", Range("F5").End(xlDown)).Count
        For j = 0 To ActiveSheet.Range("G3", Range("G3").End(xlToRight)).Count

        gender = Range("C5").Offset(i, 0)

        If gender = "F" Then
            mortable = Worksheets("Female Tabs").Range("A3:C122")
        Else: mortable = Worksheets("Male Tabs").Range("A3:C122")
        End If

        month = Range("G3").Offset(0, j)
        age = WorksheetFunction.RoundDown(Range("F5").Offset(i, 0) + (month - 3) / 12, 0)

        If age < 119 Then
            a = (12 - ((month - 3) Mod 12)) / 12
            a1 = Application.VLookup(age, mortable, 3, False)
            b = ((month - 3) Mod 12) / 12
            b1 = Application.VLookup(age + 1, mortable, 3, False)
        Else
            a1 = 0
            b1 = 0
        End If

        q = (1 / 12) * (a * a1 + b * b1)
        Worksheets("Policy Mortality Qx").Range("G5").Offset(i, j).Value = q

        Next j
    Next i
End Sub

Next, I'd deal with the Variable names - give your variables meaningful names. As well as your procedure

Also if mortable is a Range then mortable = Worksheets("Female Tabs").Range("A3:C122") needs to be Set mortable = Worksheets("Female Tabs").Range("A3:C122")

Private Sub mortable()
    Dim age As Long
    Dim month As Long
    Dim gender As String
    Dim mortable
    Dim i As Long
    Dim j As Long
    Dim ageMonths As Long
    Dim ageMonthsLookup As Long
    Dim ageMonthsIncrease As Long
    Dim ageMonthsIncreaseLookup As Long
    Dim calculation As Long
End Sub

This If block can be simplified

   If age < 119 Then
        a = (12 - ((month - 3) Mod 12)) / 12
        a1 = Application.VLookup(age, mortable, 3, False)
        b = ((month - 3) Mod 12) / 12
        b1 = Application.VLookup(age + 1, mortable, 3, False)
    Else
        a1 = 0
        b1 = 0
    End If

Into

        If age < 119 Then
            ageMonths = (12 - ((month - 3) Mod 12)) / 12
            ageMonthsLookup = Application.VLookup(age, mortable, 3, False)
            ageMonthsIncrease = ((month - 3) Mod 12) / 12
            ageMonthsIncreaseLookup = Application.VLookup(age + 1, mortable, 3, False)
            calculation = (1 / 12) * (a * a1 + b * b1)
        Else: calculation = 0
        End If

This piece

ActiveSheet.Range("F5", Range("F5").End(xlDown)).Count

Is not the best way to find the bottom of a range

Dim lastRow As Long
Dim lastColumn As Long
lastRow = Cells(Rows.Count, "F").End(xlUp).Row
lastColumn = Cells(3, Columns.Count).End(xlToLeft).Column
For i = 0 To lastRow
    For j = 0 To lastColumn

Eh, that's it for now from me.


Find a number which equals to the total number of integers greater than itself in an array I am solving interview questions from here.

  Problem : Given an integer array, find if an integer p exists in the array such that the number of integers greater than p in the array
  equals to p.  If such an integer is found return 1 else return -1

  Approach First remove all the negative numbers from the given array and then sort it. Now from the sorted array check for the given
  condition.

How can I optimize my solution :

def solve(A):
    p = -1
    sum_ind = 0
    sub_array = [x for x in A if x>=0]
    sub_array.sort()
    print sub_array
    for i in range(len(sub_array)):
        for j in range(i+1,len(sub_array)):

            if sub_array[j] > sub_array[i]:
                sum_ind += 1

        if sub_array[i] == sum_ind:
            p = 1
            break
        else:
            sum_ind = 0

    return p

Test Cases:

print solve([ ]) == (-1)
print solve([ -1, -2, 0, 0, 0, -3 ]) == (1)
print solve([4,7,8,9,11]) == (1)
print solve([-4, -2, 0, -1, -6 ]) == (1)
print solve([-4, -2, -1, -6 ]) == (-1)
print solve([6,7,5 ]) == (-1)
print solve([ 4, -9, 8, 5, -1, 7, 5, 3 ]) == (1)

Let's re-think the problem a little. Let's start as you suggest, by discarding negative values and sorting the array (I'm going to use descending order):

A = sorted([x for x in A if x >= 0], reverse=True)

Now that we have all positive values in descending order, we find a really simple solution to this problem: if any value equals its own index in the array (which is equivalent to how many values are greater than it in the array), then we return 1:

return 1 if any(v == i for i, v in enumerate(A)) else -1

You can see this if you consider a sample array--I'll borrow your example--such as [4,7,8,9,11]. Once sorted in descending order, we can pair each value with its index to get [(0, 11), (1, 9), (2, 8), (3, 7), (4, 4)]. Because of the sorting, the index will always represent the number of values in the array greater than the value at that index.

The exception to this trend is when you have multiple, equal integers, in which case we need to modify this list to not keep incrementing when iterating across duplicate values. E.g., for A = [3, 3, 3, 3, 2, 1], we need the resulting list to be [(0, 3), (0, 3), (0, 3), (0, 3), (4, 2), (5, 1)]. You can solve this using a list that retroactively fixes the enumerated list:

for i, v in A[1:]:
    if v == A[i-1][1]:
        A[i][0] = A[i-1][0]

The final function looks like the following:

def solve(A):
    A = sorted([x for x in A if x >= 0], reverse=True)
    A = list(map(list, enumerate(A)))
    for i, v in A[1:]:
        if v == A[i-1][1]:
            A[i][0] = A[i-1][0]
    return 1 if any(v == i for i, v in A) else -1


Python model for a "snake"-like game I wrote a python model for the game "snake" that I'm not really satisfied with.

My intention was to separate the game logic from the drawing and input handling (which worked quite well) but I've got several points which I don't know how to solve better.

To use this model one needs to initialize it with callbacks that fire when the score increases and when the game is over. They pass the score as an integer. I don't like this solution because it feels a lot like C, passing around function pointers.
For the drawing the user needs to access the head, tail and food variables in the model. Should I write explicit getters for that? I prefixed everything the user shouldn't directly access with an underscore. How can I make this interface more clear?
One needs to call _move_snake(dx, dy) with the direction dx, dy the snake should move although the model already has a variable that holds the snake's direction. But I guess this way it's easier for testing?

import random

class SnakeModel(object):

    """
    Model for a snake-like game.
    """

    def __init__(self, width, height,
                 gameover_callback, atefood_callback):
        """
        Initializes the game

        gameover_callback and atefood_callback are callback functions. They
        are passed the current player score (length of the snake's tail) as an
        integer.
        """
        self.width = width
        self.height = height
        self._gameover = gameover_callback
        self._ateFood = atefood_callback
        self.reset()

    def reset(self):
        """
        Starts a new game.

        Sets the starting position, length and direction of the snake as well
        as the position of the first food item.
        """
        self._dir = (0, -1)

        # You can read head, tail and food directly for drawing
        self.head = (self.width / 2, self.height - 2)
        self.tail = [(self.width / 2, self.height - 1)]
        self.food = (self.width / 2, self.height / 3)

    def step(self):
        """
        Advances the game one step
        """
        # check for collisions
        if self.head in self.tail:
            self._gameover(len(self.tail))

        # eat food
        if self.head == self.food:
            self.tail.insert(0, self.head)
            self.food = self._new_food_location()
            self._ateFood(len(self.tail))

        # move snake
        self._move_snake(*self._dir)

    def set_snake_direction(self, dx, dy):
        """
        Sets the direction the snake will move on the next step
        """
        self._dir = (dx, dy)

    def _move_snake(self, dx, dy):
        """
        Moves the snake one step in the given direction.

        The snake can move through the game area's borders and comes out on the
        other side.
        """
        x, y = self.head
        newX = (x + dx) % self.width
        newY = (y + dy) % self.height
        self.head = (newX, newY)
        self.tail.insert(0, (x, y))
        self.tail.pop()

    def _new_food_location(self):
        """
        Returns a new possible food position which is not in the snake.
        """
        game_field = set([(x, y) for x in range(self.width)
                          for y in range(self.height)])
        possible = game_field - set(self.tail) - set([self.head])
        new_position = random.choice(list(possible))
        return new_position

1. Answers to your questions

Callbacks. Why not use a return value intead?

def step(self):
    """Advance the game one step.
    Return a pair of Booleans (game over, ate food).

    """
    game_over = self.head in self.tail
    ate_food = self.head == self.food
    if ate_food:
        self.tail.insert(0, self.head)
        self.food = self._new_food_location()
    self._move_snake(*self._dir)
    return game_over, ate_food

(But see below for two bugs in this function.)
Getters are normally used in three scenarios. First, to provide a simple interface to a complex data structure. Second, to give the implementer freedom to change the implementation in future. Third, in combination with setters when writing a public API whose internal data structures have an invariant that needs to be preserved, so as to protect the internal data structures from corruption by a well-meaning but incompetent client.

None of these scenarios applies here. The internal data structures are straightforward, so the first scenario does not apply. And this is all your own code (not a public API) so the second and third scenarios do not apply.
Redundancy in _move_snake. It doesn't seem like a big deal either way. No opinion here.

2. Other comments on your code

There's a bug in step: you move the snake even if the game is over. Surely the snake should stop moving when it's dead?
Another bug in step, this time in the "eat food" logic. You insert the head into the tail:

self.tail.insert(0, self.head)

but then you call _move_snake which does the same thing. So the snake ends up containing two copies of the head location.

The usual way that "snake" games work is that when the snake eats some food, it does not grow a new tail segment immediately. Instead, it waits until the next time it moves and grows a new tail segment in the position where its old tail used to be. This is easily implemented by incrementing a counter each time the snake eats food:

self.growth_pending += 1

and then decrementing the counter instead of deleting the tail segment:

if self.growth_pending:
    self.growth_pending -= 1
else:
    self.tail.pop()

Note that these bugs would have been easy for you to spot had you actually finished writing the game and tried to run it.
Inserting an item at the beginning of a list:

self.tail.insert(0, (x, y))

takes time proportional to the length of the list (see the TimeComplexity page on the Python wiki). It would be better to use a collections.deque and call the appendleft method.
This line of code:

game_field = set([(x, y) for x in range(self.width)
                  for y in range(self.height)])

doesn't need the list comprehension, as you can pass a generator expression directly to set:

game_field = set((x, y) for x in range(self.width)
                 for y in range(self.height))

This avoids constructing a list and then immediately throwing it away again.
You can further simplify that line of code using itertools.product:

from itertools import product
game_field = set(product(range(self.width), range(self.height)))

In _new_food_location you convert possible into a list so that you can call random.choice on it. But if you used random.sample instead, you wouldn't have to do that.
In _new_food_location you build a set containing every location in the game not occupied by the snake. It would be better to build this set just once per game, and keep it up to date as the snake moves.

In detail, in reset you'd write something like:

self.growth_pending = 0
self.empty = set(product(range(self.width), range(self.height)))
self.empty.remove(self.head)
self.empty.difference_update(self.tail)

(Note the use of difference_update to avoid having to convert self.tail to a set.)

In _move_snake you'd write something like:

self.tail.appendleft(self.head)
self.head = newX, newY
self.empty.remove(self.head)
if self.growth_pending:
    self.growth_pending -= 1
else:
    self.empty.add(self.tail.pop())

And finally, in _new_food_location you'd write:

new_position = random.sample(self.empty, 1)[0]


Stack challenge - improving memory consumption I am working on this problem:

  Stacks

  Imagine, that you are employed by a software development company. You
  work now on the famous "D++ project", which is devoted to the creation
  of a new generation programming language. Your particular task is
  quite prosaic, though. You are to develop the memory manager being
  able to work with a large number of stacks.

  Input

  The first line of the input contains the total number of stack
  operations \$N\$, \$0 < N ≤ 100000\$. Each of the next \$N\$ lines contains a
  description of a stack operation, either in the form PUSH A B (meaning
  to push B into stack A), or in the form POP A (meaning to pop an
  element from stack A), where A is the number of stack (\$1 ≤ A ≤ 1000\$),
  and B is an integer (\$0 ≤ B ≤ 10^9\$). You may assume, that every
  operation is correct (i.e., before each POP operation, the respective
  stack is not empty).

  Output

  For each POP operation, described in the input, output the value,
  which this POP operation gets from the top of that stack, to which it
  is applied. Numbers should appear according to the order of the POP
  operations in the input. Each number should be output in a separate
  line.

I have developed a solution here:

#include<map>

class MyStack
{
    public:

      int data;
      MyStack *link;
      MyStack *top;

      MyStack()
      {
          link = NULL;
          top = NULL;
      }

      void push( int data )
      {
           MyStack * tmp = (MyStack *) malloc( sizeof( MyStack ) );
           tmp->data = data;
           tmp->link = top;
           top = tmp;
      }

      void pop()
      {
          MyStack *tmp = top;
          top = top->link;
          printf( "%d\n", tmp->data );
          free( tmp );
      }

      void clear()
      {
          while( top != NULL )
          {
              MyStack *tmp = top;
              top = top->link;
              free( tmp );
          }
      }

      bool isEmpty()
      {
           return top == NULL;
      }
};

int main()
{
    static MyStack* stackPtrs[1001];

    int noOfInstructions = 0;
    scanf( "%d", &noOfInstructions );

    char instruction[3];
    short stackNumber = 0;
    int stackValue = 0;
    for( int inx = 0; inx < noOfInstructions; ++inx )
    {
        scanf("%s", &instruction );

        scanf( "%hd", &stackNumber );

        if( instruction[1] == 'U' )
        {
            scanf( "%d", &stackValue );

            if( stackPtrs[stackNumber] == NULL )
            {
                MyStack *myStack = new MyStack();
                myStack->push( stackValue );
                stackPtrs[stackNumber] = myStack;
            }
            else
            {
                stackPtrs[stackNumber]->push( stackValue );
            }
        }
        else
        {
            stackPtrs[stackNumber]->pop();
            if( stackPtrs[stackNumber]->top == NULL )
            {
                stackPtrs[stackNumber] = NULL;
            }
        }
    }

    for( int inx =0; inx < 1001; ++inx )
    {
         if( stackPtrs[inx] != NULL )
         {
             stackPtrs[inx]->clear();
             stackPtrs[inx] = NULL;
         }
    }
    // Clear memory allocated here!.

    return 0;
}

The memory limit for this question is around 0.75MB = 750KB. My program, out of 20-30 runs on an average is taking around 800KB, with 770KB being the least time.

Please suggest any improvements to this program.

My main problem is that you are dynamically allocating all objects with new and or malloc. In C++ object can be created locally without new just by declaring them in the local scope.

MyStack   stack;      // Creates and initializes a MyStack object.

MyStack   data[10];   // Create and initialize an array of MyStack objects.

See no need to call new at all most of the time.

The second problem is that you are using malloc. This is C memory management. The blocks are not necessarily in the same areas as memory allocated by new and thus not compatible to be deleted by either. Also they don't run the constructor and free will not run the destructor. So a very bad idea.

The third observation is that you are building a container (linked list). There is already a built-in linked list.

std::list<int>   data;

For this structure of a stack. The list is not the optimal data structure. A std::vector or std::deque is probably better. But we have a built-in std::stack which is actually a container adapter that wraps another container type (by default it uses std::deque).

std::stack<int>   data;  // This is probably your best option
                         // for holding data.

If you want multiple stacks. I would just create a std::vector of stacks that you can the size at runtime to get all the stacks you need.

std::vector<std::stack<int>> data;
data.resize(<Number-Of-Stacks-You-Need>);

That should solve your memory problems.

Looking at your code we could go into your design of MyStack (but I think that is superflous). But some common rules:

Don't use C facilities when better C++ ones exist.
scanf() => operator>>
Don't mix dynamic memory allocation.
Only use new and delete in C++ code.
Every call to new should be matched with a call to delete
But you should not even do that normally:
a. Use local object most of the time.
b. Use containers when you have groups of objects.
c. When you really need dynamic allocation use smart pointers.
Learn the Rule of 3


x64 assembly clearmem / zeromem I've just started learning assembly yesterday, and the first useful thing I've written is a clearmem function.

I'm looking for general feedback regarding my coding of this function, whether there any flaws with it other than the obvious one of a user passing a value <= 0 to the size argument, or an invalid pointer to the ptr argument (should I really even check for that in practical use)?

I also observed an odd behavior that I still don't quite understand, so if you can explain that, then that would be much appreciated as well.

Intel x64 Assembly - on Linux

clearmem:                         ; void clearmem( void* ptr, long size )
    mov rcx, rsi                  ; copy rsi/size to rcx (aka the counter register)
    next:                         ; for( long i = size; i > 0; i-- )
        mov byte [rdi+rcx-1], 0   ;    *(reinterpret_cast<char*>(ptr+i-1)) = 0;
    loop next                     ; dec rcx; cmp rcx, 0; jg next
    ret                           ; return

Intel x64 Assembly - on Windows

clearmem proc
    xchg rcx, rdx                 ; swap arg1 and arg2... windows uses rcx for arg1, and rdx for arg2
    next:
        mov byte ptr[rdx+rcx-1], 0
    loop next
    ret
clearmem endp

Example usage in C++

#include <iostream>
// ...
extern "C" void clearmem( void* ptr, long size );
// ...
int main( int argc, char* argv[] )
{

    int z[] = {1,2,3,4,5,6,7,8,9,10};
    int zlen = sizeof(z) / sizeof(int);

    std::cout << "z[] before clearmem() = ";
    print_array<int>(z, zlen);

    clearmem(&z, sizeof(z));
    std::cout << "z[] after clearmem() = ";
    print_array<int>(z, zlen);

    return 0;
}

Output of C++ program

z[] before clearmem() = 1,2,3,4,5,6,7,8,9,10
z[] after clearmem() = 0,0,0,0,0,0,0,0,0,0

Odd Behavior

If I change all of the registers and variables to their 4 byte equivalent (e.g. rcx -> ecx, long -> int) then it still works fine in Windows, but on Linux it will segfault. I used GDB to set a break point on mov byte[rdi+rcx-1], 0 and used info registers to see the values in the registers and they were extrmely high. For instance rdi was 0x00007FFFFFFFE6E0 at the time, and considering I only have 16 GB of RAM, I would have expected a value less than 0x0000000400000000.

What's going on here? Why does this work on Windows, but not on Linux? Note: Obviously, I know I shouldn't do this, but I like breaking things...

The comments in the clearmem procedure for the Linux block look a bit confusing.  You could just have a summary of the procedure commented above, and have the individual comments for each line specify the meaning of the assembly instructions.

Specifically, the lines that describe the C++ code don't quite reflect on the assembly code.  clearmem itself doesn't take two arguments.  The for loop comment statement doesn't seem needed, and it may be clearer to specify when the loop should exit.  You also don't need the obvious comment for ret.
Since it appears the extern "C" corresponds to clearmem, I assume everything else can fully utilize C++.  If so, I'd use std::vector in place of the C-style arrays.  However, since clearmem performs pointer arithmetic, you should use std::vector::data() to pass in the underlying dynamic array.

You can then call size() for the vector object in main() instead of using sizeof.  This function returns an std::size_type, specifically std::vector<int>::size_type.
I'm not sure how print_array() is implemented, but I assume it just uses a for loop to print the array elements.  You might also not need the template argument, and with just a vector, you'll have just one argument.  In the function's loop, you should then use const_iterators for displaying the elements, although using indices is okay (but iterators are still preferred).
clearmem should take size_t instead of long; the former is the same type returned by the sizeof operator.


Pure Swift solution for socket programming I have spend quite some time now to find out how exactly to do socket programming in Swift (Xcode 6.1). There are a few examples out there, but most of them no longer work for the latest release of Swift.

Finally I came up with the following (see below). That seems to work, but I am not 100% certain that it is fully correct. Maybe some of you can have a look at it and see if I made any errors that might cause future (i.e. currently under the hood) problems.

The calls to "log...." refer to a self made logging facility, but that should be clear enough I think.

Any suggestions on how to improve this are welcome.

var status: Int32 = 0

// Protocol configuration

var hints = addrinfo(
    ai_flags: AI_PASSIVE,       // Assign the address of my local host to the socket structures
    ai_family: AF_UNSPEC,       // Either IPv4 or IPv6
    ai_socktype: SOCK_STREAM,   // TCP
    ai_protocol: 0,
    ai_addrlen: 0,
    ai_canonname: UnsafeMutablePointer<Int8>.null(),
    ai_addr: UnsafeMutablePointer<sockaddr>.null(),
    ai_next: UnsafeMutablePointer<addrinfo>.null())

// For the result from the getaddrinfo

var servinfo = UnsafeMutablePointer<addrinfo>.null()

// Get the info we need to create our socket descriptor

status = getaddrinfo(UnsafePointer<Int8>.null(), ap_servicePortNumber, &hints, &servinfo)

// Cop out if there is an error

if status != 0 { return }

// Print a list of the found IP addresses

if ap_applicationInDebugMode {

    var info = servinfo

    while info != UnsafeMutablePointer<addrinfo>.null() {

        // Check for IPv4
        if info.memory.ai_family == AF_INET {

            var ipAddressString = [CChar](count:Int(INET_ADDRSTRLEN), repeatedValue: 0)
            inet_ntop(
                info.memory.ai_family,
                info.memory.ai_addr,
                &ipAddressString,
                socklen_t(INET_ADDRSTRLEN))
            let addressStr = String.fromCString(&ipAddressString)
            var message = addressStr == nil ? "No IPv4 address found" : "IPv4 address: " + addressStr!
            log.atLevelDebug(source: SOURCE, object: nil, message: message)

        } else { // Should be IPv6

            var ipAddressString = [CChar](count:Int(INET6_ADDRSTRLEN), repeatedValue: 0)
            inet_ntop(
                info.memory.ai_family,
                info.memory.ai_addr,
                &ipAddressString,
                socklen_t(INET6_ADDRSTRLEN))
            let addressStr = String.fromCString(&ipAddressString)
            var message = addressStr == nil ? "No IPv6 address found" : "IPv6 address: " + addressStr!
            log.atLevelDebug(source: SOURCE, object: nil, message: message)
        }

        info = info.memory.ai_next
    }
}

// Get the socket descriptor, use the results from the getaddrinfo call earlier

var socketDescriptor = socket(servinfo.memory.ai_family, servinfo.memory.ai_socktype, servinfo.memory.ai_protocol);

log.atLevelDebug(source: SOURCE, object: nil, message: "Socket value: " + socketDescriptor.description)

// Since we will be listening, bind the socket descriptor to our socket

status = bind(socketDescriptor, servinfo.memory.ai_addr, servinfo.memory.ai_addrlen);

log.atLevelDebug(source: SOURCE, object: nil, message: "Status from binding: " + status.description)

// Cop out if there is an error

// PS: Need to handle "in use" errors

if status != 0 {
    let message = "Binding error, status = " + status.description + ", error = " + errno.description
    log.atLevelEmergency(source: SOURCE, object: nil, message: message)
    setApplicationError(ErrorCode.initPortForListeningBindingError, message)
    return
}

// Don't need this anymore

freeaddrinfo(servinfo)

(Remark: I am using println() for diagnostic output in this review instead of your
log.atLevelDebug(). The reason is that I wanted to test the code before posting
an answer.)

That seems to be working code (with one exception, see below), but there are some things that can be simplified
or improved.

nil is a possible value for any UnsafeMutablePointer<T>, so you can simplify your
initialization, for example:

var hints = addrinfo(
    ai_flags: AI_PASSIVE,       // Assign the address of my local host to the socket structures
    ai_family: AF_UNSPEC,       // Either IPv4 or IPv6
    ai_socktype: SOCK_STREAM,   // TCP
    ai_protocol: 0,
    ai_addrlen: 0,
    ai_canonname: nil,
    ai_addr: nil,
    ai_next: nil)

You traverse the address list from getaddrinfo with

var info = servinfo
while info != UnsafeMutablePointer<addrinfo>.null() {
    // ...
    info = info.memory.ai_next
}

Writing this as a for-loop might be easier to read:

for (var info = servinfo; info != nil; info = info.memory.ai_next) {
    // ...
}

Your printing of the numerical IP addresses with inet_ntop() does not work correctly,
the second argument must be the address of a struct in_addr/in6_addr
and not the address of struct sockaddr, for example:

var sin4addr = UnsafePointer<sockaddr_in>(info.memory.ai_addr).memory.sin_addr
var ipAddressString = [CChar](count:Int(INET_ADDRSTRLEN), repeatedValue: 0)
inet_ntop(
    info.memory.ai_family,
    &sin4addr,
    &ipAddressString,
    socklen_t(INET_ADDRSTRLEN))
let addressStr = String.fromCString(&ipAddressString)

But the more modern interface is getnameinfo() (which works for both IPv4 and IPv6
addresses). Written as a separate utility function:

func sockaddrDescription(addr: UnsafePointer<sockaddr>) -> String
{
    var host : String?
    var port : String?

    var hostBuffer = [CChar](count: Int(NI_MAXHOST), repeatedValue: 0)
    var servBuffer = [CChar](count: Int(NI_MAXSERV), repeatedValue: 0)

    if (getnameinfo(addr, socklen_t(addr.memory.sa_len),
                    &hostBuffer, socklen_t(hostBuffer.count),
                    &servBuffer, socklen_t(servBuffer.count),
                    NI_NUMERICHOST | NI_NUMERICSERV) == 0) {
            host = String.fromCString(hostBuffer)
            port = String.fromCString(servBuffer)
    }
    return "host: " + (host ?? "?") + " port: " + (port ?? "?")
}

which you could use as

for (var info = servinfo; info != nil; info = info.memory.ai_next) {
    let addrInfo = info.memory

    let desc = sockaddrDescription(addrInfo.ai_addr)
    println(desc)
}

Instead of errno you could log strerror(errno) if any of the socket functions fails.
The error message string would be more instructive than a error number. For example:

status = bind(...)
if status == -1 {
    println("bind failed: " + String(UTF8String: strerror(errno))!)
}

Your function prints all found IP addresses in debug mode, but binds the socket
only to the first one. The recommended practice (if I remember correctly, and I will
try to find a reference) is to

try all IP addresses until socket/bind/listen is successful, or
try to listen on all IP addresses.

is successful. In the first case, your main loop would look like

var socketDescriptor : Int32 = -1 // Invalid socket

for (var info = servinfo; info != nil; info = info.memory.ai_next) {
    let addrInfo = info.memory

    let sd = socket(addrInfo.ai_family, addrInfo.ai_socktype, addrInfo.ai_protocol)
    if sd == -1 {
        println("socket failed: " + String(UTF8String: strerror(errno))!)
    } else {
        status = bind(sd, addrInfo.ai_addr, addrInfo.ai_addrlen)
        if status == -1 {
            println("bind failed: " + String(UTF8String: strerror(errno))!)
        } else {
            status = listen(sd, 1)
            if status == -1 {
                println("listen failed: " + String(UTF8String: strerror(errno))!)
            } else {
                // SUCCESS !!
                socketDescriptor = sd
                break
            }
        }
        close(sd)
    }
}

and then socketDescriptor is -1 or a valid socket descriptor which is bound and
listening.

The more Swifty way however would be to define an optional

var socketDescriptor : Int32?

which is set to a valid socket descriptor or stays nil in the error case.


How do I refactor this basic form handler I've dabbled with PHP for a couple of years, I can generally get what I need done and I've been teaching myself PDO. The problem is I want to set it up with classes but i'm not really sure how to.

Below is my code, which takes data from a form, does some rough checking to see if the email is right and the fields are full. It works but it's gotten to a point where I can't take it further without breaking it or getting confused and i want to add things such as validation.

So what i'm asking for is can some show me how to create a basic connection class that outputs acouple of records from a database, this would help me a lot if anyone could.

<?php
try{
$host = "localhost";
$dbname = "db";
$user = "user";
$pass = "pass";

  $odb = new PDO("mysql:host=" . $host . ";dbname=" . $dbname, $user, $pass);
  $odb->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
  } catch (PDOException $e) {
echo "I'm afraid I can't do that.";
file_put_contents('PDOErrors.txt', $e->getMessage(), FILE_APPEND);
}

        if($_SERVER["REQUEST_METHOD"] == "POST"){
            if (isset($_POST['firstname']) && !empty($_POST['lastname']) && filter_var($_POST['email'],FILTER_VALIDATE_EMAIL) && !empty($_POST['phone'])){
                $firstname = $_POST['firstname'];
                $lastname = $_POST['lastname'];
                $email = $_POST['email'];
                $phone = $_POST['phone'];

                        $q = "INSERT INTO jobform(firstname, lastname, email, phone) VALUES (:firstname, :lastname, :email, :phone);";
                        $query = $odb->prepare($q);
                        $results = $query->execute(array(
                        ":firstname" => $firstname,
                        ":lastname" => $lastname,
                        ":email" => $email,
                        ":phone" => $phone,
                        ));
                } else {
            #echo $errMsg;
        }
     }
?>

I know you're asking for "a basic connection class", but you really need a few classes. Currently, your code has all of these (tightly-coupled) responsibilities:

Database access
Error reporting
Form validation
Data persistence
And I assume you're rendering output as well

To move past the difficulties you're facing, you need to separate these concerns into loosely-coupled objects. The prevailing architectural pattern to accomplish this is with the MVC pattern, but this is out of the scope of your question.

With this in mind, I would start with trying to isolate database access. To decouple your data from the rest, you need to put all of your database access in a separate class or set of classes. Here, there are two key patterns: object-relational mapping (ORM) and the database repository.

With ORM, you essentially try to hydrate database records into PHP objects – if you want to create a new record in the database, you create a PHP object and call a method to persist it to the database; if you want to update a record, you fetch a record, convert it to a PHP object (hydration), update the PHP object, then persist it; to delete, you either obtain a reference to a record or create a dummy PHP object, call a method to delete this record, then call another method to persist this change.

ORM has a lot of surface area; it typically relies on a database abstraction layer, requires you to write a new class for every type of record you'd like to persist, and can confuse beginners when you throw relations into the mix. While this definitely takes some investment, it provides a path to a lot of other benefits: code reuse, testing, automatic schema generation, migrations, validation, etc.

Another pattern for data persistence is the database repository. Here, all you're doing is wrapping your queries in methods and grouping these methods into classes based on which records the queries operate on. For instance, a UserRepository class might have the following methods: UserRepository::createUser(), UserRepository::getUserById(), UserRepository::getUsersByZipcode(), UserRepository::updateUser(), UserRepository::deleteUser(). Each one of these methods would accept a set of PHP parameters (arrays or ORM object and integers), perform database queries based on this data, and return the results (possibly hydrating them).

ORM and database repositories work very well together. I've personally used Doctrine 2's ORM and DBAL as well as Aura SQL's DBAL and would recommend both. I personally would start with creating a connection object with Aura SQL, then create your own class called UserRepository with the methods I listed above. Instantiate the UserRepository class by passing the AuraSQL connection object to it's constructor, then call UserRepository::createUser() to persist the user data from your form. After this, I would consider how to improve your form validation and look into using a template system like Twig.


What can I improve on my iOS PDF class? This is the first PDF class I have made for the iOS using code from Apple's samples.  There could easily be stuff here wrong that I am missing.

I intend this to get images out of for a layer, for example:

image.contents = (id)[self.test imageForPage: 3 size: CGSizeMake(4*dpc, 2*dpc)].CGImage;

Here is this hopefully decent code.  Any suggestions are welcome.

@interface PDFDocument : NSObject {
CGPDFDocumentRef pdfFile;
}

- (id) initWithURL: (NSURL*) url;
- (UIImage*) imageForPage: (size_t) pageno size: (CGSize) size;
- (CGSize) sizeOfPage: (size_t) pageno;

@end

@implementation PDFDocument

- (id) initWithURL: (NSURL*) url {
    if ([super init])
    {
        // Open PDF
        CGPDFDocumentRef doc = CGPDFDocumentCreateWithURL((CFURLRef)url);

        if (doc == NULL)
            @throw @"PDF File does not exist.";

        pdfFile = doc;

    }
    return self;
}

- (UIImage*) imageForPage: (size_t) pageno size: (CGSize) size {
    if (pdfFile) {
        // Get First Page
        CGPDFPageRef page = CGPDFDocumentGetPage(pdfFile, pageno);

        if (page == NULL)
            @throw @"Page does not exist.";

        // Get Page Size
        CGRect cropBox = CGPDFPageGetBoxRect(page, kCGPDFCropBox);

        // Start Drawing Context to Render PDF
        UIGraphicsBeginImageContext(size);

        CGContextRef context = UIGraphicsGetCurrentContext();
        CGContextSetRGBFillColor(context, 1.0, 1.0, 1.0, 1.0);

        CGRect u = { {0, 0}, size};

        CGContextFillRect(context, u);
        CGContextSetAllowsAntialiasing(context, NO);

        CGContextSaveGState(context);

        // Scale PDF
        CGContextScaleCTM(context, size.width / cropBox.size.width, size.height / cropBox.size.height);

        // Flip Context to render PDF correctly
        CGContextTranslateCTM(context, 0.0, cropBox.size.height);
        CGContextScaleCTM(context, 1.0, -1.0);

        CGContextDrawPDFPage(context, page);
        CGContextRestoreGState(context);

        UIImage *img = UIGraphicsGetImageFromCurrentImageContext(); // must retain?

        UIGraphicsEndImageContext();

        return img;

    }
    return nil;
}

- (CGSize) sizeOfPage: (size_t) pageno {
    if (pdfFile) {
        CGPDFPageRef page = CGPDFDocumentGetPage(pdfFile, pageno);

        if (page == NULL)
            @throw @"Page does not exist";

        CGRect cropBox = CGPDFPageGetBoxRect(page, kCGPDFCropBox);

        return cropBox.size;
    }

    @throw @"No document loaded.";
}

@end

One immediate change I'd make is how you're handling errors.  While try-catch blocks are extremely common in other programming languages, I actually don't see them all that very often in Objective-C.

That doesn't mean we won't have errors.  It just means error handling is typically handled differently in my experience with Objective-C.  Just take a look at some of the Foundation methods as example:

stringWithContentsOfFile:encoding:error:

You can send nil as the argument for the error, but otherwise, you send the method an NSError object, and when the method returns, if there was an error, you can check the object to find out what it was.  In the case of an init method, if an error occurred, you'd also want to return nil as well as loading the error description in the passed error object.

For completeness, it seems like it might be a good idea to create a corresponding PDFPage class.  And instead of imageForPage:size: being called over and over to create pages, the end user would create instances of PDFPage.  The PDFDocument in turn isn't much more than an array of PDFPages with some logic for adding a single page, array of pages, remove pages, from front, back, specific index, etc.


Placement of success code in a conditional I'm not sure if there's a standard view where the placement of a success result should be in a conditional that can return multiple statuses. The success condition of this function is in the middle of the conditional block:

def redeem(pin)
  success = false
  message = nil
  if !self.date_claimed.nil?
    message = 'Reward already claimed'
  elsif self.reward.redemption_pin == pin
    success = true
    self.date_claimed = Time.zone.now
    self.save
  else
    message = "Wrong pin"
  end
  return {:success => success, :message => message}
end

I could rewrite it to be at the end:

def redeem(pin)
  success = false
  message = nil
  if !self.date_claimed.nil?
    message = 'Reward already claimed'
  elsif self.reward.redemption_pin != pin
    message = "Wrong pin"
  else
    success = true
    self.date_claimed = Time.zone.now
    self.save
  end
  return {:success => success, :message => message}
end

or I could rewrite it to be at the top but conditions become slightly more complex:

def redeem(pin)
  success = false
  message = nil
  if self.reward.redemption_pin == pin && self.date_claimed.nil?
    success = true
    self.date_claimed = Time.zone.now
    self.save
  elsif date_claimed
    message = 'Reward already claimed'
  else
    message = "Wrong pin"
  end
  return {:success => success, :message => message}
end

I personally think I prefer it to be at the beginning or the end, but in this function the end seems to be the best place.

Some notes:

Imperative programming has its use cases (or so I've heard :-)) but implementing logic is definitely not one of them. Some thoughts on the matter. Use always expressions, not statements, to describe logic; that's it, don't begin with a x = 1 and then modify x somewhere else in the code. Write expression branches instead (conditionals are in expressions in Ruby). Don't think in terms of "how" but in terms of "what".
if !x.nil? -> if x.
obj.save may fail but you are not checking it.
Don't write explicit return (more on idiomatic Ruby).
As you say the order of checks is important. I usually check for "problems" first, and keep the "ok" scenario for the last branch.
You are using a value (a hash) instead of an exception to signal errors. It's slightly not idiomatical in Ruby (in the sense that people tend not to do it, not that there's anything wrong with it), but personally I like it (and use it).

You could write:

def redeem(pin)
  success, message = if date_claimed
    [false, 'Reward already claimed']
    ...
  end
  {:success => success, :message => message}
end

However, returning {success: ..., message: ...} on each branch, yet slightly more verbose, looks pretty nice and declarative:

def redeem(pin)
  if date_claimed
    {success: false, message: "Reward already claimed"}
  elsif reward.redemption_pin != pin
    {success: false, message: "Wrong pin"}
  else
    update_attribute(:date_claimed, Time.zone.now)
    {success: true}
  end
end


(Codewars) Range Extraction Kata: https://www.codewars.com/kata/range-extraction/python

  A format for expressing an ordered list of integers is to use a comma separated list of either

  individual integers
  or a range of integers denoted by the starting integer separated from the end integer in the range by a dash, '-'. The range includes all integers in the interval including both endpoints. It is not considered a range unless it spans at least 3 numbers. For example ("12, 13, 15-17")

  Complete the solution so that it takes a list of integers in increasing order and returns a correctly formatted string in the range format.

  Example:

solution([-6, -3, -2, -1, 0, 1, 3, 4, 5, 7, 8, 9, 10, 11, 14, 15, 17, 18, 19, 20])
 # returns "-6,-3-1,3-5,7-11,14,15,17-20"

  Courtesy of rosettacode.org

My Code

def solution(lst):
    res = []
    if lst:
        tmp, i, ln = lst[0], 0, len(lst)
        while i < ln:
            tmp, j = lst[i], i
            while j < ln - 1 and lst[j+1] == lst[j]+1:
                j += 1
            if j - i > 1:
                tmp = str(lst[i]) + "-" + str(lst[j])
                i = j+1
            else:
                i = (j if j > i else i+1)
            res.append(tmp)
    return ",".join(str(x) for x in res)

Grouping

If you need to group anything using; groupby is probably the easiest way

Here we can use a itertools.groupby recipe to group the consecutive ranges:

>>> for _, g in groupby(enumerate(lst), lambda i_x : i_x[0] - i_x[1]):
...     print([x for _, x in g])
[-6]
[-3, -2, -1, 0, 1]
[3, 4, 5]
[7, 8, 9, 10, 11]
[14, 15]
[17, 18, 19, 20]

Yielding the correct representation

Now all is left is to check whether there are more then 2 items in the range, and return the range. Else return all the numbers in the range normally

We can make this a generator, which yields either the range or a number, and afterwards join them together.

Full Code

def group_consecutives(lst):
    for _, g in groupby(enumerate(lst), lambda i_x : i_x[0] - i_x[1]):
        r = [x for _, x in g]
        if len(r) > 2:
            yield f"{r[0]}-{r[-1]}"
        else:
            yield from map(str, r)

def range_extraction(lst):
    return ','.join(group_consecutives(lst))


Medical database design and queries optimization I have created a MySQL database which will be used in a medical app, but I am pretty sure the design and the used queries can be improved, since I am new in database design. My biggest doubt is the s_apply column in combination table. The user should choose 1 main symptom (s_id). Main symptoms may have 0, 1 or 2 symptoms that apply (s_apply). I know it is bad practice to store the full names in s_apply, but I don't know how to improve this. I really need a way to store the symptoms that apply (s_apply) more efficient.

The d_weight in disease indicates if a disease in that context is either common (d_weight <= 5) or less common (d_weight > 5).

Combination table has 2 foreign keys: s_id and d_id.

To get all symptoms of a combination of age, gender and bodypart, I use the following query. There should be a better way to do this:

SELECT DISTINCT symptom.s_id, s_name, s_common FROM symptom, combination WHERE combination.age = '$age' AND combination.gender = '$gender' AND combination.bp_id = '$bodypart' AND combination.s_id = symptom.s_id

To get all diseases of a combination of age, gender, bodypart, s_id and symptoms that apply (s_apply), I use the following query:

SELECT DISTINCT disease.d_id, d_name, d_weight FROM disease, combination, symptom WHERE combination.age = '$age' AND combination.gender = '$gender' AND combination.bp_id = '$bodypart' AND combination.s_id = symptom.s_id AND combination.s_id = '$s_id'

Symptom table

Disease table

Combination table

The Symptom and Disease tables are well-conceived.
Their purpose is clear, and they are about specific thing each.

The Combination table on the other hand stinks.
Combination of what?
It seems your goal with this table is to represent:

One record per case, maybe?
Age, gender, body part, weight, disease, main symptom of the case
Additional symptoms

Instead of putting the addition symptoms as a concatenated list of values,
it would be better to create another table with:

foreign key on case id, and symptom id
unique constraint on case id + symptom id

Also,
the prefixing of column names with s_ for symptom and d_ for disease is not great. It would be better to drop these prefixes.
You can use table aliases s and d instead, for example:

SELECT DISTINCT s.id, s.name, s.common FROM symptom s, combination c
WHERE c.age = '$age' AND c.gender = '$gender' AND c.bp_id = '$bodypart' AND c.s_id = s.id


Assigning names to unnamed labels I'm working on a compiler. During the final stage I will get a collection of Lines.

This is the method that takes the Lines and gives name to Labels that don't have it yet.

Lines can have Labels. When a Line doesn't have a Label, its Label property is set to null. Apart from this, Labels can be unnamed (Label.Name is null)

This is the method. It looks ugly and I'm using a nested method. Please, review it and try to improve it to be elegant. Performance isn't a requirment (at least, for now):

private static void GiveNameToUnnamedLabels(IEnumerable<Line> finalCode)
{
    int count = 0;

    void GiveName(Model.Label label)
    {
        label.Name = $"dyn_label{++count}";
    }

    var unnamed = finalCode.Where(x => x.Label != null && x.Label.Name == null).ToList();
    unnamed.ForEach(x => GiveName(x.Label));
}

A few notes about your code:

Both the inner and outer method are fairly small, and the name of the outer method is already aptly descriptive, so I see no need for a named inner method here. An anonymous method would suffice.
List<T>.ForEach is a bit of an odd duck, in my opinion. It looks like functional programming (similar to Linq), but it's actually used to produce side-effects. Combining the two is giving off conflicting signals. A for or foreach loop, on the other hand, is clearly not functional-style, so side-effects would not be surprising.
I agree with keeping anonymous method parameter names short, but x isn't very descriptive. line is a little longer but much more descriptive.

Also, I find it more readable to put chained Linq calls on a line of their own:

var unnamed = finalCode
    .Where(x => x.Label != null && x.Label.Name == null)
    .ToList();

Having that said, I would go for a plain old foreach loop. It may not be as fancy, but it's simple and to the point, which should make it easier to understand:

int count = 0;
foreach (var line in finalCode)
{
    if (line.Label != null && line.Label.Name == null)
    {
        line.Label.Name = $"dyn_label{++count}";
    }
}


LeetCode 200: Number of Islands I'm posting my code for a LeetCode problem. If you'd like to review, please do so. Thank you for your time!
Problem

Given a 2d grid map of '1's (land) and '0's (water), count the number
of islands. An island is surrounded by water and is formed by
connecting adjacent lands horizontally or vertically. You may assume
all four edges of the grid are all surrounded by water.
Example 1:
Input:
11110
11010
11000
00000

Output: 1

Example 2:
Input:
11000
11000
00100
00011

Output: 3

Code
#include <vector>

class Solution {
public:
    std::size_t numIslands(std::vector<std::vector<char>>& grid) {
        std::size_t row_length = grid.size();

        if (!row_length) {
            return 0;
        }

        std::size_t col_length = grid[0].size();
        std::size_t islands = 0;

        // Runs depth first search for every cell
        for (std::size_t row = 0; row < row_length; row++) {
            for (std::size_t col = 0; col < col_length; col++) {
                if (grid[row][col] == '1') {
                    islands++;
                    depth_first_search(grid, row, col, row_length, col_length);
                }
            }
        }

        return islands;
    }

private:
    void depth_first_search(std::vector<std::vector<char>>& grid, const std::size_t row, const std::size_t col, const std::size_t row_length, const std::size_t col_length) {
        // Checks for grid boundaries and '0' cells
        if (row < 0 || row >= row_length || col < 0 || col >= col_length || grid[row][col] == '0') {
            return;
        }

        grid[row][col] = '0';
        // Recurse in 4 directions of grid
        depth_first_search(grid, row + 1, col, row_length, col_length);
        depth_first_search(grid, row - 1, col, row_length, col_length);
        depth_first_search(grid, row, col + 1, row_length, col_length);
        depth_first_search(grid, row, col - 1, row_length, col_length);
    }
};

LeetCode's Solution (Not for review)
class Solution {
private:
  void dfs(vector<vector<char>>& grid, int r, int c) {
    int nr = grid.size();
    int nc = grid[0].size();

    grid[r][c] = '0';
    if (r - 1 >= 0 && grid[r-1][c] == '1') dfs(grid, r - 1, c);
    if (r + 1 < nr && grid[r+1][c] == '1') dfs(grid, r + 1, c);
    if (c - 1 >= 0 && grid[r][c-1] == '1') dfs(grid, r, c - 1);
    if (c + 1 < nc && grid[r][c+1] == '1') dfs(grid, r, c + 1);
  }

public:
  int numIslands(vector<vector<char>>& grid) {
    int nr = grid.size();
    if (!nr) return 0;
    int nc = grid[0].size();

    int num_islands = 0;
    for (int r = 0; r < nr; ++r) {
      for (int c = 0; c < nc; ++c) {
        if (grid[r][c] == '1') {
          ++num_islands;
          dfs(grid, r, c);
        }
      }
    }

    return num_islands;
  }
};

Reference
On LeetCode, there is a class usually named Solution with one or more public functions which we are not allowed to rename.

Problem

Discuss

Offload more functionality into depth_first_search()
One improvement your solution has over LeetCode's is that you move the grid boundary check into depth_first_search(). However, you can do more, by having it return whether you are starting on an island or in the ocean:
bool depth_first_search(...) {
    // Checks for grid boundaries and '0' cells
    if (...)
        return false;

    // We are on an island, recurse
    ...

    return true;
}

Then, the loop inside numIslands() can be simplified:
for (std::size_t row = 0; row < row_length; row++)
    for (std::size_t col = 0; col < col_length; col++)
        islands += depth_first_search(grid, row, col, row_length, col_length);

Naming things
While depth_first_search() does iterate in a depth-first fashion, it is not search for anything. Rather, it is implementing a flood fill algorithm. But here we use it to remove an island. If it also performs a check to see if there is an island at the given position, I would name it check_and_remove_island(). As Emily L. mentions, a function name that reads like foo_and_bar() is usually a hint that it should be split into a foo() and bar(). If you want to go that way, I suggest you make it so you can write the following code:
if (is_island(grid, row, col)) {
    islands++;
    remove_island(grid, row, col);
}

Get rid of the size variables
I don't think it's that useful to store the sizes into separate variables, and to pass them explicitly to depth_first_search(). This information can always easily be gotten from the variable grid. This also gets rid of the check for an empty grid:
std::size_t numIslands(std::vector<std::vector<char>>& grid) {
    size_t islands{};

    for(std::size_t row = 0; row < grid.size(); row++)
        for (std::size_t col = 0; col < grid[row].size(); col++)
            islands += check_and_remove_island(grid, row, col);

    return islands;
}

You have to modify depth_first_search() to match of course.


Questionnaire program using many wxPython windows I have over 300 questions that I plan to include in the program. The flow is pretty much like this:

Create a window with the question
Store answer in variable
Create NEW window with question
Store NEW answer

(This continues on for over 300 questions.)

I have 2 questions:

Will this eventually lead to a crash since I'm creating so many windows?
Everything works with this code if you select 'Yes' to the second question (A2) but it does not work if you select 'No'. Can you please see if you can find what's wrong with it?

import wx

a1 = ['Apples', 'Bananas', 'Strawberries', 'Watermelon',
     "Don't remember", 'None of the above']

a2 = ['No', 'Yes']

a4 = ['No', 'Yes']

class Fruit(wx.Frame):

    def __init__(self, parent, id):
        wx.Frame.__init__(self, parent, id, 'Fruit', size=(300,200))

        #create panel and button
        panel = wx.Panel(self)

        # B1 - create multiple choice list
        box = wx.MultiChoiceDialog(None, """

A1.    What kind of fruit did you buy at the store?""", 'Fruit', a1)
        if box.ShowModal() == wx.ID_OK:
            a_1 = box.GetSelections()

        print (a_1, '\n')

        # A2 - create single choice list
        box = wx.SingleChoiceDialog(None, """
A2.    Do you like eating fruit?
""", 'Fruit', a2)
        if box.ShowModal() == wx.ID_OK:
            a_2 = box.GetStringSelection()

        print (a_2, '\n')

        if a_2 == 'Yes':
            box = wx.TextEntryDialog(None, "A3.    What kind of fruit is your favorite? ", "Fruit", "")
        if box.ShowModal() == wx.ID_OK:
            a_3 = box.GetValue()

        print (a_3, '\n')

        box = wx.SingleChoiceDialog(None, """
A4.    Did you eat the fruit that you bought?
""", 'Fruit', a4)
        if box.ShowModal() == wx.ID_OK:
            a_4 = box.GetStringSelection()

        print (a_4, '\n')

import wx

a1 = ['Apples', 'Bananas', 'Strawberries', 'Watermelon',
     "Don't remember", 'None of the above']

a2 = ['No', 'Yes']

a4 = ['No', 'Yes']

The Python style guide recommend ALL_CAPS for global constants.

class Fruit(wx.Frame):

    def __init__(self, parent, id):
        wx.Frame.__init__(self, parent, id, 'Fruit', size=(300,200))

        #create panel and button
        panel = wx.Panel(self)

        # B1 - create multiple choice list
        box = wx.MultiChoiceDialog(None, """

A1.    What kind of fruit did you buy at the store?""", 'Fruit', a1)

When strings get long enough that they force breaking across multiple lines, its usually best to move them to global constants

        if box.ShowModal() == wx.ID_OK:
            a_1 = box.GetSelections()

        print (a_1, '\n')

I don't think this does what you think it does. For python 2.x (which unless wxPython has been ported to python 3.x recently, must be what you are using), you shouldn't put parenthesis around your print statements.

            # A2 - create single choice list
        box = wx.SingleChoiceDialog(None, """
A2.    Do you like eating fruit?
""", 'Fruit', a2)
        if box.ShowModal() == wx.ID_OK:
            a_2 = box.GetStringSelection()

        print (a_2, '\n')

        if a_2 == 'Yes':
            box = wx.TextEntryDialog(None, "A3.    What kind of fruit is your favorite? ", "Fruit", "")
        if box.ShowModal() == wx.ID_OK:

If the answer to the previous question was "Yes", then box is now the result of asking question A3. However, otherwise its the still the previous question. As a result, it'll ask the same question again. You probably want to indent this if block so that it only happens if a_2 was Yes.

            a_3 = box.GetValue()

        print (a_3, '\n')

        box = wx.SingleChoiceDialog(None, """
A4.    Did you eat the fruit that you bought?
""", 'Fruit', a4)
        if box.ShowModal() == wx.ID_OK:
            a_4 = box.GetStringSelection()

        print (a_4, '\n')

As for your questions:

You can probably get away with creating that many windows. However, you can make sure that a dialog gets destroyed by calling its Destroy() method.
I explained above what was wrong with your logic.


JavaScript copy-to-clipboard class I am have made a small copy-to-clipboard script.
I have made a method called copyToClipboard that does the following:

create an input box;
read the content of an HTML element;
set the input box value to the content previously read;
append the input box to a container;
select and copy its value to the clipboard;
remove the input box (no longer useful);

class ActivationCodeComponent{
  constructor() {
    this.copyButton = document.querySelector('#copy_button');
  }

  copyToClipboard () {
    const el = document.createElement('input');
    const codeBox = document.querySelector('#box');
    const activationCodeContainer = document.querySelector('#activation_code');
    el.value = activationCodeContainer.textContent;
    el.setAttribute('readonly', '');
    codeBox.appendChild(el);
    el.select();
    document.execCommand('copy');
    codeBox.removeChild(el);
  }

  handleClick() {
    this.copyButton.addEventListener("click", this.copyToClipboard);
  }

  init() {
    this.handleClick();
  }
}
const activationCodeComponent = new ActivationCodeComponent();
activationCodeComponent.init();
.container  {
   text-align: center;
}

#box  {
  border: 1px solid #ccc;
  width: 200px;
  margin: 20px auto;
  padding: 0 10px;
}

.code {
  letter-spacing: 2px;
  color: #069;
  font-size: 16px;
  font-weight: bold;
}
<link href="https://maxcdn.bootstrapcdn.com/bootstrap/4.5.2/css/bootstrap.min.css" rel="stylesheet"/>

Your activation code

B12A7

  <button id="copy_button" class="btn btn-sm btn-success text-uppercase">Copy code</button>

The problem
The validity of the script is too dependent on a set of HTML elements with the IDs I have used.
Could it be made more flexible?

Style
I can not fault the style and layout.
The need to call init which is an indirect call to handleClick seams redundant. Do it in the constructor.
I am puzzled as to why you set readonly to '' (el.setAttribute('readonly', '');) What has this to do with copying to the clipboard.
Names
Naming is very poor and also you use inconsistent naming conventions across JS (camelCase) and HTML (ids are all snake_case)
Names should provide the information needed to understand its behavior and content.

ActivationCodeComponent is a bit obscure giving no indication of what it does.

.handleClick Does not handle click (.copyToClipboard does that) .handleClick adds a click listener

el Too short and obscure. Maybe inputEl

codeBox What??? Maybe inputContainer

activationCodeContainer is not containing anything. Maybe copyTextSrcEl or textSourceElement

Basic rewrite
Rewrite using document.execCommand
Note that the rewrite does not return a use-able object thus no need to store it.
class ActivationCodeComponent{
    constructor() {
        function copyToClipboard () {
            inputEl.value = textSrcEl.textContent;
            inputContainer.appendChild(inputEl);
            inputEl.select();
            document.execCommand('copy');
            inputContainer.removeChild(inputEl);
        }
        const inputEl = document.createElement('input');
        const inputContainer = document.querySelector('#box');
        const textSrcEl = document.querySelector('#activation_code');
        document.querySelector('#copy_button').addEventListener("click", copyToClipboard);
    }
}
new ActivationCodeComponent();

or
activationCodeComponent();
function activationCodeComponent() {
    function copyToClipboard () {
        inputEl.value = textSrcEl.textContent;
        inputContainer.appendChild(inputEl);
        inputEl.select();
        document.execCommand('copy');
        inputContainer.removeChild(inputEl);
    }
    const inputEl = document.createElement('input');
    const inputContainer = document.querySelector('#box');
    const textSrcEl = document.querySelector('#activation_code');
    document.querySelector('#copy_button').addEventListener("click", copyToClipboard);
}

Problems
Deprecated
Document.execCommand is a depreciated function and should NOT be used.
Fix
You should be using Clipboard.writeText to add content to the clipboard. Note that the Code Review rewrite will not have clipboard permission and thus will fail.
Instantiatable and State
The best way to avoid bugs is to make impossible to incorrectly use the code.
Never allow conflicting or overlapping behaviors. As the code is, there are dozens of ways for it to be used in a manner that will create unpredictable behaviors.

There is nothing to prevent the object ActivationCodeComponent being created more than once.

The created object's state is exposed thus making all of its behavior un-trustable.

Fix
There is no need to create an instantiatable object.

Use a function's scope to contain the behavior and protect the state.
Ensure that the function can only be called once.

User feedback
There is no user feedback to indicate if the button click has done anything. Always give feedback to all actions no matter how trivial.
Fix
The rewrite adds a simple message element that displays the result of the copy action. The message is cleared after 3 seconds.
Question
You write

"The validity of the script is too dependent on a set of HTML elements with the IDs I have used."

"Could it be made more flexible?"

There are many ways to make the code more flexible.

Pass the code the elements required
Use Element's Element.dataset to mark objects

Example marks the copy button with the query needed to locate the text source.

 const copyBtn = document.querySelector("[data-copy-src]");
 const copySrcEl = document.querySelector(copyBtn.dataset.copyButton);

B12A7

<button id="copyButton" data-copy-src="#copySrcEl">Copy code</button>

Rewrite
Example moves element queries out of the function. To use you must pass all the elements required for the function to work.
The rewrite performs the following

Marks the function to ensure that the code runs only once.
Checks all elements to make sure that they are instanceof Element.

const isElement = obj => obj instanceof Element;
const query = (qStr, root = document) => root.querySelector(qStr);
const queries = (qObj, root) => (
    Object.entries(qObj).forEach(([key, qStr]) => qObj[key] = query(qStr, root)),
    qObj
)
const copyElements = queries({
    button: "#copyButton",
    textSrcEl: "#activationCodeEl",
    messageEl: "#copyMessageEl",
});

copyToClipboard(copyElements);

function copyToClipboard({button, textSrcEl, messageEl}) {
    const okMessage = "Text copied to system copy buffer";
    const failMessage = "Text failed to copy";
    const okTypeClass = "copyOK", failTypeClass = "copyFail";
    var messageClearHdl;

    if (!isElement(button) || !isElement(textSrcEl) || !isElement(messageEl)) { return }
    if (copyToClipboard.active) { return }
    copyToClipboard = Object.freeze(
        Object.assign(copyToClipboard, { active: Symbol.for("activeCopy") })
    );

    function showMessage(text, typeClass) {
        clearTimeout(messageClearHdl);
        messageEl.textContent = text;
        messageEl.className = typeClass;
        messageClearHdl = setTimeout(() => {
                messageEl.textContent = "";
                messageEl.className = "hide";
            }, 3000);
    }
    button.addEventListener("click", () =>
        navigator.clipboard.writeText(textSrcEl.textContent).then(
            () => showMessage(okMessage, okTypeClass),
            () => showMessage(failMessage, failTypeClass)
        ));
}
.container  {
   text-align: center;
}

#box  {
  border: 1px solid #ccc;
  width: 200px;
  margin: 20px auto;
  padding: 0 10px;
}

.code {
  letter-spacing: 2px;
  color: #069;
  font-size: 16px;
  font-weight: bold;
}
.hide {
    display: none;
}
.copyOK {
    margin-top: -18px;
    font-family: arial;
    display: block;
}
.copyFail {
    margin-top: -18px;
    color: red;
    font-family: arial;
    display: block;
}

Your activation code

B12A7

  <button id="copyButton" class="btn btn-sm btn-success text-uppercase">Copy code</button>

Of course many will think that the context security is over the top. Apart from guaranteeing the code will behave as expected there is no need to include the context security.

const query = (qStr, root = document) => root.querySelector(qStr);
const queries = (qObj, root) => (
Object.entries(qObj).forEach(([key, qStr]) => qObj[key] = query(qStr, root)), qObj);

// example only so not making call
//copyToClipboard(queries({
//button: "#copyButton", textSrcEl: "#activationCodeEl", messageEl: "#copyMessageEl",
//}));

function copyToClipboard({button, textSrcEl, messageEl}) {
const okMessage = "Text copied to system copy buffer";
const failMessage = "Text failed to copy";
const okTypeClass = "copyOK", failTypeClass = "copyFail";
var messageClearHdl;

function showMessage(text, typeClass) {
    clearTimeout(messageClearHdl);
    messageEl.textContent = text;
    messageEl.className = typeClass;
    messageClearHdl = setTimeout(() => {
            messageEl.textContent = "";
            messageEl.className = "hide";
        }, 3000
    );
}
button.addEventListener("click", () => {
    navigator.clipboard.writeText(textSrcEl.textContent).then(
        () => showMessage(okMessage, okTypeClass),
        () => showMessage(failMessage, failTypeClass)
    );
});
}


Pascal Triangle in Haskell The code below generates a list of Pascal Coefficients.
e.g pascalList 3 outputs [[1], [1,1], [1,2,1], [1,3,3,1]
Can we write below sample in more idiomatic way

pascalList 0 = [[1]]
pascalList 1 = [[1], [1, 1]]
pascalList n = let pList = pascalList (n-1)
               in pList ++ [([1] ++ pascalCoeff (last pList) ++ [1])]
  where pascalCoeff (x:y:ys) = (x+y) : pascalCoeff (y:ys)
        pascalCoeff (x:[]) = []

Following code prints above list

listtoString :: [Int] -> String
listtoString [] = []
listtoString [x] = show x
listtoString (x:xs) = show x ++ " " ++ listtoString xs

pascalTriangle :: Int -> IO ()
pascalTriangle n = mapM_ putStrLn (((justify n) . map listtoString) (pascalList n))

justify :: Int -> [String] -> [String]
justify n (x:xs) = (concat (replicate n " ") ++ x) : justify (n-1) xs
justify _ [] = []

The sample output of the pascalTriangle 4 will be

    1
   1 1
  1 2 1
 1 3 3 1
1 4 6 4 1

I don't know if this is more idiomatic, but ever since I've seen an infinite list construction for the Fibonacci numbers, I've been in love with it, so here goes nothing.

pascalTriangle :: [[Integer]]
pascalTriangle = [1] : map newRow pascalTriangle
    where newRow y = 1 : zipWith (+) y (tail y) ++ [1]

To understand this, perhaps first you should look at simpler examples, e.g. an infinite list of zeros,

zeros = 0 : zeros

or the list of natural numbers,

nats = 0 : map (+1) nats

or my favourite, the list of fibonacci numbers,

fibs = 1 : 1 : zipWith (+) fibs (tail fibs)

That being said...

The line

pascalList 1 = [[1], [1, 1]]

is unnecessary, because you've already specified pascalList 0. The line

[([1] ++ pascalCoeff (last pList) ++ [1])]

is equivalent to

[[1] ++ pascalCoeff (last pList) ++ [1]]

which is equivalent to

[ 1 : pascalCoeff (last pList) ++ [1]]

Whenever possible, I try to use functions from the standard library instead of rolling my own using recursion, so for example instead of

pascalCoeff (x:y:ys) = (x+y) : pascalCoeff (y:ys)
pascalCoeff (x:[]) = []

I'd say that this function takes a list e.g. [1,2,3,4] and does the following:

  [1, 2, 3]
  [2, 3, 4]
+ ---------
  [3, 5, 7]

so it takes the tail (all but the first element) of the list, and the init (all but the last element) of the list, and adds them together element-wise.

There are built-in functions tail, and init that yield you these parts from a list, and luckily the function

zipWith (+)

does the adding part, so you can simply say

pascalCoeff y = zipWith (+) (init y) (tail y)

which is, due to the way zipWith works, the same as

pascalCoeff y = zipWith (+) y (tail y)

Similarly,

listtoString [] = []
listtoString [x] = show x
listtoString (x:xs) = show x ++ " " ++ listtoString xs

could simply be

listToString = unwords . map show

And

justify :: Int -> [String] -> [String]
justify n (x:xs) = (concat (replicate n " ") ++ x) : justify (n-1) xs
justify _ [] = []

could be

justify n ss = zipWith (++) padding ss
    where padding = [ replicate k ' ' | k <- [n, n-1 .. 1]]

or equivalently, after eta-reduction

justify n = zipWith (++) padding
    where padding = [ replicate k ' ' | k <- [n, n-1 .. 1]]

Due to the fact that lists are linked lists, appending to a list is expensive, while prepending to it is cheap. So if you do decide to construct a list element-by-element, then I'd say you should prepend the new elements, and perhaps after the list is built, use a reverse.

By the same logic, head is cheap, last is expensive.

Here's a possible implementation that uses what I've just said.

pascalList = reverse . pascalList'
pascalList' 0 = [[1]]
pascalList' n = new : old
    where new = 1 : pascalCoeff (head old) ++ [1]
          old = pascalList' (n-1)

In my opinion it is a good idea to

separate pure and impure code,
keep your code as modular as possible,
write type signatures,
use a linter like hlint,
use functions from the standard library instead of writing explicit recursions.

Keeping this in mind, here's how I'd write the printing part.

listToString :: (Show a) => [a] -> String
listToString = unwords . map show

leftPadStrings :: [String] -> [String]
leftPadStrings ss = map leftPad ss
    where maxlen = maximum $ map length ss
          leftPad s = replicate (div (maxlen - length s) 2) ' ' ++ s

paddedPascalTriangle :: Int -> [String]
paddedPascalTriangle n = leftPadStrings
                       . map listToString
                       . take n
                       $ pascalTriangle

printPascalTriangle :: Int -> IO ()
printPascalTriangle = mapM_ putStrLn . paddedPascalTriangle

Running this, you should get something like the following:

*Main> printPascalTriangle 10
             1
            1 1
           1 2 1
          1 3 3 1
         1 4 6 4 1
       1 5 10 10 5 1
     1 6 15 20 15 6 1
    1 7 21 35 35 21 7 1
  1 8 28 56 70 56 28 8 1
1 9 36 84 126 126 84 36 9 1


Where to protect DB data against XSS injections? To protect my web site against XSS injection, I am using OWASP Encoding Project.

Is it a good idea to protect all data retrieved in my DB to avoid XSS by going throuh all the data just returned by the query like this :

public function fetchDataBySql($sql,$dataArray = null,$htmlEncode = true)
{
    try
    {
        $db = Zend_Db_Table::getDefaultAdapter();
        $stmt = $db->query($sql,$dataArray);

        $return = $stmt->fetchAll();
        $this->numberRows = count($return);

        if($htmlEncode)
        {
            array_walk_recursive($return, function (&$value) {
                $value = Reform::HtmlEncode($value);
            });
        }

        return $return;
    }
    catch (Exception $e)
    {
        echo $e->getMessage();
        Application_Model_DbTable_Default::logError(300,$e->getMessage());
        return false;
    }
}

and then in my view :

<?php echo $this->myData; ?>

instead of escape strings in the view like this :

<?php echo Reform::HtmlEncode($this->myData); ?>

Is there any case where the first solution is not secure?

One might forget HTML encoding, but how about doing it automatically:

class HtmlEncode {
    private $obj;

    public __construct($obj) {
        $thos->obj = $obj;
    }

    public __call($method, $args) {
       $ret = call_user_func_array($this->obj, $args);
       if(is_scalar($ret)) return Reform::HtmlEncode($ret);
       return new HtmlEncode($ret);
    }

    public __get($name) {
       if(is_scalar($obj->$name)) return Reform::HtmlEncode($obj->$name);
       return new HtmlEncode($obj->$name);
    }
}

Create $data = new HtmlEncode($this) and now you can safely use:

<?php echo $data->myData; ?>

<?php echo $this->myMethod()->name; ?>

You always get either html encoded string or object wrapped in HtmlEncode class.

I wrote it out of my head, so it's untested, it should check if the method or variable is defined, handle arrays, but it's left up to you ;)


Laravel - Repetitive code in store and update functions I am working on a system and I can see myself using repetitive code that does not look right. I want to clean up my code but I don't really know many ways to do so. I have 2 methods (store and update). They both look like this

store

public function store(Request $request)
    {
        $user = Auth::user();

        $validatedData = $request->validate([
            'street' => ['required', 'string'],
            'number' => ['required', 'string'],
            'city' => ['required', 'string'],
            'state' => ['required', 'string'],
            'postal_code' => ['required', 'string'],
            'country' => ['required', 'string'],
            'phone' => ['required', 'string']
        ]);

        $addresses = Address::all();

        $billing = 0;
        if($request->is_billing) {
            $billing = $request->is_billing;
            foreach($addresses as $address) {
                if($address->is_billing == 1) {
                    $address->is_billing = 0;
                    $address->save();
                }
            }
        }

        $address = Address::create([
            'user_id' => $user->id,
            'token' => Str::random(32),
            'street_name' => $request->street,
            'house_number' => $request->number,
            'postal_code' => $request->postal_code,
            'state' => $request->state,
            'city' => $request->city,
            'country_id' => $request->country,
            'phone' => $request->phone,
            'is_billing' => $billing
        ]);

        return redirect('/dashboard/user/' . $user->user_token . '/addresses');
    }

update

public function update(Request $request, $id)
    {
        $addresses = Address::all();
        $address = Address::where('token', $id)->firstOrFail();
        $user = Auth::user();

        $billing = 0;
        if($request->is_billing) {
            $billing = $request->is_billing;
            foreach($addresses as $item) {
                if($item->is_billing == 1) {
                    $item->is_billing = 0;
                    $item->save();
                }
            }
        }

        $address->user_id = $user->id;
        $address->street_name = $request->street;
        $address->house_number = $request->number;
        $address->postal_code = $request->postal_code;
        $address->state = $request->state;
        $address->city = $request->city;
        $address->country_id = $request->country;
        $address->phone = $request->phone;
        $address->is_billing = $billing;

        $address->save();

        return redirect('/dashboard/user/' . $user->user_token . '/addresses');
    }

Currently, the code looks messy and I have a feeling it can be done much more efficiently. Can someone give me tips on how to clean this up?

No need to get all data from database. Instead, only update those rows that need to be updated. Only if needed, you should work with database, avoid otherwise. Database I/O is biggest speed consumer in web applications and generally in PHP applications [when used]. https://laravel.com/docs/master/eloquent#updates

Check this one for store method, should work way faster:

public function store(Request $request)
{
    $user = Auth::user();

    $validatedData = $request->validate([
        'street' => ['required', 'string'],
        'number' => ['required', 'string'],
        'city' => ['required', 'string'],
        'state' => ['required', 'string'],
        'postal_code' => ['required', 'string'],
        'country' => ['required', 'string'],
        'phone' => ['required', 'string']
    ]);

    $billing = $request->is_billing ?? 0;
    if ($billing) {
        Address::where(['is_billing' => 1])->update(['is_billing' => 0]);
    }

    $address = Address::create([
        'user_id' => $user->id,
        'token' => Str::random(32),
        'street_name' => $request->street,
        'house_number' => $request->number,
        'postal_code' => $request->postal_code,
        'state' => $request->state,
        'city' => $request->city,
        'country_id' => $request->country,
        'phone' => $request->phone,
        'is_billing' => $billing
    ]);

    return redirect('/dashboard/user/' . $user->user_token . '/addresses');
}

Next, insted line $user = Auth::user(), you can work with policies ( https://laravel.com/docs/master/authorization ). In docs you can see PostPolicy how's been created and yours should be named AddressPolicy.
And also, you should move validation to request file created with let's say

php artisan make:request AddressStoreRequest

Again, in docs you will find how to set code there ( https://laravel.com/docs/master/validation#creating-form-requests ).
That is what you can do to release controller method of code and set those code blocks in their respective classes. Although your code (way I wrote it above avoiding unnecessary DB calls) will work the same even if you don't make separate classes for form validation or for authorization.

Use this code and make similar for update method.


Project Euler #645 -- speed up Monte-Carlo simulation in Python I am trying to solve Q645. While the logic used for my code seems to be appropriate, the code itself is way too slow for the large number required in this question. May I ask for suggestion(s) to improve my code's performance?
The question is as in the link: https://projecteuler.net/problem=645
My Python code is as follow:
def Exp(D):
    day_list = [0]*D
    num_emperor = 0
    while all((d == 1 for d in day_list)) == False:
        #the birthday of the emperors are independent and uniformly distributed throughout the D days of the year
        bday = np.random.randint(0,D)
        day_list[bday] = 1
        num_emperor+=1
        #indices of d in day_list where d == 0
        zero_ind = (i for i,v in enumerate(day_list) if v == 0)
        for ind in zero_ind:
            try:
                if day_list[ind-1] and day_list[ind+1] == 1:
                    day_list[ind] = 1
            except IndexError:
                if ind == 0:
                    if day_list[-1] and day_list[1] == 1:
                        day_list[0] = 1
                elif ind == len(day_list)-1:
                    if day_list[len(day_list)-2] and day_list[0] == 1:
                        day_list[len(day_list)-1] = 1
    return num_emperor

def my_mean(values):
    n = 0
    summ = 0.0
    for value in values:
        summ += value
        n += 1
    return summ/n

def monte_carlo(iters, D):
    iter = 0
    n_emperor = 0
    while iter < iters:
        n_emperor = Exp(D)
        yield n_emperor
        iter += 1

avg_n_emperor = my_mean(monte_carlo(iters,D))
print(avg_n_emperor)

And my logic is as follow:
For the day_list inside the Exp(D) function, where D is the number of days in a year, zeros mean no holiday, and ones mean holiday. Initially the day_list is all zeros since there is no holiday to begin with.
The rules of defining a random day (d) as a holiday is as follow:

At the beginning of the reign of the current Emperor, his birthday is declared a holiday from that year onwards.

If both the day before and after a day d are holidays, then d also becomes a holiday.

I then subsequently implement the rules stated for the question, to gradually add holidays (ones) into the day_list. After num_emperor number of emperors, all the days (d) in day_list will become 1, i.e. all days will become holiday. This is the point to quit the while_loop in Exp(D) function and count the number of emperors required. To get the average number of emperors required for all the days to become holidays (avg_n_emperor), I then apply the monte-carlo method.
For my current code, the time takes is as follow:
avg_n_emperor = my_mean(monte_carlo(iters=100000,D=5)) #6-7 seconds

avg_n_emperor = my_mean(monte_carlo(iters=1000000,D=5)) #about 62 seconds

in which the time takes increase approx. linearly with the iters.
However,
avg_n_emperor = my_mean(monte_carlo(iters=1000,D=365)) #about 68 seconds

already takes about 68 seconds, and the question is asking for D=10000. Not to mention that the iters required for the answer to be accurate within 4 digits after the decimal points (as required by the question) would be much larger than 1000000 too...
Any suggestion(s) to speed up my code would be appreciated! :)

Welcome to Code Review. Nice implementation, easy to read and understand.
Optimization
There are some "expensive" operations that can be simplified. Below I commented the relevant parts:
def Exp(D):
    # the method "all" takes O(D)
    while all((d == 1 for d in day_list)) == False:
        # O(D)
        zero_ind = (i for i,v in enumerate(day_list) if v == 0)
        # O(D)
        for ind in zero_ind:
            # Here there are only O(1) operations
    return num_emperor

By \$O(D)\$ I mean that in the worst case such operation will iterate D times, where D is the number of days.
The condition in the while loop can be simplified by checking if the number of holidays is < days:
def Exp(D):
    holidays = 0
    while holidays < D:
        # increment holidays
    return num_emperor

The second optimization is to avoid the inner loops. Once the new birthday has been calculated, it's enough to "look around" that specific day:
def Exp(D):
    # ..
    while holidays < D:
        bday = np.random.randint(0,D)
        # Increment holidays only if birthday is not in a holiday
        if day_list[bday] == 0:
            holidays += 1
        day_list[bday] = 1
        num_emperor+=1

        yesterday = (bday - 1) % D
        day_before_yesterday = (bday - 2) % D
        if day_list[day_before_yesterday] == 1 and day_list[yesterday] == 0:
            day_list[yesterday] = 1
            holidays += 1

        tomorrow = (bday + 1) % D
        day_after_tomorrow = (bday + 2) % D
        if day_list[day_after_tomorrow] == 1 and day_list[tomorrow] == 0:
            day_list[tomorrow] = 1
            holidays += 1
    return num_emperor

The % operator prevents to overflow the array, so you don't need to catch exceptions.
Running the average:
avg_n_emperor = my_mean(monte_carlo(iters=1000,D=365))
# Output: 1173.786
# Running time: around 2 seconds

Regarding the style, @Peilonrayz already provided an excellent review.


F#unctional Immutable Trie (Prefix Tree) I've created the following implementation of a trie in F# as an effort to learn the language. I would like to know what could be done to make it more idiomatic. Any other pointers are welcome as well.

Of particular interest are:

Are my tests sufficient? Is there a better way to construct the tests?
Can the logic for insert be made cleaner?
To constrain type or not for functions and parameters?
What remains to be done to classify this as a proper collection?

type Trie<'T when 'T : comparison> = {
    IsTerminal: bool;
    Children: Map<'T, Trie<'T>>;
}

module Trie =
    let empty = {
        IsTerminal = false;
        Children = Map.empty;
    }

    let rec insert branch trie =
        match branch with
        | [] ->
            { trie with IsTerminal = true; }
        | el::els ->
            let child =
                match Map.tryFind el trie.Children with
                | Some t -> t
                | None -> empty
            let child' = insert els child
            let children =
                if Map.containsKey el trie.Children
                then Map.remove el trie.Children
                else trie.Children
            let children' = Map.add el child' children
            { trie with Children = children' }

    let rec contains branch trie =
        match branch with
        | [] -> trie.IsTerminal
        | el::els ->
            match Map.tryFind el trie.Children with
            | Some t -> contains els t
            | None -> false

    let rec toSeq trie =
        seq {
            for KeyValue(label, trie) in trie.Children do
                if trie.IsTerminal then yield [label]
                let children = toSeq trie
                for child in children do
                    yield label :: child
        }

    module Test =
        let ``insert can insert char into root`` =
            let result = empty |> insert ['a']
            Map.containsKey 'a' result.Children
        do assert ``insert can insert char into root``

        let ``insert can insert char into multiple levels`` =
            let result = empty |> insert ['a'..'d']
            Map.containsKey 'd' (((result.Children |> Map.find 'a').Children |> Map.find 'b').Children |> Map.find 'c').Children
        do assert ``insert can insert char into multiple levels``

        let ``contains finds branches that exist in tree`` =
            let result = empty |> insert ['a'..'d']
            contains ['a'..'d'] result
        do assert ``contains finds branches that exist in tree``

        let ``contains does not find incomplete branches`` =
            let result = empty |> insert ['a'..'d']
            result |> contains ['a'..'c'] |> not
        do assert ``contains does not find incomplete branches``

        let ``contains does not find extended branches`` =
            let result = empty |> insert ['a'..'d']
            result |> contains ['a'..'e'] |> not
        do assert ``contains does not find extended branches``

There are several libraries for writing unit tests in F#. For example:

FsUnit
Unquote
Fuchu

The logic is pretty clear, but there are a couple of comments

1.
Instead of:

        let child =
            match Map.tryFind el trie.Children with
            | Some t -> t
            | None -> empty

you can rewrite this code with using defaultArg

        let child =
            Map.tryFind el trie.Children
            |> defaultArg  <| empty

2.
You can use functions from a module Option, so..instead:

        match Map.tryFind el trie.Children with
        | Some t -> contains els t
        | None -> false

better:

        Map.tryFind el trie.Children
        |> Option.exists (contains els)


BASH correct usage of single/double quotes when writing variables to file (running Ubuntu 16.04.3)
I created small script with tasks I do every time I upgrade/change Ubuntu version. One of the tasks is to write script to file, this is actual written file:

#!/bin/bash

if [ "$1" == "eno1" ]; then
 case "$2" in
  up) nmcli radio wifi off
      notify-send "cable network detected, turning WIFI off" -u low -t 10;;
  down) nmcli radio wifi on
      notify-send "cable network unplugged, turning WIFI on" -u low -t 10;;
  esac
fi

And this is how I write it from another script:

MY_FILE="/etc/NetworkManager/dispatcher.d/99-wlan"
ni="$(nmcli dev status | grep ethernet | awk '{ print $1 }')"
echo -e '#!/bin/bash\n\nif [ "$1" == "'"$ni"'" ]; then\n case "$2" in\n'\
  ' up) nmcli radio wifi off\n      notify-send "cable network detected, turning WIFI off" --urgency critical --expire-time 6;;\n'\
  ' down) nmcli radio wifi on\n      notify-send "cable network unplugged, turning WIFI on" --urgency critical --expire-time 6;;\n'\
  ' esac\nfi' | sudo tee -a "$MY_FILE" > /dev/null
sudo chmod +x "$MY_FILE"

My question is, if syntax I'm using is correct. When I check it with shellcheck utility, it complaints about first line:

echo -e '#!/bin/bash\n\nif [ "$1" == "'"$ni"'" ]; then\n case "$2" in\n'\
        ^-- SC2016: Expressions don't expand in single quotes, use double quotes for that.
                                            ^-- SC2016: Expressions don't expand in single quotes, use double quotes for that.

can I ignore this warnings or is there a better how to write it ? Keep in mind I'm using variable $ni when I'm writing to file.

Instead of embedding sudo in the script, simply require that it be run as root, either by simply failing (using set -e), or with a friendly check such as

die() { echo "$@" >&2; exit 1; }

test $UID = 0 || die "$0: you need to be a superuser!"

That echo can be made much easier to read, and can avoid the non-portable -e option, by using a here-document:

cat <<END >"$MY_FILE"
#!/bin/bash
if [ "\$1" == "$ni" ]; then
    case "\$2" in
        up)
            nmcli radio wifi off
            notify-send "cable network detected, turning WIFI off" --urgency critical --expire-time 6
            ;;
        down)
            nmcli radio wifi on
            notify-send "cable network unplugged, turning WIFI on" --urgency critical --expire-time 6
            ;;
    esac
fi
END

(I'm assuming you meant tee rather than tee -a in your code).


What is a safe way to lock a thread in a single-threaded application? I was looking at the different possible threading methods that I could use in my single-threaded application. What I have written so far is a class that will lock the thread when I instantiate the object, then unlock the thread when the object becomes out of scope. Here is what the class looks like:

ProcessCritSec.h

#ifndef AFX_PROCESS_CRIT_SEC_H_
#define AFX_PROCESS_CRIT_SEC_H_

class CProcessCritSec
{
    public:
        CProcessCritSec(CCriticalSection* pcCriticalSection);
        virtual ~CProcessCritSec();

    private:
        CCriticalSection*   m_pcProcessCritSect;
};

#endif // !defined(AFX_PROCESS_CRIT_SEC_H_)

ProcessCritSec.cpp

#include "StdAfx.h"
#include "ProcessCritSec.h"

CProcessCritSec::CProcessCritSec(CCriticalSection* pcCriticalSection)
{
    m_pcProcessCritSect = pcCriticalSection;

    if (m_pcProcessCritSect != NULL)
    {
        m_pcProcessCritSect->Lock();
    }
}

CProcessCritSec::~CProcessCritSec()
{
    if (m_pcProcessCritSect != NULL)
    {
        m_pcProcessCritSect->Unlock();
    }
}

When I want to lock a thread I call this in a function:

CProcessCritSec cThreadSafety(&m_cCriticalSection);

Is this a safe way to locking a thread? Or is there a safer way to making my application single-threaded? Also I have a side question: When I instantiate this object in a try/catch block like so:

try
{
    CProcessCritSec cThreadSafety(&m_cCritialSection);

    Foo();
}
catch(...)
{
    //Log Exception
}

Will the thread become unlocked if an exception occurred within Foo() and the catch block catches the exception?

Is this a safe way to locking a thread?

Yes, it's the recommended way: see for example std::lock_guard

However:

I don't see why the destructor is virtual
You should probably disable/prevent the copy constructor and assignment operator
The constructor should probably take a reference (which can't be null) not a pointer
Try to ensure (though you're probably doing this already) that the destructor will not throw an exception

  Will the thread become unlocked if an exception occurred within Foo() and the catch block catches the exception?

It will unlock when CProcessCritSec is destroys, which happens as soon as it goes out of scope, which happens whenever you leave the try statement (before you enter any catch statement).


Count distinct primes, discarding palindromes, in under 2 seconds Problem Statement

  Generate as many distinct primes P such that reverse (P) is also prime
  and is not equal to P.

  Output:
  Print per line one integer( ≤ 1015 ). Don't print more than
  106 integers in all.

  Scoring:
  Let N = correct outputs. M= incorrect outputs. Your score
  will be max(0,N-M).

  Note: Only one of P and reverse(P) will be counted as correct. If both
  are in the file, one will be counted as incorrect.

  Sample Output
  107 13 31 17 2

  Explanation
  Score will be 1. Since 13,107,17 are correct. 31 is
  incorrect because 13 is already there. 2 is incorrect.

  Time Limit
  2 sec(s) (Time limit is for each input file.)

  Memory Limit
  256 MB

  Source Limit
  25 KB

My Problem

I have tried quite hard to optimize the solution, but the min possible time this program took was : 16.452 secs

My question is, is it possible to optimize the following code further, is it possible to reduce execution time to 2 secs, if we are given that we have to use the Python language.

from time import time
start = time()
lsta=[]   # empty list used to hold prime numbers created by primes function
LIMIT = pow(10,6)

# binary search function
def Bsearch(lsta,low,high,search):
    if low>high:
        return False
    else:
        mid=int((low+high)/2)
        if search<lsta[mid]:
            return(Bsearch(lsta,low,mid-1,search))
        elif search>lsta[mid]:
            return(Bsearch(lsta,mid+1,high,search))
        elif search==lsta[mid]:
            return True
        else:
            return False

# prime number creating function using sieve of Eras** algorithm
def primes(LIMIT):
    dicta = {}
    for i in range(2,LIMIT):
        dicta[i]=1
    for i in range(2,LIMIT):
        for j in range(i,LIMIT):
            if i*j>LIMIT:
                break
            dicta[i*j]=0
    for i in range(2,LIMIT):
        if(dicta[i]==1):
            lsta.append(i)

final = [] # used to hold the final output values
primes(LIMIT)
for i in range(len(lsta)):
    # prime number compared with reversed counterpart
    if(int(str(lsta[i])[::-1])<=lsta[len(lsta)-1]):
        if Bsearch(lsta,i+1,len(lsta)-1,int(str(lsta[i])[::-1])):
            if not(int(str(lsta[i])[::-1])==lsta[i]):
                final.append(str(lsta[i]))

for i in range(len(final)-1,0,-1):
    print(final[i])
print(13)
end=time()
print("Time Taken : ",end-start)

There are a few things you could optimize:

If you (or the grading server?) are running this on Python 2.x, you should definitely use xrange instead of range (about 2 seconds on my system)
In your prime sieve, you have to check the multiples only if the current number is a prime (down to 1s); also, instead of multiplying, count by multiples of is
use a regular array instead of a dictionary in the sieve (0.8s)
don't do that string-conversion and reversing more often than necessary; also instead of implementing your own binary search, just use sets for quick lookup (0.4s)

My version (updated):

def primes(limit):
    numbers = [0, 0] + [1] * (limit-2)
    for i in xrange(2, limit):
        if numbers[i]:
            for k in xrange(i**2, limit, i):
                numbers[k] = 0
    return set(i for i, e in enumerate(numbers) if e)

ps = primes(LIMIT)
final = set()
for p in ps:
    q = int(str(p)[::-1])
    if p != q and q in ps and q not in final:
        final.add(p)

Not sure whether this get's the running time down from 16s to 2s in your case, but it might be a start. As a desperate measure, you could decrease LIMIT -- better to find not quite as many than to timeout.