Force Stop by Error in httpClient.execute(httpPost); It work's at Smartphone(Android 2.3.3) but Tablet(Android 4.0) => Force Close!!

 public String getXmlFromUrl(String url) {
        String xml = null;

    try {
        // defaultHttpClient
        DefaultHttpClient httpClient = new DefaultHttpClient();
        HttpPost httpPost = new HttpPost(url);

        HttpResponse httpResponse = httpClient.execute(httpPost); // HERE FORCE CLOSE, why???
        HttpEntity httpEntity = httpResponse.getEntity();
        xml = EntityUtils.toString(httpEntity);

    } catch (UnsupportedEncodingException e) {
        e.printStackTrace();
    } catch (ClientProtocolException e) {
        e.printStackTrace();
    } catch (IOException e) {
        e.printStackTrace();
    }
    // return XML
    return xml;
}

Any suggestion with example?

You should remember this.

  From Android 3.x Honeycomb or later, you cannot perform Network IO on
  the UI thread and doing this throws
  android.os.NetworkOnMainThreadException. You must use Asynctask
  .

You have to put your getXmlFromUrl(..) code inside Asynctask method

For more Information Refer this Link
http://developer.android.com/reference/android/os/AsyncTask.html


using a cast in an overriden equals method should I use generics and if so, how? here is my issue when trying to override my equals method.

this is what I have currently

 @Override
public boolean equals(Object o) {
 if (this.name.equals(((Animal) o).getName())) {
  System.out.println("True");
return true;
} else {
 System.out.println("False");
 return false;
 }

}

I'm trying to compare the names of objects which extend the Animal class is there a better way to create this equals method without the explicit casting to compare the names of each object?, I was messing around with Generics but just couldn't get it to work properly. below is what I attempted with generics but when I tried to pass an object in the parenthesis it was just using the super equals method. (The get method literally just returns the name)

public boolean equals(Class<? extends Animal>o) {
 if (this.name.equals(o.getName())) {
   System.out.println("True");
   return true;
} else {
   System.out.println("False");
   return false;
  }

}

any help is really appreciated.

You could try something like the following in The Animal Class:

 @Override
   public boolean equals(Object obj) {
      Animal animal = null;
      if(obj == null || this ==null) {
         return false;
      }
      if(obj instanceof Animal) {
         animal = (Animal) obj;
      }
      if(this.name.equals(animal.getName())) {
         return true;
      }

      return super.equals(obj);
   }

Now if you have a Cat and a Dog which extend Animal
and you check

private Cat cat = new Cat("Pipsy");
private Dog dog = new Dog("Pupsy");

if(cat.equals(dog)) will return false because you check the name, and they have a different one.

I do not think you have to use Generics, Object is generic enough :). Hope this helped


String trim functions with destination size limitation 3 string trimming functions:
  Trim whitespace from the beginning of a string.
  Trim whitespace from the end of a string.
  Trim whitespace from both ends of a string.

Looking to improve code for:

Correctness (tested code - but so far no holes) - especially overlapping src/dest cases.
Design
Portability (3 tried: MS, *nix, embedded)
Efficiency

Note: (unsigned char) used as unless x==EOF, isspace(x) is not defined when x < 0.

/* String trim functions
 *
 * Result is sized limited.
 * Overlapping source and destination is OK.
 * Pointer parameters are assumed to point to valid strings spaces.
 */

#include <ctype.h>
#include <stddef.h>
#include <string.h>

// Return string beginning with first non-white-space in string `src`
// Return NULL when destination is too small, `dest` unchanged.

char *trim_begin(char *dest, size_t dest_size, const char *src) {
  while (isspace((unsigned char) *src)) {
    src++;
  }
  size_t src_size = strlen(src) + 1;
  if (src_size > dest_size) {
    return NULL;
  }
  return memmove(dest, src, src_size);
}

// Return string beginning string `src` but not including any of its trailing
//   white-spaces.
// Return NULL when destination is too small, `dest` unchanged.

char *trim_end(char *dest, size_t dest_size, const char *src) {
  size_t len = strlen(src);
  while (len > 0 && isspace((unsigned char) src[len - 1])) {
    len--;
  }
  size_t src_size = len + 1;
  if (src_size > dest_size) {
    return NULL;
  }
  memmove(dest, src, len);
  dest[len] = 0;
  return dest;
}

// Return string beginning with first non-white-space in string `src` but not
//   including any of its trailing white-spaces.
// Return NULL when destination is too small, `dest` unchanged.

char *trim(char *dest, size_t dest_size, const char *src) {
  while (isspace((unsigned char) *src)) {
    src++;
  }
  return trim_end(dest, dest_size, src);
}

Repeated code

size_t src_size = strlen(src) + 1;
if (src_size > dest_size) {
  return NULL;
}
return memmove(dest, src, src_size);

is a good candidate for a function (safe_memmove or something similar).

- Optimistic loops while (isspace((unsigned char) *src)) may never stop: '\0' is not a white space. Test for end of string explicitly:

    while (*src && isspace((unsigned char) *src))

  Red face above. I don't know what I was thinking.

Portability. A free-standing environment doesn't have to provide memmove or strlen or isspace. Only features confined in <float.h>, <iso646.h>, <limits.h>, <stdarg.h>, <stdbool.h>, <stddef.h>, and <stdint.h> are mandatory. I am afraid that to claim embedded portability you need to implement optional features yourself.

Sorry for nitpicking.


Circular / Cyclic Buffer implementation I am learning circular / cyclic buffer using this link. Here is the code that I have written so far:

#include <iostream>
#include <memory>
#include <cstddef>

class CircularBuffer
{
    std::unique_ptr<int[]> u_ptr;
    std::size_t head;
    std::size_t tail;
    const std::size_t capacity;
    bool full_v;
public:
    CircularBuffer(std::size_t);
    void reset();
    bool full() const;
    bool empty() const;
    std::size_t size() const;
    void write(int);
    int read();
};

// Constructor
// Set the capacity via initializer list because it is const
CircularBuffer::CircularBuffer(std::size_t space):capacity{space}
{
    u_ptr = std::unique_ptr<int[]>(new int[space]);
    head = 0;
    tail = 0;
    full_v = false;
}

// Reset the Buffer
void CircularBuffer::reset()
{
    head = 0;
    tail = 0;
    full_v = false;
    u_ptr[head] = int{};
}

// Check if buffer is full
bool CircularBuffer::full() const
{
    return full_v;
}

// Check if buffer is empty
bool CircularBuffer::empty() const
{
    return (!full_v && head == tail);
}

// Return the size of the buffer
std::size_t CircularBuffer::size() const
{
    if(full_v)
    {
        return capacity;
    }
    if(head >= tail)
    {
        return head - tail;
    }
    else
    {
        return capacity - (tail - head);
    }
}

// Write values into the buffer
void CircularBuffer::write(int data)
{
    u_ptr[head] = data;
    head = (head + 1) % capacity;
    if(full_v)
    {
        tail = (tail + 1) % capacity;
    }
    full_v = (head == tail);
}

// Read from buffer
int CircularBuffer::read()
{
    if(this -> empty())
    {
        return int{};
    }
    int ret_val = u_ptr[tail];
    full_v = false;
    tail = (tail + 1) % capacity;
    return ret_val;
}

int main()
{
    CircularBuffer cb{10};
    std::cout << "Empty: " << cb.empty() << "\n";
    for(int i = 0; i < 10; i++)
    {
        cb.write(i);
    }
    std::cout << "Full: " << cb.full() << "\n";
    std::cout << "Read: " << cb.read() << "\n";
    std::cout << "Full: " << cb.full() << "\n";
    std::cout << "Empty: " << cb.empty() << "\n";
    std::cout << "Size: " << cb.size() << "\n";
    cb.write(35);
    std::cout << "Size: " << cb.size() << "\n";
    std::cout << "Read: " << cb.read() << "\n";
    std::cout << "Size: " << cb.size() << "\n";
    cb.reset();
    std::cout << "Size: " << cb.size() << "\n";
    return 0;
}

Is the implementation correct? What are the shortcomings that can be fixed?
I see that the original reference uses a mutex object and uses lock. Is such an approach used with all data structures? If not, is there a reason why that has been used here?

Your ringbuffer has a compile time size, so it would be appropriate to make it a template class rather than simply passing it as an argument to the constructor. You never plan to change it anyway?

template<size_t bufSize>
class CircularBuffer

That leads to the next point. You use a std::unique_ptr<int[]> There is something in C++ that is much better than this std::array. With the size of the buffer a compile time template argument of the class you can easily use it

template<size_t bufSize>
class CircularBuffer {
    std::array<int, bufSize> buf;
    std::size_t head;
    std::size_t tail;
    bool full_v;

    ...
};

Note that this increases the size of the class object considerably, as the std::array is directly inlined in the class.

The other members of this class are always the same after construction so you should use static member initialization:

template<size_t bufSize>
class CircularBuffer {
    std::array<int, bufSize> buf;
    std::size_t head{ 0 };
    std::size_t tail{ 0 };
    bool full_v{ false };

    ...
};

Note that this completely removes the need to define a constructor at all. The compiler generated default constructor will do just fine.

Resetting the buffer should not change the data stored in it as it is not read anyway so just leave it alone

void CircularBuffer::reset()
{
    head = 0;
    tail = 0;
    full_v = false;
}

Can tail ever be larger than head? If not why are you checking it.


Performing non-production analytics on data I have the following Lua script which I use against my redis environment to perform some non-production analytics on my data.

My dataset contains hundreds of millions of records, hence I want to make sure that the approach that I am taking is optimal for query performance and best coding practices.

-- Tables to store the aggregations
local aUsers = {}           -- [sizeInBytes] = userID      (Table is created the other way around to allow for sorting)
local aFileTypes = {}       -- [fileTypeID] = sizeInBytes
local aFileTag = {}         -- [fileTagID] = sizeInBytes

-- Global Count of Records and Sum of Sizes in Bytes
local sumCount = 0
local sumSize = 0

-- For users 'sizeBytes' is the Table[ID] to allow sorting. Collissions are not a big deal
local function userAgregations(table, key, size)
    table[size] = key
end

-- For other agregations the `fieldID` is the key mapped against the `size` agregation
local function genericAgregations(table, key, size)
    if table[key] then
        table[key] = table[key] + size
    else
        table[key] = size
    end
end

