Writing the function strend() Due to being unable to ask a C programming language expert or advanced user in person, I'm posting this simple question about my C code. Today I was refreshing my C with exercise 5.4 of the C programming Language 2nd Edition by K&R, which is a very simple exercise:

Do you think that my way of handling pointers is 'very good' or 'world class'?

I know the example is short/simple but that question comes to my mind because I want to know the opinion from other knowledgeable people about the subject, if you want to critique my code please do so. I'm always trying to push my current C knowledge to the next level.

/*----------------------------------------------------*/
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
/*
  Exercise 5-4.
  Write the function strend(s,t)
  which returns 1 if the string t occurs at the
  end of the string s, and zero otherwise.
*/
int strend(char *s, char *t);

int main(int argc, char *argv[])
{
  int rt;

  if ( rt = strend("Unix Powered", "red"))
     printf("occurs\n");

 return EXIT_SUCCESS;
}

/*
  find if 't' occurs at the end of 's'
*/
int
strend(char *s, char *t)
{
   while (*s)
      s++;
   if ( strlen(s) < strlen(t))
      return 0;
   s -= strlen(t);
   for (; *t ; s++, t++) {
      if ( *s != *t)
         return 0;
   }
 return 1;
}
/*----------------------------------------------------*/

I would write the function like this:

#include <string.h>

/* Return 1 if string s1 ends with s2; return 0 otherwise. */
int strend(const char *s1, const char *s2)
{
    size_t l1 = strlen(s1);
    size_t l2 = strlen(s2);
    return l2 <= l1 && memcmp(s2, s1 + l1 - l2, l2) == 0;
}

If we know the length of the strings (as here), then memcmp is usually the quickest way to compare them. That's because memcmp doesn't have to check for the end of the string (unlike strcmp), and because standard library implementations of memcmp tend to be well optimized, using word-at-a-time comparison and other tricks if possible.


Iterate an array of BlockingCollections I have an array of BlockingCollections that i have initiated like this:

BlockingCollection<FxDataMapper>[] _fxDataByPair ;
arrayOfBlockingCollection = new BlockingCollection<MyClassInstance>[4];
            for (int i = 0; i < 4; i++)
                _arrayOfBlockingCollection[i] = new BlockingCollection<MyClassInstance>();

Now it want to iterate the array and extract each of the array elements and consume the BlockingCollections elements My code is this and although it works i think its not the best one

private void testArrayConsuming()
{
    Task processor = Task.Factory.StartNew(() =>
    {
        foreach (var x in arrayOfBlockingCollection )
        {
            Task processor2 = Task.Factory.StartNew(() =>
            {
                foreach (var y in x.GetConsumingEnumerable())
                        Console.WriteLine(x.Element.Value.ToString());

            });
        }
    });
}

Your code has a couple of problems:

It is a void (synchronous) whose only task is to start a Task, lying to the caller since it's in fact an asynchronous Task.
You are using Task.Factory.StartNew instead of the preferred Task.Run

So, a better approach to this would be something like this:

private async Task testArrayConsuming()
{
    foreach (var x in arrayOfBlockingCollection)
    {
        await Task.Run(() =>
        {
            foreach (var y in x.GetConsumingEnumerable())
            {
                Console.WriteLine(x.Element.Value.ToString());
            }
        });
    }
}

This would block execution, consuming one x at a time. If you want this to be parallel, use this instead:

private async Task testArrayConsuming()
{
    var tasks = new List<Task>();
    foreach (var x in arrayOfBlockingCollection)
    {
        tasks.Add(Task.Run(() =>
        {
            foreach (var y in x.GetConsumingEnumerable())
            {
                Console.WriteLine(x.Element.Value.ToString());
            }
        }));
    }

    await Task.WhenAll(tasks); // wait for all of them
}


Python script which fetches Amazon product details using its API This is a script which takes an Amazon URL as input, takes out ASIN/ISBN from the URL, and uses Amazon Python API to fetch details.

For this task, I looked at many Amazon URLs and observed the following things:

ASIN/ISBN is always present in an Amazon URL
ASIN/ISBN can be present anywhere in the URL
ASIN is always in capital letters, consists of alphabets and numbers only and length is always 10
If ASIN is not present then it's a link to a book on Amazon and ISBN will be present
ISBN consists only of numbers
Amazon uses ISBN-10 format i.e. length of ISBN is 10
Haven't come across ASIN which is purely numbers and without any alphabets
ASIN or ISBN, always followed by '/'

I hope I covered everything, but if I missed some case, please let me know.

Secondly, I care about only Amazon India. So it throws that ASIN is invalid if I give an ASIN of Amazon US or any other country, or gives pricing info zero. I don't know why it is causing such behaviour (example).

Flow: Get the URL, check for either ASIN or ISBN, if none are present return false else use API to fetch name, price, URL and product image.

import re
from amazon.api import AmazonAPI

AMAZON_ACCESS_KEY = 'AKI...JP2'
AMAZON_SECRET_KEY = 'Eto...uxV'
AMAZON_ASSOC_TAG = 'p...1'

asin_regex = r'/([A-Z0-9]{10})'
isbn_regex = r'/([0-9]{10})'

def get_amazon_item_id(url):
    # return either ASIN or ISBN
    asin_search = re.search(asin_regex, url)
    isbn_search = re.search(isbn_regex, url)
    if asin_search:
        return asin_search.group(1)
    elif isbn_search:
        return isbn_search.group(1)
    else:
        # log this URL
        return None

def get_amazon_product_meta(url):
    # the input URL is always of amazon
    amazon = AmazonAPI(AMAZON_ACCESS_KEY, AMAZON_SECRET_KEY, AMAZON_ASSOC_TAG, region="IN")

    item_id = get_amazon_item_id(url)
    if not item_id:
        return None

    try:
        product = amazon.lookup(ItemId=item_id)
    except amazon.api.AsinNotFound:
        # log this ASIN
        return None

    # product.price_and_currency returns in the form (price, currency)
    product_price = product.price_and_currency[0]

    if product_price:
        return product.offer_url, product.title, product.large_image_url, product_price
    return None

This should be a short review mainly because your code looks pretty nice.

I don't have any experience with the Amazon API, but from what I can tell you are using it as I would expect it to be used.

The only points that I want to speak on and can nit-pick are:

Your variables asin_regex and isbn_regex are slightly misleading. In Python there is a distinction between regular expression patterns and regular expression objects. The suffix _regex implies that, by itself, the variable can recognize expressions which, in turn, implies the variable is a regular expression object. Because your variables cannot, by themselves, recognize expressions, I would use the suffix _pattern.
Your logic in get_amazon_id is completely fine. Its my personal preference, in this case where you are simply returning inside the if blocks, to just use if statements, no elif or else blocks:

if asin_search:
    return asin_search.group(1)
if isbn_search:
    return isbn_search.group(1)

return None

  The above is strictly personal preference, simply getting another version out there.

However, what I would recommend doing is use a for loop to reduce some of the minor code repetition and make your code more flexible:

for search in asin_search, isbn_search:
    if search:
        return search.group(1)
return None

The above code would be best if, say, for some reason another ID type came out. So, instead of having to add another if statement, all you need to do is add new_id_search to the for loop list.
I would change your regex patterns to use the \w and \d characters:

asin_regex = r'/(\w{10})'
isbn_regex = r'/(\d{10})'

The \d character would just make your currently short regex even shorter, while the \w character helps protect against the case that for some unknown reason the ASIN or ISBN contains lower-case characters.
Finally, I would capitalize the first word of all your comments (unless it is a variable name).


Themed Folder Classification I am trying a very simple method of getting the theme information via a theme.xml within each theme folder.

What it should do:

Scan the theme directory ../themes
Return the names of all the theme folders (array)
Use the name of the theme folder returned to create a link to search in file_exists()
Find the theme.xml in every folder
Parse every theme.xml with simplexml_load_file()
Return an array ready for output in a foreach()
If !file_exists(), output "theme.xml Not Found in (themename)"

I tried to make this in 2 functions but it would always output the first or last themes info, so this is what I'm doing now, can anyone suggest a way to help separate the function from the output.

Get available theme folder names function

 public function getAvailableThemes() {
    $path = '../themes';
    $themeDir = glob($path . '/*' , GLOB_ONLYDIR);
    return $themeDir;
}

Output in wcx_themes.php file

    <?php
    $themeDir = $backend->getAvailableThemes();
    foreach($themeDir as $themeDir) {
        $preview = $backend->getThemePreview($themeDir);
        $xmlfile = $themeDir.'/theme.xml';
    if(file_exists($xmlfile)) {
        $xml = simplexml_load_file($xmlfile);
        $themename = $xml->themename;
        $themedescription = $xml->themedescription;
        $themeversion = $xml->themeversion;
        $themeauthor = $xml->themeauthor;

    echo '

Name:&nbsp;'.$themename.'

Description:&nbsp;'.$themedescription.'

Version:&nbsp;'.$themeversion.'

Author:&nbsp;'.$themeauthor.'

'.$themename.'

    INSTALL

';
    }
    else {
    echo '

    getThemePreview($themeDir).'"/>

THEME.XML DOES NOT EXIST

';
    }
    }
    ?>

Get theme preview image

    public function getThemePreview($theme) {
    $ext = '.png';
    $preview = '../themes/'.$theme.'/preview'.$ext;
    if(file_exists($preview)) {
       return $preview;
    }
    else {
      $preview = 'images/nopreview'.$ext;
    return $preview;
   }
   }

Theme.xml

<?xml version="1.0" encoding="utf-8"?>
 <theme>
  <themename>Default</themename>
  <themedescription>This is the Default Theme</themedescription>
  <themeversion>1.0</themeversion>
  <themeauthor>WCX</themeauthor>
 </theme>

Yes you can move the output away from the code, however that's usually difficult to do with this type of procedural code. The code is extremely tightly bound, which makes separation harder than it should be!

The two function you call are fine as they are. So lets focus on those for now.

getAvailableThemes()

This function, since it's only being called once, is what I believe to be clutter. I don't see a need to separate it from the main logic. Doing so only increases the amount of times we need to look back at the code.

Also, I see no reason to cut of "Directory" in $themeDir. Spell it out for clarity.

getThemePreview()

Is the return value a "preview" or a "file"? Perhaps a better name would be getThemePreviewFile.
We can refactor the code a lot:

$preview = '../themes/'.$theme.'/preview.png';

if(!file_exists($preview)) {
    $preview = 'images/nopreview.png';
}
return $preview;

I shortened it, and I added the extension into the literal because it doesn't look like the extension will change.

Now for the main file!

wcx_themes.php

First off, indentation should be key here. Indentation is also missing here! Readability is very important.

foreach($themeDir as $themeDir) - I don't like this at all. Collections should be pluralized.
The immediate solution to separating the HTML and the PHP that I can think of, is a templating library. Other than that, you could store the HTMl in a string, and then use sprintf or another form of replacement to build the output.


Simple Hello World with unit tests This is a simple hello world sample project used for instruction.  I'm just looking for general comments on the approach and design.

AdvancedHelloWorldTest.java

/**
 * AdvancedHelloWorldTest.java
 * 2014 GetGnosis.com
 */
package com.getgnosis.tutorials.tutorial01001;

