Selecting product variants that satisfy all option criteria My current query:

select `t1`.* from `product_variant_values` as t1
inner join (
    select `t1`.* from `product_variant_values` as t1
    where (
    (t1.`option_id` = 1 and t1.`value_id` = 1) or
    (t1.`option_id` = 2 and t1.`value_id` = 4) or
    (t1.`option_id` = 3 and t1.`value_id` = 7) )
    group by t1.variant_id having count(t1.id) = 3
) t2 on t2.variant_id = t1.variant_id

I have this query, and I want to know how I can get rid of the second select and just use a simple join? Here is a link to a picture of my main table: http://prntscr.com/99sxp8 . I set where condition for option_id - value_id pair and want to select those group of rows which have the same variant_id and number of rows is 3.

Update: I do not know the amount of needed rows. I just have an array of needed arguments:

.

The keys are option_id and the value is value_id. These will differ, and I want to select rows which match the arguments, and the number of rows must be equal to the count of options (or count($arguments)) and all rows must have the same variant_id, which is what I'm looking for.

Update 2: Here is the full picture of my tables.

The products table

The Product variants table

Here stock and price will be real numbers.

The Options table

Also I have option_group table, but it consists of just an id and a name. It is as option set. One product will have only one option group.

The table variantable_option_variants

These are values for options.

The product_variant_values table

This table contains all possible combination for option values and parent variant. One example: "red color - s size - long height", matches option_id/value_id set of (1:1, 2:4, 3:7).

I want to find variant_id (from product_variant_values) by a set of combinations of option_id and value_id.

Update 2: The combination for each option, option_id and value_id really exists. They are created when admin saves the option group.

User have dropdown lists for each of the different options. And when he submits the form, I get the combinations of options_id's and value_id's that should be enough to find the variant_id.

I'm not quite sure I understood your requirements, but as I understand it you want to find the variant id for a set of values with the values 1, 4, 7 for the three options.
In other words, you need to do a double join in order to access the three different values, this can be achieved using some thing like this:
SELECT t1.variant_id, t1.value_id t1v, t2.value_id t2v, t3.value_id t3v
  FROM product_variant_values as t1
       JOIN product_variant_values AS t2
         ON t1.variant_id = t2.variant_id AND t2.option_id = 2 AND t1.option_id = 1
       JOIN product_variant_values AS t3
         ON t1.variant_id = t3.variant_id and t3.option_id = 3

When running this query as is, using a temporary table built from variant_id's up to 7 from http://prntscr.com/99sxp8, I got the following output:
variant_id t1v t2v t3v
1           1   4   7
2           1   4   8
3           1   4   9
4           1   5   7
5           1   5   8
6           1   5   9
7           1   6   7

Notice in this output how I've connected the t1 table to option_id=1, and t2 to option_id=2, and so on. Therefore when adding this line:
 WHERE t1.value_id = 1 AND t2.value_id = 4 AND t3.value_id = 7

One can limit the query to a given value set:
variant_id t1v t2v t3v
1           1   4   7

In other words, variant_id = 1 is the only one having the combinations of values 1, 4, 7.
Update: Generalisation to multiple options
The method above could be extended to any number of options, but the cost of it expands for each option you add.
Another option for solving this kind of problem on this end, is to make n different selects, and then doing a intersection of matching variant_id's. If your final intersection has only one element, you've found your variant_id, and if no or more than one element you need to handle that accordingly.
Addendum: Ultimate solution
The solution above was what I came up with when focusing on the join part of your question, and your request to remove the second query. When thinking a little more on it, it's the first query you want to remove. Try this one on for size:
SELECT count(*), variant_id FROM product_variant_values
 WHERE (option_id = 1 AND value_id = 1)
    OR (option_id = 2 AND value_id = 4)
    OR (option_id = 3 AND value_id = 7)
GROUP BY variant_id
HAVING COUNT(variant_id) = 3

If you get more than one or none resulting row out of this query, you have a problem. This is also easily extensible, as you just need to change the matching option count on the HAVING COUNT section and add appropriate extra groups on the OR parts.


