I can't fault your tags at all. The only problem is that due to your table being quite complex, assistive technology may not be able to figure out the structure of your table.

I was going to recommend using scope but according to W3C there are some inconsistency issues.

  The row and col values of the scope attribute are currently supported
  to a large extent by most current versions of JAWS. However, there are
  still some problems and WindowEyes support for scope is inconsistent.
  The same is true for Japanese versions of these screen readers.
  Versions of JAWS prior to version 5 and older versions of Window-Eyes
  have inconsistent support for scope.

Using scope would look like this, with scope="col" in all  elements in the first row and scope="row" for all other  elements.

            th 1
            th 2
            th 3
            th 4

            th 5
            th 6
            th 7
            td 1
            td 2
            td 3

            td 4
            td 5
            td 6

            th 8
            td 7
            td 8
            td 9

            td 10
            td 11
            td 12

            th 9
            th 10
            td 13
            td 14
            td 15

            td 16
            td 17
            td 18

            th 11
            td 19
            td 20
            td 21

            td 22
            td 23
            td 24

JSFiddle for scope method

This is W3C's recommendation:

  At the current time, those who want to ensure consistent support
  across Assistive Technologies for tables where the headers are not in
  the first row/column may want to use the technique for complex tables
  H43: Using id and headers attributes to associate data cells with
  header cells in data tables. For simple tables that have headers in
  the first column or row we recommend the use of the th and td
  elements.

Let me just start off by saying this is a complete pain in order to be completely accessibly and scope will probably be fine in most cases. Here is the table using the id/header method (shudder):

            th 1
            th 2
            th 3
            th 4

            th 5
            th 6
            th 7
            td 1
            td 2
            td 3

            td 4
            td 5
            td 6

            th 8
            td 7
            td 8
            td 9

            td 10
            td 11
            td 12

            th 9
            th 10
            td 13
            td 14
            td 15

            td 16
            td 17
            td 18

            th 11
            td 19
            td 20
            td 21

            td 22
            td 23
            td 24

JSFiddle for id/headers

What is comes down to in the end in my opinion is an accessible solution that is also maintainable in the back end. Realistically JAWS and WindowEyes will eventually fix up their bugs and using scope will be fine even for tables as complex as this. Unless you expect many users to use assistive technologies and require strict adherence to WCAG, go with scope. Markup needs to be beautiful too :)

There is also the <colgroup> element which is relevant but support for it isn't great currently.


DateTimePrecise - current DateTime with millisecond precision Answer in this question contains DateTimePrecise class that should return current date with millisecond precision. But class is pretty bug and complicated. So I wrote my own version which seems work fine. If you see any problems with my code?

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Text;

namespace ConsoleApplication1
{
class DateTimePrecise
{
    private long startTick;
    private Stopwatch sw;

    public DateTimePrecise()
    {
        startTick = DateTime.Now.Ticks;
        sw = Stopwatch.StartNew();
    }

    public DateTime CurDateTime()
    {
        return new DateTime(startTick + sw.ElapsedTicks);
    }

}
}

I understand that my version is not accurate, meaning that actual system time may be less or more up to several milliseconds (±16 ms likely). But my version is still precise meaning that couple of measurements will defer at exactly spent milliseconds. And that's exactly what I need. I need precise but not necessary accurate clock because I only need to compare values between each other and I don't need exact current time up to milliseconds.

If you don't need a precise DateTime, only differences, then don't use DateTime at all. I think you should work only with TimeSpans.

This will avoid confusion, because with your class, it looks like DateTimePrecise
.CurDateTime() is better than DateTime.Now, even when used once. If you only used TimeSpans, this wouldn't be a problem.


Avoiding code duplication and retaining const correctness Sometimes I run across the problem of having two member function overloads that only differ in the constness of this (and the return type, but that is less important):

int& operator[](int);
const int& operator[](int)const;

Most of the time these are one-liners, but even then, I hate the code duplication.
That's why I like to use (if possible) the const version to implement the non const version via const_cast:

const int& MyClass::operator[](int index)const {
    return const_cast<int&>(const_cast<MyClass&>(*this)[index]);
}

The first const_cast is required (at least I think so), but I wonder if the second one can be circumvented and if so, if the code is more readable then.

How do you solve this problem?

First you have it the wrong way around.

You should not use the mutating version to supply data to the non-mutating function:

const int& MyClass::operator[](int index)const {
    return const_cast<int&>(const_cast<MyClass&>(*this)[index]);
}

What happens if operator[] is modified and does mutate slightly the internal state? By casting away the constness you have opened up your class to undefined behavior. You should NEVER cast away constness. If you must you can use const_cast to add constness, but removing it is so dangerous that this should be looked at as breaking code.

If you had written it the other way around:

int& MyClass::operator[](int index) {
    return const_cast<int&>(const_cast<MyClass const&>(*this)[index]);
}

This is better. Because if you change the operator[] so that it actually mutates the class it will generate a compiler error and thus prevent you from accidentally getting undefined behavior.

But personally I prefer to add another helper method and thus remove all athe casting:

private:
// NOTE: This is not a perfect solution for all situations.
//       But it works for 99% of situations and keeps the code clean
//       For the remaining situations switch back to use const cast
//       **BUT** always add never remove constness from an object.
// Never make public as it is const and gives away an internal reference.
// Only use via the public API
int& get(int index) const
{
    return stuff[index];
}

public:
// Keep the API simple/clean and DRY
// move all the work to private methods.
int&       operator[](int index)       { return get(index);}
int const& operator[](int index) const { return get(index);}

Answer to comment below:

I would not use it for const/non-const iterators

const_iterator begin() const {return const_iterator(<X>);}
iterator       begin()       {return iterator(<X>);}


Helper that hold methods for evaluate and calculate data I would like some advice to shorten down this snippet, give it more efficient flow and look. Review should focus on keeping it as a compact single method (except the two public props). The DataType and MeasureStatus is an enum. Readability is of course more important than compactness.

....
/// <summary>
/// Helper that hold methods for evaluate and calculate data
/// that will be serialized/deserialized in Serializer/SerializerBase.
/// Please don't put serializing routines in here and also understand
/// that some decisions here are based on protocol specifikations.
/// </summary>
....
public Unit MeasureType { get; set; }
public ModeType Aggregate { get; set; }
.....

public Tuple<DataType, byte> EvaluateDataType(double value, MeasureStatus? status = null)
{
    int quantityOfDecimals = 0;
    DataType dataType = DataType.MISSING_LONG;
    byte nibbled = 0;
    bool statusIsMissing = false;

    if (status != null)
        if (status == MeasureStatus.Missing)
            statusIsMissing = true;

    quantityOfDecimals =
      (this.MeasureType == Unit.kWh || this.MeasureType == Unit.ºC) ? 1 : 2;

    // Qty decimals affect limit size  of 'value'
    if (quantityOfDecimals == 1)
    {
        if (value < 3276 && value > -3276)
            if (!statusIsMissing)
                dataType = DataType.SHORT_1DEC;
            else
                dataType = DataType.MISSING_SHORT;
        else
            if (!statusIsMissing)
                dataType = DataType.LONG_1DEC;
            else
                dataType = DataType.MISSING_LONG;
    }

    // Qty decimals affect limit size  of 'value'
    if (quantityOfDecimals == 2)
    {
        if (value < 327 && value > -327)
            if (!statusIsMissing)
                dataType = DataType.SHORT_2DEC;
            else
                dataType = DataType.MISSING_SHORT;
        else
            if (!statusIsMissing)
                dataType = DataType.LONG_2DEC;
            else
                dataType = DataType.MISSING_LONG;
    }

    nibbled = (byte)(Aggregate | dataType);
    return new Tuple<DataType, byte>(dataType, nibbled);
}

There are some (potentially bizarre to me) things going on here.  A slightly larger view of the system may be helpful in further refining this method.  In any case, here's how I'd restructure this.  I've also included notes about some things I found odd...

public Tuple<DataType, byte> EvaluateDataType(double value, MeasureStatus? status = null)
{

    // As much as possible, initialize a local variable to the proper value
    // when you declare it (as if it had Java's *final* attatched).
    // This isn't always possible, of course
    DataType dataType;
    // Really, these are the only two datatypes that use 1 decimal place?
    int decimalPlaces = (MeasureType == Unit.kWh || MeasureType == Unit.ºC) ? 1 : 2;
    // Structure your code so that it's values and executions give the -reasons-
    // or the -why-.  The code itself says -what- is happening, but it's usually -why-
    // that's actually important.
    // Among other things, this means you should (usually) avoid magic numbers.
    bool valueOutOfRange = Math.Abs(value) >= (Int16.MaxValue / (10 * decimalPlaces));

    // status == null is considered -present-?
    if (status != null && status == MeasureStatus.Missing)
    {
    // Note, never leave off brackets - {} - you run a nasty risk of
    // someone writing indented code that isn't actually part of the block.
    // Also, as much as possible, test for -affirmative- results, not negative ones.
    // Although, multiple negatives should still be avoided.
        if (valueOutOfRange)
        {
            dataType = DataType.MISSING_LONG;
        }
        else
        {
            dataType = DataType.MISSING_SHORT;
        }
    }
    else
    {
        if (valueOutOfRange)
        {
            if (decimalPlaces == 1)
            {
                dataType = DataType.LONG_1DEC;
            }
            else
            {
                dataType = DataType.LONG_2DEC;
            }
        }
        else
        {
            if (decimalPlaces == 1)
            {
                dataType = DataType.SHORT_1DEC;
            }
            else
            }
                dataType = DataType.SHORT_2DEC;
            }
        }
    }

    // Are you sure you don't mean "byte nibbled = (byte) (Aggregate & dataType);"?
    // I'm assuming you're attempting to mask Aggregate for the cast, and that
    // you haven't done anything -too- out there in the operator override.
    byte nibbled = (byte) (Aggregate | dataType);
    return new Tuple<DataType, byte>(dataType, nibbled);
}