import static org.junit.Assert.*;

import java.io.ByteArrayOutputStream;
import java.io.PrintStream;

import org.junit.Test;

/**
 *  Test Class for
 *  {@link com.getgnosis.tutorials.tutorial01001.AdvancedHelloWorld}
 *
 * @author mhemphill
 * @version 1.0.0 - Mar 9, 2014
 */
public class AdvancedHelloWorldTest {

    /**
     * Test method for
     * {@link com.getgnosis.tutorials.tutorial01001.AdvancedHelloWorld
     *  #AdvancedHelloWorld()}.
     */
    @Test
    public void testAdvancedHelloWorld() {
        try {
            new AdvancedHelloWorld();
        } catch (Exception e) {
            fail("Construction failed. ");
        }
    }

    /**
     * Test method for
     * {@link com.getgnosis.tutorials.tutorial01001.AdvancedHelloWorld
     *      #main(java.lang.String[])}.
     */
    @Test
    public void testMain() {
        final ByteArrayOutputStream outContent = new ByteArrayOutputStream();
        System.setOut(new PrintStream(outContent));
        AdvancedHelloWorld.main(null);
        assertEquals("Hello World!" + System.getProperty("line.separator"), outContent.toString());
        System.setOut(null);
    }

    /**
     * Test method for
     * {@link com.getgnosis.tutorials.tutorial01001.AdvancedHelloWorld
     *      #toString()}.
     */
    @Test
    public void testToString() {
        String helloWorldString =  new AdvancedHelloWorld().toString();
        assertEquals("AdvancedHelloWorld [message=Hello World!]",helloWorldString);
    }

}

AdvancedHelloWorld.java

/**
 * AdvancedHelloWorld.java
 * 2014 GetGnosis.com
 */
package com.getgnosis.tutorials.tutorial01001;

/**
 * This is an example program that prints the {@link String} "Hello World!" to
 * the system's output display.
 *
 * @author mhemphill
 * @version 1.0.0 - Mar 9, 2014
 */
public class AdvancedHelloWorld {

    static {
        message = "Hello World!";
    }

    /**
     * The {@link String} instance representing the message to be displayed.
     */
    private final static String message;

    /**
     * The default constructor.  Initializes the value of message via the
     * static block.
     */
    public AdvancedHelloWorld() {}

    /**
     * The application entry point.  Creates a new instance of
     * {@link AdvancedHelloWorld} and prints the value of the {@link String}
     * message to the system's output display.
     *
     * @param args - @{link String} array representing the arguments to the
     * application.  Not used in this application.
     *
     */
    public static void main(String[] args) {
        System.out.println(new AdvancedHelloWorld().getMessage());
    }

    @Override
    public String toString() {
        StringBuilder builder = new StringBuilder();
        builder.append("AdvancedHelloWorld [");
        if (getMessage() != null)
            builder.append("message=").append(getMessage());
        builder.append("]");
        return builder.toString();
    }

    /**
     * Returns {@link String} instance representing the message to be displayed.
     *
     * @return message - {@link String} instance representing the message to
     * be displayed.
     */
    private final String getMessage() {
        return message;
    }

}

