Querying a list of jobs on a server using EJBs Inspired by this question, I have a Java Enterprise application running in Glassfish 2.1 and a Java SE client that communicates with the server application.

For this I have a bean that maintains a collection of jobs that is set by the server and the client then asks if his job is in the current list of jobs known to the bean.

I want to ask if this code is good.

The client uses this interface:

import javax.ejb.Remote;
@Remote
public interface GeneratorCancelledRemote {
    public boolean isJobCancelled(String jobId);
}

The server uses this interface:

import java.util.Collection;
import javax.ejb.Local;
@Local
public interface GeneratorCancelledLocal {
    public void setJobs(final Collection<String> jobs);
}

And here's the implementation:

import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import javax.ejb.Stateless;
@Stateless
public class GeneratorCancelled implements GeneratorCancelledLocal, GeneratorCancelledRemote {
    private Collection<String>  jobs;
    public void setJobs(final Collection<String> jobs) {
        this.jobs = Collections.synchronizedSet(new HashSet<String>());
        this.jobs.addAll(jobs);
    }
    @Override
    public boolean isJobCancelled(final String jobId) {
        return jobs != null && jobs.contains(jobId);
    }
}

Obviously access to the collection of jobs has to be synchronized somehow. Is the use of Collections.synchronizedSet the best way to do this? The EJB gurus around here shun on the use of synchronized in an EE context.

I found this (in German), which also explains that synchronized is not allowed in EJB.

Your @Stateless service contains a shared state. It looks weird for me.
You never modify a state of jobs, you always create a new object

this.jobs = Collections.synchronizedSet(new HashSet<String>());

Could you please clarify why ?
From synchronization standpoint you have a race condition in setJobs method, so it should be synchronized.
Now, since your write operation (setJobs method) is protected by the intrinsic lock, all read operations should be also protected by the same lock to ensure data visibility. Make isJobCancelled synchronized too.


Basic 'char' Stack implementation in C #include <stdio.h>
#include <assert.h>
#include <stdlib.h>
#define STACK_SIZE 100

typedef struct _stack
{
    char *mem;
    size_t size;
    int top;
} STACK;

STACK *stack_create(char *mem, size_t size);
void stack_push(STACK *stack, char c);
char stack_pop(STACK *stack);
char stack_peek(STACK *const stack);
void stack_print(STACK *stack);
int main() {
    char stack_mem[STACK_SIZE];

    STACK *my_stack = stack_create(stack_mem, STACK_SIZE);
    stack_push(my_stack, 'a');
    stack_push(my_stack, 'b');
    stack_push(my_stack, 'c');
    stack_push(my_stack, 'd');
    stack_print(my_stack);
    printf("Top of stack contains: %c\n", stack_peek(my_stack));
    stack_pop(my_stack);
    stack_print(my_stack);
    stack_pop(my_stack);
    stack_pop(my_stack);
    stack_pop(my_stack);
    stack_push(my_stack, 'j');
    stack_push(my_stack, 'k');
    stack_print(my_stack);

    free(my_stack);
    return EXIT_SUCCESS;
}

STACK *stack_create(char *mem, size_t size) {

    assert(mem && size);

    STACK *output = malloc(sizeof(STACK));

    output->mem = mem;
    output->size = size;
    output->top = -1;
    return output;
}

void stack_push(STACK *stack, char c)
{
    assert(stack);

    if(c < 0)
    {
        fprintf(stderr, "This stack is not designed for negative char values\n");
        return;
    }
    if((stack->top + 1) < stack->size)
    {
        stack->mem[stack->top + 1] = c;
        stack->top++;
    }
    return;
}

char stack_pop(STACK *stack)
{
    assert(stack);

    if(stack-> top < 0)
    {
        fprintf(stderr, "Cannot pop from empty stack.\n");
        return -1;
    }

    char o;
    o = stack->mem[stack->top];
    stack->top--;
    return o;
}

char stack_peek(STACK * const stack)
{
    assert(stack);

    if(stack->top < 0)
    {
        fprintf(stderr, "Nothing on stack to peek at.\n");
        return -1;
    }
    return stack->mem[stack->top];
}

void stack_print(STACK *stack)
{
    assert(stack);
    if(!stack->top)
    {
        fprintf(stderr, "Nothing on stack to print.\n");
        return;
    }
    printf("Entire stack:\n");
    int i;
    for(i = stack->top; i >= 0; --i)
    {
        printf("%c\n", stack->mem[i]);
    }
}

Please provide feedback on whatever you see fit, however, error handling would be a helpful focus.

You could allow to initialize the stack locally.
Just by separating the bigger part of stack_create to separate function.

void stack_init(STACK* stack, char* mem, size_t size)
{
    stack->mem = mem;
    stack->size = size;
    stack->top = -1;
}

char stack_mem[STACK_SIZE];
STACK stack;
stack_init(&stack, stack_mem, STACK_SIZE);

As for error handling, stack usualy does not handle it. You should instead offer the "is empty?" and "is full?" functions. Consumers of the stack are then responsible for making sure stack is not empty before they peek or pop, and that it is not full before they push. Check for emptiness is often integral part of algorithms using the stack anyway. Check for fullness not so much, it Is mostly limitation of a fixed size stack. But the consumer must check it nevertheless or they must be sure that bigger stack is never needed.

In stack_create you should check if malloc returns null then dont assign to properties and return null as well. Consumers are again responsible for checking this situation.

One last point, the stack_create could be complemented with stack_destroy.


Fractal rendering fun time I wrote up a script a while back to let me play around with fractals. The idea was to have direct access to the script that creates the fractal. None of that close, edit, then run hassle; just edit then run.

renderscript.py contains the GUI:

import tkinter as tk
from tkinter.filedialog import askopenfilename, asksaveasfile

class View(tk.Frame):
    count = 0
    def __init__(self, *args, **kwargs):
        tk.Frame.__init__(self, *args, **kwargs)
        tk.Button(self, text="open", command=self.open).pack(fill=tk.X)
        tk.Button(self, text="save", command=self.save).pack(fill=tk.X)
        tk.Button(self, text="run program", command=self.draw).pack(fill=tk.X)
        self.txt = tk.Text(self, height=30)
        scr = tk.Scrollbar(self)
        scr.config(command=self.txt.yview)
        self.txt.config(yscrollcommand=scr.set)
        scr.pack(side="right", fill="y", expand=False)
        self.txt.pack(side="left", fill="both", expand=True)
        self.pack()
    def draw(self, size=500):
        exec(str(self.txt.get(1.0, tk.END)))
        self.pixels = [[(0, 0, 0) for y in range(size)] for x in range(size)]
        self.pixels = render(self.pixels)
        window = tk.Toplevel(self)
        window.resizable(0,0)
        canvas = tk.Canvas(window, width=size, height=size, bg='white')
        canvas.pack()
        img = tk.PhotoImage(width=size, height=size)
        canvas.create_image((size/2, size/2), image=img, state="normal")
        for y in range(size):
            for x in range(size):
                img.put(self.rgbtohex(self.pixels[x][y]), (x,y))
        window.mainloop()
    def rgbtohex(self, rgb):
        return ("#" + "{:02X}" * 3).format(*rgb)
    def open(self):
        self.txt.delete(1.0, tk.END)
        self.txt.insert(tk.END, open(askopenfilename()).read())
    def save(self):
        f = asksaveasfile(mode='w', defaultextension=".py")
        if f is None:
            return
        text2save = str(self.txt.get(1.0, tk.END))
        f.write(text2save)
        f.close()

if __name__ == "__main__":
    root = tk.Tk()
    root.resizable(0,0)
    main = View(root)
    root.mainloop()

fractal.py contains example fractal routines:

class Fractal:
    def mandelbrot(self, x, y, scale, center=(2.2, 1.5)):
        n = lambda c: self.iterate_mandelbrot(c)
        return self.calcolor(x, y, scale, center, n)

    def julia(self, x, y, scale, center=(1.5, 1.5)):
        n = lambda c: self.iterate_mandelbrot(complex(0.3, 0.6), c)
        return self.calcolor(x, y, scale, center, n)

    def calcolor(self, x, y, scale, center, nf):
        c = complex(x * scale - center[0], y * scale - center[1])
        n = nf(c)
        if n is None:
            v = 1
        else:
            v = n/100.0
        return v

    def iterate_mandelbrot(self, c, z = 0):
        for n in range(256):
            z = z*z +c
            if abs(z) > 2.0:
                return n
        return None

    def griderator(self, w, h):
        for x in range(w):
            for y in range(h):
                yield x, y

    def render(self, pixels):
        scale = 1.0/(len(pixels[0])/3)
        for x, y in self.griderator(len(pixels), len(pixels[0])):
            i = self.mandelbrot(x, y, scale) * 256
            r, g, b = int(i % 16 * 16), int(i % 8 * 32), int(i % 4 * 63)
            pixels[x][y] = (r, g, b)
        return pixels

global render
render = Fractal().render

The script does block while rendering the script. Try replacing mandelbrot with julia. I am looking for feedback on style and usability.

Some suggestions:

