Implementation of ordered lock pattern Below is attempt to catch potential deadlocks at runtime in C#. Basically this class allows user to acquire locks in a pre-defined order only. I ran some basic tests and it appears to work. Please review and let me know if you find something wrong with implementation.

public class OrderedLock
{
    [ThreadStatic]
    static HashSet<int> mAcquiredLocks;
    static ConcurrentDictionary<int, object> mCreatedLocks = new ConcurrentDictionary<int, object>();

    private int mId;
    private ThreadLocal<int> mRefCount = new ThreadLocal<int>(false);
    private ReaderWriterLockSlim mLock;

    public OrderedLock(int id)
    {
        if (!mCreatedLocks.TryAdd(id, null)) throw new ApplicationException("Duplicate identifier detected");

        mId = id;
        mRefCount.Value = 0;
        mLock = new ReaderWriterLockSlim(LockRecursionPolicy.SupportsRecursion);
    }

    public void AcquireReadLock()
    {
        CheckLockOrder();
        mLock.EnterReadLock();
    }

    public void AcquireWriteLock()
    {
        CheckLockOrder();
        mLock.EnterWriteLock();
    }

    public void ReleaseReadLock()
    {
        mRefCount.Value -= 1;
        mLock.ExitReadLock();
        if (mRefCount.Value == 0) mAcquiredLocks.Remove(mId);
    }

    public void ReleaseWriteLock()
    {
        mRefCount.Value -= 1;
        mLock.ExitWriteLock();
        if (mRefCount.Value == 0) mAcquiredLocks.Remove(mId);
    }

    private void CheckLockOrder()
    {
        if (mAcquiredLocks == null) mAcquiredLocks = new HashSet<int>();
        if (!mAcquiredLocks.Contains(mId))
        {
            if (mAcquiredLocks.Count > 0 && mAcquiredLocks.Max() > mId)
                throw new ApplicationException("Invalid order of locking detected");
            else
                mAcquiredLocks.Add(mId);
        }

        mRefCount.Value += 1;
    }
}

I haven't extensively tested it, but there are a few things I've noted:

The class has two members which hold on to IDisposable resources. Therefore, the class must implement IDisposable as well and Dispose() of those members.
Many of the class members are assigned at construction time only. Mark them as readonly to signify intent and to potentially allow the runtime to perform some optimizations.
Current naming conventions favor camelCasing over Hungarian Notation for class members.
+= 1 and -= 1 can be simplified to ++ and --, respectively.
ApplicationException has been out of favor for quite a long time.
Identify your class members with the this keyword.
Forgot to put this bullet point in on the first cut: replace .Count() > 0 with .Any(). This is idiomatic to what you want, rather than how to do it. In other words, the implementation of the former could very well iterate over the entire collection to get the count, while the latter may just check for the existence of one in its collection.
Another omission: I replaced the declaration of HashSet<int> with ISet<int> since coding to interfaces allows for more flexibility in changing implementations later on.

All that being said, here's a first-cut rework:

public class OrderedLock : IDisposable
{
    private static readonly ConcurrentDictionary<int, object> createdLocks = new ConcurrentDictionary<int, object>();
    [ThreadStatic]
    private static ISet<int> acquiredLocks;

    private readonly ThreadLocal<int> refCount = new ThreadLocal<int>(false);
    private readonly ReaderWriterLockSlim locker = new ReaderWriterLockSlim(LockRecursionPolicy.SupportsRecursion);
    private readonly int id;

    /// <exception cref="InvalidOperationException">Duplicate identifier detected</exception>
    public OrderedLock(int id)
    {
        if (!createdLocks.TryAdd(id, null))
        {
            throw new InvalidOperationException("Duplicate identifier detected");
        }

        this.id = id;
        this.refCount.Value = 0;
    }

    public void AcquireReadLock()
    {
        this.CheckLockOrder();
        this.locker.EnterReadLock();
    }

    public void AcquireWriteLock()
    {
        this.CheckLockOrder();
        this.locker.EnterWriteLock();
    }

    public void ReleaseReadLock()
    {
        this.refCount.Value--;
        this.locker.ExitReadLock();
        if (this.refCount.Value == 0)
        {
            acquiredLocks.Remove(this.id);
        }
    }

    public void ReleaseWriteLock()
    {
        this.refCount.Value--;
        this.locker.ExitWriteLock();
        if (this.refCount.Value == 0)
        {
            acquiredLocks.Remove(this.id);
        }
    }

    public void Dispose()
    {
        while (this.locker.IsWriteLockHeld)
        {
            this.ReleaseWriteLock();
        }

        while (this.locker.IsReadLockHeld)
        {
            ReleaseReadLock();
        }

        this.locker.Dispose();
        this.refCount.Dispose();
        GC.SuppressFinalize(this);
    }

    /// <exception cref="InvalidOperationException">Invalid order of locking detected</exception>
    private void CheckLockOrder()
    {
        if (acquiredLocks == null)
        {
            acquiredLocks = new HashSet<int>();
        }

        if (!acquiredLocks.Contains(this.id))
        {
            if (acquiredLocks.Any() && acquiredLocks.Max() > this.id)
            {
                throw new InvalidOperationException("Invalid order of locking detected");
            }

            acquiredLocks.Add(this.id);
        }

        this.refCount.Value++;
    }
}


LinkedList Implementation in Java Please review my implementation. I know there is already an implementation in Java, but I just wanted to come up with my implementation. Let me know if I can improve this.

import java.util.NoSuchElementException;

public class LinkedListImplementation<T> {
    private int size;
    private ListNode<T> head;

    static class ListNode<T> {
        private T value;
        private ListNode<T> next;

        public ListNode(T val) {
            this.value = val;
            this.next = null;
        }

        public T getValue() {
            return value;
        }

        public ListNode<T> getNext() {
            return next;
        }

        public void setNext(ListNode<T> nextNode) {
            this.next = nextNode;
            return;
        }

        public String toString() {
            if(this.value == null) {
                return "null";
            }
            return this.value.toString();
        }
    }

    public LinkedListImplementation() {
        head = new ListNode(0);
        head.setNext(null);
        size = 0;
    }

    public void add(T val) {
        ListNode<T> cur = new ListNode<>(val);
        if(size == 0) {
            head.setNext(cur);
            size++;
            return;
        }

        ListNode<T> prev = head;
        while(prev.getNext() != null) {
            prev = prev.getNext();
        }

        prev.setNext(cur);
        size++;
        return;
    }

    public boolean remove(T val) {
        if(size == 0) {
            throw new NoSuchElementException();
        }

        ListNode<T> cur = head;
        ListNode<T> prev = null;
        while(cur.getNext() != null && cur.getNext().getValue() != val) {
            prev = cur;
            cur = cur.getNext();
        }

        if(cur.getNext() != null && cur.getNext().getValue() == val) {
            prev = cur;
            cur = cur.getNext();
            prev.setNext(cur.getNext());
            size--;
        }
        else {
            return false;
        }
        return true;
    }

    public int size() {
        return size;
    }

    public boolean isEmpty() {
        return size == 0;
    }

    public void clear() {
        head.setNext(null);
        size = 0;
        return;
    }

    public ListNode<T> get(int index) {
        if(index >= size) {
            throw new ArrayIndexOutOfBoundsException();
        }

        int curIndex = index;
        ListNode<T> cur = head;

        while(cur.getNext() != null && curIndex-- > 0) {
            cur = cur.getNext();
        }

        if(curIndex != -1) {
            throw new NoSuchElementException();
        }
        return cur.getNext();
    }

    public String toString() {
        StringBuilder sb = new StringBuilder();
        ListNode<T> cur = head;
        sb.append(cur.getValue() + "->");

        while(cur.getNext() != null) {
            sb.append(cur.getNext().getValue() + "->");
            cur = cur.getNext();
        }
        sb.append("null");
        return sb.toString();
    }

    public static void main(String[] args) {
        LinkedListImplementation<Integer> list = new LinkedListImplementation<>();
        System.out.println(list.size());
        list.add(1);
        list.add(3);
        list.add(5);
        System.out.println(list.remove(3));
        System.out.println(list.size());
        System.out.println(list.get(1));
        System.out.println(list.remove(6));
        System.out.println(list.get(0));
        System.out.println(list.get(1));
        //System.out.println(list.get(20));// ArraysIndexOutOfBoundsException.
        System.out.println(list.remove(5));
        System.out.println(list.size());
        //System.out.println(list.get(1));// ArraysIndexOutOfBoundsException.
        System.out.println(list.get(0));
        list.clear();
        System.out.println(list.size());
        //System.out.println(list.remove(1));//NoSuchElementException.
    }
}

The name LinkedListImplementation seems a little awkward, especially if someone else may try to use it.  It's understandable that you want to avoid using the same name as the official class, in which case you can consider MyLinkedList as the class name.
There's no need for that stray return at the end of clear() since it's a void function.
I'm not sure why those statements in main() are commented out.  Testing for exceptions is okay.  But, perhaps this indicates that you should have actual unit testing using JUnit.  main() would then initiate these tests without the need to comment out any of them.


Merge two sorted Linked List I want to improve this code. How to enter all elements in linked list simultaneously? I have not written code to delete a node becauseI want to only merge list for learning purpose.

#include <iostream>
#include <utility>

template<class T>
class LinkedList
{
  struct Node
  {
    T data;
    Node * next = nullptr;
    Node()          : data(), next(nullptr) {}
    Node(T&& value) : data(std::move(value)), next(nullptr) {}
  };

  Node *head;
public:
  LinkedList() : head(nullptr) {}
  LinkedList(const LinkedList& ll) = delete; //copy constructor
  LinkedList& operator=(const LinkedList& ll) = delete; //copy assignment
  ~LinkedList()
  {
    Node *tmp = nullptr;
    while(head)
    {
      tmp = head;
      head = head->next;
      delete tmp;
    }
    head = nullptr;
  }

   void insert(T&& value)
   {
    Node *node = new Node(std::move(value));
    Node *tmp = head;
    if(tmp == nullptr)
    {
      head = node;
    }
    else
    {
      while(tmp->next != nullptr)
      {
        tmp = tmp->next;
      }
      tmp->next = node;
    }
  }

  void mergeSortedList(LinkedList<T>& ll2)
  {
    Node *node = new Node();
    node->next = nullptr;
    Node *tmp = nullptr;
    tmp = node;

    Node *head1 = head;
    Node *head2 = ll2.head;

    while(head1 != nullptr && head2 != nullptr)
    {
      if(head1->data <= head2->data)
      {
        tmp->next = head1;
        tmp = tmp->next;
        head1 = head1->next;
      }
      else
      {
        tmp->next = head2;
        tmp = tmp->next;
        head2 = head2->next;
      }
    }
    while(head1 != nullptr && head2 == nullptr)
    {
      tmp->next = head1;
      tmp = tmp->next;
      head1 = head1->next;
    }
    while(head2 != nullptr && head1 == nullptr)
    {
      tmp->next = head2;
      tmp = tmp->next;
      head2 = head2->next;
    }
    tmp = tmp->next;
    delete tmp;

    // head of the new list
    head = node->next;
  }