Linear Regression and data manipulation How could I improve the following code that runs a simple linear regression using matrix algebra?  I import a .csv file (link here) called 'cdd.ny.csv', and perform the matrix calculations that solve for the coefficients (intercept and regressor) of Y = XB (i.e.,  $(X'X)^{-1}X'Y$):

import numpy
from numpy import *
import csv

df1 = csv.reader(open('cdd.ny.csv', 'rb'),delimiter=',')
tmp = list(df1)
b = numpy.array(tmp).astype('string')
b1 = b[1:,3:5]
b2 = numpy.array(b1).astype('float')

nrow = b1.shape[0]
intercept = ones( (nrow,1), dtype=int16 )

b3 = empty( (nrow,1), dtype = float )

i = 0
while i < nrow:
    b3[i,0] = b2[i,0]
    i = i + 1

X = numpy.concatenate((intercept, b3), axis=1)
X = matrix(X)

Y = b2[:,1]
Y = matrix(Y).T

m1 = dot(X.T,X).I
m2 = dot(X.T,Y)

beta = m1*m2

print beta

#[[-7.62101913]
# [ 0.5937734 ]]

To check my answer:

numpy.linalg.lstsq(X,Y)

import numpy
from numpy import *
import csv

df1 = csv.reader(open('cdd.ny.csv', 'rb'),delimiter=',')
tmp = list(df1)
b = numpy.array(tmp).astype('string')
b1 = b[1:,3:5]
b2 = numpy.array(b1).astype('float')

Firstly, I'd avoid all these abbreviated variables. It makes it hard to follow your code. You can also combine the lines a lot more

b2 = numpy.array(list(df1))[1:,3:5].astype('float')

That way we avoid creating so many variables.

nrow = b1.shape[0]
intercept = ones( (nrow,1), dtype=int16 )

b3 = empty( (nrow,1), dtype = float )

i = 0
while i < nrow:
    b3[i,0] = b2[i,0]
    i = i + 1

This whole can be replaced by b3 = b2[:,0]

X = numpy.concatenate((intercept, b3), axis=1)
X = matrix(X)

If you really want to use matrix, combine these two lines. But really, its probably better to use just array not matrix.

Y = b2[:,1]
Y = matrix(Y).T

m1 = dot(X.T,X).I
m2 = dot(X.T,Y)

beta = m1*m2

print beta


Project Euler solution #2 using Swift 3 I just wanted some review and opinions. Any better implementations or suggestions would be great.

Each new term in the Fibonacci sequence is generated by adding the previous two terms. By starting with 1 and 2, the first 10 terms will be:
1, 2, 3, 5, 8, 13, 21, 34, 55, 89, ...
By considering the terms in the Fibonacci sequence whose values do not exceed four million, find the sum of the even-valued terms.

var sequence = [1,2]

var index = 1
var sum = 0

repeat {
    sequence.append(sequence[index-1] + sequence[index])
    index = index + 1
} while sequence[sequence.endIndex - 1] < 4000000

sequence.popLast()

for numbers in sequence {
    if (numbers % 2 == 0) {
        sum += numbers
    }
}
print(sum)

This post from Martin R is a good step in the right direction toward generalizing your problem.

The helper function at the top of the post is already what you need for making a more generalized loop:

public func sequence<T>(first: T, while condition: @escaping (T)-> Bool, next: @escaping (T) -> T) -> UnfoldSequence<T, T> {
    let nextState = { (state: inout T) -> T? in
        // Return `nil` if condition is no longer satisfied:
        guard condition(state) else { return nil }
        // Update current value _after_ returning from this call:
        defer { state = next(state) }
        // Return current value:
        return state
    }
    return sequence(state: first, next: nextState)
}

And the first example he uses with the above function is also a good hint toward what you need:

for f in sequence(first: (0, 1), while: { $1 <= 50 }, next: { ($1, $0 + $1)}) {
    print(f.1)
}

As written, that loop prints the all the Fibonacci numbers up to 34, which is the largest Fibonacci number under 50.

So... how can we generalize this further?

First, let's make a function that simply sums and returns the value:

func sumFibonacciEvens(limit: Int) -> Int {
    // TODO: Implement
}

This is the basic form we want.  Now, after implementing, we'd just pass in 4 million to get the answer we want, but we can easily reuse for any limit.

I'd say let's go one further though.  Let's also make the "even only" part of this generic.

func sumFibonacciNumbers(lessThan limit: Int, condition: (Int) -> Bool) -> Int {
    // TODO: Implement
}

Now then, for this function, we pass in the limit (again, for the case we care about, four million).  But we also pass in a closure that defines what elements to sum and what elements to ignore.  So for your case, calling this function would look like:

let isEven: (Int) -> Bool = { number in
    return number % 2 == 0
}

let answer = sumFibonacciNumbers(lessThan: 4_000_000, condition: isEven)

This looks pretty good, right?  So... how do we do it?

We're going to keep the sequence function defined earlier exactly as is and use it in our sumFibonacciNumbers function, which should look something like this:

public func sumFibonacciNumbers(lessThan limit: Int, condition: (Int) -> Bool) -> Int {

    var sum = 0
    for f in sequence(first: (0,1), while: { $1 <= limit }, next: { ($1, $0 + $1)}) where condition(f.1) {
        sum += f.1
    }

    return sum
}


JavaScript 8-bit floating point To start, I am in the wrong language and I think it is time learn some C++ and compile it as an add-on for NodeJS. For now, though, I have a few code snippets that work that may be interesting or may be an abomination of coding.

This code works with weather forecast data, specifically the GRIB-2 file format. GRIB-2 compresses weather information into either integers or floating point values depending on the type (this cast is wind gusts). The data has been packed into 8-bit floating point values. I am assuming the GRIB-2 protocol follows floating point arithmetic and we can break our 8-bits into the following sections: 0 000 0000

Code to convert hex: 26 to floating-point: 0.6875:

var bin = makeWhole('100110'); //hex: 26
var fp = nBitFloatPoint(8, bin); //8 is unused but will allow different bit lengths

console.log(fp) // 0.6875

function makeWhole(a){
  var hex = a;
  if(hex.length !== 8){
    hex = prepend(hex);
  }
  return hex;
}

function prepend(hex){
  var diff = 8 - hex.length;
  var str = '';
  for(var i=0; i<diff; i++){
    str += '0';
  }
  return str + hex;
}

function nBitFloatPoint(n, bin){

  var s = bin.charAt(0);
  var e = parseInt(bin.substring(1, 4), 2) - 3;
  var m = '1.' + bin.substring(4, 8);
  var d = shiftDot(m, e);
  var result = convert(s, e, d);

  return result;

}

function convert(s, e, d){

  var strD = d.toString()
  var length, dotIndex, num, frac;
  var values = [];

  length = strD.length;
  dotIndex = strD.indexOf('.');
  num = strD.substring(0, dotIndex).split('');
  frac = strD.substring(dotIndex + 1).split('');

  //console.log(num)
  for(var i=0; i<num.length; i++){
    if(parseInt(num[i])){
      values.push(Math.pow(2, i));
    }
  }

  for(var i=0; i<frac.length; i++){
    console.log(frac)
    if(parseInt(frac[i])){
      //console.log(Math.pow(2, -i))
      values.push(Math.pow(2, -i-1));
    }
  }

  if(values.length){
    return values.reduce(arraySum);
  }
  return 0;
}

function arraySum(prev, curr, index, arr){
  return prev + curr;
}

function shiftDot(m, e){

  var str = '00000000000000000000000000' + m;

  var dot = str.indexOf('.');
  str = str.replace('.', '');

  return parseFloat(str.substring(0, dot + e) + '.' + str.substring(dot + e));
}

Is this a really hack idea that is best left to a language mean to work with binary buffers? Is the function shiftDot a plausible workaround or am I missing something best left to an internal JS function?

To start I know nothing about GRIB-2 that being said I know some ways that you could improve your code.

"Magic Numbers"

Generally you shouldn't put numbers in code where their meaning is unknown. You should set the hanging numbers to constants. Javascript (ES5) doesn't have constants but there are ways to fake it. The number "8" in your code is abstract and one would only know what it is for because you told them. But a without that it would be hard for anyone to know what it means from an initial read through of your code.

Possible Solution - You could create a constant object literal.

var Constants = {
  FLOAT_BIT_LENGTH: 8
}
console.log(Constants.FLOAT_BIT_LENGTH);

Polluting the Global Namespace

As it goes right now your code is putting a bunch of functions and variables into the Global namespace. You should encapsulate your code in a self invoking function, so you can set variables that are global to your code without foo-ing with the global variable scope, if you want other people to be able to embed your code as a module in someone else’s code.

Possible Solution - Self invoking anonymous function to create scope

(function (document) {
 /* code goes here */
})(document);

Non-Descriptive Variables and Parameters

You should be able to read the method signature and know what it does. convert(s, e, d) means nothing to anyone even knowing what the code is for. You could add documentation blocks via JSDocs, and make your function variables more descriptive.

Possible Solution - JSDocs used along with descriptive function arguments

/**
 * Converts a foo into a foobar using bar as the radix and baz as the offset
 * @param {integer} foo - Does x
 * @param {integer} bar - Does y
 * @param {float} baz - Does z
 * @return {float} - x AND y AND z
 */
convertByRadix(foo, bar, baz)

Not Making Use of Function Expressions in Callbacks

Function expressions can be used as a callback function, they are inline and make your code easier to read when the function is only a line or two and have no side effects. Also you don't need to declare all the parameters of a callback, only the ones you are using.

return values.reduce(function(prev, curr) {
  return prev + curr;
});

It may also be worth looking into typed values (that are not boxed) that are available in some Javascript compilers/libraries, like "ASM.js" or "LLJS". As well as looking into possibly using some of Javascripts bitwise operators. Also look into running your code through a linter such as JSHint.


Generic test serie for various solutions to the same algorithmic problem I am learning how to use the module unittest for test-driven development. Below is a simple yet practical example of an issue I will have many times: one single problem (e.g. an Abstract Data Type (ADT) describing a Data Type and an Operator on it), and various Data Structures (DS) implementing this ADT; in this case the problem of computing the number of elements smaller than an element x in a sorted array A (i.e. its "insertion rank"), and two solutions: binary search and Fibonacci search. This codes "does the job" in that I can define the tests in one single place for all solutions, and the list of solutions in another one, but it does not seem elegant?

import unittest

class InsertionRankTest(unittest.TestCase):
    def generic_test(self,algorithm):
        self.assertEqual(algorithm([1],0),0)
        self.assertEqual(algorithm([1],1),0)
        self.assertEqual(algorithm([1],2),1)
        self.assertEqual(algorithm([0, 10, 20, 30, 40, 50, 60, 70, 80, 90],0),0)
        self.assertEqual(algorithm([0, 10, 20, 30, 40, 50, 60, 70, 80, 90],40),4)
        self.assertEqual(algorithm([0, 10, 20, 30, 40, 50, 60, 70, 80, 90],45),5)
        self.assertEqual(algorithm([0, 10, 20, 30, 40, 50, 60, 70, 80, 90],100),10)

    def test_all_inertionRankAlgorithms(self):
        self.generic_test(binarySearchForInsertionRank)
        self.generic_test(fibonacciSearch)

def binarySearchForInsertionRank(A,x):
    """Computes the insertion rank r of x in A in time logarithmic in
    len(A).

    """
    assert(A!=None)
    if len(A)==0:
        return 0
    else:
        m = len(A)//2
        if x <= A[m]:
            return binarySearchForInsertionRank(A[:m],x)
        else:
            return m+1+binarySearchForInsertionRank(A[m+1:],x)

def fibonacciSearch(A,x):
    """Computes the insertion rank r of x in A in time logarithmic in
    r.

    """
    assert(A!=None)
    if len(A)==0:
        return 0
    l = 0
    r = 1
    while r < len(A) and A[r] < x:
        r += l
        l = r-l # i.e., former value of r
    return l + binarySearchForInsertionRank(A[l:min(r,len(A))],x)

def main():
    unittest.main(exit=False)
if __name__ == '__main__':
    main()

Does one define a class for the ADT, a class for the tests, derives the class for the ADT for each DS, and derives the test classes again? (As a bonus question: can the same tools be used to measure and compare the performance (as measured by either the number of comparisons or the running time)? What is the best way to measure the performance of each solution on each test case?)

Embrace Python's simplicity! This is a dynamic language, we care less about inheritance and complex data models and focus more on the actual problem. Of course, it has its drawbacks, but that's how it works.

The answer your question: no, in Python, we wouldn't derive classes and so on but just write the code you wrote. It's still possible to define an abstract base class A with specific methods and extend it with a few implementations, but really think about the benefits before doing this.

Note about the code: PEP 8 is a standard style guide important in the Python word: space between operators and no camelCase in variable names are the two that come to mind while reading your code.

Finally, performance is another concern than unit testing (except if you find a reliable way to test for the complexity of your algorithms). I recommend the timeit module. It doesn't say how to test for complexity, but it's a good start to measure time spent in a method.


Hackerrank Sum vs XoR I'm in the process of improving my coding style and performance hence I decided to delve into bit manipulation on Hackerrank. I thought the question was straight forward but on large input, my solution's performance declines. How do I improve it? Any smart tricks to solve the below question is welcome as well.

Below is the question

  Given an integer, \$n\$ , find each \$x\$ such that:

  \$0 \leq x \leq n \$

  \$ n + x = n \oplus x \$

  where \$\oplus\$ denotes the bitwise XOR operator. Then print an
  integer denoting the total number of \$x\$ 's satisfying the criteria
  above.

  Input Format

  A single integer, \$n\$ .

  Constraints

  \$0 \leq n \leq 10 ^ {15}\$

  Subtasks

  \$0 \leq n \leq 100\$ for \$60\%\$ of the maximum score

  Output Format

  Print the total number of integer \$x\$ 's satisfying both of the
  conditions specified above.

  Sample Input 0

5

  Sample Output 0

2

  Explanation 0

  For \$n = 5\$ , the \$x\$ values \$0\$  and \$2\$ satisfy the
  conditions:

  Thus, we print \$2\$ as our answer.

Here is my code

using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
class Solution {
     static int SumVsXoR(long number)
    {
        int count =0;
        long j = 0;
        while (j <= number)
        {
            if(j + number == (j^number)){
                count++;
            }
            j++;
        }
        return count;
    }

    static void Main(String[] args) {
        long n = Convert.ToInt64(Console.ReadLine());
        Console.WriteLine(SumVsXoR(n));
    }
}

Let's notice that
\$ n + x = n \oplus x \$  is true
when and only when
\$ n + x = n \vee x \$  is true

This is because
\$ n + x \geq  n \vee x = n \oplus x + n \wedge x \$.

Where:

\$ \vee \$ - bitwise OR
\$ \wedge \$ - bitwise AND

So we just need to calculate the number of zero bits in the \$n\$.
Let's calculate the nonzero bits:

private static int NumberOfSetBits(ulong i)
{
    i = i - ((i >> 1) & 0x5555555555555555UL);
    i = (i & 0x3333333333333333UL) + ((i >> 2) & 0x3333333333333333UL);
    return (int)(unchecked(((i + (i >> 4)) & 0xF0F0F0F0F0F0F0FUL) * 0x101010101010101UL) >> 56);
}

The method above was copy-pasted from this answer: stackoverflow.com

Then rewrite the SumVsXoR method:

private static int SumVsXoR(ulong number)
{
    int numberOfBits = (int)Math.Log(number, 2) + 1;
    return 1 << (numberOfBits - NumberOfSetBits(number));
}


Is this an optimal implementation of merge sort? I am taking an algorithm course and we are to implement merge sort that handles list of elements with even or odd number of elements and handles duplication, so I wrote this function:

void mergesort (int* list, int len)
{
        if(len == 1) return;
        int i = len/2, j = len-i;
        int list1[i], list2[j];
        for(int k=0;k<i;k++)
        {
                list1[k]= list[k];
                list2[k]= list[i+k];
        }
        if(len%2!=0) list2[j-1] = list[len-1];
        mergesort(list1 , i);
        mergesort(list2 , j);
        int k=0,l=0;
        // k represent counter over elements in list1
        // l represent counter over elements in list2
        // k+l represent counte over total elements in list
         while(k+l!=len)
        {
                if(k==i)
                {
                        for(;l<j;l++) list[k+l] = list2[l];
                        return;
                }
                else if (l==j)
                {
                        for(;k<i;k++) list[k+l] = list1[k];
                }
                else if(list1[k]<list2[l])
                {
                        list[k+l]=list1[k];
                        k++;
                }
                else if(list1[k]>list2[l])
                {
                        list[k+l] = list2[l];
                        l++;
                }
                else
                {
                //handles dublication
                list[k+l] = list1[k];
                k++;
                list[k+l] = list[l];
                l++;
                }
        }
}

I have 2 questions:

How can I make this implementation more optimal (best possible performance)?
When handling arrays of large lengths (1000000), what causes a segmentation fault?

NOTE: I tried the function using array randomly generated of length 1000 and it worked.

Suspect that segmentation fault on large arrays occurs because the list1[] and list2[] ran out of space.  With the recursive calls, code is heavily using the stack space.  Use malloc() and free() for large arrays instead of VLA[]
Memory allocation could be reduced.  Via recursion, this takes > 2n (maybe 4n) memory space.  At worst it should be 2n.
Use size_t rather than int for a integer type that can handle all array indexes.

// void mergesort (int* list, int len)
void mergesort (int* list, size_t len)

Cope with 0 length.

// if(len == 1) return;
if(len <= 1) return;


Running a MySQL query with an admin class This is my admin class, DB class and how I use OOP.

I am looking for ways to improve my code to make better use of OOP. Please help me if you think I can improve my code in some way.

db.class.php

class database
{
    private $conn;
    private $db_name = 'profil_penduduk';
    private $db_user = 'root';
    private $db_pass = '';
    private $db_host = 'localhost';

    public function connect()
    {

        $this->conn = null;
        try
        {
            $this->conn = new PDO("mysql:host=$this->db_host;dbname=$this->db_name", $this->db_user, $this->db_pass);
            $this->conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
        }
        catch(PDOException $exception)
        {
            echo "Connection error: " . $exception->getMessage();
        }

        return $this->conn;
    }
}

admin.class.php

class admin
{
    private $conn;

    function __construct()
    {
        # code...
        $database = new database();
        $db = $database->connect();
        $this->conn = $db;
    }

    public function runQuery($sql)
    {
        # code...
        $stmt = $this->conn->prepare($sql);
        return $stmt;
    }
}

This is how I fetch the data:

index.php

$auth_admin = new admin();

$admin_id = $_SESSION['admin_session'];

$stmt = $auth_admin->runQuery("SELECT * FROM admin WHERE admin_id=:admin_id");
$stmt->execute(array(":admin_id"=>$admin_id));

$adminRow=$stmt->fetch(PDO::FETCH_ASSOC);

Is there a better way for me to do this? Should I use CRUD? Or should I make a function with this query:

"SELECT * FROM admin WHERE admin_id=:admin_id"

inside the admin class?

Should I also use interface in my class? I don't understand much about interfaces, either.

@Max already did a good job at describing how you could write better code.

Here is what is wrong with your current approach:

The main problem is that your classes are tightly coupled. All the files you posted need to be aware that you are using PDO, which isn't good (it's hard to read and maintain).