In calcator, I would use a ternary expression: return 1. if n is None else n/100.
calcator can be made even more efficient by having iterate_mandelbrot return 100. if the loop finished.  Then you just divide the result of that by 100.  This will result in 1. if the loop exits, avoiding the if test entirely.
I would only do run self.mandelbrot in your loop, and store the result of that function to a 2D numpy array.  Then you can vectorize the rest of the calculation, since it is all just basic math.  This should substantially increase performance.  You can even move the n/100 outside the for loop to further improve performance.
If the previous suggestion does not increase performance enough, you might be able use multiprocessing.Pool.imap to further increase the performance of the loop.
For an even more extreme vectorization, you can do all your calculations on all pixels at once.
Rather than having a griditor function, just use itertools.product.
Follow pep8
I would put the current contents of the if __name__ == "__main__": block in a main function and just call that function inside the if __name__ == "__main__": block.
In render I would allow the code to pass a string (which defaults to mandelbrot, and use getattr to dynamically call method with that name.
I would rename iterate_mandelbrot to iterate_pixel.
I would put in mandelbrot an argument for z that lets the user change z.  Similarly, I would put an argument in julia that lets the user change (0.3, 0.6) to something else.
render should accept *args, **kwargs that are then passed directly to the mandelbrot or julia method.
I would move the julia and mandelbrot lambdas into their own methods.  Or better yet, I would refactor so you just pass the c and z argument.
In render, I would let the user set the scale with an argument.  The scale argument would default to None. If it is None, it would be computed automatically as is done now.
In render, you only ever work with integers.  So I would use n//100 in calcator to make sure it returns an integer.  This allows you to avoid the later integer conversions.

So here is my version of Fractal

import numpy as np

class Fractal:
    def mandelbrot(self, pixels, scale, center=(2.2, 1.5), z=0.):
        return self.calcolor(pixels, scale, center, zs=z)

    def julia(self, pixels, scale, center=(1.5, 1.5), c=(0.3, 0.6)):
        if not hasattr(c, 'imag'):
            c = complex(*c)
        return self.calcolor(pixels, scale, center, cs=c)

    def calcolor(self, pixels, scale, center, cs=None, zs=None):
        pixels = pixels.asarray(pixels)

        xpixels = np.arange(pixels.shape[0])[None, :]
        ypixels = np.arange(pixels.shape[1])[:, None]

        val = (xpixels+ypixels*1j)*scale-complex(*center)
        if cs is None and zs is None:
           raise ValueError('Either cs or zs must be specified')
        if cs is None:
           cs = val
        if zs is None:
           zs = val
        ns = np.full_like(val, 100, dtype='int16')

        for n in range(256):
            zs = zs**2 +cs
            ns[ns>0 & np.abs(zs)>2.0] = n
        return ns

    def calc_pixels(self, cs, zs):

    def render(self, pixels, scale=None, method='mandelbrot', *args, **kwargs):
        if scale is None:
            scale = 1.0/(len(pixels[0])/3)

        try:
            func = getattr(self, method)
        except AttributeError:
            raise ValueError('Unknown method %s' % method)
        i = func(pixels, scale, *args, **kwargs)*256

        r = i%16 * 16
        g = i*8 * 32
        b = i%4 * 63

        return np.dstack([r, g, b])


Implementation of AES using CUDA I am trying to implement AES on GPU using CUDA programming. I use 4 TBoxes in my implementation that requires 4kB of GPU Memory. I have used a 1KB array for 1KB plaintext. first all of plaintext would be copied to GPU memory, then encryption would be started using

cudaMemcpyToSymbolAsync(DEV_message, H_Message, 1024, 0, cudaMemcpyHostToDevice);

Then, the global kernel will run:

AESROUND<<< 8, 16, 16 >>>(1024);

AESROUND() performs one round of AES algorithm on a 1024-bit state, then 32 bytes of the array will XOR with 32 bits of the state.

__global__ void AESROUND_AD(const int SIZE)
{
    __shared__ unsigned char dev_rkey[16];
    __shared__ unsigned char dev_sh_state[16];

    int tid = blockIdx.x * 16 + threadIdx.x;
    if (tid < 128)
    {
        for (long long i = 0; i <SIZE/32; i++)
        {
            dev_sh_state[threadIdx.x] = dev_state[(tid + 112) % 128];
            dev_rkey[threadIdx.x] = dev_state[tid];
            __syncthreads();

            if (threadIdx.x < 4)
            {
                U32 v1 = ((U32*)dev_TE0)[*(dev_sh_state + threadIdx.x * 4)];
                U32 v2 = ((U32*)dev_TE1)[*(dev_sh_state + ((threadIdx.x * 4 + 5) % 16))];
                U32 v3 = ((U32*)dev_TE2)[*(dev_sh_state + ((threadIdx.x * 4 + 10) % 16))];
                U32 v4 = ((U32*)dev_TE3)[*(dev_sh_state + ((threadIdx.x * 4 + 15) % 16))];

                ((U32*)dev_sh_state)[threadIdx.x] = v1 ^ v2 ^ v3 ^ v4 ^ ((U32*)dev_rkey)[threadIdx.x];
            }
            __syncthreads();

            dev_state[tid] = dev_sh_state[threadIdx.x];
            __syncthreads();

        }
    }
}

There is no problem in code or in my program, but the big problem is its speed: just 1 megabyte per second. I don't know why this program is so slow.

I use Microsoft Visual C++ in Windows 8.1 with CUDA Toolkit 7.
My GPU is a GeForce GT 720M

Compute capability: 2.1
Number of SMs: 2
Graphic clock (MHz): 625
Processor clock (MHz): 1250
Memory clock (MHz): 800

My processor is Intel Core(TM) i5-3337U CPU @ 1.80GHz (4 CPUs) microprocessor.

Source code of the algorithm can be found here.

There are a few issues that I see in your code. I'm not sure if all matter, but I'll try to list them anyway.

You said you call AESROUND<<< 8, 16, 16 >>>(1024);, but you present __global__ void AESROUND_AD(const int SIZE). Are AESROUND() and AESROUND_AD() the same kernels? Is that a typo?
Calling AESROUND<<< 8, 16, 16 >>>(1024); means (amongst other) that you dynamically allocate 16 (third parameter of the <<< >>>) elements of a shared memory segment of type to-be-defined inside the kernel (declared extern __shared__ whatever mySegment[];). So you allocate that segment in your kernel call, but do not actually declare in the kernel body. Yet another typo?
In the kernel itself, you reference dev_state which isn't declared anywhere. Is that a global variable? Shouldn't that be better put a input parameter of the kernel?
The computation of tid explicitly assumes that the block size is 16, which is true here. However, if this changes when calling the kernel, then the formula would become false. So if this isn't supposed to change, then use a macro for the block size (such as #define BLOCKSIZE 16) and use it in your definition of tid, and the call to AESROUND(). But if this size might change in the call of the kernel, then use tid = blockIdx.x * blockDim.x + threadIdx.x;. And as a more general comment, try to avoid "magic numbers" in your code, or at least put them into macros or const variables, so that you give them a sensible name and therefore a meaning.

Now, the initial question was about performance:
Well, I guess that this all boils down to one simple issue: you do not maximise neither your core occupancy, nor your memory bandwidth. Indeed, you only request 16 threads per block, which isn't even enough for a warp. Therefore, irrespective of your actual GPU architecture, at any moment, at most only half of your cores are utilised. Moreover, once the initial data loaded in shared memory, you only compute using 4 threads, so only utilising 1/4 of the available cores at most.

Considering what I can see (and sort of understand) of your kernel, I have the feeling that you'd better be using one single block of 128 threads per message to compute, and to load all of it at once in your shared memory. Then to compute as you do in your kernel, but with now 4 times more threads involved. This would have the double benefit of permitting you to avoid having to store you partial results in global memory at each round, and to increase your occupancy.

And to fully utilise your GPU, you would need to compute several messages in parallel, one per block.


Reacting to moods with icons This is a class in my React app. This code works and displays an appropriate icon and question for every description.

class myQuestion extends PureComponent {
  render() {
    const { description } = this.props.question;
    let icon = icon;
    let question = question;
    if (description === 'Happy') {
      icon = <FontAwesomeIcon icon={faSmile} />;
      question = 'You answered happy. Why?';
    } else if (description === 'OK') {
      icon = <FontAwesomeIcon icon={faMeh} />;
      question = 'You answered OK. Why?';
    } else if (description === 'Angry') {
      icon = <FontAwesomeIcon icon={faAngry} />;
      question = 'You answered angry. Why?';
    } else {
      icon = '';
      question = '';
    }
    return (
      <div>
        <Question
          icon={icon}
          description={description}
          question={question}
        />
      </div>
    );
  }
}

I'd like to know how would be a better way to write this if else statement, preferably removing the let.

'description' is a prop of the question, but 'icon' and 'question' are dependent on what the 'description' is. The 'description' comes from the ID of a previous answer to a question.

Is there a way I could put this logic before the render function?

In cases where your value is determined by a string key, a key-value mapping is often the way to go. In this case, we store your icon JSX and questions in objects keyed by description. Then you simply access them using the current description as key.

const icons = {
  Happy: <FontAwesomeIcon icon={faSmile} />,
  OK: <FontAwesomeIcon icon={faMeh} />,
  Angry: <FontAwesomeIcon icon={faAngry} />
}

const questions = {
  Happy: 'You answered happy. Why?',
  OK: 'You answered OK. Why?',
  Angry: 'You answered angry. Why?'
}

class myQuestion extends PureComponent {
  render() {
    const { description } = this.props.question
    const icon = icons[description] || ''
    const question = questions[description] || ''

    return (
      <div>
        <Question
          icon={icon}
          description={description}
          question={question}
        />
      </div>
    )
  }
}


Remove the occurences of an element from array if it occurs more than n times I wanted to remove the occurrence of an element from an array if it occurs more than n times. For example:

{ 20, 37, 20, 21 } (n->1) -> { 20, 37, 21}
{ 1, 1, 1, 1, 1 }  (n->5) -> { 1, 1, 1, 1, 1 }
{ 1, 2, 3, 1, 1, 2, 1, 2, 3, 3, 2, 4, 5, 3, 1 } (n->3 ) -> { 1, 2, 3, 1, 1, 2, 2, 3, 3, 4, 5 }

I tried the following code in java:

public class arr{
    public static void main(String[] args){
        int a[] = {1,1,3,3,7,2,2,2,2}; //Array of the elements
        int b =3; // maximum occurrences
        int u=1;  // Counter for same number of elements
        int c[] = new int[nterms(a, b)]; //initializing new array
        int k=0;
        int l=0;
        for(int i =a.length-1; i>=0; i--){
            k=0;
            // Checking the duplicate elements.
            for(int j=i-1; j>=0; j--){
                if(a[i]==a[j]){
                    k++;
                }
            }
            if(k!=b){
                c[c.length-u] = a[i];
                u++;
            }
        }
        for(int i =0; i<c.length; i++)
            System.out.println(c[i]);
    }

    // Returns no. of unique elements.
    public static int nterms(int[] x, int y){
        int k=0;
        int l=0;
        for(int i =0; i<x.length; i++){
            k=0;
            for(int j=i+1; j<x.length; j++){
                if(x[i]==x[j]){
                    k++;
                }
            }
            if(k==y){
                l++;
            }
        }
        return (x.length-l);
    }
}

It works fine but can anybody recommend a more short & efficient way.

It's been said multiple times but you should avoid one letter variable most of the time.

Class name arr is also no good. It's recommended that class name follows camel case convention and using abbreviation is often considered bad.

Don't code something in your main as it makes it harder to test your code.
Putting it in an outside function makes for a clearer code :

public static int[] removeOccurencesFromArray(final int[] array, final long maxOccurence) {
    // TODO
}

Array have an "old-school" feel and I'd avoid them if possible. Consider using List (and especially ArrayList) for those kind of use case, thus transforming previous method in :

public static List<Integer> removeOccurencesFromList(final List<Integer> list, final long maxOccurence) {
    // TODO
}

Now, onto the main course :

I find your algorithm overly complex. :/
It's also higly "procedural" IMO, in modern computing we'd rather "tell" the program to do something instead of manipulating everything by ourselves.

Julien Rousé has shown a nice and short solution in another answer... which is probably faster than mine but please bear with me :)

(After rereading my answer, I see that I used some Java 8 tricks that may feel overwhelming if you are a beginner, if you are new to programming I'd recommend to not consider my solution)

Let's try to solve your problem step by step :

Here is a simple solution I came up with (almost the same as Julien Rousé)
1) count the number of occurences within your array... you end up with a structure linking all elements with the number of occurence... it's usually called a bag, while there are some java library that supports this structure let's keep things simple and use a Map
2) if any occurences count is greater than the maxOccurence parameter then it should be set to maxOccurence
3) for each elements in your array, print it if the number of occurences is greater than 0 and reduce the number of elements in your bag

Let's dig a bit in java 8... surely there's a tool for us in there.
We are looking for something that'd allow us to "tell" instead of creating everything by hand.