  void printList()
  {
    Node *node = head;
    if(node == nullptr)
    {
      std::cout << "Empty List \n";
    }
    while(node != nullptr)
    {
      std::cout << node->data << " ";
      node = node->next;
    }
    std::cout << "\n";
  }
};

int main()
{
  LinkedList<int> ll1;
  ll1.insert(5);
  ll1.insert(10);
  ll1.insert(18);
  ll1.insert(25);
  std::cout << "List 1 : ";
  ll1.printList();

  LinkedList<int> ll2;
  ll2.insert(6);
  ll2.insert(8);
  ll2.insert(11);
  ll2.insert(20);
  ll2.insert(23);
  ll2.insert(28);
  ll2.insert(40);
  std::cout << "List 2 : ";
  ll2.printList();

  ll1.mergeSortedList(ll2);
  std::cout << "Merged List : ";
  ll1.printList();
}

Node constructor

It seems odd that we can only provide rvalues as argument to the constructor.  I'd expect to be able to pass by copy, too.

The usual way to allow that is to accept a T by value, and move-construct data from that value:

Node(T value = T()) : data(std::move(value)) {}

Note two other changes there: I've added a default argument, so we don't need to write a default constructor, and I've omitted the initialization of next, as we wrote a suitable default initializer for it.

List constructor

If we provide an initializer for head, we don't need to write a default constructor.

We might want to accept a list of elements when creating a linked list.  That would look something like this:

LinkedList() = default;
LinkedList(std::initializer_list<T> elements)
{
    for (auto& e: elements)
         insert(std::move(e));
}

Pointer ownership

We need to be absolutely clear that every object created with new has exactly one owner, that knows when the object is no longer in use and will delete it at that point.  When moving nodes from one list to another, it is essential that the ownership is transferred - if both lists think they are owner of a node, it will be double-deleted, and if neither lists does, it will be leaked.

We can write code to do this manually, but it's easier and less error-prone to #include <memory> and then make use of the smart pointer types that are provided for us.


MySQL/CUPS PDF printer for STAR TSP100 ECO This is my very first attempt at Python and whilst the script works perfectly, I'd like to see if I'm missing anything obvious or doing anything particularly stupid. I think this runs on Python 2.7, but I'm not 100% sure.

Goal:
The goal here is to grab a task and its elements (records contain BLOB PDF data) and write them all to CUPS as a single print job for a receipt printer, starting with a plaintext header, followed by a list of PDF files and followed by a plaintext header.

Finally, the code should mark the task as printed in the database and remove all blob-data associated with the task as to prevent my database from turning into a massive waste of space. It should likewise delete the folder containing the 'downloaded' PDFs and header/footer files.

import os
import shutil
import MySQLdb
import cups

printername = "STARTTSP100"
dbAddress =  "nope"
dbUser = "nope"
dbPasswd = "nope"
dbDatabase = "printer"

# Connect to the Printer DB
db = MySQLdb.connect(
    host=dbAddress,
    user=dbUser,
    passwd=dbPasswd,
    db=dbDatabase
 )

curOne = db.cursor()
curOne.execute("SELECT task.id, task.header, task.footer FROM task WHERE task.printDate IS NULL ORDER BY task.regDate ASC LIMIT 5")
for row in curOne:

    printList = []

    if not os.path.exists(row[0]):
        os.makedirs(row[0])

    header = open(row[0]+"/"+row[0]+"_header.txt","w")
    header.write(row[1])
    header.close()
    printList.append(row[0]+"/"+row[0]+"_header.txt")

    curTwo = db.cursor()
    curTwo.execute("SELECT elements.id, elements.count, elements.data FROM elements WHERE task = '"+row[0]+"' ORDER BY count ASC")

    for subrow in curTwo:
        tempPdf = open(row[0]+"/"+subrow[0]+".pdf","wb")
        tempPdf.write(subrow[2])
        tempPdf.close()
        for x in range(0,subrow[1]):
            printList.append(row[0]+"/"+subrow[0]+".pdf")

    footer = open(row[0]+"/"+row[0]+"_footer.txt","w")
    footer.write(row[2])
    footer.close()
    printList.append(row[0]+"/"+row[0]+"_footer.txt")

    cups.Connection().printFiles(printername, printList, "expedition-"+row[0],{})

    curThree = db.cursor()
    curThree.execute( "UPDATE task SET printDate = NOW() WHERE id = '"+row[0]+"'" )
    curThree.close()

    curFour = db.cursor()
    curFour.execute( "UPDATE elements SET data = null WHERE task = '"+row[0]+"'" )
    curFour.close()

    shutil.rmtree(row[0])

db.commit()
db.close()

print("done")

I would put the configuration of user, password, host, etc into a dictionary. This allows you to load this dictionary from a file, if you don't want to have the password in the code. It makes it also easy to pass it to a db connect function:

# Hard-coded here, but might as well be retrieved from a config file
CONF = {'host': "nope",
        'user': "nope",
        'passwd': "nope",
        'db': "printer"}

def connect(**conf):
    """# Connect to the Printer DB"""
    return MySQLdb.connect(**conf)

db = connect(**conf)

The connect function is even transparent, so you can also pass additional keyword-arguments:

db = connect(raise_on_warnings=True, **conf)

I would split up the main part of the code into separate functions. One to create the files to print, which returns the list of files to print. And a second function for printing that task and cleaning up the task.

The first function is to_print. It yields (instead of returning the list) the files to print (as a list) for every task. Note that I re-used the cursor, since there is no need to create a new one every time.

