What's the optimal 'pythonic' way to make dot product of two lists of numbers? I just filled in the first assignment on course which involves learning Python. The assignment was to make vector class which supports scalar multiplication through operator overloading.

The instructions included these constains:

  There will be one class called MyVector
  Nothing will be imported in file within that file which contains the class
  The class will accept one argument in constructor, which must be list of numbers
  The class will have two methods:

  get_vector which returns list of numbers of the vector
  __mul__ (overloaded * operator) which performs scalar multiplication of two vectors

This is what I have sent:

class MyVector:
    """Vector class"""
    coordinates = None
    def __init__(self, coordinates):
        if not isinstance(coordinates, list):
            raise TypeError("Coordinates of vector must me single dimensional array.")
        self.coordinates = coordinates
    def f(self):
        return 'hello world'
    def get_vector(self):
        return self.coordinates
    def dimensions(self):
        return len(self.coordinates)
    def __mul__(self,other):
        if other.dimensions() != self.dimensions():
            raise ValueError("Number of dimensions of multiplied vectors must be equal.")
        tmp = 0
        for index in range(self.dimensions()):
            tmp += self.coordinates[index] * other.coordinates[index]
        return tmp

''' Just a testing section recommended in the assignment '''
if __name__ == "__main__":
    vec1 = MyVector([1,2,5,5,5]) # vektory mohou byt i jine dimenze nez 3!
    vec2 = MyVector([1,2,5,5,5])
    print(vec1.get_vector()) # Test getting the list of items
    dot_product = vec1*vec2  # Multiplication test
    print(dot_product)

The homework was OK, but the validation system is bragging that their implementation is faster:

  Message: module file vectors.py found
  Result ok
  Your elapsed time for 10,000 trials, vectors length 300: 0.675 seconds
  Our elapsed time for the same setting: 0.383 seconds

  Points: 2 out of 2

I'll repeat the relevant section of code here:

    def __mul__(self,other):
        if other.dimensions() != self.dimensions():
            raise ValueError("Number of dimensions of multiplied vectors must be equal.")
        tmp = 0
        for index in range(self.dimensions()):
            tmp += self.coordinates[index] * other.coordinates[index]
        return tmp

As you can see, as a seasoned C++ programmer I took naïve approach with indexed for loop. I know that Python has some famous tools to deal with array operation.

Is there some really pythonic way to loop over those two arrays and multiply their elements? Note that primarily I'm looking for pythonic way (since I'm learning Python), optimalization is secondary. I got my 2 points for homework, I'm not submiting it again.

The most "pythonic" way of doing this makes use of the sum and zip built-ins:

def __mul__(self,other):
    if other.dimensions() != self.dimensions():
        raise ValueError("Number of dimensions of multiplied vectors must be equal.")
    return sum(a * b for a, b in zip(self.coordinates, other.coordinates))

I suggest making use of the REPL (interactive shell) to learn about them, here is an example REPL session that you can read explaining zip and sum:

>>> x = [1, 4, 8]
>>> sum(x)
13
>>> y = [6, 1, 0]
>>> zip(x, y)
<zip object at 0x7f51da3db5c8>
>>> list(_) # `_` means previous value
[(1, 6), (4, 1), (8, 0)]
>>> help(zip)
Help on class zip in module builtins:

class zip(object)
 |  zip(iter1 [,iter2 [...]]) --> zip object
 |
 |  Return a zip object whose .__next__() method returns a tuple where
 |  the i-th element comes from the i-th iterable argument.  The .__next__()
 |  method continues until the shortest iterable in the argument sequence
 |  is exhausted and then it raises StopIteration.
 |
 |  Methods defined here:
 |
 |  __getattribute__(self, name, /)
 |      Return getattr(self, name).
 |
 |  __iter__(self, /)
 |      Implement iter(self).
 |
 |  __new__(*args, **kwargs) from builtins.type
 |      Create and return a new object.  See help(type) for accurate signature.
 |
 |  __next__(self, /)
 |      Implement next(self).
 |
 |  __reduce__(...)
 |      Return state information for pickling.

>>> help(sum)
Help on built-in function sum in module builtins:

sum(...)
    sum(iterable[, start]) -> value

    Return the sum of an iterable of numbers (NOT strings) plus the value
    of parameter 'start' (which defaults to 0).  When the iterable is
    empty, return start.

>>> zip( (1,2,3), "abc" )
<zip object at 0x7f51dba79648>
>>> list(_)
[(1, 'a'), (2, 'b'), (3, 'c')]

Other points

Tests should be automatic and NOT rely on you manually checking that wha is printed is right. This manual method will become too time consuming very soon.
f makes no sense (probably it is a leftover from a previous assignment). Please remove it.
get methods are discouraged in Python, accessing the items directly is very simple anyway.
Use @property to give convenient call sintax in dimensions(self) or remove this property completely as you can just call len(vector.coordinates) very easily.


Simple 2048 AI in Python A few weeks ago, I wrote a Python implementation of 2048. Since then, I've been working on a simple AI to play the game for me. The code uses expectimax search to evaluate each move, and chooses the move that maximizes the search as the next move to execute.

Similar to what others have suggested, the evaluation function examines monotonicity along a "snake path"; I also penalize boards that don't keep the largest tile at the snake path's terminal.

The AI can get to 2048 (and even 4096 occasionally), but not further. One of the things I've had difficulty with is tuning the evaluation function, and looking at what to try next. I have taken a look at awarding bonuses for zero tiles (for example, z*log(s) where z is the number of zeros and s is the total sum of the tiles on the board; zeros become more valuable deeper into the game) but haven't had much luck improving things. Rather, I feel like I'm guessing at what to do next to improve the heuristic (or I'll look at a board that defied my human strategy, and try and write something to mirror this), and am curious what someone else might suggest.

The code for the game and the AI can be found here. You can run aiplay with a Game instance.

Below is the relevant AI code:

from game import *

import math

def aimove(b):
    """
    Returns a list of possible moves ("left", "right", "up", "down")
    and each corresponding fitness
    """
    def fitness(b):
        """
        Returns the heuristic value of b

        Snake refers to the "snake line pattern" (http://tinyurl.com/l9bstk6)
        Here we only evaluate one direction; we award more points if high valued tiles
        occur along this path. We penalize the board for not having
        the highest valued tile in the lower left corner
        """
        if Game.over(b):
            return -float("inf")

        snake = []
        for i, col in enumerate(zip(*b)):
            snake.extend(reversed(col) if i % 2 == 0 else col)

        m = max(snake)
        return sum(x/10**n for n, x in enumerate(snake)) - \
               math.pow((b[3][0] != m)*abs(b[3][0] - m), 2)

    def search(b, d, move=False):
        """
        Performs expectimax search on a given configuration to
        specified depth (d).

        Algorithm details:
           - if the AI needs to move, make each child move,
             recurse, return the maximum fitness value
           - if it is not the AI's turn, form all
             possible child spawns, and return their weighted average
             as that node's evaluation
        """
        if d == 0 or (move and Game.over(b)):
            return fitness(b)

        alpha = fitness(b)
        if move:
            for _, child in Game.actions(b):
                return max(alpha, search(child, d-1))
        else:
            alpha = 0
            zeros = [(i,j) for i,j in itertools.product(range(4), range(4)) if b[i][j] == 0]
            for i, j in zeros:
                c1 = [[x for x in row] for row in b]
                c2 = [[x for x in row] for row in b]
                c1[i][j] = 2
                c2[i][j] = 4
                alpha += .9*search(c1, d-1, True)/len(zeros) + \
                         .1*search(c2, d-1, True)/len(zeros)
        return alpha
    return [(action, search(child, 5)) for action ,child in Game.actions(b)]

def aiplay(game):
    """
    Runs a game instance playing the move determined
    by aimove.
    """
    b = game.b
    while True:
        print(Game.string(b) + "\n")
        action = max(aimove(b), key = lambda x: x[1])[0]

        if action == "left" : b = Game.left(b)
        if action == "right": b = Game.right(b)
        if action == "up"   : b = Game.up(b)
        if action == "down" : b = Game.down(b)
        b = Game.spawn(b, 1)
        if Game.over(b):
            m = max(x for row in b for x in row)
            print("game over...best was %s" %m)
            print(Game.string(b))
            break

Don't use wildcard imports

PEP8 advises against this:

from game import *

Rewrite like this:

import math
import itertools

from game import Game

Use elif for mutually exclusive ifs

These if statements cannot all be true:

if action == "left":
    b = Game.left(b)
if action == "right":
    b = Game.right(b)
if action == "up":
    b = Game.up(b)
if action == "down":
    b = Game.down(b)

But they will all be evaluated, even if action is "left",
it will be checked against "right", "up", "down", pointlessly.

if action == "left":
    b = Game.left(b)
elif action == "right":
    b = Game.right(b)
elif action == "up":
    b = Game.up(b)
elif action == "down":
    b = Game.down(b)

Avoid shadowing variables

In this code, the parameter b of aimove is going to be shadowed by the parameters in fitness, and the many other methods where you reuse this name:

def aimove(b):
    # ...

    def fitness(b):
        # ...

    def search(b, d, move=False):
        # ...

This can be confusing and can lead to errors.

Dealing with long lines

This is just a small tip.
I don't like using \ for breaking new lines like this:

alpha += .9 * search(c1, d - 1, True) / len(zeros) + \
         .1 * search(c2, d - 1, True) / len(zeros)

I prefer to put the expression within parentheses,
which is another way of breaking a long line, like this:

alpha += (.9 * search(c1, d - 1, True) / len(zeros) +
          .1 * search(c2, d - 1, True) / len(zeros))

Easier way to make deep copies

c1 = [[x for x in row] for row in b]
c2 = [[x for x in row] for row in b]

An easier way:

from copy import deepcopy

c1 = deepcopy(b)
c2 = deepcopy(b)

Other simplifications

This expressions can be written simpler:

(b2[3][0] != m) * abs(b2[3][0] - m)

When b2[3][0] is not equal to m,
the expression becomes:

1 * abs(b2[3][0] - m)
abs(b2[3][0] - m)

When b2[3][0] is equal to m,
the expression becomes:

0 * abs(b2[3][0] - m)
0 * abs(m - m)
0 * abs(0)
0 * 0

Since the multiplication with (b2[3][0] != m) doesn't change the outcome,
you can simply write:

abs(b2[3][0] - m)

Game shouldn't be a class, in its present form

The posted code doesn't really use a Game instance.
It just takes the initial board out of the Game instance it receives,
but that's all the data a Game instance contains,
and it's never used again.
All the calls involving Game are static Game.some_function calls.
This is not a normal practice, and confusing.

As such, it would be better to move all the functions of Game to package scope,
do import game,
and call the methods like game.actions, game.over, and so on.

You point out in a comment that you like to use a class for logical grouping purposes. But classes are not the only thing that can do that, packages can do that too, in fact it's a very good way of using packages.