Your admin class also doesn't make any sense. It doesn't contain code that is specifically concerned with anything related to admins, it just contains part of the database access code (but for some odd reason not all of it).

So what I would do now is:

keep all the PDO calls in one place. There really isn't a good reason to have prepare in one place, and execute and fetch in a completely different place.
remove the generic runQuery method from admin, as it's not specific to admins.
move the admin select query into a method called getById inside the admin class.

Doing this, you basically arrive at the code @Max posted.

Misc

Class names should be upper-case.
Don't echo in classes, it makes them difficult to reuse. Just throw the exception upwards and deal with it later.
The way connect is written, you would use a new connection for each object. For example, if you not only had an admin class, but also a SomeData class, it would also call connect, creating a new connection. It's better to reuse the same connection across the request. So either pass the connection to the admin constructor, or to the getById method.
Try to be consistent with your names. Examples are admin_session vs admin_id, auth_admin vs admin, or conn vs db. These inconsistencies do decrease readability, as a reader has to think about them each time.
Either use camelCase or snake_case, don't mix them.
Your spacing is off.


Given a Tic-Tac-Toe gamestate return all wins as text In a game of Tic-TacToe given a gamestate:
game_board = [ [1, 0, 1],
               [0, 1, 0],
               [0, 1, 0] ]

I have written the following piece of code to display the output of the gamestate in regards to wins:
game_board = [ [1, 1, 1],
               [0, 1, 0],
               [0, 1, 0] ]

# Horizontals
h = [str(i+1) + ' Row' for i, v in enumerate(game_board) if sum(v) == 3]

# Verticals
v = [str(i+1) + ' Col' for i in range(3) if sum([j[i] for j in game_board]) == 3]

# Diagonals
d = [['Left Diag', '','Right Diag'][i+1] for i in [-1, 1] if sum([game_board[0][1+i], game_board[1][1]], game_board[2][1-i]) == 3]

if any([h,v,d]):
    print('You won on:', h, v, d)
else:
    print('No win yet')

The output of this is:

You won on: ['1 Row'] ['2 Col'] []

I would like to know how my solution could be written in a more Pythonic manner?

Not sure about whether a solution could be more Pythonic, but I think it can be more efficient.  At present your code is iterating over each cell twice, before anything is printed.
I would suggest using a collection of possible winning strings.  You only need to iterate over the cells once and each time you encounter a 0 that row, col and if applicable, diagonal become invalid as a winning line.
After that, it's a simple matter of iterating over the strings and printing the ones that are valid.


Finding the longest unique sub-string in a string I recently interviewed with a company for software engineering position. I was asked the question of finding the longest unique sub-string in a string.

My algorithms was as follows:

Start from the left-most character, and keep storing the character in a hash table with the key as the character and the value as the index_where_it_last_occurred. Add the character to the answer string as long as its not present in the hash table. If we encounter a stored character again, I stop and note down the length. I empty the hash table and then start again from the right index of the repeated character. The right index is retrieved from the (index_where_it_last_occurred) flag. If I ever reach the end of the string, I stop and return the longest length.

For example, say the string was abcdecfg.

I start with a, store in hash table. I store b and so on till e. Their indexes are stored as well. When I encounter c again, I stop since it's already hashed and note down the length which is 5. I empty the hash table, and start again from the right index of the repeated character. The repeated character being c, I start again from the position 3 ie., the character d. I keep doing this while I don't reach the end of string.

I know there could be better algorithms than this. But I am interested in knowing what the time complexity of this algorithm will be.  I believe it'll be \$O(n^2)\$.

import java.util.*;
public class longest
{
    static int longest_length = -1;
    public static void main(String[] args)
    {
        Scanner in = new Scanner(System.in);
        String str = in.nextLine();
        calc(str,0);
        System.out.println(longest_length);
    }

    public static void calc(String str,int index)
    {
        if(index >= str.length()) return;
        int temp_length = 0;
        LinkedHashMap<Character,Integer> map = new LinkedHashMap<Character,Integer>();
        for (int i = index; i<str.length();i++)
        {
            if(!map.containsKey(str.charAt(i)))
            {
                map.put(str.charAt(i),i);
                ++temp_length;
            }
            else if(map.containsKey(str.charAt(i)))
            {
                if(longest_length < temp_length)
                {
                    longest_length = temp_length;
                }
                int last_index = map.get(str.charAt(i));
//              System.out.println(last_index);
                calc(str,last_index+1);
                break;
            }
        }
        if(longest_length < temp_length)
            longest_length = temp_length;
    }
}

static int longest_length = -1;

No. Static is useless here and makes you method much less usable. A non-static variable would be better, but actually, why not return a result???

There's no longest_length in Java. Maybe longestLength.

Better, return the substring, the length can be easily obtained from it.

Separated printing and computation, good.

if(index >= str.length()) return;

Spacing (after if). Also according to conventions, you should always use braces (I personally don't).

LinkedHashMap<Character,Integer> map = new LinkedHashMap<Character,Integer>();

Use Java 7 diamonds or Guava's Maps.newLinkedHashMap() to save yourself repeated generics.

You need no linked map.

calc(str,last_index+1);

I find the recursion confusing here. You really do nothing, but a simple loop, so use a loop (Even a goto would be clearer than recursion here).

The algorithm indeed runs in O(n*n), which can be shown with a string like

abcde.... abcde....

where you always run through half of the string. I was assuming an unlimited alphabet here, more precisely the complexity is O(n*alphabetSize) as no runs can be longer than alphabetSize.

A much better O(n) algorithm would keep track of the starting and ending positions. Wherever you see a duplicate, advance the start. Sounds damn simple.


Windowed Sieve of Eratosthenes in Java I wanted to try and use the Sieve of Eratosthenes to find all prime numbers between some arbitrary bounds 1 <= m <= n. The simple implementation of the sieve does not consider the lower bound at all, it always starts from 1 (or actually from 2). So for a big enough n, simply creating an array would be impossible.

The algorithm first finds all prime numbers from 1 to sqrt(n), then uses those numbers to find all primes in the given range.

I'd like to know if:

I'm using more memory than necessary
I'm unnecessarily repeating some operations
I can improve the style of this code

Note: I am not validating user input for simplicity sake.

import java.util.*;

public class PrimeLister {

    private static ArrayList<Integer> segmentSieve(int upperBound) {
        boolean[] primes = new boolean[upperBound + 1];
        Arrays.fill(primes, true);
        ArrayList<Integer> numbers = new ArrayList<>();

        for (int i = 2; i <= upperBound; i++) {
            if (!primes[i])
                continue;
            for (int j = i * i; j <= upperBound; j += i) {
                primes[j] = false;
            }

            if (primes[i])
                numbers.add(i);
        }

        return numbers;
    }

    private static int findOffset(int start, int prime) {
        for (int i = 0; i < prime; i++)
            if (start++ % prime == 0)
                return i;
        return -1;
    }

    public static void listPrimes(int lowerBound, int upperBound) {
        ArrayList<Integer> segmentPrimes = segmentSieve((int) Math.floor(Math.sqrt(upperBound)));
        int[] offsets = new int[segmentPrimes.size()];
        boolean[] primes = new boolean[1 + upperBound - lowerBound];
        Arrays.fill(primes, true);

        for (int i = 0; i < offsets.length; i++) {
            int tmp = segmentPrimes.get(i);
            offsets[i] = findOffset(lowerBound, tmp);

            for (int j = offsets[i]; j < primes.length; j += tmp) {
                if (!primes[j] || (j + lowerBound) == tmp)
                    continue;
                primes[j] = false;
            }
        }

        for (int i = 0; i < primes.length; i++) {
            if (primes[i] && (i + lowerBound) != 1)
                System.out.println(i + lowerBound);
        }

        System.out.println();
    }

    public static void main(String[] args) {
        Scanner in = new Scanner(System.in);
        int lowerBound = in.nextInt();
        int upperBound = in.nextInt();
        listPrimes(lowerBound, upperBound);
        in.close();
    }
}

What makes your code difficult to read is nonsense (in the sense of 'carries no meaning') like int tmp, or stuff like the condition (j + lowerBound) == tmp. The superfluous parentheses are just noise but j + lowerBound does not make sense at all, as it corresponds to offsetting the current index j into the window by the window's lower bound. lowerBound + j would make sense, as it corresponds to the actual number represented by the jth slot of the window. The fact that operator + is commutative is beside the point; it's humans who must understand your code. And your code is so difficult to understand that you don't even understand it fully yourself! Trying to express code with the greatest possible clarity can be a great aid in understanding the problem under consideration; on the other hand, churning out code when the problem has not been understood achieves the opposite effect.

The problem under consideration has no need of arrays containing offsets into the window. The algorithm just needs one offset during each iteration of the outer loop, just once. The computation of the offset would have warranted some 'hands-free' thinking time before the actual coding.

The first location that needs to be worked on by the inner 'cross off the composites' loop is p * p, where p is the current prime; let's call this start. This value needs to be reduced to an offset within the window being worked on, which is trivial (start - lower_bound) if start >= lower_bound. If start < lower_bound then it's time to unsheath the modulo operator.

// (p is the current prime)

int stride = p << (p & 1);
int start = p * p;
int offset;

if (start >= lower_bound)
{
   offset = start - lower_bound;
}
else
{
   int before_the_segment = (lower_bound - start) % stride;

   offset = before_the_segment == 0 ? 0 : stride - before_the_segment;
}

// ...

An alternative expression for the tricky case would be

offset = (stride - (lower_bound - start) % stride) % stride

but it leaves no clue for the reader as to what's intended... This can be simplified further to drop one module operator, by careful consideration of the magnitudes involved. But it is easy to shoot oneself in the foot. If there's no pressing need for shaving cycles then it's better to leave the simpler - if more verbose - code in place.

The original code uses the prime p itself as stride, which has the effect that all even composites get crossed off multiple times (first during the run with p = 2, and then again during all even steps during the runs with other primes). This means that the code does almost double the amount of work that's necessary.

A simple fix is to use p + p as stride when p <> 2. In effect this amounts to using a two-spoke wheel and ignoring the even spoke entirely except for its one lone prime occupant (the number 2). Another solution - which halves memory consumption - would be to represent only the even numbers in the sieve array and to pull the only even prime out of thin air when needed. Higher-order wheels would further reduce memory consumption and the amount of work done, but they would complicate the code considerably. By contrast, the two-spoke wheel gives a lot of bang while adding only one minuscule complication to the code.


A basic calculator in Python This is a simple calculator I created using my Python basic knowledge. I would like to know any coding techniques to simplify and enhance the code to make it efficient. I would also like to know some advanced techniques. Also, please specify where I can reduce duplication to make it DRY.

import math
import cmath
print("""Choose from
        1. Perform basic BODMAS
        2. Find squareroot
        3. Solve quadratic equation
        4. Find Trignometric values
        5. Find logarithms""")
choose = input("enter required number \n")

if choose == '1':

    def bodmas():
        num1 = float(input("Enter number 1 \n"))
        num2 = float(input("Enter number 2 \n"))
        op= input("Enter add,sub,mul,div \n")
        if op == "add":
            res = num1+num2
        elif op == "sub":
            res = num1 - num2
        elif op == "mul":
            res = num1*num2
        elif op == "div":
            res = num1/ num2
        else:
            res = print("Error")
        return res
    print(bodmas())

elif choose == '2':

    def sqrt_n():
        num1 = float(input("Enter number"))
        res = math.sqrt(num1)
        return res
    print(sqrt_n())

elif choose =='3':

    def quad_s():
        print("Standard form of Quadratic equation (Ax^2 + Bx + C)")
        A = int(input("Enter coefficient A: "))
        B = int(input("Enter coefficient B: "))
        C = int(input("Enter coefficient C: "))
        D = (B**2)-(4*A*C)  # Calculate Discriminant

        sol1 = (-B-cmath.sqrt(D))/(2*A)
        sol2 = (-B+cmath.sqrt(D))/(2*A)

        z =print("The solutions are {0} and {1}" .format(sol1 ,sol2))
        return z
    print(quad_s())
elif choose == '4':

    choice = input("Choose sin,cos,tan: ")
    if choice == "sin":
        ang = float(input("Enter angle: "))
        x = math.sin(math.radians(ang))
        print (x)
    elif choice == "cos":
        ang = float(input("Enter angle: "))
        x = math.cos(math.radians(ang))
        print (x)
    elif choice == "tan":
        ang = float(input("Enter angle: "))
        x = math.tan(math.radians(ang))
        print (x)
    else:
        print("Error")

elif choose == "5":
    z = int(input("Enter Value: "))
    res = math.log10(z)
    print (res)
else
    print("Error")

How can I use the functions in a more preferable and efficient way?

Welcome to Code Review :)
Here are some nits and tips to get you on your way.