-- For users only, sort the table by Key (size) and print return it
local function printTopUsers(t)
    local str = "["
    local sortedTable = {}
    local maxResults = 25

    for key in pairs(t) do
        sortedTable[#sortedTable+1] = key
    end

    table.sort(sortedTable, function(a,b)
        return a > b
    end)

    if #sortedTable < 25 then
        maxResults = #sortedTable
    end

    for i=1, maxResults do
        str = str .. sortedTable[i] .. ":" .. t[sortedTable[i]] .. ","
    end

    local formatted = str .. "]"
    return formatted
end

-- For other fields just return the table
local function printTable(t)
    local str = "["
    for key,value in pairs(t) do
        str = str .. key .. ":" .. value .. ","
    end

    local formatted = str .. "]"
    return formatted
end

-- Return all the keys in redis that match a given pattern (Yes, its non-production)
-- Where the keys are:  0:0000:00:00000:00:00:00:0
-- Sample query is:     5:123?:??:12346:1?:??:??:2--
local matches = redis.call("KEYS", query)

-- For each Key match, get the value and perform the agregations
for _,key in ipairs(matches) do
    local val = redis.call("GET", key)
    local sCount, sSize = val:match("([^:]+):([^:]+)")
    local count = tonumber(sCount)
    local size = tonumber(sSize)

    -- Extract a part of the key which refers to a particular agregation fieldID
    userAgregations(aUsers, string.sub(key, 11, 15), size)
    genericAgregations(aFileTypes, string.sub(key, 17, 18), size)
    genericAgregations(aFileTags, string.sub(key, 17, 18), size)

    -- Increment the Global sum and count
    sumCount = sumCount + count
    sumSize = sumSize + size
end

-- Contatenate and return response
local globalAgregation = string.format("[%s:%s]", sumCount, sumSize)
local result = printTopUsers(aUsers) .. printTable(aFileTypes) .. printTable(aFileTags) .. globalAgregation

return result

There are a few little things that can be improved:

-- For users 'sizeBytes' is the Table[ID] to allow sorting. Collissions are not a big deal
local function userAgregations(table, key, size)
   table[size] = key
end

If using LuaJIT, this function might be optimized away, but in regular Lua there's no reason to make a function that just does an assignment like this. Just do it straight in your code; table assignments are supposed to be understandable.

In genericAgregations (should be genericAggregations!) you're performing one table access too many:

local function genericAgregations(table, key, size)
   local value = table[key]
   table[key] = value and (value + size) or size
end

In printTopUsers, don't use a custom function in table.sort to sort in descending order. Sort normally and then perform the numeric for backwards.

Also, instead of concatenating str, create a temporary table, insert the bits of the string in it and in the end generate the string at once with table.concat. It's faster than multiple concatenations.

Same in printTable.

As for the redis-specific stuff, I have no knowledge but I hope the little tweaks above are useful!


Priority stack in Java I have this (simple) data structure that maintains multiple LIFO stacks. Each such stack is assigned a priority. The less is the value of a priority key, the higher the priority. Pushing operation requires the element to push and its priority key. Whenever popping the queue, it removes the most recently added element of the highest priority stack.

I have no clue where one would need such a data structures (maybe someone could tell me?), yet it was fun implementing it.

PriorityStack.java:

package net.coderodde.util;

import java.util.ArrayList;
import java.util.EmptyStackException;
import java.util.List;
import java.util.Map;
import java.util.TreeMap;

/**
 * This class implements a priority stack. Each element added comes with a
 * priority key. The smaller the key, the higher is the priority of the element.
 * The elements with the same priority are organized with LIFO rule
 * ("last in, first out").
 *
 * @author  Rodion "rodde" Efremov
 * @version 1.6
 * @param   <E> the element type.
 * @param   <P> the priority key type.
 */
public class PriorityStack<E, P extends Comparable<? super P>> {

    private final Map<P, List<E>> map;
    private int size;

    public PriorityStack() {
        this.map = new TreeMap<>();
    }

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

    /**
     * Pushes the input element to this stack.
     *
     * @param element  the element to push.
     * @param priority the priority of the input element.
     */
    public void push(E element, P priority) {
        if (!map.containsKey(priority)) {
            map.put(priority, new ArrayList<>());
        }

        map.get(priority).add(element);
        ++size;
    }

    /**
     * Returns but not removes the topmost element of the highest priority
     * substack.
     *
     * @return the top element.
     */
    public E peek() {
        if (size == 0) {
            throw new EmptyStackException();
        }

        List<E> list = map.entrySet().iterator().next().getValue();
        return list.get(list.size() - 1);
    }

    /**
     * Pops the topmost element from the stack with the highest priority.
     *
     * @return an element.
     * @throws EmptyStackException if the stack is empty.
     */
    public E pop() {
        if (size == 0) {
            throw new EmptyStackException();
        }

        Map.Entry<P, List<E>> entry = map.entrySet()
                                         .iterator()
                                         .next();
        P priority = entry.getKey();
        List<E> list = entry.getValue();
        E ret = list.remove(list.size() - 1);

        if (list.isEmpty()) {
            map.remove(priority);
        }

        --size;
        return ret;
    }

    public static void main(final String... args) {
        PriorityStack<String, Integer> stack = new PriorityStack<>();

        // -1: He is a
        // 2:  funky programmer
        // 3:  that likes to
        // 5:  code algorithms
        // 7:  and
        // 8:  data structures! :-]

        stack.push("a", -1);
        stack.push("and", 7);
        stack.push("is", -1);
        stack.push("programmer", 2);
        stack.push("algorithms", 5);
        stack.push("code", 5);
        stack.push(":-]", 8);
        stack.push("He", -1);
        stack.push("to", 3);
        stack.push("structures!", 8);
        stack.push("likes", 3);
        stack.push("data", 8);
        stack.push("who", 3);
        stack.push("funky", 2);

        while (!stack.empty()) {
            System.out.print(stack.pop());
            System.out.print(" ");
        }

        System.out.println();
    }
}

So, what do you think?

You can consider implementing the Deque interface so that you can potentially open up its usage as an implementation of the Collections classes.

Your field size seems to be underused here, as I can sort of see a size() method using it too... BTW, regardless whether you use the Deque interface, your method empty() should be written as isEmpty(), to follow Java methods' naming convention as 'is...' for boolean return values.

Back to talking about Collections classes, they throw NoSuchElementException when no element can be returned, e.g. when empty, so perhaps you can throw that too instead of EmptyStackException.

Since you are relying on your TreeMap inner implementation, you can consider having your field declaration as TreeMap too instead of Map, this being one of those cases where using the implementation (class) rather than the interface makes more sense. With a TreeMap, you can replace map.entrySet().iterator().next().getValue() with the neater method map.firstEntry().

In your push() method, if you are on Java 8, you can consider computeIfAbsent():

public void push(E element, P priority) {
    map.computeIfAbsent(priority, ArrayList::new).add(element);
    size++;
}

Last but not least, you should have better unit testing too... Your current case looks OK already, so it's just a matter of comparing the output when you pop() to something you can compare with, such as a List. Remember to test for edge cases, such as getting from an empty instance, or adding null elements (which should be allowed given your ArrayList implementation).


Princeton KMP implementation I implemented a Python version of KMP followed by Princeton Prof Robert Sedgewick's discussion.

However, I got a Time Limit Exceeds error. Per leetcode's explanation, TLE indicates that the solution is correct but it is extremely inefficient when test case with huge input size.

If I understand TLE correctly, this means that my solution works, but some part of my solution should be inefficient and need to be enhanced? Would this be due to an extremely large space cost when precomputing the DFA table?

class Solution(object):

    def dfa(self, haystack, needle):

        R = 256 # R : the alphabet size . We set 256 for ASCII
        M = len(needle) # M : the patten length

        # create a dictionary to encode every unique char from needle with a unique int value for later dfa retrieval.
        global needle_dict
        #needle_dict = dict(zip(list(set(needle)), range(len(list(set(needle))))))
        needle_dict = dict(zip(list(set(needle).union(set(haystack))), range(len(list(set(needle).union(set(haystack)))))))
        # create dfa table, where the # of dfa columns is M, the # of dfa rows is R.
        dfa = [[0 for i in range(M)] for j in range(R)]

        # At state 0  find the first char in the needle and set this first char's corresponding state as 1
        dfa[needle_dict[needle[0]]][0] = 1

        X = 0 # X to record previous state

        for state in range(1, M): # dfa column
            for row in range(0, R): # dfa row
                dfa[row][state] = dfa[row][X] # copy mismatch cases
            dfa[needle_dict[needle[state]]][state] = state + 1 # update the match case for present state
            X = dfa[needle_dict[needle[state]]][X] # update X

        return dfa

    def strStr(self, haystack, needle):
        if len(needle) == 0:
            return 0
        if (haystack == None or needle == None or len(haystack) < len(needle) ):
            return -1
        # precompute dfa[][]from pattern
        dfa_table = self.dfa(haystack, needle)

        i, state = 0, 0
        while (i < len(haystack) and state < len(needle)):
            state = dfa_table[needle_dict[haystack[i]]][state]
            i += 1

        if state == len(needle):
            return i - len(needle)
        else:
            return -1

A few things pop out:

this:

needle_dict = dict(zip(list(set(needle).union(set(haystack))), range(len(list(set(needle).union(set(haystack)))))))

...creates two identical sets.  More space(and time, perhaps) efficient to do:

        straws = list(set(needle).union(set(haystack)))
        needle_dict = dict(zip(straws), range(len(straws)))

this:

dfa = [[0 for i in range(M)] for j in range(R)]

...does needless iteration.  Instead do:

    dfa = [ list(range(M)) for j in range(R) ]

There's a small bug in:

if len(needle) == 0:
    return 0
if (haystack == None or needle == None or len(haystack) < len(needle) ):
    return -1

... in that you're calling len(needle) before testing if it's None.

It's unclear why you're doing:

global needle_dict

... it may have performance implications (though I'm not positive).


Doubly linked list code in C Let me know if it seems right to you and if anything can be optimized.

/*
 * Doubly Linked List
 *
 * Each node of the list contain two references (or links) – one to the previous node and other to the next node.
 * The previous link of the first node and the next link of the last node points to NULL.
 * In comparison to singly-linked list, doubly-linked list requires handling of more pointers
 * but less information is required as one can use the previous links to observe the preceding element.
 */

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

typedef struct Node {
    int data;
    struct Node *next;
    struct Node *prev;
} Node;

void insert(Node *current, int data);
void delete(Node *current, int data);
void print(Node *current);
int find(Node *current, int data);

void insert(Node *current, int data) {

    // current is pointing to first element
    // we iterate until we find the end
    while(current->next != NULL) {
        current = current->next;
    }
    // create a new Node and insert the item
    current->next = (Node *)malloc(sizeof(Node));
    (current->next)->prev = current;
    current = current->next;
    current->data = data;
    current->next = NULL;
}

void delete(Node *current, int data){

    // Iterate until we find a pointer next to the one we need to delete
    while (current->next != NULL && (current->next)->data != data) {
        current = current->next;
    }

    // Item is not found
    if(current->next == NULL) {
        printf("\nElement %d is not present in the list\n", data);
        return;
    }

    // The element is found in the node next to the one that current points to
    // We removed the node which is next to the pointer (which is also temp)
    Node *tmp = current->next;
    // In special case that we are deleting last node
    if(tmp->next == NULL) {
        current->next = NULL;
    } else {
        current->next = tmp->next;
        (current->next)->prev = tmp->prev;
    }
    tmp->prev = current;

    // We got rid of the node, now time to dellocate the memory
    free(tmp);

    return;
}
void print(Node *current) {
    while(current != NULL) {
        printf("%d ", current->data);
        current = current->next;
    }
}

int find(Node *current, int data) {
    // First pointer is head aka dummy node with no data
    // so we go to next one
    current = current->next;

    // Iterate through the linked list
    while(current != NULL) {
        if(current->data == data) {
            return 1;
        }
        current = current->next;
    }
    return 0;
}

int main() {

    Node *head = (Node *)malloc(sizeof(Node));
    head->next = NULL;
    head->prev = NULL;

    int data = 0;
    int usr_input = 0;

    while(1){
        printf("0. Exit\n");
        printf("1. Insert\n");
        printf("2. Delete\n");
        printf("3. Print\n");
        printf("4. Find\n");

        scanf("%d", &usr_input);

        // can also use a switch instead
        if( usr_input == 0) {
            exit(0);

        } else if(usr_input == 1) {
            printf("\nEnter an element you want to insert: ");
            scanf("%d", &data);
            insert(head, data);

        } else if(usr_input == 2) {
            printf("\nEnter an element you want to delete: ");
            scanf("%d", &data);
            delete(head, data);

        } else if(usr_input == 3) {
            printf("The list is ");
            print(head->next);
            printf("\n\n");

        } else if(usr_input == 4) {
            printf("\nEnter an element you want to find: ");
            scanf("%d", &data);
            int is_found = find(head, data);
            if (is_found) {
                printf("\nElement is found\n\n");
            } else {
                printf("\nElement is NOT found\n\n");
            }
        }
    }
    return 0;
}

Some mostly stylistic improvements you could apply:

There is no need to cast the return value of malloc(), unless you plan on compiling this code as C++. In C, void* converts implicitly to any other pointer type. So the cast is just boilerplate in this case.
Prefer using sizeof(variable) instead of sizeof(Type). Example:

Node *head = malloc(sizeof(*head));

This makes maintenance easier, since now if you introduce a new Node type, you only have to change one place, with no chance of updating one side of the expression and forgetting the other. Note that in this example, the dereference of head is valid because sizeof is a compile-time operation, so there is no actual pointer access taking place here, just type inference.
Consider giving more robust names to your functions. find(), insert(), etc, are very common names, which can apply to many things, not just lists. In C, you cannot overload function names, so in any major project, these names would be very likely to cause collisions with other global identifiers. Name prefixes are a common approach in C to solve this problem. Consider prefixing the function names with list_ or any other prefix you might prefer. I.e.:

void list_insert(Node *current, int data);
void list_delete(Node *current, int data);

Instead of returning an integer 0|1 to represent a boolean value, you can use the bool type, declared in the <stdbool.h> header, which is more idiomatic.
delete() function is a void function, so it doesn't need an explicit return statement at the end.
Looks like you are using printf() to log errors:

if(current->next == NULL) {
    printf("\nElement %d is not present in the list\n", data);
    return;
}

For error output, it is more correct to use fprintf and write to the stderr standard stream.
The list printing function print() could be made more flexible by receiving the output stream as a parameter. So you don't limit the caller to always printing to stdout, it can also be used to dump the list to a file (notice the use of fprintf()).

void print(Node *current, FILE *stream) {
    while(current != NULL) {
        fprintf(stream, "%d ", current->data);
        current = current->next;
    }
}

When you want to allocate and immediately zero-initialize data, such as in here:

Node *head = (Node *)malloc(sizeof(Node));
head->next = NULL;
head->prev = NULL;

The standard function calloc() can be used to make code more concise:

Node *head = calloc(1, sizeof(*head));


Decoupling UI code from game logic IIUC I should decouple UI logic from the model and controller in my project. I didn't succeed and had to resort to TextView etc in my game logic:

package dev.game.adventure;

import android.graphics.drawable.Drawable;
import android.widget.TextView;

public class Adventure {

    private Player player;

    public Player getPlayer() {
        return player;
    }

    public Adventure(TextView t, AdventureActivity target, PlaceView placeView) {
        World world = new AdventureWorld(this, t, target, placeView);
        Person me = new Person(world, "You", 0, target);
        placeView.mainCharacter = me;
        player = new Player(world, me, t);
    }

    public Drawable loadPicture(int imageName, AdventureActivity target) {
        Drawable im = target.getResources().getDrawable(imageName);
        return im;
    }
}

I have similar to the above in several classes, passing on an android activity and/or a textview into model and controller objects. Will this be a problem and is it possible to solve?

OOP doesn't mean to "split up" code into random classes.

The ultimate goal of OOP is to reduce code duplication, improve readability and support reuse as well as extending the code.

Doing OOP means that you follow certain principles which are (among others):

information hiding / encapsulation
single responsibility
separation of concerns
KISS (Keep it simple (and) stupid.)
DRY (Don't repeat yourself.)
"Tell! Don't ask."
Law of demeter ("Don't talk to strangers!")
replace branching with polymorphism

You example suffers multiple violations of this OO principles.

e.g:

Single Responsibility / Separation of concerns.

The (only) method in you class does nothing but delegates the call to one of it's parameters while passing the other parameter. This is the reverse version of feature envy. This method has no reason to exist here.

And since this is the only method in this class (you showed) the whole class has no reason to exist.

interface segregation

Your constructor has 3 parameters but  there is only one member variable initialized.

The complete content should be outside the constructor and only the Player instance should be passed in to be assigned to the member variable.


Replacement function for jQuery .post Today I just completed a personal POST function that emulates jQuery's using $.ajax, but also implementing a possible redirect. I just finished up and it looks like it is functional, but since this is something I want to keep long term and integrate in many projects, I want to make sure it is as functional as possible.

function postR(url, params, redir = false, callback = null) {
    if(redir){
        var form = document.createElement('form');
        form.method = 'post';
        form.action = url;
        for (var key in params) {
            var input = document.createElement('input');
            input.type = 'hidden';
            input.name = key;
            input.value = params[key];
            form.appendChild(input);
        }
        document.body.appendChild(form);
        form.submit();
    }else{
        if(callback && typeof(callback) === "function"){
            $.ajax({
                type: "POST",
                url: url,
                data: params,
                success: function(data){
                    callback(data)
                }
            });
        }else{
            $.ajax({
                type: "POST",
                url: url,
                data: params
            });
        }
    }
}

The purpose for building this was because I wanted to use jQuery's post function to do some JS work with a form before it was submitted, but also wanted to redirect alongside the POST request, like a natural form.

I would:

Use a FormData instead of creating a form element.
Give it a name better than "postR", like... simply "post"?
Add an error handler to the $.ajax call
Unify both $.ajax calls

Pseudo code (Not tested):

function post(url, params, redir = false, callback = null) {
  var formData = new FormData();

  for (var key in params) {
    formData.append(key, value);
  }

  $.ajax({
    type: "POST",
    url: url,
    data: formData,
    success: function(data) {
      typeof(callback) === "function" && callback(data);
      redir && window.location.reload();
    }
  });
}


Advice needed for scopes in JavaScript I would like to connect this "JS" to Bugzilla (example: bugzilla.mozilla.org or landfill.bugzilla.org).

I started to learn JS language today and I would like to ask you:

How can I not do bad things in global scope?
How should I use functions (in var or not)?

quicksearch = document.getElementById('quicksearch_top');
comment = document.getElementById('comment');

severity = document.getElementById('bug_severity');
priority = document.getElementById('priority');

commit_top = document.getElementById('commit_top');
commit = document.getElementById('commit');

function focusonload() {
// may be it should be
// in next function?

    function cursorfocus(s) {
        x = window.scrollX;
        y = window.scrollY;
        s.focus();
        window.scrollTo(x, y);
    }

    if (comment !== null) {
        cursorfocus(comment)
    } else {
        quicksearch.focus();
    }
}

function navigation(keypressed) {

    keypressed = keypressed || window.event;

    function selectelement(w, select) {
        w.value = select;
    }

    keyCode = keypressed.keyCode || keypressed.which,
    kn = {
        enter: 13,
        save: 83,
        down: 40,
        up: 38,
        p1: 49,
        p2: 50,
        p3: 51,
        p4: 52,
        p5: 53,
    };

    if (keypressed.altKey) {
        if (keyCode == nk.save && commit_top!==null){
            commit_top.click();
        }

        var key_str;
        if (priority!==null){
            switch (keyCode) {
                case kn.p1: key_str = "P1"; break;
                case kn.p2: key_str = "P2"; break;
                case kn.p3: key_str = "P3"; break;
                case kn.p4: key_str = "P4"; break;
                case kn.p5: key_str = "P5"; break;
            }
        }
        selectelement(priority, key_str);
        severity.focus();

    } else if (keypressed.ctrlKey) {
        switch (keyCode) {
            case kn.enter:
                if (commit!==null&&comment.value!=="") {
                    commit.click();
                }
                break;
            case kn.up:
                if (quicksearch!==null) {
                    quicksearch.focus();
                }
                break;
            case kn.down:
                if (comment!==null) {
                    comment.focus();
                }
                break;
        }
    }
}

focusonload();
document.onkeydown = navigation;
// how to call this functions when page
// would be loaded at all (with window.onload)

How can I not mess up the global scope?

You can use an IIFE ( Immediately Invoked Function Expression ) to surround your code and then assign all your variables with var <- Very important.

(function(){

  var quicksearch = document.getElementById('quicksearch_top');
  var comment = document.getElementById('comment');

  var severity = document.getElementById('bug_severity');
  var priority = document.getElementById('priority');

  var commit_top = document.getElementById('commit_top');
  var commit = document.getElementById('commit');

  //the rest of your code here

}());

  How should I use functions (in var or not)?

It really is up to you, at this point I prefer function.

Other than that:

You should use lowerCamelCasing, so

commit_top -> commitTop
quicksearch -> quickSearch
selectelement -> selectElement

etc.etc

Your keycode map is very nice, keep it that way!

One last comment is that you should not use document.onkeydown = navigation;, there is a good chance that you will break something this way.

Try document.addEventListener("keydown", navigation, true); instead.


Hashing an arbitrary object to a sparse array of bits I have this method in one of the java classes I'm working on (an implementation of a counting bloom filter):

private int[] hash(V v){
    Random r=new Random(v.hashCode());
    int l=r.nextInt(size);
    int[] keys=new int[size];
    for(int i=0;i<bpe;i++){
        keys[(r.nextInt(size)+l)%size]=1;
    }
    return keys;
}

Where:

V is one of the type parameters for the enclosing generic class
size is a field set on initialization, defaults to 64 unless specified otherwise from the constructor
bpe is also a field, set on initialization to Math.max(5, Math.min(size/8,(int)Math.sqrt(size)))

Is there a better way to do this?

Wasting space?
I don't know if there's a better way of generating your hashed bits, but you are using 32x the amount of space that you need to.  I would suggest using all the bits in your keys array rather than just 1 bit per int.
What is l for?
One thing I don't understand about your hash is the use of l.  You are already using random numbers to generate your hash, so why do you need to add two random numbers together?  Is it supposed to generate more randomness?  I don't think it does.
An example of packing the bits
// Only allocate one int for every 32 bits.
keys = new int[(size+31)/32];
for(int i=0;i<bpe;i++){
    int bitNum = (r.nextInt(size)+l)%size;
    keys[bitNum >> 5] |= (1 << (bitNum & 31));
}

To retrieve the bit, previously, you would do:
isBitSet = keys[bitNum] != 0;

Now you would do:
isBitSet = (keys[bitNum >> 5] & (1 << (bitNum & 31))) != 0;


Min and max of numbers read from a file I had to create this program, and I did it fine, all is working and stuff but I was wondering, what is better way of doing such thing? I want more efficient method.

Problem assignment:

  In the file is unknown amount of numbers. Create a program, which
  output is amount of numbers, the lowest number and the highest number.

#include <iostream>
#include <fstream>

/*
INPUT EXAMPLE:
12
15
-1
2
20
-4

OUTPUT EXAMPLE:
numbers: 6
minimum: -4
maximum: 20
*/
int main(){
    std::ifstream fin("input.txt");
    if (fin.is_open()){
        int n;
        int min,max;
        int i = 0;
        while (fin >> n){
            if (!n) break;
            if (i++ == 0){
            min = n; max = n;
            }
            if (n < min)
                min = n;
            else if (n > max)
                max = n;
        }
        if (fin.good())
         std::cout << "numbers: " << i << '\n' << "minimum: " << min << '\n' << "maximum: " << max << '\n';
        else std::cout << "plz you even tryin'\n";
    }
    system("PAUSE");
    return 0xDEADBEEF;
}

Any improvements?

As far as performance is concerned, there are no problems in this code. It's using C++ streams in the idiomatic way, which is good.

If you're experiencing performance issues, you could try detaching from C streams at the start of your program:

int main(){
    std::ios_base::sync_with_stdio(false);

When you do this, you must then not mix C++ I/O (IO streams) with C I/O (printf and friends) in your program, but mixing these is bad practice anyway.

Here are some other comments to your code:

The names n and i don't say anything about the variables' use. I had to read through most of your code to figure out what i means. Always use descriptive names for your variables, at least for those whose value is supposed to persist for extended periods of time. I would be willing to accept the n, as it is basically one-shot, but i should be renamed at least to count or inputCount or something more descriptive.
Your looping and termination logic seems odd. You are using the correct construct while (fin >> n), looping while there's valid input left. But then you have this block:

if (!n) break;

This will cause the loop to terminate when a 0 is entered in the input. This behaviour does no match the assignment, and hence it represents a bug. Based on the assignment, you should only terminate when input ends, which will be correctly caught by the while loop. The if (!n) line should therefore be removed.

Related, your after-loop test for fin.good() is incorrect. If the loop is (correctly) terminated by being unable to read more numbers because the file has ended, both eofbit and failbit will be set on fin. The condition find.good() will therefore fail. You might want to augment the condition to check for EOF:

if (find.good() || fin.eof())

Indentation in the post-loop conditional is inconsistent with the rest of the program, and should be fixed. The same holds for the if (i++ == 0){ block (unless that was just screw-up of the code formatter here on Stack Exchange).
The return value of main is interpreted by the operating system as the return code of the program. On all OSes I know, 0 indicates success, non-zero indicates failure. You should therefore return 0 on successful program termination. Or just omit the return altogether, return 0; is implied in the main function in C++.

Or, if you want to be really beyond reproach, and comaptible even with OSes where 0 does not necessarily mean success, you can change it to return EXIT_SUCCESS;; this requires #include <cstdlib>.
Error output (such as your "plz you even tryin'\n" message) should be sent to std::cerr (standard error stream, usually file descriptor 2), not to std::cout (standard output stream, usually file descriptor 1). A more helpful error message would be in order as well. Something like

  Error when reading file 'input.txt', possibly encountered a non-number.

When printing this message, you should consider returning 1 (or EXIT_FAILURE) from main, instead of allowing fallback to the default success.
The outer if (fin.is_open()) could be removed. If the file is not open, fin >> n will fail, and you would at least print an error message instead of failing silently.
system("PAUSE"); is a very non-portable and extremely cumbersome solution looking for a problem. You're calling an external program just to wait for a key press. You should get rid of it altogether: your program is not interactive at all, it could run just fine as part of a script. Requesting input like this ruins that. Just drop that line altogether.


Testing out a program that calculates the days left in the month I'm new to coding C++ and I want to get some insight on my progress. Can you guys take a look at my program and give me some feedback on it?

#include <iostream>
using namespace std;

int main(){

int day{};
int monthNum{};
cout << "Calculate how many days are left in the month. \nPlease enter the month number and the day, seperated by a space. ex 1 22 for Jan 22nd: ";
cin >> monthNum >> day;

if (monthNum < 1 || monthNum > 12 || day < 1 || day > 31 || (monthNum == 2 && (day < 1 || day > 29))){
    cout << "Error, not a valid input. \nPlease make sure you're month and day are valid." << endl;
    return 0;
}

int daysInCurrentMonth{};
char leapYear;
int doNothing{};  //Used to do nothing if certain conditions are false

if (monthNum == 2){
    cout << "\nIs it leap year? Type y for yes and n for no. ";
    cin >> leapYear;
    ((leapYear == 'y') ? daysInCurrentMonth = 29 : daysInCurrentMonth = 28);
    if (leapYear != 'y' && leapYear != 'n'){
        cout << "invalid input..." << endl << endl;
        return 0;
    }else{
        doNothing = 0;
    }
}

cout << "\nYou entered day " << day << " of month " << monthNum << endl;

((monthNum == 4 || monthNum == 6 || monthNum == 9 || monthNum == 11) ? daysInCurrentMonth = 30 : doNothing = 0);
((monthNum == 1 || monthNum == 3 || monthNum == 5 || monthNum == 7 || monthNum == 8 || monthNum == 10 || monthNum == 12) ? daysInCurrentMonth = 31 : doNothing = 0);

int daysLeftInMonth{daysInCurrentMonth - day};
cout << "\nThere are " << daysLeftInMonth << " days left in the month." << endl;

cout << endl;
return 0;
}

This is my third iteration of this program. This is the tutorial series that I've been watching. I'm currently on section 9 and just finished learning about conditional operators. I'm trying to refine the project as I go along. Also, I know it's pretty early to be thinking about this, but if anyone is able to, I'd like to know if the way I structure this is in the right direction for becoming a professional programmer, i.e. it's easy to follow and is readable, or it looks like garbage.

Don't use using namespace std
Prefer using \n over std::endl
The last return 0 is not necessary in this case as the compiler will generate it for you
When using the ternary operator you can just assign at the start instead of having to do it in both branches
Not part of the code but you should check your spelling

I'm not sure if you do the input on purpose to learn how it works but it can be done without user interaction.
Have a look at ctime as it offers many functions that can be useful here.

Here's a small program showcasing how to use those functions:

#include <ctime>
#include <iostream>

int main() {
    std::time_t time = std::time(nullptr);
    auto today = std::localtime(&time);

    int day = today->tm_mday;
    int month = today->tm_mon + 1;
    int year = today->tm_year + 1900;
    int month_length = 30;

    constexpr int february = 2;
    if (month == february) {
        month_length = is_leapyear(year) ? 29 : 28;
    }

    std::cout << month_length - day << " days remain in this month.\n";
}

Since the is_leapyear function is trivial I left it out.

Of course if you're using C++20 then you can just use chrono which offers an amazing amount of time and date related functions.


Binary search an integer array I'd like this code to be improved

package com.array.demo;

public class BinarySearchMethod {

public static void main(String[] args) {

    int[] arr = { 2, 4, 5, 6, 8, 9, 11, 15, 17, 19, 22, 111, 117, 234, 543 };

    int search = 2;
    BinarySearchMethod b = new BinarySearchMethod();
    int size = arr.length;

    int searchPos = b.binarySearch(arr, size, search);

    if (searchPos == -1) {
        System.out.println("Element not found");
    } else {
        System.out.println("Element found in the position " + searchPos);
    }

}

public int binarySearch(int[] arr, int size, int search) {
    int start = 0;
    int end = size - 1;

    while (start <= end) {
        int mid = (start + end) / 2;
        if (arr[mid] == search) {
            return mid;
        } else if (arr[mid] < search) {
            end = mid - 1;

        } else {
            start = mid + 1;
        }
    }

    return -1;

}

}

For beginner code, this is quite good. I am impressed. Good enough for me to assume you have worked from some example code. That is not a criticism, everyone does that (me too).
Still, many examples are based on other examples, and they have some issues that are 'historical' in nature, and then there are some practices in Java which make sense which are not visible in your code.
Style
Really, it's good, no big problems.
There is an extra empty-line in the middle of your if/else/else statement which is inconsistent. If I were to be extremely nit-picky, I would say that I personally would not put the empty line after the return statement (but like you I also would put one before the statement).
Order of comparison.... I am not sure why I dislike this one, and I have no reference for it, but, you have the line:

} else if (arr[mid] < search) {

and I would strongly prefer it to be:
} else if (search > arr[mid]) {

'Finally', the arr and search parameters to your method are constants, and are not changed. You should show this by adding 'final' to the declaration:
final int[] arr, ...., final int search

Mid Point Bug
There is a 'famous' bug in your code.
Basically, for large arrays, you risk overflowing the calculation. Your code should be:
int mid = start + (end - start) / 2;

Generalized search
In Java it is common practice to have a general method that will search in any sub-part of the array. Then, there should also be a version which searches the whole array.
Your method is unusual because it does neither..... it searches sub array sections, but only from the beginning (it has a size argument...). Your method should look like:
public int binarySearch(int[] arr, int start, int length, int search) {
    int end = start + length - 1;

In addition, you should have the 'simplified search everything method' that looks like:
public int binarySearch(int[] arr, int search) {
    return binarySearch(arr, 0, arr.length, search);
}

Input Validation
It is common in Java to validate the inputs. You should have this as your first lines:
if (start < 0 || start >= arr.length) {
    throw new IllegalArgumentException("Invalid start index " + start + " in array of size " + arr.length);
}

if (length < 0 || start + length > arr.length) {
    throw new IllegalArgumentException("Invalid length " + length + " in array of size " + arr.length + " and start from " + start);
}

Redundant else
You have the following code:

    if (arr[mid] == search) {
        return mid;
    } else if (arr[mid] < search) {
        end = mid - 1;

    } else {
        start = mid + 1;
    }

Because the first part has return mid;, there is no need for it to be an else afterwards:
    if (arr[mid] == search) {
        return mid;
    }

    if (arr[mid] < search) {
        end = mid - 1;

    } else {
        start = mid + 1;
    }

A return statement inside an if block means that the block will never complete normally. This is considered to be good practice in Java (both having 'early returns' and also removing the else when you have an early return).
Insertion Point
One of the features of Java's library functions for binary search (and what many people find complicated) is the "insertion point". This is a 'clever' way for the method to say: Well, I could not find the value when I searched, but, if it as here, it would need to go at position x.
So, in a small array [1, 3, 5], if we were to search for 2, it is not there, but, if it were to be inserted, it would be at position 1 (where the 3 is now). How has java 'standardised' for showing you this?
It returns a negative value for when the value is not found. It would be 'nice' if it could return just -1 if the value was not found but should be inserted at position +1. Unfortunately, if the value should be inserted at position 0, then it can't return -0, so, it has to take the negative of the insertion point, and then subtract 1. So, an insertion point of 0 becomes -1, an insertion point of 1 becomes -2, and so on.
This is actually really easy to implement (even if it is hard to understand sometimes).
In your code, you just need to change the line:

return -1;

to be:
return -start - 1

Result
Putting all the above together, I would have your function as:
public int binarySearch(final int[] arr, int start, final int length, final int search) {
    if (start < 0 || start >= arr.length) {
        throw new IllegalArgumentException("Invalid start index " + start + " in array of size " + arr.length);
    }

    if (length < 0 || start + length > arr.length) {
        throw new IllegalArgumentException("Invalid length " + length + " in array of size " + arr.length + " and start from " + start);
    }

    int end = start + length - 1;

    while (start <= end) {
        int mid = start + (end - start) / 2;
        if (arr[mid] == search) {
            return mid;
        }
        if (search > arr[mid]) {
            end = mid - 1;
        } else {
            start = mid + 1;
        }
    }

    return -start - 1;
}


Can you replace a REPLACE statement, or 9? I would really like something to replace all of these REPLACE Statements.

SELECT DISTINCT
CAH.CaseNbr
    , REPLACE(REPLACE(REPLACE(REPLACE(REPLACE(REPLACE(REPLACE(REPLACE(REPLACE(
            (SELECT table1.Degree
            FROM SDUJS.dbo.fnWorstDegreeCharge(CB.CaseID) AS table1), 'F1', 'F')
            ,'F2','F'),'F3','F'),'F4','F'),'F5','F'),'F6','F'),'FA','F'),'FB','F')
            ,'FC','F') AS Degree
    , dbo.fncasecounty(CB.CaseID) AS County
FROM
    Justice.dbo.CaseBase AS CB
    INNER JOIN Justice.dbo.xCaseBaseChrg AS xCBC WITH (NOLOCK) ON xCBC.CaseID = CB.CaseID
    INNER JOIN Justice.dbo.Chrg AS Chrg WITH (NOLOCK) ON Chrg.ChargeID = xCBC.ChargeID
    INNER JOIN Justice.dbo.CaseAssignHist AS CAH WITH (NOLOCK) ON CAH.CaseID = CB.CaseID
    INNER JOIN Justice.dbo.ClkCaseHdr AS CCH WITH (NOLOCK) ON CCH.CaseID = CB.CaseID
WHERE
    CCH.DtFile BETWEEN '07/01/2010' AND '06/30/2011'
    AND
    CCH.CaseUTypeID IN (858,4330,865,4329,6362,5112)
ORDER BY CaseNbr

All possible entries for that column

  M1, M2, MO, PO, CHINS, F1, F2, F3, F4, F5, F6, FA, FB, FC, 00

I want all the Felonies to be represented by F

Code after the review

I decided not to do this as a follow up question because there isn't much that I think can be done to this query now to make it faster, except for get rid of the DISTINCT which I don't feel comfortable doing right now.

SELECT DISTINCT
    CAH.CaseNbr
    , (SELECT table1.Degree FROM SDUJS.dbo.fnWorstDegreeCharge(CB.CaseID) AS table1) AS Degree
    , dbo.fncasecounty(CB.CaseID) AS County
FROM
    Justice.dbo.CaseBase AS CB
    INNER JOIN Justice.dbo.CaseAssignHist AS CAH WITH (NOLOCK) ON CAH.CaseID = CB.CaseID
    INNER JOIN Justice.dbo.ClkCaseHdr AS CCH WITH (NOLOCK) ON CCH.CaseID = CB.CaseID
WHERE
    CCH.DtFile BETWEEN '07/01/2010' AND '06/30/2011'
    AND
    CCH.CaseUTypeID IN (858,4330,865,4329,6362,5112)

Missing REPLACE statement

I moved the "REPLACE" to the function that I am calling, but I used @rolfl's wonderful Case Statement idea to get rid of all the REPLACE statements inside that function.

This strikes me as best being done by using a temp table, or with clause.

Also, the DISTINCT part worries me.... why is that needed? Are your joins not good?

Still, I think you may have some more contentment with:

SELECT DISTINCT
    CAH.CaseNbr,
    (SELECT CASE WHEN Degree in ('F1','F2','F3','F4','F5','F6','FA','FB','FC')
                 THEN 'F'
                 ELSE Degree
            END
     FROM SDUJS.dbo.fnWorstDegreeCharge(CB.CaseID)) AS Degree,
  dbo.fncasecounty(CB.CaseID) AS County
....

You should still consider a better mechanism for joining. The procedure route seems 'klunky'


VBA Loop through multiple sheets to output formulas in multiple columns I write decently sloppy code and piecemeal together various things I find online to help me accomplish what needs to be done. This loop goes through 4 different worksheets to paste a formula in 3 columns. Based on some previous feedback from the site, I removed all of my .Select references and hopefully did things more cleanly. 2 questions that I have:

1) Can anyone think of a way I can further optimize this to have it run more quickly?

2) Is there a better way to define my RRSData!A2:O10000 formula.  I just chose 10,000 because there shouldn't be that many in the future, but its obviously working harder than it needs to now since there are only ~1000 rows currently.

Dim LastRow1 As Long, i1 As Long, InxW As Long
Dim wsNames As Variant

wsNames = Array("Sheet1", "Sheet2", "Sheet3", "Sheet4")

For InxW = LBound(wsNames) To UBound(wsNames)
   With Worksheets(wsNames(InxW))
        LastRow = Cells(Rows.Count, "B").End(xlUp).Row
        For i1 = 25 To LastRow
            With Range("KA1").Offset(i1 - 1, 0)
                .Formula = "=IF(SUMIF(RRSData!$A$2:$O$10000,OFFSET($KA$1,CELL(""row"",THIS),-285),RRSData!$M$2:$M$10000)>0,SUMIF(RRSData!$A$2:$O$10000,OFFSET($KA$1,CELL(""row"",THIS),-285),RRSData!$M$2:$M$10000),"""")"
                .NumberFormat = "0.00"
                .Value = .Value
            End With
            With Range("KB1").Offset(i1 - 1, 0)
                .FormulaR1C1 = "=IF(RC[-1]="""","""",If(RC[-1]>1.1,""High"",If(RC[-1]<0.8,""Low"",""Neutral"")))"
                .Value = .Value
            End With
            With Range("KC1").Offset(i1 - 1, 0)
                .FormulaR1C1 = "=IF(RC[-2]="""","""",IF(RC[-2]>1.1,""+5%"",IF(RC[-2]<0.8,""-5%"",""0%"")))"
                .Value = .Value
            End With
        Next i1
    End With
Next InxW

The code works, but it takes probably 2 minutes to loop through the four sheets, and the total number of rows across the four sheets is <500

A couple of tips.

When using With, make sure that you correctly reference the subordinate lines. Check the following example.

With Range("KA1").Offset(i1 - 1, 0) '<--- You have ...
With .Range("KA1").Offset(i1 - 1, 0) ' ... should be

Consider using FillDown or some similar command to do bulk edits. This will save looping, especially the loop that flips between the VBA engine and the Excel engine (which is expensive time-wise). Normally I suggest using arrays, but your use of formula suggests this other way.

The following example has not been tested.

Dim formulaOneRange as Range
    Set formulaOneRange = .Range("KA25:KA" & lastRow)
    With formulaOneRange
        .Formula = "=IF(SUMIF(RRSData!$A$2:$O$10000,OFFSET($KA$1,CELL(""row"",THIS),-285),RRSData!$M$2:$M$10000)>0,SUMIF(RRSData!$A$2:$O$10000,OFFSET($KA$1,CELL(""row"",THIS),-285),RRSData!$M$2:$M$10000),"""")"
        .NumberFormat = "0.00"
        .Value = .Value
    End With
    Set formulaOneRange = .Range("KB25:KB" & lastRow)
    With formulaOneRange
        .FormulaR1C1 = "=IF(RC[-1]="""","""",If(RC[-1]>1.1,""High"",If(RC[-1]<0.8,""Low"",""Neutral"")))"
        .Value = .Value
    End With
    Set formulaOneRange = .Range("KC25:KC" & lastRow)
    With formulaOneRange
        .FormulaR1C1 = "=IF(RC[-1]="""","""",If(RC[-1]>1.1,""High"",If(RC[-1]<0.8,""Low"",""Neutral"")))"
        .Value = .Value
    End With

Of course, the code could be tidies up a bit as per below, but I thought the extra lines will show my thinking a bit more clearly.

' My example code above ...
Set formulaOneRange = .Range("KC25:KC" & lastRow)
With formulaOneRange

' Could be ....
With .Range("KC25:KC" & lastRow)

Also, defining the range in an easy to manage variable would assist if you wanted to use FillDown or do some other manipulation within each section.


Underscore.js and jQuery templating I want to improve my code or structure. How does it look? Any comments or suggestions?

$(function() {

    function searchGit() {
        //grab value of search field
        var search = $('#search').val();

        // Validates entered value
        if (search.length > 0) {
            //pull json data from url
            $.ajax({
                url: 'https://api.github.com/legacy/repos/search/' + search,
                dataType: 'json',
                success: function(data) {

                    // use the results to template the html using _
                    var results = data.repositories,
                        resultsTemplate = _.template($("#results-template").html()),
                        resultingHtml = resultsTemplate({results : results});

                    // place the generated data into the html
                    $("#results-container").html(resultingHtml);
                }
            });
        } else {
            alert("Please enter repo name");
        }

        return false;
    }

    $('#submit').on('click', searchGit);
});

I would only change a few things around.  First, this is all one big monolithic function. Break it apart into smaller individual functions and try to have each function do one job only.

Second, I would convert from using the success callbacks to using Promises. That might be a matter of taste; Feel free to ignore. Third, I would encapsulate your code inside an IIFE to create a private scope. It also insures that $ is going to be jQuery.

Next, I would move all of the individual functions out of the document.ready event.  There is no reason to wait until then to load the functions.  Lastly, I would change to using the submit event instead of the click event. I would guess that is really when you need to do your call.

(function( $ ) {
  // IIFE for a private scope

  function getData( search ) {
    return  $.ajax({
       url: 'https://api.github.com/legacy/repos/search/' + search,
       dataType: 'json'
    });
  }

  function searchGit( event ) {
    //grab value of search field
    event.preventDefault();
    var search = $('#search').val();
    // Validates entered value
    if ( search.length ) {
      $.when( getData( search ) ).done( function( data ) {
        processData( data );
      });
      /* could also be written like this:
         var call = getData( search );
         call.done( function( data ) { ... });
                  OR
         getData( search ).done( function( data ) { ...});
      */
    } else {
      alert( "Please enter repo name" );
    }
    return false;
  }

  function processData( data ) {
    // use the results to template the html using _
    var results = data.repositories,
        resultsTemplate = _.template($("#results-template").html()),
        resultingHtml = resultsTemplate({results : results});

    // place the generated data into the html
    $("#results-container").html( resultingHtml );
  }

  $(function(){
    $('#submit').on( 'submit', searchGit );
  });

})( jQuery );

Eventually, I would recommend replacing all of the DOM selectors with variables.  That way it would be less coupled together.  For example, in the process data function:

  function processData( data , templateName, resultsElement ) {
    // use the results to template the html using _
    var results = data.repositories,
        resultsTemplate = _.template( $( templateName ).html() ),
        resultingHtml = resultsTemplate( {results : results} );
    // place the generated data into the html
    $( resultsElement ).html( resultingHtml );
  }

Hope that helps.


Optimize the Buddhabrot I am currently working on my own implementation of the Buddhabrot. So far I am using the std::thread-class from C++11 to concurrently work through the following iteration:

void iterate(float *res){
    //generate starting point
    std::default_random_engine generator;
    std::uniform_real_distribution<double> distribution(-1.5,1.5);

    double ReC,ImC;
    double ReS,ImS,ReS_;
    unsigned int steps;

    unsigned int visitedPos[maxCalcIter];

    unsigned int succSamples(0);

    //iterate over it
    while(succSamples < samplesPerThread){
        steps = 0;
        ReC = distribution(generator)-0.4;
        ImC = distribution(generator);
        double p(sqrt((ReC-0.25)*(ReC-0.25) + ImC*ImC));
        while (( ((ReC+1)*(ReC+1) + ImC*ImC) < 0.0625) || (ReC < p - 2*p*p + 0.25)){
            ReC = distribution(generator)-0.4;
            ImC = distribution(generator);
            p = sqrt((ReC-0.25)*(ReC-0.25) + ImC*ImC);
        }
        ReS = ReC;
        ImS = ImC;
        for (unsigned int j = maxCalcIter; (ReS*ReS + ImS*ImS < 4)&&(j--); ){
            ReS_ = ReS;
            ReS *= ReS;
            ReS += ReC - ImS*ImS;
            ImS *= 2*ReS_;
            ImS += ImC;
            if ((ReS+0.5)*(ReS+0.5) + ImS*ImS < 4){
                visitedPos[steps] = int((ReS+2.5)*0.25*outputSize)*outputSize + int((ImS+2)*0.25*outputSize);

            }
            steps++;

        }
        if ((steps > minCalcIter)&&(ReS*ReS + ImS*ImS > 4)){
            succSamples++;
            for (int j = steps; j--;){
                //std::cout << visitedPos[j] << std::endl;
                res[visitedPos[j]]++;
            }
        }
    }
}

These are my declarations and main():

#include <iostream>
#include <string>
#include <thread>
#include <random>
#include <vector>

#define outputSize 20000

#define samplesPerThread 5000
#define numThreads 16

#define maxCalcIter 60000
#define minCalcIter 40000

//iterate function

int main(){
    float *outImg = new float[outputSize*outputSize];

    std::vector<std::thread> threads;
    for (int i = numThreads; i--; ){
        threads.push_back(std::thread(iterate,outImg));
    }
    for (auto& t : threads){
        t.join();
    }

    return 0;
}

I am basically working in every thread so long that I generated enough trajectories of sufficient length which in expectation takes the same time in every thread.

But I really have the feeling that this function might me very unoptimized since its code is so very readable. Can anybody come up with some fancy optimizations? When it comes to compiling I just use:

  g++ -O4 -std=c++11 -I/usr/include/OpenEXR/ -L/usr/lib64/ -lHalf -lIlmImf -lm buddha_cpu.cpp -o buddha_cpu

Any hints on crunching some more numbers/sec would be really appreciated. Also, any links to further literature are totally welcome.

Instead of talking about the algorithm, I will review what can be done better even if I don't understand the algorithm. Actually, I will review what can be done better so that I can understand the algorithm:

First of all, don't use macros to define your constants. Since you are using C++11, you can make them constexpr variables instead and use them as array indices too if needed:

constexpr unsigned int outputSize = 20000;
constexpr unsigned int samplesPerThread = 5000;
constexpr unsigned int numThreads = 16;

constexpr unsigned int maxCalcIter = 60000;
constexpr unsigned int minCalcIter = 40000;

You are using std::sqrt but your forgot to include the header <cmath>. While it has probably been included by some other header, it is not guaranteed by the standard thus not including it may cause your program to fail to compile with some implementations.
By the way, try to always std::-qualify the functions from the standard library. Unless you are explicitly using argument-dependent lookup of course.
This line is not really readable:

for (unsigned int j = maxCalcIter; (ReS*ReS + ImS*ImS < 4)&&(j--); ){

It would be better as:

for (unsigned int j = maxCalcIter; (ReS*ReS + ImS*ImS < 4) && (j > 0); --j){

You may have heard that iterating from max to zero is faster, but in most of the cases, this is at best a micro-optimization. The "natural" ordering is from 0 to max and that's what readers will expect. Generally speaking, choosing a good algorithm and good data structures will help you more than micro-optimizing. Worse: if micro-optimizations are done too early when they are not needed, it will hinder readability.

Considering that step is incremented for each iteration of the loop, you can put the incrementation in the for too:

for (unsigned int j = 0 ;
     (ReS*ReS + ImS*ImS < 4) && (j < maxCalcIter) ;
     ++j, ++steps)
{ /* ... */ }

By the way, with modern caches and processors, iterating forward may be significantly faster than iterating backwards if your iterator is used to access memory. Therefore, I expect the last loop of your function to be faster (and more readable) if you iterate forward instead of backwards since the index j is actually used to access memory.
Now, let's see your main function too: if you don't write any return statement in main, the compiler will automagically add return 0;. Unless you also intend to return error codes, not writing return 0; may be a way of documenting that your program cannot return error codes.
You are allocating memory manually in the following line:

float *outImg = new float[outputSize*outputSize];

You should avoid manually allocating memory since it can lead to memory leaks if you forget to delete it later (hint: your forgot). You can always use an std::vector to create a dynamic array and you can access its underlying memory thanks to the method data().

Even better: you know the size of the allocated memory at compile-time. Therefore, you can use an std::array instead. This class allocates its memory on the stack, which means that iteration and some other operations may be faster. Avoid using one if your array is so big that it could blow the stack though (well, in your case, I guess that it would blow the stack).
You are using threads.push_back(std::thread(/* ... */)) to create new threads and add them to threads. You can use emplace_back to create them directly into the vector instead. Your code will even be shorter:

threads.emplace_back(/* ... */);


Deallocate all memory in a container of pointers I want to erase all memory in a container. Currently I'm using this :

template<typename Cont>
void clear_mem(Cont &container)
{
    while (!container.empty())
    {
        delete container.back();
        container.pop_back();
    }
}

Should the operation of deleting the memory held by the contents be separated by clearing the container ?
Is it pointless to make this function a template, since it can only  operate on vector and list ?

This assumes that the container holds raw pointer and it owns the pointers. it is a better idea to then let the container hold smart pointers.

These will automatically clear the object they hold when they get destroyed (using delete by default).

typedef std::unique_ptr<Type> Type_ptr;
std::vector<Type_ptr> container;

and use emplace to fill it;

container.emplace_back(std::make_unique<Type>());


Reduce an X/Y Value to an Index In a 1D array and Return the Array Value Initially, this is not an attempt at early optimization, though it's thoroughly possible I'm looking in the wrong place :).
The following function returns a height value for aliased columns in a kind of voxel-style height-map renderer.
It is called many, many times for both AI and player controlled entities and used for collision detection. It is not being called unnecessarily, by any entities that are static by nature. So, it's really a question of whether there are any tricky operations I'm missing that could allow the interpreter to better JIT-compile my code.
Please assume that 'mapcol' is an array of [0,255] Numbers with a length of mapscalex*mapscalez. The map dimensions may actually be quite a bit bigger than is suggested here; relevant only because I think any memoization would flood RAM.
I've made my best attempt to sanitize this code for brevity. It naturally references variables that are necessarily augmented on level-load, here represented as simple globals.
var mapscalex = 256;
var mapscalez = 256;
var tilescale = 16;
var tilescaley = 4;

//Returns the Y value for a pair of X/Zs
function getmapheight(x,z){
      //Drop the coordinate to the map scale
      x = x/tilescale;
      z = z/tilescale;

      //Clamp the coordinate to the map dims
      x = Math.min(mapscalex-1,Math.max(0,Math.floor(x)));
      z = Math.min(mapscalez-1,Math.max(0,Math.floor(z)));

      //Project the 2D coord into a 1D coord for map sampling,--
      //and then return the map value scaled into world coordinates
      return(mapcol[(x+z*mapscalex)]*tilescaley);
}

From a short review;

the tilescale is 16 which is a great value to multiply or divide with, so
x = x/tilescale;
z = z/tilescale;

can become
x = x >> 4; //Divide by 16 (tilescale)
z = z >> 4; //Divide by 16 (tilescale)

this does mean that changing the tilescale can become a performance drag (like in real games)

So are 256 and 4, so
return(mapcol[(x+z*mapscalex)]*tilescaley);

could be
return(mapcol[(x+z<<8)]<<2); //Nice comment here

~~ is faster than Math.floor() but for negative values it does return ~~-6.3 -> 6

Don't count on Math to be fast, I dont think I've ever seen Math be faster in anything, even an if statement is faster than Math.max(0,Math.floor(x))

When you know that x or z are either negative or greater than 255, why not just exit with a fixed value?

Check out 8 bit arrays, they might reduce the need for clamping in your code


Database schema for holding game save files I have a database schema for a game to hold save files where some tables have a one-to-one relationship to classes.  I was wondering if I should split up some tables.

Example: troop_class table. It could have a PK of

{ GameID,PostionX,PostionY,PostionZ,Team }

but then team don't really rely on the positions and could be split to two new tables

game_units Keys{GameID, troop_id} PK{GameID}

and

troop_team{troop_id, Team} PK{troop_id}.

I am wondering if I should break up the one-to-one mapping for table-class and start to normalize and create proper PK instead of using id.

CREATE TABLE accounts(
    AccountName VARCHAR(50) UNIQUE NOT NULL,
    Password BLOB NOT NULL,
    PRIMARY KEY(AccountName)
) Engine=InnoDB;

CREATE TABLE maps(
    maps_id INT NOT NULL UNIQUE,
    MapName VARCHAR(30) NOT NULL,
    PRIMARY KEY (maps_id),
    INDEX(maps_id)
) ENGINE=InnoDB;

CREATE TABLE game_searching_player(
    AccountName VARCHAR(50) NOT NULL,
    MapID INT NOT NULL,
    PRIMARY KEY (AccountName, MapID),
    FOREIGN KEY (AccountName) REFERENCES accounts(AccountName)
                      ON DELETE CASCADE ,
    FOREIGN KEY (MapID) REFERENCES maps(maps_id)
                      ON DELETE CASCADE ,
    INDEX(AccountName)
) ENGINE=InnoDB;

CREATE TABLE games(
    games_id INT NOT NULL AUTO_INCREMENT UNIQUE,
    Player1 VARCHAR(50) NOT NULL,
    Player2 VARCHAR(50) NOT NULL,
    WhosTurn INT NOT NULL,
    TurnHasEnded BOOL NOT NULL,
    HaveWon BOOL NOT NULL,
    TurnNumber INT NOT NULL,
    MapID INT NOT NULL,
    Type VARCHAR(30) NOT NULL,
    PRIMARY KEY (games_id),
    FOREIGN KEY (Player1) REFERENCES accounts(AccountName)
                      ON DELETE CASCADE ,
    FOREIGN KEY (Player2) REFERENCES accounts(AccountName)
                      ON DELETE CASCADE ,
    FOREIGN KEY (MapID) REFERENCES maps(maps_id)
                      ON DELETE CASCADE ,
    INDEX(games_id,Player1, Player2  )
) ENGINE=InnoDB;

CREATE TABLE player(
    GameID INT NOT NULL,
    Player VARCHAR(30) NOT NULL,
    VictoryResources INT NOT NULL,
    UniversalResources INT NOT NULL,
    Type VARCHAR(30) NOT NULL,
    PRIMARY KEY (GameID, Player),
    FOREIGN KEY (GameID) REFERENCES games(games_id)
                      ON DELETE CASCADE ,
    INDEX(GameID)

) ENGINE=InnoDB;

CREATE TABLE troop_class (
    troop_class_id INT NOT NULL AUTO_INCREMENT UNIQUE,
    GameID INT NOT NULL,
    Health INT NOT NULL,
    CombatInitiative INT NOT NULL,
    Spotted BOOL NOT NULL,
    Entrenched INT NOT NULL,
    Initiative INT NOT NULL,
    Ammo INT NOT NULL,
    Petrol INT NOT NULL,
    ActionPoints INT NOT NULL,
    IsMoving BOOL NOT NULL,
    PositionX INT NOT NULL,
    PositionY INT NOT NULL,
    PositionZ INT NOT NULL,
    Team VARCHAR(30) NOT NULL,
    Name VARCHAR(30) NOT NULL,
    Type VARCHAR(30) NOT NULL,
    PRIMARY KEY (troop_class_id),
    FOREIGN KEY (GameID) REFERENCES games(games_id)
                      ON DELETE CASCADE ,
    INDEX(GameID)
) ENGINE=InnoDB;

CREATE TABLE supply_unit(
    supply_unit_id INT NOT NULL UNIQUE,

    SupplyPetrol INT NOT NULL,
    SupplyAmmo INT NOT NULL,
    VictoryResources INT NOT NULL,
    LoadWeight INT NOT NULL,
    MaxWeight INT NOT NULL,
    PRIMARY KEY (supply_unit_id),
    FOREIGN KEY (supply_unit_id) REFERENCES troop_class(troop_class_id)
                      ON DELETE CASCADE

) ENGINE=InnoDB;

CREATE TABLE central_warehouse (
    central_warehouse_id INT NOT NULL AUTO_INCREMENT UNIQUE,
    GameID INT NOT NULL,
    SupplyPetrol INT NOT NULL,
    SupplyAmmo INT NOT NULL,
    VictoryResources INT NOT NULL,
    LoadWeight INT NOT NULL,
    MaxWeight INT NOT NULL,
    PositionX INT NOT NULL,
    PositionY INT NOT NULL,
    PositionZ INT NOT NULL,
    Team VARCHAR(30) NOT NULL,
    Name VARCHAR(30) NOT NULL,
    Type VARCHAR(30) NOT NULL,
    PRIMARY KEY (central_warehouse_id),
    FOREIGN KEY (GameID) REFERENCES games(games_id)
                      ON DELETE CASCADE ,
    INDEX(GameID)

) ENGINE=InnoDB;

Here are a few suggestions, many of which are just "best practices" I've picked up along the way (or simplifications that would make things a bit more concise and easier to read).  There are also a few ideas that will likely prevent common errors from happening to the code as it's maintained in the future (that I've run across in many code reviews).

I'd create an id column on the accounts table:

CREATE TABLE accounts(
  account_id INT UNIQUE NOT NULL,
  AccountName VARCHAR(50) UNIQUE NOT NULL,
  Password BLOB NOT NULL,
  PRIMARY KEY(account_id)
) Engine=InnoDB;

Then the tables that reference accounts like game_searching_player will reference an INT instead of VARCHAR(50) - (I think I see at least 4 references to that column, which would be much simpler if they were ints.)  You may also likely get a small amount of performance improvement by using INT for these keys instead of VARCHARs.
MySQL automatically indexes PRIMARY KEYs, so no need for those explicit INDEX lines.

For example, this:

PRIMARY KEY (maps_id),
INDEX(maps_id)

would become just

 PRIMARY KEY (maps_id)

MySQL's InnoDB engine automatically indexes FOREIGN KEYs, so no need for those explicit INDEX lines either (player and troop_class and game_searching_player).

For example, this:

CREATE TABLE player(
  ...

  PRIMARY KEY (GameID, Player),
  FOREIGN KEY (GameID) REFERENCES games(games_id)
              ON DELETE CASCADE ,
  INDEX(GameID)

) ENGINE=InnoDB;

becomes this:

CREATE TABLE player(
  ...

  PRIMARY KEY (GameID, Player),
  FOREIGN KEY (GameID) REFERENCES games(games_id)
              ON DELETE CASCADE

) ENGINE=InnoDB;

You don't have AUTO_INCREMENT on all your table IDs, but I suspect that you actually want it on them all, since it will simplify your insertion code.
In MySQL all PRIMARY KEYs are also guaranteed to be UNIQUE and NOT NULL, so those attributes aren't really needed on those columns, though my personal preference is that it adds to the readability to leave those attributes on the columns, but thought I'd mention it for reference.
Your player table should probably reference the (new) id in the  accounts table (with a ON DELETE CASCADE setting too).  It also looks like players can have names up to 30 characters long, but accounts can have names up to 50 characters.  (The difference in lengths may also of course indicate that I completely mis-understand the purpose of the player and account tables, and that they really shouldn't be related in any way.)
While it will work perfectly fine the way you have it shown here, in most of the environments and organizations I've worked with/in, it has been standard best practice to make the name of every table's "id" column be id, rather than including the table name in the column name (like maps_id).  This "coding style" choice allows the code to be more concise and since this is a very common practice, it doesn't add any "cognitive load" as maintainers read your queries/schema/code.


Merging trips to the same location with overlapping dates I have a SQLite database that only contains the holidays for 1 user.

I have a new Trip object (newTrip) that has a StartDate(long unix time) and an EndDate(long unix time) and the trip Iso3Code(string) for the location.

I need to retrieve from the database all existing Trips that can be merged with the newTrip. A trip can be merged if the dates of the new trip fall at the same time of the existing trips or on the day before or after the existing trips and have the same Iso2Code`.

So far I have this:

public static List<Trip> GetTripsThatCanBeMerged(Trip trip)
{
    //ignore times - just get for that date and get the date before for the start date and the day after for the end data, as the day before and after can be merged.
    DateTime startDate = DateTimeHelper.UnixDateToDateTime(trip.StartDate).Date.AddDays(-1);
    DateTime endDate = DateTimeHelper.UnixDateToDateTime(trip.EndDate).Date.AddDays(1);

    //now convert back to unix
    long startDateUnix = DateTimeHelper.DateTimeToUnixTimestamp(startDate);
    long endDateUnix = DateTimeHelper.DateTimeToUnixTimestamp(endDate);

    List<Trip> trips = null;

    using (SQLiteConnection conn = new SQLiteConnection(new SQLitePlatformWinRT(), _sqliteDatabasePath))
    {
        trips = conn.GetAllWithChildren<Trip>(p =>
            ((p.StartDate >= startDateUnix && p.StartDate <= endDateUnix) ||
            (p.EndDate >= startDateUnix && p.EndDate <= endDateUnix) ||
            (p.StartDate <= startDateUnix && (endDateUnix >= p.StartDate && endDateUnix <= p.EndDate))) &&
            p.Iso3Code == trip.Iso3Code
            , true);
    }

    return trips;
}

I think this is correct but can anyone suggest a less complex way of doing it, or see any errors.

Let's look at this line:

(p.StartDate <= startDateUnix && (endDateUnix >= p.StartDate && endDateUnix <= p.EndDate))

If p.StartDate <= startDateUnix then p.StartDate <= endDateUnix, since startDateUnix <= endDateUnix. So endDateUnix >= p.StartDate is redundant.

So the first part of the query can be written as

(p.StartDate >= startDateUnix && p.StartDate <= endDateUnix) ||
(p.EndDate >= startDateUnix && p.EndDate <= endDateUnix) ||
(p.StartDate <= startDateUnix && endDateUnix <= p.EndDate)

I would recommend rewriting the first two conditions as follows:

(startDateUnix <= p.StartDate && p.StartDate <= endDateUnix) ||
(startDateUnix <= p.EndDate && p.EndDate <= endDateUnix)

I find it easier to see at a glance that it's saying that p.StartDate lies within [startDateUnix, endDateUnix] (similarly for p.EndDate).

Another way to test for overlapping intervals is

p.StartDate <= endDateUnix && startDateUnix <= p.EndDate


Numeric double integration I've made a simple program for numerically aproximating double integral, which accepts that the bounds of the inner integral are functions:

import numpy as np
import time

def double_integral(func, limits, res=1000):
        t = time.clock()
        t1 = time.clock()
        t2 = time.clock()
        s = 0
        a, b = limits[0], limits[1]
        outer_values = np.linspace(a, b, res)
        c_is_func = callable(limits[2])
        d_is_func = callable(limits[3])
        for y in outer_values:
            if c_is_func:
                c = limits[2](y)
            else:
                c = limits[2]
            if d_is_func:
                d = limits[3](y)
            else:
                d = limits[3]
            dA = ((b - a) / res) * ((d - c) / res)
            inner_values = np.linspace(c, d, res)
            for x in inner_values:
                t2 = time.clock() - t2
                s += func(x, y) * dA
            t1 = time.clock() - t1
         t = time.clock() - t
         return s, t, t1 / res, t2 / res**2

This is, however, terribly slow. When res=1000, such that the integral is a sum of a million parts, it takes about 5 seconds to run, but the answer is only correct to about the 3rd decimal in my experience. Is there any way to speed this up?

The code i am running to check the integral is

def f(x, y):
    if (4 - y**2 - x**2) < 0:
        return 0              #This is to avoid taking the root of negarive #'s
    return np.sqrt(4 - y**2 - x**2)

def c(y):
    return np.sqrt(2 * y - y**2)

def d(y):
    return np.sqrt(4 - y**2)
# b d
# S S f(x,y) dx dy
# a c
a, b, = 0, 2
print(double_integral(f, [a, b, c, d]))

The integral is eaqual to 16/9

If you want to use numpy, use numpy properly. Inestead of

for x in inner_values:
    s += func(x, y) * dA

use the more idiomatic, and much faster

s += dA * np.sum(func(inner_values, y))

Note: this requires rewriting f as

return np.sqrt(np.maximum(0, 4 - y**2 - x**2))

so it can accept an array as an input. This does not reduce accuracy, but brings time down to a much more accaptable .04 seconds for a 100x improvement. The takeaway here is Numpy is not magic. It provides quick vectorization.


Compare length of iterator with as few `next` calls as possible. How can I deduplicate this code? I'm writing a few simple functions that compare the length of an iterator to an integer, returning as early as possible. Right now I have:

class ILengthComparator:
    def __init__(self, iterator: Iterator):
        self.iterator = iterator

    def has_more(self) -> bool:
        try:
            next(self.iterator)
        except StopIteration:
            return False

        return True

    def __eq__(self, length: int) -> bool:
        if length < 0:
            return False

        if not self.has_more():
            return length == 0

        return self == length-1

    def __gt__(self, length: int) -> bool:
        if length < 1:
            return False

        if not self.has_more():
            return True

        return self > length-1

    def __lt__(self, length: int) -> bool:
        if length < 0:
            return True

        if not self.has_more():
            return False

        return self < length - 1

Clearly this class isn't finished (__neq__, __gte__, __lte__), and anything after the first comparison isn't guaranteed to work (as the iterator may be consumed), but I'm wondering: is it is possible to logically simplify this into a single function with an operator function as an argument (ie operator.eq, operator.lt), or at least just reduce the semi-duplicate parts (ie, every function has a comparison with length against a number, and a case for checking if the iterator is empty - each with a rather ad-hoc return value).

(any suggestions don't need to be recursive or use the has_more method, those were just the first thing I reached for when trying to solve this problem)

Pastebin with the above some quick and dirty unit tests: https://pastebin.com/N65Q2qcJ

To know the answer to your comparison to \$N\$, you need to attempt at least:

\$N\$ next calls if you test with \$\lt\$ (succeeding will result to False) or \$\ge\$ (succeeding will result to True);
\$N+1\$ next calls in all 4 other cases.

Obviously, if the iterator length is fewer than \$N\$ there will be less calls.

So, in order to simplify things, you could extract the first \$N+1\$ elements of the iterator and count them to compare this count to \$N\$:

extracting can be simplified with itertools.islice
counting can be done using either

enumerate and dumping the content into a 1-length collections.deque

from itertools import islice
from collections import deque

def compare_length(iterable, n, operation):
    chunck = islice(iterable, max(0, n+1))
    try:
        (length, _) = deque(enumerate(chunk, 1), maxlen=1)
    except ValueError:
        length = 0
    return operation(length, n)

sum

from itertools import islice

def compare_length(iterable, n, operation):
    chunck = islice(iterable, max(0, n+1))
    length = sum(1 for _ in chunck)
    return operation(length, n)

The former should be more efficient, the latter more readable. As suggested in the question, this function takes the operation to perform as its third argument and compares min(<iterable length>, n+1) to n. This is sufficient in most cases.

If you truly want to reduce the amount of next calls needed for \$\lt\$ and \$\ge\$, you can keep your class approach and factorize the min(<iterable length>, n) part into an helper method:

from itertools import islice

class ILengthComparator:
    def __init__(self, iterable):
        self._iterable = iterable

    def _length_up_to(self, n):
        return sum(1 for _ in islice(self._iterable, max(0, n)))

    def __eq__(self, n):
        return self._length_up_to(n+1) == n

    def __ne__(self, n):
        return self._length_up_to(n+1) != n

    def __lt__(self, n):
        return self._length_up_to(n) < n

    def __le__(self, n):
        return self._length_up_to(n+1) <= n

    def __gt__(self, n):
        return self._length_up_to(n+1) > n

    def __ge__(self, n):
        return self._length_up_to(n) >= n

Lastly, if you want to avoid consuming the iterator while performing those tests, you can check itertools.tee.


Optimize KnockoutJs Pagination Computed Function I have written a function that builds an array of paging options for rendering on the view.

I have modelled it after DataTables for jQuery, a live example with lots of data is available here: http://www.companieshousedata.co.uk/a/490

Is there a better way to write this?
The function also returns ellipsis (as strings) where necessary.

Model/object:

function PageDirection(data) {
    var self = this;
    self.display = data.display;
    self.value = data.value;
    self.disabled = function (currentPage, lastPage) {
        return self.value() > lastPage || self.value() < 1 || self.value() == null;
    };
}

Computed observable:

self.pageDirections = ko.computed(function () {
    var pageIndex = self.pageIndex();
    var lastPage = self.lastPage();

    //Previous
    var directions = [
        new PageDirection({
            display: "Previous",
            value: ko.computed(function() {
                return parseInt(pageIndex, 10) - 1;
            })
        })
    ];

    //6 or less pages
    if (self.lastPage() <= 6) {
        for (var i = 0; i < lastPage; i++) {
            directions.push(new PageDirection({
                display: i + 1,
                value: ko.observable(i + 1)
            }));
        }
    } else {
        if (pageIndex > 4) {
            //1
            directions.push(new PageDirection({
                display: 1,
                value: ko.observable(1)
            }));
            //...
            directions.push(new PageDirection({
                display: "...",
                value: ko.observable(null)
            }));

            if (lastPage - pageIndex <= 3) {
                //last 5
                for (var k = lastPage - 5; k < lastPage; k++) {
                    directions.push(new PageDirection({
                        display: k + 1,
                        value: ko.observable(k + 1)
                    }));
                }
            } else {
                //-1
                directions.push(new PageDirection({
                    display: pageIndex - 1,
                    value: ko.observable(pageIndex - 1)
                }));
                //current
                directions.push(new PageDirection({
                    display: pageIndex,
                    value: ko.observable(pageIndex)
                }));
            }

            if (lastPage - pageIndex > 3) {
                //+1
                directions.push(new PageDirection({
                    display: pageIndex + 1,
                    value: ko.observable(pageIndex + 1)
                }));
                //...
                directions.push(new PageDirection({
                    display: "...",
                    value: ko.observable(null)
                }));
            }
        } else {
            //first 5
            for (var l = 0; l < 5 ; l++) {
                directions.push(new PageDirection({
                    display: l + 1,
                    value: ko.observable(l + 1)
                }));
            }
            //...
            directions.push(new PageDirection({
                display: "...",
                value: ko.observable(null)
            }));

        }
        if (lastPage - pageIndex > 3) {
            //last
            directions.push(new PageDirection({
                display: lastPage,
                value: ko.observable(lastPage)
            }));
        }
    }

    //Next
    directions.push(new PageDirection({
        display: "Next",
        value: ko.computed(function () {
            return parseInt(pageIndex, 10) + 1;
        })
    }));

    return directions;
});

View (for reference):

Let's take a look at that first line of the PageDirection function:

var self = this;

This is a common practice to keep a reference to the this object within the closure. A different way to do this would be to bind the context of the closure using Function.bind()
this.disabled = function (currentPage, lastPage) {
    return this.value() > lastPage || this.value() < 1 || this.value() == null;
}.bind(this);

However, there appears to be no need to bind the context

function PageDirection(data) {
    this.display = data.display;
    this.value = data.value;
    this.disabled = function (currentPage, lastPage) {
        console.log('this.value: ', this.value(), ' > lastPage: ', this.value() > lastPage);
        return this.value() > lastPage || this.value() < 1 || this.value() == null;
    };
}

const pd1 = new PageDirection({value: _ => 16, display: 'BMW'});
console.log('pd1 disabled: ', pd1.disabled(1, 10));

Though maybe the KO code invokes that method differently, in which case the function may need to be bound.
Another option would be to use an ES6 arrow function expression, which doesn't have its own this binding:
this.disabled = (currentPage, lastPage) =>
                this.value() > lastPage || this.value() < 1 || this.value() == null;

To be honest, the logic in the observable looks a bit overly-complex. I didn't fully study it and grok it but I would look to see if some of the logic can be simplified - for example:

} else {
    if (pageIndex > 4) {

Unless it is the case that the subsequent logic wouldn't work you might consider if that can be converted to:
} else if (pageIndex > 4) {

Which could decrease the sub-sequent indentation levels.


Reversing a linked list (Java) Triggered by an interview question (I didn't answer during the interview) I wrote a Java program to reverse a linked list. I went with a loop-based approach instead of recursion. I would like to know the following:

Is the reverse() method efficient as it is, or can it be improved?
Are the inline comments inside the reverse() method clear or are they confusing and/or superfluous?

The following is the code for the entire LinkedList class I wrote, containing a public reverse() method:

public class LinkedList<T> {
    /**
     * A Node represents a single item inside the linked list. Each node contains
     * the value of the item and a pointer to the next node.
     */
    class Node {
        T value;
        Node next;
    }

    /**
     * The 'head' is the beginning of the linked list.
     */
    Node head;

    /**
     * Add an item to the end of the linked list.
     *
     * @param value - The value of the item to be added
     */
    public void add(T value) {
        // Create the new node to be added
        Node newNode = new Node();
        newNode.value = value;

        // If list is empty, add it at the head
        if (head == null) {
            head = newNode;
            return;
        }

        // If the list is not empty, find the end of the list and add it there
        Node current = head;
        while (current.next != null) {
            current = current.next;
        }
        current.next = newNode;
    }

    /**
     * Perform an in-place reversal of the elements in the linked list.
     *
     * After this method is called, the order of the elements in the linked list
     * will be reversed.
     */
    public void reverse() {
        Node current = head;        // Start at the beginning
        Node previous = null;       // In each iteration, points to the current node's previous node
        Node next;                  // In each iteration, we use this to backup the current node's next node
        while (current != null) {
            next = current.next;        // Backup current node's next node
            current.next = previous;    // Point current node backwards - to the previous node
            previous = current;         // Set new previous to current node, for next iteration
            head = current;             // Point head to current, so by end of loop, it will point to the original tail
            current = next;             // Point current to the original next node, for next iteration
        }
    }

    @Override
    public String toString() {
        StringBuilder sb = new StringBuilder("");
        Node current = head;
        while (current != null) {
            sb.append(current.value + " ");
            current = current.next;
        }
        return sb.toString();
    }
}

The following is the code I used to test the above method, just in case it is required:

public class LinkedListReversal {
    public static void main(String[] args) {
        LinkedList<Integer> list = new LinkedList<>();
        list.add(1);
        list.add(2);
        list.add(3);
        list.add(4);

        System.out.println(list);

        list.reverse();

        System.out.println(list);
    }
}

Questions

  Is the reverse() method efficient as it is, or can it be improved?

You cannot reverse a standard linked list without touching all its nodes, which has a lower-bound asymptotic complexity of o(N) operations.  Your reverse() method has O(N) complexity and uses O(1) extra space, so you cannot improve its asymptotic complexity in either of those dimensions.  Note that the usual recursive solution consumes o(N) overhead, all on the stack, so an iterative implementation is clearly a superior choice.

You could, however, make it slightly more efficient by updating head just once, at the end, instead of at every iteration.  In particular, instead of head = current; inside the loop, you could use head = previous; immediately after the loop.  No other changes would be required.

Consider, however, that if speed of the reverse() method were of paramount importance, such that it was reasonable to sacrifice a bit of list-traversal performance, then you could create a List class for which reverse() is O(1).  I would do that with an internal circular, doubly-linked list of Nodes, with node traversal performed via a Node method that relies on an instance variable of the host List to determine which direction to consider as forward.  With such a structure, (logically) reversing the list would be achieved simply by changing the value of the single variable that controls list direction.  Example:

int direction = 0;

class Node {
    T value;
    Node[] links = new Node[2];
    public Node next() {
        assert ((direction >= 0) && (direction < links.length));
        return links[direction];
    }
}

public void reverse() {
    direction = 1 - direction;
}

  Are the inline comments inside the reverse() method clear or are they confusing and/or superfluous?

They are both clear and largely superfluous.  You've chosen reasonably good names for your variables, so the meaning of the code is fairly well readable from the code itself.  More generally, I rarely think it appropriate to comment every single one of a series of simple statements.  It is more effective to document at a somewhat higher level: if each statement reads well in and of itself then document the purpose of groups of lines.

Also, I prefer trailing comments to be very short.  If you have something longer to say then put it on its own line or lines.

Additional comments

Clearly your LinkedList class is constructed for the express purpose of demonstrating reverse(), as it is missing methods that one would expect a general-purpose list class to provide.  That might or might not be an issue if you were to present such an answer in an interview.  Note that you could provide a full-featured implementation of java.util.List by extending AbstractSequentialList and implementing its listIterator() method.

On general principles, the inner Node class should have a constructor with which its value field and maybe its next field can be initialized.  It may be appropriate to make the value final, too, as @forsvarir also suggested -- this is possible only if you provide a constructor.