I also defined a partial function using functools, which makes creating a file_name following the format task_id/file_name a bit easier. I also used str.format to avoid potentially costly string additions (I don't know how large your task_ids are.

In addition I used tuple unpacking to give row[0], row[1] and row[2] meaningful names.

And finally, you should use with..as to automatically close a file after you are finished with it.

def to_print(db):
    sub_query = "SELECT elements.id, elements.count, elements.data FROM elements WHERE task = '%s' ORDER BY count ASC"

    cursor = db.cursor()
    cursor.execute(
        "SELECT task.id, task.header, task.footer FROM task WHERE task.printDate IS NULL ORDER BY task.regDate ASC LIMIT 5")
    for task_id, header, footer in cursor:
        path_join = functools.partial(os.path.join, task_id)
        print_list = []
        if not os.path.exists(task_id):
            os.makedirs(task_id)

        header_file_name = "{}_header.txt".format(path_join(task_id))
        with open(header_file_name, "w") as header_file:
            header_file.write(header)
        print_list.append(header_file_name)

        cursor2 = db.cursor()
        cursor2.execute(sub_query, (task_id,))
        for file_name, elements_count, data in cursor2:
            pdf_file_name = "{}.pdf".format(path_join(file_name))
            with open(pdf_file_name, "wb") as pdf_file:
                pdf_file.write(data)
            print_list.extend([pdf_file_name for _ in range(elements_count)])
        cursor2.close()

        footer_file_name = "{}_footer.txt".format(path_join(task_id))
        with open(footer_file_name, "w") as footer_file:
            footer_file.write(footer)
        print_list.append(footer_file_name)

        yield task_id, print_list
    cursor.close()

The second function marks a task as finished:

def mark_finished(db, task_id):
    cursor = db.cursor()
    cursor.execute(
        "UPDATE task SET printDate = NOW() WHERE id = '%s'", (task_id,))
    cursor.execute(
        "UPDATE elements SET data = null WHERE task = '%s'", (task_id,))
    cursor.close()
    shutil.rmtree(task_id)

It is basically the same as before, only now I re-use the cursor and use the functionality of cursor.execute that you can give it data in the sprintf-like format "name: %s" % ("Graipher",).

Finally, I would put the code calling all of this in a if __name__ == "__main__" guard to allow importing you module from another script.

Here is also the usage of the generator returned by to_print, we just iterate over the tasks. The actual printing is inlined here, you could also stick it into a function.

if __name__ == "__main__":
    db = connect(**CONF)
    for task_id, print_list in to_print(db):
        cups.Connection().printFiles(PRINTER, print_list, "expedition-" + task_id, {})
        mark_finished(db, task_id)
    db.commit()
    db.close()

    print("done")

This can be broken down into even smaller tasks (the writing of the files is still some-what redundant for example), which I did in my final code:

import os
import shutil
import MySQLdb
import cups
import functools
from contextlib import closing

PRINTER = "STARTTSP100"

# Hard-coded here, but might as well be retrieved from a config file
CONF = {'host': "nope",
        'user': "nope",
        'passwd': "nope",
        'db': "printer"}

def connect(**conf):
    """# Connect to the Printer DB"""
    return MySQLdb.connect(**conf)

def create_file(task_id, extension, data):
    file_name = "{}{}".format(os.path.join(task_id, task_id), extension)
    with open(file_name, "w") as write_file:
        write_file.write(data)
    return file_name

def create_files(db, task_id, query):
    cursor = db.cursor()
    cursor.execute(query, task_id)
    for file_name, elements_count, data in cursor:
        pdf_file_name = "{}.pdf".format(os.path.join(task_id, file_name))
        with open(pdf_file_name, "wb") as pdf_file:
            pdf_file.write(data)
        yield [pdf_file_name for _ in range(elements_count)]
    cursor.close()

def to_print(db):
    with db as cursor:
        cursor.execute(
            "SELECT task.id, task.header, task.footer FROM task WHERE task.printDate IS NULL ORDER BY task.regDate ASC LIMIT 5")

        sub_query = "SELECT elements.id, elements.count, elements.data FROM elements WHERE task = '%s' ORDER BY count ASC"

        for task_id, header, footer in cursor:
            if not os.path.exists(task_id):
                os.makedirs(task_id)

            print_list = [create_file(task_id, "_header.txt", header)]

            for files in create_files(db, task_id, sub_query):
                print_list.extend(files)

            print_list.append(create_file(task_id, "_footer.txt", footer))

            yield task_id, print_list

def mark_finished(db, task_id):
    with db as cursor:
        cursor.execute(
            "UPDATE task SET printDate = NOW() WHERE id = '%s'", task_id)
        cursor.execute(
            "UPDATE elements SET data = null WHERE task = '%s'", task_id)
    shutil.rmtree(task_id)

def print_files(print_list, name, printer=PRINTER)
    cups.Connection().printFiles(printer, print_list, name, {})

if __name__ == "__main__":
    with closing(connect(**CONF)) as db:
        for task_id, print_list in to_print(db):
            print_files(print_list, "expedition-" + task_id)
            mark_finished(db, task_id)
    print("done")

Note the addition of contextlib.closing to ensure that the db connection is closed even if an exception is raised somewhere in-between and using with db as cursor to make sure the cursor is also closed and comitted.


Sending file[type="file"] multiple content as list of base64's There is a code that takes all files from input of multiple files, gets base64 for each of them, packs to array and jsonifies it. How to simplify and improve quality of code?

text = $('#input-' + id).val();
var files = document.querySelector('#files' + id + ' > input[type=file]').files;
if (text != '' || files.length > 0) {
    var files_obj = [];
    var reader = new FileReader();
    reader.onload = function(readerEvt) {
        var binaryString = readerEvt.target.result;
        files_obj.push(btoa(binaryString));
    };
    for (var i = 0; i < files.length; i++) {
        reader.readAsBinaryString(files[i]);
    }
    while (files_obj.length < files.length) {}
    $('input[type="file"]').val("");
    files_json = JSON.stringify(files_obj);
    json = '{"action": "message", "from": ' + userId + ', "text": "' + text + '", "room": ' + id + ', "files": ' + files_json + '}';
    socket.send(json);
    $('#input-' + id).val("");
}

while (files_obj.length < files.length) {}

That's not the correct way to wait for the FileReader. onload will get called whenever a file has been successfully read, so you can check for files.length there, and call the rest of the function as needed. Right now you're spin-looping during at least partly CPU intensive (base64 encoding) operations. Which is a huge waste.


NumPy array filter optimisation I've got a filter that I'd like to work out how to optimize. The most basic implementation, using loops is like the following:

import numpy as np
nrow = 500
ncol = 500
refArray = np.random.randint( 0,100, (nrow,ncol) )
boundsArray = np.random.randint( 90,95, (2,nrow,ncol) )
window = 31
halfwindow = window/2
result = np.zeros_like(refArray)
## we'll ignore edges for the moment

for i in range(halfwindow, nrow - halfwindow):
    for j in range(halfwindow, ncol - halfwindow):
        window = refArray[ (i-halfwindow):(i+1+halfwindow), (j-halfwindow):(j+1+halfwindow)]
        valid = (window >= boundsArray[0,i,j]) & (window <= boundsArray[1,i,j])
        result[ (i-halfwindow):(i+1+halfwindow), (j-halfwindow):(j+1+halfwindow) ] \
           += valid

So, for each location (i,j) in an array, I find a window around this location, and identify all elements which meet a particular requirement, then move on. At each step I count how often element (i,j) has met the requirement.

I'm looking for suggestions on approaches to write this efficiently. I can't see how I can avoid the loops, so I'd consider options like Cython or f2py.

Here's a partial solution:

as_strided = np.lib.stride_tricks.as_strided
def fun2(refArray):
        y = len(range(-halfwindow,halfwindow+1))
        II=slice(halfwindow, nrow - halfwindow)
        bA = boundsArray[:,II,II]
        x=len(range(halfwindow,nrow-halfwindow))
        shape = (x,x) + (y,y)
        strides = refArray.strides
        strides = strides + strides
        print(shape, strides)
        A = as_strided(refArray, shape=shape,strides=strides)
        print(A.shape,(refArray[0,0],refArray[-1,-1]),(A[0,0,0,0],A[-1,-1,-1,-1]))
        valid = (A >= bA[0,:,:,None,None]) & \
             (A <= bA[1,:,:,None,None])
        valid = valid.sum((-2,-1))
        # # valid pts per window
        print(valid.shape)
        return valid

It uses np.lib.stride_tricks.as_strided to create overlapping windows.  It's an idea that has been presented in several SO answers.

In timing it is 4-5x faster than your iteration.

A is a (470,470,31,31) view of refArray, i.e. 470x470 windows, each 31x31.  And with the strides value they overlap.

But it is partial.  valid.sum() gives the same values as your result.sum().  So it finds the same total number of 'valid' points. But valid (after summing on the 31x31 dimensions) is a (470,470) array, one value for each 'window', and that value is the number of valid points within the window.  It is the equivalent of doing windows[i-halfwindow,j-halfwindow] = valid.sum() in your iteration.

I think your result is the number of windows in which each point is valid.

I haven't figured out how to map the (470,470,31,31) valid array back on to the (500,500) points.  But I pretty sure there is such a mapping.

This is a temporary fix:

def fun2(refArray):
        y = len(range(-halfwindow,halfwindow+1))
        II=slice(halfwindow, nrow - halfwindow)
        bA = boundsArray[:,II,II]
        x=len(range(halfwindow,nrow-halfwindow))
        shape = (x,x) + (y,y)
        strides = refArray.strides
        strides = strides + strides
        A = as_strided(refArray, shape=shape,strides=strides)
        valid = (A >= bA[0,:,:,None,None]) & \
             (A <= bA[1,:,:,None,None])
        return valid

def fun3(refArray,valid):
        result = np.zeros_like(refArray)
        n0,n1,n2,n3 = valid.shape
        for i in range(n0):
            for j in range(n1):
                result[ i:(i+n2),j:(j+n3) ] += valid[i,j]
        return result
result = fun3(refArray, fun2(refArray))

fun3 has the same sort of iteration as your code, but there's still a 2x speedup.

While each window has the same number of points, the reverse is not true.  Corner points occur in only one window each.  Middle ones around 900 each.  That may make a pure vectorized numpy solution impossible.


Makefile for building ASM/C project I am building a bare bones freestanding kernel in ASM and C. My make file is below.

CFLAGS= -ffreestanding -nostdlib -gdwarf-4 -m32 -ggdb3 -Wall -Wextra -Werror -std=c11
ASMCFLAGS= -f elf32 -F dwarf -g -w+all
LDFLAGS= --nmagic -m elf_i386 --script=os.lds

BUILD_DIR=../build/os
BUILD_DIR_IMM=$(BUILD_DIR)/imm
BUILD_DIR_IMM_ASM=$(BUILD_DIR_IMM)/asm
BUILD_DIR_IMM_C=$(BUILD_DIR_IMM)/c
BUILD_OUTPUT=$(BUILD_DIR)/os

OS_C_SRCS := $(wildcard *.c)
OS_C_OBJS := $(patsubst %.c, $(BUILD_DIR_IMM_C)/%.o, $(OS_C_SRCS))

OS_ASM_SRCS := $(wildcard *.asm)
OS_ASM_OBJS := $(patsubst %.asm, $(BUILD_DIR_IMM_ASM)/%.o, $(OS_ASM_SRCS))

OS_SRCS := $(OS_C_SRCS) $(OS_ASM_SRCS)
OS_OBJS := $(OS_ASM_OBJS) $(OS_C_OBJS)

all: $(BUILD_OUTPUT)

prepdir:
    mkdir -p $(BUILD_DIR_IMM_C)
    mkdir -p $(BUILD_DIR_IMM_ASM)

$(OS_C_OBJS): $(OS_C_SRCS) $(OS_ASM_OBJS) prepdir
    $(CC) $(CFLAGS) -c $< -o $@

$(BUILD_DIR_IMM_ASM)/__init.o: __init.asm prepdir
    nasm $(ASMCFLAGS) __init.asm -o $@

$(BUILD_DIR_IMM_ASM)/kinit.o: kinit.asm prepdir
    nasm $(ASMCFLAGS) kinit.asm -o $@

$(BUILD_DIR_IMM_ASM)/keyboard.o: keyboard.asm prepdir
    nasm $(ASMCFLAGS) keyboard.asm -o $@

$(BUILD_DIR_IMM_ASM)/interrupt.o: interrupt.asm prepdir
    nasm $(ASMCFLAGS) interrupt.asm -o $@

$(BUILD_DIR_IMM_ASM)/string.o: string.asm prepdir
    nasm $(ASMCFLAGS) string.asm -o $@

$(BUILD_DIR_IMM_ASM)/vid.o: vid.asm prepdir
    nasm $(ASMCFLAGS) vid.asm -o $@

$(BUILD_OUTPUT): $(OS_C_OBJS) $(OS_ASM_OBJS)
    ld $(LDFLAGS) $(OS_OBJS) -o $@

clean:
    rm -rf $(BUILD_DIR)

This makefile is used as a child makefile.

My primary concern is that I cannot pass multiple files to NASM, so I need a rule for each assembly file I have. My secondary concern is cross dependencies on other files, for instance, my C files should be rebuild if any C header files change.

I see a number of things that may help you improve this.

Don't manually write a Makefile

Since you're using standard tools such as gcc and ld, you could use: autotools (that is the collection of autoconf and related software) instead of writing and maintaining your own Makefile.  There is a learning curve, of course, but the benefits are considerable, especially if you're writing software that will become open source.  To do that here, you can start with these two files:

configure.ac

AC_PREREQ([2.69])
AC_INIT([FULL-PACKAGE-NAME], [VERSION], [BUG-REPORT-ADDRESS])
AC_CONFIG_SRCDIR([foo.c])
AC_CONFIG_HEADERS([config.h])

AC_PROG_CC

PACKAGE_CFLAGS="-ffreestanding -nostdlib -gdwarf-4 -m32 -ggdb3 -Wall -Wextra -Werror -std=c11"
PACKAGE_LDFLAGS="--nmagic -m elf_i386 --script=os.lds"
PACKAGE_NASMFLAGS="-f elf32 -F dwarf -g -w+all"

AC_SUBST([PACKAGE_CFLAGS])
AC_SUBST([PACKAGE_LDFLAGS])
AC_SUBST([PACKAGE_NASMFLAGS])

AM_INIT_AUTOMAKE(foreign)
AC_CONFIG_FILES([Makefile])
AC_OUTPUT

Makefile.am

bin_PROGRAMS = os
os_SOURCES = foo.c ${asm_sources}
os_NASMFLAGS = @PACKAGE_NASMFLAGS@
os_LDFLAGS = @PACKAGE_LDFLAGS@
AM_CPPFLAGS = @PACKAGE_CFLAGS@
NASM = /usr/bin/nasm

asm_sources = thing1.asm thing2.asm

.asm.o:
        $(NASM) $(os_NASMFLAGS) $< -o $@

With these, you can execute autoreconf -i in the same directory.  Next, create a build directory anywhere (one of the advantages of automake) and execute the generated configure file from that directory.  That will create a nice Makefile for you which you can then execute in the usual way by typing make.

Another tool I use very often is CMake, which does something similar to autotools in that it creates build scripts for you, but it's not tied to Linux and BSD and can work with Windows-based compilers as well.

With that said, if you're determined to do it the hard way, there are some things you can improve in your existing file.

Define an implicit rule

GNU Make (which I assume that you're using) knows how to create object files from assembly files, but only by using as rather than nasm.  It's very simple to teach it a new trick, however, by defining an implicit rule.  Here's one:

.asm.o:
        $(NASM) $(NASMFLAGS) $< -o $@

This is a simple rule that tells make that the "recipe" to create a .o file from a .asm file is to run the command listed below.  That way you only need to list a dependency and not the rule each time.  That is:

thing1.o: thing1.asm

However, even this is often not necessary if you've defined the implicit rule.  The next suggestion explains.

Make make work harder

The purpose to having make is to offload some of the work of the programmer to quickly do only what's necessary to rebuild a new binary.  You can make it work harder than you currently are, while making things easier to maintain and a bit shorter as well.  If you're using GNU Make, you can define an implicit rule like this (note the slightly different syntax from the standard implicit rule listed above):

%.o : %.asm
        $(NASM) $(NASMFLAGS) $< -o $@

Then simply tell make the object files needed and let it figure out by itself that it needs to run the compiler on C files and nasm on assembly files:

os : foo.o thing1.o thing2.o
        ld $(LDFLAGS) $^ -o $@

Don't include paths in filenames

Inserting and maintaining paths everywhere is tedious and error prone.  Better is to use VPATH within the Makefile like this:

VPATH=../src

Now assuming your directory tree looks like this:

.
├── build
└── src

You can navigate into the empty build directory and type make -f ../src/Makefile.  All of the binaries will be created in the build directory and the source directory will be untounched.  VPATH tells make where to look for sources it needs.  This is a much nicer way to do things.

Mark phony targets .PHONY

There isn't really an all file created and there really isn't a clean file.  These are actually just tags that identify a target whose rules should always be run.  For this reason they should be explicitly declared as phony:

.PHONY: clean
clean:
        rm -rf os $(OBJECT_FILES)

Explicitly name sources

Lines like this one:

OS_C_SRCS := $(wildcard *.c)

generally should be avoided.  The reason is that if you create, say, a small test file to try out a concept while working on the source and then forget to delete it, it becomes part of the build whether you wanted it or not.  It's usually far better to explicitly name files rather than using wildcards.

Putting it all together

Here's a revised Makefile which implements these suggestions:

Makefile

CFLAGS= -ffreestanding -nostdlib -gdwarf-4 -m32 -ggdb3 -Wall -Wextra -Werror -std=c11
NASM=nasm
NASMFLAGS= -f elf32 -F dwarf -g -w+all
LDFLAGS= --nmagic -m elf_i386 -L$(VPATH) --script=os.lds
VPATH=../src
OBJECT_FILES=foo.o thing1.o thing2.o

.PHONY: all
all: os

%.o : %.asm
        $(NASM) $(NASMFLAGS) $< -o $@

os : foo.o thing1.o thing2.o
        $(LD) $(LDFLAGS) $^ -o $@

.PHONY: clean
clean:
        rm -rf os $(OBJECT_FILES)

Create libraries

One thing I forgot to mention was that it's often good practice to create libraries.  By grouping related object code together into a library, you gain a number of advantages.  First, recompiling the project typically becomes a bit faster.  Second, it allows (forces?) you to think carefully about the interface.  A well-defined interface to library code not only makes it easier to use (and potentially replace with an improved or alternate version) but also aids in testing.  Third, by having a separate library, testing can often be simplified by writing tests that thoroughly exercise each library.  When all libraries are tested, it's easier to test the main application with confidence.


Script for handling PPA's on Ubuntu I'm writing a bash script to handle the PPA's on Ubuntu & derivatives. I was wondering how to know if I'm handling this the right way.

So, the script works (flawlessly), but I posted it to a blog and got this feedback :

  The way you wrote you script is the one we all use when we begin : we don't know the variables, so we use traditionnal commands, and in the end if we remove the "#" arguments from the script, the code is sad (maybe he meant 'poor' ?)

Here is a sample :

#! /bin/bash
SCRIPT="ppa-tool"
VERSION="2.0.1"
DATE="2014-03-26"
RELEASE="$(lsb_release -si) $(lsb_release -sr)"

[ -n "`echo "$1" | grep ppa`" ] && PPA=$1
[ -n "`echo "$2" | grep ppa`" ] && PPA=$2

helpsection()
{
echo -e "Usage : $SCRIPT [OPTION]... [PPA]...
-h, --help     shows this help
-c, --check    check if [PPA] is available for your release

Version $VERSION - $DATE"
exit 0
}

error() { echo -e "$SCRIPT - Oops, something went wrong\nTry « $SCRIPT --help » for more information." && exit 0 ; }

ppa_verification()
{
wget http://ppa.launchpad.net/$(echo $PPA | sed -e 's/ppa://g')/ubuntu/dists -O /tmp/"$SCRIPT-check.tmp" -q
if [[ -n "`cat "/tmp/$SCRIPT-check.tmp" | grep $(lsb_release -sc)`" ]] ; then
    echo -e "$SCRIPT : '$PPA' is available for $RELEASE"
else
    echo -e "$SCRIPT : '$PPA' is NOT available for $RELEASE"
fi
rm "/tmp/$SCRIPT-check.tmp"
}

[ "$1" == "--help" ] || [ "$1" == "-h" ] && helpsection

if [ "$1" == "--check" ] || [ "$1" == "-c" ] || [ "$1" == "check" ]  ; then
    [ -n "`echo $2`" ] && ppa_verification && exit 0
    error
fi

error

(complete script - also in French)

Apparently, my syntax is bad, but I don't see what I could do to improve this, and I don't understand the feedback I recieved. Could you help me figure it out ?

EDIT: From Unix&Linux (where I first posted, but was redirected here), someone said it wasn't good to use capital letters for variables. So $SCRIPT should become $script.

User experience

-h  or --help should print the usage message to standard output, which you do.  However, on any unrecognizable command line, you should print the same (or similar) message to standard error and exit with a non-zero status.
Similarly, other error messages should go to standard error and result in a non-zero status.

Style issues

Good job, double-quoting almost every variable you used.  A lot of shell programmers neglect to do so.
Good job, breaking the program up into functions.  A lot of shell programmers don't.
Indent your code consistently.  It's hard to see where your functions begin and end.
Make use of case to replace convoluted if statements.
Do all of your command-line parsing in one place, and have a coherent parsing strategy.  You can either use getopt (which has a bit of a learning curve) or build something based on shift (which is easier for a beginner to understand).
You can pass parameters to functions.  Passing $PPA to ppa_verification() would be more elegant than using a global.

There isn't 100% consensus on whether variable names should be $ALL_CAPS or $lower_case.  One common convention is $ALL_CAPS for global and/or exported variables, $lower_case for local or non-exported variables.

Use of wget

Instead of writing the output of wget to a temporary file, you could just pipe it to grep directly: wget -q -O - "$url" | grep -q "$release".
But then, to distinguish between a wget failure (e.g. network problem) and the release not being found by grep, you would have to interrogate ${PIPESTATUS[0]} and ${PIPESTATUS[1]}, which is a bit complicated.
Why not try to fetch the subdirectory of dists/ that you care about instead?  Then you only need to test whether the server returned an HTTP 200 (Success!) or HTTP 404 (Not Found).

#! /bin/bash
SCRIPT="ppa-tool"
VERSION="200_success"
DATE="2014-03-26"
RELEASE="$(lsb_release -si) $(lsb_release -sr)"

helpsection()
{
    echo "Usage : $SCRIPT [OPTION]... [PPA]...
-h, --help     shows this help
-c, --check    check if [PPA] is available for your release

Version $VERSION - $DATE"
}

ppa_verification()
{
    local ppa="${1#ppa:}"

    local codename="$(lsb_release -sc)"
    local url="http://ppa.launchpad.net/$ppa/ubuntu/dists/$codename/"

    wget "$url" -q -O /dev/null
    ######################################################################
    # Exit Status
    #
    # Wget may return one of several error codes if it encounters problems.
    # 0 No problems occurred.
    # 1 Generic error code.
    # 2 Parse error--for instance, when parsing command-line options, the `.wgetrc' or `.netrc'...
    # 3 File I/O error.
    # 4 Network failure.
    # 5 SSL verification failure.
    # 6 Username/password authentication failure.
    # 7 Protocol errors.
    # 8 Server issued an error response.
    ######################################################################
    case $? in
      0) # Success
        echo "$SCRIPT : '$ppa' is available for $RELEASE"
        ;;
      8) # HTTP 404 (Not Found) would result in wget returning 8
        echo "$SCRIPT : '$ppa' is NOT available for $RELEASE"
        return 1
        ;;
      *)
        echo "$SCRIPT : Error fetching $url" >&2
        return 3
    esac
}

