Finding optimal mapping patterns that transform relational data into a linear format I'm trying to upgrade my javascript to be more functional. I have used function composition and curry successful on less complicated code but i'm now running into a point where i'm not sure on how to approach it.

Given the below relational data structures.

// Contains all node types and its property definitions. Including any additional information like readable descriptions etc..

const nodes = [{
  id: '0ec7deff-91af-e911-812a-00155d0a5146',
  description: 'Airplanes',
  properties: [{
    id: '0fc7deff-91af-e911-812a-00155d0a5146',
    description: 'Weight'
  }, {
    id: '06c7deff-91af-e911-812a-00155d0a5146',
    description: 'Color'
  }]
}, {
  id: '278182d0-4ba2-4813-b74e-277a2296e864',
  description: 'Manufacturers',
  properties: [{
    id: '003e14d7-c11a-41e1-ad08-ab8896a3cb55',
    description: 'Boeing'
  }]
}]

// Contains only the to be used nodes and there relations(defined through children nodes) in the eventual output. The above definitions can be used to pull in additional data.

const rootNode = {
  id: '0ec7deff-91af-e911-812a-00155d0a5146',
  properties: [{
   id: '0fc7deff-91af-e911-812a-00155d0a5146',
  }],
  children: [{
    id: '278182d0-4ba2-4813-b74e-277a2296e864',
    properties: [{
      id: '003e14d7-c11a-41e1-ad08-ab8896a3cb55',
    }]
  }],
}

The rootNode is the starting point. It can contain an infinite amount of children including properties that need to be displayed. Each node needs to be checked for child nodes and be added to the eventual result. For example a linear set to be displayed on the x axis where the properties will be shown on a next row also on the x axis.

[{
  id: '0ec7deff-91af-e911-812a-00155d0a5146',
  description: 'Airplanes',
  properties: [{
    id: '0fc7deff-91af-e911-812a-00155d0a5146',
    description: 'Weight'
  }]
 }, {
   id: '278182d0-4ba2-4813-b74e-277a2296e864',
   description: 'Manufacturers',
   properties: [{
     id: '003e14d7-c11a-41e1-ad08-ab8896a3cb55',
     description: 'Boeing'
   }]
 }]

The eventual output will be shown in a 2 dimensional grid or table.

Airplanes     Manufacturers
Weight        Boeing

Or in a more elaborate example if i would provide more data and a larger query.

Airplanes                       Manufacturers
Weight    Color    Type         Boeing    Lockhead

I build a function that works and uses recursion but it just doesn't feel right. I want to pull it apart into smaller units. Note that lodash is used here, hence the shorthands etc.

function initializeNodeMapper(nodes) {
  const result = []

  return function mapNodes(node) {
    const usedPropertyIds = map(node.properties, 'id')
    const currentNode = find(nodes, ['id', node.id])
    const nodeWithReducedProperties = reduce(currentNode, (acc, value) => ({
        ...acc,
        properties: filter(value, property => indexOf(usedPropertyIds, property.id) !== -1),
    }), currentNode)

    result.push(nodeWithReducedProperties)

    if (node.children && node.children.length > 0) {
      forEach(node.children, mapNodes)
    }

    return result
  }
}

const mapData = initializeNodeMapper(nodes)
const mappedData = mapData(initialNode)

Using functional patterns i want to find a more optimal pattern of mapping and testing the code.

Some slight improvements:

You could do without the result array (less state ).
I think using vanilla object destructuring is more clear than the lowdash reduce method which is confusing to me (unless I still don't understand it and my code actually is not equivalent to yours ).

const allNodes = [
];

const rootNode = {
};

function initializeNodeMapper(nodes) {
  return function mapNodes(node) {
    const propertyIds = node.properties.map(({ id }) => id);
    const currentNode = nodes.find(({ id }) => id === node.id);
    const nodeWithFilteredProperties = {
      ...currentNode,
      properties: currentNode.properties.filter(({ id }) =>
        propertyIds.includes(id)
      )
    };

    return node.children
      ? [nodeWithFilteredProperties].concat(node.children.map(mapNodes))
      : [nodeWithFilteredProperties];
  };
}

const mappedData = initializeNodeMapper(allNodes)(rootNode);

Update:

Are you looking for something like this?

const nodes = [
];

const rootNode = {
};

const pipe = (...fns) => x => fns.reduce((v, f) => f(v), x);

const getFlattenedNodes = node =>
  node.children
    ? [node].concat(
        node.children.reduce(
          (acc, val) => acc.concat(getFlattenedNodes(val)),
          []
        )
      )
    : [node];

const getPropertyIds = node => node.properties.map(({ id }) => id);

const getNodeWithProperties = nodesWithProperties => node =>
  nodesWithProperties.find(({ id }) => id === node.id);

const getNodesWithFilteredProperties = propertyIdsAndNodesToBeFiltered =>
  propertyIdsAndNodesToBeFiltered.map(([propertyIds, nodeWithProperties]) => ({
    ...nodeWithProperties,
    properties: nodeWithProperties.properties.filter(({ id }) =>
      propertyIds.includes(id)
    )
  }));

const getPropertyIdsAndNodes = nodesWithProperties => flattenNodes =>
  flattenNodes.map(node => [
    getPropertyIds(node),
    getNodeWithProperties(nodesWithProperties)(node)
  ]);

const mappedData = pipe(
  getFlattenedNodes,
  getPropertyIdsAndNodes(nodes),
  getNodesWithFilteredProperties
)(rootNode);


Doubly linked std::list simple implementation I've implemented a simple doubly linked list structure. I would appreciate all criticism relevant to code, style, flow, and so forth.

How to implement end iterator and pop_back/pop_front properly? Where memory may leak?

P.S.I didn't make it template, because this is simple version version.

#include <cstddef>
#include <iterator>

using T = int;

class List {
/// Nodes
public:
    class Node {
    public:
        T data;
        Node *next;
        Node *prev;

    public:
        explicit Node(Node *next_, Node *prev_) : next(next_), prev(prev_) {}

        explicit Node(const T &_data, Node *next_, Node *prev_) :
                data(_data), next(next_), prev(prev_) {}

        ~Node() {}
    };
///

/// Iterators
public:
    class Iterator : public std::iterator<std::bidirectional_iterator_tag, T> {
    public:
        Node *position;
        Node *tail;

    public:
        Iterator(Node *node) : position(node) {}

        Iterator(Node *node, Node *_tail) : position(node), tail(_tail) {
        }

        Iterator &operator++() noexcept {
            position = position->next;
            return *this;
        }

        Iterator operator++(int) noexcept {
            auto old = *this;
            position = position->next;
            return old;
        }

        Iterator &operator--() noexcept {
            if (position == nullptr) position = tail;
            else
                position = position->prev;
            return *this;
        }

        Iterator operator--(int) noexcept {
            auto old = *this;
            if (position == nullptr) position = tail;
            else
                position = position->prev;
            return old;
        }

        T &operator*() const noexcept {
            return (position->data);
        }

        bool operator!=(Iterator other) noexcept {
            return position != other.position;
        }

        bool operator==(Iterator other) noexcept {
            return position == other.position;
        }

        ~Iterator() {}
    };
///

/// list
private:
    Node *head = nullptr;
    Node *tail = nullptr;
    size_t size_ = 0;

public:
    List() {}

    void push_back(const T &_data) {
        Node *node = new Node(_data, nullptr, tail);
        if (tail != nullptr) {
            node->prev = tail;
            tail->next = node;
            tail = node;
        } else {
            head = tail = node;
        }
        ++size_;
    }

    void push_front(const T &_data) {
        Node *node = new Node(_data, head, nullptr);
        if (head != nullptr) {
            head->prev = node;
            node->next = head;
            head = node;
        } else {
            head = tail = node;
        }
        ++size_;
    }

    void pop_back() {
        tail = tail->prev;
        if (tail != nullptr) {
            tail->next = nullptr;
        }
        --size_;
    }

    void pop_front() {
        head = head->next;
        if (head != nullptr) {
            head->prev = nullptr;
        }
        --size_;
    }

    size_t size() {
        return size_;
    }

    size_t size() const {
        return size_;
    }

    Iterator begin() {
        return Iterator(head, tail);
    }

    Iterator begin() const {
        return Iterator(head, tail);
    }

    Iterator end() {
        Iterator iter(nullptr, tail);
        return iter;
    }

    Iterator end() const {
        Iterator iter(nullptr, tail);
        return iter;
    }

    bool empty() const {
        return head == nullptr;
    }

    ~List() {
        Node * tmp = nullptr;
        while (head)
        {
            tmp = head;
            head = head->next;
            delete tmp;
        }
        head = tail = nullptr;
    }
};

Take care to be explicit about compiler generated functions

Effectively your List class violates the Rule of Three/Five/Zero, thus the compiler generated copy/move constructors will create unexpected side effects by implementing a shallow copy mechanism.

Let's suppose you have code like

int main() {
    List l1;
    List l2;

    l1.push_back(5);
    l1.push_back(42);
    l1.push_back(21);

    l2 = l1; // Copy the list
    *l1.begin() = 80; // Change the original list

    for(auto value : l2) {
        std::cout << value << std::endl;
    }
}

The output is

80
42
21

I doubt you really intended that. See the Live Demo please.

Why not making your class a template?

using T = int;

looks strange.

Why not making the List class a template like

template<typename T>
class List {
    // ...
};

Compiles and works the same way here

Prefer smart pointers instead of raw pointers

You should prefer using smart pointers instead of managing new and delete yourself.

I have evolved the demo from above with changing the relevant parts

class Node;
class Node {
public:
    T data;
    std::shared_ptr<Node> next;
    std::shared_ptr<Node> prev;

public:
    explicit Node(std::shared_ptr<Node> next_, std::shared_ptr<Node> prev_) : next(next_), prev(prev_) {}

    explicit Node(const T &_data, std::shared_ptr<Node> next_, std::shared_ptr<Node> prev_) :
            data(_data), next(next_), prev(prev_) {}

    ~Node() {}
};

class Iterator : public std::iterator<std::bidirectional_iterator_tag, T> {
public:
    std::weak_ptr<Node> position;
    std::weak_ptr<Node> tail;

public:
    Iterator(std::shared_ptr<Node> node) : position(node) {}

    Iterator(std::shared_ptr<Node>node, std::shared_ptr<Node>_tail) : position(node), tail(_tail) {
    }

    Iterator &operator++() noexcept {
        position = position.lock()->next;
        return *this;
    }

    Iterator operator++(int) noexcept {
        auto old = *this;
        position = position->next;
        return old;
    }

    Iterator &operator--() noexcept {
        if (position == nullptr) position = tail;
        else
            position = position->prev;
        return *this;
    }

    Iterator operator--(int) noexcept {
        auto old = *this;
        if (position == nullptr) position = tail;
        else
            position = position->prev;
        return old;
    }

    T &operator*() const noexcept {
        return (position.lock()->data);
    }

    bool operator!=(Iterator other) noexcept {
        return position.lock().get() != other.position.lock().get();
    }

    bool operator==(Iterator other) noexcept {
        return position.lock().get() == other.position.lock().get();
    }

    ~Iterator() {}
};

Solve the shallow copy problem

As mentioned in my 1st point you have a problem with The Rule of Three/Five. Your List class will be shallow copied. So you should provide the relevant operations.
This will be a lot easier using smart pointers as mentioned in my previous point

List(const List<T>& rhs) {
    copy(rhs);
}

List(List<T>&& rhs) {
    move(std::move(rhs));
}

List& operator=(const List<T>& rhs) {
    copy(rhs);
    return *this;
}

List& operator=(List<T>&& rhs) {
    move(std::move(rhs));
    return *this;
}

void copy(const List<T>& rhs) {
    clear();
    for(auto value : rhs) {
        push_back(value);
    }
}

void move(List<T>&& rhs) {
    head = std::move(rhs.head);
    tail = std::move(rhs.tail);
}

void clear() {
    std::shared_ptr<Node> temp = head;
    std::shared_ptr<Node> next;
    do {
        if(temp) {
            next = temp->next;
        }
        temp = nullptr;
    } while(next);
}

~List() {
}

Output is as expected for the example mentioned in the 1st paragraph:

5
42
21

Again you can check the improved code here.


Project Euler Problem 6: Sum square difference Continuing to work my way through some of of Project Euler. Problem 6 solved by my code below. Is it better to use sumOfTheSquares += i*i or utilize Math.Pow()?

  The sum of the squares of the first ten natural numbers is,
  \$1^2 + 2^2 + ... + 10^2 = 385\$

  The square of the sum of the first ten natural numbers is,
  \$(1 + 2 + ... + 10)^2 = 55^2 = 3025\$

  Hence the difference between the sum of the squares of the first ten natural numbers and the square of the sum is 3025 − 385 = 2640.

  Find the difference between the sum of the squares of the first one hundred natural numbers and the square of the sum.

class Program
{
    static void Main(string[] args)
    {
        Console.WriteLine(SumSquareDifference(100));
        Console.ReadLine();
    }

    static int SumSquareDifference(int upperValue)
    {
        int sumOfTheSquares = 0;
        for (int i = 1; i <= upperValue; i++)
        {
            sumOfTheSquares += (int)Math.Pow(i,2); //Can't formulate this myself...
        }

        int squareOfTheSums =  (int)Math.Pow((upperValue + 1) * (upperValue / 2),2);

        return squareOfTheSums - sumOfTheSquares;
    }
}

I don't know about the difference between sumOfTheSquares += i*i and Math.pow() but the sum of squared of first n natural numbers is as follows

\$ 1^2 + 2^2 + 3^2 + 4^2 = \dfrac{n(n+1)(2n+1)}{6}\$

So its faster than using a for loop

Edit: General way to prove this is to use mathematical induction
Here's a link!


Check if nullable int has value and compare value to another integer Question might be more appropriate in StackOverflow, however I'd also like a review.

I'd like to make my code inline as I feel like this could be possible, I'm just not sure how.

int highestWeightOfParcel  = 0;
if (collo.WeightGrammes.HasValue)
{
    if (collo.WeightGrammes > highestWeightOfParcel)
    {
        highestWeightOfParcel = collo.WeightGrammes.Value;
    }
}

Could I apply the same technique as this? :

int heigth = collo.HeightMm.HasValue ? collo.HeightMm.Value < 10 ? 1 : (collo.HeightMm.Value / 10) : 0,

Is there a 'better'/simpler way of writing this?

There is really no need to check HasValue in this situation. It is sufficient to compare with highestWeightOfParcel because if collo.WeightGrammes is null the comparison is false for any value of highestWeightOfParcel:

int highestWeightOfParcel  = 0;

if (collo.WeightGrammes > highestWeightOfParcel)
{
    highestWeightOfParcel = collo.WeightGrammes.Value;
}


Project Euler problem 1 in Factor I'm a complete beginner of Factor, but I just managed to solved Euler Problem #1 using it, and would love to have the code reviewed. Anything that can be improved? In particular I'm wondering if there is a cleaner or more idiomatic way to write the mult3or5? word.

USING: math kernel sequences math.ranges prettyprint ;
IN: euler1

: mult? ( x y -- ? ) rem 0 = ;

: mult3? ( x -- ? ) 3 mult? ;
: mult5? ( x -- ? ) 5 mult? ;

: mult3or5? ( x -- ? ) dup mult3? swap mult5? or ;

: sumMultsOf3or5 ( seq -- n ) [ mult3or5? ] filter sum ;

: solveEuler1 ( -- ) 0 1000 (a,b) sumMultsOf3or5 . ;

dup f swap g is a common idiom, so Factor has bi.

: mult3or5? ( x -- ? ) [ mult3? ] [ mult5? ] bi or ;


Struct Serialization using Unsafe Field I want to use structs as a container for data packets for asynchronous networking in C#. Found out that you can create a union style struct without the need to mark the struct itself as unsafe--instead marking the field as unsafe.
Example:
[StructLayout(LayoutKind.Explicit, CharSet = CharSet.Ansi, Size = 8, Pack = 4)]
struct DataPacketStruct {
    private unsafe fixed byte bytes[8];
    // Publicly accessible fields.
    public byte header;
    public int size;

    public void Serialize(ref byte[] buffer, int startIndex) {
        unsafe {
            for (int i = 0; i < 8; i++) buffer[startIndex + i] = bytes[i];
        }
    }

    public void Deserialize(byte[] buffer) {
        unsafe {
            for (int i = 0; i < 8; i++) bytes[i] = buffer[i];
        }
    }

    public int SizeOf() { unsafe { return sizeof(DataPacketStruct); } }
}

I know I need array out of bounds checking--but apart from that. Potential downsides or undefined behaviors? Or is this usage valid? Also are there any potential performance concerns or any alternatives with similar performance?
Also an issue I can see right off the bat is not having compile-time numeric constants like in C++. Unfortunately have to hard code in the field offsets and structure size because of this: Size = 8 and byte[8].

There's a safe way to do that using System.Runtime.InteropServices.Marshal (link).
[StructLayout(LayoutKind.Sequential, Size = 8, Pack = 4)]
struct DataPacketStruct
{
    public byte Header { get; set; }
    public int Size { get; set; }

    public void Serialize(byte[] buffer, int startIndex)
    {
        int size = SizeOf();
        IntPtr ptr = Marshal.AllocHGlobal(size);
        Marshal.StructureToPtr(this, ptr, true);
        Marshal.Copy(ptr, buffer, startIndex, size);
        Marshal.FreeHGlobal(ptr);
    }

    public static DataPacketStruct Deserialize(byte[] buffer, int startIndex)
    {
        var result = new DataPacketStruct();
        int size = result.SizeOf();
        IntPtr ptr = Marshal.AllocHGlobal(size);
        Marshal.Copy(buffer, startIndex, ptr, size);
        result = (DataPacketStruct)Marshal.PtrToStructure(ptr, typeof(DataPacketStruct));
        Marshal.FreeHGlobal(ptr);
        return result;
    }

    public int SizeOf() => Marshal.SizeOf(this);
}

Also I used public properties and followed the Naming Guidelines in the example.
static void Main(string[] args)
{
    var data = new DataPacketStruct() { Header = byte.MaxValue, Size = int.MaxValue };
    byte[] a = new byte[8];
    data.Serialize(a, 0);
    Console.WriteLine(string.Join(" ", a.Select(x => x.ToString("X2"))));
    var data2 = DataPacketStruct.Deserialize(a, 0);
    Console.WriteLine(JsonSerializer.Serialize(data2, new JsonSerializerOptions { WriteIndented = true }));
    Console.ReadKey();
}

Output:
FF 00 00 00 FF FF FF 7F
{
  "Header": 255,
  "Size": 2147483647
}


Algorithm to find all legal moves in Reversi game Reversi Game:
The image from the Reversi game:

I have my algorithm to find all legal moves in Reversi game.
Here's the code with comments:
"""
-1    => no disc
0/1   => player's disc
"""

for row_idx, row in enumerate(board):
    for column_idx, column in enumerate(board):
        if board[row_idx][column_idx] == -1:
            """
            Loop through square with chosen `-1` in middle
            5   5   5
            5  -1   5
            5   5   5
            """
            for i in range(-1, 2, 1):
                for j in range(-1, 2, 1):
                    """
                    Check if loop is not beyond borders
                    """
                    if row_idx + i < 0 or row_idx + 1 > (len(board) - 1) or column_idx + j < 0 or column_idx + j > (len(board) - 1):
                        continue

                    """
                    If there is a opponent disc, go straight in this direction (that way)
                    """
                    if board[row_idx + i][column_idx + j] == self.opponent_color:
                        i2, j2 = i, j
                        for _ in range(8):
                            """
                            Check if loop is not beyond borders
                            """
                            if row_idx + i + i2 < 0 or row_idx + i + i2 > (len(board) - 1) or column_idx + j + j2 < 0 or column_idx + j + j2 > (len(board) - 1):
                                continue

                            """
                            Break if the `no disc` is on `that way`
                            """
                            if board[row_idx + i + i2][column_idx + j + j2] == -1:
                                break

                            """
                            The legal move has been found, then break.
                            """
                            if board[row_idx + i + i2][column_idx + j + j2] == self.my_color:
                                legal_moves.append((row_idx, column_idx))
                                break

                            i2 += i
                            j2 += j

Is there any better way to do it? I am not sure about those nested for loops.
Thanks for any help or any hint!

Iteration
This:
for row_idx, row in enumerate(board):
    for column_idx, column in enumerate(board):
        if board[row_idx][column_idx] == -1:

is effectively incorrect. Is your board in row-major or column-major order? Whichever is true, you cannot iterate over the outer index of the board and get both a row and a column. What you likely meant instead is:
for row_idx, row in enumerate(board):
    for column_idx, cell in enumerate(row):
        if cell == -1:

Another problematic iteration is this:
i2, j2 = i, j
for _ in range(8):
    # ...
    i2 += i
    j2 += j

should simply be
for delta in range(1, 8):
    # ... use i*delta instead of i2
    # ... use j*delta instead of j2

Combined range checking
if row_idx + i < 0 or row_idx + 1 > (len(board) - 1) or column_idx + j < 0 or column_idx + j > (len(board) - 1):

should be
if not (0 <= row_idx + i < len(board)) or not (0 <= column_idx < len(board)):


The effect of typing a sentence out The following code is complete and works perfectly. What I want to do to is  make it more modular in a way where I can use the code again - similar to an api. I also hardly ever code in javascript, and I think my code looks disgusting, I feel like there are cleaner ways to write what I've written but I just don't have enough knowledge yet. If anyone has some advice/suggestion that would be appreciated.

The purpose of the module is to give the effect of a sentence being typed, then being backspaced out one character at a time. Here is a demo on my site: meneliktucker.me.

There are four main values to set:

sentences() - which is an array of sentences to be written out.
pause_sentence_time - the amount of time the sentence will pause after be written out - so the user can read the sentence before the sentence is deleted.
write_time - the time in ms to print a character
delete_time - the time in ms to delete a character

var delete_characters, delete_time, loop_characters,
loop_sentences, pause_sentence_time, print_char, sentences,
str_distance, write_time;

pause_sentence_time = 750;
write_time = 60;
delete_time = 40;

sentences = function() {
  var all_sentences;
  return all_sentences = ['is a student at the uOttawa.',
                       'loves to build things.'];
};

window.automatic_typing = function() {
  var char_array;
  char_array = sentences()[0].split('');
  loop_characters(char_array, 0, 0);
};

loop_sentences = function(prev_index) {
  var char_array, char_index, current_index;
  current_index = (prev_index + 1) % sentences().length;
  char_index = str_distance(prev_index);
  char_array = sentences()[current_index].split('');
  loop_characters(char_array, char_index, current_index);
};

loop_characters = function(char_array, char_index, index) {
  var timer;
  timer = 60;
  if (char_index === char_array.length) {
    timer = 750;
  }
  setTimeout((function() {
    if (char_index === char_array.length) {
      return delete_characters(index, str_distance(index));
    }
  print_char(char_array[char_index++]);
  loop_characters(char_array, char_index, index);
  }), timer);
};

print_char = function(c) {
  $('.automatic-typing').append(c);
};

delete_characters = function(index, dist) {
  setTimeout((function() {
    var temp;
    temp = $('.automatic-typing').text();
    if (temp.length === dist) {
      return loop_sentences(index);
    }
    $('.automatic-typing').text(temp.slice(0, temp.length - 1));
    delete_characters(index, dist);
  }), 40);
};

//The purpose of this function is to count the number of letters that
//the current and next sentence share from the start of both sentences.
//ex. the distance between 'this is cool' and 'this is not cool' is 7.

str_distance = function(index) {
  var counter, i, next_index, next_str_array, str_array, _i, _ref;
  str_array = sentences()[index].split('');
  next_index = index + 1;
  next_str_array = sentences()[next_index % sentences().length].split('');
  counter = 0;
  for (i = _i = 0, _ref = str_array.length; _i <= _ref; i = _i += 1) {
    if (str_array[i] === next_str_array[i]) {
      counter++;
    } else {
      break;
    }
  }
  if (str_array[counter] !== " " && counter !== 0) {
    while (true) {
      if (str_array[--counter] === " ") {
        break;
      }
    }
  }
  return counter;
};

Here is an update of what I've done:

I decided to make the code I provided earlier into a jQuery plugin. I reviewed jQuery plugins that I've used in the past then re-designed my code the JavaScript way. By using common feature in the JavaScript language such as the call method and utilizing object properties and prototypes.

I wrote it so you can call this jQuery plugin by simply entering in the element you want to have the effect apply to and then inputting your own customized values for the pause time, write time, delete time and sentence array.

Here is a sample of how to do that:

  $('.automatic-typing').automaticTyping({
    writeTime   :  60,
    deleteTime  :  40,
    pauseTime   :  750,
    sentences   : ['Enter in a sentence here',
                  'Another here',
                  '... As many as you want.'
                   ]
  });

Here is the re-designed code:

(function($) {
  "use strict";
  var methods = {
    init: function(options){

      options = $.extend({
        writeTime         :   60,
        deleteTime        :   60,
        pauseTime         :   1000,
        sentences         :   [
       'if you are seeing this',
       'you have not set your own values'
        ],
        charArray         :   [], //do not set this value and values below this point
        sentencesIndex    :   0,
        charIndex         :   0,
        distance          :   0
      }, options);

      this.each(function(){

        options = $.extend(true, {}, options); //deep copy the options object for every element
        options.charArray = options.sentences[0].split("");
        methods.loopCharacters.call(this, options);

      });

    },

    loopSentences: function(options){

      options.sentencesIndex = (options.sentencesIndex + 1) % options.sentences.length;
      options.charIndex = options.distance;
      options.charArray = options.sentences[options.sentencesIndex].split("");
      methods.loopCharacters.call(this, options);

    },

    loopCharacters: function(options){

      var i = options.charIndex;
      var len = options.charArray.length;
      var timer = options.writeTime;

      if (i === len) {
        timer = options.pauseTime;
      }

      setTimeout((function() {
        if (i === len) {
          methods.setDistance.call(this, options);
          return methods.deleteCharacters.call(this, options);
        }

        $(this).append(options.charArray[options.charIndex]);
        options.charIndex++;
        methods.loopCharacters.call(this, options);

      }).bind(this), timer);

    },

    deleteCharacters: function(options){

      setTimeout((function() {
        var currentSentence = $(this).text();

        if (currentSentence.length === options.distance) {
          return methods.loopSentences.call(this, options);
        }

        $(this).text(currentSentence.slice(0, currentSentence.length - 1));
        methods.deleteCharacters.call(this, options);

      }).bind(this), options.deleteTime);

    },

    setDistance: function(options){

      var nextSentenceIndex = options.sentencesIndex + 1;
      var cArr = options.charArray;
      var nextcArr = options.sentences[nextSentenceIndex % options.sentences.length].split('');
      var counter = 0;

      for (var i = 0; i <= cArr.length; i++) {
        if (cArr[i] === nextcArr[i]) {
      counter++;
        } else { break; }
      }

      if (cArr[counter] !== " " && counter !== 0) {
        while (true) {
      if (cArr[--counter] === " ") { break; }
        }
      }

      options.distance = counter;
    }

  };

  $.fn.automaticTyping = function(method){

    if ( typeof method === 'object' ) {
      return methods.init.apply( this, arguments );
    } else {
      $.error( 'Method ' +  method + ' does not exist on jQuery.');
    }

  };

})(jQuery);


Overriding GetHashCode and Equals I'm creating a class to wrap a list of Mask and I'd like to know if I'm overriding the GetHashCode() and Equals() correctly.

Mask is a wrapper for a List of Points. It does implement IEnumerable<Point>.

public class MaskCollection : IEnumerable<Mask>
    {
        #region Variables

        private List<Mask> masks;

        public int Count { get { return masks.Count; } }

        #endregion

        #region Overrides

        public override int GetHashCode()
        {
            int hash = 0;

            foreach(Mask mask in masks)
            {
                hash ^= mask.Center.GetHashCode();
            }

            return hash;
        }

        public override bool Equals(object obj)
        {
            if (ReferenceEquals(this, obj)) return true;

            if (!(obj is MaskCollection)) return false;

            MaskCollection other = (MaskCollection)obj;
            if (this.Count != other.Count) return false;

            HashSet<Mask> uniqueMasks = new HashSet<Mask>(masks);
            foreach(Mask mask in other)
            {
                if (!uniqueMasks.Contains(mask)) return false;
            }

            return true;
        }

        #endregion
    }

And if I am overloading them correctly, can this code be improved? My Equals() looks really slow, if they actually are equal.

This implementation of Equals violates the contract of Equals.

From MSDN:

  The following statements must be true for all implementations of the
  Equals(Object) method. In the list, x, y, and z represent object
  references that are not null.

  ...
  x.Equals(y) returns the same value as y.Equals(x).

The problem is with this code

HashSet<Mask> uniqueMasks = new HashSet<Mask>(masks);
foreach(Mask mask in other)
{
    if (!uniqueMasks.Contains(mask)) return false;
}

return true;

As it's possible that all masks in other are in uniqueMasks, but uniqueMasks contains a mask not in other (i.e. other \$\subsetneq\$ uniqueMasks).

For example, the following code will print False True.

var x = new MaskCollection(new[] { new Mask(0), new Mask(0) });
var y = new MaskCollection(new[] { new Mask(0), new Mask(1) });
Console.WriteLine(x.Equals(y));
Console.WriteLine(y.Equals(x));

Given this implementation of Mask

public class Mask : IEquatable<Mask>
{
    private readonly int center;

    public Mask(int center)
    {
        this.center = center;
    }

    public int Center
    {
        get { return this.center; }
    }

    public bool Equals(Mask other)
    {
        return this.center == other.center;
    }

    public override int GetHashCode()
    {
        return this.center.GetHashCode();
    }
}

You could write

return new HashSet<Mask>(masks).SetEquals(other);

but I would probably recommend not overriding GetHashCode and Equals for this class, if you don't have a good reason to do so.


Stopping a long-running asynchronous computation I'm writing a program that will perform a long-running computation, gradually improving a solution. On top of that, there's a simple UI that basically allows the user to stop the computation (letting the worker finish a step it's working on) and shows the best solution.
Here's a simplified version of what I have, I'm using async/await for the first time. Is there anything you would improve, or any other comments?

Worker code

public class SearchEngine
{
    //...
    public async Task<IProblemSolution> Search(CancellationTokenSource cts)
    {
        await Task.Factory.StartNew(() => {
            BestSolution = Problem.RandomSolution();
            while (!cts.IsCancellationRequested)
            {
                IProblemSolution s = Problem.RandomSolution(); // for simplification
                if (s.ObjectiveValue() < BestSolution.ObjectiveValue())
                    BestSolution = s;
            }
        });
        return BestSolution;
    }
}

UI

    private async void StartButton_Click(object sender, RoutedEventArgs e)
    {
        if (CancelTokenSrc != null)
            return;

        CancelTokenSrc = new CancellationTokenSource();
        Engine = new SearchEngine(new SampleProblem());

        StopButton.IsEnabled = true;
        StartButton.IsEnabled = false;
        Task<IProblemSolution> t = Engine.Search(CancelTokenSrc);
        await t;

        ResultLabel.Content = t.Result.ObjectiveValue().ToString();
        StopButton.IsEnabled = false;
        StartButton.IsEnabled = true;
        CancelTokenSrc = null;
    }

    private void StopButton_Click(object sender, RoutedEventArgs e)
    {
        if (CancelTokenSrc != null)
            CancelTokenSrc.Cancel();
    }

Task-based Asynchronous Pattern (MSDN)

public async Task<IProblemSolution> Search(CancellationTokenSource cts)

As I explain in this answer, one would rightfully expect the signature of that method to read like this:
public async Task<IProblemSolution> SearchAsync(CancellationTokenSource cts)

The pattern for async methods is quite simple: use the async keyword, return Task or Task<T>, and put an "Async" suffix to the method's name.

I haven't written much async code (read: never wrote a line of C# that leverages features of .net 4.5), but from previous reviews of such code I think this:
    Task<IProblemSolution> t = Engine.Search(CancelTokenSrc);
    await t;

    ResultLabel.Content = t.Result.ObjectiveValue().ToString();

Could be written like this:
    var searchResult =  await Engine.SearchAsync(CancelTokenSrc);
    ResultLabel.Content = searchResult.ObjectiveValue().ToString();
    //...

My understanding is that await somehow deals with Task.Result and so the type of searchResult would be IProblemSolution - I might be wrong here though.
Other than that, I find you could give var some lovin', and change this:
IProblemSolution s = Problem.RandomSolution();

To this:
var s = Problem.RandomSolution();

Lastly, I might be biased because I use ReSharper and with that tool IntelliSense/auto-complete is much "smarter", but I don't like names like cts - I'd rather call it cancelTokenSource and then I can type ctsTab and poof I'm referring to a readable identifier with a pronounceable name.


Refactor module for calls to internal image service Looking for help refactoring the following code to follow a Functional Programming paradigm.

The first function builds a configuration object with default settings and optional presets, while the buildResourcePath will assemble a URL for the image service.

getSizedUrl and getSizedSkuUrl returns a URL that will be used for an image tag.

var imgSvcPath = './imageSizer.svc';

/**
 * configureImg
 * Build a configurtion object with sane defaults applied that can be used to construct a url for the ImageResizer service
 */
function configureImg( options ){
    options = options || {};

    var defaults = {
            type: null,
            src: '',
            width: 120,
            height: null
        },
        // use Image presets if an option.type provided
        presets = options.type ? Images.getPresets( options.type ) : {};

    return $.extend( defaults, options, presets );
};

/**
 * buildResourcePath
 * Construct a url for the ImageResize service to be used for the src of <img />
 */

function buildResourcePath( svcName, options ){
    // imageSizer.svc/GetResizedImage?path=[path]&size=[size]&quality=[quality]
    var resourcePath = [],
        conf = configureImg( options ),
        src = ( conf.skuId !== undefined )
            ? '?sku=' + conf.skuId
            : '?path=' + encodeURI( conf.src ),
        size = '&size=' + ( conf.height ? 'h|' + conf.height : 'w|' + conf.width ),
        quality = conf.quality ? '&quality=' + conf.quality : '';

    resourcePath.push( imgSvcPath, svcName, src, size, quality );
    return resourcePath.join( '' );
};

/**
 * getSizedUrl
 * return src of <img /> for GetResizedImage
 */
var getSizedUrl = function( options ){
    if( 'undefined' === typeof options.src ) {
        console.error( 'upi.Services.Image::getSizedUrl configuration options must provide an image src string' );
    }

    return buildResourcePath( 'GetResizedImage', options );
};

/**
 * getSizedSkuUrl
 * return src for an <img /> for GetResizedSkuImage
 */
var getSizedUrlForSku = function( skuId, options ){
    if( 'undefined' === typeof skuId ) {
        console.error( 'upi.Services.Image::getSizedSkuUrl configuration options must provide sku ID' );
    }

    options = options || {};
    options[ 'skuId' ] = skuId;
    return buildResourcePath( 'GetResizedSkuImage', options );
};

Image = {
    getSizedUrlForSku: getSizedUrlForSku,
    getSizedUrl: getSizedUrl
};

// Store everything in a scope. It's to preven you from polluting the global
// scope as well as protect your code from collisions.
;(function(ns) {

  // Pulling out constants into this scope. They only need to be declared once.
  var imgSvcPath = './imageSizer.svc';
  var defaults = {
    type: null,
    src: '',
    width: 120,
    height: null
  };

  function configureImg(options) {
    var options = options || {};
    var presets = options.type ? Images.getPresets(options.type) : {};

    // It's best if you extend to a blank object. Don't override the defaults.
    return $.extend({}, defaults, options, presets);
  }

  function buildResourcePath(svcName, options) {

    var conf = configureImg(options);

    // I assume you use jQuery because of $.extend so I introduce $.param
    // which takes a map and converts it into a serialized parameter list

    var params = {};

    // And ternaries can be very messy and unreadable. Now using if-else instead.
    if (conf.skuId !== undefined) params.sku = conf.skuId;
    else params.path = conf.src;

    if (conf.height) params.size = 'h|' + conf.height;
    else params.size = 'w|' + conf.width;

    if (conf.quality) params.quality = conf.quality;

    return imgSvcPath + '?' + $.param(params);
  }

  // Let's append our API to the namespace

  ns.getSizedUrl = function(options) {
    if ('undefined' === typeof options.src) {
      console.error('upi.Services.Image::getSizedUrl configuration options must provide an image src string')
    }
    return buildResourcePath('GetResizedImage', options)
  };

  ns.getSizedUrlForSku = function(skuId, options) {
    if ('undefined' === typeof skuId) {
      console.error('upi.Services.Image::getSizedSkuUrl configuration options must provide sku ID')
    }
    options = options || {};
    options['skuId'] = skuId;
    return buildResourcePath('GetResizedSkuImage', options)
  };

  // Change your namespace to something else. Image is already taken.
  // Image is the constructor for making image elements with JS.
}(this.MyImage = this.MyImage || {}))


Naming Vegetable Factories Given the code below, I'm curious to know if you'd have better naming suggestions for types:

VegetablePresentersFactory
VegetablePresenterFactory

It bothers me a bit that their names only differ by a single character ('s'), and that this character is located in the middle of the name.

class VegetablesApp {
    private final VegetablePresentersFactory vegetablePresentersFactory; // gets injected

    public void displayVegetablePresenters(AllVegetables all) {
        List<VegetablePresenter> presenters = vegatablePresentersFactory.create(all);

        displayPresenters(presenters);
    }
}

class VegetablePresentersFactory {
    private final VegetablePresenterFactory vegetablePresenterFactory;

    public List<VegetablePresenter> create(AllVegetables all) {
        List<VegetablePresenter> presenters = new ArrayList<>();

        list.add(vegetablePresenterFactory.create(all.getTomato()));
        list.add(vegetablePresenterFactory.create(all.getLettuce()));
        list.add(vegetablePresenterFactory.create(all.getBacon()));

        return presenters;
    }
}

interface VegetablePresenterFactory {
    VegetablePresenter create(Vegetable vegetable);
}

How about replacing "s" of plural with the word "List":

interface VegetablePresenterListFactory {
    List<VegetablePresenter> create(AllVegetables allVegetables);
}

Like you said, the s in the middle is indeed a problem.
The difference is too small: it can lead to errors, and it hurts readability.

The same is true for name vs names in this code snippet:

void printNames(List<String> names) {
    for (String name : names) {
        // ...
    }
}

Although errors may sound unlikely because name and names are completely different types,
it can still happen when there are methods with the same name that happen to work on both objects,
for example isEmpty.

In such cases I use the suffix List to disambiguate, so nameList.
It's true that the term "List" is a bit redundant.
But it's better to be redundant than ambiguous,
so the little redundancy is justified by the improved readability.

UPDATE

You mentioned in the comment that "List" is a bit unfortunate if in the future you change the interface to use a Set instead of a List.
I think that's a moot point.
Changing the interface is a big deal,
and must be done carefully.
If you change the method to return a Set instead of a List,
I think you should be careful enough to notice that the interface name should change as well.
In terms of migrating users of the API,
this should not be a significant additional burden.

That said, I do have an alternative offering, using the word "Items" instead of "List":

interface VegetablePresenterItemsFactory {
    List<VegetablePresenter> create(AllVegetables allVegetables);
}

The good thing about "Items" is that it's less technical.
It's vague enough to be flexible, but not too vague to be meaningless.
Perhaps there are even better synonyms, you get the idea.


Optimizing "complex" iteration over Map with auxiliary maps This will be tough to explain. I am implementing a tool to support RFC6020. But when it comes to the Choice Statement, things get weird.

For the sake of better understanding, I have to remember you a couple of things:

I have a XML and a YANG file according to the RFC.
A Choice (and it's cases) are not included on the XML, only the children of the selected case. But the .yang instead, contains a full specification of the Choice and it's cases.

Based on this there is a moment where I have only read the .yang files and send a response to something that has only knowledge of the XML structure, thus no choices.

Basically my main problem is that this (complete) path on a .yang file:

  /container/choice/case/leaf

would be simplified like this on an XPATH notation of the .xml file:

  /container/leaf

Here is the basic logic of this task:

Consider that I have:

A Map with all the elements and their complete path. (Map<String, Object> paths)
A Collection of case paths. (Collection<String> casePaths)
An auxiliary Map as an alias os those paths
(Map<String, String> choiceCaseAliases, being: "simplified path" ➔ "complete path")

I have the following ugly method:

private Map<String, String> resolveChoicePaths() {
    String path;
    for (String casePath: casePaths) {
        String choiceCase = casePath.replaceFirst(".*/([^/]+/)", "$1")+"/";
        boolean mapModified = false;
        Map<String, String> tmp = new LinkedHashMap<>();
        for (Iterator entries = choiceCaseAliases.entrySet().iterator(); entries.hasNext();) {
            Map.Entry<String, String> entry = (Map.Entry<String, String>) entries.next();
            if (entry.getKey().contains(choiceCase)) {
                path = entry.getKey().replace(choiceCase, StringUtils.EMPTY);
                tmp.put(path, entry.getValue());
                entries.remove();
                mapModified = true;
            }
        }
        if (!mapModified) {
            for (String pathKey: paths.keySet()) {
                if (pathKey.contains(choiceCase)) {
                    path = pathKey.replace(choiceCase, StringUtils.EMPTY);
                    choiceCaseAliases.put(path, pathKey);
                }
            }
        } else {
            choiceCaseAliases.putAll(tmp);
        }
    }
    return choiceCaseAliases;
}

which is call here when paths map is completely populated:

Map<String, String> resolvedChoicePaths = resolveChoicePaths();
for (Map.Entry<String, String> entry: resolvedChoicePaths.entrySet()) {
    paths.getNodes().put(entry.getKey(), paths.getNodes().get(entry.getValue()));
    paths.getNodes().remove(entry.getValue());
}

Every thing works exactly as expected, but I think there must be a cleaner way to implement the same thing this method is doing. I think that this logic might be an overkill approach (that works, yay!) to the problem which actually is kind of complex, but I don't know if that much.

OBS:

Technically, I need to put and delete things at the same iteration on  the same map, but since it's not possible AFAIK I have a temporary map for this.
That regex which is used to get a String choiceCase is bacause the parent statement of a case is always a Choice. So from a path /container/choice/case I can get the String: choice/case/ and search for it on the other elements complete path.

Just some points:

I don't see the initialization of choiceCaseAliases,casePaths, or entry. If they are class variables, that's fine, as long as it is meant to be a class variable. If they are outside the method just so that another method can set the values and have the current method run, then it is not okay. If they are class variable just for the method, have them as method arguments.
Here:

    for (String casePath: casePaths) {

and here:

            for (String pathKey: paths.keySet()) {

You don't follow Java conventions. There should be a space before and after the ::

    for (String casePath : casePaths) {

and:

            for (String pathKey : paths.keySet()) {

I wouldn't create a new temporary map for each loop. Instead, I would create one outside the loop, and dump the contents of the temporary map into the map at the end:

private Map<String, String> resolveChoicePaths() {
    String path;
    Map<String, String> tmp = new LinkedHashMap<>();
    for (String casePath : casePaths) {
        String choiceCase = casePath.replaceFirst(".*/([^/]+/)", "$1") + "/";
        boolean mapModified = false;
        for (Iterator entries = choiceCaseAliases.entrySet().iterator(); entries.hasNext();) {
            Map.Entry<String, String> entry = (Map.Entry<String, String>) entries.next();
            if (entry.getKey().contains(choiceCase)) {
                path = entry.getKey().replace(choiceCase, StringUtils.EMPTY);
                tmp.put(path, entry.getValue());
                entries.remove();
                mapModified = true;
            }
        }
        if (!mapModified) {
            for (String pathKey : paths.keySet()) {
                if (pathKey.contains(choiceCase)) {
                    path = pathKey.replace(choiceCase, StringUtils.EMPTY);
                    tmp.put(path, pathKey);
                }
            }
        }
    }
    choiceCaseAliases.putAll(tmp);
    return choiceCaseAliases;
}


"Reverse Groups" challenge I'm trying to do the Reverse Groups Challenge on CodeEval:

  Given a list of numbers and a positive integer k, reverse the elements of the list, k items at a time. If the number of elements is not a multiple of k, then the remaining items in the end should be left as is.

My logic is functional, but when I try to submit my solution is says that it timed out at 10 seconds. How can I improve the speed here?

File.open(ARGV.first).readlines.each do |line|
  values, k = line.split(';')
  k = k.to_i
  values = values.split(',')
  current = values.shift(k)
  str = ""
  while current.count % k == 0 do
    current.reverse_each{ |val| str += "#{val},"}
    current = values.shift(k)
  end
  str << "#{current.join(',')}"
  puts str
end

Other attempt:

open(ARGV[0]).readlines.each do |line|
  values, k = line.chomp.split(';')
  k = k.to_i
  values = values.split(',')
  current = values.shift(k)
  str = ""
  while current.count % k == 0 do
    current.reverse_each{ |val| str << "#{val},"}
    current = values.shift(k)
  end
  str << "#{current.join(',')}"
  puts str
end

I can't say what CodeEval's time limits are, or why this code should particularly slow. There are a lot of things you could do differently, though:

Instead of readlines, you can use each_line to iterate the lines, instead of reading all of them into an array, and then iterating.
You can get the values and k in a single line:

*values, k = line.chomp.split(/[,;]/)

Look up each_slice; it'll iterate an array in "chunks" of the given length
Don't do "manual" string concatenation. You already use join in one place, but you can use it anywhere you need a string representation of an array.
You're shifting k items off of the array, and you then check if the number of shifted items is divisible by k. But shift will either shift exactly k items or fewer, so the modulus operator is a really roundabout way of saying current.count == k.
You code fails if there are blank lines. CodeEval doesn't say whether the file can contain blank lines, but, if it does, well...

All in all, you're approaching this very procedurally. A nicer approach would be something like

Chunk the items in k-sized arrays (the last chunk may be smaller)
Reverse the chunks that are k items long
Put the arrays back together into one
Join with commas and print

Or, in code:

File.open(ARGV.first).each_line do |line|
  *items, k = line.chomp.split(/[,;]/)
  next unless k # skip empty lines
  puts items.each_slice(k.to_i).map { |group| group.count < k.to_i ? group : group.reverse }.flatten.join(",")
end

Edit: As Naklion points out in the comments, using flat_map is of course better than using map and flatten separately, i.e.:

items.each_slice(k.to_i).flat_map { |group| group.count < k.to_i ? group : group.reverse }.join(",")

I've update the rest of the code blocks to use flat_map

That 3rd line is a bit long, though, so to spell it out:

File.open(ARGV.first).each_line do |line|
  *items, k = line.chomp.split(/[,;]/)
  next unless k
  chunks = items.each_slice(k.to_i)
  reversed = chunks.flat_map { |chunk| chunk.count < k.to_i ? chunk : chunk.reverse }
  puts reversed.join(",")
end

You can also do pop off the last chunk if you know it to be smaller than k, which lets you say simply map(&:reverse) on the remaining elements

File.open(ARGV.first).each_line do |line|
  *items, k = line.chomp.split(/[,;]/)
  next unless k
  k = k.to_i
  chunks = items.each_slice(k).to_a
  tail = items.count % k == 0 ? [] : chunks.pop
  puts chunks.flat_map(&:reverse).concat(tail).join(",")
end

Last alternative: Pop off enough items that the remaining ones are cleanly divisible by k before slicing

File.open(ARGV.first).each_line do |line|
  *items, k = line.chomp.split(/[,;]/)
  next unless k
  k = k.to_i
  tail = items.pop(items.count % k)
  puts items.each_slice(k).flat_map(&:reverse).concat(tail).join(",")
end

But frankly, I like the first code block the best.

All of these are a little faster than your code, by the way. It's not a huge difference, but it's faster.


calculate team sizes - Warzone - duos In call of Duty warzone, there is a Duos mode (Teams of two). As the game progresses and people are eliminated some teams might have just one player left and some still with two. The game tells you the number of players left and the number of teams. For example, 5 players left, 3 teams left. In this case we can know that this will be two teams of 2, and one team of 1.
Anyway I worked out a formula for calculating the number of teams with 1 or 2 players. below is a short code and some test cases I would appreciate any review / feedback on.
source code
from typing import Tuple

def team_counts(players: int, teams: int) -> Tuple[int, int]:
    if players <= 0 or teams <= 0:
        raise ValueError(f"{players=}, {teams=} - Players and teams must be a positive value")
    if teams > players or teams < players / 2:
        raise ValueError(f"{players=}, {teams=} - Teams must be less than players and greater than or equal to half of players")
    twos = players - teams
    ones = teams - twos
    return ones, twos

Test code
from teams_of_two import team_counts
import pytest

def test_players_teams_positive():
    assert team_counts(players=1, teams=1) == (1, 0)

def test_players_negative_teams_positive():
    with pytest.raises(ValueError):
        team_counts(players=-1, teams=1)

def test_players_positive_teams_negative():
    with pytest.raises(ValueError):
        team_counts(players=1, teams=-1)

def test_players_zero_teams_positive():
    with pytest.raises(ValueError):
        team_counts(players=0, teams=1)

def test_players_positive_teams_zero():
    with pytest.raises(ValueError):
        team_counts(players=1, teams=0)

def test_to_many_teams():
    with pytest.raises(ValueError):
        team_counts(players=1, teams=2)

Good job overall. You're developing good habits: you're testing your code, you've used the type annotations, you're using f-strings.
If I were to be nit-picky, here's what I'd still suggest:

some of your lines are too long and I have to horizontally scroll to see everything. try to shorten them as specified in PEP8
add a docstrings to your function
function names are usually describing an action so you should be calling yours something like: def count_teams() - I'm really bad at naming things but you got my point

The final code would look like this:
from typing import Tuple

def count_teams(players: int, teams: int) -> Tuple[int, int]:
    """
    Calculate the number of teams with 1 or 2 players
    in call of Duty WarZone (Duos mode)
    """

    if players <= 0 or teams <= 0:
        raise ValueError(f"{players=}, {teams=} - Players and "
                         f"teams must be a positive value")

    if teams > players or teams < players / 2:
        raise ValueError(f"{players=}, {teams=} - Teams must be "
                         f"less than players and greater than or"
                         f" equal to half of players")

    twos = players - teams
    ones = teams - twos

    return ones, twos

As far as your tests go, the only thing you could improve is to parametrize the tests that are the same but use different values. You have on the pytest's official docs some examples.
And another small nit-pick would be to first import the 3rd party libraries and then the local ones, with a blank line between each group:
import pytest

from teams_of_two import team_counts

# tests ...

LE: edited the chain comparison advice which wasn’t applicable in this case.


Memoization for calculating minimal traversal distance within a positive matrix The code below is for calculating the minimal traversing distance from the top left point of the matrix to the bottom right point of the matrix.

GitHub

Here is the core functionality. Please note that y is a global variable keeping track of the memoization.

def findLeastPath(matrix, row=0, column=0, path=[], sum_=0):
        row_max = matrix.shape[0]
        column_max = matrix.shape[1]

        if column == column_max - 1 and row == row_max - 1:
            y[row][column] = matrix[row][column]
            return matrix[row][column] + sum_
        else:
            current_cost = matrix[row][column]
            path.append((row, column, current_cost))
            if column == column_max - 1:
                if y[row+1][column] != 0:
                    y[row][column] = current_cost + y[row+1][column]
                    return current_cost + y[row+1][column]
                else:
                    down = findLeastPath(matrix, row + 1, column, path, sum_ + current_cost)
                    y[row][column] = current_cost + y[row+1][column]
                    return down
            elif row == row_max - 1:
                if y[row][column+1] != 0:
                    y[row][column] = current_cost + y[row][column + 1]
                    return current_cost + y[row][column + 1]
                else:
                    right = findLeastPath(matrix, row, column + 1, path, sum_ + current_cost)
                    y[row][column] = current_cost + y[row][column + 1]
                    return right
            else:
                if y[row][column + 1] != 0:
                    right = y[row][column + 1] + current_cost
                else:
                    findLeastPath(matrix, row, column + 1, path, sum_ + current_cost)
                    right = y[row][column + 1] + current_cost
                if y[row + 1][column] != 0:
                    down = y[row + 1][column] + current_cost
                else:
                    findLeastPath(matrix, row + 1, column, path, sum_ + current_cost)
                    down = y[row+1][column] + current_cost
                if y[row + 1][column + 1] != 0:
                    diagonal = y[row + 1][column + 1] + current_cost
                else:
                    findLeastPath(matrix, row + 1, column + 1, path, sum_ + current_cost)
                    diagonal = y[row+1][column+1] + current_cost

                min_val = min(right, down, diagonal)

                y[row][column] = min_val

                return min_val

Some documentation is sorely needed, both about the problem itself, and the approach taken to solve it.
Making y global is not a good idea. If you want to solve more than one matrix, the caller of this function would be responsible for resetting y between calls. Instead you could pass y around as an argument, perhaps initializing it like this:

def findLeastPath(matrix, y=None, row=0, column=0, path=[], sum_=0):
    if y is None:
        y = numpy.zeros_like(matrix)

I see that the possible moves are only right, down and diagonally down-right. The problem could be solved in a simple way by looping once through the array row by row. One can always easily compute the minimum cost of reaching the current node, because it is only possible to arrive from nodes whose cost has already been computed.
Dijkstra's algorithm is another option if costs are non-negative. Basically, always extend the shortest path by one step. Pro: may not have to visit all the nodes. Con: needs to maintain a priority queue.


node.js MVVM framework concept Not sure how subjective this question is however I am looking for a peer review and general feedback on the intuitiveness and scaffolding potential of a framework concept. I have a gist on GitHub here: node.js MVVM framework concept

Is this more or less intuitive then using pseudo code and a template engine, such as handlebars, for rendering html?

The markup module will use three objects, the first being out design object:

var welcomeDesign = [{
  handle:'pageTitle'
  tag:'h1',
  attr:{
    class:'mainHeading'
  }
},{
  tag:'p',
  handle:'welcomeParagraph',
  content:[{
    content:'Welcome!',
    handle:'welcomeContent'
  },{
    tag:'img'
    handle:'welcomeBanner'
  }]
}];

The second object is our data:

var data = [{
  published:'11-8-13',
  title:'Hello World',
  content:'Welcome to...'
  assets:[{
    type:'img',
    path:'/images/'
    name:'cat.jpg'
  }]
}];

The third will be our mapping logic:

var imageAsset = {
    attr:{
      src:{
        key:'assets',
        map:function(obj, values){
          return values[0].path + values[0].name
        }
      }
    }
};

var welcome = {
  pageTitle:{
    content: {
      key:['published','title']
      map: function(object, values){
        return values[0] + ': ' + values[1];
      }
    }
  },
  welcomeContent:{
    content:{
      key:'content'
    }
  },
  welcomeBanner:imageAsset
};

and finally the methods for bringing them together:

var markup = require('markup'); //our module

var welcomeMessage = markup.design(welcomeDesign); //create a design instance

var homeWelcome = new welcomeMessage(data); //instantiate with our data
homeWelcome.render(welcome); //render with our logic

It's better than rendering HTML in some other language manually, but it's at a disadvantage to HTML super-set template languages.

Mirroring the structure of HTML in JS makes the design more verbose because you need to map object fields to HTML tokens like attr: and not tangibly better than HTML that's parameterized with jQuery.  Mistakes in such a scheme, while not as bad as rendering HTML directly, are easier to make and harder to find than writing straight HTML or a super-set thereof because no IDE is going to understand how the JS object maps to an HTML snippet to validate it.  Most HTML super-set template languages have the same problem with IDEs, to be fair, but it's usually easier to see what the HTML emitted is going to look like.

Another thing I might look into refactoring is the mapping object, which is using specific attribute names.  You have an abstraction in the design, handle, that could just as easily be applied to attribute names, and your mapping would not have to reference the design's HTML at all.  That might be an idealistic complaint, though.

In summary, what I would rather see is roughly the same data and mapping methodology, but make the design something my IDE (and eyes) will better understand as HTML.


Dynamic Array Based Stack in C I wrote a dynamic stack in C that uses an array as a structure.  I tried to maintain O(1) for push and pop and believe I have done so.  I want to know what can be written in a cleaner way and if there are any non-trivial bugs.
#include <stdio.h>
#include <stdlib.h>

int push(int val, int *c);
int pop(int *c);

int *stack;

int main(){
    int *c = malloc(sizeof(int));
    stack = malloc(sizeof(int));
    *c = 0;
    int i;
    for(;;){
       printf("1. Push\n2. Pop\n3. Stack\n4. Quit\n>>> ");
       scanf("%d", &i);
       if(i == 1){
           printf("Value: ");
           scanf("%d", &i);
           push(i, c);
       }
       else if(i == 2)
           printf("Value popped: %d\n", pop(c));
       else if(i == 3)
           for(int i = 0; i < *c; i++)
               printf("%d\n", stack[i]);
       else
           break;
    }
    free(stack);
    return 0;
}

int push(int val, int *c){
    int *r;
    r = realloc(stack, ((*c)+1)*sizeof(int));
    if (r == NULL){
        free(stack);
        exit(0);
    }
    stack = r;
    stack[*c] = val;
    ++(*c);
    return *c;
}

int pop(int *c){
    if (!(*c)) return -1;
    int x = stack[(*c)-1];
    stack[(*c)-1] = NULL;
    int *r;
    printf("%d\n", *c);
    r = realloc(stack, ((*c)-1)*sizeof(int));
    if(r == NULL){
        free(stack);
        exit(0);
    }
    --(*c);
    stack = r;
    return x;
}

```

Create a struct that encapsulates all the details of a stack
The problem is that your stack just looks like a pointer to an int, indistinguishable from other pointers to ints. And the first element it points to is treated differently from the other elements. In this case, it is better to create a struct that keeps track of the allocated memory and the size of it, like so:
struct Stack {
    size_t size;
    int *data;
};

You initialize it as follows:
struct Stack stack = {0, NULL};

Now you should change push() and pop() to take a pointer to a struct stack:
void push(struct Stack *stack, int val) {
    stack->size++;
    int *new_data = realloc(stack->data, stack->size * sizeof *stack->data);

    if (!new_data) {
        // error handling here, or just
        abort();
    }

    stack->data = stack->new_data;
    stack->data[stack->size - 1] = val;
}

And similar for pop(). Note that it is common to have functions that operate on an object take the pointer to that object as the first parameter. Also, I made the function return void, there is no need to return the size of the stack size that information is already available to the caller.
Avoid using global variables
You should avoid using global variables if possible. My example code above no longer requires there to be a global stack. This change allows the code to manage multiple stacks without conflicts.
Add functions to create and destroy stacks
Instead of requiring the caller to know how to properly initialize a struct Stack and to free it after use, create functions that do this for you. That allows you to change the internals of struct Stack later, without having to change all the places where a stack is used.
Use a common prefix to avoid name conflicts
push() and pop() are very generic names. There are many more things that can have push and pop operations, such as FIFO queues. I recommend you use a common prefix for all data structures and functions for your stack. This can simply be Stack or stack if you think that is unlikely to conflict with anything else.


Making an efficient and healthy db call for a real-time browser game I'm doing a calculation for a real-time browser game, where users can invest some of their balances to the site. I need to update their invest's based on AN_AMOUNT_FROM_SITE and their percentage of shares (see PROFIT line). This function needs to run every 20-100 ms because AN_AMOUNT_FROM_SITE is coming consistently.

When this process finish, I'm saving a few unimportant data in callback().

When FUNDS collection has a few hundred document, this process will be very slow. Are there any ways I can reduce this process with Mongo to make it more efficient?

FUNDS.aggregate().group({ //get sum of invested amounts
        _id: '$id1',
        invest_total: { $sum: '$invest_amount'}}).exec(function ( e1, d ) {
             if( !e1 && d ){
                FUNDS.find().exec(function( e2, invests ){ //find all invests
                    if ( !e2 && invests ){
                      //calculate PROFIT value to each invest according to their rates (it can be +/-)
                        for (var i = 0; i < invests.length; i++) {

                        var invest_amount = invests[i].invest_amount,
                            invest_id     = invests[i].invest_id,
                            PROFIT        = AN_AMOUNT_FROM_SITE * invest_amount / d[0].invest_total; //
                            //update each invest
                            FUNDS.findOneAndUpdate({ 'invest_id': invest_id }, { $inc: { 'invest_profit': profit } }, function(e3) {
                                callback()
                            });
                    };
                }

                })
             }
})

The power of the aggregation framework is it's ability to iterate over the dataset in various useful ways without incurring extra round trips between the database and the app.

Your code uses one stage for aggregation (grouping by id1), and then jumps out of aggregation to iterate over the entire FUNDS collection for every user. That is very expensive, giving you exponential time complexity or worse.

The code below makes the database do all the heavy lifting in 3 aggregation framework stages, and returns an array with all the necessary data.

pivot over the user_id, calculating invest_amount, saving unique records in an array called records
unwind records. Now you have your original documents, but with a useful extra field.
calculate profit

Then we end up with an array of objects which can be iterated over, applying the the $inc update.

FUNDS.aggregate([
    {
        "$group": {
            "_id": '$id1',
            "invest_total": {"$sum": "$invest_amount"},
            "records": {"$push": "$$ROOT"}
        }
    },
    {
        "$unwind": "$records"
    },
    {
        "$project": {
            "invest_id": "$records.invest_id",
            "user_id": "$records.id1",
            "invest_total": "$invest_total",
            "invest_amount": "$records.invest_amount",
            "profit": {
                "$divide": [
                    { "$multiply": ["$records.invest_amount", AN_AMOUNT_FROM_SITE ] },
                    "$invest_total"
                ]
            }
        }
    }
]).exec(function(err,docs){
    docs.forEach(function(d){
        FUNDS.findOneAndUpdate(
            { "invest_id": d.invest_id},
            { "$inc": { 'invest_profit': d.profit } },
            function(e3){
                callback();
            }
        );
    });
});


Vampire number generator Inspired by Reddit r/dailyprogrammer

  A vampire number v is a number v=xy with an even number n of digits formed by multiplying a pair of n/2-digit numbers (where the digits are taken from the original number in any order) x and y together. Pairs of trailing zeros are not allowed. If v is a vampire number, then x and y are called its "fangs."
  EDIT FOR CLARITY Vampire numbers were original 2 two-digit number (fangs) that multiplied to form a four digit number. We can extend this concept to an arbitrary number of two digit numbers. For this challenge we'll work with three two-digit numbers (the fangs) to create six digit numbers with the same property - we conserve the digits we have on both sides of the equation.

As always, looking for general feedback over code readability, structure and efficiency. But also I was looking to make the code a bit more flexible.

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

/**
 * Created on 6/29/2016.
 *
 * Inspired by r/dailyprogrammer
 * https://www.reddit.com/r/dailyprogrammer/comments/3moxid/20150928_challenge_234_easy_vampire_numbers/
 *
 */

public class VampireNumbers {

    /**
     * Calls generate function then displays return value.
     * @param args Not used in this application.
     */
    public static void main(String args[]){
        generate().forEach(System.out::println); // Generates vampires, displays them on screen.
    }

    /**
     * Generates list of vampire numbers. Currently only supports 3 fangs of length 2.
     * @return List of vampires generated along with their fangs.
     */
    private static List<String> generate(){
        ArrayList<String> vampires = new ArrayList<>();
        for (int i = 10; i < 100; i++) {
            String fangOne = String.valueOf(i);
            for (int j = i; j < 100; j++) {
                String fangTwo = String.valueOf(j);
                for (int k = j; k < 100; k++) {
                    String fangThree = String.valueOf(k);
                    String vampire = String.valueOf(i * j * k);
                    if (sortVampire(fangOne + fangTwo + fangThree).equals(sortVampire(vampire))){
                            vampires.add(String.format("%s * %s * %s = %s", fangOne, fangTwo, fangThree, vampire));
                    }
                }
            }
        }
        return vampires;
    }

    /**
     * Used to sort a string of numbers. Helpful when determining if all fangs
     * are within the generated vampire number.
     * @param vampire String of numbers
     * @return The sorted character list of the given string.
     */
    private static List<Character> sortVampire(String vampire){
        List<Character> list = new ArrayList<>();
        for (int i = 0; i < vampire.length(); i++){
            list.add(vampire.charAt(i));
        }
        Collections.sort(list);
        return list;
    }
}

It works as of now but can only generate 3 fangs(of length 2) creating vampire numbers of length 6. Was looking to expand the parameters of the generate function to something like:

private static List<String> generate(int fangLength, int vampireLength)

Also I had a question over this line specifically:

if (sortVampire(fangOne + fangTwo + fangThree).equals(sortVampire(vampire)))

Instead of concatenating all three fangs, would it be worth it to use a StringBuilder in this scenario?

Also I had a question over this line specifically:

if (sortVampire(fangOne + fangTwo + fangThree).equals(sortVampire(vampire)))

  Instead of concatenating all three fangs, would it be worth it to use
  a StringBuilder in this scenario?

Not in this case. The compiler is smart enough to replace the concatenation of the String with the use of a StringBuilder. Using a StringBuilder to create a String from multiple concatenation is typically useful when it is done inside a loop.

For 3 fangs of length 2, your code is straight-forward and will be performant. A couple of notes:

There is a mathematical property that could be used to further improve performance. For bigger lengths, it can improve considerably the performance.
To determine whether all of the fangs have the same digits as their product, you are currently creating a list of characters and sorting those lists to test for equality. You could do that in a simpler and faster way without sorting anything.

To support arbitrary vampireLength, i.e. an arbitrary count of numbers to multiply to make up the final vampire number, we can resort to a recursive algorithm. For that, we will define a method generate

List<String> generate(int fangLength, int vampireLength, int... previousNumbers)

It takes as input the fangLength (count of digits in each fang), the vampireLength (count of fangs) and the previousVampires (array of all the numbers considered so far). This previousNumbers will be sorted ascendingly by construction. It returns the list of all the solutions.

The base case is when vampireLength is 0: it means that previousNumbers contains all the numbers to decide whether their product makes a vampire number. In this case, we perform the multiplication of all those numbers and compare the digits together: if they are the same then we can build the String representing the solution and return it in a singleton list; if they are not, we can return an empty list.
In the recursive case, we need to loop over all the possible numbers. First, the upper bound to consider: with a count of digits fangLength to consider, the upper bound is simply \$10^{\text{fangLength}}\$. Then, the lower bound: if there were no previous numbers then it is 10 (the starting point); else it is the last considered previousNumbers (so the greater). For each of those possible numbers i, we concatenate all the solutions by invoking the generate method again, adding i to the previousNumbers.

A sample code would be:

private static List<String> generate(int fangLength, int vampireLength, int... previousNumbers) {
    if (vampireLength == 0) {
        int product = Arrays.stream(previousNumbers).reduce(1, (a, b) -> a * b);
        if (sameDigits(product, previousNumbers)) {
            String op = Arrays.stream(previousNumbers).mapToObj(String::valueOf).collect(Collectors.joining(" * "));
            return Collections.singletonList(op + " = " + product);
        }
        return Collections.emptyList();
    }
    List<String> list = new ArrayList<>();
    int end = (int) Math.pow(10, fangLength);
    int start = previousNumbers.length == 0 ? 10 : previousNumbers[previousNumbers.length - 1];
    for (int i = start; i < end; i++) {
        int[] currentVampires = Arrays.copyOf(previousNumbers, previousNumbers.length + 1);
        currentVampires[currentVampires.length - 1] = i;
        list.addAll(generate(fangLength, vampireLength - 1, currentVampires));
    }
    return list;
}

private static boolean sameDigits(int target, int... numbers) {
    int[] array = new int[10];
    Arrays.stream(numbers).flatMap(i -> String.valueOf(i).chars()).forEach(c -> array[c - '0']++);
    String.valueOf(target).chars().forEach(c -> array[c - '0']--);
    return Arrays.stream(array).allMatch(i -> i == 0);
}

Some notes in this code:

It is using the Stream API to make the code more fluent: the product is calculated by reducing over the array of numbers and multiplying each of them together; the String solution is build by joining all the numbers, converted to a String, separated by " * ".
The sameDigits method was rewritten to implement the simpler algorithm of checking whether the digits are the same.


Escape some char in a byte array I am trying to construct a binary message packet. It needs to escape binary value 0 by appending extra 0 in front of it:

//Before escape
data[] = { 1, 2, 3, 4, 5, 0, 6, 7, 8, 9 }

//After escape - value 0 is appended after 0
data[] = { 1, 2, 3, 4, 5, 0, 0, 6, 7, 8, 9 }

My source code:

uint8_t escape_data[32];
uint8_t escape_index = 0;
uint8_t data[] = { 1, 2, 3, 4, 5, 0, 6, 7, 8, 9 };

for (int i = 0; i < sizeof(data); i++)
{
    escape_data[escape_index++] = data[i];
    if (data[i] == 0) {
        escape_data[escape_index++] = 0;
    }
}

Is there any elegant way to solve this problem?

P.S the code above is designed to be simple to read, I ignore those array sizing for simplicity and hard coding them, I want to know a good algorithm or a good way to write to solve this problem with the least variable and memory footprint

Searching for 0
Appending byte in between array

Hard coding 32 as the size of the destination array regardless of the size if the source array is not so good, as it might not be big enough. Better approaches would be:

Make the destination twice as big as the input: that way it will always be big enough to contain the escape zeros. The drawback is that most often it will be too big.
Count the number of zeros first, and the allocate the exact size that you need. The drawback is that the counting step is an extra \$O(N)\$ operation. On the other hand, if the counting step find no zeros, then you can skip the copying step. If most input will not contain zeros, then this approach should work pretty well.


Shifting an array with less touches I have an array that I am shifting by a certain number of spaces.  I am wondering if there is an better way to do the shift with less touches.  Here is my code

function rotate_array(array, shift){

    if(array.length -1 <= shift){
        shift %= array.length;
    }
    for(var i =0; i < shift; i++){
        var first = array[0];
        for(var j =0; j < array.length -1 ;j++){
            array[j] = array[j+1]
        }
        array[array.length - 1]  = first
    }
}

 var array = [4,2,1,3,5];
 rotate_array(array, 22);

  for(var i =0; i < array.length; i++){
    console.log(array[i])
 };

You can just use concat and splice..

shift %= array.length;
array = array.concat(array.splice(0,shift));

Or if you want the same array referance you can shift and push

for(i = 0; i < shift; i ++){
   var item = array.shift();
   array.push(item);
}

or shorten to

while((shift--) > 0){
   array.push(array.shift());
}


Remove nested for loops in python I have the following working code, but it is too slow because I have to run the same code for several files inside a folder. Please help me avoid the nested for loop or make it more efficient.
columnNames = {33:"boy", 24: "girl"}
events = [{1:"bo", 2: "irl"}, {1:"oy", 207: "gir"}, {1:"bboy", 2: "girly"}]

def create_game_data():
    columns = {}
    for column in columnNames.keys():
        columnValues = []
        for event in events:
            columnValues.append(columnNames[column]) if column in columnNames.keys() else columnValues.append("NA")
        columns[column] = columnValues
    return columns

Take note of the following.

columnNames is a dictionary of with 15 keys and pairs.
events is a list of dictionaries with 2000 items.
Every dictionary in the events list has 13 keys and pairs

My algorithm works in the following steps.

Loop through every list item in the events list (list of dictionaries) and append every value associated with the key from the outer for loop to the list called columnValues.

Replace the current key (from the outer for loop) with columnVales.
The desired output should be
{33: ['boy', 'boy', 'boy'], 24: ['girl', 'girl', 'girl']}

I'm not sure if you really desire that output. But I'll do some refactoring to show what your code does.

In your code

def create_game_data():
    columns = {}
    for column in columnNames.keys():
        columnValues = []
        for event in events:
            columnValues.append(columnNames[column]) if column in columnNames.keys() else columnValues.append("NA")
        columns[column] = columnValues
    return columns

there is an if condition always true. if column in columnNames.keys() is always true for your loop for column in columnNames.keys(). So we skip that test

def create_game_data():
    columns = {}
    for column in columnNames.keys():
        columnValues = []
        for event in events:
            columnValues.append(columnNames[column])
        columns[column] = columnValues
    return columns

Next we find that you loop over the events without using them. So we could safely write

def create_game_data():
    columns = {}
    for column in columnNames.keys():
        columnValues = []
        for _ in events:
            columnValues.append(columnNames[column])
        columns[column] = columnValues
    return columns

This is equivalent to

def create_game_data():
    columns = {}
    for column in columnNames.keys():
        columns[column] = [columnNames[column]] * len(events)
    return columns

Now we chose to iterate over the dict in a key-value manner

def create_game_data():
    columns = {}
    for column, name in columnNames.items():
        columns[column] = [name] * len(events)
    return columns

Which is easily written as comprehension

def create_game_data():
    return {k:[v] * len(events) for k, v in columnNames.items()}

Is that really what you want?


Recording timeback seconds of video based on compared pixels from an image The code reads from the screen, compares a pixel in a previously provided sample image and, if the pixels match, it records timeback seconds of video leading up to it. The output is put into a folder 'tm'. Since I do not assume the pixel to be occurring all the time, the writing process is buffered with a thread, so as not to slow down the capturing process. The pictures in the folder 'tm' can in the end be combined to make up a shortened movie, containing all the frames with the pixel in place.

I don't even know where to start optimizing. What should I improve in terms of readability and speed?

import PIL.ImageGrab
import PIL.Image
import time
from collections import deque
import threading
from numpy import *

def get_pixel_colour(i_x, i_y):
    return PIL.ImageGrab.grab().load()[i_x, i_y]

class framebuffer:
    def __init__(self,size):
        self.buffer = deque(maxlen=size)
        self.i= 0
    def add_frame(self, frame):
        self.buffer.append(frame)
    def remove_frame(self):
        return self.buffer.popleft()
    def write_frame(self):
        x = self.buffer.popleft()
        self.i+=1
        x.save("tm\screengrab"+ str(self.i).zfill(10) +".jpg")
        print "tm\screengrab"+ str(self.i).zfill(10) +".jpg written, ", len(self.buffer), "to go "
    def is_empfty(self):
        return (len(self.buffer)>0)

class mythread(threading.Thread):
    def run (self):
        while(True):
            if (len(self.frameb.buffer) > 0):
                self.frameb.write_frame()
            else:
                time.sleep(1)
    def setbuffer(self,frameb):
            self.frameb = frameb

seedimage = PIL.Image.open("seed.bmp")
pos_x = 983
pos_y = 263
recpixel = seedimage.load()[pos_x, pos_y]
print "The pixel is now", recpixel
fps = 50
skipticks = 1/(fps*1.0)
i= 0
timeback = fps * 3
allframes=framebuffer(timeback)
nextsnap=time.clock()
recordframes = framebuffer(None)
t= mythread()
t.setbuffer(recordframes)

print 'Starting thread'
t.setDaemon(True)
t.start()
print 'starting program'
print skipticks, fps
while (True):
    tim= time.clock()
    i=i+1
    x = PIL.ImageGrab.grab()
    allframes.add_frame(x)
    imp_pixel = x.load()[pos_x, pos_y]
    r, i = array(recpixel),array(imp_pixel)
    if (sum(r-i)<1 and sum(r-i)>-1 ):
        print "recognized", sum(r-i),"with size:", len(allframes.buffer)
        recordframes.buffer.extend(allframes.buffer)
        allframes.buffer.clear()
        sleep(10)

    # this is the sleep that limits the fps
    nextsnap+=skipticks
    sleeptime = nextsnap-time.clock()
    if (sleeptime>0):
        time.sleep (sleeptime)

import PIL.ImageGrab
import PIL.Image
import time
from collections import deque
import threading
from numpy import *

Generally, import * is frowned upon because it makes it hard to figure out where things come frome

def get_pixel_colour(i_x, i_y):
    return PIL.ImageGrab.grab().load()[i_x, i_y]

You don't appear to actually use this function

class framebuffer:

Python style guide recommends CamelCase for class names

    def __init__(self,size):
        self.buffer = deque(maxlen=size)
        self.i= 0

i is kinda hard to guess what its for.

    def add_frame(self, frame):
        self.buffer.append(frame)
    def remove_frame(self):
        return self.buffer.popleft()

You don't ever use this method

    def write_frame(self):
        x = self.buffer.popleft()
        self.i+=1
        x.save("tm\screengrab"+ str(self.i).zfill(10) +".jpg")
        print "tm\screengrab"+ str(self.i).zfill(10) +".jpg written, ", len(self.buffer), "to go "
    def is_empfty(self):

Misspelled name and never used

        return (len(self.buffer)>0)

You don't need the parens around the whole expression

class mythread(threading.Thread):
    def run (self):
        while(True):

No need for those parens. Let your tokens breath

            if (len(self.frameb.buffer) > 0):

Its odd that you define a is_empfty function but don't call it.

                self.frameb.write_frame()
            else:
                time.sleep(1)

Python has a Queue.Queue class which implements a thread-safe queue. That'll be a better idea then a list because its guaranteed to be thread safe. Also, if you try to pop from an empty queue it will wait until an item is pushed onto the queue. That way you don't need to do the time.sleep() and loop thing here

    def setbuffer(self,frameb):

The python style guide recommends names like set_buffer

            self.frameb = frameb

I'd also implement something like this by passing the frame buffer to the constructor. I'd also probably combine these two classes. They are tightly coupled anyways.

seedimage = PIL.Image.open("seed.bmp")
pos_x = 983
pos_y = 263

Instead, how about storing the position as a tuple? You can pass a tuple to index the arrays

recpixel = seedimage.load()[pos_x, pos_y]
print "The pixel is now", recpixel
fps = 50
skipticks = 1/(fps*1.0)
i= 0

Any sort of complex logic like this should really be in a function. It'll be somewhat faster that way.

timeback = fps * 3
allframes=framebuffer(timeback)
nextsnap=time.clock()
recordframes = framebuffer(None)
t= mythread()
t.setbuffer(recordframes)

print 'Starting thread'
t.setDaemon(True)
t.start()

I'd do at least the setDaemon instead the thread. (And are you sure you want a Deamon thread?)

print 'starting program'
print skipticks, fps
while (True):
    tim= time.clock()
    i=i+1

use for i in itertools.count(): instead the while loop, and you want have to manually track the i

    x = PIL.ImageGrab.grab()
    allframes.add_frame(x)

avoid names like x. Makes it hard to follow your code

    imp_pixel = x.load()[pos_x, pos_y]
    r, i = array(recpixel),array(imp_pixel)

This override your previous i, suggesting it wasn't really important enough to keep track of anyways

    if (sum(r-i)<1 and sum(r-i)>-1 ):

python has a syntax -1 < sum(r-1) < 1 which is nicer for these sorts of things

        print "recognized", sum(r-i),"with size:", len(allframes.buffer)
        recordframes.buffer.extend(allframes.buffer)

Why don't you make allframes a list? It doesn't use any of the other behavior of the class you made.

        allframes.buffer.clear()
        sleep(10)

I don't know why you are sleeping here

    # this is the sleep that limits the fps
    nextsnap+=skipticks
    sleeptime = nextsnap-time.clock()
    if (sleeptime>0):
        time.sleep (sleeptime)

Note that time.sleep isn't very accurate as timing goes.


Loading GIF form on a Windows console app using threads I have a main form which does processing for an extended period of time. Normally, progress bars are used to show either the loading status or simply show the program is still running/hasn't freezed.

For this form, I wanted to give it a more snazzy look by reusing a common loading GIF found on the web that many users would be familiar with.

This is how I implement said GIF:

ShowLoadingGif();

              //Heavy lifting

HideLoadingGif();

private void ShowLoadingGif()
{
    this.Enabled = false;

    Point formCenterPt = new Point()
    {
        X = this.Location.X + (this.Width/2),
        Y = this.Location.Y + (this.Height/2)
    };

    loadingThread = new Thread(() => ShowLoading(formCenterPt));
    loadingThread.IsBackground = true;
    loadingThread.Name = "Loading gif";
    loadingThread.Start();
}

private void HideLoadingGif()
{
    try
    {
        this.Enabled = true;
        if(loadingThread.IsAlive)
        loadingThread.Abort();
    }
    catch (Exception ex)
    {
        Log(String.Concat("Loading gif thread release failed: ",ex));
    }
}

private void ShowLoading(Point parentCenter)
{
    frmLoadingGif loading = new frmLoadingGif();

    Point offsetPt = new Point()
    {
        X = parentCenter.X - (loading.Width /2),
        Y = parentCenter.Y - (loading.Height/2)
    };

    loading.StartPosition = FormStartPosition.Manual;
    loading.Location = offsetPt;
    loading.ShowDialog();
}

What this does is produces a loading GIF (I grabbed mine from ajaxloader) in the center of the form while processing is ongoing, and also disables the form.

My question is, whether or not what I did was overkill for something as simple as an inprogresss status, or if using threads in this way is the best way to do it. I realize that using a control on the main form would have been easier, but I couldn't find a way to do so with a GIF while also making the background completely transparent.

Why have I chosen to use a form for showing the GIF?

The alternative would have been to add the picture box to the main form. However, this didn't seem the best solution because I wanted to center the GIF to the form itself, and with the TableLayoutPanel I was using, you can't simply change the location of a control in that manner (please correct me if I'm wrong here). I also wanted to keep the transparency, threading, and other attributes of the picturebox control separate from the main form so I could reuse it elsewhere.

Finally, I wanted to show the GIF loading on top of the current form, and not just in the corner/bottom in its own section like a progress bar would. As you'll see in the picture, the loading GIF is shown on top of the control behind it, a DataGridView.

I don't know much about threading, so I can't really comment on that, but there is an opportunity to extract some logic that I am sure is used elsewhere in your project. You have this in ShowLoadingGif()

Point formCenterPt = new Point()
{
    X = this.Location.X + (this.Width/2),
    Y = this.Location.Y + (this.Height/2)
};

And this in ShowLoading()

Point offsetPt = new Point()
{
    X = parentCenter.X - (loading.Width /2),
    Y = parentCenter.Y - (loading.Height/2)
};

I would create two functions that take a form as an argument and return a point representing the StartingPosition and center point respectively.

I'm not a fan of this If statement.

try
{
    this.Enabled = true;
    if(loadingThread.IsAlive)
    loadingThread.Abort();
}

First off, at the very least, loadingThread.Abort needs to be indented properly.

if(loadingThread.IsAlive)
    loadingThread.Abort();

But I still don't like it. Let's say down the road Mr. Maintainer realizes that something else should happen if(loadingThread.IsAlive. Then they do something like this.

if(loadingThread.IsAlive)
    loadingThread.Abort();
    foo.bar();

The problem here is, foo.bar() will always be executed, but it looks like it will only be executed if(loadingThread.IsAlive).

So, there are really only two options in my mind.

Write it as a single line If statement.

if (loadingThread.IsAlive) loadingThread.Abort();

Or put the braces in.

if (loadingThread.IsAlive)
{
    loadingThread.Abort();
}

This also makes me uncomfortable.

catch (Exception ex)
{
    Log(String.Concat("Loading gif thread release failed: ",ex));
}

You're swallowing every possible exception with nothing more than a message in a log file. You should figure out which exceptions you can gracefully handle and catch only those.


Decorator to cache a function result for some time I recently played around with a script that got some data from the Google API. As I didn't want to spam requests at the service (and potentially get blocked), I made this decorator, which caches the result of a function for a specified amount of time. Any call after the time-to-live (TTL) will call the function again.

This is the first decorator I wrote that takes an optional argument (the time to keep the cache). That code was taken from this StackOverflow answer by @Eric. I also couldn't abstain from using the new walrus operator (Python 3.8+), since I'm always looking for opportunities to use it in order to get a better feel for it.

Any and all advice on how to make this better or more readable are welcome.

from datetime import datetime
from functools import wraps

DEBUG = True

def temporary_cache(*args, ttl=60):
    """A decorator that ensures that the result of the function call
    is cached for `ttl` seconds (default: 60).

    Warning: The returned object is stored directly, mutating it also mutates the
    cached object. Make a copy if you want to avoid that.
    """
    def decorator(func):
        func.cache = None
        func.cache_time = datetime.fromordinal(1)

        @wraps(func)
        def inner(*args, **kwargs):
            if ((now := datetime.now()) - func.cache_time).total_seconds() > ttl:
                func.cache = func(*args, **kwargs)
                func.cache_time = now
            elif DEBUG:
                # for debugging, disable in production
                print("Cached", func.__name__)
            return func.cache
        return inner
    if len(args) == 1 and callable(args[0]):
        return decorator(args[0])
    elif args:
        raise ValueError("Must supply the decorator arguments as keywords.")
    return decorator

Example usages:

import time

@temporary_cache
def f():
    return datetime.now()

@temporary_cache(ttl=1)
def g():
    return datetime.now()

if __name__ == "__main__":
    print(f())
    # 2020-05-12 10:41:18.633386
    time.sleep(2)
    print(f())
    # Cached f
    # 2020-05-12 10:41:18.633386

    print(g())
    # 2020-05-12 10:41:20.635594
    time.sleep(2)
    print(g())
    # 2020-05-12 10:41:22.636782

Note that f was still cached, while g was not, because the TTL is shorter than the time between calls.

Rather than using *args you can supply a default positional only argument.

def temporary_cache(fn=None, *, ttl=60):
    ...
    if fn is not None:
        return decorator(fn)
    return decorator

If you feel following "flat is better than nested" is best, we can use functools.partial to remove the need to define decorator.

def temporary_cache(fn=None, *, ttl=60):
    if fn is None:
        return functools.partial(temporary_cache, ttl=ttl)

    @functools.wraps(fn)
    def inner(*args, **kwargs):
    ...

  for debugging, disable in production

You can use logging for this. I will leave actually implementing this as an exercise.

  I also couldn't abstain from using the new walrus operator (Python 3.8+), since I'm always looking for opportunities to use it in order to get a better feel for it.

A very reasonable thing to do. Abuse the new feature until you know what not to do. +1

However, I don't think this is a good place for it.
Given all the brackets in such a small space I'm getting bracket blindness.
I can't tell where one bracket ends and the others starts.
I am not a fan of func.cache = ... and func.cache_time. You can stop assigning to a function by using nonlocal.

Bringing this all together, and following some of my personal style guide, gets the following. I'm not really sure which is better, but it's food for thought.

from datetime import datetime
import functools

def temporary_cache(fn=None, *, ttl=60):
    if fn is None:
        return functools.partial(temporary_cache, ttl=ttl)

    cache = None
    cache_time = datetime.fromordinal(1)

    @functools.wraps(fn)
    def inner(*args, **kwargs):
        nonlocal cache, cache_time
        now = datetime.now()
        if ttl < (now - cache_time).total_seconds():
            cache = fn(*args, **kwargs)
            cache_time = now
        elif DEBUG:
            # for debugging, disable in production
            print("Cached", fn.__name__)
        return cache
    return inner