Having tests is great (I'd love to see more questions with tests), keep it up! Other parts I would keep simpler.

You might want this as a tag in your revision control system instead of commenting:

* @version 1.0.0 - Mar 9, 2014

This is also in the revision control history, so it's usually unnecessary:

* @author mhemphill

See: Pros and cons of versioning javadoc
I would not want to maintain comments on test methods like this:

/**
 * Test method for
 * {@link com.getgnosis.tutorials.tutorial01001.AdvancedHelloWorld
 *      #toString()}.
 */
@Test
public void testToString() {
    ...
}

I don't see why would it be useful. I haven't seen anyone who likes reading javadoc of test classes instead of the test code directly. The name of the method is good, it contains the main point: it's a test for toString, keep that and remove the superfluous comment.

@Test
public void testMain() {
    final ByteArrayOutputStream outContent = new ByteArrayOutputStream();
    System.setOut(new PrintStream(outContent));
    AdvancedHelloWorld.main(null);
    assertEquals("Hello World!" + System.getProperty("line.separator"), outContent.toString());
    System.setOut(null);
}

The System.setOut(null) statements should be in an @After tearDown method or at least in a finally block. If the assertEquals throws an exception (or something else) out won't be set back.
I the same testMain method I'd create a local variable for the expected result:

final String expected = "Hello World!" + System.getProperty("line.separator");
assertEquals(expected, outContent.toString());

It's easier to read. (Clean Code by Robert C. Martin, G19: Use Explanatory Variables;  Refactoring: Improving the Design of Existing Code by Martin Fowler, Introduce Explaining Variable)

@Test
public void testAdvancedHelloWorld() {
    try {
        new AdvancedHelloWorld();
    } catch (Exception e) {
        fail("Construction failed. ");
    }
}

The following is the same:

@Test
public void testAdvancedHelloWorld() {
    new AdvancedHelloWorld();
}

If the constructor throws an exception (or anything inside a @Test method) JUnit will tell you.
This comments does not say anything which isn't in the code already, I'd remove it:

/**
 * The default constructor. Initializes the value of message via the static
 * block.
 */

(Clean Code by Robert C. Martin: Chapter 4: Comments, Noise Comments)

static {
    message = "Hello World!";
}

/**
 * The {@link String} instance representing the message to be displayed.
 */
private final static String message;

The following is the same:

private final static String message = "Hello World!";

I would keep it simple.

@Override
public String toString() {
    StringBuilder builder = new StringBuilder();
    builder.append("AdvancedHelloWorld [");
    if (getMessage() != null)
        builder.append("message=").append(getMessage());
    builder.append("]");
    return builder.toString();
}

The if condition can't be false (the message field is final and has a non-null value), so the following is the same:

StringBuilder builder = new StringBuilder();
builder.append("AdvancedHelloWorld [");
builder.append("message=").append(getMessage());
builder.append("]");
return builder.toString();

I'd move the main method to a separate class. It's usually a good idea to separate a class from its clients.


Finding the 'Minimum' AND of all the possible subsets of a set The subset size should be greater than or equal to 2.

How can I speed up this code?

#include<stdio.h>
#include<stdlib.h>
#include<math.h>
#include<malloc.h>
#include <iostream>
#include <vector>
long long int set[100000];
long long int counter, j;
long long  int ans,min,count;
bool ha;
long long int pow_set_size;
long long int power[100000];
void printPowerSet(long long int set_size)
{
    pow_set_size =power[set_size];
    count=4;

    for(counter = 3; counter < pow_set_size; counter++)
    {
      ha=0;
        if(counter!=count)
       {
            for(j = 0; j < set_size; j++)
       {
          if(counter & (1<<j))
          {
              if(!ha)
                  {
                  ans=set[j];
                  ha=1;
              }
              else
             {
                  ans=((ans) & (set[j]));
              }
           }

       }
          // printf("%lld\n",ans);
            if(counter==3)
            {
            min=ans;
        }
            else if(min>ans)
                {
                min=ans;
            }
       }

        else
        {
            count=count*2;
        }
    }

   printf("%lld\n",min);
}

void pre()
    {
    power[0]=1;
    for(int i=1;i<=100000;i++)
        {
        power[i]=power[i-1]*2;

    }

}

int main()
    {
    long long int test;
    long long int len;
   long long int a;
    scanf("%lld",&test);

    pre();
    while(test--)
        {
        scanf("%lld",&len);
        for(int i=0;i<len;i++)
            {
            scanf("%lld",&set[i]);
        }
        printPowerSet(len);

    }
    return 0;
}

There are a few points I'd make about this code.

Choose your language. Right now, you're using a number of C++ headers, so a C compiler can't process your code. Unfortunately, you don't seem to be putting any C++ features to use to make the code more readable, understandable or safe than C code would be.
If you are going to write C++, one starting point is to avoid using built-in array types (as a rule) and printf/scan (nearly always). Unless you have some very specific reason to do otherwise, you're usually better off using std::vector instead of an array, and iostreams instead of printf/scanf.
Some of your variable names would benefit from better names. ha and ans were the two that I found particularly difficult to understand (to the point that I'm still not entirely sure what they mean).
Your code will overflow arithmetic limits on most hardware. In particular:

power[0]=1;
for(int i=1;i<=100000;i++)
{
    power[i]=power[i-1]*2;

...attempts to store 2100000 into a long long. On most typical hardware, long long is limited to 263-1. Although hardware may (probably will) someday expand from its current 64-bit word size to something larger, and long long may expand when it does so, this won't work on most current implementations, and I don't think we can expect to see implementations where it will work any time soon either.

Your only real choices here are to use some type that can store numbers up to 2100000, or else do things some other way so you don't need to store such large numbers. Oh, the final possibility would be to use unsigned long long, and live with the fact that you're storing the value modulo 264-1. For some purposes this works quite nicely (but I'm not sure what you're really using this for, so I'm not sure whether it's adequate for your purposes or not).
The code above also overflows the end of the array. You've defined the array power as:

long long int power[100000];

This means the valid subscripts into this array run from 0 through 99999. Unfortunately, your code loops through 100000, so it tries to write to an element one beyond the end of the array, which gives undefined behavior.

In C and C++, you typically use < for the comparison in a loop like this, so you'd use something like:

static const int size = 100000;

long long int power[size];

// ...

for (int i=0; i<size; i++)
    // write value to power[i]

Indentation. Sometimes indentation gets messed up when you paste here (especially if you have a mixture of tabs and spaces to indent lines), but you definitely want indentation to be more consistent than the code the way I'm seeing it above.


Unfair semaphore in Java I'm tutor for a university operating systems course, and part of my work is creating exercises for them. Those exercises are mostly about concurrency and synchronization issues. For the next exercise I intend to show them the finished implementation of an unfair counting semaphore, and their job is to make a fair one. Because of a lot of factors I'm constrained to using Java as implementation language.

Though I'm fully capable of writing Java programs, it's not exactly my "mother tongue". As such, I'm probably unaware of some idioms and best practices.

As I want to give my students not just something that "works", but code that could actually be found in a production system, I hope you can give me suggestions for my implementation:

/** Simple, unfair counting semaphore.

    This class implements a simple and unfair counting semaphore.
*/
public class UnfairSemaphore implements Semaphore {
    /** Count of available permits.
     */
    protected int count;

    /** Object used to communicate with waiting threads.

        A distinct object is used herefore (and not the semaphore itself)
        to keep any signals from notify() behind the abstraction.
     */
    protected Object wire = new Object();

    /** Construct a semaphore with the given initial count.

        Sets the initial count of the newly constructed semaphore to the
        given value, which may be negative.
    */
    public UnfairSemaphore(int initial) {
        count = initial;
    }

    /** Construct a semaphore with 1 initial permit.
     */
    public UnfairSemaphore() {
        this(1);
    }

    // Implementation
    public void acquire() throws InterruptedException {
        synchronized (wire) {
            while (count <= 0) {
                wire.wait();
            }
            --count;
        }
    }

    // Implementation
    public void release() {
        synchronized (wire) {
            ++count;
            if (count > 0) {
                wire.notifyAll();
            }
        }
    }
}

Semaphore is an interface with just the two methods acquire and release.

I'm using the member wire to synchronize on and to call notifyAll and wait in order to keep the signals resulting from these "behind the abstraction", that is to make them invisible to the code using the semaphore. (I could use just notify here, couldn't I?)

I'm neither interested in making this code faster or to use different, possibly better classes (Atomic* or whatever) to implement this.

What I do want is code that correctly handles all cases. (I just don't want to give them "educational code" which shows the idea, but fails to reliably work in practice)

I don't like using the object as a mutex and Condition variable. Doing that would let another programmer spin up a thread to lock and never release the Semaphore.

Instead you should either provide a private Object lock = new Object(); and sync on that or use the java.util.concurrent.locks.ReentrantLock and its condition:

private final Lock mutex = new ReentrantLock();
private final Condition condition = mutex.newCondition();

public void acquire() throws InterruptedException {
    mutex.lockInterruptibly();//let interrupt throw as soon as possible
    try{
        while (count <= 0) {
            condition.await();
        }
        --count;
    } finally {
        mutex.unlock();
    }
}

// Implementation
public void release() {
    mutex.lock();
    try{
        ++count;
        if (count > 0) {
            condition.signal();
        }
    } finally {
        mutex.unlock();
    }
}

This makes it easy to change the semaphore to a fair one by passing true to the constructor.

Though I expect you will want to implement the wait queue yourself as an example.

If you want to go a step lower and no use the pre-implemented locks can instead use LockSupport which lets to park the current thread and unpark it from another thread safely.


Virtual machine using RPython and PyPy I'm writing a virtual machine in Python using RPython and the PyPy toolchain. The RPython will still work in the ordinary Python 2 interpreter; it's just a bit slow unless it's compiled to C code with PyPy.

Does anyone have any positive or negative feedback about my VM?

The virtual machine has 4 instructions:

EOP - End of Program
EOI - End of Instruction
PUSH - Push item onto stack
PRINT - Print the item at the top of the stack

bcode = []
stack = []
regs = []
sp = 0
bcd = []

'''
Instructions
'''
OP_EOP = 0
OP_EOI = 1
OP_PUSH = 2
OP_PRINT = 3

def load_program(f2o):
    f = open(f2o, "r")
    f2 = f.read()
    f2 = f2.replace("\n"," ")
    bcode = f2.split(" ")
    i = 0
    for item in bcode:
        item = int(item, 16)
        bcd.append(item)
        i += 1
    return bcd

# VM action functions
def do_EOP():
    print "End of Program"

def do_PUSH(ba, b, ip):
    i = 2
    loop = 1
    cb = len(b)

    stack.insert(0, ba)

def do_PRINT(stack):
    stk = stack[0].split(" ")
    for item in stk:
        print unichr(int(item))

def execute_program(b):
    ip = 0
    sp = 0
    loop = 1
    cb = len(b)
    while (loop):
        if ip < cb:
            bc = b[ip]
            if bc == OP_PUSH:
                if bc != OP_EOI:
                    ba = str(b[ip + 1])
                    do_PUSH(ba, b, ip)
            elif bc == OP_PRINT:
                do_PRINT(stack)
            ip += 1
        else:
            loop = 0

def run_program(f):
    b = load_program(f)
    execute_program(b)

def main(argv):
    run_program(argv[1])
    return 0

def target(*args):
    return main, None

if __name__ == '__main__':
    import sys
    main(sys.argv)

Here's a file that includes the bytecode that prints "Hello World!" to the screen:

0002 0048 0001 0003
0002 0065 0001 0003
0002 006C 0001 0003
0002 006C 0001 0003
0002 006F 0001 0003
0002 0020 0001 0003
0002 0057 0001 0003
0002 006F 0001 0003
0002 0072 0001 0003
0002 006C 0001 0003
0002 0064 0001 0003
0002 0021 0001 0003

You can test the VM like this; it's written in Python 2.7:

  python vm.py

bcode = []
stack = []
regs = []
sp = 0
bcd = []

Global variables like this are frowned upon. To be pythonic you should really put them a in a class or something.

'''
Instructions
'''
OP_EOP = 0
OP_EOI = 1
OP_PUSH = 2
OP_PRINT = 3

def load_program(f2o):

f2o? What in the world is that?

    f = open(f2o, "r")
    f2 = f.read()

My recollection of RPython is that it doesn't support this pattern. Are you sure it works in RPython?

    f2 = f2.replace("\n"," ")
    bcode = f2.split(" ")
    i = 0
    for item in bcode:
        item = int(item, 16)
        bcd.append(item)
        i += 1

What are you ding with i? You count it, but don't do anything with the value that you count.

    return bcd

# VM action functions
def do_EOP():
    print "End of Program"

def do_PUSH(ba, b, ip):

ba? b? pick variables names that let me know what they represent.

    i = 2
    loop = 1
    cb = len(b)

None of these last three lines do anything. They set variables local to the function when then gets thrown away.

    stack.insert(0, ba)

For a stack, we usually use the end of the list as the top of stack. That way you can simply append() and pop() the list. Its rather inefficient to insert at the begining.

def do_PRINT(stack):
    stk = stack[0].split(" ")
    for item in stk:
        print unichr(int(item))

def execute_program(b):
    ip = 0
    sp = 0
    loop = 1

Use True and False for true/false.

    cb = len(b)
    while (loop):

You don't need the ( or )

        if ip < cb:
            bc = b[ip]
            if bc == OP_PUSH:
                if bc != OP_EOI:

Are you expecting bc to change between those two lines?

                    ba = str(b[ip + 1])

You read the next piece of bytecode to push it, but you don't skip increment ip to account for it.

                    do_PUSH(ba, b, ip)

            elif bc == OP_PRINT:
                do_PRINT(stack)
            ip += 1
        else:
            loop = 0

def run_program(f):
    b = load_program(f)
    execute_program(b)

def main(argv):
    run_program(argv[1])
    return 0

def target(*args):
    return main, None

if __name__ == '__main__':
    import sys
    main(sys.argv)

The biggest issue with your code is that you have a lot of code that can't possibly do anything useful. Reading me leaves me to wonder if you just haven't cleaned up your code or are confused about the code actually works.


Flip coin Game in an Object oriented way I started out creating a simple game in an Object Oriented way to practice.

The rules are pretty simple

There are two players
You let one player choose if they want Heads or Tails.
You assign the leftover option to the other player.
You simulate a coin flip and say who won that game.

More than the logic, naming conventions etc. I was looking at the most object-oriented way of structuring code to implement this from this exercise.

I created three classes for this, the Player, Coin and CoinGame and an enum as well.

The Player Class

enum CoinOptions
{
    Heads, Tails
}

internal class Player
{
    public string playerName { get; set; }
    public CoinOptions coinOption { get; set; }
    public Player(string name)
    {
        playerName = name;
    }
    public CoinOptions chooseCoinOption()
    {

        //Simulate the user picking one option

        //Console.WriteLine("Chose a coin option");
        //Console.ReadKey();

        //Lets Assume they pick Heads
        coinOption = CoinOptions.Heads;

        return coinOption;
    }
    public void setCoinOption(CoinOptions opponent)
    {
        coinOption = opponent == CoinOptions.Heads ? CoinOptions.Tails : CoinOptions.Heads;
    }
    public bool didPlayerWin(CoinOptions winningFlip)
    {
        return winningFlip == coinOption;
    }
}

Coin class

class Coin
{
    private Random random;
    public Coin(Random rand)
    {
        random = rand;
    }
    public CoinOptions flipCoin()
    {

        double probability = random.NextDouble();
        return probability > 0.5 ? CoinOptions.Heads : CoinOptions.Tails;
    }
}

CoinGame Class

internal class CoinGame
{
    private List<Player> players = new List<Player>(Enumerable.Repeat(default(Player), 2));
    private Coin coin;
    private CoinOptions winningFlip;

    public CoinGame(Coin coinUsed, List<string> playerNames = null)
    {
        for (int i = 0; i < players.Count; i++)
        {
            var playerName = playerNames != null ? playerNames[i] : $"player{i}";
            players[i] = new Player(playerName);
        }
        coin = coinUsed;
    }

    public void playCoinFlip()
    {
        int chooseIndex = new Random().Next(2);
        int applyIndex = chooseIndex == 0 ? 1 : 0;
        var selectedOption = players[chooseIndex].chooseCoinOption();
        players[applyIndex].setCoinOption(selectedOption);
        winningFlip = coin.flipCoin();
    }
    public void gameResult()
    {
        foreach (var player in players)
        {
            if(player.didPlayerWin(winningFlip))
            {
                Console.WriteLine($"The player {player.playerName} won choosing {player.coinOption}");
            }
            else
            {
                Console.WriteLine($"The player {player.playerName} lost choosing {player.coinOption}");
            }
        }
    }

}

And the calling code

class Program
{
    static void Main(string[] args)
    {
        var coin = new Coin(new Random(12312));
        var coinGame = new CoinGame(coin);
        coinGame.playCoinFlip();
        coinGame.gameResult();
        Console.ReadKey();
    }
}

My main concern is if there is a better strategy to

Initialize the players in the game
Allowing a random user to choose their coin option while simultaneously setting the other user with the leftover option

Smurf naming convention

Link

internal class Player
{
    public string playerName { get; set; }

playerName is redundant since it's a preoperty of the Player class. Name is sufficient.

Assigning a coin option to a player

There are a few issues here, it's hard to separately address them.

Firstly, you're violating SRP by making your Player class responsible for UI interactions:

public CoinOptions chooseCoinOption()
{
    //Console.WriteLine("Chose a coin option");
    //Console.ReadKey();

    return coinOption;
}

I see where your confusion comes from. "The player chooses the side of the coin" is a correct statement. But, and this is an important distinction, this is not equivalent to "the Player class should handle the UI logic for choosing the side of the coin". The second statement is incorrect as it breaks SRP.

The Player class needs to be told what its option is (= an external caller sets the value). The decision logic for deciding which value to set is not part of the Player class' responsibility. It shouldn't care why or how a particular side was chosen, it should only care which side was chosen.

chooseCoinOption() needs to be removed.

Secondly, the setCoinOption(CoinOptions opponent) method's intention really irks me.

public void setCoinOption(CoinOptions opponent)
{
    coinOption = opponent == CoinOptions.Heads ? CoinOptions.Tails : CoinOptions.Heads;
}

As an aside, opponent is a really bad name for this variable. When reading your code I initially assumed opponent to be of the Player type. A better name for your parameter would've been opponentCoinOption.

You're adding in a lot of custom logic that does not simplify the code. Anyone who calls this method needs to know exactly what it does (= pick the other option), which means that it's a meaningless encapsulation. Calling the method does not meaningfully simplify the external caller's responsibility.

When removing the opponent parameter, you are left with a default set method. And since the property already has a public setter:

public CoinOptions coinOption { get; set; }

There is no point to also having a setter method. You need to remove one of the setters. Which one you remove is up to you. I prefer just using the default set; in this case.

Checking for victory

public bool didPlayerWin(CoinOptions winningFlip)
{
    return winningFlip == coinOption;
}

This method doesn't really provide anything. It's a wrapper around an equality check, and the value you're comparing (coinOption) is already publically accessible.

I'd remove this method altogether.

Secondly, I just want to point out that the Player class should not be determining what a Win is. A win is defined by the rules of the game, and the Player does not define the rules of the game. This is again violating SRP.

As a simple example, imagine if the rules of the game were changed so that you lose the game when your option is called. Suddenly, your method name conflicts with the rules of the game, which proves the point that your Player class somehow depends on the rules of the game you're playing, which means you're leaking your abstractions.

I know this is pedantic, but if you had named this method HasCoinOptionEqualTo(CoinOptions option), I would not have considered this an SRP vioaltion, because you are no longer checking for a win, you are simply checking an equality (which coincidentally just happens to be the win condition of the game you're playing).

Minor comments

 private List<Player> players = new List<Player>(Enumerable.Repeat(default(Player), 2));

I don't see you having defined a default for Player anywhere, which means that you're simply creating a list with two null values. What is the benefit of doing so? I suggest simply doing new List<Player>() and adding the players at a later stage.

However, I would take it further and do away with the list altogether. You will always have two players, and you're going to have to create logic to always retrieve the correct player from the list. It's much easier (believe me, I've built a few two player games before) if you just keep these as two separate properties:

private Player player1;
private Player player2;

class Coin { ... }

I see no added purpose to making an object out of Coin. This can be a simple static class:

public static class Coin
{
    private static Random random = new Random();

    public static CoinOptions Flip()
    {
        // ...
    }
}

internal class CoinGame
{
    public void playCoinFlip() { ... }

    public void gameResult() { ... }
}

Similar to mentioned before, don't mix your game logic with your UI logic. gameResult() should not be in here.

Instead, playCoinFlip() should return the outcome of the game (in whatever form you choose), and then the UI logic can choose to display the outcome however it wants.

This ties into how your main function works:

coinGame.playCoinFlip();
coinGame.gameResult();

Doing it this way requires the main function to call different methods in a particular order, which again requires the main method to know how the CoinGame works internally, thus infringing on your encapsulation.

It would be much cleaner if you could do something like:

Player winner = coinGame.playGame();

Console.WriteLine($"{winner.Name} won the coin flip!");

public void playCoinFlip()
{
    int chooseIndex = new Random().Next(2);
    int applyIndex = chooseIndex == 0 ? 1 : 0;
    var selectedOption = players[chooseIndex].chooseCoinOption();
    players[applyIndex].setCoinOption(selectedOption);
    winningFlip = coin.flipCoin();
}

What I find very weird is that you let the external caller (your main function) choose the names of the players, but you don't allow the external caller to choose the coin option that each player chooses.

I understand that since either choice is a 50/50, it doesn't really matter here. But if that is the case, then Player shouldn't even need a CoinOptions to begin with.

I would reshuffle this so your CoinGame receives two players, rather than two names. This gives your external caller to choose the coin options.

public CoinGame(Player p1, Player p2)
{
    player1 = p1;
    player2 = p2;
}


Test if a network is online by using urllib2 My concerns:

Is the code pythonic
Is the code practical

I tend on expanding the usefulness of this script, if you don't think it's stupid idea. One thing I will implement is command-line arguments by way of argparse.

#!/usr/bin/env python

'''
Test if network is online by sending and http request to a remote
web-server. I wrote this because my network connection sucks.
'''

import urllib2
import sys

def is_online(**kwargs):
    '''
    Determine weather or not your network is online
    by sending a HTTP GET request. If the network is
    online the function will return true. If the network
    is not online the function will return false
    '''

    # Set the default request time out.
    # This will be passed as a keyword argument
    # to urllib2.urlopen.
    kwargs.setdefault('timeout', 0.30)
    kwargs.setdefault('url', 'http://www.google.com')
    timeout = kwargs.get('timeout')
    url = kwargs.get('url')

    try:
        print 'Sending request to {}'.format(url)
        response = urllib2.urlopen(url, timeout=timeout)
    except KeyboardInterrupt:
        sys.exit(0)
    except Exception:
        # Generally using a catch-all is a bad practice but
        # I think it's ok in this case
        response = False
        print 'Request to {} failed'.format(url)
    if response:
        return True
    else:
        return False

def main():
    '''
    Continually check if the network is online.
    If the network is online stop checking.
    '''
    while True:
        if is_online():
            print 'Net is up'
            break
        else:
            print 'Net down.'

if __name__ == '__main__':
    main()

I would use Python's socket library instead of urllib. Then, you could test like this:

import socket
REMOTE_SERVER = "www.google.com"
def is_connected():
  try:
    host = socket.gethostbyname(REMOTE_SERVER)
    s = socket.create_connection((host, 80), 2)
    return True
  except:
     pass
  return False
print is_connected()

Borrowed from here

I think the most obvious thing to point out is this:

if response:
    return True
else:
    return False

You can actually just return the variable, so just return response would replace all those lines.

There's not really any reason to use .format for one variable.

'Sending request to {}'.format(url)

Plain string concatenation is fine, for one variable.

Anyway, what's the point of printing out that you're requesting google.com?

'Request to {} failed'.format(url)

The level of abstraction that you put into this, is that it'll return whether you're connected or not.

Printing out anything other than: 'Net is up' or 'Net down' (Which could better be: 'Net is down, retrying connection') seems superfluous.

kwargs.setdefault('timeout', 0.30)

I would avoid the magic numbers (undefined numbers: 0.30), define it as a constant (in all caps) to improve readability.

  Is this code Pythonic?

I'd recommend reading PEP8, Python's official style guide. It has many guidelines to follow on just about everything; it should really be a Python programmer's Bible. There's also a nice little Online PEP8 checker to help/show you where your code doesn't meet the standards of PEP8.

The term 'Pythonic' is not so easily defined, so I'm not really going to give you a 'Yes or No' answer, but, I'd just really recommend reading over PEP8.

  Is this code practical?

Yes, it's practical.

However, one thing more to point out; While you keep retrying and retrying:

Put a some kind of wait in between network connection tests.
Every time you call is_online, when the Net is down, you redo the url setup, consider keeping that seperate so you can only do the bare minimum for the continual tests:

kwargs.setdefault('timeout', 0.30)
kwargs.setdefault('url', 'http://www.google.com')
timeout = kwargs.get('timeout')
url = kwargs.get('url')


Find the index of a tuple in a list of tuples in Python I'm a Python beginner.
I have the following array of unique tuples I'm playing with:
array = [(1, 2), (1, 7), (1, 5), (2, 5), (2, 7), (2, 9), (3, 4), (3, 5), (4, 5), (4, 7), (5, 8), (6, 7), (8, 9)]

I want to find the index of a given tuple (u, v) in this array. I wrote the following line which I believe can be improved, probably from removing the enumerate() to get simpler if conditions but can't figure it out...
Example for the tuple (6, 7) - Inputs are: 6 and 7
array = [(1, 2), (1, 7), (1, 5), (2, 5), (2, 7), (2, 9), (3, 4), (3, 5), (4, 5), (4, 7), (5, 8), (6, 7), (8, 9)]
# Inputs: 6 and 7
position = [i for i, tupl in enumerate(array) if (tupl[0] == 6 and tupl[1] == 7)]
print(position)
# Output: [11]


Sorting random non-repeating numbers I am working on a program in which 10000 random non-repeating numbers are selectively sorted into ascending order"

import java.util.Random;

public class Sorting {
    public static void main (String[] args){
        //Here, we initiate an array with the integers
        //1 through 10,000 in order.
        Random rgen = new Random();
        int[] intArray = new int[10000];
        for (int i=0; i<10000; i++) {
            intArray[i] = i+1;
        }
        //Here, we randomize the positions.
        for (int i=0; i<10000; i++) {
            int randomPosition = rgen.nextInt(10000);
            int temp = intArray[i];
            intArray[i] = intArray[randomPosition];
            intArray[randomPosition] = temp;
        }
        //Here, we check for the maximum in the
        //unsorted section of the list.
        int max;
        for (int i=9999; i>=0; i--) {
            max = i;
            for (int j=i-1; j>=0; j--)
                if (intArray[j]>intArray[max]) {
                    max = j;
                }
            //Here, we swap the positions of the
            //maximum and the last value in the
            //unsorted section of the list.
            if (max != i) {
                int tmp = intArray[i];
                intArray[i] = intArray[max];
                intArray[max] = tmp;
            }
        }
        //Here, we output the result.
        for (int k=0; k<10000; k++) {
            System.out.println(intArray[k]);
        }
    }
}

I tested it and it works as it should. I would only like to remark a couple of things.

You may want to split your long commented function into shorter ones:

static int[] InitArray(int length){...}
static int[] SelectionSort(int[]arr){...}
static void Swap(int[]arr, int i, int j){...}
print the result in the main(), as you do know.

This makes your code easier to read and easier to reuse.

Another thing I'd remove is the if(max != i). I'd rather make the swap every time.  If you look carefully, there's small chance that you do this operation unnecessarily, and when you do do it, it won't harm you.

Note: If you keep doing the swap as you do now, you won't have any problem by removing the if. However, if you modify it to make the swap without a temp variable, it might go wrong.


Conways game of life in Haskell I started with Haskell some months ago, but didn't really use it since then. As a simple training I implemented Conways game of life. What/How could it be improved?

module Main where

import Control.Monad.State

data CellState = Dead | Alive deriving (Eq)
data Cell = Cell CellState Int deriving (Eq)
data Row = Row [Cell] Int deriving (Eq)
data Grid = Grid [Row]
data NextError = EmptyListError

type GridState a = State Grid a

instance Show Grid where
        show (Grid rows) = unlines $ map show rows

instance Show Row where
        show (Row cells n) = (show n) ++ ": " ++ (unwords $ map show cells)

instance Show Cell where
        show (Cell color _) = show color

instance Show CellState where
        show c = case c of
                Dead -> "_"
                Alive -> "#"

main::IO()
main = print $ run (execState initializeGrid (createGrid 20 20)) 100

run :: Grid -> Int -> [Grid]
run g n = scanl (\s f -> f s) g $ replicate n playRound

initializeGrid :: GridState ()
initializeGrid = do
        setPositionToColor 0 0 Alive
        setPositionToColor 1 1 Alive
        setPositionToColor 1 2 Alive
        setPositionToColor 2 0 Alive
        setPositionToColor 2 1 Alive
        setPositionToColor 4 4 Alive
        setPositionToColor 5 4 Alive
        setPositionToColor 6 4 Alive
        setPositionToColor 5 5 Alive
        setPositionToColor 5 4 Alive
        setPositionToColor 5 6 Alive

createGrid :: Int -> Int -> Grid
createGrid x y = Grid $ map createRow (take x [0,1..]) where
                        createRow = Row (map createCell (take y [0,1..]))
                        createCell = Cell Dead

setPositionToColor :: Int -> Int -> CellState -> GridState ()
setPositionToColor x y color = do
        grid <- get
        let rows = getRows grid
            cells = getCells (rows !! x)
            newState = Grid $ replaceListElement rows x $ Row (replaceListElement cells y $ Cell color y) x
        put newState

getCells :: Row -> [Cell]
getCells (Row c _) = c

getRows :: Grid -> [Row]
getRows (Grid r) = r

replaceListElement :: [a] -> Int -> a -> [a]
replaceListElement list position replacement =
        let (x, _:xs) = splitAt position list
        in x ++ replacement : xs

playRound :: Grid -> Grid
playRound (Grid rows) = Grid $ map (updateRow rows) rows

updateRow :: [Row] -> Row -> Row
updateRow rows row@(Row cells p) = Row (map (updateCell rows row) cells) p

updateCell :: [Row] -> Row -> Cell -> Cell
updateCell rows row cell@(Cell _ position) =
        let upperRowCells = getCells $ previous rows row
            upperCell = upperRowCells !! position
            lowerRowCells = getCells $ next rows row
            lowerCell = lowerRowCells !! position
            neighbourCells = [upperCell, lowerCell] ++
                getNextPrevious (getCells row) cell ++
                getNextPrevious upperRowCells upperCell ++
                getNextPrevious lowerRowCells lowerCell
            countLivingNeighbours = length $ filter (==Alive) $ map getCellColor neighbourCells
            isLiving = getCellColor cell == Alive
        in Cell (calcCellState isLiving countLivingNeighbours) position

getCellColor :: Cell -> CellState
getCellColor (Cell c _) = c

calcCellState :: Bool -> Int -> CellState
calcCellState living livingNeighbours
        | living && livingNeighbours < 2 = Dead
        | living && livingNeighbours <= 3 = Alive
        | living && livingNeighbours > 3 = Dead
        | not living && livingNeighbours == 3 = Alive
        | otherwise = Dead

getNextPrevious :: Eq a => [a] -> a -> [a]
getNextPrevious l e = [next l e, previous l e]

next :: Eq a => [a] -> a ->  a
next l@(x:_) e = case dropWhile (/= e) l of
                   (_:y:_) -> y
                   _ -> x

previous :: Eq a => [a] -> a -> a
previous l e = case takeWhile (/=e) l of
                [] -> last l
                x -> last x

A few things which came to my mind while reading your code:

The Show instance for CellState might be a bit nicer by not writing out the case expression manually but rather using

instance Show CellState where
    show Dead  = "_"
    show Alive = "#"

You could shorten initializeGrid by using mapM_:

initializeGrid :: GridState ()
initializeGrid = mapM_ (\(x, y) -> setPositionToColor x y Alive)
    [(0,0),(1,1),(1,2),(2,0),(2,1),(4,4),(5,4),(6,4),(5,5),(5,4),(5,6)]

take n [0,1..] is the same as [0,1..n] (looking at createGrid).
callCellState could be shortened by only explicitely testing the conditions determining whether the cell becomes (or remains) alive:

calcCellState :: Bool -> Int -> CellState
calcCellState living livingNeighbours
    | living && livingNeighbours `elem` [2,3] = Alive
    | not living && livingNeighbours == 3     = Alive
    | otherwise                               = Dead

On a more general note, seeing how long the updateCell function is and how you had to roll your own next and previous function I do suspect that lists are not the most appropriate data structure for this kind of problem. A vector may be easier to work with.


Encasing a string in double quotes I need to add some characters to a String before using it for an API call. The idea is to add a " char both at the beginning and at the end of the String, to make sure the API does an exact search for the input string.

Version 1:

StringBuilder(escapeJava(query))
                .insert(0, DOUBLE_QUOTE_CHAR)
                .append(DOUBLE_QUOTE_CHAR)
                .toString();

Version 2:

String.format("%c" + escapeJava(query) + "%c", DOUBLE_QUOTE_CHAR, DOUBLE_QUOTE_CHAR)

I prefer the readability of the first one, but I don't think it will be as quick as the second version, and might look like overkill.

Version 1 uses insert(0, ...) which has to move over all the characters already in the StringBuilder's buffer, so is unnecessarily wasting time.

A more efficient construct would be:

StringBuilder().append(DOUBLE_QUOTE_CHAR)
               .append(escapeJava(query))
               .append(DOUBLE_QUOTE_CHAR)
               .toString();

Version 2 uses StringBuilder under the hood, to construct "%c" + escapeJava(query) + "%c", so your belief the second version is faster may be in error.

Which leads us to the most efficient, most readable version:

DOUBLE_QUOTE_CHAR + escapeJava(query) + DOUBLE_QUOTE_CHAR;


"Prev" / "Next" buttons for a circular list I have an array of strings and on click of the "NEXT" button it displays the next array item in a
 tag while on click of the "PREV" button it displays the previous one. When it reaches to the end (beginning) it continues from the first (last) item.

I have had some hard time to find a way to get the right item index when PREV is clicked and this is what i could have come up with.

I've used a  modulo operator on decreasing array indices.  I would like to know if there is a better way of doing this. By the way ES6 syntax is just what I like so I am not interested in any ES5 compatibility improvements.

var message = ["dog", "cat", "bear", "penguin", "tiger", "eagle", "John Doe"],
        pel = document.getElementById("text"),
        idx = 0,
    getNext = e => pel.innerText = e.target.id == "right" ? message[idx = ++idx%message.length]
                                                          : message[idx = (message.length - (message.length - --idx)%message.length)%message.length];
document.getElementById("right").onclick = getNext;
document.getElementById("left").onclick = getNext;
<!DOCTYPE html>

    <link rel="stylesheet" href="style.css">

dog

   <button id="left" style = "width: 50px; background-color:gold" onclick="getNext">PREV</button>
   <button id="right" style = "width: 50px; background-color:tomato" onclick="getNext">NEXT</button>

I recommend splitting up the work into one function that is pure and another one that has side-effects.

const resultBox = document.getElementById('result')
const messages = ["cat", "dog", "fish"];
const length = messages.length;

const getNextIdx = (idx = 0, length, direction) => {
   switch (direction) {
     case 'next': return (idx + 1) % length;
     case 'prev': return (idx == 0) && length - 1 || idx - 1;
     default:     return idx;
   }
}

let idx; // idx is undefined, so getNextIdx will take 0 as default
const getNewIndexAndRender = (direction) => {
     idx = getNextIdx(idx, length, direction);
     result.innerHTML = messages[idx]
}

getNewIndexAndRender();

<button onclick="getNewIndexAndRender('prev')">prev</button>
<button onclick="getNewIndexAndRender('next')">next</button>

Note: getNextIdx is a pure function, which will never modify any value, it takes the arguments it needs and returns a new value, we pass into it the currentIdx, the lengthOfTheArray and an action which lets the function decide, what to do.


Dynamic field performance A have a model with a dynamic field like so:

def resolveClient() {
    if (prevCall && prevCall.client) {
        return prevCall.client
    } else {
        return client
    }
}

Pretty simple. However, using a dynamic field makes me unable to do queries at the database level. I can't query using HQL or Criteria. This seems to be impacting performance pretty heavily. What's a better method to do something like this?

In order to resolve the client with a GORM/Hibernate query you basically need to persist the resolved client value.

Based on your resolveClient() method I'm assuming your domain class model goes something like this:

class SomeDomainClass {
    Call prevCall
    Client client

    def resolveClient() {
        if (prevCall && prevCall.client) {
            return prevCall.client
        } else {
            return client
        }
    }
}

class Call {
    Client
}

class Client { }

If you were dealing with two properties that were in the SomeDomainClass then you'd probably be able to use a derived property. But since one of the properties in an association, a derived property won't work. Instead, you can do this:

class SomeDomainClass {
    Call prevCall
    Client client
    Client resolvedClient

    def resolveClient() {
        prevCall?.client ?: client
    }

    def beforeInsert() {
        resolvedClient = resolveClient()
    }

    def beforeUpdate() {
        resolvedClient = resolveClient()
    }
}

The new property resolvedClient, which is maintained by the beforeInsert() and beforeUpdate() methods, takes care of saving the current resolved client to the GORM store (database). With that value persisted you can use it in GORM queries:

def instances = SomeDomainClass.where {
    resolvedClient == someClient
}.list()

On the Groovy side, it would still be best to use the resolveClient() method because the resolvedClient property can get out of sync if prevCall changes.

def client = instance.resolveClient()


K-means clustering algorithm in python Here is my implementation of the k-means algorithm in python. I would love to get any feedback on how it could be improved or any logical errors that you may see. I've left off a lot of the boilerplate code like the command line argument parsing, error handling for data read in from CSV file, etc. and just added the meat of the algorithm.

import os
import numpy as np

# kmeans clustering algorithm
# data = set of data points
# k = number of clusters
# c = initial list of centroids (if provided)
#
def kmeans(data, k, c):
    centroids = []

    centroids = randomize_centroids(data, centroids, k)

    old_centroids = [[] for i in range(k)]

    iterations = 0
    while not (has_converged(centroids, old_centroids, iterations)):
        iterations += 1

        clusters = [[] for i in range(k)]

        # assign data points to clusters
        clusters = euclidean_dist(data, centroids, clusters)

        # recalculate centroids
        index = 0
        for cluster in clusters:
            old_centroids[index] = centroids[index]
            centroids[index] = np.mean(cluster, axis=0).tolist()
            index += 1

    print("The total number of data instances is: " + str(len(data)))
    print("The total number of iterations necessary is: " + str(iterations))
    print("The means of each cluster are: " + str(centroids))
    print("The clusters are as follows:")
    for cluster in clusters:
        print("Cluster with a size of " + str(len(cluster)) + " starts here:")
        print(np.array(cluster).tolist())
        print("Cluster ends here.")

    return

# Calculates euclidean distance between
# a data point and all the available cluster
# centroids.
def euclidean_dist(data, centroids, clusters):
    for instance in data:
        # Find which centroid is the closest
        # to the given data point.
        mu_index = min([(i[0], np.linalg.norm(instance-centroids[i[0]])) \
                            for i in enumerate(centroids)], key=lambda t:t[1])[0]
        try:
            clusters[mu_index].append(instance)
        except KeyError:
            clusters[mu_index] = [instance]

    # If any cluster is empty then assign one point
    # from data set randomly so as to not have empty
    # clusters and 0 means.
    for cluster in clusters:
        if not cluster:
            cluster.append(data[np.random.randint(0, len(data), size=1)].flatten().tolist())

    return clusters

# randomize initial centroids
def randomize_centroids(data, centroids, k):
    for cluster in range(0, k):
        centroids.append(data[np.random.randint(0, len(data), size=1)].flatten().tolist())
    return centroids

# check if clusters have converged
def has_converged(centroids, old_centroids, iterations):
    MAX_ITERATIONS = 1000
    if iterations > MAX_ITERATIONS:
        return True
    return old_centroids == centroids

Avoid comments that are identical to the code:

# check if clusters have converged  <-- Remove this
def has_converged(centroids, old_centroids, iterations):

MAX_ITERATIONS = 1000

Constants should be put at the top of the file to be easier to tweak.

Separate calculations from IO, your function kmeans should return the values and then another function (maybe pretty_format_k_means) should create a human readable message.

# k = number of clusters
# c = initial list of centroids (if provided)

Multi-character variable names are allowed, rename your variables (and function arguments) to more meaningful names, and then you can delete the comments.


Filtering JSON objects from array of values My code works, but it seems to be running exceptionally slow.

I have an array of values to search, and I have a JSON that I'm filtering.

var JSONPull;
var FilterJSON = [];
var TypeIDs = [34,35,36,37,38,39,40,11399,1230,17470,17471,1228,17463,17464,1224,17459,17460,18,17455,17456,1227,17867,17868,20,17452,17453,1226,17448,17449,21,17440,17441,1231,17444,17445,1229,17865,17866,1232,17436,17437,19,17466,17467,1225,17432,17433,1223,17428,17429,22,17425,17426,11396,17869,17870];

fetch('URL API')
    .then(res => res.json())
    .then((out) => {
        JSONPull = out;
        TypeIDs.forEach(function (index){
            FilterJSON = JSONPull.filter((element) => {
                console.log("index: "+index);
                console.log(element);
                console.log("type_id: "+ element.type_id);
                element.type_id === index;
            });
        })
    })

The console.logs are more just to watch the code while testing, but definitely shouldn't be causing these performance issues.

Thanks in advance for any help/assistance

You are basically making your object filtering into an \$0(m * n)\$ complexity function ( where \$m\$ is number of type id’s) that your are filtering against and \$n\$ is number of objects in the fetched result set.   This function should only be \$0(n)\$.

To do this, make type id’s into a Set which allows \$O(1)\$ lookup and then use this Set to filter the fetched objects.

For example:

const typeIds = new Set([ /* ids here */ ]);
const isFilteredType = (element) => typeIds.has(element.type);

// and later when filtering results
.then( (out) => out.filter(isFilteredType) );

It is also customary in javascript to use lowercase first letter for variable names. Your code would look odd to most JS developer for this reason.

There is really not much of a reason to use var in modern JavaScript.  You should primarily use const with let being used where the variable needs to be reassignable.

It is not really clear here why JSONpull and FilterJSON are even needed.


AJAX call to make an HTTP request I am using node.js with express. There is a button that the user can click, and doing so calls this method on the server:

app.get('/getTime', function(req, res) {

    var url = 'http://www.ctabustracker.com/bustime/api/v1/gettime';
    http.get(url + '?key=' + apikey, function(resp) {

        var xml = '';
        resp.on('data', function(chunk) {
            xml += chunk;
        });

    resp.on('end', function() {
            console.log(xml);
            res.send(xml);
        });

    });

});

This works, strictly speaking, in that the client side does receive the XML data. What bothers me is that there are two response objects that I have only dealt with by calling one "res" and the other "resp". I have a feeling I am handling the callbacks the wrong way, or that the entire structure of this task is done incorrectly.

I would appreciate some guidance about the "right way" to do this, as I am sure this is not it.

Since the HTTP response implements Writable Stream and Express wraps around HTTP, you can pipe the output to the response, like so:

app.get('/getTime', function(req, res) {
    var url = 'http://www.ctabustracker.com/bustime/api/v1/gettime';

    http.get(url + '?key=' + apikey, function(incoming) {
        incoming.pipe(res);
    }
});

It will not only cut down on latency times but also cut down on lines of code.


FormArray total of each formgroup via RxJS Stackblitz - https://stackblitz.com/edit/angular-plp4rb-formarray-total-prs9ay

I'm new to RxJS observables.

I've created a form array where the total for a line is quantity * rate. Via valueChanges for the line form group I set the total via the calcTotal method.

On form changes I emit all the lines up to parent to calculate the total.

Questions:

Do I have any memory leaks?
Why is it if I stick debounceTime(1) inside the lineItem valueChanges then this stops working where the calculation is out by the last change?

    lineItem.valueChanges.pipe(
      takeUntil(this.destroyed$),
      debounceTime(1)
    ).subscribe(value => this.calcTotal(lineItem))

Why is it if I remove the {emitEvent: false} and put in a debounceTime as above the total stops working altogether?
Should i be using some other way like combineLatest? If so how do I collect the valueChanges observables (I'll figure it out from here if this is required)?

import { Component, OnInit, Input, Output, EventEmitter, ViewChild } from '@angular/core';
import { FormArray, FormBuilder, FormControl, FormGroup } from '@angular/forms'
import { Subject } from 'rxjs'
import { takeUntil, debounceTime} from 'rxjs/operators'
import { MatTable } from '@angular/material';

import { Line } from '../app.component';

@Component({
  selector: 'app-total-calculator',
  templateUrl: './total-calculator.component.html',
  styleUrls: ['./total-calculator.component.css']
})
export class TotalCalculatorComponent implements OnInit {
  displayedColumns = ['quantity', 'rate', 'total'];
  formGroup: FormGroup;

  @Input() lines: Line[];
  @Output() linesChange = new EventEmitter<Line[]>();
  @ViewChild(MatTable) private matTable: MatTable<any>;

  get linesArray(): FormArray {
   return this.formGroup.get('lines') as FormArray;
  }

  private destroyed$ = new Subject<void>();

  constructor(private fb: FormBuilder) {
   this.buildForm();
  }

  ngOnDestroy() {
   this.destroyed$.next();
   this.destroyed$.complete();
  }

  addLineClick() {
    this.addLineFormGroup();
  }

  addLineFormGroup() {
    this.linesArray.push(this
      .getLineFormGroup());
    this.matTable.renderRows();
  }

  private buildForm() {
     // formarray
     this.formGroup = this.fb.group({
       lines: this.fb.array([])
     });

     // subscribe to any changes and emit these
     this.formGroup.valueChanges.pipe(
       takeUntil(this.destroyed$),
       debounceTime(300)
     ).subscribe(value => {
        if (!this.formGroup.valid) {
          return;
        }
        this.linesChange.emit(value['lines']);
    });
  }

  getLineFormGroup(): FormGroup {
    let lineItem = this.fb.group({
      quantity: new FormControl(),
      rate: new FormControl(),
      total: new FormControl()
    });
    lineItem.valueChanges.pipe(
      takeUntil(this.destroyed$),
    ).subscribe(value => this.calcTotal(lineItem))
    return lineItem
  }

  calcTotal(line: FormGroup) {
    const quantity =  +line.controls['quantity'].value;
    const rate =      +line.controls['rate'].value;
    line.controls['total'].setValue((quantity * rate).toFixed(2), {emitEvent: false});
  }

  ngOnInit() {
  }

}
```

Do I have any memory leaks?

No

Why is it if I stick debounceTime(1) inside the lineItem valueChanges then this stops working where the calculation is out by the last change?

  Beacuse debounceTime(1) delays the response by 1ms inside the lineItem
  and this.linesChange.emit(value['lines']);  is already run with old
  total

The implementation below will sort your problems.

  getLineFormGroup(): FormGroup {
    let lineItem = this.fb.group({
      quantity: new FormControl(),
      rate: new FormControl(),
      total: new FormControl()
    });
    lineItem.valueChanges.pipe(
      takeUntil(this.destroyed$),
      debounceTime(20)
    ).subscribe(value =>
      {
        console.log("should be called first")
        this.calcTotal(lineItem)

        if (!this.formGroup.valid) {
          return;
        }
        this.linesChange.emit(this.formGroup.value['lines']);
      }
    )
    return lineItem
}

Why is it if I remove the {emitEvent: false} and put in a debounceTime as above the total stops working altogether?

  The lineItem.valueChanges observable will fire in a circular fashion
  when you remove this {emitEvent: false}.
  line.controls['total'].setValue() will fire lineItem.valueChanges
  again. Which in-turn crashes the app and stop.

Should i be using some other way like combineLatest? If so how do I collect the valueChanges observables (I'll figure it out from here if this is required)?

  I would suggest not required


Get a number from a class of a HTML element I've got an element with multiple classes in a page and I need to dynamically get the number at the end of a specific class name.

Example:

    ...

In particular I've to get "3" from gallery-columns-3.

I've made this function in Javascript/Jquery, but I think that there is a better solution.

//return the number of columns from gallery class
function get_gallery_columns($el){
    var class_arr = $el.attr('class').split(/\s+/).filter(function (el) {
        return el.includes("gallery-columns-");
    });

    if(class_arr.length < 1){
        return false;
    }

    return parseInt(class_arr[0].replace(/\D/g,''));
}

I wanted to do this to dynamically create the layout of Wordpress galleries.

$(document).ready(function() {

//galleries selector
var $galleries = $('div[class*=gallery-columns-]');
fix_gallery($galleries);

//responsive gallery on resize
$(window).on('resize', function(){
    fix_gallery($galleries);
});

//fixes galleries size
function fix_gallery($galleries){
    $galleries.each(function(index) {
        var $current_gall = $(this);
        var n_cols = get_gallery_columns($current_gall);

        if(n_cols === false){
            return;
        }

        //if there are columns, unwrap images from them
        $current_gall.find(".gallery-group").replaceWith(function() { return $(this).contents(); });

        //change columns on mobile/tablet
        if (window.matchMedia("(max-width: 640px)").matches && n_cols > 2) {
            n_cols = 2;
        } else if (window.matchMedia("(max-width: 800px)").matches && n_cols > 4) {
            n_cols = 4;
        } else if (window.matchMedia("(max-width: 980px)").matches && n_cols > 6) {
            n_cols = 6;
        }

        //gallery pictures selector
        var $pictures = $current_gall.find('.gallery-item');
        //number of pictures
        var pic_number = $pictures.length;

        for (var i = 0;i <= pic_number; i += n_cols){
            $pictures.filter(':eq('+i+'),:lt('+(i+n_cols)+'):gt('+i+')').wrapAll('
');
        }

        $pictures.width((100/n_cols) + '%');

    });
}

//return the number of columns from gallery class
function get_gallery_columns($el){
    var class_arr = $el.attr('class').split(/\s+/).filter(function (el) {
        return el.includes("gallery-columns-");
    });

    if(class_arr.length < 1){
        return false;
    }

    return parseInt(class_arr[0].replace(/\D/g,''));
}

});

You might use something like the snippet below, that is:

sure a bit shorter
probably faster, because:

using find() rather than filter() makes the loop stop as soon as the desired result is got
looking for the involved class-prefix and extracting the number are both processed in the same statement

and browser agnostic, while includes() is not recognized by IE

function get_gallery_columns($el) {
  var matches, result = false;
  $el.attr('class').split(/\s+/).find(function (className) {
    if (matches = className.match(/^gallery-columns-(\d+)$/)) {
      result = +matches[1];
      return true;
    }
  });
  return result;
}

console.log(get_gallery_columns($('#gallery-1')));
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<div id="gallery-1" class="gallery galleryid-555 gallery-columns-3 gallery-size-thumbnail">
    ...
</div>


Convert Roman Numerals to integers I am a beginner, and so I have decided to program a Roman Numeral to Arabic integer converter. I came up with all of the code on my own.
There are differing opinions about what constitutes 'proper' Roman Numerals. For example, 'IIII' is odd but known to exist in ancient writings, and 'VIIV' is a silly but parsable way to write '8'.
And so my program will treat any valid string as acceptable and attempt to parse it.
It works and I would welcome any comments or considerations, especially with regards to the following:

general best practice,
whether the input verification can be made cleaner,
if there is a nicer way than using a dictionary to match the characters and values,
efficiency of the part which calculates the totals, and
if I should add comments to the code.

import re

bad = 1

while bad == 1:
    roman = input('Enter a Roman Numeral: ')
    test = re.findall('[^IVXLCDM]+', roman)
    lengthtracker = 0
    for item in test:
        length = len(item)
        lengthtracker = lengthtracker + length
        if length != 0:
            print('Roman Numerals may contain only the characters I, V, X, L, C, D, M')
            break
        else:
            continue
    if lengthtracker == 0:
        bad = 0

print('Roman Numeral verified!')

ref = dict()
ref['I'] = 1
ref['V'] = 5
ref['X'] = 10
ref['L'] = 50
ref['C'] = 100
ref['D'] = 500
ref['M'] = 1000

n = len(roman)
m = 0
total = 0
negtotal = 0
tenttotal = 0
while m < n-1:
    if ref[roman[m]] == ref[roman[m+1]]:
        tenttotal = tenttotal + ref[roman[m]]
    elif ref[roman[m]] > ref[roman[m+1]]:
        total = total + tenttotal + ref[roman[m]]
        tenttotal = 0
    else:
        negtotal = negtotal + tenttotal + ref[roman[m]]
        tenttotal = 0
    print(total, negtotal, tenttotal)
    m = m + 1

total = total + tenttotal + ref[roman[m]] - negtotal

print('It equals:', total)

Dict initialization and reuse
This:
ref = dict()
ref['I'] = 1
ref['V'] = 5
ref['X'] = 10
ref['L'] = 50
ref['C'] = 100
ref['D'] = 500
ref['M'] = 1000

should use a dict literal:
ref = {
    'I': 1,
    'V': 5,
    'X': 10,
    'L': 50,
    'C': 100,
    'D': 500,
    'M': 1000,
}

You should declare this at the top of the program, so that this:
'[^IVXLCDM]+'

can become
'[^' + ''.join(ref.keys()) + ']+'

and this
'Roman Numerals may contain only the characters I, V, X, L, C, D, M'

can become
'Roman Numerals may contain only the characters ' + ', '.join(ref.keys())

No-op continue
This:
    else:
        continue

has no effect and can be deleted.
Booleans
bad should use True and False rather than 1 and 0.


Super simple jQuery slider I created the most simple though still quite flexible jQuery slider ever! Or at least, I hope so.

var slider = $(".slider-ul");

slider.each(function () {

    var e = $(this),
        images = e.find("li"),
        current = null;

    slide();

    function slide() {

        images.each(function() {

            var li = $(this),
                next,
                pDone = false,
                sDone = false;

            if (li.hasClass("primary")) {
                li.removeClass("primary");
                pDone = true;
            } else if (li.hasClass("secondary")) {
                li.removeClass("secondary").addClass("primary");

                next = li.next();

                console.log(next);
                if (next.size()) {
                    next.addClass("secondary");
                    sDone = true;
                } else {
                    images.filter(":first").addClass("secondary");
                    sDone = true;
                }
            }

            if (sDone && pDone) {
                return false;
            }
        });

        setTimeout(slide, 5000);

    }
});

How does this look?

In this code, you set sDone = true in both branches of the if-else:

if (next.size()) {
    next.addClass("secondary");
    sDone = true;
} else {
    images.filter(":first").addClass("secondary");
    sDone = true;
}

So you could move that line outside of the if-else:

if (next.size()) {
    next.addClass("secondary");
} else {
    images.filter(":first").addClass("secondary");
}
sDone = true;

At the top you declared the current variable,
but then you don't use it. If you don't need it, then remove it.


Cons of declaring derived class type member in another derived class (both of which share the same base class) I'd like some input on the way I've structured some things in a side-project I'm working on.

I declare an abstract class, CelestialObj, which will contain the common elements between Planets, and Moons (both of which I'd like to be derived from CelestialObj, obviously).

Now, a Planet must keep track of its Moon objects. Therefore, each of my Planet objects will contain a vector<Moon*> stack. I was wondering if this is bad practice or can cause me trouble later down the line? I can't really find much information regarding this issue on various forums/google searches. Here's a SCCCE version of the code:

class CelestialObj {
protected:
    int size;
    //virtual function definitions here, imaging they're in Moon/Planet too
};

class Moon : public CelestialObj {
public:
    int distFromPlanet;
    Moon() : distFromPlanet(10), size(1000) { }
};

class Planet : public CelestialObj {
public:
    vector<Moon> moons;
    Planet() : size(2000) { Moon mun; moons.push_back(mun); }
};

The actual code is much more organized, with members declared protected/private where appropriate, but this conveys the intended logic.

Is there anything that stands out as dangerous? Or that should be avoided? Minor things such as size being const, and public/private/protected declarations should not be discussed, as they're fine in my actual code. It's just the organization I'm kind of worried about. Is there an alternate organization method anyone recommends? If so, why?

The code itself
I have little to say about the code itself, other than to address your concerns with the vector<Moon> in your Planet class. If you have a C++11 compiler, you might want to add a move constructor and/or provide methods that exploit the move semantics as much as possible. Something like:

class Planet : public CelestialObj
{
    public:
        Planet & add_moons  (vector<Moon>&& vMoon){};
        Planet & add_moon  (Moon&& moon){};//copy push_back
};

The rest of this answer is more about the limitations of your model, and some little niggles I have with a couple of things you mentioned.

At first glance, I don't see any glaring omissions or down-right dangerous code. However, I am a bit of a space-geek at times, and I find the name of your base class a tad risky/counter intuitive.
To me a CelestialObj could be anything ranging from a Planet, a Moon, some Asteroid, Meteorite Planetoids and DwarfPlanets, all the way through to a Star, Nebula, BlackHole, and even things like Galaxy or a Constellation could be considered a celestial object.

Because I don't know what you are going to use these classes for, and not knowing how detailed you want to go, I'm just going to point out a couple of things that you might want to keep in the back of your head.
It's easier to change your approach in the earlier stages, than it will be further down the line.

Of course, a Galaxy is but a CelestialObj that contains a vector<CelestialObj>, mainly Star instances. Each Star can contain a similar vector, representing solar systems and so on.
A Planetoid could be implemented as a specific type of Asteroid, just like a DwarfPlanet is a variation on the Planet class. Great, all this can be made to fit your current hierarchy perfectly, but I would change one thing:

Currently, your Planet class has a vector<Moon> property. Given that there can be objects in orbit around moons, planets can have other objects in its orbit (ie Saturn's rings etc), and that, well, basically: anything can revolve around anything, I'd move the vector up to the CelestialObj class, and make it more generic (vector<CelestialObj> satellites;).

This should allow you to fit more into your current model, but the moment you add bigger objects, or you want to create more complex systems, you'll find it simply can't cope with the added complexities.

When you add nebula's and black holes to the mix, then things become quite tricky, of course, as things like luminescense aren't simple properties.
Forgive my geeky-ness here, but computing the brightness and luminosity of stars is fairly simple, but a galaxy, well: the sum of all celestial bodies isn't quite enough. There's reflection to take into consideration, and black holes trap light in its orbit, aswell as bend the light or even absorb it.

Note:
Now I know you didn't want to discuss minor things, but if ever you extend the CelestialObj class to create a Star, you'll find that a const int size and const int mass is as pointless as a broken pencil. Either way, in terms of celestial bodies: I'd consider it acceptable to use a bigger datatype than a mere int: I'd use a double.
To fix this, you might choose to create a class like this:

class Star : public CelestialObj
{};
class DyingStar : public Star
{
    public://assume double size;
        DyingStar (Star &dead_star_shining, long ttl){};
        Star &pass_time ( long time ){};//heat up planets, swallow closest, alter size_factor
        double getSize( void ) { return size * size_factor; };
    protected:
        double size_factor;
}

Other things that I'm not even going to touch on include neutron stars, wormholes, binary star systems, the list is endless...

Again, this isn't as much a critique of your actual code. This "review" is meant more as a sort of "consider this, how would you fit it into your current model?" type of deal.

In response to comments:
You probably realized that you've chosen a hideously complex thing to try and represent in a relatively simple hierarchical structure. I would, however advise you to not try and use the CelestialObj base class as the only base-class, to extend on to represent well, the entire universe basically.
If you did that, then the only suitable class name would be class BigBang, which, by definition, would have to use multiple inheritance, inheriting both from the Quantum, Matter, DarkMatter, Space and tons of other classes, and would have to implement the Chaos interface. Depending on who you ask, you'd probably have to use the God template, too. Save yourself the trouble, and just use a set of base classes:

CelestialObj is fine, and can be split up into 2 main groups: ReflectingObj (ie: bodies that don't emit light: planets, moons, asteroids) and FusionObj (suns in various stages, including dying suns, neutron stars, pulsars and... The core of a black hole. Everything that is either the remains of a star or that consumes light/celestial bodies).
Cluster: A base class that is either vector-like or doubly linked-list like you can feed CelestialObj instances to, to form SolarSystems, Galaxy's and the actual BlackHole class (because black holes can have light in orbit around it. Light has dual properties (energy and matter, so if you want: create a separate class for this tricky little bugger)
Special cases of Clusters include, for example, Constellations, as they can be a group of stars, combined with galaxies (ie the constellation Orion). The members of a constellation might be too far apart for any of its members to influence the other, so this cluster doesn't have to provide methods for member-interaction, but it should be able to group instaces of CelestialObj alongside Cluster isntances.


Pairs with Specific Difference python Given an array arr of distinct integers
  and a nonnegative integer k, write a function
  findPairsWithGivenDifference that returns an array of all pairs [x,y]
  in arr, such that x - y = k. If no such pairs exist, return an empty
  array.

  In your solution, try to reduce the memory usage while maintaining
  time efficiency. Prove the correctness of your solution and analyze
  its time and space complexities.

  Note: the order of the pairs in the output array should maintain the
  order of the y element in the original array.

  Examples:

  input:  arr = [0, -1, -2, 2, 1], k = 1 output: [[1, 0], [0, -1], [-1,
  -2], [2, 1]]

  input:  arr = [1, 7, 5, 3, 32, 17, 12], k = 17

  output: []

  Constraints:

  [time limit] 5000ms

  [input] array.integer arr

  0 ≤ arr.length ≤ 100 [input]integer k

  k ≥ 0 [output] array.array.integer

def find_pairs_with_given_difference(arr, k):
    numbers = set()
    output = []
    # insert arr element into set
    for i in range(len(arr)):
        numbers.add(arr[i])

    # loop through the entire array
    for i in range(len(arr)):
        difference = arr[i]
        if difference - k in numbers:
            output.append([difference,(difference - k)])
    return output

Test case #1
Input: [4,1], 3
Expected: [[4,1]]
Actual: [[4, 1]]

Test Case #2
Input: [1,5,11,7], 4
Expected: [[5,1],[11,7]]

The code in the post (and in WolframH's answer) has a bug:

>>> find_pairs_with_given_difference([1], 0)
[[1, 1]]

There is no pair of items [1, 1] in the input array.


Repository and service improvements I have a MongoRepository:
@Repository
public interface GooglePlayGameRepository extends MongoRepository<GooglePlayGame, String> {

    Optional<GooglePlayGame> findByTitle(String title);

    List<GooglePlayGame> findByTitleContainsIgnoreCase(String title);

    @Aggregation("{$sample: {size: ?0} }")
    List<GooglePlayGame> findRandomGames(Long number);

}

And service:
@Service
public class GooglePlayGameService {

    private final GooglePlayGameRepository googlePlayGameRepository;

    public GooglePlayGameService(GooglePlayGameRepository googlePlayGameRepository) {
        this.googlePlayGameRepository = googlePlayGameRepository;
    }

    public void saveToLibrary(GooglePlayGame googlePlayGame) {
        googlePlayGameRepository.save(googlePlayGame);
    }

    public GooglePlayGame getGameByTitle(String title) throws NoSuchGooglePlayGameFoundException {
        Optional<GooglePlayGame> googlePlayGame = googlePlayGameRepository.findByTitle(title);
        return googlePlayGame.orElseThrow(
                () -> new NoSuchGooglePlayGameFoundException("Game was not found: " + title)
        );
    }

    public List<GooglePlayGame> findByTitle(String title) {
        List<GooglePlayGame> googlePlayGames = googlePlayGameRepository.findByTitleContainsIgnoreCase(title);
        return googlePlayGames.isEmpty()
                ? Collections.emptyList()
                : googlePlayGames;
    }

    public Long getLibrarySize() {
        return googlePlayGameRepository.count();
    }

    public List<GooglePlayGame> getRandomGames(Long number) {
        return googlePlayGameRepository.findRandomGames(number);
    }

}

What do you think about methods getGameByTitle (returns exact match) and findByTitle (looking for similar matches by title)? Is it good practice to handle Optional in Service layer and just throws Exception? Or should I use Optional as a return type in Service layer too and handle it in other places which will use this service?
About findByTitle: is it necessary to use this construction?
googlePlayGames.isEmpty()
          ? Collections.emptyList()
          : googlePlayGames;

Maybe I should just check it in code like this (not in the service):
if (googlePlayGames.isEmpty()){
   log.error("Game {} doesn't exist in library", title);
}

I have another variant for these methods:
public GooglePlayGame getGameByTitle(String title) throws NoSuchGooglePlayGameFoundException {
    return findByTitle(title).stream()
             .findFirst()
             .orElseThrow(NoSuchGooglePlayGameFoundException::new);
 }

public List<GooglePlayGame> findByTitle(String title) {
     List<GooglePlayGame> games = googlePlayGameRepository.findByTitleContainsIgnoreCase(title);
     if (!games.isEmpty()) {
         return games;
     }
     return Collections.emptyList();
 }

Which one is better?

You don't need to make additional save method since save method already exist.

public void saveToLibrary(GooglePlayGame googlePlayGame) {
        googlePlayGameRepository.save(googlePlayGame);
    }

You have two methods one called findByTitle(String title) and findByTitleContainsIgnoreCase

To make other programmer understand whats going on maybe renaming findByTitleContainsIgnoreCase to findListOfGamesByTitlesince you are returning List and also it can be optional since games might not be there at all.
In your service call findListOfGamesByTitle just pass String (title.equalsIgnoreCase())

Why are you throwing exception at method level and also inside the body in getGameByTitle , do you want to pass that exception further ? there is no point doing it.

Yes, you should handle optional in the service layer because next layer is the constructor.

The sentence and the place you mentioned is not understood so please fix it a bit
i am referring to the following -> Maybe I should just check it in code like this (not in the service):


Rotate every letter in string by K places The challenge:
Given a string S and a fixed number K, rotate every letter in S by K places (if reach the end of the alphabet, go back to the beginning). Letters should remain the same case. Symbols should remain unencrypted.

Constraints:
K is between 0 and 100. S is a valid ASCII string without spaces, between 1 and 100 letters long.

Input:
The first line contains an integer, N , which is the length of the unencrypted string.
The second line contains the unencrypted string, S.
The third line contains the integer encryption key, K, which is the number of letters to rotate.

I'd appreciate any comments and suggestions as to how my code can be improved.

var lowerString = "abcdefghijklmnopqrstuvwxyz";
var lower = lowerString.split("");

var upperString = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
var upper = upperString.split("");

function main() {
    var n = parseInt(readLine());
    var s = readLine();
    var k = parseInt(readLine());

    printCipher(s, k);
}

function printCipher(str, key){
    var newStr = "";
    str.split("").forEach(function(elem, index){
        var changedItem = cipher(elem, key);
        newStr += changedItem;
    });
    console.log(newStr);
}

function cipher(item, key){
    var arrToUse = null;
    if(lower.indexOf(item) >= 0){
        arrToUse = lower;
    }
    else if (upper.indexOf(item) >= 0){
        arrToUse = upper;
    }
    if(arrToUse !== null){
        var pos = arrToUse.indexOf(item);
        var newPos = getCipherPosition(pos, key);
        return arrToUse[newPos];
    }
    return item;
}

function getCipherPosition(pos, key){
    const ALPHA_COUNT = 26;
    if(key > ALPHA_COUNT){
        key = key % ALPHA_COUNT;
    }

    if(pos + key >= ALPHA_COUNT){
       return key + pos - ALPHA_COUNT;
    }
    return key + pos;
}

if the only thing you need to encrypt in the string are the letters, you can be more concise by using only one loop doing something like:

if c >= 'a' && c <= 'z' {
    a = (((c-'a')+K)%26)+'a'
} else if c>= 'A' && c <= 'Z' {
    a = (((c-'A')+K)%26)+'A'
} else {
    a = c
}

with c being the char from the original string and a the char to add to the encrypted string.


Fibonacci using cache This is my simple Fibonacci implementation in CoffeeScript. What do you think?  Using cache (fibResults), it's very very fast.

fibResults = []
defaultMax = 1000
nrMax = defaultMax

fibonacci = (n) ->
  return fibResults[n] if fibResults[n]

  if n < 2
    fibResults[n] = n
  else
    fibResults[n] = fibonacci(n - 1) + fibonacci(n - 2)

nrMax = process.argv[2] if process.argv[2]

console.log("#{i}: #{fibonacci(i)}") for i in [0..nrMax]

Something I noticed in your algorithm is that despite using memoization, you are still reassigning the Fibonnacci number into the array every time you calculate it.

You could avoid reassigning by having a function like computeIfAbsent that determines if the Fibonacci for a given n has already been calculated, and if not, use the Fibonacci function to calculate it and put it into the array, but only this time.

Also, we could avoid having to calculate the first two Fibonaccis by having them memoized from the start, and make the code fail for any index n smaller than 0.

memo = [0,1]

computeIfAbsent = (n, f) ->
    fib = memo[n]
    if typeof fib is 'undefined'
        fib = f(n)
        memo[n] = fib
    fib

fibonacci = (x) ->
    if x < 0
        throw Error("Invalid index: " + x)
    computeIfAbsent(x, (n) -> fibonacci(n-1) +  fibonacci(n-2))

console.log("#{i}: #{fibonacci(i)}") for i in [0..79]

Still, you must be aware that this code will not detect arithmetic overflows. It might look like it is calculating it right, but in fact, the calculations could be wrong due to overflow. I think your version and mine start to produce invalid results around 79.

You can check using a Fibonacci Calculator


C# based IRC application (with WPF as UI) I would like to know how I could improve my C# project. I'm not really sure whether or not I'm allowed to just post a link to the Google Code project here, so sorry if it's not allowed, I'll edit my question if someone tells me so.

http://code.google.com/p/simple-irc/source/browse/#svn%2Ftrunk

The reason I'm posting the entire project is because I would like to know if there's anything I can improve coding-wise (coding conventions I'm missing or misusing?), structural-wise (should anything be restructured, do I need to split it up in different classes etc?) and document-wise (how should I document my variables, methods, etc?).

Edit: Maybe I should explain a few things to save anyone who reads this some time. The project is split up in three "modules". The UI part of the application is in the "Source" folder, the core IRC library is in the "Library" folder and any plugins (that the UI module may load) are in the "Plugins" folder.

Edit2: I'll edit my question as a reply to the replies, this is a picture of the application: http://matthias.van-eeghem.com/data/simpleIRC.png

This is a small bit of code (as I apparently have to include that according to the FAQ):

/// <summary>
/// Called when the client receives the your hostName is event
/// </summary>
/// <param name="serverIndex">The server index</param>
/// <param name="hostName">The displayed hostName</param>
private void OnDisplayedHostReceived(int serverIndex, string hostName)
{
    string Output = "Your displayed hostName is: " + hostName + "\r";
    Core.ServerList[serverIndex].Logger.Log(Output);
    ServerWindow.List[serverIndex].SendText(Output);

    foreach (PluginInterface Plugin in MainWindow.PluginList) if(Plugin != null) Plugin.OnDisplayedHostReceived(serverIndex, hostName);
}

/// <summary>
/// Called when the server sends all the commands supported to the client
/// </summary>
/// <param name="serverIndex">The index of the server</param>
/// <param name="commandsSupported">The list of the commands supported</param>
private void OnCommandsSupportedReceived(int serverIndex, string commandsSupported)
{
    string Output = "Supported: " + commandsSupported + "\r";
    Core.ServerList[serverIndex].Logger.Log(Output);
    ServerWindow.List[serverIndex].SendText(Output);

    foreach (PluginInterface Plugin in MainWindow.PluginList) if(Plugin != null) Plugin.OnCommandsSupportedReceived(serverIndex, commandsSupported);
}

/// <summary>
/// Called when the client times out from the server
/// </summary>
/// <param name="serverIndex">The serverIndex</param>
private void OnClientTimeout(int serverIndex)
{
    Server Server = Core.ServerList[serverIndex];
    // Could not resolve hostName
    string Output = "* Lost connection to " + Server.Hostname + "\r";
    Core.ServerList[serverIndex].Logger.Log(Output);
    ServerWindow.List[serverIndex].SendText(Output);

    Output = "* Trying to reconnect..\r";
    Core.ServerList[serverIndex].Logger.Log(Output);
    ServerWindow.List[serverIndex].SendText(Output);

    // Loop through all channels
    for (int i = 0; i < Core.ChannelList.Count; i++)
    {
        Channel Channel = Core.ChannelList[i];
        if(Channel != null && Channel.Server != null)
        {
            if (Channel.Server.Index == serverIndex)
            {
                Output = "* Lost connection to " + Server.Hostname + "\r";
                Core.ServerList[serverIndex].Logger.Log(Output);
                ChannelWindow.List[i].SendText(Output);
            }
        }
    }

    // Try reconnecting
    Server.Connect();

    foreach (PluginInterface Plugin in MainWindow.PluginList) if(Plugin != null) Plugin.OnClientTimeout(serverIndex);
}

I'd love any feedback for this code specifically but I'd also appreciate any comments in regards to the entire project structure etc.

One thing I noticed at a glance is that the main window code-behind is doing a lot of work that isn't really UI-focused.  For example, the code to handle loading plugins from external DLLs is not something I would expect a Window subclass to do. I would highly recommend restructuring the application to utilize pattern that helps you to break things up into more single-purpose classes.

A good example of a pattern that I use often is Model-View-Presenter, or MVP.  A more common pattern in WPF projects is Model-View-ViewModel, or MVVM.

The separation of view logic from business logic makes the application more modular, allowing you to swap out components more easily.  For example, if you decide later that a different UI technology (e.g., HTML/Javascript Metro) is more to your liking, you just need to swap out the view.