However, the option is probably to keep the Game class,
but make its methods non-static,
and work with instances, instead of calling static methods.
Instead of manipulating a board directly,
let the Game class contain the board,
and hide within the details of all the operations on the board.
It will be good encapsulation.
For example,
the static call Game.over(b) should be replaced with the instance method call game.over(), with no more board parameter.


Angular directive link vs controller? I have a very simple directive <my-card> that lists out some data.

Example Usage

my-card(ng-repeat="item in parentCtrl.items" item='item')

The only main functionality I have is a button that toggles some additional data.

Here is my directive/controller definition. Should I use the link function instead of creating a controller? The reason I moved the logic into the MyCardCtrl was so that I can easily unit test it.

MyCardCtrl

  function MyCardCtrl() {
    var vm = this;

    this.cardClass = 'my-card-' + vm.item.type;

    this.toggle = function() {
      vm.item.showMore = !vm.item.showMore;
    };

    this.init = function(element) {
      var img = angular.element(element[0].querySelector('.card-bg'));
      img.css('background-image', 'url(' + vm.item.image_url + ')');
    };
  }

myCard Directive

  function myCard() {
    return {
      restrict: 'E',
      replace: true,
      templateUrl: 'my-card.html'
      scope: {
        item: '='
      },
      link: function(scope, element, attrs, myCardCtrl) {
        myCardCtrl.init(element);
      },
      controller: 'MyCardCtrl',
      controllerAs: 'card',
      bindToController: true
    };
  }

  angular
    .module('components')
    .directive('myCard',      [myCard])
    .controller('MyCardCtrl', [MyCardCtrl]);

myCard template (written in Jade)

 .my-card(ng-class='class.cardClass')
    button(ng-click='card.toggleMeta()')
    .my-card-content(ng-show='card.item.showMore')
      // expose item object
      h1 {{::item.name}}
    .my-card-more(ng-show='!card.item.showMore')
    .my-card-bg

Just get rid of the link function entirely. You can set the background-image in the template with ng-style, and then you have one less function to worry about:

.my-card(ng-style='{"background-image": "url(" + card.item.image_url + ")"}', ng-class='card.cardClass')
  button(ng-click='card.toggle()')
  .my-card-content(ng-show='card.item.showMore')
    h1 {{::card.item.name}}
  .my-card-more(ng-show='!card.item.showMore')
  .my-card-bg

If you want to build up the url(card.item.image_url) string in the controller for easier testing, you can still use ng-style to apply the resulting variable. For example:

function MyCardCtrl() {
  var vm = this;

  vm.backgroundUrl = 'url("' + vm.item.image_url + '")';

  vm.cardClass = 'my-card-' + vm.item.type;

  vm.toggle = function() {
    vm.item.showMore = !vm.item.showMore;
  };
}

Also, if you're on Angular 1.5+, this directive can be written even more simply as a component:

angular
  .module('components')
  .component('myCard', {
    bindings: {
      item: '<'
    },
    templateUrl: 'my-card.html',
    controller: 'MyCardCtrl',
    controllerAs: 'card'
  })
  .controller('MyCardCtrl', [MyCardCtrl]);


Building balanced BST from sorted array: recursive and iterative approaches I am implementing an algorithm to solve the following problem:

  Given a sorted (increasing order) array with unique integer elements, write an algorithm to create a binary search tree with minimal height.

The solution of this problem using recursion is quite straightforward:

class Node:
    def __init__(self, value):
        self.value = value
        self.left = None
        self.right = None

def build_bst_recursive(array):
    size = len(array)
    return _build_bst(array, first=0, last=size - 1)

def _build_bst(array, first, last):
    if last < first:
        return None
    mid = (first + last) // 2
    node = Node(mid)
    node.left = _build_bst(array, first, mid - 1)
    node.right = _build_bst(array, mid + 1, last)
    return node

Out of curiosity I also would like to solve the same problem using an iterative algorithm. Here is what I came up with:

class Stack:
    class _Node:
        def __init__(self, value):
            self.value = value
            self.next_node = None

    def __init__(self):
        self.head_node = None

    def is_empty(self):
        return self.head_node is None

    def push(self, value):
        node = self._Node(value)
        node.next_node = self.head_node
        self.head_node = node

    def pop(self):
        if self.is_empty():
            raise Exception("Stack is empty.")
        node = self.head_node
        self.head_node = node.next_node
        return node.value

def build_bst_iterative(array):
    size = len(array)

    # Stack stores tuples of the first and the last indices of half-segments:
    stack_indices = Stack()
    stack_indices.push((0, size - 1))

    # Stack stores tree nodes:
    stack_nodes = Stack()

    # Add to the stack a node that will be a root of the tree:
    root_node = Node(None)
    stack_nodes.push(root_node)

    while not stack_indices.is_empty():
        first, last = stack_indices.pop()
        node = stack_nodes.pop()

        if last < first:
            # The segment is degenerated. Keep the node empty.
            continue
        elif last == first:
            # Assign the value, it is the last bottom node.
            node.value = array[last]
            continue

        mid = (first + last) // 2
        node.value = array[mid]
        node.left = Node(None)
        node.right = Node(None)

        # Update stacks:
        # (right half-segment)
        stack_indices.push((mid + 1, last))
        stack_nodes.push(node.right)
        # (left half-segment)
        stack_indices.push((first, mid - 1))
        stack_nodes.push(node.left)
    assert stack_nodes.is_empty()

    return root_node

I would highly appreciate if you could review the code and suggest any approaches for improvement. For example, I am trying to understand if it is possible to use only one stack.

EDIT After looking into my code in the morning, I realise that there is no need for two stacks because I always push and pop to the two stacks at the same time. Here there is an updated version of the code:

def build_bst_iterative_one_stack(array):
    size = len(array)

    # Add to the stack a node that will be a root of the tree:
    root_node = Node(None)

    # Stack stores tuples of the current node, and the first and the last indices of half-segments:
    stack = Stack()
    stack.push((root_node, 0, size - 1))

    while not stack.is_empty():
        node, first, last = stack.pop()

        if last < first:
            # The segment is degenerated. Do nothing.
            continue
        elif last == first:
            # Assign the value, it is the last bottom node.
            node.value = array[last]
            continue

        mid = (first + last) // 2
        node.value = array[mid]
        node.left = Node(None)
        node.right = Node(None)

        # Update the stack with the left and the right half-segment:
        stack.push((node.right, mid + 1, last))
        stack.push((node.left, first, mid - 1))
    assert stack.is_empty()

    return root_node

Let me start with an all too common hindsight:
your code lacks test support and scaffold.

The most basic test support for a class is a succinct __str__, for class Node:

def __str__(self):
    return '<' + ('.' if not self.left else (str(self.left) + '|')) \
        + str(self.value) + ('.' if not self.right
                             else ('|' + str(self.right))) + '>'

(a more elaborate one would allow to limit recursion.) The result for

if __name__ == '__main__':
    ascending = [chr(x) for x in range(ord('a'), ord('g'))]
    print(build_bst_recursive(ascending))
    print(build_bst_iterative_one_stack(ascending))

  <<.0|<.1.>>|2|<<.3.>|4|<.5.>>>
  <<<.None.>|a|<.b.>>|c|<<.d.>|e|<.f.>>>