It is much more Pythonic to define your functions outside the user flow and put the user flow inside of a main() function. The flow should look like this:

def bodmas():
    ...

def sqrt_n():
    ...

def main():
    print("""Choose from
        ...
    """)
    choose = input("enter required number \n")
    if choose == '1':
        ...
        bodmas()
        ...
    elif choose == '2':
        ...

Use an if __name__ == '__main__': guard to invoke your main() function.

if __name__ == '__main__':
    main()

Do you want to handle and catch errors?

A = int(input("Enter coefficient A: "))
...
sol1 = (-B-cmath.sqrt(D))/(2*A)

What if the user enters a non-numeric coefficient? Or what if the user enters 0? This is also applicable if the user tries to divide by zero in the bodmas() function.
Consider following a style guide like PEP8 to make your code more readable and consistent.

Happy coding!


Custom Hashset in Java I have written this custom hashset and though it isn't completed yet, I would like to know if there is anything I am overlooking in terms of clean code conventions. My aim was also to apply generics to it. So, would also want any input regarding that.

  public class CustomHashset<T> {

    private static final int SIZE = 100;
    private Entry<T>[] buckets;
    private int size;

    public CustomHashset() {
      this.buckets = new Entry[SIZE];
      this.size= 0;

    }

    private int hash(T element) {
        return element.hashCode() % buckets.length;
    }

    public Boolean add(T element) {
        int index = hash(element);

        Entry<T> current = buckets[index];
        while (current!=null) {
            if (current.key.equals(element)) return false;
            current = current.next;
        }
        Entry<T> entry = new Entry<T>();
        entry.setKey(element);
        entry.setNext(buckets[index]);
        buckets[index] = entry;
        size++;

        return true;
    }

    public int size() {
        return size;
    }

    private static class Entry<T> {
        private T key;
        private Entry next;

        public  T getKey() {
            return key;
        }

        public void setKey(T element) {
            this.key = element;
        }

        public Entry getNext () {
            return next;
        }

        public void setNext(Entry next) {
            this.next = next;
        }
    }
}

Improving names:

Your class has 2 size variables, which looks kinda strange. Better to rename this private static one to something like INITIAL_CAPACITY (or DEFAULT_INITIAL_CAPACITY, if you have plans adding constructor with initialCapacity parameter), because size = how many elements are stored, but this variable denotes initial array length.
method hash in reality returns bucket index for given element, thus, should be renamed. For example, to indexFor or bucketIndex.

Other:

in Entry - next is not parameterized (as well as its getter and setter).


Deleting millions of logs in batches I have a fast growing table with logs that I frequently have to delete (the server doesn't have much resources). It grows by at least two million entries every day. The database is a SQL Server 2012.

Currently I use this script to delete entries older than 7 days:

set nocount on;
declare @r int = 1
while @r > 0
begin
    begin transaction;
        delete top(10000)
        from [Logs]
        where
            cast([Timestamp] as date) < cast(dateadd(day, -7, GETUTCDATE()) as date))
        set @r = @@ROWCOUNT;
    commit transaction;
end

When I don't do it daily then it runs for a couple of hours (3-5) blocking other tasks that usually exit with timeouts (they need it for reporting).

I use transactions because I sometimes need to stop it before it's finished and this is easier to do with smaller batches. Otherwise there is too much to rollback.

The table is a pretty normal log:

CREATE TABLE [dbo].[Log](
    [Id] [bigint] IDENTITY(1,1) NOT NULL,
    [Timestamp] [datetime2](7) NOT NULL,
    [Environment] [nvarchar](50) NOT NULL,
    [Logger] [nvarchar](50) NOT NULL,
    [Message] [nvarchar](max) NULL,
    [Exception] [nvarchar](max) NULL,
 CONSTRAINT [PK_Log_Id] PRIMARY KEY CLUSTERED
(
    [Id] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON, FILLFACTOR = 80) ON [PRIMARY]
) ON [PRIMARY] TEXTIMAGE_ON [PRIMARY]
GO

Is there a way to make this script faster? Am I doing anything terribly wrong here?

Here is another recent Code Review question that has a lot of similarities to yours: Daily SQL job to delete records older than 24 hours.
I think I would recommend three things here:

Snapshot isolation
Not using TOP
Doing the work in another table

Snapshot isolation
Snapshot isolation is a really cool tool that effectively prevents most things from blocking readers. As a result, even if your delete is running long, report-writers can still hit your table and they'll see the most recent, valid data. Then once your delete finishes, they'll start seeing the data without your deleted rows. This lets you do whatever you need to do without having to worry as much about end-users. It doesn't make your code faster, but it will somewhat reduce the need for it to be.

Not using TOP
Because you're using TOP, you effectively force all of the data to be re-sorted every time. Additionally, TOP will usually introduce row goals. This isn't necessarily a bad thing, but it may choose less-ideal plans in the interest of getting a subset of rows as quickly as possible.
Because your Id column is an indexed IDENTITY column, and because we're deleting the oldest data (I'm assuming you don't generally update old data), you can do something like this:
DELETE FROM [Logs]
  WHERE Id BETWEEN @LowestCurrentIndex AND @HighestCurrentIndex
    AND CAST([Timestamp] AS date) < CAST(DATEADD( DAY, -7, GETUTCDATE()) AS date);

This assumes you can maintain @LowestCurrentIndex and @HighestCurrentIndex as the range of values to currently consider. This will get you nice index accesses as well.
A potential enhancement is to get a separate table that has all potentially affected rows, like so:
SELECT Id
  INTO #OldData
  FROM [Logs]
  WHERE CAST([Timestamp] AS date) < CAST(DATEADD( DAY, -7, GETUTCDATE()) AS date);