In the Collectors util class (https://docs.oracle.com/javase/8/docs/api/java/util/stream/Collectors.html), we can find some really fancy tools. For our use case, groupingBy and counting sounds useful.

groupingBy allows you to transform a stream of data into a map where the key is found using the first parameter.
counting counts the number of input elements (from Javadoc).

list.stream().collect(Collectors.groupingBy(element -> element, Collectors.counting()));

With this simple line, we now have the number of occurence of each elements of your list.

This is still not enough as we want to consider the maximum number of occurences.

We want to have the number of occurence if it's below a threshold.
Collectors (again) comes to the rescue :

list.stream().collect(Collectors.groupingBy(element -> element,
            Collectors.collectingAndThen(Collectors.counting(), v -> Math.min(v, maxOccurence))));

So we are counting unless we have more than maxOccurence, in which case the min will always give us maxOccurence as a result.
That's not very palatable though IMO, I'll use static import to make it cleaner. Let's also not forget to store the result.

    Map<Integer, Long> value = list.stream().collect(groupingBy(element -> element,
            collectingAndThen(counting(), v -> Math.min(v, maxOccurence))));

We can now easily compute the result :

    List<Integer> result = new ArrayList<>();
    for (Integer element : list) {
        long remaining = value.get(element);
        if (remaining > 0) {
            value.put(element, remaining - 1);
            result.add(element);
        }
    }

    return result;

If you really want to have an array at the end, use the toArray method ;)


Tic Tac Toe with SOLID principles I am making a Tic Tac Toe game to be played in the console either against another human or a simple AI. It is a little rough but it works. What concerns me is how well it adheres to SOLID and other design principles as this is my first time using them in practice. I would appreciate a review of how my classes are structured and any advice. I am sharing a link from GitHub because there is too much code to post everything here.

These are some of the largest classes and give me an uneasy feeling.

GameBoard.class - has the game board and holds players' markers

package board;

import java.util.ArrayList;

public class GameBoard {

    private Cell[] board;

    private ArrayList<Integer> availableSpots;

    public GameBoard(int n) {
        availableSpots = new ArrayList<Integer>();

        //the game board
        board = new Cell[n];
        for (int i = 0; i < n; i++) {
            board[i] = new Cell();
        }

        //list of initial open cells
        for (int i = 0; i < 9; i++) {
            availableSpots.add(i);
        }
    }

    public Cell[] getBoard() {
        return board;
    }

    public void addMarker(int n, String s) {
        if (availableSpots.contains(n)) {
            board[n].setMarker(s);
            removeSpotFromList(n);
            //System.out.println(availableSpots.size());
        }
        else {
                System.out.println("Cell not available");

            }
    }

    public String getCellValue(int n) {
        return board[n].getMarker();
    }

    public ArrayList<Integer> getAvailableSpots() {
        return availableSpots;
    }

    //remove number from arraylist
    public void removeSpotFromList(int n) {
        availableSpots.set(n, null);
        availableSpots.trimToSize();
        for (int i=0; i<availableSpots.size(); i++) {
            //System.out.println(availableSpots.get(i) + ", ");
        }
    }

}

Game.class - switches between players and calls each turn

import board.GameBoard;
import board.PrintBoard;
import consuleInput.InputReader;
import import players.Playable;
import players.PlayerGroup;

public class Game {

    public static void play() {
        Modes modes =  new Modes();
        InputReader ir = new InputReader();
        GameBoard bg = new GameBoard(9);
        PrintBoard pb = new PrintBoard();
        PlayerGroup pg = new PlayerGroup();
        GameStateChecker gsc = new GameStateChecker();
        System.out.println("choose a mode:");
        System.out.println("1. Player vs player");
        System.out.println("2. player v computer");
        System.out.println("3. computer vs computer");

        int mode =  ir.toIntegerReader("type below");

        modes.chooseMode(mode, pg);

        pb.print(bg);

        int counter = 9;
        Playable currentPlayer;
        boolean hasWinner = false;
        int player = 0;
        //int choice;
        do{
            currentPlayer = pg.getPlayer(player);
            //int choice = ir.toIntegerReader("pick a square (0-8):");
            bg.addMarker(currentPlayer.playerMove(bg.getAvailableSpots()), currentPlayer.getMarker());
            pb.print(bg);
            hasWinner = gsc.checkPatterns(bg, currentPlayer);
            //System.out.println("is there a winner " + hasWinner);
            counter--;
            if (player == 0) {
                player = 1;
            }
            else {
                player = 0;
            }
            /*
            currentPlayer = pg.getPlayer(1);
            choice = ir.toIntegerReader("pick a square (0-8):");
            bg.addMarker(choice, currentPlayer.getMarker());
            pb.print(bg);
            hasWinner = gsc.checkPatterns(bg, currentPlayer);
            counter--;
            */
        }
        while (!hasWinner);

    }
}

Aside from RobAu's comment, I think the addMarker method can be simplified into:

board[n].setMarker(s);

By delegating the checking the availability of the cell to the Cell class. I think it would be best to define your own exception extending from IllegalArgumentException.

public void setMarker(String marker) {
    if(!marker.equals(" "))
        throw new CellIsNotAvailableException();
    this.marker = marker;
}

Another thing I want to point out is the way you name your methods and variables. One should always name their methods and variables properly such that it won't be neccessary to add a comment explaining what these things do (It's fine to write comments explaining why they exist though). As much as possible, Express yourself in code! Take addMarker for example. I think addMarker would be better written as markCellWithMarker(int cellNumber, String marker) (I consider X's and O's as tokens to be used in the game), and getCellValue as getMarkerAt(int cellNumber).

Applying these changes, here's my refactored version of the GameBoard Class:

public class GameBoard {

    private Cell[] board;

    public GameBoard(int numberOfCells) {
        board = new Cell[numberOfCells];
        for (int i = 0; i < numberOfCells; i++) {
            board[i] = new Cell();
        }
    }

    public void markCellWithMarker(int cellNumber, String token) {
        board[cellNumber].setMarker(token);
    }

    public String getMarkerAt(int cellNumber) {
        return board[cellNumber].getMarker();
    }
}

As for the Game class, it doesn't really justify its existence as it is right now. It contains only one method that is supposed to run a game (if I read your code right). Refactoring it into this might justify its existence:

public class Game {
    private GameBoard gameBoard;
    private Player[] players;
    private int turnNumber;

    //In case you want a two player game.
    public Game(int numberOfCells, Player player1, player player2){
        gameBoard = new GameBoard(numberOfCells);
        setFirstTurnPlayer(player1, player2);
    }

    //In case you want a single player game against the computer.
    public Game(int numberOfCells, Player player){
        gameBoard = new GameBoard(numberOfCells);
        Player computer = new Player("computer");
        setFirstTurnPlayer(player1, computer);
    }

    private void setFirstTurnPlayer(Player player1, Player player2){
        //It's up to you how you decide the turnorder: die roll, coin toss or something.
        //The first one in the player array is considered as the turn player.
    }

    public String getTurnPlayer(){
        return players[turnNumber%2].getName();
    }

    public void markCellWithMarker(int cellNumber){
        String markerToPlace = players[turnNumber%2].getPlayerMarker()
        gameBoard.markCellWithMarker(cellNumber, markerToPlace)
        checkIfTurnPlayerWinsTheGame()
        turnNumber++;
    }

    //GameStateChecker code moved here. I think this one can be made shorter and more readable.
    public void checkIfTurnPlayerWinsTheGame(){
        //Rename currentPlayer as turnPlayer
        Player currentPlayer = players[turnNumber%2]
        if ((board[0].getMarker().equals(currentPlayer.getMarker()) && board[1].getMarker().equals(currentPlayer.getMarker()) && board[2].getMarker().equals(currentPlayer.getMarker())) ||
            (board[3].getMarker().equals(currentPlayer.getMarker()) && board[4].getMarker().equals(currentPlayer.getMarker()) && board[5].getMarker().equals(currentPlayer.getMarker())) ||
            (board[6].getMarker().equals(currentPlayer.getMarker()) && board[7].getMarker().equals(currentPlayer.getMarker()) && board[8].getMarker().equals(currentPlayer.getMarker())) ||
            (board[0].getMarker().equals(currentPlayer.getMarker()) && board[3].getMarker().equals(currentPlayer.getMarker()) && board[6].getMarker().equals(currentPlayer.getMarker())) ||
            (board[1].getMarker().equals(currentPlayer.getMarker()) && board[4].getMarker().equals(currentPlayer.getMarker()) && board[5].getMarker().equals(currentPlayer.getMarker())) ||
            (board[2].getMarker().equals(currentPlayer.getMarker()) && board[5].getMarker().equals(currentPlayer.getMarker()) && board[8].getMarker().equals(currentPlayer.getMarker())) ||
            (board[0].getMarker().equals(currentPlayer.getMarker()) && board[4].getMarker().equals(currentPlayer.getMarker()) && board[8].getMarker().equals(currentPlayer.getMarker())) ||
            (board[2].getMarker().equals(currentPlayer.getMarker()) && board[4].getMarker().equals(currentPlayer.getMarker()) && board[6].getMarker().equals(currentPlayer.getMarker())))
        throw new GameIsOverException(currentPlayer.getName() + " wins!")
}

Here's the code I wrote for a game called ConnectFour which I think is a bit similar to tic-tac-toe. This may help you see some of the things I mentioned here on proper naming and stuff.


Getting the lengths of each word in its String parameter Is this a smart method?

public static ArrayList<Integer> getWordLengths( String s )
{

    String str = " " + s + " ";

    ArrayList<Integer> list = new ArrayList<Integer>();

    for ( int i = 0; i < str.length(); i++ )
    {
        if ( str.charAt(i) == ' ' )
        {
            for ( int j = i+1; j < str.length() ; j++)
            {
                if ( str.charAt(j) == ' ')
                {
                    list.add( j - i - 1 );
                    j = str.length();
                }
            }
        }
    }
    return list;
}

Sample output:

System.out.println( getWordLengths("This is really easy"));

[4, 2, 6, 4]

so it worked.

We are only allowed to use length and charAt from the String class.

//only allowed to use length and charAt from string class
public static ArrayList<Integer> getWordLengths( String s )
{
    String str = s + " ";

    ArrayList<Integer> list = new ArrayList<Integer>();

    int count = 0;

    for ( int i = 0; i < str.length(); i++ )
    {
        if ( str.charAt(i) == ' ' )
        {
            list.add(count);
            count = 0;
        }
        else
        {
            count++;
        }
    }
    return list;
}

Output:

[4, 2, 6, 4]

As long as the length and charAt limitations only apply to string methods and not to use of a List this should be fine.  Just because you need to count more than one thing doesn't mean you need more than one loop.


Recursive implementation of palindrome string checker I've tried to refactor my program to single return statement at the end of the program, however, it ruins the end statements of the recursion because I want to return from function in specific line and not in the end of the function.

I'd like suggestions on how this code can work and be elegantly designed to comply with

Multiple return statements
Single return statement

private static boolean isPalindrome(String pal)
{
        if(pal.length() == 1 || pal.length() == 2 )
        {
            if(pal.length()==1)
            {
                return true;
            }

            else
            {
                if(pal.charAt(0) == pal.charAt(1))
                {
                    return true;
                }
                return false;
            }
        }

        else
        {
            if(pal.charAt(0) == pal.charAt(pal.length()-1))
            {
                return isPalidrome(pal.substring(1, pal.length()-1));
            }
            return false;
        }
    }
}

For all practical purposes, recursion is a bad idea for this problem, mainly because String.substring() is an expensive operation, especially since Java 7.  Let's assume that you're doing this just as a fun educational exercise.

Single return statements are overrated.  There's no practical advantage to forcing your code to have just one return for its own sake.

Your base cases are poorly chosen.  A zero-length string will cause a crash.  If you handle empty strings as a base case, then you no longer need to treat two-character strings as a special case.

You take the string length often enough that it's worth assigning it to a variable.