PPA=
while [ -n "$*" ] ; do
    case "$1" in
      -h|--help)
        helpsection
        exit 0
        ;;
      --check=*)
        PPA="${1#*=}"
        ;;
      -c|--check|check)
        PPA="$2"
        shift
        ;;
      *)
        helpsection >&2
        exit 2
        ;;
    esac
    shift
done

if [ -z "$PPA" ]; then
    helpsection >&2
    exit 2
fi

ppa_verification "$PPA"


Converting NameValue pair entity to XML I have the following code, to store a web service name value pair response into DB. I am saving it in xml column of DB. Kindly highlight the issues with this approach. Please note that the name value pair (i.e response.details) array is dynamic and i cannot directly map it to individual db columns.

XElement ex = new XElement("ServiceResponse","");
        for (int i = 0; i < response.details.Length; i++)
        {
            string Name = Regex.Replace(response.details[i].name.Trim(),@"[^A-Za-z0-9.]+","");
            ex.Add(new XElement("Details",new XElement(Name, response.details[i].value)));
        }
using (DBDataContext con = new DBDataContext())
        {
            con.Requests.InsertOnSubmit(new Nettium.FraudManagementBase.DataContext.Request
            {
                response = ex
            }
                );
            con.SubmitChanges();
        }

Thanks

You could achieve a little more readability using Linq2Xml like this:

var ex = new XElement("ServiceResponse",
    response.details.Select(detail =>
        new XElement("Details",
            new XElement(CleanName(detail.name), detail.value))));

I have moved the line of code that performs some kind of cleaning on the detail's name to a separate method, like so:

private static string CleanName(string name)
{
    return Regex.Replace(name.Trim(), @"[^A-Za-z0-9.]+", "");
}

You could rename this method to communicate better what it does (for me it's not really obvious).

Two more things:

it's good to follow the C# naming guidelines. In your code, the local variables begin with a capital letter and the public fields/properties of the Request and Detail classes begin with lowercase letters. It should be the other way around.
you could add a using Nettium.FraudManagementBase.DataContext; clause at the top of the file. The data access code will look like this:

using (DBDataContext con = new DBDataContext())
{
    con.Requests.InsertOnSubmit(new Request { response = ex });
    con.SubmitChanges();
}

which is much cleaner IMO.

So, the final code could look like this:

var xml = new XElement("ServiceResponse",
    response.Details.Select(detail =>
        new XElement("Details",
            new XElement(CleanName(detail.Name), detail.Value))));

using (DBDataContext con = new DBDataContext())
{
    con.Requests.InsertOnSubmit(new Request { Response = xml });
    con.SubmitChanges();
}


Calculating the posterior distribution in multivariate Gaussian processes Matlab profiler throws this horrible bottleneck which is called 700000 times:

for k =1:model.nlf,
    for r =1:model.nlf,
        KuyDinvKyu = zeros(model.k,model.k);
        for q =1:model.nout,
            KuyDinvKyu = KuyDinvKyu + model.KuyDinv{k,q}*model.Kyu{q,r};
        end
        if (k == r)
            model.A{k,r} = model.Kuu{k} + KuyDinvKyu;
        else
            model.A{k,r} = KuyDinvKyu;
        end
    end
end

Even if the math is correct, there must be a faster way.

Assuming model.KuyDinv{k,q} and model.Kyu{q,r} are a matrices (as the name would imply), there is little you can do. You can move the initialization of KuyDinvKyu outside the loop, and eliminate the branch to compute the diagonals:

% define it once -- saves many calls to 'zeros'
KuyDinvKyu_0 = zeros(model.k);

for k = 1:model.nlf
    for r = 1:model.nlf

        KuyDinvKyu = KuyDinvKyu_0;

        for q = 1:model.nout
            KuyDinvKyu = KuyDinvKyu + model.KuyDinv{k,q}*model.Kyu{q,r};
        end

        % removed IF
        model.A{k,r} = KuyDinvKyu;

    end
end

% without IF
for c = 1:model.nlf
    model.A{c,c} = model.A{c,c} + model.Kuu{c};
end

If model.KuyDinv{k,q} and/or model.Kyu{q,r} contain scalars, well then we can optimize this much further for sure. So I need to know the size and type of data that model.Kyu{q,r} and  model.KuyDinv{k,q} will contain.

It might also be that your overall data design (e.g., the choice to use cells) is flawed and causes inefficiencies. So it could also be helpful to see some more surrounding code, so I can determine if there is some improvement to be made in that respect.


Binary search to identify duplicate user names I am trying to implement the code described this post about how Reddit identifies duplicate user names (I do believe it is called binary search). Just to clarify, this is mainly for me figuring out how these things work; thanks @Graipher for pointing to bisect). The way I understand it, the steps are:

You have a new user name you are comparing to an alphabetically sorted list of existing names
You compare that new name to the middle item in the list.
If there is no match, you check if the new name is higher or lower in the alphabet than the middle item, and focus on this side of the list, again checking the middle item.
You repeat until there is a match ("User name taken") or none ("User name available").

Below is what I have. It seems to work when I test it on the names in the list and random other names. It'd cool if someone had some feedback re:

Will this work correctly or is there some kind of bug?
Is the while loop the right way to go? Are there better options? I especially don't like the separate loop for a list of length 1.
Performance-wise, are there any real hiccups in here?
Any other comments are much appreciated!

-

def checker(new_name, old_names):
    """
    The checker takes a string new_name and checks if it is contained in the list old_names.
    It prints out the results.
    """
    print "***\n\ninitializing the checker"
    print "searching for ", new_name
    print "old names", old_names
    old_names= sorted(old_names)
    #set the match variable to false
    namematch= False
    #iterate over list while new_name does not match
    while not namematch and len(old_names) > 1:
        middle= len(old_names)/2
        print "middle item is", old_names[middle], "middle index is", middle
        #match
        if old_names[middle] == new_name:
            namematch= True
        #no match
        else:
            if sorted(list((new_name, old_names[middle])))[0] != new_name:
                print "word is to the right"
                old_names = old_names[middle:]
            else:
                print "word is to the left"
                old_names = old_names[:middle]
    #check for list of length 1: is it new_name
    if (len(old_names)) == 1 and (old_names[0] == new_name):
        namematch = True
    if not namematch:
        print "this name is not taken", new_name
    else:
        print "this name is taken:", new_name

Here is my test data:

-

names_list= [u'annie', u'max', u'chris', u'11alpha', u'zotti', u'zatti', u'zutti', u'?andy', u'getr\xfc', u'zilly']

checker('max', names_list)  # True
checker('fail', names_list)  # False

The description you are providing and your implementation do correspond to binary search indeed. As such, it would probably make sense to perform a few cosmetic changes to your function to make this cleaner and easier to (re)use:

call it binary_search (or something like that)
makes the signature (parameter names and documentation) more generic and not based on the concept of name. The parameters could be called item/element/x/needle and sorted_list/sorted_array.
make the return value clearer, for instance a boolean value telling whether the value was found or not (or maybe an index giving the position)
move all the input/output logic (except maybe for the debug statements) out of the function.

As for the performances, I am a bit concerned by the fact that you are actually performing copies by using the [:] sublist notation. Thus, each step of the algorithm, instead of operating in constant times will be bigger if the sublist you are copying gets bigger.
Also, you are re-sorting the whole array everytime the function is called (and even more that this apparently). The point of using binary search is that you pay the cost of sorting (O(n*log(n))) upfront assuming you'll amortize it by performing many searches in O(log(n)) times. If you have to re-sort everytime, you might as well use a linear search iterating through the array and stopping when the element is found.