Then you can just join between the two (and if you have an index on #OldData.Id it'll be a great merge-join) with the same bounds logic.
DELETE [Logs]
  FROM [Logs]
    INNER JOIN #OldData OldData
      ON [Logs].Id = OldData.Id
  WHERE OldData.Id BETWEEN @LowestCurrentIndex AND @HighestCurrentIndex -- This could be on either table

Do the work in another table
If copying the data is less expensive than deleting it (very possible), then you may be able to more efficiently do the work with less user interruptions by doing all of your work in a separate table, and then switching the tables. Broadly speaking, the workflow would look like this:

Copy all of the valid data into another identically formatted table (same indices, constraints, etc)
Truncate the original table
Perform a partition switch

Alternatively, if partition switching isn't your jam, you can do a very similar thing manually:

Copy all of the valid data into another identically formatted table (same indices, constraints, etc)
Rename the original table to something else
Rename the table from #1 to the original table's name
Truncate the original (now renamed) table from #2

These will result in much less index maintenance, and many of the operations can be done via minimal logging if well structured.


Two icon animations — one on the left, one on the right I have two icon animations that pretty much do the same thing, the only difference between them is positioning... one is left and the other right.

I would like to use one block of code for both left and right icon animations.

//////////// Left Icon Animation ////////////
$('.recent .left-icon').css({opacity:0, left:33});
$('.recent .controls-wrap').hover(function() {
    $(this).find('.left-icon').stop()
        .animate({left:63}, {duration:300})
        .animate({opacity:'0.99'}, {duration:400});
},function() {
    $(this).find('.left-icon').stop()
        .animate({left:33}, {duration:550})
        .animate({opacity:'0'}, {duration:300});
});

//////////// Right Icon Animation ////////////
$('.recent .right-icon').css({opacity:0, right:33});
$('.recent .controls-wrap').hover(function() {
    $(this).find('.right-icon').stop()
        .animate({right:63}, {duration:300})
        .animate({opacity:'0.99'}, {duration:400});
},function() {
    $(this).find('.right-icon').stop()
        .animate({right:33}, {duration:550})
        .animate({opacity:'0'}, {duration:300});
});

Make a function with one parameter, depending on the direction (right or left), copy a block of code and replace the key word there.


Report a status based on a list of results In one of my services, I have the function as presented below:

private def convertCreateResultsToCreateAllReasult(results: List[CreateResult.Result]): CreateAllResult.Result = {
    val firstConflictResult = results.collectFirst {
      case r: CreateResult.Conflict => r
    } match {
      case Some(cr: CreateResult.Conflict) => Some(CreateAllResult.Conflict(cr.message))

      case None => None
    }

    val firstNotFoundResult = results.collectFirst {
      case r: CreateResult.NotFound => r
    } match {
      case Some(nfr: CreateResult.NotFound) => Some(CreateAllResult.NotFound(nfr.message))

      case None => None
    }

    val firstFailedResult = results.collectFirst {
      case r: CreateResult.Failed => r
    } match {
      case Some(fr: CreateResult.Failed) => Some(CreateAllResult.Failed(fr.message))

      case None => None
    }

    val failedResults = List(firstConflictResult, firstNotFoundResult, firstFailedResult).flatten

    if (failedResults.isEmpty) {

      val createdDefinitions = results.collect {
        case r: CreateResult.Ok => r.definition
      }

      CreateAllResult.Ok(createdDefinitions)

    } else {

      failedResults.head

    }
  }

The argument of this function is a list of result objects, where the result object looks like this:

object CreateResult {

    sealed trait Result

    case class Ok(definition: ChecklistRuleDefinition) extends Result
    case class Conflict(message: String) extends Result
    case class NotFound(message: String) extends Result
    case class Failed(message: String) extends Result

  }

As you can see the result can be Ok (which is considered successful) and everything else (that considered as failed for various reasons).

The function should:

Return first fails result converted into a different type in case there is one such result in the list.
or
Return all successful results converted into CreateAllResult.Ok (if all of them successful).

Here is the CreateAllResult object:

object CreateAllResult {

    sealed trait Result

    case class Ok(definitions: List[ChecklistRuleDefinition]) extends Result
    case class Conflict(message: String) extends Result
    case class NotFound(message: String) extends Result
    case class Failed(message: String) extends Result

  }

The thing is, whatever I posted here, it seems to be working as expected. But, I hate the code. I know know that scala has all the bells and whistles to make the code nicer and more readable. I just didn't find the way to do so.

How can this code be written in a nicer, scala like, way?

Something like this maybe?

  @tailrec
  def trf(
    results: List[CreateResult.Result],
    conf: Option[CreateAllResult.Conflict] = None,
    nf:   Option[CreateAllResult.NotFound] = None,
    f:    Option[CreateAllResult.Failed] = None,
    defs: List[ChecklistRuleDefinition] = Nil
  ): List[CreateAllResult.Result] = results.map {
    case r: CreateResult.Conflict :: tail if conf.isEmpty =>
         trf(tail, Some(CreateAllResult.Conflict(r)), nt, f, Nil)
    case r: CreateResult.NotFound :: tail if nf.isEmpty  =>
         trf(tail, conf, Some(CreateAllResult.NotFound(r)), f, Nil)
    case r: CreateResult.Failed  if f.isEmpty =>
         trf(tail, conf, nf, Some(CreateAllResult.Failed(r)), Nil)
    case Ok(defn) :: tail if conf ++ nf ++ f isEmpty =>
         trf(tail, None, None, None, dfn :: defs)
    case _ if defs.isEmpty => conf ++ nf ++ f
    case _ =>  CreateAllResult.Ok(defs.reverse)
  }

Oh, wait you are only returning the first failure, are you? I didn't realize that at first, thought you wanted the first of each type.
Well, that makes it kinda simpler ...
How about this:

object Failure {
   def unapply(res: Result) = res match {
    case r: CreateResult.Conflict  => Some(CreateAllResult.Conflict(r))
    case r: CreateResult.NotFound  => Some(CreateAllResult.NotFound(r))
    case r: CreateResult.Failed => Some(CreateAllResult.Failed(r))
   }
}

def trf(results: List[CreateResult.Result]) = results
  .collectFirst { case Failure(r) => r }
  .getOrElse(CreateAllResult.Ok(
     results.collect { case r: CreateResult.Ok => r.definition }
   ))


First Common Lisp vector math code I'm studying Common Lisp on my own. Coming from C++, Common Lisp feels kind of strange sometimes. I'd like to get some feedback on what I understood so far.

For learning purposes, I wrote this simple vector class package. The only requirements apart from correctness are non-destructiveness and performance. I want it to be as fast as possible. Is this the way to do it? Are there any serious flaws?

(in-package :cl-user)
(defpackage math
  (:use :cl)
  (:export :vector3
       :create-vector3
       :vector3-x
       :vector3-y
       :vector3-z
       :neg
       :mul
       :div
       :add
       :sub
       :dot
       :len
       :norm))
(in-package :math)

(defclass vector3 ()
  ((x :accessor vector3-x
      :initarg :x
      :initform 0
      :type double-float)
   (y :accessor vector3-y
      :initarg :y
      :initform 0
      :type double-float)
   (z :accessor vector3-z
      :initarg :z
      :initform 0
      :type double-float)))

(defun create-vector3 (x y z)
  (make-instance 'vector3 :x x :y y :z z))

(defmethod neg ((v vector3))
  (create-vector3 (- (vector3-x v))
          (- (vector3-y v))
          (- (vector3-z v))))

(defmethod mul ((s double-float) (v vector3))
  (create-vector3 (* s (vector3-x v))
          (* s (vector3-y v))
          (* s (vector3-z v))))

(defmethod div ((v vector3) (s double-float))
  (create-vector3 (/ (vector3-x v) s)
          (/ (vector3-y v) s)
          (/ (vector3-z v) s)))

(defmethod add ((a vector3) (b vector3))
  (create-vector3 (+ (vector3-x a) (vector3-x b))
          (+ (vector3-y a) (vector3-y b))
          (+ (vector3-z a) (vector3-z b))))

(defmethod sub ((a vector3) (b vector3))
  (create-vector3 (- (vector3-x a) (vector3-x b))
          (- (vector3-y a) (vector3-y b))
          (- (vector3-z a) (vector3-z b))))

(defmethod dot ((a vector3) (b vector3))
  (+ (* (vector3-x a) (vector3-x b))
     (* (vector3-y a) (vector3-y b))
     (* (vector3-z a) (vector3-z b))))

(defmethod len ((v vector3))
  (sqrt (+ (* (vector3-x v) (vector3-x v))
       (* (vector3-y v) (vector3-y v))
       (* (vector3-z v) (vector3-z v)))))

(defmethod norm ((v vector3))
  (let ((l (len v)))
    (if (> l 0)
    (let ((inv (/ 1.0 l)))
      (create-vector3 (* (vector3-x v) inv)
              (* (vector3-y v) inv)
              (* (vector3-z v) inv)))
    (create-vector3 (vector3-x v)
            (vector3-y v)
            (vector3-z v)))))

Here are a few minor suggestions:

Use uninterned symbols instead of keywords in defpackage (e.g., #:neg instead of :neg) to avoid polluting the keyword package.
Fix your indentation.
Add a defgeneric with docs for each defmethod (you can fold defmethod inside defgeneric)
Decide if your objects are read-only; if they are, replace :accessor with :reader, if they are not, add mutator functions (e.g., scale in-place)
Rename norm to normalize (norm in math refers to the length of the vector)
Use (plusp x) instead of (> x 0)
Use (/ x) instead of (/ 1 x)
In the else clause in norm can just return the argument or (create-vector3 0 0 0) for clarity


Writing strings to the screen over time The following methods just write strings to the screen over time to make it look like a typing effect, instead of all at once. The only difference is the argument in the foreach loop.

I end up having lots of similar methods like these. How do I refactor them?

public IEnumerator TypeCharText(string message, Text textComp, bool resetTextComp = false)
{
    if (resetTextComp)
        textComp.text = "";

    foreach (char letter in message.ToCharArray())
    {
        textComp.text += letter;

        if (textComp.text.Length >= message.Length)
            OnMessageTyped(message);

        yield return new WaitForSeconds(2f);
    }
}

public IEnumerator TypeWordText(string message, Text textComp, bool resetTextComp = false)
{
    if (resetTextComp)
        textComp.text = "";

    foreach (string word in message.Split(' '))
    {
        textComp.text += " " + word;

        if (textComp.text.Length >= message.Length)
            OnMessageTyped(message);

        yield return new WaitForSeconds(2f);
    }
}

Here's what I have attempted:

public static IEnumerator TypeText<T>(Func<string, T> action, string message, Text textComp, bool resetTextComp = false)
{
    if (resetTextComp)
        textComp.text = "";

    textComp.text += action(message);

    if (textComp.text.Length >= message.Length)
        OnMessageTyped(message);

    yield return new WaitForSeconds(wordPause);
}

and to call it (StartCoroutine is a built-in Unity3D method that just calls a method):

StartCoroutine(TextTyper.TypeText<string>((x) => { x.Split(' '); return x;}, message, vanishText, true));
StartCoroutine(TextTyper.TypeText<string>((x) => { x.ToCharArray(); return x;}, message, vanishText, true));

It doesn't split the string up though or turn it into char array; action(message) still has the entire string.

You need a way to specify how the message will be transformed into a sequence, therefore your function should be typed as Func<string, IEnumerable<string>>. And the lambda expressions would look like this:

x => x.Split(' ')
x => x.Select(c => new string(c, 1))

But why pass a delegate for the split-algorithm instead of simply passing the sequence of text pieces directly?

public IEnumerator TypeWordText(IEnumerable<string> pieces, message, Text textComp,
                                bool resetTextComp = false, string separator = "")
{
    if (resetTextComp)
        textComp.text = "";

    foreach (string piece in pieces)
    {
        textComp.text += separator + piece;

        if (textComp.text.Length >= message.Length)
            OnMessageTyped(message);

        yield return new WaitForSeconds(2f);
    }
}

Since in one method you add a space and not in the other, I have added the separator as optional parameter.

I also suggest to transform the char-sequence into a string-sequence (with strings of length one), so that you can always work with strings.

TextTyper.TypeText(message.Split(' '), message, vanishText, true, " ")

TextTyper.TypeText(message.Select(c => new string(c, 1)), message, vanishText, true)

Note that a string is an IEnumerable<char>. The Selects will be executed lazily at each loop of the foreach-statement. Therefore, in a certain way, you are still passing an algorithm to the method in the second call, but hidden in an enumerator.

Algorithm: But why split the string at all, just in order to recombine it afterwards? Instead of splitting the message, you could keep it as is and keep track of the typed length in an int variable. Then display message.Substring(0, typedLength). This change requires a strategy for the increment of the typed length. See Strategy pattern.

public interface ITypingStrategy
{
    void TypeNext(string message, ref typedLength);
}

Now you can implement different strategies like CharByCharTypingStrategty or WordByWordTypingStrategy as classes. The adapted TypeWordText method would have an ITypingStrategy argument instead of an enumeration of text pieces.


JavaScript telegram bot that sends images based on user search In my freetime, I did this little Telegram bot in Node.js that provides some basic text-based commands and one command where a user can search for an image in order for the bot to send back an image related to that word.

Is there anything about this code I can optimize regarding performance and / or code quality in general?

// Request module
const request = require('request');

// Telebot module
const Telebot = require('telebot');

// Bot
const bot = new Telebot("YOUR TELEGRAM API TOKEN");

// DOMParser
const jsdom = require('jsdom');
const { JSDOM } = jsdom;

// /start command
bot.on('/start', (msg) => msg.reply.text('Hello! For informations about usage, creator, etc., please use the command /help!'));

// /help command
bot.on('/help', (msg) => msg.reply.text('Usage:\n /imageof <your_word> sends you an random image.\nExample:\n/imageof dog - You\'ll get an random image of a dog.\n\nAbout:\nVersion: 2.0\nCreator: @CodeFox\nGitHub: https://github.com/CodeF0x/image-search-telegram-bot\nAll images are taken from https://unsplash.com.'));

// /imageof command
bot.on(/^\/imageof (.+)$/, (msg, props) => {
    request(`https://unsplash.com/search/photos/${props.match[1]}`, function (error, response) { // Get the search results of bing
        var html = new JSDOM(response.body); // Parse the response
        var images = html.window.document.getElementsByClassName('_2zEKz'); // Get all images - in this case by class name, otherwise we would get profile pictures too
        var sources = []; // Array to pick random url from
        for (var i = 0; i < images.length; i++) { // Loop through all images and push only valid url to the array
            if (images[i].src.includes('https')) {
                sources.push(images[i].src);
            }
        }
        // Check if the array containing the url has any values
        if (typeof sources[0] !== "undefined") {
            sendPhoto(msg, sources[Math.floor(Math.random() * sources.length)]); // Random url as parmeter
        } else {
            sendError(msg, props);
        }
    });
});

// Actual function to send the photo
const sendPhoto = (msg, url) => msg.reply.photo(url); // Send the photo

// Function to send an error message
const sendError = (msg, props) => msg.reply.text(`⚠ Sorry, I couldn't find any image for "${props.match[1]}". ⚠`);

bot.start();

// Request module
const request = require('request');

// Telebot module
const Telebot = require('telebot');

// Bot
const bot = new Telebot("YOUR TELEGRAM API TOKEN");

// DOMParser
const jsdom = require('jsdom');
const { JSDOM } = jsdom;

First of all is the comment noise. It's pretty clear what the code is already doing. You don't need to put comments on code that's already obvious. If you really need comments, save it for "weird" cases or things that is normally done one way yet you did it in another.

// /start command
bot.on('/start', (msg) => msg.reply.text('Hello! For informations about usage, creator, etc., please use the command /help!'));

// /help command
bot.on('/help', (msg) => msg.reply.text('Usage:\n /imageof <your_word> sends you an random image.\nExample:\n/imageof dog - You\'ll get an random image of a dog.\n\nAbout:\nVersion: 2.0\nCreator: @CodeFox\nGitHub: https://github.com/CodeF0x/image-search-telegram-bot\nAll images are taken from https://unsplash.com.'));

This is fine for now. But should your app grow, consider putting the messages in a dedicated object. This allows you to easily look up and modification of messaging without diving into code.

    var html = new JSDOM(response.body); // Parse the response
    var images = html.window.document.getElementsByClassName('_2zEKz'); // Get all images - in this case by class name, otherwise we would get profile pictures too
    var sources = []; // Array to pick random url from

I find it strange that you use const on the stuff above, but var down here.

    var images = html.window.document.getElementsByClassName('_2zEKz'); // Get all images - in this case by class name, otherwise we would get profile pictures too
    var sources = []; // Array to pick random url from
    for (var i = 0; i < images.length; i++) { // Loop through all images and push only valid url to the array
        if (images[i].src.includes('https')) {
            sources.push(images[i].src);
        }
    }

Given the existence of array methods that let you transform one array to another, I find this pattern of "create array and push items from another array into it" an anti-pattern. Use array.map and array.filter instead. Also, you might want to check if https is actually the start of src.

const sources = Array.prototype.slice.call(images) // Convert NodeList to regular array.
  .filter(img => img.src.indexOf('https') === 0)   // Only https images
  .map(img => img.src)                             // Extract src


Floor the larger number floor[1*e] + floor[2*e] + floor[3*e] + ... + floor[n*e],

  where floor[x] is the largest integer that is not greater than x,
  and e is Euler's number: 2.7182818284..

  Input

  A single line which contains a single integer: n.

  Output

  A single line which contains a single integer which should be
  the answer.

  Constraints

  1 ≤ n ≤ 10 ^ 4000

  Subtasks

  Subtask #1 (50 points): 1 ≤ n ≤ 10 ^ 100

  Subtask #2 (50 points): Original constraints.

  Example

  Input:

  3

  Output:

  15

  Explanation

floor[1*e] = floor[2.71828..] = 2.

floor[2*e] = floor[5.43656..] = 5.

floor[3*e] = floor[8.15484..] = 8.

  So the answer is 2+5+8=15.

My Code

Scanner in=new Scanner(System.in);
int T = in.nextInt();
double e=2.7182818284;
long sum=0;

for (int t_i = 1; t_i <= T; t_i++)
{

   sum=Math.floor(sum+t_i*e);
   // System.out.println(Math.floor(sum));

}
System.out.printf("%.0f",(sum));

This runs into a time-limit exceeded.

int, long and double are not precise enough. You should probably use BigInteger and/or BigDecimal instead.

Same problem with e=2.7182818284;. When you do 100000000000L*e you're already off by almost 6. And this is nothing compared to \$10^{100}\$.

Even if you use Math.E instead it's only 2.718281828459045. So you probably want to calculate e to an arbitrary precision (based on input n) instead. (Check here for a hint).

Note that none of those will help you with the time-limit-exceeded problem though. You'll have to find a smarter way so that you don't have to iterate all \$10^{4000}\$ numbers. Note that if you would add a number each millisecond and you ran the program for 300 years you would still only reach about \$10^{12}\$ or \$10^{13}\$. So no where close to what you want.

The best I can come up with is to keep track of the actual sum, the floored sum and the error on each order of magnitude.

So for n = 10, then n=100, then n=1000, ...

At each step you use the result of the previous one. That way you only need to do 10 iterations in the loop at that step to go up an order of magnitude.

Once you have those results you can calculate for any n the wanted result. The tricky part here is how to correct the error's at each step.

Even for \$n=10^{4000}\$ this should only result in O(10000) operations which (depending on your machine specs) runs in less than a minute. (Might also depend on how long calculations with BigInteger/BigDecimal take, I don't have much experience with those myself).

How to actually implement this I'll leave up to you, since this site is all about improving existing code, not just writing new code to solve the problem for you :)

Disclaimer: I did not test this myself yet, so not 100% sure that it will be fast enough even if implemented correctly.


2D falling snow animation I was hoping that someone could review this code.

My main concern is that the contents of main() are difficult to read. I thought about extracting some segments of code into functions (to aid comprehension) but it felt unnecessary because these segments are not completely repeated.

#include <SFML/Graphics.hpp>
#include <random>

// Maps input in the range [0..1] to an output in the range [0..1].
// Represents the piecewise function:
// y(x) = 2*x^2             when x < 0.5
//      = -2*x^2 + 4*t - 1  when x >= 0.5
float easeInOutQuad(float normalisedTime)
{
  auto & t = normalisedTime;
  return t < 0.5 ? 2*t*t : 2*t*(2 - t) - 1;
}

struct Tween
{
  float begin;
  float change;
  float time{0};
  float duration;
  float current() const
  {
    return begin + change*easeInOutQuad(time/duration);
  }
};

int main()
{
  sf::RenderWindow window{sf::VideoMode{500, 500}, "Snow"};
  auto const inverseFramerate = 1.f/60;
  window.setFramerateLimit(static_cast<unsigned>(1/inverseFramerate));
  auto const numberOfSnowflakes = 200;
  Tween tweens[numberOfSnowflakes];
  float yPositions[numberOfSnowflakes];
  auto generator = std::mt19937{};
  // Returns a normally distributed random number.
  auto randomChangeInX = [&generator]()
  {
    static auto distribution = std::normal_distribution<float>{0.f, 10.f};
    return distribution(generator);
  };
  // Returns a uniformly distributed random number in the range [min..max).
  auto randomBetween = [&generator](float min, float max)
  {
    return std::generate_canonical<float, 10>(generator)*(max - min) + min;
  };
  for (auto i = 0; i < numberOfSnowflakes; ++i)
  {
    tweens[i].begin = randomBetween(0, window.getSize().x);
    tweens[i].change = randomChangeInX();
    tweens[i].duration = randomBetween(1, 5);
    yPositions[i] = randomBetween(-20, window.getSize().y);
  }
  auto const defaultRadius = 5.f;
  auto circle = sf::CircleShape{};
  while (window.isOpen())
  {
    auto event = sf::Event{};
    while (window.pollEvent(event))
    {
      if (event.type == sf::Event::Closed)
      {
        window.close();
      }
    }
    for (auto i = 0; i < numberOfSnowflakes; ++i)
    {
      tweens[i].time += inverseFramerate;
      if (tweens[i].time > tweens[i].duration)
      {
        tweens[i].begin += tweens[i].change;
        tweens[i].change = randomChangeInX();
        tweens[i].time = 0;
      }

      yPositions[i] += 1/tweens[i].duration;
      if (yPositions[i] > window.getSize().y)
      {
        yPositions[i] = -defaultRadius*2;
        tweens[i].begin = randomBetween(0, window.getSize().x);
        tweens[i].change = randomChangeInX();
        tweens[i].time = 0;
      }
    }
    window.clear();
    for (auto i = 0; i < numberOfSnowflakes; ++i)
    {
      circle.setPosition(tweens[i].current(), yPositions[i]);
      circle.setRadius(defaultRadius/tweens[i].duration);
      window.draw(circle);
    }
    window.display();
  }
}

Tween is not a very good name. How about Snowflake? And why is it a struct and exposing all of its implementation details. I would personally place it in a class, initialize its values via constructor, and give it a public function like fall() or something to update it each frame.

class Snowflake
{
public:
    void fall();
private:
    sf::CircleShape snowflake;
    float begin;
    float change;
    float time{ 0 };
    float duration;
    float current() const
    {
        return begin + change * easeInOutQuad(time / duration);
    }
};

By putting the circle in the class you can update it internally. More on all that later.

float easeInOutQuad(float normalisedTime)
{
  auto & t = normalisedTime;
  return t < 0.5 ? 2*t*t : 2*t*(2 - t) - 1;
}

It seems a little unusual to assign a reference to the argument and then return the reference. I'm actually a little surprised this didn't boom. it's a trivially copied float. Just pass it by value.

auto const inverseFramerate = 1.f / 60;
window.setFramerateLimit(static_cast<unsigned>(1 / inverseFramerate));

That is an odd way to write

window.setFramerateLimit(60);

Now you could replace the magic number 60 that I just passed here with a named constant but I would actually argue that this is one of those rare cases when you shouldn't. This will be the only call to this function and the name won't be more readable here. Most readers are likely to immediately recognize that the 60 is setting 60 fps.

No cast. No equation. easily readable. less lines.

auto const numberOfSnowflakes = 200;
Tween tweens[numberOfSnowflakes];
float yPositions[numberOfSnowflakes];

This is a much more useful named constant. However. Don't use C array[]s. std::array or std::vector are better C++ containers. std::array is static sized like the C-style array[] and std::vector is dynamically sized. Don't forget to #include whichever you choose to use. You should keep the constant but switch to the more preferred constexpr

constexpr int number_of_snowflakes = 200;

auto generator = std::mt19937{};

A lot of your difficult to read code revolves around your use and misuse of this object. Allow me to show you a slightly easier to use way of handling PRNG. I'd wrap it all into its own named function and do away with the two harder to read and use lambdas. Then inside the function you seed the generator and declare it static so it doesn't get destroyed when it goes out of scope.

//we take unsigned and return float in order to work with SFML's window size which is unsigned
float generateRandom(unsigned min, unsigned max)
{
    float real_min = static_cast<float>(min);
    float real_max = static_cast<float>(max);
    std::random_device rd;
    static const std::mt19937 generator(rd());
    std::uniform_real_distribution<float> distribution(real_min, real_max);

    return distribution(generator);
}

Now when you want a value between 0 - width you do

start = generateRandom(0, window.getSize().x)

This is a weird reimplementation of uniform_real_distribution. You did it so you could work with negative ranges because you want your snowflakes to start off-screen. But all you need is an offset applied after the fact.

auto randomBetween = [&generator](float min, float max)
{
    return std::generate_canonical<float, 10>(generator)*(max - min) + min;
};

Now we get to initialization. In the future it is best to declare and initialize variables together. If you need helper functions have them defined beforehand and ready to use rather than defining them in between instantiation and initialization.

for (auto i = 0; i < numberOfSnowflakes; ++i)
{
    tweens[i].begin = randomBetween(0, window.getSize().x);
    tweens[i].change = randomChangeInX();
    tweens[i].duration = randomBetween(1, 5);
    yPositions[i] = randomBetween(-20, window.getSize().y);
}
auto const defaultRadius = 5.f;
auto circle = sf::CircleShape{};

Most of this can be eliminated by doing two things. Move the sf::Circle into our new Snowflake class and we use default initialization within the class. You can also be completely rid of the second yPositions array as the Snowflake class circle can maintain its own position. You will have to pass the starting coordinates to the constructor because the Snowflake class won't have full access to the sf::RenderWindow class. Our class is now starting to look like this:

class Snowflake
{
public:
    explicit Snowflake(sf::Vector2f start_position);

    void fall();
private:
    float change{ generateRandom(1, 10) };
    float duration{ generateRandom(1, 5) };
    float time{ 0 };
    sf::Vector2f position;
    sf::CircleShape snowflake{ default_radius / duration };
    float current() const
    {
        return position.x + change * easeInOutQuad(time / duration);
    }
};

You can do the initialization like so:

std::vector<Snowflake> snowflakes;
snowflakes.reserve(number_of_snowflakes);
for (int i = 0; i < number_of_snowflakes; ++i)
{
    float x = generate_random(0, window.getSize().x);
    float y = generate_random(-20, window.getSize().y);
    sf::Vector2f start_position(x, y);
    snowflakes.emplace_back(Snow(start_position));
}

This will give you a nice standard container of snowflakes at default initialized

So now we get to your animation loop. I see now why you did inverseFramerate now, but it was a complicated way of having your snow fall at varying rates. As you have moved everything into the Snowflake class you can have a much simpler animation loop.

while (window.isOpen())
{
    sf::Event event;
    while (window.pollEvent(event))
    {
        if (event.type == sf::Event::Closed)
        {
            window.close();
        }
    }

    for (auto&& snow : snowflakes)
    {
        snow.fall(window.getSize());
    }

    window.clear();
    for (auto&& snow : snowflakes)
    {
        snow.draw(window);
    }
    window.display();
}

All that leaves us with is implementing the fall() and draw() methods from above. draw() is simple and would look something like this:

void draw(sf::RenderWindow& window) { window.draw(snowflake); }

and the fall() method I just copied over your implementation details using my new class member variables. It looks something like this:

Put it all together and it would look like this:

#include <random>
#include <vector>

#include <SFML/Graphics.hpp>

constexpr int number_of_snowflakes = 200;
constexpr float single_frame = 1.F / 60;
constexpr float default_radius = 5.F;

// Maps input in the range [0..1] to an output in the range [0..1].
// Represents the piecewise function:
// y(x) = 2*x^2             when x < 0.5
//      = -2*x^2 + 4*t - 1  when x >= 0.5
float ease_in_and_out(float normalised_time)
{
    float t = normalised_time;
    return t < 0.5 ? 2 * t*t : 2 * t*(2 - t) - 1;
}

//we take unsigned and return float in order to work with SFML's window size which is unsigned
float generate_random(unsigned min, unsigned max)
{
    float real_min = static_cast<float>(min);
    float real_max = static_cast<float>(max);
    static std::random_device rd;
    static std::mt19937 generator(rd());
    std::uniform_real_distribution<float> distribution(real_min, real_max);

    return distribution(generator);
}

class Snowflake
{
public:
    explicit Snowflake(sf::Vector2f start_position) : position{ start_position }
    {
        snowflake.setPosition(position);
        begin = start_position.x;
    }

    void fall(sf::Vector2u window_size)
    {
        time += single_frame;
        if (time > duration)
        {
            begin += change;
            change = generate_random(1, 10);
            time = 0.F;
        }

        position.y = snowflake.getPosition().y;
        position.y += 1 / duration;
        if (position.y > window_size.y)
        {
            position.y = -10.F;
            begin = generate_random(0, window_size.x);
            change = generate_random(1, 10);
            time = 0;
            snowflake.setRadius(default_radius / duration);
        }

        position.x = shift_x();
        snowflake.setPosition(position);
    }
    void draw(sf::RenderWindow& window) { window.draw(snowflake); }
private:
    sf::Vector2f position;
    float begin;
    float change{ generate_random(1, 10) };
    float duration{ generate_random(1, 5) };
    float time{ 0 };
    sf::CircleShape snowflake{ default_radius / duration };

    float shift_x() const
    {
        return begin + change * ease_in_and_out(time / duration);
    }
};

int main()
{
    sf::RenderWindow window{ sf::VideoMode{500, 500}, "Snow" };
    window.setFramerateLimit(60);

    std::vector<Snowflake> snowflakes;
    snowflakes.reserve(number_of_snowflakes);
    for (int i = 0; i < number_of_snowflakes; ++i)
    {
        float x = generate_random(0, window.getSize().x);
        float y = generate_random(0, window.getSize().y) - 20.F;
        sf::Vector2f start_position(x, y);
        snowflakes.emplace_back(Snowflake(start_position));
    }

    while (window.isOpen())
    {
        sf::Event event;
        while (window.pollEvent(event))
        {
            if (event.type == sf::Event::Closed)
            {
                window.close();
            }
        }

        for (auto&& snow : snowflakes)
        {
            snow.fall(window.getSize());
        }

        window.clear();
        for (auto&& snow : snowflakes)
        {
            snow.draw(window);
        }
        window.display();
    }
}

Could this still be improved? Sure. I left a few magic numbers behind, and we all know those are bad. The names are more descriptive and better describe what is happening but they could probably be improved. SFML allows for classes to inherit from sf::Drawable and you can fix the syntax so it can be called window.draw(Snowflake); instead of passing a reference to the window object to your class. I would also probably extract the fall() method to a couple private helpers. This would

hide the implementation details of the public method that don't need to be exposed.
allow you to simplify some of the functionality. (There's a lot of resetting values to default.)


Flux: async communication with websockets I want to arrange async server-client communication with websockets and vanilla flux architecture. There is an excellent article about "Async Server-Side Communication with the Flux Architecture". I gonna implement it analogue with SockJS and ES6 promises:

ActionCreator.js     - it's the same but moved to ES6

import * as constants from ('../constants/constants'),
import TodoService = require('../services/todo-service');

class ActionCreator {

  addTodo(text) {
    var that = this;
    service.addTodo({text: text}).then(function(todo) {
        that.dispatch(constants.ADD_TODO, todo);
    });
  }
}

export default new ActionCreator();

services/todo-service.js - even shorter

import WebSocketWrapper from 'WebSocketWrapper';

class TodoService {

    addTodo(todo) {
        //maybe do something with todos before send them to the server
        return WebSocketWrapper.send(todo);
    }
}

export default new TodoService();

WebSocketWrapper.js

/**
 * Due fact that SockJS provide raw websocket API and do not implement pub/sub, we need to do wrapper, which will corellate request/response
 */

import SockJS from 'sockjs';
import selectn from 'selectn';

var reqId = 1;
const responseDeffers = {};

class WebSocketWrapper {

    constructor(url) {
        this.ws = new SockJS(url);
        this.ws.onopen = this.onOpen;
        this.ws.onmessage = this.onMessage;
        this.ws.onclose = this.onClose;
        this.ws.onerror = this.onError;
    },

    onMessage(message) {
        let requestId = selectn('message.data.request_id');
        if (!requestId) return;
        //resolve or reject the promise which is kept in ActionCreator
        if (message.data.result === 'Ok') resolveRequest(requestId, message);
        else rejectRequest(requestId, message.error);
    },

    /**
     * Return Promise, which will be resolved/rejected on server response with the same requestId
     */
    send(request) {
        return new Promise(function(resolve, reject) {
            //cache resolve/reject function to be able to invoke themn on server response
            responseDeffers[reqId++] = {
                resolve: res,
                reject: rej
            };
            //send message via websockets
            if (this.ws.readyState == this.ws.OPEN) {
                this.ws.send(JSON.stringify(request));
            } else {
                reject('Websocket connection is closed');
            }
        });
    },

    resolveRequest(requestId, message) {
        let response = JSON.parse(message.data);
        responseDeffers[request_id].resolve(response);
    },

    rejectRequest(requestId, message) {
        responseDeffers[requestId].reject(message);
    }

    onOpen() {},

    oClose() {},

    onError(e) {}
}

export default new WebSocketStore(ENV.props.url);

ActionCreator.js

Why write a class for when you just export a single instance? When one makes a class, one expects to be able to make multiple instances. This doesn't. You can just roll with object literals. You could even just write functions and just export them.
service is undefined. Where is it?

todo-service.js

Same as above, why write a class?

WebSocketWrapper.js

Same as above, why write a class? Even more interesting is that you have "tracking variables" (reqId and responseDeffers) outside the class.
Not sure how you'd benefit with let in you code. You're declaring them at the very top of a function scope. var would have sufficed.

Just because JS has ES6 doesn't mean you have to use classes, let and all the new stuff.


Segment tree challenge Here is my implementation of a segment tree with a sample driver file (I didn't make an update function because I didn't need it for what I was doing).

I'm a huge fan of geeksforgeeks, but I do not understand why this link makes such a complicated implementation.  I didn't use the height or compute the maximum size in order to build mine. Not to mention, their structure really isn't a tree; it's an integer array.

I'm looking for what I implemented well and what I did not implement well.

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

typedef struct segment_tree_node_s
{
  struct segment_tree_node_s * left;
  struct segment_tree_node_s * right;
  int start;
  int end;
  int sum;
} segment_tree_node_t;

typedef struct segment_tree_s
{
  segment_tree_node_t * root;
} segment_tree_t;

int build_tree(segment_tree_node_t **, int, int);
int query_tree(segment_tree_node_t *, int, int);

static const int ra[] = {1,3,5,7,9,11};
int main(int argc, char * argv[])
{
  segment_tree_t * new_tree = (segment_tree_t *) malloc(sizeof(segment_tree_t));
  int length = sizeof(ra)/sizeof(ra[0]);
  int sum = build_tree(&new_tree->root, 0, length - 1);
  int sum_2 = query_tree(new_tree->root, 0, 0);
  int sum_3 = query_tree(new_tree->root, 0, 1);
  int sum_4 = query_tree(new_tree->root, 0, 2);
  int sum_5 = query_tree(new_tree->root, 0, 3);
  int sum_6 = query_tree(new_tree->root, 0, 4);
  int sum_7 = query_tree(new_tree->root, 1, 5);
  int sum_8 = query_tree(new_tree->root, 2, 4);
  int sum_9 = query_tree(new_tree->root, 4, 5);
  int sum_10 = query_tree(new_tree->root, 5, 5);

  printf("Sum of tree [0-5]: %d\n", sum);
  printf("Sum of tree [0-0]: %d\n", sum_2);
  printf("Sum of tree [0-1]: %d\n", sum_3);
  printf("Sum of tree [0-2]: %d\n", sum_4);
  printf("Sum of tree [0-3]: %d\n", sum_5);
  printf("Sum of tree [0-4]: %d\n", sum_6);
  printf("Sum of tree [1-5]: %d\n", sum_7);
  printf("Sum of tree [2-4]: %d\n", sum_8);
  printf("Sum of tree [4-5]: %d\n", sum_9);
  printf("Sum of tree [5-5]: %d\n", sum_10);

  return 0;
}

int build_tree(segment_tree_node_t ** node, int start, int end)
{
  *node = (segment_tree_node_t *)malloc(sizeof(segment_tree_node_t));
  (*node)->sum = 0;

  if(start != end)
  {
    (*node)->start = start;
    (*node)->end   = end;
    int mid = start + (end - start)/2;
    return (*node)->sum = build_tree(&((*node)->left), start, mid) + build_tree(&((*node)->right), mid + 1, end);
  }
  else
  {
    (*node)->start = start;
    (*node)->end   = end;
    (*node)->sum = ra[start];
    return ra[start];
  }
}

int query_tree(segment_tree_node_t * node, int start, int end)
{
    /* Total overlap */
    if(node->start >= start && node->start <= end)
    {
        if(node->end <= end && node->end >= start)
            return node->sum;
    }

    /* Partial overlap */
    if((start >= node->start && start <= node->end) || (end >= node->end && end <= node->start) || (start <= node->start && end >= node->start))
            return query_tree(node->left, start, end) + query_tree(node->right, start, end);

    /* No overlap */
    return 0;
}

Bounds checking simplification

Your bounds checking code is overly complicated.  This total overlap check:

/* Total overlap */
if(node->start >= start && node->start <= end)
{
    if(node->end <= end && node->end >= start)
        return node->sum;
}

could be reduced to:

/* Total overlap */
if(node->start >= start && node->end <= end)
{
    return node->sum;
}

In this partial overlap check:

/* Partial overlap */
if((start >= node->start && start <= node->end) ||
       (end >= node->end && end <= node->start) ||
       (start <= node->start && end >= node->start))

I noticed that the second of the three conditions always evaluates to false.  Some of the other parts are also unnecessary.  In fact the whole thing could be rewritten as:

/* Partial overlap */
if (start <= node->end && end >= node->start)

Use of globals

The function build_tree() references the array ra directly.  It would be better for that array to be passed in as an argument.  Otherwise that function can't be used for any other segment tree.


Handling shared state among a lot of elements in Angular I am working on a project in Angular where I have a number of similar data objects. When you click on anyone of them it's state and amount of data shown will change. All of the objects start in the minimal state, when you click on one it becomes medium. From a medium you can click on the button to go to maximum or click anywhere else on it to go back down. There can only be one medium or maximum at a time. If there is one medium there can be no maximum, and vice versa.

Since all of the objects are by default min, I don't track that. I use a string to track which element is in an expanded state. Then I compare that objects key and the string in an ng-if to change its what is being shown. The size is managed by a conditional class, and CSS.

I chose ng-if, because there maybe a lot of elements loaded in the beginning, but there can only be one med/max at a time. I would rather load the med/max when the user wants it, instead of loading all of the med/max at that start and use ng-show for state view.

Is there a cleaner way to handle state change among many related objects? Does Angular have something to deal with this?

angular.module('SizeAndState', [])
.factory('Data', function() {
        return {
            query: function() {
                return {
                    a:{ name: 'A', date: '1/2/03', number: 1},
                    b:{ name: 'B', date: '4/5/06', number: 2},
                    c:{ name: 'C', date: '8/9/10', number: 3},
                    d:{ name: 'D', date: '3/2/01', number: 4},
                    e:{ name: 'E', date: '1/2/14', number: 5},
                }
            }
        };
    })
    .controller('SizeAndStateController', ['$scope', 'Data', function($scope, Data) {
        $scope.toMin = function (clickEvent){
            clickEvent.stopPropagation();
            $scope.medElement = '';
            $scope.maxElement = '';

        };
        $scope.toMed = function (elementName, clickEvent){
            if($scope.maxElement=='') {
               if($scope.medElement == elementName){
                   $scope.toMin(clickEvent);
               } else {
                   $scope.medElement = elementName;
               }
            }
        };
        $scope.toMax = function (clickEvent){
            clickEvent.stopPropagation();
            $scope.maxElement = $scope.medElement;
            $scope.medElement = '';

        };
        $scope.data = Data.query();
        $scope.medElement = '';
        $scope.maxElement = '';
    }]);
.campaign{
  display: inline-block;
  width: 100px;
  height: 100px;
  background-color: #ff685b;
  border: 1px solid #ffffff;
  color: #ffffff;
  vertical-align: top;
}
.campaign.med{
  width: 125px;
  height: 125px;
}
.campaign.max{
  width: 150px;
  height: 150px;
}

        {{element.name}} Min

   Med

{{element.date}}

            <button class="btn"
                    ng-click="toMax($event)">
     to Max
            </button>

   Max

{{element.date}}

{{element.number}}

           <button
                ng-if="maxElement == element.name"
                ng-click="toMin($event)">
             to Min
         </button>

Your View looks fine (HTML and CSS). However, representing the state of your data elements with two string variables that may or may not be storing the "name" field on the parent data object is a very bad way of representing state.

The guiding principal for these design questions: your object model should make it impossible to ever represent an inconsistent or meaningless state. With your object model there are many ways to represent inconsistent and meaningless states, for example:

It's possible for a data item to be simultaneously in state Med
and state Max
If medElement and maxElement variables contain
values which are not equal to the name field on the parent object,
those values are ignored

As a result your controller code is needlessly complex and very difficult to read.

Solution:

Represent the state of your data objects with an enum called ElementSize that can take exactly one of three possible state values: minimum, medium, and maximum. Then, use the ng-switch directive to specify what html you want to render for each of the three possible states.


Strong Password Detection in Python This is a problem from "Automate the boring stuff".

  Write a function that uses regular expressions to make sure the password
  string it is passed is strong. A strong password is defined as one that is at
  least eight characters long, contains both uppercase and lowercase characters, and has at least one digit. You may need to test the string against multiple regex patterns to validate its strength.

How to improve this code?

#! /usr/bin/python3

import re

def uppercase_check(password):
    if re.search('[A-Z]', password): #atleast one uppercase character
        return True
    return False

def lowercase_check(password):
    if re.search('[a-z]', password): #atleast one lowercase character
        return True
    return False

def digit_check(password):
    if re.search('[0-9]', password): #atleast one digit
        return True
    return False

def user_input_password_check():
    password = input("Enter password : ")
        #atleast 8 character long
    if len(password) >= 8 and uppercase_check(password) and lowercase_check(password) and digit_check(password):
        print("Strong Password")
    else:
        print("Weak Password")

user_input_password_check()

I'm submitting this as a different answer because it goes in a different direction from my previous one, eliminating the bool cast as well as individual functions. You can simply define a tuple of regular expressions and apply all.

rexes = ('[A-Z]', '[a-z]', '[0-9]')
# ...
if len(password) >= 8 and all(re.search(r, password) for r in rexes)):
    print('Strong password')


Excuse me, how many of you are new here? Mind if I JOIN you? I had perceived an uptick in new reviewers around here, and I wanted to see if the data backed that observation up. It's been a while since I've written any SQL, so this was a nice little exercise in using the Stack Exchange Data Explorer.

The query finds the first question and first answer from every user, and then groups those first posts by week. It turns out that I was right, but the growth of first questions is outpacing the growth of first answers.

The query feels repetitive and I'm not a huge fan of the Full Outer Join.
Is there a better way to write this?

WITH FirstAnswers
AS (
  SELECT
    Users.Id UserId
    ,dateadd(week, datediff(week, 0, Convert(Date,Min(Posts.CreationDate))), 0) WeekOf
  FROM Posts
  INNER JOIN Users
    ON Posts.OwnerUserId = Users.Id
  WHERE PostTypeId = 2 --answer
    AND Posts.CreationDate > '2011-Jan-01' --There was very little activity prior to this date. Including it skews the graph.
  GROUP BY Users.Id
),

FirstQuestions
AS (
  SELECT
    Users.Id UserId
    ,dateadd(week, datediff(week, 0, Convert(Date,Min(Posts.CreationDate))), 0) WeekOf
  FROM Posts
  INNER JOIN Users
    ON Posts.OwnerUserId = Users.Id
  WHERE PostTypeId = 1 --question
    AND Posts.CreationDate > '2011-Jan-01' --There was very little activity prior to this date. Including it skews the graph.
  GROUP BY Users.Id
)

SELECT ISNULL(a.WeekOf,b.WeekOf) As WeekOf
  , a.AnswerCount, b.QuestionCount
FROM (
  SELECT WeekOf, Count(UserId) AnswerCount
  FROM FirstAnswers
  GROUP BY WeekOf
) a
FULL OUTER JOIN (
  SELECT WeekOf, Count(UserId) QuestionCount
  FROM FirstQuestions
  GROUP BY WeekOf
 )b
ON a.WeekOf = b.WeekOf
ORDER BY WeekOf

Technically, the full outer join is insufficient.  If during a certain week there was not a single user who posted their first question or answer, the plot for that week needs to be 0.  It would be misleading to plot a direct line from the previous week to the following week, skipping the quiet week.  So, strictly speaking, you need to generate a series of all the weeks, to which you LEFT OUTER JOIN the FirstQuestions and FirstAnswers CTEs.  However, generating a date series in SQL Server is a hassle compared to some developer-friendlier databases.  I'm willing to pretend that there aren't weeks with no new user activity (actually demonstrably false for the week of 2011-01-10).

First rewrite

All of the information you need is contained in the Posts table.  There is no need to join the Users table.

The subselects in the query suggest that your CTEs are underdeveloped.  In my first rewrite below, I've reorganized FirstAnswers and FirstQuestions to perform the role of your subselects.

a and q would make more meaningful table aliases than a and b.

Your function names are inconsistently capitalized: dateadd, datediff, Convert, Min, ISNULL.  I believe that Convert(Date, …) isn't needed at all.

WITH FirstPosts AS (
  SELECT OwnerUserId As UserId
       , PostTypeId
       , DATEADD(week, DATEDIFF(week, 0, MIN(CreationDate)), 0) AS WeekOf
    FROM Posts
    WHERE CreationDate > '2011-01-01' -- There was very little activity prior to this date. Including it skews the graph.
    GROUP BY OwnerUserId, PostTypeId
), FirstAnswers AS (
  SELECT WeekOf
       , COUNT(UserId) AS UserCount
    FROM FirstPosts
    WHERE PostTypeId = 2 -- answer
    GROUP BY WeekOf
), FirstQuestions AS (
  SELECT WeekOf
       , COUNT(UserId) AS UserCount
    FROM FirstPosts
    WHERE PostTypeId = 1 -- question
    GROUP BY WeekOf
)
SELECT ISNULL(a.WeekOf, q.WeekOf) AS WeekOf
     , ISNULL(a.UserCount, 0) AS AnswerCount
     , ISNULL(q.UserCount, 0) AS QuestionCount
  FROM FirstAnswers AS a
    FULL OUTER JOIN FirstQuestions AS q
      ON a.WeekOf = q.WeekOf
  ORDER BY 1;

Better solution

As it turns out, there is a much slicker way to avoid the join altogether by using a conditional COUNT():

WITH FirstPosts AS (
  SELECT OwnerUserId As UserId
       , PostTypeId
       , DATEADD(week, DATEDIFF(week, 0, MIN(CreationDate)), 0) AS WeekOf
    FROM Posts
    WHERE CreationDate > '2011-01-01' -- There was very little activity prior to this date. Including it skews the graph.
    GROUP BY OwnerUserId, PostTypeId
)
SELECT WeekOf
     , COUNT(CASE WHEN PostTypeId = 2 THEN UserId END) AS AnswerCount
     , COUNT(CASE WHEN PostTypeId = 1 THEN UserId END) AS QuestionCount
  FROM FirstPosts
  GROUP BY WeekOf
  ORDER BY WeekOf;