Overall, the function could be written more compactly.

public static boolean isPalindrome(String s) {
    int len = s.length();
    if (len <= 1) {
        return true;
    }
    return (s.charAt(0) == s.charAt(len - 1)) &&
           isPalindrome(s.substring(1, len - 1));
}


Java - Functional interface for merging two lists Hello dear colleagues.
As an exercise for Java streams, I've written simple program, which scrapes links to references from a news portal. Basically, I wanted to find out, which portals are the most referenced.
Right now, I ended up with the following piece of code:
final Map<String, List<URL>> hostToURLs = Analyzer.mapByHost(ReferencesStore.read());
// E.g. bbc.com -> [ https://www.bbc.com/article1, https://www.bbc.com/article2, etc ]

// The following creates LinkedHashMap sorted by the number of URLs
final LinkedHashMap<String, List<URL>> sortedHostToURLs = hostToURLs
                .entrySet()
                .stream()
                .sorted(Map.Entry.comparingByValue(Comparator.comparingInt(List::size)))
                .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue, (m, n) -> { m.addAll(n); return m; }, LinkedHashMap::new));

I'm interested, if the the ugly merge lambda
(m,n)-> {m.addAll(n); return m;}
in the collect method can be replaced with some method reference from  standard library. I wasn't able to find anything useful in the  documentation, nor was I able to ask google the right question to find out if such merge function exists. Thank you in advance.

The thing is that the merge function is never called in your case, because all you are doing is reordering an existing map, so IMO you don't really need to worry about it.
Personally in such cases I just use (m, n) -> m, because it's short and doesn't distract.
Another variant would be to use a function that throws an exception when called. The JDK does this itself when you use the toMap overload that doesn't take a merge function:
private static <T> BinaryOperator<T> throwingMerger() {
  return (u,v) -> { throw new IllegalStateException(String.format("Duplicate key %s", u)); };
}

Another thing you may want to consider it not to use a (Linked)HashMap at all here, but simply collect the Entrys into a list. If you need to lookup an entry by key later, then just keep the original map around.

EDIT: In cases where the merge function is used, your implemention is fine. You may want to just put it in a variable, so that its name can describe its function.
One thing you could consider is using a function that creates a new list instead of "reusing" one of the existing lists. That would reflect the functional style of the code better, where one normally uses immutable data structures.
The basic problem is that Java's List interface isn't intended for functional/immutable situations and doesn't have a simple method to concatinate two lists.
If you are open to additional libraries you'd have more options. For example, Apache's Commons has a union method.


Matching parentheses I have a string that contains a random order of parenthesis {[()]}. I want to check if for any given parenthesis is there a matching closing one.