Although, I'm a little suspicious of anything that's moving some sort of DataType around like this (especially because you're only storing a byte).  Normally it's best to prefer some sort of implemented generic, so you can get compile time checking of your datatypes.  Nothing like mismatches at runtime to crash your probe into Mars (granted, that was a mismatch in the measure type, which you're attempting to cover here.  However, you still need to deal with value mis-matches...).


Extension Method to append a list of parameters to existing Uri as query In Windows Universal, none of the builder classes for web requests are available, nor can I use System.Web.

How would you optimize (shorten, perform) the extension method below, considering the following requirements:

Sample: http://www.foo.com?bar=foo&foobar=bar

Append a list of key/value parameters as uri parameter
Encode the parameters for the web
Append also if the original uri already contains any query parameters (you do not have to check whether there is a identical pair

How much do you like this implementation ?:

    public static Uri AppendParameters(this Uri uri, IDictionary<string, string> parameters)
    {
        var concatenatedParameters = parameters.Select(pair =>
            $"{WebUtility.UrlEncode(pair.Key)}={WebUtility.UrlEncode(pair.Value)}");

        var query = string.Join("&", concatenatedParameters);
        var uriString = string.IsNullOrEmpty(uri.Query) ?
            uri.OriginalString + $"?{query}" :
            uri.OriginalString + $"&{query}";

        return new Uri(uriString);
    }

You should validate both your arguments for null and throw an ArgumentNullException accordingly.

if (uri == null)
{
    throw new ArgumentNullException(nameof(uri));
}

You could add an early return if the parameters collection is empty as there's no work to actually do.

I'd also use the UriBuilder class like this:

var builder = new UriBuilder(uri);
builder.Query = uri.Query != null && uri.Query.Length > 1
    ? $"{uri.Query.Substring(1)}&{query}"
    : builder.Query = query;

return builder.Uri;

It's basically the same, but it looks a little clearer to me. If you don't want to use the builder, you can at least fully interpolate your strings:

uri.OriginalString + $"?{query}"

Could be

$"{uri.OriginalString}?{query}"

Update

  Can you explain why uri.Query != null && uri.Query.Length > 1 is better than !string.IsNullOrEmpty(uri.Query)

The reason is that uri.Query may return just a question mark e.g. in this example:

new Uri("http://www.google.com?").Query
// returns "?"

If we just checked null or string.Empty then we could end up with a uri like https://www.google.com/?&foo=bar. By checking if the length is greater than 1, we can be sure we won't output a URI with "?&" in it. Although it is valid, it's a bit ugly.


filter date for a specific hour and minute selected_hour=12
dates=[]
date1 = datetime.datetime(2012, 9, 10, 11, 46, 45)
date2 = datetime.datetime(2012, 9, 10, 12, 46, 45)
date3 = datetime.datetime(2012, 9, 10, 13, 47, 45)
date4 = datetime.datetime(2012, 9, 10, 13, 06, 45)
dates.append(date1)
dates.append(date2)
dates.append(date3)
dates.append(date4)

for i in dates:
    if (i.hour<=selected_hour+1 and i.hour>=selected_hour-1):
        if not i.hour==selected_hour:
            if i.hour==selected_hour-1 and i.minute>45:
                print i
            if i.hour==selected_hour+1 and i.minute<=15:
                print i
        if i.hour==selected_hour:
            print i

Result is :

    2012-09-10 11:46:45
    2012-09-10 12:46:45
    2012-09-10 13:06:45

I want to filter my dates based on selected_hour. The output should be 15 minutes later and before of the selected_hour including all selected_hour's minutes. This codes does what I want but, i am looking for a nicer way to do that. How to get same results in a shorter and better way ?

Try something like this:

for i in dates:
    if i.hour == selected_hour:
        print i
    elif i.hour == (selected_hour - 1) % 24 and i.minute > 45:
        print i
    elif i.hour == (selected_hour + 1) % 24 and i.minute <= 15:
        print i

The biggest technique I used to condense your code basically was "Don't repeat yourself"! You have a lot of redundant if statements in your code which can be simplified.

Another thing that makes my code more understandable is that it's flat, not nested. Ideally, you'd want to minimize the number of logic branches.

Edit: As @Dougal mentioned, my original code wouldn't work on the special case of midnight. I added in a modulus operator on my two elif conditions so that the selected_hour test wraps in a 24 digit circle, just like real time.


Check if a given path exists in a graph I'm using BFS to see if a given path exists in a graph:

static class Node{
    int data;
    Node next;

    public Node(int data){
        this.data = data;
    }
}

//Adjency List to store the reference to head of each Node/Vertice
static class AdjList implements Iterable<Integer>{
    Node head;
    public void add(int to){
        if(head==null){
            head = new Node(to);
        }else{
            Node temp = head;
            while(temp.next!=null)
                temp = temp.next;
            temp.next = new Node(to);
        }
    }

    //Used to iterate over the adjacency list while BFS
    @Override
    public Iterator<Integer> iterator(){
        Iterator<Integer> it = new Iterator<Integer>(){
            Node temp = head;
            @Override
            public boolean hasNext(){
                return temp!=null;
            }

            @Override
            public Integer next(){
                Node toRet = temp;
                temp = temp.next;
                return toRet.data;
            }

            @Override
            public void remove(){
                throw new UnsupportedOperationException();
            }
        };
        return it;
    }

}

static class Graph{
    AdjList[] lists;
    int numberOfNodes;

    public Graph(int numberOfNodes){
        lists = new AdjList[numberOfNodes];
        this.numberOfNodes = numberOfNodes;
        for(int i=0; i<numberOfNodes; i++){
            lists[i] = new AdjList();
        }
    }

    //Adds edge from/to vertice
    public void addEdge(int from, int to){
        lists[from].add(to);
    }

    public boolean BFS(int from, int to){
        LinkedList<Integer> queue = new LinkedList<>();
        queue.add(from);
        boolean[] visited = new boolean[numberOfNodes];
        visited[from]= true;

        while(!queue.isEmpty()){
            if(visited[to]==true)
                return true;
            int curr = queue.poll();
            System.out.print(curr + " ");
            Iterator<Integer> iter = lists[curr].iterator();
            while(iter.hasNext()){
                int currVal = iter.next();
                if(visited[currVal]==false){
                    queue.add(currVal);
                    visited[currVal]=true;
                }
            }
        }
        return false;
    }

}

public static void main(String[] args) {
    Graph myGraph = new Graph(3);
    myGraph.addEdge(0, 1);
    myGraph.addEdge(1,2);
    myGraph.addEdge(2,1);
    System.out.println("Path from 2 to 0 exists : " + Boolean.valueOf(myGraph.BFS(2,0)));
}

I could've used DFS for the same purpose which requires lesser memory than BFS, but since my Graph was pretty small, it wouldn't have made much of a difference. Apart from that, under what circumstances should I prefer either of them?

Regarding the code, I'd have only 2 suggestions:

Use brackets even for single-line ifs. It improves readability and makes the code less error prone.
Try not to shorten member names too much (like iter or it for example). It improves readability.

As for the chosen algorithm and possible alternative I'd suggest Iterative Deepening DFS. It has the best of both Depth-First Search and Breadth-First Search - it's complete, finds the optimal solution, and doesn't use too much space.

Let me know if anything's unclear.


Block a thread - spin-lock replacement I have implemented several lock-free algorithms to maximise throughput but have always baulked when performing a spin-lock. I can usually convince myself that this is the only way but it still nags at me that a spin-lock is a terribly bad idea.

I recently came across a FIFOMutex code fragment and thought perhaps that would be a solution. I have hacked it around a bit because I don't want mine to consume interrupt exceptions but essentially it looks like this:

/**
 * Queue of blocked writers.
 */
private static class BlockedQueue {
  // Everyone who is waiting.
  private final Queue<Thread> waiting = new ConcurrentLinkedQueue<>();
  // Ensure only one thread unlocks at a time.
  private final AtomicBoolean lock = new AtomicBoolean(false);

  public void sleep() {
    Thread me = Thread.currentThread();
    // Put me in the queue.
    waiting.add(me);
    try {
      do {
        // Park until I am head of the queue and I hold the lock
        LockSupport.park(this);
        /*
         * If I am not at the head of the queue - go back to sleep.
         *
         * If I am at the head of the queue - grab the lock and get out of the loop.
         *
         * If I was at the head and I couldn't get the lock - go back to sleep -
         * someone else is still in the process of exiting.
         */
      } while (!(waiting.peek() == me && lock.compareAndSet(false, true)));
    } finally {
      // Take me from the queue - it must be me because I locked everyone else out.
      waiting.remove();
      // Done with the lock.
      lock.set(false);
    }
  }

  public void wakeup() {
    // Wake the first in the queue ... unpark does nothing with a null parameter.
    LockSupport.unpark(waiting.peek());
  }
}
// Use this to block for a while.
private final BlockedQueue block = new BlockedQueue();

/**
 * Wait for a time when it is likely that a free slot is available.
 */
private void waitForFree() {
  // Still full?
  while (isFull()) {
    // Park me 'till something is removed.
    block.sleep();
  }
}

/**
 * A slot has been freed up. If anyone is waiting, let the next one know.
 */
private void signalFree() {
  block.wakeup();
}

See how I can now call block.sleep() in what used to be a spin-lock around while(isFull()).

What I like most about this is that the wakeup method is truly simple and that it will only wake up the thread that has been waiting longest.

I have some questions though:

Will this do what I want it to do - i.e. handle multiple threads waiting for a resource without consuming unnecessary CPU.
Is the while ((removed = waiting.remove()) != me) necessary? Logic suggests that the head of the queue at that point must be me but is there a scenario where it may not be?

I believe there's race conditions in your code, Specifically, if you call wakeup() asymettrically (more times than you call sleep()) then you run the risk of a few things:

you could have multiple running threads (i.e. there's not just one thread using the resource)
you may possibly trigger a condition where calling wakeup() twice, very fast, will cause the same thread to be unparked (i.e. waiting.peek() in wakeup()) will return the same value twice....).
it is possible that the queue does not contain a value when the wakeup is called because the 'active' thread is in the process of looking for itself (and removing other content from the queue) as it goes.

I have tried to find a way to make it work to my satisfaction below, but, frankly, I don't think it is possible without changing the code so much it is no longer 'yours'.

I think the reality is that a secure mechanism for this will necessarily rely on having a lock that contains both the unpark and the queue management.... at which point, using the park/unpark is no longer the right tool, so it defeats this process...

Feel free to read through what I was about to write, but, I am essentially abandoning it....

DISREGARD BELOW HERE

Right, you'll have to work with me on this one.... (it takes multiple 'heads' to work around 'threads'....)...

First, let me restate the problem I think you are trying to solve:

you have a resource that only one thread can access at a time
you want the 'waiting' threads to be queued (essentially in FIFO order) until the resource is available.
you want the waiting threads to be (b)locked rather than spinning.

You want the 'outside' code to look something like (this code will be in the resource-using threads):

try {
    blockedqueue.sleep(); // wait for my turn to use the resource
    doSomethingWith(resource);
} finally {
    blockedqueue.wakeup(); // indicate that another thread can use the resource.
}

I see that you start the BlockedQueue off in an 'unblocked' state, so the first thread in will immediately return. Only 'subsequent' threads will wait.

There is a gap in your logic though.... (hate to be the bearer of bad news....)...

It is possible for the queue to be empty when wakeup() is called even though there are blocked threads. This is because you may be removing() items in your loop at the same time as when the wake-up is called.

Also, I think that you should be more precise about what threads can re-awaken the queue. I like the try-finally concept which will assure that the same thread that calls sleep() will also call wakeup().

Consider the following alteration. Instead of using an AtomicBoolean to lock the thread, use an AtomicReference  (and I have renamed it to 'active' instead of 'lock'....

public void wakeup() {
    // Unlock.
    if (active.compareAndSet(this, null)) {
        // Wake the first in the queue (and remove it too) ... unpark does nothing with a null parameter.
        LockSupport.unpark(waiting.poll());
    } else {
        throw new IllegalMonitorStateException("Cannot unlock a thread unless you are the owner of the lock");
    }
}

This will prevent spurious wake-ups, but if you have asymmetrical code (more wakeup() than sleep()) you will get exceptions (I think that's a good thing).

In your sleep() method you can also gain some efficiency by not using the queue at all if you're the only active thread...

public void sleep() {
    final Thread me = Thread.currentThread();
    if (blocked.compareAndSet(null, me) {
        // nothing is waiting, next threads will, but we can just keep going
        // there is a slight possibility that we can 'jump the queue'
        // if sleep() is called at **just** the right time...
        return;
    }

    // here we know that some other thread is active....
    // Put me in the queue.
    waiting.add(me);
    try {
        // Block while not first in queue or we're blocked
        // wait till we are at the front of the queue and we are also
        // not blocked by anything. If we are blocked, then park.
        // If not, we immediately block the next front-of-queue as well.
        while (waiting.peek() != me || !blocked.compareAndSet(false, true)) {
            LockSupport.park(this);
        }
    } finally {
       // Take me from the queue.
       Thread removed;
       while ((removed = waiting.remove()) != me) {
           // Put it back! It wasn't me!
           waiting.add(removed);
       }
    }
}

I think you may have a problem with initial state, and spurious wake-ups.... Consider the following

You have multiple instances of a Runnable (ta and tb for thread A and thread B), each of them does blockingqueue.sleep(). The expectation is that they will be woken up in turn when the queue's wakup() method is called (wake up both ta and tb by calling wakeup() twice).

So, both threads are asleep, the queue contains [ta, tb] and the atomic boolean blocked is false (the


Getting data from API every x seconds and store it in a database I'm currently getting data from an API every 15 seconds. This data contains a json with 100-2000 new items every time. Now i don't need all the items that are in there but still quite some of them and i want to store these items in a database. I will probably delete items that are added longer then 5-7 days.

Now i just want to check if the way i'm doing this now is "normal". Because i will run this all day and it's going to get a lot of items.

The script looks like this now:

function fetchItems() {
  axios
    .get("http://lorem.com")
    .then((response) => {

      for (let i = 0; i < response.data.stashes.length; i++) {
        if (some statements) { //filter some of the stashes here with some statements
          stashes.push(filterItems(response.data.stashes[i])); //filter items in stashes and push to new array
        }
      }
      //filtering done
      //add all items to database
      for (let i = 0; i < stashes.length; i++) {
        for (let j = 0; j < stashes[i].items.length; j++) {
          let item = { data i want to store };
          let sql = 'INSERT INTO items SET ?';
          let query = db.query(sql, item, (error, result) => {
            if (error) console.log(error);
            console.log('item added');
          });
        }
      }
    })
    .catch((error) => {
      console.log(error);
    });
}

Is this the way this should be done?

Quickly overviewing the code - it would work.
If you are into a more readable approach, I may have just a few (minor) comments:

Declare fetchItems as async, and use await for axios.

Instead of a for loop, use response.data.stashes.filter().
Adding all items in separate queries seems odd, is there a way to push all of them in a single transaction? Build the query outside in a helper function.

So your flow may look something like this:

const fetchItems = () => {/*... axios ...*/};
const filterItems = (item) => {/*.. return true or false */};
const buildQuery = (items) => {/*.. return SQL string ..*/};

async function execute() {
  try {
    const items = (await fetchItems()).filter(filterItems);
    const query = buildQuery(items);
    await db.query(sql);
  } catch (err) {
  /* handle it */
  }
}


Moving objects in loop in graphics.py Currently I am trying to teach myself more about functions with parameters and return values that connects with one to another.

In this code I created a square object with graphics.py that moves to a direction in loop whenever you press an arrow key.

from graphics import *
import keyboard, time
width = 200
height = 200
win = GraphWin("MOVEMENTS", width, height)
win.setBackground(color_rgb(25,25,25))

key = ""

def movement(key):
    if keyboard.is_pressed("Right"):
        key = "d"
    if keyboard.is_pressed("Left"):
        key = "a"
    if keyboard.is_pressed("Down"):
        key = "s"
    if keyboard.is_pressed("Up"):
        key = "w"
    return key

def horizontal_movement(x):
    global key
    key = movement(key)
    if key == "d":
        x += 20
    elif key == "a":
        x -= 20
    return x

def vertical_movement(y):
    global key
    key = movement(key)
    if key == "s":
        y += 20
    elif key == "w":
        y -= 20
    return y

def main():
    x = 0
    y = 0
    radius = 10
    player = Rectangle(Point((width/2)-radius+x,(height/2)-radius+y), Point((width/2)+radius+x, (height/2)+radius+y))

    while(True):
        player.undraw()
        player = Rectangle(Point((width/2)-radius+x,(height/2)-radius+y), Point((width/2)+radius+x,(height/2)+radius+y))
        player.setFill("green")
        player.setWidth(2)
        player.draw(win)

        x = horizontal_movement(x)
        y = vertical_movement(y)

        update(10)

main()

I want to know if there is a better code design that can move the movement(key) function into the horizontal_movement(x) and vertical_movement(y) function because right now I feel like I have typed some unecessary coding (the movement function and global key) into the script and I want it be as efficient as possible.

If there is an alternative that makes the script more efficient or perform better, please let me know so I can improve further.

While you seem to be on a good way, your code still hast a global variable it does not need. You can simplify that code a lot by using something like this:

OFFSETS = {"w": (0, -1), "Up": (0, -1),   # weird coordinate system...
           "a": (-1, 0), "Left": (-1, 0),
           "s": (0, 1), "Down": (0, 1),
           "d": (1, 0), "Right": (1, 0)}

def get_offsets():
    for key, offset in OFFSETS.items():
        if keyboard.is_pressed(key):
            return offset
    return 0, 0

This just returns a tuple of changes for each defined key, and zero otherwise.

You should do as little as possible inside your loop (since that affects performance). You can set the graphic options once before the loop if you afterward continue modifying the object(s) in place. With the Rectangle that is easy, since you can just overwrite the p1 and p2 attributes. The two Point objects have an internal _move method that you can use (or you code it yourself):

class Point:
    ...
    def _move(self, dx, dy):
        self.x += dx
        self.y += dy

Another small performance optimization is to do nothing if the rectangle does not move. I would also add a if __name__ == "__main__": guard to allow importing from this module without running the game and follow Python's official style-guide, PEP8, which recommend adding spaces around operators. It also recommends putting imports from different modules on separate lines and not using unnecessary parenthesis.

With these changes you get this for your main:

def main():
    x = 0
    y = 0
    radius = 10
    speed = 20
    lower_left = Point((width / 2) - radius, (height / 2) - radius)
    upper_right = Point((width / 2) + radius, (height / 2) + radius)
    player = Rectangle(lower_left, upper_right)
    player.setFill("green")
    player.setWidth(2)
    player.draw(win)

    while True:
        dx, dy = get_offsets()
        if dx != 0 or dy != 0:
            player.undraw()
            dx, dy = dx * speed, dy * speed
            lower_left._move(dx, dy)
            upper_right._move(dx, dy)
            player.p1, player.p2 = lower_left, upper_right
            player.draw(win)
        update(10)

if __name__ == "__main__":
    main()

In addition, you might want to look for a different keyboard event handler, the keyboard module requires administrator rights (at least on UNIX). I know I certainly wouldn't just run a game that needs that. In other words, I would at least read the whole source code to make there is no exploit hidden in it (which was still possible with the code you have so far), but probably stay on the safe side and not play it at all.


Compute arithmetic sum of a constant difference sequence 3.1

  Write a program that computes the arithmetic sum of a constant
  difference sequence:

D0 = A
Dn+1 = Cdn + B

  Run the following values and compare to the closed form solution:

Maximum index = 10,000, a = 1:0; c = 5; b = 2^-15

I am not sure I perfectly understand the problem above or how to confirm that I have the correct answer, but I wrote the following short program and I would appreciate any feedback.

(defun d-n-plus-1 (d-n c b) (+ (* d-n c) b))

(defun d-depth (depth d-n c b a)
  (if (equal depth 0) (d-n-plus-1 d-n c b)
    (d-depth (1- depth)
         (if (null d-n) a (d-n-plus-1 d-n c b))
         c b a)))

(format t "result: ~a ~%" (d-depth 10000 nil 5 (expt 2 -15) 1))

In addition, I wrote a version that just uses a loop.  I'm not crazy about "loop for x from 1 to depth" or that I used setq in this version.  Can you think of a better way?  Any other suggestions for this version are also welcome.

(defun d_n+1 (d_n c b) (+ (* d_n c) b))

(defun d-depth (depth d_n c b)
  (loop for x from 0 to depth do
    (setq d_n (d_n+1 d_n c b)))
  d_n)

(format t "result: ~a ~%" (d-depth 10000 1 5 (expt 2 -15)))

I notice three things about your code:

You're relying on tail-recursion. This is okay if you know your program will only ever run on implementations that perform tail call optimization, but you should be aware that the Common Lisp standard does not require TCO (and more practically speaking some implementations do indeed not offer it). So if you want your program to be portable, you should rewrite it using a loop¹.
Your d-depth function takes d-n and a as parameters, but only uses a in place of d-n if d-n is nil. It'd make more sense to me to remove the a parameter and instead pass in the a value as the initial value for d-n (instead of nil).
I would also write d_n instead of d-n to emphasize that n is an index, which is usually written using an underscore in ASCII. Also I'd call d-n-plus-1 d_n+1 instead, there's no reason to spell out "plus" in lisp.

In response to your update:

  I'm not crazy about "loop for x from 1 to depth" or that I used setq in this version. Can you think of a better way?

for x from 1 to depth is equivalent to repeat depth (unless you actually use x, which you don't). However in your code you're actually starting at 0, not 1 so you'd need repeat (+ 1 x) to get the same number of iterations.

The setq can be replaced using the for var = start then replacement-form syntax. The result would be something like this:

(defun d-depth (depth a c b)
  (loop repeat (+ 1 depth)
        for d_n = a then (d_n+1 d_n c b)
        finally (return d_n)))

¹ Or in scheme :p


Decimal to roman python I'm a beginner as you can see and I would like to know how I can improve my code. Studying for 6 months now. Thank you.

roman_dict = {1: 'I', 4: 'IV',  5: 'V', 9: 'IX', 10: 'X', 40: 'XL', 50: 'L', 90: 'XC', 100: 'C', 400: 'CD',
              500: 'D', 900: 'CM', 1000: 'M'}

divide_list = [1000, 100, 10, 1]

def not_in_dict(fixed_decimal, divide_num):
    sub_count = 0
    sub_roman_multi = roman_dict[divide_num]
    temp_decimal = fixed_decimal
    while temp_decimal not in roman_dict:
        temp_decimal -= divide_num
        sub_count += 1
    return roman_dict[temp_decimal]+(sub_count*sub_roman_multi)

def decimal_to_roman(decimal):
    original_decimal = decimal
    roman = ""
    for divide_num in divide_list:
        if decimal >= divide_num:
            reminder = decimal//divide_num
            if(reminder >= 1) and ((reminder*divide_num) in roman_dict):
                roman += roman_dict[(reminder*divide_num)]
                decimal -= reminder*divide_num
            else:
                roman += not_in_dict(reminder*divide_num, divide_num)
                decimal -= (reminder*divide_num)
    return str(original_decimal)+' = '+roman

If you use a list of tuples instead of a dictionary and reverse the order, you can simply iterate over it. Your while loop also becomes a lot easier to understand and there is no longer any need to outsource it to another function that returns the literal and its count.

Instead of manually adding strings (something you should basically never do in in Python), use str.join.

ROMAN_LITERALS = [(1000, 'M'), (900, 'CM'), (500, 'D'), (400, 'CD'), (100, 'C'),
                  (90, 'XC'),  (50, 'L'), (40, 'XL'), (10, 'X'), (9, 'IX'),
                  (5, 'V'), (4, 'IV'), (1, 'I')]

def decimal_to_roman(x):
    out = []
    for value, literal in ROMAN_LITERALS:
        while x >= value:
            x -= value
            out.append(literal)
    return "".join(out)

Instead of the while loop you can also use integer division like you did:

def decimal_to_roman(x):
    out = []
    for value, literal in ROMAN_LITERALS:
        n = x // value               # will be 0 if value is too large
        out.extend([literal] * n)    # will not do anything if n == 0
        x -= n * value               # will also not do anything if n == 0
    return "".join(out)


Reading the contents of an XPI file I wrote this code to list contents of an XPI file and then read the contents in it.

I was thinking of these areas of improvement:

I put the zr.open in the try because if the file does not exist it will throw exception, and if zip reader is left open it usually crashes the add-on.
I'm reusing the input stream create instance. How is it not safe? Is there an async version of this?
Should I close the input stream instance after I am all done or each time after init?
Is Cu.forceGC needed? I'm not sure I thought it might be.
My exception checking is weird; I check the .name I'm not sure if this is the recommended way

var zr = Cc["@mozilla.org/libjar/zip-reader;1"].createInstance(Ci.nsIZipReader);
Cu.import('resource://gre/modules/osfile.jsm');
Cu.import('resource://gre/modules/FileUtils.jsm');

var reusableStreamInstance = Cc['@mozilla.org/scriptableinputstream;1'].createInstance(Ci.nsIScriptableInputStream);

var pathToXpiToRead = OS.Path.join(OS.Constants.Path.profileDir, 'extensions', 'PortableTester@jetpack.xpi');
var nsiFileXpi = new FileUtils.File(pathToXpiToRead);

try {
  zr.open(nsiFileXpi); //if file dne it throws here
  var entries = zr.findEntries('*');
  while (entries.hasMore()) {
    var entryPointer = entries.getNext(); //just a string of "zip path" (this means path to file in zip, and it uses forward slashes remember)
    var entry = zr.getEntry(entryPointer); // should return true on `entry instanceof Ci.nsIZipEntry`
    if (!entry.isDirectory) {
        var inputStream = zr.getInputStream(entryPointer);
        reusableStreamInstance.init(inputStream);
        var fileContents = reusableStreamInstance.read(entry.realSize);
        console.log('contenst of file=', fileContents);
    } else {
        console.log('is directory, no stream to read');
    }
  }
} catch (ex) {
  console.warn('exception occured = ', ex);
  if (ex.name == 'NS_ERROR_FILE_NOT_FOUND') {
    Services.ww.activeWindow.alert('XPI at path does not exist!\n\nPath = ' + pathToXpiToRead);
  }
} finally {
  zr.close();
  console.log('zr closed');
  //Cu.forceGC(); //im not sure shoud i do this here?
}

I put the zr.open in the try because if the file does not exist it will throw exception, and if zip reader is left open it usually crashes the add-on.

Crash? I wouldn't know about that. But yes, it should be closed. But then again, only .close() it after it has actually been opened.

try {
  zr.open();
  try {
    // do some stuff
  }
  // another catch possible as well.
  finally {
    zr.close();
  }
}
catch (ex) {
  // handle
}

var reusableStreamInstance = Cc['@mozilla.org/scriptableinputstream;1'].createInstance(Ci.nsIScriptableInputStream);

  I'm resuing the input stream create instnace, how is it not safe?

The stream wrappers, or XPCOM in general really, usually have a contract that you should call .init() only once, unless stated otherwise. nsIScriptableInputStream does not state otherwise, and while it just works(tm) to call .init() again and again right now, that might change in the future.

Better create a stream wrapper per wrapped stream.

  Should I close the input stream instance after i am all done or everytime after init

Yes.

  Is Cu.forceGC needed? I'm not sure I thought it might be.

If you're not sure, then it is not needed. If you know it is "needed", check if there is another way. Manually calling the garbage collector does not only impact performance, it is - or at least used to be - a sure way to crash when calling it too often/too randomly.

  My exception checking is weird, I check the .name I'm not sure if this is the recommended way

Actually, you can check ex.result for nsIExceptions, even within the catch.

catch (ex if ex.result == Components.results.NS_ERROR_FILE_NOT_FOUND) {
  Services.ww.activeWindow.alert('XPI at path does not exist!\n\nPath = ' + pathToXpiToRead);
}
catch (ex) {
  console.warn('exception occured = ', ex);
}

My take

var {classes: Cc, interfaces: Ci, results: Cr, Constructor: CC, utils: Cu } =
  Components;

let ZipFileReader =
  CC("@mozilla.org/libjar/zip-reader;1", "nsIZipReader", "open");
let ScriptableInputStream =
  CC("@mozilla.org/scriptableinputstream;1", "nsIScriptableInputStream", "init");

function handleEntry(name) {
  try {
    let entry = this.getEntry(name);
    if (entry.isDirectory) {
      console.log(name + " is directory, no stream to read");
      return false;
    }
    let stream = new ScriptableInputStream(this.getInputStream(name));
    try {
      // Use readBytes to get binary data, read to read a (null-terminated) string
      let contents = stream.readBytes(entry.realSize);
      console.log("Contents of " + name, contents);
    }
    finally {
      stream.close();
    }
    return true;
  }
  catch (ex) {
    console.warn("Failed to read " + name);
  }
  return false;
}

try {
  var xpi = Services.dirsvc.get("ProfD", Ci.nsIFile);
  "extensions/noverflow@sdrocking.com.xpi".
    split("/").forEach(p => xpi.append(p));

  let reader = new ZipFileReader(xpi);
  try {
    let entries = reader.findEntries('*');
    while (entries.hasMore()) {
      let name = entries.getNext();
      if (handleEntry.call(reader, name)) {
        console.debug("Handled entry " + name);
      }
    }
  }
  finally {
    reader.close();
  }
}
catch (ex if ex.result == Cr.NS_ERROR_FILE_NOT_FOUND) {
  Services.ww.activeWindow.alert("XPI at path does not exist!\n\nPath = " + xpi.path);
}
catch (ex) {
  console.warn("exceptional exception", ex);
}

Edit I moved xpi into the try-catch block, so it doesn't pseudo-leak (becomes a global variable, i.e. this.xpi or window.xpi).

Nested try-blocks

As you asked for this in the comments...
First lets see what happens with this:

try {
  try {
    throw new Error("oops");
  }
  finally {
    console.log("finally");
  }
}
catch (ex) {
  console.error("outer", ex.message);
}

Result

"finally"
"outer" "oops"

Now, if we already caught the exception in the inner try-block by adding a catch block

try {
  try {
    throw new Error("oops");
  }
  catch (ex) {
    console.error("inner", ex.message);
  }
  finally {
    console.log("finally");
  }
}
catch (ex) {
  console.error("outer", ex.message);
}

Result

"inner" "oops"
"finally"

And now, lets re-throw the error.

try {
  try {
    throw new Error("oops");
  }
  catch (ex) {
    console.error("inner", ex.message);
    throw ex;
  }
  finally {
    console.log("finally");
  }
}
catch (ex) {
  console.error("outer", ex.message);
}

Result

"inner" "oops"
"finally"
"outer" "oops"

So there you have it. Any given exception will be caught only once, by the nearest enclosing catch-block, unless it is re-thrown. Of course any new exceptions raised in "inner" block (because code in catch-block way do something that throws ;) will be caught by the "outer" block.


Picasso's Transformation class to Crop borders from Image It's a implementation of Transformation from Picasso library looks for the first different pixel color in both sides, in the vertical and horizontal, and crops the bitmap from there.

Here is the Bitmap class doc from Android library.

public class CropMiddleFirstPixelTransformation implements Transformation {
    private int mWidth;
    private int mHeight;

    @Override
    public Bitmap transform(Bitmap source) {
        int width = source.getWidth();
        int height = source.getHeight();

        int[] horizontalMiddleArray = new int[width];
        source.getPixels(horizontalMiddleArray, 0, width, 0, height / 2, width, 1);

        int[] verticalMiddleArray = new int[height];
        source.getPixels(verticalMiddleArray, 0, 1, width / 2, 0, 1, height);

        int left = getFirstNonWhitePosition(horizontalMiddleArray);
        int right = getLastNonWhitePosition(horizontalMiddleArray);

        int top = getFirstNonWhitePosition(verticalMiddleArray);
        int bottom = getLastNonWhitePosition(verticalMiddleArray);

        mWidth = right - left;
        mHeight = bottom - top;

        if (!isNegative(left, right, top, bottom)) {
            return source;
        }

        Bitmap bitmap = Bitmap.createBitmap(source, left, top, mWidth , mHeight);
        source.recycle();
        return bitmap;

    }

    private boolean isNegative(int... values) {
        for (int i : values) {
            if (i < 0) {
                return false;
            }
        }
        return true;

    }

    private int getFirstNonWhitePosition(int[] horizontalMiddleArray) {
        int left = 0;
        for (int i = 0; i < horizontalMiddleArray.length; i++) {
            if (i == 0) {
                left = horizontalMiddleArray[i];
            }
            if (left != horizontalMiddleArray[i]) {
                return i;
            }
        }
        return -1;
    }

    private int getLastNonWhitePosition(int[] horizontalMiddleArray) {
        int right = 0;
        int length = horizontalMiddleArray.length;
        for (int i = length - 1; i > 0; i--) {
            if (i == length - 1) {
                right = horizontalMiddleArray[i];
            }
            if (right != horizontalMiddleArray[i]) {
                return i;
            }
        }
        return -1;
    }

    @Override
    public String key() {
        return "CropMiddleFirstPixelTransformation(width=" + mWidth + ", height=" + mHeight + ")";
    }
}

What could be improved in this implementation?

I think I could eliminate one of this methods: getLastNonWhitePosition, getFirstNonWhitePosition using Java 8 and functional programming, but I cannot do that.

To be fair, I think it is quite readable  and I do not know of a way to really improve it using Java 8 constructs. I have a tip or two anyway:

In the getXNonWhitePosition methods, I'd remove the internal check to assign the first value, assigning it before the loop:

private int getFirstNonWhitePosition(int[] horizontalMiddleArray) {
    if(horizontalMiddleArray.length == 0) {
        return -1;
    }
    int left = horizontalMiddleArray[0];
    for (int i = 1; i < horizontalMiddleArray.length; i++) {
        if (left != horizontalMiddleArray[i]) {
            return i;
        }
    }
    return -1;
}

You could implement the isNegative with a Java 8 stream, e.g.:

private boolean isNegative(final int... values) {
    return IntStream.of(values).allMatch(value -> value > 0);
}

Why do you save mWidth and mHeight in the fields? Because it seems to me like the transformation has nothing to do with it. These values will change after each transformation application. Using them for the toString feels quite odd.

If I would really want to use Java 8 for the getXNonWhitePosition methods, I think something like the following would do:

private int getFirstNonWhitePosition(final int[] horizontalMiddleArray) {
    if(horizontalMiddleArray.length == 0) {
        return -1;
    }
    final int left = horizontalMiddleArray[0];
    return IntStream.range(1, horizontalMiddleArray.length)
            .filter(index -> horizontalMiddleArray[index] != left)
            .findFirst()
            .orElse(-1);
}

private int getLastNonWhitePosition(final int[] horizontalMiddleArray) {
    if(horizontalMiddleArray.length == 0) {
        return -1;
    }
    final int right = horizontalMiddleArray[horizontalMiddleArray.length - 1];
    return IntStream.rangeClosed(2, horizontalMiddleArray.length)
            .map(index -> horizontalMiddleArray.length - index)
            .filter(index -> horizontalMiddleArray[index] != right)
            .findFirst()
            .orElse(-1);
}

This is not tested, though I think it works. But again, I do not think this is much of an improvement. You could technically extract to a function which receives some value to check against, and some kind of indexing stream, but that feels like over engineering to me.


Anagram Substring Search I am having difficulty understanding the runtime and space complexity while implementing the said question.

Following is the code:

import java.util.HashMap;
import java.util.Map;

public class AnagaramInBigString {

private static Map<Character, Integer> frequencyMap(String str)
{
    Map<Character, Integer> source = new HashMap<Character,Integer>();
    for(int i = 0; i < str.length(); i++)
    {

        if(str.charAt(i) == ' ')
        {
            continue;
        }
        else
        {

            if(source.containsKey(str.charAt(i)))

            {
                source.put(str.charAt(i), source.get(str.charAt(i) + 1));
            }
            else source.put(str.charAt(i), 1);
        }
    }

    return source;
}

public static void main(String []args)
{
    String check= "BACDGABCDA";
    String sample = "ABCD";
    System.out.println(sample.length());

    for(int i= 0; i <= check.length() - sample.length() ; i++)
    {
        String checkSub = check.substring(i, i + sample.length());
        System.out.println(checkSub);
        System.out.println(frequencyMap(checkSub).equals(frequencyMap(sample)));
    }
}

}

Is the time complexity O(check*sample)? Space complexity is O(sample)?
Also please suggest on how I can improve the efficiency of this code.

Like:

I can calculate the hashmap for sample string only once instead of doing it again and again with every iteration.

Time and space complexity is usually represented with n and m, so O(check*sample) and O(sample) would be O(nm) and O(n).

For complexity, if m is sample, and n is check, time complexity is actually O(n(n-m)), or \$O(n^2 - m)\$, which I would say is simple O(n). Same with space complexity, as well (this I am not sure about, please correct me if I am incorrect).

Also, you have:

public class AnagaramInBigString {

and:

private static Map<Character, Integer> frequencyMap(String str)
{

One word to describe this: inconsistency. Be consistent. Either have a newline, or don't.

I suggest not have a new line, as it wastes newlines, and doesn't it make it a bit more readable. It also follows Java conventions.

This crazy code:

    if(str.charAt(i) == ' ')
    {
        continue;
    }
    else
    {

        if(source.containsKey(str.charAt(i)))

        {
            source.put(str.charAt(i), source.get(str.charAt(i) + 1));
        }
        else source.put(str.charAt(i), 1);
    }

I can't even follow it. In addition to the above suggestion, always put braces for if, for, and while statements, no matter how long or short the statements are. Result:

    if (str.charAt(i) == ' ') {
        continue;
    } else {
        if (source.containsKey(str.charAt(i))) {
            source.put(str.charAt(i), source.get(str.charAt(i) + 1));
        } else {
            source.put(str.charAt(i), 1);
        }
    }

The continue can be removed by reversing the expression:

    if (str.charAt(i) != ' ') {
        if (source.containsKey(str.charAt(i))) {
            source.put(str.charAt(i), source.get(str.charAt(i) + 1));
        } else {
            source.put(str.charAt(i), 1);
        }
    }

and the put can be simplified with ternary:

    if (str.charAt(i) != ' ') {
        source.put(str.charAt(i), source.containsKey(str.charAt(i)) ?
                source.get(str.charAt(i) + 1) :
                1);
    }

Also consider using JUnit 4 for testing.


Find All Substrings Interview Query in C++ The following is my code for printing all the substrings of an input string. For example, with "abc", it would be "a","ab","abc","b","bc","c". Could someone please review it for efficiency (and possibly suggest alternatives)?

void findAllSubstrings(const char *s){
    int x=0;
    while(*(s+x)){
        for(int y=0; y<=x; y++)
            cout<<*(s+y);
        cout<<'\n';
        x++;
    }
    if(*(s+1))
        findAllSubstrings(s+1);
    else
        return;
}

Your problem is in O(n²), at least. This seems not to be optimizable. If you want only distinct substrings, then you will have to use a table of already encountered strings, which will make your code slower.

However, you can switch the algorithm from recursive to iterative, which is usually slightly faster. It's a micro-optimization, so do not expect a x2 improvement in speed...

void findAllSubstrings2(const char *s)
{
    while(*s)
    {
        int x=0;
        while(*(s + x))
        {
            for(int y = 0; y <= x; y++)
                std::cout << *(s + y);
            std::cout << "\n";
            x++;
        }
        s++;
    }
}

I've done a profile test, on Codepad and Ideone (different versions of same compilers + different machines). The io operations are left for the profile test, because what matters here is the comparison between the 2 functions.


Implementing "Pierre the Tight-rope" Walker from LYAH Learn You a Haskell presents an excellent introduction to >>= with an example.

Basically, Pierre is a tight-rope walker that has Birds (Int) on each side of his tight-rope (represented by a tuple (Int, Int)). Birds can land on each side (only one side at a time) with addRight and addLeft. If the difference of birds on left and right is > 4, then Pierre falls.

LYAH says to use the Error Monad:

  As an exercise, you can rewrite that with the error monad so that when the
  tightrope walker slips and falls, we remember how many birds were on each side
  of the pole when he fell.

Here's how I implemented it (but LYAH provided the type synonyms):

-- @author: LYAH
type Birds = Int
type Pole = (Birds, Birds)

addLeft :: Birds -> Pole -> Either Pole Pole
addLeft n (x, y) = let newLeft = x + n
                       newPole = (newLeft, y)
                       diff    = abs (newLeft - y)
                   in if (diff > 4) then Left  newPole
                      else               Right newPole

addRight :: Birds -> Pole -> Either Pole Pole
addRight n (x, y) = let newRight = y + n
                        newPole = (x, newRight)
                        diff    = abs (x - newRight)
                    in if (diff > 4) then Left  newPole
                       else               Right newPole

walkTightRope :: Either Pole Pole
walkTightRope = addLeft 2 (0,0) >>= addRight 4

walkTightRopeFailed :: Either Pole Pole
walkTightRopeFailed = addLeft 2 (0,0) >>= addRight 10 >>= addLeft 2

walkTightRopeDo :: Either Pole Pole
walkTightRopeDo = do
    p1 <- addLeft 2 (0,0)
    p2 <- addRight 1 p1
    return p2

Please critique my implementation.

I don't see glaring flaws. A couple notes though:

-You could combine the addLeft/Right functions and pass in a tuple representing how many birds you want to add ( (0,2) for 2 right ); the functions are almost identical.

-I've found where bindings to read neater if I have more then 2 (personal choice).

-AFAIK, the Error Monad /= the Either Monad.


Selector on every radio button click I am constructing a selector on every radio button click. Since I am using the table repeatedly on every radio click, I cached it like:

var $t1 = $("#tableone");

but inside the radio check event, I need to retrieve the selector to construct a string.

Approach 1:

$radio.click(function () {
        var temp = $t1.selector + " ." + $(this).attr("mobnum");

Note: If I do not $t1.selector, it comes as [object][Object] which I do not want, so I have to use $t1.selector.

Since I am using $t1.selector to construct temp every time radio is clicked, is there still a benefit caching the table at the beginning?

Approach 2:

$radio.click(function () {
var temp = $("#tableone") + " ." + $(this).attr("mobnum");

Which one's better?

I don't think your approach 2 will do what you want. So I the first one would be better.

It looks to me though that you are building another selector. Probably to find the child element. So I would recommend something like this:

var $c = $t1.find("." + $(this).attr("mobnum")).

This returns the child element. This way you are already selecting only out of the children of $t1. Which would be more efficient, in theory.


Checking for balanced parentheses I wrote a method to check if each parenthesis in a string is closed. It should take into account the order in which a parenthesis is presented, e.g. "hello)goodbye(" should return false even though there are an equal number of opening and closing parentheses. My method is as follows:

def balance(chars: List[Char]): Boolean = {
    def findNextParen(chars: List[Char]): List[Char] = {
      return chars.dropWhile(char => char != ')' && char != '(')
    }

    def findClosingParen(chars: List[Char]): List[Char] = {
      val nextParen = findNextParen(chars)
      if(nextParen.isEmpty) {
        nextParen
      } else if(nextParen.head == '(') {
        findClosingParen(findClosingParen(nextParen.tail).tail)
      }
      else {
        nextParen
      }
    }
    val nextParen = findNextParen(chars)

    if(nextParen.isEmpty) {
      true
    } else if(nextParen.head == ')') {
      false
    } else if(findClosingParen(nextParen.tail).isEmpty) {
      false
    } else balance(findClosingParen(nextParen.tail).tail)
  }

is there a way to make this more Scala-y? I pretty much feel like I wrote Java code and compiled it with a Scala compiler.

Possibly the most egregiously non-Scala thing in your code is the if...else if...else if...else chain.  Those are a bad smell in any language, particularly OO ones or functional languages with pattern matching.  Scala is both.  That really has to go.

Folding is the most efficient and functional way to solve this, but that's a more advanced subject for somebody who is new to Scala and FP.  Here is a simple recursive solution which illustrates the clarity of pattern matching:

def balance(chars: List[Char]): Boolean = {
    def go(cs: List[Char], level: Int): Boolean = cs match {
        case Nil => level == 0
        case ')' :: _ if level < 1 => false
        case ')' :: xs => go(xs, level - 1)
        case '(' :: xs => go(xs, level + 1)
        case _ :: xs => go(xs, level)
    }
    go(chars, 0)
}

This should show how much pattern matching simplifies a recursive procedure.  Each match defines one of the possible states and the appropriate action.

Because I used pattern matching in this way

It is clear that I have comprehensively covered the possible range of states (OK, I haven't dealt with Null but this is Scala - don't use Null).
The corresponding actions are simple and the small differences between each one easy to see.

Compare this with the complexity, lack of clarity and fragility of your if...else if chain.  It is hard to compare your different conditions, hard to see if you have been comprehensive and nothing about an if chain even compels you to be testing related conditions - you can have anything in each condition.

This is a naive example but it is a good place to start.  That said, Ben's is the best answer.

There is a very small amount of code duplication in this version.  The final three pattern matches do the same thing with only a minor change to the input parameters.  In such a small, easy to read set of code this is really not a sin (and addressing that would make the function structure more complex and less clear). Replacing the recursion with a fold, however, would remove the duplication, because the fold would take care of the repeated application of the function,which could be reduced to a simple closure adjusting the level.

Other notes about this solution:

It is fully tail recursive optimisable, unlike your code (again, the pattern matching makes this easy to see, where the if chain does not)
Adding a @tailrec annotation is good practice in such Scala code
It uses an inner function for recursion rather than exposing internal state (unlike your accepted answer).


Haskell's `partition` Learn You a Haskell mentions the partition function:

  partition takes a list and a predicate and returns a pair of lists.
  The first list in the result contains all the elements that satisfy
  the predicate, the second contains all the ones that don't.

How's my implementation? I'd prefer to avoid the ++ function, but I'm not sure how to avoid it here.

partition' :: (a -> Bool) -> [a] -> ([a], [a])
partition' f []     = ([], [])
partition' f ys = partition'' f ys [] []
                       where partition'' f [] as bs = (as, bs)
                             partition'' f (x:xs) as bs
                               | f x       = partition'' f xs (as ++ [x]) bs
                               | otherwise = partition'' f xs as (bs ++ [x])

As @aled1027 suggests, partition' is just a fancy form of filter.  Therefore, it would be useful to study how filter can be implemented without ++.

filter' :: (a -> Bool) -> [a] -> [a]
filter' _ [] = []
filter' f (x:xs)
  | f x       = x:rest
  | otherwise = rest
  where rest = filter' f xs

The key to avoiding ++, I think, is to force yourself to write x: first, then figure out how to fill in everything surrounding it.  The next logical step would be to write

  | f x = x:filter' f xs

… and the rest should follow naturally.

Here's what I came up with for partition' (hover for spoiler):

  partition' :: (a -> Bool) -> [a] -> ([a], [a])
partition' _ [] = ([], [])
partition' f (x:xs)
  | f x       = ((x:accepted), rejected)
  | otherwise = (accepted, (x:rejected))
  where (accepted, rejected) = partition' f xs


Counting block combinations I: Project Euler 114 Problem statement (For more detailed description (including pictures), please, visit the link):

  A row measuring seven units in length has red blocks with a minimum
  length of three units placed on it, such that any two red blocks
  (which are allowed to be different lengths) are separated by at least
  one grey square. There are exactly seventeen ways of doing this.

  How many ways can a row measuring fifty units in length be filled?

Code:

from scipy import special
import time
import itertools

def partitionfunc(n,k,l=3):
    '''n is the integer to partition, k is the
    length of partitions, l is the min partition element size'''
    if k < 1:
        return 0
    if k == 1:
        if n >= l:
            yield (n,)
        return 0
    for i in range(l,n+1):
        for result in partitionfunc(n-i,k-1,i):
            yield (i,)+result

def valid_partitions(p):
    total =  p
    count = 0 #Max. number of tiles that can be placed on a row.
    while True:
        count += 1
        total -=3
        if total <= 3:
            break
        total-=1
    '''Find all the valid partitions with length [1,count] that can be placed on the row with length p'''
    data = []
    for k in range(1,count+1):
        min_part = k*3
        for n in range(min_part,p+1):
            Allowed = []
            LIST = list(partitionfunc(n,k))
            for b in LIST:
                if sum(b) + (len(b)-1) <= p:
                    Allowed.append(b)
            data+= Allowed
    return data

def count_permutations(array):
    '''Counts how many possible permutations are there for the particular partition'''
    get_unique_elements = set(array)
    total_length = len(array)
    lengths = [array.count(x) for x in get_unique_elements]
    answer = 1
    for b in lengths:
        answer*= special.comb(total_length, b)
        total_length-= b
    return answer

def calculate_ways(m,n):
    return special.comb(n-m+1,n-2*m+1)

def final(w):
    total_variations = 0
    data = valid_partitions(w)
    for q in data:
        m = len(q)
        remain = w - sum(q) - m + 1
        n = 2*m -1 + remain
        total_variations+= calculate_ways(m,n)*count_permutations(q)
    return int(total_variations+1)

if __name__ == '__main__':
    start = time.time()
    print('Answer: {}'.format(final(50)))
    print(time.time()-start)

I'll explain the reasoning behind the code using example provided by the Euler:

Step 1

First, we find maximum number of tiles that can be placed on the row with length 7. At most only 2 tiles can be placed, hence number equals 2 (call this number count).

Then we iterate through [1,count] and and find all the valid partitions of the number seven:

In our case:

1 tile: (3), (4), (5), (6), (7)  (The reason we omit (1) and (2) is because minimum length of red tile is 3)

2 tiles: (3, 3) (Note, that although (3,4) is a partition of 7 too, but it won't work in our case, because it is specified that there must be at least 1 tile gap between two red tiles.

Step 2.

For each partition obtained in step 1, we calculate number of ways the partition can be placed on the row.

For example:

(5) Represents 1 tile with length 5. There are 3 ways to place such tile.

(3,3) Represents 2 tiles both with length 3. There is 1 way to place them on the row.

When we add up the numbers, we end up with 16. We need to add 1, because (I'm curious why) the row that doesn't contain any red tiles is a valid case too. The final answer is 17.

I believe that there are a lot of things that can be improved. I'm glad to hear any suggestions!

P.S The problem I would like to specifically point out is the variable/function names. I believe there is a possibility to make them way more descriptive/clear then they are now.

Firstly, I have to admit to not doing too much in the way of thinking up better variable names; in part because the algorithm was a little hard to follow without solving the problem myself.

The spacing is all over the place: I'm not a PEP 8 purist but you have -= with a space before on one line, with a space after on another, and neither on a third. I fixed all the things my IDE complained about, including a redundant import of itertools.

   LIST = list(partitionfunc(n,k))
   for b in LIST:
      ...

Writing LIST to not shadow the builtin list is evil but it's also unecessary: you don't need to listify the generator to iterate over it. That becomes:

   for b in partition(n, k):
      ...

I changed the partition from tuples to lists, since I thought that was more natural in Python for an homogenous sequence of arbitrary length.

There is a loop in valid_partitions:

count = 0 #Max. number of tiles that can be placed on a row.
while True:
    count += 1
    total -=3
    if total <= 3:
        break
    total-=1

I replaced this with:

max_tiles = (p + 1) // 4

I made valid_partitions a generator, and lost some temporary variables like 'data'. I also changed the range() to start from 1 as the comment suggests.

There is a little logic in the function final that I felt was doing too much, so moved it into calculate_ways()

for q in data:
    m = len(q)
    remain = w - sum(q) - m + 1
    n = 2*m -1 + remain
    total_variations+= calculate_ways(m,n)*count_permutations(q)

final() became:

def total_variations(w):
    return sum(calculate_ways(q, w) * count_permutations(q) for q in valid_partitions(w)) + 1

Mayble the one liner is a little too dense, and one should keep the for loop.

calculate_ways() is now a bit opaque and could probably use tidying but at least it's all in one place now.

I added MIN_SIZE = 3. It's probably overkill to pass 3 around as a parameter but probably worth flagging as a magic number.

from scipy import special
import time

MIN_SIZE = 3

def partition(n, k, min=MIN_SIZE):
    """n is the integer to partition, k is the
    length of partitions, min is the min partition element size"""
    if k == 1:
        if n >= min:
            yield [n]
        return 0
    for i in range(min, n+1):
        for result in partition(n - i, k - 1, i):
            yield [i] + result

def valid_partitions(p):
    """Find all the valid partitions with length [1,max_tiles] that can be placed on the row with length p"""
    max_tiles = (p + 1) // 4
    for k in range(1, max_tiles+1):
        min_part = k * MIN_SIZE
        for n in range(min_part, p+1):
            for b in partition(n, k):
                if sum(b) + (len(b)-1) <= p:
                    yield b

def count_permutations(array):
    """Counts how many possible permutations are there for the particular partition"""
    get_unique_elements = set(array)
    lengths = [array.count(x) for x in get_unique_elements]
    total_length = len(array)
    answer = 1
    for b in lengths:
        answer *= special.comb(total_length, b, exact=True)
        total_length -= b
    return answer

def calculate_ways(q, w):
    m = len(q)
    remain = w - sum(q) - m + 1
    n = 2*m - 1 + remain
    return special.comb(n - m + 1, n - 2*m + 1, exact=True)

def total_variations(w):
    return sum(calculate_ways(q, w) * count_permutations(q)

if __name__ == '__main__':
    start = time.time()
    print('Answer: {}'.format(total_variations(50)))
    print(time.time()-start)

As always with Project Euler, there may be some deep mathematical insight that can optimise the algorithm. And there still are a lot of single letter variables; it's a start, though.


Efficient method to look for an instance of an object in a collection I'm trying to determine if an instance of an object exists within a collection.

I'm using array_walk_recursive to walk and check the leaves of an array.

I've added a small short circuit, but still all leaves will be iterated over.

I could alternatively use something like an exception which is thrown on the first find, to break out early, but that feels kind of wrong.

Am I missing a simple alternative?

<?php

class Fish {}
class Mammal {}
class Whale extends Mammal {}
class Human extends Mammal {}
class Shark extends Fish {}

function anyFish(array $collection)
{
    $found = false;
    array_walk_recursive($collection, function ($v) use (&$found) {
        if(!$found && $v instanceof Fish)
            $found = true;
    });

    return $found;
}

$seas = [
    'pacific'  => [new Whale, new Shark],
    'atlantic' => [new Human, new Shark]
];

$pub = [new Human, new Human];

var_dump(anyFish($seas));
var_dump(anyFish($pub));

Output:

bool(true)
bool(false)

Found on Stackoverflow: https://stackoverflow.com/questions/17853113/break-array-walk-from-anonymous-function)

I think good coding practices should prioritize efficiency before code brevity.  It is important to not ask for unnecessary work to be done (this is part of the artistry of being a clever coder).  Implementing an early exit, in my opinion is a non-negotiable factor in selecting the "best" code design.

Using a function like array_walk_recursive() is handy for its leafnode traversal, but I agree that the syntax of making an early exit is a little unsightly.  For this reason, I recommend a language construct as part of the recursive design.  The conditional logic is the only part that can be condensed, so I've tried to boil it down as much as possible.

Code: (Demo)

function anyFish(array $collection)
{
    foreach ($collection as $item)
    {
        if((is_array($item) && anyFish($item)) || $item instanceof Fish)
        {
            return true;
        }
    }
    return false;
}

I don't think it is horrible to look at, but everybody loves their own babies.  I'm not sure that I can make it any more readable.


How to do these extraction/computations efficiently? I have a set of products(e.g. P0006, P0009, etc.) and a set of production lines (eg. L1H175, L2H175). A product can be produced on a subset of the lines set (and not necessarily all of them).

If we decide to produce a product on a production line(assuming that the line can actually produce it), we need to buy the appropriate raw materials (e.g. If we decide to produce P0009 on L1h175 we need to buy the set of the selected raw materials in the image). Each raw material has a price and a property(S or NS).

What I want the code to do:

For each product, return where it can be produced. For example, to see where P0009 can be produced, I need to look at all the rows of the fist columns and whenever I find it, I add the appropriate line id to the set of lines that produce that product.
Reversely, for each line, what are the products that it can produce.
For each (p,l) couple store the set of all need raw materials, to produce product p on l, the property of each raw material(coded as 1 if it's S and 0 otherwise) and the total price (I don't need individual prices).

Here is how I am doing it now:

def process(x, y, recipes_df):
    raw_materials = {}
    total_cost = 0
    for product, line, rm, rm_type, rm_price in \
    recipes_df[['product','line', 'rm','rm_type','rm_price']].values:
        if (x,y) == (product,line):
            total_cost += float(rm_price)
            raw_materials[rm] = 1 if rm_type == 'S' else 0
    return (raw_materials, total_cost)

lines_set = set(recipes_df['line'])
lines = []

rm_data = {}
products_set = set()
for line in lines_set:
    for row in recipes_df.itertuples(index=False):
        # if the line_id in this row correspond to line in the outer loop
        if row[1] == line:
            # extract the raw material information by using the process function.
            rm_data[row[0]] = process(row[0],row[1], recipes_df)

            # add the product to the set of products that can be manufactured on the line
            products_set.add(row[0])

            # add the informations to lines list
            lines.append((rm_data, products_set))

The file has more than 3000 lines, my approach takes a lot of time.

Observations:
0. There is no assumption about how the file is sorted, meaning that, I can find P0009 in line 1 and in line 3000, for example, with a lot of other products in between. Same remark for the lines.
1. The process function search in all the file every time it's cold for a couple (p,l), which may be inefficient
2. For every line in lines_set we browse all the file.

Question: How to do all of this more efficiently?

Edit : I did only 2. and 3., the 1. is very similar to 2.

Edit2: recipes_df is a pandas data frame

Edit3:  The link to the data

In general if you have relational data in a list-like data structure, you want to convert it into a more appropriate data structure before you do anything tricky with it.  I'm not sure what the type of recipes_df so I'm just going to copy your code that iterates over it and go from there.

from collections import defaultdict
from typing import Dict, List, Set, Tuple

# For each product, return where it can be produced.
product_lines: Dict[str, Set[str]] = defaultdict(set)

# For each line, what are the products that it can produce.
line_products: Dict[str, Set[str]] = defaultdict(set)

# For each (p,l) couple store the set of all need raw materials
# to produce product p on l, the property of each raw material
# (coded as 1 if it's S and 0 otherwise), and the total price
class Recipe:
    def __init__(self):
        self.raw_materials: List[Tuple[str, int]] = []
        self.total_price = 0.0
recipes: Dict[Tuple[str, str], Recipe] = defaultdict(Recipe)

# Loop over recipes_df and populate all these structures.
for product, line, rm, rm_type, rm_price in \
        recipes_df[['product','line', 'rm','rm_type','rm_price']].values:
    product_lines[product].add(line)
    line_products[line].add(product)
    recipes[(product, line)].raw_materials.append((rm, 1 if rm_type == 'S' else 0))
    recipes[(product, line)].total_price += rm_price

At this point you've done a single pass through the entirety of recipes_df and you've completely populated the dictionaries that will let you find all the information you're looking for in constant time.  From here you can convert those dictionaries into whatever other output format you need.


Improving speed of Animation class This is a class to hold an animation and be able to refer to different animations by titles, but it seems like calling draw(Graphics2D g) and update every frame, would, with this implementation, cause a big slowdown with all the list.get(Object obj) calls. Is this a big deal, and if so how should I change the implementation? Other comments on the code are very welcome.

package core.graphics;

import java.awt.Graphics2D;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

public class AnimationSet {

    private int currentAnimation;

    private Map<String, Integer> namesToAnimation;
    private List<Animation> animations;

    public AnimationSet() {
        currentAnimation = 0;
        namesToAnimation = new HashMap<String, Integer>();
        animations = new ArrayList<Animation>();
    }
    public void addAnimation(String title, Animation animation){
        animations.add(animation);
        namesToAnimation.put(title, animations.size()-1);
    }
    public void goToAnimation(String title){
        currentAnimation = namesToAnimation.get(title);
    }
    public void removeAnimation(String title){
        animations.remove(namesToAnimation.get(title));
        namesToAnimation.remove(title);
    }
    public void draw(Graphics2D g, int x, int y){
        g.drawImage(animations.get(currentAnimation).getImage(), x, y, null);
    }
    public void update(){
        animations.get(currentAnimation).update();;
    }
}

As your List is instantiated as an ArrayList, the lookup time for get is constant time, \$O(1)\$.

There is however, somethings that I can point out.

Final

Whenever possible, add the keyword final to member fields of your class. This keyword should be used for values/references that never change. Your namesToAnimation and animations variables can be marked with final.

Why Map<String, Integer>?

You seem to only use your namesToAnimation as a lookup for String to index in the 'animations' list. You never increase/decrease your currentAnimation value. Then why do you use Map<String, Integer>? I think you can get rid of your list entirely and use Map<String, Animation>

Rewritten code according to what I have mentioned above:

public class AnimationSet {

    private Animation currentAnimation;
    private final Map<String, Animation> animations;

    public AnimationSet() {
        animations = new HashMap<>();
    }
    public void addAnimation(String title, Animation animation){
        animations.put(title, animation);
    }
    public void goToAnimation(String title){
        currentAnimation = animations.get(title);
    }
    public void removeAnimation(String title){
        animations.remove(title);
    }
    public void draw(Graphics2D g, int x, int y){
        g.drawImage(currentAnimation.getImage(), x, y, null);
    }
    public void update(){
        currentAnimation.update();
    }
}

Usefulness

I'm sorry, but I can't really understand why you have this class? What functionality does it provide to you? Would it be easier to have the same functionality without this class?

Hopefully you have a good use case for this class, in which case, great! I'm just not sure how I myself would use this class, which brings me to the next point:

Documentation

Although your naming is really good (you have no idea how happy I am to be able to say that!), your class could use some documentation in the form of JavaDoc. Stating the purpose of the class and the usage of the class and it's methods in JavaDoc helps any potential users of your class, it can even help yourself several times.

Thread safety

Your code would likely cause problems if it would be used from multiple threads. To avoid those problems, learn about Java Concurrency. If you're not using multiple threads now, you might be later, that's why I'm mentioning this. It's something for you to learn in the future :)


Asynchronously accepting multiple client connections without impacting main thread I am implementing a multi-player game in C# and having seen all sorts of projects, from indie developers all the way to large studios struggle to patch networking into existing products in the past, have made the decision to get the networking aspect done first, and build the game on top of that.

I've so far gotten to the stage where my server can accept clients asynchronously, but I have a strong feeling that I've implemented the threading very badly. I think it is possible that I am misunderstanding and/or misusing async/await and Tasks, but having read documentation and similar questions I think I will need practical experience to get my head wrapped around the correct usage of these.

As a result, I am hoping that someone would be generous enough to look over my code and help push me onto the right track. I have excluded 'using' statements and the client code for the purposes of this question, however I can confirm the code does function correctly.

I am not too concerned about issues such as naming, and the fact that I have a bit too much code logic in the Main function right now -- these will be modified and extracted to more appropriate locations  respectively as the project progresses.

I have denoted lines I am particularly concerned about with <---- THIS

namespace Game_Server {
    class ServerMain {
        static void Main(string[] args) {
            Server server = new Server();

            server.Start().Start(); //<---- THIS

            while (true) {
                Console.WriteLine($"Number of connected users: {server.clients.Count}");
            }
        }
    }

    class Server {
        private TcpListener server { get; }
        private ConcurrentBag<TcpClient> _clients { get; set; }
        public List<TcpClient> clients { get {
                return _clients.ToList();
            } } //<---- THIS (This was just something I threw up to test my connection model, but any suggestions for handling a thread-safe list of clients would be greatly appreciated!)

        public Server() {
            server = new TcpListener(
                IPAddress.Parse("127.0.0.1"),
                6780
                );
            _clients = new ConcurrentBag<TcpClient>();
        }

        public Task Start() {
            server.Start();

            return new Task(async () => { // <---- THIS
                while (true) {
                    TcpClient client = await server.AcceptTcpClientAsync();
                    _clients.Add(client);
                    Console.WriteLine("Connection on port 6780 from client " + client.Client.RemoteEndPoint);
                }
            });
        }
    }
}

Additionally, my usage of any features in more recent C# iterations (C# 6 most notably) may not be up to scratch, so I'd appreciate any suggestions for making my code more condensed or 'modern'

I apologise if this question doesn't fit perfectly here -- I was concerned it was a bit too specific and if it is, please comment and I'll try to open it up a bit.

Some quick remarks:

Always add access modifiers.
You're not using string[] args, so remove it.
I'm not a fan of private properties, certainly not when they could be fields (and thus they should not have getters/setters). private TcpListener server { get; }, private ConcurrentBag<TcpClient> _clients { get; set; }
Be consistent in naming: _clients starts with an underscore, which is a naming convention usually used for fields, but server doesn't start with an underscore.
Properties should be PascalCase: public List<TcpClient> clients. I feel the same applies to private ones (previous remark).
No underscores in names (with the exception of an underscore prefix for fields, of course): Game_Server.


Dictionary with ISet key as a collection of names I'm writing a command-line utility and I need to find commands (and parameters) by name. The name can either be a full name like save or a shortcut s.

I thought I use a dictionary with an ISet key and a custom comparer. At first I had a list and searched for the name with LINQ but I'd like to have something more convenient. The performance doesn't matter - this time convenience goes first. There will be at most a few dozens of commands. I know I could use a string and map each name to the command but this isn't cool :-)

First, there is a NameSet that is the base class for concrete sets.

class NameSet : HashSet<string>
{
    protected NameSet(IEnumerable<string> keys, IEqualityComparer<string> keyComparer)
    : base(keys ?? throw new ArgumentNullException(nameof(keys)), keyComparer)
    { }
}

one with the suffix CI which stands for Case Insensitive (like the collation in Sql Server)

class NameSetCI : NameSet
{
    private NameSetCI(IEnumerable<string> keys, IEqualityComparer<string> keyComparer)
    : base(keys, keyComparer)
    {}

    public static NameSetCI Create(params string[] keys) =>
        new NameSetCI(keys, StringComparer.OrdinalIgnoreCase);
}

the other with the suffix CS which obviously stands for Case Sensitive.

class NameSetCS : NameSet
{
    private NameSetCS(IEnumerable<string> keys, IEqualityComparer<string> keyComparer)
    : base(keys, keyComparer)
    { }

    public static NameSetCS Create(params string[] keys) =>
        new NameSetCS(keys, StringComparer.Ordinal);
}

The comparer for this is very simple. It just looks if there is any overlapping set.

internal class SetComparer : IEqualityComparer<ISet<string>>
{
    public bool Equals(ISet<string> x, ISet<string> y) => x.Overlaps(y);

    public int GetHashCode(ISet<string> obj) => 0; // Force Equals.
}

With the hash code 0 it doesn't seem to be O(1) anymore but all the keys are in one place an the logic is just a single Overlaps method. LINQ wouldn't be faster anyway and it would mean a lot more work.

Example:

var dic = new Dictionary<NameSetCI, string>(new SetComparer());

dic.Add(NameSetCI.Create("foo", "bar"), "fb");
dic.Add(NameSetCI.Create("qux"), "q");
dic[NameSetCI.Create("baz")] = "b";
dic[NameSetCI.Create("bar")].Dump(); // fb
dic.Add(NameSetCI.Create("foo"), "f"); // bam!

You can just use a single class with a generic Create method which has constraints for IEqualityComparer<string>:

internal class NameSetGeneric : HashSet<string>
{
    private NameSetGeneric(IEnumerable<string> keys, IEqualityComparer<string> keyComparer)
        : base(keys ?? throw new ArgumentNullException(nameof(keys)), keyComparer)
    {
    }

    public static NameSetGeneric Create<T>(T comparer, params string[] keys)
        where T : IEqualityComparer<string> => new NameSetGeneric(keys, comparer);
}

You can even go further and make the whole class generic, but that's only if you want to work with different data types.

Example usage:

var dic = new Dictionary<NameSetGeneric, string>(new SetComparer());
dic.Add(NameSetGeneric.Create(StringComparer.OrdinalIgnoreCase, "foo", "bar"), "fb");
dic.Add(NameSetGeneric.Create(StringComparer.OrdinalIgnoreCase, "qux"), "q");
dic[NameSetGeneric.Create(StringComparer.OrdinalIgnoreCase, "baz")] = "b";
dic[NameSetGeneric.Create(StringComparer.OrdinalIgnoreCase, "bar")].Dump(); //fb
dic.Add(NameSetGeneric.Create(StringComparer.OrdinalIgnoreCase, "foo"), "f"); // bam!


Create an array of categories from comma-seperated text input This code works just fine, but could obviously be more elegantly written.

The basic idea is to create an array of categories from a text input.   The code splits up the text input by commas to create the array and trims away white space.

I considered using unshift to add the 'Inbox' default category to the beggining, but figure there must be an even better way.  All in all, this is probably the 4th or 5th version of this code.

    var CategoriesForm = event.target.category.value;
    var CategoriesFormTrimmed = CategoriesForm.replace(" ", "");
    var categoriesDefault = ['Inbox']; //default category
    var categoriesJoined = [];
    if (CategoriesForm) {
      var splitUpCategories = CategoriesFormTrimmed.split(',');
      categoriesJoined = categoriesDefault.concat(splitUpCategories);
      console.log(categoriesJoined);
    } else {
      categoriesJoined = ['Inbox'];
    }

That same operation can be streamlined into:

var categoriesJoined = event.target.category.value
  .split(',')
  .map(function(category){ return category.trim(); })
  .filter(function(category){ return !!category; })
  .concat(['Inbox']);

or Inbox first:

var categoriesJoined = ['Inbox'].concat(event.target.category.value
  .split(',')
  .map(function(category){ return category.trim(); })
  .filter(function(category){ return !!category; }))

We split first by , giving us access to an array.
We clean up each item using map by returning a cleaned value using trim
Then filter out those who are totally whitespace (they end up as blanks when trimmed).
Add in "Inbox".


Closures in C#, necessary or not? I've written JavaScript for a while, but lately I've been writing a lot of C#.  I wanted to write a method that could take the column name of a DataTable and return an average for those values.  Having never written closures in C#, but being comfortable with them in JavaScript, I came up with this:

public static double GetColumnAverage(DataTable dt,string columnName)
{
    Func<DataTable, double> getAverage = (t) => t.AsEnumerable().Where(x => x[columnName] != DBNull.Value)
                                                                        .Average(x => x.Field<dynamic>(columnName));
    bool isNumeric = true;
    foreach (DataRow row in dt.Rows)
    {
        var type = row[columnName].GetType();
        //not all numeric data types, but close enough for this
        isNumeric = type == typeof(int) || type == typeof(double) || type == typeof(float) || type == typeof(uint)
            || type == typeof(byte);
        if (isNumeric ==false)
        {
            throw new Exception();
        }
    }
    return getAverage(dt);
}

From this I learned that the parameters passed to an inner delegate are matched to the outer delegate, so Intellisense picked up the t parameter as a DataTable (not sure about implications, but neat).  Cool.  My initial thought was to create a private helper method IsNumeric that would return a bool if the value in the DataRow was something we could average.

I know that I could probably improve this a little, but I'm wondering if there's any practical reason to using a closure here, or if there are any potential pitfalls that might make it a bad idea?

I would recommend reading this article by Eric Lippert

You'll definitally want an extension method for this... let me hit the google real quick.... Using some inspired code from this SO post I wrote this.

static class Extensions
{
    private const HashSet<Type> NumericTypes = new HashSet<Type>
        {typeof(decimal), typeof(byte), typeof(sbyte), typeof(short), typeof(ushort),
        typeof(int),typeof(uint), typeof(long), typeof(ulong), typeof(float), typeof(double)};

    /// <summary>Determines whether the specified object is numeric.</summary>
    public static bool IsNumeric(this object obj) { return NumericTypes.Contains(obj.GetType()); }
}

Now in your code you can just say

if(!row[columnName].IsNumeric())
    throw new Exception();

However, now I will review your code in general.

You should not throw Exception, you should always be specific with your exception throwing. In this case I probably would throw an ArgumentException, but really this seems like a specific case where you should define your own.
I'm a bit confused by your declaring isNumeric outside the loop, when you don't use it outside that scope.
instead of saying if (isNumeric == false) just say if(!isNumeric)
I don't see why you nested this function inside your loop. its not taking full advantage of the fact that it is nested, and could just aswell be a function outside. Plus why declare it if you might be throwing an exception.. might as well wait. And while you're waiting, lets just take this one liner and make it a statement instead. (My thoughts jumped around there a bit)

In the end your code could just look like.

public static double GetColumnAverage(DataTable dt, string columnName)
{
    foreach (DataRow row in dt.Rows)
        if (!row[columnName].IsNumeric())
            throw new ArgumentException("-Something meaningful here-");
    return dt.AsEnumerable().Where(x => x[columnName] != DBNull.Value).Average(x => x.Field<dynamic>(columnName));
}


Is there a better way to extract a single value instead of a sequence? The following code works, but I'd like to re-write, so that a single index is returned, rather than a sequence of indexes visited.

Here are the defs and function.

(def usage-vec-len 13)
(def read-levels [9999 3000 2000 1000 100 90 80 70 60 50 40 30 10])
(defn sel-adj-idx
"Finds the correct position at which to bin part of the reading."
[reading]
(for [idx (reverse (range 0 usage-vec-len))
     :let [out-idx idx]
     :when (and (<= reading (nth read-levels idx)) (>= idx 0))]
  out-idx))

If I run this with a reading of 5, I get what I expect

wtr-usage1.core=> (sel-adj-idx 5)
(12 11 10 9 8 7 6 5 4 3 2 1 0)
wtr-usage1.core=> (first (reverse (sel-adj-idx 5)))
0
wtr-usage1.core=>

I keep going until the first index. The value of 5 would be binned there.
I would like just to return the index, but still need to traverse read-levels to compare the reading against each of the levels.

Is there a better form to use for this purpose? I could probably write this recursively, but am trying to see if I can do it with Clojure's rich set of sequence functions.

Why not use just what you posted? Like this:

;use better names
(defn sel-adj-idx-fixed [reading] (first (reverse (sel-adj-idx reading))))

for is used to generate sequences. Try loop for more general looping needs.
You should not hard code the length of a data structure. Each time you update read-levels you will need to update usage-vec-len. Use count instead. It is supposed to be fast for vectors.
A suggestion follows, Not a clojure programmer, so ignore the style:

(defn bin-idx [reading]
   "......"
   (second (first (drop-while #(> reading (first %))
                              (map vector
                                   (reverse read-levels)
                                   (range))))))