Sessions class With assoc-array - practicing I try to dealing with sessions.. including arrays before moving to "database".

Sessions class.file

<?php
session_start();
class Sessions {

public static function ready($name){
  return isset($_SESSION[$name]) ? true : false;
}
    public static function get($name){
      if(isset($_SESSION[$name])){
        return $_SESSION[$name];
      }else {
        return false;
      }
      }

public static function set($name, $val = []){
  return $_SESSION[$name] = $val;

}

    public static function des($name){
      if(self::ready($name)){
       unset($_SESSION[$name]);
      }

    }

}

index.php file

<?php

include_once "Sessions.php";

const BR = '
';

$er = [];
$data = array([
    'id'=> '1',
    'username'=> 'user1',
    'password'=>'123456'
  ],
  [
    'id'=> '2',
    'username'=> 'user2',
    'password'=>'654321'
  ]
);

//submit
if(isset($_POST['submit'])){
  // Error Msgs
$er[] = "Not found in array";
// Forms Inputs
$username = $_POST['username'];
$password = $_POST['password'];

//fetch users in array
foreach ($data as $key => $value) {
  if(in_array($username, $value)){
    if( $username == $value['username'] && $password == $value['password'] ){
      $uid = $value['id'];
      $unm = $value['username'];
      //Sessions class
      Sessions::set('id', $uid);
      Sessions::set('username', $unm );
      // Error Msgs
     unset($er[0]);
    }
}
  }
}