Tic-Tac-Toe code using Python with Tkinter `I made this Tic-Tac-Toe game first, by using user input in the console here:
https://pastebin.com/6zLjrWcf
, and now new and improved using Tkinter:

import tkinter as tk
from tkinter import ttk
import time

def create_button(relx, rely):
    button = tk.Button(width=10, height=2, command=lambda: callback(button))
    button.place(relx=relx, rely=rely)
    return button

def check_win():
    if (buttons[0]['text'] == buttons[1]['text'] == buttons[2]['text'] != '') or \
            (buttons[3]['text'] == buttons[4]['text'] == buttons[5]['text'] != '') or \
            (buttons[6]['text'] == buttons[7]['text'] == buttons[8]['text'] != '') or \
            (buttons[0]['text'] == buttons[3]['text'] == buttons[6]['text'] != '') or \
            (buttons[1]['text'] == buttons[4]['text'] == buttons[7]['text'] != '') or \
            (buttons[2]['text'] == buttons[5]['text'] == buttons[8]['text'] != '') or \
            (buttons[2]['text'] == buttons[4]['text'] == buttons[6]['text'] != '') or \
            (buttons[0]['text'] == buttons[4]['text'] == buttons[8]['text'] != ''):
        return True
    else:
        return False

def callback(button):
    global turn, x
    if x == 1:
        time.sleep(1)
        game.quit()
    invalid['text'] = ''
    if button['text'] != '':
        invalid['text'] = 'Invalid space try again'
        return
    button['text'] = turn
    if check_win():
        invalid['text'] = 'Player ' + turn + ' WINS!!!!!'
        x = 1
    turn = ('0' if turn == 'X' else 'X')
    label_button['text'] = 'PLAYER ' + turn + '\'S TURN.'

x = 0
turn = 'X'
game = tk.Tk()
game.title('TicTacToe')
game.geometry('700x500')
buttons = []
for i in range(1, 10):
    button_created = create_button(0.25 if i / 3 <= 1 else 0.45 if i / 3 <= 2 else 0.65, 0.2 if i in [1, 4, 7] else
                                   0.4 if i in [2, 5, 8] else 0.6)
    buttons.append(button_created)
label_button = ttk.Button(game, text='PLAYER ' + turn + '\'S TURN.', style='Fun.TButton', width=20, state='disabled')
label_button.pack(pady=30)
invalid = tk.Label(text='')
invalid.place(relx=0.4, rely=0.12)
game.mainloop()

My main question is if there is a way to compact check_win()?
Also please review the rest of the code.

Let us begin with some layout. This is a typical grid use case so that you won't have to manually manage grid. At the lowest level, a grid structure essentially places buttons by calculating coordinates. But this gets tedious over time, that's why a grid layout is provided. Also, it was a clever way of cheking win by being symbol agnostic. A normal tic tac toe game would have used checkwin(symbol) to determine win.

On strings

To escape ' within ' ' you can use " instead. From

'\'S TURN.'

to

"'S TURN."

and you can also use string formatting to clear up some clutter.

"PLAYER {}'S TURN.".format(turn)

On layout

Modifying your create_button function to this allows a grid structure

def create_button(x, y):
    button = tk.Button(width=10, height=2, command=lambda: callback(button))
    button.grid(row=x, column=y)
    return button

Then we modify others since different layouts can't be mixed

label_button = ttk.Button(
                        game,
                        text="PLAYER {}'S TURN.".format(turn),
                        style='Fun.TButton', width=20,
                        state='disabled')
label_button.grid(row=0, column=1)
invalid = tk.Label(text='')
invalid.grid(row=4, column=1)

adding the buttons can be then done as

buttons = []

buttons.append(create_button(1, 0))
buttons.append(create_button(1, 1))
buttons.append(create_button(1, 2))
buttons.append(create_button(2, 0))
buttons.append(create_button(2, 1))
buttons.append(create_button(2, 2))
buttons.append(create_button(3, 0))
buttons.append(create_button(3, 1))
buttons.append(create_button(3, 2))

You can use a loop for the row and itertools.cycle for the 0, 1, 2 if you want to simplify it.

The check_win function

Simplifying if

Adding a () to if statements allows you to write or without \

if ... :
    ...

to

if (...):
    ...

thus the win_function can be simplified from

if (buttons[0]['text'] == buttons[1]['text'] == buttons[2]['text'] != '') or \
(buttons[3]['text'] == buttons[4]['text'] == buttons[5]['text'] != '') or \

to

def check_win():
    if (
        (buttons[0]['text'] == buttons[1]['text'] == buttons[2]['text'] != '') or
        (buttons[3]['text'] == buttons[4]['text'] == buttons[5]['text'] != '') or
        ...
        ):
        return True
    else:
        return False

Simplifying values write-up

This can also be further simplified by defining a function to replace buttons[0]['text']

def btext(i):
    return buttons[i]['text']

and using it

def check_win():
    if (
        (btext(0) == btext(1) == btext(2) != '') or
        (btext(3) == btext(4) == btext(5) != '') or
        (btext(6) == btext(7) == btext(8) != '') or
        (btext(0) == btext(3) == btext(6) != '') or
        (btext(1) == btext(4) == btext(7) != '') or
        (btext(2) == btext(5) == btext(8) != '') or
        (btext(2) == btext(4) == btext(6) != '') or
        (btext(0) == btext(6) == btext(8) != '')
        ):
        return True
    else:
        return False

On architecture

A common pattern is the MVC (Model, View, Controller). While checking and updating gui directly works here, you might consider adding states in a structure like this:

board = [
    ['', '', ''],
    ['', '', ''],
    ['', '', '']
]

Operations are done on this and the gui is updated according to this.


Separate a module into smaller parts The file is quite long, 1000 LOC so I want to separate it into smaller files.

https://github.com/anvoz/world-js/blob/v1.0/js/world.core.js

Here is a brief version of the code:

(function(window, undefined) {
    var WorldJS = window.WorldJS = function() {
        // WorldJS Constructor
        this.nextSeedId = 1;
        this.Statistic = { population: 0 };
        this.Knowledge = {
            completed: [],
            gain: function(world) { /* ... */ }
        };
    }
    WorldJS.prototype.someMethods = function() {};

    var Seed = WorldJS.prototype.Seed = function() {
        // Seed Constructor
    };
    Seed.prototype.someMethods = function() {};
})(window);

// Create a world
var world = new WorldJS();
// Create a seed
var seed = new world.Seed();

Seems like I was right with the Seed class. So I can put Seed in a new file. Like this:

(function(window, undefined) {
    var WorldJS = window.WorldJS;
    var Seed = WorldJS.prototype.Seed = function() {
        // Seed Constructor
    };
    Seed.prototype.someMethods = function() {};
})(window);

How can I put the Knowledge property to a new file? Is it a good practice if I change Knowledge to a class just like the Seed class and use a new lowercase property to hold data like this:

world.knowledge = new world.Knowledge();

I don't see the advantage of adding Seed and Knowledge into the prototype of World (unless you've got more code to tell me otherwise). If you don't need anything from the instance at all, then you don't need them in the instance. You can put them like static members instead.

With that, you can do what jQuery did. jQuery's jQuery and $ point to a constructor function that builds jQuery objects. That's why you can do jQuery() and $().

But in JS, functions are objects and like any other object, you can add properties. It's the same reason why you can also do jQuery.each or $.each. Basically they made their constructor their namespace as well.

So you can do the following to World:

(function(window){

  var World = window.World = function(){/*World constructor code*/};

  World.prototype.someFn = function(){/*...*/};

}(window));

And like how non-instance jQuery plugins extend (and yes, you can place this in another file. Just make sure the World library is loaded first):

(function(World){

  //This part would be synonymous to $.somePlugin = function(){...}
  var Seed = World.Seed = function(){/*Seed constructor code*/};

  Seed.prototype.someFn = function(){/*...*/};

}(World));

To use them:

var myWorld = new World();        //Using World as a constructor
var mangoSeed = new World.Seed(); //Using World to access the Seed constructor


Finger painting code I have a very simple view that handles touch events and draws accordingly. It's nothing significant, but it does use a bit more CPU than I would like (35%). Again, it is the bare minimum (<90 lines), but I can't really think of a simpler way to draw using user input. What would you change?

import UIKit

class DrawView: UIView {

    var mainImageView = UIImageView()

    var tempImageView = UIImageView()

    var r = CGFloat(0)
    var g = CGFloat(0)
    var b = CGFloat(0)
    var a = CGFloat(1)

    var lineWidth = CGFloat(5)

    var dot = true
    var lastPoint = CGPoint.zeroPoint

    required init(coder aDecoder: NSCoder) {
        super.init(coder: aDecoder)
        self.backgroundColor = UIColor.clearColor()
        self.multipleTouchEnabled = false
        mainImageView.frame = self.frame
        tempImageView.frame = self.frame
        self.addSubview(mainImageView)
        self.addSubview(tempImageView)
    }

    override func touchesBegan(touches: Set<NSObject>, withEvent event: UIEvent) {
        dot = true
        if let touch = touches.first as? UITouch {
            lastPoint = touch.locationInView(self)
        }
    }

    func drawLineFrom(fromPoint: CGPoint, toPoint: CGPoint) {
        UIGraphicsBeginImageContextWithOptions(self.frame.size, false, 0)
        let context = UIGraphicsGetCurrentContext()
        tempImageView.image?.drawInRect(self.frame)

        CGContextMoveToPoint(context, fromPoint.x, fromPoint.y)
        CGContextAddLineToPoint(context, toPoint.x, toPoint.y)

        CGContextSetLineCap(context, kCGLineCapRound)
        CGContextSetLineWidth(context, lineWidth)
        CGContextSetRGBStrokeColor(context, r, g, b, a)
        CGContextSetBlendMode(context, kCGBlendModeNormal)

        CGContextStrokePath(context)

        tempImageView.image = UIGraphicsGetImageFromCurrentImageContext()
        UIGraphicsEndImageContext()
    }

    override func touchesMoved(touches: Set<NSObject>, withEvent event: UIEvent) {
        dot = false
        if let touch = touches.first as? UITouch {
            let currentPoint = touch.locationInView(self)
            drawLineFrom(lastPoint, toPoint: currentPoint)
            lastPoint = currentPoint
        }
    }

    override func touchesEnded(touches: Set<NSObject>, withEvent event: UIEvent) {
        if dot {
            drawLineFrom(lastPoint, toPoint: lastPoint)
        }

        UIGraphicsBeginImageContextWithOptions(self.frame.size, false, 0)
        mainImageView.image?.drawInRect(self.frame, blendMode: kCGBlendModeNormal, alpha: a)
        tempImageView.image?.drawInRect(self.frame, blendMode: kCGBlendModeNormal, alpha: a)
        mainImageView.image = UIGraphicsGetImageFromCurrentImageContext()
        UIGraphicsEndImageContext()

        tempImageView.image = nil
    }

    func setColor(color: UIColor) {
        let components = CGColorGetComponents(color.CGColor)
        r = components[0]
        g = components[1]
        b = components[2]
    }

    func erase() {
        mainImageView.image = nil
    }

}

The first thing that strikes me as odd about your code is having the four single-character variables to represent color components.  I would rather see a single drawColor variable with the UIColor type.  Not only does this eliminate the need for the setColor method, but it also more easily lets us pull that color out for numerous reasons (make some other thing the same color as our DrawView's current drawColor, or see if the DrawView's drawColor is the same shade as something else, etc.  There are plenty of reasons. Then only internal to the specific necessary methods do we break that color down into its components for the necessary Core Graphics function call.

Our code has no sense of organization.  We should put our event-handling methods together, rather than separate them by the big drawLine method.

Our drawLine method isn't very nicely named.  First of all, we can inspect the method to know that it expects points, and the "from" outside of the parenthesis looks and feels very odd.  I'd prefer the method be declared something more like this:

drawLine(from fromPoint: CGPoint, to toPoint: CGPoint)

Which means when it's called by an end user, the call looks like this:

drawView.drawLine(from: pointA, to: pointB)

But internally, we refer to the points as the fromPoint and the toPoint.

mainImageView.frame = self.frame
tempImageView.frame = self.frame

This bit isn't really going to work.  If these two views are subviews of the DrawView and we intend for them to be in the same spot, we need to better.

For a start, their frame property should be set to self's bounds property.  For any UIView, the frame describes its position in its superview, so the X and Y will be how far it is from the top left corner.  Meanwhile, its bounds property will always have an X and Y of zero.  So if I set my subview's frame to be equal to its parent's bounds property, it will be the same size, but it will be positioned to the parent's top left corner--and I think this is what we're actually trying to achieve right?

But even this isn't enough.  What happens when self is resized by something?  Now our manually sized subviews are no longer the correct size.  We need to programmatically add autolayout constraints so that the four edges of these subviews are pinned to its parent view, so they will always be the same size.

Ultimately, there's no real reason to use our image view subviews.  Using Core Graphics, we can add lines like this directly to any view.  This Code Review question shows an example of drawing lines directly on a view.  The only reason we need an image view is if we want to display a UIImage.  And there's no need to do so here.

Even if we want to be able to generate a UIImage from what the user has drawn on our DrawView, there are means for doing that in iOS (that question is Objective-C, but it can be done in Swift too).  So there really is no reason to be using the image views here at all.


Check WiFi Connectivity and internet connectivity in Background thread - with a return value I have written this class to check internet connectivity on Android, while there are many instances when WiFi is connected is taken as an implication that internet is connected as well. I wanted to have a method which actually checks if internet is connected. One issue was to run the internet code in a background thread and while doing so - return a value, so I used a future task, below is the code:

public class CheckWiFi {
public static String TAG = "Check Internet";

public static boolean isWiFiConnected(Context ctx) {
    ConnectivityManager cm = (ConnectivityManager) ctx.getSystemService(Context.CONNECTIVITY_SERVICE);
    NetworkInfo ni = cm.getActiveNetworkInfo();
    return ni != null;
}

public static Boolean isNetworkConnected(Context ctx) {

    ExecutorService service = Executors.newFixedThreadPool(10);

    FutureTask<Boolean> task1 = new FutureTask<Boolean>(new CheckConnection(ctx));
    service.submit(task1);
    try {
        return task1.get();
    } catch (java.util.concurrent.ExecutionException ex) {
        ex.printStackTrace();
        return false;
    } catch (InterruptedException ex) {
        ex.printStackTrace();
        return false;
    }

}

public static class CheckConnection implements Callable {
    Context ctx;

    public CheckConnection(Context ctx) {
        this.ctx = ctx;
    }

    @Override
    public Boolean call() {
        if (isWiFiConnected(ctx)) {
            try {
                HttpURLConnection urlc = (HttpURLConnection)
                        (new URL("http://clients3.google.com/generate_204").openConnection());
                urlc.setRequestProperty("User-Agent", "Android");
                urlc.setRequestProperty("Connection", "close");
                urlc.setConnectTimeout(1500);
                urlc.connect();
                return (urlc.getResponseCode() == 204 && urlc.getContentLength() == 0);
            } catch (IOException e) {
                Log.e(TAG, "Error checking internet connection", e);
            }
        } else {
            Log.d(TAG, "No network available!");
        }
        return false;
    }

}

}

What are your thoughts about it?

Also that I am getting a warning:

Unchecked assignment: 'network.CheckWiFi.CheckConnection' to 'java.util.concurrent.Callable<java.lang.Boolean>'

Your implementation of isNetworkConnected does not make much sense for the following reasons:

You create a new, private ExecutorService each time the method runs. The intention behind Executors is that they're created once and then used over a longer time span to execute multiple (short) tasks. This should save you from having to create a new Thread for each task, because the executor manages a number of threads which get re-used for multiple tasks in succession.
You don't need that FutureTask<Boolean>. The usual idiom of the ExecutorService is like:

Callable myCallable = new CheckConnection(...);
Future future = service.submit( myCallable );
...
Boolean result = future.get();
It does not make any sense to submit a (single) task to an executor and then start waiting for completion immediately. This will block exactly as if there was no Executor involved at all. In your case, you could just write Boolean result = new CheckConnection(...).call(); with the same effect w.r.t. concurrency.

Suggestion

Don't run your checking code and wait for the result. Better have your checking code run in the background periodically (e.g. in a dedicated Thread or a ScheduledExecutorService) and have that code post its result back to any interested party.

You can use a Handler to send a message from a background thread to the UI thread. For example, send one message each time the connectivity state changes.


Having graphics resize in line with the Frame in Swing I have drawn a box within my JPanel so that it resizes appropriately when the JFrame is resized.

This is how I implement it:

public void drawBox(Graphics g){
    int num1 = getWidth()/4, num2 = getHeight()/4, num3 = getWidth() - num1,
            num4 = getHeight() - num2;
    g.drawLine(num1, num2, num3, num2);
    g.drawLine(num1, num2, num1, num4);
    g.drawLine(num3, num2, num3, num4);
    g.drawLine(num1, num4, num3, num4);
}

What I have done works but is there a better way to do this? I'd like general feedback, cleaner code would be my main priority though.

Naming and Initialization

It's very hard to read when many variables are initialized on a single line.

numN naming pattern is also confusing. Since they correspond to coordinates, why not name them like this?

final int topLeftX = getWidth()/4
final int topLeftY = getHeight()/4
final int bottomRightX = ...

The 4 number may also be extracted into a vairable.

Draw a Rectangle

If I'm not mistaken, with drawLine(args) you are trying to draw a rectangle. It may be replaced with the respective method call, thus reducing the stuff to a single line:

g.drawRect(topLeftX, topLeftY, width, height);


Quicklyish finding primes by trial division I ask because it seems to be taking waaaay more space/time than it should. Also, I know that if I want to be super efficient I should use Java's BitSet and a Sieve of Eratosthenes, but I wrote it like this to compare to a friend's implementation in C#.

(defn primes-upto [n]
  (let [primes (transient [2 3])]
    (loop [i 5]
      (if (> i n)
        (persistent! primes)
        (do (when
                (loop [j 0 is-prime true]
                  (let [p (primes j)]
                    (when is-prime
                      (if (> (* p p) i)
                        true
                        (recur (inc j) (not= 0 (mod i p)))))))
              (conj! primes i))
            (recur (+ i 2)))))))

I get (primes-upto 100000) taking about 40 ms. I can't make it go much faster, but I can make it reliable. At present, it isn't.

The program works:

(primes-upto 10) ;[2 3 5 7]

But if you take away the transients, it fails:

(defn primes-upto [n]
  (let [primes [2 3]]
    (loop [i 5]
      (if (> i n)
        primes
        (do (when
              (loop [j 0, is-prime true]
                (let [p (primes j)]
                  (when is-prime
                    (or (> (* p p) i)
                        (recur (inc j) (not= 0 (mod i p)))))))
              (conj primes i))
          (recur (+ i 2)))))))

(primes-upto 10); java.lang.IndexOutOfBoundsException: null ...

The program is depending upon accidents of how transients are implemented. Transients don't promise to update their bindings. They revoke the promise of immutability.

Rewriting the program idiomatically, I get ...

(defn primes-upto [n]
  (loop [primes [2 3], i 5]
    (if (> i n)
      primes
      (let [i-prime? (loop [j 1]
                       (let [p (long (primes j))]
                         (or (> (* p p) i)
                             (and (not= 0 (mod i p))
                                  (recur (inc j))))))]
        (recur (if i-prime? (conj primes i) primes) (+ i 2))))))

... including a few variations:

The inner loop is simpler, and employs and and or for brevity.
Since we increment i by 2, we don't need to check for division by
2, so start with j at 1.
Convert p to a long explicitly, to help with type inference.
Dispense with transients.

When all's said and done, it's a little faster, but not much. Transients don't help much either.


NAry Tree implementation In C# Here I implement N-Ary tree in C#.

namespace ConsoleApplication1
{

    public class tree {
        public int info;
        public tree child;
        public tree siblings;

        public tree(int data)
        {
            info = data;
        }
    }

    public class NAryTree
    {
        public tree ROOT;
        public tree search(tree root, int data)
        {
            if (root == null)
                return null;

            if (data == root.info)
                return root;

            tree t = search(root.child, data);
            if (t == null)
                t = search(root.siblings, data);

            return t;
        }

        public void CreateNarray( int[] data)
        {  tree temp  = null;
            if(ROOT != null)
             temp = search(ROOT,data[0]);

            if (temp == null)
            {
                temp = new tree(data[0]);

            }
            if (this.ROOT == null)
                ROOT = temp;
            tree parent = temp;

            for (int j = 0; j < data[1]; j++)
            {
                // for first child
                if (j == 0)
                {
                    parent.child = new tree(data[j+2]);
                    parent = parent.child;
                }
                //for all other childs
                else
                {
                    parent.siblings = new tree(data[j+2]);
                    parent = parent.siblings;
                }

            }

        }

        public void Print()
        {
            preorder(ROOT);
        }
       public void preorder( tree p)
         {
        if(p==null)
        return;
         Console.WriteLine(""+p.info);
         preorder(p.child);

        preorder(p.siblings);
           }
    }

    class Program
    {
        static void Main(string[] args)
        {

            NAryTree naryTree = new NAryTree();
            //1st element in each row is node Value, 2nd - no of child, ,>=3rd....=>value of child

              int[][] data={new int[] {1,3,2,3,4}, new int[] {2,3,1,6,50},new int[]{3,3,8,9,10},new int[]{4,3,0,0,0,0}};

              for (int i = 0; i < data.GetLength(0); i++ )
              {
                 naryTree.CreateNarray(  data[i]);
              }

              naryTree.Print();
            Console.ReadLine();
        }
    }
}

Sorry for My Sub Standard English.

There are some points I would like to address to you, but I am missing a little bit the purpose of your exercise. From what I understand, every node of your tree can have only one child and only one sibling. Is it a feature or maybe you should change the Tree class to have as type for both Child and Siblings as IEnumerable<Tree>?

One thing that usually makes reviewer happy, is to give a consistant and understandable naming through the whole application. For example, I think it would improve readability renaming the class Tree to Node, and the field Info to Value.

I would avoid, if possible, the usage of fields and I would stick with public properties. A good explanation about why can be found here. For now then, your Tree class should look like this:

public class Node
{
    public int Value { get; set; }
    public Node Children { get; set; }
    public Node Siblings { get; set; }

    public Node(int data)
    {
        Value = data;
    }
}

If you are trying to implement a N-ary tree, I guess you would like that (as by definition), you won't have more than N children for each node. As you implemented it, as I was stating in my first request, you're only implementing a 1-ary tree.
For a N-ary tree, an idea could be adding N as property of the NAryTree, and change the types accordingly to avoid having more than N children for each node.

For the NAryTree, if I only read to signatures of methods, I would get the wrong idea about what you're trying to do. Print() gives me the idea you're trying to display on screen the content of the tree, but PreOrder() leads me to think you're sorting an unsorted tree (so, this is a bad name), and CreateNArray(int[] data) makes me think you're creating a new N-Array from scratch (again, which is your purpose here? I can't understand if a better name would be Initialize(int[] data) or Add(int[] data)).
Another suggestion I would like to give you, is to extract an interface for your NAryTree: this would  Accordingly to what I was saying before, I would rename Print() to ToString() and return a string as type.
Maybe adding the AddChild(int value) to the Node class would help you to keep easier the implementation of the CreateNArray method, if I got what you're trying to do.


Toggling ActionBar visibility for SherlockFragmentActivity Method implemented in parent class (extends SherlockFragmentActivity) (source)

ViewPager viewPager; // initialized in onCreate method
public void toggleActionBar() {
    try {
        FrameLayout.LayoutParams lp = new FrameLayout.LayoutParams(
                FrameLayout.LayoutParams.MATCH_PARENT,
                FrameLayout.LayoutParams.WRAP_CONTENT);
        if (getSupportActionBar().isShowing()) {
            getSupportActionBar().hide();
            lp.topMargin = 0;
        } else {
            getSupportActionBar().show();
            lp.topMargin = getSupportActionBar().getHeight();
        }
        viewPager.setLayoutParams(lp);
    } catch (Throwable t) {
        Log.e(TAG, "Toggle of Actionbar failed", t);
    }
}

Call made from one of Fragments in ViewPager (source)

try {
    ((DilbertFragmentActivity) getSherlockActivity()).toggleActionBar();
} catch (Throwable t) {
    Log.e(TAG, "Toggle of Actionbar failed", t);
}

MinSdkVersion is equal 8, ActionBar support is done by ActionBarSherlock library.

Three points:

getSupportActionBar() is called four times within the same method. Store it as a local variable and use the variable four times instead, to improve code cleaniness and readability.
When catching exceptions, be as specific as possible. Catching Throwable is a horrible idea. Only catch the exceptions you need to catch (Which involves neither Errors or RuntimeExceptions. You shouldn't even wrap ((DilbertFragmentActivity) getSherlockActivity()).toggleActionBar(); inside a try-catch statement. If you think that something can go wrong (like a NullPointerException), make sure that it doesn't go wrong before you try to do it (by checking if something is not null for example).
If viewPager can be marked as private, it is good practice to also mark it as private.


Removing properties and elements set to null in JavaScript I'm looking for a review of the following code.

Is there a better way to remove all properties that evaluate to truthy for a given function?
Is there a better way to prevent a stack overflow in the recursive function?

This is the entire Node.js module:

// The removePropertiesWhereTruthy function accepts the following parameters:
//
// 1. an object or array
// 2. a function to call against each non-object or non-array to determine if it's a truthy value according to that function (e.g. _.isNull)
// 3. An optional maxRecursion value to prevent a stack-overflow when there's a circular reference or exceptionally deeply nested objects.
//
'use strict';

var _  = require('lodash');

function removePropertiesWhereTruthy(obj,func,maxRecursion) {

  maxRecursion = _.isNumber(maxRecursion) ? maxRecursion - 1 : 20;
  if(maxRecursion < 0) {
    // prevent stack overflow by allowing caller to set a max time for
    // recursion and default to 20 if not set.
    return;
  }
  if(_.isObject(obj) && !_.isArray(obj)) {
    _.forOwn(obj, function(v, k) {
      if(_.isObject(v)) { // also true for array
        return removePropertiesWhereTruthy(v,func,maxRecursion);
      } else if(func(v)) {
        delete obj[k];
      }
    });
  } else if(_.isArray(obj)) {
    _.remove(obj, function(item){
      return func(item);
    });
    _.each(obj, function(v){
      if(_.isObject(v)) { // also true for (nested) array
        return removePropertiesWhereTruthy(v,func,maxRecursion);
      }
    });
  } else {
    throw new Error('This function should only be called with objects and arrays');
  }
}

function removePropertiesWithNullValues(obj, maxRecursion) {
  return removePropertiesWhereTruthy(obj, _.isNull, maxRecursion);
}

module.exports = {
  removePropertiesWithNullValues: removePropertiesWithNullValues
};

I can't think of any good use-case for this method. It may help to describe some examples of why you need this functionality.

That said, a few issues I see:

func could be named better (predicate perhaps?).
maxRecursion seems a bit unstable. This is where I have a hard time seeing the purpose of this method. What happens if you hit level 20 and it hasn't fully run it's course? Now you've got an object that is in a half-complete state without any way to know it. What if it spends 20 cycles on the first property which happens to be a circular reference?
I think the use of lodash here adds confusion to what would otherwise be pretty straightforward javascript (opinion).

Some fixes:

Don't use a recursive function. You could use a stack or a queue here to traverse the object tree.
Rather than limit the depth to avoid stack overflows for circular references, you could try to detect them by keeping a table of references and avoid re-traversing those trees altogether.
Rather than modifying an existing object, you could create a new one. That way, this method wouldn't produce any side-effects and potentially leave you in an unstable state. At minimum, you could return true/false if the method succeeded.


Implementing `Applicative Cons` Given the following definition:

data Cons a = Cons a (Cons a)
             | Empty
             deriving Show

I implemented the following attempt at its Applicative instance:

instance Applicative Cons where
    pure x               = Cons x Empty
    Empty <*> _          = Empty
    (Cons g gs) <*> x    = append (fmap g x) (gs <*> x)

append :: Cons a -> Cons a -> Cons a
append Empty x           = x
append x Empty           = x
append (Cons x Empty) ys = Cons x ys
append (Cons x xs) ys    = Cons x (append xs ys)

Please review this implementation - for correctness and whether it's idiomatic/verbose, etc.

As for being idiomatic, the only thing I'd change is the use of the variable g to f. Using g when f is available as a name is like using y when x is available.

There is significant redundancy in append, though. A sufficient implementation looks like this:

append :: Cons a -> Cons a -> Cons a
append Empty x           = x
append (Cons x xs) ys    = Cons x (append xs ys)

You could argue that your second pattern was an optimization in the case that the second list is empty, but the tradeoff is adding O(n) overhead every time the second list isn't empty. There's a reason (++) doesn't do that check for [a].  The third case was pure overhead, and always an improvement to remove.


Simple budget program I've decided to start working on a budget application for the purposes of enhancing my Python skills. In its current form this program takes given expenses and income and tells the user whether they have a surplus, a deficit, or if they are breaking even. I will add more functionality to this program as I move along in the development process.

import os
import sys

class Application():
    def __init__(self):
        self.income = 0
        self.expenses = 0
        self.expense_list = []
        self.expense_name = []
        self.income_name = []
        self.income_list = []
        self.prompt_income()

    def income_ask(self):
        add_income = input('Add income? [y/n]: ')
        return add_income

    def income_sum(self):
        self.income = sum(self.income_list)

    def expense_ask(self):
        add_expense = input('Add expense? [y/n]: ')
        return add_expense

    def expense_sum(self):
        self.expenses = sum(self.expense_list)

    def income_check(self):
        if not self.income_list:
            print('Please enter atleast one source of income. ')
            self.prompt_income()
        else:
            return

    def expense_check(self):
        if not self.expense_list:
            print('Please enter atleast one expense. ')
            self.prompt_expense()
        else:
            return

    def prompt_income(self):
        x = False
        while not x:
            result = self.income_ask()
            if result == 'y':
                income_input = int(input('Enter source of income. [Numbers Only]: '))
                self.income_list.append(income_input)
                income_name = input('Enter income name. [Name Only]: ')
                self.income_name.append(income_name)
            else:
                self.income_check()
                x = True
        self.income_sum()
        name = [name for name in self.income_name]
        income = [income for income in self.income_list]
        incomedict = dict(zip(name, income))
        for k in incomedict:
            print(k + ': ', '$' + str(incomedict[k]))
        print('Total user income: ', '$' + str(self.income))
        self.prompt_expense()

    def prompt_expense(self):
        x = False
        while not x:
            result = self.expense_ask()
            if result == 'y':
                expense_input = int(input('Enter expense amount. [Numbers Only]: '))
                self.expense_list.append(expense_input)
                expense_name = input('Enter expense name. [Name Only]: ')
                self.expense_name.append(expense_name)
            else:
                self.expense_check()
                x = True
        self.expense_sum()
        name = [name for name in self.expense_name]
        expense = [income for income in self.expense_list]
        expensedict = dict(zip(name, expense))
        for k in expensedict:
            print(k + ': ', '$' + str(expensedict[k]))
        print('Total user expenses: ', '$' + str(self.expenses))
        self.uservalue()

    def uservalue(self):
        valoutput = self.income - self.expenses
        if valoutput < 0:
            print('You are in the negative, you have a deficit of ' + '$' + str(valoutput))
        if valoutput == 0:
            print('You have broken even, you are spending exactly as much as you make.')
        if valoutput > 0:
            print('You are in the positive, you have a surplus of ' + '$' + str(valoutput))
        another = input('Would you like to run another analysis? [y/n]: ')
        if another == 'y':
            self.reset_program()
        else:
            self.close_program()

    def reset_program(self):
        self.income = 0
        self.expenses = 0
        del self.expense_list[0:]
        del self.expense_name[0:]
        del self.income_name[0:]
        del self.income_list[0:]
        self.prompt_income()

    def close_program(self):
        print('Exiting Program.')
        sys.exit(0)

if __name__ == '__main__':
Application()

Here are the few things I've noticed after a first glance on the program:

import os is not used, you can remove it
() after the class Application can be omitted
keep two blank lines between the imports and the class definition, after the class code and the if __name__ == '__main__' line
variable naming - there are some poor variable names like x or k - consider renaming them to something more descriptive
the following code block can be removed (unless you have it there as a stub for future development):

else:
    return

the class itself feels overloaded - this may be because it does two different not-directly related things at the same time - prompting the user and making budget calculations - since you are going to continue to develop the class - make sure Application is not becoming a God Object
the class and its methods would benefit from having detailed documentation strings

As a side note, take a look at the The Architecture of Open Source Applications project - it is really pure informational gold in terms of designing, thinking and development of your software project.


MySQL JSON diff procedure I'm building a procedure which compares two JSON objects and return two JSON objects with differences between each (compared keys and values).
But I have relatively superficial SQL knowledge, so I need your review to avoid any simple mistakes and improve my code.

Compares "first" against "second" and returns the values in "first" that are not present in "second" OR has different value

DELIMITER $$

DROP PROCEDURE IF EXISTS json_diff $$

# Compares "first" against "second" and
# returns the values in "first" that are not present in "second" OR has different value
CREATE PROCEDURE json_diff(IN first JSON, IN second JSON, INOUT firstResult JSON, INOUT secondResult JSON)
BEGIN
    DECLARE oldKey VARCHAR(300);
    DECLARE oldVal, newVal INT;
    DECLARE i, secondHasKey INT DEFAULT 0;
    DECLARE oldKeys, newKeys JSON;

    SELECT JSON_KEYS(first) INTO oldKeys;
    SELECT JSON_KEYS(second) INTO newKeys;

    WHILE i < JSON_LENGTH(oldKeys) DO
        SELECT JSON_EXTRACT(oldKeys, CONCAT('$[',i,']')) INTO oldKey;
        SELECT JSON_EXTRACT(first, CONCAT('$.', oldKey)) INTO oldVal;

        SELECT JSON_CONTAINS_PATH(second, 'one', CONCAT('$.', oldKey)) INTO secondHasKey;

        IF secondHasKey = 1 THEN
            SELECT JSON_EXTRACT(second, CONCAT('$.', oldKey)) INTO newVal;
            IF oldVal != newVal THEN
                SELECT JSON_INSERT(firstResult, CONCAT('$.', oldKey), oldVal) INTO firstResult;
                SELECT JSON_INSERT(secondResult, CONCAT('$.',oldKey), newVal) INTO secondResult;
            end if;
        ELSE
            SELECT JSON_INSERT(firstResult, CONCAT('$.', oldKey), oldVal) INTO firstResult;
        end if;

        SET i = i + 1;
    end while;
END $$

Use above PROCEDURE to make bidirectional comparison.

DELIMITER $$

DROP PROCEDURE IF EXISTS json_diff_both $$

CREATE PROCEDURE json_diff_both(IN first JSON, IN second JSON, INOUT firstResult JSON, INOUT secondResult JSON)
BEGIN
    CALL json_diff(first, second, firstResult, secondResult);
    CALL json_diff(second, first, secondResult, firstResult);
END $$

You are missing a second part of the diff.

I've added the missing part, renamed the variables a bit to make it easier to work with, and added some comments.

DELIMITER $$

DROP PROCEDURE IF EXISTS JSON_OBJECT_DIFF $$

CREATE PROCEDURE JSON_OBJECT_DIFF(IN a JSON, IN b JSON, INOUT aResult JSON, INOUT bResult JSON)
BEGIN
    DECLARE aKeys, bKeys JSON;
    DECLARE currentKey, currentKeyPath TEXT;
    DECLARE aVal, bVal JSON;
    DECLARE aKeysLength, bKeysLength, aHasKey, bHasKey INT;
    DECLARE i INT DEFAULT 0;

    SELECT JSON_KEYS(a) INTO aKeys;
    SELECT JSON_KEYS(b) INTO bKeys;
    SELECT JSON_LENGTH(aKeys) INTO aKeysLength;
    SELECT JSON_LENGTH(bKeys) INTO bKeysLength;

    # Iterate keys in `a` as `currentKey`:
    # * If exists in `b` and !=, insert `a(currentKey)` and `b(currentKey)` into `aResult` and `bResult`
    # * If does not exist in `b`, insert `a(currentKey)` into `aResult`

    WHILE i < aKeysLength DO
        SELECT JSON_EXTRACT(aKeys, CONCAT('$[', i, ']')) INTO currentKey; /* Already quoted */
        SELECT CONCAT('$.', currentKey) INTO currentKeyPath;
        SELECT JSON_EXTRACT(a, currentKeyPath) INTO aVal;

        SELECT JSON_CONTAINS_PATH(b, 'one', currentKeyPath) INTO bHasKey;

        IF bHasKey = 1 THEN
            SELECT JSON_EXTRACT(b, currentKeyPath) INTO bVal;

            IF aVal != bVal THEN
                SELECT JSON_INSERT(aResult, currentKeyPath, aVal) INTO aResult;
                SELECT JSON_INSERT(bResult, currentKeyPath, bVal) INTO bResult;
            end if;
        ELSE
            SELECT JSON_INSERT(aResult, currentKeyPath, aVal) INTO aResult;
        END IF;

        SET i = i + 1;
    END WHILE;

    # Iterate keys in `b` as `currentKey`:
    # * If does not exist in `a`, insert `b(currentKey)` into `bResult`

    SET i = 0;
    WHILE i < bKeysLength DO
        SELECT JSON_EXTRACT(bKeys, CONCAT('$[', i, ']')) INTO currentKey; /* Already quoted */
        SELECT CONCAT('$.', currentKey) INTO currentKeyPath;

        SELECT JSON_CONTAINS_PATH(a, 'one', currentKeyPath) INTO aHasKey;

        IF aHasKey = 0 THEN
            SELECT JSON_EXTRACT(b, currentKeyPath) INTO bVal;
            SELECT JSON_INSERT(bResult, currentKeyPath, bVal) INTO bResult;
        END IF;

        SET i = i + 1;
    END WHILE;
END $$


Binary Heap Order Fixing in Java I am implementing a binary heap in Java. I've come to the point where I have to fix the heap ordering whenever a new node is being inserted to the heap. The code below fixes the heap ordering. I have a gut feeling that the code can be improved.

private void fixHeapOrder(Node anomalousNode)
{
    if (this.heapType == 0) {
        // Min-heaps
        while (anomalousNode.getParent().getValue() > anomalousNode.getValue()) {
            // In min-heaps, the parent's value must be less than or equal to the
            // value of its children.

            Node.swapValues(anomalousNode.getParent(), anomalousNode);
            anomalousNode = anomalousNode.getParent();
        }
    } else if (this.heapType == 1) {
        // Max-heaps
        while (anomalousNode.getParent().getValue() < anomalousNode.getValue()) {
            // In max-heaps, the parent's value must be greater than or equal to the
            // value of its children.

            Node.swapValues(anomalousNode.getParent(), anomalousNode);
            anomalousNode = anomalousNode.getParent();
        }
    }
}

How else can I improve the code?

if (this.heapType == 0) {

and

    } else if (this.heapType == 1) {

Rather than using magic numbers, it is typical to use Enum values for this in Java.

    if (heapType == HeapType.MIN) {

and

    } else if (heapType == HeapType.MAX) {

or in the latter case, just

    } else {

Presumably there are only two types of heap.

You don't have to say this.heapType.  It's enough to just say heapType if there is no conflict.  You can add the this. if you find it makes things clearer, but it's not functionally necessary.  You only need to specify this. if there is another variable with the same name as the object field.

Another way to do this would be with an interface and separate implementations for Max and Min heaps.  That may not be better though.  It's hard to comment with so little context.  It would save the if/else here but might cause problems elsewhere.

In general, I can't comment on how this method works with other methods like the caller or swapValues, as those definitions are not included.  Should you work directly with the value rather than the node?  Should this be integrated into the caller's logic?  I can't really tell.