highlights an oversight with _build_bst() (not using node = Node(array[mid])) and a problem in the stack variants with always creating two child nodes (or with the handling commented # The segment is degenerated. Do nothing.).

Your code is more or less in line with the Style Guide for Python Code (good - do you use an IDE, your IDE's PEP8 support?); make it a habit to provide documentation strings for everything public, too.

If I was serious about comparing alternative implementations, I'd try to define an interface and hope to avoid names like build_bst_iterative_one_stack.
I got used to the builtin len(), I don't see an advantage to introducing size = len(array) where used once.

_build_bst() is pretty basic; if it was public, it would need a doc string specifying last to be inclusive. I don't like the name array - would prefer values or ordered (ascending would be misleading, as _build_bst() works perfectly for descending values).

I'd hesitate to roll my own stack class, more so if the stated purpose was something else, entirely. If I did, I'd just extend list:

class Stack(list):
    ''' syntactic sugar: adds push(), is_empty() to list's pop() '''
    push = list.append

    def is_empty(self):
        return not bool(self)

(Skipping build_bst_iterative().)
In build_bst_iterative_one_stack(), I'd prefer toDo over stack. The comment stack stores […and…] indices of half-segments is funny in the first tuple pushed two statements down not being a half.
I don't have an inspiration how to fix the introduction of None-Nodes without duplicating the check.

Your "build_bst_iterative*()" implementations are straightforward conversions of the basic recursive approach (see Create Balanced Binary Search Tree from Sorted linked list for one working without "random" access); I would not expect insights beyond doesn't get prettier for explicit stack handling.

Much to my dismay, I didn't find a decent web reference for a genuine iterative approach (numerous weird ones, some as new as 2019/3/2):
First consider the case of 2ⁿ-1 nodes: with levels numbered from 0 for bottom (up to n-1), the level of each node is the number of trailing zeroes in its ordinal. Have an array to reference one node for each level, initialised to Nones. For each value, create a Node. Check the reference at the level indicated by the number of trailing zeroes in its ordinal: if None, just plant the node there and make whatever is one level lower its left descendant. If not None, make node the right descendant of the node at the next higher level an set this level to None. (As an alternative to checking this reference, you can inspect the bit in the ordinal just two above the trailing zeroes.)
There are numerous ways to handle N ≠ 2ⁿ-1; one being to aim for a complete binary tree: compute the ordinal rb of the rightmost node at the bottom level. Proceed as above to rb+1, and increase the "ordinal number" by two instead of one thereafter.
(Finally, return the node at the highest level as the root of the tree built…)


Making loop in CSS Stylus more efficient I'm not sure if this could be more efficient because I don't think I should need 2 keyframe loops in addition to the for loop?

for i in (1..19)
.l:nth-child({i})
n = (20 - i)
height 0
if i >= 10
  margin-left ((n * n) * (1.25/n)) + 17 px
  box-shadow 0px 0px (n+4)px 0 rgba(40,70,200, 0.5),0px 0px (i+8)px 0 rgba(80,100,235, 0.45),0px 0px 14px 0 rgba(115,215,235, 0.35),0px 0px 23px 0 rgba(110,130,215, 0.5)
  width (n * 1.05) + 0px
  animation (heights- + i) 0.75s (n/80)s linear forwards
else
  margin-left ((i * i) * (1.25/i)) + 17 px
  box-shadow 0px 0px (i+4)px 0 rgba(40,70,200, 0.5),0px 0px (i+8)px 0 rgba(80,100,235, 0.45),0px 0px 14px 0 rgba(115,215,235, 0.35),0px 0px 23px 0 rgba(110,130,215, 0.5)
  width (i * 1.05) + 0px
  animation (heightsx- + i) 0.75s (i/80)s linear forwards

for i in (10..19)
n = (20 - i)
$keyframe-name = (heights- + i)
@keyframes {$keyframe-name}
  0%
    height 0
  91%
    height ((n + 4) * 1.4) px
  98%
    height ((i + 38) * 5.4) px
  100%
    height ((n + 38) * 4.4) px
    opacity (n/6)
    if i == 10
    height ((i + 43) * 4.4) px
    opacity (i/6)

for i in (1..9)
$keyframe-namex = (heightsx- + i)
@keyframes {$keyframe-namex}
  0%
    height 0
  91%
    height ((i + 4) * 1.4) px
  98%
    height ((i + 38) * 5.4) px
  100%
    height ((i + 38) * 4.4) px
    opacity (i/16)

Based on what I can tell of these three loops is that they are not nested, which means that you are running one loop for the whole range of 1-19 another loop for the range of 1-9 (which you already went over once) and another loop for the range of 10-19 which you have already looped over as well.  and the first loop that goes over the entire range already divides the top half from the bottom half to change other things, I may be looking at this all wrong, but I put the last two loops inside the first loops if-then statements and came up with this

for i in (1..19)
  .l:nth-child({i})
  n = (20 - i)
  height 0
  if i >= 10
    margin-left ((n * n) * (1.25/n)) + 17 px
    box-shadow 0px 0px (n+4)px 0 rgba(40,70,200, 0.5),0px 0px (i+8)px 0 rgba(80,100,235, 0.45),0px 0px 14px 0 rgba(115,215,235, 0.35),0px 0px 23px 0 rgba(110,130,215, 0.5)
    width (n * 1.05) + 0px
    animation (heights- + i) 0.75s (n/80)s linear forwards

    $keyframe-name = (heights- + i)
    @keyframes {$keyframe-name}
      0%
        height 0
      91%
        height ((n + 4) * 1.4) px
      98%
        height ((i + 38) * 5.4) px
      100%
        height ((n + 38) * 4.4) px
        opacity (n/6)
        if i == 10
          height ((i + 43) * 4.4) px
          opacity (i/6)

  else
    margin-left ((i * i) * (1.25/i)) + 17 px
    box-shadow 0px 0px (i+4)px 0 rgba(40,70,200, 0.5),0px 0px (i+8)px 0 rgba(80,100,235, 0.45),0px 0px 14px 0 rgba(115,215,235, 0.35),0px 0px 23px 0 rgba(110,130,215, 0.5)
    width (i * 1.05) + 0px
    animation (heightsx- + i) 0.75s (i/80)s linear forwards

    $keyframe-namex = (heightsx- + i)
    @keyframes {$keyframe-namex}
      0%
        height 0
      91%
        height ((i + 4) * 1.4) px
      98%
        height ((i + 38) * 5.4) px
      100%
        height ((i + 38) * 4.4) px
        opacity (i/16)

I agree with @Cimmanon in that you would get more answers to this question if we had somewhere that we could run the code and tinker with it a little bit.  I assume that you would have done it this way to begin with unless there was another reason for doing it this way.

I also moved some variables around to merge them and use them in other expressions.

like

    animation (heights- + i) 0.75s (n/80)s linear forwards

    $keyframe-name = (heights- + i)
    @keyframes {$keyframe-name}
      0%
        height 0
      91%
        height ((n + 4) * 1.4) px
      98%
        height ((i + 38) * 5.4) px
      100%
        height ((n + 38) * 4.4) px
        opacity (n/6)
        if i == 10
          height ((i + 43) * 4.4) px
          opacity (i/6)

you create $keyframe-name to hold the value heights- + i when you could have just put that expression into @keyframes{heights- + i}, but since we merged the two for loops into one we can use $keyframe-name like this

$keyframe-name = (heights- + i)
animation ($keyframe-name) 0.75s (n/80)s linear forwards
@keyframes {$keyframe-name}
  0%
    height 0
  91%
    height ((n + 4) * 1.4) px
  98%
    height ((i + 38) * 5.4) px
  100%
    height ((n + 38) * 4.4) px
    opacity (n/6)
    if i == 10
      height ((i + 43) * 4.4) px
      opacity (i/6)

and do the same thing inside the else statement

$keyframe-namex = (heightsx- + i)
animation ($keyframe-namex) 0.75s (i/80)s linear forwards
@keyframes {$keyframe-namex}
  0%
    height 0
  91%
    height ((i + 4) * 1.4) px
  98%
    height ((i + 38) * 5.4) px
  100%
    height ((i + 38) * 4.4) px
    opacity (i/16)


Keeping attribute names simple when using getters and setters Is the use of _pn in __init__ pythonic? It seems that most examples change the actual attribute to self._pn to indicate it's private, but I would like to be able to call self.pn but have the advantage of checks with setters.

The following code will cause a recursive loop:

class part:
'Holds various attributes of interest for a part number'
def __init__(self, pn):
    self.pn = pn

def set_pn(self, pn):
    if type(pn) != str:
        raise Exception("Part number needs to be a string")
    self.pn = _pn

pn = property(fset = set_pn)

The following won't, but I'm not sure if _pn is pythonic:

class part:
'Holds various attributes of interest for a part number'
def __init__(self, _pn):
    self.pn = _pn

def set_pn(self, pn):
    if type(pn) != str:
        raise Exception("Part number needs to be a string")
    self.pn = _pn

_pn = property(fset = set_pn)

I think this is the best way to do it:

class Part(object):
    def __init__(self, pn):
        self.pn = pn

    @property
    def pn(self):
        return self._pn

    @pn.setter
    def pn(self, value):
        if type(value) != str:
            raise Exception("Part number must be a string")
        self._pn = value

Remarks:

The _ prefix for properties is well-accepted, see the docs
self.pn = pn is to reuse the validation implemented in the setter
Class names should be CamelCase
Modern classes should extend object (class Part(object): ... as opposed to plain class Path: ...)
Doc strings should be """class description""" instead of 'class description'


Calculating the Collatz conjecture of a number I've made a Collatz Conjecture sequence calculator and I would like some feedback on what I can do better in regards to the code I've implemented, i.e. bad practices you see, improvements that can be made to make it faster, errors that might arise from this code that I am not managing, etc.

public static void CollatzConjecture()
{
    Console.WriteLine("Enter an integer number into the program to calculate its Collatz Conjecture sequence:");
    var consoleReading = string.Empty;
    var collatzConjectureNumber = 0;
    while (true)
    {
        consoleReading = Console.ReadLine();
        if (int.TryParse(consoleReading, out collatzConjectureNumber))
            break;
        else
        {
            Console.WriteLine("Please insert an integer number:");
        }
    }
    var collatzNumberCollection = new List<int>();

    Console.WriteLine("The entered number is {0}, calculating Collatz's sequence:", collatzConjectureNumber);

    collatzNumberCollection.Add(collatzConjectureNumber);

    int collatzStepCounter = 0;
    while (collatzConjectureNumber > 1)
    {
        collatzConjectureNumber = (collatzConjectureNumber%2).Equals(0)
            ? collatzConjectureNumber / 2
            : collatzConjectureNumber * 3 + 1;

        collatzNumberCollection.Add(collatzConjectureNumber);

        Console.WriteLine("Element {0} of the {1}'s collatz sequence is: {2}", ++collatzStepCounter, collatzNumberCollection[0], collatzConjectureNumber);
        Thread.Sleep(100);
    }

    Console.WriteLine("The Collatz Conjecture sequence for the number {0} contains {1} numbers", collatzNumberCollection[0], collatzNumberCollection.Count);

    Console.WriteLine("The Collatz Conjecture sequence has been calculated, press enter key to exit");
    Console.ReadLine();
}

The code works as intended but, as said, I would like feedback on what I can improve in this code. That explains why I post this question even though there exist duplicates of this one, I just want feedback on my code, that's all.

Thanks in advance for your responses.

Single Responsibility Principle

The CollatzConjecture method is the so called god-method. What does this mean? It does everything. But what exactly? I has three responsibilities:

read the number from the console
calculate the Collatz Conjecture
print the results

Each of those features should be encapsulated in their own method. But why? So that you can better test it. Currently you cannot write a unit test to validate whether the calculation is correct because it's bound to the console so you need to extract the math first:

public static IEnumerable<int> CollatzConjecture(int value)
{
    while (value > 1)
    {
        value =
            (value % 2) == 0
            ? value / 2
            : value * 3 + 1;

        yield return value;
    }
}

Now you have a deferred method that does the math. Notice the yield return.

You could already use it but you want to see the output so you need another method that can print it. Here it is:

public static void PrintCollatzConjecture(int number, IEnumerable<int> collatzConjecture, int delayInMilliseconds = 100)
{
    var counter = 0;
    foreach (var value in collatzConjecture)
    {
        Console.WriteLine("Element {0} of the {1}'s collatz sequence is: {2}", ++counter, number, value);
        Thread.Sleep(delayInMilliseconds);
    }
    Console.WriteLine("The Collatz Conjecture sequence for the number {0} contains {1} numbers", number, counter);
    Console.WriteLine("The Collatz Conjecture sequence has been calculated, press enter key to exit");
    Console.ReadLine();
}

No magic here. Just an ordinary loop that prints each element. Additionally you can specify a different delay.

To make it interactive you need to create the third method that will read the number from the user:

public static int ReadNumber()
{
    Console.WriteLine("Enter an integer number into the program to calculate its Collatz Conjecture sequence:");

    var result = 0;
    var input = (string)null;

    var isValidNumber = true;
    do
    {
        if (!isValidNumber)
        {
            Console.WriteLine("Please insert an integer number:");
        }
        input = Console.ReadLine();
        isValidNumber = int.TryParse(input, out result);
    } while (!isValidNumber);

    Console.WriteLine("The entered number is {0}, calculating Collatz's sequence:", result);

    return result;
}

All three features are now encapsulated. You are free to modify and optimize any of them without breaking the other two. This is what we always strive for. One module/class/method does only one thing at a time.

If you assemble everything your Main will be nice and short:

void Main()
{
    var number = ReadNumber();
    var collatzConjecture = CollatzConjecture(number);
    PrintCollatzConjecture(number, collatzConjecture);
}

C# 6

With the latest C# there is an easier way to build string. You can put the variables inside them. In order to do this you need to prefix it with the $ e.g.:

Console.WriteLine($"The entered number is {result}, calculating Collatz's sequence:");


Notify Property Changed Inspired by a SO post, just as an exercise, I tried to implement a way of notifying a consumer of a class about whether a property on an object has changed or not. I'm not attempting to use this in anyway, but I would like to learn from it.

interface INotifyPropertyChanged
{
    function PropertyChanged($name);
    function RegisterChangeCallback($callback);
}

abstract class NotifyPropertyChanged implements INotifyPropertyChanged
{
    public $changes;
    public $trackChanges;
    private $callback;

    public function PropertyChanged($name)
    {
        if( $this->callback != null){
            call_user_func($this->callback, $name);
        }

        if( !isset($this->changes))
            $this->changes = Array();

        if( !in_array($name, $this->changes))
            $this->changes[] = $name;
    }

    public function RegisterChangeCallback($callback)
    {
        $this->callback = $callback;
    }

    public function __set($name, $value)
    {
        $previousValue = $this->$name;
        $this->$name = $value;

        if( $this->trackChanges && $previousValue !== $value) {
            $this->PropertyChanged($name);
        }
    }

    public function __get($name)
    {
        return $this->$name;
    }
}

Then, implementing a class;

class SampleClass extends NotifyPropertyChanged
{
    protected $name;
}

And finally, usage;

$class = new SampleClass();
$class->name = "value";
$class->trackChanges = true;
$class->name = "test";
var_dump($class->changes);

And if you want, you can call $class->RegisterChangeCallback(..) to be told when it does change.

Like I said, I'm not planning on using this, it was an exercise to see if I could do something similar to .NETs INotifyPropertyChanged. And, what better way to get feedback on an idea.

First of all it is useless to use __set or __get if you're trying to access the non existing property that might also cause a cyclic request of the method.  Instead you should have  an array that contains the values  now second thing if you will have the property inside the object the __set or __get methods won't be called. So you should fix those and it should work.  If needed I will post a code with the fix

<?php
interface INotifyPropertyChanged
{
    function PropertyChanged($name);
    function RegisterChangeCallback($callback);
}

abstract class NotifyPropertyChanged implements INotifyPropertyChanged
{
    public $changes;
    public $trackChanges;
    private $callback;
    protected $values = array();
    public function PropertyChanged($name)
    {
        if( $this->callback != null){
            call_user_func($this->callback, $name);
        }

        if( !isset($this->changes))
            $this->changes = Array();

        if( !in_array($name, $this->changes))
            $this->changes[$name][] = $this->$name;
    }

    public function RegisterChangeCallback($callback)
    {
        $this->callback = $callback;
    }

    public function __set($name, $value)
    {
        $previousValue = $this->$name;
        if( $this->trackChanges && $previousValue !== $value) {
            $this->PropertyChanged($name);
        }
        $this->values[$name] = $value;
    }

    public function __get($name)
    {
    if(array_key_exists($name,$this->values)){
        return $this->values[$name];
        }
        return null;
    }
}

class SampleClass extends NotifyPropertyChanged
{
}

$class = new SampleClass();
$class->name = "value";
$class->trackChanges = true;
$class->name = "test";
$class->name = "test2";
var_dump($class->changes);

the above code results which also allows you to have all changes

array(1) {
    ["name"]=>
    array(2) {
        [0]=>
        string(5) "value"
        [1]=>
        string(4) "test"
    }
}


Using numpy polynomial module - is there a better way? I'm working on a project where I need to solve for one of the roots of a quartic polymonial many, many times.  Is there a better, i.e., faster way to do this?  Should I write my own C-library?  The example code is below.

# this code calculates the pH of a solution as it is
# titrated with base and then plots it.

import numpy.polynomial.polynomial as poly
import numpy as np
import matplotlib.pyplot as plt

# my pH calculation function
# assume two distinct pKa's  solution is a quartic equation
def pH(base, Facid1, Facid2):
    ka1 = 2.479496e-6
    ka2 = 1.87438e-9
    kw = 1.019230e-14
    a = 1
    b = ka1+ka2+base
    c = base*(ka1+ka2)-(ka1*Facid1+ka2*Facid2)+ka1*ka2-kw
    d = ka1*ka2*(base-Facid1-Facid2)-kw*(ka1+ka2)
    e = -kw*ka1*ka2
    p = poly.Polynomial((e,d,c,b,a))
    return -np.log10(p.roots()[3]) #only need the 4th root here

# Define the concentration parameters
Facid1 = 0.002
Facid2 = 0.001
Fbase = 0.005    #the maximum base addition

# Generate my vectors
x = np.linspace(0., Fbase, 200)
y = [pH(base, Facid1, Facid2) for base in x]

# Make the plot frame
fig = plt.figure()
ax = fig.add_subplot(111)

# Set the limits
ax.set_ylim(1, 14)
ax.set_xlim(np.min(x), np.max(x))

# Add my data
ax.plot(x, y, "r-") # Plot of the data use lines

#add title, axis titles, and legend
ax.set_title("Acid titration")
ax.set_xlabel("Moles NaOH")
ax.set_ylabel("pH")
#ax.legend(("y data"), loc='upper left')

plt.show()

Based on the answer, here is what I came up with.  Any other suggestions?

# my pH calculation class
# assume two distinct pKa's  solution is a quartic equation
class pH:
    #things that don't change
    ka1 = 2.479496e-6
    ka2 = 1.87438e-9
    kw = 1.019230e-14
    kSum = ka1+ka2
    kProd = ka1*ka2
    e = -kw*kProd

    #things that only depend on Facid1 and Facid2
    def __init__(self, Facid1, Facid2):
        self.c = -(self.ka1*Facid1+self.ka2*Facid2)+self.kProd-self.kw
        self.d = self.kProd*(Facid1+Facid2)+self.kw*(self.kSum)

    #only calculate things that depend on base
    def pHCalc(self, base):
        pMatrix = [[0, 0, 0, -self.e],  #construct the companion matrix
                   [1, 0, 0, self.d-base*self.kProd],
                   [0, 1, 0, -(self.c+self.kSum*base)],
                   [0, 0, 1, -(self.kSum+base)]]
        myVals = la.eigvals(pMatrix)
        return -np.log10(np.max(myVals)) #need the one positive root

NumPy computes the roots of a polynomial by first constructing the companion matrix in Python and then solving the eigenvalues with LAPACK. The companion matrix case looks like this using your variables (as a==1):

[0 0 0 -e
 1 0 0 -d
 0 1 0 -c
 0 0 1 -b]

You should be able to save some time by updating a matrix like this directly on each iteration of base. Then use numpy.linalg.eigvals(m).max() to obtain the largest eigenvalue. See the sources.


Optmize logic IF statement I have a little dilema with such if statement, I have tried optimizing it in order to remove some checks, but could not think of how to make this code part cleaner. Can someone please make a review, and tell if it's possible to optimize such statement of maybe there is some law for this? Thank you

//letter = 'E' or 'R' ( char )
//second letter = 'Q' or 'A' ( char )
//DoFlip = false or true ( bool )

if((letter == 'E' && secondLetter == 'Q' && DoFlip == false)    ||
       (letter == 'E' && secondLetter == 'A' && DoFlip == true) ||
       (letter == 'R' && secondLetter == 'Q' && DoFlip == false)||
       (letter == 'R' && secondLetter == 'A' && DoFlip== true))
    {
      //DoSmth here
    }

Firstly, this can be rewritten as:

if(((letter == 'E' || letter == 'R')&& secondLetter == 'Q' && DoFlip == false)||
   ((letter == 'E' || letter == 'R') && secondLetter == 'A' && DoFlip == true)
    {
      //DoSmth here
    }

According to distributive law -

A && B || A && C = (B || C) && A

In your logic - (letter == 'E' || letter == 'R') can take place of A with (secondLetter == 'Q' && DoFlip == false) as B and (secondLetter == 'A' && DoFlip == true) as C.

So your logic can be simplified to -

if((letter == 'E' || letter == 'R') &&
   ((secondLetter == 'Q' && DoFlip == false) ||
   (secondLetter == 'A' && DoFlip == true))


A method that returns an array with no duplicates I implemented a method in Java that returns an array of integers with no duplicates (i.e. an array that has no repeated numbers).

My solution seems rather long. I would like to know of ways to improve it...

public class IntArrayProcessor {

    private int[] a;

    public IntArrayProcessor(int[] a) {
        this.a = a;
    }

    /**
     *
     * @return Array with no repeated integers.
     */
    public int[] getSet() {
        /* creates an array with the same entries and length as this.a */
        int[] duplicateA = new int[this.a.length];

        /* stores the number of repeated entries in array this.a */
        int numberOfDuplicates = 0;

        /* is the integer a duplicate or not? */
        boolean isDuplicate;

        /**
         * Counts the number of duplicates in array this.a
         */
        for (int i = 0; i < this.a.length; i++) {
            duplicateA[i] = this.a[i];
        }

        for (int i = 0; i < duplicateA.length; i++) {
            isDuplicate = false;
            for (int j = i + 1; j < this.a.length; j++) {
                if (duplicateA[i] == this.a[j]) {
                    isDuplicate = true;
                }
            }
            if (isDuplicate) {
                numberOfDuplicates++;
            }
        }

        /*
         * the noDuplicate array has the lenght of the this.a array minus the
         * number of repeated entries
         */
        int[] noDuplicate = new int[this.a.length - numberOfDuplicates];

        /* to keep track of the noDuplicate indexes */
        numberOfDuplicates = 0;

        /**
         * An array with no repeated numbers
         */
        for (int i = 0; i < duplicateA.length; i++) {
            isDuplicate = false;
            for (int j = i + 1; j < this.a.length; j++) {
                if (duplicateA[i] == this.a[j]) {
                    isDuplicate = true;
                }
            }
            if (!(isDuplicate)) {
                noDuplicate[numberOfDuplicates] = duplicateA[i];
                numberOfDuplicates++;
            }
        }
        return noDuplicate;
    }
}

Wouldn't it be easier to just use:
public static class IntArrayProcessor {

    private Integer[] arr;

    public IntArrayProcessor(Integer[] arr) {
        this.arr = arr;
    }

    public Integer[] unique() {
        final Set<Integer> uniqueSet = new HashSet<Integer>(Arrays.asList(this.arr));
        return uniqueSet.toArray(new Integer[uniqueSet.size()]);
    }
}

I changed the method name from getSet() to the more descriptive unique().
This simply generates a hash set, which contains only the unique values of the array. The hash set (uniqueSet) is then converted from Set to Integer[], and returned.
Given that this is using built-in methods and objects, it would likely be faster than most custom implementations.
Notice that HashSet:

makes no guarantees as to the insertion order of the set; in particular it does not guarantee that the order will remain constant over time.
— HashSet documentation

If you need to keep insertion order, use LinkedHashSet as suggested in @mrblewog's comment. If you want to have the internal Set sorted, use SortedSet or TreeSet.
I also changed int[] to Integer[] so that it can be used with generics (generics do not support primitive types).


Checking for Null before adding into List Is there any way to refactor this?

public IEnumerable<Option> Options
{
    get
    {
        {
            List<Option> ListOption = new List<Option>();

            if (!String.IsNullOrEmpty(Option1))
            {
                ListOption.Add(new Option() {Name=Option1 });
            }
            if (!String.IsNullOrEmpty(Option2))
            {
                ListOption.Add(new Option() { Name = Option2 });
            }
            if (!String.IsNullOrEmpty(Option3))
            {
                ListOption.Add(new Option() { Name = Option3 });
            }
            if (!String.IsNullOrEmpty(Option4))
            {
                ListOption.Add(new Option() { Name = Option4 });
            }

            return ListOption;
        }
    }
}

public class Option
{
    public string Name { get; set; }
}

This is a bit neater:

List<Option> ListOption = new List<Option> { };
Action<string> add = x => { if (!String.IsNullOrEmpty(x)) ListOption.Add(new Option { Name = x }); }
add(Option1);
add(Option2);
add(Option3);
add(Option4);
return ListOption;

This is perhaps even better:

return (new string[] {Option1, Option2, Option3, Option4})
       .Where(x => !String.IsNullOrEmpty(x))
       .Select(x => new Option { Name = x })
       .ToList();


Macro to remove rows with data not meeting certain requirements I have created several macros that delete rows based on cell values. I run it 5 times for different data, but think there should be a way to combine them and speed up the macro. Currently, it is running a little slow.

It is pulling a date from another worksheet and if it doesn't match, removing the line. I am basically running the same macro over and over, just changing the values slightly.

I am just changing the value in column E (120 in this example, but could be 30, 60, 90, etc) and seeing if it matches the date on another worksheet. The cell on the other worksheet changes depending on E value
If 30, use date A1
If 60, use date in A2
etc.

Here is one of the five macros I have:

Sub Remove_FutureRenewals_120()
    Dim Firstrow As Long
    Dim LastRow As Long
    Dim Lrow As Long
    Dim CalcMode As Long
    Dim ViewMode As Long

Firstrow = ThisWorkbook.Worksheets(2).UsedRange.Cells(1).Row
LastRow = ThisWorkbook.Worksheets(2).Cells(ThisWorkbook.Worksheets(2).Rows.Count, "A").End(xlUp).Row

With Application
    CalcMode = .Calculation
    .Calculation = xlCalculationManual
    '.ScreenUpdating = False
End With

With ActiveSheet
    For Lrow = LastRow To Firstrow Step -1
        With .Cells(Lrow, "E")
            If Not IsError(.Value) Then
                If .Value = "120" _
                    And Format(.Offset(0, -1).Value, "YYYYMMDD") <> Format(ThisWorkbook.Worksheets("Date Calc").Cells(5, "A").Value, "YYYYMMDD") _
                    Then .EntireRow.Delete
            End If
        End With

    Next Lrow

End With

End Sub

An interesting learning experience.

Well done for doing the deletion in a backwards loop. Also, well done for declaring all variables.

As you have implied, there is room for some improvement. What I suggest below may not increase speed, but should at least help with maintainability and readability.

Three different ways of accessing a sheet

You are using three different ways of accessing a sheet: index, ActiveSheet and name. Each has their uses and are valid, but mixing methods in a single routine makes it a little more confusing to read.

You haven't identified how the routine is called, and this makes a difference on the applicability of ActiveSheet. How can you guarantee that the active sheet is the one that you want to clean up?

You also reference your FirstRow and LastRow from a fixed sheet, not your ActiveSheet - how can you be sure that they are correct?

Speed up routine

You set your calculation mode and attempted to turn off screen updating. But you don't turn them back again at the end of the routine.

Also consider setting .EnableEvents = False so that you do not fire events every time you delete a row.

Tweaks

Consider using a Select Case to identify a valid row. I have an example in the code below.

Consider creating a Union for each row that you find and want to delete. Then use this union to delete all the rows at once, instead of one at a time.

Put your date value into a variable, rather than accessing the cell each time in the loop. This will save some time and accessing the Excel cells/ranges is relatively expensive.

Perhaps this code?

Sub Remove_FutureRenewals()
    Dim Firstrow As Long
    Dim LastRow As Long
    Dim Lrow As Long
    Dim CalcMode As Long
    Dim ViewMode As Long
    Dim SelectedSheet as Worksheet
    Dim BaseDate as String

    Set SelectedSheet = ActiveSheet ' addresses any change to ActiveSheet while routine is running.
    BaseDate = Format(ThisWorkbook.Worksheets("Date Calc").Cells(5, "A").Value, "YYYYMMDD")

    Firstrow = SelectedSheet.UsedRange.Cells(1).Row
    LastRow = SelectedSheet.Cells(ThisWorkbook.Worksheets(2).Rows.Count, "A").End(xlUp).Row

    With Application
        CalcMode = .Calculation
        .Calculation = xlCalculationManual
        .ScreenUpdating = False
        .EnableEvents = False
    End With

    With SelectedSheet
        For Lrow = LastRow To Firstrow Step -1
            Select Case .Cells(Lrow, "E").Value ' always be explicit.
                Case "120", "90", "60" 'etc
                    'If Format(.Cells(Lrow, "E").Offset(0, -1).Value, "YYYYMMDD") <> BaseDate Then
                    If Format(.Cells(Lrow, "D").Value, "YYYYMMDD") <> BaseDate Then
                        .EntireRow.Delete
                    End If ' Better practice to use a full If-The-Endif block rather than a single line
            End Select
        Next Lrow
    End With
    With Application
        .Calculation = CalcMode
        .ScreenUpdating = True
        .EnableEvents = True
    End With

End Sub

This code still has a lot of "magic numbers" in it, but should give you some ideas on where to go next.


Convert IEnumerable to HTML table string My code can generate HTML table strings well, but it depends on JSON.NET.

I'm converting IEnumerable to an HTML table string using Json.NET but I think I shouldn't.

void Main()
{
    var datas = new[] { new {Name="Test"} };
    var array = datas.ToArray().ToHtmlTable(); //Run Success
    var set = datas.ToHashSet().ToHtmlTable();//Run Succes
    var list = datas.ToList().ToHtmlTable(); //Run Succes
    var enums = datas.AsEnumerable().ToHtmlTable(); //Run Succes
}

public static class HTMLTableHelper
{
    public static string ToHtmlTable(this IEnumerable enums)
    {
        return ToHtmlTableConverter(enums);
    }

    public static string ToHtmlTable(this System.Data.DataTable dataTable)
    {
        return ConvertDataTableToHTML(dataTable);
    }

    private static string ToHtmlTableConverter(object enums)
    {
        var jsonStr = JsonConvert.SerializeObject(enums);
        var data = JsonConvert.DeserializeObject<System.Data.DataTable>(jsonStr);
        var html = ConvertDataTableToHTML(data);
        return html;
    }

    private static string ConvertDataTableToHTML(System.Data.DataTable dt)
    {
        var html = new StringBuilder("
");

        //Header
        html.Append("

");
        for (int i = 0; i < dt.Columns.Count; i++)
            html.Append("" + dt.Columns[i].ColumnName + "	");
        html.Append("

");

        //Body
        html.Append("
");
        for (int i = 0; i < dt.Rows.Count; i++)
        {
            html.Append("
");
            for (int j = 0; j < dt.Columns.Count; j++)
                html.Append("" + dt.Rows[i][j].ToString() + "	");
            html.Append("
");
        }

        html.Append("
");
        html.Append("
");
        return html.ToString();
    }
}

I think it is bad because SerializeObject to string could be DeserializeObject to DataTable is a waste of efficiency.

var jsonStr = JsonConvert.SerializeObject(enums);
var data = JsonConvert.DeserializeObject<System.Data.DataTable>(jsonStr);

I want to remove dependencies on JSON.NET because it can reduce the size of the library.

Do you think I should replace them with something else if there is a better way?

I can use reflection to solve it:

void Main()
{
    var datas = new[] { new {Name="Test1",Value="Test2"} };
    var array = datas.ToArray().ToHtmlTable(); //Run Success
    var set = datas.ToHashSet().ToHtmlTable();//Run Succes
    var list = datas.ToList().ToHtmlTable(); //Run Succes
    var enums = datas.AsEnumerable().ToHtmlTable(); //Run Succes
}

public static class HTMLTableHelper
{
    public static string ToHtmlTable<T>(this IEnumerable<T> enums)
    {
        var type = typeof(T);
        var props = type.GetProperties();
        var html = new StringBuilder("
");

        //Header
        html.Append("

");
        foreach (var p in props)
            html.Append("" + p.Name + "	");
        html.Append("

");

        //Body
        html.Append("
");
        foreach (var e in enums)
        {
            html.Append("
");
            props.Select(s => s.GetValue(e)).ToList().ForEach(p => {
                html.Append("" + p + "	");
            });
            html.Append("
");
        }

        html.Append("
");
        html.Append("
");
        return html.ToString();
    }
}

Name	Value

Test1	Test2


Custom sorting algo / optimized bubble sort I'm a novice and started diving into data structures and algorithms recently. I was taught bubble sort recently and I found that the original bubble sort algorithm is so inefficient, so I wrote a modified, yet simple version of it.

Algorithm:

Loop through the provided array and store the index of the max element
Swap the last element with the max element
Decrease the total looping length by 1 and repeat

Code:

import java.util.Scanner;

class Methods extends Main
  {
    int[] getArray(int len)
      {
        int[] array = new int[len];

        for (int i = 0; i < array.length; i++)
          {
            array[i] = sc.nextInt();
          }
        return array;
      }

    int [] pushMaxSort(int[] arr)
      {
        int len = arr.length;

        while (len>0)
          {
            int max=arr[0];
            int index=0;
            for(int i=0; i<len;i++)
              {
                if(arr[i]>max)
                  {
                    max = arr[i];
                    index = i;
                  }
              }
            int temp = arr[len-1];
            arr[len-1] = arr[index];
            arr[index] = temp;
            len--;
          }

        return arr;
      }

    void printArr(int []arr)
      {
        System.out.println("sorted array :");
        for(int i=0;i<arr.length;i++)
          {
            System.out.print("\t" +arr[i]);
          }
      }

  }

class Main
  {
    static Scanner sc = new Scanner(System.in);

    public static void main(String[] args)
      {
        long t = System.currentTimeMillis();
        Methods o = new Methods();
        o.printArr(o.pushMaxSort(o.getArray(sc.nextInt())));
        long t2 = System.currentTimeMillis();
        System.out.println("\n\n time " + (t2-t));
      }
  }

Also, I need to learn to describe complexity (using Big O notation). I am vaguely guessing this has \$O(n \log{n})\$ but please let me know of the exact running time.

The design makes no sense in terms of Object-Oriented principles:

The inheritance relation between the two classes makes no logical sense.  It seems to serve the sole purpose of sharing the scanner instance.  Inheritance is more than mechanism for sharing variables.  It has application (aka business) meaning.  It defines "is kind of" relationship: a Dog is a kind of Animal.
The class names say absolutely nothing about what the code does. Well, one could argue that Main serves as the program's entry point.  I've seen this "naming convention" in actual java projects so that's passable.  I would call it something like ArraySorter.  It is also acceptable to add the main() method to the other methods instead of putting in a separate class.  Methods class is another matter: There has to be better name that describes what the collection of methods do.  In conclusion, it is up to you whether PushMaxSorter has its own main() method or keep it separate.
So, if we look at Methods as PushMaxSorter that is a class that implements your improved bubble sort algorithm, then getArray() can be regarded as a constructor.  and it should take a Scanner instance as argument.
So, from the above constructor, we deduce that PushMaxSorter holds the int array as internal data structure.  So the sort method should accept no arg and return void and if you wish you can add getArray() that is a "classic" getter method: allows access to the instance variable.
regarding printArr(): so we now can deduce that this method also accept no arg.  It is considered "best parctice" that a class with a data structure (or just a bunch of variables) will have a string representation of its internal state by overriding Object's toString() method.  This allows the caller to conrtol to which output the String is written.

Regarding complexity, I admit this is not my expertise. However, I do not see any  signs of exponential iteration on the data structure.  Your algorithm may improve upon the performance of classic bubble sort, but complexity is about scaling: comparing the performance of the same algorithm on input that is increasing in size. I'd say complexity here is the same as classic bubble sort, which is O(n^2)

Note: a more robust and extensible design will say that you need to decouple the data structure from the sorting algorithm (much like the Java Collections framework) so that you can apply your improved algorithm not only to array of integers.  But I think that is an "advanced" topic that requires deeper study of advanced OO principles.


Updating entities with EF (Delete, Add, Update) with one method I have a WCF service, which has a method for updating a EF entity. The update method works just fine but looks ugly. I think due to some EF design I have to manually set the change tracker state to Modified or Deleted or Added. The update method in the server side looks like this:

public Event UpdateEvent(Guid sessionID, Event _event)
{
    Session session = IFSHelperClass.CheckSession(sessionID);

    try
    {
         using (IFSDB ifsdb = new IFSDB())
         {
             ifsdb.Events.Attach(_event);
             ifsdb.ObjectStateManager.ChangeObjectState(_event, System.Data.EntityState.Modified);

             // mark the modified and added items
             // for the navigation property 'EventPersons'
             foreach (var p in _event.EventPersons)
             {
                if (p.ChangeTracker.State == IFSEntities.ObjectState.Added)
                    ifsdb.ObjectStateManager.ChangeObjectState(p, System.Data.EntityState.Added);

                 if (p.ChangeTracker.State == IFSEntities.ObjectState.Modified)
                    ifsdb.ObjectStateManager.ChangeObjectState(p, System.Data.EntityState.Modified);
             }

             // since the deleted items will not be available in the
             // navigation property, delete them manually from the
             // ChangeTracker object.
             if (_event.ChangeTracker.ObjectsRemovedFromCollectionProperties.Count > 0)
             {
                 var personsToDelete = _event.ChangeTracker.ObjectsRemovedFromCollectionProperties.Where(t => t.Key == "EventPersons").Select(t => t.Value)
                     .FirstOrDefault()
                     .ToList();

                foreach (EventPerson p in personsToDelete)
                {
                  ifsdb.EventPersons.Attach(p);
                  ifsdb.EventPersons.DeleteObject(p);
                }
            }

            // save all changes and accept them
            ifsdb.SaveChanges();
            _event.AcceptChanges();
            foreach (var p in _event.EventPersons)
                p.AcceptChanges();

            return _event;
          }
      }
      catch (Exception ex)
      {
          Logger.LogError("UpdateEvent()", ex.Message, ex.ToString(), session.User.UserID);
          throw ex;
      }
 }

As you can see, II iterate through the navigation property items to check for updates or deletes and mark them manually.

Is this the way? or am I doing something ugly here?

You shouldn't expose entities in WCF service. Every change in entity will cause WCF webservice change and incompatibility for existing consumers. I would create decicated data transfer objects for WCF webservice and use library like Automapper to handle property mapping.

EF entities often contain various properties (example: creation date, creator info), that shouldn't be modified, but when you expose whole entities, you allow this, which in future may cause security issues.

Small example (without Automapper):

public class EventDTO
{
    public int Id { get; set; }
    public DateTime EventDate { get; set; }
    public string Title { get; set; }
}

In webservice class:

public Event UpdateEvent(Guid sessionID, EventDTO eventDTO)
{
    using (IFSDB ifsdb = new IFSDB())
    {
        var event = ifsdb.Events.First(item => item.Id == eventDTO.Id);
        event.EventDate = eventDTO.EventDate;
        event.Title = eventDTO.Title;
        ifsdb.SaveChanges();
    }
}


Excel To CSV Converter Ch 9 ATBS I am learning more about python using the book "Automate The Boring Stuff". The problem I completed was "Excel To CSV Converter". I want to kick bad habits to the curb, and make sure I'm learning the best python practices to further my development. I would really appreciate feedback on code efficiency, and code neatness. Is my code PEP-8 compliant? Does it implement all the best practices possible? Thank you in advance, all feedback is appreciated and considered!

import openpyxl, csv, os, re, shutil

path = os.getcwd()
new_path = path + '\\' + 'csv_files'

for excel in os.listdir('.'):

    if not excel.endswith('xlsx'):
        continue

    workbook = openpyxl.load_workbook(excel)

    for sheets in workbook.sheetnames:
        wb_name = re.sub('.xlsx', '', excel)
        csv_name = wb_name + '_' + sheets + '.csv'
        csv_file = open(csv_name, 'w', newline='')
        csv_writer = csv.writer(csv_file)
        sheet = workbook.active

        for row_number in range(1, sheet.max_row + 1):
            row_info = []

            for col_number in range(1, sheet.max_column + 1):
                data = sheet.cell(row=row_number, column=col_number).value
                row_info.append(data)

            csv_writer.writerow(row_info)

        csv_file.close()
        shutil.move(os.path.join(path, csv_name), os.path.join(new_path, csv_name))

There are a few things you could improve. First, the most important if you want to write Python code that scales to more than a script that does just one thing, you should write functions (or classes). They have the nice feature that you can give them a name, which tells you roughly what they do, and you can give them a docstring which can describe what they do in even more detail.

Bug:

While you do iterate over the sheet names, you do not actually change sheets. workbook.active does not automatically switch to another name. Instead, directly iterate over the workbook.

Otherwise here are a few suggestions:

Instead of using os.listdir, you can use pathlib.Path, which has a glob method, to get all excel files in the current directory:

from pathlib import Path

excel_files = Path(".").glob("*.xlsx")

This way you also don't have to deal with different path delimiters in Unix and Windows and manually getting the current directory.

Python can also work with relative paths, so no need to get the current directory at all.
You should use the with keyword to ensure that the files you open are closed, even in the event of an exception.
openpyxl sheets have the attributes rows and columns, which lets you easily iterate over them. You should also learn about list comprehensions. In addition, a csv.writer has the writerows method that can take an iterable of rows to write.
Using regex just to replace a single string with nothing is overkill. You could just use str.replace. But here building the string constructively is probably easier. Learn about f-strings in Python 3.6+.
Instead of moving the file afterwards with shutils, directly save it to the right file.
Use a if __ name__ == "__main__": guard to allow importing from this script from another script.

With these changes your code could become:

import csv
import openpyxl
from pathlib import Path

def convert_all_sheets_to_csv(file_name, target_dir="."):
    """Convert all sheets to csv files saved in the folder `target_dir`."""
    workbook = openpyxl.load_workbook(file_name)
    out_file_name_template = Path(target_dir) / Path(file_name).stem
    for sheet in workbook:
        out_file_name = f"{out_file_name_template}_{sheet.title}.csv"
        convert_sheet_to_csv(sheet, out_file_name)

def convert_sheet_to_csv(sheet, file_name):
    """Convert the content of an excel sheet to a csv file."""
    with open(file_name, "w") as f:
        writer = csv.writer(f)
        writer.writerows([cell.value for cell in row] for row in sheet.rows)

if __name__ == "__main__":
    for excel_file in Path(".").glob("*.xlsx"):
        convert_all_sheets_to_csv(excel_file, target_dir="csv_files")


Creating an array of associative arrays in PHP I am fetching rows from the DB and returning the result in JSON. The output needs to be an array of associative arrays.

$i = 0;
foreach($resultSet as $r)
{
  $output[$i]['key1'] = $r['col1'];
  $output[$i]['key2'] = $r['col2'];
  ...
  $output[$i]['keyN'] = $r['colN'];
  $i++;
}

Is there a more elegant way to do this? Is there anything wrong with this approach? If yes, what is it?

Instead of declaring $i=0; before the loop, you can also write :

foreach($resultSet as $i => $r)
{
  $output[$i]['key1'] = $r['col1'];
  $output[$i]['key2'] = $r['col2'];
  ...
  $output[$i]['keyN'] = $r['colN'];
}

Also, if you could rename your column in the DB query, you could have :

foreach($resultSet as $i => $r)
{
  $output[$i] = $r;
}

Which is pretty much like doing $output = $resultSet;

Edit :
If you do need to do some processing on $resultSet for whatever reason and you'd rather have different names, you can consider doing :

for (array('key1' => 'col1', ... 'keyN' => 'colN' as key => col)
{
   $output[$i][$key] = $r[$col];
}


CodeWars "Dbftbs Djqifs" kata code I have been working on the following kata from CodeWars. It's basically a Caesar Cipher — see the test cases below my solution code.

Any comments/feedback appreciated!

def encryptor(key, message):
    new = ""
    for letter in message:
        if letter.isalpha():
            id = ord(letter)
            shift = id+key
            if 97 <= id <= 122:
                while shift>122:
                    shift -= 26
                while shift<97:
                    shift += 26
                letter = chr(shift)
            if 65 <= id <= 90:
                while shift>90:
                    shift -= 26
                while shift<65:
                    shift += 26
                letter = chr(shift)
        new = new + letter
    return new

And here are test cases that give you an idea of what is being looked for:

test.assert_equals(encryptor(13, ''), '')
test.assert_equals(encryptor(13, 'Caesar Cipher'), 'Pnrfne Pvcure')
test.assert_equals(encryptor(-5, 'Hello World!'), 'Czggj Rjmgy!')
test.assert_equals(encryptor(27, 'Whoopi Goldberg'), 'Xippqj Hpmecfsh')

There is not too much to be said about the code as it is. A few (very minor) things I can point out as suggestions:

Make if 97 <= id <= 122: followed by if 65 <= id <= 90: an if-elif clause. That makes it clearer the code path can only follow one of these (or even neither).
Dedent the bottom letter = chr(shift) by one indentation and remove the other one; avoids duplication
There is inconsistent white space around operators (sometimes one space each side, sometimes no space). I think PEP 8 recommends a single space on each side.
Some people will argue that you shouldn't do new = ""; ... ; new = new + letter. But instead, something like letters = []; ...; letters.append(letter); ...; new = "".join(letters).
That would gain you some speed. But only if your string becomes (very) long. For small strings, I think the current way (appending to a string) is clearer.
Just so you know.

I can, however, suggest to use str.maketrans and str.translate.
 You'll need to set up the translation table in a smart way (the algorithm), but the actually encoding is then very clear and straightforward. Left as an exercise to the reader.

Which leads to the fun fact: rot13 is available as a standard encoding.
In Python 2:

>>> 'Caesar Cipher'.encode('rot13')
'Pnrfne Pvcure'

In Python 3 is slight more effort:

>>> import codecs
>>> codecs.encode('Caesar Cipher', 'rot13')
'Pnrfne Pvcure'


Crawl site getting URL and status code I wrote a crawler that for every page visited collects the status code.
Below my solution. Is this code optimizable?

import urllib

def getfromurl(url):
    start = urllib.urlopen(url)
    raw = ''
    for lines in start.readlines():
        raw += lines
    start.close()
    return raw

def dumbwork(start_link, start_url, text, pattern, counter):
    if counter < 2:
        counter = counter +1
        while start_link != -1:
            try:
                start_url = text.find('/', start_link)
                end_url = text.find('"', start_url + 1)
                url = 'http:/' + text[start_url + 1 : end_url]
                page_status = str(urllib.urlopen(url).getcode())
                row = url + ', ' + page_status
                t.write(row + '\n')
                temp = str(getfromurl(url))
                print row
                dumbwork(temp.find(pattern), 0, temp, pattern, counter)
                start_link = text.find(pattern, end_url + 1)
            except Exception, e:
                break
    else:
        pass

t = open('inout.txt', 'w')
text = str(getfromurl('http://www.site.it'))
pattern = '<a href="http:/'
start_link = text.find(pattern)
dumbwork(start_link, 0, text, pattern, 0)
t.close()

You're taking for granted that a link will be '<a href="http:/', which is definitely not always the case. What about https:// for example, or if you have something like '<a class="visited" href="http:/' ? That's why you should use a library to parse the DOM objects instead of relying on raw text parsing.
Naming:

usually a row is related to a database, while a line is related to a text file.
temp means nothing, it's the new content, so you should use something like new_html_content.
It takes a bit to understand that counter is actually the max depth that you want to follow, so why not call it depth
Function names should explain what they do, dumbwork name doesn't, something like recurse_page may be better.
start_link is good for the first link (almost, see below) but the parameter to the function is actually the current link being parsed, so why not call it current_link?
You used snake case for start_link, you should keep using it, so get_from_url may be better.
start_link, start_url and end_url are not links or urls, they're actually the index of the string, so they should be start_link_index, start_url_index and end_url_index
text is the HTML content, so just rename it to html_content

The lines doing something with row should be next to each other, or better yet, in a separate function.
That 2 should be in a constant so that the first line of the function can be something like if depth < MAX_DEPTH:
You're trapping exceptions but you're not doing anything with them, you should at least log somewhere what happened.
The text.find to get the url are probably better off in a separate function, to improve readability, something like
getfromurl already returns a string, no need for the str()
You're using always the same name for the file which, when opened with w will overwrite the contents. You should at least check if the file already exists.
You're opening a file and leaving it open for the whole duration of the process. This is not bad in itself, but I'd probably put a function called append_to_file where I open the file with a instead of w, write the line and immediately close it. Inside of that function you will also convert the status code to a string.

In the end, your worker loop may look something like this:

def recurse_page(current_link_index, start_index, html_content, pattern, depth):
    if depth < MAX_DEPTH:
        depth += 1
        while current_link_index > -1:
            try:
                url = get_url(html_content, start_index)
                append_to_file(url, urllib.urlopen(url).getcode())
                new_html_content = get_from_url(url)
                recurse_page(new_html_content.find(pattern), 0, new_html_content, pattern, depth)
                current_link_index = html_content.find(pattern, end_url_index + 1)
            except Exception, e:
                # TODO: Proper error handling
                break

It's not complete code, but it should give you an idea of what I mean.


Count patterns that start and end with "1", with 0's in between A string contains many patterns of the form 1(0+)1 where (0+) represents any non-empty consecutive sequence of 0's. The patterns are allowed to overlap.

For example, consider string "1101001", we can see there are two consecutive sequences "1(0)1" and "1(00)1" which are of the form 1(0+)1.

public class Solution {

        static int patternCount(String s){
            String[] sArray = s.split("1");
            for(String str : sArray) {
                if(Pattern.matches("[0]+", str)) {
                    count++;
                }
            }
            return count;
        }

        public static void main(String[] args) {
            int result = patternCount("1001010001");
            System.out.println(result);//3
        }
    }

Sample Input:

100001abc101, 1001ab010abc01001, 1001010001

Sample Output:

2 2 3

But still something i feel might fail in future could you please help me to optimize my code as per the Requirement

Although the other 2 answers currently given are technically correct. I find it more logical to search for a 1 followed by any number of 0's ending with another 1 which we cannot consume yet (as it can be used again in the following match).

So the pattern should look like 1[0]+(?=1)

Which turns the method implementation to this:

static int patternCount(String s) {
    Pattern pattern = Pattern.compile("1[0]+(?=1)");
    Matcher matcher = pattern.matcher(s);
    int count = 0;
    while (matcher.find()) {
        count++;
    }
    return count;
}


Java console calculator package calc;

import com.sun.tools.corba.se.idl.constExpr.Equal;    //imports .equals(variable)
import java.util.Scanner;    //imports scanners

public class Calc {

    public static void main(String[] args) {
        boolean go = true;    //sets up loop

        while(go)    //creates loop to top
        {
            System.out.println("Hello this is my calculator!");
            System.out.println("To add, type a, to subtract, type s.");
            System.out.println("To multiply, type m, to divide, type d.");

            Scanner scan = new Scanner(System.in);    //sets up scanners
            Scanner scan1 = new Scanner(System.in);

            String action = scan.nextLine();    //tells comp. to take user input

            if("a".equals(action))    //addition
            {
                System.out.println("Now type in the first number you would like to add.");
                int add1 = scan.nextInt();
                System.out.println("Now type the second number.");
                int add2 = scan.nextInt();
                int add3 = add1 + add2;
                System.out.println(add1 + " added to " + add2 + " equals " + add3 + "!");
            }
            if("s".equals(action))    //subtraction
            {
                System.out.println("Now type in the first number you would like to subtract.");
                int sub1 = scan.nextInt();
                System.out.println("Now type the second number.");
                int sub2 = scan.nextInt();
                int sub3 = sub1 - sub2;
                System.out.println(sub1 + " subtracted bye " + sub2 + " equals " + sub3 + "!");
            }
            if("m".equals(action))    //multiplacation
            {
                System.out.println("Now type in the first number you would like to multiply.");
                int mul1 = scan.nextInt();
                System.out.println("Now type the second number.");
                int mul2 = scan.nextInt();
                int mul3 = mul1 * mul2;
                System.out.println(mul1 + " multiplied bye " + mul2 + " equals " + mul3 + "!");
            }
            if("d".equals(action))    //division
            {
                System.out.println("Now type in the first number you would like to divide.");
                int div1 = scan.nextInt();
                System.out.println("Now type the second number.");
                int div2 = scan.nextInt();
                int div3 = div1 / div2;
                System.out.println(div1 + " divided bye " + div2 + " equals " + div3 + "!");
            }

            System.out.println("Would you like to start over? (yes,no)");
            String startOver = scan1.nextLine();

            if("no".equals(startOver))
            {
                go = false;
                System.out.println("Bye");
            }
        }
    }
}

Would it be possible to make this more compact?
Does this code contain dead code?

You do "the same" for all four choices - time to abstract!

import java.util.Scanner;    //imports scanners

public class Calc {

    enum Op {

        ADD("a", "add", "added to"),
        SUB("s", "subtract", "subtracted by"),
        MUL("m", "multiply", "multiplied by"),
        DIV("d", "divide", "divided by");
        public String key;
        public String command;
        public String result;

        Op(String key, String command, String result) {
            this.key = key;
            this.command = command;
            this.result = result;
        }

        public int eval(int x, int y) {
            switch (this) {
                case ADD:
                    return x + y;
                case SUB:
                    return x - y;
                case MUL:
                    return x * y;
                case DIV:
                    return x / y;
                default:
                    throw new AssertionError();
            }
        }
    }

    public static void main(String[] args) {
        while (true) //creates loop to top
        {
            System.out.println("Hello this is my calculator!");
            System.out.println("To add, type a, to subtract, type s.");
            System.out.println("To multiply, type m, to divide, type d.");

            Scanner scan = new Scanner(System.in);    //sets up scanners
            Scanner scan1 = new Scanner(System.in);

            String action = scan.nextLine();    //tells comp. to take user input

            Op op = null;
            for (Op operation : Op.values()) {
                if (operation.key.equals(action)) {
                    op = operation;
                    break;
                }
            }

            if (op != null) {
                System.out.println("Now type in the first number you would like to " + op.command + ".");
                int x = scan.nextInt();
                System.out.println("Now type the second number.");
                int y = scan.nextInt();
                int z = op.eval(x, y);
                System.out.println(x + " " + op.result + " " + x + " equals " + z + "!");
            }

            System.out.println("Would you like to start over? (yes,no)");
            String startOver = scan1.nextLine();

            if ("no".equals(startOver)) {
                System.out.println("Bye");
                return;
            }
        }
    }
}

So all operation specific information is captured in the Op enums. If you want to add other operations like min, max, and, or, xor or mod, you don't have to introduce new if-blocks, just new enums.


In-Place Quicksort in Python Code is below. I'd like to know if this is a solid/efficient implementation of the in-place quicksort algorithm and if my Python style is good (I know there are no comments for now). Code is also on Github here if you'd like: https://github.com/michaelrbock/data-structs-and-algos/blob/master/Sorts/quicksort.py Thanks!

def quicksort(lst):
    if len(lst) <= 1:
        return lst
    lst, store_index = partition(lst)
    return quicksort(lst[:store_index-1]) + [lst[store_index-1]] + quicksort(lst[store_index:])

def partition(lst):
    if len(lst) % 2 == 0:
        middle = (len(lst) / 2) - 1
    else:
        middle = len(lst) / 2

    pivot_choice = get_median( [lst[0], lst[middle], lst[len(lst)-1]] )

    if pivot_choice == lst[0]:
        PIVOT_INDEX = 0
    elif pivot_choice == lst[middle]:
        PIVOT_INDEX = middle
    elif pivot_choice == lst[len(lst)-1]:
        PIVOT_INDEX = len(lst) - 1

    pivot = lst[PIVOT_INDEX]
    lst[0], lst[PIVOT_INDEX] = lst[PIVOT_INDEX], lst[0]
    i = 1
    for j in range(1, len(lst)):
        if lst[j] < pivot:
            lst[j], lst[i] = lst[i], lst[j]
            i += 1
    lst[0], lst[i-1] = lst[i-1], lst[0]
    return lst, i

def get_median(nums):
    values = sorted(nums)
    if len(values) % 2 == 1:
        return values[(len(values)+1)/2-1]
    else:
        lower = values[len(values)/2-1]
        upper = values[len(values)/2]
    return (lower+upper)/2

Personally I like the style (especially the nice clear function and variable names). As its quicksort most of the code is straightforward enough that comments would probably only add meaning when it comes to the swapping. You should probably try and use four spaces, rather than tabs, for indenting the code (it's just common practice). The only minor stylistic change I would personally make is to uncapitalise PIVOT_INDEX. I know technically it is constant (in that it doesn't change value after it's assigned), but I just prefer to have one clear assignment for any constant values. However, I'm sure other people would disagree.

As for efficiency, you're always going to be bounded by the algorithm itsef, but there are a couple changes you could make. You calculate len(lst) multiple times in partition. Rather than doing that, you could just assign it like list_length = len(lst) once at the start of the partition function. Not a big deal I know. The only other meaningful change I can think of is in your get_median function. As you know that it's always called with three values you could change it to:

def get_median(nums):
    values = sorted(nums)
    return values[1]

If you really want to be fancy you can replace the get_median and the whole if-then-else construct that determines the PIVOT_INDEX with something like:

PIVOT_INDEX = sorted(zip([lst[0], lst[middle], lst[list_length - 1]], [0, middle, list_length]))[1][1]

which will give you the index (0, middle or list_length) of the median value in the same way.


Very basic isomorphic JavaScript application I'm trying to update my skillset by learning how to write isomorphic JavaScript applications.  For my stack, I've chosen React and Express - no database yet.

The problem many people seem to face when it comes to writing these kinds of applications is getting data from the server to the client so the view can be rendered properly client-side.  Apparently the best way to do this is to JSON encode the data and include that in the server-rendered page.

All of this is done and working, but I would really appreciate if somebody who knows what they're doing could weigh in on my techniques before I develop bad habits.

A working repository (very small) is available on GitHub, but I have included the relevant concerning code below.

To get the data to the client, I pass it down through React until it hits the base template, where it is encoded and included in an invisible element:

return this.isServer ? (

            <title>{ this.props.title || "Untitled" }</title>
            <link rel="stylesheet" href="/static/theme/style.scss"/>

            {
                //If data is to be passed into the view, encode it as JSON and insert it into a hidden element
                this.props.data &&
                    <x-data>
                        { new Buffer(JSON.stringify(this.props.data)).toString("base64") }
                    </x-data>
            }

My Files

                { this.props.children }

) : this.props.children;

The application is rendered into the  element on the server, and if this code is running on the client, it is handled elsewhere, in my client entrypoint:

import App from "./views/index.jsx";

//Get data from server, which is passed down through the x-data element
//It is base64 encoded JSON
let props = {};
let xData = document.body.querySelector("x-data");
if (xData) {
    let json = JSON.parse(window.atob(xData.innerText));
    Object.assign(props, json);
}

//All pages will export an App class which renders the current page
ReactDOM.render(
    React.createElement(App, props),
    document.body.querySelector("main")
);

The App class is the base React component to render this specific page.  I'm not sure how I'll work out having multiple pages in the future, but this is how I have it working now.

Am I following a proper design pattern, or have I invented some mutant hellspawn technique that should be vanquished with a quick rm -rf?

You can render your webpage on the server complete with all props/state in place and at client-side React will pick up from initial state when he founds markup already existing.

So write your components the normal way, query database (or what have you) for props and render on server using ReactDOMServer. Here's a little example.

(As far as I can tell, you're doing something similar in your repo, so this is probably late to the party :)


Why does my implementation of the Sieve of Eratosthenes take so long? def sieve_of_erastothenes(number):
    """Constructs all primes below the number"""
    big_list = [2] + range(3, number+1, 2)  # ignore even numbers except 2
    i = 1  # start at n=3, we've already done the even numbers
    while True:
        try:
            # use the next number that has not been removed already
            n = big_list[i]
        except IndexError:  # if we've reached the end of big_list we're done here
            return big_list
        # keep only numbers that are smaller than n or not multiples of n
        big_list = [m for m in big_list if m<=n or m%n != 0]
        i += 1

On my computer, this code takes about half a second to calculate all primes below 10,000, fifteen seconds to do 100,000, and a very long time to calculate primes up to one million. I'm pretty sure it's correct (the first few numbers in the returned sequence are right), so what can I do to optimise it?

I've tried a version that removes the relevant numbers in-place but that takes even longer (since it takes \$O(n)\$ to move all the remaining numbers down).

The problem is that the way this is set up, the time complexity is (close to) \$O(n**2)\$. For each remaining prime, you run over the entire list.

In the last case, where you want to check for prime numbers up to 1,000,000 you end up iterating over your list 78,498 ** 2 times. 6,161,936,004 iterations is a lot to process.

Below is an approach that crosses off the numbers instead, editing the large list of numbers in-place. This makes for a \$O(n)\$ solution, which times as such:

range 10,000 : 549 usec per loop
range 100,000 : 14.8 msec per loop
range 1,000,000: 269 msec per loop
range 10,000,000: 3.26 sec per loop

def sieve_of_erastothenes(limit):
    """Create a list of all numbers up to `number`, mark all non-primes 0,
    leave all the primes in place. Filter out non-primes in the last step.
    """
    sieve = range(limit + 1)

    # There are no prime factors larger than sqrt(number):
    test_max = int(math.ceil(limit ** 0.5))

    # We know 1 is not a prime factor
    sieve[1] = 0

    # The next is a cheat which allows us to not even consider even numbers
    # in the main loop of the program. You can leave this out and replace the
    # current xrange(3, test_max, 2) with xrange(2, test_max)

    # All multiples of 2 are not prime numbers, except for 2 itself:
    sieve[4::2] = (limit / 2 - 1) * [0]

    for prime in xrange(3, test_max, 2):
        if sieve[prime]:
            # If the number hasn't been marked a composite, it is prime.
            # For each prime number, we now mark its multiples as non-prime.
            multiples_count = limit / prime - (prime - 1)
            sieve[prime * prime::prime] = multiples_count * [0]
    # Filter out all the zeroes, we are left with only the primes
    return filter(None, sieve)


SPOJ FASHION challenge I just started attempting SPOJ problems. And I'm aware of Java using more memory and time compared to most of the other languages. But even some of the solutions in Java have better results than my accepted code. And it hurts!

Here is my accepted code for FASHION with TIME: 0.41 and MEM: 4284M.The best solution in Java for the same question has TIME: 0.04 and MEM: 1398M.

import java.util.Arrays;
import java.util.Scanner;

public class Main {

    public static void main(String[] args) {
        int i = 0;
        Scanner sc = new Scanner(System.in);
        int testCases = sc.nextInt();
        while (testCases > 0) {
            int modelsWithMe = sc.nextInt();
            int[] men = new int[modelsWithMe];
            int[] women = new int[modelsWithMe];
            int hotnessSum = 0;
            for (i = 0; i < modelsWithMe; i++) {
                men[i] = sc.nextInt();
            }
            for (i = 0; i < modelsWithMe; i++) {
                women[i] = sc.nextInt();
            }
            Arrays.sort(men);
            Arrays.sort(women);
            for (i = 0; i < modelsWithMe; i++) {
                hotnessSum = hotnessSum + men[i] * women[i];
            }
            System.out.println(hotnessSum);
            testCases--;
        }

    }
}

I did see a few questions on performance, such as this and this. I am doing a few of it and some are not applicable for this solution's point of view. Here, maybe, I can read the entire line at once and split the string instead of calling nextInt each time and reduce I/O operations. What else can I do to reach the level of efficiency and speed as that of top answers? Best practices may be?

you can abuse the fact that

  All hotness ratings are (integers) on a scale of 0 to 10

Just keep track of how many men/women there are of each score. This looks something like this:

int[] men = new int[11];
for (i = 0; i < modelsWithMe; i++) {
    men[sc.nextInt()]++;
}

Then for the score you need to figure out how to combine the pairs given those frequency arrays.

int m = 10;
int w = 10;
while(m >= 0){
    while(men[m] == 0) {
        m--;
    }
    while(women[w] == 0){
        w--;
    }
    hotnessSum += m*w;
    men[m]--;
    women[w]--;
}

This calculation can still be optimised greatly. If you have 21 man score 10 and 30 women score 10 for example, you could add 21*10*10 to the total and reduce the frequencies of both by 21 instead of looping 21 times to achieve the same total. I'll leave turning this into code to you.


Checking for vowels rewrite A few days ago I posted a little program that would check for vowels within a given word. I have rewritten this program, it can only find vowels in the English language still but, it will now tell you each vowel that is contained in your word, along with a count of how many vowels there are total, it's still not very exciting but I would like some more feedback on what I've done, anything will help:

using System;
using System.Linq;

namespace CheckForVowels
{
    class Vowels
    {
        static string say(string input)
        {
            Console.ForegroundColor = ConsoleColor.Green;
            Console.WriteLine(input);
            return input;
        }
        static string prompt(string input)
        {
            Console.ForegroundColor = ConsoleColor.Cyan;
            Console.Write($"{input}: ");
            return input;
        }
        static void Main(string[] args)
        {
            say("We will be checking your input to see if it contains vowels or not");
            prompt("Enter a word");

            var count = 0;
            var vowels = Console.ReadLine()
                .Where(c => "aeiouAEIOU".Contains(c))
                .Distinct();

            foreach (var vowel in vowels)
            {
                count += 1;
                say($"Your word contains vowel: {vowel}.");
            }

            say($"You have a total of {count} vowels in your word.");

                if (!vowels.Any())
                say($"Your word contains no vowels.");

        }

    }
}

You should adhere to the following:

Capitalize method names.
You don't need count variable.
Don't specify a return type, then not utilize it.

Point (1) and (3) could be rectified:

protected static void WriteToConsole(string input)
{
     Console.ForegroundColor = ConsoleColor.Cyan;
     Console.WriteLine(input);
}

The application code could then be:

var vowels = Console.ReadLine().Where(c => "aeiouAEIOU".Contains(c));

if(vowels.Any())
{
     foreach(var vowel in vowels.Distinct())
         WriteToConsole(String.Format("Your sentence contains the following vowel {0}", vowel);

    WriteToConsole(String.Format("Your sentence contains {0} vowels", vowels.Count());
}
else
{
     WriteToConsole("Your sentence doesn't have any vowels.");
}

Also to point out, in the other question to avoid duplicate entries an uneeded iterations of repeated vowels, I called .Distinct.  This alleviated that, but that would throw off your official count of how many vowels exist within the sentence.  So I call Distinct on the loop rather than the initial input.

Also to avoid an unneeded count variable, I call .Count() on the IEnumerable, the feature exist so why not use it?