?>
<?php
//Sessions class
$susername = Sessions::ready('username');
$sid = Sessions::ready('id');
if( $susername AND $sid) { ?>
<?$sid?>
<?$susername?>
<?=BR.Sessions::get('username').BR?>
<?=BR.Sessions::get('id').BR?>

Out

<? if( isset( $er[0] ) ) { echo $er[0];}else { unset( $er[0] ); } ?>

<form  method="post">
  <input type="text" name="username" value="">
  <input type="password" name="password" value="">
  <input type="submit" name="submit" value="submit">
</form>

logout.php file

<?php
include_once "Sessions.php";
Sessions::des('username');
Sessions::des('id');
header("Location:session.php");
?>

Is it good for beginner..! is there something wrong..! is there better way for that messy code ?

return isset($_SESSION[$name]) ? true : false; is more succintly written as:

return isset($_SESSION[$name]);

The boolean evaluation of isset() is all you need.
You can use the null coalescing operator to tight up:

if(isset($_SESSION[$name])){
    return $_SESSION[$name];
}else {
    return false;
}

Into:

return $_SESSION[$name] ?? false;

You never need to check if a variable exists before calling unset(). If the variable doesn't exist, the script smoothly, silently continues processing as usual.
The tabbing of your code is needing a fair amount of tender loving care.  Use 4 spaces to indent where necessary. PSR standards are available for everyone's benefit.
I don't think I support the technique of pushing an error message into an array pre-emptively, then removing the element if everything passes.  Add a descriptive error to the error array ONLY if an error is detected.
If you are not going to use a variable (e.g. $key in your loop), then don't bother declaring it.
I realize this is just practice, but try to store only id values in session data if your project permits it.  There is such a thing as Session Hijacking -- it may be an extreme scenario, but it is best to avoid storing any "real world" identifying data in session arrays.
I generally avoid using AND and OR in my if expressions so that I am never tripped up by a fringe "precedence" gotcha.
If you want to display boolean values for diagnostics, I recommend var_export().
Finally, read about how to destroy a session on logout: PHP Session Destroy on Log Out Button


Multithreaded web scraper with proxy and user agent switching I am trying to improve the performance of my scraper and plug up any possible security leaks (identifying information being revealed).

Ideally, I would like to achieve a performance of 10 pages per second. What would I need to do to achieve the biggest performance boost besides get a faster connection / dedicated server? What could be improved?

PS: I am only using eBay.com as an example here. The production version of the scraper will obey robot.txt requests, avoid peak traffic hours, and be throttled so I am not effectively DDoSing the site.

import requests
from bs4 import BeautifulSoup
from multiprocessing.dummy import Pool as ThreadPool

import logging
logging.basicConfig(level=logging.DEBUG, format='%(asctime)s - %(levelname)s - %(message)s')
logging.getLogger("requests").setLevel(logging.WARNING)

# NOTE: The next two sections are for demo purposes only, they will be imported from modules
# this will be stored in proxies.py module
from random import choice
proxies = [
    {'host': '1.2.3.4', 'port': '1234', 'username': 'myuser', 'password': 'pw'},
    {'host': '2.3.4.5', 'port': '1234', 'username': 'myuser', 'password': 'pw'},
]
def check_proxy(session, proxy_host):
    response = session.get('http://canihazip.com/s')
    returned_ip = response.text
    if returned_ip != proxy_host:
        raise StandardError('Proxy check failed: {} not used while requesting'.format(proxy_host))
def random_proxy():
    return choice(proxies)
# / end of proxies.py

# this will be stored in user_agents.py module
from random import choice
user_agents = [
    'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_5) AppleWebKit/600.8.9 (KHTML, like Gecko) Version/8.0.8 Safari/600.8.9',
    'Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/45.0.2454.101 Safari/537.36',
]
def random_user_agent():
    return choice(user_agents)
# / end of user_agents.py

def scrape_results_page(url):
    proxy = random_proxy()  # will be proxies.random_proxy()
    session = requests.Session()
    session.headers = random_user_agent()  # will imported with "from user_agents import random_user_agent"
    session.proxies = {'http': "http://{username}:{password}@{host}:{port}/".format(**proxy)}
    check_proxy(session, proxy['host'])  # will be proxies.check_proxy(session, proxy['host'])

    response = session.get(url)
    soup = BeautifulSoup(response.text, 'lxml')
    try:
        first_item_title = soup.find('h3', class_="lvtitle").text.strip()
        return first_item_title
    except Exception as e:
        print url
        logging.error(e, exc_info=True)
        return None

page_numbers = range(1, 10)
search_url = 'http://www.ebay.com/sch/Gardening-Supplies-/2032/i.html?_pgn='
url_list = [search_url + str(i) for i in page_numbers]

# Make the Pool of workers
pool = ThreadPool(8)
# Open the urls in their own threads and return the results
results = pool.map(scrape_results_page, url_list)
# Close the pool and wait for the work to finish
pool.close()
pool.join()

To get it working you will need to enter valid proxies or comment out the session.proxies = ... and check_proxy(session, proxy['host']) lines.

Performance

Each call to scrape_results_page will also call check_proxy.
As such, the check_proxy will get called for the same proxies multiple times, and I'm wondering if there's a reason for re-checking the proxies.
If not, then you can save time by checking the list of proxies once at the beginning of the program,
and remove the check from scrape_results_page.

Once you have a set of verified proxies,
instead of selecting them randomly,
it would be better to use them round-robin style,
to balance the load.
Admittedly this tip may not make any visible difference whatsoever.

Other improvements

There are a lot of parameters buried in the implementation,
for example the ebay url, canihazip.com, the number of threads and page numbers, and possibly others.
It would be better to define such values at the top,
in variables with descriptive names, all upper-cased to follow the convention for "constants".


CSV file cell slicer This program is supposed to find and slice any cell within a CSV file which contains more than a defined number of characters.

The files can be pretty large, so being a beginner I would like to know if it is written correctly, and if I could make it more efficient. One of the lines is also more than 80 characters long, which bothers me.

import configparser, csv, sys

if len(sys.argv) < 3 :
        usage = """Usage: %s [inputfile][output file]\nThis program requires 2 \
arguments to function properly.\n[input file] is the file to clean\n[output fil\
e] is the name of the file that will be created as a result of this program\n"""
        print(usage % (sys.argv[0]))
else :
        #reads the config file
        config = configparser.ConfigParser()
        config.read('csv_cleaner.ini')
        config = config['CONFIG']
        encoding = config['character_encoding']
        size = int(config['truncation_size'])
        #opens target file and creates the receiving one
        with open(sys.argv[1], 'r', newline='', encoding=encoding)as csv_file, \
             open(sys.argv[2],'x', newline='', encoding=encoding)as output_file:
                #helps with parsing
                if config['detect_dialect'] :
                        dialect = csv.Sniffer().sniff(csv_file.read(2048))
                        dialect.escapechar = '\\'
                        #return to beginning of file
                        csv_file.seek(0)
                        #creates reader and writer
                        reader = csv.reader(csv_file, dialect)
                        dialect.delimiter = config['delimiter_in_output']
                        writer = csv.writer(output_file, dialect)
                        #loops through file's lines
                        for row in reader :
                                #slices cells and loops through line's columns
                                row=[col[:(size-2)]+(col[(size-2):]and'..')for col in row]
                                #writes in new file
                                writer.writerow(row)

This program uses a config file :

[CONFIG]
character_encoding = UTF-8
delimiter_in_output = ;
#set this option to False if the file is not recognized
detect_dialect = True
truncation_size = 255

The main improvement you can get is separating your concerns. Currently parsing the commandline, parsing the config file and actually truncating the columns is all mashed up together. Instead write short functions for each of these things:

import configparser
import csv
import sys

USAGE = """
Usage: %s [inputfile] [output file]
This program requires 2 arguments to function properly.
[input file] is the file to clean
[output file] is the name of the file that will be created as a result of this program
"""

def read_config(file_name):
    config = configparser.ConfigParser()
    config.read(file_name)
    return config['CONFIG']

def detect_dialect_from_file(csv_file):
    dialect = csv.Sniffer().sniff(csv_file.read(2048))
    dialect.escapechar = '\\'
    # return to beginning of file
    csv_file.seek(0)
    return dialect

def truncate_row(row, size):
    return [col if len(col) <= size else col[:size - 2] + ".." for col in row]

def truncate_cells(csv_file_name, output_file_name, encoding, size,\
                   output_delimiter, detect_dialect, **kwargs):
    # opens target file and creates the receiving one
    with open(csv_file_name, 'r', newline='', encoding=encoding) as csv_file,\
         open(output_file_name, 'x', newline='', encoding=encoding) as output_file:
        # helps with parsing
        dialect = (detect_dialect_from_file(csv_file)
                   if detect_dialect
                   else "excel")
        reader = csv.reader(csv_file, dialect)
        dialect.delimiter = output_delimiter
        writer = csv.writer(output_file, dialect)
        # loops through file's lines
        for row in reader:
            # writes in new file
            writer.writerow(truncate_row(row, size))

def main():
    print(len(sys.argv))
    if len(sys.argv) < 3:
        print(usage % (sys.argv[0]))
        sys.exit(1)
    # reads the config file
    config = read_config('csv_cleaner.ini')
    config['size'] = int(config['truncation_size'])
    config['detect_dialect'] = bool(config['detect_dialect'])

    truncate_cells(sys.argv[1], sys.argv[2], **config)

if __name__ == "__main__":
    main()

Here I made the following additional changes:

According to Python's official style-guide, PEP8, lines should be indented by 4 spaces. Also spaces should surround operators and keywords. Imports should also be on separate lines.
While [col[:(size-2)]+(col[(size-2):]and'..')for col in row] works, I find [col if len(col) <= size else col[:size - 2] + ".." for col in row] a bit more readable.
When detect_dialect is false in your case, the script should not run (since it is all nested under that if). But since the value is a string, it is truthy, regardless of whether or not you put True or False there.
When using a multiple line string using triple-quotes, there is no need to use explicit line-breaks and line-continuation characters. I made it a global constant here to avoid indentation issues.

Note that I (or rather my linter) think that 'x' is not a valid file mode.


Length of longest non-repeating substring challenge using sliding window I am working on Leetcode challenge #3 (https://leetcode.com/problems/longest-substring-without-repeating-characters/)

Here is my solution using sliding window and a dictionary. I specifically added start = seen[s[i]]+1 to skip ahead. I am still told I am far slower than most people (for example, given abcdefgdabc, I am skipping abc when I see the second d. I thought this would save ton of time, but apparently this algorithm has a poor run time.

 class Solution(object):
    def lengthOfLongestSubstring(self, s):
        """
        :type s: str
        :rtype: int
        """
        seen = {}
        start = 0
        max_size = 0

        # check whether left pointer has reached the end yet
        while start < len(s):
            size = 0
            for i in range(start, len(s)):
                if s[i] in seen:
                    start = seen[s[i]]+1
                    seen = {}
                    break
                else:
                    seen[s[i]] = i
                    size += 1
            max_size = max(size, max_size)
        return max_size

UPDATE

    i = 0
    j = 0
    seen = {}
    max_length = 0

    while j < len(s):
        if s[j] not in seen:
            seen[s[j]] = j
            j += 1
            max_length = max(max_length, j-i)
        else:
            i = j = seen[s[j]] + 1
            seen = {}
    return max_length

In this loop, while start < len(s):, the string s is not changing, so the length of the string is not going to change either.  Yet, for each iteration, you will evaluate len(s) in order to evaluate the loop condition.  len(s) is undoubtably a fast operation, but you should still cache the result to avoid the function call every iteration.

limit = len(s)
while start < limit:
    # ...

The loop for i in range(start, len(s)):, you are again calling len(s) for each iteration (of the outer loop).  You could replace this with for i in range(start, limit):, but we will still have a different issue...

You are (mostly) not using i inside the loop; you are using s[i].  And you look up the character s[i] in 3 different places inside the loop.  Instead of looping over the indices, and looking up the character at the index, you should directly loop over the characters of the string.

for ch in s[start:]:
    # ...

Except for that pesky seen[...] = i statement.  You still want the index.  The enumerate() function can be used to count as you are looping:

for i, ch in enumerate(s[start:], start):
    if ch in seen:
       start = seen[ch]+1
       seen = {}
       break
    else:
       seen[ch] = i
       size += 1

Hint

When scanning the string abcdefgdhijklmn, after encountering the second d, you reset to the character after the first d, and continue your scan.  But ...

Do you have to?  Aren’t all the characters between the first d and the second d unique?  Is there any way you could preserve that information, and continue on, without needing to restart with the e?  Maybe you don’t need nested loops!


Incremental accumulator by subvectoring The purpose of the following code is to, given a vector, compute the two accumulators: 1. head-till-nth element, 2. (n+1)th element-till-last (where n, and this is the catch, iterates from 1 till length-of-list, inclusive).

(let [my-list [1 2 3 4 5 4 3 2 6]]
  (loop [elem 0]
    (when (<= elem (count my-list))
      (do        (println (reduce + (take elem my-list)))
                 (println (reduce + (drop elem my-list)))
                 (recur (inc elem))))))

While this code gets the job done, it is not functional but rather imperative (using a local, elem, as a running index and comparing against the vector's length to determine termination).

I considered using doseq but I can see no immediate way to incrementally slice the vector per the requirements. Using for is also wrong as I'm not trying to create a list comprehension.

I'd appreciate any answer showing the idiomatic Clojure way of doing this.

To code in a more functional style, break it down starting with the smallest pieces.  I would do it like this:

(defn head-accum [n coll]
  (reduce + (take n coll)))

(defn tail-accum [n coll]
  (reduce + (drop n coll)))

(defn pair-accum [n coll]
  [ (head-accum n coll)
    (tail-accum n coll) ] )

(defn all-pair-accum [coll]
  (let [n-vals (count coll) ]
    (for [split-val (range 1 n-vals) ]
      (pair-accum split-val coll))))

(println (all-pair-accum (range 11)))

;=> ([0 55] [1 54] [3 52] [6 49] [10 45] [15 40] [21 34] [28 27] [36 19] [45 10])


String de-duplication using a custom string pool First let me address why I'm not using the built-in string interning. My code is used in a utility that needs to free all of its memory once it's closed. Strings that are interned are not likely to be freed until the CLR is restarted. I can't create that situation since my utility is used on servers that primarily run .NET based services. (See this reference on CLR not releasing interned strings)

On to the question:

I have created a custom string pool in C#, implemented as an extension method in a static class that contains the actual "pool" in the form of a dictionary:

internal static Dictionary<string, string> StringPool = new Dictionary<string, string>();

internal static string Pool(this string value)
{
    if (!string.IsNullOrEmpty(value) && value.Length <= 200)
    {
        string poolRef;

        if (StringPool.TryGetValue(value, out poolRef))
            value = poolRef;
    else
        StringPool.Add(value, value);
}

return value;
}

To use it, I simply append .Pool() to any string in my project that I want to be pooled. The assumption I am making is that this approach is storing two copies of strings. My assumption comes from the fact that I can recover the actual strings by iterating the Keys in the StringPool object and printing or viewing their contents.

The question I have is whether anyone knows of another indexed object type that I could use as a StringPool object such that the string data would only be stored once, as the Key, rather than twice, as both Key and Value. Additionally I need the fast Key lookup feature that a Dictionary has.

Here's the pseudo code that I believe conveys what I'm after in practice:

internal static CoolClass<string> StringPool = new CoolClass<string>();

internal static string Pool(this string s)
{
    if (!string.IsNullOrEmpty(s) && s.Length <= 200)
    {
        string keyRef = null;

        // obtain reference to key in CoolClass if it already exists
        if (StringPool.TryGetKey(s, out keyRef))
            s = keyRef;
    else
        StringPool.AddKey(s);
}

return s;
}

So, you want a Collection which should hold your strings. The easiest and fastest would be to simply use an array. So let us do it.

Because you are using this for pooling strings, we we only need a string[] array. We should add a method bool TryGetValue(string key, out string value) and a string Add(string value).

If we use an array we need to initialize this array to a given capacity at the constructor level and add a possibility to let the array grow if needed.

I decided to use the name you want CoolClass but it should be better named to something more meaningful.

We need to check if a given "key" is in the array at two places, the TryGetValue() and the Add() method. So let us introduce another method int IndexOf(string key) which returns the index of the given keyand if it isn't in the array it returns -1, which should be extracted to a const.

This leads to

public class CoolClass
{
    private string[] array;
    private int capacity;
    private int lastElement = -1;
    private const int elementNotFound = -1;

    public CoolClass() : this(1024) { }
    public CoolClass(int capacity)
    {
        this.capacity = capacity;
        array = new string[capacity];
    }

    public bool TryGetValue(string key, out string value)
    {
        int i;
        if ((i = IndexOf(key)) != elementNotFound)
        {
            value = array[i];
            return true;
        }

        value = null;
        return false;
    }

    public void Add(String value)
    {
        if (value != null)
        {
            int i;
            if ((i = IndexOf(value)) != elementNotFound)
            {
                lastElement++;
                if (lastElement >= capacity)
                {
                    Grow();
                }
                array[lastElement] = value;
            }
        }
    }

    private int IndexOf(string key)
    {
        for (int i = 0; i < lastElement; i++)
        {
            if (array[i] == key)
            {
                return i;
            }
        }
        return elementNotFound;
    }

    private void Grow()
    {
        capacity = capacity * 2;
        Array.Resize(ref array, capacity);

    }
}

But maybe we should think about this. Why shouldn't we rename the TryGetValue() to GetOrAdd() and remove the Add() method  ? Because we don't want to handle cases where the string isn't pooled in the client code.

public class CoolClass
{
    private string[] array;
    private int capacity;
    private int lastElement = -1;
    private const int elementNotFound = -1;

    public CoolClass() : this(1024) { }
    public CoolClass(int capacity)
    {
        this.capacity = capacity;
        array = new string[capacity];
    }

    public string GetOrAdd(string key)
    {
        if (key == null) { return null; }

        int i;
        if ((i = IndexOf(key)) != elementNotFound)
        {
            return array[i];
        }

        lastElement++;
        if (lastElement >= capacity)
        {
            Grow();
        }

        array[lastElement] = key;
        return array[lastElement];

    }

    private int IndexOf(string key)
    {
        for (int i = 0; i < lastElement; i++)
        {
            if (array[i] == key)
            {
                return i;
            }
        }
        return elementNotFound;
    }

    private void Grow()
    {
        capacity = capacity * 2;
        Array.Resize(ref array, capacity);

    }
}

You reallay should use braces {} for singleline if statements too, this won't harm but makes the code less error prone.

Also it isn't mentioned explicitly in the naming guidelines how to name internal static variables, you should use camelCase casing for naming them.

By using a guard clause like

if (string.IsNullOrEmpty(value) || value.Length > 200) { return value; }

you can return early and therefor you save horizontal spacing which improves the readability of the code.


Run python File With get request flask API Code is Working Review and Recommend What Are Best Practice
What I am trying to achieve.

API with Flask that runs any python file from the current directory.

Run the file and get output in JSON

Below is the code for the app.py
from flask import Flask,jsonify
from flask_restful import Api,Resource
import os

app = Flask(__name__)
api = Api(app)

class callApi(Resource):
    def get(self,file_name):
        my_dir = os.path.dirname(__file__)
        file_path = os.path.join(my_dir, file_name)
        file = open(file_path)
        getvalues={}
        exec(file.read(),getvalues)
        return jsonify({'data':getvalues['total']})

api.add_resource(callApi,"/callApi/<string:file_name>")

if __name__ == '__main__':
   app.run(debug='true')

Below is the code for the main.py which sends a request to API.with Filename which to run.
The filename will be changed as per requirements.
import requests

BASE = 'https://127.0.0.1/callApi/runMe.py'

response = requests.get(BASE)
print(response.encoding)

Below is the File which runs by exec from API
API/app.py can access this file because both are in the same dir.
def fun():
    a = 10
    b = 10
    total = a+b
    print(total)
    return total

total = fun()

Is there any better way to write all this code please let me know.
here are refs which I used to make this
Eval function
Exec Function
Exec Docs
Running a python script saved in local machine from google sheets on a button press

To be honest this code looks pretty dangerous. exec is dangerous in its own right, and using it in conjunction with user-provided input makes an explosive combination.
One flaw is the path traversal vulnerability. For example, providing a file name like "../root/something/script.py" I should be able to invoke files outside your directory. The file has to exist to be executed, but a hacker might find some file lying on your system, that can be exploited in a way you did not foresee.
Your script also does not verify that the resulting path really exists (for this, simply use os.path.exists). Thus, validation of user input is lacking.
And probably this code can be exploited in ways I have not thought about.
But since you are reading files from a specific directory, you could simply run a dir of that location, using for example the os.scandir function, and then you can generate a whitelist of files that are allowed to run. Anything else should be disallowed outright.
Personally I would ditch this approach. It would be better to build a library of functions, and invoke only functions that are known and understood, rather than arbitrary files. The rule of thumb is that user input can never be trusted, so it has to be validated thoroughly.


Identify the Length of the Longest Substring Without Repeating Characters Length of Longest Substring Without Repeating Characters

Problem (from Leetcode)

  Given a string, find the length of the longest substring without repeating characters.

Examples

Given abcabcbb, the answer is abc, which the length is 3.

bbbbb, the answer is b, with the length of 1.

Given pwwkew, the answer is wke, with the length of 3. Note that the answer must be a substring, pwke is a subsequence and not a substring.

Discussion

Approach

Keep track of the substring characters in a Set.
Keep track of the evaluated ("seen") characters in a Queue.
Iterate through the characters in the input String.

If the substring characters include the current character

Iterate through the head of the seen characters Queue until you find a match in the Queue.
For each non-matching character from the Queue, remove it from the Queue and remove it from the Set.
Once you find a matching character, remove it from the head of the Queue and add it to the tail of the Queue.

Else

Add the current character to the Set
Add the current character to the tail of the Queue.

If the size of the Set is greater than the current longest substring length, replace the current longest substring length with the Set size.

General Questions

Naming sucks - open to a better name - there's gotta be something better than LongestSubstringWithoutRepeatingCharactersLengthIdentifier...right?
Is there a cleaner implementation that uses other data structures?

Implementation

import java.util.HashSet;
import java.util.Queue;
import java.util.Set;
import java.util.concurrent.LinkedBlockingQueue;

public class LongestSubstringWithoutRepeatingCharactersLengthIdentifier {
    public static int identify(String s) {
        int longestSubstringLength = 0;
        Set<Character> substringCharacters = new HashSet<>();
        Queue<Character> seenCharacters = new LinkedBlockingQueue<>();

        for (char currentCharacter : s.toCharArray()) {
            if (substringCharacters.contains(currentCharacter)) {
                while (seenCharacters.peek() != currentCharacter) {
                    substringCharacters.remove(seenCharacters.poll());
                }
                seenCharacters.poll();
            } else {
                substringCharacters.add(currentCharacter);
            }

            seenCharacters.add(currentCharacter);

            if (substringCharacters.size() > longestSubstringLength) {
                longestSubstringLength = substringCharacters.size();
            }
        }
        return longestSubstringLength;
    }
}

LinkedBlockingQueue is not the best choice. It's based on a linked list, which lacks spacial locality. An continuous array-backed queue would be more efficient (there're a few options in the standard library).
 if (substringCharacters.size() > longestSubstringLength) {
     longestSubstringLength = substringCharacters.size();
 }

I'd use the Math.max method here.
You can avoid using the queue altogether. You can keep the leftmost index of the current "window" and the set instead.


HackerRank Grid Search challenge I’ve written an answer for The Grid Search challenge on HackerRank.
Please let me know of any improvements that can be made to my implementation (specifically if there's any way to make the algorithm more efficient. I'd like to make this algorithm optimal).

P.S: My implementation is a brute force algorithm, and I reckon that there's a much better way of solving this problem. But, as the grid is unsorted, I'm not exactly sure what that better method would be.

Problem statement (as written on HackerRank):

  Given a 2D array of digits, try to find the location of a given 2D pattern of digits.

Answer:

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;

public class Solution {
    public static void main(String[] args) throws IOException {
        Solution solution = new Solution();
        try(BufferedReader input = new BufferedReader(new InputStreamReader(System.in))){
            int numTestCases = Integer.parseInt(input.readLine());
            assert numTestCases >= 1 && numTestCases <= 5;
            int[][] grid;
            int[][] pattern;

            for(int t = 0; t < numTestCases; t++) {
                grid = solution.buildArray(input);
                pattern = solution.buildArray(input);
                // Gotta STDOUT in HackerRank for problem to be graded.
                System.out.println(solution.findPattern(grid, pattern));
            }
        }catch(IOException e){
            e.printStackTrace();
        }
    }

    /**
     * Determines whether pattern exists in grid.
     *
     * @param grid       2D array of digits
     * @param pattern    the 2D sequence of digits to be found in grid
     * @return "YES" if grid contains pattern. "NO" if not.
     */
    public String findPattern(int[][] grid, int[][] pattern){
        // R, C, r, & c are the same letters used in the problem
        for(int R = 0; R < grid.length - pattern.length + 1; R++){
            for(int C = 0; C < grid[0].length - pattern[0].length + 1; C++){
                outerLoop:
                for(int r = 0; r < pattern.length; r++){
                    for(int c = 0; c < pattern[0].length; c++){
                        if(grid[R + r][C + c] != pattern[r][c]){
                            break outerLoop;
                        }
                    }
                    if(r == pattern.length - 1){
                        return "YES";
                    }
                }
            }
        }
        return "NO";
    }

    /**
     * Builds and returns an array built according to data received from STDIN.
     *
     * @param input    STDIN stream
     * @return a 2D primitive array containing 0-9 digits
     * @throws IOException
     */
    public int[][] buildArray(BufferedReader input) throws IOException {
        String[] sizeParameters = input.readLine().split(" ");
        int rows = Integer.parseInt(sizeParameters[0]);
        int columns = Integer.parseInt(sizeParameters[1]);
        int[][] array = new int[rows][columns];

        for(int i = 0; i < rows; i++){
            String rowOfNumbers = input.readLine();
            for(int j = 0; j < columns; j++){
                array[i][j] = Character.getNumericValue(rowOfNumbers.charAt(j));
            }
        }

        return array;
    }

}

Code organization

This is usually a cause for concern:

Solution solution = new Solution();

Programming contests do not generally require much object design. And in this particular case, instance methods just obfuscate reading. Make them public static. And if it gets so big that you need to divvy it up, do it properly, then.

This is usually a cause for concern, too:

    }catch(IOException e){
        e.printStackTrace();
    }

Just throw the exception, if you cannot do anything with it. And some automated judges show you the STDERR but not STDOUT, so you may be wasting your chance to see whether something went wrong or you returned wrong answer, and what went wrong. (Just knowing if your program has thrown an exception is good. If the automated judge does not support exception but supports process exit codes, you do a System.exit if an exception thrown.)

Performance

Some contests test your code with just small and large data sets, which means you need to find and algorithm with good average complexity. Some contest, such as ACM, tests the code also with the known worst case scenarios of the best known algorithms that could be used to solve the problem. In that case you need to find good worst case complexity.

Some contests have inputs to weed out naive implementations as your current one. One such case would be 1000X1000 grid of 1s, and 500X500 pattern of 1s with a 2 in the bottom right corner. Against such a case you can try an algorithm like :

Scan the pattern, find the row with most diversity (e.g. with the most elements which have a different value to its right)
for each row in the grid do a KMP substring search.
for each match compare the rest of the pattern in the grid, using the row and column offset of the match.

Which, I believe, will have a complexity : O(R(C+c) + m(rc)), where m is the number of the matches for the pattern row used; which can still be bad for large m. In that case you can do a search for a pattern column in parallel (real or simulated).


Better "find and highlight" implementation in HTML element I'm trying to figure out what is best and fastest implementation of a browser-like find and highlight function inside a website using JavaScript.
This function is for one HTML element without any children. Of course it can be expanded to elements with children with some simple loops. That is not part of question.
I wrote this small code to make something working here:

CSS

span.highlight{background:yellow; padding:3px;}

HTML

<input type="search"/>

The Ama...

JavaScript

var s = document.querySelector('input[type="search"]'),
    p = document.querySelector('p'),
    find = function(){
    var words = p.innerText.split(' ');
    for(var i=0; i<words.length; i++){
        if(words[i].toLowerCase() == s.value.toLowerCase()){
            words[i] = '' + words[i] + "";
            p.innerHTML = words.join(' ');
        }
        else{

        }
    }
}
s.addEventListener('keydown', find , false);
s.addEventListener('keyup', find , false);

This code is working fine. Checkout live example here. But this is not fast and sometimes crash the browser. How can I make this functionality with a better approach?

Please note I don't want a full code review here. So don't remind me that querySelector is not supported in IE7 and same things.

Let's start by optimising your code.  Simply said, loops are the danger here.

A while loop can be quicker than a for loop, especially in cases like this. Get the length once.
Avoid looking into arrays.  Do word = words[i] once, which is much faster.
You are joining in every iteration. You should join once only after you have updated your array.
I guess the empty else isn't done yet.

var s = document.querySelector('input[type="search"]'),
    p = document.querySelector('p'),
    find = function(){
        var words = p.innerText.split(' '),
            i = words.length,
            word = '';

        while(--i) {
            word = words[i];
            if(word.toLowerCase() == s.value.toLowerCase()){
                words[i] = '' + word + "";
            }
            else{

            }
        }

        p.innerHTML = words.join(' ');
    }

s.addEventListener('keydown', find , false);
s.addEventListener('keyup', find , false);

Last but surely not least, the page you want to read.


Simple random number-guessing game I am so new to programming. Trying to learn as I go. I made a very simple "Guess the number" game. Just wanted to get some critiques, see if I could clean it up or change anything.

# user guesses the number. Returns Yes or sorry.
import random

# Instruct user to guess a number.
number = int(input("Guess a number, see if you're right."))

rn = random.randint(1,10)

if number == rn:  # User number equals random number
    print("You got it.")

else:  # Users number is not the same as random number.
    print("sorry")

use good variable names: If you use randomNumberinstead of rn and userNumber instead of number then the 'if'-statement changes to

if randomNumber == userNumber :

and your comment in this line becomes obsolete.

The current version of your program can be rewritten without any variables at all:

import random
if 1<=int(input("Guess a number, see if you're right!"))<=10:
  if random.randint(1,10) == 1:  # User succeeds, propability=1/10
      print("You got it.")
  else:  # user fails, propability=9/10
      print("Sorry.")
else: # this input number cannot succeed
    print("Sorry.")

use comments wisely: Don't comment to much. If you change your code then you have to change your comments, too. Otherwise the comments are contradictory to the code. So your comment in the first lines says that 'Yes' is a possible answer of your program, but this is not true.

A comment like

# Instruct user to guess a number.

is not useful. The purpose of the statement it described is immediate clear.

docstrings:
The text  in the first line of the module that describes the module (docstring) should be a '''-string  and not a #-comment.

user interface:

An  input text like

"Input an integer number between 1 and 10 and press <return>!"

would be more useful.

If the user enters a value that is not a number, e.g. the string 'a', an error message like

Traceback (most recent call last):
  File "python", line 5, in <module>
ValueError: invalid literal for int() with base 10: 'a'

is printed. This is not very user friendly. Maybe it is also useful to warn a user if he enters an integer less than 1 or greater than 10.

very pedantic: In accordance to the other output you print, if a user fails you should print Sorry. instead of sorry. Sorry! may me even better. The exclamation mark may be also appropriate here:

Guess a number, see if you're right!

Maybe you should ask he user again for input if invalid  input was entered.

don't use Magic Numbers:
 Use something like

MAX_NUMBER=10
....
random_number=random(1, MAX_NUMBER)

Also other  occurrences of 10 should be replaced by MAX_NUMBER, e.g. in the input text.

format: Placing an empty line after almost every line of code blows up the code but does not make  it more readable. Use empty line to structure the code. So I think it makes no sense to separate the 'if' and the 'else'-block of an 'if'-statement by an empty line, because they belong together.


Building a report of DNA sites and chunks Here is the slow part of my code:

def Prepare_WCfst_file():
    content_WCfst = "iteration\tlocus\tFromSite\tToSite\tWCFst\n"
    nbchunks = nbsites/nbsitesPerChunk
    if nbchunks%1 != 0:
        print("\n\nThe number of sites per chunk for WCFst is not a divisor of the number of sites per locus.\nProcess is aborted!\n\n")
        sys.exit(1)
    nbchunks = int(nbchunks)
    nbsitesPerChunk_minus1 = nbsitesPerChunk - 1
    for iteration in xrange(nbiterations):
        for locus in xrange(nbloci):
            FromSite = 0
            ToSite = nbsitesPerChunk_minus1
            for chunk in xrange(nbchunks):
                content_WCfst += str(iteration) + "\t" + str(locus) + "\t" + str(FromSite) + "\t" + str(ToSite) + "\n"
                FromSite = ToSite + 1
                ToSite = FromSite + nbsitesPerChunk_minus1
    return content_WCfst

Typically, nbiterations and nbloci take values between 1 and 100. nbchunks however can take values of the order of 10^6 to 10^9. This function will be called about 100 to 10,000 times in total.

Suggested data to benchmark

nbloci = 10^6
nbiterations = 20
nbloci = 2
nbsitesPerChunk = 10

What the function does

Basically, what this piece of code does is to create a long string content_WCfst (which will afterward be written on a file). The file needs to contain 4 columns: iteration, locus, FromSite and ToSite. The difference between the columns FromSite and ToSite is always nbsitesPerChunk_minus1. By the end the number of lines is nbiterations * nbloci * nbchunks.

My thoughts about what to improve

I think the part that can be improved is the three loops or eventually only the most internal loop (for chunk in xrange(nbchunks):)

Note that I've tried to replace

str(iteration) + "\t" + str(locus) + "\t" + str(FromSite) + "\t" + str(ToSite) + "\n"

by

"\t".join(str(iteration), str(locus), str(FromSite), str(ToSite), "\n")

but it was even slower.

Note also that the function doesn't receive any arguments and therefore, all parameters are defined globally outside this function. I don't expect it to be an issue as they are not called over and over again.

I am hoping that a solution can exist so that the looping occurs in C, using NumPy eventually.

nbchunks = nbsites/nbsitesPerChunk
if nbchunks%1 != 0:
    print("\n\nThe number of sites per chunk for WCFst is not a divisor of the number of sites per locus.\nProcess is aborted!\n\n")
    sys.exit(1)
nbchunks = int(nbchunks)

This chunk of code is not good because:

nbchunks = nbsites / nbsitesPerChunk

In python-2 is already int for sure, in Python-3 you can write:

nbchunks = nbsites // nbsitesPerChunk

and it will be int for sure, so you can remove

nbchunks = int(nbchunks)

to avoid clutter.

The line:

if nbchunks%1 != 0

(minor) should be spaced like if nbchunks % 1 != 0
(SERIOUS) is always false as the remainder between a natural number and one is always zero!

Also the error message:

print("\n\nThe number of sites per chunk for WCFst is not a divisor of the number of sites per locus.\nProcess is aborted!\n\n")

is not connected to the if statement...

Exiting explicit like sys.exit(1) should be avoided, you should instead:

raise AdequateException(message)


Finding Wilson Primes A Wilson prime is a prime number p such that p² divides (p−1)!+1. The first three Wilson Primes are 5, 13 and 563 and the fourth is larger than \$2×10^{13}\$. I was curious as to how much memory/processing power it would take to calculate the Wilson Primes using a brute force type method.

When I was coding this I kept efficiency in mind, and for this reason I used Stacks rather than Vectors.

#include <iostream>
#include <stack>

using namespace std;

stack<int> findPrimesUnder(int limit){ //stack is used for O(k) complexity where k is constant
    stack<int> primes;

    for(int i = 2; i <= limit; i++){
        int numFactors = 0;
        for(int j = i-1; j > 1; j--){//anything divided by itself is 1 so can be excluded for efficiency by subtracting 1 from i
                                     //also, anything divided by 1 is itself so is excluded for efficiency;
            if (float(i)/j == i/j){
                numFactors++;
            }
        }
        if(numFactors == 0)
            primes.push(i);

    }

    return primes;
}

int factorial(int n){
    if(n == 1){
        return 1;
    }
    else{
        return n*factorial(n-1);
    }
}

int main(){
    int limit;
    cout << "Enter limit: "; cin >> limit;

    stack<int> primes = findPrimesUnder(limit);
    stack<int> wilsonPrimes;

    bool descriptive = false;

    while(!primes.empty()){
        unsigned long long int firstWilsonCheck = (factorial(primes.top()-1)+1)/primes.top();//((p-1)! + 1)/p is always an int where p is prime
        double secondWilsonCheck = double(firstWilsonCheck)/primes.top();

        if(secondWilsonCheck == int(secondWilsonCheck))
            wilsonPrimes.push(primes.top());

        if(descriptive){
            cout << "Prime: " << primes.top() << endl;
            cout << "First Check: " << firstWilsonCheck << endl;
            cout << "Second Check: " << secondWilsonCheck << endl;

            cout << "------------------------" << endl;
        }

        primes.pop();
    }

    cout << "These are the Wilson Primes under " << limit << ":" << endl;
    while(!wilsonPrimes.empty()){
        cout << wilsonPrimes.top() << endl;
        wilsonPrimes.pop();

    }

    return 0;
}

Output:

Enter limit: 100
These are the Wilson Primes under 100:
5
13
37
41
43
47
53
59
61
67
71
73
79
83
89
97

This code calculates the first two Wilson Primes fine (5 and 13), however it cannot calculate any after 13 because the numbers just get too big, I assume.

My questions are:

How could this code be made to be more efficient?
How can I increase precision on larger numbers?

When working with number theory problems like this, efficiency improvements are usually found more in math than in code.

Let us deal with the first problem: For large primes, the factorials overflow. We can deal with this by finding the remainder the factorial leaves on dividing by p^2 instead of the factorial itself.

int factorialremainder(int n, int mod) {
    if(n == 1) {
        return 1;
    } else {
        return (n*factorial(n-1))%mod;
    }
}

Now how do we check whether a number is Wilson prime? Easy. Just check whether (factorialremainder(p-1, p*p) + 1)%(p*p) == 0.

That brings us to the next point, you seem to not know how to use the remainder operation. You can find the remainder on dividing x by y using x%y. x is divisible by y iff x%y == 0. You don't need to do (float(i)/j == i/j) gymnastics anymore.

Finally, your primality testing can be improved a lot. Most numbers have small factors so it actually makes sense to run the loop over j in the reverse direction. Also, you don't need to go till i-1, square root of i is enough. So to find primes you can just do

bool isprime(int n) {
  // Assuming n > 1
  for(int i = 2; i*i <= n; ++i) {
    if(n%i == 0) return false;
  }
  return true;
}

Actually, since you are interested in finding all primes upto a certain range, you should try using some method like Sieve of Eratosthenes.

Also, stack is a very weird data structure to use for the problem. Why not just use a vector?


Navigating the robot by finding the shortest way I have this task where I have to make my robot find the shortest way when I enter a destination point. I've created some functions to assign numbers (distances) for each square and counts its way back to my robot by deleting the other options. Then the robot should only follow the numbers. Here's also a screenshot of the map it navigates in:
<img src="https://i.stack.imgur.com/ZzM52.jpg" alt="Here's also a screenshot of the map it navigates in:
" />
My code works so far, however I think I should be able to reach the same outcome by using less for loops and with a more efficient writing. I believe if I see different ways of thinking in my earlier stages, I can have a broader perspective in the future. So, any ideas on how to reach my goal with a shorter code?
#Your Code Starts Here
"""
for x in range(0, map.width):
    for y in range(0, map.height):
        if map.blocked(x, y):
            map.value[x, y] = -1
        else:
            map.value[x, y] = 0
"""
def fill_around(n,m,number):
    for x in range (n-1,n+2):
        for y in range (m-1,m+2):
            if map.value[x,y] != 0:
                pass
            elif x==n+1 and y==m+1 or x==n-1 and y==m-1 or x==n-1 and y==m+1 or x==n+1 and y==m-1:
                pass
            elif map.blocked(x,y) == True:
                map.value[x,y]= -1
            elif x==n and y==m:
                map.value[x,y]= number
            else:
                map.value[x,y]= number+1

def till_final(final_x,final_y):
    final_x=9
    final_y=1
    fill_around(1,1,1)
    for p in range(2,17):
        for k in range(0, map.width):
            for l in range(0, map.height):
                if map.value[final_x,final_y] ==0 and map.value[k,l]==p:
                    fill_around(k,l,p)

def delete_duplicates(final_x,final_y):
    for k in range(0, map.width):
        for l in range(0, map.height):
            if map.value[k,l] == map.value[final_x,final_y] and k != final_x and l != final_y:
                map.value[k,l] = 0

if structure
        if map.value[x,y] != 0:
            pass
        elif x==n+1 and y==m+1 or x==n-1 and y==m-1 or x==n-1 and y==m+1 or x==n+1 and y==m-1:
            pass
        elif map.blocked(x,y) == True:
            map.value[x,y]= -1
        elif x==n and y==m:
            map.value[x,y]= number
        else:
            map.value[x,y]= number+1

is a little awkward. Rephrase if; pass into if not, delete == True since it's redundant, and you have:
if map.value[x, y] == 0 and not (
    (x==n+1 or x==n-1) and
    (y==m+1 or y==m-1)
):
    if map.blocked(x, y):
        map.value[x, y] = -1
    elif x == n and y == m:
        map.value[x, y] = number
    else:
        map.value[x, y] = number + 1

You can further simplify by checking for coordinate difference in a set membership operation, though I haven't tested its performance:
if map.value[x, y] == 0 and not (
    {x - n, y - m} <= {-1, 1}
):
    # ...

General
You have heavily nested loops that iterate over a matrix-like structure. That's a job for numpy. Vectorization may or may not make your code shorter, but it will definitely speed it up.


My first Go program: Caesar Cipher I'm learning Go at this moment and it is my first program in Go, so I will be thankfull for any suggestion, remarks and observations.

package main

import (
    "fmt"
)

const ascii = "abcdefghijklmnopqrstuvwxyz"
var goodness_values = []float32 { .0817,.0149,.0278,.0425,.1270,.0223,.0202, .0609,.0697,.0015,.0077,.0402,.0241,.0675, .0751,.0193,.0009,.0599,.0633,.0906,.0276, .0098,.0236,.0015,.0197,.0007 }

func zip(s1 string, s2 string) map[string]string {
    result := make(map[string] string)

    for i, v := range s1 {
        result[string(v)] = string(s2[i])
    }

    return result
}

func crypt(s string, table map[string]string) string {
    result := ""

    for _, v := range s {
        result += table[string(v)]
    }

    return result
}

func encode(message string, key int) string {
    shifted_ascii := ascii[key:] + ascii[:key]
    trans_table := zip(ascii, shifted_ascii)

    return crypt(message, trans_table)
}

func decode(secret string, key int) string {
    shifted_ascii := ascii[key:] + ascii[:key]
    trans_table := zip(shifted_ascii, ascii)

    return crypt(secret, trans_table)
}

func goodness(version string, letter_goodness map[string] float32) float32 {
    var result float32 = 0.0

    for _, v := range version {
        result += letter_goodness[string(v)]
    }

    return result
}

func crack(secret string) string {
    var best_score float32  = 0.0
    result := ""
    letter_goodness := make(map[string] float32)

    for i, v := range ascii {
        letter_goodness[string(v)] = goodness_values[i]
    }

    for i := 0; i <= 26; i++ {
        version := decode(secret, i);
        score := goodness(version, letter_goodness);

        if(score > best_score) {
            best_score = score
            result = version
        }
    }

    return result
}

func main() {
    fmt.Printf("%s\n", encode("hello", 10))
    fmt.Printf("%s\n", decode("rovvy", 10))
    fmt.Printf("%s\n", crack("pybrscpexnkwoxdkvmyxdbsledsyxcdydronopsxsdsyxkxnnocsqxypzbyqbkwwsxqvkxqekqoc"))
}

It looks like it was written in another language and then translated to Go. It looks complicated. Go is designed to be efficient. Maps are not as efficient as slices and arrays for simple indexing. decode(secret, i) executes 26 times instead of once. Etc.

Here's an alternate version,

package main

import "fmt"

const alphabet = "abcdefghijklmnopqrstuvwxyz"

func rotate(s string, rot int) string {
    rot %= 26
    b := []byte(s)
    for i, c := range b {
        c |= 0x20
        if 'a' <= c && c <= 'z' {
            b[i] = alphabet[(int(('z'-'a'+1)+(c-'a'))+rot)%26]
        }
    }
    return string(b)
}

func decode(cipher string, rot int) (text string) {
    return rotate(cipher, -rot)
}

func encode(text string, rot int) (cipher string) {
    return rotate(text, rot)
}

var frequencies = []float32{
    .0817, .0149, .0278, .0425, .1270, .0223, .0202, .0609, .0697, .0015, .0077, .0402, .0241,
    .0675, .0751, .0193, .0009, .0599, .0633, .0906, .0276, .0098, .0236, .0015, .0197, .0007,
}

func crack(cipher string) (text string) {
    var sums [26]float32
    for _, c := range cipher {
        c |= 0x20
        for j := range sums {
            if 'a' <= c && c <= 'z' {
                sums[j] += frequencies[(int(c)-int('a')+j)%26]
            }
        }
    }
    var maxi, maxsum = 0, float32(0)
    for i, sum := range sums {
        if sum > maxsum {
            maxsum = sum
            maxi = i
        }
    }
    return rotate(cipher, maxi)
}

func main() {
    fmt.Printf("%s\n", encode("hello", 10))
    fmt.Printf("%s\n", decode("rovvy", 10))
    fmt.Printf("%s\n", crack("pybrscpexnkwoxdkvmyxdbsledsyxcdydronopsxsdsyxkxnnocsqxypzbyqbkwwsxqvkxqekqoc"))
}

Output:

rovvy
hello
forhisfundamentalcontributionstothedefinitionanddesignofprogramminglanguages


Longest Increasing Path in a Matrix from leetcode Solved this:

  Given an integer matrix, find the length of the longest increasing
  path.

  From each cell, you can either move to four directions: left, right,
  up or down. You may NOT move diagonally or move outside of the
  boundary (i.e. wrap-around is not allowed).

Solution: do dfs traversal from every greater node to smaller node and store the result. Reuse the result when the same node is traversed again. Take the maximum of all the directions and return the result.

class Solution(object):
    def longestIncreasingPath(self, matrix):
        """
        :type matrix: List[List[int]]
        :rtype: int
        """
        def check_boundary(matrix, (row, col)):
            if row >= len(matrix) or row < 0:
                return False
            if col >= len(matrix[0]) or col < 0:
                return False
            return True

        def dfs(current, matrix, visited, memory):
            cost = 0
            if not check_boundary(matrix, current):
                return -1
            if current in visited:
                return memory[current]
            for direction in [[-1, 0], [1, 0], [0, -1], [0, 1]]:
                new = current[0] + direction[0], current[1] + direction[1]
                visited.add(current)
                if check_boundary(matrix, new) and matrix[current[0]][current[1]] > matrix[new[0]][new[1]]:
                    cost = max(cost, 1 + dfs(new, matrix, visited, memory))
            memory[(current)] = cost
            return cost

        if not matrix:
            return 0
        maximum = 0
        visited, memory = set(), {}
        for i in range(len(matrix)):
            for j in range(len(matrix[0])):
                maximum = max(maximum, dfs((i, j), matrix, visited, memory))
        return maximum + 1

Overall, this is quite clean code. Your use of closures works quite well to hide implementation details.
The most obvious thing to change is to remove the class. In python things don't have to be in a class, and nothing is made better here by using one.

Here are some small other improvements
This version of check_boundary is smaller, and faster, and arguably cleaner.

def check_boundary(matrix, point):
    return 0 < point[0] < len(matrix) and 0 < point[1] < len(matrix[0])

dfs should use keyword arguments so you don't have to pass in initial conditions.

def dfs(current, matrix, visited=set(), memory={}):

This will simplify the end of your main method to

if not matrix:
    return 0
maximum = 0
for i in range(len(matrix)):
    for j in range(len(matrix[0])):
        maximum = max(maximum, dfs((i, j), matrix))
return maximum + 1

This line for direction in [[-1, 0], [1, 0], [0, -1], [0, 1]]: should have Directions factored out, and it should probably be a tuple of tuples instead of a list of lists, for a minor performance boost.
Other than that, this looks good.


Generalized solution for project euler #1 in haskell with recursion checkIfMultiple x [] = False
checkIfMultiple x (n:ns) = x `mod` n == 0 || checkIfMultiple x ns

findMultiples limit ns = [x | x <- [0..limit], checkIfMultiple x ns]

sumMultiples limit ns = sum (findMultiples limit ns)

sumMultiples 999 [3, 5]
Things I am curious about:

Passing down limit and ns from sumMultiples to findMultiples might be redundant. I was wondering, if I could somehow use composition here or some other shortcut.

What is the convention regarding writing out the type in Haskell? Is it always recommended?

Any other suggestion is appreciated, thank you.

I decided to do euler1 before looking at your solution and this is what I came up with:
euler1 :: [Int] -> Int -> Int
euler1 ns limit =
  sum .
  filter (\x -> any (\n -> x `mod` n == 0) ns) .
  take limit $
  [0..]

So my answers to your questions are:

Yes, composition helps cut down on the things you have to name, and thus on the things you have to pass along to intermediate functions.
Haskell loves lists, and [0..] is often a nice starting point for a composition chain.
Your checkIfMultiple recursion is good, but you will often find a very short synonym for this type of thing (common operations on a list) in base (eg all).
Types are awesome.
List comprehensions are amazing but they can sometimes interrupt a good compositional chain, compared with the equivalent filters and/or fmaps.


"Max Min" in HackerRank I am trying to implement the solution for the problem "Max Min" in HackerRank. The solution runs for some test cases but for few test cases it pops out showing "Terminated due to Time Out"

  Problem:

  Given a list of \$N\$ integers, your task is to select \$K\$ integers
  from the list such that its unfairness is minimized.

  Unfairness = Max{K integer Set} - Min{K integer Set}

  where max denotes the largest integer among the elements of \$K\$, and
  min denotes the smallest integer among the elements of \$K\$.

  Input Format

  The first line contains an integer \$N\$.
  The second line contains an integer \$K\$.
  \$N\$ lines follow. Each line contains an integer that belongs to the list \$N\$.

  Note: Integers in the list \$N\$ may not be unique.

  Output Format

  An integer that denotes the minimum possible value of unfairness.

  Constraints

  \$2 \le N \le 10^5\$
  \$2 \le K \le N\$
  \$0 \le \text{integer in} N \le 10^9\$

  Sample Input #00

7
3
10
100
300
200
1000
20
30

  Sample Output #00

20

Can anyone please point to me which part of my code is taking longer to implement and what a better solution can be?

#include <stdio.h>
#include <string.h>
#include <math.h>
#include <stdlib.h>
#include <limits.h>

#define MAX 100000
#define MAX_VAL 1000000001

int list[MAX];

int main() {

    int N, K;
    int i;
    int unfairness=0;
    scanf_s("%d %d", &N, &K);
    int set = K - 1;
    for (i = 0; i < N; i++)
        scanf_s("%d", &list[i]);
    for (int l = 0; l < N - 1; l++){
        for (int j = 0; j < N - l - 1; j++)
        {
            if (list[j] > list[j + 1])
            {
                int temp;
                temp = list[j];
                list[j] = list[j + 1];
                list[j + 1] = temp;
            }
        }
    }
    for (int x = 0; x < N - set; x++)
    {
        int differ = list[x + set] - list[x];
        //printf("Dif - %d\n", differ);
        //printf("Unfair - %d\n", unfairness);
        if (unfairness == 0){
            unfairness = differ;
        }
        else if (differ<unfairness)
            unfairness = differ;
    }
    printf("%d", unfairness);
    return 0;
}

The slowness comes from the sort algorithm you've chosen. It's \$O(N^2)\$ and you can do better by using a faster sort algorithm,
such as merge sort or quicksort.
Once you do that, the solution will pass with flying colors.

You could simplify the loop that checks the maximum difference by initializing unfairness to the diff of the first interval:

int unfairness = list[set] - list[0];

And then start the loop from 1, and simplify:

for (int x = 1; x < N - set; x++) {
    int diff = list[x + set] - list[x];
    if (diff < unfairness) {
        unfairness = diff;
    }
}

Also, it would be good to put this code in a separate function.
It's good to have many short functions that do one thing,
than one long function that does many things.

Instead of initializing i at the top of the function,
it would be better to do that inside the for-loop where you use it,
just like you did for other loops.

I recommend to avoid using l as a variable name,
because on some displays and fonts it's too hard to distinguish from 1 or I.


Turning music into an image I was bored, so I wrote something that could turn .wav files in to .bmps:

#include <stdio.h>
char bmp[12000029]; // Need 12000028 bits, 1 pixel is 3
                    // bits and table is 28 (1500^2 * 3 + 28)

int main() {

  bmp[0]  = 0x42;   // Bitmap signature
  bmp[1]  = 0x4D;   // 'BM'

  bmp[2]  = 0x1C;   //
  bmp[3]  = 0x1B;   // Size of bitmap
  bmp[4]  = 0xB7;   // in bytes
  bmp[5]  = 0x00;   //

  bmp[10] = 0x1A;   // Image data location

  bmp[14] = 0x0C;   // Size of header

  bmp[18] = 0xDC;   //
  bmp[19] = 0x05;   // Dimensions of image
  bmp[20] = 0xDC;   // 1500 * 1500
  bmp[21] = 0x05;   //

  bmp[22] = 0x01;   // Something
  bmp[24] = 0x18;   // standard (?)

  FILE *fp;
  fp = fopen("file.wav", "rb");
  if (fp == NULL) {
    printf("Error opening 'file.wav'\n");
    return 1;
  }
  for (int i = 41; i < 12000043; i++) {     // Actual music data starts at 41
    bmp[i - 15] = fgetc(fp);                // Store first data to bmp[26] (41 - 15)
  }
  fclose(fp);

  fp = fopen("file.bmp", "w+");
  for (int i = i; i < 12000028; i++) {
    fputc(bmp[i], fp);
  }
  fclose(fp);

return 0;
}

I know this is probably the most random thing ever, but it is quite intriguing in my opinion.

(You just got Rickroll'd).

Strange loop initialization

Hm, what's up with this loop initialization?

  for (int i = i; i < 12000028; i++) {

i is not even defined at this point. Write it the non-confusing way:

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

Magic number 12000028

The number 12000028 and 12000028 + 1 and 12000028 + 15 appears in multiple places in the code. It would be better to #define and reuse.

#define BMP_SIZE 12000028

char bmp[BMP_SIZE]; // Need 12000028 bits, 1 pixel is 3
// ...

int main() {

    // ...

    for (int i = 26; i < BMP_SIZE; i++) {
        bmp[i] = fgetc(fp);
    }

    // ...

    for (int i = 0; i < BMP_SIZE; i++) {
        fputc(bmp[i], fp);
    }

    // ...
}

Error checking

You checked nicely the result of opening the input file file.wav.
But then, why didn't you do the same for the output file file.bmp?

Use puts instead of printf when possible

When you print plain text ending with newline and you don't need to print variables, then puts is better than printf.
So instead of:

printf("Error opening 'file.wav'\n");

This is slightly better:

puts("Error opening 'file.wav'");

Usability

The input and output filenames are hardcoded.
That makes this difficult to use.
It would be great to take them as command line arguments

Suggested implementation

Applying the above suggestions:

#include <stdio.h>
#define BMP_SIZE 12000028

char bmp[BMP_SIZE]; // Need 12000028 bytes, 1 pixel is 3
// bits and table is 28 (1500^2 * 3 + 28)

int main(int argc, char ** argv) {

    bmp[0]  = 0x42;   // Bitmap signature
    bmp[1]  = 0x4D;   // 'BM'

    bmp[2]  = 0x1C;   //
    bmp[3]  = 0x1B;   // Size of bitmap
    bmp[4]  = 0xB7;   // in bytes
    bmp[5]  = 0x00;   //

    bmp[10] = 0x1A;   // Image data location

    bmp[14] = 0x0C;   // Size of header

    bmp[18] = 0xDC;   //
    bmp[19] = 0x05;   // Dimensions of image
    bmp[20] = 0xDC;   // 1500 * 1500
    bmp[21] = 0x05;   //

    bmp[22] = 0x01;   // Something
    bmp[24] = 0x18;   // standard (?)

    if (argc != 3) {
        printf("usage: %s input.wav output.bmp\n", argv[0]);
        return 2;
    }
    char * input = argv[1];
    char * output = argv[2];

    FILE *fp;
    fp = fopen(input, "rb");
    if (fp == NULL) {
        printf("Error opening %s for reading", input);
        return 1;
    }
    for (int i = 26; i < BMP_SIZE; i++) {
        bmp[i] = fgetc(fp);
    }
    fclose(fp);

    fp = fopen(output, "w+");
    if (fp == NULL) {
        printf("Error opening %s for writing", output);
        return 1;
    }
    for (int i = 0; i < BMP_SIZE; i++) {
        fputc(bmp[i], fp);
    }
    fclose(fp);

    return 0;
}


Functions to convert camelCase strings to snake_case I have written two functions, one actual and one helper, to convert camelCase strings into snake_case strings (I call it joint-lower case). Any ideas for improvement are welcome.

def _cc2jl(string):
    """Camel case to joint-lower helper."""

    for index, current in enumerate(string):

        if is_upper(current):
            if index > 0:
                previous = string[index-1]

                if is_upper(previous):
                    try:
                        next = string[index+1]
                    except IndexError:
                        yield current.lower()
                        continue
                    else:
                        if is_upper(next):
                            yield current.lower()
                            continue

                yield '_' + current.lower()
                continue

            yield current.lower()
            continue

        yield current

def cc2jl(string):
    """Converts a camel case string to joint-lower."""

    return ''.join(_cc2jl(string))

Expected behaviour:

>>> cc2jl('m')
'm'
>>> cc2jl('AA')
'aa'
>>> cc2jl('MySQLDatabase')
'my_sql_database'
>>> cc2jl('GarbageCollection')
'garbage_collection'
>>> cc2jl('AAAAAAAAAAAAAAAAA')
'aaaaaaaaaaaaaaaaa'

A couple of things:

Why 2 functions?
Now that we established (1), you can get rid of the yields.
yield is useful when you want to process stuff in chunks and do not want to wait for the entire population to be created first. This is not the case here.
There are some continues that do make sense and some that are redundant. Yours are of the latter type.
Nested if statements without corresponding else clauses can be merged with and if readability is not reduced.

Putting all that together, we get the following:

def cc2jl(my_str):
  """Camel case to joint-lower"""

  r = my_str[0].lower()
  for i, letter in enumerate(my_str[1:], 1):
    if letter.isupper():
      try:
        if my_str[i-1].islower() or my_str[i+1].islower():
          r += '_'
      except IndexError:
        pass
    r += letter.lower()
  return r

print(cc2jl('m'))                  #-> m
print(cc2jl('AA'))                 #-> aa
print(cc2jl('MySQLDatabase'))      #-> my_sql_database
print(cc2jl('GarbageCollection'))  #-> garbage_collection
print(cc2jl('AAAAAAAAAAAAAAAAA'))  #-> aaaaaaaaaaaaaaaaa

Finally, some thoughts on avoiding the try block that bothers me personally.

We can leverage from the short-circuiting used in Python's logical expressions and (re-)write the following:

def cc2jl(my_str):
  """Camel case to joint-lower"""

  r = my_str[0].lower()
  for i, letter in enumerate(my_str[1:], 1):
    if letter.isupper():
      if my_str[i-1].islower() or (i != len(my_str)-1 and my_str[i+1].islower()):
        r += '_'
    r += letter.lower()
  return r

Note the i != len(my_str)-1 on the left side of the and. If this returns False the my_str[i+1] is not evaluated at all and thus cannot raise the IndexError.


Computing the item price and vat for a given pricing, order, and exchange rate How can I improve the readability of the below function using a list comprehension?
Also, is there a way to improve items() performance?
pricing = {'prices': [{'product_id': 1, 'price': 599, 'vat_band': 'standard'},
                      {'product_id': 2, 'price': 250, 'vat_band': 'zero'},
                      {'product_id': 3, 'price': 250, 'vat_band': 'zero'}],
           'vat_bands': {'standard': 0.2, 'zero': 0}}
order = {'order': {'id': 12, 'items': [{'product_id': 1, 'quantity': 1}, {'product_id': 2,'quantity': 5}]}}
exchange_rate = 1.1

def items():
    """
    computes the item price and vat for a given pricing, order, and exchange rate.
    returns list of items dictionaries
    """
    return [{'product_id': item['product_id'],
             'quantity': item['quantity'],
             'price': round(product['price'] * exchange_rate, 2),
             'vat': round(pricing['vat_bands']['standard'] * product['price'] * exchange_rate, 2)}
            if product['vat_band'] == 'standard' else
            {'product_id': item['product_id'],
             'quantity': item['quantity'],
             'price': round(product['price'] * exchange_rate, 2),
             'vat': 0}
            for item in order['order']['items'] for product in pricing['prices']
            if item['product_id'] == product['product_id']]

print(items())

Output:
[{'product_id': 1, 'quantity': 1, 'price': 658.9, 'vat': 131.78},
 {'product_id': 2, 'quantity': 5, 'price': 275.0, 'vat': 0}]

As a general rule it is best not to nest comprehensions. It makes code hard to read. You are better off just writing a for loop and appending the results to a list or using a generator.
Here are a couple rules with comprehensions that will make your code less brittle and easy for others to work with:

Don't nest comprehensions.
If a comprehension is too long for one line of code, don't use it.
If you need an else, don't use a comprehension.

Of course there are exceptions to these rules, but they are a good place to start.
One of the reasons nested list comprehension is an issue is it often results in a exponential increase in computation needed. For each item in the order you have to loop through every product. This is not efficient. You want to go from O(n x m) to O(n + m). You should loop through products once and through order items once.
You can see in the updated code below that I loop through the list of products and create a dictionary with the key as the product ID. This makes it so that, while looping through the order items, I can simply get the product by looking up the key. It is much more performant and readable.
pricing = {
    "prices": [
        {"product_id": 1, "price": 599, "vat_band": "standard"},
        {"product_id": 2, "price": 250, "vat_band": "zero"},
        {"product_id": 3, "price": 250, "vat_band": "zero"},
    ],
    "vat_bands": {"standard": 0.2, "zero": 0},
}
order = {
    "order": {
        "id": 12,
        "items": [{"product_id": 1, "quantity": 1}, {"product_id": 2, "quantity": 5}],
    }
}
exchange_rate = 1.1

def calculate_exchange_rate(price, rate=None):
    if rate is None:
        rate = exchange_rate
    return round(price * rate, 2)

def items():
    """
    computes the item price and vat for a given pricing, order, and exchange rate.
    returns list of items dictionaries
    """
    item_list = []
    products = {p["product_id"]: p for p in pricing["prices"]}

    for item in order["order"]["items"]:
        product = products.get(item["product_id"])
        vat = 0
        if product["vat_band"] == "standard":
            vat = pricing["vat_bands"]["standard"] * product["price"]
        item_list.append(
            {
                "product_id": item["product_id"],
                "quantity": item["quantity"],
                "price": calculate_exchange_rate(product["price"]),
                "vat": calculate_exchange_rate(vat),
            }
        )
    return item_list

print(items())


Optimizing HLSL shader I am trying to optimize my vertex and pixel shader code. Do you have any suggestions what could I possibly do to improve performance? I am using it to draw textured cuboids, and if there are many objects in the screen my framerate drops.

VERTEX SHADER

        float4x4 WorldViewProj                      : register(c0);
        float4x3 WorldInversedMatrix                : register(c4);
        float4x4 World                              : register(c7);
        float3 viewPosition                         : register (c11);
        float3 dirLightDir                          : register(c12);
        float xCoordMultiplication                  : register (c13);
        float yCoordMultiplication                  : register (c14);

        struct VsInput
        {
            float4 position : POSITION;
            float3 normal   : NORMAL;
            float2 texCoord : TEXCOORD;
        };

        struct VsOutput
        {
            float4 position : POSITION;
            float3 dirLightPosition : COLOR;
            float2 texCoord : TEXCOORD0;
            float3 normal   : TEXCOORD1;
            float3 view : TEXCOORD2;
        };

        VsOutput main(VsInput IN)
        {
            VsOutput OUT;

            OUT.position = mul(IN.position, WorldViewProj);

            //if X/YCoordMultiplication != 1, then texture is multiplicated (for example when drawing tiled floor, and my texture is single tile
            OUT.texCoord = float2(IN.texCoord.x * xCoordMultiplication,
                                  IN.texCoord.y * yCoordMultiplication);
            OUT.normal = mul(WorldInversedMatrix, IN.normal);

            float3 worldPosition = mul(IN.position, World).xyz;
            OUT.view = viewPosition - worldPosition;

            OUT.dirLightPosition = mul(dirLightDir, World);
            return OUT;
        }

PIXEL SHADER

float4 filterColor              : register (c1);
float3 specularLightIntensity   : register (c2);

texture texTexture;
sampler textureSampler  : register(s0) = sampler_state {
    Texture = (texTexture);
    Filter = MIN_MAG_MIP_POINT;
    AddressU = Wrap;
    AddressV = Wrap;
};

struct VsOutput
{
    float4 position : POSITION;
    float3 dirLightPosition : COLOR;
    float2 texCoord : TEXCOORD0;
    float3 normal   : TEXCOORD1;
    float3 view : TEXCOORD2;
};

float4 main(VsOutput IN) : COLOR
{

    float4 texColor = tex2D(textureSampler, IN.texCoord);

    float3 specular = specularLightIntensity;
    float materialPower = 9.0f;

    float3 light = normalize(IN.dirLightPosition);
    float3 view = normalize(IN.view);
    float3 normal = normalize(IN.normal);
    float3 halfway = normalize(light + view);

    float3 processedSpecular = pow(saturate(dot(normal, halfway)), materialPower)
                        * specular;

    float alphaChannel = texColor.a + filterColor.a;

    float4 color = 1.2f * (texColor + filterColor.a * filterColor   + float4(processedSpecular, 0.0f));

    return float4(color.r, color.g, color.b, filterColor.a + texColor.a);
}

Late response to this, but all I can suggest is leveraging the per-component math of HLSL better in your shaders...

Vertex Shader

    float xCoordMultiplication                  : register (c13);
    float yCoordMultiplication                  : register (c14);

Make that a float2

    float2 xyCoordMultiplier   : register (c13) : register (c14);

I think that's how you tie two different registers to one global. Might want to double check MSDN HLSL reference to be sure, though.

With your xy multiplier now a float 2, you can change this...

        OUT.texCoord = float2(IN.texCoord.x * xCoordMultiplication,
                              IN.texCoord.y * yCoordMultiplication);

... to this ...

        OUT.texCoord = IN.texCoord.xy * xyCoordMultiplier.xy;

You save yourself having to cast another float2 (for the x & y math operations), and you leverage .xy per-component math which is faster then doing it each separately.

In your pixel shader...

float3 processedSpecular = pow(saturate(dot(normal, halfway)), materialPower) * specular;

... HLSL has an intrinsic function called "lit" that you can read more about here... http://developer.download.nvidia.com/cg/lit.html

Basically you hand it a sun luminence dot (NDotL ... dot ( normal, light)), a specular reflection dot (NDotH ... dot ( normal, halfway)) and a specular pow value (your materialPower) it does all the pow, saturate, etc.. basically creates your float specular luminence to * by your float3 specularLightIntensity.

While you still have to calculate the dots in the shader, you'd use the lit function, b/c it replaces the pow,saturate,etc you have with a built-in function doing it more efficiently then you coding it in the shader yourself.

I would also change this stuff around...

float alphaChannel = texColor.a + filterColor.a;

float4 color = 1.2f * (texColor + filterColor.a * filterColor   + float4(processedSpecular, 0.0f));

return float4(color.r, color.g, color.b, filterColor.a + texColor.a);

... you're not using alphaChannel; you're adding filterColor.a + texColor.a in the return again.. so just make your return be...

float4 color;
       color.rgb = 1.2f * (texColor.rgb + filterColor.a * filterColor.rgb   + processedSpecular);
       color.a = texColor.a + filterColor.a;

return color;

... that way you declare 1 float4 return value, and do the math for .rgb and .a separately. B/c as you're doing it in your initial way, you're wasting calculations a) generating a float alphaChannel var that never gets used, b) calculating color.rgba when you just replace color.a in the return anyways (by re-calculating alphaChannel), c) you're casting another float4 in your return, and inefficiently comma-delim'ing your color.r, color.g, color.b values when you can just color.rgb them together.

The idea is to reduce the number of variables casts you make... so while you can "float4 ( somevalue.r, somevalue.g, somevalue.b, somevalue.a )"... if you're already creating a return value, then just use it as the return value instead of recasting it.

Sometimes your return value you're dealing with may just be a float3, eg: if you're only pulling the rgb of a texture, working with it, and the alpha .a will always be 1. Even then.. just cast a float4 from the start, do all your work with the .rgb float3 parts of it, and then set the .a part of it to 1 and return it...

eg:

float4 color;
       color.a = 1;
       color.rgb = tex2d (sample, uv).rgb;
       color.rgb = (complex code that alters it)
       ... ... ...
return color;

You create a single float4, and then just work with it's .rgb through the shader, but return the float4 whole at the end.

You can do this in other places, too... often people will pack HLSL vars full.. eg: they'll take 2 float4's and use the .rgb parts for colors and then pack a float2 in the .a alphas of them...

eg:

struct someObject
{
    float4 sunU; // .rgb = sun vec * light matrix // .a = UV.x
    float4 skyV; // .rgb = sky vec * light matrix // .a = UV.y
}

void vertex_shader ( in blah i, out someObject o)
{
    o.sunU.rgb = suncolor.rgb;
    o.sunU.a = uv.x;
    o.skyV.rgb = skycolor.rgb;
    o.skyV.a = uv.y;
}

I prefer casting a variable once, and then loading it like I did above. But, I see folks double-casting vars all the time, like so...

o.sunU = float4 ( suncolor.rgb, uv.x );
o.skyV = float4 ( skycolor.rgb, uv.y );

.. I mean... it does the same thing: loads the float4's with their values. But, it casts a second float4 on the right-side of the equation to do so. We're casting 4 float4's there.. the sunU and skyV, and then the right-side equations to piece together what we're packing in them. The way I wrote it eariler, you only cast 2 float4's in the struct, then use swizzling to tell them what goes in what parts without having to cast more vars to do so en-masse.

It seems like such a minor thing, and overall it probably is, but when you have something like that in a pixel shader firing off for every pixel... it's just unnecessary overhead ... X many pixels times Y number of frames per second... it adds up.


Simple Kanban component I am looking for some suggestions on how to make my code cleaner and simpler. Or put another way: more elegant and idiomatic. Any other comments on quality or anything would be appreciated.

class App extends Component {
      constructor(props){
        super(props);

        this.state = {
            colIds : ["winnie", "bob", "thomas", "george"],
            cols : [
              {name:"Winnnie", headerColor: "#8E6E95", id : "winnie" },
              {name:"Bob",  headerColor: "#39A59C", id :"bob"},
              {name:"Thomas",  headerColor: "#344759", id: "thomas"},
              {name:"George",  headerColor: "#E8741E", id: "george"}
            ],
            cards : [
              {colid: "winnie", task: "buy eggs"},
              {colid: "bob", task: "clean basement"},
              {colid: "thomas", task: "pack"},
              {colid: "george", task: "pay bills"},
            ]
          }

          this.addCard = this.addCard.bind(this);
          this.moveCardRight = this.moveCardRight.bind(this);
          this.moveCardLeft = this.moveCardLeft.bind(this);
      }

      getColCards(colid){
          let cards = this.state.cards.filter( c => c.colid == colid);
          return cards;
      }

      addCard(colid){
        let cardTask = window.prompt("Whats the task you want to add");
        let currentCards = this.state.cards.slice();
        let newCard = {colid: colid, task: cardTask};
        currentCards.push(newCard);
        this.setState({cards: currentCards});

      }
      moveCardRight(card){
        let currentCardCol = card.colid;
        let nextIndex =  this.state.colIds.indexOf(currentCardCol) + 1;

        if(nextIndex > this.state.colIds.length - 1 ){
          return null;
        }
        let currentCards = this.state.cards.slice();
        for(let i = 0; i < currentCards.length; i++){
            if(currentCards[i].task === card.task){
               currentCards[i] = {
                 ...currentCards[i],
                 colid : this.state.colIds[nextIndex]
               }
            }
        }
        this.setState({cards: currentCards});
      }

      moveCardLeft(card){
        let currentCardCol = card.colid;
        let nextIndex =  this.state.colIds.indexOf(currentCardCol) - 1;

        if(nextIndex < 0 ){
          return null;
        }
        let currentCards = this.state.cards.slice();
        for(let i = 0; i < currentCards.length; i++){
            if(currentCards[i].task === card.task){
               currentCards[i] = {
                 ...currentCards[i],
                 colid : this.state.colIds[nextIndex]
               }
            }
        }
        this.setState({cards: currentCards});
      }
      render() {
        let cols =[
                      {name:"Winnnie", headerColor: "#8E6E95"},
                      {name:"Bob",  headerColor: "#39A59C"},
                      {name:"Thomas",  headerColor: "#344759"},
                      {name:"George",  headerColor: "#E8741E"}
        ];
        let colCards = this.state.cols.map(c => {
        let cards = this.getColCards(c.id).map(c =>  {
            return

                     this.moveCardLeft(c)}> {"<   "}
                    {c.task}
                     this.moveCardRight(c)}> {"    >"}

          })

          return  <CardCol  name={c.name}  headerColor={c.headerColor} addCard={this.addCard} id={c.id}>
                     {cards}
                  </CardCol>
        })
        return (

          {colCards}

        );
      }
    }

Your implementation is great and works well, but there are some things you could change.

Eliminate repetitive code

You have two large methods, moveCardLeft and moveCardRight, that do almost the same thing. These can either be combined entirely or the matching code can be separated out to a new method and referenced by the original two. For my example, I chose the former:

moveCard(card, dir) {
  let nextIndex = this.state.colIds.indexOf(card.colid);

  if (dir === "left") nextIndex--;
  else if (dir === "right") nextIndex++;
  else return null;

  if (nextIndex < 0 || nextIndex > this.state.colIds.length - 1) {
    return null;
  }

  let currentCards = this.state.cards.slice();
  for (let i = 0; i < currentCards.length; i++) {
    if (currentCards[i].task === card.task) {
      currentCards[i] = {
        ...currentCards[i],
        colid: this.state.colIds[nextIndex]
      }
    }
  }
  this.setState({ cards: currentCards });
}

With this, you just have to pass in an extra parameter for the direction (left or right). It eliminates the repetition which is almost always beneficial.

Use consistent formatting

This includes indentation, new lines, and general spacing.

As you have it written in your post (maybe this is just a copy/paste error), the map inside of a map in your render method is not indented properly, which will undoubtedly confuse you at some point. Here is what it should look like:

render() {
  let cols = [
    { name: "Winnnie", headerColor: "#8E6E95" },
    { name: "Bob", headerColor: "#39A59C" },
    { name: "Thomas", headerColor: "#344759" },
    { name: "George", headerColor: "#E8741E" }
  ];
  let colCards = this.state.cols.map(c => {
    let cards = this.getColCards(c.id).map(c => {
      return (

           this.moveCard(c, "left")}>{"<   "}
            {c.task}
           this.moveCard(c, "right")}>{"    >"}

      );
    });

    return (
      <CardCol name={c.name} headerColor={c.headerColor} addCard={this.addCard} id={c.id}>
        {cards}
      </CardCol>
    );
  });

  return (

      {colCards}

  )
}

Other thoughts

Use strict equality (===) by default and only fall back to loose/abstract equality (==) when absolutely necessary.
Consider how much more concise your code could be if you got rid of the colIds and simply used the cols instead to organize the cards. Additionally, consider placing the cards inside of their respective cols instead of separating them out into their own object. I haven't tried this on your code, so it may end up being less concise, but it's good to consider various data structures in anything you write.


Simulate a person who is working in a quarry with classes I build my first "bigger" program with classes. Does this look good? Or do you see something, which I could do better?
Do you think I could use a delegate for changing the stones to money here? And how to do this?

using System;

// collect stones
// if 20 stones collected --> event --> change stones for money

namespace FunInTheQuarry
{
    class MainClass
    {
        static void Main(string[] args)
        {
            Quarry quarry = new Quarry();
            Person person1 = new Person();

            for (int i = 0; i < 1000; i++)
            {
                System.Threading.Thread.Sleep(300);
                person1.Work();
                Console.WriteLine("{0} stones in the quarry.", Quarry.stone);
            }
            Console.Read();
        }
    }

    public class Quarry
    {
        public static int stone { get; set; }

        public Quarry()
        {
            stone = 10000;
        }
    }

    public class Person
    {
        public int stone;
        public int money;

        public Person()
        {
            stone = 0;
            money = 0;
        }

        public void Work()
        {
            stone += 1;
            Quarry.stone -= 1;
            if (this.stone >= 20)
            {
                ChangeStonesToMoney(ref this.stone, ref this.money);
            }
        }

        public void ChangeStonesToMoney(ref int stone, ref int money)
        {
            stone -= 20;
            money += 1;
            Console.WriteLine("One Person has now {0} dollar.", this.money);
        }

    }
}

EDIT: this is my revised work.

http://pastebin.com/99gRrUqv

Before worrying about delegates, there are some very basic notions we should review! :)

Never, ever getting back together expose your fields. They should be private. Use properties like you did in your Quarry class instead.
This pattern is called encapsulation and is an important practice in the software development process.

In general, an instance (like new Quarry()) shouldn't deal with static fields. Why is that? Imagine the following scenario where you have 2 quarries.

Quarry quarry = new Quarry();
quarry.stone -= 100; //Somebody's been working hard
Quarry quarry2 = new Quarry();
Console.WriteLine(quarry.stone);

What is printed? Is it 9900? Nope, because your instance constructor resets the static field. The stone field shouldn't be static. Each Quarry should have its own number of stones. Maybe the number of stones could be customized with a parameter in the constructor?

public class Quarry
{
    public int Stones { get; set; }

    public Quarry(int stones)
    {
        Stones = stones;
    }
}

This way each of your Quarry is independant.

Next problem, there is no relation between your Person and your Quarry. This is bad. A Person works in a Quarry (in your example, we'll keep it simple), but in your code, we don't see this relation. To keep it simple, we'll say that a Person Works with a Quarry. What does it mean? It means that the Work method should receive a Quarry parameter.

public class Person
{
    public int stone;
    public int money;

    public Person()
    {
        stone = 0;
        money = 0;
    }

    public void Work(Quarry quarry)
    {
        stone += 1;
        quarry.Stones -= 1;
        if (this.stone >= 20)
        {
            ChangeStonesToMoney(ref this.stone, ref this.money);
        }
    }

    public void ChangeStonesToMoney(ref int stone, ref int money)
    {
        stone -= 20;
        money += 1;
        Console.WriteLine("One Person has now {0} dollar.", this.money);
    }
}

Notice that stones isn't the best variable name you can use. Perhaps workedStones would be better. Or something like this.

Also, you use Thread.Sleep(300); in your main loop, but I assume that it's the person's work that takes 300 milliseconds (damn those persons are fast!). So maybe the sleep should be in the Work method. Using Thread.Sleep() is unorthodox and is usually not recommended, but for your example I get why you're using it! :)

I noticed you were using ref. That's bad. I mean, you used it in a "good" scenario, as you want to modify an int by reference, but you don't need to do this and you shouldn't include ref everywhere just for the kick of it.

You already have access to stone and money, so why pass them as parameter?

public void Work()
{
    stone += 1;
    Quarry.stone -= 1;
    if (this.stone >= 20)
    {
        ChangeStonesToMoney();
    }
}

public void ChangeStonesToMoney()
{
    stone -= 20;
    money += 1;
    Console.WriteLine("One Person has now {0} dollar.", this.money);
}

Now, I don't have time to go for a longer review, but I suggest you think about the following points :

Why is the Person class responsible for knowing how many stones are worth a dollar? Shouldn't another class be responsible for this?
Shouldn't a Quarry have a list of Person that works for it? Instead of having different people work on a quarry once in awhile like it is actually done at the moment.
Are these people under the minimum wage?

Otherwise, there's some good stuff in there, you've done a good job for a first little application :)