Example:

 }}}{{{ //true
 {[]    //false
 {[()]} //true

private static bool checkIfWellFormatted(string inputString)
{

    if (string.IsNullOrEmpty(inputString))
        throw new ArgumentException("String is empty");
    if ((inputString.Length % 2) != 0)
        return false;

    Dictionary<char, int> _inputDictionary = new Dictionary<char, int>();

    foreach (Char c in inputString)
    {
        if (!_inputDictionary.ContainsKey(c))
            _inputDictionary.Add(c, 0);
        else
            _inputDictionary[c] += 1;

    }

    foreach (var item in _inputDictionary)
    {
        char oppKey = '\0';
        if (item.Key == '{')
            oppKey = '}';
        if (item.Key == '}')
            oppKey = '{';

        if (item.Key == '(')
            oppKey = ')';
        if (item.Key == ')')
            oppKey = '(';

        if (item.Key == '[')
            oppKey = ']';
        if (item.Key == ']')
            oppKey = '[';

        if (_inputDictionary.ContainsKey(oppKey))
        {
            var value = _inputDictionary[oppKey];
            if (value != item.Value)
                return false;

        }
        else
            return false;

    }

    return true;
}

Here, the second iteration over the dictionary has \$O(n)\$ complexity. Can I improve its time complexity?

You could evict the count dictionary altogether and use three counters. Next, you scan each character and increment/decrement the corresponding counter. The answer is true if after scanning all the characters all three counters are zero. For example, if we scan [, we increment the counter for [ and ]; if we scan ], we decrement that very same counter. (Before the scan all counters are set to zero.)

All in all, I had this in mind:

private static bool WellFormatted(string inputString)
{
    int braceCount = 0;         // Counts '{' and '}'.
    int parenthesisCount = 0;   // Counts '(' and ')'.
    int squareBracketCount = 0; // Counts '[' and ']'.

    foreach (char c in inputString)
    {
        switch (c)
        {
            case '{':
                braceCount++;
                break;

            case '}':
                braceCount--;
                break;

            case '(':
                parenthesisCount++;
                break;

            case ')':
                parenthesisCount--;
                break;

            case '[':
                squareBracketCount++;
                break;

            case ']':
                squareBracketCount--;
                break;
        }
    }

    return braceCount == 0
        && parenthesisCount == 0
        && squareBracketCount == 0;
}

Also, from mathematical perspective, I would accept the empty string as a valid "parenthezation."


String manipulations: transform "a-b-c" into "a(b(c))" function dashesToParentheses(str) {
    var list = str.split('-');
    return str.replace(/-/g, '(') + repeatString(')', list.length - 1);
}

function repeatString(str, times) {
    if (times == 1)
        return str;

    return new Array(times + 1).join(str);
}

dashesToParentheses('a-b-c') // "a(b(c))"
dashesToParentheses('a-b') // "a(b)"
dashesToParentheses('a') // "a"
dashesToParentheses('') // ""

dashesToParentheses works correct. Can I make it simpler or/and faster?

Having split the string you can join it with brackets instead of replacing them.  You could optionally choose to remove the repeatString function and your +/- 1, but it does make a lot of sense the way you have it.

function dashesToParentheses(str) {
    var list = str.split('-');
    return list.join('(') + Array(list.length).join(')');
}


NUMAH LITTERS OV KITTEHS ON TEH NETZ It's not fair that 1% of the users hold 75% of the lolcode questions. #OccupyMatsMug
  ~ user2296177

I agree. Without further ado, here's the LOLCODE version of 99 bottles of beer on the wall:

OBTW
    PRINT TEH LOLCODE VERSHUN OV N BOTTLEZ OV BER ON TEH WALL.
TLDR

HAI 1.3

  HOW IZ I COUNTDOWN YR NUMAH
    I HAS A NAUW ITZ A NUMBR
    I HAS A LEZZ ITZ A NUMBR
    IM IN YR LOOPZ NERFIN YR COWNTR TIL BOTH SAEM COWNTR AN DIFF OF 1 AN NUMAH
      NAUW R SUM OF NUMAH AN COWNTR
      LEZZ R DIFF OF NAUW AN 1
      VISIBLE SMOOSH NAUW AN " LITTERS OV KITTEHS ON TEH NETZ" MKAY
      VISIBLE SMOOSH NAUW AN " LITTERS OV KITTEHS" MKAY
      VISIBLE "WAN FALLS DOWN AN BREAKZ PAH"
      VISIBLE SMOOSH LEZZ AN " LITTERS OV KITTEHS ON TEH NETZ" MKAY
      VISIBLE ""
    IM OUTTA YR LOOPZ

    VISIBLE "1 LITTER OV KITTEHS ON TEH NETZ"
    VISIBLE "1 LITTER OV KITTEHS"
    VISIBLE "WAN FALLS DOWN AN BREAKZ PAH"
    VISIBLE "NO MOAR LITTERs OV KITTEHS ON TEH NETZ"
  IF U SAY SO

  I IZ COUNTDOWN YR 99 MKAY

KTHXBYE

Disclaimer: No actual kittens were harmed while writing this program.

The number of iterations is held in the variable NUMAH. I know NUMBR is a more fitting name, but that's already claimed as a type.

The above is compiled using the latest lci using the 1.3 specification (thanks to Pimgd for the link). Since the 1.3 specification doesn't contain every feature of the language, see the 1.2 specification as well.

I don't think LOLCODE has templates or the likes, so getting all the output on screen is a bit tedious.

Output:

99 LITTERS OV KITTEHS ON TEH NETZ
99 LITTERS OV KITTEHS
WAN FALLS DOWN AN BREAKZ PAH
98 LITTERS OV KITTEHS ON TEH NETZ

etc.

1 LITTER OV KITTEHS ON TEH NETZ
1 LITTER OV KITTEHS
WAN FALLS DOWN AN BREAKZ PAH
NO MOAR LITTERs OV KITTEHS ON TEH NETZ

Is this idiomatic LOLCODE?

Variable capitalization

IT IZ PRETTY GUD LOLCODE
HOWEVEZ ME THINKS THAT TEH LOLCODEZ R EZYR 2 READ IF TEH VARZ R lowercase
ZIS BECUZ ALL CAPS IS HARD TOO READ
(all spelling "mistakes" made by my cat, who assisted me in that part of this review)

More seriously, it is an issue that there are no good LOLCODE syntax highlighters. To be "stylish" and keep everything uppercase is a choice you can make, but personally I prefer to use lowercase or camelCase variable names whilst keeping the language constructs all caps. Similar to SQL, this allows you to see what parts of the code are variables, and what parts are language constructs.

Compare:

I HAS A NAUW ITZ A NUMBR
I HAS A LEZZ ITZ A NUMBR
IM IN YR LOOPZ NERFIN YR COWNTR TIL BOTH SAEM COWNTR AN DIFF OF 1 AN NUMAH
  NAUW R SUM OF NUMAH AN COWNTR
  LEZZ R DIFF OF NAUW AN 1
  VISIBLE SMOOSH NAUW AN " LITTERS OV KITTEHS ON TEH NETZ" MKAY
  VISIBLE SMOOSH NAUW AN " LITTERS OV KITTEHS" MKAY
  VISIBLE "WAN FALLS DOWN AN BREAKZ PAH"
  VISIBLE SMOOSH LEZZ AN " LITTERS OV KITTEHS ON TEH NETZ" MKAY
  VISIBLE ""
IM OUTTA YR LOOPZ

with...

I HAS A nauw ITZ A NUMBR
I HAS A lezz ITZ A NUMBR
IM IN YR LOOPZ NERFIN YR cowntr TIL BOTH SAEM cowntr AN DIFF OF 1 AN numah
  nauw R SUM OF numah AN cowntr
  lezz R DIFF OF nauw AN 1
  VISIBLE SMOOSH nauw AN " LITTERS OV KITTEHS ON TEH NETZ" MKAY
  VISIBLE SMOOSH nauw AN " LITTERS OV KITTEHS" MKAY
  VISIBLE "WAN FALLS DOWN AN BREAKZ PAH"
  VISIBLE SMOOSH lezz AN " LITTERS OV KITTEHS ON TEH NETZ" MKAY
  VISIBLE ""
IM OUTTA YR LOOPZ

Variable naming

Regarding NUMAH, have you considered using NUMBAH? Similar meaning, but uses a more common mispronunciation.

Working around the spec

IM IN YR LOOPZ NERFIN YR COWNTR TIL BOTH SAEM COWNTR AN DIFF OF 1 AN NUMAH
  NAUW R SUM OF NUMAH AN COWNTR

This construct is non-obvious. You had to work around the spec to do something that you wanted to do, and I think it warrants explanation. Add a comment via BTW:

IM IN YR LOOPZ NERFIN YR COWNTR TIL BOTH SAEM COWNTR AN DIFF OF 1 AN NUMAH
  NAUW R SUM OF NUMAH AN COWNTR BTW workaround loop counters starting at 0

Correctness in corner cases

Your function doesn't check if the value passed in is greater than 0. This means that it falsely forces a KITTEH to break their paw when it wasn't necessary.

Add a guard clause at the top of the function; you can do an early return via GTFO.

DIFFRINT numbah AN BIGGR OF numbah AN 0
O RLY? YA RLY
    GTFO
OIC

Program definition

OBTW
    PRINT TEH LOLCODE VERSHUN OV N BOTTLEZ OV BER ON TEH WALL.
TLDR

HAI 1.3

Aside from the weird phrasing (does this code print the lolcode version of/and bottles of beer on the wall?), there's something else wrong here: Your comment is before the HAI 1.3. So any interpreter loading your file might not know what version you are using. Put your version number at the top.

Possible alternative dialects

VISIBLE "NO MOAR LITTERs OV KITTEHS ON TEH NETZ"

Did you make a typo here, is this an alternative kitty dialect that uses lower case 's' for certain plurals? I'm not sure.

Lastly, I think this version of 99 LITTERS OV KITTEHS ON TEH NETZ is not authentic, as anyone knows that when KITTEHS fall, they'll be alright. This version is sad, as all the KITTEHS just break their paws. Proper LOLCODE uses cats, it doesn't abuse them.


Ugly optimized caching of SimpleDateFormat Concerning the optimization of this simple function

String formatDate(String format, Locale locale, Date date) {...}

formatting a Date using a SimpleDateFormat created with the arguments format and locale. I'm curious  what you thing about the code below. I know it's terrible, but let's assume two things

The code must be as fast as possible.
There's no suitable object to hold the cache.
It must not cause any memory leaks.

Because of 1., a thread local cache should be used. However, the passed format and arguments may differ from those used in the cache and this must be checked. Funnily, although SimpleDateFormat is mutable (which is the cause of all problems), there seem to be no way how to change its format or locale. Actually, the parameters hardly ever change, but there's no guarantee.

Because of 2., the cache must be a static field.

Because of 3., no ThreadLocal.initialValue can be used as it'd lead to memory leaks just like in this question. For the same reason no custom class may be put into the ThreadLocal.

This all is pretty restrictive. So I wrote the following ugliness

private String toString(Date obj, String format, Locale locale) {
    return getSimpleDateFormat(format, locale).format(obj);
}

private SimpleDateFormat getSimpleDateFormat(String format, Locale locale) {
    Object[] list = SDF.get();
    // An identity check is faster and correct as missing an entry is allowed.
    // It's surely good enough as passing equals but not same parameters hardly ever happens.
    if (list!=null && list[0] == format && list[1] == locale) {
        return (SimpleDateFormat) list[2];
    }
    final SimpleDateFormat result = new SimpleDateFormat(format, locale);
    list = new Object[] {format, locale, result};
    SDF.set(list);
    return result;
}

/** Contains triples (String format, Locale locale, SimpleDateFormat sdf) */
private static final ThreadLocal<Object[]> SDF = new ThreadLocal<Object[]>();

I don't care about naming and such, as this was just a quick implementation of an idea. I'm curious if there's a substantially better solution satisfying all the above conditions.

If you say, it's premature optimization, I'll fully agree, but let's assume for a while it must be done. If you say, that's hardly faster than no caching, then show me your figures.

Obviously, I'm caching just one value per thread. It should be enough in many cases like producing a CSV with one one or more dates per row as I haven't yet see them to be formatted differently.

However, time is "date" too (at least in Java), and a CSV line containing both date and time in separate columns is rather common and this would render my poor man's cache fully ineffective.

What about using a WeakHashMap?  The memory leak is not really an issue since the memory can be reclaimed at any time.

Also, if I were using a map, I would define a FormatAndLocale util class, with valid hashCode and equals, to be used as keys.


Koch snowflake in Python with numpy and pygame I've drawn a Koch snowflake with Python 3. I'm new to the Python world and would appreciate all feedback on how to make the code more Pythonic as I'm not used to these idioms, styles and modules. All other feedback is of course also greatly appreciated.

The snowflake is constantly redrawn in preparation for the future. This main loop is based on the pygame.draw example but feels a bit clunky with done=False;while not done etc.

import pygame
import numpy
from math import pi, sin, cos

# some constants
FPS = 10
WINDOW_SIZE = [400,400]
LINE_WIDTH = 1
MIN_LINE_LENGTH = 1 # when to terminate recursion

# main loop
def main():
    # init pygame and window
    pygame.init()
    screen = pygame.display.set_mode(WINDOW_SIZE)
    # loop until user exits
    done = False
    clock = pygame.time.Clock()
    while not done:
        # limit frame rate
        clock.tick(FPS)
        # check if user exited
        for event in pygame.event.get():
            if event.type == pygame.QUIT:
                done=True
        # clear screen
        screen.fill([255,255,255])
        # calculate the three corners of the snowflake,
        # an equilateral triangle centered on the screen
        screen_center = numpy.array([ WINDOW_SIZE[0]/2, WINDOW_SIZE[1]/2 ])
        radius = WINDOW_SIZE[1]/2 # not really radius? but distance from center to corner
        snowflake_top   = screen_center + vector_from_polar( radius, pi/2 )
        snowflake_left  = screen_center + vector_from_polar( radius, pi/2 + 2*pi/3 )
        snowflake_right = screen_center + vector_from_polar( radius, pi/2 + 4*pi/3 )
        # draw the snowflake
        draw_koch_line( screen, snowflake_top,   snowflake_left  )
        draw_koch_line( screen, snowflake_left,  snowflake_right )
        draw_koch_line( screen, snowflake_right, snowflake_top   )
        # flip buffers
        pygame.display.flip()

# vector_from_polar: constructs a vector from its angle and mangitude
def vector_from_polar( magnitude, angle ):
    return magnitude * numpy.array( [ numpy.cos( angle ), numpy.sin( angle ) ] )

# draw_koch_line: (approximately) draws a Koch line between the points specified
def draw_koch_line(screen, line_start, line_end ):
    if numpy.linalg.norm( line_end - line_start ) / 3 < MIN_LINE_LENGTH:
        # last iteration: draw the line
        pygame.draw.line(screen, [0,0,0], line_start, line_end, LINE_WIDTH )
    else:
        # find the normal to this line
        line_normal = numpy.array([
                line_end[1]-line_start[1],
                line_start[0]-line_end[0] ])
        # find the three points of the "triangle" used to define the segments
        triangle_left  = 2/3*line_start + 1/3*line_end # 1/3 of this line
        triangle_right = 1/3*line_start + 2/3*line_end # 2/3 of this line
        triangle_top   = 1/2*line_start + 1/2*line_end + \
                         numpy.sqrt(3)/2/3 * line_normal # point "above" the line
        # recurse for each segments
        draw_koch_line(screen, line_start,     triangle_left)
        draw_koch_line(screen, triangle_left,  triangle_top)
        draw_koch_line(screen, triangle_top,   triangle_right)
        draw_koch_line(screen, triangle_right, line_end)

main()

This is good code — I only have a couple of minor points.

Since nothing changes or animates, there's no need to have a top-level loop or a clock. You can just draw the snowflake and then wait for the user to quit (calling event.wait instead of event.get so that the program isn't busy doing nothing).
It's clearer to write pygame.Color('white') and pygame.Color('black') instead of [255, 255, 255] and [0, 0, 0].
Instead of:

# find the normal to this line
line_normal = numpy.array([
        line_end[1]-line_start[1],
        line_start[0]-line_end[0] ])

consider using the cross product:

line_normal = numpy.cross(line_end - line_start, [0, 0, 1])

There's repeated code in the computation of the triangle points that could be factored out into a function:

def interpolated(x, start, end):
    """Return vector interpolated between start and end by x."""
    return (1 - x) * start + x * end

and then:

triangle_left  = interpolated(1/3, line_start, line_end)
triangle_right = interpolated(2/3, line_start, line_end)


Computing the distance and weight matrix of a graph I would like to create an igraph object. First, I set up the size of igraph object — n, m, then create a random matrix. As a result matrix B will be filled. Matrix B is the adjacency matrix for the igraph object. Inside for-loops I compute the distance d (scalar) and the weight matrix W. I would like to improve the perfomance of calculation. I think it is possible because I have used 4 for-loops in my code:

n <- 6
m <- 7

mat <- matrix(sample(0:255, n*m, replace=T), nrow = n, ncol = m)
R <- 2

a <- 1
b <- 1

d <- 0
e <- 0
g <- 0

W <- matrix(0, n, m)
B <- matrix(0, n*m, n*m)

for (i in 1:n){
for (j in 1:m){
  for (i0 in 1:n){
  for (j0 in 1:m){
            d <- ((i0 - i)^2+(j0 - j)^2)^(1/2)
            e <- ifelse(d <= R, 1, 0)
            g <- abs(mat[i,j] - mat[i0,j0])
            W[i0,j0] <- e * ((a*g)^2 + (b*d)^2)^(1/2)
  } # for i0
  } # for j0
  B[m * (i-1)+j,] <- c(t(W))

} #for_j
} #for_i

Could anyone please give me an idea how to rewrite the above code with fewer or no for-loops?

Here is how you would do it with no for loops. The dist() function computes the distance between every pair of rows of a matrix. So you have to apply it to two matrices:

the matrix C where each row contains the i and j coordinates of each vertex.
the matrix matrix(M, ncol = 1) where each row contains the value at a vertex.

M <- t(mat)
C <- cbind(c(row(M)), c(col(M)))
D <- as.matrix(dist(C, method = "euclidean"))
E <- as.numeric(D <= R)
G <- as.matrix(dist(matrix(M, ncol = 1), method = "manhattan"))
B <- E * ((a * G) ^ 2 + (b * D) ^ 2) ^ (1 / 2)

Note that in R, matrices are stored using "Column Major", i.e. column by column. So it would be more natural to pick that the second row/col in B corresponds to the item at row 2 and col 1 in mat. You chose the opposite (row 1 col 2) which forced me to start my code with M <- t(mat). If you want to pick the more natural approach as I suggest, just replace that first line with M <- mat.


Minimize Number of Lists I have a string, where I am only interested in getting the numbers encapsulated in single quotes.

For instance if I have the string "hsa456456 ['1', '2', ...]

I only want the 1 and the 2 and whatever numbers follow

To do this, I have the following code:

import re

#pattern = re.compile("dog")
#l = re.findall(pattern, "dog dog dog")

#valuepattern=re.compile('\'\d{1,6}\'')
valuepattern = re.compile('\'\d+\'')
li = []
s = "hsa04012 [[['7039', '1956', '1398', '25'], ['7039', '1956', '1399', '25']], [['1839', '1956', '1398', '25'], ['1839', '1956', '1399', '25']], [['1399', '25']], [['1398', '25']], [['727738', '1956', '1398', '25'], ['727738', '1956', '1399', '25']], [['1956', '1398', '25'], ['1956', '1399', '25']], [['1950', '1956', '1398', '25'], ['1950', '1956', '1399', '25']], [['374', '1956', '1398', '25'], ['374', '1956', '1399', '25']], [['2069', '1956', '1398', '25'], ['2069', '1956', '1399', '25']], [['685', '1956', '1398', '25'], ['685', '1956', '1399', '25']]]"

#if match:
#    print match.group()
#else:
#    print "no match"

l = re.findall(valuepattern, s)
#print l

for item in l:
    li.append(item.strip("'"))
    #print item

for item in li:
    print item

My areas of interest is to minimize the number of lists. Right now, I use two l and li. I take the item from l and append it to li after stripping. I was curious if there was a way to accomplish this operation all within one list... without the need for li and then appending.

New regex

If you change your regular expression to the following you won't need to even do str.strip()

valuepattern = re.compile("'(\d+)'")

List Comprehension

Alternatively if you don't want to do that, you could do the following. Currently you have:

for item in l:
    li.append(item.strip("'"))

This can be replaced with a list comprehension:

l = [x.strip("'") for x in l]

Final Note

As you compile your regular expression, you can replace

re.findall(valuepattern, s)

with

valuepattern.findall(s)


A function that checks for duplicate elements in an array Is there a simpler way to implement this function?

function has_duplicates(arr) {
  var myObject = {};

  for (var key in arr) {
    if (myObject[arr[key]])
      return true;
    else
      myObject[arr[key]] = true;
  }
  return false;
}

console.log(has_duplicates(["foo", "bar", "baz", "quux", "foo"]));

function has_duplicates(arr) {

In JS, function names are usually in camelCase. So this should be hasDuplicates.

for (var key in arr) {

Don't use for-in with arrays. for-in loops will run through properties, including prototype properties. Use a regular for loop with counters. A forEach would also work, but you can't bail out in the middle of the array.

So far, iirc, a loop and hash is favorable in this scenario since it's just O(n) and you can bail out in the middle of the operation.

Another way to do it is the following. The concept is that when a value appears twice, indexOf and lastIndexOf will not be the same, thus adding the entry to the array created by filter.

function hasDuplicates(arr){
  return !!arr.filter(fuction(value){
    return arr.indexOf(value) !== arr.lastIndexOf(value);
  }).length;
}

Another way to do it is by using ES6's Set. The concept is that sets only hold unique values. When you try to add a duplicate, it won't be added to the set. If there is a mismatch in the array length and set size, you jumped over a duplicate somewhere, thus revealing the existence of the duplicate.

function hasDuplicates(arr){
  var set = new Set();
  arr.forEach(function(value){ set.add(value); });
  return set.size !== arr.length;
}

Going with @PeterCordes's suggestion, you can use Set and check on each iteration the size mismatch. We can assume false until a mismatch happens.

function hasDuplicates(arr){
  var set = new Set();
  for(var i = 0; i < arr.length; i++){
    set.add(arr[i]);
    if(set.size !== i + 1) return true;
  }
  return false;
}


Objective C implementation of Swift's if-let This [Objective-]C-preprocessor macro currently handles single-variable if-let-not-nil and if-let-is-type, but doesn't handle multiple simultaneous tests and doesn't handle where clauses.

I prefer functions to macros, but I don't see any way to pass a local variable name to a function such that it would create a code block which can use that name internally when being compiled. Is there any way to make this a function?

This does not handle commas in code block: this can be worked around with () brackets, e.g.
     foo = @[1, 2];
         becomes
     foo = (@[1, 2]);, but I'd prefer something that doesn't surprise anyone using the macro, especially as the compiler error when commas are not wrapped in brackets is unhelpful.

#define IF_LET(tempType, temp, optional, code) \
if ((optional)!=nil && [optional isKindOfClass:tempType.class]) { \
    tempType *temp = (tempType*)optional;\
    {code}\
}

example usage:

for (SKNode *node in touchedNodes) {
    IF_LET(Tile, tile, node, {
        // Tile (capital) is one of my own classes
        // tile (lowercase) is defined at this point by the macro and only available within this code block
        SKAction *scale = [SKAction scaleBy: 1.2 duration: 0.15];
        SKAction *action = ([SKAction sequence:@[scale, scale.reversedAction]]);
        [tile runAction:action];
    })
}

SKLabelNode *t = nil;
IF_LET(SKLabelNode, temp, t, {
    // doesn't run: t was nil
})

t = [SKLabelNode labelNodeWithText:@"some text"];
IF_LET(NSObject, temp, t, {
    // this block runs, letting you do stuff with the local scope variable
    // NSObject *temp = (NSObject*)t;
}) else {
    // else block available for use, but
    // this one doesn't run because t is not nil and is a kind of NSObject
}

IF_LET(NSArray, temp, t, {
    // doesn't run, t is of wrong type
})

How can I improve this macro?

My suggestion would be: Don't do it. The main disadvantages
(in my opinion) are:

As you noticed, commas inside the macro argument are not handled,
leading to "strange" error messages:

for (SKNode *node in touchedNodes) {
    IF_LET(Tile, tile, node, {
        // Use of undeclared identifier 'IF_LET'
        SKAction *scale = [SKAction scaleBy: 1.2 duration: 0.15];
        SKAction *action = [SKAction sequence:@[scale, scale.reversedAction]];
        // Too many arguments provided to function-like macro invocation
        [tile runAction:action];
    })
}

It is not obvious how this must be fixed, and enclosing all Objective-C
message calls feels unnatural.
Macros are not known to the debugger. As a consequence, you cannot
set a breakpoint at a certain line inside the block – the debugger will
stop at the end of the block instead.
For the same reason you cannot single-step through the block statements.

Some of your intended applications have alternatives in pure
Objective-C, which are (almost) as good.

Example 1: Your code

for (SKNode *node in touchedNodes) {
    IF_LET(Tile, tile, node, {
        SKAction *scale = [SKAction scaleBy: 1.2 duration: 0.15];
        SKAction *action = ([SKAction sequence:@[scale, scale.reversedAction]]);
        [tile runAction:action];
    })
}

can be replaced by

for (Tile *tile in touchedNodes) {
    if ([tile isKindOfClass:Tile.class]) {
        SKAction *scale = [SKAction scaleBy: 1.2 duration: 0.15];
        SKAction *action = ([SKAction sequence:@[scale, scale.reversedAction]]);
        [tile runAction:action];
    }
}

The fast-enumeration for statement does not care how about the
type of the loop variable, you only have to verify its class before
accessing it.

Example 2: Your code

SKLabelNode *t = nil;
IF_LET(SKLabelNode, temp, t, {
    // doesn't run: t was nil
    [temp setText:@"text"];
})

can be replaced by (the even shorter)

SKLabelNode *t = nil;
[t setText:@"text"];

because sending Objective-C message to nil is a no-op.

Example 3: Your code

t = [SKLabelNode labelNodeWithText:@"some text"];
IF_LET(NSObject, temp, t, {
    // this block runs, letting you do stuff with the local scope variable
    // NSObject *temp = (NSObject*)t;
}) else {
    // else block available for use, but
    // this one doesn't run because t is not nil and is a kind of NSObject
}

is equivalent to

t = [SKLabelNode labelNodeWithText:@"some text"];
if (t != nil) {
    // Local scope for if-case
} else {
    // Local scope for else-case
}


Express.js routes management for large web application I'm currently working on a large web application with numerous routes.  Currently, our routes are in one large list in a single JSON file.  We recently ran into a problem due to the precedence of params being set from top to bottom in the file.  This question details the issue we encountered further: Node.js Express route naming and ordering: how is precedence determined?

My plan is to propose an alternate routing solution based on my work from a personal project.  I feel like it's a valid suggestion but missed out on a chance to really make my case in a meeting the other day.  Here is a routes.js file from my personal project that demonstrates the pattern I would like to use, let me know what you think or of possible better alternatives/edits.  Thanks!

By creating separate routers for each resource, different controllers can access the same URLs without the params being overridden.

const express = require('express');

const usersRouter = express.Router();
const songsRouter = express.Router();
const playlistsRouter = express.Router();

const usersController = require('../controllers/users');
const songsController = require('../controllers/songs');
const playlistController = require('../controllers/playlists');

usersRouter.route('/')
  .post(usersController.create)

songsRouter.route('/')
  .get(songsController.index)
  .post(songsController.create)

playlistsRouter.route('/')
  .get(playlistController.index)
  .post(playlistController.create)

songsRouter.route('/:id')
  .get(songsController.show)
  .put(songsController.update)
  .delete(songsController.destroy)

playlistsRouter.route('/:id')
  .get(playlistController.show)
  .put(playlistController.update)
  .delete(playlistController.destroy)

module.exports = {
  songs: songsRouter,
  playlists: playlistsRouter,
  users: usersRouter,
};

A couple questions that come to my mind are:

How optimal it would be to have all those Express router instances?
Is this approach scalable?

absolutely, you should compose your express application in terms of a hierarchy of routers

instancing all those routers should be negligible in terms of your application's performance
this is the only approach i can imagine feasibly maintaining at scale, it buys you some separation of concerns


Data structure for inserting and replacing data in list I have a list of key value pairs, that is used as an intermediate store for building URL parameters or JSON data. Currently this is done with a linked list:

struct _kvp {
    char *key;
    struct _v *value; /* the implementation of _v does not matter right now */
    unsigned char is_array;
    struct _kvp *next;
};

struct _kvp_store {
    t_object x_ob;
    struct _kvp *first_data;
    struct _kvp *last_data;
};

Now I want to be able to do several things with the key value pairs:

Add a value identified with a key, that is not an array member: If an item with the same key already exists, I will replace that. If that item is currently an array, all array members have to be deleted first. Otherwise, append the data to the list.
Add an array member to the linked list: if an item with the same key exists and is not an array member, then replace that with the new key value pair, else if an item with the same key already exists and is an array member, add the new key value pair after all other items with the same key, otherwise append to the list.

This means, that for every added or replaced item, the whole list has to be scanned, running it in O(n) for each item or O(n²) for building a list from scratch. Is there a better data structure in C to use for that, that is just as simple to implement?

static void kvp_replace_single(
        struct _kvp *const item,
        struct _v *const value,
        const unsigned char is_array) {
    kvp_val_free(item->value);
    item->value = value;
    item->is_array = is_array;
}

static void kvp_replace_array(
        struct _kvp *const item,
        const char *const key,
        struct _v *const value) {
    struct _kvp *to_free;

    kvp_val_free(item->value);
    item->value = value;
    item->is_array = 0;
    while (item->next != NULL && strcmp(item->next->key, key) == 0) {
        to_free = item->next;
        item->next = to_free->next;
        kvp_free(to_free);
    }
}

static void kvp_add_array(
        struct _kvp_store *const store,
        struct _kvp *const item,
        char *const key,
        struct _v *const value) {
    struct _kvp *it = item;
    struct _kvp *new;

    while (it->next != NULL && strcmp(it->next->key, key) == 0) {
        it = it->next;
    }
    new = kvp_create(key, value, 1);
    kvp_insert(store, it, new);
}

static void kvp_add(
        struct _kvp_store *const store,
        char *const key,
        struct _v *const value,
        const unsigned char is_array) {
    struct _kvp *it = store->first_data;
    unsigned char found = 0;

    while (it != NULL) {
        if (strcmp(it->key, key) == 0) {
            found = 1;
            if (!it->is_array) {
                kvp_replace_single(it, value, is_array);
            } else if (!is_array) {
                kvp_replace_array(it, key, value);
            } else {
                kvp_add_array(store, it, key, value);
            }
            break;
        }
        it = it->next;
    }

    if (!found) {
        struct _kvp *new = kvp_create(key, value, is_array);
        kvp_insert(store, store->last_data, new);
    }
}

A hashtable sounds like a better fit for your problem than using a linked list like this, giving you O(1) lookup time in the average case (O(n) for worst case).

You can find a popular hashing algorithm here: djb2

Also are you sure of your use of const?

struct _kvp *const item;
const struct _kvp *item;


Highest pit - only climbing through the pit once Problem description:

  A non-empty zero-indexed array A consisting of N integers is given. A pit in this array is any triplet of integers (P, Q, R) such that:

  0 ≤ P < Q < R < N;

  sequence [A[P], A[P+1], ..., A[Q]] is strictly decreasing, i.e. A[P] > A[P+1] > ... > A[Q];

  sequence A[Q], A[Q+1], ..., A[R] is strictly increasing, i.e. A[Q] < A[Q+1] < ... < A[R].

  The depth of a pit (P, Q, R) is the number min{A[P] − A[Q], A[R] − A[Q]}.

  For example, consider array A consisting of 10 elements such that:

  A[0] =  0
  A[1] =  1
  A[2] =  3
  A[3] = -2
  A[4] =  0
  A[5] =  1
  A[6] =  0
  A[7] = -3
  A[8] =  2
  A[9] =  3

  Triplet (2, 3, 4) is one of pits in this array, because sequence [A[2], A[3]] is strictly decreasing (3 > −2) and sequence [A[3], A[4]] is strictly increasing (−2 < 0). Its depth is min{A[2] − A[3], A[4] − A[3]} = 2.

  Triplet (2, 3, 5) is another pit with depth 3.

  Triplet (5, 7, 8) is yet another pit with depth 4. There is no pit in this array deeper (i.e. having depth greater) than 4.

Imagine I would write this code at an interview. What would you say?

Time complexity: \$O(n)\$

Space complexity: \$O(n)\$

Auxiliary space complexity: \$O(1)\$

public class FindDeepestPit {

    public static void main(String[] args) {
        int[] heights = { 0, 9, 6, -2, 7, 8, 0, -3, 2, 3 };
        int result = findDeepestPit(heights);
        System.out.println(result);
    }

    private static int findDeepestPit(int[] heights) {
        int firstIndex = 0;
        int deepest = -1;
        int depth = 0;
        boolean climbingUp = false;

        /*
        * mark current position as highest (firstIndex)
        * - go to next as long as we're going down
        * - when we're not going down anymore, switch to mark us going up
        * - go up until we can't go up anymore, then save the current depth of the pit, and mark the current position as highest
        * */

        for (int i = 0; i < heights.length - 1; i++) {
            int currentHeight = heights[i];
            int nextHeight = heights[i + 1];
            // find higher point
            if (!climbingUp) { // climbing down
                if (currentHeight < nextHeight) {
                    // we can't go further down here
                    climbingUp = true;
                    deepest = i;
                }
            } else { // climbing up
                if (currentHeight > nextHeight) {
                    // we can't get further up here.
                    int lastIndex = i;
                    int depthA = heights[firstIndex] - heights[deepest];
                    int depthB = heights[lastIndex] - heights[deepest];
                    int currDepth = Math.min(depthA, depthB);
                    depth = Math.max(depth, currDepth);
                    firstIndex = i;
                    climbingUp = false;
                }
            }
        }

        int depthA = heights[firstIndex] - heights[deepest];
        int depthB = heights[heights.length - 1] - heights[deepest];
        int currDepth = Math.min(depthA, depthB);
        depth = Math.max(depth, currDepth);

        return depth;
    }
}

Primary concerns:

Is the approach clear?
Are there any edge cases I did not think about?
At an interview, would adding unit tests for this method be a good idea?

Your code over-complicates the basic premise. The idea of having an ascending or descending element is right, but can be simplified further by just tracking maxima/minima, there is no need to track the actual index of the deepest pit.

If you identify peaks, and pits, and as you walk through the data, identify whether the current value increases the depth of the current pit, and increase the max if it does, then your code can be reduced to just a single check that identifies inflections, and another max/min check that compares depths.

private static int deepest(int[] data) {

    if (data.length < 1) {
        return 0;
    }

    int inflection = 0;
    int max = 0;
    int descent = 0;
    boolean ascending = true;
    for (int i = 1; i < data.length; i++) {
        boolean goingup = data[i] == data[i - 1] ? ascending : data[i] >= data[i - 1];
        if (goingup != ascending) {
            ascending = goingup;
            descent = ascending ? (data[inflection] - data[i - 1]) : 0;
            inflection = i - 1;
        }

        max = Math.max(max, Math.min(descent, data[i] - data[inflection]));
    }
    return max;
}

(See this running with some tests in ideone)


Simple code for calculating damage increase In my little game project, I have the following piece of code, in the static class DamageHelper, that takes a Spell, and returns it's damage in the Entity:

public static float DamageIncrease(SpellData attack, Entity caster)
{
    //Elemental damage is more increased by wisdom attribute
    //Impact damage is more increased by dominance attribute
    //summon level is increased by control attribute

    float bonus = 0f;
    switch (attack.element)
    {
        case Element.Fire:
        case Element.Earth:
        case Element.Water:
        case Element.Wind:
            bonus = caster.wisdom * 0.3f + caster.dominance * 0.1f + caster.control * 0.1f;
            break;
        case Element.Impact:
            bonus = caster.wisdom * 0.1f + caster.dominance * 0.3f + caster.control * 0.1f;
            break;
        default:
            bonus = caster.wisdom * 0.1f + caster.dominance * 0.1f + caster.control * 0.3f;
            break;
    }

    return attack.damage + bonus;
}

The function above is called by the this RealDamage function, that may also be improved:

public static float RealDamage(SpellData attack, Entity target, Entity caster)
    {
        float max_variation = attack.damage_variation / 2f;
        float min_variation = max_variation - attack.damage_variation;
        float resist_percent = 0f;
        float dmg = DamageIncrease(attack, caster) + Random.Range(min_variation, max_variation);
        switch (attack.element)
        {
            case (Element.Fire):
                resist_percent = target.GetFireResistance();
                break;
            case (Element.Earth):
                resist_percent = target.GetEarthResistance();
                break;
            case (Element.Water):
                resist_percent = target.GetWaterResistance();
                break;
            case (Element.Wind):
                resist_percent = target.GetWindResistance();
                break;
            case (Element.Impact):
                resist_percent = target.GetPhysicalResistance();
                break;
        }

        dmg -= (dmg * resist_percent / 100f);

        if (dmg < 1f)
            dmg = 0f;

        return dmg;
    }

and this one is called when player casts a spell:

float damage = DamageHelper.RealDamage(data, e, spellCaster);
dmg.GetComponent<DamageText>().MakeTooltip(data.element, damage, e.transform.position);
e.TakeDamage(damage);

It's pretty simple, but I would like to know if there are any ways it can be improved. Also, I will probably add more formulas, since I have to get the bonus from the Entity equipment. In this case, where should I put these?

Define some data structures.

Data definitions are clear, complete, obvious, in one place
DRY principle - a single place to change data values
client code does not change if data values change
Declared "Default" Element to keep code consistent
Reusable

.

public DamageFactor {
   public float Wisdom    { get; protected set; }
   public float Domanince { get; protected set; }
   public float Control   { get; protected set; }

   public DamageFactor (int wisdom, int domanince, int control) { ... }
}

public Dictionary<Element, DamageFactor> Damage = new Dictionary<Element, DamageFactor>() {
    { Element.Fire,    new DamageFactor { Wisdom = 0.3f, Dominance = 0.1f, Control = 0.1f },
    { Element.Earth,   new DamageFactor { Wisdom = 0.3f, Dominance = 0.1f, Control = 0.1f },
    { Element.Wind,    new DamageFactor { Wisdom = 0.3f, Dominance = 0.1f, Control = 0.1f },
    { Element.Water,   new DamageFactor { Wisdom = 0.3f, Dominance = 0.1f, Control = 0.1f },
    { Element.Impact,  new DamageFactor { Wisdom = 0.1f, Dominance = 0.3f, Control = 0.1f },
    { Element.Default, new DamageFactor { Wisdom = 0.1f, Dominance = 0.1f, Control = 0.3f }
};

public Dictionary<Element, int> Resistance = new Dictionary<Element, int>() {
    { Element.Fire, 12 },
    { Element.Earth, 9 },
    { Element.Water, 7 },
    { Element.Wind, 5 },
    { Element.Impact, 3 },
    { Element.Default, 1 }
};

public static float DamageIncrease(SpellData attack, Entity caster) {
//Elemental damage is more increased by wisdom attribute
//Impact damage is more increased by dominance attribute
//summon level is increased by control attribute

float bonus = 0f;
bonus = caster.wisdom    * Damage[attack.element].Wisdom +
        caster.dominance * Damage[attack.element].Dominance +
        caster.control   * Damage[attach.element].Control;

return attack.damage + bonus;
}

public static float RealDamage(SpellData attack, Entity target, Entity caster) {
    float max_variation = attack.damage_variation / 2f;
    float min_variation = max_variation - attack.damage_variation;
    float resist_percent = 0f;
    float dmg = DamageIncrease(attack, caster) + Random.Range(min_variation, max_variation);

    resist_percent = Resistance[attack.element];
    dmg -= (dmg * resist_percent / 100f);

    if (dmg < 1f)
        dmg = 0f;

    return dmg;
}


Checking if a number is power of 2 or not This is my solution in Java and I need your opinion:

public class Main {

  /**
   *
   */

  public static void main(String[] args) {

    Scanner in = new Scanner(System.in);
    int number = in.nextInt();
    in.close();

    if (isPowerOfTwo(number)) {
      System.out.println("yes");
    } else {
      System.out.printf("no");
    }
  }

  private static boolean isPowerOfTwo(int number) {

    if (number % 2 != 0) {
      return false;
    } else {

      for (int i = 0; i <= number; i++) {

        if (Math.pow(2, i) == number) return true;
      }
    }
    return false;
  }
}

There's actually a bit hack for this :

private static boolean isPowerOfTwo(int number) {

    return number > 0 && ((number & (number - 1)) == 0);

}

(ref : Bit Twiddling hacks)

This exploits the fact, that in binary notation a power of two is a 1 followed by a number of 0's, and the number just below is all 1's equal to that number of 0's :

  100000000 // number
& 011111111 // number - 1
-----------
  000000000


NCAA Pool - Generate random teams for a list of players Anyways, I am running a pool where 64 entrants will receive a random team from a field of 64 teams.  I'd like for it to be randomized to a decent extent, and although I understand the limitations of RNGs, I think this should be good enough.  See my code below - are there any issues with this?  Could this be done in an easier way?  Or alternatively, is this better asked in Cross Validated?

import random

regions = ["East", "West", "Midwest", "South"]
playerlist = []
teamlist = []

# this will be an actual list of player names when completed
for i in range(0, 64):
    playerlist.append("Player " + str(i))

for i in range(0, 7):
    random.shuffle(playerlist)

for i in range(0, 4):
    for j in range(1, 17):
        teamlist.append(playerlist[i*16+j-1] + ": " + regions[i] + " " + str(j))

#  edited: this is unnecessary
#for i in range(0, 7):
#    random.shuffle(teamlist)

print(teamlist)

Randomly shuffling a list seven times is not any better than doing it once. So random.shuffle(playerlist) is enough.

In Python you almost never want to iterate over the indices of a list, rather iterate over the list itself. Here you could do:

import random
import itertools

regions = "East", "West", "Midwest", "South"
players = ["Player {}".format(i) for i in range(64)]
teams = range(1, len(players) / len(regions) + 1)
random.shuffle(players)

teamlist = ["{}: {} {}".format(player, *team) for player, team in
            zip(players, itertools.product(regions, teams))]

print(teamlist)

Here I used str.format to make formatting the different strings easier and itertools.product to construct a list (actually an iterable) of region, team:

>>> list(itertools.product(regions, teams))
[('East', 1), ('East', 2), ('East', 3), ('East', 4), ('East', 5), ('East', 6), ('East', 7), ('East', 8), ('East', 9), ('East', 10), ('East', 11), ('East', 12), ('East', 13), ('East', 14), ('East', 15), ('East', 16), ('West', 1), ('West', 2), ('West', 3), ('West', 4), ('West', 5), ('West', 6), ('West', 7), ('West', 8), ('West', 9), ('West', 10), ('West', 11), ('West', 12), ('West', 13), ('West', 14), ('West', 15), ('West', 16), ('Midwest', 1), ('Midwest', 2), ('Midwest', 3), ('Midwest', 4), ('Midwest', 5), ('Midwest', 6), ('Midwest', 7), ('Midwest', 8), ('Midwest', 9), ('Midwest', 10), ('Midwest', 11), ('Midwest', 12), ('Midwest', 13), ('Midwest', 14), ('Midwest', 15), ('Midwest', 16), ('South', 1), ('South', 2), ('South', 3), ('South', 4), ('South', 5), ('South', 6), ('South', 7), ('South', 8), ('South', 9), ('South', 10), ('South', 11), ('South', 12), ('South', 13), ('South', 14), ('South', 15), ('South', 16)]

Note that 0 is the default start argument for a range.

If your script becomes longer, you should probably add a if __name__ == "__main__": guard to allow importing your code from other scripts.


Code refactory for a form validation I wrote some code to validate a form, I'm using live validation, as the user types, here is the snippet that I wrote just for the live validation, but I think I'm repeating too much and I'm looking for some advices:

  $('form input').focus(function(){
      var inp = $(this).attr('name');
      switch(inp)
      {
        case 'password2':
          var theIn = $(this).parent().prev().find('input');
          if(!theIn.val() != '')
          {
            theIn.addClass('error');
          }else{
            theIn.removeClass('error');
          }

        var theInp = $(this).parent().prevAll().find('input,select');
        theInp.each(function(){
        if((!$(this).val()) || ($(this).val() == '-1'))
        {
          $(this).addClass('error');
        }else{
          $(this).removeClass('error');
        }
        });

        break;
        default:
        //Find all previous inputs and see if they are OK
        var theInp = $(this).parent().prevAll().find('input,select');
        theInp.each(function(){
        if((!$(this).val()) || ($(this).val() == '-1'))
        {
          $(this).addClass('error');
        }else{
          $(this).removeClass('error');
        }
        });
      }
    });

/*
* Register front on keypress
 */
  $('form input,form select').each(function(){

    //Select <select>
    $(this).change(function(){
      if($(this).val() == '-1')
      {
        $(this).addClass('error');
      }else{
        $(this).removeClass('error');
      }
    });
    $(this).keyup(function(e){
      var type = $(this).attr('name');
      $(this).parent().find('span.preloader').css('visibility','hidden');
      switch(type)
      {
        case 'email':
          var pattern = /^[-a-z0-9~!$%^&*_=+}{\'?]+(\.[-a-z0-9~!$%^&*_=+}{\'?]+)*@([a-z0-9_][-a-z0-9_]*(\.[-a-z0-9_]+)*\.(aero|arpa|biz|com|coop|edu|gov|info|int|mil|museum|name|net|org|pro|travel|mobi|[a-z][a-z])|([0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}))(:[0-9]{1,5})?$/i;
          if(!$(this).val().match(pattern))
          {
            $(this).addClass('error');
          }else{
            $(this).removeClass('error');
          }
        break;
        case 'password2':

          if($(this).val() != $(this).parent().prev().find('input').val())
          {
            $(this).addClass('error');
            $(this).parent().prev().find('input').addClass('error');
          }else{
            $(this).removeClass('error');
            $(this).parent().prev().find('input').removeClass('error');
          }
        break;
        case 'password':
          if($(this).val() == $(this).parent().next().find('input').val())
          {
            $(this).parent().next().find('input').removeClass('error');
            $(this).removeClass('error');
          }else{
            $(this).addClass('error');
          }
          return true;
        break;
        default:
          if((!$(this).val()) || ($(this).val() == '-1'))
          {
            $(this).addClass('error');
          }else{
            $(this).removeClass('error');
          }
      }
  });
});

Overall, I see the following issues with your code

It's coupled way to tightly to the markup
It's overly complex
You call $(this) again and again without ever caching the result
It's repetitive

I won't be posting fully revised code, as there's a lot I'd do differently. Instead, I'll try to explain the steps you could take.

Right now, you're relying on the markup never, ever changing. For instance, you're using $(this).parent().prev().find('input') to find the other password field, but if the markup changes just slightly, your code will no longer find the right input. You'd have to change your code each time you change your markup. Obviously, that's not very maintainable.

You're doing a similar thing to find all other inputs/selects in the form - again, this will break if the markup changes.

Instead, do something like

$("form").each(function () {
  var form = $(this),
      fields = form.find("input, select");

  // ... everything here has access to the form and fields variables
});

Or at the very least use $(this).closest("form").find("input, select")

In either case, you greatly reduce the coupling between markup and code.

Also, do not rely an input's name to tell you its intended type. A name can be anything. For instance, you could have a field called "recipient" which should be an email-address, but then you'd have to update your code to look for that.

In the case of email-addresses specifically, HTML5 has a type-attribute value you can use:

<input name="whatever" type="email">

Your other checks seem to be about checking presence of any value - i.e. required fields. Again, HTML5 has an attribute for that:

<input name="whatever" type="email" required>

So now, your code can check the input's type to determine it's intended type, and check for the required attribute to determine if it's required (seems logical, right?)

Depending on the browser, it might actually "help out" should your validation fail. For instance, Chrome respects and enforces the email type, and so requires the user to type an email-address.

Incidentally, it makes more sense to use an empty string "" as the nothing-selected value in the select element. -1 is a value, whereas an empty string is, well, an empty string; no value at all. JavaScript itself considers "" == false but considers -1 == true.

Last up is the password and password repeat. Instead of the complex jQuery selector chain to find the other password field, use HTML5's data-* attributes to say what you should check against. For instance:

<input id="password" type="password" data-match="password_confirmation">

<input id="password_confirmation" type="password" data-match="password">

Now each element can tell you what its counterpart is, regardless of how your markup is put together.

As for structure and repetition: Don't use the switch statement for big blocks of code. It's hard to read, and hard to maintain. Instead, DRY (Don't Repeat Yourself) you code, by breaking it into functions. For instance, you could have functions like checkEmail and checkSelection that you can pass an element to, and they'll report true or false, depending on element's value.

Do the same for the bit of code that adds the error class: Have a function, like, say

function markElement(element, valid) {
  if(valid) {
    $(element).removeClass("error");
  } else {
    $(element).addClass("error");
  }
}

Now you have some more generic building blocks.

Lastly, don't do all the validation logic in the event handlers themselves. The validation logic is the same regardless of how/why something needs to be validated. Instead, write the validation code separately and use the event handlers to call it.


More Revealing Module Patter in Javascript It is known a Revealing Module Pattern in javascript, and here it is a little example.

var Module = (function(){
     var a = 3,
         init = function(a_user){ a = a_user; },
         update = function(){},
         get = function(){return a},
         set = function(a_user){ a = a_user; };

     return {
         init : init,
         update : update,
         get : get,
         set : set
     }
})();

It's good for lots of uses. But I modified it in 2 steps, and some people tell me that I've overloaded it, and my new variations take lots of memory. So I ask, if my variations are really bad, and I shouldn't use it?

Variation 1

I like to separate public and private logic. It's useful, because I separate methods, which I allow to users of my module (in the client code).

For example:

var MyModule = (function(){
     var
         pr = {}, // Private Scope
         PU = {}; // Public Scope

     pr.fetch = function(){}; // fetching data
     pr.build = function(){}; // building DOM of module
     pr.show  = function(){}; // showing our object
     pr.hide  = function(){}; // hiding our object
     //... Other private methods

     // Public methods
     PU.init = function(){
         pr.fetch().build().show();
         return PU; // for making chains
     }
     PU.update = function(){};
     //... other Public methods

     return PU;
})();

I know, that in usual Revealing Module Pattern methods that are in last return are public, but other - private. But when you calling private method there, you just type it name, and it's unknown if it is a private method of this module, or it's a global function, that's why I have separated object - pr and PU.

Variation 2:

Sometimes I had need to insert some of modules twice on a page (or maybe more). So, I understand that it's not a module anymore, because Module can't have instances, Module is just a bundle of related functions. But I've decided to make a Module that can generate instances of itself.
I've used my code from Variation 1 with a little fix:

var MyModule = function(){
     var
         pr = {}, // Private Scope
         PU = {}; // Public Scope

     pr.fetch = function(){}; // fetching data
     pr.build = function(){}; // building DOM of module
     pr.show  = function(){}; // showing our object
     pr.hide  = function(){}; // hiding our object
     //... Other private methods

     // Public methods
     PU.init = function(){
         pr.fetch().build().show();
         return PU; // for making chains
     }

     PU.update = function(){};
     //... other Public methods

     // Constructor
     PU.__construct = function() {
         // constructor code
         return PU;
     }

     return PU.__construct.apply(this, arguments);
};

// Usage:
var m1 = new MyModule(/* some options*/);
m1.init();

var m2 = new MyModule(/* other options */);
m2.init();

Are these variation really bad? Should I avoid them, as some people have said to me some time earlier...

There's nothing really wrong with variation 1, but there's also no point in putting things in pr. Sure you can chain it, but why chain something where this only refers to half the stuff you need (the private stuff)? You might as well just use function declarations. And you can get rid of PU, since it's kind of a meaningless variable name:

var MyModule = (function(){
    function fetch(obj){}; // fetching data
    function build(obj){}; // building DOM of module
    function show(obj){};  // showing our object
    function hide(obj){};  // hiding our object
     //... Other private methods
     return {
         // Public methods
         init: function(){
             show(build(fetch(this)));
             return this; // for making chains
         },
         update: function(){}
         // ... other Public methods
     };
})();

The problem with variation two is that you create those functions whenever the constructor is called (so, more than once).

The normal way to avoid this is to do something like

var MyModule = (function(){

    function fetch(obj){}; // fetching data
    function build(obj){}; // building DOM of module
    function show(obj){};  // showing our object
    function hide(obj){};  // hiding our object

    function MyModule(){
        // constructor code
    };

    MyModule.prototype.init = function(){
        show(build(fetch(this)));
        return this;
    };

    MyModule.prototype.update = function(){};
    //... other Public methods

    return MyModule;

}());

Does that make sense?

This is not really a "module" anymore, by the way, just a normal constructor with some function properties attached to the prototype. But it looks like that's what you want, since you need multiple instances of it.